[Mesa-dev] [PATCH] egl/wayland: Support for KHR_partial_update
Harish Krupo
harish.krupo.kps at intel.com
Wed Oct 18 06:06:45 UTC 2017
Hi Eric,
Eric Engestrom <eric at engestrom.ch> writes:
> 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`.
>
Quoting from the swap buffers with damage spec:
If <n_rects> is 0 then <rects> is ignored and the entire
surface is implicitly damaged and the behaviour is equivalent
to calling eglSwapBuffers.
If we were to use n_rects, then the code would be something like this:
if (n_rects > 0) { // with_damage case
if (!try_damage_buffer(dri2_surf, rects, n_rects))
wl_surface_damage(dri2_surf->wl_surface_wrapper,
0, 0, INT32_MAX, INT32_MAX);
}
else {
// if (!dri2_surf->base.SetDamageRegionCalled)
wl_surface_damage(dri2_surf->wl_surface_wrapper,
0, 0, INT32_MAX, INT32_MAX);
}
The problem here is when n_rects is zero, then according the
swap_buffers spec the damage should be set to maximum which it will.
This code will also be executed by SwapBuffers. In this case the maximum
damage should be set only when no previous damage has already been set by
SetDamageRegion otherwise the previous SetDamageRegion call would be in
vain. (Also wayland requires that some buffer damage is set otherwise
there will be no damage at all.)
Thank you
Regards
Harish Krupo
>>
>> Thank you
>>
>> Regards
>> Harish Krupo
>>
>> > Thanks
>> > Emil
>>
> (sorry for the formatting, replying from my phone)
More information about the mesa-dev
mailing list