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