[PATCH v2] wayland: Add support for eglSwapInterval
Pekka Paalanen
ppaalanen at gmail.com
Fri Sep 13 00:48:21 PDT 2013
On Thu, 12 Sep 2013 16:58:56 +0100
Neil Roberts <neil at linux.intel.com> wrote:
> I like Kristian's proposal to throttle the swap buffers to sync
> callbacks. It has the added benefit that we can stop the client from
> unnecessarily using 3 buffers by waiting for the sync event in
> get_back_bo. The previous patch would cause the client to use three
> buffers because it would only block when all three buffer slots are
> full. It also means if the compositor is doing something weird and
> keeping all three buffers then the render will fail instead of
> blocking forever, which I think makes more sense.
>
> Daniel, is there any situation where the compositor would keep hold of
> more than one before? If not, surely whenever you attach and commit a
> new buffer you will get the release event for the old buffer
> immediately? If that's the case then I don't think there'd be any need
> to periodically send sync requests to the compositor.
No, you generally don't get the release event as an immediate reply to a
commit request.
1. When you commit a new buffer, compositor schedules a repaint, and
continues processing your requests. If you simply asked for one
wl_display.sync right after the commit, it is acked now.
2. Some time later, the compositor actually enters repaint. It submits
a command stream to the GPU, and schedules a page flip onto the screen.
At this point the compositor sends the frame callbacks, if you
requested one. Then the compositor continues processing requests in the
mean time.
3. Some more time later, the GPU is done, and the page flip is done.
The compositor gets an async notification that the page flip is
complete. Only at *this point* is the compositor able to send a release
for the old buffers.
So you see, step 3 comes much later than your sync for the commit. The
compositor cannot signal the release of any buffers before the page
flip is done, because the GPU or scanout may be using the buffers
still, and you risk re-using a buffer while it being read.
The step 3 is pratically guaranteed to come, the wait for it is not
indefinite, but it is not immediate either. It is not synchronized to
the Wayland protocol, like many other replies are.
The only time when you could get the release for the old buffer as an
immediate reply is, when the old buffer has never been on screen yet.
Provided you also did a wl_display.sync after the commit.
Also don't forget about sub-surfaces, which may hold one additional
buffer indefinitely (until the parent surface is committed).
Unfortunately, I do not have any good suggestions how to solve this,
other than sending the release with 'post', not 'queue'. Could we
perhaps use the hypothetical presentation time stamp event, that would
be fired at step 3, to flush out the release events for a client?
(To complement the frame callback which tells when a client should
start preparing a new frame, there was a plan to add a presentation
timestamp callback to be used for AV-sync etc. telling the time the
frame actually turned into light. I don't know what the status of that
proposal is nowadays.)
Hmm, or could we simply rely on the "never been on screen" behaviour?
Does that make the wl_display.sync proposition work after all?
I'm a little sceptical, but can't think far enough...
Re-thinking again, I would need to re-check the code whether the "never
been on screen" actually applies for non-sub-surfaces. In theory it
should.
Thanks,
pq
> It doesn't look like the old patch has landed in master yet, so here
> is a version 2 of the patch which implements Kristian's suggestion.
>
> Regards,
> - Neil
>
> -- >8 --
>
> 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 instead of
> installing a frame callback it will just call the display sync method
> and throttle itself to that. It needs to at least throttle itself to
> that so that it will still be sure to receive buffer release events.
>
> 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 | 7 ++
> src/egl/drivers/dri2/platform_wayland.c | 127
> ++++++++++++++++++++++++++++++--
> src/egl/drivers/dri2/platform_x11.c | 6 -- 3 files changed, 126
> insertions(+), 14 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h
> b/src/egl/drivers/dri2/egl_dri2.h index fba5f81..cc657ba 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -175,6 +175,7 @@ struct dri2_egl_surface
> int dx;
> int dy;
> struct wl_callback *frame_callback;
> + struct wl_callback *throttle_callback;
> int format;
> #endif
>
> @@ -221,6 +222,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 ffc5959..1cb92ac
> 100644 --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -180,8 +180,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;
> }
>
> /**
> @@ -216,6 +224,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->throttle_callback)
> + wl_callback_destroy(dri2_surf->throttle_callback);
>
> if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> dri2_surf->wl_win->private = NULL;
> @@ -261,8 +271,20 @@ 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->throttle_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 {
> + /* If we aren't throttling to the frame callbacks then the
> compositor
> + * may have sent a release event after the last attach so
> we'll wait
> + * until the sync for the commit request completes in order to
> have the
> + * best chance of reusing a buffer */
> + do {
> + if (wl_display_dispatch_queue(dri2_dpy->wl_dpy,
> + dri2_dpy->wl_queue) == -1)
> + return EGL_FALSE;
> + } while (dri2_surf->throttle_callback != NULL);
> + }
>
> if (dri2_surf->back == NULL) {
> for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> @@ -452,6 +474,21 @@ static const struct wl_callback_listener
> frame_listener = { };
>
> static void
> +wayland_throttle_callback(void *data,
> + struct wl_callback *callback,
> + uint32_t time)
> +{
> + struct dri2_egl_surface *dri2_surf = data;
> +
> + dri2_surf->throttle_callback = NULL;
> + wl_callback_destroy(callback);
> +}
> +
> +static const struct wl_callback_listener throttle_listener = {
> + wayland_throttle_callback
> +};
> +
> +static void
> create_wl_buffer(struct dri2_egl_surface *dri2_surf)
> {
> struct dri2_egl_display *dri2_dpy =
> @@ -511,11 +548,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)
> @@ -559,6 +599,18 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv,
>
> wl_surface_commit(dri2_surf->wl_win->surface);
>
> + if (draw->SwapInterval == 0) {
> + /* If we're not throttling to the frame callback we'll at
> least throttle
> + * to sync events from the server so that it has a chance to
> send us
> + * buffer release events. */
> + dri2_surf->throttle_callback =
> wl_display_sync(dri2_dpy->wl_dpy);
> + wl_callback_add_listener(dri2_surf->throttle_callback,
> + &throttle_listener, dri2_surf);
> + wl_proxy_set_queue((struct wl_proxy *)
> dri2_surf->throttle_callback,
> + dri2_dpy->wl_queue);
> +
> + }
> +
> (*dri2_dpy->flush->flush)(dri2_surf->dri_drawable);
> (*dri2_dpy->flush->invalidate)(dri2_surf->dri_drawable);
>
> @@ -802,6 +854,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)
> {
> @@ -817,6 +923,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 =
> @@ -876,9 +983,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 ec76aec..f2cd4fc 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,
More information about the wayland-devel
mailing list