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

[Weng, Chuanbo]

From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of 
Axel Davy
Sent: Friday, September 9, 2016 2:12 PM
To: Weng, Chuanbo <chuanbo.w...@intel.com>; mesa-dev@lists.freedesktop.org; 
emil.l.veli...@gmail.com
Subject: Re: [Mesa-dev] [PATCH 2/3] egl: return corresponding offset of 
EGLImage instead of 0.






That doesn't seem good to me.



With that patch, that means that since no one is implementing



__DRI_IMAGE_ATTRIB_OFFSET



(yes I know in a later patch you implement it for i965),



then what used to work will stop working (as the queryImage will return false).



You need to introduce some interface version implementation check.

[Chuanbo] Maybe I can add more comment to git log (such as "This patch 
just implements egl loader side, the driver side

implementation is also needed for corresponding platform"), so user can be 
aware of this.

Introduce interface version implementation check will make mesa code more 
complex, because we should also add related check to

other dri2 functions(dri2_).

Another solution is combining the three patches into one patch, as I did before:

https://lists.freedesktop.org/archives/mesa-dev/2016-August/126945.html

This is not as easy as this version for reviewers, but more clearer for users.

Emil, what do you think?



No, that's not ok.

First i965 isn't the only one to implement the dri image interface (see the 
gallium one), second a new implementer doesn't have to start from the most 
recent version, and can choose to implement older version, which wouldn't 
implement your new functionnality.

[Chuanbo] OK. I’ll send out a new version based on your comments. 
Thanks.



The code has to be something like:

if (offsets) {

   offsets[0] = 0;

   if (dri2_dpy->image->base.version >= 13)

dri2_dpy->image->queryImage(dri2_img->dri_image, __DRI_IMAGE_ATTRIB_OFFSET, 
offsets);

}

Axel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Axel Davy]

That doesn't seem good to me.

With that patch, that means that since no one is implementing

__DRI_IMAGE_ATTRIB_OFFSET

(yes I know in a later patch you implement it for i965),

then what used to work will stop working (as the queryImage will return false).

You need to introduce some interface version implementation check.
[Chuanbo] Maybe I can add more comment to git log (such as "This patch 
just implements egl loader side, the driver side
implementation is also needed for corresponding platform"), so user can be 
aware of this.
Introduce interface version implementation check will make mesa code more 
complex, because we should also add related check to
other dri2 functions(dri2_).
Another solution is combining the three patches into one patch, as I did before:
https://lists.freedesktop.org/archives/mesa-dev/2016-August/126945.html
This is not as easy as this version for reviewers, but more clearer for users.
Emil, what do you think?


No, that's not ok.

First i965 isn't the only one to implement the dri image interface (see 
the gallium one), second a new implementer doesn't have to start from 
the most recent version, and can choose to implement older version, 
which wouldn't implement your new functionnality.



The code has to be something like:

if (offsets) {

   offsets[0] = 0;

   if (dri2_dpy->image->base.version >= 13)

dri2_dpy->image->queryImage(dri2_img->dri_image, 
__DRI_IMAGE_ATTRIB_OFFSET, offsets);


}

Axel

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Weng, Chuanbo]

Hi Axel and Emil,
Thanks Axel for your review. Please see my comments below.

-Original Message-
From: Axel Davy [mailto:axel.d...@ens.fr] 
Sent: Friday, September 9, 2016 1:25 AM
To: Weng, Chuanbo <chuanbo.w...@intel.com>; mesa-dev@lists.freedesktop.org; 
emil.l.veli...@gmail.com
Subject: Re: [Mesa-dev] [PATCH 2/3] egl: return corresponding offset of 
EGLImage instead of 0.

Hi,

That doesn't seem good to me.

With that patch, that means that since no one is implementing

__DRI_IMAGE_ATTRIB_OFFSET

(yes I know in a later patch you implement it for i965),

then what used to work will stop working (as the queryImage will return false).

