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.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
