| Message ID | 1444168656-6576-2-git-send-email-lina.iyer@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote: > 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. 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. It's an interesting idea. :-) While I need to give it some more thinking for how/if this could fit into the runtime PM API, let me start by providing some initial feedback on the patch as such. > > 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) I think you want to return int instead of void. > +{ > + int ret; > + int (*callback)(struct device *); > + struct device *dev = get_cpu_device(smp_processor_id()); Perhaps we should follow the other runtime PM APIs and have the struct *device provided as an in-parameter!? > + > + trace_rpm_suspend(dev, 0); > + > + /** > + * Use device usage_count to disallow bubbling up suspend. > + * This CPU has already decided to suspend, we cannot > + * prevent it here. > + */ > + if (!atomic_dec_and_test(&dev->power.usage_count)) > + return 0; > + > + ret = rpm_check_suspend_allowed(dev); I don't think you can use this function. For example it calls __dev_pm_qos_read_value() which expects the dev->power.lock to be held. > + if (ret) > + return ret; > + > + __update_runtime_status(dev, RPM_SUSPENDING); > + > + pm_runtime_cancel_pending(dev); Hmm. For the same struct device (CPU) could really calls to cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to protect against that? I don't have such in-depth knowledge about CPU idle, so apologize if this may be a stupid question. If the answer to the above is *no*, I believe you don't need to care about the intermediate RPM_SUSPENDING state and you don't need an atomic counter either, right!? Instead you could then just update the runtime PM status to RPM_SUSPENDED if the RPM callback doesn't return an error. > + callback = RPM_GET_CALLBACK(dev, runtime_suspend); > + > + ret = callback(dev); > + if (!ret) > + __update_runtime_status(dev, RPM_SUSPENDED); > + else > + __update_runtime_status(dev, RPM_ACTIVE); > + > + trace_rpm_return_int(dev, _THIS_IP_, ret); > +} > + > +void cpu_pm_runtime_resume(void) Similar comments as for the suspend function. > +{ > + int ret; > + int (*callback)(struct device *); > + struct device *dev = get_cpu_device(smp_processor_id()); > + > + trace_rpm_resume(dev, 0); > + > + if (dev->power.runtime_status == RPM_ACTIVE) > + return 1; > + > + atomic_inc(&dev->power.usage_count); > + > + __update_runtime_status(dev, RPM_RESUMING); > + > + callback = RPM_GET_CALLBACK(dev, runtime_resume); > + > + ret = callback(dev); > + if (!ret) > + __update_runtime_status(dev, RPM_ACTIVE); > + else > + __update_runtime_status(dev, RPM_SUSPENDED); > + > + trace_rpm_return_int(dev, _THIS_IP_, ret); > +} > + > /** > * rpm_resume - Carry out runtime resume of given device. > * @dev: Device to resume. > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 3bdbb41..3655ead 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -31,6 +31,8 @@ static inline bool queue_pm_work(struct work_struct *work) > return queue_work(pm_wq, work); > } > > +extern void cpu_pm_runtime_suspend(void); > +extern void cpu_pm_runtime_resume(void); extern int ... > extern int pm_generic_runtime_suspend(struct device *dev); > extern int pm_generic_runtime_resume(struct device *dev); > extern int pm_runtime_force_suspend(struct device *dev); > @@ -273,5 +275,4 @@ static inline void pm_runtime_dont_use_autosuspend(struct device *dev) > { > __pm_runtime_use_autosuspend(dev, false); > } > - > #endif > -- > 2.1.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19 2015 at 03:44 -0600, Ulf Hansson wrote: >On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote: >> 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. 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. > >It's an interesting idea. :-) > >While I need to give it some more thinking for how/if this could fit >into the runtime PM API, let me start by providing some initial >feedback on the patch as such. > >> >> 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) > >I think you want to return int instead of void. > The outcome of this function would not change the runtime state of the CPU. The void return seems appropriate. >> +{ >> + int ret; >> + int (*callback)(struct device *); >> + struct device *dev = get_cpu_device(smp_processor_id()); > >Perhaps we should follow the other runtime PM APIs and have the struct >*device provided as an in-parameter!? > But that information is can be deduced by this function - the function is called for that CPU from *that* CPU. Also, the absence of an argument, ensures that the caller won't make a mistake of calling any other CPUs runtime PM from a CPU or worse, pass a device that is not a CPU. >> + + trace_rpm_suspend(dev, 0); >> + >> + /** >> + * Use device usage_count to disallow bubbling up suspend. >> + * This CPU has already decided to suspend, we cannot >> + * prevent it here. >> + */ >> + if (!atomic_dec_and_test(&dev->power.usage_count)) >> + return 0; >> + >> + ret = rpm_check_suspend_allowed(dev); > >I don't think you can use this function. For example it calls >__dev_pm_qos_read_value() which expects the dev->power.lock to be >held. > Right. I realized that. Will fix. >> + if (ret) >> + return ret; >> + >> + __update_runtime_status(dev, RPM_SUSPENDING); >> + >> + pm_runtime_cancel_pending(dev); > >Hmm. For the same struct device (CPU) could really calls to >cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to >protect against that? > That wouldnt happen, the functions are only called that CPU on that CPU. See the explanation above. >I don't have such in-depth knowledge about CPU idle, so apologize if >this may be a stupid question. > >If the answer to the above is *no*, I believe you don't need to care >about the intermediate RPM_SUSPENDING state and you don't need an >atomic counter either, right!? > This calls into genpd framework, which expects devices to be RPM_SUSPENDING in pm_genpd_power_off; I wanted to keep the behavior between the frameworks consistent. >Instead you could then just update the runtime PM status to >RPM_SUSPENDED if the RPM callback doesn't return an error. > >> + callback = RPM_GET_CALLBACK(dev, runtime_suspend); >> + >> + ret = callback(dev); >> + if (!ret) >> + __update_runtime_status(dev, RPM_SUSPENDED); >> + else >> + __update_runtime_status(dev, RPM_ACTIVE); >> + >> + trace_rpm_return_int(dev, _THIS_IP_, ret); >> +} >> + >> +void cpu_pm_runtime_resume(void) > >Similar comments as for the suspend function. > >> +{ >> + int ret; >> + int (*callback)(struct device *); >> + struct device *dev = get_cpu_device(smp_processor_id()); >> + >> + trace_rpm_resume(dev, 0); >> + >> + if (dev->power.runtime_status == RPM_ACTIVE) >> + return 1; >> + >> + atomic_inc(&dev->power.usage_count); >> + >> + __update_runtime_status(dev, RPM_RESUMING); >> + >> + callback = RPM_GET_CALLBACK(dev, runtime_resume); >> + >> + ret = callback(dev); >> + if (!ret) >> + __update_runtime_status(dev, RPM_ACTIVE); >> + else >> + __update_runtime_status(dev, RPM_SUSPENDED); >> + >> + trace_rpm_return_int(dev, _THIS_IP_, ret); >> +} >> + >> /** >> * rpm_resume - Carry out runtime resume of given device. >> * @dev: Device to resume. >> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h >> index 3bdbb41..3655ead 100644 >> --- a/include/linux/pm_runtime.h >> +++ b/include/linux/pm_runtime.h >> @@ -31,6 +31,8 @@ static inline bool queue_pm_work(struct work_struct *work) >> return queue_work(pm_wq, work); >> } >> >> +extern void cpu_pm_runtime_suspend(void); >> +extern void cpu_pm_runtime_resume(void); > >extern int ... > >> extern int pm_generic_runtime_suspend(struct device *dev); >> extern int pm_generic_runtime_resume(struct device *dev); >> extern int pm_runtime_force_suspend(struct device *dev); >> @@ -273,5 +275,4 @@ static inline void pm_runtime_dont_use_autosuspend(struct device *dev) >> { >> __pm_runtime_use_autosuspend(dev, false); >> } >> - >> #endif >> -- >> 2.1.4 >> > >Kind regards >Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? If that's true, I'm not sure I'm following the changelog description that talks about avoiding the calling into the domain. 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? Hmm, if that's the case though, where would the callbacks associated with the CPU (e.g. current CPU PM notifier stuff) get called? > + ret = callback(dev); > + if (!ret) > + __update_runtime_status(dev, RPM_SUSPENDED); > + else > + __update_runtime_status(dev, RPM_ACTIVE); > + > + trace_rpm_return_int(dev, _THIS_IP_, ret); > +} Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Thanks, Lina >> + ret = callback(dev); >> + if (!ret) >> + __update_runtime_status(dev, RPM_SUSPENDED); >> + else >> + __update_runtime_status(dev, RPM_ACTIVE); >> + >> + trace_rpm_return_int(dev, _THIS_IP_, ret); >> +} > >Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21 October 2015 at 03:59, Lina Iyer <lina.iyer@linaro.org> wrote: > On Mon, Oct 19 2015 at 03:44 -0600, Ulf Hansson wrote: >> >> On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote: >>> >>> 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. 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. >> >> >> It's an interesting idea. :-) >> >> While I need to give it some more thinking for how/if this could fit >> into the runtime PM API, let me start by providing some initial >> feedback on the patch as such. >> >>> >>> 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) >> >> >> I think you want to return int instead of void. >> > The outcome of this function would not change the runtime state of the > CPU. The void return seems appropriate. If the runtime PM suspend callbacks returns and error code, will that prevent the CPU from going idle? In other words do you manage idling of the CPU via runtime PM callbacks for the CPU idle driver? If not, don't you need to check a return value from this API to know whether it's okay to proceed idling the CPU? > >>> +{ >>> + int ret; >>> + int (*callback)(struct device *); >>> + struct device *dev = get_cpu_device(smp_processor_id()); >> >> >> Perhaps we should follow the other runtime PM APIs and have the struct >> *device provided as an in-parameter!? >> > But that information is can be deduced by this function - the function > is called for that CPU from *that* CPU. Also, the absence of an > argument, ensures that the caller won't make a mistake of calling any > other CPUs runtime PM from a CPU or worse, pass a device that is not a > CPU. Okay! As long as we decide to use the API *only* for CPUs that makes sense. Although, I was thinking that we perhaps shouldn't limit the use of the API to CPUs, but I don't know of any similar devices as of now. > >>> + + trace_rpm_suspend(dev, 0); >>> + >>> + /** >>> + * Use device usage_count to disallow bubbling up suspend. >>> + * This CPU has already decided to suspend, we cannot >>> + * prevent it here. >>> + */ >>> + if (!atomic_dec_and_test(&dev->power.usage_count)) >>> + return 0; >>> + >>> + ret = rpm_check_suspend_allowed(dev); >> >> >> I don't think you can use this function. For example it calls >> __dev_pm_qos_read_value() which expects the dev->power.lock to be >> held. >> > Right. I realized that. Will fix. > >>> + if (ret) >>> + return ret; >>> + >>> + __update_runtime_status(dev, RPM_SUSPENDING); >>> + >>> + pm_runtime_cancel_pending(dev); >> >> >> Hmm. For the same struct device (CPU) could really calls to >> cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to >> protect against that? >> > That wouldnt happen, the functions are only called that CPU on that CPU. > See the explanation above. > >> I don't have such in-depth knowledge about CPU idle, so apologize if >> this may be a stupid question. >> >> If the answer to the above is *no*, I believe you don't need to care >> about the intermediate RPM_SUSPENDING state and you don't need an >> atomic counter either, right!? >> > This calls into genpd framework, which expects devices to be > RPM_SUSPENDING in pm_genpd_power_off; I wanted to keep the behavior > between the frameworks consistent. Okay, it makes sense. Thanks for clarifying. > > >> Instead you could then just update the runtime PM status to >> RPM_SUSPENDED if the RPM callback doesn't return an error. >> >>> + callback = RPM_GET_CALLBACK(dev, runtime_suspend); >>> + >>> + ret = callback(dev); >>> + if (!ret) >>> + __update_runtime_status(dev, RPM_SUSPENDED); >>> + else >>> + __update_runtime_status(dev, RPM_ACTIVE); >>> + >>> + trace_rpm_return_int(dev, _THIS_IP_, ret); >>> +} >>> + >>> +void cpu_pm_runtime_resume(void) >> [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 28 2015 at 04:43 -0600, Ulf Hansson wrote: >On 21 October 2015 at 03:59, Lina Iyer <lina.iyer@linaro.org> wrote: >> On Mon, Oct 19 2015 at 03:44 -0600, Ulf Hansson wrote: >>> >>> On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote: >>>> >>>> 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. 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. >>> >>> >>> It's an interesting idea. :-) >>> >>> While I need to give it some more thinking for how/if this could fit >>> into the runtime PM API, let me start by providing some initial >>> feedback on the patch as such. >>> >>>> >>>> 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) >>> >>> >>> I think you want to return int instead of void. >>> >> The outcome of this function would not change the runtime state of the >> CPU. The void return seems appropriate. > >If the runtime PM suspend callbacks returns and error code, will that >prevent the CPU from going idle? > It should not. I dont think runtime PM should fail, because the CPU determines its own state. >In other words do you manage idling of the CPU via runtime PM >callbacks for the CPU idle driver? > No. >If not, don't you need to check a return value from this API to know >whether it's okay to proceed idling the CPU? > >> >>>> +{ >>>> + int ret; >>>> + int (*callback)(struct device *); >>>> + struct device *dev = get_cpu_device(smp_processor_id()); >>> >>> >>> Perhaps we should follow the other runtime PM APIs and have the struct >>> *device provided as an in-parameter!? >>> >> But that information is can be deduced by this function - the function >> is called for that CPU from *that* CPU. Also, the absence of an >> argument, ensures that the caller won't make a mistake of calling any >> other CPUs runtime PM from a CPU or worse, pass a device that is not a >> CPU. > >Okay! As long as we decide to use the API *only* for CPUs that makes sense. > >Although, I was thinking that we perhaps shouldn't limit the use of >the API to CPUs, but I don't know of any similar devices as of now. > >> >>>> + + trace_rpm_suspend(dev, 0); >>>> + >>>> + /** >>>> + * Use device usage_count to disallow bubbling up suspend. >>>> + * This CPU has already decided to suspend, we cannot >>>> + * prevent it here. >>>> + */ >>>> + if (!atomic_dec_and_test(&dev->power.usage_count)) >>>> + return 0; >>>> + >>>> + ret = rpm_check_suspend_allowed(dev); >>> >>> >>> I don't think you can use this function. For example it calls >>> __dev_pm_qos_read_value() which expects the dev->power.lock to be >>> held. >>> >> Right. I realized that. Will fix. >> >>>> + if (ret) >>>> + return ret; >>>> + >>>> + __update_runtime_status(dev, RPM_SUSPENDING); >>>> + >>>> + pm_runtime_cancel_pending(dev); >>> >>> >>> Hmm. For the same struct device (CPU) could really calls to >>> cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to >>> protect against that? >>> >> That wouldnt happen, the functions are only called that CPU on that CPU. >> See the explanation above. >> >>> I don't have such in-depth knowledge about CPU idle, so apologize if >>> this may be a stupid question. >>> >>> If the answer to the above is *no*, I believe you don't need to care >>> about the intermediate RPM_SUSPENDING state and you don't need an >>> atomic counter either, right!? >>> >> This calls into genpd framework, which expects devices to be >> RPM_SUSPENDING in pm_genpd_power_off; I wanted to keep the behavior >> between the frameworks consistent. > >Okay, it makes sense. Thanks for clarifying. > >> >> >>> Instead you could then just update the runtime PM status to >>> RPM_SUSPENDED if the RPM callback doesn't return an error. >>> >>>> + callback = RPM_GET_CALLBACK(dev, runtime_suspend); >>>> + >>>> + ret = callback(dev); >>>> + if (!ret) >>>> + __update_runtime_status(dev, RPM_SUSPENDED); >>>> + else >>>> + __update_runtime_status(dev, RPM_ACTIVE); >>>> + >>>> + trace_rpm_return_int(dev, _THIS_IP_, ret); >>>> +} >>>> + >>>> +void cpu_pm_runtime_resume(void) >>> > >[...] > >Kind regards >Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + + /** + * Use device usage_count to disallow bubbling up suspend. + * This CPU has already decided to suspend, we cannot + * prevent it here. + */ + if (!atomic_dec_and_test(&dev->power.usage_count)) + 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); + + ret = callback(dev); + if (!ret) + __update_runtime_status(dev, RPM_SUSPENDED); + else + __update_runtime_status(dev, RPM_ACTIVE); + + trace_rpm_return_int(dev, _THIS_IP_, ret); +} + +void cpu_pm_runtime_resume(void) +{ + int ret; + int (*callback)(struct device *); + struct device *dev = get_cpu_device(smp_processor_id()); + + trace_rpm_resume(dev, 0); + + if (dev->power.runtime_status == RPM_ACTIVE) + return 1; + + atomic_inc(&dev->power.usage_count); + + __update_runtime_status(dev, RPM_RESUMING); + + callback = RPM_GET_CALLBACK(dev, runtime_resume); + + ret = callback(dev); + if (!ret) + __update_runtime_status(dev, RPM_ACTIVE); + else + __update_runtime_status(dev, RPM_SUSPENDED); + + trace_rpm_return_int(dev, _THIS_IP_, ret); +} + /** * rpm_resume - Carry out runtime resume of given device. * @dev: Device to resume. diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 3bdbb41..3655ead 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -31,6 +31,8 @@ static inline bool queue_pm_work(struct work_struct *work) return queue_work(pm_wq, work); } +extern void cpu_pm_runtime_suspend(void); +extern void cpu_pm_runtime_resume(void); extern int pm_generic_runtime_suspend(struct device *dev); extern int pm_generic_runtime_resume(struct device *dev); extern int pm_runtime_force_suspend(struct device *dev); @@ -273,5 +275,4 @@ static inline void pm_runtime_dont_use_autosuspend(struct device *dev) { __pm_runtime_use_autosuspend(dev, false); } - #endif
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. 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. 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(-)