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

[Marathe, Yogesh]

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.

> Best regards,
> Tomasz
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev