<div dir="ltr">On 1 October 2013 18:22, 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">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>Last time I reviewed this patch, I asked for additional context on this quote. Can you put the additional context into the commit message so that in the future people don't have to go to the mailing list archives to understand the quote? Thanks. </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. V6 (idr): Major refactor / rewrite based on feedback from Paul. * Each subtest is moved to its own function with its own "pass" tracking. * Each subtest restored program, pipeline, and uniform state before ending. Tests also do not depend on state left over from previous tests. * The bind_program and bind_pipeline functions are eliminated. * s/BindPipeline/BindProgramPipeline/g * Paul also suggested moving piglit_init earlier in the file. This has not been done yet because I wasn't sure where to put it. First? After the utility functions? After the subtest functions? 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">ian.d.romanick@intel.com</a>> --- tests/all.tests | 1 + .../arb_separate_shader_objects/CMakeLists.gl.txt | 1 + .../mix_pipeline_useprogram.c | 415 +++++++++++++++++++++ 3 files changed, 417 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 ee6219c..e0cdc8b 100644 --- a/tests/all.tests +++ b/tests/all.tests @@ -1275,6 +1275,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') arb_separate_shader_objects['ProgramUniform coverage'] = concurrent_test('arb_separate_shader_object-ProgramUniform-coverage') arb_separate_shader_objects['ValidateProgramPipeline'] = concurrent_test('arb_separate_shader_object-ValidateProgramPipeline') diff --git a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt index d78f270..0ed3e2f 100644 --- a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt @@ -11,6 +11,7 @@ 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-ProgramUniform-coverage ProgramUniform-coverage.c) piglit_add_executable (arb_separate_shader_object-UseProgramStages-non-separable UseProgramStages-non-separable.c) piglit_add_executable (arb_separate_shader_object-ValidateProgramPipeline ValidateProgramPipeline.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..7c7fb63 --- /dev/null +++ b/tests/spec/arb_separate_shader_objects/mix_pipeline_useprogram.c @@ -0,0 +1,415 @@ +/* + * Copyright © 2013 Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com">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 + + /* Require OpenGL 2.0+, but do *not* allow a core profile. The test + * uses GLSL 1.10, and GLSL less than 1.30 is deprecated. In + * addition, the test tries to use fixed-function drawing. + */ + config.supports_gl_compat_version = 20; + + config.window_width = 32; + config.window_height = 32; + +PIGLIT_GL_TEST_CONFIG_END + +static GLuint prog; +static GLuint pipe; + +static const float red[4] = {1.0, 0.0, 0.0, 1.0}; +static const float violet[4] = {1.0, 0.0, 1.0, 1.0}; +static const float green[4] = {0.0, 1.0, 0.0, 1.0}; +static const float blue[4] = {0.0, 0.0, 1.0, 1.0}; +static const float gb[4] = {0.0, 1.0, 1.0, 1.0}; + +static bool +check_GetInteger_value(GLenum pname, GLint expected) +{ + bool pass = true; + 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 bool +restore_default_program_state(void) +{ + GLint active_shader_pipe; + + glUseProgram(0); + glBindProgramPipeline(0); + + /* Reset uniform values to known state. + */ + glProgramUniform1f(prog, 0, 0.0); + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, &active_shader_pipe); + + glProgramUniform1f(active_shader_pipe, 0, 0.0); </blockquote><div> </div><div>Is ACTIVE_PROGRAM known to be nonzero at this point? If not, the above call to glProgramUniform1f() will cause an INVALID_VALUE error. It would be nice to have either a comment here explaining why ACTIVE_PROGRAM is known not to be zero, or a test: if (active_shader_pipe != 0) </div><div> glProgramUniform1f(...); </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"> + + return piglit_check_gl_error(GL_NO_ERROR); +} + +static bool +draw(const float *expected) +{ + 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; + + /* glUniform* uses either the currently bound program or the active + * program of the currently bound pipeline. glGetUniform* uses the + * program expclitly passed to it. THESE MAY BE DIFFERENT! + */ + 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 bool +test_UseProgram_then_BindProgramPipeline(void) +{ + static const char subtest_name[] = + "glUseProgram, then glBindProgramPipeline"; + bool subtest = true; + + if (!piglit_automatic) + printf("%s...\n", subtest_name); + + glUseProgram(prog); + subtest = set_and_check_uniform(prog, 1.0) && subtest; + + /* Program rendering */ + subtest = draw(violet) && subtest; + + glBindProgramPipeline(pipe); + + /* It must ignore the pipeline */ + subtest = set_and_check_uniform(prog, 0.0) && subtest; + + /* Still program rendering */ + subtest = draw(red) && subtest; + + subtest = restore_default_program_state() && subtest; + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL, + subtest_name); + return subtest; +} + +static bool +test_BindProgramPipeline_without_UseProgram(void) +{ + static const char subtest_name[] = + "glBindProgramPipeline without glUseProgram"; + bool subtest = true; + GLint active_shader_pipe; + + if (!piglit_automatic) + printf("%s...\n", subtest_name); + + glBindProgramPipeline(pipe); + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, &active_shader_pipe); + subtest = set_and_check_uniform(active_shader_pipe, 1.0) && subtest; + + /* Pipeline rendering */ + subtest = draw(green) && subtest; + + subtest = restore_default_program_state() && subtest; + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL, + subtest_name); + return subtest; +} + +static bool +test_BindProgramPipeline_then_UseProgram(void) +{ + static const char subtest_name[] = + "glBindProgramPipeline, then glUseProgram"; + bool subtest = true; + + if (!piglit_automatic) + printf("%s...\n", subtest_name); + + glBindProgramPipeline(pipe); + glUseProgram(prog); + + /* UseProgram rendering */ + subtest = draw(red) && subtest; + + subtest = restore_default_program_state() && subtest; + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL, + subtest_name); + return subtest; +} + +static bool +test_BindProgramPipeline_then_UseProgram_0(void) +{ + static const char subtest_name[] = + "glBindProgramPipeline, then glUseProgram(0)"; + bool subtest = true; + + if (!piglit_automatic) + printf("%s...\n", subtest_name); + + glBindProgramPipeline(pipe); + + /* Pipeline rendering */ + subtest = draw(gb) && subtest; + + glUseProgram(prog); + subtest = set_and_check_uniform(prog, 1.0) && subtest; + glUseProgram(0); + + /* Still pipeline rendering with the same value for the uniform. + */ + subtest = draw(gb) && subtest; + + subtest = restore_default_program_state() && subtest; + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL, + subtest_name); + return subtest; +} + +static bool +test_UseProgram_then_BindProgramPipeline_then_UseProgram_0(void) +{ + static const char subtest_name[] = + "glUseProgram, then glBindProgramPipeline, then " + "glUseProgram(0)"; + bool subtest = true; + GLint active_shader_pipe; + + if (!piglit_automatic) + printf("%s...\n", subtest_name); + + glGetProgramPipelineiv(pipe, GL_ACTIVE_PROGRAM, &active_shader_pipe); + + glUseProgram(prog); + + /* Program rendering */ + subtest = draw(red) && subtest; + + glBindProgramPipeline(pipe); + subtest = set_and_check_uniform(prog, 1.0) && subtest; + + /* Still program rendering */ + subtest = draw(violet) && subtest; + + glUseProgram(0); + subtest = set_and_check_uniform(active_shader_pipe, 0.0) && subtest; + + /* Pipeline rendering */ + subtest = draw(gb) && subtest; + + subtest = restore_default_program_state() && subtest; + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL, + subtest_name); + return subtest; +} + +static bool +test_final_sanity(void) +{ + static const char subtest_name[] = "Final sanity test"; + bool subtest = true; + + if (!piglit_automatic) + printf("%s...\n", subtest_name); + + /* Fixed function rendering */ + subtest = draw(blue) && subtest; + + piglit_report_subtest_result(subtest ? PIGLIT_PASS : PIGLIT_FAIL, + subtest_name); + return subtest; +} + +enum piglit_result +piglit_display(void) +{ + GLint active_shader_pipe; + GLint uniform_loc_pipe; + GLint uniform_loc_prog; + bool pass = true; + + 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>It looks like this comment, from my previous review of this patch, never got addressed: "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"> + + /* Set up fixed function to draw blue if we lose our shader. This + * color is chosen because neither of the programs used elsewhere in + * the test can generate blue as a color. + */ + glColor4f(0.0, 0.0, 1.0, 1.0); + + pass = test_UseProgram_then_BindProgramPipeline() && pass; + pass = test_BindProgramPipeline_without_UseProgram() && pass; + pass = test_BindProgramPipeline_then_UseProgram() && pass; + pass = test_BindProgramPipeline_then_UseProgram_0() && pass; + pass = test_UseProgram_then_BindProgramPipeline_then_UseProgram_0() + && pass; + pass = test_final_sanity() && pass; + + piglit_present_results(); + + return pass ? PIGLIT_PASS : PIGLIT_FAIL; +} + +void +piglit_init(int argc, char **argv) +{ </blockquote><div> </div><div>Also, this comment, from my previous review, doesn't seem to have been addressed: "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"> + 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"; + bool pass = true; + + 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; + + /* Sanity check initial state. + */ + pass = check_GetInteger_value(GL_PROGRAM_PIPELINE_BINDING, 0) && pass; + pass = check_GetInteger_value(GL_CURRENT_PROGRAM, 0) && pass; + + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; + + /* If the set up went bad, none of the subtests will work. Bail out + * now. + */ + if (!pass) + piglit_report_result(PIGLIT_FAIL); +} </blockquote><div> </div><div>Finally, this comment, from my previous review, doesn't seem to have been addressed: "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>With those issues addressed, this patch is: Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div></div></div></div>