Re: [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Imre Deak <imre.deak@intel.com>]

On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> > > wrote:
> > > > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen
> > > > > > wrote:
> > > > > > > Introduce pm_runtime_get_noidle to for situations where
> > > > > > > it is
> > > > > > > not
> > > > > > > desireable to touch an idling device. One use scenario is
> > > > > > > periodic
> > > > > > > hangchecks performed by the drm/i915 driver which can be
> > > > > > > omitted
> > > > > > > on a device in a runtime idle state.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > > > - Update documentation for bool return value
> > > > > > > 
> > > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.int
> > > > > > > el.c
> > > > > > > om>
> > > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > > > Cc: linux-pm@vger.kernel.org
> > > > > > 
> > > > > > Well, I don't quite see how this can be used in a non-racy
> > > > > > way
> > > > > > without doing an additional pm_runtime_resume() or
> > > > > > something
> > > > > > like
> > > > > > that in the same code path.
> > > > > 
> > > > > We don't want to resume, that would be the whole point. We'd
> > > > > like
> > > > > to
> > > > > ensure that we hold a reference _and_ the device is already
> > > > > active. So
> > > > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > > > addition
> > > > > after taking the reference.
> > > > 
> > > > Right, and that under the lock.
> > > 
> > > Which basically means you can call pm_runtime_resume() just fine,
> > > because it will do nothing if the status is RPM_ACTIVE already.
> > > 
> > > So really, why don't you use pm_runtime_get_sync()?
> > 
> > The difference would be that if the status is not RPM_ACTIVE
> > already we
> > would drop the reference and report error. The caller would in this
> > case forego of doing something, since we the device is suspended or
> > on
> > the way to being suspended. One example of such a scenario is a
> > watchdog like functionality: the watchdog work would
> > call pm_runtime_get_noidle() and check if the device is ok by doing
> > some HW access, but only if the device is powered. Otherwise the
> > work
> > item would do nothing (meaning it also won't reschedule itself).
> > The
> > watchdog work would get rescheduled next time the device is woken
> > up
> > and some work is submitted to the device.
> 
> So first of all the name "pm_runtime_get_noidle" doesn't make sense.
> 
> I guess what you need is something like
> 
> bool pm_runtime_get_if_active(struct device *dev)
> {
> 	unsigned log flags;
> 	bool ret;
> 
> 	spin_lock_irqsave(&dev->power.lock, flags);
> 
> 	if (dev->power.runtime_status == RPM_ACTIVE) {

But here usage_count could be zero, meaning that the device is already
on the way to be suspended (autosuspend or ASYNC suspend), no? In that
case we don't want to return success. That would unnecessarily prolong
the time the device is kept active.

> 		atomic_inc(&dev->power.usage_count);
> 		ret = true;
> 	} else {
> 		ret = false;
> 	}
> 
> 	spin_unlock_irqrestore(&dev->power.lock, flags);
> }
> 
> and the caller will simply bail out if "false" is returned, but if
> "true"
> is returned, it will have to drop the usage count, right?

Yes.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx