[Mesa-dev] [PATCH 6/7] i965/gen7: Add support for transform feedback.

[Eric Anholt]

On Thu, 22 Dec 2011 19:22:16 -0800, Paul Berry <stereotype441 at gmail.com> wrote:
> On 22 December 2011 16:54, Eric Anholt <eric at anholt.net> wrote:
> 
> > Fixes almost all of the transform feedback piglit tests.  Remaining
> > are a few tests related to tesselation for
> > quads/trifans/tristrips/polygons with flat shading.
> > ---
> >  src/mesa/drivers/dri/i965/gen7_sol_state.c |  199
> > ++++++++++++++++++++++++++-
> >  1 files changed, 191 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen7_sol_state.c
> > b/src/mesa/drivers/dri/i965/gen7_sol_state.c
> > index 650f625..a5e28b6 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_sol_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_sol_state.c

> >  static void
> > -upload_sol_state(struct brw_context *brw)
> > +upload_3dstate_so_buffers(struct brw_context *brw)
> > +{
> > +   struct intel_context *intel = &brw->intel;
> > +   struct gl_context *ctx = &intel->ctx;
> > +   /* BRW_NEW_VERTEX_PROGRAM */
> > +   const struct gl_shader_program *vs_prog =
> > +      ctx->Shader.CurrentVertexProgram;
> > +   const struct gl_transform_feedback_info *linked_xfb_info =
> > +      &vs_prog->LinkedTransformFeedback;
> > +   struct gl_transform_feedback_object *xfb_obj =
> > +      ctx->TransformFeedback.CurrentObject;
> >
> 
> Can we have a "/* NEW_TRANSFORM_FEEDBACK */" comment here?

Why, yes!

> It looks like we're not setting "SO Buffer Object Control State".  Is that
> ok?  I'm not too familiar with memory object control states so I'm not
> sure, but it seemed to me that it might be sensible to mark the stream
> output as L3 cacheable.

0 is the "don't override my page tables" value.  We don't set it
anywhere else, either.

> > +/**
> > + * Outputs the 3DSTATE_SO_DECL_LIST command.
> > + *
> > + * The data output is a series of 64-bit entries containing a SO_DECL per
> > + * stream.  We only have one stream of rendering coming out of the GS
> > unit, so
> > + * we only emit stream 0 (low 16 bits) SO_DECLs.
> > + */
> > +static void
> > +upload_3dstate_so_decl_list(struct brw_context *brw,
> > +                           struct brw_vue_map *vue_map)
> > +{
> > +   struct intel_context *intel = &brw->intel;
> > +   struct gl_context *ctx = &intel->ctx;
> > +   /* BRW_NEW_VERTEX_PROGRAM */
> > +   const struct gl_shader_program *vs_prog =
> > +      ctx->Shader.CurrentVertexProgram;
> > +   /* NEW_TRANSFORM_FEEDBACK */
> > +   const struct gl_transform_feedback_info *linked_xfb_info =
> > +      &vs_prog->LinkedTransformFeedback;
> > +   int i;
> > +   uint16_t so_decl[128];
> >
> 
> Can we add an assertion to verify that there is no danger of overflowing
> this array?  I think STATIC_ASSERT(ARRAY_SIZE(so_decl) >=
> MAX_PROGRAM_OUTPUTS) ought to do the trick.

Done.

> > +   /* Construct the list of SO_DECLs to be emitted.  The formatting of the
> > +    * command is feels strange -- each dword pair contains a SO_DECL per
> > stream.
> > +    */
> > +   for (i = 0; i < linked_xfb_info->NumOutputs; i++) {
> > +      int buffer = linked_xfb_info->Outputs[i].OutputBuffer;
> > +      uint16_t decl = 0;
> > +      int vert_result = linked_xfb_info->Outputs[i].OutputRegister;
> > +
> > +      buffer_mask |= 1 << buffer;
> > +
> > +      decl |= buffer << SO_DECL_OUTPUT_BUFFER_SLOT_SHIFT;
> > +      decl |= vue_map->vert_result_to_slot[vert_result] <<
> > +        SO_DECL_REGISTER_INDEX_SHIFT;
> > +      decl |= ((1 << linked_xfb_info->Outputs[i].NumComponents) - 1) <<
> > +        SO_DECL_COMPONENT_MASK_SHIFT;
> > +
> > +      /* FINISHME */
> > +      assert(linked_xfb_info->Outputs[i].DstOffset ==
> > next_offset[buffer]);
> >
> 
> FYI, this assertion should hold true until we implement
> ARB_transfrom_feedback3 (which allows holes in the transform feedback
> structure).  I think Marek has some plans to implement that for Gallium
> (not sure of his timeframe though), so we may want to keep an eye out.

Thanks for the clarification there.  I thought that was probably the
case (it was for our tests), but wasn't sure.  For the record, I didn't
even look at the extension spec while writing the code, thanks to the
many testcases and your gen6 code to work from.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111223/2b28a54a/attachment.pgp>