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

[Harish Krupo]

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.

Thank you

Regards
Harish Krupo

> Thanks
> Emil