[PATCH mesa 1/2] wayland: Block for the frame callback in get_back_bo not dri2_swap_buffers

[Kristian Høgsberg]

On Fri, Nov 15, 2013 at 01:50:49PM +0000, Neil Roberts wrote:
> As per Kristian's suggestion we can avoid the problem of effectively
> disabling the event queuing mechanism by only doing the sync request
> when the swap interval is zero. To fix the bug of using three buffers
> we can just block for the frame callback in get_back_bo instead of
> swap_buffers. I was originally a bit reluctant to do this because it
> will end up blocking in an unusual place (ie, usually in glClear) but
> I suppose that doesn't really matter.
> 
> I've split that change out into a separate patch because it is an
> independent problem. The new swap interval patch can now share the
> callback variable with the frame callback because they are now blocked
> for in the same place.

I finally merged this, thanks Neil.  As we discussed in IRC, this is
the right thing to do for applications that are synced to the
compositors repaint loop.  Waiting for the frame event in
get_back_bo() means that the rendering for the next frame will be
submitted after the compositor submits its rendering for the current
frame.  That way we don't risk starving or delaying the compositor by
submitting rendering that will only be visible in the next frame.
 
> Just to note, in addition to these two patches I think it would be
> good to apply these other two patches which are buried in this thread
> (although they are not required):
> 
> http://lists.freedesktop.org/archives/wayland-devel/2013-October/011766.html
> http://lists.freedesktop.org/archives/wayland-devel/2013-September/010967.html

Both applied.

Kristian

> - Neil
> 
> ------- >8 --------------- (use git am --scissors to automatically chop here)
> 
> Consider a typical game-style main loop which might be like this:
> 
> while (1) {
> 	draw_something();
> 	eglSwapBuffers();
> }
> 
> In this case the game is relying on eglSwapBuffers to throttle to a sensible
> frame rate. Previously this game would end up using three buffers even though
> it should only need two. This is because Mesa decides whether to allocate a
> new buffer in get_back_bo which would be before it has tried to read any
> events from the compositor so it wouldn't have seen any buffer release events
> yet.
> 
> This patch just moves the block for the frame callback to get_back_bo.
> Typically the compositor will send a release event immediately after one of
> the attaches so if we block for the frame callback here then we can be sure to
> have completed at least one roundtrip and received that release event after
> attaching the previous buffer before deciding whether to allocate a new one.
> 
> dri2_swap_buffers always calls get_back_bo so even if the client doesn't
> render anything we will still be sure to block to the frame callback. The code
> to create the new frame callback has been moved to after this call so that we
> can be sure to have cleared the previous frame callback before requesting a
> new one.
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 34 +++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index f9065bb..5ce8981 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -263,8 +263,19 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
>        dri2_egl_display(dri2_surf->base.Resource.Display);
>     int i;
>  
> -   /* There might be a buffer release already queued that wasn't processed */
> -   wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy, dri2_dpy->wl_queue);
> +   if (dri2_surf->frame_callback == NULL) {
> +      /* There might be a buffer release already queued that wasn't processed */
> +      wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy, dri2_dpy->wl_queue);
> +   } else {
> +      /* We throttle to the frame callback here so that we can be sure to have
> +       * received any release events before trying to decide whether to
> +       * allocate a new buffer */
> +      do {
> +         if (wl_display_dispatch_queue(dri2_dpy->wl_dpy,
> +                                       dri2_dpy->wl_queue) == -1)
> +            return EGL_FALSE;
> +      } while (dri2_surf->frame_callback != NULL);
> +   }
>  
>     if (dri2_surf->back == NULL) {
>        for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> @@ -557,18 +568,7 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv,
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
> -   int i, ret = 0;
> -
> -   while (dri2_surf->frame_callback && ret != -1)
> -      ret = wl_display_dispatch_queue(dri2_dpy->wl_dpy, dri2_dpy->wl_queue);
> -   if (ret < 0)
> -      return EGL_FALSE;
> -
> -   dri2_surf->frame_callback = wl_surface_frame(dri2_surf->wl_win->surface);
> -   wl_callback_add_listener(dri2_surf->frame_callback,
> -                            &frame_listener, dri2_surf);
> -   wl_proxy_set_queue((struct wl_proxy *) dri2_surf->frame_callback,
> -                      dri2_dpy->wl_queue);
> +   int i;
>  
>     for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++)
>        if (dri2_surf->color_buffers[i].age > 0)
> @@ -581,6 +581,12 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv,
>        return EGL_FALSE;
>     }
>  
> +   dri2_surf->frame_callback = wl_surface_frame(dri2_surf->wl_win->surface);
> +   wl_callback_add_listener(dri2_surf->frame_callback,
> +                            &frame_listener, dri2_surf);
> +   wl_proxy_set_queue((struct wl_proxy *) dri2_surf->frame_callback,
> +                      dri2_dpy->wl_queue);
> +
>     dri2_surf->back->age = 1;
>     dri2_surf->current = dri2_surf->back;
>     dri2_surf->back = NULL;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel