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

[Emil Velikov]

On 21 March 2017 at 03:05, Nanley Chery <nanleychery at gmail.com> wrote:
> 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.
>>
>> > > 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.

Gents, with the above discussion in mind should we land this patch,
someone is working on alternative solution, or other?

Thanks
Emil