[Intel-gfx] [PATCH v2] drm/i915: Debugfs disable RPS boost and idle

[Chris Wilson]

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 at 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

-- 
Chris Wilson, Intel Open Source Technology Centre