<div dir="ltr">On 4 September 2013 12:57, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com" target="_blank">gregory.hainaut@gmail.com</a>><br>


<br>
Bind various stages (including geometry and tesselation) to the pipeline<br>
and check the result with GetProgramPipelineiv.  I take also the<br>
opportunity to use glCreateShaderProgramv.<br>
<br>
AMD generates an GL_INVALID_VALUE error when glActiveShaderProgram is<br>
called with a program that isn't bound to any stage to the pipeline.  No<br>
such error is specified in the spec, so this may be their bug.  Ian has<br>
submitted a GL spec bug for this issue.  It seems likely that the next<br>
version of the GL spec will include this as an error.  We can update the<br>
test when that happens.<br>
<br>
Spec quote:<br>
<br>
    "An INVALID_VALUE error is generated if program is not the name of<br>
    either a program or shader object.  An INVALID_OPERATION error is<br>
    generated if program is the name of a shader object."<br>
<br>
V4:<br>
* split new test in a separate commit<br>
* Merge the 2 vertex shaders with the help of the __VERSION__ macro<br>
* Properly set shader version with asprintf<br>
<br>
V5 (idr):<br>
* Use 'pass = foo() && pass;' idiom instead of 'pass &= foo();'.  C's<br>
  short-circuit evaulation rules cause these to be different.<br>
* s/GLboolean/bool/g<br>
* Produce no output with -auto.<br>
* Don't add tests from future commits to CMakeLists.gl.txt yet.<br>
* Trivial reformatting.<br>
---<br>
 tests/all.tests                                    |   5 +<br>
 tests/spec/CMakeLists.txt                          |   1 +<br>
 .../arb_separate_shader_objects/CMakeLists.gl.txt  |  12 +<br>
 .../arb_separate_shader_objects/CMakeLists.txt     |   1 +<br>
 .../GetProgramPipelineiv.c                         | 291 +++++++++++++++++++++<br>
 5 files changed, 310 insertions(+)<br>
 create mode 100644 tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt<br>
 create mode 100644 tests/spec/arb_separate_shader_objects/CMakeLists.txt<br>
 create mode 100644 tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c<br>
<br>
diff --git a/tests/all.tests b/tests/all.tests<br>
index dfbb756..9464244 100644<br>
--- a/tests/all.tests<br>
+++ b/tests/all.tests<br>
@@ -1234,6 +1234,11 @@ add_concurrent_test(arb_occlusion_query, 'occlusion_query_meta_fragments')<br>
 add_concurrent_test(arb_occlusion_query, 'occlusion_query_meta_no_fragments')<br>
 add_concurrent_test(arb_occlusion_query, 'occlusion_query_order')<br>
<br>
+# Group ARB_separate_shader_objects<br>
+arb_separate_shader_objects = Group()<br>
+spec['ARB_separate_shader_objects'] = arb_separate_shader_objects<br>
+arb_separate_shader_objects['GetProgramPipelineiv'] = concurrent_test('arb_separate_shader_object-GetProgramPipelineiv')<br>
+<br>
 # Group ARB_sampler_objects<br>
 arb_sampler_objects = Group()<br>
 spec['ARB_sampler_objects'] = arb_sampler_objects<br>
diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt<br>
index 17c991a..8057d20 100644<br>
--- a/tests/spec/CMakeLists.txt<br>
+++ b/tests/spec/CMakeLists.txt<br>
@@ -20,6 +20,7 @@ add_subdirectory (arb_sampler_objects)<br>
 add_subdirectory (arb_seamless_cube_map)<br>
 add_subdirectory (amd_seamless_cubemap_per_texture)<br>
 add_subdirectory (amd_vertex_shader_layer)<br>
+add_subdirectory (arb_separate_shader_objects)<br>
 add_subdirectory (arb_shader_texture_lod/execution)<br>
 add_subdirectory (arb_shader_objects)<br>
 add_subdirectory (arb_shading_language_420pack/execution)<br>
diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt<br>
new file mode 100644<br>
index 0000000..2ea2c80<br>
--- /dev/null<br>
+++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt<br>
@@ -0,0 +1,12 @@<br>
+include_directories(<br>
+       ${GLEXT_INCLUDE_DIR}<br>
+       ${OPENGL_INCLUDE_PATH}<br>
+)<br>
+<br>
+link_libraries (<br>
+       piglitutil_${piglit_target_api}<br>
+       ${OPENGL_gl_LIBRARY}<br>
+       ${OPENGL_glu_LIBRARY}<br>
+)<br>
+<br>
+piglit_add_executable (arb_separate_shader_object-GetProgramPipelineiv GetProgramPipelineiv.c)<br>
diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.txt b/tests/spec/arb_separate_shader_objects/CMakeLists.txt<br>
new file mode 100644<br>
index 0000000..144a306<br>
--- /dev/null<br>
+++ b/tests/spec/arb_separate_shader_objects/CMakeLists.txt<br>
@@ -0,0 +1 @@<br>
+piglit_include_target_api()<br>
diff --git a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c<br>
new file mode 100644<br>
index 0000000..f933fba<br>
--- /dev/null<br>
+++ b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c<br>
@@ -0,0 +1,291 @@<br>
+/*<br>
+ * Copyright © 2013 Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com" target="_blank">gregory.hainaut@gmail.com</a>><br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS<br>
+ * IN THE SOFTWARE.<br>
+ */<br>
+<br>
+#include "piglit-util-gl-common.h"<br>
+<br>
+PIGLIT_GL_TEST_CONFIG_BEGIN<br>
+<br>
+       config.supports_gl_compat_version = 10;<br></blockquote><div><br></div><div>This should be:<br><br></div><div>config.supports_gl_compat_version = 20;<br></div><div>config.supports_gl_core_version = 31;<br></div><div>

 </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+       config.window_width = 32;<br>
