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

[Harish Krupo]

Eric Engestrom <eric.engestrom at imgtec.com> writes:

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

Sorry, should have posted it.
Here are the ones 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.

I will update the commit in v2.

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