[Piglit] [v4 4/4] tests: spec: tests for EXT_image_dma_buf_import

Eric Anholt eric at anholt.net
Thu May 2 16:57:49 PDT 2013


Topi Pohjolainen <topi.pohjolainen at intel.com> writes:

This is too massive of a patch.  For your reviewers going through
200-line tests, it would be really good to have a test per patch so that
they can incrementally review things and check them off their lists.

> These are rather simple. Without the capability of creating
> textures out of the images (glEGLImageTargetTexture2DOES()), one
> cannot do much with the images as such.
>
> In the tests I've chosen the approach where the creator releases
> its own references to the buffers even though the EGL stack closes
> the file descriptors used for buffer exporting.
>
> v2:
>    - use 'struct piglit_dma_buf *' instead of 'void *'
>    - use ARRAY_SIZE
>    - clarified the contents of the missing attributes test vector
>    - clarified the re-using of the buffer in case of:
> 	tests/spec/ext_image_dma_buf_import/invalid_attributes.c
> 	tests/spec/ext_image_dma_buf_import/invalid_hints.c
> 	tests/spec/ext_image_dma_buf_import/missing_attributes.c
>
>         Old text:
> 	  * "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."
>
> 	New text:
> 	  * One re-uses the buffer for all the tests. Each test is expected to
> 	  * fail meaning that the ownership is not transferred to the EGL in
> 	  * any point.
>
>    - clarified the 'close_buffer'-test by:
>      "Here on checks that the creator of the buffer can drop its reference once
>       it has given the buffer to EGL, i.e., after calling 'eglCreateImageKHR()'"
g>
>      The test also now checks that 'eglDestroyImageKHR()' does not generate any
>      error.
>
>    - added test for context other than 'EGL_NO_CONTEXT'
>    - added test for sampling ARGB8888
>    - added test for sampling XRGB8888
>    - added test for sampling ARGB8888 without other mip-levels being present
>
> v3:
>    - added test for YUV420 having all planes in the same buffer
>    - modified the tests to close the file descriptors whenever EGL does not
>      take the ownership
>
> v4:
>    - fix the checking for the extension (EGL, not GL)
>    - include declaration for 'close()' (unistd.h)
>
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  tests/spec/CMakeLists.txt                          |   1 +
>  .../ext_image_dma_buf_import/CMakeLists.gles1.txt  |  20 ++
>  .../ext_image_dma_buf_import/CMakeLists.gles2.txt  |  16 ++
>  tests/spec/ext_image_dma_buf_import/CMakeLists.txt |   1 +
>  tests/spec/ext_image_dma_buf_import/close_buffer.c | 120 +++++++++
>  .../spec/ext_image_dma_buf_import/create_yuv420.c  | 157 +++++++++++
>  .../create_yuv420_same_fd.c                        | 138 ++++++++++
>  .../ext_image_dma_buf_import/invalid_attributes.c  | 290 +++++++++++++++++++++
>  .../spec/ext_image_dma_buf_import/invalid_hints.c  | 145 +++++++++++
>  .../ext_image_dma_buf_import/missing_attributes.c  | 186 +++++++++++++
>  .../ext_image_dma_buf_import/sample_argb8888.c     | 220 ++++++++++++++++
>  .../sample_argb8888_level_zero_only.c              | 218 ++++++++++++++++
>  .../ext_image_dma_buf_import/sample_xrgb8888.c     | 221 ++++++++++++++++
>  13 files changed, 1733 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.gles2.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/create_yuv420_same_fd.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
>  create mode 100644 tests/spec/ext_image_dma_buf_import/sample_argb8888.c
>  create mode 100644 tests/spec/ext_image_dma_buf_import/sample_argb8888_level_zero_only.c
>  create mode 100644 tests/spec/ext_image_dma_buf_import/sample_xrgb8888.c
>
> diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt
> index a1492cc..56c0111 100644
> --- a/tests/spec/CMakeLists.txt
> +++ b/tests/spec/CMakeLists.txt
> @@ -68,6 +68,7 @@ add_subdirectory (ext_texture_array)
>  add_subdirectory (ext_texture_integer)
>  add_subdirectory (arb_draw_buffers)
>  add_subdirectory (oes_draw_texture)
> +add_subdirectory (ext_image_dma_buf_import)
>  add_subdirectory (arb_blend_func_extended)
>  add_subdirectory (ext_unpack_subimage)
>  add_subdirectory (arb_vertex_array_object)
> diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
> new file mode 100644
> index 0000000..8cfc90a
> --- /dev/null
> +++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
> @@ -0,0 +1,20 @@
> +#add_definitions(-DSOURCE_DIR="${piglit_SOURCE_DIR}/")
> +
> +include_directories(
> +	${OPENGL_INCLUDE_PATH}
> +	)
> +
> +link_libraries(
> +	${OPENGL_gles1_LIBRARY}
> +	${OPENGL_egl_LIBRARY}
> +	piglitutil_gles1
> +	)
> +
> +piglit_add_executable(ext_image_dma_buf_import-invalid_hints invalid_hints.c)
> +piglit_add_executable(ext_image_dma_buf_import-invalid_attributes invalid_attributes.c)
> +piglit_add_executable(ext_image_dma_buf_import-missing_attributes missing_attributes.c)
> +piglit_add_executable(ext_image_dma_buf_import-close_buffer close_buffer.c)
> +piglit_add_executable(ext_image_dma_buf_import-create_yuv420 create_yuv420.c)
> +piglit_add_executable(ext_image_dma_buf_import-create_yuv420_same_fd create_yuv420_same_fd.c)
> +
> +# vim: ft=cmake:
> diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt
> new file mode 100644
> index 0000000..f60afb6
> --- /dev/null
> +++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt
> @@ -0,0 +1,16 @@
> +#add_definitions(-DSOURCE_DIR="${piglit_SOURCE_DIR}/")
> +
> +include_directories(
> +	${OPENGL_INCLUDE_PATH}
> +	)
> +
> +link_libraries(
> +	${OPENGL_egl_LIBRARY}
> +	piglitutil_gles2
> +	)
> +
> +piglit_add_executable(ext_image_dma_buf_import-sample_argb8888 sample_argb8888.c)
> +piglit_add_executable(ext_image_dma_buf_import-sample_xrgb8888 sample_xrgb8888.c)
> +piglit_add_executable(ext_image_dma_buf_import-sample_argb8888_level_zero_only sample_argb8888_level_zero_only.c)
> +
> +# vim: ft=cmake:
> diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.txt b/tests/spec/ext_image_dma_buf_import/CMakeLists.txt
> new file mode 100644
> index 0000000..144a306
> --- /dev/null
> +++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.txt
> @@ -0,0 +1 @@
> +piglit_include_target_api()
> diff --git a/tests/spec/ext_image_dma_buf_import/close_buffer.c b/tests/spec/ext_image_dma_buf_import/close_buffer.c
> new file mode 100644
> index 0000000..df421f0
> --- /dev/null
> +++ b/tests/spec/ext_image_dma_buf_import/close_buffer.c
> @@ -0,0 +1,120 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "piglit-util-egl.h"
> +#define EGL_EGLEXT_PROTOTYPES 1
> +#include <EGL/eglext.h>
> +
> +/**
> + * @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."
> + *
> + *
> + * Here on checks that the creator of the buffer can drop its reference once
> + * it has given the buffer to EGL, i.e., after calling 'eglCreateImageKHR()'.

I can't parse "here on checks".  "This test checks"?

> +#define fourcc_code(a,b,c,d) ((uint32_t)(a) | ((uint32_t)(b) << 8) | \
> +				((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))
> +#define DRM_FORMAT_ARGB8888 fourcc_code('A', 'R', '2', '4')

This is in many subtests, and should pretty clearly be in a header.

> +static bool
> +test_create_and_destroy(void *buf, 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)0, attr);

Actually use NULL for consistency with the quoted spec?

> +	/**
> +	 * Release the creator side of the buffer, EGL should have the
> +	 * ownership now.
> +	 */
> +	piglit_destroy_dma_buf(buf);
> +	if (!piglit_check_egl_error(EGL_SUCCESS))
> +		return false;
> +
> +	if (!img) {
> +		fprintf(stderr, "image creation succeed but returned NULL\n");
> +		return false;
> +	}
> +
> +	eglDestroyImageKHR(eglGetCurrentDisplay(), img);
> +
> +	return piglit_check_egl_error(EGL_SUCCESS);

