* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox