Re: [PATCH v2 5/5] drm/i915/skl: Updated the gen9_enable_rps function

[Damien Lespiau <damien.lespiau@intel.com>]

On Wed, Feb 18, 2015 at 07:31:17PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> On SKL, GT frequency is programmed in units of 16.66 MHZ units compared
> to 50 MHZ for older platforms. Also the time value specified for Up/Down EI &
> Up/Down thresholds are expressed in units of 1.33 us, compared to 1.28
> us for older platforms. So updated the gen9_enable_rps function as per that.
> 
> v2: Updated to use new macro GT_INTERVAL_FROM_US
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Ok, we might as well throw away the init sequence from the PM guide if
it's to reuse code that is easier to read. While what you're doing seems
correct, we might as well go full on and reuse gen6_set_rps() entirely?

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e08a710..6532060 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4035,27 +4035,50 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
>  static void gen9_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 threshold_up, threshold_down; /* in % */
> +	u32 ei_up, ei_down;
>  
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	gen6_init_rps_frequencies(dev);
>  
> -	I915_WRITE(GEN6_RPNSWREQ, 0xc800000);
> -	I915_WRITE(GEN6_RC_VIDEO_FREQ, 0xc800000);
> +	/* Program defaults and thresholds for RPS*/
> +	I915_WRITE(GEN6_RPNSWREQ,
> +		GEN9_FREQUENCY(dev_priv->rps.rp1_freq * GEN9_FREQ_SCALER));

This register is going to be overridde by gen6_set_rps() very soon, how
about letting that function do it? (cur_freq should be 0 at this point
so set_rps() should do something).

> +	I915_WRITE(GEN6_RC_VIDEO_FREQ,
> +		GEN9_FREQUENCY(dev_priv->rps.rp1_freq * GEN9_FREQ_SCALER));

Note that we don't seem to use video turbo in the rest of the driver,
gen6_set_rps() will set GEN6_RP_MEDIA_HW_NORMAL_MODE, which ignores the
both the turbo bit and the requested freq from the video turbo register.
Seems fine to initialize it to RP1 though.

> +
> +	ei_up = 84480; /* 84.48ms */
> +	ei_down = 448000;
> +	threshold_up = 90;
> +	threshold_down = 70;
> +
> +	I915_WRITE(GEN6_RP_UP_EI,
> +		GT_INTERVAL_FROM_US(ei_up));
> +	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> +		GT_INTERVAL_FROM_US((ei_up * threshold_up / 100)));

Those 2 are going to be overridden by gen6_set_rps().

> +
> +	I915_WRITE(GEN6_RP_DOWN_EI,
> +		GT_INTERVAL_FROM_US(ei_down));
> +	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
> +		GT_INTERVAL_FROM_US((ei_down * threshold_down / 100)));

Those 2 are going to be overridden by gen6_set_rps().

> +
> +	/* 1 second timeout*/
> +	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, GT_INTERVAL_FROM_US(1000000));

This one need to stay here.

> +	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> +		(dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23 |
> +		(dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14);

This one are going to be overridden by gen6_set_rps().

> -	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 0xf4240);
> -	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, 0x12060000);
> -	I915_WRITE(GEN6_RP_UP_THRESHOLD, 0xe808);
> -	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 0x3bd08);
> -	I915_WRITE(GEN6_RP_UP_EI, 0x101d0);
> -	I915_WRITE(GEN6_RP_DOWN_EI, 0x55730);
>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa);

This one need to stay here indeed.

> -	I915_WRITE(GEN6_PMINTRMSK, 0x6);
>  	I915_WRITE(GEN6_RP_CONTROL, GEN6_RP_MEDIA_TURBO |
>  		   GEN6_RP_MEDIA_HW_MODE | GEN6_RP_MEDIA_IS_GFX |
>  		   GEN6_RP_ENABLE | GEN6_RP_UP_BUSY_AVG |
>  		   GEN6_RP_DOWN_IDLE_AVG);

This is going to be overridden by gen6_set_rps(), including the media
turbo mode that is just going to use the normal freq.

> +	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> +	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +
>  	gen6_enable_rps_interrupts(dev);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx