Harish Krupo <harish.krupo....@intel.com> writes:
> Hi Eric,
>
> Eric Engestrom <eric.engest...@imgtec.com> writes:
>
>> On Monday, 2017-10-23 16:20:54 +0530, Harish Krupo wrote:
>>> This passes 33/37 deqp tests related to partial_update, 4 are not
>>> supported. Tests 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.
>>>
>>> v2: Remove unnecessary return statement. Keep function names
>>> consistent. (Emil Velikov)
>>> Add not supported list to commit message. (Eric Engestrom)
>>>
>>> v3: Remove explicit with_damage variable. (Eric Engestrom)
>>>
>>> Signed-off-by: Harish Krupo <harish.krupo....@intel.com>
>>> ---
>>> src/egl/drivers/dri2/platform_wayland.c | 54
>>> ++++++++++++++++++++++-----------
>>> 1 file changed, 36 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/platform_wayland.c
>>> b/src/egl/drivers/dri2/platform_wayland.c
>>> index b38eb1c335..8846099d57 100644
>>> --- a/src/egl/drivers/dri2/platform_wayland.c
>>> +++ b/src/egl/drivers/dri2/platform_wayland.c
>>> @@ -790,27 +790,44 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
>>> return ret;
>>> }
>>>
>>> +/**
>>> + * Called via eglSetDamageRegionKHR(), drv->API.SetDamageRegion().
>>> + */
>>> static EGLBoolean
>>> -try_damage_buffer(struct dri2_egl_surface *dri2_surf,
>>> - const EGLint *rects,
>>> - EGLint n_rects)
>>> +dri2_wl_set_damage_region(_EGLDriver *drv,
>>> + _EGLDisplay *dpy,
>>> + _EGLSurface *surf,
>>> + const EGLint *rects,
>>> + EGLint n_rects)
>>> {
>>> - if (wl_proxy_get_version((struct wl_proxy *)
>>> dri2_surf->wl_surface_wrapper)
>>> - < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION)
>>> - return EGL_FALSE;
>>> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
>>>
>>> - for (int i = 0; i < n_rects; i++) {
>>> - const int *rect = &rects[i * 4];
>>> + /* 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 ||
>>> + wl_proxy_get_version((struct wl_proxy *)
>>> dri2_surf->wl_surface_wrapper)
>>> + < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION) {
>>> + wl_surface_damage(dri2_surf->wl_surface_wrapper,
>>> + 0, 0, INT32_MAX, INT32_MAX);
>>> + } else {
>>
>> I know Emil suggested you remove the `return` in an earlier version, but
>> if you add it back here you can drop the else, and the diff will look
>> much cleaner, keeping only the version check getting an `|| !n_rects`
>> and `return false` becoming `damage(everything)`.
>>
>> Other than that, it looks good to me. Thanks :)
>>
>
> Ok, will do that change.
> It would be something like this:
> if (!n_rects ||
> wl_proxy_get_version((struct wl_proxy *) dri2_surf->wl_surface_wrapper)
> < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION) {
> wl_surface_damage(dri2_surf->wl_surface_wrapper,
> 0, 0, INT32_MAX, INT32_MAX);
> if (!n_rects)
> return EGL_TRUE;
>
> return EGL_FALSE;
> }
>
> I have a small confusion though:
> As per spec [1]:
> * If eglSetDamageRegionKHR has already been called on <surface> since the
> most recent frame boundary, an EGL_BAD_ACCESS error is generated
>
> The "already been called" part is confusing. Should it be interpreted
> as already been called and the previous call returned a true value or it
> has already been called irrespective of the previous return value?
>
> AFAICT from deqp [2]: it expects true on the first call, false on the
> second and expects EGL_BAD_ACCESS (it follows the 2nd approach where
> irrespective of the return value, calling eglSetDamageRegionKHR twice is
> an error). But in the current implementation the SetDamageRegionCalled
> variable will be set only when we are successful in setting the damage
> with the window system. In my case I always get a false return value (I
> am testing on gnome wayland). Thus it ends up not returning
> EGL_BAD_ACCESS and the test fails.
>
> To avoid this problem in the previous patch I set the return value to
> true and set the damage region to full when version doesn't match. :)
>
> One way to fix this would be to set SetDamageRegionCalled to true
> irrespective of the return value.
>
> Is this okay? I am still trying to see if this would cause
> any problem.
>
> [1]
> https://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_partial_update.txt
> [2]
> https://android.googlesource.com/platform/external/deqp/+/master/modules/egl/teglNegativePartialUpdateTests.cpp#412
>
Gentle ping :)
(Also can we return false without setting any error? )
> Thank you
>
> Regards
> Harish krupo
>
>>> + for (int i = 0; i < n_rects; i++) {
>>> + const int *rect = &rects[i * 4];
>>>
>>> - wl_surface_damage_buffer(dri2_surf->wl_surface_wrapper,
>>> - rect[0],
>>> - dri2_surf->base.Height - rect[1] - rect[3],
>>> - rect[2], rect[3]);
>>> + wl_surface_damage_buffer(dri2_surf->wl_surface_wrapper,
>>> + rect[0],
>>> + dri2_surf->base.Height - rect[1] -
>>> rect[3],
>>> + rect[2], rect[3]);
>>> + }
>>> }
>>> +
>>> return EGL_TRUE;
>>> }
>>> +
>>> /**
>>> - * Called via eglSwapBuffers(), drv->API.SwapBuffers().
>>> + * Called via eglSwapBuffersWithDamage{KHR,EXT}(),
>>> + * drv->API.SwapBuffersWithDamageEXT().
>>> */
>>> static EGLBoolean
>>> dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>>> @@ -875,9 +892,8 @@ 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))
>>> - wl_surface_damage(dri2_surf->wl_surface_wrapper,
>>> - 0, 0, INT32_MAX, INT32_MAX);
>>> + if (n_rects || !dri2_surf->base.SetDamageRegionCalled)
>>> + dri2_wl_set_damage_region(drv, disp, draw, rects, n_rects);
>>>
>>> if (dri2_dpy->is_different_gpu) {
>>> _EGLContext *ctx = _eglGetCurrentContext();
>>> @@ -928,7 +944,8 @@ dri2_wl_query_buffer_age(_EGLDriver *drv,
>>> static EGLBoolean
>>> dri2_wl_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
>>> {
>>> - return dri2_wl_swap_buffers_with_damage(drv, disp, draw, NULL, 0);
>>> + return dri2_wl_swap_buffers_with_damage(drv, disp, draw,
>>> + NULL, 0);
>>> }
>>>
>>> static struct wl_buffer *
>>> @@ -1166,7 +1183,7 @@ static const struct dri2_egl_display_vtbl
>>> dri2_wl_display_vtbl = {
>>> .swap_buffers = dri2_wl_swap_buffers,
>>> .swap_buffers_with_damage = dri2_wl_swap_buffers_with_damage,
>>> .swap_buffers_region = dri2_fallback_swap_buffers_region,
>>> - .set_damage_region = dri2_fallback_set_damage_region,
>>> + .set_damage_region = dri2_wl_set_damage_region,
>>> .post_sub_buffer = dri2_fallback_post_sub_buffer,
>>> .copy_buffers = dri2_fallback_copy_buffers,
>>> .query_buffer_age = dri2_wl_query_buffer_age,
>>> @@ -1378,6 +1395,7 @@ dri2_initialize_wayland_drm(_EGLDriver *drv,
>>> _EGLDisplay *disp)
>>> disp->Extensions.EXT_buffer_age = EGL_TRUE;
>>>
>>> disp->Extensions.EXT_swap_buffers_with_damage = EGL_TRUE;
>>> + disp->Extensions.KHR_partial_update = EGL_TRUE;
>>>
>>> /* Fill vtbl last to prevent accidentally calling virtual function
>>> during
>>> * initialization.
>>> --
>>> 2.14.2
>>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
