[Mesa-dev] [RFC PATCH] egl/android: Dequeue buffers inside EGL calls

[Tomasz Figa]

On Mon, Apr 3, 2017 at 2:42 PM, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 04/02/2017 08:12 PM, Tomasz Figa wrote:
>>
>> Sorry for replying to myself, just got enlightened...
>>
>> On Mon, Apr 3, 2017 at 2:07 AM, Tomasz Figa <tfiga at chromium.org> wrote:
>>>
>>> Hi Mauro,
>>>
>>> On Mon, Apr 3, 2017 at 1:38 AM, Mauro Rossi <issor.oruam at gmail.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> 2017-03-30 16:17 GMT+02:00 Emil Velikov <emil.l.velikov at gmail.com>:
>>>>>
>>>>>
>>>>> On 30 March 2017 at 11:55, 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.
>>>>>>
>>>>> Thank you Tomasz.
>>>>>
>>>>> I'm fairly confident that this should resolve the crash [in
>>>>> swap_buffers] that Mauro was seeing.
>>>>> Mauro can you give it a test ?
>>>>
>>>>
>>>>
>>>> After applying last version of Tomasz patch,
>>>> I could not boot nougat-x86, the same way as per Tapani get_back_bo()
>>>> throwing and EGL_BAD_ALLOC
>>>> which is a show stopper for surfaceflinger
>>>
>>>
>>> Hmm, must be something I missed in the code, because with my patch
>>> applied, there should be no condition that could make get_back_bo()
>>> fail, unless previous swap_buffers() failed in droid_dequeue_buffer()
>>> or there is something wrong with the first buffer being dequeued in
>>> create_surface(). Would you be able to check where exactly
>>> get_back_bo() fails with your setup?
>>
>>
>> Ah, wait, I just realized that get_back_bo() is valid only when the
>> image loader is used, which is only on DMA-buf FD-based systems. No
>> wonder that it fails on android-x86.
>>
>> Tapani, would you be able to give a bit more details on the crash
>> being observed without that call? AFAICT, without my patch, even with
>> the call, the code is still not fully correct, because on the first
>> call to swap_buffers() without any rendering the dri2_surf->buffer
>> would be NULL and get_back_bo() would simply fail (but not crash
>> indeed). With my patch, it won't fail, because there is always a
>> buffer dequeued, so it should be the closest to correct behavior.
>
>
> Reason why that crash started to happen (observed only in one app so far!)
> were changes done in 'EGL/Android: Add EGL_EXT_buffer_age extension'. That
> patch adds 'age' for buffers and modifies droid_swap_buffers to set back
> buffer age to value 1. My original patch was to set the age only if there is
> a back buffer but then discussed with Emil and decided to unify approach
> with other backends and call get_back_bo.
>
> You are right that in the crash case dri2_surf->buffer is NULL as well.
>
> for long time flow looks like this:
>
> 01-01 00:00:54.087  2771  2791 D EGL-DRI2: droid_swap_buffers:
> dri2_surf->buffer 0xc8c88788 dri2_surf->back 0xca35ede8
> 01-01 00:00:54.104  2771  2791 D EGL-DRI2: droid_swap_buffers:
> dri2_surf->buffer 0xc8e84e08 dri2_surf->back 0xca35edd8
> 01-01 00:00:54.120 2771 2791 D EGL-DRI2: droid_swap_buffers:
> dri2_surf->buffer 0xc8c885a8 dri2_surf->back 0xca35ede0
>
> then all of sudden there is a frame where buffer and back are 0:
>
> 01-01 00:00:55.386  2771  2791 D EGL-DRI2: droid_swap_buffers:
> dri2_surf->buffer 0xc8c885a8 dri2_surf->back 0xca35ede0
> 01-01 00:00:55.390  2771  2831 D EGL-DRI2: droid_swap_buffers:
> dri2_surf->buffer 0xc8c8b2a8 dri2_surf->back 0xca360098
> 01-01 00:00:55.391  2771  2831 D EGL-DRI2: droid_swap_buffers:
> dri2_surf->buffer 0x0 dri2_surf->back 0x0

I guess it means that there was no rendering happening between the two
swaps above, so my patch should take care of this case indeed and the
call to get_back_bo() is not needed anymore (actually it should have
been update_buffers(), not get_back_bo()).

Thanks for really helpful explanation.

Best regards,
Tomasz