[Piglit] [PATCH] gs: Test interaction between gl_PrimitiveIDIn and primitive restart.
Paul Berry
stereotype441 at gmail.com
Wed Sep 4 07:28:32 PDT 2013
On 3 September 2013 13:17, Chad Versace <chad.versace at linux.intel.com>wrote:
> On 08/26/2013 11:38 PM, Paul Berry wrote:
>
>> Verified using the NVIDIA proprietary driver for linux.
>> ---
>> tests/all.tests | 9 +
>> .../glsl-1.50/execution/**geometry/CMakeLists.gl.txt | 1 +
>> .../execution/geometry/**primitive-id-restart.c | 265
>> +++++++++++++++++++++
>> 3 files changed, 275 insertions(+)
>> create mode 100644 tests/spec/glsl-1.50/**execution/geometry/primitive-
>> **id-restart.c
>>
>
>
>
> +#define LONGEST_INPUT_SEQUENCE 12
>>
>
> Did you choose 12 to give gl_PrimitiveIDIn an opportunity to increment
> for GL_TRIANGLES_ADJANCENCY? If so, comment please. If not, then comment
> double-please, because I don't see the reason.
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.
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.
>
>
> +#define NUM_INPUT_ELEMENTS \
>> + (LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 3) / 2)
>> +#define MAX_TOTAL_PRIMS \
>> + (LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 1) / 2)
>>
>
> The term 'INPUT' in the var name NUM_INPUT_ELEMENTS confused me. So went my
> mind... "The ELEMENTS
> are qualified as INPUT ELEMENTS, which distinguishes them from... OUTPUT
> ELEMENTS?
> SOMETHING_ELSE ELEMENTS? What type of elements are these, if there be
> INPUT and
> OTHER_TYPES of elements? I do not know these ELEMENTS." Eventually, I saw
> that they
> were just vanilla ELEMENTS. Oh well.
>
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?
>
> The expression for MAX_TOTAL_PRIMS may look mysterious to the uninitiated.
> Please
> comment with
> /* Sum of 1, 2, ..., LONGEST_INPUT_SEQUENCE. */
>
> I found the expression for NUM_INPUT_ELEMENTS mysterious, and eventually
> discovered
> its round-about derivation. The following comment, or something similar,
> is strongly needed:
> /* Sum of 2, 3, ..., LONGEST_INPUT_SEQUENCE + 1. */
Done.
>
>
>
>
> +static const char *vs_text =
>> + "#version 150\n"
>> + "\n"
>> + "void main()\n"
>> + "{\n"
>> + "}\n";
>> +
>> +static const char *gs_template =
>> + "#version 150\n"
>> + "#define INPUT_LAYOUT %s\n"
>> + "layout(INPUT_LAYOUT) in;\n"
>>
>
> This shader preamble feels weird. I expected the macro INPUT_LAYOUT to
> be used elsewhere, but it isn't. I think
> #version 150
> layout(%s) in;
> is easier to read. Anyways... just ignore me here if you want to call
> bikeshed.
Changed.
>
>
> + "layout(points, max_vertices = 1) out;\n"
>> + "\n"
>> + "out int primitive_id;\n"
>> + "\n"
>> + "void main()\n"
>> + "{\n"
>> + " primitive_id = gl_PrimitiveIDIn;\n"
>> + " EmitVertex();\n"
>> + "}\n";
>>
>
>
>
>
> + /* Find out how many times the GS got invoked so we'll know
>>
>> + * how many transform feedback outputs to examine.
>> + */
>> + glGetQueryObjectuiv(generated_**query, GL_QUERY_RESULT,
>> + &primitives_generated);
>> + if (primitives_generated > MAX_TOTAL_PRIMS) {
>> + printf("Expected no more than %d primitives, got %d\n",
>> + MAX_TOTAL_PRIMS, primitives_generated);
>> + pass = false;
>> +
>> + /* Set primitives_generated to MAX_TOTAL_PRIMS so that
>> + * we don't overflow the buffer in the loop below.
>> + */
>>
>
> The test neglects to clamp primitives_generated like the comment says.
Oops, I'm not sure how that happened. I've added this line:
primitives_generated = MAX_TOTAL_PRIMS;
>
>
> + }
>> +
>> + /* Check transform feedback outputs. */
>> + readback = glMapBuffer(GL_TRANSFORM_**FEEDBACK_BUFFER,
>> GL_READ_ONLY);
>> + for (i = 0; i < primitives_generated; i++) {
>> + if (readback[i] != i) {
>> + printf("Expected primitive %d to have
>> gl_PrimitiveIDIn"
>> + " = %d, got %d instead\n", i, i,
>> readback[i]);
>> + pass = false;
>>
>
> If gl_PrimitiveIDIn fails to increment at the first occurrence of the
> primitive
> restart, then this errors gets emitted up to sum(i, {i, 2, 12}) = 77 times.
> I'd prefer that this error message not spam the console and instead be
> emitted
> only once. But, if you disagree, then I defer.
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.)
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.
>
>
> + }
>> + }
>> + glUnmapBuffer(GL_TRANSFORM_**FEEDBACK_BUFFER);
>> +
>> + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
>> +}
>>
>
>
> Other than the requests for clarification, and the clamping issue,
> the test's behavior looks correct to me.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130904/80330f15/attachment.html>
More information about the Piglit
mailing list