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. Best regards, Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
