[Mesa-dev] [PATCH 1/2] loader/dri3: Use client local back to front blit in copySubBuffer if available

[Axel Davy]

Hi,

On 05/09/2017 08:37, Thomas Hellstrom wrote:
> Hi!
>
> On 09/05/2017 07:36 AM, Axel Davy wrote:
>> On 04/09/2017 14:27, Thomas Hellstrom wrote:
>>> The copySubBuffer functionality always attempted a server side blit 
>>> from
>>> back to fake front if a fake front was present, and we weren't 
>>> displaying
>>> on a remote GPU.
>>>
>>> Now that we always have local blit capability on modern drivers, first
>>> attempt a local blit, and only if that fails, try the server blit.
>>>
>>> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
>>> ---
>>>   src/loader/loader_dri3_helper.c | 16 +++++++---------
>>>   1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/loader/loader_dri3_helper.c 
>>> b/src/loader/loader_dri3_helper.c
>>> index e3120f5..c0a6e0d 100644
>>> --- a/src/loader/loader_dri3_helper.c
>>> +++ b/src/loader/loader_dri3_helper.c
>>> @@ -635,14 +635,6 @@ loader_dri3_copy_sub_buffer(struct 
>>> loader_dri3_drawable *draw,
>>>                                       back->image,
>>>                                       0, 0, back->width, back->height,
>>>                                       0, 0, __BLIT_FLAG_FLUSH);
>>> -      /* We use blit_image to update our fake front,
>>> -       */
>>> -      if (draw->have_fake_front)
>>> -         (void) loader_dri3_blit_image(draw,
>>> - dri3_fake_front_buffer(draw)->image,
>>> -                                       back->image,
>>> -                                       x, y, width, height,
>>> -                                       x, y, __BLIT_FLAG_FLUSH);
>>>      }
>> Why removing that part ? It's a bit easier to read when the 
>> is_different_gpu path is separated to the normal path here.
>
> It's less code. We're avoiding duplicating the same code in two paths.
>
>>
>>> loader_dri3_swapbuffer_barrier(draw);
>>> @@ -656,7 +648,13 @@ loader_dri3_copy_sub_buffer(struct 
>>> loader_dri3_drawable *draw,
>>>      /* Refresh the fake front (if present) after we just damaged 
>>> the real
>>>       * front.
>>>       */
>>> -   if (draw->have_fake_front && !draw->is_different_gpu) {
>>> +   if (draw->have_fake_front &&
>>> +       !loader_dri3_blit_image(draw,
>>> + dri3_fake_front_buffer(draw)->image,
>>> +                               back->image,
>>> +                               x, y, width, height,
>>> +                               x, y, __BLIT_FLAG_FLUSH) &&
>>> +       !draw->is_different_gpu) {
>>>         dri3_fence_reset(draw->conn, dri3_fake_front_buffer(draw));
>>>         dri3_copy_area(draw->conn,
>>>                        back->pixmap,
>>
>> In the case of is_different_gpu, reverting to using a server copy if 
>> loader_dri3_blit_image fails doesn't seem a good choice, because what 
>> the server sees is the linear copy of the buffers. You'd have to add 
>> a blit from the linear copy to the tiled buffer (which won't work if 
>> local blit is not available).
>
> The server copy is never performed if is_different_gpu is true. In the 
> "is_different_gpu" path, the functionality is not changed, except the 
> back-to-fake-front blit is processed *after* the linear-to-real-front 
> blit has been scheduled.
>
Right, this is correct.

> Since, in the is_different_gpu case, dri3 refuses to start unless we 
> have local blit capability, a failure of loader_dri3_blit_image means 
> that we have failed a context creation and in that case, skipping the 
> back-to-fake-front blit seems reasonable to me.
Right, initially it was possible to not get the context for the blit, 
and thus the blit could fail. However I see the current code should 
always get the context, everything should be fine then.

Reviewed-by: Axel Davy <axel.davy at normalesup.org>

>
> Thanks,
> /Thomas
>
>>
>>
>> Yours,
>>
>>
>> Axel Davy
>>
>