[Mesa-dev] [PATCH v4] egl/android: support for EGL_KHR_partial_update

[Harish Krupo]

On 06/08/2017 07:23 PM, Eric Engestrom wrote:
> On Thursday, 2017-06-08 17:36:47 +0530, Harish Krupo wrote:
>> Hi Tapani,
>>
>> On 06/08/2017 04:32 PM, Tapani Pälli wrote:
>>> Harish, please find small addition below.
>>>
>>> I noticed we still have one crasher in tests when this extension is
>>> enabled, it's related to buffer age so I've sent a fix separately here:
>>>
>>> https://lists.freedesktop.org/archives/mesa-dev/2017-June/158391.html
>>>
>>> On 06/07/2017 07:11 PM, Harish Krupo wrote:
>>>> diff --git a/src/egl/drivers/dri2/platform_android.c
>>>> b/src/egl/drivers/dri2/platform_android.c
>>>> index 055dc0f996..303e19052f 100644
>>>> --- a/src/egl/drivers/dri2/platform_android.c
>>>> +++ b/src/egl/drivers/dri2/platform_android.c
>>>> @@ -656,6 +656,39 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay
>>>> *disp, _EGLSurface *draw)
>>>>       return EGL_TRUE;
>>>>    }
>>>> +static EGLBoolean
>>>> +droid_set_damage_region(_EGLDriver *drv,
>>>> +                        _EGLDisplay *disp,
>>>> +                        _EGLSurface *draw, const EGLint* rects,
>>>> EGLint n_rects)
>>>> +{
>>>> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>>> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
>>>> +   android_native_rect_t* droid_rects = NULL;
>>>> +
>>>> +   if (n_rects == 0)
>>>> +      return EGL_TRUE;
>>>> +
>>>> +   droid_rects = calloc(n_rects, sizeof(android_native_rect_t));
> 
> no need to zero it out it you're going to set all the fields below.
> 

I will replace it with malloc and also throw a BAD_ALLOC error if this 
fails (as suggested by Emil).

>>>> +   EGLint num_drects;
> 
> nit: either move this into the `for()` (we require C99 support, which
> includes this) or at the top of the block.
> 
>>>> +
>>>> +   for (num_drects = 0; num_drects < n_rects; num_drects++) {
>>>> +      EGLint i = num_drects * 4;
>>>> +      droid_rects[num_drects].left = rects[i];
>>>> +      droid_rects[num_drects].bottom = rects[i + 1];
>>>> +      droid_rects[num_drects].right = rects[i] + rects[i + 2];
>>>> +      droid_rects[num_drects].top = rects[i + 1] + rects[i + 3];
>>>> +   }
>>>> +
>>>> +   /*
>>>> +    * XXX/TODO: Need to check the return value of this function.
>>>> +    */
>>>
>>> Why not just do it now then, return EGL_FALSE if return value != 0, I
>>> don't think anything else is required here.
>>>
>>
>>
>> Wasn't sure about the return values of the perform method. Window.h
>> documents only -ENOENT. Sure, I will make this change.
>> I will send the patch immediately after the fix for buffer age is merged.
>>
> 
> Indeed, the only documented [1] return value is -ENOENT:
>> returns -ENOENT if [native_window_set_surface_damage] is not supported
>> by the surface's implementation.
> 
> Since droid_set_damage_region() can only return an EGLBoolean anyway,
> I think a simple `err == 0` is all you need here:
> 
>     int err;
>     /* ... */
>     err = native_window_set_surface_damage(dri2_surf->window, droid_rects, n_rects);
>     free(droid_rects);
>     return err == 0 ? EGL_TRUE : EGL_FALSE;
> 

OK, I will do this.

> With these, you can add my:
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
> 

Thanks, will add it.

> BTW, did you run dEQP-EGL.functional.partial_update.* ?
> What are the results?
> 

The test results in the commit message (36/37 passed) refer to those 
deqp test cases which match the regex *partial_update*. This includes 
all the tests with dEQP-EGL.functional.partial_update.* and also the 
negative tests dEQP-EGL.functional.negative_partial_update.*

> Cheers,
>    Eric
> 
> [1] https://android.googlesource.com/platform/system/core/+/master/libsystem/include/system/window.h#530
>