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

[Emil Velikov]

Hi Tomasz,

On 19 April 2017 at 08:00, Tomasz Figa <tfiga at chromium.org> wrote:
> 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)
> +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)
> +{
> +   if (dri2_surf->window->dequeueBuffer(dri2_surf->window, &dri2_surf->buffer,
> +                                        &dri2_surf->acquire_fence_fd))
> +      return EGL_FALSE;
>
>     dri2_surf->buffer->common.incRef(&dri2_surf->buffer->common);
>
> @@ -254,6 +258,14 @@ droid_window_dequeue_buffer(struct dri2_egl_surface *dri2_surf)
>        dri2_surf->back = &dri2_surf->color_buffers[0];
>     }
>
> +   /* free outdated buffers and update the surface size */
> +   if (dri2_surf->base.Width != dri2_surf->buffer->width ||
> +       dri2_surf->base.Height != dri2_surf->buffer->height) {
> +      droid_free_local_buffers(dri2_surf);
> +      dri2_surf->base.Width = dri2_surf->buffer->width;
> +      dri2_surf->base.Height = dri2_surf->buffer->height;
> +   }
> +
>     return EGL_TRUE;
>  }
>
> @@ -263,6 +275,9 @@ droid_window_enqueue_buffer(_EGLDisplay *disp,
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>
> +   /* In case we haven't done any rendering. */
> +   wait_and_close_acquire_fence(dri2_surf);
> +
>     /* To avoid blocking other EGL calls, release the display mutex before
>      * we enter droid_window_enqueue_buffer() and re-acquire the mutex upon
>      * return.
> @@ -319,6 +334,7 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>        _eglError(EGL_BAD_ALLOC, "droid_create_surface");
>        return NULL;
>     }
> +   dri2_surf->acquire_fence_fd = -1;
>
>     if (!_eglInitSurface(&dri2_surf->base, disp, type, conf, attrib_list))
>        goto cleanup_surface;
> @@ -360,10 +376,18 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>     if (window) {
>        window->common.incRef(&window->common);
>        dri2_surf->window = window;
> +      if (!droid_window_dequeue_buffer(disp, dri2_surf)) {
Haven't checked the refcounting too close, mostly a gut feeling.

Do we need to refcount twice - once prior to calling
droid_window_dequeue_buffer and a second time within?
Hmm actually it's consistent with the teardown side - once in
droid_destroy_surface itself and again in droid_window_enqueue_buffer.

For the series
Cc: <mesa-stable at lists.freedesktop.org>
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

Thanks
Emil