[Mesa-dev] [PATCH 04/13] egl/main: add support for fourth plane tokens

[Ben Widawsky]

On 16-11-18 14:19:28, Emil Velikov wrote:
>On 15 November 2016 at 14:24, Varad Gautam <varadgautam at gmail.com> wrote:
>> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>
>> The EGL_EXT_dma_buf_import_modifiers extension adds support for a
>> fourth plane, just like DRM KMS API does.
>>
>> Bump maximum dma_buf plane count to four.
>>
>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
>> ---
>>  src/egl/drivers/dri2/egl_dri2.c |  2 +-
>>  src/egl/main/eglimage.c         | 12 ++++++++++++
>>  src/egl/main/eglimage.h         |  2 +-
>>  3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
>> index 9a41ad0..58d16e1 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -2038,7 +2038,7 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
>>      * "If <target> is EGL_LINUX_DMA_BUF_EXT, and the EGL_LINUX_DRM_FOURCC_EXT
>>      *  attribute indicates a single-plane format, EGL_BAD_ATTRIBUTE is
>>      *  generated if any of the EGL_DMA_BUF_PLANE1_* or EGL_DMA_BUF_PLANE2_*
>> -    *  attributes are specified."
>> +    *  or EGL_DMA_BUF_PLANE3_* attributes are specified."
>>      */
>>     for (i = plane_n; i < DMA_BUF_MAX_PLANES; ++i) {
>>        if (attrs->DMABufPlaneFds[i].IsPresent ||
>> diff --git a/src/egl/main/eglimage.c b/src/egl/main/eglimage.c
>> index 411d1ca..cd170c6 100644
>> --- a/src/egl/main/eglimage.c
>> +++ b/src/egl/main/eglimage.c
>> @@ -133,6 +133,18 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs, _EGLDisplay *dpy,
>>           attrs->DMABufPlanePitches[2].Value = val;
>>           attrs->DMABufPlanePitches[2].IsPresent = EGL_TRUE;
>>           break;
>> +      case EGL_DMA_BUF_PLANE3_FD_EXT:
>> +         attrs->DMABufPlaneFds[3].Value = val;
>> +         attrs->DMABufPlaneFds[3].IsPresent = EGL_TRUE;
>> +         break;
>> +      case EGL_DMA_BUF_PLANE3_OFFSET_EXT:
>> +         attrs->DMABufPlaneOffsets[3].Value = val;
>> +         attrs->DMABufPlaneOffsets[3].IsPresent = EGL_TRUE;
>> +         break;
>> +      case EGL_DMA_BUF_PLANE3_PITCH_EXT:
>> +         attrs->DMABufPlanePitches[3].Value = val;
>> +         attrs->DMABufPlanePitches[3].IsPresent = EGL_TRUE;
>> +         break;
>These should be within an extension guard. Otherwise we'll parse them
>(and try to push down) even if we don't support them.
>
>Something line the following should do it. Either squashed here or
>separate patch is fine.
>
>src/egl/main/egldisplay.h
>+ EGLBoolean EGL_EXT_dma_buf_import_modifiers;
>
>src/egl/main/eglapi.c
>+   _EGL_CHECK_EXTENSION(EGL_EXT_dma_buf_import_modifiers);
>
>src/egl/main/eglimage.h
>-   /* EGL_EXT_image_dma_buf_import */
>+   /* EGL_EXT_image_dma_buf_import and EGL_EXT_dma_buf_import_modifiers */
>
>src/egl/main/eglimage.c
>+      case EGL_DMA_BUF_PLANE3_FD_EXT:
>+         if (!disp->Extensions.EXT_dma_buf_import_modifiers) {
>+             err = EGL_BAD_ATTRIBUTE;
>+             break;
>+         }
>+         attrs->DMABufPlaneFds[3].Value = val;
>+         attrs->DMABufPlaneFds[3].IsPresent = EGL_TRUE;
>+         break;
>and same for OFFSET and PITCH.
>
>IMHO we want to keep the new code relatively bug free, so it's better
>to address those irrespective of the bugs/extra work mentioned below.
>
>Afaict none of the existing attribs honour their respective extension
>(bool). Some of them are kind of ok like EGL_EXT_image_dma_buf_import
>were we don't have the API/vfunc so even if we parse the values we
>cannot push them further down. Either way correct extensions' attrib
>parsing can be addressed, as independent work at a later point in
>time.
>
>Thanks
>Emil

I never bothered to understand how the attrib_list gets populated, so I'll
assume you're correct about this. So with Emil's suggestion, everything up to
here is:
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>