On 4 September 2013 12:57, Ian Romanick <[email protected]> wrote:
> From: Gregory Hainaut <[email protected]> > > Bind various stages (including geometry and tesselation) to the pipeline > and check the result with GetProgramPipelineiv. I take also the > opportunity to use glCreateShaderProgramv. > > AMD generates an GL_INVALID_VALUE error when glActiveShaderProgram is > called with a program that isn't bound to any stage to the pipeline. No > such error is specified in the spec, so this may be their bug. Ian has > submitted a GL spec bug for this issue. It seems likely that the next > version of the GL spec will include this as an error. We can update the > test when that happens. > > Spec quote: > > "An INVALID_VALUE error is generated if program is not the name of > either a program or shader object. An INVALID_OPERATION error is > generated if program is the name of a shader object." > > V4: > * split new test in a separate commit > * Merge the 2 vertex shaders with the help of the __VERSION__ macro > * Properly set shader version with asprintf > > V5 (idr): > * Use 'pass = foo() && pass;' idiom instead of 'pass &= foo();'. C's > short-circuit evaulation rules cause these to be different. > * s/GLboolean/bool/g > * Produce no output with -auto. > * Don't add tests from future commits to CMakeLists.gl.txt yet. > * Trivial reformatting. > --- > tests/all.tests | 5 + > tests/spec/CMakeLists.txt | 1 + > .../arb_separate_shader_objects/CMakeLists.gl.txt | 12 + > .../arb_separate_shader_objects/CMakeLists.txt | 1 + > .../GetProgramPipelineiv.c | 291 > +++++++++++++++++++++ > 5 files changed, 310 insertions(+) > create mode 100644 > tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > create mode 100644 tests/spec/arb_separate_shader_objects/CMakeLists.txt > create mode 100644 > tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c > > diff --git a/tests/all.tests b/tests/all.tests > index dfbb756..9464244 100644 > --- a/tests/all.tests > +++ b/tests/all.tests > @@ -1234,6 +1234,11 @@ add_concurrent_test(arb_occlusion_query, > 'occlusion_query_meta_fragments') > add_concurrent_test(arb_occlusion_query, > 'occlusion_query_meta_no_fragments') > add_concurrent_test(arb_occlusion_query, 'occlusion_query_order') > > +# Group ARB_separate_shader_objects > +arb_separate_shader_objects = Group() > +spec['ARB_separate_shader_objects'] = arb_separate_shader_objects > +arb_separate_shader_objects['GetProgramPipelineiv'] = > concurrent_test('arb_separate_shader_object-GetProgramPipelineiv') > + > # Group ARB_sampler_objects > arb_sampler_objects = Group() > spec['ARB_sampler_objects'] = arb_sampler_objects > diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt > index 17c991a..8057d20 100644 > --- a/tests/spec/CMakeLists.txt > +++ b/tests/spec/CMakeLists.txt > @@ -20,6 +20,7 @@ add_subdirectory (arb_sampler_objects) > add_subdirectory (arb_seamless_cube_map) > add_subdirectory (amd_seamless_cubemap_per_texture) > add_subdirectory (amd_vertex_shader_layer) > +add_subdirectory (arb_separate_shader_objects) > add_subdirectory (arb_shader_texture_lod/execution) > add_subdirectory (arb_shader_objects) > add_subdirectory (arb_shading_language_420pack/execution) > diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > new file mode 100644 > index 0000000..2ea2c80 > --- /dev/null > +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > @@ -0,0 +1,12 @@ > +include_directories( > + ${GLEXT_INCLUDE_DIR} > + ${OPENGL_INCLUDE_PATH} > +) > + > +link_libraries ( > + piglitutil_${piglit_target_api} > + ${OPENGL_gl_LIBRARY} > + ${OPENGL_glu_LIBRARY} > +) > + > +piglit_add_executable (arb_separate_shader_object-GetProgramPipelineiv > GetProgramPipelineiv.c) > diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.txt > b/tests/spec/arb_separate_shader_objects/CMakeLists.txt > new file mode 100644 > index 0000000..144a306 > --- /dev/null > +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.txt > @@ -0,0 +1 @@ > +piglit_include_target_api() > diff --git a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c > b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c > new file mode 100644 > index 0000000..f933fba > --- /dev/null > +++ b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c > @@ -0,0 +1,291 @@ > +/* > + * Copyright © 2013 Gregory Hainaut <[email protected]> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the > "Software"), > + * to deal in the Software without restriction, including without > limitation > + * the rights to use, copy, modify, merge, publish, distribute, > sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the > next > + * paragraph) shall be included in all copies or substantial portions of > the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + */ > + > +#include "piglit-util-gl-common.h" > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 10; > This should be: config.supports_gl_compat_version = 20; config.supports_gl_core_version = 31; > + > + config.window_width = 32; > + config.window_height = 32; > + > +PIGLIT_GL_TEST_CONFIG_END > + > +static bool pass; > + > +enum piglit_result > +piglit_display(void) > +{ > + /* UNREACHED */ > + return PIGLIT_FAIL; > +} > + > +static GLbitfield > +stage2bitfield(GLint stage) > +{ > + switch(stage) { > + case GL_VERTEX_SHADER: return GL_VERTEX_SHADER_BIT; > + case GL_FRAGMENT_SHADER: return GL_FRAGMENT_SHADER_BIT; > + case GL_GEOMETRY_SHADER: return GL_GEOMETRY_SHADER_BIT; > + case GL_TESS_CONTROL_SHADER: return > GL_TESS_CONTROL_SHADER_BIT; > + case GL_TESS_EVALUATION_SHADER:return > GL_TESS_EVALUATION_SHADER_BIT; > + case GL_COMPUTE_SHADER: return GL_COMPUTE_SHADER_BIT; > + default:return 0; > 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? > + } > + return 0; > This return statement is unreachable. > +} > + > +static void > +check_stage(GLint pipe, GLint expected, GLint stage, bool supported) > +{ > + GLint param = 0; > + glGetProgramPipelineiv(pipe, stage, ¶m); > + > + if (!supported) { > + pass = piglit_check_gl_error(GL_INVALID_ENUM) && pass; > + } else if (param != expected) { > + fprintf(stderr, "Failed to get program of stage %s.\n", > + piglit_get_gl_enum_name(stage)); > + pass = false; > + } > +} > + > +static void > +use_stage_and_check(GLint pipe, GLint program, GLint stage, bool > supported) > +{ > + if (!piglit_automatic) { > + printf("Attach program (%d) to stage (%s). Expected to be " > + "supported: %s\n", > + program, > + piglit_get_gl_enum_name(stage), > + supported ? "yes" : "no"); > 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. > + } > + > + glUseProgramStages(pipe, stage2bitfield(stage), program); > + if (!supported) { > + pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass; > + } else { > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + } > + > + check_stage(pipe, program, stage, supported); > +} > + > +void > +piglit_init(int argc, char **argv) > +{ > + GLint vs, fs, gs, tcs, tes; > + GLint ver; > + GLuint pipe = 0; > + GLint param = 0; > + char version[100]; > + const char *shader_source[2]; > + > + static const char *vs_source = > + "#if __VERSION__ > 140\n" > + "out gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + "};\n" > There's no need to redeclare this since we're not changing anything from the default declaration. > + "\n" > + "in vec4 position;\n" > + "#endif\n" > + "\n" > + "void main()\n" > + "{\n" > + "#if __VERSION__ > 140\n" > + " gl_Position = position;\n" > + "#else\n" > + " gl_Position = gl_Vertex;\n" > + "#endif\n" > + "}\n"; > + static const char *fs_source = > + "void main()\n" > + "{\n" > + " gl_FragColor = vec4(0.0, 1.0, 0.0, 0.0);\n" > + "}\n"; > + static const char *gs_source = > + "in gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + " float gl_PointSize;\n" > + " float gl_ClipDistance[];\n" > + "} gl_in[];\n" > + "\n" > + "out gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + " float gl_PointSize;\n" > + " float gl_ClipDistance[];\n" > + "};\n" > These redeclarations aren't needed either. > + "\n" > + "layout(triangles) in;\n" > + "layout(triangle_strip, max_vertices = 3) out;\n" > + "void main() {\n" > + " for(int i = 0; i < gl_in.length(); i++) {\n" > + " gl_Position = gl_in[i].gl_Position;\n" > + " EmitVertex();\n" > + " }\n" > + " EndPrimitive();\n" > + "}\n"; > + static const char *tc_source = > + "in gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + " float gl_PointSize;\n" > + " float gl_ClipDistance[];\n" > + "} gl_in[];\n" > + "\n" > + "out gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + " float gl_PointSize;\n" > + " float gl_ClipDistance[];\n" > + "} gl_out[];\n" > + "\n" > + "layout( vertices = 3 ) out;\n" > + "void main( )\n" > + "{\n" > + " gl_out[ gl_InvocationID ].gl_Position = gl_in[ > gl_InvocationID ].gl_Position;\n" > + " gl_TessLevelOuter[0] = 1.0;\n" > + " gl_TessLevelOuter[1] = 1.0;\n" > + " gl_TessLevelOuter[2] = 1.0;\n" > + " gl_TessLevelInner[0] = 1.0;\n" > + " gl_TessLevelInner[1] = 1.0;\n" > + "}\n"; > + static const char *te_source = > + "in gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + " float gl_PointSize;\n" > + " float gl_ClipDistance[];\n" > + "} gl_in[];\n" > + "\n" > + "out gl_PerVertex {\n" > + " vec4 gl_Position;\n" > + " float gl_PointSize;\n" > + " float gl_ClipDistance[];\n" > + "};\n" > I'm not terribly familiar with tessellation control and tessellation evaluation shaders, but I suspect they don't require redeclaring gl_PerVertex either. > + "\n" > + "layout( triangles, equal_spacing) in;\n" > + " \n" > + "void main( )\n" > + "{\n" > + " vec4 p0 = gl_in[0].gl_Position;\n" > + " vec4 p1 = gl_in[1].gl_Position;\n" > + " vec4 p2 = gl_in[2].gl_Position;\n" > + " \n" > + " vec3 p = gl_TessCoord.xyz;\n" > + " \n" > + " gl_Position = p0*p.x + p1*p.y + p2*p.z;\n" > + "}\n"; > + const bool has_gs = piglit_get_gl_version() >= 32 > + || > piglit_is_extension_supported("GL_ARB_geometry_shader4"); > + const bool has_tess = > + > piglit_is_extension_supported("GL_ARB_tessellation_shader"); > 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. > + > + piglit_require_gl_version(20); > With my suggestion above about config.supports_gl_compat_version, this shouldn't be necessary. > + piglit_require_extension("GL_ARB_separate_shader_objects"); > + > + pass = true; > + > + if (piglit_get_gl_version() >= 43) { > + ver = 430; > + } else if(piglit_get_gl_version() >= 32) { > + ver = 150; > + } else { > + ver = 120; > + } > + snprintf(version, 99, > + "#version %d\n" > + "#extension GL_ARB_separate_shader_objects : enable\n\n", > + ver); > + version[99] = '\0'; > This should use asprintf(). > + > + shader_source[0] = version; > + if (piglit_get_gl_version() >= 40) { > Shouldn't this be "if (has_tess)"? > + shader_source[1] = tc_source; > + tcs = glCreateShaderProgramv(GL_TESS_CONTROL_SHADER, 2, > + shader_source); > + pass = piglit_link_check_status(tcs) && pass; > + > + shader_source[1] = te_source; > + tes = glCreateShaderProgramv(GL_TESS_EVALUATION_SHADER, 2, > + shader_source); > + pass = piglit_link_check_status(tes) && pass; > + } else { > + tcs = 0; > + tes = 0; > + } > + > + if (piglit_get_gl_version() >= 32) { > Shouldn't this be "if (has_gs)"? > + shader_source[1] = gs_source; > + gs = glCreateShaderProgramv(GL_GEOMETRY_SHADER, 2, > + shader_source); > + pass = piglit_link_check_status(gs) && pass; > + } else { > + gs = 0; > + } > + > + shader_source[1] = fs_source; > + fs = glCreateShaderProgramv(GL_FRAGMENT_SHADER, 2, shader_source); > + pass = piglit_link_check_status(fs) && pass; > + > + shader_source[1] = vs_source; > + vs = glCreateShaderProgramv(GL_VERTEX_SHADER, 2, shader_source); > + pass = piglit_link_check_status(vs) && pass; > + > + glGenProgramPipelines(1, &pipe); > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + > + glActiveShaderProgram(pipe, fs); > + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, ¶m); > + if (param != fs) { > + fprintf(stderr, "Failed to get Active Program.\n"); > + pass = false; > + } > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + > + use_stage_and_check(pipe, vs, GL_VERTEX_SHADER, true); > + use_stage_and_check(pipe, fs, GL_FRAGMENT_SHADER, true); > + use_stage_and_check(pipe, gs, GL_GEOMETRY_SHADER, has_gs); > + use_stage_and_check(pipe, tes, GL_TESS_EVALUATION_SHADER, > has_tess); > + use_stage_and_check(pipe, tcs, GL_TESS_CONTROL_SHADER, has_tess); > + > + glActiveShaderProgram(pipe, vs); > + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, ¶m); > + if (param != vs) { > + fprintf(stderr, "Failed to get Active Program.\n"); > + pass = false; > + } > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + > + > + glUseProgramStages(pipe, GL_ALL_SHADER_BITS, 0); > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + > + check_stage(pipe, 0, GL_VERTEX_SHADER, true); > + check_stage(pipe, 0, GL_FRAGMENT_SHADER, true); > + check_stage(pipe, 0, GL_GEOMETRY_SHADER, has_gs); > + check_stage(pipe, 0, GL_TESS_EVALUATION_SHADER, has_tess); > + check_stage(pipe, 0, GL_TESS_CONTROL_SHADER, has_tess); > + > + > + piglit_present_results(); > + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL); > +} > -- > 1.8.1.4 > If all of those are addressed, this patch is: Reviewed-by: Paul Berry <[email protected]>
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
