[Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Dave Gordon]

On 10/12/15 22:14, 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 at linux.intel.c
>>>>>>> om>
>>>>>>> Reported-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>>>>> Cc: "Rafael J. Wysocki" <rjw at rjwysocki.net>
>>>>>>> Cc: linux-pm at 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.

How about pm_runtime_get_unless_idle(), which would be analogous to 
kref_get_unless_zero() ?

.Dave.

> 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) {
> 		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?
>
> Thanks,
> Rafael