[Piglit] [PATCH 1/4] ATI_fs: add api error tests
Eric Anholt
eric at anholt.net
Tue Nov 21 20:09:31 UTC 2017
Miklós Máté <mtmkls at gmail.com> writes:
> One for each paragraph in the Errors section of the spec.
>
> Signed-off-by: Miklós Máté <mtmkls at gmail.com>
> ---
> tests/spec/ati_fragment_shader/error01-genzero.c | 51 ++++
> tests/spec/ati_fragment_shader/error02-inside.c | 59 +++++
> tests/spec/ati_fragment_shader/error03-outside.c | 89 +++++++
> tests/spec/ati_fragment_shader/error04-endshader.c | 93 ++++++++
> tests/spec/ati_fragment_shader/error05-passes.c | 123 ++++++++++
> .../spec/ati_fragment_shader/error06-regswizzle.c | 263 +++++++++++++++++++++
> tests/spec/ati_fragment_shader/error07-instcount.c | 89 +++++++
> tests/spec/ati_fragment_shader/error08-secondary.c | 82 +++++++
> tests/spec/ati_fragment_shader/error09-allconst.c | 78 ++++++
> tests/spec/ati_fragment_shader/error10-dotx.c | 115 +++++++++
> .../spec/ati_fragment_shader/error11-invaliddst.c | 171 ++++++++++++++
> .../spec/ati_fragment_shader/error12-invalidsrc.c | 151 ++++++++++++
> .../spec/ati_fragment_shader/error13-invalidarg.c | 121 ++++++++++
> .../spec/ati_fragment_shader/error14-invalidmod.c | 129 ++++++++++
We tend to implement these API error tests as a single .c file. As
you've experienced, this is a whole lot of copy and pasting, and it's
usually not too big of a deal to get the implementation to pass all of
them.
If it *is* hard to get the implementation to pass all of them, you may
look into piglit_report_subtest_result().
That said, there are some sizeable tests here and some nice usage of
subtests in a few of them, so I think you've generally made the right
choice.
Also, don't forget to add your tests to all.py! If you don't, your
tests won't be run in everyone's regression testing. Also, the build
system for the tests should be in the same commit as the tests
themselves.
> diff --git a/tests/spec/ati_fragment_shader/error01-genzero.c b/tests/spec/ati_fragment_shader/error01-genzero.c
> new file mode 100644
> index 000000000..a19fb2ffc
> --- /dev/null
> +++ b/tests/spec/ati_fragment_shader/error01-genzero.c
> @@ -0,0 +1,51 @@
> +/* TODO license header */
You can see a sample of the license to use in the COPYING file -- just
copy and paste and change the date and "Intel Corporation" to your name.
> +
> +/**
> + * Tests API errors for GL_ATI_fragment_shader.
> + * One for each paragraph in the Errors section.
> + */
I'd move the comment from piglit_init() to here.
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + /* UNREACHED */
> + return PIGLIT_FAIL;
> +}
> +
> +#define check_gl_error(err) if (!piglit_check_gl_error(err)) piglit_report_result(PIGLIT_FAIL)
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + unsigned id = 42;
Unnecessary initializer.
> + piglit_require_extension("GL_ATI_fragment_shader");
> +
> + /*
> + * Paragraph 1 of the Errors section:
> + *
> + * The error INVALID_VALUE is generated if GenFragmentShadersATI is
> + * called where <range> is zero.
> + */
> +
> + id = glGenFragmentShadersATI(0);
> + check_gl_error(GL_INVALID_VALUE);
> +
> + /* The spec also says that 0 is returned */
> + if (id != 0)
> + piglit_report_result(PIGLIT_FAIL);
> +
> + /* Note that the spec also says that no shaders are generated,
> + * but there is no way of testing that */
Our general style is that the closing */ of a multi-line comment goes on
its own line.
> +
> + piglit_report_result(PIGLIT_PASS);
> +}
> diff --git a/tests/spec/ati_fragment_shader/error02-inside.c b/tests/spec/ati_fragment_shader/error02-inside.c
> new file mode 100644
> index 000000000..5dee70cf6
> --- /dev/null
> +++ b/tests/spec/ati_fragment_shader/error02-inside.c
> @@ -0,0 +1,59 @@
> +/* TODO license header */
> +
> +/**
> + * Tests API errors for GL_ATI_fragment_shader.
> + * One for each paragraph in the Errors section.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + /* UNREACHED */
> + return PIGLIT_FAIL;
> +}
> +
> +#define check_gl_error(err) if (!piglit_check_gl_error(err)) piglit_report_result(PIGLIT_FAIL)
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + piglit_require_extension("GL_ATI_fragment_shader");
> +
> + /*
> + * Paragraph 2 of the Errors section:
> + *
> + * The error INVALID_OPERATION is generated if GenFragmentShadersATI,
> + * BindFragmentShaderATI, DeleteFragmentShaderATI, or
> + * BeginFragmentShaderATI are specified inside a
> + * Begin/EndFragmentShaderATI pair.
> + */
> +
> + glBeginFragmentShaderATI();
> +
> + check_gl_error(GL_NO_ERROR);
> + glGenFragmentShadersATI(1);
> + check_gl_error(GL_INVALID_OPERATION);
Instead of the macro, we'll often have
bool pass = true;
...
pass = piglit_check_gl_error(GL_NO_ERROR) && pass
glGenFragmentShadersATI(1);
pass = piglit_check_gl_error(GL_INVALID_OPERATION) && pass
...
piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
That way you get to see multiple errors if there are different cases
failing.
> +
> + check_gl_error(GL_NO_ERROR);
I'd drop the GL_NO_ERROR checks -- you know there's no error because the
last thing you did is check the errors.
> + glBindFragmentShaderATI(2);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glDeleteFragmentShaderATI(3);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glBeginFragmentShaderATI();
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + piglit_report_result(PIGLIT_PASS);
> +}
> diff --git a/tests/spec/ati_fragment_shader/error03-outside.c b/tests/spec/ati_fragment_shader/error03-outside.c
> new file mode 100644
> index 000000000..9a366c212
> --- /dev/null
> +++ b/tests/spec/ati_fragment_shader/error03-outside.c
> @@ -0,0 +1,89 @@
> +/* TODO license header */
> +
> +/**
> + * Tests API errors for GL_ATI_fragment_shader.
> + * One for each paragraph in the Errors section.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + /* UNREACHED */
> + return PIGLIT_FAIL;
> +}
> +
> +#define check_gl_error(err) if (!piglit_check_gl_error(err)) piglit_report_result(PIGLIT_FAIL)
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + piglit_require_extension("GL_ATI_fragment_shader");
> +
> + /*
> + * Paragraph 3 of the Errors section:
> + *
> + * The error INVALID_OPERATION is generated if EndFragmentShaderATI,
> + * PassTexCoordATI, SampleMapATI, ColorFragmentOp[1..3]ATI, or
> + * AlphaFragmentOp[1..3]ATI is specified outside a
> + * Begin/EndFragmentShaderATI pair.
> + */
> +
> + check_gl_error(GL_NO_ERROR);
> + glEndFragmentShaderATI();
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glPassTexCoordATI(GL_REG_0_ATI, GL_TEXTURE0_ARB, GL_SWIZZLE_STR_ATI);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glSampleMapATI(GL_REG_0_ATI, GL_TEXTURE1_ARB, GL_SWIZZLE_STR_ATI);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glColorFragmentOp1ATI(GL_MOV_ATI, GL_REG_0_ATI, GL_NONE, GL_NONE,
> + GL_REG_1_ATI, GL_NONE, GL_NONE);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glColorFragmentOp2ATI(GL_ADD_ATI, GL_REG_0_ATI, GL_NONE, GL_NONE,
> + GL_REG_1_ATI, GL_NONE, GL_NONE,
> + GL_REG_2_ATI, GL_NONE, GL_NONE);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glColorFragmentOp3ATI(GL_MAD_ATI, GL_REG_0_ATI, GL_NONE, GL_NONE,
> + GL_REG_1_ATI, GL_NONE, GL_NONE,
> + GL_REG_2_ATI, GL_NONE, GL_NONE,
> + GL_REG_3_ATI, GL_NONE, GL_NONE);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glAlphaFragmentOp1ATI(GL_MOV_ATI, GL_REG_0_ATI, GL_NONE,
> + GL_REG_1_ATI, GL_NONE, GL_NONE);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glAlphaFragmentOp2ATI(GL_ADD_ATI, GL_REG_0_ATI, GL_NONE,
> + GL_REG_1_ATI, GL_NONE, GL_NONE,
> + GL_REG_2_ATI, GL_NONE, GL_NONE);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + check_gl_error(GL_NO_ERROR);
> + glAlphaFragmentOp3ATI(GL_MAD_ATI, GL_REG_0_ATI, GL_NONE,
> + GL_REG_1_ATI, GL_NONE, GL_NONE,
> + GL_REG_2_ATI, GL_NONE, GL_NONE,
> + GL_REG_3_ATI, GL_NONE, GL_NONE);
> + check_gl_error(GL_INVALID_OPERATION);
> +
> + piglit_report_result(PIGLIT_PASS);
> +}
> diff --git a/tests/spec/ati_fragment_shader/error04-endshader.c b/tests/spec/ati_fragment_shader/error04-endshader.c
> new file mode 100644
> index 000000000..a0801ff86
> --- /dev/null
> +++ b/tests/spec/ati_fragment_shader/error04-endshader.c
> @@ -0,0 +1,93 @@
> +/* TODO license header */
> +
> +/**
> + * Tests API errors for GL_ATI_fragment_shader.
> + * One for each paragraph in the Errors section.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + /* UNREACHED */
> + return PIGLIT_FAIL;
> +}
> +
> +#define check_gl_error(err) if (!piglit_check_gl_error(err)) piglit_report_result(PIGLIT_FAIL)
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + piglit_require_extension("GL_ATI_fragment_shader");
> +
> + /*
> + * Paragraph 4 of the Errors section:
> + *
> + * The error INVALID_OPERATION is generated by EndFragmentShaderATI if
> + * <argN> passed to ColorFragmentOp[1..3]ATI or
> + * AlphaFragmentOp[1..3]ATI is PRIMARY_COLOR_ARB or
> + * SECONDARY_INTERPOLATOR_ATI on the first pass of a two-pass shader,
> + * or if the shader cannot be compiled due to some other
> + * implementation-dependent limitation. EndFragmentShaderATI will
> + * still have a side-effect if this error is encountered: the
> + * Begin/EndFragmentShaderATI pair will be closed, and the current
> + * shader will be undefined.
> + */
> +
> + /* use GL_PRIMARY_COLOR_ARB in the first pass */
> + glBeginFragmentShaderATI();
> + glColorFragmentOp1ATI(GL_MOV_ATI, GL_REG_0_ATI, GL_NONE, GL_NONE,
> + GL_PRIMARY_COLOR_ARB, GL_NONE, GL_NONE);
> + glPassTexCoordATI(GL_REG_0_ATI, GL_REG_0_ATI, GL_SWIZZLE_STR_ATI);
> + /* note: Mesa requires at least 1 arith instruction per pass,
> + * but this is not in the spec */
Is this something we can just fix in Mesa instead of sprinkling
workarounds all over the tests? Or is this something that we think the
spec intended?
In summary, things I'd like to see to review and apply this patch:
- License headers
- Build system
- all.py
- Drop redundant no-error checks.
- Per-file top comment replaced with the comment of the spec requirement
being tested.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20171121/e5a99310/attachment.sig>
More information about the Piglit
mailing list