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

[Nicolai Hähnle]

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

Fair enough.

Acked-by: Nicolai Hähnle <nicolai.haehnle at 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.