[Piglit] [v5 05/12] tests: spec: EXT_image_dma_buf_import invalid attributes

[Eric Anholt]

Topi Pohjolainen <topi.pohjolainen at intel.com> writes:
> diff --git a/tests/spec/ext_image_dma_buf_import/invalid_attributes.c b/tests/spec/ext_image_dma_buf_import/invalid_attributes.c
> new file mode 100644
> index 0000000..a6a6954
> --- /dev/null
> +++ b/tests/spec/ext_image_dma_buf_import/invalid_attributes.c
> @@ -0,0 +1,286 @@
> +/*
> + * 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>
> +#include "ext_image_dma_buf_fourcc.h"
> +
> +/**
> + * @file invalid_attributes.c
> + *
> + * From the EXT_image_dma_buf_import spec:
> + *
> + * "* If <target> is EGL_LINUX_DMA_BUF_EXT and <buffer> is not NULL, the
> + *    error EGL_BAD_PARAMETER is generated.
> + *
> + *  * 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.
> + *
> + *  * 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.
> + *
> + *  * 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."
> + */
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +	config.supports_gl_es_version = 10;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +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 },
> +	};

I think this particular subtest could be better done like the
test_invalid_hints, where a single attribute as a function parameter is
plugged into the stock array. It would clarify what's going on in the
test, and then you could easily do PLANE2_* checks, too.

> +	for (i = 0; i < ARRAY_SIZE(excess_attributes); ++i) {
> +		/**
> +		 * 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."
> +		 */

Please remove this copypasta from the eglCreateImageKHR callsites that
aren't testing this particular spec text.

> +		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_invalid_context(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(), eglGetCurrentContext(),
> +			EGL_LINUX_DMA_BUF_EXT, (EGLClientBuffer)NULL, attr);
> +
> +	if (!piglit_check_egl_error(EGL_BAD_CONTEXT)) {
> +		if (img)
> +			eglDestroyImageKHR(eglGetCurrentDisplay(), img);
> +		return false;
> +	}

It's not explicitly called out in the spec, but I think this error case
would be EGL_BAD_PARAMETER like the other bad parameter cases for this
call.

> +
> +/**
> + * 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.
> + */

"One re-uses" reads awkwardly.  I think "We reuse" would be better.

> +enum piglit_result
> +piglit_display(void)
> +{
> +	const unsigned char pixels[2 * 2 * 4];
> +	struct piglit_dma_buf *buf;
> +	unsigned stride;
> +	unsigned offset;
> +	int fd;
> +	enum piglit_result res;
> +	bool pass;
> +
> +	res = piglit_create_dma_buf(2, 2, 4, pixels, 2 * 4, &buf, &fd, &stride,
> +				&offset);
> +	if (res != PIGLIT_PASS)
> +		return res;
> +
> +	pass = test_excess_attributes(fd, 2, 2, 2 * 4, 0);
> +	pass = test_buffer_not_null(fd, 2, 2, 2 * 4, 0) && pass;
> +	pass = test_invalid_context(fd, 2, 2, 2 * 4, 0) && pass;
> +	pass = test_invalid_format(fd, 2, 2, 2 * 4, 0) && pass;
> +	pass = test_pitch_zero(fd, 2, 2, 2 * 4, 0) && pass;

You should pass in the allocated stride from the piglit_create_dma_buf,
or for an implementation that aligns stride, you'll get BAD_ACCESS
errors by ignoring it.  Also, please make variables "w" and "h" and use
them so I don't have to see this wall of 2s and 4s all over.  The "4"
parameter in create_dma_buf was not obvious what it was doing (cpp, not
size)
-------------- 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/20130503/c841bef2/attachment.pgp>