<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2017-04-20 18:51 GMT+02:00 Mauro Rossi <span dir="ltr"><<a href="mailto:issor.oruam@gmail.com" target="_blank">issor.oruam@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="gmail-h5">2017-04-20 4:18 GMT+02:00 Tomasz Figa <span dir="ltr"><<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-m_8564887339281271716gmail-HOEnZb"><div class="gmail-m_8564887339281271716gmail-h5">On Wed, Apr 19, 2017 at 11:51 PM, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>> wrote:<br>
> Hi Tomasz,<br>
><br>
> On 19 April 2017 at 08:00, Tomasz Figa <<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>> wrote:<br>
>> Android buffer queues can be abandoned, which results in failing to<br>
>> dequeue next buffer. Currently this would fail somewhere deep within<br>
>> the DRI stack calling loader's getBuffers*(), without any error<br>
>> reporting to the client app. However Android framework code relies on<br>
>> proper signaling of this event, so we move buffer dequeue to<br>
>> createWindowSurface() and swapBuffers() call, which can generate proper<br>
>> EGL errors. To keep the performance benefits of delayed buffer handling,<br>
>> if any, fence wait and DRI image creation is kept delayed until<br>
>> getBuffers*() is called by the DRI driver.<br>
>><br>
>> v2:<br>
>> - add missing fence wait in DRI loader case,<br>
>> - split simple code moving to a separate patch (Emil),<br>
>> - fix previous rebase error.<br>
>><br>
>> Signed-off-by: Tomasz Figa <<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>><br>
>> ---<br>
>> src/egl/drivers/dri2/egl_dri2.<wbr>h | 1 +<br>
>> src/egl/drivers/dri2/platform_<wbr>android.c | 94 +++++++++++++++++++-----------<wbr>---<br>
>> 2 files changed, 54 insertions(+), 41 deletions(-)<br>
>><br>
>> diff --git a/src/egl/drivers/dri2/egl_dri<wbr>2.h b/src/egl/drivers/dri2/egl_dri<wbr>2.h<br>
>> index f16663712d..92b234d622 100644<br>
>> --- a/src/egl/drivers/dri2/egl_dri<wbr>2.h<br>
>> +++ b/src/egl/drivers/dri2/egl_dri<wbr>2.h<br>
>> @@ -288,6 +288,7 @@ struct dri2_egl_surface<br>
>> #ifdef HAVE_ANDROID_PLATFORM<br>
>> struct ANativeWindow *window;<br>
>> struct ANativeWindowBuffer *buffer;<br>
>> + int acquire_fence_fd;<br>
>> __DRIimage *dri_image_back;<br>
>> __DRIimage *dri_image_front;<br>
>><br>
>> diff --git a/src/egl/drivers/dri2/platfor<wbr>m_android.c b/src/egl/drivers/dri2/platfor<wbr>m_android.c<br>
>> index 9a84a4c43d..0a75d790c5 100644<br>
>> --- a/src/egl/drivers/dri2/platfor<wbr>m_android.c<br>
>> +++ b/src/egl/drivers/dri2/platfor<wbr>m_android.c<br>
>> @@ -189,15 +189,9 @@ droid_free_local_buffers(struc<wbr>t dri2_egl_surface *dri2_surf)<br>
>> }<br>
>> }<br>
>><br>
>> -static EGLBoolean<br>
>> -droid_window_dequeue_buffer(s<wbr>truct dri2_egl_surface *dri2_surf)<br>
>> +static void<br>
>> +wait_and_close_acquire_fence(<wbr>struct dri2_egl_surface *dri2_surf)<br>
>> {<br>
>> - int fence_fd;<br>
>> -<br>
>> - if (dri2_surf->window->dequeueBuf<wbr>fer(dri2_surf->window, &dri2_surf->buffer,<br>
>> - &fence_fd))<br>
>> - return EGL_FALSE;<br>
>> -<br>
>> /* If access to the buffer is controlled by a sync fence, then block on the<br>
>> * fence.<br>
>> *<br>
>> @@ -215,15 +209,25 @@ droid_window_dequeue_buffer(st<wbr>ruct dri2_egl_surface *dri2_surf)<br>
>> * any value except -1) then the caller is responsible for closing the<br>
>> * file descriptor.<br>
>> */<br>
>> - if (fence_fd >= 0) {<br>
>> + if (dri2_surf->acquire_fence_fd >= 0) {<br>
>> /* From the SYNC_IOC_WAIT documentation in <linux/sync.h>:<br>
>> *<br>
>> * Waits indefinitely if timeout < 0.<br>
>> */<br>
>> int timeout = -1;<br>
>> - sync_wait(fence_fd, timeout);<br>
>> - close(fence_fd);<br>
>> + sync_wait(dri2_surf->acquire_f<wbr>ence_fd, timeout);<br>
>> + close(dri2_surf->acquire_fence<wbr>_fd);<br>
>> + dri2_surf->acquire_fence_fd = -1;<br>
>> }<br>
>> +}<br>
>> +<br>
>> +static EGLBoolean<br>
>> +droid_window_dequeue_buffer(_<wbr>EGLDisplay *disp,<br>
>> + struct dri2_egl_surface *dri2_surf)<br>
>> +{<br>
>> + if (dri2_surf->window->dequeueBuf<wbr>fer(dri2_surf->window, &dri2_surf->buffer,<br>
>> + &dri2_surf->acquire_fence_fd))<br>
>> + return EGL_FALSE;<br>
>><br>
>> dri2_surf->buffer->common.inc<wbr>Ref(&dri2_surf->buffer->common<wbr>);<br>
>><br>
>> @@ -254,6 +258,14 @@ droid_window_dequeue_buffer(st<wbr>ruct dri2_egl_surface *dri2_surf)<br>
>> dri2_surf->back = &dri2_surf->color_buffers[0];<br>
>> }<br>
>><br>
>> + /* free outdated buffers and update the surface size */<br>
>> + if (dri2_surf->base.Width != dri2_surf->buffer->width ||<br>
>> + dri2_surf->base.Height != dri2_surf->buffer->height) {<br>
>> + droid_free_local_buffers(dri2_<wbr>surf);<br>
>> + dri2_surf->base.Width = dri2_surf->buffer->width;<br>
>> + dri2_surf->base.Height = dri2_surf->buffer->height;<br>
>> + }<br>
>> +<br>
>> return EGL_TRUE;<br>
>> }<br>
>><br>
>> @@ -263,6 +275,9 @@ droid_window_enqueue_buffer(_E<wbr>GLDisplay *disp,<br>
>> {<br>
>> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);<br>
>><br>
>> + /* In case we haven't done any rendering. */<br>
>> + wait_and_close_acquire_fence(<wbr>dri2_surf);<br>
>> +<br>
>> /* To avoid blocking other EGL calls, release the display mutex before<br>
>> * we enter droid_window_enqueue_buffer() and re-acquire the mutex upon<br>
>> * return.<br>
>> @@ -319,6 +334,7 @@ droid_create_surface(_EGLDrive<wbr>r *drv, _EGLDisplay *disp, EGLint type,<br>
>> _eglError(EGL_BAD_ALLOC, "droid_create_surface");<br>
>> return NULL;<br>
>> }<br>
>> + dri2_surf->acquire_fence_fd = -1;<br>
>><br>
>> if (!_eglInitSurface(&dri2_surf-><wbr>base, disp, type, conf, attrib_list))<br>
>> goto cleanup_surface;<br>
>> @@ -360,10 +376,18 @@ droid_create_surface(_EGLDrive<wbr>r *drv, _EGLDisplay *disp, EGLint type,<br>
>> if (window) {<br>
>> window->common.incRef(&window-<wbr>>common);<br>
>> dri2_surf->window = window;<br>
>> + if (!droid_window_dequeue_buffer(<wbr>disp, dri2_surf)) {<br>
> Haven't checked the refcounting too close, mostly a gut feeling.<br>
><br>
> Do we need to refcount twice - once prior to calling<br>
> droid_window_dequeue_buffer and a second time within?<br>
> Hmm actually it's consistent with the teardown side - once in<br>
> droid_destroy_surface itself and again in droid_window_enqueue_buffer.<br>
<br>
</div></div>These are different refcounts, one for the window and one the buffer.<br>
However according to the ANativeWindow spec, it might not be necessary<br>
to increment the refcount if the driver references the buffer only<br>
between dequeue and queue. Still, I'd say it's something for a<br>
separate cleanup.<br>
<span class="gmail-m_8564887339281271716gmail-"><br>
><br>
> For the series<br>
> Cc: <<a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop<wbr>.org</a>><br>
> Reviewed-by: Emil Velikov <<a href="mailto:emil.velikov@collabora.com" target="_blank">emil.velikov@collabora.com</a>><br>
<br>
</span>Thanks!<br>
<br>
I'd also like to hear from Mauro if this version, combined with the<br>
patch to use cancelBuffer instead of queueBuffer for destroy surface,<br>
by any chance helps with his wallpaper issue.<br>
<br>
Best regards,<br>
Tomasz<br></blockquote><div><br></div></div></div><div><div>Hi Tomasz,</div><div><br></div><div>I will check Black Wallpaper negative test case this evening or the next,</div><div>I'll be back with results in a couple of days top.</div><div><br></div><div>...</div></div><span class="gmail-HOEnZb"><font color="#888888"><div><br></div><div>Mauro</div></font></span></div><br></div></div>
</blockquote></div><br></div><div class="gmail_extra">Hi Tomasz,</div><div class="gmail_extra"><br></div><div class="gmail_extra">wallpaper is working Ok with nougat-x86, I've rebased on top of 17.1.0rc1 </div><div class="gmail_extra">and applied the three patches after reverting 4d45584,</div><div class="gmail_extra">tested with HD7750</div><div class="gmail_extra"><br></div><div class="gmail_extra">You should have no issue in rebasing on mesa-dev, as I saw no conflict,</div><div class="gmail_extra">and the series should also be nominated candidate for 17.1.0rc2</div><div class="gmail_extra"><br></div><div class="gmail_extra">Mauro</div></div>