Re: [PATCH 04/10] drm/i915: add RPS configuration for Haswell

[Ben Widawsky <ben@bwidawsk.net>]

On Mon,  2 Jul 2012 11:51:05 -0300
Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:

> Most of the RPS and RC6 enabling functionality is similar to what we had
> on Gen6/Gen7, so we preserve most of the registers.
> 
> Note that Haswell only has RC6, so account for that as well. As suggested
> by Daniel Vetter, to reduce the amount of changes in the patch, we still
> write the RC6p/RC6pp thresholds, but those are ignored on Haswell.
> 
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++++++++++++++------------
>  2 files changed, 26 insertions(+), 12 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..a3ee1b1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2402,20 +2402,24 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	I915_WRITE(GEN6_RC6p_THRESHOLD, 100000);
>  	I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
>  
> +	/* 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;
>  
> -	if (rc6_mode & INTEL_RC6p_ENABLE)
> -		rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
> +	/* We don't use those on Haswell */
> +	if (!IS_HASWELL(dev)) {
> +		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");

Belongs in a separate patch, but meh.

>  
>  	I915_WRITE(GEN6_RC_CONTROL,
>  		   rc6_mask |
> @@ -2433,10 +2437,19 @@ 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);
> +	}
> +

I can't really comment much on this other than what I said below. It'd
be nice if the policy didn't change between platforms unless we
know/understand why.

>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
>  	I915_WRITE(GEN6_RP_CONTROL,
>  		   GEN6_RP_MEDIA_TURBO |
> @@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>  		   GEN6_RP_MEDIA_IS_GFX |
>  		   GEN6_RP_ENABLE |
>  		   GEN6_RP_UP_BUSY_AVG |
> -		   GEN6_RP_DOWN_IDLE_CONT);
> +		   (IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT);

I think this warrants an explanation. I think we would ideally like to
avoid different algorithms for different platforms.

>  
>  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
>  		     500))



-- 
Ben Widawsky, Intel Open Source Technology Center