| Message ID | 1398446956-5245-1-git-send-email-daisy.sun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 25, 2014 at 7:29 PM, Daisy Sun <daisy.sun@intel.com> wrote: > RP frequency request is affected by 2 modules: normal turbo > algorithm and RPS boost algorithm. By adding RPS boost algorithm > to the mix, the final frequency becomes relatively unpredictable. > Add a switch to enable/disable RPS boost functionality. When > disabled, RP frequency will follow the normal turbo algorithm only This only really describes what the patch does, not why the rps boost is a problem and why exactly we need to disable it. If you want to set a fixed frequency then we have interfaces in sysfs for that, if the booster does something stupid then we need to fix that. -Daniel > > Issue: VIZ-3801 > Signed-off-by: Daisy Sun <daisy.sun@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 8 ++++++-- > 3 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 1e83ae4..2077bbd 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -3486,6 +3486,45 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops, > i915_drop_caches_get, i915_drop_caches_set, > "0x%08llx\n"); > > +static int i915_rps_disable_boost_get(void *data, u64 *val) > +{ > + struct drm_device *dev = data; > + drm_i915_private *dev_priv = dev->dev_private; > + > + if (INTEL_INFO(dev)->gen < 6) > + return -ENODEV; > + > + *val = dev_priv->rps.debugfs_disable_boost; > + > + return 0; > +} > + > +static int i915_rps_disable_boost_set(void *data, u64 val) > +{ > + struct drm_device *dev = data; > + drm_i915_private *dev_priv = dev->dev_private; > + int ret; > + > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > + DRM_DEBUG_DRIVER("Setting RPS disable Boost-Idle mode to %s\n", > + val ? "on" : "off"); > + > + ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); > + if (ret) > + return ret; > + > + dev_priv->rps.debugfs_disable_boost = val; > + > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + return 0; > +} > + > +DEFINE_SIMPLE_ATTRIBUTE(i915_rps_disable_boost_fops, > + i915_rps_disable_boost_get, i915_rps_disable_boost_set, > + "%llu\n"); > + > static int > i915_max_freq_get(void *data, u64 *val) > { > @@ -3821,6 +3860,7 @@ static const struct i915_debugfs_files { > {"i915_wedged", &i915_wedged_fops}, > {"i915_max_freq", &i915_max_freq_fops}, > {"i915_min_freq", &i915_min_freq_fops}, > + {"i915_rps_disable_boost", &i915_rps_disable_boost_fops}, > {"i915_cache_sharing", &i915_cache_sharing_fops}, > {"i915_ring_stop", &i915_ring_stop_fops}, > {"i915_ring_missed_irq", &i915_ring_missed_irq_fops}, > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 272aa7a..9c427da 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -847,6 +847,7 @@ struct intel_gen6_power_mgmt { > int last_adj; > enum { LOW_POWER, BETWEEN, HIGH_POWER } power; > > + bool debugfs_disable_boost; > bool enabled; > struct delayed_work delayed_resume_work; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 75c1c76..6acac14 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3163,7 +3163,9 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv) > struct drm_device *dev = dev_priv->dev; > > mutex_lock(&dev_priv->rps.hw_lock); > - if (dev_priv->rps.enabled) { > + > + if (dev_priv->rps.enabled > + && !dev_priv->rps.debugfs_disable_boost) { > if (IS_VALLEYVIEW(dev)) > vlv_set_rps_idle(dev_priv); > else > @@ -3178,7 +3180,9 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv) > struct drm_device *dev = dev_priv->dev; > > mutex_lock(&dev_priv->rps.hw_lock); > - if (dev_priv->rps.enabled) { > + > + if (dev_priv->rps.enabled > + && !dev_priv->rps.debugfs_disable_boost) { > if (IS_VALLEYVIEW(dev)) > valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); > else > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thanks for the timely reply, Daniel. The intention to bring debugfs for RPS boost and idle is when boost and idle are disabled, we are able to have a clear vision of what normal turbo algorithm. It‘s very helpful to verify if the turbo algorithm is working as expected, at least from the VPG validation team. Without the debugfs hooks, the RPS boost or idle may kicks in at anytime and any circumstances, which makes it complicated. It does not mean RPS boost and idle is doing anything wrong, that's a different story. A fixed frequency setting is useful in some other cases but not enough to verify turbo algorithm. I'd like to add the purpose in commit msg if it's not clear. On 4/25/2014 10:41 AM, Daniel Vetter wrote: > On Fri, Apr 25, 2014 at 7:29 PM, Daisy Sun <daisy.sun@intel.com> wrote: >> RP frequency request is affected by 2 modules: normal turbo >> algorithm and RPS boost algorithm. By adding RPS boost algorithm >> to the mix, the final frequency becomes relatively unpredictable. >> Add a switch to enable/disable RPS boost functionality. When >> disabled, RP frequency will follow the normal turbo algorithm only > This only really describes what the patch does, not why the rps boost > is a problem and why exactly we need to disable it. If you want to set > a fixed frequency then we have interfaces in sysfs for that, if the > booster does something stupid then we need to fix that. > -Daniel > >> Issue: VIZ-3801 >> Signed-off-by: Daisy Sun <daisy.sun@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_pm.c | 8 ++++++-- >> 3 files changed, 47 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 1e83ae4..2077bbd 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -3486,6 +3486,45 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops, >> i915_drop_caches_get, i915_drop_caches_set, >> "0x%08llx\n"); >> >> +static int i915_rps_disable_boost_get(void *data, u64 *val) >> +{ >> + struct drm_device *dev = data; >> + drm_i915_private *dev_priv = dev->dev_private; >> + >> + if (INTEL_INFO(dev)->gen < 6) >> + return -ENODEV; >> + >> + *val = dev_priv->rps.debugfs_disable_boost; >> + >> + return 0; >> +} >> + >> +static int i915_rps_disable_boost_set(void *data, u64 val) >> +{ >> + struct drm_device *dev = data; >> + drm_i915_private *dev_priv = dev->dev_private; >> + int ret; >> + >> + flush_delayed_work(&dev_priv->rps.delayed_resume_work); >> + >> + DRM_DEBUG_DRIVER("Setting RPS disable Boost-Idle mode to %s\n", >> + val ? "on" : "off"); >> + >> + ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); >> + if (ret) >> + return ret; >> + >> + dev_priv->rps.debugfs_disable_boost = val; >> + >> + mutex_unlock(&dev_priv->rps.hw_lock); >> + >> + return 0; >> +} >> + >> +DEFINE_SIMPLE_ATTRIBUTE(i915_rps_disable_boost_fops, >> + i915_rps_disable_boost_get, i915_rps_disable_boost_set, >> + "%llu\n"); >> + >> static int >> i915_max_freq_get(void *data, u64 *val) >> { >> @@ -3821,6 +3860,7 @@ static const struct i915_debugfs_files { >> {"i915_wedged", &i915_wedged_fops}, >> {"i915_max_freq", &i915_max_freq_fops}, >> {"i915_min_freq", &i915_min_freq_fops}, >> + {"i915_rps_disable_boost", &i915_rps_disable_boost_fops}, >> {"i915_cache_sharing", &i915_cache_sharing_fops}, >> {"i915_ring_stop", &i915_ring_stop_fops}, >> {"i915_ring_missed_irq", &i915_ring_missed_irq_fops}, >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 272aa7a..9c427da 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -847,6 +847,7 @@ struct intel_gen6_power_mgmt { >> int last_adj; >> enum { LOW_POWER, BETWEEN, HIGH_POWER } power; >> >> + bool debugfs_disable_boost; >> bool enabled; >> struct delayed_work delayed_resume_work; >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 75c1c76..6acac14 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3163,7 +3163,9 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv) >> struct drm_device *dev = dev_priv->dev; >> >> mutex_lock(&dev_priv->rps.hw_lock); >> - if (dev_priv->rps.enabled) { >> + >> + if (dev_priv->rps.enabled >> + && !dev_priv->rps.debugfs_disable_boost) { >> if (IS_VALLEYVIEW(dev)) >> vlv_set_rps_idle(dev_priv); >> else >> @@ -3178,7 +3180,9 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv) >> struct drm_device *dev = dev_priv->dev; >> >> mutex_lock(&dev_priv->rps.hw_lock); >> - if (dev_priv->rps.enabled) { >> + >> + if (dev_priv->rps.enabled >> + && !dev_priv->rps.debugfs_disable_boost) { >> if (IS_VALLEYVIEW(dev)) >> valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); >> else >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Fri, Apr 25, 2014 at 11:16 PM, Sun, Daisy <daisy.sun@intel.com> wrote: > Thanks for the timely reply, Daniel. > > The intention to bring debugfs for RPS boost and idle is when boost and idle > are disabled, we are able to have a clear vision of what normal turbo > algorithm. > It‘s very helpful to verify if the turbo algorithm is working as expected, > at least from the VPG validation team. > > Without the debugfs hooks, the RPS boost or idle may kicks in at anytime and > any circumstances, which makes it complicated. > It does not mean RPS boost and idle is doing anything wrong, that's a > different story. A fixed frequency setting is useful in some other cases but > not enough to verify turbo algorithm. > > I'd like to add the purpose in commit msg if it's not clear. Yeah if this is useful for the hw validation team then a debugfs interface makes sense. But the patch must mention this motivation since if you try to use this in production to work around issues with the turbo booster then that's not good ;-) Please update the commit message with these important details and resubmit the patch. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1e83ae4..2077bbd 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3486,6 +3486,45 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops, i915_drop_caches_get, i915_drop_caches_set, "0x%08llx\n"); +static int i915_rps_disable_boost_get(void *data, u64 *val) +{ + struct drm_device *dev = data; + drm_i915_private *dev_priv = dev->dev_private; + + if (INTEL_INFO(dev)->gen < 6) + return -ENODEV; + + *val = dev_priv->rps.debugfs_disable_boost; + + return 0; +} + +static int i915_rps_disable_boost_set(void *data, u64 val) +{ + struct drm_device *dev = data; + drm_i915_private *dev_priv = dev->dev_private; + int ret; + + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + + DRM_DEBUG_DRIVER("Setting RPS disable Boost-Idle mode to %s\n", + val ? "on" : "off"); + + ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); + if (ret) + return ret; + + dev_priv->rps.debugfs_disable_boost = val; + + mutex_unlock(&dev_priv->rps.hw_lock); + + return 0; +} + +DEFINE_SIMPLE_ATTRIBUTE(i915_rps_disable_boost_fops, + i915_rps_disable_boost_get, i915_rps_disable_boost_set, + "%llu\n"); + static int i915_max_freq_get(void *data, u64 *val) { @@ -3821,6 +3860,7 @@ static const struct i915_debugfs_files { {"i915_wedged", &i915_wedged_fops}, {"i915_max_freq", &i915_max_freq_fops}, {"i915_min_freq", &i915_min_freq_fops}, + {"i915_rps_disable_boost", &i915_rps_disable_boost_fops}, {"i915_cache_sharing", &i915_cache_sharing_fops}, {"i915_ring_stop", &i915_ring_stop_fops}, {"i915_ring_missed_irq", &i915_ring_missed_irq_fops}, diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 272aa7a..9c427da 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -847,6 +847,7 @@ struct intel_gen6_power_mgmt { int last_adj; enum { LOW_POWER, BETWEEN, HIGH_POWER } power; + bool debugfs_disable_boost; bool enabled; struct delayed_work delayed_resume_work; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 75c1c76..6acac14 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3163,7 +3163,9 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; mutex_lock(&dev_priv->rps.hw_lock); - if (dev_priv->rps.enabled) { + + if (dev_priv->rps.enabled + && !dev_priv->rps.debugfs_disable_boost) { if (IS_VALLEYVIEW(dev)) vlv_set_rps_idle(dev_priv); else @@ -3178,7 +3180,9 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; mutex_lock(&dev_priv->rps.hw_lock); - if (dev_priv->rps.enabled) { + + if (dev_priv->rps.enabled + && !dev_priv->rps.debugfs_disable_boost) { if (IS_VALLEYVIEW(dev)) valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); else
RP frequency request is affected by 2 modules: normal turbo algorithm and RPS boost algorithm. By adding RPS boost algorithm to the mix, the final frequency becomes relatively unpredictable. Add a switch to enable/disable RPS boost functionality. When disabled, RP frequency will follow the normal turbo algorithm only Issue: VIZ-3801 Signed-off-by: Daisy Sun <daisy.sun@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 8 ++++++-- 3 files changed, 47 insertions(+), 2 deletions(-)