[Piglit] [PATCH] gs: Test interaction between gl_PrimitiveIDIn and primitive restart.

[Chad Versace]

On 09/04/2013 09:28 AM, Paul Berry wrote:
> 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 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?

Yes, please rename NUM_INPUT_ELEMENTS to NUM_ELEMENTS. I find that name more
clear. As for LONGEST_INPUT_SEQUENCE vs LONGEST_SEQUENCE, I've no preference there.


>>> +
>>> +       /* 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.

If you think the flood of error messages are helpful during diagnosis, then
this sounds good to me.


Thanks for the changes.
Reviewed-by: Chad Versace <chad.versace at linux.intel.com>