[Piglit] [PATCH] ARB_draw_indirect: add initial tests
Eric Anholt
eric at anholt.net
Wed May 8 16:16:59 PDT 2013
Christoph Bumiller <christoph.bumiller at speed.at> writes:
> From: Christoph Bumiller <e0425955 at student.tuwien.ac.at>
>
> Is that sufficient to get anyone looking at my mesa patches
> (I've rebased them already) or do I need to make more/nicer
> tests ? How many ?
> If I'd get paid anything for this I'd gladly spend my time writing
> loads of tests, it's rather relaxing, compared to reverse engineering.
I've put this off for a long time because reviewing a giant commit of
1100 lines all at once means I never want to start. Splitting your new
tests up into separate commits can help that, and as you get reviews you
can get them pushed.
I do like how much coverage there is of the extension here: Almost all
of the error cases, all of the draw entrypoints, and some interesting
interactions with other extensions (prim restart).
> ---
> tests/spec/CMakeLists.txt | 1 +
> tests/spec/arb_draw_indirect/CMakeLists.gl.txt | 19 +
> tests/spec/arb_draw_indirect/CMakeLists.txt | 1 +
> tests/spec/arb_draw_indirect/draw-arrays-multi.c | 66 ++++
> .../arb_draw_indirect/draw-arrays-prim-restart.c | 141 +++++++
> tests/spec/arb_draw_indirect/draw-arrays.c | 67 ++++
> tests/spec/arb_draw_indirect/draw-common.c | 408 ++++++++++++++++++++
> tests/spec/arb_draw_indirect/draw-elements-multi.c | 66 ++++
> tests/spec/arb_draw_indirect/draw-elements.c | 67 ++++
> tests/spec/arb_draw_indirect/errors-multi.c | 140 +++++++
> tests/spec/arb_draw_indirect/errors.c | 159 ++++++++
The multi tests ought to go under spec/GL_ARB_multi_draw_indirect. Oh,
but that's a build system nightmare with the shared code. Maybe just
put them in all.tests under that group and leave them built from this
directory?
Also, all.tests additions are missing, so your tests won't actually get
run.
> diff --git a/tests/spec/arb_draw_indirect/CMakeLists.txt b/tests/spec/arb_draw_indirect/CMakeLists.txt
> new file mode 100644
> index 0000000..144a306
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/CMakeLists.txt
> @@ -0,0 +1 @@
> +piglit_include_target_api()
> diff --git a/tests/spec/arb_draw_indirect/draw-arrays-multi.c b/tests/spec/arb_draw_indirect/draw-arrays-multi.c
> new file mode 100644
> index 0000000..b8dd1da
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/draw-arrays-multi.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright © The Piglit project 2012
If you wrote the code, it should be copyright you. I've had advice from
an armchair lawyer I respect that if you don't, it can complicate things
for you if you ever need to enforce your copyright. Of course, this is
all MIT so we don't ever expect to actually enforce anything because
there's so little to enforce, but let's do it right anyway.
(There's some of this that has been copied around since nh's original
codebase, so I never got a chance to stop it entirely)
> +/**
> + * @file draw-elements.c
Wrong filename.
> + *
> + * Tests that glDrawElementsIndirect works correctly with various combinations
> + * of draw command data generated via transform feedback.
> + */
Wrong command name.
> +#include "piglit-util-gl-common.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.supports_gl_core_version = 31;
> +
> + config.window_width = 1;
> + config.window_height = 1;
> + config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +unsigned num_indirect_draws;
> +
> +void write_commands_tfb(int indexed, int dump);
> +void setup_indirect_draw();
> +int compare_results(int indexed);
Make a header file in the same directory for prototypes for shared code?
> +enum piglit_result
> +piglit_display(void)
> +{
> + write_commands_tfb(0, 0);
> + setup_indirect_draw();
> +
> + glBeginTransformFeedback(GL_POINTS);
> + /* If <stride> is zero, the array elements are treated as
> + * tightly packed. */
> + glMultiDrawArraysIndirect(GL_POINTS, NULL,
> + num_indirect_draws, 0);
> + glEndTransformFeedback();
> +
> + return compare_results(0) ? PIGLIT_FAIL : PIGLIT_PASS;
> +}
> diff --git a/tests/spec/arb_draw_indirect/draw-arrays-prim-restart.c b/tests/spec/arb_draw_indirect/draw-arrays-prim-restart.c
> new file mode 100644
> index 0000000..005419f
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/draw-arrays-prim-restart.c
> @@ -0,0 +1,141 @@
> +/**
> + * @file errors.c
filename
> + *
> + * Tests that primitive restart is applied to glDrawArraysIndirect.
> + *
> + * The following vertices are drawn as a triangle strip:
> + * 1,5***3,6
> + * ****
> + * 0,4****2
> + * If the primitive is not restarted on index 3, the first 2 triangles
> + * will cover the whole window (0,1,2),(2,1,3).
> + */
I totally failed to parse that diagram of the vertices. If the comment
had used spaces to line up the *s (which i think are supposed to
represent the window?) it might have made sense.
We could also use testing of primitive restart with DrawElements, since
they're possibly quite different implementations for restart indices
other than ~0 and dx-only hardware.
> +enum piglit_result
> +piglit_display(void)
> +{
> + const float green[] = { 0, 1, 0, 0 };
> + const float clear[] = { 0, 0, 0, 0 };
> +
> + glClearColor(clear[0], clear[1], clear[2], clear[3]);
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + glEnable(GL_PRIMITIVE_RESTART);
> + glPrimitiveRestartIndex(3);
> + glDrawArraysIndirect(GL_TRIANGLE_STRIP, NULL);
> +
> + piglit_present_results();
> +
> + if (!piglit_probe_pixel_rgb(20, 16, clear) ||
> + !piglit_probe_pixel_rgb( 1, 16, green))
> + return PIGLIT_FAIL;
You can't probe after a present_results() -- the backbuffer's
undefined. Most tests as a result do:
bool pass = true;
pass = pass && piglit_probe_pixel_rgb(20, 16, clear);
pass = pass && piglit_probe_pixel_rgb( 1, 16, green);
piglit_present_results();
return pass ? PIGLIT_PASS : PIGLIT_FAIL;
which is why you see that pattern all over.
> diff --git a/tests/spec/arb_draw_indirect/draw-arrays.c b/tests/spec/arb_draw_indirect/draw-arrays.c
> new file mode 100644
> index 0000000..c6c4521
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/draw-arrays.c
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright © The Piglit project 2012
> + *
> + * 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.
> + *
> + * Authors: Christoph Bumiller
> + */
> +
> +/**
> + * @file draw-elements.c
> + *
> + * Tests that glDrawElementsIndirect works correctly with various combinations
> + * of draw command data generated via transform feedback.
file/command name.
> diff --git a/tests/spec/arb_draw_indirect/draw-common.c b/tests/spec/arb_draw_indirect/draw-common.c
> new file mode 100644
> index 0000000..290dcb5
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/draw-common.c
> @@ -0,0 +1,408 @@
> +/*
> + * Copyright © The Piglit project 2012
> + *
> + * 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.
> + *
> + * Authors: Christoph Bumiller
> + */
> +
> +/**
> + * @file draw-common.c
> + *
> + * Common initialization and generation of draw commands for testing the 4
> + * gl[Multi]DrawIndirect{Elements,Arrays} functions.
> + *
> + * The draw commands:
> + *
> + * typedef struct {
> + * GLuint count;
> + * GLuint primCount;
> + * GLuint firstIndex;
> + * GLint baseVertex;
> + * GLuint reservedMustBeZero;
> + * } DrawElementsIndirectCommand;
> + *
> + * typedef struct {
> + * GLuint count;
> + * GLuint primCount;
> + * GLuint first;
> + * GLuint reservedMustBeZero;
> + * } DrawArraysIndirectCommand;
> + *
> + * are generated by a vertex shader and written to the DRAW_INDIRECT_BUFFER via
> + * transform feedback.
> + * The vertex shader uses gl_VertexID as source to derive the various paramters.
> + *
> + * Then, the DrawIndirect commands stream vertices into a result buffer, also
> + * via transform feedback.
> + * Each vertex consists of a single uint that has the value of the vertex index.
> + * The sequence in which these values are written to the result buffer is used
> + * to check that the intended draw command has been executed.
> + */
Thanks for the big comment here, it helped a lot.
> +static GLuint tfb[2];
> +static GLuint prog[2];
> +static GLuint vao[2];
The code would have been clarified by using two named versions of each
of these instead of an array.
> +static void
> +dump_draw_commands(int indexed)
bools should use "bool"
> +int
> +compare_results(int indexed)
unused arg?
> +void
> +write_commands_tfb(int indexed, int dump)
these two are apparently bools, so they should be "bool".
> +{
> + static const char *varyingsIndexed[2] = { "commandIndexed", "zero" };
> + static const char *varyingsArrays[2] = { "commandArrays", "zero" };
> +
> + if (generate_data(indexed))
> + piglit_report_result(PIGLIT_FAIL);
> +
> + glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, tfb[0]);
> + glTransformFeedbackVaryings(prog[0], 2,
> + indexed ? varyingsIndexed : varyingsArrays,
> + GL_INTERLEAVED_ATTRIBS);
> + glLinkProgram(prog[0]);
> +
> + glUseProgram(prog[0]);
> + glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, tfb[0]);
> + glBeginTransformFeedback(GL_POINTS);
> + glBindVertexArray(vao[0]);
> + glDrawArrays(GL_POINTS, 0, num_indirect_draws);
> + glEndTransformFeedback();
> +
> + if (dump)
> + dump_draw_commands(indexed);
> +}
> +
> +/* Vertex program the writes the DrawIndirect command to output variables
"that"
> + * to be captured by transform feedback.
> + */
> +static const char *vs_source_gen = "#version 140\n"
> + "flat out uvec4 commandIndexed;\n"
> + "flat out uvec3 commandArrays;\n"
> + "flat out uint zero;\n"
> + "void main() {\n"
> + " uint id = uint(gl_VertexID);\n"
> +
> + " commandIndexed.x = ((id >> 2u) & 0x7u) + 1u;\n" /* vertex count */
> + " commandIndexed.y = ((id >> 5u) & 0x3u) + 1u;\n" /* instance count */
> + " commandIndexed.z = ((id >> 1u) & 0x1u) * 8u;\n" /* first index */
> + " commandIndexed.w = ((id >> 0u) & 0x1u) * 16u;\n" /* base vertex */
> +
> + " commandArrays.x = ((id >> 2u) & 0x7u) + 1u;\n" /* vertex count */
> + " commandArrays.y = ((id >> 5u) & 0x3u) + 1u;\n" /* instance count */
> + " commandArrays.z = ((id >> 0u) & 0x3u) * 8u;\n" /* first vertex */
> +
> + " zero = 0u;\n"
> + "}\n";
I will be honest, my eyes have totally glazed over on the giant nested
for loops and this bitmasking stuff. Not knocking the code, just saying
that while I've reviewed a lot of this patch, I haven't reviewed these
bits.
> diff --git a/tests/spec/arb_draw_indirect/draw-elements-multi.c b/tests/spec/arb_draw_indirect/draw-elements-multi.c
> new file mode 100644
> index 0000000..36d2067
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/draw-elements-multi.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright © The Piglit project 2012
> + *
> + * 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.
> + *
> + * Authors: Christoph Bumiller
> + */
> +
> +/**
> + * @file draw-elements.c
filename
> + *
> + * Tests that glDrawElementsIndirect works correctly with various combinations
> + * of draw command data generated via transform feedback.
command name
> +enum piglit_result
> +piglit_display(void)
> +{
> + write_commands_tfb(1, 0);
> + setup_indirect_draw();
> +
> + glBeginTransformFeedback(GL_POINTS);
> + /* If <stride> is zero, the array elements are treated as
> + * tightly packed. */
> + glMultiDrawElementsIndirect(GL_POINTS, GL_UNSIGNED_SHORT, NULL,
> + num_indirect_draws, 0);
> + glEndTransformFeedback();
> +
> + return compare_results(1) ? PIGLIT_FAIL : PIGLIT_PASS;
> +}
Looks like we're missing any testing of non-zero stride?
> diff --git a/tests/spec/arb_draw_indirect/errors-multi.c b/tests/spec/arb_draw_indirect/errors-multi.c
> new file mode 100644
> index 0000000..399b209
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/errors-multi.c
> +/**
> + * @file errors.c
wrong filename
> + *
> + * Tests error conditions for for glMultiDraw*Indirect:
> + */
> +enum piglit_result
> +piglit_display(void)
> +{
> + GLuint zeroData[128];
> + GLuint cmdBuf;
> + GLuint idxBuf;
> +
> + memset(zeroData, 0, sizeof(zeroData));
> +
> + if (piglit_get_gl_version() >= 31) {
> + GLuint vao;
> + glGenVertexArrays(1, &vao);
> + glBindVertexArray(vao);
> + }
> +
> + glClearColor(0.0, 0.0, 0.0, 0.0);
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + glGenBuffers(1, &cmdBuf);
> + glBindBuffer(GL_DRAW_INDIRECT_BUFFER, cmdBuf);
> + glBufferData(GL_DRAW_INDIRECT_BUFFER,
> + sizeof(zeroData), zeroData, GL_STATIC_DRAW);
> + glGenBuffers(1, &idxBuf);
> + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, idxBuf);
> + glBufferData(GL_ELEMENT_ARRAY_BUFFER,
> + sizeof(zeroData), zeroData, GL_STATIC_DRAW);
mixed-up indentation.
> + /* <stride> must be a multiple of four, otherwise an INVALID_VALUE
> + * error is generated.
> + *
> + * Test both functions with strides 3,4,5 and 8.
> + */
> + glMultiDrawArraysIndirect(GL_POINTS, NULL, 1, 3);
> + if (!piglit_check_gl_error(GL_INVALID_VALUE))
> + return PIGLIT_FAIL;
> + glMultiDrawElementsIndirect(GL_POINTS, GL_UNSIGNED_BYTE, NULL, 1, 3);
> + if (!piglit_check_gl_error(GL_INVALID_VALUE))
> + return PIGLIT_FAIL;
> +
> + glMultiDrawArraysIndirect(GL_POINTS, NULL, 1, 4);
> + if (!piglit_check_gl_error(GL_NO_ERROR))
> + return PIGLIT_FAIL;
> + glMultiDrawElementsIndirect(GL_POINTS, GL_UNSIGNED_BYTE, NULL, 1, 4);
> + if (!piglit_check_gl_error(GL_NO_ERROR))
> + return PIGLIT_FAIL;
> +
> + glMultiDrawArraysIndirect(GL_POINTS, NULL, 1, 5);
> + if (!piglit_check_gl_error(GL_INVALID_VALUE))
> + return PIGLIT_FAIL;
> + glMultiDrawElementsIndirect(GL_POINTS, GL_UNSIGNED_BYTE, NULL, 1, 5);
> + if (!piglit_check_gl_error(GL_INVALID_VALUE))
> + return PIGLIT_FAIL;
> +
> + glMultiDrawArraysIndirect(GL_POINTS, NULL, 1, 8);
> + if (!piglit_check_gl_error(GL_NO_ERROR))
> + return PIGLIT_FAIL;
> + glMultiDrawElementsIndirect(GL_POINTS, GL_UNSIGNED_BYTE, NULL, 1, 8);
> + if (!piglit_check_gl_error(GL_NO_ERROR))
> + return PIGLIT_FAIL;
Also check for the negative primcount error?
> diff --git a/tests/spec/arb_draw_indirect/errors.c b/tests/spec/arb_draw_indirect/errors.c
> new file mode 100644
> index 0000000..c9ca75b
> --- /dev/null
> +++ b/tests/spec/arb_draw_indirect/errors.c
> +/**
> + * @file errors.c
> + *
> + * Tests various error conditions for glDraw*Indirect:
> + */
Not tested:
- INVALID_ENUM for bad prim mode.
- INVALID_ENUM for bad index type.
- INVALID_OPERATION for DrawArraysIndirect with no indirect buffer bound
in core.
Of those, the last one I'd like to definitely see tested.
-------------- 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/20130508/d83a1c3b/attachment.pgp>
More information about the Piglit
mailing list