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

[Emil Velikov]

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.

Thanks
Emil