<div dir="ltr">On 4 September 2013 12:57, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com" target="_blank">gregory.hainaut@gmail.com</a>> 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." </blockquote><div> </div><div>Could we have some additional context on this quote (what case are you talking about? What behaviour of AMD's is incorrect?) </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> --- 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 <<a href="mailto:gregory.hainaut@gmail.com" target="_blank">gregory.hainaut@gmail.com</a>> + * + * 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) </blockquote><div> </div><div>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"? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +{ + 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"); + } +} </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +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; </blockquote><div> </div><div>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. </div><div>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 :) </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +} </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +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); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* 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"); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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; </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + /* 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"); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* 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"); </blockquote><div> </div>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. </div><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* UseProgram rendering */ + subtest = draw(red) && subtest; </blockquote><div> <div>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. </div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* TEST 4: UseProgram(0) after BindPipeline */ + if (!piglit_automatic) + printf("glBindPipeline, then glUseProgram(0)...\n"); + bind_program(false); + bind_pipeline(true); </blockquote><div> </div><div>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: </div><div>bind_program(true); bind_pipeline(true); </div><div>bind_program(false); since it clearly captures the intent of the subtest. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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"); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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"); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + 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 <a href="mailto:Piglit@lists.freedesktop.org" target="_blank">Piglit@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a> </blockquote></div> </div></div>