[Piglit] [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders

[Ilia Mirkin]

On Mon, Jun 16, 2014 at 1:20 PM, Jordan Justen <jljusten at gmail.com> wrote:
> On Mon, Jun 16, 2014 at 1:05 AM, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com> wrote:
>> On Sun, 2014-06-15 at 19:18 -0400, Ilia Mirkin wrote:
>>> On Wed, Jun 11, 2014 at 12:31 PM, Jordan Justen <jljusten at gmail.com> wrote:
>>> > On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral <itoral at igalia.com> wrote:
>>> >> Hi Chris,
>>> >>
>>> >> thanks for the quick review!
>>> >>
>>> >> On Wed, 2014-06-11 at 21:45 +1200, Chris Forbes wrote:
>>> >>> I sent comments on patches 1, 3, 5, 9, 11, 16, 18
>>> >>
>>> >> We will look into these.
>>> >>
>>> >>> Patches 2, 4, 6-8, 10, 12-15, 17 are:
>>> >>>
>>> >>> Reviewed-by: Chris Forbes <chrisf at ijw.co.nz>
>>> >>>
>>> >>> You should also include a patch to docs/GL3.txt marking off this
>>> >>> subfeature for i965 :)
>>> >>>
>>> >>>
>>> >>> Do you have a bunch of piglits which exercise all the tricky corners
>>> >>> of this? I see a few tests
>>> >>> which require the existence of the stream layout qualifier, but not
>>> >>> covering all the behavior.
>>> >>
>>> >> No, so far we have been testing this with a standalone program. We will
>>> >> check what already exists in piglit and add missing test cases.
>>> >
>>> > This is the only piglit test that I know about:
>>> > http://patchwork.freedesktop.org/patch/19224/
>>> >
>>> > Note that it passed on nvidia, but failed on catalyst.
>>>
>>> I just ran that test against this code (+ a few changes to add support
>>> in gallium) and I got:
>>>
>>> $ MESA_EXTENSION_OVERRIDE=GL_ARB_gpu_shader5
>>> bin/arb_gpu_shader5-xfb-streams -fbo -auto
>>> Failed to compile geometry shader: 0:3(1): error: invalid input layout
>>> qualifiers used
>>>
>>> source:
>>> #version 150
>>> #extension GL_ARB_gpu_shader5 : enable
>>> layout(points, invocations = 32) in;
>>> layout(points, max_vertices = 4) out;
>>> out float stream0_0_out;
>>> layout(stream = 1) out vec2 stream1_0_out;
>>> layout(stream = 2) out float stream2_0_out;
>>> layout(stream = 3) out vec3 stream3_0_out;
>>> layout(stream = 1) out vec3 stream1_1_out;
>>> layout(stream = 2) out vec4 stream2_1_out;
>>> void main() {
>>>   gl_Position = gl_in[0].gl_Position;
>>>   stream0_0_out = 1.0 + gl_InvocationID;
>>>   EmitVertex();
>>>   EndPrimitive();
>>>   stream3_0_out = vec3(12.0 + gl_InvocationID, 13.0 + gl_InvocationID,
>>>                        14.0 + gl_InvocationID);
>>>   EmitStreamVertex(3);
>>>   EndStreamPrimitive(3);
>>>   stream2_0_out = 7.0 + gl_InvocationID;
>>>   stream2_1_out = vec4(8.0 + gl_InvocationID, 9.0 + gl_InvocationID,
>>>                        10.0 + gl_InvocationID, 11.0 + gl_InvocationID);
>>>   EmitStreamVertex(2);
>>>   EndStreamPrimitive(2);
>>>   stream1_0_out = vec2(2.0 + gl_InvocationID, 3.0 + gl_InvocationID);
>>>   stream1_1_out = vec3(4.0 + gl_InvocationID, 5.0 + gl_InvocationID,
>>>                        6.0 + gl_InvocationID);
>>>   EmitStreamVertex(1);
>>>   EndStreamPrimitive(1);
>>> }PIGLIT: {'result': 'fail' }
>>>
>>> I guess it doesn't like
>>>
>>> "layout(points, invocations=32) in"? Not sure. I could also have
>>> messed something up... doing a bisect over your patches blames
>>>
>>> "glsl: Add parsing support for multi-stream output in geometry shaders."
>>>
>>> Before that, it goes until the first stream=1 layout and fails there
>>> (which is expected).
>>>
>>>   -ilia
>>
>> I will take a look at it.
>
> Does anyone see any bugs in the test?
>
> If not, perhaps I should just push it to piglit.
>
> It never got a reviewed-by, but it certainly had time for review. :)

I'm not qualified to review, but it passed on nvidia blob in my tests.
You can add my Tested-by if it helps.

  -ilia