| Message ID | 1430391335-7588-3-git-send-email-ahaslam@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >From: Axel Haslam <ahaslam+renesas@baylibre.com> > >Add the core changes to be able to declare multiple states. >When trying to set a power domain to off, genpd will be able >to choose from an array of states declared by the platform. > >The power on and off latencies are now tied to a state. > >States should be declared in ascending order from shallowest >to deepest, deepest meaning the state which takes longer to >enter and exit. > >the power_off and power_on function can use the 'state_idx' >field of the generic_pm_domain structure, to distinguish between >the different states and act accordingly. > > - if the genpd is initially off, the user should set the state_idx > field when registering the genpd. > > - if no states are passed to pm_genpd_init, a single OFF > state with no latency is assumed. > >Example: > >static int pd1_power_on(struct generic_pm_domain *domain) >{ > /* domain->state_idx = state the domain is coming from */ >} > >static int pd1_power_off(struct generic_pm_domain *domain) >{ > /* domain->state_idx = desired powered off state */ >} > >const struct genpd_power_state pd_states[] = { > { > .name = "RET", > .power_on_latency_ns = ON_LATENCY_FAST, > .power_off_latency_ns = OFF_LATENCY_FAST, > }, > { > .name = "DEEP_RET", > .power_on_latency_ns = ON_LATENCY_MED, > .power_off_latency_ns = OFF_LATENCY_MED, > }, > { > .name = "OFF", > .power_on_latency_ns = ON_LATENCY_SLOW, > .power_off_latency_ns = OFF_LATENCY_SLOW, > } >}; > I am trying to use your patches in fashioning a genpd for the cpu domain. The cpus are part of a power domain that can be powered off when the cpus are powered off as part of the cpuidle. One of the biggest power savings comes from powering off the domain completely. However, powering off the domain involves flushing of caches (possibly) and turning off some regulators and peripheral hardware. The power benefit is only realized when the domain remains powered off for a certain period of time, otherwise the overhead of powering on/off would add up to the ineffeciency in the system. So a governor that determines the idle state of the domain has two things that needs to match, the time available to power off the domain (which we can get from timer wheel) and the other residency as dictated by the platform. I was wondering if we could match the idle state definition of the domain with that of the cpu, which has a precedence in the kernel. The idle state of the cpu [1] is a superset of the idle state definition you have above. That way a shim layer could pick up domain idle states from DT and pass the states to pm_genpd_init(). The use of these values could depend on the genpd governor. Thanks, Lina >struct generic_pm_domain pd1 = { > .name = "PD1", > .state_idx = 2, /* needed if domain is not on at init. */ > .power_on = pd1_power_on, > .power_off = pd1_power_off, > ... >}; > >int xxx_init_pm_domain(){ > > pm_genpd_init(struct generic_pm_domain, > pd1, pd_states, ARRAY_SIZE(pd_states), true); > >} > >Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> >--- [1]. Documentation/devicetree/bindings/arm/idle-states.txt -- 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 Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >>From: Axel Haslam <ahaslam+renesas@baylibre.com> >> >>Add the core changes to be able to declare multiple states. >>When trying to set a power domain to off, genpd will be able >>to choose from an array of states declared by the platform. >> >>The power on and off latencies are now tied to a state. >> >>States should be declared in ascending order from shallowest >>to deepest, deepest meaning the state which takes longer to >>enter and exit. >> >>the power_off and power_on function can use the 'state_idx' >>field of the generic_pm_domain structure, to distinguish between >>the different states and act accordingly. >> >> - if the genpd is initially off, the user should set the state_idx >> field when registering the genpd. >> >> - if no states are passed to pm_genpd_init, a single OFF >> state with no latency is assumed. >> >>Example: >> >>static int pd1_power_on(struct generic_pm_domain *domain) >>{ >> /* domain->state_idx = state the domain is coming from */ >>} >> >>static int pd1_power_off(struct generic_pm_domain *domain) >>{ >> /* domain->state_idx = desired powered off state */ >>} >> >>const struct genpd_power_state pd_states[] = { >> { >> .name = "RET", >> .power_on_latency_ns = ON_LATENCY_FAST, >> .power_off_latency_ns = OFF_LATENCY_FAST, >> }, >> { >> .name = "DEEP_RET", >> .power_on_latency_ns = ON_LATENCY_MED, >> .power_off_latency_ns = OFF_LATENCY_MED, >> }, >> { >> .name = "OFF", >> .power_on_latency_ns = ON_LATENCY_SLOW, >> .power_off_latency_ns = OFF_LATENCY_SLOW, >> } >>}; >> > > I am trying to use your patches in fashioning a genpd for the cpu > domain. > > The cpus are part of a power domain that can be powered off when the > cpus are powered off as part of the cpuidle. One of the biggest power > savings comes from powering off the domain completely. However, powering > off the domain involves flushing of caches (possibly) and turning off > some regulators and peripheral hardware. The power benefit is only > realized when the domain remains powered off for a certain period of > time, otherwise the overhead of powering on/off would add up to the > ineffeciency in the system. > > So a governor that determines the idle state of the domain has two > things that needs to match, the time available to power off the domain > (which we can get from timer wheel) and the other residency as dictated > by the platform. > > I was wondering if we could match the idle state definition of the > domain with that of the cpu, which has a precedence in the kernel. The > idle state of the cpu [1] is a superset of the idle state definition you > have above. That way a shim layer could pick up domain idle states from > DT and pass the states to pm_genpd_init(). The use of these values could > depend on the genpd governor. IMO, we need to keep the domain latency descriptions separate from the the devices within those domains, which could be CPUs or any other form of device. It should be the job of the governor then to look at the domain latency along with latencies of the other devices in the domain to make its decision. 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, May 08 2015 at 14:53 -0600, Kevin Hilman wrote: >Lina Iyer <lina.iyer@linaro.org> writes: > >> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >>>From: Axel Haslam <ahaslam+renesas@baylibre.com> >>> >>>Add the core changes to be able to declare multiple states. >>>When trying to set a power domain to off, genpd will be able >>>to choose from an array of states declared by the platform. >>> >>>The power on and off latencies are now tied to a state. >>> >>>States should be declared in ascending order from shallowest >>>to deepest, deepest meaning the state which takes longer to >>>enter and exit. >>> >>>the power_off and power_on function can use the 'state_idx' >>>field of the generic_pm_domain structure, to distinguish between >>>the different states and act accordingly. >>> >>> - if the genpd is initially off, the user should set the state_idx >>> field when registering the genpd. >>> >>> - if no states are passed to pm_genpd_init, a single OFF >>> state with no latency is assumed. >>> >>>Example: >>> >>>static int pd1_power_on(struct generic_pm_domain *domain) >>>{ >>> /* domain->state_idx = state the domain is coming from */ >>>} >>> >>>static int pd1_power_off(struct generic_pm_domain *domain) >>>{ >>> /* domain->state_idx = desired powered off state */ >>>} >>> >>>const struct genpd_power_state pd_states[] = { >>> { >>> .name = "RET", >>> .power_on_latency_ns = ON_LATENCY_FAST, >>> .power_off_latency_ns = OFF_LATENCY_FAST, >>> }, >>> { >>> .name = "DEEP_RET", >>> .power_on_latency_ns = ON_LATENCY_MED, >>> .power_off_latency_ns = OFF_LATENCY_MED, >>> }, >>> { >>> .name = "OFF", >>> .power_on_latency_ns = ON_LATENCY_SLOW, >>> .power_off_latency_ns = OFF_LATENCY_SLOW, >>> } >>>}; >>> >> >> I am trying to use your patches in fashioning a genpd for the cpu >> domain. >> >> The cpus are part of a power domain that can be powered off when the >> cpus are powered off as part of the cpuidle. One of the biggest power >> savings comes from powering off the domain completely. However, powering >> off the domain involves flushing of caches (possibly) and turning off >> some regulators and peripheral hardware. The power benefit is only >> realized when the domain remains powered off for a certain period of >> time, otherwise the overhead of powering on/off would add up to the >> ineffeciency in the system. >> >> So a governor that determines the idle state of the domain has two >> things that needs to match, the time available to power off the domain >> (which we can get from timer wheel) and the other residency as dictated >> by the platform. >> >> I was wondering if we could match the idle state definition of the >> domain with that of the cpu, which has a precedence in the kernel. The >> idle state of the cpu [1] is a superset of the idle state definition you >> have above. That way a shim layer could pick up domain idle states from >> DT and pass the states to pm_genpd_init(). The use of these values could >> depend on the genpd governor. > >IMO, we need to keep the domain latency descriptions separate from the >the devices within those domains, which could be CPUs or any other form >of device. Sure, the devices could be any. All I am saying is the format of idle state definitions could be the same (same as cpu). There is a good value in specifying the residency of an idle state in addition to the the enter and exit latency. >It should be the job of the governor then to look at the domain latency >along with latencies of the other devices in the domain to make its >decision. Agreed and thats how it should be. In all probablility, a cpu-domain governor would need a residency value of the domain idle state to best determine the idle state of the domain. If we have not stored the residency in struct genpd_power_state, then the governor would not have a good way to make that decision. I am not ready yet with my patches, but feel free to browse through my working tree at [1] to see how I plan to use this patchset. Thanks, Lina [1]. https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3 -- 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
Hi Lina, + Bartosz who was interested in the patches too. On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote: > On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote: >> >> Lina Iyer <lina.iyer@linaro.org> writes: >> >>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >>>> >>>> From: Axel Haslam <ahaslam+renesas@baylibre.com> >>>> >>>> Add the core changes to be able to declare multiple states. >>>> When trying to set a power domain to off, genpd will be able >>>> to choose from an array of states declared by the platform. >>>> >>>> The power on and off latencies are now tied to a state. >>>> >>>> States should be declared in ascending order from shallowest >>>> to deepest, deepest meaning the state which takes longer to >>>> enter and exit. >>>> >>>> the power_off and power_on function can use the 'state_idx' >>>> field of the generic_pm_domain structure, to distinguish between >>>> the different states and act accordingly. >>>> >>>> - if the genpd is initially off, the user should set the state_idx >>>> field when registering the genpd. >>>> >>>> - if no states are passed to pm_genpd_init, a single OFF >>>> state with no latency is assumed. >>>> >>>> Example: >>>> >>>> static int pd1_power_on(struct generic_pm_domain *domain) >>>> { >>>> /* domain->state_idx = state the domain is coming from */ >>>> } >>>> >>>> static int pd1_power_off(struct generic_pm_domain *domain) >>>> { >>>> /* domain->state_idx = desired powered off state */ >>>> } >>>> >>>> const struct genpd_power_state pd_states[] = { >>>> { >>>> .name = "RET", >>>> .power_on_latency_ns = ON_LATENCY_FAST, >>>> .power_off_latency_ns = OFF_LATENCY_FAST, >>>> }, >>>> { >>>> .name = "DEEP_RET", >>>> .power_on_latency_ns = ON_LATENCY_MED, >>>> .power_off_latency_ns = OFF_LATENCY_MED, >>>> }, >>>> { >>>> .name = "OFF", >>>> .power_on_latency_ns = ON_LATENCY_SLOW, >>>> .power_off_latency_ns = OFF_LATENCY_SLOW, >>>> } >>>> }; >>>> >>> >>> I am trying to use your patches in fashioning a genpd for the cpu >>> domain. >>> >>> The cpus are part of a power domain that can be powered off when the >>> cpus are powered off as part of the cpuidle. One of the biggest power >>> savings comes from powering off the domain completely. However, powering >>> off the domain involves flushing of caches (possibly) and turning off >>> some regulators and peripheral hardware. The power benefit is only >>> realized when the domain remains powered off for a certain period of >>> time, otherwise the overhead of powering on/off would add up to the >>> ineffeciency in the system. >>> >>> So a governor that determines the idle state of the domain has two >>> things that needs to match, the time available to power off the domain >>> (which we can get from timer wheel) and the other residency as dictated >>> by the platform. >>> >>> I was wondering if we could match the idle state definition of the >>> domain with that of the cpu, which has a precedence in the kernel. The >>> idle state of the cpu [1] is a superset of the idle state definition you >>> have above. That way a shim layer could pick up domain idle states from >>> DT and pass the states to pm_genpd_init(). The use of these values could >>> depend on the genpd governor. >> >> >> IMO, we need to keep the domain latency descriptions separate from the >> the devices within those domains, which could be CPUs or any other form >> of device. > > > Sure, the devices could be any. All I am saying is the format of idle > state definitions could be the same (same as cpu). There is a good > value in specifying the residency of an idle state in addition to the > the enter and exit latency. > >> It should be the job of the governor then to look at the domain latency >> along with latencies of the other devices in the domain to make its >> decision. > > > Agreed and thats how it should be. In all probablility, a cpu-domain > governor would need a residency value of the domain idle state to best > determine the idle state of the domain. > > If we have not stored the residency in struct genpd_power_state, then > the governor would not have a good way to make that decision. > > I am not ready yet with my patches, but feel free to browse through my > working tree at [1] to see how I plan to use this patchset. I took a quick look at the tree you mentioned and i have a couple of doubts about the concepts. i will list what i understood so maybe you can comment if im wrong: - genpd cpu-domain governor is not meant to replace the cpuidle governor. right? - the idea is to tie the cpuidle states to the cpu-domain states. there would be a c-state for doing a cluster off, with the off/on latencies and min residency values that would represent the cost of turning the cluster off. - the cpuidle governor (menu governor)has some extra logic to decide the idle state aside from the exit latencies and target residency (eg, buckets, performance multiplier). Even if the next wakeup, latencies and target residencies justify going to a deep c-state, the governor can decide to demote that state based on the recent history. im not sure were the line is between the governor taking a c state decision and genpd taking another one, I feel the cpu-domain governor will not always respect the same state decision as that the cpuidle governor may have taken, and i guess there should not be two places were the decision is taken. (but again,maybe i got the idea wrong) - the cpu_pm_enter call in cpuidle, which will notify of the intention to suspend, happens before the actual suspend. when the genpd gets the notifications you will call pm_runtime_put_sync(dev) for the cpu which will potentially turn off the cluster using the genpd-off callback (regulators..etc...), but the cpu is not yet be suspended right? -maybe it was discussed before, But i think that if the cpuidle governor would save the time a cpu entered in a particular state, we could add a .validate callback in the menu governor that would take the a cpumask of the cpus in the cluster and would compare the time each cpu entered a c-state against the predicted time to get how much "remaining time" is "probably" left. That way we could know if a cluster off i worth it or not, and it would use the inteligence allready available in the cpuidle governor. Regards Axel > > Thanks, > Lina > > [1]. > https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3 -- 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, May 11 2015 at 09:53 -0600, Axel Haslam wrote: >Hi Lina, > >+ Bartosz who was interested in the patches too. > >On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote: >> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote: >>> >>> Lina Iyer <lina.iyer@linaro.org> writes: >>> >>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >>>>> >>>>> From: Axel Haslam <ahaslam+renesas@baylibre.com> >>>>> >>>>> Add the core changes to be able to declare multiple states. >>>>> When trying to set a power domain to off, genpd will be able >>>>> to choose from an array of states declared by the platform. >>>>> >>>>> The power on and off latencies are now tied to a state. >>>>> >>>>> States should be declared in ascending order from shallowest >>>>> to deepest, deepest meaning the state which takes longer to >>>>> enter and exit. >>>>> >>>>> the power_off and power_on function can use the 'state_idx' >>>>> field of the generic_pm_domain structure, to distinguish between >>>>> the different states and act accordingly. >>>>> >>>>> - if the genpd is initially off, the user should set the state_idx >>>>> field when registering the genpd. >>>>> >>>>> - if no states are passed to pm_genpd_init, a single OFF >>>>> state with no latency is assumed. >>>>> >>>>> Example: >>>>> >>>>> static int pd1_power_on(struct generic_pm_domain *domain) >>>>> { >>>>> /* domain->state_idx = state the domain is coming from */ >>>>> } >>>>> >>>>> static int pd1_power_off(struct generic_pm_domain *domain) >>>>> { >>>>> /* domain->state_idx = desired powered off state */ >>>>> } >>>>> >>>>> const struct genpd_power_state pd_states[] = { >>>>> { >>>>> .name = "RET", >>>>> .power_on_latency_ns = ON_LATENCY_FAST, >>>>> .power_off_latency_ns = OFF_LATENCY_FAST, >>>>> }, >>>>> { >>>>> .name = "DEEP_RET", >>>>> .power_on_latency_ns = ON_LATENCY_MED, >>>>> .power_off_latency_ns = OFF_LATENCY_MED, >>>>> }, >>>>> { >>>>> .name = "OFF", >>>>> .power_on_latency_ns = ON_LATENCY_SLOW, >>>>> .power_off_latency_ns = OFF_LATENCY_SLOW, >>>>> } >>>>> }; >>>>> >>>> >>>> I am trying to use your patches in fashioning a genpd for the cpu >>>> domain. >>>> >>>> The cpus are part of a power domain that can be powered off when the >>>> cpus are powered off as part of the cpuidle. One of the biggest power >>>> savings comes from powering off the domain completely. However, powering >>>> off the domain involves flushing of caches (possibly) and turning off >>>> some regulators and peripheral hardware. The power benefit is only >>>> realized when the domain remains powered off for a certain period of >>>> time, otherwise the overhead of powering on/off would add up to the >>>> ineffeciency in the system. >>>> >>>> So a governor that determines the idle state of the domain has two >>>> things that needs to match, the time available to power off the domain >>>> (which we can get from timer wheel) and the other residency as dictated >>>> by the platform. >>>> >>>> I was wondering if we could match the idle state definition of the >>>> domain with that of the cpu, which has a precedence in the kernel. The >>>> idle state of the cpu [1] is a superset of the idle state definition you >>>> have above. That way a shim layer could pick up domain idle states from >>>> DT and pass the states to pm_genpd_init(). The use of these values could >>>> depend on the genpd governor. >>> >>> >>> IMO, we need to keep the domain latency descriptions separate from the >>> the devices within those domains, which could be CPUs or any other form >>> of device. >> >> >> Sure, the devices could be any. All I am saying is the format of idle >> state definitions could be the same (same as cpu). There is a good >> value in specifying the residency of an idle state in addition to the >> the enter and exit latency. >> >>> It should be the job of the governor then to look at the domain latency >>> along with latencies of the other devices in the domain to make its >>> decision. >> >> >> Agreed and thats how it should be. In all probablility, a cpu-domain >> governor would need a residency value of the domain idle state to best >> determine the idle state of the domain. >> >> If we have not stored the residency in struct genpd_power_state, then >> the governor would not have a good way to make that decision. >> >> I am not ready yet with my patches, but feel free to browse through my >> working tree at [1] to see how I plan to use this patchset. > > >I took a quick look at the tree you mentioned and i have a couple of >doubts about >the concepts. i will list what i understood so maybe you can comment >if im wrong: Hi Axel, Thanks for looking at my series. > >- genpd cpu-domain governor is not meant to replace the cpuidle governor. >right? > That is correct. I am not replacing the cpuidle governor. What I am working on is independent of the cpuidle governor. >- the idea is to tie the cpuidle states to the cpu-domain states. there >would be a c-state for doing a cluster off, with the off/on latencies >and min residency >values that would represent the cost of turning the cluster off. Not necessarily. The map is not to individual C-States of the cpu, but only to the power down state of the cpu (the ones which send out CPU_PM notifications), at this time. The residencies of the individual cpu c-states are for the cpu only. A similar but independent one could exist for the cpu-domain is the proposal here. > >- the cpuidle governor (menu governor)has some extra logic to decide the idle >state aside from the exit latencies and target residency (eg, buckets, >performance multiplier). >Even if the next wakeup, latencies and target residencies justify going to a >deep c-state, the governor can decide to demote that state based on the >recent history. im not sure were the line is between the governor taking a >c state decision and genpd taking another one, I feel the cpu-domain >governor will not always respect the same state decision as that the cpuidle >governor may have taken, and i guess there should not be two places >were the decision >is taken. (but again,maybe i got the idea wrong)o Absolutely. The cpus will obey the idle state suggested by the menu or any other governor. Once a state is chosen, the cpu is expected to enter the state or bail out if there was a pending interrupt. CPU-Domain decisions will be based on the cpuidle governor decisions. For states that CPU_PM notifications are not sent out, we can be sure that the cpu is not powered off and so the domain would not be powered off as well. Hence the tie up of the pm_runtime_put_sync() with CPU_PM notifications. But take the case where the menu governor decides for each cpu, that it can power down the cpu. If there is enough time where all cpus are powered off in the domain, then the domain can safely be powered down, until the first of the cpus in that domain wakes up. The time between the last cpu down and the first cpu up is generally a good opportunity for power saving. Power savings could come from turning off peripheral hardware, flushing cashes, switching regulators to supply a low load mode. There is usually a cost benefit table associated with the sleep time available for the domain vs turning off these components and that cost benefict analysis in my case is the residency - the sleep time that the domain needs to sleep in order to reap the benefits of the overhead in powering off and on the domain while the cpus are sleeping. Its fairly common for the mobile cpus to sleep tens of milliseconds between active operations and it is beneficial to flush caches and power the domain in many such cases. > >- the cpu_pm_enter call in cpuidle, which will notify of the intention to >suspend, happens before the actual suspend. when the genpd gets the >notifications >you will call pm_runtime_put_sync(dev) for the cpu which will potentially >turn off the cluster using the genpd-off callback (regulators..etc...), >but the cpu is not yet be suspended right? > Also correct. Most architectures (ARM especially)that allow powering off the cpu domain, usually would do it through a peripheral power-controller that is capable of being triggered when the last cpu in the domain is powered off. In QCOM SoCs that I am most familiar with, ARM WFI signal (which means that the processor is clock gated, therefore not running) from a processor is used to trigger an external power controller for the cpu. This ensures that there is no race with the cpu actually running when the power-controller is powering down the cpu on its behalf. The same WFI signal is also sent to the domain power-controller that can be configured to power down the caches and turn off regulators etc. On the way up, the interrupt to the core is trapped to wake up the domain first (if powered off), before the cpu is resumed. The CPU_PM notifications do get sent before the cpu is actually powered off, but is sent at a point, when the decision has been made to power off the cpu. So the domain decision can be made decisively knowing what state the cpus are going to be in. In all likelihood, there might be a pending interrupt for the cpu, when this domain decision happens, in which case, both the cpu and the domain would bail out of their idle states. >-maybe it was discussed before, But i think that if the cpuidle governor >would save the time a cpu entered in a particular state, we could add >a .validate callback >in the menu governor that would take the a cpumask of the cpus in the cluster >and would compare the time each cpu entered a c-state against the predicted >time to get how much "remaining time" is "probably" left. That way we could >know if a cluster off i worth it or not, and it would use the inteligence >allready available in the cpuidle governor. Fair point. And that could be ideal. The governor algorithm need not exist with GenPD and could be handled by the menu governor. I can work on that, but since it is external to menu governor decision, I had chosen to put as a genpd governor. This can be discussed more. Thanks, Lina >> [1]. >> https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3 -- 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
Hi Lina On Mon, May 11, 2015 at 6:50 PM, Lina Iyer <lina.iyer@linaro.org> wrote: > On Mon, May 11 2015 at 09:53 -0600, Axel Haslam wrote: >> >> Hi Lina, >> >> + Bartosz who was interested in the patches too. >> >> On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote: >>> >>> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote: >>>> >>>> >>>> Lina Iyer <lina.iyer@linaro.org> writes: >>>> >>>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >>>>>> >>>>>> >>>>>> From: Axel Haslam <ahaslam+renesas@baylibre.com> >>>>>> >>>>>> Add the core changes to be able to declare multiple states. >>>>>> When trying to set a power domain to off, genpd will be able >>>>>> to choose from an array of states declared by the platform. >>>>>> >>>>>> The power on and off latencies are now tied to a state. >>>>>> >>>>>> States should be declared in ascending order from shallowest >>>>>> to deepest, deepest meaning the state which takes longer to >>>>>> enter and exit. >>>>>> >>>>>> the power_off and power_on function can use the 'state_idx' >>>>>> field of the generic_pm_domain structure, to distinguish between >>>>>> the different states and act accordingly. >>>>>> >>>>>> - if the genpd is initially off, the user should set the state_idx >>>>>> field when registering the genpd. >>>>>> >>>>>> - if no states are passed to pm_genpd_init, a single OFF >>>>>> state with no latency is assumed. >>>>>> >>>>>> Example: >>>>>> >>>>>> static int pd1_power_on(struct generic_pm_domain *domain) >>>>>> { >>>>>> /* domain->state_idx = state the domain is coming from */ >>>>>> } >>>>>> >>>>>> static int pd1_power_off(struct generic_pm_domain *domain) >>>>>> { >>>>>> /* domain->state_idx = desired powered off state */ >>>>>> } >>>>>> >>>>>> const struct genpd_power_state pd_states[] = { >>>>>> { >>>>>> .name = "RET", >>>>>> .power_on_latency_ns = ON_LATENCY_FAST, >>>>>> .power_off_latency_ns = OFF_LATENCY_FAST, >>>>>> }, >>>>>> { >>>>>> .name = "DEEP_RET", >>>>>> .power_on_latency_ns = ON_LATENCY_MED, >>>>>> .power_off_latency_ns = OFF_LATENCY_MED, >>>>>> }, >>>>>> { >>>>>> .name = "OFF", >>>>>> .power_on_latency_ns = ON_LATENCY_SLOW, >>>>>> .power_off_latency_ns = OFF_LATENCY_SLOW, >>>>>> } >>>>>> }; >>>>>> >>>>> >>>>> I am trying to use your patches in fashioning a genpd for the cpu >>>>> domain. >>>>> >>>>> The cpus are part of a power domain that can be powered off when the >>>>> cpus are powered off as part of the cpuidle. One of the biggest power >>>>> savings comes from powering off the domain completely. However, >>>>> powering >>>>> off the domain involves flushing of caches (possibly) and turning off >>>>> some regulators and peripheral hardware. The power benefit is only >>>>> realized when the domain remains powered off for a certain period of >>>>> time, otherwise the overhead of powering on/off would add up to the >>>>> ineffeciency in the system. >>>>> >>>>> So a governor that determines the idle state of the domain has two >>>>> things that needs to match, the time available to power off the domain >>>>> (which we can get from timer wheel) and the other residency as dictated >>>>> by the platform. >>>>> >>>>> I was wondering if we could match the idle state definition of the >>>>> domain with that of the cpu, which has a precedence in the kernel. The >>>>> idle state of the cpu [1] is a superset of the idle state definition >>>>> you >>>>> have above. That way a shim layer could pick up domain idle states >>>>> from >>>>> DT and pass the states to pm_genpd_init(). The use of these values >>>>> could >>>>> depend on the genpd governor. >>>> >>>> >>>> >>>> IMO, we need to keep the domain latency descriptions separate from the >>>> the devices within those domains, which could be CPUs or any other form >>>> of device. >>> >>> >>> >>> Sure, the devices could be any. All I am saying is the format of idle >>> state definitions could be the same (same as cpu). There is a good >>> value in specifying the residency of an idle state in addition to the >>> the enter and exit latency. >>> >>>> It should be the job of the governor then to look at the domain latency >>>> along with latencies of the other devices in the domain to make its >>>> decision. >>> >>> >>> >>> Agreed and thats how it should be. In all probablility, a cpu-domain >>> governor would need a residency value of the domain idle state to best >>> determine the idle state of the domain. >>> >>> If we have not stored the residency in struct genpd_power_state, then >>> the governor would not have a good way to make that decision. >>> >>> I am not ready yet with my patches, but feel free to browse through my >>> working tree at [1] to see how I plan to use this patchset. >> >> >> >> I took a quick look at the tree you mentioned and i have a couple of >> doubts about >> the concepts. i will list what i understood so maybe you can comment >> if im wrong: > > > Hi Axel, > > Thanks for looking at my series. > >> >> - genpd cpu-domain governor is not meant to replace the cpuidle governor. >> right? >> > That is correct. I am not replacing the cpuidle governor. What I am > working on is independent of the cpuidle governor. > >> - the idea is to tie the cpuidle states to the cpu-domain states. there >> would be a c-state for doing a cluster off, with the off/on latencies >> and min residency >> values that would represent the cost of turning the cluster off. > > > Not necessarily. The map is not to individual C-States of the cpu, but > only to the power down state of the cpu (the ones which send out CPU_PM > notifications), at this time. The residencies of the individual cpu > c-states are for the cpu only. A similar but independent one could exist > for the cpu-domain is the proposal here. Thanks for the clarification, i think most of my confusion is that i had imagined doing a cluster off as simply a more expensive c-state. >> >> >> - the cpuidle governor (menu governor)has some extra logic to decide the >> idle >> state aside from the exit latencies and target residency (eg, buckets, >> performance multiplier). >> Even if the next wakeup, latencies and target residencies justify going to >> a >> deep c-state, the governor can decide to demote that state based on the >> recent history. im not sure were the line is between the governor taking a >> c state decision and genpd taking another one, I feel the cpu-domain >> governor will not always respect the same state decision as that the >> cpuidle >> governor may have taken, and i guess there should not be two places >> were the decision >> is taken. (but again,maybe i got the idea wrong)o > > > Absolutely. The cpus will obey the idle state suggested by the menu or > any other governor. Once a state is chosen, the cpu is expected to enter > the state or bail out if there was a pending interrupt. > > CPU-Domain decisions will be based on the cpuidle governor decisions. > For states that CPU_PM notifications are not sent out, we can be sure > that the cpu is not powered off and so the domain would not be powered > off as well. Hence the tie up of the pm_runtime_put_sync() with CPU_PM > notifications. > > But take the case where the menu governor decides for each cpu, that it > can power down the cpu. If there is enough time where all cpus are > powered off in the domain, then the domain can safely be powered down, > until the first of the cpus in that domain wakes up. The time between > the last cpu down and the first cpu up is generally a good opportunity > for power saving. Power savings could come from turning off peripheral > hardware, flushing cashes, switching regulators to supply a low load > mode. There is usually a cost benefit table associated with the sleep > time available for the domain vs turning off these components and that > cost benefict analysis in my case is the residency - the sleep time that > the domain needs to sleep in order to reap the benefits of the overhead > in powering off and on the domain while the cpus are sleeping. Its > fairly common for the mobile cpus to sleep tens of milliseconds between > active operations and it is beneficial to flush caches and power the > domain in many such cases. > >> >> - the cpu_pm_enter call in cpuidle, which will notify of the intention to >> suspend, happens before the actual suspend. when the genpd gets the >> notifications >> you will call pm_runtime_put_sync(dev) for the cpu which will potentially >> turn off the cluster using the genpd-off callback (regulators..etc...), >> but the cpu is not yet be suspended right? >> > > Also correct. Most architectures (ARM especially)that allow powering off > the cpu domain, usually would do it through a peripheral power-controller > that is capable of being triggered when the last cpu in the domain is > powered off. In QCOM SoCs that I am most familiar with, ARM WFI signal > (which means that the processor is clock gated, therefore not running) > from a processor is used to trigger an external power controller for the > cpu. This ensures that there is no race with the cpu actually running > when the power-controller is powering down the cpu on its behalf. The > same WFI signal is also sent to the domain power-controller that can > be configured to power down the caches and turn off regulators etc. On > the way up, the interrupt to the core is trapped to wake up the domain > first (if powered off), before the cpu is resumed. > > The CPU_PM notifications do get sent before the cpu is actually powered > off, but is sent at a point, when the decision has been made to power > off the cpu. So the domain decision can be made decisively knowing what > state the cpus are going to be in. > > In all likelihood, there might be a pending interrupt for the cpu, when > this domain decision happens, in which case, both the cpu and the domain > would bail out of their idle states. > >> -maybe it was discussed before, But i think that if the cpuidle governor >> would save the time a cpu entered in a particular state, we could add >> a .validate callback >> in the menu governor that would take the a cpumask of the cpus in the >> cluster >> and would compare the time each cpu entered a c-state against the >> predicted >> time to get how much "remaining time" is "probably" left. That way we >> could >> know if a cluster off i worth it or not, and it would use the inteligence >> allready available in the cpuidle governor. > > > Fair point. And that could be ideal. The governor algorithm need not > exist with GenPD and could be handled by the menu governor. I can work on > that, but since it is external to menu governor decision, I had > chosen to put as a genpd governor. This can be discussed more. maybe it can be usefull to start a separate thread about this discussion. i can post a RFC to start the thread.. Regards Axel > > Thanks, > Lina > >>> [1]. >>> >>> https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-8916-3 -- 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 Tue, May 12 2015 at 05:58 -0600, Axel Haslam wrote: >On Mon, May 11, 2015 at 6:50 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >> On Mon, May 11 2015 at 09:53 -0600, Axel Haslam wrote: >>> + Bartosz who was interested in the patches too. >>> On Sat, May 9, 2015 at 12:31 AM, Lina Iyer <lina.iyer@linaro.org> wrote: >>>> On Fri, May 08 2015 at 14:53 -0600, Kevin Hilman wrote: >>>>> Lina Iyer <lina.iyer@linaro.org> writes: >>>>>> On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: [...] > maybe it can be usefull to start a separate thread about this discussion. >i can post a RFC to start the thread.. > I was planning to post my patches soon. I am sure plenty of conversations would follow :) I think what started off this discussion (residency in idle states) could probably be submitted separately. 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/domain.c b/drivers/base/power/domain.c index b30a42f..86f0c46 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -47,6 +47,12 @@ __retval; \ }) +#define GENPD_MAX_NAME_SIZE 20 + +static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd, + const struct genpd_power_state *st, + unsigned int st_count); + static LIST_HEAD(gpd_list); static DEFINE_MUTEX(gpd_list_lock); @@ -170,12 +176,13 @@ static void genpd_set_active(struct generic_pm_domain *genpd) static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd) { + unsigned int state_idx = genpd->state_idx; s64 usecs64; if (!genpd->cpuidle_data) return; - usecs64 = genpd->power_on_latency_ns; + usecs64 = genpd->states[state_idx].power_on_latency_ns; do_div(usecs64, NSEC_PER_USEC); usecs64 += genpd->cpuidle_data->saved_exit_latency; genpd->cpuidle_data->idle_state->exit_latency = usecs64; @@ -183,6 +190,7 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd) static int genpd_power_on(struct generic_pm_domain *genpd) { + unsigned int state_idx = genpd->state_idx; ktime_t time_start; s64 elapsed_ns; int ret; @@ -196,10 +204,10 @@ static int genpd_power_on(struct generic_pm_domain *genpd) return ret; elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); - if (elapsed_ns <= genpd->power_on_latency_ns) + if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns) return ret; - genpd->power_on_latency_ns = elapsed_ns; + genpd->states[state_idx].power_on_latency_ns = elapsed_ns; genpd->max_off_time_changed = true; genpd_recalc_cpu_exit_latency(genpd); pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n", @@ -210,6 +218,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd) static int genpd_power_off(struct generic_pm_domain *genpd) { + unsigned int state_idx = genpd->state_idx; ktime_t time_start; s64 elapsed_ns; int ret; @@ -223,10 +232,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd) return ret; elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); - if (elapsed_ns <= genpd->power_off_latency_ns) + if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns) return ret; - genpd->power_off_latency_ns = elapsed_ns; + genpd->states[state_idx].power_off_latency_ns = elapsed_ns; genpd->max_off_time_changed = true; pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n", genpd->name, "off", elapsed_ns); @@ -847,6 +856,8 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd) || atomic_read(&genpd->sd_count) > 0) return; + /* Choose the deepest state when suspending */ + genpd->state_idx = genpd->state_count - 1; genpd_power_off(genpd); genpd->status = GPD_STATE_POWER_OFF; @@ -1468,6 +1479,61 @@ static void genpd_free_dev_data(struct device *dev, dev_pm_put_subsys_data(dev); } +static int genpd_alloc_states_data(struct generic_pm_domain *genpd, + const struct genpd_power_state *st, + unsigned int st_count) +{ + int ret = 0; + unsigned int i; + + if (IS_ERR_OR_NULL(genpd)) { + ret = -EINVAL; + goto err; + } + + if (!st || (st_count < 1)) { + ret = -EINVAL; + goto err; + } + + /* Allocate the local memory to keep the states for this genpd */ + genpd->states = kcalloc(st_count, sizeof(*st), GFP_KERNEL); + if (!genpd->states) { + ret = -ENOMEM; + goto err; + } + + for (i = 0; i < st_count; i++) { + genpd->states[i].power_on_latency_ns = + st[i].power_on_latency_ns; + genpd->states[i].power_off_latency_ns = + st[i].power_off_latency_ns; + } + + /* + * Copy the latency values To keep compatibility with + * platforms that are not converted to use the multiple states. + * This will be removed once all platforms are converted to use + * multiple states. note that non converted platforms will use the + * default single off state. + */ + if (genpd->power_on_latency_ns != 0) + genpd->states[0].power_on_latency_ns = + genpd->power_on_latency_ns; + + if (genpd->power_off_latency_ns != 0) + genpd->states[0].power_off_latency_ns = + genpd->power_off_latency_ns; + + genpd->state_count = st_count; + + /* to save memory, Name allocation will happen if debug is enabled */ + pm_genpd_alloc_states_names(genpd, st, st_count); + +err: + return ret; +} + /** * __pm_genpd_add_device - Add a device to an I/O PM domain. * @genpd: PM domain to add the device to. @@ -1891,9 +1957,37 @@ void pm_genpd_init(struct generic_pm_domain *genpd, const struct genpd_power_state *states, unsigned int state_count, bool is_off) { + static const struct genpd_power_state genpd_default_state[] = { + { + .name = "OFF", + .power_off_latency_ns = 0, + .power_on_latency_ns = 0, + }, + }; + int ret; + if (IS_ERR_OR_NULL(genpd)) return; + /* If no states defined, use the default OFF state */ + if (!states || (state_count < 1)) + ret = genpd_alloc_states_data(genpd, genpd_default_state, + ARRAY_SIZE(genpd_default_state)); + else + ret = genpd_alloc_states_data(genpd, states, state_count); + + if (ret) { + pr_err("Fail to allocate states for %s\n", genpd->name); + return; + } + + /* Sanity check for initial state */ + if (genpd->state_idx >= genpd->state_count) { + pr_warn("pm domain %s Invalid initial state.\n", + genpd->name); + genpd->state_idx = genpd->state_count - 1; + } + INIT_LIST_HEAD(&genpd->master_links); INIT_LIST_HEAD(&genpd->slave_links); INIT_LIST_HEAD(&genpd->dev_list); @@ -2247,6 +2341,33 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); #include <linux/kobject.h> static struct dentry *pm_genpd_debugfs_dir; +static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd, + const struct genpd_power_state *st, + unsigned int st_count) +{ + unsigned int i; + + if (IS_ERR_OR_NULL(genpd)) + return -EINVAL; + + if (genpd->state_count != st_count) { + pr_err("Invalid allocated state count\n"); + return -EINVAL; + } + + for (i = 0; i < st_count; i++) { + genpd->states[i].name = kstrndup(st[i].name, + GENPD_MAX_NAME_SIZE, GFP_KERNEL); + if (!genpd->states[i].name) { + pr_err("%s Failed to allocate state %d name.\n", + genpd->name, i); + return -ENOMEM; + } + } + + return 0; +} + /* * TODO: This function is a slightly modified version of rtpm_status_show * from sysfs.c, so generalize it. @@ -2283,6 +2404,7 @@ static int pm_genpd_summary_one(struct seq_file *s, [GPD_STATE_REPEAT] = "off-in-progress", [GPD_STATE_POWER_OFF] = "off" }; + unsigned int state_idx = genpd->state_idx; struct pm_domain_data *pm_data; const char *kobj_path; struct gpd_link *link; @@ -2294,7 +2416,11 @@ static int pm_genpd_summary_one(struct seq_file *s, if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup))) goto exit; - seq_printf(s, "%-30s %-15s ", genpd->name, status_lookup[genpd->status]); + + seq_printf(s, "%-30s %-15s ", genpd->name, + (genpd->status == GPD_STATE_POWER_OFF) ? + genpd->states[state_idx].name : + status_lookup[genpd->status]); /* * Modifications on the list require holding locks on both @@ -2382,4 +2508,11 @@ static void __exit pm_genpd_debug_exit(void) debugfs_remove_recursive(pm_genpd_debugfs_dir); } __exitcall(pm_genpd_debug_exit); +#else +static inline int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd, + const struct genpd_power_state *st, + unsigned int st_count) +{ + return 0; +} #endif /* CONFIG_PM_ADVANCED_DEBUG */ diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c index 2a4154a..5e63a84 100644 --- a/drivers/base/power/domain_governor.c +++ b/drivers/base/power/domain_governor.c @@ -124,8 +124,12 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) return genpd->cached_power_down_ok; } - off_on_time_ns = genpd->power_off_latency_ns + - genpd->power_on_latency_ns; + /* + * Use the only available state, until multiple state support is added + * to the governor. + */ + off_on_time_ns = genpd->states[0].power_off_latency_ns + + genpd->states[0].power_on_latency_ns; /* * It doesn't make sense to remove power from the domain if saving * the state of all devices in it and the power off/power on operations @@ -215,8 +219,11 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) * The difference between the computed minimum subdomain or device off * time and the time needed to turn the domain on is the maximum * theoretical time this domain can spend in the "off" state. + * Use the only available state, until multiple state support is added + * to the governor. */ - genpd->max_off_time_ns = min_off_time_ns - genpd->power_on_latency_ns; + genpd->max_off_time_ns = min_off_time_ns - + genpd->states[0].power_on_latency_ns; return true; } diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 6b4802e..581ae08 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -86,6 +86,10 @@ struct generic_pm_domain { void (*detach_dev)(struct generic_pm_domain *domain, struct device *dev); unsigned int flags; /* Bit field of configs for genpd */ + struct genpd_power_state *states; + unsigned int state_count; /* number of states */ + unsigned int state_idx; /* state that genpd will go to when off */ + }; static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)