Tomasz, > -----Original Message----- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Thursday, August 10, 2017 5:29 PM > To: Marathe, Yogesh <yogesh.mara...@intel.com> > Hi Yogesh, > > On Thu, Aug 10, 2017 at 5:25 PM, Marathe, Yogesh > <yogesh.mara...@intel.com> wrote: > > Hi Tomasz, > > > >> -----Original Message----- > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > >> Behalf Of Tomasz Figa > >> Sent: Tuesday, August 8, 2017 7:45 AM > >> To: Marathe, Yogesh <yogesh.mara...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev