[Mesa-dev] [PATCH 3/3] egl/android: Dequeue buffers inside EGL calls (v2)

[Tomasz Figa]

On Sat, Apr 22, 2017 at 11:53 PM, Mauro Rossi <issor.oruam at gmail.com> wrote:
> Hi Tomasz,
>
> last minute doubt
>
> 2017-04-19 9:00 GMT+02:00 Tomasz Figa <tfiga at chromium.org>:
>>
>> Android buffer queues can be abandoned, which results in failing to
>> dequeue next buffer. Currently this would fail somewhere deep within
>> the DRI stack calling loader's getBuffers*(), without any error
>> reporting to the client app. However Android framework code relies on
>> proper signaling of this event, so we move buffer dequeue to
>> createWindowSurface() and swapBuffers() call, which can generate proper
>> EGL errors. To keep the performance benefits of delayed buffer handling,
>> if any, fence wait and DRI image creation is kept delayed until
>> getBuffers*() is called by the DRI driver.
>>
>> v2:
>>  - add missing fence wait in DRI loader case,
>>  - split simple code moving to a separate patch (Emil),
>>  - fix previous rebase error.
>>
>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>> ---
>>  src/egl/drivers/dri2/egl_dri2.h         |  1 +
>>  src/egl/drivers/dri2/platform_android.c | 94
>> +++++++++++++++++++--------------
>>  2 files changed, 54 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.h
>> b/src/egl/drivers/dri2/egl_dri2.h
>> index f16663712d..92b234d622 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.h
>> +++ b/src/egl/drivers/dri2/egl_dri2.h
>> @@ -288,6 +288,7 @@ struct dri2_egl_surface
>>  #ifdef HAVE_ANDROID_PLATFORM
>>     struct ANativeWindow *window;
>>     struct ANativeWindowBuffer *buffer;
>> +   int acquire_fence_fd;
>>     __DRIimage *dri_image_back;
>>     __DRIimage *dri_image_front;
>>
>> diff --git a/src/egl/drivers/dri2/platform_android.c
>> b/src/egl/drivers/dri2/platform_android.c
>> index 9a84a4c43d..0a75d790c5 100644
>> --- a/src/egl/drivers/dri2/platform_android.c
>> +++ b/src/egl/drivers/dri2/platform_android.c
>> @@ -189,15 +189,9 @@ droid_free_local_buffers(struct dri2_egl_surface
>> *dri2_surf)
>>     }
>>  }
>>
>> -static EGLBoolean
>> -droid_window_dequeue_buffer(struct dri2_egl_surface *dri2_surf)
>
>
> Here droid_window_dequeue_buffer() had one argument...TO BE CONTINUED
>
>
>>
>> +static void
>> +wait_and_close_acquire_fence(struct dri2_egl_surface *dri2_surf)
>>  {
>> -   int fence_fd;
>> -
>> -   if (dri2_surf->window->dequeueBuffer(dri2_surf->window,
>> &dri2_surf->buffer,
>> -                                        &fence_fd))
>> -      return EGL_FALSE;
>> -
>>     /* If access to the buffer is controlled by a sync fence, then block
>> on the
>>      * fence.
>>      *
>> @@ -215,15 +209,25 @@ droid_window_dequeue_buffer(struct dri2_egl_surface
>> *dri2_surf)
>>      *    any value except -1) then the caller is responsible for closing
>> the
>>      *    file descriptor.
>>      */
>> -    if (fence_fd >= 0) {
>> +    if (dri2_surf->acquire_fence_fd >= 0) {
>>         /* From the SYNC_IOC_WAIT documentation in <linux/sync.h>:
>>          *
>>          *    Waits indefinitely if timeout < 0.
>>          */
>>          int timeout = -1;
>> -        sync_wait(fence_fd, timeout);
>> -        close(fence_fd);
>> +        sync_wait(dri2_surf->acquire_fence_fd, timeout);
>> +        close(dri2_surf->acquire_fence_fd);
>> +        dri2_surf->acquire_fence_fd = -1;
>>     }
>> +}
>> +
>> +static EGLBoolean
>> +droid_window_dequeue_buffer(_EGLDisplay *disp,
>> +                            struct dri2_egl_surface *dri2_surf)
>
>
> ...and here has two arguments, _EGLDisplay *disp was added
>
> Is it necessary or wanted for future proof design or upcoming changes?

Oh, good catch. There should be one more hunk in the patch that adds a
mutex unlock, to allow Android code triggered by
window->dequeueBuffer() to execute other EGL calls. I've been seeing
deadlocks without that change. The mutex doesn't protect anything used
in the swap buffers path and I'd even argue that the global EGL mutex
that Mesa's EGL code acquires is actually non-conformant with the EGL
spec...

Best regards,
Tomasz