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

[Mike Lothian]

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
>