[Mesa-dev] [PATCH] EGL/android: Enhance pbuffer implementation

[Emil Velikov]

On 28 November 2016 at 17:46, Tomasz Figa <tfiga at chromium.org> wrote:
> On Mon, Nov 28, 2016 at 10:35 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 25 November 2016 at 03:58, Liu Zhiquan <zhiquan.liu at intel.com> wrote:
>>> Some dri drivers will pass multiple bits in buffer_mask parameter
>>> to droid_image_get_buffer(), more than the actual supported buffer
>>> type combination. For such case, will go through all the bits, and
>>> will not return error when unsupported buffer is requested, only
>>> return error when the allocation for supported buffer failed.
>>>
>>> Signed-off-by: Liu Zhiquan <zhiquan.liu at intel.com>
>>> Signed-off-by: Long, Zhifang <zhifang.long at intel.com>
>>> ---
>>>  src/egl/drivers/dri2/platform_android.c | 209 +++++++++++++++++++-------------
>>>  1 file changed, 126 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
>>> index 373e2c0..c70423d 100644
>>> --- a/src/egl/drivers/dri2/platform_android.c
>>> +++ b/src/egl/drivers/dri2/platform_android.c
>>> @@ -392,13 +392,13 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>>>     }
>>>
>>>     if (dri2_surf->dri_image_back) {
>>> -      _eglLog(_EGL_DEBUG, "%s : %d : destroy dri_image_back", __func__, __LINE__);
>>> +      _eglLog(_EGL_DEBUG, "destroy dri_image_back");
>>>        dri2_dpy->image->destroyImage(dri2_surf->dri_image_back);
>>>        dri2_surf->dri_image_back = NULL;
>>>     }
>>>
>>>     if (dri2_surf->dri_image_front) {
>>> -      _eglLog(_EGL_DEBUG, "%s : %d : destroy dri_image_front", __func__, __LINE__);
>>> +      _eglLog(_EGL_DEBUG, "destroy dri_image_front");
>>>        dri2_dpy->image->destroyImage(dri2_surf->dri_image_front);
>>>        dri2_surf->dri_image_front = NULL;
>>>     }
>> Unrelated changes ?
>>
>>
>>> @@ -434,81 +434,98 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
>>>  }
>>>
>>>  static int
>>> -get_back_bo(struct dri2_egl_surface *dri2_surf)
>>> +get_front_bo(struct dri2_egl_surface *dri2_surf, unsigned int format)
>>>  {
>> While I see Tomasz concern I'm wondering if we cannot make this
>>
>> __DRIimage *
>> get_bo(..., uint32_t buffer_mask)
>> {
>> ...
>> }
>>
>
> Hmm, could you point me to the common code we would get by keeping
> these two functions together? From what I see, both cases have
> significantly different handling and that's why I asked for splitting
> them. It's visible clearly in my original patch which ended up with
> much less code than the one that was merged (and this one is trying to
> fix).
>
>>
>> droid_image_get_buffers(...)
>> {
>>    struct dri2_egl_surface *dri2_surf = loaderPrivate;
>>
>>    images->image_mask = 0;
>>    images->front = NULL;
>>    images->back = NULL;
>>
>>    if (update_buffers(dri2_surf) < 0)
>>       return 0;
>>
>>    if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
>>       images->front = get_front_bo(... buffer_mask);
>>       if (images->front < 0) {
>>          _eglError(EGL_BAD_PARAMETER, "droid_get_front_bo");
>>          return 0;
>>      }
>>     ...
>>    }
>>    if (buffer_mask & __DRI_IMAGE_BUFFER_BACK) {
>>       images->back = get_front_bo(... buffer_mask);
>>       if (images->back < 0)
>
> This code is actually where the duplication is (doing the same for
> each of the bits but dispatching different allocation/dequeue code),
> but we can't do much since "images" is not an array, but a struct.
>
Having a closer look - something have gone short circuited on my end.
Please ignore my comments above.

>>  ....
>>
>>
>> This way things would be reasonably simple and reusable.
>> Hit I'm thinking that we want to unify much/most of the platform_foo.c
>> code... one of these days.
>
> Hmm, pbuffer handling should be mostly the same for all (I'm guessing
> that X11 might be doing some funky stuff, though...). I was wondering
> why we even need to have any pbuffer code in platform backends.
> (Pixmaps and windows are a different, completely platform-specific
> thing, though...)
>
Roughly what I'm wondering as well ;-)

Thanks
Emil