Hi Emil,
Emil Velikov <emil.l.veli...@gmail.com> writes:
> On 27 October 2017 at 05:54, Harish Krupo <harish.krupo....@intel.com> wrote:
>> 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.
>>
> Is my assumption correct, that things were working correctly with v2
> and it broke with v3?
>
No, this isn't the case. It still works well with v3.
> AFAICT my folding the try_damage_buffer() we introduced a bug, whereby
> when old wayland is used we won't set any damage.
> Neither the requested rect, nor the fallback "full" damage - we simply
> simply fail, thus the predicament.
>
What I was trying to say is that, if we were to return a false when fail
to set the given rects (as per Eric's suggestion, if I have understood
it correctly) then we would end up with the above issue. To summarize
the above problem: Can we return false to the user without setting an
error code?
> Personally, I'd get v2 apply the trivial changes suggested.
Okay, I can do this too (and submit the try_damage_buffer rework into
SetDamageRegion as a separate patch?)
Thank you
Regards
Harish Krupo
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
