On 8 September 2013 19:42, Ian Romanick <[email protected]> wrote:
> On 09/07/2013 09:01 PM, Paul Berry wrote: > > On 4 September 2013 12:57, Ian Romanick <[email protected] > > <mailto:[email protected]>> wrote: > > > > From: Ian Romanick <[email protected] > > <mailto:[email protected]>> > > > > Separate shader objects can either connect outputs to inputs by > matching > > names (rendezvous by name) or by assigning explicit locations via the > > layout(location=...) (rendezvous by location). This is a simple test > > that a vertex shader with a different name than the fragment shader > > input can by matched by the location. > > > > v2: Fix some dumb bugs in the test. Use multiple values to ensure > we're > > not matching using rendezvous-by-dumb-luck. > > > > Signed-off-by: Ian Romanick <[email protected] > > <mailto:[email protected]>> > > --- > > tests/all.tests | 1 + > > .../arb_separate_shader_objects/CMakeLists.gl.txt | 1 + > > .../rendezvous_by_location.c | 97 > > ++++++++++++++++++++++ > > 3 files changed, 99 insertions(+) > > create mode 100644 > > tests/spec/arb_separate_shader_objects/rendezvous_by_location.c > > > > diff --git a/tests/all.tests b/tests/all.tests > > index ceefaf3..a8e389c 100644 > > --- a/tests/all.tests > > +++ b/tests/all.tests > > @@ -1241,6 +1241,7 @@ > > arb_separate_shader_objects['GetProgramPipelineiv'] = > > concurrent_test('arb_separ > > 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['Rendezvous by location'] = > > plain_test('arb_separate_shader_object-rendezvous_by_location -fbo') > > arb_separate_shader_objects['ValidateProgramPipeline'] = > > concurrent_test('arb_separate_shader_object-ValidateProgramPipeline') > > > > # Group ARB_sampler_objects > > diff --git > > a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > > b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > > index ad22987..66176f1 100644 > > --- a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > > +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > > @@ -12,5 +12,6 @@ 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-rendezvous_by_location > > rendezvous_by_location.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/rendezvous_by_location.c > > b/tests/spec/arb_separate_shader_objects/rendezvous_by_location.c > > new file mode 100644 > > index 0000000..00a8cf9 > > --- /dev/null > > +++ b/tests/spec/arb_separate_shader_objects/rendezvous_by_location.c > > @@ -0,0 +1,97 @@ > > +/* > > + * Copyright © 2013 Intel Corporation > > + * > > + * 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. > > + */ > > + > > +#include "piglit-util-gl-common.h" > > + > > +PIGLIT_GL_TEST_CONFIG_BEGIN > > + > > + config.supports_gl_compat_version = 10; > > + config.window_visual = PIGLIT_GL_VISUAL_RGB | > > PIGLIT_GL_VISUAL_DOUBLE; > > + > > +PIGLIT_GL_TEST_CONFIG_END > > + > > +static const char *vs_code = > > + "#version 110\n" > > + "#extension GL_ARB_separate_shader_objects: require\n" > > + "layout(location = 2) out vec3 a;\n" > > + "layout(location = 3) out vec3 b;\n" > > + "void main()\n" > > + "{\n" > > + " gl_Position = gl_Vertex;\n" > > + " a = vec3(0, 0, 1);\n" > > + " b = vec3(1, 0, 0);\n" > > + "}\n" > > + ; > > + > > +static const char *fs_code = > > + "#version 110\n" > > + "#extension GL_ARB_separate_shader_objects: require\n" > > + "layout(location = 3) in vec3 a;\n" > > + "layout(location = 2) in vec3 b;\n" > > + "void main()\n" > > + "{\n" > > + " gl_FragColor = vec4(cross(a, b), 1);\n" > > > > > > This should be cross(b, a). As written you're computing cross(vec3(1, > > 0, 0), vec3(0, 0, 1)), which equals vec3(0, -1, 0). > > D'oh... I was thinking in NDC (where positive Z goes away from the > screen) instead of "real" coordinates. Math is hard. > > > You're usually really good about testing your code before submitting it, > > so I'm wondering: does this test pass on catalyst? If so that's really > > bad because it means catalyst is doing rendezvous by name in spite of > > locations being specified. > > I think I had asked Ken to test on AMD (I only have GL 3.3 NVIDIA > hardware handy), but I don't think he ever did. I may have just tested > on Mesa where it spectacularly failed regardless. :) > > > Incidentally, I found that in order to run this test on NVIDIA, I had to > > change the shaders to use #version 140, and I had to replace gl_Vertex > > with a user-defined vertex attribute. I believe those are both > > necessary because of NVIDIA bugs, but it might be nice to go ahead and > > do them anyway, just so that it's easier to validate the test against > > existing implementations. But I don't feel terribly strongly about it. > > You had to replace gl_Vertex, but it let you use gl_FragColor? Would > 1.30 also work on NVIDIA? I suspect they don't enable the 'in' and > 'out' keywords based on extensions. I'll add separate compile tests for > that. :) > Ok, I investigated more thoroughly. It appears that: - In GLSL 1.40 and above, NVIDIA prohibits the use of gl_Vertex. In GLSL 1.30, gl_Vertex is allowed but generates a warning. - gl_FragColor is allowed but generates a warning in both GLSL 1.30 and 1.40. - In GLSL 1.30, NVIDIA handles "in" and "out" just fine, but it fails to parse the "layout" keyword properly. I found a workaround to this, but it's stupid: if I add "#extension GL_ARB_fragment_coord_conventions: require" to *both* the vs and gs, then they can be GLSL 1.30 and they work properly. I also tried using "#extension GL_ARB_explicit_attrib_location: require" instead, and it doesn't work. > > The spec also says that layout() cannot be applied to the old keywords > (attribute or varying). > > Since Mesa is going to enable this extension on all drivers, I'd like to > have as many of the tests run on as many drivers as possible. Very few > Mesa drivers support GLSL 1.40. Maybe I should dynamically set the > version? > Given what I've discovered about what the NVIDIA driver allows, I think that's probably the best bet. > > > With the order of arguments to cross() fixed, this patch is: > > > > Reviewed-by: Paul Berry <[email protected] > > <mailto:[email protected]>> > > > > > > + "}\n" > > + ; > > + > > +enum piglit_result > > +piglit_display(void) > > +{ > > + static const float expected[] = { > > + 0.0f, 1.0f, 0.0f, 1.0f > > + }; > > + bool pass; > > + > > + piglit_draw_rect(-1, -1, 2, 2); > > + pass = piglit_probe_rect_rgba(0, 0, piglit_width, > piglit_height, > > + expected); > > + > > + piglit_present_results(); > > + return pass ? PIGLIT_PASS : PIGLIT_FAIL; > > +} > > + > > +void piglit_init(int argc, char **argv) > > +{ > > + GLuint vs_prog; > > + GLuint fs_prog; > > + GLuint pipeline; > > + > > + piglit_require_vertex_shader(); > > + piglit_require_extension("GL_ARB_separate_shader_objects"); > > + > > + vs_prog = glCreateShaderProgramv(GL_VERTEX_SHADER, 1, > &vs_code); > > + piglit_link_check_status(vs_prog); > > + > > + fs_prog = glCreateShaderProgramv(GL_FRAGMENT_SHADER, 1, > > &fs_code); > > + piglit_link_check_status(fs_prog); > > + > > + glGenProgramPipelines(1, &pipeline); > > + glUseProgramStages(pipeline, GL_VERTEX_SHADER_BIT, vs_prog); > > + glUseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, > fs_prog); > > + piglit_program_pipeline_check_status(pipeline); > > + > > + if (!piglit_check_gl_error(0)) > > + piglit_report_result(PIGLIT_FAIL); > > + > > + glBindProgramPipeline(pipeline); > > +} > > -- > > 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
