Re: [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Saturday, August 09, 2014 10:53:03 AM Daniel Vetter wrote:
> On Sat, Aug 9, 2014 at 3:21 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, 9 Aug 2014, Rafael J. Wysocki wrote:
> >
> >> > > > Well it works currently. So where do you see the problem?
> >> > >
> >> > > Sampling registers from an timer - in particular, we really do not want
> >> > > to disable runtime pm whilst trying to monitor the impact of runtime pm.
> >> >
> >> > In that case you can grab a runtime pm reference iff the device is powered
> >> > on already. Which won't call anything scary, just amounts to an
> >> > atomic_add_unless or so, and then drop it again.
> >> >
> >> > Unfortunately there doesn't seem to be such a thing around already, so
> >> > need to add it first. Greg, how much would you freak out if we add
> >> > something like
> >> >
> >> > /**
> >> >  * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
> >> >  *
> >> >  * Returns true if an rpm ref has been acquire, false otherwise. Can be
> >> >  * called from atomic context to e.g. sample perfomance counters (where we
> >> >  * obviously don't want to disturb system state if everything is off atm).
> >> >  */
> >> > static inline bool pm_runtime_get_unless_suspended(struct device *dev)
> >> > {
> >> >     return atomic_add_unless(&dev->power.usage_count, 1, 0);
> >> > }
> >>
> >> I don't think it'll work universally.
> >>
> >> That'd need to be synchronized with other stuff done under the spinlock
> >> and in fact, what you're interested in is runtime_status (and that being
> >> RPM_ACTIVE) and not just the usage count.
> >
> > That's right.  You'd need to acquire the spinlock, test runtime_status,
> > do the register sampling if the status is RPM_ACTIVE, and then drop the
> > spinlock.
> >
> > I suppose wrapper routines for acquiring and releasing the spinlock
> > could be added to the runtime-PM API.  Something like this:
> >
> > #define pm_runtime_lock(dev, flags)                     \
> >                 spin_lock_irqsave(&(dev)->power.lock, flags)
> > #define pm_runtime_unlock(dev, flags)                   \
> >                 spin_unlock_irqrestore(&(dev)->power.lock, flags)
> >
> > It looks a little silly but it would work.
> 
> Oh right, I've totally ignored all the async resuming/suspending
> stuff. Anyway what we want to do is sample a perf monitoring unit on
> the gpu from an hrtimer and then expose that as a perf pmu. But we
> don't want to wake up the gpu for the sampling or hold a special
> reference, since that disturbs the sampling and also tends to upset
> the gpu.

The way to go is as Alan said: take the spinlock, check the runtime status,
do stuff and release the spinlock.

Rafael