On 17 October 2017 17:36:06 BST, Harish Krupo <harish.krupo....@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.veli...@gmail.com> writes:
>
> > Hi Harish,
> >
> > Overall looks great, a few comments/questions inline.
> >
> > 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.
> >>
> >> 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.
> >
>
> 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)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
