On 09/07/2013 01:09 PM, Paul Berry wrote: > On 4 September 2013 12:57, Ian Romanick <[email protected] > <mailto:[email protected]>> wrote: > > From: Gregory Hainaut <[email protected] > <mailto:[email protected]>> > > 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 <[email protected] > <mailto:[email protected]>> > --- > 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 <[email protected] > <mailto:[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. > + */ > + > +/** > + * \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 > [email protected] <mailto:[email protected]> > http://lists.freedesktop.org/mailman/listinfo/piglit _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
