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....@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....@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
