[Piglit] [PATCH 1/4] ATI_fs: add api error tests
Miklós Máté
mtmkls at gmail.com
Wed Nov 22 00:02:11 UTC 2017
On 21/11/17 21:09, Eric Anholt wrote:
> 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.
Thanks for the review. I'll refine the code accordingly.
>
> 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.
This is just a preview of my work, in the final version I'll add the
tests to all.py. I'll also remove your existing ati-fs-bad-delete test,
because it's a subset of my api-gen test :)
>
>> 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.
OK, I'll use that license.
>
>> +
>> +/**
>> + * 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.
Good idea, I'll do that.
>
>> +
>> +#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.
First I did that, but I had a difficult time finding out where Mesa
failed to throw the expected error, so I made the check fatal. If people
prefer non-fatal checks I can change it back.
>
>> +
>> + 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.
As far as I understand the errors are pushed into a stack. I do these no
error checks to make sure the stack is empty before the critical call.
If it's not possible for one gl call to push more than one error into
the stack, then the no error checks can indeed be trimmed down in this test.
>
>> + 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?
I guess this is a hw limitation on r200 that the spec fails to mention.
I don't have such hw for testing.
The spec says "Either one or two passes may be specified in a shader.
Each pass may use up to 8 pairs of instructions ...". It doesn't say
anything about the minimum amount of instructions. In my reading this:
glBeginFragmentShaderATI();
glEndFragmentShaderATI();
is invalid, because it's zero passes, but this:
glBeginFragmentShaderATI();
glSampleMapATI(GL_REG_0_ATI, GL_TEXTURE0_ARB, GL_SWIZZLE_STR_ATI);
glEndFragmentShaderATI();
is a valid one pass shader. If someone can confirm this on an r200, I'll
gladly remove the check in mesa and the workaround in the tests.
>
> 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.
>
I'll do those and resend.
MM
More information about the Piglit
mailing list