[Intel-gfx] [PATCH v2] drm/i915: Set softmin frequency on idle->busy transition

[Chris Wilson]

On Thu, Jun 16, 2016 at 05:19:49PM +0200, Michał Winiarski wrote:
> If the GPU load is low enough, it's possible that we'll be stuck at idle
> frequency rather than transition into softmin frequency requested by
> userspace.
> Since we assume that idle_freq <= min_freq_softlimit and
> valleyview_set_rps is already skipping write when
> requested_freq == cur_freq we can also remove vlv_set_idle function.
> 
> v2: Use intel_set_rps, drop vlv_set_idle
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=89728
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak at intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 658a756..41c5d25 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4798,6 +4798,7 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  	WARN_ON(val > dev_priv->rps.max_freq);
>  	WARN_ON(val < dev_priv->rps.min_freq);
> +	WARN_ON(val < dev_priv->rps.idle_freq);

The hw range is min_freq, max_freq.

We assert earlier if idle_freq is out of range.

>  
>  	if (WARN_ONCE(IS_CHERRYVIEW(dev_priv) && (val & 1),
>  		      "Odd GPU freq value\n"))
> @@ -4815,31 +4816,11 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
>  }
>  
> -/* vlv_set_rps_idle: Set the frequency to idle, if Gfx clocks are down
> - *
> - * * If Gfx is Idle, then
> - * 1. Forcewake Media well.
> - * 2. Request idle freq.
> - * 3. Release Forcewake of Media well.
> -*/
> -static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> -{
> -	u32 val = dev_priv->rps.idle_freq;
> -
> -	if (dev_priv->rps.cur_freq <= val)
> -		return;
> -
> -	/* Wake up the media well, as that takes a lot less
> -	 * power than the Render well. */
> -	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> -	valleyview_set_rps(dev_priv, val);
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
> -}
> -
>  void gen6_rps_busy(struct drm_i915_private *dev_priv)
>  {
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {

/* Ensure we start at the user's desired minimum frequency */
> +		intel_set_rps(dev_priv, dev_priv->rps.min_freq_softlimit);

Only if cur_freq < min_freq_softlimit

>  		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
>  			gen6_rps_reset_ei(dev_priv);
>  		I915_WRITE(GEN6_PMINTRMSK,
> @@ -4852,10 +4833,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  {
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {
> -		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -			vlv_set_rps_idle(dev_priv);
> -		else
> -			gen6_set_rps(dev_priv, dev_priv->rps.idle_freq);
> +		intel_set_rps(dev_priv, dev_priv->rps.idle_freq);
>  		dev_priv->rps.last_adj = 0;
>  		I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>  	}
> @@ -4905,8 +4883,11 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>  
>  void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  {
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);

And that is bogus. The comment that was removed doesn't actually reflect
what the code was doing! Lowlevel set_rps itself is doing forcewake gets,
using whatever domain is actually required to access the registers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre