Re: [PATCH 4/7] drm/i915: add RPS configuration for Haswell

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 29 Jun 2012 11:50:35 -0300
Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:

> Split Haswell-specific GT algorithms into its own function.
> 
> Note that Haswell only has RC6, so account for that as well.
> 
> v2: reuse gen6 routines to simplify the code flow.
> v2.1.: actually use proper RP controls for HSW.
> 
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 73 ++++++++++++++++++++++++++++-------------
>  2 files changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f17de3d..9d5bf06 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4156,6 +4156,7 @@
>  #define   GEN6_RP_UP_IDLE_MIN			(0x1<<3)
>  #define   GEN6_RP_UP_BUSY_AVG			(0x2<<3)
>  #define   GEN6_RP_UP_BUSY_CONT			(0x4<<3)
> +#define   GEN7_RP_DOWN_IDLE_AVG			(0x2<<0)
>  #define   GEN6_RP_DOWN_IDLE_CONT		(0x1<<0)
>  #define GEN6_RP_UP_THRESHOLD			0xA02C
>  #define GEN6_RP_DOWN_THRESHOLD			0xA030
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 29720d2..c6b5201 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2332,9 +2332,10 @@ int intel_enable_rc6(const struct drm_device *dev)
>  	if (INTEL_INFO(dev)->gen == 5)
>  		return 0;
>  
> -	/* Sorry Haswell, no RC6 for you for now. */
> +	/* Haswell does not has RC6p nor RC6pp
> +	 */
>  	if (IS_HASWELL(dev))
> -		return 0;
> +		return INTEL_RC6_ENABLE;

This looks a little convoluted.  Should we call this routine on gen5?
If not, do we need these checks and return values?  If we got rid of
them we could just move the HSW check to enable_rps instead...

Or just make it return the bitmask directly and do rc6_mask =
calc_rc6_bits() or somesuch.  That would save us lines in enable_rps.

>  
>  	/*
>  	 * Disable rc6 on Sandybridge
> @@ -2396,26 +2397,34 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	for_each_ring(ring, dev_priv, i)
>  		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>  
> +	/* Check if we are enabling RC6 */
> +	rc6_mode = intel_enable_rc6(dev_priv->dev);
> +
> +	if (rc6_mode & INTEL_RC6_ENABLE)
> +		rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
> +
> +	/* Write RC6 thresholds for RC1e and RC6 states */
>  	I915_WRITE(GEN6_RC_SLEEP, 0);
>  	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
>  	I915_WRITE(GEN6_RC6_THRESHOLD, 50000);
> -	I915_WRITE(GEN6_RC6p_THRESHOLD, 100000);
> -	I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
>  
> -	rc6_mode = intel_enable_rc6(dev_priv->dev);
> -	if (rc6_mode & INTEL_RC6_ENABLE)
> -		rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
> +	/* We don't use those on Haswell */
> +	if (!IS_HASWELL(dev)) {
> +		I915_WRITE(GEN6_RC6p_THRESHOLD, 100000);
> +		I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
> +
> +		if (rc6_mode & INTEL_RC6p_ENABLE)
> +			rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
>  
> -	if (rc6_mode & INTEL_RC6p_ENABLE)
> -		rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
> +		if (rc6_mode & INTEL_RC6pp_ENABLE)
> +			rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
>  
> -	if (rc6_mode & INTEL_RC6pp_ENABLE)
> -		rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
> +	}
>  
>  	DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> -			(rc6_mode & INTEL_RC6_ENABLE) ? "on" : "off",
> -			(rc6_mode & INTEL_RC6p_ENABLE) ? "on" : "off",
> -			(rc6_mode & INTEL_RC6pp_ENABLE) ? "on" : "off");
> +			(rc6_mask & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> +			(rc6_mask & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> +			(rc6_mask & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
>  
>  	I915_WRITE(GEN6_RC_CONTROL,
>  		   rc6_mask |
> @@ -2433,12 +2442,32 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
>  		   dev_priv->max_delay << 24 |
>  		   dev_priv->min_delay << 16);
> -	I915_WRITE(GEN6_RP_UP_THRESHOLD, 10000);
> -	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 1000000);
> -	I915_WRITE(GEN6_RP_UP_EI, 100000);
> -	I915_WRITE(GEN6_RP_DOWN_EI, 5000000);
> +
> +	if (IS_HASWELL(dev)) {
> +		I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> +		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
> +		I915_WRITE(GEN6_RP_UP_EI, 66000);
> +		I915_WRITE(GEN6_RP_DOWN_EI, 350000);
> +	} else {
> +		I915_WRITE(GEN6_RP_UP_THRESHOLD, 10000);
> +		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 1000000);
> +		I915_WRITE(GEN6_RP_UP_EI, 100000);
> +		I915_WRITE(GEN6_RP_DOWN_EI, 5000000);
> +	}
> +
>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> -	I915_WRITE(GEN6_RP_CONTROL,
> +
> +	/* We have a bit different default configuration for HSW */
> +	if (IS_HASWELL(dev))
> +		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 |
> +		   GEN7_RP_DOWN_IDLE_AVG);
> +	else
> +		I915_WRITE(GEN6_RP_CONTROL,
>  		   GEN6_RP_MEDIA_TURBO |
>  		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
>  		   GEN6_RP_MEDIA_IS_GFX |

The "else" here scares me a little since we may miss it with a new hw
generation.  Maybe a new 'gt' function pointer or two should be used to
write these?  That'll at least force us to implement it for new hw...

-- 
Jesse Barnes, Intel Open Source Technology Center