[Mesa-dev] [PATCH 8/8] i965: Flush pipeline on EndTransformFeedback.

[Paul Berry]

On 14 December 2011 01:44, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 12/13/2011 03:35 PM, Paul Berry wrote:
> > A common use case for transform feedback is to perform one draw
> > operation that writes transform feedback output to a buffer, followed
> > by a second draw operation that consumes that buffer as vertex input.
> > Since vertex input is consumed at an earlier pipeline stage than
> > writing transform feedback output, we need to flush the pipeline to
> > ensure that the transform feedback output is completely written before
> > the data is consumed.
> >
> > In an ideal world, we would do some dependency tracking, so that we
> > would only flush the pipeline if the next draw call was about to
> > consume data generated by a previous draw call in the same batch.
> > However, since we don't have that sort of dependency tracking
> > infrastructure right now, we just unconditionally flush the buffer
> > every time glEndTransformFeedback() is called.  This will cause a
> > performance hit compared to the ideal case (since we will sometimes
> > flush the pipeline unnecessarily), but fortunately the performance hit
> > will be confined to circumstances where transform feedback is in use.
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.c |    8 ++++++--
> >  src/mesa/drivers/dri/i965/brw_context.h |    5 +++++
> >  src/mesa/drivers/dri/i965/gen6_sol.c    |   16 ++++++++++++++++
> >  3 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> > index eb68bf4..f7b88c3 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -52,6 +52,7 @@
> >
> >  #include "tnl/t_pipeline.h"
> >  #include "glsl/ralloc.h"
> > +#include "intel_batchbuffer.h"
> >
> >  /***************************************
> >   * Mesa's Driver Functions
> > @@ -109,7 +110,8 @@ static void brwPrepareExecBegin(struct gl_context
> *ctx)
> >     }
> >  }
> >
> > -static void brwInitDriverFunctions( struct dd_function_table *functions
> )
> > +static void brwInitDriverFunctions(struct intel_context *intel,
> > +                                   struct dd_function_table *functions)
> >  {
> >     intelInitDriverFunctions( functions );
> >
> > @@ -117,6 +119,8 @@ static void brwInitDriverFunctions( struct
> dd_function_table *functions )
> >     brw_init_queryobj_functions(functions);
> >
> >     functions->PrepareExecBegin = brwPrepareExecBegin;
> > +   if (intel->gen == 6)
> > +      functions->EndTransformFeedback = gen6_end_transform_feedback;
>
> Unfortunately, this doesn't work: brwInitDriverFunctions is called
> before intelInitContext, so intel->gen isn't set yet.  So this function
> pointer never gets set, and gen6_end_transform_feedback never happens.
>
> I blithely tried moving the brwInitDriverFunctions call after
> intelInitContext, but that crashes.  Also, setting anything in
> "functions" after intelInitContext simply doesn't take; something
> must've copied the table (remapping code most likely).  In other words,
> a generation check just isn't going to happen.
>
> I think the best solution is to set the function pointer unconditionally
> and add
>
>   if (intel->gen < 6)
>      return;
>
> to the top of gen6_end_transform_feedback so it becomes a no-op on
> earlier platforms.
>
> Sorry for suggesting the generation check earlier; I didn't realize it
> was impossible.
>

Thanks for catching this. I blithely re-ran my tests after you made your
suggestion yesterday, and since there were no additional failures I figured
everything was ok.  It turns out that an additional flush was happening
that was masking the problem (the flush we do when reassigning GS URB slots
to the VS).  But that flush won't always happen.  I'm planning to do
another update to the transform feedback tests soon, so I'll try to catch
that case when I do.

On further reflection, adding "if (intel->gen < 6) return;" to the top of
gen6_end_transform_feedback() is unnecessary.  At the moment we don't
support transform feedback when gen < 6, so the function will never be
called on earlier platforms.  And if we do add transform feedback support
to gen < 6 in the future, we will need to do this flush.  We'll need to do
the flush on gen7 too.  So I'm just going to set the function pointer
unconditionally and rename gen6_end_transform_feedback() to
brw_end_transform_feedback().

Revised patch to follow.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111214/6ab9eaaf/attachment-0001.htm>