<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 19, 2017 at 2:37 AM, Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Varad Gautam <<a href="mailto:varad.gautam@collabora.com">varad.gautam@collabora.com</a>> 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 <<a href="mailto:pekka.paalanen@collabora.co.uk">pekka.paalanen@collabora.co.uk</a>> Signed-off-by: Varad Gautam <<a href="mailto:varad.gautam@collabora.com">varad.gautam@collabora.com</a>> Signed-off-by: Louis-Francis Ratté-Boulianne <<a href="mailto:lfrb@collabora.com">lfrb@collabora.com</a>> Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com">eric.engestrom@imgtec.com</a>> Reviewed-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>> Signed-off-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>> --- 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) { </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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; </blockquote><div> </div><div>Do we really want 0 here? How about INVALID? Not that it matters since they're all required to be the same... </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + } } - 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) { </blockquote><div> </div><div>I think we need a version check here as well. How about </div><div>if (has_modifier) { </div><div> 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; } </div><div> /* Do the import */ </div><div>} else { </div><div> /* Do the import without modifiers */ </div><div>} </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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; </blockquote><div> </div><div>This code looks like it's badly in need of a macro... Not that you need to add one in this patch, but ouch... </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>