[Mesa-dev] [PATCH v3] egl/wayland: Support for KHR_partial_update

[Emil Velikov]

On 27 October 2017 at 05:54, Harish Krupo <harish.krupo.kps at intel.com> wrote:
> 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.
>
Is my assumption correct, that things were working correctly with v2
and it broke 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.

Personally, I'd get v2 apply the trivial changes suggested.