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

[Emil Velikov]

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}


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


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

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