Re: [PATCH 1/2] wayland: Add support for eglSwapInterval
On Wed, 11 Sep 2013 22:29:13 +0200
Axel Davy wrote:
> I think you should too set the number of back buffers to 4 instead of
> 3.
>
> It looks like if the compositor wants to use the buffers as
> framebuffers and do a pageflip,
> it uses 2 buffers at a time (one used for the frame displayed, and
> one used for the pending pageFlip request). The third buffer sent is
> not released and waits for the next frame. If there is only 3
> buffers, the client will be without free buffers and blocked in
> eglSwapBuffers. I may be wrong, but I think the number of back
> buffers should be set to 4.
We had a lengthy discussion in IRC about that, and came to the idea
that we might need the maximum to be actually 5, not 4.
The +1 comes from sub-surfaces, and *might* happen when you have a
swapinterval=0 EGL rendering on a sub-surface in a busy-loop in a
thread, the sub-surface is in synchronized mode, parent surface gets
committed only occasionally, and the sub-surface ends up in an hardware
overlay plane. Sheesh...
If we really want to guarantee maximal framerate on the sub-surface
whose display is throttled not only by the compositor, but other parts
of the client too...
Thanks,
pq
> Le 11/09/2013 20:28, Neil Roberts a écrit :
> > 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 it simply
> > doesn't install a frame callback so that the next time
> > eglSwapBuffers is called it won't delay.
> >
> > The second change is that in get_back_bo instead of returning with
> > an error if all three buffers are locked it will now block in a
> > dispatch loop so that it can receive the buffer release events. The
> > assumption is that the compositor is unlikely to lock all three
> > buffers so if we find that all the buffers are locked then we are
> > probably just rendering faster than we are processing the release
> > events. Therefore the release events should be available very early.
> >
> > 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 | 6 ++
> > src/egl/drivers/dri2/platform_wayland.c | 121
> > ++--
> > src/egl/drivers/dri2/platform_x11.c | 6 -- 3 files changed,
> > 107 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.h
> > b/src/egl/drivers/dri2/egl_dri2.h index fba5f81..849927b 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.h
> > +++ b/src/egl/drivers/dri2/egl_dri2.h
> > @@ -221,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 ffc5959..83e7aab
> > 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;
> > }
> >
> > /**
> > @@ -261,24 +269,36 @@ 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->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];
> > + /* There might be a buffer release
Re: [PATCH 1/2] wayland: Add support for eglSwapInterval
I think you should too set the number of back buffers to 4 instead of 3.
It looks like if the compositor wants to use the buffers as framebuffers
and do a pageflip,
it uses 2 buffers at a time (one used for the frame displayed, and one
used for the pending pageFlip request). The third buffer sent is not
released and waits for the next frame. If there is only 3 buffers, the
client will be without free buffers and blocked in eglSwapBuffers.
I may be wrong, but I think the number of back buffers should be set to 4.
Axel Davy
Le 11/09/2013 20:28, Neil Roberts a écrit :
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 it simply doesn't
install a frame callback so that the next time eglSwapBuffers is called it
won't delay.
The second change is that in get_back_bo instead of returning with an error if
all three buffers are locked it will now block in a dispatch loop so that it
can receive the buffer release events. The assumption is that the compositor
is unlikely to lock all three buffers so if we find that all the buffers are
locked then we are probably just rendering faster than we are processing the
release events. Therefore the release events should be available very early.
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 | 6 ++
src/egl/drivers/dri2/platform_wayland.c | 121 ++--
src/egl/drivers/dri2/platform_x11.c | 6 --
3 files changed, 107 insertions(+), 26 deletions(-)
diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index fba5f81..849927b 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -221,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 ffc5959..83e7aab 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;
}
/**
@@ -261,24 +269,36 @@ 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->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];
+ /* 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);
+
+ 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)
+break;
+
+ /* If we make it here then here then all of the buffers are locked.
+ * It wouldn't make sense for the compositor to keep all three
+ * buffers so it must mean that we are rendering too fast without
