V4. Add a case to valid gl_VertexID for bug 87611

Submitted by Wang, Shuo on April 1, 2015, 9:47 a.m.

Details

Message ID 1427881633-30823-1-git-send-email-shuo.wang@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Wang, Shuo April 1, 2015, 9:47 a.m.
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 <shuo.wang@intel.com>
---
 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

Patch hide | download patch | download mbox

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 <shuo.wang@intel.com> */
+
+
+#include "piglit-util-gl.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
+
+/* 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";
+
+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;
+	enum piglit_result result;
+
+	glUseProgram(prog);
+
+	glClear(GL_COLOR_BUFFER_BIT);
+
+	/* 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);
+
+	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);
+
+	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);
+}

Comments

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 <shuo.wang@intel.com>
> ---
>  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 <shuo.wang@intel.com> */
> +
> +
> +#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?

> +
> +	/* 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);
> +}
>
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:idr@freedesktop.org] 

Sent: Wednesday, April 8, 2015 9:22 AM
To: Wang, Shuo; piglit@lists.freedesktop.org
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 <shuo.wang@intel.com>

> ---

>  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 <shuo.wang@intel.com> */

> +

> +

> +#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); }

>
On 04/07/2015 07:21 PM, Ian Romanick wrote:
> This test uses tabs for indentation.  The coding style for piglit is to
> never used tabs.  Instead, use 4-spaces for each indentation level.

Uh, since when?  IFAIK, we've been using 8-space tabs for indentation 
for a long time in piglit.

-Brian
On Tue, Apr 07, 2015 at 07:49:33PM -0600, Brian Paul wrote:
> On 04/07/2015 07:21 PM, Ian Romanick wrote:
> > This test uses tabs for indentation.  The coding style for piglit is to
> > never used tabs.  Instead, use 4-spaces for each indentation level.

This is the style we use for python, but I didn't think it was the style
for C/C++.

> 
> Uh, since when?  IFAIK, we've been using 8-space tabs for indentation 
> for a long time in piglit.
> 
> -Brian
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit