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

[Kenneth Graunke]

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.

> > 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.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20170308/bf1466a7/attachment.sig>