I don't think this test has actually tested what it set out to,
specifically the text that "If eglCreateImageKHR is successful, EGL
assumes ownership of the file descriptors and is responsible for closing
them".  To do so, I think after the destroy you want to try to close(fd)
and check for EBADF (though apparently it may not necessarily happen
until the destroy of the EGL display).

Also, this test would be cleaned up by merging this test function into
the piglit_display below.

> diff --git a/tests/spec/ext_image_dma_buf_import/create_yuv420.c b/tests/spec/ext_image_dma_buf_import/create_yuv420.c
> new file mode 100644
> index 0000000..aa148b8
> --- /dev/null
> +++ b/tests/spec/ext_image_dma_buf_import/create_yuv420.c
> @@ -0,0 +1,157 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "piglit-util-egl.h"
> +#define EGL_EGLEXT_PROTOTYPES 1
> +#include <EGL/eglext.h>
> +#include <unistd.h>
> +
> +/**
> + * @file create_yuv420.c
> + *
> + * Allocates three dma buffers from via drm and creates an YUV420 formatted EGL
> + * image each buffer representing one plane.
> + */
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +	config.supports_gl_es_version = 10;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +#define fourcc_code(a,b,c,d) ((uint32_t)(a) | ((uint32_t)(b) << 8) | \
> +				((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))
> +#define DRM_FORMAT_YUV420 fourcc_code('Y', 'U', '1', '2')
> +
> +static bool
> +test_create_and_destroy(unsigned w, unsigned h,
> +			void *buf0, void *buf1, void *buf2,
> +			int fd0, int fd1, int fd2,
> +			unsigned stride0, unsigned stride1, unsigned stride2,
> +			unsigned offset0, unsigned offset1, unsigned offset2)
> +{
> +	EGLImageKHR img;
> +	EGLint attr[] = {
> +		EGL_WIDTH, w,
> +		EGL_HEIGHT, h,
> +		EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_YUV420,
> +		EGL_DMA_BUF_PLANE0_FD_EXT, fd0,
> +		EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset0,
> +		EGL_DMA_BUF_PLANE0_PITCH_EXT, stride0,
> +		EGL_DMA_BUF_PLANE1_FD_EXT, fd1,
> +		EGL_DMA_BUF_PLANE1_OFFSET_EXT, offset1,
> +		EGL_DMA_BUF_PLANE1_PITCH_EXT, stride1,
> +		EGL_DMA_BUF_PLANE2_FD_EXT, fd2,
> +		EGL_DMA_BUF_PLANE2_OFFSET_EXT, offset2,
> +		EGL_DMA_BUF_PLANE2_PITCH_EXT, stride2,
> +		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)0, attr);

I don't think every test needs the comment about how to call the
function.  Usually we have a comment like this when testing the
exceptions, like an invalid display or a real EGL context or a non-null
buffer.

I'm going to stop here -- this is too much in a single commit, and
hopefully once you get them split out appropriately we can get things
reviewed and merged.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130502/472471b4/attachment.pgp>


More information about the Piglit mailing list