[Mesa-dev] [PATCH] egl/android: prevent deadlock in droid_query_buffer_age

[He, Min]

Hi, Tomasz

On 4/27/2018 5:01 PM, Tomasz Figa wrote:
> Hi Min,
>
> On Fri, Apr 27, 2018 at 11:36 AM Min He <min.he at intel.com> wrote:
>
>> To avoid blocking other EGL calls, release the display mutex before
>> calling update_buffers(), which will call droid_window_dequeue_buffer().
>> The lock appears like below:
>> 1. Consumer thread: updateTexImage() -> updateAndReleaseLocked() ->
>> syncForReleaseLocked() -> eglDupNativeFenceFDANDROID() ->
>> _eglLockDisplay() -> waiting for the dpy lock.
>> 2. Producer thread: eglQuerySurface() (acquired dpy lock) ->
>> droid_query_buffer_age() -> update_buffers() ->
>> android::Surface::dequeueBuffer() ->
>> android::BufferQueueProducer::waitForFreeSlotThenRelock()
>> This patch fixes some failure cases in android graphics cts test.
>> Signed-off-by: Min He <min.he at intel.com>
>> Signed-off-by: Chenglei Ren <chenglei.ren at intel.com>
>> Tested-by: Chenglei Ren <chenglei.ren at intel.com>
>> ---
>>    src/egl/drivers/dri2/platform_android.c | 7 +++++++
>>    1 file changed, 7 insertions(+)
>> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
>> index 7f1a496..c6f895a 100644
>> --- a/src/egl/drivers/dri2/platform_android.c
>> +++ b/src/egl/drivers/dri2/platform_android.c
>> @@ -610,11 +610,18 @@ droid_query_buffer_age(_EGLDriver *drv,
>>    {
>>       struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surface);
>> +   /* To avoid blocking other EGL calls, release the display mutex before
>> +    * we enter droid_window_dequeue_buffer() and re-acquire the mutex
> upon
>> +    * return.
>> +    */
>> +   mtx_unlock(&disp->Mutex);
>>       if (update_buffers(dri2_surf) < 0) {
>>          _eglError(EGL_BAD_ALLOC, "droid_query_buffer_age");
>> +      mtx_lock(&disp->Mutex);
>>          return -1;
>>       }
>> +   mtx_lock(&disp->Mutex);
> With droid_window_enqueue_buffer(), we put the unlock just around
> window->queueBuffer(). I'd say that at least for consistency, we should do
> similar thing inside droid_window_dequeue_buffer(). Moreover,
> droid_window_dequeue_buffer() actually does much more inside than
> droid_window_enqueue_buffer(), so we might actually want to have the mutex
> held there, when accessing internal data.
I don't see the disp variable passed to update_buffer(), will it be used 
inside of later
calls to droid_window_dequeue_buffer() ? If not, why can't we release 
the lock here and
then capture it later? I'm new to the mesa, so please correct me if I 
made any mistakes.

Thanks
> Best regards,
> Tomasz