[Mesa-dev] [PATCH v2 2/3] egl: return corresponding offset of EGLImage instead of 0.

[Axel Davy]

I'm not sure calling queryImage with an unsupported attribute is legal, 
thus I think a small check doesn't hurt.

It'd give

if (offsets) {
    offsets[0] = 0;
    if (dri2_dpy->image->base.version >= 13) {
       EGLint img_offset = 0;
       bool ret = dri2_dpy->image->queryImage(dri2_img->dri_image,
                                      __DRI_IMAGE_ATTRIB_OFFSET, 
&img_offset);
       if (ret)
          offsets[0] = img_offset;
    }
}
return EGL_TRUE;

or alternatively, if you think not being able to give the offset 
indicates an error,

if (offsets) {
    offsets[0] = 0;
    if (dri2_dpy->image->base.version >= 13) {
       EGLint img_offset = 0;
       bool ret = dri2_dpy->image->queryImage(dri2_img->dri_image,
                                      __DRI_IMAGE_ATTRIB_OFFSET, 
&img_offset);
       if (!ret)
          return EGL_FALSE;
       offsets[0] = img_offset;
    }
}
return EGL_TRUE;

Axel Davy

On 09/09/2016 17:34, Weng, Chuanbo wrote:
> The comments from Emil can be summarized into the following code:
>
>     if (offsets) {
>        offsets[0] = 0;
>        EGLint img_offset = 0;
>        bool ret = dri2_dpy->image->queryImage(dri2_img->dri_image,
>                     __DRI_IMAGE_ATTRIB_OFFSET, &img_offset);
>        if(ret == true)
>           offsets[0] = img_offset;
>     }
>
>     return EGL_TRUE;
>
> Emil, is it right?
>
> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: Friday, September 9, 2016 8:32 PM
> To: Weng, Chuanbo <chuanbo.weng at intel.com>
> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>; Axel Davy <axel.davy at ens.fr>
> Subject: Re: [PATCH v2 2/3] egl: return corresponding offset of EGLImage instead of 0.
>
> On 9 September 2016 at 08:58, Chuanbo Weng <chuanbo.weng at intel.com> wrote:
>> The offset should not always be 0. For example, if EGLImage is created
>> from a 2D texture with EGL_GL_TEXTURE_LEVEL=1, then the offset should
>> be the actual start of miplevel 1 in bo.
>>
>> v2: Add version check of __DRIimageExtension implementation (Suggested
>> by Axel Davy).
>>
> Just to elaborate what Axel was saying from another perspective:
>
> With current master the offset is explicitly zeroed, while with the
> (v2) patch you'll _only_ set the value when you have v13 driver.
> Thus using the loader + v12 driver you'll get a regression since a) the offset will remain garbage and b) the function
> (dri2_export_dma_buf_image_mesa) will fail.
>
>
>> Signed-off-by: Chuanbo Weng <chuanbo.weng at intel.com>
>> ---
>>   src/egl/drivers/dri2/egl_dri2.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c
>> b/src/egl/drivers/dri2/egl_dri2.c index 859612f..c529e55 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -2249,6 +2249,7 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, _EGLDisplay *disp, _EGLImage *im
>>      struct dri2_egl_image *dri2_img = dri2_egl_image(img);
>>
>>      (void) drv;
>> +   EGLint img_offset = 0;
>>
>>      /* rework later to provide multiple fds/strides/offsets */
>>      if (fds)
>> @@ -2259,8 +2260,14 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, _EGLDisplay *disp, _EGLImage *im
>>         dri2_dpy->image->queryImage(dri2_img->dri_image,
>>                                    __DRI_IMAGE_ATTRIB_STRIDE, strides);
>>
>> -   if (offsets)
>> +   if (offsets){
>>         offsets[0] = 0;
>> +      if(dri2_dpy->image->base.version >= 13){
> Please move the variable declaration in local scope and add space between ){
>
> Functionality wise we don't need the version check, esp. since you want to set the offset only when queryImage() succeeds...
>
>> +         dri2_dpy->image->queryImage(dri2_img->dri_image,
>> +             __DRI_IMAGE_ATTRIB_OFFSET, &img_offset);
> ... which you don't seem to be checking here, so you'll effectively store/use garbage.
>
> So unless Alex feels strongly for the version check I'd omit it, and fix the rest of this patch.
>
> -Emil