2017-04-20 4:18 GMT+02:00 Tomasz Figa <[email protected]
<mailto:[email protected]>>:
On Wed, Apr 19, 2017 at 11:51 PM, Emil Velikov
<[email protected] <mailto:[email protected]>> wrote:
> Hi Tomasz,
>
> On 19 April 2017 at 08:00, Tomasz Figa <[email protected]
<mailto:[email protected]>> 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 <[email protected]
<mailto:[email protected]>>
>> ---
>> 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: <[email protected]
<mailto:[email protected]>>
> Reviewed-by: Emil Velikov <[email protected]
<mailto:[email protected]>>
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.
