[Mesa-stable] [Mesa-dev] [PATCH] i965: Select pipeline and emit state base address in Gen8+ HiZ ops.

[Nanley Chery]

On Wed, Mar 08, 2017 at 10:27:20AM -0800, Nanley Chery wrote:
> On Wed, Mar 08, 2017 at 10:07:12AM -0800, Nanley Chery wrote:
> > On Wed, Mar 08, 2017 at 02:17:59AM -0800, Kenneth Graunke wrote:
> > > On Thursday, March 2, 2017 4:36:08 PM PST Nanley Chery wrote:
> > > > On Mon, Feb 06, 2017 at 03:55:49PM -0800, Kenneth Graunke wrote:
> > > > > If a HiZ op is the first thing in the batch, we should make sure
> > > > > to select the render pipeline and emit state base address before
> > > > > proceeding.
> > > > > 
> > > > > I believe 3DSTATE_WM_HZ_OP creates 3DPRIMITIVEs internally, and
> > > > > dispatching those on the GPGPU pipeline seems a bit sketchy.  I'm
> > > > 
> > > > Yes, it does seem like we currently allow HZ_OPs within a GPGPU
> > > > pipeline. This patch should fix that problem.
> > > > 
> > > > > not actually sure that STATE_BASE_ADDRESS is necessary, as the
> > > > > depth related commands use graphics addresses, not ones relative
> > > > > to the base address...but we're likely to set it as part of the
> > > > > next operation anyway, so we should just do it right away.
> > > > > 
> > > > 
> > > > I agree, re-emitting STATE_BASE_ADDRESS doesn't seem necessary. I think
> > > > we should drop this part of the patch and add it back in later if we get
> > > > some data that it's necessary. Leaving it there may be distracting to
> > > > some readers and the BDW PRM warns that it's an expensive command:
> > > > 
> > > > 	Execution of this command causes a full pipeline flush, thus its
> > > > 	use should be minimized for higher performance.
> > > 
> > > I think it should be basically free, actually.  We track a boolean,
> > > brw->batch.state_base_address_emitted, to avoid emitting it multiple
> > > times per batch.
> > > 
> > > Let's say the first thing in a fresh batch is a HiZ op, followed by
> > > normal drawing.  Previously, we'd do:
> > > 
> > >     1. HiZ op commands
> > >     2. STATE_BASE_ADDRESS (triggered by normal rendering upload)
> > >     3. rest of normal drawing commands
> > > 
> > > Now we'd do:
> > > 
> > >     1. STATE_BASE_ADDRESS (triggered by HiZ op)
> > >     2. HiZ op commands
> > >     3. normal drawing commands (second SBA is skipped)
> > > 
> > > In other words...we're just moving it a bit earlier.  I suppose there
> > > could be a batch containing only HiZ ops, at which point we'd pay for
> > > a single STATE_BASE_ADDRESS...but that seems really unlikely.
> > > 
> > 
> > Sorry for not stating it up front, but the special case you've mentioned
> > is exactly what I'd like not to hurt unnecessarily.
> > 
> 
> Correct me if I'm wrong, but after thinking about it some more, it seems
> that performance wouldn't suffer by emitting the SBA since the pipeline
> was already flushed at the end of the preceding batch. It may also
> improve since the pipelined HiZ op will likely be followed by other
> pipelined commands. I'm not totally confident in my understanding on
> pipeline flushes by the way. Is this why you'd like to emit the SBA here?
> I think it's fine to leave it if we expound on the rationale.
> 

Thinking about it some more, we probably don't want to make separate
performance and bug-fix changes in the same patch. I have another
comment at the bottom of this email.

> -Nanley
> 
> > > > > Cc: "17.0" <mesa-stable at lists.freedesktop.org>
> > > > > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > > > > ---
> > > > >  src/mesa/drivers/dri/i965/gen8_depth_state.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > > index a7e61354fd5..620b32df8bb 100644
> > > > > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > > @@ -404,6 +404,9 @@ gen8_hiz_exec(struct brw_context *brw, struct intel_mipmap_tree *mt,
> > > > >     if (op == BLORP_HIZ_OP_NONE)
> > > > >        return;
> > > > >  
> > > > 
> > > > It would be helpful if you included the rationale here as a code
> > > > comment. Something like the first two sentences of your commit message
> > > > should work.
> > > 
> > > I can do that.
> > > 
> > > > > +   brw_select_pipeline(brw, BRW_RENDER_PIPELINE);
> > > > 
> > > > According to Vol07 of the BDW+ PRMs,
> > > > 
> > > > 	The previously active pipeline needs to be flushed via the
> > > > 	MI_FLUSH command immediately before switching to a different
> > > > 	pipeline via use of the PIPELINE_SELECT command.
> > > > 
> > > > However it doesn't look like MI_FLUSH is present after HSW. So there
> > > > shouldn't be any additional work to do here.
> > > 
> > > Flushes are definitely required when switching the pipeline, but I
> > > believe that brw_emit_select_pipeline() does that work.
> > > 
> > > FWIW, MI_FLUSH was replaced by PIPE_CONTROL many generations ago.
> > > I believe the validation team stopped testing MI_FLUSH on Sandybridge.
> > > 
> > 
> > Thanks for letting me know about the MI_FLUSH replacement. Do you know
> > what bits must be set to perform an equivalent operation? From the looks of
> > it, brw_emit_select_pipeline() actually avoids emitting PIPE_CONTROL BDW+.
> > 

I'd assume that the following new commit implements the equivalent
flushing operation:

   commit bd25d9670b466043cdb5d9668f82accbd587c889
   Author: Topi Pohjolainen <topi.pohjolainen at intel.com>
   Date:   Wed Mar 15 21:31:07 2017 +0200
   
   i965/gen8+: Do full stall when switching pipeline
   
   just as earlier gens do.
   
   CC: "17.0 13.0" <mesa-stable at lists.freedesktop.org>
   Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96743
   Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
   Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>