Re: [PATCH v2 3/4] drm/i915/chv: Tune L3 SQC credits based on actual latencies

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Apr 26, 2016 at 07:39:33PM +0300, Imre Deak wrote:
> While browsing BSpec I bumped into a note saying we need to tune these
> values based on actual measurements done after initial enabling. I've
> checked that it indeed improves things on BXT. I haven't checked this on
> CHV, but here it is if someone wants to give it a go.
> 
> v2:
> - Add note about the discrepancy wrt. to the spec in the formula
>   calculating the credit encodings. (Mika, Ville)
> - Move the WA comment to the new function. (Ville)
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> CC: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c | 47 +++++++++++++++++++++++++++--------------
>  2 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0cb2e17..e25e78f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6073,6 +6073,12 @@ enum skl_disp_power_wells {
>  #define  VLV_B0_WA_L3SQCREG1_VALUE		0x00D30000
>  
>  #define GEN8_L3SQCREG1				_MMIO(0xB100)
> +/*
> + * Note that on CHV the following has an off-by-one error wrt. to BSpec.
> + * Using the formula in BSpec leads to a hang, while the formula here works
> + * fine and matches the formulas for all other platforms. A BSpec change
> + * request has been filed to clarify this.
> + */
>  #define  L3_GENERAL_PRIO_CREDITS(x)		(((x) >> 1) << 19)
>  #define  L3_HIGH_PRIO_CREDITS(x)		(((x) >> 1) << 14)
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a9b7626..b217c44 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6669,11 +6669,38 @@ static void lpt_suspend_hw(struct drm_device *dev)
>  	}
>  }
>  
> +static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
> +				   int general_prio_credits,
> +				   int high_prio_credits)
> +{
> +	u32 misccpctl;
> +
> +	/*
> +	 * WaProgramL3SqcReg1Default:bdw

I would have left this first w/a comment in the caller.

> +	 * WaTempDisableDOPClkGating:bdw

This one seems good here.

> +	 * For CHV see gfxspecs/Related Documents/Performance Guide/
> +	 * LSQC Setting Recommendations[CHV,BXT].

The chv comment would also seems better placed in the caller.

With those 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	 */
> +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> +
> +	I915_WRITE(GEN8_L3SQCREG1,
> +		   L3_GENERAL_PRIO_CREDITS(general_prio_credits) |
> +		   L3_HIGH_PRIO_CREDITS(high_prio_credits));
> +
> +	/*
> +	 * Wait at least 100 clocks before re-enabling clock gating.
> +	 * See the definition of L3SQCREG1 in BSpec.
> +	 */
> +	POSTING_READ(GEN8_L3SQCREG1);
> +	udelay(1);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +}
> +
>  static void broadwell_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum pipe pipe;
> -	uint32_t misccpctl;
>  
>  	ilk_init_lp_watermarks(dev);
>  
> @@ -6704,21 +6731,7 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
>  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
>  
> -	/*
> -	 * WaProgramL3SqcReg1Default:bdw
> -	 * WaTempDisableDOPClkGating:bdw
> -	 */
> -	misccpctl = I915_READ(GEN7_MISCCPCTL);
> -	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> -	I915_WRITE(GEN8_L3SQCREG1, L3_GENERAL_PRIO_CREDITS(30) |
> -				   L3_HIGH_PRIO_CREDITS(2));
> -	/*
> -	 * Wait at least 100 clocks before re-enabling clock gating. See
> -	 * the definition of L3SQCREG1 in BSpec.
> -	 */
> -	POSTING_READ(GEN8_L3SQCREG1);
> -	udelay(1);
> -	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +	gen8_set_l3sqc_credits(dev_priv, 30, 2);
>  
>  	/*
>  	 * WaGttCachingOffByDefault:bdw
> @@ -6988,6 +7001,8 @@ static void cherryview_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
>  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
>  
> +	gen8_set_l3sqc_credits(dev_priv, 38, 2);
> +
>  	/*
>  	 * GTT cache may not work with big pages, so if those
>  	 * are ever enabled GTT cache may need to be disabled.
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx