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

[Mario Kleiner]

On Fri, May 4, 2018 at 6:31 PM, Mike Lothian <mike at fireburn.co.uk> wrote:
> Hi
>
> I'm still seeing the freeze in the Steam client with this patch
>
> Just about to test the other one
>

Thanks for testing. So the plasmashell hang is gone, right?
Maybe the steam issue is a different bug. With *only* patch 2/2
applied, seing ORPHAN printouts would hint at a similar problem,
absence of them would probably mean something else.

-mario

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