[Mesa-dev] [v8 8/9] egl/dri2: support for creating images out of dma buffers

[Pohjolainen, Topi]

On Wed, Jul 24, 2013 at 05:02:50PM -0700, Chad Versace wrote:
> On 07/24/2013 03:23 AM, Topi Pohjolainen wrote:
> >v2:
> >    - upon success close the given file descriptors
> >
> >v3:
> >    - use specific entry for dma buffers instead of the basic for
> >      primes, and enable the extension based on the availability
> >      of the hook
> >
> >v4 (Chad):
> >    - use ARRAY_SIZE
> >    - improve the comment about the number of file descriptors
> >    - in case of invalid format report EGL_BAD_ATTRIBUTE instead
> >      of EGL_BAD_MATCH
> >    - take into account specific error set by the driver.
> >
> >v5:
> >    - fix error handling
> >
> >v6 (Chad):
> >    - fix invalid plane count checking
> >
> >Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >---
> >  src/egl/drivers/dri2/egl_dri2.c | 262 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 262 insertions(+)
> 
> 
> 
> >+/* Returns the total number of file descriptors. Zero indicates an error. */
> >+static unsigned
> >+dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
> >+{
> >+   unsigned i, plane_n;
> >+
> >+   switch (attrs->DMABufFourCC.Value) {
> >+   case DRM_FORMAT_RGB332:
> >+   case DRM_FORMAT_BGR233:
> >+   case DRM_FORMAT_XRGB4444:
> >+   case DRM_FORMAT_XBGR4444:
> >+   case DRM_FORMAT_RGBX4444:
> >+   case DRM_FORMAT_BGRX4444:
> >+   case DRM_FORMAT_ARGB4444:
> >+   case DRM_FORMAT_ABGR4444:
> >+   case DRM_FORMAT_RGBA4444:
> >+   case DRM_FORMAT_BGRA4444:
> >+   case DRM_FORMAT_XRGB1555:
> >+   case DRM_FORMAT_XBGR1555:
> >+   case DRM_FORMAT_RGBX5551:
> >+   case DRM_FORMAT_BGRX5551:
> >+   case DRM_FORMAT_ARGB1555:
> >+   case DRM_FORMAT_ABGR1555:
> >+   case DRM_FORMAT_RGBA5551:
> >+   case DRM_FORMAT_BGRA5551:
> >+   case DRM_FORMAT_RGB565:
> >+   case DRM_FORMAT_BGR565:
> >+   case DRM_FORMAT_RGB888:
> >+   case DRM_FORMAT_BGR888:
> >+   case DRM_FORMAT_XRGB8888:
> >+   case DRM_FORMAT_XBGR8888:
> >+   case DRM_FORMAT_RGBX8888:
> >+   case DRM_FORMAT_BGRX8888:
> >+   case DRM_FORMAT_ARGB8888:
> >+   case DRM_FORMAT_ABGR8888:
> >+   case DRM_FORMAT_RGBA8888:
> >+   case DRM_FORMAT_BGRA8888:
> >+   case DRM_FORMAT_XRGB2101010:
> >+   case DRM_FORMAT_XBGR2101010:
> >+   case DRM_FORMAT_RGBX1010102:
> >+   case DRM_FORMAT_BGRX1010102:
> >+   case DRM_FORMAT_ARGB2101010:
> >+   case DRM_FORMAT_ABGR2101010:
> >+   case DRM_FORMAT_RGBA1010102:
> >+   case DRM_FORMAT_BGRA1010102:
> >+   case DRM_FORMAT_YUYV:
> >+   case DRM_FORMAT_YVYU:
> >+   case DRM_FORMAT_UYVY:
> >+   case DRM_FORMAT_VYUY:
> >+      plane_n = 1;
> >+      break;
> >+   case DRM_FORMAT_NV12:
> >+   case DRM_FORMAT_NV21:
> >+   case DRM_FORMAT_NV16:
> >+   case DRM_FORMAT_NV61:
> >+      plane_n = 2;
> >+      break;
> >+   case DRM_FORMAT_YUV410:
> >+   case DRM_FORMAT_YVU410:
> >+   case DRM_FORMAT_YUV411:
> >+   case DRM_FORMAT_YVU411:
> >+   case DRM_FORMAT_YUV420:
> >+   case DRM_FORMAT_YVU420:
> >+   case DRM_FORMAT_YUV422:
> >+   case DRM_FORMAT_YVU422:
> >+   case DRM_FORMAT_YUV444:
> >+   case DRM_FORMAT_YVU444:
> >+      plane_n = 3;
> >+      break;
> >+   default:
> >+      _eglError(EGL_BAD_ATTRIBUTE, "invalid format");
> >+      return 0;
> >+   }
> >+
> >+   /**
> >+     * The spec says:
> >+     *
> >+     * "* If <target> is EGL_LINUX_DMA_BUF_EXT, and the list of attributes is
> >+     *    incomplete, EGL_BAD_PARAMETER is generated."
> >+     */
> >+   for (i = 0; i < plane_n; ++i) {
> >+      if (!attrs->DMABufPlaneFds[i].IsPresent ||
> >+         !attrs->DMABufPlaneOffsets[i].IsPresent ||
> >+         !attrs->DMABufPlanePitches[i].IsPresent) {
> >+            _eglError(EGL_BAD_PARAMETER, "plane attribute(s) missing");
> >+         return 0;
> >+      }
> >+   }
> 
> Indentation for the if-condition should not be aligned left of the opening paren.
> That is, the two `!attr` have off-by-one indetation.

I was wondering about that - indentation by three versus this special case.

> 
> Also, the entirety of the if-body should be left-aligned. But, `_eglError` and
> `return` are not aligned.
> 
> Other than indentation, up to here the function looks good. But...
> 
> >+
> >+   /**
> >+    * The spec says:
> >+    *
> >+    * "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."
> >+    */
> >+   for ( ; i < 3; ++i) {
> >+      if (attrs->DMABufPlaneFds[i].IsPresent ||
> >+         attrs->DMABufPlaneOffsets[i].IsPresent ||
> >+         attrs->DMABufPlanePitches[i].IsPresent) {
> >+            _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");
> >+         return 0;
> >+      }
> >+   }
> >+
> >+   return plane_n;
> >+}
> 
> This loop looks buggy. I think the loop head should be
>   for (i = plane_n; i < 3; ++i)

If the 'return' statement in the previous loop is not taken, one always ends up
here with 'i == plane_n'. I can reset 'i' explicitly also if you think it is
clearer.

> 
> Also, this loop has the same indentation problems as the previous.