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

[Weng, Chuanbo]

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

-----Original Message-----
From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf Of Emil Velikov
Sent: Tuesday, August 30, 2016 6:29 PM
To: Weng, Chuanbo <chuanbo.weng at intel.com>
Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>
Subject: Re: [Mesa-dev] [PATCH v2] egl: return corresponding offset of EGLImage instead of 0.

On 25 August 2016 at 18:50, 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 drm bo.
>
> v2: version bump on the EGL image interface and add gallium pieces.
>
> Signed-off-by: Chuanbo Weng <chuanbo.weng at intel.com>
> ---
>  include/GL/internal/dri_interface.h      | 4 +++-
>  src/egl/drivers/dri2/egl_dri2.c          | 3 ++-
>  src/gallium/state_trackers/dri/dri2.c    | 8 +++++++-
>  src/gbm/backends/dri/gbm_dri.c           | 5 +++--
>  src/mesa/drivers/dri/i965/intel_screen.c | 9 +++++++--
I'm likely the only one here, but I'd suggest splitting things up:
- introduce flag - include/
- wire up loaders, one at a time - src/{egl,gbm}
- implement in driver, one at a time - src/{mesa,gallium}
[****Chuanbo****] I'm not sure whether I understand correctly. Do you mean that I should split this patch into
three patches as you suggested?


> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -2257,7 +2257,8 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, _EGLDisplay *disp, _EGLImage *im
>                                   __DRI_IMAGE_ATTRIB_STRIDE, strides);
>
>     if (offsets)
> -      offsets[0] = 0;
> +      dri2_dpy->image->queryImage(dri2_img->dri_image,
> +                                 __DRI_IMAGE_ATTRIB_OFFSET, offsets);
>
Grr... nope. Only overwrite the offsets[0] value if queryImage() succeeds.

>     return EGL_TRUE;
>  }
> diff --git a/src/gallium/state_trackers/dri/dri2.c 
> b/src/gallium/state_trackers/dri/dri2.c
> index 9803b0e..a7f20da 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -1004,6 +1004,12 @@ dri2_query_image(__DRIimage *image, int attrib, int *value)
>     case __DRI_IMAGE_ATTRIB_NUM_PLANES:
>        *value = 1;
>        return GL_TRUE;
> +   case __DRI_IMAGE_ATTRIB_OFFSET:
> +      /*TODO: We just set the offset to 0 here. It can be set
> +       * to corresponding value if someone has the requirement.
> +       */
> +      *value = 0;
> +      return GL_TRUE;
Either implement this properly or let the driver return false.

> @@ -1313,7 +1319,7 @@ dri2_get_capabilities(__DRIscreen *_screen)
>
>  /* The extension is modified during runtime if DRI_PRIME is detected 
> */  static __DRIimageExtension dri2ImageExtension = {
> -    .base = { __DRI_IMAGE, 12 },
> +    .base = { __DRI_IMAGE, 13 },
>
As above - either implement and bump or leave as-is. The former would be preferable, but not required.


> -   if (!dri->image || dri->image->base.version < 12) {
> +   if (!dri->image || dri->image->base.version < 12 || 
> + !dri->image->mapImage) {
Hmm we want these NULL checks split out and ported to older loader(s).
Otherwise we will get NULL deref when using older loader + newer dri module(s).
[****Chuanbo****] So we just need to leave it as original?


> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c

> +    .mapImage                           = NULL,
> +    .unmapImage                         = NULL,
Consider looking into/implementing the map/unmap interfaces. IIRC there was an implementation (slightly* different one) a while back [1].
You really want this if you're thinking for Android and/or CrOS ARC + a better gralloc [2].
[****Chuanbo****] Is it required to implement map/unmap interfaces? Since I'm not interested in Android and/or CrOS ARC + a better gralloc.
And I find that blitImage and getCapabilities are not implemented when bump verison. So I don’t think it's required to implement map/unmap
interface. 

Thanks
Emil

[1] https://lists.freedesktop.org/archives/mesa-dev/2014-April/057163.html
[2]
https://github.com/robherring/gbm_gralloc
https://lists.freedesktop.org/archives/mesa-dev/2016-May/115581.html
_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev