[Mesa-dev] [PATCH] egl: wayland: bufferAge: only request a back buffer if there isn't one

[Lionel Landwerlin]

On 07/12/16 10:55, Eric Engestrom wrote:
> On Tuesday, 2016-12-06 12:13:32 +0000, Lionel Landwerlin wrote:
>> On 06/12/16 11:36, Eric Engestrom wrote:
>>> On Saturday, 2016-12-03 22:47:17 +0000, Lionel Landwerlin wrote:
>>>> Seeing gtk+ application lockup when they query the buffer age of a surface.
>>>>
>>>> Since we update the buffer age field only when creating buffers & swaping
>>>> them on the client side, there shouldn't be any need for requesting a new
>>>> back buffer if there is already one available.
>>>>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84016
>>>> ---
>>>>    src/egl/drivers/dri2/platform_wayland.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
>>>> index 395f1e1..84d9ddd 100644
>>>> --- a/src/egl/drivers/dri2/platform_wayland.c
>>>> +++ b/src/egl/drivers/dri2/platform_wayland.c
>>>> @@ -798,7 +798,7 @@ dri2_wl_query_buffer_age(_EGLDriver *drv,
>>>>    {
>>>>       struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surface);
>>>> -   if (get_back_bo(dri2_surf) < 0) {
>>>> +   if (dri2_surf->back == NULL && get_back_bo(dri2_surf) < 0) {
>>> I'm not convinced this is the right fix: this change the behaviour in
>>> two ways:
>>> 1. the backing dri_image might not be allocated, which we don't care
>>>      about here because we only want to read dri2_surf->back->age.
>> I don't see anything in the specification that requires allocating the image
>> when the buffer age is queried.
>> So this doesn't seem to be an issue right?
> I should've been more explicit: this change is fine :)
>
>>> 2. wl_display_dispatch_queue_pending() not being called, ie. we might
>>>      operate on stale data.
>> I don't really agree with this argument :
>>
>> My understanding (correct me if I'm wrong) is that the age value of the back
>> buffers is entirely managed by the client.
>> It is to be updated when a swap buffer request is issued (also in
>> destructions and cases that don't really concern us here, like memory
>> pressure events etc...).
>> The only information that the display server gives us is that a buffer is
>> not used anymore and can therefore be reused as a back buffer, it does not
>> change the buffer age value a any buffer.
> Not directly, but if I'm reading the code right, a buffer could be
> released during wl_display_dispatch_queue_pending(), the new one not
> have a backing dri_image (dri2_surf->back->dri_image == NULL), resulting
> in a createImage() and `age = 0`.

Ah thanks for the hint. I'm not quite sure about this particular case 
because right after the wl_display_dispatch_queue_pending() we make sure 
to allocate a dri_image and reset age to 0.
Now I could see other cases where we could have a stale value (as you've 
mentioned), for example with resize scenarios.

I'll send another patch to reset the age to 0 upon destruction.

>
> In this situation, the buffer_age returned with your patch would not
> match the back buffer that would actually be used.
>
> Is there anything that makes this not happen in the real world?
> Like I said, I'm not an expert in this area, I'm just reading the code,
> which I could be misunderstanding :)

I'm no expert either :)
Thanks again!

>
> Cheers,
>    Eric
>
>
>> Reading the dri3 backend for example, there is no communication between the
>> client & server upon buffer age query.
>>
>>
>> Thanks a lot for looking this patch.
>>
>> Cheers,
>>
>> -
>> Lionel
>>
>>> If I understand (2) correctly, it means after your patch, we might
>>> return the buffer age of a buffer about to be released, instead of
>>> processing the queue, letting it get released and getting a new one.
>>> This might fix your lockup, but I think this results in potentially
>>> invalid data being returned to the client.
>>>
>>> Cheers,
>>>     Eric
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>