[Mesa-dev] [PATCH v3] egl/wayland: Support for KHR_partial_update
Harish Krupo
harish.krupo.kps at intel.com
Fri Oct 27 04:54:23 UTC 2017
Hi Eric,
Eric Engestrom <eric.engestrom at 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.kps at 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
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
>>
More information about the mesa-dev
mailing list