[Mesa-dev] [PATCH] egl/wayland: Ensure we get a back buffer

[Pekka Paalanen]

On Tue, 16 May 2017 11:02:22 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Commit 9ca6711faa03 changed the Wayland winsys to only block for the
> frame callback inside SwapBuffers, rather than get_back_bo. get_back_bo
> would perform a single non-blocking Wayland event dispatch, to try to
> find any release events which we had pulled off the wire but not
> actually processed. The blocking dispatch was moved to SwapBuffers.
> 
> This removed a guarantee that we would've processed all events inside
> get_back_bo(), and introduced a failure whereby the server could've sent
> a buffer release event, but we wouldn't have read it. In clients
> unconstrained by SwapInterval (rendering ~as fast as possible), which
> were being displayed on a hardware overlay (buffer release delayed),

Hi,

hardware overlay, or as in the bug report, on the primary plane, i.e.
the only composite-bypass case in pre-atomic Weston available without
hacks.

> this could lead to get_back_bo() failing because there were no free
> buffers available to it.
> 
> The drawing rightly failed, but this was papered over because of the
> path in eglSwapBuffers() which attempts to guarantee a BO, in order to
> support calling SwapBuffers twice in a row with no rendering actually
> having been performed.
> 
> Since eglSwapBuffers will perform a blocking dispatch of Wayland
> events, a buffer release would have arrived by that point, and we
> could then choose a buffer to post to the server. The effect was that
> frames were displayed out-of-order, since we grabbed a frame with random
> past content to display to the compositor.
> 
> Ideally get_back_bo() failing should store a failure flag inside the
> surface and cause the next SwapBuffers to fail, but for the meantime,
> restore the correct behaviour such that get_back_bo() no longer fails.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Reported-by: Eero Tamminen <eero.t.tamminen at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98833
> Fixes: 9ca6711faa03 ("Revert "wayland: Block for the frame callback in get_back_bo not dri2_swap_buffers"")
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index 52ab9f7579..7837790bdb 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -353,7 +353,7 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
>     /* There might be a buffer release already queued that wasn't processed */
>     wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy, dri2_surf->wl_queue);
>  
> -   if (dri2_surf->back == NULL) {
> +   while (dri2_surf->back == NULL) {
>        for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
>           /* Get an unlocked buffer, preferrably one with a dri_buffer
>            * already allocated. */
> @@ -364,6 +364,14 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
>           else if (dri2_surf->back->dri_image == NULL)
>              dri2_surf->back = &dri2_surf->color_buffers[i];
>        }
> +
> +      if (dri2_surf->back)
> +         continue;

Would 'break' be more readable?

> +
> +      /* If we don't have a buffer, then block on the server to release one for
> +       * us, and try again. */
> +      if (wl_display_dispatch_queue(dri2_dpy->wl_dpy, dri2_surf->wl_queue) < 0)
> +          return -1;
>     }
>  
>     if (dri2_surf->back == NULL)

The swrast path does not need fixing, because there buffer release can
never be delayed due to composite bypass, right?

Nothing looks suspicious to me, so:
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170516/35df278e/attachment.sig>