Re: [PATCH v2] drm/i915: Debugfs disable RPS boost and idle

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx