[Piglit] [PATCH 2/9] sso: new test GetProgramPipelineiv
Paul Berry
stereotype441 at gmail.com
Fri Sep 6 14:50:21 PDT 2013
On 4 September 2013 12:57, Ian Romanick <idr at freedesktop.org> wrote:
> From: Gregory Hainaut <gregory.hainaut at gmail.com>
>
> 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 <gregory.hainaut at gmail.com>
> + *
> + * 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 <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130906/be0a371b/attachment-0001.html>
More information about the Piglit
mailing list