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@xxxxxxxxx wrote:
> > > > > > > > > From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > > > > > > > > 
> > > > > > > > > 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@xxxxxxxxx>
> > > > > > > > 
> > > > > > > > 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/