Re: [PATCH] rpm

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote:
> 2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++------------
> >  drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
> >  drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
> >  4 files changed, 27 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5f35048..a72d8b8 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
> >         if (INTEL_INFO(dev)->gen < 6)
> >                 return 0;
> >
> > +       intel_runtime_pm_get(dev_priv);
> >         gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >
> >         return 0;
> > @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
> >                 return 0;
> >
> >         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +       intel_runtime_pm_put(dev_priv);
> >
> >         return 0;
> >  }
> 
> Just a minor comment on the chunk above. I didn't look at the rest of the patch.
> 
> We used to have exactly the code that you rewrote, but we decided to
> remove it because, without it, we can test that gen6_gt_force_wake_get
> actually wakes up the HW and keeps it awake until someone calls
> gen6_gt_force_wake_put.

But why? gen6_gt_force_wake_get() should never be called outside of a
rpm context - it is lowlevel register access.
 
> If we add these get/put calls above, we won't really detect any
> problems related with the actual gen6_gt_force_wake_get/put functions,
> so we may hide problems.

See above. That's how the design got broken in the first place and we
have very lowlevel code being copied and pasted throughout the driver.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre