On Fri, May 19, 2017 at 2:37 AM, Daniel Stone <[email protected]> wrote:
> From: Varad Gautam <[email protected]> > > 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 <[email protected]> > Signed-off-by: Varad Gautam <[email protected]> > Signed-off-by: Louis-Francis Ratté-Boulianne <[email protected]> > Reviewed-by: Eric Engestrom <[email protected]> > Reviewed-by: Daniel Stone <[email protected]> > Signed-off-by: Daniel Stone <[email protected]> > --- > 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
