Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Lina Iyer <lina.iyer@linaro.org>]

On Fri, Oct 23 2015 at 17:46 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> On Fri, Oct 23 2015 at 15:19 -0600, Kevin Hilman wrote:
>>>Lina Iyer <lina.iyer@linaro.org> writes:
>>>
>>>> CPU devices that use runtime PM, have the followign characteristics -
>>>> 	- Runs in a IRQs disabled context
>>>> 	- Every CPU does its own runtime PM
>>>> 	- CPUs do not access other CPU's runtime PM
>>>> 	- The runtime PM state of the CPU is determined by the CPU
>>>>
>>>> These allow for some interesting optimizations -
>>>> 	- The CPUs have a limited runtime PM states
>>>> 	- The runtime state of CPU need not be protected by spinlocks
>>>> 	- Options like auto-suspend/async are not relevant to CPU
>>>> 	  devices
>>>>
>>>> A simplified runtime PM would therefore provide all that is needed for
>>>> the CPU devices.
>>>
>>>I like the idea of optimizing things for CPUs.  I've assumed we would
>>>eventually run into latency issues when using runtime PM and genpd on
>>>CPUs, but I guess we're already there.
>>>
>>>> After making a quick check for the usage count of the
>>>> CPU devices (to allow for the CPU to not power down the domain), the
>>>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>>>> is also avoided.
>>>
>>>This part is confusing (or more accurately, I am confused) more on that below...
>>>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>> ---
>>>>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/pm_runtime.h   |  3 ++-
>>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>>> index e1a10a0..5f7512c 100644
>>>> --- a/drivers/base/power/runtime.c
>>>> +++ b/drivers/base/power/runtime.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <linux/pm_wakeirq.h>
>>>>  #include <trace/events/rpm.h>
>>>>  #include "power.h"
>>>> +#include <linux/cpu.h>
>>>>
>>>>  typedef int (*pm_callback_t)(struct device *);
>>>>
>>>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>>>  	goto out;
>>>>  }
>>>>
>>>> +void cpu_pm_runtime_suspend(void)
>>>> +{
>>>> +	int ret;
>>>> +	int (*callback)(struct device *);
>>>> +	struct device *dev = get_cpu_device(smp_processor_id());
>>>> +
>>>> +	trace_rpm_suspend(dev, 0);
>>>> +
>>>> +	/**
>>>
>>>nit: the double '*' indicates kerneldoc, but this is just a multi-line
>>>comment.
>>>
>>>> +	 * Use device usage_count to disallow bubbling up suspend.
>>>
>>>I don't understand this comment.
>>>
>>>> +	 * This CPU has already decided to suspend, we cannot
>>>> +	 * prevent it here.
>>>> +	 */
>>>> +	if (!atomic_dec_and_test(&dev->power.usage_count))
>>>
>>>Isn't this basically a _put_noidle() ?
>>>
>>>> +		return 0;
>>>> +
>>>> +	ret = rpm_check_suspend_allowed(dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	__update_runtime_status(dev, RPM_SUSPENDING);
>>>> +
>>>> +	pm_runtime_cancel_pending(dev);
>>>> +	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>>>
>>>If the CPU device is part of a domain (e.g. cluster), then 'callback'
>>>here will be the domain callback, right?
>>>
>> Yes, thats correct.
>>
>>>If that's true, I'm not sure I'm following the changelog description
>>>that talks about avoiding the calling into the domain.
>>>
>> Partly correct. Avoid calling into the domain if its not the last
>> device.
>>
>>>It seems to me that you'll still call into the domain, but patch 2/2
>>>optimizes that path by only doing the *real* work of the domain for the
>>>last man standing.  Am I understanding that correctly?
>>>
>> Yes
>>
>>>Hmm, if that's the case though, where would the callbacks associated with the
>>>CPU (e.g. current CPU PM notifier stuff) get called?
>>>
>>
>> They are called from cpuidle driver as it is today.
>>
>
>And if the CPU _PM notifiers are eventually converted into runtime PM
>callbacks, they would then be called by the domain callbacks, but
>wouldn't that mean they would only be called after the last man
>standing?
>
These runtime PM functions are called from every CPU that is going idle,
and should therefore be able to execute callbacks for _PM notifications
for all CPUs from runtime PM.

The genpd callbacks are however only for the last man standing.

Thanks,
LIna