On Wed, Jun 11, 2014 at 2:49 AM, Iago Toral <ito...@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 <chr...@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. -Jordan >> On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga <ito...@igalia.com> >> 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 | 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_visitor.h | 1 + >> > src/mesa/drivers/dri/i965/gen6_queryobj.c | 8 +-- >> > src/mesa/drivers/dri/i965/gen7_sol_state.c | 65 >> > ++++++++++++++--------- >> > src/mesa/main/context.c | 2 +- >> > src/mesa/main/mtypes.h | 10 +++- >> > src/mesa/main/queryobj.c | 18 ++++--- >> > src/mesa/main/shaderapi.c | 1 + >> > src/mesa/main/shaderobj.c | 1 + >> > src/mesa/program/program.c | 1 + >> > 25 files changed, 432 insertions(+), 68 deletions(-) >> > >> > -- >> > 1.9.1 >> > >> > _______________________________________________ >> > mesa-dev mailing list >> > mesa-dev@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
