[Intel-gfx] [Mesa-dev] [PATCH] drm/i915: Enable the HiZ RAW Stall Optimization on Gen8.

[Kenneth Graunke]

On Sunday, January 11, 2015 05:46:09 PM Ben Widawsky wrote:
> On Sun, Jan 11, 2015 at 04:05:25PM -0800, Kenneth Graunke wrote:
> > On Sunday, January 11, 2015 01:49:41 PM Ben Widawsky wrote:
> > > On Sat, Jan 10, 2015 at 06:44:49PM -0800, Kenneth Graunke wrote:
> > > > This is an important optimization for avoiding read-after-write (RAW)
> > > > stalls in the HiZ buffer.  Certain workloads would run very slowly with
> > > > HiZ enabled, but run much faster with the "hiz=false" driconf option.
> > > > With this patch, they run at full speed even with HiZ.
> > > > 
> > > > Improves performance in OglVSInstancing by 3.2x on Broadwell GT3e
> > > > (Iris Pro 6200).
> > > > 
> > > > Thanks to Jesse Barnes for finding this missing bit!
> > > > Thanks to Chris Wilson for helping me find where to set it.
> > > > 
> > > > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > > > Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > Here's an alternate patch which implements the workaround in the kernel
> > > > instead of Mesa.  It's probably better to do it there, since the kernel
> > > > does it on Haswell already.
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > index dabc1d8..23020d6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > @@ -796,6 +796,16 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
> > > >  			  HDC_DONOT_FETCH_MEM_WHEN_MASKED |
> > > >  			  (IS_BDW_GT3(dev) ? HDC_FENCE_DEST_SLM_DISABLE : 0));
> > > >  
> > > > +	/* From the Haswell PRM, Command Reference: Registers, CACHE_MODE_0:
> > > > +	 * "The Hierarchical Z RAW Stall Optimization allows non-overlapping
> > > > +	 *  polygons in the same 8x4 pixel/sample area to be processed without
> > > > +	 *  stalling waiting for the earlier ones to write to Hierarchical Z
> > > > +	 *  buffer."
> > > > +	 *
> > > > +	 * This optimization is off by default for Broadwell; turn it on.
> > > > +	 */
> > > > +	WA_CLR_BIT_MASKED(CACHE_MODE_0_GEN7, HIZ_RAW_STALL_OPT_DISABLE);
> > > > +
> > > >  	/* Wa4x4STCOptimizationDisable:bdw */
> > > >  	WA_SET_BIT_MASKED(CACHE_MODE_1,
> > > >  			  GEN8_4x4_STC_OPTIMIZATION_DISABLE);
> > > > @@ -836,6 +846,11 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
> > > >  			  HDC_FORCE_NON_COHERENT |
> > > >  			  HDC_DONOT_FETCH_MEM_WHEN_MASKED);
> > > >  
> > > > +	/* According to the CACHE_MODE_0 default value documentation, some
> > > > +	 * CHV platforms disable this optimization by default.  Turn it on.
> > > > +	 */
> > > > +	WA_CLR_BIT_MASKED(CACHE_MODE_0_GEN7, HIZ_RAW_STALL_OPT_DISABLE);
> > > > +
> > > >  	/* Improve HiZ throughput on CHV. */
> > > >  	WA_SET_BIT_MASKED(HIZ_CHICKEN, CHV_HZ_8X8_MODE_IN_1X);
> > > >  
> > > 
> > > I think you should do this as two separate patches, 1 per platform. For the BSW
> > > patch (given that I had the same functionality in the kernel patch I asked you
> > > to look at ;-) and FWIW, Jordan has numbers on BSW B-step with my kernel patch
> > > which we can use for the commit):
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > 
> > Huh, I don't recall seeing that kernel patch.  Sorry.  I guess I'll split it
> > and resubmit...
> > 
> 
> It's not my call, it's just nice to have platform specific bisection. And the
> patch wasn't on the list, it was the one I kept asking you to look at in my
> branch :-)
>
> > > I haven't looked at Broadwell docs, so I'll let someone else take care of that.
> > > 
> > > I don't know if I agree with Chris that we should call these in the workaround
> > > section, but whatever. init_clock_gating is equally sucky.
> > 
> > init_clock_gating doesn't work.  The register writes don't stick and they have
> > no effect at all.  Setting them here makes them actually take effect in the
> > context.
> > 
> > --Ken
> 
> Separate thread now, but are you sure? We're setting at least two context
> specific registers in there today, among them: GEN7_FF_THREAD_MODE (which is
> important to performance).
> 
> AFAIK it should stick, and if it doesn't it's not expected behavior. Unless you
> know something I do not?

Jesse had suggested setting it in broadwell_init_clock_gating on January 5th,
and Valtteri tried it on January 7th.  He found "no noticeable difference".
I tried it again, and confirmed his result: there was zero performance impact.

Setting it via an LRI in Mesa did have a performance impact.  I reverted my
Mesa patch, and tried setting it here, and it had the same performance impact.
I rebooted between kernels several times to confirm.  It works here, but it
doesn't there.

I'm pretty sure I confirmed the same result with this bit.  Feel free to try.

Perhaps we should move the rest of the per-context bits here instead of
*_init_clock_gating.  We should also confirm that the other bits are actually
having an effect.

I don't know why it works on Haswell, but it does there - the HiZ RAW stall
bit is set via haswell_init_clock_gating, and it's clearly having an impact.
Maybe it has something to do with the golden context, which is new on BDW.
But I'm probably wrong about that.  Setting it when a context is active does
seem more reliable...

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150111/2077de9a/attachment.sig>