+       config.window_height = 32;<br>
+<br>
+PIGLIT_GL_TEST_CONFIG_END<br>
+<br>
+static bool pass;<br>
+<br>
+enum piglit_result<br>
+piglit_display(void)<br>
+{<br>
+       /* UNREACHED */<br>
+       return PIGLIT_FAIL;<br>
+}<br>
+<br>
+static GLbitfield<br>
+stage2bitfield(GLint stage)<br>
+{<br>
+       switch(stage) {<br>
+               case GL_VERTEX_SHADER: return GL_VERTEX_SHADER_BIT;<br>
+               case GL_FRAGMENT_SHADER: return GL_FRAGMENT_SHADER_BIT;<br>
+               case GL_GEOMETRY_SHADER: return GL_GEOMETRY_SHADER_BIT;<br>
+               case GL_TESS_CONTROL_SHADER: return GL_TESS_CONTROL_SHADER_BIT;<br>
+               case GL_TESS_EVALUATION_SHADER:return GL_TESS_EVALUATION_SHADER_BIT;<br>
+               case GL_COMPUTE_SHADER: return GL_COMPUTE_SHADER_BIT;<br>
+               default:return 0;<br>
</blockquote><div><br></div><div>Can we have an assertion on the default case, just so that in case we ever pass an incorrect enum to this function, we don't get a silent failure? <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+       }<br>
+       return 0;<br></blockquote><div><br></div><div>This return statement is unreachable.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+<br>
+static void<br>
+check_stage(GLint pipe, GLint expected, GLint stage, bool supported)<br>
+{<br>
+       GLint param = 0;<br>
+       glGetProgramPipelineiv(pipe, stage, &param);<br>
+<br>
+       if (!supported) {<br>
+               pass = piglit_check_gl_error(GL_INVALID_ENUM) && pass;<br>
+       } else if (param != expected) {<br>
+               fprintf(stderr, "Failed to get program of stage %s.\n",<br>
+                       piglit_get_gl_enum_name(stage));<br>
+               pass = false;<br>
+       }<br>
+}<br>
+<br>
+static void<br>
+use_stage_and_check(GLint pipe, GLint program, GLint stage, bool supported)<br>
+{<br>
+       if (!piglit_automatic) {<br>
+               printf("Attach program (%d) to stage (%s). Expected to be "<br>
+                      "supported: %s\n",<br>
+                      program,<br>
+                      piglit_get_gl_enum_name(stage),<br>
+                      supported ? "yes" : "no");<br></blockquote><div><br></div><div>Why only print this when the test is run manually?  It seems like it would be useful information to see in an automatic test run as well.<br>

</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+       }<br>
+<br>
+       glUseProgramStages(pipe, stage2bitfield(stage), program);<br>
+       if (!supported) {<br>
+               pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;<br>
+       } else {<br>
+               pass = piglit_check_gl_error(GL_NO_ERROR) && pass;<br>
+       }<br>
+<br>
+       check_stage(pipe, program, stage, supported);<br>
+}<br>
+<br>
+void<br>
+piglit_init(int argc, char **argv)<br>
+{<br>
+       GLint vs, fs, gs, tcs, tes;<br>
+       GLint ver;<br>
+       GLuint pipe = 0;<br>
+       GLint param = 0;<br>
+       char version[100];<br>
+       const char *shader_source[2];<br>
+<br>
+       static const char *vs_source =<br>
+               "#if __VERSION__ > 140\n"<br>
+               "out gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "};\n"<br></blockquote><div><br></div><div>There's no need to redeclare this since we're not changing anything from the default declaration.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+               "\n"<br>
+               "in vec4 position;\n"<br>
+               "#endif\n"<br>
+               "\n"<br>
+               "void main()\n"<br>
+               "{\n"<br>
+               "#if __VERSION__ > 140\n"<br>
+               "       gl_Position = position;\n"<br>
+               "#else\n"<br>
+               "       gl_Position = gl_Vertex;\n"<br>
+               "#endif\n"<br>
+               "}\n";<br>
+       static const char *fs_source =<br>
+               "void main()\n"<br>
+               "{\n"<br>
+               "       gl_FragColor = vec4(0.0, 1.0, 0.0, 0.0);\n"<br>
+               "}\n";<br>
+       static const char *gs_source =<br>
+               "in gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "    float gl_PointSize;\n"<br>
+               "    float gl_ClipDistance[];\n"<br>
+               "} gl_in[];\n"<br>
+               "\n"<br>
+               "out gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "    float gl_PointSize;\n"<br>
+               "    float gl_ClipDistance[];\n"<br>
+               "};\n"<br></blockquote><div><br></div><div>These redeclarations aren't needed either.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+               "\n"<br>
+               "layout(triangles) in;\n"<br>
+               "layout(triangle_strip, max_vertices = 3) out;\n"<br>
+               "void main() {\n"<br>
+               "  for(int i = 0; i < gl_in.length(); i++) {\n"<br>
+               "    gl_Position = gl_in[i].gl_Position;\n"<br>
+               "    EmitVertex();\n"<br>
+               "  }\n"<br>
+               "  EndPrimitive();\n"<br>
+               "}\n";<br>
+       static const char *tc_source =<br>
+               "in gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "    float gl_PointSize;\n"<br>
+               "    float gl_ClipDistance[];\n"<br>
+               "} gl_in[];\n"<br>
+               "\n"<br>
+               "out gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "    float gl_PointSize;\n"<br>
+               "    float gl_ClipDistance[];\n"<br>
+               "} gl_out[];\n"<br>
+               "\n"<br>
+               "layout( vertices = 3 )  out;\n"<br>
+               "void main( )\n"<br>
+               "{\n"<br>
+               "        gl_out[ gl_InvocationID ].gl_Position = gl_in[ gl_InvocationID ].gl_Position;\n"<br>
+               "        gl_TessLevelOuter[0] = 1.0;\n"<br>
+               "        gl_TessLevelOuter[1] = 1.0;\n"<br>
+               "        gl_TessLevelOuter[2] = 1.0;\n"<br>
+               "        gl_TessLevelInner[0] = 1.0;\n"<br>
+               "        gl_TessLevelInner[1] = 1.0;\n"<br>
+               "}\n";<br>
+       static const char *te_source =<br>
+               "in gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "    float gl_PointSize;\n"<br>
+               "    float gl_ClipDistance[];\n"<br>
+               "} gl_in[];\n"<br>
+               "\n"<br>
+               "out gl_PerVertex {\n"<br>
+               "    vec4 gl_Position;\n"<br>
+               "    float gl_PointSize;\n"<br>
+               "    float gl_ClipDistance[];\n"<br>
+               "};\n"<br></blockquote><div><br></div><div>I'm not terribly familiar with tessellation control and tessellation evaluation shaders, but I suspect they don't require redeclaring gl_PerVertex either.<br>

</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+               "\n"<br>
+               "layout( triangles, equal_spacing)  in;\n"<br>
+               " \n"<br>
+               "void main( )\n"<br>
+               "{\n"<br>
+               "        vec4 p0 = gl_in[0].gl_Position;\n"<br>
+               "        vec4 p1 = gl_in[1].gl_Position;\n"<br>
+               "        vec4 p2 = gl_in[2].gl_Position;\n"<br>
+               " \n"<br>
+               "        vec3 p = gl_TessCoord.xyz;\n"<br>
+               " \n"<br>
+               "        gl_Position = p0*p.x + p1*p.y + p2*p.z;\n"<br>
+               "}\n";<br>
+       const bool has_gs = piglit_get_gl_version() >= 32<br>
+               || piglit_is_extension_supported("GL_ARB_geometry_shader4");<br>
+       const bool has_tess =<br>
+               piglit_is_extension_supported("GL_ARB_tessellation_shader");<br></blockquote><div><br></div><div>Add "piglit_get_gl_version() >= 40 ||" to this conditional, in case there's a GL-4.0 compliant implementation that doesn't expose GL_ARB_tessellation_shader.<br>

</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+       piglit_require_gl_version(20);<br></blockquote><div><br></div><div>With my suggestion above about config.supports_gl_compat_version, this shouldn't be necessary.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+       piglit_require_extension("GL_ARB_separate_shader_objects");<br>
+<br>
+       pass = true;<br>
+<br>
+       if (piglit_get_gl_version() >= 43) {<br>
+               ver = 430;<br>
+       } else if(piglit_get_gl_version() >= 32) {<br>
+               ver = 150;<br>
+       } else {<br>
+               ver = 120;<br>
+       }<br>
+       snprintf(version, 99,<br>
+                "#version %d\n"<br>
+                "#extension GL_ARB_separate_shader_objects : enable\n\n",<br>
+                ver);<br>
+       version[99] = '\0';<br></blockquote><div><br></div><div>This should use asprintf().<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+<br>
+       shader_source[0] = version;<br>
+       if (piglit_get_gl_version() >= 40) {<br></blockquote><div><br></div><div>Shouldn't this be "if (has_tess)"?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+               shader_source[1] = tc_source;<br>
+               tcs = glCreateShaderProgramv(GL_TESS_CONTROL_SHADER, 2,<br>
+                                            shader_source);<br>
+               pass = piglit_link_check_status(tcs) && pass;<br>
+<br>
+               shader_source[1] = te_source;<br>
+               tes = glCreateShaderProgramv(GL_TESS_EVALUATION_SHADER, 2,<br>
+                                            shader_source);<br>
+               pass = piglit_link_check_status(tes) && pass;<br>
+       } else {<br>
+               tcs = 0;<br>
+               tes = 0;<br>
+       }<br>
+<br>
+       if (piglit_get_gl_version() >= 32) {<br></blockquote><div><br></div><div>Shouldn't this be "if (has_gs)"?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+               shader_source[1] = gs_source;<br>
+               gs = glCreateShaderProgramv(GL_GEOMETRY_SHADER, 2,<br>
+                                           shader_source);<br>
+               pass = piglit_link_check_status(gs) && pass;<br>
+       } else {<br>
+               gs = 0;<br>
+       }<br>
+<br>
+       shader_source[1] = fs_source;<br>
+       fs = glCreateShaderProgramv(GL_FRAGMENT_SHADER, 2, shader_source);<br>
+       pass = piglit_link_check_status(fs) && pass;<br>
+<br>
+       shader_source[1] = vs_source;<br>
+       vs = glCreateShaderProgramv(GL_VERTEX_SHADER, 2, shader_source);<br>
+       pass = piglit_link_check_status(vs) && pass;<br>
+<br>
+       glGenProgramPipelines(1, &pipe);<br>
+       pass = piglit_check_gl_error(GL_NO_ERROR) && pass;<br>
+<br>
+       glActiveShaderProgram(pipe, fs);<br>
+       glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, &param);<br>
+       if (param != fs) {<br>
+               fprintf(stderr, "Failed to get Active Program.\n");<br>
+               pass = false;<br>
+       }<br>
+       pass = piglit_check_gl_error(GL_NO_ERROR) && pass;<br>
+<br>
+       use_stage_and_check(pipe, vs, GL_VERTEX_SHADER, true);<br>
+       use_stage_and_check(pipe, fs, GL_FRAGMENT_SHADER, true);<br>
+       use_stage_and_check(pipe, gs, GL_GEOMETRY_SHADER, has_gs);<br>
+       use_stage_and_check(pipe, tes, GL_TESS_EVALUATION_SHADER, has_tess);<br>
+       use_stage_and_check(pipe, tcs, GL_TESS_CONTROL_SHADER, has_tess);<br>
+<br>
+       glActiveShaderProgram(pipe, vs);<br>
+       glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, &param);<br>
+       if (param != vs) {<br>
+               fprintf(stderr, "Failed to get Active Program.\n");<br>
+               pass = false;<br>
+       }<br>
+       pass = piglit_check_gl_error(GL_NO_ERROR) && pass;<br>
+<br>
+<br>
+       glUseProgramStages(pipe, GL_ALL_SHADER_BITS, 0);<br>
+       pass = piglit_check_gl_error(GL_NO_ERROR) && pass;<br>
+<br>
+       check_stage(pipe, 0, GL_VERTEX_SHADER, true);<br>
+       check_stage(pipe, 0, GL_FRAGMENT_SHADER, true);<br>
+       check_stage(pipe, 0, GL_GEOMETRY_SHADER, has_gs);<br>
+       check_stage(pipe, 0, GL_TESS_EVALUATION_SHADER, has_tess);<br>
+       check_stage(pipe, 0, GL_TESS_CONTROL_SHADER, has_tess);<br>
+<br>
+<br>
+       piglit_present_results();<br>
+       piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);<br>
+}<br>
<span><font color="#888888">--<br>
1.8.1.4<br></font></span></blockquote><div><br></div><div>If all of those are addressed, this patch is:<br><br></div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
</div></div></div></div>