[Mesa-dev] [PATCH] egl/wayland: Support for KHR_partial_update

[Emil Velikov]

On 17 October 2017 at 17:34, Harish Krupo <harish.krupo.kps at intel.com> wrote:
> Hi Emil,
>
> Thank you for the comments, will fix the code accordingly and send a
> v2 patch.
> I have answered the question inline.
>
> Emil Velikov <emil.l.velikov at gmail.com> writes:
>
>> Hi Harish,
>>
>> Overall looks great, a few comments/questions inline.
>>
>> On 13 October 2017 at 19:49, Harish Krupo <harish.krupo.kps at intel.com> wrote:
>>> This passes 33/37 deqp tests related to partial_update, 4 are not
>>> supported.
>>>
>>> Signed-off-by: Harish Krupo <harish.krupo.kps at intel.com>
>>> ---
>>>  src/egl/drivers/dri2/platform_wayland.c | 68 ++++++++++++++++++++++++++++-----
>>>  1 file changed, 59 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
>>> index 14db55ca74..483d588b92 100644
>>> --- a/src/egl/drivers/dri2/platform_wayland.c
>>> +++ b/src/egl/drivers/dri2/platform_wayland.c
>>> @@ -810,15 +810,39 @@ try_damage_buffer(struct dri2_egl_surface *dri2_surf,
>>>     }
>>>     return EGL_TRUE;
>>>  }
>>> +
>>>  /**
>>> - * Called via eglSwapBuffers(), drv->API.SwapBuffers().
>>> + * Called via eglSetDamageRegionKHR(), drv->API.SetDamageRegion().
>>>   */
>>>  static EGLBoolean
>>> -dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>>> -                                 _EGLDisplay *disp,
>>> -                                 _EGLSurface *draw,
>>> -                                 const EGLint *rects,
>>> -                                 EGLint n_rects)
>>> +wl_set_damage_region(_EGLDriver *drv,
>> Yes, it might be a bit confusing, but please stay consistent and call
>> this dri2_wl_set_damage_region.
>>
>>> +                     _EGLDisplay *dpy,
>>> +                     _EGLSurface *surf,
>>> +                     const EGLint *rects,
>>> +                     EGLint n_rects)
>>> +{
>>> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
>>> +
>>> +   /* The spec doesn't mention what should be returned in case of
>>> +    * failure in setting the damage buffer with the window system, so
>>> +    * setting the damage to maximum surface area
>>> +   */
>>> +   if (!n_rects || !try_damage_buffer(dri2_surf, rects, n_rects)) {
>>> +      wl_surface_damage(dri2_surf->wl_surface_wrapper,
>>> +                        0, 0, INT32_MAX, INT32_MAX);
>>> +      return EGL_TRUE;
>> Drop this return - it's only confusing the reader.
>>
>>> +   }
>>> +
>>> +   return EGL_TRUE;
>> ... since this should suffice.
>>
>>> +}
>>> +
>>> +static EGLBoolean
>>> +dri2_wl_swap_buffers_common(_EGLDriver *drv,
>>> +                            _EGLDisplay *disp,
>>> +                            _EGLSurface *draw,
>>> +                            const EGLint *rects,
>>> +                            EGLint n_rects,
>>> +                            EGLBoolean with_damage)
>>>  {
>>>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
>>> @@ -876,7 +900,17 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>>>     /* If the compositor doesn't support damage_buffer, we deliberately
>>>      * ignore the damage region and post maximum damage, due to
>>>      * https://bugs.freedesktop.org/78190 */
>>> -   if (!n_rects || !try_damage_buffer(dri2_surf, rects, n_rects))
>>> +
>>> +   if (!with_damage) {
>>> +
>>> +      /* If called from swapBuffers, check if the damage region
>>> +       * is already set, if not set to full damage
>>> +       */
>>> +      if (!dri2_surf->base.SetDamageRegionCalled)
>>> +         wl_surface_damage(dri2_surf->wl_surface_wrapper,
>>> +                           0, 0, INT32_MAX, INT32_MAX);
>>> +   }
>>> +   else if (!n_rects || !try_damage_buffer(dri2_surf, rects, n_rects))
>>>        wl_surface_damage(dri2_surf->wl_surface_wrapper,
>>>                          0, 0, INT32_MAX, INT32_MAX);
>>>
>>> @@ -912,6 +946,20 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>>>     return EGL_TRUE;
>>>  }
>>>
>>> +/**
>>> + * Called via eglSwapBuffers(), drv->API.SwapBuffers().
>>> + */
>>> +static EGLBoolean
>>> +dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>>> +                                 _EGLDisplay *disp,
>>> +                                 _EGLSurface *draw,
>>> +                                 const EGLint *rects,
>>> +                                 EGLint n_rects)
>>> +{
>>> +   return dri2_wl_swap_buffers_common(drv, disp, draw,
>>> +                                      rects, n_rects, EGL_TRUE);
>> Hmm what will happen if use calls:
>>
>>   eglSetDamageRegionKHR(dpy, surf, rects, n_rects)
>>   ...
>>   eglSwapBuffersWithDamageKHR(dpy, surf, NULL, 0)
>>
>>
>> eglSwapBuffersWithDamageKHR should behave identical as eglSwapBuffers, correct?
>> If so, the proposed code seems buggy - we might as well drop the
>> explicit with_damage and derive it from rects/n_rects locally.
>>
>
> Yes, in the above case, the surface damage will be set to the full
> surface just as the spec defines it.
> Now consider this scenario:
>
>    eglSetDamageRegionKHR(dpy, surf, rects, n_rects)
>    ...
>    eglSwapBuffers(dpy, surf)
>    (from dEQP)
>
> In this case if we were the distinguish based on n_rects then we would
> end up setting the buffer damage to full because SwapBuffers would call
> SwapBuffersWithDamage with n_rects as 0 (same as the above scenario).
> This would defeat the purpose of SetDamageRegion. Thus the explicit
> with_damage variable.
> Please correct me if I have misunderstood.
>
i should have said is, as-is SetDamageRegionCalled is not checked when
SwapBuffersWithDamage(... NULL, 0)

Although, the specs do mention that "damage" refers to different
concepts - buffer vs surface. For the EGL_KHR_partial_update and
EGL_KHR_swap_buffers_with_damage respectively.
Which brings the question if and to what extend SetDamageRegionCalled
should be honoured.

Ideally we'll rework things to follow the spec better, yet I'm not
that familiar with Wayland (or Android really) for suggestion anything
:-\
In the interim, this approach should be fine with the odd comment or two.

Thanks
Emil