Thank you for the clearly information, I will work on it and send out a new version later.
Thanks, Shuo -----Original Message----- From: Ian Romanick [mailto:[email protected]] Sent: Wednesday, April 8, 2015 9:22 AM To: Wang, Shuo; [email protected] Cc: Jin, Gordon Subject: Re: [Piglit] [PATCH] V4. Add a case to valid gl_VertexID for bug 87611 This test uses tabs for indentation. The coding style for piglit is to never used tabs. Instead, use 4-spaces for each indentation level. Other comments are below. On 04/01/2015 02:47 AM, Wang Shuo wrote: > This case is passed by master since Ken's commit is already pushed > (c633528cbac007a73a066f269b3c9a25daf1e21a) > Below is the test point of this case: > 1. Use user-allocated vertex arrays (NOT vertex buffer objects). > 2. Use a glDrawArrays with a non-zero 'first'. > 3. Use primitive restart to trigger multiple _mesa_prim without an > intervening state change. > For primitive restart test: > In OpenGL, primitive restarting to work with all rendering > commands, including the non-indexed commands. However, > implementations did not implement this, and the rule itself > makes no sense.(https://www.opengl.org/wiki/Vertex_Rendering) > Also apparently GL 4.5 changed this de-facto behaviour into > specified behaviour -- primitive restart is only supposed to > work on Draw*Elements*. > 4. Validate the value of gl_VertexID in the vertex shader. > Both glDrawArrays testing and glDrawElements testing test the > value of gl_VertexID > 5. Using GL_TRIANGLE_FAN because GL_TRIANGLE_STRIP are supported > by the HW path, and GL_TRIANGLE_FAN specifically to hit the > SW primitive restart path which force the draw to be decomposed > into multiple draws > > V2: > 1.Fix some Piglit style issue > > V3: > 1.Make sure GL_TRIANGLE_FAN should be used. > > V4: > 1.Fix some GL version issue and coding style issue. > > Signed-off-by: Wang Shuo <[email protected]> > --- > tests/all.py | 1 + > tests/spec/gl-3.1/CMakeLists.gl.txt | 1 + > tests/spec/gl-3.1/primitive-restart-vertex-id.c | 137 > ++++++++++++++++++++++++ > 3 files changed, 139 insertions(+) > create mode 100644 tests/spec/gl-3.1/primitive-restart-vertex-id.c > > diff --git a/tests/all.py b/tests/all.py index b2266db..465c2b5 100644 > --- a/tests/all.py > +++ b/tests/all.py > @@ -1152,6 +1152,7 @@ with profile.group_manager( > g(['gl-3.1-genned-names'], 'genned-names') > g(['gl-3.1-minmax'], 'minmax') > g(['gl-3.1-vao-broken-attrib'], 'vao-broken-attrib') > + g(['gl-3.1-primitive-restart-vertex-id'], > + 'primitive-restart-vertex-id') > g(['gl-3.0-required-renderbuffer-attachment-formats', '31'], > 'required-renderbuffer-attachment-formats') > g(['gl-3.0-required-sized-texture-formats', '31'], diff --git > a/tests/spec/gl-3.1/CMakeLists.gl.txt > b/tests/spec/gl-3.1/CMakeLists.gl.txt > index a5f28c1..4ecb0b2 100644 > --- a/tests/spec/gl-3.1/CMakeLists.gl.txt > +++ b/tests/spec/gl-3.1/CMakeLists.gl.txt > @@ -15,5 +15,6 @@ piglit_add_executable (gl-3.1-genned-names > genned-names.c) piglit_add_executable (gl-3.1-minmax minmax.c) > piglit_add_executable (gl-3.1-primitive-restart-xfb > primitive-restart-xfb.c) piglit_add_executable > (gl-3.1-vao-broken-attrib vao-broken-attrib.c) > +piglit_add_executable (gl-3.1-primitive-restart-vertex-id > +primitive-restart-vertex-id.c) > > # vim: ft=cmake: > diff --git a/tests/spec/gl-3.1/primitive-restart-vertex-id.c > b/tests/spec/gl-3.1/primitive-restart-vertex-id.c > new file mode 100644 > index 0000000..0f92d1f > --- /dev/null > +++ b/tests/spec/gl-3.1/primitive-restart-vertex-id.c > @@ -0,0 +1,137 @@ > +/* > + * Copyright © 2015 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. > + */ > + > +/** > + * \file primitive-restart-vertex-id.c > + * > + * Check gl_VertexID > + * This case is added for *Bug 87611 - Exercise gl_VertexID corner > + * cases in i965 driver*, and used to watch the status of *Bug 85529 > + * - Surfaces not drawn in Unvanquished* This case is > + * passed by master now. > + * > + * > + * \author Wang Shuo <[email protected]> */ > + > + > +#include "piglit-util-gl.h" > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 10; With only this in the configuration, the test will never get an OpenGL 3.1 context on Mesa. You should add: config.supports_gl_core_version = 31; This test also needs GLSL 1.30, and that is (currently) only available with OpenGL 3.0 or later. You should change the "supports_gl_compat_version" line to: config.supports_gl_compat_version = 30; Then you can delete the GLSL version check in piglit_init. > + > + config.window_visual = PIGLIT_GL_VISUAL_RGB | > +PIGLIT_GL_VISUAL_DOUBLE; > + > +PIGLIT_GL_TEST_CONFIG_END > + > +/* Validate the value of gl_VertexID in the vertex shader. */ static > +const GLchar *vertShaderText = > + "#version 130\n" > + "in vec4 piglit_vertex;\n" > + "void main()\n" > + "{\n" > + " gl_Position = piglit_vertex;\n" > + " if (gl_VertexID >= 2 && gl_VertexID <= 5 ) {\n" > + " gl_FrontColor = vec4(0.0, 1.0, 0.0, 1.0);\n" > + " } else {\n" > + " gl_FrontColor = vec4(1.0, 0.0, 0.0, 1.0);\n" > + " }\n" > + "}\n"; In core profile (or in OpenGL 3.1 on Mesa), you must have a fragment shader. Since the test requires GLSL 1.30 anyway, I think these shaders would work best: static const GLchar *vertShaderText = "#version 130\n" "in vec4 piglit_vertex;\n" "flat out int pass;\n" "\n" "void main()\n" "{\n" " gl_Position = piglit_vertex;\n" " pass = int(gl_VertexID >= 2 && gl_VertexID <= 5);\n" "}\n"; static const GLchar *fragShaderText = "#version 130\n" "flat in int pass;\n" "\n" "void main()\n" "{\n" " gl_FragColor = bool(pass)\n" " ? vec4(0.0, 1.0, 0.0, 1.0)\n" " : vec4(1.0, 0.0, 0.0, 1.0);\n" "}\n"; > + > +static GLint prog; > + > +static const GLfloat Vcoords[6][2] = { {-1, -1}, {-1, -1}, {-1, -1}, > +{1, -1}, {1, 1}, {-1, 1}}; > + > +static const unsigned short indices[] = { > + 2, 0xFFFF, 2, 3, 0xFFFF, 0xFFFF, 2, 3, 4, 5, 0xFFFF, 0xFFFF, > +0xFFFF,5 }; > + > + > +enum piglit_result > +piglit_display(void) > +{ > + static const GLfloat expColor[4] = {0, 1, 0, 1}; > + GLboolean pass = GL_TRUE; bool pass = true; > + enum piglit_result result; I think "result" is not used. It should be removed. > + > + glUseProgram(prog); > + > + glClear(GL_COLOR_BUFFER_BIT); I think having the clear between the two draws prevents the test from exercising the case we wanted to test. Did you try this with Mesa from just before Ken's fix? [Shuo] I had test it by V3, and I will double check it during the new version. > + > + /* Use user-allocated vertex arrays (NOT vertex buffer objects). */ > + glVertexAttribPointer(PIGLIT_ATTRIB_POS, 2, GL_FLOAT, GL_FALSE, 0, > Vcoords); > + glEnableVertexAttribArray(PIGLIT_ATTRIB_POS); > + > + /* Use a glDrawArrays with a non-zero 'first'. */ > + glDrawArrays(GL_TRIANGLE_FAN, 2, 4); > + > + pass = piglit_probe_pixel_rgba(20, 20, expColor); > + > + glDisableVertexAttribArray(PIGLIT_ATTRIB_POS); > + > + /* Use primitive restart to trigger multiple _mesa_prim without an > + * intervening state change. > + * Because for OpenGL, PRIMITIVE_RESTART work with all > + * rendering commands, including the non-indexed commands. However, > + * implementations did not implement this, and the rule itself makes > + * no sense (https://www.opengl.org/wiki/Vertex_Rendering) > + * So for testing PRIMITIVE_RESTART I used glDrawElements > + */ > + glUseProgram(prog); > + > + glClear(GL_COLOR_BUFFER_BIT); > + > + glEnable(GL_PRIMITIVE_RESTART_FIXED_INDEX); GL_PRIMITIVE_RESTART_FIXED_INDEX only exists when the extension GL_ARB_ES3_compatibility is available. You can get the same result in OpenGL 3.1 by using GL_PRIMITIVE_RESTART and setting the restart index to 0xffff. The function glPrimitiveRestartIndex only exists in OpenGL 3.1. In earlier versions with GL_NV_primitive_restart you use GL_PRIMITIVE_RESTART_NV (as client state) and glPrimitiveRestartNV. if (piglit_get_gl_version() >= 31) { glEnable(GL_PRIMITIVE_RESTART); glPrimitiveRestartIndex(0x0ffff); } else { glEnableClientState(GL_PRIMITIVE_RESTART_NV); glPrimitiveRestartIndexNV(0x0ffff); } > + > + glVertexAttribPointer(PIGLIT_ATTRIB_POS, 2, GL_FLOAT, GL_FALSE, 0, > Vcoords); > + glEnableVertexAttribArray(PIGLIT_ATTRIB_POS); > + > + glDrawElements(GL_TRIANGLE_FAN, ARRAY_SIZE(indices), > +GL_UNSIGNED_SHORT, indices); > + > + pass = piglit_probe_pixel_rgba(20, 20, expColor) && pass; > + > + glDisableVertexAttribArray(PIGLIT_ATTRIB_POS); > + > + glDisable(GL_PRIMITIVE_RESTART_FIXED_INDEX); Same here. if (piglit_get_gl_version() >= 31) glDisable(GL_PRIMITIVE_RESTART); else glDisableClientState(GL_PRIMITIVE_RESTART_NV); > + > + piglit_present_results(); > + > + return pass ? PIGLIT_PASS : PIGLIT_FAIL; } > + > + > +void > +piglit_init(int argc, char **argv) > +{ > + > + piglit_require_GLSL_version(130); > + > + if (!piglit_is_extension_supported("GL_NV_primitive_restart") && > + piglit_get_gl_version() < 31) { > + printf("GL_NV_primitive_restart or GL 3.1 required\n"); > + piglit_report_result(PIGLIT_SKIP); > + } > + > + prog = piglit_build_simple_program(vertShaderText, NULL); } > _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
