[Piglit] [PATCH 3/3] tests: spec: tests for EXT_image_dma_buf_import

Wed Apr 24 06:40:34 PDT 2013

On 04/16/2013 12:45 PM, Topi Pohjolainen wrote:
> These are rather simple. Without the capability of creating
> textures out of the images (glEGLImageTargetTexture2DOES()), one
> cannot do much with the images as such.
>
> Extra attention should be paid to the buffer lifecycle:
>
>     "3. Does ownership of the file descriptor pass to the EGL library?
>
>      ANSWER: If eglCreateImageKHR is successful, EGL assumes ownership
>      of the file descriptors and is responsible for closing them."
>
> In the tests I've chosen the approach where the creator releases
> its own access to the buffers as at least in Intel's case I cannot
> see how the EGL driver could do it on the creators behalf.
>
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>   tests/spec/CMakeLists.txt                          |   1 +
>   .../ext_image_dma_buf_import/CMakeLists.gles1.txt  |  19 ++
>   tests/spec/ext_image_dma_buf_import/CMakeLists.txt |   1 +
>   tests/spec/ext_image_dma_buf_import/close_buffer.c | 116 ++++++++++
>   .../spec/ext_image_dma_buf_import/create_yuv420.c  | 145 ++++++++++++
>   .../ext_image_dma_buf_import/invalid_attributes.c  | 255 +++++++++++++++++++++
>   .../spec/ext_image_dma_buf_import/invalid_hints.c  | 144 ++++++++++++
>   .../ext_image_dma_buf_import/missing_attributes.c  | 182 +++++++++++++++
>   8 files changed, 863 insertions(+)
>   create mode 100644 tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
>   create mode 100644 tests/spec/ext_image_dma_buf_import/CMakeLists.txt
>   create mode 100644 tests/spec/ext_image_dma_buf_import/close_buffer.c
>   create mode 100644 tests/spec/ext_image_dma_buf_import/create_yuv420.c
>   create mode 100644 tests/spec/ext_image_dma_buf_import/invalid_attributes.c
>   create mode 100644 tests/spec/ext_image_dma_buf_import/invalid_hints.c
>   create mode 100644 tests/spec/ext_image_dma_buf_import/missing_attributes.c

> diff --git a/tests/spec/ext_image_dma_buf_import/close_buffer.c b/tests/spec/ext_image_dma_buf_import/close_buffer.c

