[Mesa-dev] [PATCH] egl: wayland: bufferAge: only request a back buffer if there isn't one

[Eric Engestrom]

On Tuesday, 2016-12-06 12:13:32 +0000, Lionel Landwerlin wrote:
> On 06/12/16 11:36, Eric Engestrom wrote:
> > On Saturday, 2016-12-03 22:47:17 +0000, Lionel Landwerlin wrote:
> > > Seeing gtk+ application lockup when they query the buffer age of a surface.
> > > 
> > > Since we update the buffer age field only when creating buffers & swaping
> > > them on the client side, there shouldn't be any need for requesting a new
> > > back buffer if there is already one available.
> > > 
> > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84016
> > > ---
> > >   src/egl/drivers/dri2/platform_wayland.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> > > index 395f1e1..84d9ddd 100644
> > > --- a/src/egl/drivers/dri2/platform_wayland.c
> > > +++ b/src/egl/drivers/dri2/platform_wayland.c
> > > @@ -798,7 +798,7 @@ dri2_wl_query_buffer_age(_EGLDriver *drv,
> > >   {
> > >      struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surface);
> > > -   if (get_back_bo(dri2_surf) < 0) {
> > > +   if (dri2_surf->back == NULL && get_back_bo(dri2_surf) < 0) {
> > I'm not convinced this is the right fix: this change the behaviour in
> > two ways:
> > 1. the backing dri_image might not be allocated, which we don't care
> >     about here because we only want to read dri2_surf->back->age.
> 
> I don't see anything in the specification that requires allocating the image
> when the buffer age is queried.
> So this doesn't seem to be an issue right?

I should've been more explicit: this change is fine :)

> 
> > 2. wl_display_dispatch_queue_pending() not being called, ie. we might
> >     operate on stale data.
> 
> I don't really agree with this argument :
> 
> My understanding (correct me if I'm wrong) is that the age value of the back
> buffers is entirely managed by the client.
> It is to be updated when a swap buffer request is issued (also in
> destructions and cases that don't really concern us here, like memory
> pressure events etc...).
> The only information that the display server gives us is that a buffer is
> not used anymore and can therefore be reused as a back buffer, it does not
> change the buffer age value a any buffer.

Not directly, but if I'm reading the code right, a buffer could be
released during wl_display_dispatch_queue_pending(), the new one not
have a backing dri_image (dri2_surf->back->dri_image == NULL), resulting
in a createImage() and `age = 0`.

In this situation, the buffer_age returned with your patch would not
match the back buffer that would actually be used.

Is there anything that makes this not happen in the real world?
Like I said, I'm not an expert in this area, I'm just reading the code,
which I could be misunderstanding :)

Cheers,
  Eric


> 
> Reading the dri3 backend for example, there is no communication between the
> client & server upon buffer age query.
> 
> 
> Thanks a lot for looking this patch.
> 
> Cheers,
> 
> -
> Lionel
> 
> > 
> > If I understand (2) correctly, it means after your patch, we might
> > return the buffer age of a buffer about to be released, instead of
> > processing the queue, letting it get released and getting a new one.
> > This might fix your lockup, but I think this results in potentially
> > invalid data being returned to the client.
> > 
> > Cheers,
> >    Eric
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>