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

[Mike Lothian]

Hi

Yes I've not seen any freezes with Plasmashell, so a big improvement

Had issues with patch 2

Cheers

Mike

On 4 May 2018 at 17:36, Mario Kleiner <mario.kleiner.de at gmail.com> wrote:
> 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
>>>>