You need to introduce some interface version implementation check.
[Chuanbo] Maybe I can add more comment to git log (such as "This patch 
just implements egl loader side, the driver side 
implementation is also needed for corresponding platform"), so user can be 
aware of this.
Introduce interface version implementation check will make mesa code more 
complex, because we should also add related check to
other dri2 functions(dri2_).
Another solution is combining the three patches into one patch, as I did before:
https://lists.freedesktop.org/archives/mesa-dev/2016-August/126945.html
This is not as easy as this version for reviewers, but more clearer for users.
Emil, what do you think?


(another nitpick: queryImage return true or false, not EGL_TRUE/EGL_FALSE. It's 
better to convert, instead of assuming they're the same)
[Chuanbo] Agree.

Yours,

Axel Davy

On 06/09/2016 11:06, Chuanbo Weng 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.
>
> Signed-off-by: Chuanbo Weng <chuanbo.w...@intel.com>
> ---
>   src/egl/drivers/dri2/egl_dri2.c | 12 +---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
> b/src/egl/drivers/dri2/egl_dri2.c index 859612f..8ef0acd 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -2249,6 +2249,8 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, 
> _EGLDisplay *disp, _EGLImage *im
>  struct dri2_egl_image *dri2_img = dri2_egl_image(img);
>   
>  (void) drv;
> +   EGLBoolean ret = EGL_TRUE;
> +   EGLint img_offset = 0;
>   
>  /* rework later to provide multiple fds/strides/offsets */
>  if (fds)
> @@ -2259,10 +2261,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)
> -  offsets[0] = 0;
> +   if (offsets){
> +  ret = dri2_dpy->image->queryImage(dri2_img->dri_image,
> + __DRI_IMAGE_ATTRIB_OFFSET, _offset);
> +  if(ret == EGL_TRUE)
> +offsets[0] = img_offset;
> +   }
>   
> -   return EGL_TRUE;
> +   return ret;
>   }
>   
>   #endif


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Axel Davy]

Hi,

That doesn't seem good to me.

With that patch, that means that since no one is implementing

__DRI_IMAGE_ATTRIB_OFFSET

(yes I know in a later patch you implement it for i965),

then what used to work will stop working (as the queryImage will return 
false).


You need to introduce some interface version implementation check.


(another nitpick: queryImage return true or false, not 
EGL_TRUE/EGL_FALSE. It's better to convert, instead of assuming they're 
the same)


Yours,

Axel Davy

On 06/09/2016 11:06, Chuanbo Weng 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.

Signed-off-by: Chuanbo Weng 
---
  src/egl/drivers/dri2/egl_dri2.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 859612f..8ef0acd 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2249,6 +2249,8 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, 
_EGLDisplay *disp, _EGLImage *im
 struct dri2_egl_image *dri2_img = dri2_egl_image(img);
  
 (void) drv;

+   EGLBoolean ret = EGL_TRUE;
+   EGLint img_offset = 0;
  
 /* rework later to provide multiple fds/strides/offsets */

 if (fds)
@@ -2259,10 +2261,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)

-  offsets[0] = 0;
+   if (offsets){
+  ret = dri2_dpy->image->queryImage(dri2_img->dri_image,
+   __DRI_IMAGE_ATTRIB_OFFSET, _offset);
+  if(ret == EGL_TRUE)
+offsets[0] = img_offset;
+   }
  
-   return EGL_TRUE;

+   return ret;
  }
  
  #endif



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Chuanbo Weng]

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.

Signed-off-by: Chuanbo Weng 
---
 src/egl/drivers/dri2/egl_dri2.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 859612f..8ef0acd 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2249,6 +2249,8 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, 
_EGLDisplay *disp, _EGLImage *im
struct dri2_egl_image *dri2_img = dri2_egl_image(img);
 
(void) drv;
+   EGLBoolean ret = EGL_TRUE;
+   EGLint img_offset = 0;
 
/* rework later to provide multiple fds/strides/offsets */
if (fds)
@@ -2259,10 +2261,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)
-  offsets[0] = 0;
+   if (offsets){
+  ret = dri2_dpy->image->queryImage(dri2_img->dri_image,
+   __DRI_IMAGE_ATTRIB_OFFSET, _offset);
+  if(ret == EGL_TRUE)
+offsets[0] = img_offset;
+   }
 
-   return EGL_TRUE;
+   return ret;
 }
 
 #endif
-- 
1.9.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev