[Piglit] [PATCH 5/9] sso: Test mixing separable and non-separable programs in various ways
Paul Berry
stereotype441 at gmail.com
Sat Sep 7 11:09:04 PDT 2013
On 4 September 2013 12:57, Ian Romanick <idr at freedesktop.org> wrote:
> From: Gregory Hainaut <gregory.hainaut at gmail.com>
>
> the goal is to test the state mix of glUseProgram /
> glBindProgramPipeline / glActiveProgram.
>
> Ian R. quote:
> "In this case, either the UseProgram state or the
> BindProgramPipeline state. If UseProgram sets a non-zero program,
> that state is used. Otherwise the BindProgramPipeline state is
> used.....In this case, I think AMD's behavior is incorrect."
>
Could we have some additional context on this quote (what case are you
talking about? What behaviour of AMD's is incorrect?)
>
> Note: Nvidia seems to be fine.
>
> V4:
> * split into a separate commit
> * Merge the 2 vertex shaders with the help of the __VERSION__ macro
> * Properly set shader version with asprintf
> * Use standard bool
> * Split test into subtest
> * Fix expected of GL_PROGRAM_PIPELINE_BINDING (based on nvidia behavior)
>
> V5 (idr):
>
> * Expect GL_PROGRAM_PIPELINE_BINDING to still be pipe when a
> non-separable program is also bound (via glUseProgram). After
> discussions in Khronos, that was determined to be the correct
> behavior. Fix NVIDIA drivers should be available soon.
> * Don't test glUseProgramStages with a non-separable program. There is
> a separate test for that now.
> * Use 'pass = foo() && pass;' idiom instead of 'pass &= foo();'. C's
> short-circuit evaulation rules cause these to be different.
> * Use piglit_build_simple_program for non-separable shader program.
> * #extension is only necessary to use layout qualifiers, so remove it.
> * s/GLboolean/bool/g
> * Produce no output with -auto.
> * Use descriptive names for subtests.
> * Add test to CMakeLists.gl.txt.
> * Trivial reformatting.
>
> Note: vertex shader doesn't work with FGLRX. It would require GLSL150
> but that mean you can't use fixed pipeline anymore...
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
> tests/all.tests | 1 +
> .../arb_separate_shader_objects/CMakeLists.gl.txt | 1 +
> .../mix_pipeline_useprogram.c | 358
> +++++++++++++++++++++
> 3 files changed, 360 insertions(+)
> create mode 100644
> tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
>
> diff --git a/tests/all.tests b/tests/all.tests
> index 346556d..bfd6025 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -1240,6 +1240,7 @@ spec['ARB_separate_shader_objects'] =
> arb_separate_shader_objects
> arb_separate_shader_objects['GetProgramPipelineiv'] =
> concurrent_test('arb_separate_shader_object-GetProgramPipelineiv')
> arb_separate_shader_objects['IsProgramPipeline'] =
> concurrent_test('arb_separate_shader_object-IsProgramPipeline')
> arb_separate_shader_objects['UseProgramStages - non-separable program'] =
> concurrent_test('arb_separate_shader_object-UseProgramStages-non-separable')
> +arb_separate_shader_objects['Mix BindProgramPipeline and UseProgram'] =
> concurrent_test('arb_separate_shader_object-mix_pipeline_useprogram')
>
> # Group ARB_sampler_objects
> arb_sampler_objects = Group()
> diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> index 3397b4d..70ef99a 100644
> --- a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> @@ -11,4 +11,5 @@ link_libraries (
>
> piglit_add_executable (arb_separate_shader_object-GetProgramPipelineiv
> GetProgramPipelineiv.c)
> piglit_add_executable (arb_separate_shader_object-IsProgramPipeline
> IsProgramPipeline.c)
> +piglit_add_executable (arb_separate_shader_object-mix_pipeline_useprogram
> mix_pipeline_useprogram.c)
> piglit_add_executable
> (arb_separate_shader_object-UseProgramStages-non-separable
> UseProgramStages-non-separable.c)
> diff --git
> a/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
> b/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
> new file mode 100644
> index 0000000..8761a0e
> --- /dev/null
> +++ b/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c
> @@ -0,0 +1,358 @@
> +/*
> + * 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.
> + */
> +
> +/**
> + * \file mix_pipeline_useprogram.
> + * Test mixing separable and non-separable programs in various ways.
> + *
> + * Section 2.11.3 (Program Objects) of the OpenGL 4.1 spec says:
> + *
> + * "The executable code for an individual shader stage is taken from
> the
> + * current program for that stage. If there is a current program
> object
> + * established by UseProgram, that program is considered current for
> all
> + * stages. Otherwise, if there is a bound program pipeline object (see
> + * section 2.11.4), the program bound to the appropriate stage of the
> + * pipeline object is considered current. If there is no current
> program
> + * object or bound program pipeline object, no program is current for
> any
> + * stage."
> +
> + * Section 2.11.4 (Program Pipeline Objects) of the OpenGL 4.1 spec says:
> + *
> + * "If no current program object has been established by UseProgram,
> the
> + * program objects used for each shader stage and for uniform updates
> are
> + * taken from the bound program pipeline object, if any. If there is a
> + * current program object established by UseProgram, the bound program
> + * pipeline object has no effect on rendering or uniform updates.
> When a
> + * bound program pipeline object is used for rendering, individual
> shader
> + * executables are taken from its program objects as described in the
> + * discussion of UseProgram in section 2.11.3)."
> + *
> + * Section 2.11.7 (Uniform Variables) of the OpenGL 4.1 spec says:
> + *
> + * "If a non-zero program object is bound by UseProgram, it is the
> active
> + * program object whose uniforms are updated by these commands. If no
> + * program object is bound using UseProgram, the active program
> object of
> + * the current program pipeline object set by ActiveShaderProgram is
> the
> + * active program object. If the current program pipeline object has
> no
> + * active program or there is no current program pipeline object, then
> + * there is no active program."
> + *
> + * Note that with these rules, it's not possible to mix program objects
> bound
> + * to the context with program objects bound to a program pipeline
> object; if
> + * any program is bound to the context, the current pipeline object is
> + * ignored.
> + */
> +#include "piglit-util-gl-common.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> +
> + config.window_width = 32;
> + config.window_height = 32;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static GLuint prog;
> +static GLuint pipe;
> +static bool pass = true;
> +static bool subtest = true;
> +
> +static bool
> +check(GLenum pname, GLint expected)
>
This is a nit-pick, but several of the tests in this series include
functions with names like "check", and I'm having trouble keeping track of
what is checked by each one. Could we rename this to something like
"check_Integerv"?
> +{
> + GLint value = 0;
> + glGetIntegerv(pname, &value);
> +
> + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> + if (value != expected) {
> + fprintf(stderr, "Failed to get %s expected %d got %d\n",
> + piglit_get_gl_enum_name(pname), expected, value);
> + return false;
> + } else {
> + return true;
> + }
> +}
> +
> +static void
> +bind_program(bool enable)
> +{
> + if (enable) {
> + glUseProgram(prog);
> + if (!piglit_automatic)
> + printf("Enable monolithic program\n");
> + subtest = check(GL_CURRENT_PROGRAM, prog) && subtest;
> + } else {
> + glUseProgram(0);
> + if (!piglit_automatic)
> + printf("Disable monolithic program\n");
> + subtest = check(GL_CURRENT_PROGRAM, 0) && subtest;
> + }
> +}
> +
> +static void
> +bind_pipeline(bool enable)
> +{
> + if (enable) {
> + glBindProgramPipeline(pipe);
> + if (!piglit_automatic)
> + printf("Bind pipeline\n");
> + } else {
> + glBindProgramPipeline(0);
> + if (!piglit_automatic)
> + printf("Unbind pipeline\n");
> + }
> +}
>
It seems inconsistent that bind_program calls check(GL_CURRENT_PROGRAM) but
bind_pipeline doesn't call check(GL_PROGRAM_PIPELINE_BINDING). But I'm not
entirely sure what kinds of bugs we're trying to guard against with these
checks, so I don't really have a good suggestion for what to do instead.
Personally, I'd be tempted to remove all the calls to check() entirely,
since they seem kind of tangential to the primary purpose of the test.
That would make the printf's unnecessary, at which point we could just
replace bind_program and bind_pipeline with direct calls to glUseProgram()
and glBindProgramPipeline(), which would make the test a whole lot easier
to follow.
> +
> +static bool
> +draw(float expected[4])
> +{
> + piglit_draw_rect(-1, -1, 2, 2);
> + return piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
> + expected);
> +}
> +
> +static bool
> +set_and_check_uniform(GLint program, float expected)
> +{
> + float value;
> + glUniform1f(0, expected);
> + glGetUniformfv(program, 0, &value);
> + if (value != expected) {
> + fprintf(stderr,
> + "Failed to get uniform value of %d, expected %f, "
> + "got %f\n",
> + program, expected, value);
> + return false;
> + }
> + return true;
> +}
> +
> +static void
> +report_subtest(const char* msg)
> +{
> + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL,
> msg);
> +
> + pass = subtest && pass;
> + subtest = true;
>
I find it really hard to keep track of the use of the "pass" and "subtest"
globals in this test. Could we switch to an approach where each function
that needs to keep track of pass/fail has a local "pass" boolean, which it
initializes to true, sets to false in the event of failure, and returns at
the end of the function? We do that in a lot of other tests and I've never
had trouble following that.
Side question, since I've never written a test using subtests before: is it
necessary for the test to keep track of an aggregate pass/fail state, and
report that correctly at the conclusion of the test, or is it ok to report
subtest results and then always return PIGLIT_PASS from piglit_display()?
I'm assuming it's the former, although I wish it were the latter :)
> +}
>
Would you have any objection to moving piglit_init before piglit_display in
this test? A long time ago I started doing that in my piglit tests because
I usually find it easier to follow the logic of piglit_dispaly once I've
seen piglit_init. That seems particularly true of this test, since the
shader source code is contained in piglit_init.
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + GLint active_shader_pipe;
> + GLint uniform_loc_pipe;
> + GLint uniform_loc_prog;
> + float red[4] = {1.0, 0.0, 0.0, 1.0};
> + float green[4] = {0.0, 1.0, 0.0, 1.0};
> + float blue[4] = {0.0, 0.0, 1.0, 1.0};
> + float gb[4] = {0.0, 1.0, 1.0, 1.0};
> +
> + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM,
> &active_shader_pipe);
> + uniform_loc_pipe = glGetUniformLocation(active_shader_pipe,
> "blue");
> + uniform_loc_prog = glGetUniformLocation(prog, "blue");
> + /* otherwise it is difficult which program is really updated */
> + assert(uniform_loc_prog == 0);
> + assert(uniform_loc_pipe == 0);
>
This seems really dodgy to me. My understanding is that OpenGL makes no
guarantees as to where any particular uniform is located, even in the
circumstance we have here where only one uniform exists.
IMHO we should write the test so that (a) if the "blue" uniform gets
assigned the same nonzero location in both prog and active_shader_pipe, the
test is still just as effective, and (b) if the "blue" uniform gets
assigned different locations in prog and active_shader_pipe, the test still
passes (but it's ok if it's a less mean test in this case). I think all
that would be required is to add a "location" argument to
set_and_check_uniform, and pass either uniform_loc_prog or uniform_loc_pipe
to it at each call site, as appropriate.
> +
> + /* wrong setup: stop here */
> + if (!pass)
> + return PIGLIT_FAIL;
> +
> + /* Set up fixed function to draw blue if we lose our shader. */
> + glColor4f(0.0, 0.0, 1.0, 1.0);
> +
> + /* TEST 1: BindPipeline after UseProgram */
> + if (!piglit_automatic)
> + printf("glUseProgram, then glBindPipeline...\n");
>
It's confusing that so many of the comments and printfs in this test refer
to the non-existent function "glBindPipeline", when the actual GL function
is glBindProgramPipeline.
> + bind_program(true);
> + subtest = set_and_check_uniform(prog, 1.0) && subtest;
> + bind_pipeline(true);
> + /* It must ignore the pipeline */
> + subtest = set_and_check_uniform(prog, 0.0) && subtest;
>
+ /* But the pipeline is attached to the binding point */
> + subtest = check(GL_PROGRAM_PIPELINE_BINDING, pipe) && subtest;
> +
> + /* UseProgram rendering */
> + subtest = draw(red) && subtest;
> +
> + report_subtest("glUseProgram, then glBindPipeline");
>
I would find this a lot easier to read if each subtest were contained in
its own function. That, combined with my suggestion above about making the
pass/fail booleans local, would have the advantage of avoiding the need for
separate "subtest" and "pass" booleans--each subtest would have its own
"pass" boolean, which it would report to piglit_report_subtest_result() and
then return to piglit_display(). piglit_display() would keep track of the
aggregate "pass" boolean for the whole test.
> +
> + /* TEST 2: BindPipeline without UseProgram */
> + if (!piglit_automatic)
> + printf("glBindPipeline without glUseProgram...\n");
> + bind_program(false);
> + bind_pipeline(true);
> + subtest = set_and_check_uniform(active_shader_pipe, 1.0) &&
> subtest;
> + subtest = check(GL_PROGRAM_PIPELINE_BINDING, pipe) && subtest;
> +
> + /* Pipeline rendering */
> + subtest = draw(green) && subtest;
> +
> + report_subtest("glBindPipeline without glUseProgram");
> +
> + /* TEST 3: UseProgram after BindPipeline */
> + if (!piglit_automatic)
> + printf("glBindPipeline, then glUseProgram...\n");
> + bind_program(true);
> + subtest = check(GL_PROGRAM_PIPELINE_BINDING, pipe) && subtest;
> +
> + report_subtest("glBindPipeline, then glUseProgram");
>
Shouldn't this call to report_subtest() be after the call to draw(red) that
follows? As written, a failure in the draw call will be classified as a
failure in subtest 4.
>
> +
> + /* UseProgram rendering */
> + subtest = draw(red) && subtest;
>
It would also be nice if each subtest started and ended in a GL state where
the current program and the currently bound pipeline are both 0, and if
subtests didn't rely on uniform settings from previous subtests. It took
me a long time to reassure myself that subtest 3 was correct, because it
relies implicitly on the fact that (a) subtest 2 leaves the pipeline bound,
and (b) subtest 1 leaves 0.0 in prog's value of the "blue" uniform.
> +
> + /* TEST 4: UseProgram(0) after BindPipeline */
> + if (!piglit_automatic)
> + printf("glBindPipeline, then glUseProgram(0)...\n");
> + bind_program(false);
> + bind_pipeline(true);
>
The reliance on previous state is particularly confusing here, since it
looks like we are calling UseProgram(0) *before* BindProgramPipeline. To
understand that the test is correct, you have to look all the way back to
subtest 2 and see that it leaves the program pipeline bound, so the call to
UseProgram(0) (inside bind_program) is indeed after a call to
BindProgramPipeline. I'd far prefer to see this:
bind_program(true);
bind_pipeline(true);
bind_program(false);
since it clearly captures the intent of the subtest.
> + subtest = check(GL_PROGRAM_PIPELINE_BINDING, pipe) && subtest;
> +
> + /* Sanity check */
> + /* Pipeline rendering */
> + subtest = draw(green) && subtest;
> +
> + bind_program(false);
> +
> + /* Pipeline rendering */
> + subtest = draw(green) && subtest;
> +
> + bind_pipeline(true);
> + subtest = check(GL_PROGRAM_PIPELINE_BINDING, pipe) && subtest;
> + subtest = draw(green) && subtest;
> +
> + report_subtest("glBindPipeline, then glUseProgram(0)");
> +
> + /* TEST 5: like previous test but use a real program before
> + * UseProgram(0)
> + */
> + if (!piglit_automatic)
> + printf("glUseProgram, then glBindPipeline, "
> + "then glUseProgram(0)...\n");
>
I don't understand the relationship between this summary and the test that
follows it. Based on the summary, I expect to see a call to
glBindProgramPipeline(pipe) happen at a time when the current program is
nonzero. But I don't see that happening anywhere in the subtest.
> + bind_program(false);
> + bind_pipeline(true);
> +
> + /* Sanity check */
> + /* Pipeline rendering */
> + subtest = draw(green) && subtest;
> +
> + /* Set wrong uniform value */
> + subtest = set_and_check_uniform(active_shader_pipe, 0.0) &&
> subtest;
> +
> + bind_program(true);
> + bind_program(false);
> +
> + /* Pipeline rendering: with bad uniform */
> + subtest = draw(gb) && subtest;
> +
> + /* pipeline program must be still active */
> + subtest = set_and_check_uniform(active_shader_pipe, 1.0) &&
> subtest;
> +
> + bind_pipeline(true);
> + /* Pipeline rendering */
> + subtest = draw(green) && subtest;
> +
> + report_subtest("glUseProgram, then glBindPipeline, "
> + "then glUseProgram(0)");
> +
> + /* TEST 6: Sanity check */
> + if (!piglit_automatic)
> + printf("Final sanity test...\n");
> + bind_program(false);
> + bind_pipeline(false);
> + /* Fixed function rendering */
> + subtest = draw(blue) && subtest;
> +
> + report_subtest("Final sanity test");
>
The only parts of this test that rely on compatibility behaviour are the
VS's use of gl_Vertex, and this final sanity test. It seems to me that the
final sanity test is tangential to the primary purpose of the test; I'd
rather see this subtest removed, and gl_Vertex changed to a user-defined
vertex attribute, so that we can add supports_gl_core_version to the config
block.
> +
> + piglit_present_results();
> +
> + return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> +}
> +
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + GLint vs_prog;
> + GLint fs_prog;
> + GLint vs, fs;
> + const char *vs_source =
> + "#version 110\n"
> + "void main()\n"
> + "{\n"
> + " gl_Position = gl_Vertex;\n"
> + "}\n";
> + const char *fs_source_r =
> + "#version 110\n"
> + "uniform float blue;\n"
> + "void main()\n"
> + "{\n"
> + " gl_FragColor = vec4(1.0, 0.0, blue, 1.0);\n"
> + "}\n";
> + const char *fs_source_g =
> + "#version 110\n"
> + "uniform float blue;\n"
> + "void main()\n"
> + "{\n"
> + " gl_FragColor = vec4(0.0, 1.0, 1.0 - blue, 1.0);\n"
> + "}\n";
> +
> + pass = true;
> +
> + piglit_require_gl_version(20);
> + piglit_require_extension("GL_ARB_separate_shader_objects");
> +
> + /* Standard program (ie not separate) */
> + prog = piglit_build_simple_program(vs_source, fs_source_r);
> +
> + /* Now create program for the pipeline program */
> + vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vs_source);
> + fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, fs_source_g);
> +
> + vs_prog = glCreateProgram();
> + glProgramParameteri(vs_prog, GL_PROGRAM_SEPARABLE, GL_TRUE);
> + glAttachShader(vs_prog, vs);
> + glLinkProgram(vs_prog);
> + pass = piglit_link_check_status(vs_prog) && pass;
> + glDeleteShader(vs);
> +
> + fs_prog = glCreateProgram();
> + glProgramParameteri(fs_prog, GL_PROGRAM_SEPARABLE, GL_TRUE);
> + glAttachShader(fs_prog, fs);
> + glLinkProgram(fs_prog);
> + pass = piglit_link_check_status(fs_prog) && pass;
> + glDeleteShader(fs);
> +
> + glGenProgramPipelines(1, &pipe);
> + glUseProgramStages(pipe, GL_VERTEX_SHADER_BIT, vs_prog);
> + glUseProgramStages(pipe, GL_FRAGMENT_SHADER_BIT, fs_prog);
> + glActiveShaderProgram(pipe, fs_prog);
> + pass = piglit_program_pipeline_check_status(pipe) && pass;
> +
> + pass = check(GL_PROGRAM_PIPELINE_BINDING, 0) && pass;
> + pass = check(GL_CURRENT_PROGRAM, 0) && pass;
> +
> + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> +}
> --
> 1.8.1.4
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130907/aa9b6a11/attachment-0001.html>
More information about the Piglit
mailing list