Re: drm/i915: Remove RPM suspend dependency on rps.enabled and related changes

[David Weinehall <tao@acc.umu.se>]

On Sat, Aug 20, 2016 at 10:39:00AM +0530, Sagar Arun Kamble wrote:
> For Gen9, RPM suspend is failing if rps.enabled=false. This is needed for
> other platforms as RC6 and RPS enabling is indicated by rps.enabled.
> RPM Suspend depends only on RC6, so we need to remove the check of rps.enabled.
> For Gen9 RC6 and RPS enabling is separated hence do rps.enabled check only
> for non-Gen9 platforms. Once RC6 and RPS enabling is separated for other GENs
> this check can be completely removed.
> Moved setting of rps.enabled to platform level functions as there is case of
> disabling of RPS in gen9_enable_rps.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++--
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 13ae340..bc2c67b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2284,9 +2284,17 @@ static int intel_runtime_suspend(struct device *device)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	int ret;
>  
> -	if (WARN_ON_ONCE(!(dev_priv->rps.enabled && intel_enable_rc6())))
> +	if (WARN_ON_ONCE(!intel_enable_rc6()))
>  		return -ENODEV;
>  
> +	/*
> +	 * Once RC6 and RPS enabling is separated for non-GEN9 platforms
> +	 * below check should be removed.
> +	*/
> +	if (!IS_GEN9(dev))

IS_GEN9(dev_priv)

> +		if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> +			return -ENODEV;
> +
>  	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev)))

And while at it you could change this one too. Eventually
all feature macros will be modified to take only dev_priv,
and having things slowly migrate while other things are
changed will make that transition easier.

>  		return -ENODEV;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99014d7..954e332 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4966,6 +4966,7 @@ static void gen9_disable_rc6(struct drm_i915_private *dev_priv)
>  static void gen9_disable_rps(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE(GEN6_RP_CONTROL, 0);
> +	dev_priv->rps.enabled = false;
>  }

Inconsistent coding style -- all other functions like this have a
newline before that line you added.
 
>  static void gen6_disable_rps(struct drm_i915_private *dev_priv)
> @@ -4973,11 +4974,16 @@ static void gen6_disable_rps(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
>  	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>  	I915_WRITE(GEN6_RP_CONTROL, 0);
> +
> +	dev_priv->rps.enabled = false;
> +
>  }

Don't add an extra newline after that statement, please.

>  static void cherryview_disable_rps(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
> +
> +	dev_priv->rps.enabled = false;
>  }
>  
>  static void valleyview_disable_rps(struct drm_i915_private *dev_priv)
> @@ -4989,6 +4995,8 @@ static void valleyview_disable_rps(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	dev_priv->rps.enabled = false;
>  }
>  
>  static void intel_print_rc6_info(struct drm_i915_private *dev_priv, u32 mode)
> @@ -5206,6 +5214,8 @@ static void gen9_enable_rps(struct drm_i915_private *dev_priv)
>  	reset_rps(dev_priv, gen6_set_rps);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	dev_priv->rps.enabled = true;
>  }
>  
>  static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
> @@ -5349,6 +5359,8 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
>  	reset_rps(dev_priv, gen6_set_rps);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	dev_priv->rps.enabled = true;
>  }
>  
>  static void gen6_enable_rps(struct drm_i915_private *dev_priv)
> @@ -5445,6 +5457,8 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  	}
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	dev_priv->rps.enabled = true;
>  }
>  
>  static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
> @@ -5919,6 +5933,8 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
>  	reset_rps(dev_priv, valleyview_set_rps);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	dev_priv->rps.enabled = true;
>  }
>  
>  static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
> @@ -5999,6 +6015,8 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
>  	reset_rps(dev_priv, valleyview_set_rps);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	dev_priv->rps.enabled = true;
>  }
>  
>  static unsigned long intel_pxfreq(u32 vidfreq)
> @@ -6588,7 +6606,6 @@ void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
>  		ironlake_disable_drps(dev_priv);
>  	}
>  
> -	dev_priv->rps.enabled = false;
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -6632,7 +6649,6 @@ void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
>  	WARN_ON(dev_priv->rps.efficient_freq < dev_priv->rps.min_freq);
>  	WARN_ON(dev_priv->rps.efficient_freq > dev_priv->rps.max_freq);
>  
> -	dev_priv->rps.enabled = true;
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }

Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>

-- 
 /) David Weinehall <tao@acc.umu.se> /) Northern lights wander      (\
//  Maintainer of the v2.0 kernel   //  Dance across the winter sky //
\)  http://www.acc.umu.se/~tao/    (/   Full colour fire           (/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx