[Piglit] [v8 10/13] tests/spec: EXT_image_dma_buf_import sample xrgb

[Chad Versace]

On 08/01/2013 10:45 PM, Pohjolainen, Topi wrote:
> On Wed, Jul 31, 2013 at 04:10:22PM -0700, Chad Versace wrote:
>> On 07/10/2013 01:24 AM, Topi Pohjolainen wrote:
>>> v2:
>>>     - compile only on platforms that have drm (Eric)
>>>     - use standard drm definitions for fourcc instead of duplicated
>>>       local (Daniel, Eric)
>>>     - use helper variables for width, height and cpp instead
>>>       of repeating the magic numbers over and over again (Eric)
>>>     - removed irrelevant quote of the spec (Eric)
>>>     - used the same refactored common logic as the ARGB test
>>>
>>> v3 (Eric):
>>>     - use properly linked egl-extension calls
>>>
>>> v4 (Eric):
>>>     - add to 'all.tests'
>>>     - removed inclusion of standard EGL entrypoints as there is the
>>>       special dispatcher for the test cases to use
>>>
>>> v5 (Eric, Chad):
>>>     - use image external extension for sampling
>>>     - add into all.tests
>>>
>>> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>>> ---
>>>   tests/all.tests                                    |  1 +
>>>   .../ext_image_dma_buf_import/CMakeLists.gles2.txt  |  1 +
>>>   .../ext_image_dma_buf_import/sample_xrgb8888.c     | 69 ++++++++++++++++++++++
>>>   3 files changed, 71 insertions(+)
>>>   create mode 100644 tests/spec/ext_image_dma_buf_import/sample_xrgb8888.c
>>>
>>> diff --git a/tests/all.tests b/tests/all.tests
>>> index c2e0626..405596b 100644
>>> --- a/tests/all.tests
>>> +++ b/tests/all.tests
>>> @@ -1743,6 +1743,7 @@ add_plain_test(ext_image_dma_buf_import, 'ext_image_dma_buf_import-invalid_attri
>>>   add_plain_test(ext_image_dma_buf_import, 'ext_image_dma_buf_import-missing_attributes')
>>>   add_plain_test(ext_image_dma_buf_import, 'ext_image_dma_buf_import-ownership_transfer')
>>>   add_plain_test(ext_image_dma_buf_import, 'ext_image_dma_buf_import-sample_argb8888')
>>> +add_plain_test(ext_image_dma_buf_import, 'ext_image_dma_buf_import-sample_xrgb8888')
>>>
>>>   ext_packed_depth_stencil = Group()
>>>   spec['EXT_packed_depth_stencil'] = ext_packed_depth_stencil
>>> diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt
>>> index 3185bd4..5fc6031 100644
>>> --- a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt
>>> +++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt
>>> @@ -18,6 +18,7 @@ if(LIBDRM_FOUND)
>>>   	)
>>>
>>>   	piglit_add_executable(ext_image_dma_buf_import-sample_argb8888 sample_argb8888.c sample_common.c image_common.c)
>>> +	piglit_add_executable(ext_image_dma_buf_import-sample_xrgb8888 sample_xrgb8888.c sample_common.c image_common.c)
>>>   endif(LIBDRM_FOUND)
>>>
>>>   # vim: ft=cmake:
>>> diff --git a/tests/spec/ext_image_dma_buf_import/sample_xrgb8888.c b/tests/spec/ext_image_dma_buf_import/sample_xrgb8888.c
>>> new file mode 100644
>>> index 0000000..43415ae
>>> --- /dev/null
>>> +++ b/tests/spec/ext_image_dma_buf_import/sample_xrgb8888.c
>>> @@ -0,0 +1,69 @@
>>> +/*
>>> + * 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"
>>> +#include <unistd.h>
>>> +#include <drm_fourcc.h>
>>> +#include "sample_common.h"
>>> +#include "image_common.h"
>>> +
>>> +/**
>>> + * @file sample_xrgb8888.c
>>> + *
>>> + * Create EGL image out of XRGB8888 formatted dma buffer, set it as external
>>> + * texture, set texture filters for avoiding need for other mipmap-levels and
>>> + * sample it using a shader program. Additionally check that alpha channel is
>>> + * set to one as specified in OES_EGL_image_external:
>>> + *
>>> + * "If the EGLImage associated with the external texture contains alpha
>>> + *  values then the value of the alpha component returned is taken from
>>> + *  the image; otherwise the alpha component is 1.0."
>>> + */
>>> +
>>> +PIGLIT_GL_TEST_CONFIG_BEGIN
>>> +
>>> +	config.supports_gl_es_version = 20;
>>> +
>>> +PIGLIT_GL_TEST_CONFIG_END
>>> +
>>> +enum piglit_result
>>> +piglit_display(void)
>>> +{
>>> +	const unsigned char src[] = {
>>> +		10, 20, 30, 40, 50, 60, 70, 80,
>>> +		11, 22, 33, 44, 55, 66, 77, 88 };
>>> +	enum piglit_result res = dma_buf_create_and_sample_32bpp(
>>> +					2, 2, 4, DRM_FORMAT_XRGB8888, src);
>>> +
>>> +	if (res != PIGLIT_PASS)
>>> +		return res;
>>> +
>>> +	return probe_rect_argb(2, 2, src, true);
>>> +}
>>> +
>>> +void
>>> +piglit_init(int argc, char **argv)
>>> +{
>>> +	piglit_require_egl_extension("EGL_EXT_image_dma_buf_import");
>>> +	piglit_require_extension("GL_OES_EGL_image_external");
>>> +}
>>>
>>
>> This test is extremely similar to the test in patch 9. The only differences
>> are the DRM_FORMAT_XRGB888 and that it checks alpha == 1.
>>
>> When tests differ only by formats, then a common convention in Piglit is to
>> fold all such tests into one excutable that accepts the format as a command
>> line parameter. The cumulative benefit of doing this throughout Piglit is
>> that it diminishes Piglit's build time.
>>
>> Please fold this test into patch 9. If I recall correctly, the Piglit framework
>> strips all common command line args such as '-fbo' and '-auto' out of argv before
>> passing argv to piglit_init().
>
> It would in fact require two arguments, one for format and another boolean for
> telling if alpha channel is expected to be forced to 1.0. Would that be okay?

The spec says that alpha gets forced to 1.0 if the format has no alpha channel.
Since that is dictated by the spec, then I think the test only needs one argument,
the format, because force_alpha can be deduced from the format. At least, I
think that should work. Am I missing something?