[Mesa-dev] [PATCH 3/3] egl/android: Dequeue buffers inside EGL calls (v2)
Mauro Rossi
issor.oruam at gmail.com
Fri Apr 21 07:35:27 UTC 2017
2017-04-20 18:51 GMT+02:00 Mauro Rossi <issor.oruam at gmail.com>:
>
>
> 2017-04-20 4:18 GMT+02:00 Tomasz Figa <tfiga at chromium.org>:
>
>> On Wed, Apr 19, 2017 at 11:51 PM, Emil Velikov <emil.l.velikov at gmail.com>
>> wrote:
>> > 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.
>>
>> These are different refcounts, one for the window and one the buffer.
>> However according to the ANativeWindow spec, it might not be necessary
>> to increment the refcount if the driver references the buffer only
>> between dequeue and queue. Still, I'd say it's something for a
>> separate cleanup.
>>
>> >
>> > For the series
>> > Cc: <mesa-stable at lists.freedesktop.org>
>> > Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>>
>> Thanks!
>>
>> I'd also like to hear from Mauro if this version, combined with the
>> patch to use cancelBuffer instead of queueBuffer for destroy surface,
>> by any chance helps with his wallpaper issue.
>>
>> Best regards,
>> Tomasz
>>
>
> Hi Tomasz,
>
> I will check Black Wallpaper negative test case this evening or the next,
> I'll be back with results in a couple of days top.
>
> ...
>
> Mauro
>
>
Hi Tomasz,
wallpaper is working Ok with nougat-x86, I've rebased on top of 17.1.0rc1
and applied the three patches after reverting 4d45584,
tested with HD7750
You should have no issue in rebasing on mesa-dev, as I saw no conflict,
and the series should also be nominated candidate for 17.1.0rc2
Mauro
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170421/2d7b0008/attachment-0001.html>
More information about the mesa-dev
mailing list