[Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
This series brings multi-stream support to geometry shaders as defined by ARB_gpu_shader5. It also covers some missing multi-stream functionality from ARB_transform_feedback3. This is combined work from Samuel Iglesias and myself. The series includes both required infrastructure in Mesa and specific implementation for i965. Summary: Patch 1: GLSL parsing bits. Patches 2-8: transform feedback support. Patches 9-13: support for vertex/primitive emission to non-zero streams. Patches 14-15: ir_reader support. Patches 16-18: transform feedback queries support (ARB_transform_feedback3). Notes: I am not very happy with patch 11 but I could not find a better way to do this, hopefully someone here knows how to do this better. There is a comment in the patch explaining the problem, but here goes a summary: EmitVertex() is not a builtin-function like most others, when this is called we don't really generate code, we simply generate an ir_emit_vertex() to mark the point where vertex emission is to be done. Then, this will be processed at native code-generation time via vec4_gs_visitor::visit(ir_emit_vertex *) (in the i965 driver) to do what this means for the specifc hardware we are dealing with. EmitStreamVertex(n) is the same thing, with the exception that it takes a compile-time constant parameter with the stream id. The problem we have here is that this input variable and its value are not really important until we generate native code, it won't be used for anything until that moment, and this makes Mesa think that it is unused in the optimization passes for dead code that we run before native code genration. As a consequence, these passes will kill the variable and by the time we reach vec4_gs_visitor::visit(ir_emit_vertex *) we have lost the streamId information. The way patch 11 works around this problem is to never generate an ir_call for EmitStreamVertex(n), instead, when it detects that we are calling this function, it evaluates the parameters at that moment (we can do this because by definition the parameter must be a compile-time constant expression), and generates the ir_emit_vertex with the stream value directly. This is more efficient, gets the problem solved and also solves another thing: we want to control if we are emitting vertices to streams other than 0 and this also gives as the opportuity to detect this situation in a place where we can set state accordingly for later use. However, having to create a special case for this in does not look very nice. The same goes for EndStreamPrimitive(n). Iago Toral Quiroga (16): mesa: add StreamId information to transform feedback outputs. i965: Enable transform feedback for streams > 0 glsl: Assign GLSL StreamIds to transform feedback outputs. glsl: Fail to link if inter-stage input/outputs are not assigned to stream 0 glsl: Add methods to retrive a varying's name and streamId. glsl: Two varyings can't write to the same buffer from different streams. glsl: Only geometry shader outputs can be associated with non-zero streams. glsl: Store info about geometry shaders that emit vertices to non-zero streams. i965/gs: Set number of control data bits for stream mode. glsl: Add support for EmitStreamVertex() and EndStreamPrimitive(). glsl: Can't emit vertices to non-zero streams with output != GL_POINTS. i965/gs: Set control data bits for vertices emitted in stream mode. glsl: include streamId when reading/printing emit-vertex and end-primitive IR. mesa: Include stream information in indexed queries. i965: Implement GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN with non-zero streams. mesa: Enable simultaneous transform feedback queries on different streams. Samuel Iglesias Gonsalvez (2): glsl: Add parsing support for multi-stream output in geometry shaders. glsl: include streamId when reading/printing ir_variable IR. src/glsl/ast.h| 5 ++ src/glsl/ast_function.cpp | 37 ++--- src/glsl/ast_to_hir.cpp | 6 +++ src/glsl/ast_type.cpp | 19 ++- src/glsl/builtin_functions.cpp| 60 + src/glsl/glsl_parser.yy | 45 src/glsl/glsl_parser_extras.cpp | 4 ++ src/glsl/glsl_parser_extras.h | 5 ++ src/glsl/ir.h | 23 +--- src/glsl/ir_print_visitor.cpp | 15 +++--- src/glsl/ir_reader.cpp| 24 +++-- src/glsl/link_varyings.cpp| 40 +- src/glsl/link_varyings.h | 17 ++ src/glsl/linker.cpp | 33 src/mesa/drivers/dri/i965/brw_vec4_gs.c | 9 ++-- src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 51 +- src/mesa/drivers/dri/i965/brw_vec4_gs_visit
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
I sent comments on patches 1, 3, 5, 9, 11, 16, 18 Patches 2, 4, 6-8, 10, 12-15, 17 are: Reviewed-by: Chris Forbes 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. -- Chris On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga wrote: > This series brings multi-stream support to geometry shaders as defined by > ARB_gpu_shader5. It also covers some missing multi-stream functionality from > ARB_transform_feedback3. This is combined work from Samuel Iglesias and > myself. > > The series includes both required infrastructure in Mesa and specific > implementation for i965. > > Summary: > Patch 1: GLSL parsing bits. > Patches 2-8: transform feedback support. > Patches 9-13: support for vertex/primitive emission to non-zero streams. > Patches 14-15: ir_reader support. > Patches 16-18: transform feedback queries support (ARB_transform_feedback3). > > Notes: > I am not very happy with patch 11 but I could not find a better way to do > this, > hopefully someone here knows how to do this better. There is a comment in the > patch explaining the problem, but here goes a summary: > > EmitVertex() is not a builtin-function like most others, when this is > called we don't really generate code, we simply generate an ir_emit_vertex() > to mark the point where vertex emission is to be done. Then, this will be > processed at native code-generation time via > vec4_gs_visitor::visit(ir_emit_vertex *) (in the i965 driver) to do what this > means for the specifc hardware we are dealing with. EmitStreamVertex(n) is > the same thing, with the exception that it takes a compile-time constant > parameter with the stream id. The problem we have here is that this input > variable and its value are not really important until we generate native code, > it won't be used for anything until that moment, and this makes Mesa think > that > it is unused in the optimization passes for dead code that we run before > native > code genration. As a consequence, these passes will kill the variable and by > the > time we reach vec4_gs_visitor::visit(ir_emit_vertex *) we have lost > the streamId information. > > The way patch 11 works around this problem is to never generate an ir_call for > EmitStreamVertex(n), instead, when it detects that we are calling this > function, > it evaluates the parameters at that moment (we can do this because by > definition > the parameter must be a compile-time constant expression), and generates the > ir_emit_vertex with the stream value directly. This is more efficient, gets > the problem solved and also solves another thing: we want to control if we are > emitting vertices to streams other than 0 and this also gives as the > opportuity > to detect this situation in a place where we can set state accordingly for > later use. However, having to create a special case for this in does not > look very nice. The same goes for EndStreamPrimitive(n). > > Iago Toral Quiroga (16): > mesa: add StreamId information to transform feedback outputs. > i965: Enable transform feedback for streams > 0 > glsl: Assign GLSL StreamIds to transform feedback outputs. > glsl: Fail to link if inter-stage input/outputs are not assigned to > stream 0 > glsl: Add methods to retrive a varying's name and streamId. > glsl: Two varyings can't write to the same buffer from different > streams. > glsl: Only geometry shader outputs can be associated with non-zero > streams. > glsl: Store info about geometry shaders that emit vertices to non-zero > streams. > i965/gs: Set number of control data bits for stream mode. > glsl: Add support for EmitStreamVertex() and EndStreamPrimitive(). > glsl: Can't emit vertices to non-zero streams with output != > GL_POINTS. > i965/gs: Set control data bits for vertices emitted in stream mode. > glsl: include streamId when reading/printing emit-vertex and > end-primitive IR. > mesa: Include stream information in indexed queries. > i965: Implement GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN with non-zero > streams. > mesa: Enable simultaneous transform feedback queries on different > streams. > > Samuel Iglesias Gonsalvez (2): > glsl: Add parsing support for multi-stream output in geometry shaders. > glsl: include streamId when reading/printing ir_variable IR. > > src/glsl/ast.h| 5 ++ > src/glsl/ast_function.cpp | 37 ++--- > src/glsl/ast_to_hir.cpp | 6 +++ > src/glsl/ast_type.cpp | 19 ++- > src/glsl/builtin_functions.cpp| 60 + > src/glsl/glsl_parser.yy | 45 > src/glsl/glsl_parser_extras.cpp
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
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 > > 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. Iago > -- Chris > > > > On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga wrote: > > This series brings multi-stream support to geometry shaders as defined by > > ARB_gpu_shader5. It also covers some missing multi-stream functionality from > > ARB_transform_feedback3. This is combined work from Samuel Iglesias and > > myself. > > > > The series includes both required infrastructure in Mesa and specific > > implementation for i965. > > > > Summary: > > Patch 1: GLSL parsing bits. > > Patches 2-8: transform feedback support. > > Patches 9-13: support for vertex/primitive emission to non-zero streams. > > Patches 14-15: ir_reader support. > > Patches 16-18: transform feedback queries support (ARB_transform_feedback3). > > > > Notes: > > I am not very happy with patch 11 but I could not find a better way to do > > this, > > hopefully someone here knows how to do this better. There is a comment in > > the > > patch explaining the problem, but here goes a summary: > > > > EmitVertex() is not a builtin-function like most others, when this is > > called we don't really generate code, we simply generate an ir_emit_vertex() > > to mark the point where vertex emission is to be done. Then, this will be > > processed at native code-generation time via > > vec4_gs_visitor::visit(ir_emit_vertex *) (in the i965 driver) to do what > > this > > means for the specifc hardware we are dealing with. EmitStreamVertex(n) is > > the same thing, with the exception that it takes a compile-time constant > > parameter with the stream id. The problem we have here is that this input > > variable and its value are not really important until we generate native > > code, > > it won't be used for anything until that moment, and this makes Mesa think > > that > > it is unused in the optimization passes for dead code that we run before > > native > > code genration. As a consequence, these passes will kill the variable and > > by the > > time we reach vec4_gs_visitor::visit(ir_emit_vertex *) we have lost > > the streamId information. > > > > The way patch 11 works around this problem is to never generate an ir_call > > for > > EmitStreamVertex(n), instead, when it detects that we are calling this > > function, > > it evaluates the parameters at that moment (we can do this because by > > definition > > the parameter must be a compile-time constant expression), and generates the > > ir_emit_vertex with the stream value directly. This is more efficient, gets > > the problem solved and also solves another thing: we want to control if we > > are > > emitting vertices to streams other than 0 and this also gives as the > > opportuity > > to detect this situation in a place where we can set state accordingly for > > later use. However, having to create a special case for this in does not > > look very nice. The same goes for EndStreamPrimitive(n). > > > > Iago Toral Quiroga (16): > > mesa: add StreamId information to transform feedback outputs. > > i965: Enable transform feedback for streams > 0 > > glsl: Assign GLSL StreamIds to transform feedback outputs. > > glsl: Fail to link if inter-stage input/outputs are not assigned to > > stream 0 > > glsl: Add methods to retrive a varying's name and streamId. > > glsl: Two varyings can't write to the same buffer from different > > streams. > > glsl: Only geometry shader outputs can be associated with non-zero > > streams. > > glsl: Store info about geometry shaders that emit vertices to non-zero > > streams. > > i965/gs: Set number of control data bits for stream mode. > > glsl: Add support for EmitStreamVertex() and EndStreamPrimitive(). > > glsl: Can't emit vertices to non-zero streams with output != > > GL_POINTS. > > i965/gs: Set control data bits for vertices emitted in stream mode. > > glsl: include streamId when reading/printing emit-vertex and > > end-primitive IR. > > mesa: Include stream information in indexed queries. > > i965: Implement GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN with non-zero > > streams. > > mesa: Enable simultaneous transform feedback queries on different > > streams. > > > > Samuel Iglesias Gonsalvez (2): > > glsl: Add parsing support for multi-stream output in geometry shaders. > > glsl: include streamId when reading/
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral 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 >> >> 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. -Jordan >> On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga >> wrote: >> > This series brings multi-stream support to geometry shaders as defined by >> > ARB_gpu_shader5. It also covers some missing multi-stream functionality >> > from >> > ARB_transform_feedback3. This is combined work from Samuel Iglesias and >> > myself. >> > >> > The series includes both required infrastructure in Mesa and specific >> > implementation for i965. >> > >> > Summary: >> > Patch 1: GLSL parsing bits. >> > Patches 2-8: transform feedback support. >> > Patches 9-13: support for vertex/primitive emission to non-zero streams. >> > Patches 14-15: ir_reader support. >> > Patches 16-18: transform feedback queries support >> > (ARB_transform_feedback3). >> > >> > Notes: >> > I am not very happy with patch 11 but I could not find a better way to do >> > this, >> > hopefully someone here knows how to do this better. There is a comment in >> > the >> > patch explaining the problem, but here goes a summary: >> > >> > EmitVertex() is not a builtin-function like most others, when this is >> > called we don't really generate code, we simply generate an >> > ir_emit_vertex() >> > to mark the point where vertex emission is to be done. Then, this will be >> > processed at native code-generation time via >> > vec4_gs_visitor::visit(ir_emit_vertex *) (in the i965 driver) to do what >> > this >> > means for the specifc hardware we are dealing with. EmitStreamVertex(n) is >> > the same thing, with the exception that it takes a compile-time constant >> > parameter with the stream id. The problem we have here is that this input >> > variable and its value are not really important until we generate native >> > code, >> > it won't be used for anything until that moment, and this makes Mesa think >> > that >> > it is unused in the optimization passes for dead code that we run before >> > native >> > code genration. As a consequence, these passes will kill the variable and >> > by the >> > time we reach vec4_gs_visitor::visit(ir_emit_vertex *) we have lost >> > the streamId information. >> > >> > The way patch 11 works around this problem is to never generate an ir_call >> > for >> > EmitStreamVertex(n), instead, when it detects that we are calling this >> > function, >> > it evaluates the parameters at that moment (we can do this because by >> > definition >> > the parameter must be a compile-time constant expression), and generates >> > the >> > ir_emit_vertex with the stream value directly. This is more efficient, gets >> > the problem solved and also solves another thing: we want to control if we >> > are >> > emitting vertices to streams other than 0 and this also gives as the >> > opportuity >> > to detect this situation in a place where we can set state accordingly for >> > later use. However, having to create a special case for this in does not >> > look very nice. The same goes for EndStreamPrimitive(n). >> > >> > Iago Toral Quiroga (16): >> > mesa: add StreamId information to transform feedback outputs. >> > i965: Enable transform feedback for streams > 0 >> > glsl: Assign GLSL StreamIds to transform feedback outputs. >> > glsl: Fail to link if inter-stage input/outputs are not assigned to >> > stream 0 >> > glsl: Add methods to retrive a varying's name and streamId. >> > glsl: Two varyings can't write to the same buffer from different >> > streams. >> > glsl: Only geometry shader outputs can be associated with non-zero >> > streams. >> > glsl: Store info about geometry shaders that emit vertices to non-zero >> > streams. >> > i965/gs: Set number of control data bits for stream mode. >> > glsl: Add support for EmitStreamVertex() and EndStreamPrimitive(). >> > glsl: Can't emit vertices to non-zero streams with output != >> > GL_POINTS. >> > i965/gs: Set control data bits for vertices emitted in stream mode. >> > glsl: include streamId when reading/printing emit-vertex and >> > end-primitive IR. >> > mesa: Include stream information in indexed queries. >> > i965: Implemen
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
On Wed, Jun 11, 2014 at 12:31 PM, Jordan Justen wrote:
> On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral 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
>>>
>>> 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
On Sun, 2014-06-15 at 19:18 -0400, Ilia Mirkin wrote:
> On Wed, Jun 11, 2014 at 12:31 PM, Jordan Justen wrote:
> > On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral 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
> >>>
> >>> 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.
Thanks for reporting!
Sam
signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
On Mon, Jun 16, 2014 at 1:05 AM, Samuel Iglesias Gonsálvez
wrote:
> On Sun, 2014-06-15 at 19:18 -0400, Ilia Mirkin wrote:
>> On Wed, Jun 11, 2014 at 12:31 PM, Jordan Justen wrote:
>> > On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral 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
>> >>>
>> >>> 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. :)
Thanks,
-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/18] Multi-stream support for geometry shaders
On Mon, Jun 16, 2014 at 1:20 PM, Jordan Justen wrote:
> On Mon, Jun 16, 2014 at 1:05 AM, Samuel Iglesias Gonsálvez
> wrote:
>> On Sun, 2014-06-15 at 19:18 -0400, Ilia Mirkin wrote:
>>> On Wed, Jun 11, 2014 at 12:31 PM, Jordan Justen wrote:
>>> > On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral 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
>>> >>>
>>> >>> 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
