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

[Nanley Chery]

On Mon, Mar 20, 2017 at 08:05:22PM -0700, Nanley Chery wrote:

Okay, I've re-read the email. 

> On Mon, Mar 20, 2017 at 08:01:25PM -0700, Nanley Chery wrote:
> > On Thu, Mar 16, 2017 at 05:34:13PM -0700, Kenneth Graunke wrote:
> > > On Wednesday, March 8, 2017 10:27:20 AM PDT 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.
> > > > > > > > 
> > 
> > Why should we do it right away if it will happen later on? I don't see
> > why this part of the patch is necessary.
> > 
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > 
> > > Why?  We really think there are going to be batches with only
> > > 3DSTATE_WM_HZ_OP and no normal rendering or BLORP?  It sounds
> > > really hypothetical to me.
> > > 
> > 
> > I've commented on the performance implications of that snippet because
> > it is the only functional change I can see from emitting SBA. That
> > unfortunately seems to have distracted us from the more important
> > question found above. Sorry about that.
> > 

I've commented on the performance implications of that snippet because
it is the only functional change I can see from emitting SBA.
Unfortunately, discussing the impact of this change is seems to have
distracted us from the more important question of why we're making this
change. Sorry about that.

> > > > 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.
> > > 
> > > Performance is not a motivation for this patch.  Having the GPU do
> > > work without a pipeline selected or state base addresses in place seems
> > > potentially dangerous.  I was hoping it would help with GPU hangs.  I'm
> > > not certain that it does, and it might be safe to skip this, but it
> > > seems like a lot of mental gymnastics to prove that it's safe for very
> > > little upside.
> > > 
> > 
> > I was only referring to the portion of the patch that emits SBA.
> > 
> 
> Sorry, I definitely did not read this thoroughly enough. Please ignore
> my earlier reply.

Do we have any data that suggests that we need to emit SBA before
emitting HiZ ops? If we don't, I don't think we should speculatively do
so.