All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Update rps interrupt limits
@ 2013-11-12 20:32 jeff.mcgee
  2013-11-12 20:32 ` jeff.mcgee
  0 siblings, 1 reply; 6+ messages in thread
From: jeff.mcgee @ 2013-11-12 20:32 UTC (permalink / raw)
  To: intel-gfx

From: Jeff McGee <jeff.mcgee@intel.com>

This bug was found on OTC Haswell Android product which makes use of
sysfs to set rps min and max range at boot.

This patch follows the original patch in implementing the fix with
a minimum of code change, which was desirable for late
stage product. A cleaner fix might involve refactoring the rps
interface a bit by perhaps making gen6_rps_limits a public function
that could be called from debugfs with the desired min/max. And the
register update could be placed in that function. Let me know if
something like that is more desirable for upstream and provide any
thoughts on form.

I also noticed that writing the interrupt limits register has been
removed for Valleyview recently by Chris Wilson, so I have removed
those parts of the original patch. I guess that means that it is
not relevant for Valleyview?

Jeff McGee (1):
  drm/i915: Update rps interrupt limits

 drivers/gpu/drm/i915/i915_sysfs.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_pm.c   | 10 +++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

-- 
1.8.4.2

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Update rps interrupt limits
  2013-11-12 20:32 [PATCH] drm/i915: Update rps interrupt limits jeff.mcgee
@ 2013-11-12 20:32 ` jeff.mcgee
  2013-11-12 21:04   ` [PATCH v2] " jeff.mcgee
  0 siblings, 1 reply; 6+ messages in thread
From: jeff.mcgee @ 2013-11-12 20:32 UTC (permalink / raw)
  To: intel-gfx

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.

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   | 10 +++++++++-
 2 files changed, 19 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..447c70d 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,13 @@ 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, limits);
+
 		return;
+	}
 
 	gen6_set_rps_thresholds(dev_priv, val);
 
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2] drm/i915: Update rps interrupt limits
  2013-11-12 20:32 ` jeff.mcgee
@ 2013-11-12 21:04   ` jeff.mcgee
  2013-11-13 19:09     ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: jeff.mcgee @ 2013-11-12 21:04 UTC (permalink / raw)
  To: intel-gfx

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.

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/i915: Update rps interrupt limits
  2013-11-12 21:04   ` [PATCH v2] " jeff.mcgee
@ 2013-11-13 19:09     ` Daniel Vetter
  2014-02-04 17:37       ` [PATCH v3] " jeff.mcgee
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-11-13 19:09 UTC (permalink / raw)
  To: jeff.mcgee; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v3] drm/i915: Update rps interrupt limits
  2013-11-13 19:09     ` Daniel Vetter
@ 2014-02-04 17:37       ` jeff.mcgee
  2014-02-07  9:26         ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: jeff.mcgee @ 2014-02-04 17:37 UTC (permalink / raw)
  To: intel-gfx

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.

v2: correct compile issue missed on rebase
v3: add igt testcases to signed-off-by section

Testcase: igt/pm_rps/min-max-config-idle
Testcase: igt/pm_rps/min-max-config-loaded
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 33bcae3..0c741f4 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -357,6 +357,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);
 
@@ -426,6 +431,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 9ab3883..2570789 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3003,6 +3003,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;
@@ -3011,8 +3014,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.5.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v3] drm/i915: Update rps interrupt limits
  2014-02-04 17:37       ` [PATCH v3] " jeff.mcgee
@ 2014-02-07  9:26         ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-02-07  9:26 UTC (permalink / raw)
  To: jeff.mcgee; +Cc: intel-gfx

On Tue, Feb 04, 2014 at 11:37:01AM -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.
> 
> v2: correct compile issue missed on rebase
> v3: add igt testcases to signed-off-by section
> 
> Testcase: igt/pm_rps/min-max-config-idle
> Testcase: igt/pm_rps/min-max-config-loaded
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-07  9:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-12 20:32 [PATCH] drm/i915: Update rps interrupt limits jeff.mcgee
2013-11-12 20:32 ` jeff.mcgee
2013-11-12 21:04   ` [PATCH v2] " jeff.mcgee
2013-11-13 19:09     ` Daniel Vetter
2014-02-04 17:37       ` [PATCH v3] " jeff.mcgee
2014-02-07  9:26         ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.