[PATCH 1/2] loader_dri3: Wait for pending swaps to complete before drawable_fini.

[Mike Lothian]

Hi

I'm still seeing the freeze in the Steam client with this patch

Just about to test the other one

Mike

On 4 May 2018 at 17:17, Mike Lothian <mike at fireburn.co.uk> wrote:
> Hi Mario
>
> Again thanks for looking into this issue :D
>
> Tested-by: Mike Lothian <mike at fireburn.co.uk>
>
> I'll give the other patch a whirl now
>
> Cheers
>
> Mike
>
> On 4 May 2018 at 14:45, Mario Kleiner <mario.kleiner.de at gmail.com> wrote:
>> Before destroying the loader_dri3_drawable, make sure all pending
>> swaps for it have completed. This guides against the following scenario,
>> which happens, e.g., with KDE Plasma-5's plasmashell (which uses
>> QT-5's QtGui/QtQuick for rendering), when it repaints multiple
>> UI elements, each represented by its own Window/GLXDrawable, using
>> one common GLXContext for all GLXDrawable's:
>>
>> 1. glXMakeCurrent(dpy, drawable1, context);
>> 2. glXXX render to drawable1
>> 3. glXSwapBuffers(dpy, drawable1); #1
>> 4. glXMakeCurrent(dpy, drawable2, context);
>> 5. glXXX render to drawable2
>> 6. glXSwapBuffers(dpy, drawable2);
>> // While the swap #1 is still pending for drawable1:
>> 7. glXMakeCurrent(dpy, drawable1, context);
>> 8. glXXX render to drawable1
>> 9. glXSwapBuffers(dpy, drawable1);
>>
>> Binding a different drawable2 to the same context via glXMakeCurrent
>> will cause its previous drawable1 to be released (cfe. dri3_bind_context
>> -> driReleaseDrawables), which in turn calls loader_dri3_drawable_fini().
>> This unselects for Present notify event delivery on the associated
>> X-Window and loses all dri3 related state. If drawable1 is selected for
>> the context again [7], a new incarnation of loader_dri3_drawable is
>> created in dri3_bind_context->driFetchDrawable->dri3_create_drawable->
>> loader_dri3_drawable_init(), which again selects for Present notify
>> event delivery for the underlying X-Window, but the new incarnation lost
>> all state wrt. to previous rendering and swaps. The server now delivers
>> PresentPixmapIdle and PresentPixmapComplete events from the completed
>> previous swapbuffers call #1 [3] to the new loader_dri3_drawable, which
>> doesn't expect those. One problem is that the new incarnation has a
>> draw->send_sbc == 0, but now receives PresentPixmapComplete events with
>> sbc's > 0, therefore updating draw->recv_sbc to > 0 in
>> dri3_handle_present_event(). The draw->recv_sbc > draw_send_sbc is
>> misinterpreted as sbc wraparound, triggers recv_sbc wraparound handling
>> and ends up with a very large draw->recv_sbc. During the next swapbuffers
>> call [9], the totally wrong recv_sbc is used for calculating the target_msc
>> for the PresentPixmap request, leading to a target_msc billions of vblanks
>> in the future, leading to a swap that never completes and thereby frozen UI
>> and hang of the client.
>>
>> Make sure that a loader_dri3_drawable can only be destroyed after all
>> its pending swaps have completed, to prevent misdelivery of PresentNotify
>> events to the right X-Window, but the wrong incarnation of the associated
>> loader_dri3_drawable.
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>> Cc: xorg-devel at lists.x.org
>> Cc: daniel at fooishbar.org
>> Cc: eero.t.tamminen at intel.com
>> Cc: mike at fireburn.co.uk
>> ---
>>  src/loader/loader_dri3_helper.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
>> index 6bb11c4..7bd79af 100644
>> --- a/src/loader/loader_dri3_helper.c
>> +++ b/src/loader/loader_dri3_helper.c
>> @@ -234,6 +234,9 @@ loader_dri3_drawable_fini(struct loader_dri3_drawable *draw)
>>  {
>>     int i;
>>
>> +   if (draw->special_event)
>> +      loader_dri3_swapbuffer_barrier(draw);
>> +
>>     draw->ext->core->destroyDrawable(draw->dri_drawable);
>>
>>     for (i = 0; i < ARRAY_SIZE(draw->buffers); i++) {
>> --
>> 2.7.4
>>