[Mesa-dev] [PATCH] loader/dri3: Improve dri3 thread-safety

[Thomas Hellstrom]

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 <thellstrom at 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.

/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