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

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

[Daniel Vetter]

On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.mateo@intel.com wrote:
> > > > > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > 
> > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > with the create/destroy request, and handle the forcewake get/put
> > > > > > > directly.
> > > > > > > 
> > > > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > 
> > > > > > Looks like a fixup that should be squashed into relevant earlier patches.
> > > > > 
> > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > broken due to this - we must be able to read registers in atomic
> > > > > context!
> > > > > 
> > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > 
> > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if
> > > > you want to read registers from atomic context you have to have a runtime
> > > > pm reference from someone else.
> > > 
> > > Nope. That cannot work.
> > 
> > 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);
}

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Greg KH]

On Fri, Aug 08, 2014 at 11:37:01AM +0200, Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> > On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.mateo@intel.com wrote:
> > > > > > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > > 
> > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > > with the create/destroy request, and handle the forcewake get/put
> > > > > > > > directly.
> > > > > > > > 
> > > > > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > 
> > > > > > > Looks like a fixup that should be squashed into relevant earlier patches.
> > > > > > 
> > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > > broken due to this - we must be able to read registers in atomic
> > > > > > context!
> > > > > > 
> > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > > 
> > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if
> > > > > you want to read registers from atomic context you have to have a runtime
> > > > > pm reference from someone else.
> > > > 
> > > > Nope. That cannot work.
> > > 
> > > 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'd freak out a lot :)

Rafael, isn't there some other better way to resolve this issue without
resorting to something as "horrid" as the above?

thanks,

greg k-h

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

[Rafael J. Wysocki]

On Friday, August 08, 2014 11:37:01 AM Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> > On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.mateo@intel.com wrote:
> > > > > > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > > 
> > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > > with the create/destroy request, and handle the forcewake get/put
> > > > > > > > directly.
> > > > > > > > 
> > > > > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > 
> > > > > > > Looks like a fixup that should be squashed into relevant earlier patches.
> > > > > > 
> > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > > broken due to this - we must be able to read registers in atomic
> > > > > > context!
> > > > > > 
> > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > > 
> > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if
> > > > > you want to read registers from atomic context you have to have a runtime
> > > > > pm reference from someone else.
> > > > 
> > > > Nope. That cannot work.
> > > 
> > > 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.

Rafael

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

[Rafael J. Wysocki]

On Friday, August 08, 2014 06:41:10 AM Greg KH wrote:
> On Fri, Aug 08, 2014 at 11:37:01AM +0200, Daniel Vetter wrote:
> > On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> > > On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.mateo@intel.com wrote:
> > > > > > > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > > > 
> > > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > > > with the create/destroy request, and handle the forcewake get/put
> > > > > > > > > directly.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > > > 
> > > > > > > > Looks like a fixup that should be squashed into relevant earlier patches.
> > > > > > > 
> > > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > > > broken due to this - we must be able to read registers in atomic
> > > > > > > context!
> > > > > > > 
> > > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > > > 
> > > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if
> > > > > > you want to read registers from atomic context you have to have a runtime
> > > > > > pm reference from someone else.
> > > > > 
> > > > > Nope. That cannot work.
> > > > 
> > > > 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'd freak out a lot :)
> 
> Rafael, isn't there some other better way to resolve this issue without
> resorting to something as "horrid" as the above?

I'm not sure how to solve this at all, because the above isn't going to work
in general in my opinion.

Can anyone please try to explain the problem to me without referring to the
i915 internals?

Rafael

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

[Alan Stern]

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.

Alan Stern

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

[Daniel Vetter]

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.

Note that those registers are just status indicator registers, so no
counters that will get reset when the device is suspended. For the
later counters (we have them too) we're not sure yet how to handle
them, especially since the amount the gpu saves/restores into its own
context storage depends upon the platform.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Rafael J. Wysocki]

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