[Piglit] [PATCH v3] arb_shader_atomic_counters-semantics: test different binding points

[Alejandro Piñeiro]

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,
>> +                                         &param);
>> +        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.


BR

-- 
Alejandro Piñeiro (apinheiro at igalia.com)