[Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS

[Marathe, Yogesh]

Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga at chromium.org]
> Sent: Thursday, August 10, 2017 5:29 PM
> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> Hi Yogesh,
> 
> On Thu, Aug 10, 2017 at 5:25 PM, Marathe, Yogesh
> <yogesh.marathe at intel.com> wrote:
> > Hi Tomasz,
> >
> >> -----Original Message-----
> >> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> >> Behalf Of Tomasz Figa
> >> Sent: Tuesday, August 8, 2017 7:45 AM
> >> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> >> >> > >> Changing the topic, the patch doesn't seem to change the
> >> >> > >> implementation of swapBuffers to stop doing a flush on the
> >> >> > >> buffer, which defeats the purpose of the fence, as the it is
> >> >> > >> likely already signaled at the time it is passed to
> >> >> > >> queueBuffer. Shouldn't
> >> we fix this?
> >> >> > >>
> >> >> > >
> >> >> > > I have been wondering about it all the while, when I had
> >> >> > > prints in
> >> >> > > Fence::getSignalTime() to check finfo->status from consumer
> >> >> > > side during initial revisions, I always found it to be signaled!
> >> >> > >
> >> >> > > Can we really remove that flush in swapBuffers? In that case I
> >> >> > > believe the consumer _must_ wait on fence before really
> >> >> > > accessing it, so that would trigger a change in buffer consumer /
> application!
> >> >> >
> >> >> > The consumer must _always_ wait on the acquire fence if it's a
> >> >> > valid FD, as this is how the ANativeWindow interface is defined.
> >> >> > You can see Mesa already does it in droid_dequeue_buffer(). If
> >> >> > you find a consumer that is not doing so, it's a bug in the consumer.
> >> >> > There is no compatibility concern here, as it's strictly
> >> >> > regulated by Android
> >> specifications.
> >> >>
> >> >> I checked this, yes, BufferConsumer waits on fence provided its valid.
> >> >
> >> > Hi Tomasz,
> >> >
> >> > Is it ok to move that 'flush' removal change to separate commit? I
> >> > would opt for that. This gflush removal change is going to trigger
> >> > additional tests, while this one fixes the issue for now and has
> >> > list of review comments done. If this is fine, I'll push v6 for this.
> >>
> >> I'm okay with either.
> >>
> >
> > I found GLConsumer aosp has glFlush() is already, it means we had two
> > flush calls in the path, one in swapBuffers and other in libgui on consumer.
> >
> > I went ahead and removed  dri2_flush_drawable_for_swapbuffer.
> > Functionally, things seem to be ok. I assume this will be valid only
> > for android with valid fence changes and not for other platforms. Is this right
> expectation? Diff below.
> >
> > diff --git a/src/egl/drivers/dri2/platform_android.c
> > b/src/egl/drivers/dri2/platform_android.c
> > index 8bca753..80da021 100644
> > --- a/src/egl/drivers/dri2/platform_android.c
> > +++ b/src/egl/drivers/dri2/platform_android.c
> > @@ -706,7 +706,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLSurface *draw)
> >     if (dri2_surf->back)
> >        dri2_surf->back->age = 1;
> >
> > -   dri2_flush_drawable_for_swapbuffers(disp, draw);
> >
> >     /* dri2_surf->buffer can be null even when no error has occured. For
> >      * example, if the user has called no GL rendering commands since
> > the
> >
> > If this is only change, I don’t think we need separate patch here. Please correct
> me if I'm wrong.
> 
> I think I have been mistaken in what
> dri2_flush_drawable_for_swabuffers() does. We need to keep it there as it
> makes the DRI2 driver issue operations finalizing the buffer, e.g.
> remaining drawing or a multisample resolve if necessary. However it doesn't do
> any synchronous wait on the queued operations, so there is no performance
> loss.
> 

I am OK either ways. Another thing, glFlush in libgui seems legitimate, it is
required in order to have eglDupNativeFenceFDANDROID() return a valid fd!

Now only pending thing is the 'old display surface' fence then, and we have decided
to wait for someone else from android to comment on it.

> Best regards,
> Tomasz