On Thu, May 23, 2013 at 09:39:30PM -0700, Chad Versace wrote:
> When touching the src/egl/drivers/dri2 directory, use a commit subject
> that looks like "egl/dri2: STUFF", not "egl: dri2: STUFF".
>
> On 05/02/2013 12:08 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
> >
> >Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> >---
> > src/egl/drivers/dri2/egl_dri2.c | 280
> > ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 280 insertions(+)
> >
> >diff --git a/src/egl/drivers/dri2/egl_dri2.c
> >b/src/egl/drivers/dri2/egl_dri2.c
> >index 1011f27..cfa7cf0 100644
> >--- a/src/egl/drivers/dri2/egl_dri2.c
> >+++ b/src/egl/drivers/dri2/egl_dri2.c
> >@@ -34,6 +34,7 @@
> > #include <errno.h>
> > #include <unistd.h>
> > #include <xf86drm.h>
> >+#include <drm_fourcc.h>
> > #include <GL/gl.h>
> > #include <GL/internal/dri_interface.h>
> > #include <sys/types.h>
> >@@ -507,6 +508,10 @@ dri2_setup_screen(_EGLDisplay *disp)
> > disp->Extensions.KHR_gl_texture_2D_image = EGL_TRUE;
> > disp->Extensions.KHR_gl_texture_cubemap_image = EGL_TRUE;
> > }
> >+ if (dri2_dpy->image->base.version >= 8 &&
> >+ dri2_dpy->image->createImageFromDmaBufs) {
> >+ disp->Extensions.EXT_image_dma_buf_import = EGL_TRUE;
> >+ }
> > }
> > }
> >
> >@@ -1170,6 +1175,279 @@ dri2_create_image_mesa_drm_buffer(_EGLDisplay *disp,
> >_EGLContext *ctx,
> > return dri2_create_image(disp, dri_image);
> > }
> >
> >+static EGLBoolean
> >+dri2_check_dma_buf_attribs(const _EGLImageAttribs *attrs)
> >+{
> >+ unsigned i;
> >+
> >+ /**
> >+ * The spec says:
> >+ *
> >+ * "Required attributes and their values are as follows:
> >+ *
> >+ * * EGL_WIDTH & EGL_HEIGHT: The logical dimensions of the buffer in
> >pixels
> >+ *
> >+ * * EGL_LINUX_DRM_FOURCC_EXT: The pixel format of the buffer, as
> >specified
> >+ * by drm_fourcc.h and used as the pixel_format parameter of the
> >+ * drm_mode_fb_cmd2 ioctl."
> >+ *
> >+ * * EGL_DMA_BUF_PLANE0_FD_EXT: The dma_buf file descriptor of plane 0
> >of
> >+ * the image.
> >+ *
> >+ * * EGL_DMA_BUF_PLANE0_OFFSET_EXT: The offset from the start of the
> >+ * dma_buf of the first sample in plane 0, in bytes.
> >+ *
> >+ * * EGL_DMA_BUF_PLANE0_PITCH_EXT: The number of bytes between the
> >start of
> >+ * subsequent rows of samples in plane 0. May have special meaning
> >for
> >+ * non-linear formats."
> >+ *
> >+ * "* If <target> is EGL_LINUX_DMA_BUF_EXT, and the list of attributes
> >is
> >+ * incomplete, EGL_BAD_PARAMETER is generated."
> >+ */
> >+ if (attrs->Width <= 0 || attrs->Height <= 0 ||
> >+ !attrs->DMABufFourCC.IsPresent ||
> >+ !attrs->DMABufPlaneFds[0].IsPresent ||
> >+ !attrs->DMABufPlaneOffsets[0].IsPresent ||
> >+ !attrs->DMABufPlanePitches[0].IsPresent) {
> >+ _eglError(EGL_BAD_PARAMETER, "attribute(s) missing");
> >+ return EGL_FALSE;
> >+ }
> >+
> >+ /**
> >+ * Also:
> >+ *
> >+ * "If <target> is EGL_LINUX_DMA_BUF_EXT and one or more of the values
> >+ * specified for a plane's pitch or offset isn't supported by EGL,
> >+ * EGL_BAD_ACCESS is generated."
> >+ */
> >+ for (i = 0; i < sizeof(attrs->DMABufPlanePitches) /
> >+ sizeof(attrs->DMABufPlanePitches[0]); ++i) {
>
> Use ARRAY_SIZE here.
Will do.
>
> >+ if (attrs->DMABufPlanePitches[i].IsPresent &&
> >+ attrs->DMABufPlanePitches[i].Value <= 0) {
> >+ _eglError(EGL_BAD_ACCESS, "invalid pitch");
> >+ return EGL_FALSE;
> >+ }
> >+ }
> >+
> >+ return EGL_TRUE;
> >+}
> >+
> >+/* Returns the total number of file descriptors zero indicating an error. */
>
> /* Returns the total number of file descriptors. Zero indicates an error. */
Ok.
>
> >+static unsigned
> >+dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
> >+{
> >+ 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:
> >+ /* There must be one and only one plane present */
> >+ if (attrs->DMABufPlaneFds[0].IsPresent &&
> >+ attrs->DMABufPlaneOffsets[0].IsPresent &&
> >+ attrs->DMABufPlanePitches[0].IsPresent &&
> >+ !attrs->DMABufPlaneFds[1].IsPresent &&
> >+ !attrs->DMABufPlaneOffsets[1].IsPresent &&
> >+ !attrs->DMABufPlanePitches[1].IsPresent &&
> >+ !attrs->DMABufPlaneFds[2].IsPresent &&
> >+ !attrs->DMABufPlaneOffsets[2].IsPresent &&
> >+ !attrs->DMABufPlanePitches[2].IsPresent)
> >+ return 1;
> >+ case DRM_FORMAT_NV12:
> >+ case DRM_FORMAT_NV21:
> >+ case DRM_FORMAT_NV16:
> >+ case DRM_FORMAT_NV61:
> >+ /* There must be two and only two planes present */
> >+ if (attrs->DMABufPlaneFds[0].IsPresent &&
> >+ attrs->DMABufPlaneOffsets[0].IsPresent &&
> >+ attrs->DMABufPlanePitches[0].IsPresent &&
> >+ attrs->DMABufPlaneFds[1].IsPresent &&
> >+ attrs->DMABufPlaneOffsets[1].IsPresent &&
> >+ attrs->DMABufPlanePitches[1].IsPresent &&
> >+ !attrs->DMABufPlaneFds[2].IsPresent &&
> >+ !attrs->DMABufPlaneOffsets[2].IsPresent &&
> >+ !attrs->DMABufPlanePitches[2].IsPresent)
> >+ return 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:
> >+ /* All three planes must be specified */
> >+ if (attrs->DMABufPlaneFds[0].IsPresent &&
> >+ attrs->DMABufPlaneOffsets[0].IsPresent &&
> >+ attrs->DMABufPlanePitches[0].IsPresent &&
> >+ attrs->DMABufPlaneFds[1].IsPresent &&
> >+ attrs->DMABufPlaneOffsets[1].IsPresent &&
> >+ attrs->DMABufPlanePitches[1].IsPresent &&
> >+ attrs->DMABufPlaneFds[2].IsPresent &&
> >+ attrs->DMABufPlaneOffsets[2].IsPresent &&
> >+ attrs->DMABufPlanePitches[2].IsPresent)
> >+ return 3;
> >+ break;
> >+ default:
> >+ /*
> >+ * The spec says:
> >+ *
> >+ * "If <target> is EGL_LINUX_DMA_BUF_EXT, and the
> >EGL_LINUX_DRM_FOURCC_EXT
> >+ * attribute is set to a format not supported by the EGL,
> >EGL_BAD_MATCH
> >+ * is generated."
> >+ */
> >+ _eglError(EGL_BAD_MATCH, "invalid format");
> >+ return 0;
>
> This is a subtle point. I think EGL_BAD_ATTRIBUTE should be returned here.
>
> Usually in EGL, if an attribute value is completely invalid (for example, if
> the value of EGL_LINUX_DRM_FOURCC_EXT is none of the above) then the returned
> error is EGL_BAD_ATTRIBUTE. The error EGL_BAD_MATCH is generated when the
> implementation does not support the requested attribute, but otherwise the
> attribute is valid.
>
> So, return EGL_BAD_ATTRIBUTE here. And, if a driver rejects a particular
> fourcc
> format, then return EGL_BAD_MATCH there.
Makes sense, I'll switch.
>
> >+ }
> >+
> >+ /**
> >+ * 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."
> >+ */
> >+ _eglError(EGL_BAD_ATTRIBUTE, "invalid number of planes");
> >+
> >+ return 0;
> >+}
> >+
> >+/**
> >+ * The spec says:
> >+ *
> >+ * "If eglCreateImageKHR is successful for a EGL_LINUX_DMA_BUF_EXT target,
> >+ * the EGL takes ownership of the file descriptor and is responsible for
> >+ * closing it, which it may do at any time while the EGLDisplay is
> >+ * initialized."
> >+ */
> >+static void
> >+dri2_take_dma_buf_ownership(const int *fds, unsigned num_fds)
> >+{
> >+ int already_closed[num_fds];
> >+ unsigned num_closed = 0;
> >+ unsigned i, j;
> >+
> >+ for (i = 0; i < num_fds; ++i) {
> >+ /**
> >+ * The same file descriptor can be referenced multiple times in case
> >more
> >+ * than one plane is found in the same buffer, just with a different
> >+ * offset.
> >+ */
> >+ for (j = 0; j < num_closed; ++j) {
> >+ if (already_closed[j] == fds[i])
>
> The condition above has undefined behavior, because the array is initialized
> on
> the stack. If you declare the array like this, though:
> int already_closed[num_fds] = {0,};
> then it will be initialized with all zeros.
There is the explicit counter 'num_closed' telling how many valid elements there
are in 'already_closed'.
>
> From O'Reilly's "C in a Nutshell", Chapter 8 "Arrays":
> "If you do not explicitly initialize n array variable, the usual rules
> apply:
> if the array has automatic storage duration, then its elements have
> undefined
> values."
>
> "If the definition of an array contains both a length specifier and an
> initialization
> list, then [...] Any elements for which there is no initializer are
> initialized to
> zero [...]"
>
> However, it's not even correct to zero-fill `already_closed`, because one of
> the fds
> could be 0 itself. Extremely unlikely, but possible. To ensure correctness,
> `already_closed`
> should be initialized to a number than is an invalid fd:
>
> for (i = 0; i < num_fds; ++i)
> already_closed[i] = -1;
>
> >+ break;
> >+ }
> >+
> >+ if (j == num_closed) {
> >+ close(fds[i]);
> >+ already_closed[num_closed++] = fds[i];
> >+ }
> >+ }
> >+}
> >+
> >+static _EGLImage *
> >+dri2_create_image_dma_buf(_EGLDisplay *disp, _EGLContext *ctx,
> >+ EGLClientBuffer buffer, const EGLint *attr_list)
> >+{
> >+ struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> >+ _EGLImage *res;
> >+ EGLint err;
> >+ _EGLImageAttribs attrs;
> >+ __DRIimage *dri_image;
> >+ unsigned num_fds;
> >+ unsigned i;
> >+ int fds[3];
> >+ int pitches[3];
> >+ int offsets[3];
> >+
> >+ /**
> >+ * The spec says:
> >+ *
> >+ * ""* If <target> is EGL_LINUX_DMA_BUF_EXT and <buffer> is not NULL, the
> >+ * error EGL_BAD_PARAMETER is generated."
> >+ */
> >+ if (buffer != NULL) {
> >+ _eglError(EGL_BAD_PARAMETER, "buffer not NULL");
> >+ return NULL;
> >+ }
> >+
> >+ err = _eglParseImageAttribList(&attrs, disp, attr_list);
> >+ if (err != EGL_SUCCESS) {
> >+ _eglError(err, "bad attribute");
> >+ return NULL;
> >+ }
> >+
> >+ if (!dri2_check_dma_buf_attribs(&attrs))
> >+ return NULL;
> >+
> >+ num_fds = dri2_check_dma_buf_format(&attrs);
> >+ if (!num_fds)
> >+ return NULL;
> >+
> >+ for (i = 0; i < num_fds; ++i) {
> >+ fds[i] = attrs.DMABufPlaneFds[i].Value;
> >+ pitches[i] = attrs.DMABufPlanePitches[i].Value;
> >+ offsets[i] = attrs.DMABufPlaneOffsets[i].Value;
> >+ }
> >+
> >+ 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,
> >+ NULL);
>
> Here, if the driver rejects the fourcc format, then we need to emit
> EGL_BAD_MATCH. To do that, we need an additional 'error' parameter to
> createImageFromDmaBufs. For a precedent, see the signature of
> createImageFromTexture and __DRI_IMAGE_ERROR_BAD_MATCH.
>
> However, if createImageFromDmaBufs fails for a different reason, unrelated
> to EGL_BAD_MATCH, then it needs to return an appropriate DRI error code,
> which we will here translate to an EGL error code. Again, see how
> the __DRI_IMAGE_ERROR codes work. You may need to add additional DRI error
> codes to handle all the ways createImageFromDmaBufs could fail, or maybe
> not; I haven't thought that through.
Ok, I'll work on it.
>
> >+
> >+ res = dri2_create_image(disp, dri_image);
> >+ if (res)
> >+ dri2_take_dma_buf_ownership(fds, num_fds);
> >+
> >+ return res;
> >+}
> >+
> > #ifdef HAVE_WAYLAND_PLATFORM
> >
> > /* This structure describes how a wl_buffer maps to one or more
> >@@ -1364,6 +1642,8 @@ dri2_create_image_khr(_EGLDriver *drv, _EGLDisplay
> >*disp,
> > case EGL_WAYLAND_BUFFER_WL:
> > return dri2_create_image_wayland_wl_buffer(disp, ctx, buffer,
> > attr_list);
> > #endif
> >+ case EGL_LINUX_DMA_BUF_EXT:
> >+ return dri2_create_image_dma_buf(disp, ctx, buffer, attr_list);
> > default:
> > _eglError(EGL_BAD_PARAMETER, "dri2_create_image_khr");
> > return EGL_NO_IMAGE_KHR;
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
