Re: [Piglit] [PATCH] program_interface_query: add new subtest for getprogramresourceiv

Martin Peres Wed, 20 May 2015 03:54:31 -0700

On 20/05/15 11:39, Samuel Iglesias Gonsálvez wrote:

On 19/05/15 12:02, Martin Peres wrote:

On 19/05/15 08:46, Samuel Iglesias Gonsalvez wrote:

Test that uniforms inside arrays of uniform blocks with instance name are
queried properly.


Tested on NVIDIA's proprietary driver version 340.65

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90397

Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
---
   tests/spec/arb_program_interface_query/common.h       | 12 +++++++++---
   .../getprogramresourceiv.c                            | 19
+++++++++++++++++++
   .../spec/arb_program_interface_query/resource-query.c | 16
++++++++++++----
   tests/spec/arb_shader_storage_buffer_object/minmax.c  |  4 ++--
   4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/tests/spec/arb_program_interface_query/common.h
b/tests/spec/arb_program_interface_query/common.h
index 55f0358..4079ac8 100755
--- a/tests/spec/arb_program_interface_query/common.h
+++ b/tests/spec/arb_program_interface_query/common.h
@@ -102,13 +102,19 @@ static const char fs_std[] =
       "uniform fs_uniform_block {"
       "    vec4 fs_color;\n"
       "    float fs_array[4];\n"
-    "};"
+    "};\n"
+    "uniform fs_array_uniform_block {\n"
+    "    vec4 fs_color;\n"
+    "    float fs_array[4];\n"
+    "} ubo[4];\n"

Hmm, aside from the obvious problem of having ubo != uniform_block,

I am going to rename that name uniform block in the second version of
the patch.


sounds nicer indeed!

I
wonder what would happen if you instead had:

+    "} faub1[2], faub2[2];\n"

That kind of definition is not allowed in the spec. I think it is only
one instance name per interface block.

I tried defining "faub2[2]" by repeating the fs_array_uniform_block
definition but i965 doesn't allowed it, although NVIDIA does.


I hope that no games rely on this...

I am worried that you would get the following resources listed:

+                       "fs_array_uniform_block[0]",
+                       "fs_array_uniform_block[1]",
+                       "fs_array_uniform_block[0]",
+                       "fs_array_uniform_block[1]",

In my tests on NVIDIA, I get:

+                       "fs_array_uniform_block[0]",
+                       "fs_array_uniform_block[1]",

But they are not repeated. I guess NVIDIA thinks they are the same
uniform block but I did not check it further.


Seems like a lovely bug!

which would be a clear indication that the driver is broken. My view on
this is that named uniform blocks should use their named version when
being referenced. What do you think?

I have been checking what the specs say about it.

* In ARB_program_interface_query, GetProgramResourceName() is equivalent
to GetActiveUniformBlockName().

* Looking at ARB_uniform_buffer_object, GetActiveUniformBlockName() is
talking about uniform block name.

* Looking at GLSL spec (for example 4.4), when talking about interface
block (4.3.9 Interface Blocks), its definition is:

layout-qualifieropt interface-qualifier block-name { member-list }
instance-nameopt ;

But also, the same sections talks about the name:

"For blocks declared as arrays, the array index must also be included
  when accessing members, as in this example

   uniform Transform { // API uses “Transform[2]” to refer to instance 2
     mat4 ModelViewMatrix;
     mat4 ModelViewProjectionMatrix;
     vec4 a[]; // array will get implicitly sized
     float Deformation;
   } transforms[4];

  [...]
  When using OpenGL API entry points to identify the name of an
  individual block in an array of blocks, the name string must include
  an array index (e.g., Transform[2])."

Notice that it is talking about the block name, not the instance name.
So the current behaviour is right.

Wow, I am baffled by those crazy rules... Thanks for tracking this downthe spec!

       "in vec4 fs_input1;\n"
       "out vec4 fs_output0;\n"
       "out vec4 fs_output1;\n"
       "void main() {\n"
-        "fs_output0 = fs_color * fs_input1 * fs_array[2];\n"
-        "fs_output1 = fs_color * fs_input1 * fs_array[3];\n"
+        "fs_output0 = fs_color * fs_input1 * fs_array[2] * \n"
+        "          ubo[0].fs_array[2] * ubo[2].fs_array[2];\n"
+        "fs_output1 = fs_color * fs_input1 * fs_array[3] * \n"
+        "             ubo[1].fs_array[3] * ubo[3].fs_array[3];\n"
       "}";
     static const char vs_stor[] =
diff --git
a/tests/spec/arb_program_interface_query/getprogramresourceiv.c
b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
index da9751a..03f2fc6 100755
--- a/tests/spec/arb_program_interface_query/getprogramresourceiv.c
+++ b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
@@ -454,6 +454,25 @@ static const struct subtest_t subtests[] = {
       { GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
       { 0, 0, { 0 } }}
    },
