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

[Tomasz Figa]

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.

>  ....
>
>
> 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...)

Best regards,
Tomasz