[Piglit] [PATCH v3] arb_shader_atomic_counters-semantics: test different binding points
Timothy Arceri
t_arceri at yahoo.com.au
Sat Aug 1 17:13:23 PDT 2015
On Sat, 2015-08-01 at 17:56 +0200, Alejandro Piñeiro wrote:
>
> On 01/08/15 15:47, Timothy Arceri wrote:
> > On Sat, 2015-08-01 at 15:32 +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
> > > ---
> > >
> > > Update after Timothy Arceri second patch review. As in the previous one,
> > > it doesn't touch the tesselation tests because I don't have a way to
> > > test them on my system. It is likely that the new method test_shader
> > > would need to be adapted to include them.
> > >
> > > I also noted that in all the tests, start_buffer, expected_buffer and
> > > expected_color has the same value. It could be possible to make
> > > just one definition. I also didn't touch it, as I assume that
> > > was made on purpose.
> > >
> > > tests/spec/arb_shader_atomic_counters/semantics.c | 157
> > > ++++++++++++++++---
> > > ---
> > > 1 file changed, 117 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/tests/spec/arb_shader_atomic_counters/semantics.c
> > > b/tests/spec/arb_shader_atomic_counters/semantics.c
> > > index 694f1fe..4c9c55c 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,55 @@ 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;
> > > +}
> > > +
> > > +static bool
> > > +test_shader(GLuint index,
> > > + const char *vs_source,
> > > + const char *fs_source,
> > > + const char *gs_source,
> > > + const uint32_t *start_buffer,
> > > + const uint32_t *expected_buffer,
> > > + const uint32_t *expected_color)
> > > +{
> > > + GLuint prog;
> > > + bool ret = true;
> > > + GLuint buffer;
> > > +
> > > + glGenBuffers(1, &buffer);
> > > + prog = glCreateProgram();
> > > + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, index, buffer);
> > > +
> > > + ret =
> > > + atomic_counters_compile(prog, GL_FRAGMENT_SHADER,
> > > fs_source) &&
> > > + atomic_counters_compile(prog, GL_VERTEX_SHADER,
> > > vs_source)
> > > &&
> > > + (gs_source != NULL ? atomic_counters_compile(prog,
> > > GL_GEOMETRY_SHADER, gs_source) : true) &&
> > > + 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, index);
> > > +
> > > + glDeleteProgram(prog);
> > > + glDeleteBuffers(1, &buffer);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static bool
> > > +run_test_vertex(GLint max_bindings)
> > > {
> > > const char *fs_source = "#version 140\n"
> > > "flat in ivec4 vcolor;\n"
> > > @@ -48,10 +97,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 +115,35 @@ 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);
> > > + bool ret = true;
> > > + char *vs_source;
> > > + int asprintf_ret;
> > > + int i;
> > > +
> > > + for (i = 0; i < max_bindings; i++) {
> > > + asprintf_ret = asprintf(&vs_source, vs_template, i);
> > > + assert(asprintf_ret > 0);
> > > +
> > > + ret = test_shader(i, vs_source, fs_source, NULL,
> > > + start_buffer, expected_buffer,
> > > expected_color);
> > > +
> > > + free(vs_source);
> > > +
> > > + if (!ret)
> > > + break;
> > > + }
> >
> > Maybe I should have been more clear but can't you just put the entire for
> > loop
> > in the new test_shader() function?
>
> Yes, I also thought on that. The problem is that as part of the loop you
> are updating the xx_source based on a template and the loop counter. And
> each test has a different template (the vertex program has a template
> for the vertex shader, the fragment program has a template for the
> fragment shader, and so on). So in order to put the loop on test_shader,
> it would need to include the stage, and considerer the string a template
> based on that. Initially I considered that in that way, test_shader
> would be somewhat too complex. In any case, I will update the patch, and
> then you could decide which option do you prefer.
Thanks, I see what you mean now. However I think I still prefer V4 I can see
some minor improvements which I'll make before pushing your patch. Thanks for
making the updates :)
>
>
> BR
>
More information about the Piglit
mailing list