> +/**
> + * @file close_buffer.c
> + *
> + * From the EXT_image_dma_buf_import spec:
> + *
> + * "3. Does ownership of the file descriptor pass to the EGL library?
> + *
> + *   ANSWER: If eglCreateImageKHR is successful, EGL assumes ownership of the
> + *   file descriptors and is responsible for closing them."

In don't understand why this issue is quoted here, so it feels like noise to me.
A little more explanation about its context would help. Are you somehow testing
this text?

> diff --git a/tests/spec/ext_image_dma_buf_import/create_yuv420.c b/tests/spec/ext_image_dma_buf_import/create_yuv420.c

create_yuv420.c looks good to me.

> diff --git a/tests/spec/ext_image_dma_buf_import/invalid_attributes.c b/tests/spec/ext_image_dma_buf_import/invalid_attributes.c

> +static bool
> +test_excess_attributes(int fd, unsigned w, unsigned h, unsigned stride,
> +		unsigned offset)
> +{
> +	unsigned i;
> +	const EGLint excess_attributes[][2 * 7 + 1] = {
> +		{ EGL_HEIGHT, w,
> +		  EGL_WIDTH, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_DMA_BUF_PLANE1_FD_EXT, fd,
> +		  EGL_NONE },
> +		{ EGL_HEIGHT, w,
> +		  EGL_WIDTH, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_DMA_BUF_PLANE1_OFFSET_EXT, 0,
> +		  EGL_NONE },
> +		{ EGL_HEIGHT, w,
> +		  EGL_WIDTH, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_DMA_BUF_PLANE1_PITCH_EXT, stride,
> +		  EGL_NONE },
> +	};
> +
> +	for (i = 0; i < sizeof(excess_attributes) /
> +			sizeof(excess_attributes[0]); ++i) {

This should use the ARRAY_SIZE defined in piglit-util.h.

> +		/**
> +		 * The spec says:
> +		 *
> +		 *  "If <target> is EGL_LINUX_DMA_BUF_EXT, <dpy> must be a valid
> +		 *   display, <ctx> must be EGL_NO_CONTEXT, and <buffer> must be
> +		 *   NULL, cast into the type EGLClientBuffer."
> +		 */
> +		EGLImageKHR img = eglCreateImageKHR(eglGetCurrentDisplay(),
> +				EGL_NO_CONTEXT, EGL_LINUX_DMA_BUF_EXT,
> +				(EGLClientBuffer)NULL, excess_attributes[i]);
> +
> +		if (!piglit_check_egl_error(EGL_BAD_ATTRIBUTE)) {
> +			if (img)
> +				eglDestroyImageKHR(eglGetCurrentDisplay(), img);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +static bool
> +test_buffer_not_null(int fd, unsigned w, unsigned h, unsigned stride,
> +		unsigned offset)
> +{
> +	EGLImageKHR img;
> +	EGLint attr[] = {
> +		EGL_WIDTH, w,
> +		EGL_HEIGHT, h,
> +		EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		EGL_NONE
> +	};
> +
> +	/**
> +	 * The spec says:
> +	 *
> +	 *     "If <target> is EGL_LINUX_DMA_BUF_EXT, <dpy> must be a valid
> +	 *      display, <ctx> must be EGL_NO_CONTEXT, and <buffer> must be
> +	 *      NULL, cast into the type EGLClientBuffer."
> +	 */
> +	img = eglCreateImageKHR(eglGetCurrentDisplay(), EGL_NO_CONTEXT,
> +			EGL_LINUX_DMA_BUF_EXT, (EGLClientBuffer)1, attr);
> +
> +	if (!piglit_check_egl_error(EGL_BAD_PARAMETER)) {
> +		if (img)
> +			eglDestroyImageKHR(eglGetCurrentDisplay(), img);
> +		return false;
> +	}

In addition to verifying that EGL_BAD_PARAMETER is raised when `buffer != NULL`,
we also need a test that verifies EGL_BAD_CONTEXT is raised when `ctx != EGL_NO_CONTEXT`.

> +
> +	return true;
> +}

> +
> +/**
> + * On re-uses the buffer for all the tests. The spec says:

I'm not sure what you mean here. Did you mean "The same buffer is re-used for all tests"?

> + *
> + * "If <target> is EGL_LINUX_DMA_BUF_EXT and eglCreateImageKHR fails,
> + *  EGL does not retain ownership of the file descriptor and it is the
> + *  responsibility of the application to close it."

Again, I don't understand why the fd comment is here, so it feels like noise
to me. Please explain.

> + */
> +enum piglit_result
> +piglit_display(void)

> diff --git a/tests/spec/ext_image_dma_buf_import/invalid_hints.c b/tests/spec/ext_image_dma_buf_import/invalid_hints.c

> +/**
> + * On re-uses the buffer for all the tests. The spec says:
> + *
> + * "If <target> is EGL_LINUX_DMA_BUF_EXT and eglCreateImageKHR fails,
> + *  EGL does not retain ownership of the file descriptor and it is the
> + *  responsibility of the application to close it."

Same questions as above.

> + */
> +enum piglit_result
> +piglit_display(void)

> diff --git a/tests/spec/ext_image_dma_buf_import/missing_attributes.c b/tests/spec/ext_image_dma_buf_import/missing_attributes.c

> +static bool
> +test_all(int fd, unsigned w, unsigned h, unsigned stride, unsigned offset)
> +{
> +	/* there are six mandatory attributes */

It took me a minute to see that this array did. A comment here would be nice
that quickly explains that each array has exactly one mandatory attribute
removed.

> +	const EGLint missing_attributes[][2 * 5 + 1] = {
> +		{ EGL_HEIGHT, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_NONE },
> +		{ EGL_WIDTH, w,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_NONE },
> +		{ EGL_WIDTH, w,
> +		  EGL_HEIGHT, h,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_NONE },
> +		{ EGL_WIDTH, w,
> +		  EGL_HEIGHT, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_NONE },
> +		{ EGL_WIDTH, w,
> +		  EGL_HEIGHT, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> +		  EGL_NONE },
> +		{ EGL_WIDTH, w,
> +		  EGL_HEIGHT, h,
> +		  EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +		  EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> +		  EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> +		  EGL_NONE },
> +	};
> +	bool pass = true;
> +	unsigned i;
> +
> +	for (i = 0; i < sizeof(missing_attributes) /
> +			sizeof(missing_attributes[0]); ++i) {

ARRAY_SIZE

> +		pass &= test_missing(fd, missing_attributes[i]);
> +	}
> +
> +	return pass;
> +}
> +
> +/**
> + * On re-uses the buffer for all the tests. The spec says:
> + *
> + * "If <target> is EGL_LINUX_DMA_BUF_EXT and eglCreateImageKHR fails,
> + *  EGL does not retain ownership of the file descriptor and it is the
> + *  responsibility of the application to close it."

Again, same questions as above.

> + */
> +enum piglit_result
> +piglit_display(void)