[Mesa-dev] [PATCH 7/8] i965 gen6+: Make intel_batchbuffer_emit_mi_flush() actually flush.

[Paul Berry]

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

> On 12/13/2011 03:35 PM, Paul Berry wrote:
> > Previous to this patch, the function intel_batchbuffer_emit_mi_flush()
> > was a bit of a misnomer.  On Gen4+, when not using the blit engine, it
> > didn't actually flush the pipeline--it simply generated a
> > _3DSTATE_PIPE_CONTROL command with the necessary bits set to flush GPU
>
> It's actually just called "PIPE_CONTROL", never 3DSTATE_PIPE_CONTROL.
>

(Checks the docs).  Hmm, you're right.  For some reason we call it
_3DSTATE_PIPE_CONTROL in our #defines (see intel_reg.h).  Still, it seems
better for the commit message to match the documentation.  I'll change the
commit message.


>
> > caches.  This was usually sufficient, since in most situations where
> > intel_batchbuffer_emit_mi_flush() wass called, all we really care
>
> "... was called, all we really cared" (typo, tense)
>

Heh, oops.


>
> This makes a lot of sense.  At some point, I think we ought to convert
> some of the callers of this function to emit the proper PIPE_CONTROL
> directly (with only the necessary bits and workarounds).  But this is
> the right way to start.  Nice work figuring this out.
>

Agreed.  As my CS professor Bob Keller once said, "get it right first, then
make it fast."


>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> > about was ensuring cache coherency.
> >
> > However, with the advent of OpenGL 3.0, there are two cases in which
> > data output by one stage of the pipeline might be consumed, in a later
> > draw operation, by an earlier stage of the pipeline:
> >
> > (a) When using textures in the vertex shader.
> >
> > (b) When using drawing with a vertex buffer that was previously
> >     generated using transform feedback.
> >
> > This patch addresses case (a) by changing
> > intel_batchbuffer_emit_mi_flush() so that on Gen6+, it sets the
> > PIPE_CONTROL_CS_STALL bit (this forces the pipeline to actually
> > flush).  (Case (b) will be addressed by the next patch in the series).
> >
> > This is not an ideal solution--in a perfect world, the driver would
> > have some buffer dependency tracking so that we would only have to
> > flush the pipeline in the two cases above.  Until that dependency
> > tracking is implemented, however, it seems prudent to have
> > intel_batchbuffer_emit_mi_flush() actually flush the pipeline, so that
> > we get correct rendering, at the expense of a (hopefully small)
> > performance hit.
> >
> > The change is only applied to Gen6+, since at the moment only Gen6+
> > supports the OpenGL 3.0 features that make a full pipeline flush
> > necessary.
> > ---
> >  src/mesa/drivers/dri/intel/intel_batchbuffer.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > index 6991db8..4ff098a 100644
> > --- a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > @@ -461,7 +461,8 @@ intel_batchbuffer_emit_mi_flush(struct intel_context
> *intel)
> >                  PIPE_CONTROL_WRITE_FLUSH |
> >                  PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> >                  PIPE_CONTROL_TC_FLUSH |
> > -                PIPE_CONTROL_NO_WRITE);
> > +                PIPE_CONTROL_NO_WRITE |
> > +                   PIPE_CONTROL_CS_STALL);
> >        OUT_BATCH(0); /* write address */
> >        OUT_BATCH(0); /* write data */
> >        ADVANCE_BATCH();
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111214/4cfaeb6c/attachment-0001.htm>