[Mesa-dev] [PATCH v2] egl_dri2: add support for using modifier attributes in eglCreateImageKHR

[Emil Velikov]

On 16 November 2016 at 09:28, Varad Gautam <varadgautam at gmail.com> wrote:
> From: Pekka Paalanen <pekka.paalanen at 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.
>
> v2: clear modifier assembling and error label name (Eric Engestrom)
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
> ---

> +   int nonzero_modifier_found = 0;
>     unsigned error;
>
>     /**
> @@ -2106,18 +2125,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] = (attrs.DMABufPlaneModifiersHi[i].Value << 32) |
> +                        attrs.DMABufPlaneModifiersLo[i].Value;
> +         if (modifiers[i] != 0)
> +            nonzero_modifier_found = EGL_TRUE;
integer storage and EGL_TRUE -> bool and true/false ?


> +   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;
> +      }
> +
Using something like the following might be better?

if (nonzero_modifier_found) {
   if (!dri2_dpy->image->createImageFromDmaBufs2)
     # assert should never reach here, since the extension should be
advertised only if the API is available.
   use new API
else
   use old API


> +      case EGL_DMA_BUF_PLANE3_MODIFIER_LO_EXT:
> +         if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> +            goto bad_param;
> +         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 bad_param;
> +         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;
>
> +bad_param:
Using goto to jump to another case statement is "evil". Please don't use them.


Thanks
Emil