[PATCH mesa v4] wayland: Add support for eglSwapInterval

Jason Ekstrand jason at jlekstrand.net
Fri Oct 25 16:17:10 PDT 2013


My knowledge of mesa and of the wl_display_prepare_read stuff is a bit
lacking.  From what I can see, it looks good.


On Fri, Oct 25, 2013 at 9:50 AM, Neil Roberts <neil at linux.intel.com> wrote:

> Ok, here is version 4 of the patch taking into account the discussion
> with Jason Ekstrand. The assumption is that if we have enough buffer
> slots then we should always get a release event immediately after one
> of the attaches. That means we can just rely on sending a sync request
> after the commit in order to get a buffer release and we don't need to
> bother with the request to disable the queuing mechanism.
>
> The previous version of the patch would block in a loop calling
> wl_dispatch_queue if it couldn't find a buffer. This is only a
> sensible option if we know that the compositor isn't queueing the
> release events. If not this loop would just block indefinitely. If the
> theory about getting release events is correct then we should never
> actually hit this loop so it probably doesn't really matter what it
> does. However, I didn't like the idea of having a loop there that
> would just block forever so I changed it to poll the compositor with a
> sync request every 10ms in order to force it to flush the queue. It
> prints a warning if this case is hit so that we will know there is a
> problem.
>
> I made the change to make it use 4 buffer slots in this patch and
> tested that it does use exactly all 4 of them when the application is
> fullscreen. This does work and it doesn't hit the polling path. I
> guess we could change to be five in order to cope with the subsurface
> case but I'm a bit reluctant to do that because it seems like quite a
> corner case and maybe it's better to just let it hit the warning path
> in that case.
>
> In the previous versions of the patch it would only do a sync request
> if the swap interval is zero. In this version I've changed it so that
> it always installs it. This is necessary because if an application is
> doing swap interval 1 but isn't installing a frame callback it would
> end up rendering and calling get_back_bo before we've handled any data
> from the compositor and it would use a redundant third buffer.
>
> Regards,
> - Neil
>
> ------- >8 --------------- (use git am --scissors to automatically chop
> here)
>
> The Wayland EGL platform now respects the eglSwapInterval value. The value
> is
> clamped to either 0 or 1 because it is difficult (and probably not useful)
> to
> sync to more than 1 redraw.
>
> The main change is that if the swap interval is 0 then Mesa won't install a
> frame callback so that eglSwapBuffers can be executed as often as
> necessary.
> However it now always does a sync request after the swap buffers and blocks
> until this is complete in get_back_bo. The compositor is likely to send a
> release event while processing the new buffer attach and this makes sure we
> will receive that before deciding whether to allocate a new buffer. This is
> done even if the application is using swap interval 1 because otherwise if
> the
> application is not installing its own frame callback it may end up calling
> get_back_bo before we've handled any data from the compositor and it would
> end
> up using a redundant extra buffer.
>
> If there are no buffers available then instead of returning with an error,
> get_back_bo will now poll the compositor by repeatedly sending sync
> requests
> every 10ms. This is a last resort and in theory this shouldn't happen
> because
> there should be no reason for the compositor to hold on to more than three
> buffers. That means whenever we attach the fourth buffer we should always
> get
> an immediate release event which should come in with the notification for
> the
> first sync request that we are throttled to.
>
> When the compositor is directly scanning out from the application's buffer
> it
> may end up holding on to three buffers. These are the one that is is
> currently
> scanning out from, one that has been given to DRM as the next buffer to
> flip
> to, and one that has been attached and will be given to DRM as soon as the
> previous flip completes. When we attach a fourth buffer to the compositor
> it
> should replace that third buffer so we should get a release event
> immediately
> after that. This patch therefore also changes the number of buffer slots
> to 4
> so that we can accomodate that situation.
>
> If DRM eventually gets a way to cancel a pending page flip then the
> compositors
> can be changed to only need to hold on to two buffers and this value can be
> put back to 3.
>
> This also moves the vblank configuration defines from platform_x11.c to the
> common egl_dri2.h header so they can be shared by both platforms.
> ---
>  src/egl/drivers/dri2/egl_dri2.h         |   9 +-
>  src/egl/drivers/dri2/platform_wayland.c | 204
> +++++++++++++++++++++++++++++---
>  src/egl/drivers/dri2/platform_x11.c     |   6 -
>  3 files changed, 193 insertions(+), 26 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h
> b/src/egl/drivers/dri2/egl_dri2.h
> index 7a2e098..7de5916 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -174,6 +174,7 @@ struct dri2_egl_surface
>     int                    dx;
>     int                    dy;
>     struct wl_callback    *frame_callback;
> +   struct wl_callback    *sync_callback;
>     int                   format;
>  #endif
>
> @@ -194,7 +195,7 @@ struct dri2_egl_surface
>  #endif
>        int                 locked;
>        int                 age;
> -   } color_buffers[3], *back, *current;
> +   } color_buffers[4], *back, *current;
>  #endif
>
>  #ifdef HAVE_ANDROID_PLATFORM
> @@ -220,6 +221,12 @@ struct dri2_egl_image
>     __DRIimage *dri_image;
>  };
>
> +/* From xmlpool/options.h, user exposed so should be stable */
> +#define DRI_CONF_VBLANK_NEVER 0
> +#define DRI_CONF_VBLANK_DEF_INTERVAL_0 1
> +#define DRI_CONF_VBLANK_DEF_INTERVAL_1 2
> +#define DRI_CONF_VBLANK_ALWAYS_SYNC 3
> +
>  /* standard typecasts */
>  _EGL_DRIVER_STANDARD_TYPECASTS(dri2_egl)
>  _EGL_DRIVER_TYPECAST(dri2_egl_image, _EGLImage, obj)
> diff --git a/src/egl/drivers/dri2/platform_wayland.c
> b/src/egl/drivers/dri2/platform_wayland.c
> index bb8d113..1bf96d7 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -34,6 +34,7 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <xf86drm.h>
> +#include <poll.h>
>
>  #include "egl_dri2.h"
>
> @@ -183,8 +184,16 @@ dri2_create_window_surface(_EGLDriver *drv,
> _EGLDisplay *disp,
>                            _EGLConfig *conf, EGLNativeWindowType window,
>                            const EGLint *attrib_list)
>  {
> -   return dri2_create_surface(drv, disp, EGL_WINDOW_BIT, conf,
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +   _EGLSurface *surf;
> +
> +   surf = dri2_create_surface(drv, disp, EGL_WINDOW_BIT, conf,
>                               window, attrib_list);
> +
> +   if (surf != NULL)
> +      drv->API.SwapInterval(drv, disp, surf,
> dri2_dpy->default_swap_interval);
> +
> +   return surf;
>  }
>
>  /**
> @@ -219,6 +228,8 @@ dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLSurface *surf)
>
>     if (dri2_surf->frame_callback)
>        wl_callback_destroy(dri2_surf->frame_callback);
> +   if (dri2_surf->sync_callback)
> +      wl_callback_destroy(dri2_surf->sync_callback);
>
>     if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
>        dri2_surf->wl_win->private = NULL;
> @@ -257,6 +268,52 @@ dri2_release_buffers(struct dri2_egl_surface
> *dri2_surf)
>  }
>
>  static int
> +poll_compositor(struct dri2_egl_display *dri2_dpy)
> +{
> +   static GLboolean seen_wait_warning = GL_FALSE;
> +   struct pollfd pollfd;
> +
> +   /* This path is a last resort and it implies that something has gone
> wrong
> +    * so we'll warn about it */
> +   if (!seen_wait_warning) {
> +      _eglLog(_EGL_WARNING, "waiting for the compositor to release a
> buffer");
> +      seen_wait_warning = GL_TRUE;
> +   }
> +
> +   /* The plan here is that we'll wait for up to 10ms for some data from
> the
> +    * compositor. If so we'll just return so that it will check if there
> is
> +    * now a buffer available. Otherwise we'll wait for up to 10ms for
> some new
> +    * data before issuing a roundtrip. The roundtrip is necessary because
> the
> +    * compositor may be queuing release events and we need to cause it to
> +    * flush the queue */
> +
> +   if (wl_display_prepare_read_queue(dri2_dpy->wl_dpy,
> +                                     dri2_dpy->wl_queue) != 0)
> +      return wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy,
> +                                               dri2_dpy->wl_queue);
> +
> +   pollfd.fd = wl_display_get_fd(dri2_dpy->wl_dpy);
> +   pollfd.events = POLLIN;
> +   pollfd.revents = 0;
> +
> +   if (poll(&pollfd, 1, 10) < 0) {
> +      wl_display_cancel_read(dri2_dpy->wl_dpy);
> +      return -1;
> +   }
> +
> +   if (pollfd.revents) {
> +      if (wl_display_read_events(dri2_dpy->wl_dpy) < 0)
> +         return -1;
> +
> +      return wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy,
> +                                               dri2_dpy->wl_queue);
> +   } else {
> +      wl_display_cancel_read(dri2_dpy->wl_dpy);
> +      return roundtrip(dri2_dpy);
> +   }
> +}
> +
> +static int
>  get_back_bo(struct dri2_egl_surface *dri2_surf, __DRIbuffer *buffer)
>  {
>     struct dri2_egl_display *dri2_dpy =
> @@ -264,24 +321,46 @@ get_back_bo(struct dri2_egl_surface *dri2_surf,
> __DRIbuffer *buffer)
>     __DRIimage *image;
>     int i, name, pitch;
>
> -   /* 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->sync_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 always want to throttle to a sync event after the commit so
> that
> +       * we can be sure the compositor has had a chance to handle it and
> send
> +       * us a release event before we look for a free buffer */
> +      do {
> +         if (wl_display_dispatch_queue(dri2_dpy->wl_dpy,
> +                                       dri2_dpy->wl_queue) == -1)
> +            return EGL_FALSE;
> +      } while (dri2_surf->sync_callback != NULL);
> +   }
>
>     if (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. */
> -        if (dri2_surf->color_buffers[i].locked)
> -            continue;
> -         if (dri2_surf->back == NULL)
> -           dri2_surf->back = &dri2_surf->color_buffers[i];
> -         else if (dri2_surf->back->dri_image == NULL)
> -           dri2_surf->back = &dri2_surf->color_buffers[i];
> +      while (1) {
> +         for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> +            /* Get an unlocked buffer, preferrably one with a dri_buffer
> +             * already allocated. */
> +            if (dri2_surf->color_buffers[i].locked)
> +               continue;
> +            if (dri2_surf->back == NULL)
> +               dri2_surf->back = &dri2_surf->color_buffers[i];
> +            else if (dri2_surf->back->dri_image == NULL)
> +               dri2_surf->back = &dri2_surf->color_buffers[i];
> +         }
> +
> +         if (dri2_surf->back != NULL)
> +            break;
> +
> +         /* If we make it here then here then all of the buffers are
> locked.
> +          * In theory this shouldn't happen because the compositor
> shouldn't
> +          * be holding on to so many buffers. As a last resort we will
> poll
> +          * the compositor at regular intervals in order to ensure we
> +          * eventually get a buffer */
> +
> +         poll_compositor(dri2_dpy);
>        }
>     }
>
> -   if (dri2_surf->back == NULL)
> -      return -1;
>     if (dri2_surf->back->dri_image == NULL) {
>        dri2_surf->back->dri_image =
>           dri2_dpy->image->createImage(dri2_dpy->dri_screen,
> @@ -455,6 +534,21 @@ static const struct wl_callback_listener
> frame_listener = {
>  };
>
>  static void
> +wayland_sync_callback(void *data,
> +                          struct wl_callback *callback,
> +                          uint32_t time)
> +{
> +   struct dri2_egl_surface *dri2_surf = data;
> +
> +   dri2_surf->sync_callback = NULL;
> +   wl_callback_destroy(callback);
> +}
> +
> +static const struct wl_callback_listener throttle_listener = {
> +   wayland_sync_callback
> +};
> +
> +static void
>  create_wl_buffer(struct dri2_egl_surface *dri2_surf)
>  {
>     struct dri2_egl_display *dri2_dpy =
> @@ -514,11 +608,14 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv,
>     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);
> +   if (draw->SwapInterval > 0) {
> +      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);
> +   }
>
>     for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++)
>        if (dri2_surf->color_buffers[i].age > 0)
> @@ -562,6 +659,16 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv,
>
>     wl_surface_commit(dri2_surf->wl_win->surface);
>
> +   /* We always want to throttle to a sync callback even if we are also
> +    * waiting for a frame callback. The sync callback is checked in
> +    * get_back_bo so that we always give a chance for the compositor to
> handle
> +    * the commit and send a release event before checking for a free
> buffer */
> +   dri2_surf->sync_callback = wl_display_sync(dri2_dpy->wl_dpy);
> +   wl_callback_add_listener(dri2_surf->sync_callback,
> +                            &throttle_listener, dri2_surf);
> +   wl_proxy_set_queue((struct wl_proxy *) dri2_surf->sync_callback,
> +                      dri2_dpy->wl_queue);
> +
>     (*dri2_dpy->flush->flush)(dri2_surf->dri_drawable);
>     (*dri2_dpy->flush->invalidate)(dri2_surf->dri_drawable);
>
> @@ -808,6 +915,60 @@ static const struct wl_registry_listener
> registry_listener = {
>     registry_handle_global_remove
>  };
>
> +static EGLBoolean
> +dri2_swap_interval(_EGLDriver *drv,
> +                   _EGLDisplay *disp,
> +                   _EGLSurface *surf,
> +                   EGLint interval)
> +{
> +   if (interval > surf->Config->MaxSwapInterval)
> +      interval = surf->Config->MaxSwapInterval;
> +   else if (interval < surf->Config->MinSwapInterval)
> +      interval = surf->Config->MinSwapInterval;
> +
> +   surf->SwapInterval = interval;
> +
> +   return EGL_TRUE;
> +}
> +
> +static void
> +dri2_setup_swap_interval(struct dri2_egl_display *dri2_dpy)
> +{
> +   GLint vblank_mode = DRI_CONF_VBLANK_DEF_INTERVAL_1;
> +
> +   /* We can't use values greater than 1 on Wayland because we are using
> the
> +    * frame callback to synchronise the frame and the only way we be sure
> to
> +    * get a frame callback is to attach a new buffer. Therefore we can't
> just
> +    * sit drawing nothing to wait until the next ‘n’ frame callbacks */
> +
> +   if (dri2_dpy->config)
> +      dri2_dpy->config->configQueryi(dri2_dpy->dri_screen,
> +                                     "vblank_mode", &vblank_mode);
> +   switch (vblank_mode) {
> +   case DRI_CONF_VBLANK_NEVER:
> +      dri2_dpy->min_swap_interval = 0;
> +      dri2_dpy->max_swap_interval = 0;
> +      dri2_dpy->default_swap_interval = 0;
> +      break;
> +   case DRI_CONF_VBLANK_ALWAYS_SYNC:
> +      dri2_dpy->min_swap_interval = 1;
> +      dri2_dpy->max_swap_interval = 1;
> +      dri2_dpy->default_swap_interval = 1;
> +      break;
> +   case DRI_CONF_VBLANK_DEF_INTERVAL_0:
> +      dri2_dpy->min_swap_interval = 0;
> +      dri2_dpy->max_swap_interval = 1;
> +      dri2_dpy->default_swap_interval = 0;
> +      break;
> +   default:
> +   case DRI_CONF_VBLANK_DEF_INTERVAL_1:
> +      dri2_dpy->min_swap_interval = 0;
> +      dri2_dpy->max_swap_interval = 1;
> +      dri2_dpy->default_swap_interval = 1;
> +      break;
> +   }
> +}
> +
>  EGLBoolean
>  dri2_initialize_wayland(_EGLDriver *drv, _EGLDisplay *disp)
>  {
> @@ -824,6 +985,7 @@ dri2_initialize_wayland(_EGLDriver *drv, _EGLDisplay
> *disp)
>     drv->API.DestroySurface = dri2_destroy_surface;
>     drv->API.SwapBuffers = dri2_swap_buffers;
>     drv->API.SwapBuffersWithDamageEXT = dri2_swap_buffers_with_damage;
> +   drv->API.SwapInterval = dri2_swap_interval;
>     drv->API.Terminate = dri2_terminate;
>     drv->API.QueryBufferAge = dri2_query_buffer_age;
>     drv->API.CreateWaylandBufferFromImageWL =
> @@ -883,9 +1045,13 @@ dri2_initialize_wayland(_EGLDriver *drv, _EGLDisplay
> *disp)
>     dri2_dpy->extensions[2] = &use_invalidate.base;
>     dri2_dpy->extensions[3] = NULL;
>
> +   dri2_dpy->swap_available = EGL_TRUE;
> +
>     if (!dri2_create_screen(disp))
>        goto cleanup_driver;
>
> +   dri2_setup_swap_interval(dri2_dpy);
> +
>     /* The server shouldn't advertise WL_DRM_CAPABILITY_PRIME if the driver
>      * doesn't have createImageFromFds, since we're using the same driver
> on
>      * both sides.  We don't want crash if that happens anyway, so fall
> back to
> diff --git a/src/egl/drivers/dri2/platform_x11.c
> b/src/egl/drivers/dri2/platform_x11.c
> index a518db1..cc9a049 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -39,12 +39,6 @@
>
>  #include "egl_dri2.h"
>
> -/* From xmlpool/options.h, user exposed so should be stable */
> -#define DRI_CONF_VBLANK_NEVER 0
> -#define DRI_CONF_VBLANK_DEF_INTERVAL_0 1
> -#define DRI_CONF_VBLANK_DEF_INTERVAL_1 2
> -#define DRI_CONF_VBLANK_ALWAYS_SYNC 3
> -
>  static void
>  swrastCreateDrawable(struct dri2_egl_display * dri2_dpy,
>                       struct dri2_egl_surface * dri2_surf,
> --
> 1.8.3.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20131025/837dcd8b/attachment-0001.html>


More information about the wayland-devel mailing list