[Intel-gfx] [PATCH v2] drm/i915:bxt: implement WaProgramL3SqcReg1DefaultForPerf

[Daniel Vetter]

On Thu, Apr 21, 2016 at 08:19:46AM +0000, Gore, Tim wrote:
> 
> 
> Tim Gore 
> Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ
> 
> 
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > Sent: Wednesday, April 20, 2016 7:48 PM
> > To: Ville Syrjälä
> > Cc: Gore, Tim; Daniel Vetter; Thierry, Michel; intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915:bxt: implement
> > WaProgramL3SqcReg1DefaultForPerf
> > 
> > On Wed, Apr 20, 2016 at 09:31:57PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 20, 2016 at 07:19:32PM +0100, Chris Wilson wrote:
> > > > On Wed, Apr 20, 2016 at 03:51:49PM +0000, Gore, Tim wrote:
> > > > >
> > > > > Tim Gore
> > > > > Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way,
> > > > > Swindon SN3 1RJ
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Thierry, Michel
> > > > > > Sent: Wednesday, April 20, 2016 4:00 PM
> > > > > > To: Gore, Tim; intel-gfx at lists.freedesktop.org
> > > > > > Subject: Re: [PATCH v2] drm/i915:bxt: implement
> > > > > > WaProgramL3SqcReg1DefaultForPerf
> > > > > >
> > > > > > On 4/20/2016 3:23 PM, tim.gore at intel.com wrote:
> > > > > > > From: Tim Gore <tim.gore at intel.com>
> > > > > > >
> > > > > > > This patch applies a performance enhancement workaround based
> > > > > > > on analysis of DX and OCL S-Curve workloads.
> > > > > > >
> > > > > > > v2: Only apply to B0 onwards
> > > > > > >
> > > > > > > Signed-off-by: Tim Gore <tim.gore at intel.com>
> > > > > > > ---
> > > > > > >   drivers/gpu/drm/i915/i915_reg.h | 1 +
> > > > > > >   drivers/gpu/drm/i915/intel_pm.c | 4 ++++
> > > > > > >   2 files changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > b/drivers/gpu/drm/i915/i915_reg.h index f0a6d85..13e154a
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > @@ -6074,6 +6074,7 @@ enum skl_disp_power_wells {
> > > > > > >
> > > > > > >   #define GEN8_L3SQCREG1
> > 	_MMIO(0xB100)
> > > > > > >   #define  BDW_WA_L3SQCREG1_DEFAULT		0x784000
> > > > > > > +#define  BXT_WA_L3SQCREG1_DEFAULT		0xF84000
> > > > > > >
> > > > > > >   #define GEN7_L3CNTLREG1
> > 	_MMIO(0xB01C)
> > > > > > >   #define  GEN7_WA_FOR_GEN7_L3_CONTROL
> > > > > > 	0x3C47FF8C
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > b/drivers/gpu/drm/i915/intel_pm.c index b7c2186..eecdc3a6
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > @@ -76,6 +76,10 @@ static void bxt_init_clock_gating(struct
> > > > > > > drm_device
> > > > > > *dev)
> > > > > > >   	if (IS_BXT_REVID(dev_priv, BXT_REVID_B0,
> > REVID_FOREVER))
> > > > > > >   		I915_WRITE(GEN9_CLKGATE_DIS_0,
> > > > > > I915_READ(GEN9_CLKGATE_DIS_0) |
> > > > > > >   			   PWM1_GATING_DIS |
> > PWM2_GATING_DIS);
> > > > > > > +
> > > > > > > +	/* WaProgramL3SqcReg1DefaultForPerf:bxt */
> > > > > > > +	if (IS_BXT_REVID(dev, BXT_REVID_B0, REVID_FOREVER))
> > > > > > > +		I915_WRITE(GEN8_L3SQCREG1,
> > > > > > BXT_WA_L3SQCREG1_DEFAULT);
> > > > > > >   }
> > > > > > >
> > > > > > >   static void i915_pineview_get_mem_freq(struct drm_device
> > > > > > > *dev)
> > > > > > >
> > > > > >
> > > > > > Isnt it better to add this to bxt_init_workarounds() instead of
> > > > > > bxt_init_clock_gating()?
> > > > >
> > > > > There is an equivalent Wa for bdw in bdw_init_clock_gating, so I
> > > > > just tried to stay consistent with that. I'm not sure if this W/a
> > > > > will persist across suspend/resume. bxt_init_workarounds only gets
> > > > > called after reset, not on resume as far as I know. I'll
> > > > > investigate further
> > > >
> > > > init_clock_gating() is called on init/reset/resume. Use it to set
> > > > global registers. (Once upon a time it did only setup the clock
> > > > gatings...)
> > >
> > > It's not called on reset. Or at least that was the case last time I
> > > looked. Which is a rather big problem. My old idea was that we'd move
> > > anything that gets clobbered by a GPU reset out from init_clock_gating
> > > into some more suitable place that does get called on reset.
> > 
> > Oh, I call upon Daniel!
> > -Chris
> > 
> Looks to me that init_clock_gating is called on reset thus:
>     I915_handle_error
>       ->  i915_reset_and_wakeup
>          ->  intel_finish_reset
>             ->  intel_modeset_init_hw
>                ->  intel_init_clock_gating
>                   ->  display.init_clock_gating
> 
> Or perhaps we should put WaProgramL3SqcReg1DefaultForPerf in the new
> gtt_write_workarounds function in i915_gem_gtt.c. I believe this is called on
> Init, reset and resume. ?

intel_modeset_init_hw should only be called on old crap where a gpu reset
kills the entire chip, including the display block. That's not the case on
modern stuff, much less once we have TDR.

If it's getting called unconditionally that's a bug.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch