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

[Tomasz Figa]

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.

Best regards,
Tomasz