On Tuesday, 2016-11-15 19:54:28 +0530, Varad Gautam wrote:
> From: Pekka Paalanen <pekka.paala...@collabora.co.uk>
>
> allow creating EGLImages with dmabuf format modifiers when target is
> EGL_LINUX_DMA_BUF_EXT for EGL_EXT_image_dma_buf_import_modifiers.
>
> Signed-off-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> Signed-off-by: Varad Gautam <varad.gau...@collabora.com>
> ---
> src/egl/drivers/dri2/egl_dri2.c | 68
> ++++++++++++++++++++++++++++++++++-------
> src/egl/main/egldisplay.h | 1 +
> src/egl/main/eglimage.c | 49 +++++++++++++++++++++++++++++
> src/egl/main/eglimage.h | 2 ++
> 4 files changed, 109 insertions(+), 11 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 58d16e1..4eb1861 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1937,6 +1937,21 @@ 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;
> + }
> + }
> +
> return EGL_TRUE;
> }
>
> @@ -2043,7 +2058,9 @@ 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) {
> _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");
> return 0;
> }
> @@ -2076,6 +2093,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];
> + int nonzero_modifier_found = 0;
> unsigned error;
>
> /**
> @@ -2106,18 +2125,45 @@ 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] = attrs.DMABufPlaneModifiersHi[i].Value;
> + modifiers[i] = (modifiers[i] << 32) |
> + attrs.DMABufPlaneModifiersLo[i].Value;
I'd prefer this, it's clearer IMO, and avoids temporarily invalid values
(eg. `hi` unshifted):
modifiers[i] = attrs.DMABufPlaneModifiersHi[i].Value << 32 |
attrs.DMABufPlaneModifiersLo[i].Value;
Otherwise, it all looks good to me (one nit-pick below), but I don't
know Gallium enough, so I'm only r-b'ing the EGL bits.
Patches 1-4, 7-8, 12-13 are:
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
> + if (modifiers[i] != 0)
> + nonzero_modifier_found = EGL_TRUE;
> + } else {
> + modifiers[i] = 0;
> + }
> }
>
> - 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 (nonzero_modifier_found && dri2_dpy->image->createImageFromDmaBufs2) {
> + 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 (nonzero_modifier_found) {
> + _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/egldisplay.h b/src/egl/main/egldisplay.h
> index 62d9a11..7a59dc5 100644
> --- a/src/egl/main/egldisplay.h
> +++ b/src/egl/main/egldisplay.h
> @@ -101,6 +101,7 @@ struct _egl_extensions
> EGLBoolean EXT_buffer_age;
> EGLBoolean EXT_create_context_robustness;
> EGLBoolean EXT_image_dma_buf_import;
> + EGLBoolean EXT_image_dma_buf_import_modifiers;
> EGLBoolean EXT_swap_buffers_with_damage;
>
> EGLBoolean KHR_cl_event2;
> diff --git a/src/egl/main/eglimage.c b/src/egl/main/eglimage.c
> index cd170c6..f98b937 100644
> --- a/src/egl/main/eglimage.c
> +++ b/src/egl/main/eglimage.c
> @@ -109,6 +109,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)
> + goto out;
> + 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)
> + goto out;
> + attrs->DMABufPlaneModifiersHi[0].Value = val;
> + attrs->DMABufPlaneModifiersHi[0].IsPresent = EGL_TRUE;
> + break;
> case EGL_DMA_BUF_PLANE1_FD_EXT:
> attrs->DMABufPlaneFds[1].Value = val;
> attrs->DMABufPlaneFds[1].IsPresent = EGL_TRUE;
> @@ -121,6 +133,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)
> + goto out;
> + 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)
> + goto out;
> + 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;
> @@ -133,6 +157,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)
> + goto out;
> + 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)
> + goto out;
> + attrs->DMABufPlaneModifiersHi[2].Value = val;
> + attrs->DMABufPlaneModifiersHi[2].IsPresent = EGL_TRUE;
> + break;
> case EGL_DMA_BUF_PLANE3_FD_EXT:
> attrs->DMABufPlaneFds[3].Value = val;
> attrs->DMABufPlaneFds[3].IsPresent = EGL_TRUE;
> @@ -145,6 +181,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)
> + goto out;
> + 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)
> + goto out;
> + 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) {
> @@ -181,6 +229,7 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs,
> _EGLDisplay *dpy,
> }
> break;
>
> +out:
Nit: call this `fail`, `error`, `bad_param`, or anything else that
indicates that it's an error path?
Cheers,
Eric
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
