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

[Nanley Chery]

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.

-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+.
> 
> -Nanley
> 
> > --Ken
> 
>