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

[Eric Engestrom]

On 17 October 2017 17:36:06 BST, 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 might need to double check the spec, but I thought "no damage hint"
meant "no way to reduce the update, redraw the whole screen"?

If that's the case, then SwapBuffers() and SwapBuffersWithDamages(NULL, 0)
should have the same effect: not setting any damage region.

You can therefore drop the `with_damage` bool and just use `n_rects > 0`.

> 
> Thank you
> 
> Regards
> Harish Krupo
> 
> > Thanks
> > Emil
> 
(sorry for the formatting, replying from my phone)