+ { &prog_std, GL_UNIFORM, "fs_array_uniform_block.fs_array",
"fs_array_uniform_block[0]", {
+    { GL_NAME_LENGTH, 1, { 35 } },
+    { GL_TYPE, 1, { GL_FLOAT } },
+    { GL_ARRAY_SIZE, 1, { 4 } },
+    { GL_OFFSET, 1, { 0 } }, /* valid index == anything but -1 */
+    { GL_BLOCK_INDEX, 1, { 2 } }, /* compared to
fs_array_uniform_block[0]'s idx */
+    { GL_ARRAY_STRIDE, 1, { 0 } }, /* valid index == anything but -1 */
+    { GL_MATRIX_STRIDE, 1, { 0 } },
+    { GL_IS_ROW_MAJOR, 1, { 0 } },
+    { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index ==
anything but -1 */
+    { GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
+    { GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
+    { GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
+    { GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
+    { GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 1 } },
+    { GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
+    { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
+    { 0, 0, { 0 } }}
+ },
    { &prog_stor, GL_BUFFER_VARIABLE, "gs_buf_var", "gs_buffer_block", {
       { GL_NAME_LENGTH, 1, { 11 } },
       { GL_TYPE, 1, { GL_FLOAT_VEC4 } },
diff --git a/tests/spec/arb_program_interface_query/resource-query.c
b/tests/spec/arb_program_interface_query/resource-query.c
index 92b8cd8..1db5585 100755
--- a/tests/spec/arb_program_interface_query/resource-query.c
+++ b/tests/spec/arb_program_interface_query/resource-query.c
@@ -189,13 +189,21 @@ PIGLIT_GL_TEST_CONFIG_END
    * white space anywhere in the string.
    */
   static const char *st_r_uniform[] = {"vs_test", "gs_test", "fs_color",
-                     "fs_array[0]", "sa[0].a[0]", "sa[1].a[0]",
+                     "fs_array[0]",
+                     "fs_array_uniform_block.fs_color",
+                     "fs_array_uniform_block.fs_array[0]",
+                     "sa[0].a[0]", "sa[1].a[0]",
                        NULL};
   static const char *st_r_tess_uniform[] = {"tcs_test", "tes_test",
NULL};
   static const char *st_r_cs_uniform[] = {"cs_test", "tex", NULL};
   static const char *st_r_uniform_block[] = {"vs_uniform_block",
                          "gs_uniform_block",
-                       "fs_uniform_block", NULL};
+                       "fs_uniform_block",
+                       "fs_array_uniform_block[0]",
+                       "fs_array_uniform_block[1]",
+                       "fs_array_uniform_block[2]",
+                       "fs_array_uniform_block[3]",
+                       NULL};
   static const char *st_r_tess_uniform_block[] = {"tcs_uniform_block",
                           "tes_uniform_block", NULL};
   static const char *st_r_cs_uniform_block[] = {"cs_uniform_block",
NULL};
@@ -346,10 +354,10 @@ struct subtest_t {
   }
     static const struct subtest_t subtests[] = {
- ST( 6, 12, -1, -1,  vs_std,    NULL,    NULL,  gs_std,  fs_std,
NULL, GL_UNIFORM, "(vs,gs,fs)", st_r_uniform),
+ ST( 8, 35, -1, -1,  vs_std,    NULL,    NULL,  gs_std,  fs_std,
NULL, GL_UNIFORM, "(vs,gs,fs)", st_r_uniform),
    ST( 2,  9, -1, -1,    NULL, tcs_sub, tes_sub,    NULL,    NULL,
NULL, GL_UNIFORM, "(tes,tcs)", st_r_tess_uniform),
    ST( 2,  8, -1, -1,    NULL,    NULL,    NULL,    NULL,    NULL,
cs_sub, GL_UNIFORM, "(cs)", st_r_cs_uniform),
- ST( 3, 17,  2, -1,  vs_std,    NULL,    NULL,  gs_std,  fs_std,
NULL, GL_UNIFORM_BLOCK, "(vs,gs,fs)", st_r_uniform_block),
+ ST( 7, 26,  2, -1,  vs_std,    NULL,    NULL,  gs_std,  fs_std,
NULL, GL_UNIFORM_BLOCK, "(vs,gs,fs)", st_r_uniform_block),
    ST( 2, 18, -1, -1,    NULL, tcs_sub, tes_sub,    NULL,    NULL,
NULL, GL_UNIFORM_BLOCK, "(tcs,tes)", st_r_tess_uniform_block),
    ST( 1, 17, -1, -1,    NULL,    NULL,    NULL,    NULL,    NULL,
cs_sub, GL_UNIFORM_BLOCK, "(cs)", st_r_cs_uniform_block),
    ST( 2, 10, -1, -1,  vs_std,    NULL,    NULL,    NULL,    NULL,
NULL, GL_PROGRAM_INPUT, "(vs)", st_r_in_vs),
diff --git a/tests/spec/arb_shader_storage_buffer_object/minmax.c
b/tests/spec/arb_shader_storage_buffer_object/minmax.c
index 71d86fe..281bb99 100644
--- a/tests/spec/arb_shader_storage_buffer_object/minmax.c
+++ b/tests/spec/arb_shader_storage_buffer_object/minmax.c
@@ -31,8 +31,8 @@
     PIGLIT_GL_TEST_CONFIG_BEGIN
   -    config.supports_gl_compat_version = 40;
-    config.supports_gl_core_version = 40;
+    config.supports_gl_compat_version = 31;
+    config.supports_gl_core_version = 31;

I think this got included in the patch for no good reason :)

Oh right! I am going to fix it :-)

Thanks!


You're welcome!
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] program_interface_query: add new subtest for getprogramresourceiv

Reply via email to