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

[Eric Engestrom]

On Friday, 2017-10-20 11:35:40 +0000, Harish Krupo wrote:
> This passes 33/37 deqp tests related to partial_update, 4 are not
> supported. Tests not supported:
> dEQP-EGL.functional.negative_partial_update.not_postable_surface
> dEQP-EGL.functional.negative_partial_update.not_current_surface
> dEQP-EGL.functional.negative_partial_update.buffer_preserved
> dEQP-EGL.functional.negative_partial_update.not_current_surface2
> Reason: No matching egl config found.
> 
> v2: Remove unnecessary return statement. Keep function names
>     consistent.  (Emil Velikov)
>     Add not supported list to commit message. (Eric Engestrom)
> 
> Signed-off-by: Harish Krupo <harish.krupo.kps at intel.com>
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 67 ++++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index b38eb1c335..daa148380b 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -809,15 +809,38 @@ 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)
> +dri2_wl_set_damage_region(_EGLDriver *drv,
> +                     _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;
> +}
> +
> +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);
> @@ -875,7 +898,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);

Per spec [1]:
> If <n_rects> is 0 then <rects> is ignored and the entire
> surface is implicitly damaged and the behaviour is equivalent
> to calling eglSwapBuffers.

I interpret this as "the entire surface is implicitly damaged" being
overridden by "the behaviour is equivalent to calling eglSwapBuffers",
as the former was just a reiteration of the latter before
EGL_KHR_partial_update.
In other words: SwapBuffers() == SwapBuffersWithDamage(NULL, 0)

One could argue that the "the entire surface is implicitly damaged" is
more important, in which case the behaviour counter-intuitive IMO:

SetDamage(rect)
SwapBuffersWithDamage(NULL, 0)

would result in the whole screen being redrawn, which doesn't sound like
what the client wanted.


Since SwapBuffers() works by calling SwapBuffersWithDamage(NULL, 0),
`with_damage == !n_rects` and can be dropped.

Your dri2_wl_set_damage_region() correctly handles the `!n_rects` case
already, so the whole above `if {} else if {}` block can be replaced
with:

   if (n_rects != 0 || !dri2_surf->base.SetDamageRegionCalled)
      dri2_wl_set_damage_region(drv, dpy, surf, rects, n_rects);

[1] https://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt

>  
> @@ -911,6 +944,20 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>     return EGL_TRUE;
>  }
>  
> +/**
> + * Called via eglSwapBuffers(), drv->API.SwapBuffers().

This was already wrong before, you only moved it, but you might as well
fix it :)
* Called via eglSwapBuffersWithDamage{KHR,EXT}(), drv->API.SwapBuffersWithDamageEXT().

> + */
> +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);
> +}
> +
>  static EGLint
>  dri2_wl_query_buffer_age(_EGLDriver *drv,
>                           _EGLDisplay *disp, _EGLSurface *surface)
> @@ -928,7 +975,8 @@ dri2_wl_query_buffer_age(_EGLDriver *drv,
>  static EGLBoolean
>  dri2_wl_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
>  {
> -   return dri2_wl_swap_buffers_with_damage(drv, disp, draw, NULL, 0);
> +   return dri2_wl_swap_buffers_common(drv, disp, draw,
> +                                      NULL, 0, EGL_FALSE);
>  }
>  
>  static struct wl_buffer *
> @@ -1166,7 +1214,7 @@ static const struct dri2_egl_display_vtbl dri2_wl_display_vtbl = {
>     .swap_buffers = dri2_wl_swap_buffers,
>     .swap_buffers_with_damage = dri2_wl_swap_buffers_with_damage,
>     .swap_buffers_region = dri2_fallback_swap_buffers_region,
> -   .set_damage_region = dri2_fallback_set_damage_region,
> +   .set_damage_region = dri2_wl_set_damage_region,
>     .post_sub_buffer = dri2_fallback_post_sub_buffer,
>     .copy_buffers = dri2_fallback_copy_buffers,
>     .query_buffer_age = dri2_wl_query_buffer_age,
> @@ -1378,6 +1426,7 @@ dri2_initialize_wayland_drm(_EGLDriver *drv, _EGLDisplay *disp)
>     disp->Extensions.EXT_buffer_age = EGL_TRUE;
>  
>     disp->Extensions.EXT_swap_buffers_with_damage = EGL_TRUE;
> +   disp->Extensions.KHR_partial_update = EGL_TRUE;
>  
>     /* Fill vtbl last to prevent accidentally calling virtual function during
>      * initialization.
> -- 
> 2.14.2
>