| Message ID | 1398718432-23364-1-git-send-email-daisy.sun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 28, 2014 at 01:53:52PM -0700, Daisy Sun 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. > > Intention: when boost and idle are disabled, we have a clear vision > of turbo algorithm. It‘s very helpful to verify if the turbo > algorithm is working as expected. > Without debugfs hooks, the RPS boost or idle may kicks in at > anytime and any circumstances. > > V1->V2: Follow Daniel's comment to explain the intention. > > 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..ff71214 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; > + struct 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; > + struct drm_i915_private *dev_priv = dev->dev_private; > + int ret; > + > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); I'm not really sure why you feel it's necessary to flush the wq here. Note that you have no real safety since you cannot acquire the lock, and another event can get queued up after the flush. In other words, whatever you're trying to do probably can fail. Also note that without this, a simple atomic_t would suffice for debugfs_disable_boost. > + > + 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 Mon, Apr 28, 2014 at 07:20:17PM -0700, Ben Widawsky wrote: > On Mon, Apr 28, 2014 at 01:53:52PM -0700, Daisy Sun 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. > > > > Intention: when boost and idle are disabled, we have a clear vision > > of turbo algorithm. It‘s very helpful to verify if the turbo > > algorithm is working as expected. > > Without debugfs hooks, the RPS boost or idle may kicks in at > > anytime and any circumstances. > > > > V1->V2: Follow Daniel's comment to explain the intention. > > > > 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..ff71214 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; > > + struct 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; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + int ret; > > + > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > > I'm not really sure why you feel it's necessary to flush the wq here. > Note that you have no real safety since you cannot acquire the lock, and > another event can get queued up after the flush. In other words, > whatever you're trying to do probably can fail. > > Also note that without this, a simple atomic_t would suffice for > debugfs_disable_boost. Simple? That puts another locked instruction on the common side as opposed to taking the mutex inside debugfs. Personally I'd complain much more about the lack of CODING_STYLE and good naming. -Chris
On Tue, Apr 29, 2014 at 07:48:41AM +0100, Chris Wilson wrote: > On Mon, Apr 28, 2014 at 07:20:17PM -0700, Ben Widawsky wrote: > > On Mon, Apr 28, 2014 at 01:53:52PM -0700, Daisy Sun 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. > > > > > > Intention: when boost and idle are disabled, we have a clear vision > > > of turbo algorithm. It‘s very helpful to verify if the turbo > > > algorithm is working as expected. > > > Without debugfs hooks, the RPS boost or idle may kicks in at > > > anytime and any circumstances. > > > > > > V1->V2: Follow Daniel's comment to explain the intention. > > > > > > 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..ff71214 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; > > > + struct 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; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + int ret; > > > + > > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > > > > I'm not really sure why you feel it's necessary to flush the wq here. > > Note that you have no real safety since you cannot acquire the lock, and > > another event can get queued up after the flush. In other words, > > whatever you're trying to do probably can fail. > > > > Also note that without this, a simple atomic_t would suffice for > > debugfs_disable_boost. > > Simple? That puts another locked instruction on the common side as > opposed to taking the mutex inside debugfs. Personally I'd complain much > more about the lack of CODING_STYLE and good naming. Also it's just a single bool and it doesn't matter if the value changes while someone is holding struct_mutex, so no atomic_t or struct_mutex necessary here.
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1e83ae4..ff71214 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; + struct 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; + struct 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. Intention: when boost and idle are disabled, we have a clear vision of turbo algorithm. It‘s very helpful to verify if the turbo algorithm is working as expected. Without debugfs hooks, the RPS boost or idle may kicks in at anytime and any circumstances. V1->V2: Follow Daniel's comment to explain the intention. 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(-)