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 <[email protected]> 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel >
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
