Re: [PATCH v2] drm/i915: Update rps interrupt limits

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 12, 2013 at 03:04:48PM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> sysfs changes to rps min and max delay were only triggering an update
> of the rps interrupt limits if the active delay required an update.
> This change ensures that interrupt limits are always updated.

So this means that e.g. if we set the limit to 600MHz and the gpu runs at
that, and then we increase it to 800MHz or, then we'll never get the up
interrupt?

If that's the case then I think we should add subtests to the "does rps
still work" testcase from the earlier patch for resetting rps after a gpu
hang. Which means a behavioural test like I've suggested (instead of just
checking register values like Jesse suggested) sounds like the better
approach.

Looks like you guys are the only ones actually changing the limits at
runtime ;-)
-Daniel
> 
> v2: correct compile issue missed on rebase
> 
> OTC-Tracker: VIZ-3144
> Change-Id: I0adb8d968190e138382b449dcec7e297743b9cca
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_pm.c   | 11 ++++++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 05d8b16..6f34e3a 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -349,6 +349,11 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
>  		else
>  			gen6_set_rps(dev, val);
>  	}
> +	else if (!IS_VALLEYVIEW(dev))
> +		/* We still need gen6_set_rps to process the new max_delay
> +		   and update the interrupt limits even though frequency
> +		   request is unchanged. */
> +		gen6_set_rps(dev, dev_priv->rps.cur_delay);
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> @@ -418,6 +423,11 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
>  		else
>  			gen6_set_rps(dev, val);
>  	}
> +	else if (!IS_VALLEYVIEW(dev))
> +		/* We still need gen6_set_rps to process the new min_delay
> +		   and update the interrupt limits even though frequency
> +		   request is unchanged. */
> +		gen6_set_rps(dev, dev_priv->rps.cur_delay);
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 172efa0..a4f45ed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3538,6 +3538,9 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  	dev_priv->rps.last_adj = 0;
>  }
>  
> +/* gen6_set_rps is called to update the frequency request, but should also be
> + * called when the range (min_delay and max_delay) is modified so that we can
> + * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
>  void gen6_set_rps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3546,8 +3549,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	WARN_ON(val > dev_priv->rps.max_delay);
>  	WARN_ON(val < dev_priv->rps.min_delay);
>  
> -	if (val == dev_priv->rps.cur_delay)
> +	if (val == dev_priv->rps.cur_delay) {
> +		/* min/max delay may still have been modified so be sure to
> +		 * write the limits value */
> +		I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> +			   gen6_rps_limits(dev_priv, val));
> +
>  		return;
> +	}
>  
>  	gen6_set_rps_thresholds(dev_priv, val);
>  
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch