[Piglit] [PATCH v2] arb_shader_atomic_counters-semantics: test different binding points
Timothy Arceri
t_arceri at yahoo.com.au
Fri Jul 31 17:47:32 PDT 2015
On Fri, 2015-05-15 at 11:35 +0200, Alejandro Piñeiro wrote:
> Piglit was only checking the expected effects using binding point 0.
> With this commit, all the values between 0 and
> GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS are checked. It also checks that
> the binding point used on the shader and the one returned by
> glGetActiveAtomicCounterBufferiv(..., ...,
> GL_ATOMIC_COUNTER_BUFFER_BINDING, ...) is the same.
>
> Test added in order to detect things like this mesa bug:
> https://bugs.freedesktop.org/show_bug.cgi?id=90175
> ---
>
> Now it tests all the values between 0 and
> GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS,
> as suggested on the review. I also added a check of the value returned
> by glGetActiveAtomicCounterBufferiv compared with the binding point used
> on the shader, as suggested on the mesa thread.
>
> I moved the max_bindings for to the subtest itself, in order to avoid too
> many
> pass/fail messages output. Due this now the subtest also handle the atomic
> counter buffer and the call to glBindBufferBase.
>
> Final note: something I forgot to mention on the first patch. I only
> modified
> the fs, vs and gs subtests, but not the tesselation control/evaluation
> shaders.
> This is because I can't test them on my system, so I prefered to not modify
> them, but probably it makes sense to add the same tests there too.
>
>
> tests/spec/arb_shader_atomic_counters/semantics.c | 167 +++++++++++++++++--
> ---
> 1 file changed, 128 insertions(+), 39 deletions(-)
>
> diff --git a/tests/spec/arb_shader_atomic_counters/semantics.c
> b/tests/spec/arb_shader_atomic_counters/semantics.c
> index 694f1fe..b8a3b5b 100644
> --- a/tests/spec/arb_shader_atomic_counters/semantics.c
> +++ b/tests/spec/arb_shader_atomic_counters/semantics.c
> @@ -24,7 +24,8 @@
> /** @file semantics.c
> *
> * Tests that the atomic built-in functions have the expected effects
> - * on memory and return the expected results.
> + * on memory and return the expected results, with different binding
> + * points.
> */
>
> #include "common.h"
> @@ -40,7 +41,23 @@ PIGLIT_GL_TEST_CONFIG_BEGIN
> PIGLIT_GL_TEST_CONFIG_END
>
> static bool
> -run_test_vertex(void)
> +atomic_counters_expected_binding(GLuint prog,
> + GLuint binding)
> +{
> + GLint param;
> +
> + glGetActiveAtomicCounterBufferiv(prog, 0,
> + GL_ATOMIC_COUNTER_BUFFER_BINDING,
> + ¶m);
> + if (binding != param)
> + printf ("Wrong binding point on the shader, %i expected, %i found\n",
> + binding, param);
> +
> + return binding==param;
Not a big deal but we normaly leave spaces between the ==
> +}
> +
> +static bool
> +run_test_vertex(GLint max_bindings)
> {
> const char *fs_source = "#version 140\n"
> "flat in ivec4 vcolor;\n"
> @@ -48,10 +65,10 @@ run_test_vertex(void)
> "void main() {\n"
> " fcolor = vcolor;\n"
> "}\n";
> - const char *vs_source = "#version 140\n"
> + const char *vs_template = "#version 140\n"
> "#extension GL_ARB_shader_atomic_counters : enable\n"
> "\n"
> - "layout(binding = 0, offset = 0) uniform atomic_uint x;\n"
> + "layout(binding = %i, offset = 0) uniform atomic_uint x;\n"
> "in vec4 piglit_vertex;\n"
> "flat out ivec4 vcolor;\n"
> "\n"
> @@ -66,26 +83,49 @@ run_test_vertex(void)
> const uint32_t expected_buffer[] = { 0x0 };
> const uint32_t expected_color[] = { 0xfffffffe, 0xfffffffe,
> 0xffffffff, 0x0 };
> - GLuint prog = glCreateProgram();
> - bool ret =
> - atomic_counters_compile(prog, GL_FRAGMENT_SHADER,
> fs_source) &&
> - atomic_counters_compile(prog, GL_VERTEX_SHADER, vs_source)
> &&
> - atomic_counters_draw_point(prog, 1, start_buffer) &&
> - piglit_probe_rect_rgba_uint(0, 0, 1, 1, expected_color) &&
> - atomic_counters_probe_buffer(0, 1, expected_buffer);
> + GLuint prog;
> + bool ret = true;
> + char *vs_source;
> + int asprintf_ret;
> + int i;
> + GLuint buffer;
> +
> + glGenBuffers(1, &buffer);
> + for (i = 0; i < max_bindings; i++) {
> + prog = glCreateProgram();
> + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, i, buffer);
> +
> + asprintf_ret = asprintf(&vs_source, vs_template, i);
> + assert(asprintf_ret > 0);
> +
> + ret =
> + atomic_counters_compile(prog, GL_FRAGMENT_SHADER, fs_source) &&
> + atomic_counters_compile(prog, GL_VERTEX_SHADER, vs_source) &&
> + atomic_counters_draw_point(prog, 1, start_buffer) &&
> + piglit_probe_rect_rgba_uint(0, 0, 1, 1, expected_color) &&
> + atomic_counters_probe_buffer(0, 1, expected_buffer) &&
> + atomic_counters_expected_binding(prog, i);
> +
> + free(vs_source);
> + glDeleteProgram(prog);
> +
> + if (!ret)
> + break;
> + }
> +
> + glDeleteBuffers(1, &buffer);
It would be nice to move most of this into a function maybe just called
test_shader and pass in the expected values and shader strings. You could have
a NULL test for when geom shader is not available, this way a tessellation
test would only require a couple of lines of code.
Also you are using 2 space indentation. The rest of the patch uses 8 spaces,
please use 8 to be consistent.
Other than that the patch looks good. If you make these changes I will commit
it for you.
>
> - glDeleteProgram(prog);
> return ret;
> }
>
> static bool
> -run_test_fragment(void)
> +run_test_fragment(GLint max_bindings)
> {
> - const char *fs_source = "#version 140\n"
> + const char *fs_template = "#version 140\n"
> "#extension GL_ARB_shader_atomic_counters : enable\n"
> "\n"
> "out ivec4 fcolor;\n"
> - "layout(binding = 0, offset = 0) uniform atomic_uint x;\n"
> + "layout(binding = %i, offset = 0) uniform atomic_uint x;\n"
> "\n"
> "void main() {\n"
> " fcolor.x = int(atomicCounterDecrement(x));\n"
> @@ -105,20 +145,44 @@ run_test_fragment(void)
> const uint32_t expected_buffer[] = { 0x0 };
> const uint32_t expected_color[] = { 0xfffffffe, 0xfffffffe,
> 0xffffffff, 0x0 };
> - GLuint prog = glCreateProgram();
> - bool ret =
> - atomic_counters_compile(prog, GL_FRAGMENT_SHADER,
> fs_source) &&
> - atomic_counters_compile(prog, GL_VERTEX_SHADER, vs_source)
> &&
> - atomic_counters_draw_point(prog, 1, start_buffer) &&
> - piglit_probe_rect_rgba_uint(0, 0, 1, 1, expected_color) &&
> - atomic_counters_probe_buffer(0, 1, expected_buffer);
> + GLuint prog;
> + bool ret;
> + char *fs_source;
> + int asprintf_ret;
> + int i;
> + GLuint buffer;
> +
> + glGenBuffers(1, &buffer);
> + for (i = 0; i < max_bindings; i++) {
> + prog = glCreateProgram();
> + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, i, buffer);
> +
> + asprintf_ret = asprintf(&fs_source, fs_template, i);
> + assert(asprintf_ret > 0);
> +
> + ret =
> + atomic_counters_compile(prog, GL_FRAGMENT_SHADER, fs_source) &&
> + atomic_counters_compile(prog, GL_VERTEX_SHADER, vs_source) &&
> + atomic_counters_draw_point(prog, 1, start_buffer) &&
> + piglit_probe_rect_rgba_uint(0, 0, 1, 1, expected_color) &&
> + atomic_counters_probe_buffer(0, 1, expected_buffer) &&
> + atomic_counters_expected_binding(prog, i);
> +
> +
> + free(fs_source);
> + glDeleteProgram(prog);
> +
> + if (!ret)
> + break;
> + }
> +
> + glDeleteBuffers(1, &buffer);
>
> - glDeleteProgram(prog);
> return ret;
> }
>
> static bool
> -run_test_geometry(void)
> +run_test_geometry(GLint max_bindings)
> {
> const char *fs_source = "#version 140\n"
> "flat in ivec4 gcolor;\n"
> @@ -126,7 +190,7 @@ run_test_geometry(void)
> "void main() {\n"
> " fcolor = gcolor;\n"
> "}\n";
> - const char *gs_source = "#version 150\n"
> + const char *gs_template = "#version 150\n"
> "#extension GL_ARB_shader_atomic_counters : enable\n"
> "\n"
> "layout(points) in;\n"
> @@ -134,7 +198,7 @@ run_test_geometry(void)
> "\n"
> "flat out ivec4 gcolor;\n"
> "\n"
> - "layout(binding = 0, offset = 0) uniform atomic_uint x;\n"
> + "layout(binding = %i, offset = 0) uniform atomic_uint x;\n"
> "\n"
> "void main() {\n"
> " gl_Position = gl_in[0].gl_Position;\n"
> @@ -157,15 +221,38 @@ run_test_geometry(void)
> const uint32_t expected_color[] = { 0xfffffffe, 0xfffffffe,
> 0xffffffff, 0x0 };
> GLuint prog = glCreateProgram();
> - bool ret =
> - atomic_counters_compile(prog, GL_FRAGMENT_SHADER,
> fs_source) &&
> - atomic_counters_compile(prog, GL_GEOMETRY_SHADER,
> gs_source) &&
> - atomic_counters_compile(prog, GL_VERTEX_SHADER, vs_source)
> &&
> - atomic_counters_draw_point(prog, 1, start_buffer) &&
> - piglit_probe_rect_rgba_uint(0, 0, 1, 1, expected_color) &&
> - atomic_counters_probe_buffer(0, 1, expected_buffer);
> + bool ret;
> + char *gs_source;
> + int asprintf_ret;
> + int i;
> + GLuint buffer;
> +
> + glGenBuffers(1, &buffer);
> + for (i = 0; i < max_bindings; i++) {
> + prog = glCreateProgram();
> + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, i, buffer);
> +
> + asprintf_ret = asprintf(&gs_source, gs_template, i);
> + assert(asprintf_ret > 0);
> +
> + ret =
> + atomic_counters_compile(prog, GL_FRAGMENT_SHADER, fs_source) &&
> + atomic_counters_compile(prog, GL_GEOMETRY_SHADER, gs_source) &&
> + atomic_counters_compile(prog, GL_VERTEX_SHADER, vs_source) &&
> + atomic_counters_draw_point(prog, 1, start_buffer) &&
> + piglit_probe_rect_rgba_uint(0, 0, 1, 1, expected_color) &&
> + atomic_counters_probe_buffer(0, 1, expected_buffer) &&
> + atomic_counters_expected_binding(prog, i);
> +
> + free(gs_source);
> + glDeleteProgram(prog);
> +
> + if (!ret)
> + break;
> + }
> +
> + glDeleteBuffers(1, &buffer);
>
> - glDeleteProgram(prog);
> return ret;
> }
>
> @@ -331,6 +418,7 @@ piglit_init(int argc, char **argv)
> {
> GLuint fb, rb, buffer;
> enum piglit_result status = PIGLIT_PASS;
> + GLint max_bindings;
>
> piglit_require_extension("GL_ARB_shader_atomic_counters");
>
> @@ -345,21 +433,22 @@ piglit_init(int argc, char **argv)
> glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER,
> GL_COLOR_ATTACHMENT0,
> GL_RENDERBUFFER, rb);
>
> - glGenBuffers(1, &buffer);
> - glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, 0, buffer);
> + glGetIntegerv(GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS,
> &max_bindings);
>
> atomic_counters_subtest(&status, GL_FRAGMENT_SHADER,
> "Fragment shader atomic built-in
> semantics",
> - run_test_fragment);
> + run_test_fragment, max_bindings);
>
> atomic_counters_subtest(&status, GL_VERTEX_SHADER,
> "Vertex shader atomic built-in semantics",
> - run_test_vertex);
> + run_test_vertex, max_bindings);
>
> atomic_counters_subtest(&status, GL_GEOMETRY_SHADER,
> "Geometry shader atomic built-in
> semantics",
> - run_test_geometry);
> + run_test_geometry, max_bindings);
>
> + glGenBuffers(1, &buffer);
> + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, 0, buffer);
> atomic_counters_subtest(&status, GL_TESS_CONTROL_SHADER,
> "Tessellation control shader atomic built
> -in "
> "semantics",
More information about the Piglit
mailing list