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.
Thank you
Regards
Harish Krupo
> Thanks
> Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
