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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
--
/) David Weinehall <tao@xxxxxxxxxx> /) 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
