[Mesa-dev] [PATCH 3/8] egl/gbm: Implement EGL_EXT_buffer_age

[Kristian Høgsberg]

On Wed, Dec 19, 2012 at 11:17 PM, Eric Anholt <eric at anholt.net> wrote:
> Kristian Høgsberg <krh at bitplanet.net> writes:
>
>> On Sat, Dec 15, 2012 at 2:44 AM, Kristian Høgsberg <hoegsberg at gmail.com> wrote:
>>> On Fri, Dec 14, 2012 at 04:54:32PM -0800, Eric Anholt wrote:
>>>> Kristian Høgsberg <krh at bitplanet.net> writes:
>>>>
>>>> > ---
>>>> >  src/egl/drivers/dri2/egl_dri2.h     |    1 +
>>>> >  src/egl/drivers/dri2/platform_drm.c |   23 +++++++++++++++++++++++
>>>> >  2 files changed, 24 insertions(+)
>>>> >
>>>> > diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
>>>> > index be359d3..fa2a9f3 100644
>>>> > --- a/src/egl/drivers/dri2/egl_dri2.h
>>>> > +++ b/src/egl/drivers/dri2/egl_dri2.h
>>>> > @@ -189,6 +189,7 @@ struct dri2_egl_surface
>>>> >     struct {
>>>> >        struct gbm_bo       *bo;
>>>> >        int                  locked;
>>>> > +      int                  age;
>>>> >     } color_buffers[3], *back, *current;
>>>> >  #ifndef HAVE_WAYLAND_PLATFORM
>>>> >     __DRIbuffer           *dri_buffers[__DRI_BUFFER_COUNT];
>>>> > diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
>>>> > index 3e04a6c..1923033 100644
>>>> > --- a/src/egl/drivers/dri2/platform_drm.c
>>>> > +++ b/src/egl/drivers/dri2/platform_drm.c
>>>> > @@ -324,11 +324,16 @@ dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
>>>> >  {
>>>> >     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>>> >     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
>>>> > +   int i;
>>>> >
>>>> >     if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
>>>> >        if (dri2_surf->current)
>>>> >      _eglError(EGL_BAD_SURFACE, "dri2_swap_buffers");
>>>> > +      for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++)
>>>> > +         if (dri2_surf->color_buffers[i].bo)
>>>> > +            dri2_surf->color_buffers[i].age++;
>>>> >        dri2_surf->current = dri2_surf->back;
>>>> > +      dri2_surf->current->age = 1;
>>>> >        dri2_surf->back = NULL;
>>>>
>>>> OK, if I'm following this right, the ages of my buffers with this code are:
>>>
>>> You're missing the if (dri2_surf->color_buffers[i].bo) condition on
>>> incrementing age.  We start out with bo = NULL for color_buffer[0..2]
>>> and allocate the bos on demand...
>>>
>>>> entering my first swap
>>>> buffers[0] = 0 <- front
>>>> buffers[1] = 0 <- back
>>>>
>>>> after first swap:
>>>> buffers[0] = 1 <- back
>>>> buffers[1] = 1 <- front
>>>
>>> and so here back age will be 0 because we pick a new color_buffer from
>>> the pool that doesn't yet have a bo and whose age was never incremented.
>>>
>>>> after second swap:
>>>> buffers[0] = 1 <- front
>>>> buffers[1] = 2 <- back
>>>>
>>>> But reading the spec, I think it's supposed to be:
>>>>
>>>> entering my first swap
>>>> buffers[0] = 0 <- front
>>>> buffers[1] = 0 <- back
>>>>
>>>> after first swap:
>>>> buffers[0] = 0 <- back
>>>> buffers[1] = 1 <- front
>>>>
>>>> after second swap:
>>>> buffers[0] = 1 <- front
>>>> buffers[1] = 2 <- back
>>>>
>>>> Note how after the first swap, my backbuffer should have an age of 0, for
>>>> "unknown junk".
>>>
>>> Yup, and that's what this patch gets.
>>
>> Does the explanation above make sense?
>
> Sort of -- it's all predicated on there being no way for buffers to be
> allocated other than a swapbuffers -- so no MESA_copy_sub_buffers or its
> cousins, and nothing for binding the front buffer as an egl image (do we
> actually not support any of those?).  It seems more future-proof to just
> implement the "if (buffer->age != 0)" check from the spec, along with or
> in place of the "if (buffer->bo != NULL)"

The way I think of this is that if buffer->bo != NULL, then that slot
in the color_buffer pool is in use and we have a valid buffer age.  In
the code the invariant is that valid buffer iff age > 0, and thus,
testing one or the other doesn't make a difference.  Given that the
spec says "increment if buffer age > 0", I agree that it makes more
sense to go for that condition.

Buffers are allocated at getbuffers time and we don't support any kind
of copy operations for swapbuffers in the gbm or wayland backends.  We
always only swap and copying will require exporting some kind of "copy
region" extension from the dri driver.

Kristian