Hi Emil,
Thanks for your review!
Please see my comments and questions below.
-----Original Message-----
From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of
Emil Velikov
Sent: Tuesday, August 30, 2016 6:29 PM
To: Weng, Chuanbo <chuanbo.w...@intel.com>
Cc: ML mesa-dev <mesa-dev@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.w...@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.w...@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
