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

[Eric Engestrom]

On Monday, 2017-10-16 13:54:25 +0000, Emil Velikov wrote:
> Hi Harish,
> 
> Overall looks great, a few comments/questions inline.
> 

I agree with everything Emil said :)

> 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.

Which 4 are not supported? It's definitely not obvious :)
I'm guessing dEQP-EGL.functional.negative_partial_update.not_current_surface2
isn't supported (pbuffer), but I can't tell which other ones would be
unsupported.

> >
> > 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