[Piglit] [PATCH 5/9] sso: Test mixing separable and non-separable programs in various ways
Ian Romanick
idr at freedesktop.org
Mon Sep 9 11:48:52 PDT 2013
On 09/07/2013 01:09 PM, Paul Berry wrote:
> On 4 September 2013 12:57, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> From: Gregory Hainaut <gregory.hainaut at gmail.com
> <mailto: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?)
This came from a discussion that we had on IRC a very long time ago, so
my recollection may be slightly off.
The spec says, basically, that glUniform* always uses the program set by
glUseProgram. If no program is set by glUseProgram, glUniform* uses the
glActiveShaderProgram set on the pipeline bound by glBindProgramPipeline.
My recollection is that Gregory observed AMD using whichever state
(glUseProgram or glBindProgramPipeline) was most recently set.
I think we need to re-run on AMD to get concrete data to put in the
commit message.
> 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
> <mailto: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
> <mailto: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"?
Agreed.
> +{
> + 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 :)
Between this and the inconsistencies you identify below, I decided to do
a fairly major refactor / rewrite of this test. I should have this out
on the list pretty soon.
> +}
>
>
> 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 <mailto:Piglit at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list