<div dir="ltr">On 3 September 2013 13:17, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>></span> wrote:<br><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"><div class="im">On 08/26/2013 11:38 PM, Paul Berry wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Verified using the NVIDIA proprietary driver for linux.<br>
---<br>
tests/all.tests | 9 +<br>
.../glsl-1.50/execution/<u></u>geometry/CMakeLists.gl.txt | 1 +<br>
.../execution/geometry/<u></u>primitive-id-restart.c | 265 +++++++++++++++++++++<br>
3 files changed, 275 insertions(+)<br>
create mode 100644 tests/spec/glsl-1.50/<u></u>execution/geometry/primitive-<u></u>id-restart.c<br>
</blockquote>
<br>
<br>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+#define LONGEST_INPUT_SEQUENCE 12<br>
</blockquote>
<br>
Did you choose 12 to give gl_PrimitiveIDIn an opportunity to increment<br>
for GL_TRIANGLES_ADJANCENCY? If so, comment please. If not, then comment<br>
double-please, because I don't see the reason.</blockquote><div><br></div><div>Actually, thes constant determines the maximum number of vertices that we emit before the primitive restart; we emit the same number of vertices after primitive restart as well, so 6 would have been sufficient to allow gl_PrimitiveIDIn an opportunity to increment for GL_TRIANGLES_ADJACENCY.<br>
<br>To be honest it was a rather arbitrary choice. I suppose I chose 12 because that was the minimum number that would ensure that there are at least two primitives before primitive restart in all drawing modes. I've made a comment to that effect. <br>
</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"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+#define NUM_INPUT_ELEMENTS \<br>
+ (LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 3) / 2)<br>
+#define MAX_TOTAL_PRIMS \<br>
+ (LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 1) / 2)<br>
</blockquote>
<br></div>
The term 'INPUT' in the var name NUM_INPUT_ELEMENTS confused me. So went my<br>
mind... "The ELEMENTS<br>
are qualified as INPUT ELEMENTS, which distinguishes them from... OUTPUT ELEMENTS?<br>
SOMETHING_ELSE ELEMENTS? What type of elements are these, if there be INPUT and<br>
OTHER_TYPES of elements? I do not know these ELEMENTS." Eventually, I saw that they<br>
were just vanilla ELEMENTS. Oh well.<br></blockquote><div><br></div><div>I don't have strong feelings about these names. Would you like me to change NUM_INPUT_ELEMENTS to NUM_ELEMENTS? If so, do you think it would make sense to also change LONGEST_INPUT_SEQUENCE to LONGEST_SEQUENCE?</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">
<br>
The expression for MAX_TOTAL_PRIMS may look mysterious to the uninitiated. Please<br>
comment with<br>
/* Sum of 1, 2, ..., LONGEST_INPUT_SEQUENCE. */<br>
<br>
I found the expression for NUM_INPUT_ELEMENTS mysterious, and eventually discovered<br>
its round-about derivation. The following comment, or something similar, is strongly needed:<br>
/* Sum of 2, 3, ..., LONGEST_INPUT_SEQUENCE + 1. */</blockquote><div><br></div><div>Done.<br></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">
<div class="im"><br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+static const char *vs_text =<br>
+ "#version 150\n"<br>
+ "\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
+ "}\n";<br>
+<br>
+static const char *gs_template =<br>
+ "#version 150\n"<br>
+ "#define INPUT_LAYOUT %s\n"<br>
+ "layout(INPUT_LAYOUT) in;\n"<br>
</blockquote>
<br></div>
This shader preamble feels weird. I expected the macro INPUT_LAYOUT to<br>
be used elsewhere, but it isn't. I think<br>
#version 150<br>
layout(%s) in;<br>
is easier to read. Anyways... just ignore me here if you want to call bikeshed.</blockquote><div><br></div><div>Changed.<br></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">
<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ "layout(points, max_vertices = 1) out;\n"<br>
+ "\n"<br>
+ "out int primitive_id;\n"<br>
+ "\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
+ " primitive_id = gl_PrimitiveIDIn;\n"<br>
+ " EmitVertex();\n"<br>
+ "}\n";<br>
</blockquote>
<br>
<br>
<br>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ /* Find out how many times the GS got invoked so we'll know<div class="im"><br>
+ * how many transform feedback outputs to examine.<br>
+ */<br>
+ glGetQueryObjectuiv(generated_<u></u>query, GL_QUERY_RESULT,<br>
+ &primitives_generated);<br>
+ if (primitives_generated > MAX_TOTAL_PRIMS) {<br>
+ printf("Expected no more than %d primitives, got %d\n",<br>
+ MAX_TOTAL_PRIMS, primitives_generated);<br>
+ pass = false;<br>
+<br>
+ /* Set primitives_generated to MAX_TOTAL_PRIMS so that<br>
+ * we don't overflow the buffer in the loop below.<br>
+ */<br>
</div></blockquote>
<br>
The test neglects to clamp primitives_generated like the comment says.</blockquote><div><br></div><div>Oops, I'm not sure how that happened. I've added this line:<br><br>primitives_generated = MAX_TOTAL_PRIMS;<br>
</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"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+<br>
+ /* Check transform feedback outputs. */<br>
+ readback = glMapBuffer(GL_TRANSFORM_<u></u>FEEDBACK_BUFFER, GL_READ_ONLY);<br>
+ for (i = 0; i < primitives_generated; i++) {<br>
+ if (readback[i] != i) {<br>
+ printf("Expected primitive %d to have gl_PrimitiveIDIn"<br>
+ " = %d, got %d instead\n", i, i, readback[i]);<br>
+ pass = false;<br>
</blockquote>
<br></div>
If gl_PrimitiveIDIn fails to increment at the first occurrence of the primitive<br>
restart, then this errors gets emitted up to sum(i, {i, 2, 12}) = 77 times.<br>
I'd prefer that this error message not spam the console and instead be emitted<br>
only once. But, if you disagree, then I defer.</blockquote><div><br></div><div>Yes, that's true. In fact, on Ivy Bridge it currently does this for GL_LINE_LOOP, because of what appears to be a hardware bug: sending a single vertex down the pipeline in GL_LINE_LOOP mode causes a degenerate line loop to be emitted consisting of a single line primitive from the vertex to itself. Mesa's software primitive restart code doesn't expect this, so it doesn't increment the primitive ID, and as a result all of the remaining primitive IDs are off by one. (I haven't yet decided whether this is worth working around, and if so how.)<br>
</div><div><br>On the other hand, when I was using this test to develop the necessary primitive ID workarounds on Ivy Bridge, I found it really useful to see all the errors, because that made it easy to tell what the actual sequence of primitive ID's was, so I could see what I did wrong. So I think I will leave it as is.<br>
</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"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+ }<br>
+ glUnmapBuffer(GL_TRANSFORM_<u></u>FEEDBACK_BUFFER);<br>
+<br>
+ piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);<br>
+}<br>
</blockquote>
<br>
<br></div>
Other than the requests for clarification, and the clamping issue,<br>
the test's behavior looks correct to me.<br>
<br>
<br>
</blockquote></div><br></div></div>