[Mesa-dev] [PATCH v13 04/36] egl/dri2: Create EGLImages with dmabuf modifiers
Jason Ekstrand
jason at jlekstrand.net
Fri May 19 18:08:48 UTC 2017
On Fri, May 19, 2017 at 2:37 AM, Daniel Stone <daniels at collabora.com> wrote:
> From: Varad Gautam <varad.gautam at collabora.com>
>
> Allow creating EGLImages with dmabuf format modifiers when target is
> EGL_LINUX_DMA_BUF_EXT for EGL_EXT_image_dma_buf_import_modifiers.
>
> v2:
> - clear modifier assembling and error label name (Eric Engestrom)
> v3:
> - remove goto jumps within switch-case (Emil Velikov)
> - treat zero as valid modifier (Daniel Stone)
> - ensure same modifier across all dmabuf planes (Emil Velikov)
> v4:
> - allow modifiers to add extra planes (Louis-Francis Ratté-Boulianne)
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
> Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb at collabora.com>
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
> Reviewed-by: Daniel Stone <daniels at collabora.com>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> src/egl/drivers/dri2/egl_dri2.c | 95 ++++++++++++++++++++++++++++++
> ++++++-----
> src/egl/main/eglimage.c | 48 +++++++++++++++++++++
> src/egl/main/eglimage.h | 2 +
> 3 files changed, 134 insertions(+), 11 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_
> dri2.c
> index ceffd281d5..f06b5535c7 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1949,6 +1949,33 @@ dri2_check_dma_buf_attribs(const _EGLImageAttribs
> *attrs)
> }
> }
>
> + /**
> + * If <target> is EGL_LINUX_DMA_BUF_EXT, both or neither of the
> following
> + * attribute values may be given.
> + *
> + * This is referring to EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT and
> + * EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT, and the same for other planes.
> + */
> + for (i = 0; i < DMA_BUF_MAX_PLANES; ++i) {
> + if (attrs->DMABufPlaneModifiersLo[i].IsPresent !=
> + attrs->DMABufPlaneModifiersHi[i].IsPresent) {
> + _eglError(EGL_BAD_PARAMETER, "modifier attribute lo or hi
> missing");
> + return EGL_FALSE;
> + }
> + }
> +
> + for (i = 0; i < DMA_BUF_MAX_PLANES; ++i) {
>
This loop could start at 1 instead of 0. Also, you're checking that they
all match but you aren't checking that they're all present if the 0th is
present. This also means that if 0 isn't present but 1 is, then you'll be
checking 1 against uninitialized data in 0. I think we want to fail if
[0].IsPresent != [i].IsPresent.
> + if (attrs->DMABufPlaneModifiersLo[i].IsPresent) {
> + if ((attrs->DMABufPlaneModifiersLo[0].Value !=
> + attrs->DMABufPlaneModifiersLo[i].Value) ||
> + (attrs->DMABufPlaneModifiersHi[0].Value !=
> + attrs->DMABufPlaneModifiersHi[i].Value)) {
> + _eglError(EGL_BAD_PARAMETER, "modifier attributes not equal");
> + return EGL_FALSE;
> + }
> + }
> + }
> +
> return EGL_TRUE;
> }
>
> @@ -2057,7 +2084,25 @@ dri2_check_dma_buf_format(const _EGLImageAttribs
> *attrs)
> for (i = plane_n; i < DMA_BUF_MAX_PLANES; ++i) {
> if (attrs->DMABufPlaneFds[i].IsPresent ||
> attrs->DMABufPlaneOffsets[i].IsPresent ||
> - attrs->DMABufPlanePitches[i].IsPresent) {
> + attrs->DMABufPlanePitches[i].IsPresent ||
> + attrs->DMABufPlaneModifiersLo[i].IsPresent ||
> + attrs->DMABufPlaneModifiersHi[i].IsPresent) {
> +
> + /**
> + * The modifiers extension spec says:
> + *
> + * "Modifiers may modify any attribute of a buffer import,
> including
> + * but not limited to adding extra planes to a format which
> + * otherwise does not have those planes. As an example, a
> modifier
> + * may add a plane for an external compression buffer to a
> + * single-plane format. The exact meaning and effect of any
> + * modifier is canonically defined by drm_fourcc.h, not as part
> of
> + * this extension."
> + */
> + if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
> + attrs->DMABufPlaneModifiersHi[i].IsPresent)
> + continue;
> +
> _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");
> return 0;
> }
> @@ -2090,6 +2135,8 @@ dri2_create_image_dma_buf(_EGLDisplay *disp,
> _EGLContext *ctx,
> int fds[DMA_BUF_MAX_PLANES];
> int pitches[DMA_BUF_MAX_PLANES];
> int offsets[DMA_BUF_MAX_PLANES];
> + uint64_t modifiers[DMA_BUF_MAX_PLANES];
> + bool has_modifier = false;
> unsigned error;
>
> /**
> @@ -2120,18 +2167,44 @@ dri2_create_image_dma_buf(_EGLDisplay *disp,
> _EGLContext *ctx,
> fds[i] = attrs.DMABufPlaneFds[i].Value;
> pitches[i] = attrs.DMABufPlanePitches[i].Value;
> offsets[i] = attrs.DMABufPlaneOffsets[i].Value;
> + if (attrs.DMABufPlaneModifiersLo[i].IsPresent) {
> + modifiers[i] =
> + ((uint64_t) attrs.DMABufPlaneModifiersHi[i].Value << 32) |
> + attrs.DMABufPlaneModifiersLo[i].Value;
> + has_modifier = true;
> + } else {
> + modifiers[i] = 0;
>
Do we really want 0 here? How about INVALID? Not that it matters since
they're all required to be the same...
> + }
> }
>
> - dri_image =
> - dri2_dpy->image->createImageFromDmaBufs(dri2_dpy->dri_screen,
> - attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
> - fds, num_fds, pitches, offsets,
> - attrs.DMABufYuvColorSpaceHint.Value,
> - attrs.DMABufSampleRangeHint.Value,
> - attrs.DMABufChromaHorizontalSiting.Value,
> - attrs.DMABufChromaVerticalSiting.Value,
> - &error,
> - NULL);
> + if (has_modifier && dri2_dpy->image->createImageFromDmaBufs2) {
>
I think we need a version check here as well. How about
if (has_modifier) {
if (dri2_dpy->image->base.version < 15 ||
dri2_dpy->image->createImageFromDmaBufs2 == NULL) {
_eglError(EGL_BAD_MATCH, "unsupported dma_buf format modifier");
return EGL_NO_IMAGE_KHR;
}
/* Do the import */
} else {
/* Do the import without modifiers */
}
That also makes the check conditional on has_modifier which, IMHO, is
easier to read than putting the error for "you don't have the DRI
extension" right before the fallback.
> + dri_image =
> + dri2_dpy->image->createImageFromDmaBufs2(dri2_dpy->dri_screen,
> + attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
> + fds, num_fds, pitches, offsets, modifiers,
> + attrs.DMABufYuvColorSpaceHint.Value,
> + attrs.DMABufSampleRangeHint.Value,
> + attrs.DMABufChromaHorizontalSiting.Value,
> + attrs.DMABufChromaVerticalSiting.Value,
> + &error,
> + NULL);
> + } else {
> + if (has_modifier) {
> + _eglError(EGL_BAD_MATCH, "unsupported dma_buf format modifier");
> + return EGL_NO_IMAGE_KHR;
> + }
> +
> + dri_image =
> + dri2_dpy->image->createImageFromDmaBufs(dri2_dpy->dri_screen,
> + attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
> + fds, num_fds, pitches, offsets,
> + attrs.DMABufYuvColorSpaceHint.Value,
> + attrs.DMABufSampleRangeHint.Value,
> + attrs.DMABufChromaHorizontalSiting.Value,
> + attrs.DMABufChromaVerticalSiting.Value,
> + &error,
> + NULL);
> + }
> dri2_create_image_khr_texture_error(error);
>
> if (!dri_image)
> diff --git a/src/egl/main/eglimage.c b/src/egl/main/eglimage.c
> index fed390a498..c558f2f02b 100644
> --- a/src/egl/main/eglimage.c
> +++ b/src/egl/main/eglimage.c
> @@ -106,6 +106,18 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs,
> _EGLDisplay *dpy,
> attrs->DMABufPlanePitches[0].Value = val;
> attrs->DMABufPlanePitches[0].IsPresent = EGL_TRUE;
> break;
> + case EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersLo[0].Value = val;
> + attrs->DMABufPlaneModifiersLo[0].IsPresent = EGL_TRUE;
> + break;
> + case EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersHi[0].Value = val;
> + attrs->DMABufPlaneModifiersHi[0].IsPresent = EGL_TRUE;
> + break;
>
This code looks like it's badly in need of a macro... Not that you need to
add one in this patch, but ouch...
> case EGL_DMA_BUF_PLANE1_FD_EXT:
> attrs->DMABufPlaneFds[1].Value = val;
> attrs->DMABufPlaneFds[1].IsPresent = EGL_TRUE;
> @@ -118,6 +130,18 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs,
> _EGLDisplay *dpy,
> attrs->DMABufPlanePitches[1].Value = val;
> attrs->DMABufPlanePitches[1].IsPresent = EGL_TRUE;
> break;
> + case EGL_DMA_BUF_PLANE1_MODIFIER_LO_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersLo[1].Value = val;
> + attrs->DMABufPlaneModifiersLo[1].IsPresent = EGL_TRUE;
> + break;
> + case EGL_DMA_BUF_PLANE1_MODIFIER_HI_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersHi[1].Value = val;
> + attrs->DMABufPlaneModifiersHi[1].IsPresent = EGL_TRUE;
> + break;
> case EGL_DMA_BUF_PLANE2_FD_EXT:
> attrs->DMABufPlaneFds[2].Value = val;
> attrs->DMABufPlaneFds[2].IsPresent = EGL_TRUE;
> @@ -130,6 +154,18 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs,
> _EGLDisplay *dpy,
> attrs->DMABufPlanePitches[2].Value = val;
> attrs->DMABufPlanePitches[2].IsPresent = EGL_TRUE;
> break;
> + case EGL_DMA_BUF_PLANE2_MODIFIER_LO_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersLo[2].Value = val;
> + attrs->DMABufPlaneModifiersLo[2].IsPresent = EGL_TRUE;
> + break;
> + case EGL_DMA_BUF_PLANE2_MODIFIER_HI_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersHi[2].Value = val;
> + attrs->DMABufPlaneModifiersHi[2].IsPresent = EGL_TRUE;
> + break;
> case EGL_DMA_BUF_PLANE3_FD_EXT:
> if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> err = EGL_BAD_PARAMETER;
> @@ -148,6 +184,18 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs,
> _EGLDisplay *dpy,
> attrs->DMABufPlanePitches[3].Value = val;
> attrs->DMABufPlanePitches[3].IsPresent = EGL_TRUE;
> break;
> + case EGL_DMA_BUF_PLANE3_MODIFIER_LO_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersLo[3].Value = val;
> + attrs->DMABufPlaneModifiersLo[3].IsPresent = EGL_TRUE;
> + break;
> + case EGL_DMA_BUF_PLANE3_MODIFIER_HI_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> + err = EGL_BAD_PARAMETER;
> + attrs->DMABufPlaneModifiersHi[3].Value = val;
> + attrs->DMABufPlaneModifiersHi[3].IsPresent = EGL_TRUE;
> + break;
> case EGL_YUV_COLOR_SPACE_HINT_EXT:
> if (val != EGL_ITU_REC601_EXT && val != EGL_ITU_REC709_EXT &&
> val != EGL_ITU_REC2020_EXT) {
> diff --git a/src/egl/main/eglimage.h b/src/egl/main/eglimage.h
> index a909d9b588..eb66280ff9 100644
> --- a/src/egl/main/eglimage.h
> +++ b/src/egl/main/eglimage.h
> @@ -73,6 +73,8 @@ struct _egl_image_attribs
> struct _egl_image_attrib_int DMABufPlaneFds[DMA_BUF_MAX_PLANES];
> struct _egl_image_attrib_int DMABufPlaneOffsets[DMA_BUF_MAX_PLANES];
> struct _egl_image_attrib_int DMABufPlanePitches[DMA_BUF_MAX_PLANES];
> + struct _egl_image_attrib_int DMABufPlaneModifiersLo[DMA_
> BUF_MAX_PLANES];
> + struct _egl_image_attrib_int DMABufPlaneModifiersHi[DMA_
> BUF_MAX_PLANES];
> struct _egl_image_attrib_int DMABufYuvColorSpaceHint;
> struct _egl_image_attrib_int DMABufSampleRangeHint;
> struct _egl_image_attrib_int DMABufChromaHorizontalSiting;
> --
> 2.13.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170519/b8f88b01/attachment-0001.html>
More information about the mesa-dev
mailing list