On 8 November 2017 at 22:10, Harish Krupo <harish.krupo....@intel.com> wrote:
> 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?
>
I should have made it clearer - my description is about a bug
introduced with v3.
You're right on the should we return true/false... topic. I'd stick
with the current behaviour - try partial and fallback to full damage
returning true.
One could propagate the set_full_damage failure, but I'd keep that
separate patch.
>> 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?)
>
I don't see much value in the folding try_damage_buffer and
dri2_wl_set_damage_region, but if you prefer sure thing.
Please move the respective comment blocks alongside the code changes.
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
