[Mesa-dev] [PATCH 3/6] i965/gen6-7: Implement stall and flushes required prior to switching pipelines.

[Kenneth Graunke]

On Sunday, January 3, 2016 3:41:08 PM PST Francisco Jerez wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > On Saturday, January 2, 2016 10:48:02 PM PST Francisco Jerez wrote:
> >> Switching the current pipeline while it's not completely idle or the
> >> read and write caches aren't flushed can lead to corruption.  Fixes
> >> misrendering of at least the following Khronos CTS test:
> >> 
> >>  ES31-CTS.shader_image_load_store.basic-allTargets-store-fs
> >> 
> >> The stall and flushes are no longer required on Gen8+.
> >> 
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93323
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_misc_state.c | 28 ++++++++++++++++++++++++
+++
> > +
> >>  1 file changed, 28 insertions(+)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/
drivers/
> > dri/i965/brw_misc_state.c
> >> index 7d53d18..75540c1 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> >> @@ -886,6 +886,34 @@ brw_emit_select_pipeline(struct brw_context *brw, 
enum 
> > brw_pipeline pipeline)
> >>  
> >>           brw->ctx.NewDriverState |= BRW_NEW_CC_STATE;
> >>        }
> >> +
> >> +   } else if (brw->gen >= 6) {
> >> +      /* From "BXML » GT » MI » vol1a GPU Overview » [Instruction]
> >> +       * PIPELINE_SELECT [DevBWR+]":
> >
> > Can we cite the public docs?
> >
> 
> The public docs for PIPELINE_SELECT seemed rather inaccurate.  The IVB
> version I have in front of me right now is missing this one workaround,
> and the BDW version mentions it incorrectly.  Sigh...
> 
> >> +       *
> >> +       *   Project: DEVSNB+
> >> +       *
> >> +       *   Software must ensure all the write caches are flushed through 
a
> >> +       *   stalling PIPE_CONTROL command followed by another 
PIPE_CONTROL
> >> +       *   command to invalidate read only caches prior to programming
> >> +       *   MI_PIPELINE_SELECT command to change the Pipeline Select 
Mode.
> >> +       */
> >> +      const unsigned dc_flush =
> >> +         brw->gen >= 7 ? PIPE_CONTROL_DATA_CACHE_INVALIDATE : 0;
> >
> > I was going to suggest doing a brw_emit_post_sync_nonzero_flush first
> > on Sandybridge, but I forgot that we now just emit that at the start
> > of every state upload.  Fairly moot anyway since we don't do GPGPU on
> > Sandybridge anyway.
> >
> Hmm, that sounds very sensible to me, it would be rather fragile for
> this function to rely on a flush with post-sync op having been done
> previously, even if at this point this will only be called once at
> context creation on SNB -- Although for the same reason it seems rather
> fragile for brw_emit_pipe_control_flush() to assume that the workaround
> has been applied already.  I'd be inclined to change
> brw_emit_pipe_control_flush() to emit the post-sync op when needed on
> SNB just like we do for other PIPE_CONTROL workarounds on Gen7 and Gen8.

Yeah, that's probably a better idea - these PIPE_CONTROL helpers didn't
exist back in the day.  I originally moved the SNB workaround to the
start of every draw operation because we were forgetting it in various
places, and it was really fragile.   Putting it in the pipe control
emitter functions would also solve the "I forgot it" problem.

That said, what we have seems to work...so...*shrug*... 

> 
> >> +
> >> +      brw_emit_pipe_control_flush(brw,
> >> +                                  PIPE_CONTROL_RENDER_TARGET_FLUSH |
> >> +                                  PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> >> +                                  dc_flush |
> >> +                                  PIPE_CONTROL_NO_WRITE |
> >> +                                  PIPE_CONTROL_CS_STALL);
> >
> > Why RENDER_TARGET_FLUSH, DEPTH_CACHE_FLUSH, DATA_CACHE_INVALIDATE,
> > and NO_WRITE?  The cited workaround explains a CS Stall and the RO
> > invalidations below, but I'm not seeing why the others are needed.
> >
> It also says that "software must ensure all the write caches are
> flushed".

Oh, somehow I misread that as basically "Software must ensure that
<some stuff> by doing a stalling PIPE_CONTROL."  But that alone
definitely doesn't flush write caches.  Thanks, that makes total sense.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160104/0ab9873a/attachment.sig>