On Fri, 13 Sep 2013 17:04:38 +0100
Neil Roberts <n...@linux.intel.com> wrote:
> Here is another version of the patch which brings back the blocking
> when there are no buffers available to cope with the situation where
> the compositor isn't immediately releasing buffers. Maybe we could
> leave the decision about whether to increase the buffer count to 4 as
> a separate patch.
Hi Neil,
yes, I think this approach is how it should work. There are a few
details commented below.
>
> -- >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. When the application is not running fullscreen the compositor is
> likely to release the previous buffer immediately so this gives the
> application the best chance of reusing the buffer.
>
> If there are no buffers available then instead of returning with an error,
> get_back_bo will now block until a buffer becomes available. This is necessary
> if the compositor is not releasing buffers immediately. As there are only
> three buffers, this could actually mean that the client ends up throttled to
> the vblank anyway because Weston can hold on to three buffers when the client
> is fullscreen. We could fix this by increasing the buffer count to 4 or
> changing Weston and KMS to allow cancelling a pending buffer swap, but for now
> this patch ignores that problem.
A good explanation, cool.
> 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.
This little part could be a separate patch, so it could go in already.
> ---
> src/egl/drivers/dri2/egl_dri2.h | 7 ++
> src/egl/drivers/dri2/platform_wayland.c | 159
> ++++++++++++++++++++++++++++----
> src/egl/drivers/dri2/platform_x11.c | 6 --
> 3 files changed, 147 insertions(+), 25 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..6ee6ffb 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,24 +271,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->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;
Here you compare to -1 and return EGL_FALSE, but...
> + } while (dri2_surf->throttle_callback != NULL);
What was the rationale for actually waiting for the sync to return
here, instead of implicitly relying it to kick the release events and
waiting below for a buffer to become available?
> + }
>
> 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.
> + * The compositor shouldn't be keeping so many buffers for a long
> + * time so we should be able to reliably wait for a release event */
> + if (wl_display_dispatch_queue(dri2_dpy->wl_dpy,
> + dri2_dpy->wl_queue) < 0)
> + return -1;
...here you compare to 0 and return -1. Could use consistency, and
fixing the return value in one of these places.
> }
> }
>
> - 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,
> @@ -452,6 +484,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;
Would it be appropriate to add:
assert(callback == dri2_surf->throttle_callback);
or do you expect to possibly have multiple sync callbacks in flight
for one dri2_surf, where only the first one is meaningful?
> +
> + 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 +558,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 +609,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. */
A few points about this comment:
- You'll be throttling to *one* sync event.
- You should document why this one sync event is needed, and why it
needs to be waited for. Otherwise one could simply wait for release
events only.
> + 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 +864,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 +933,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 +993,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,
Thanks,
pq
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
