[Mesa-dev] [PATCH 3/3] egl/android: Dequeue buffers inside EGL calls (v2)
Tapani Pälli
tapani.palli at intel.com
Fri Apr 21 04:23:25 UTC 2017
On 04/20/2017 07:51 PM, Mauro Rossi wrote:
>
>
> 2017-04-20 4:18 GMT+02:00 Tomasz Figa <tfiga at chromium.org
> <mailto:tfiga at chromium.org>>:
>
> On Wed, Apr 19, 2017 at 11:51 PM, Emil Velikov
> <emil.l.velikov at gmail.com <mailto:emil.l.velikov at gmail.com>> wrote:
> > Hi Tomasz,
> >
> > On 19 April 2017 at 08:00, Tomasz Figa <tfiga at chromium.org
> <mailto: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
> <mailto: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
> <mailto:mesa-stable at lists.freedesktop.org>>
> > Reviewed-by: Emil Velikov <emil.velikov at collabora.com <mailto: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.
>
> If you/Tapani/others have experience with Android-CTS 7 or other means
> to launch CTS/piglit test sessions on nougat, could you please share
> with me?
> Any attempt to launch Android CTS 7 has failed for me.
>
> In this moment I'm only able to launch Android CTS 6 on marshmallow-x86,
> and results diff is not as easy as for piglit on linux.
I use dEQP so that I build cts suite (m -j4 cts) and then install deqp
apk from that build (can be found from
out/com.drawelements.deqp_intermediates folder) and then run dEQP tests
via adb like this:
adb shell "am start -n com.drawelements.deqp/android.app.NativeActivity
-e cmdLine 'deqp --deqp-case=dEQP-VK.wsi.android.swapchain.render.basic
--deqp-log-filename=/sdcard/dEQP-Log.qpa'"
> I wish some GPU compatibility test suite had been integrated in Android
> Studio/GUI
>
> Mauro
>
More information about the mesa-dev
mailing list