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@xxxxxxxxx> > > > --- > > > 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. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx