On 06.11.2017 12:53, Thomas Hellstrom wrote:
On 11/06/2017 12:14 PM, Nicolai Hähnle wrote:
On 03.11.2017 12:02, Thomas Hellstrom wrote:
It turned out that with recent changes that call into dri3 from glFinish(), it appears like different thread end up waiting for X events simultaneously, causing deadlocks since they steal events from eachoter and update the dri3
counters behind eachothers backs.

This patch intends to improve on that. It allows at most one thread at a
time to wait on events for a single drawable. If another thread intends to do the same, it's put to sleep until the first thread finishes waiting, and then it rechecks counters and optionally retries the waiting. Threads that
poll for X events never pulls X events off the event queue if there are
other threads waiting for events on that drawable. Counters in the
dri3 drawable structure are protected by a mutex. Finally, the mutex we
introduce is never held while waiting for the X server to avoid
unnecessary stalls.

This does not make dri3 drawables completely thread-safe but at least it's a
first step.

Bugzilla: https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D102358&d=DwICaQ&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=yqE5Xb9bQg5hA8gP7s0b3dSSZoPWaAEKACqD8qfhdZo&s=wf-xBzPlZ805RCV1hnDgoyW0fYe2ZX3Qwie66SU936g&e= Fixes: d5ba75f8881 "st/dri2 Plumb the flush_swapbuffer functionality through to dri3"
Signed-off-by: Thomas Hellstrom <thellst...@vmware.com>
---
  src/loader/loader_dri3_helper.c | 77 +++++++++++++++++++++++++++++++----------
  src/loader/loader_dri3_helper.h | 10 ++++++
  2 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 19ab581..7e6b8b2 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -32,7 +32,6 @@
    #include <X11/Xlib-xcb.h>
  -#include <c11/threads.h>
  #include "loader_dri3_helper.h"
    /* From xmlpool/options.h, user exposed so should be stable */
@@ -186,8 +185,11 @@ dri3_fence_await(xcb_connection_t *c, struct loader_dri3_drawable *draw,
  {
     xcb_flush(c);
     xshmfence_await(buffer->shm_fence);
-   if (draw)
+   if (draw) {
+      mtx_lock(&draw->mtx);
        dri3_flush_present_events(draw);
+      mtx_unlock(&draw->mtx);
+   }
  }
    static void
@@ -245,6 +247,9 @@ loader_dri3_drawable_fini(struct loader_dri3_drawable *draw)
        xcb_discard_reply(draw->conn, cookie.sequence);
        xcb_unregister_for_special_event(draw->conn, draw->special_event);
     }
+
+   cnd_destroy(&draw->event_cnd);
+   mtx_destroy(&draw->mtx);
  }
    int
@@ -276,6 +281,8 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
       draw->cur_blit_source = -1;
     draw->back_format = __DRI_IMAGE_FORMAT_NONE;
+   mtx_init(&draw->mtx, mtx_plain);
+   cnd_init(&draw->event_cnd);
       if (draw->ext->config)
draw->ext->config->configQueryi(draw->dri_screen,
@@ -407,13 +414,27 @@ dri3_handle_present_event(struct loader_dri3_drawable *draw,
  }
    static bool
-dri3_wait_for_event(struct loader_dri3_drawable *draw)
+dri3_wait_for_event_locked(struct loader_dri3_drawable *draw)
  {
     xcb_generic_event_t *ev;
     xcb_present_generic_event_t *ge;
       xcb_flush(draw->conn);

(Why) Doesn't the flush need to be protected as well? Can it be removed entirely given that it's already called from loader_dri3_wait_for_msc? Though dri3_find_back is different - why?


Thanks for reviewing.

AFAIK, (correc me if I'm wrong) xcb should be thread-safe enough to allow multiple threads to call xcb_flush() simultaneously so we shouldn't need to explicitly "serialize" it.

There might indeed be unnecessary xcb_flushes(). In the dri3_find_back case when we're initially just checking for arrived events, we know that any preceding swapbuffers (that govern the reuse of back buffers) will have flushed xcb. However when we explicitly wait for special events, any forgotten flush would be catastrophic, causing a deadlock.

We should probably audit the code for unnecessary xcb flushes, but as a follow-up patch.

Fair enough.

Acked-by: Nicolai Hähnle <nicolai.haeh...@amd.com>



/Thomas



Apart from that, the patch does indeed look like a good first step, though Keep in mind that I'm not too familiar with this stuff...

Cheers,
Nicolai



--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to