* [RFC] cpuidle - remove the power_specified field in the driver [not found] <50831CA3.2020602@linaro.org> @ 2012-11-12 20:26 ` Daniel Lezcano 2012-11-12 21:09 ` Julius Werner 2012-11-18 8:40 ` Francesco Lavra 0 siblings, 2 replies; 8+ messages in thread From: Daniel Lezcano @ 2012-11-12 20:26 UTC (permalink / raw) To: linux-pm Cc: jwerner, khilman, len.brown, g.trinabh, deepthi, linux-kernel, rjw, akpm, snanda, linaro-dev This patch follows the discussion about reinitializing the power usage when a C-state is added/removed. https://lkml.org/lkml/2012/10/16/518 We realized the power usage field is never filled and when it is filled for tegra, the power_specified flag is not set making all these values to be resetted when the driver is initialized with the set_power_state function. Julius and I feel this is over-engineered and the power_specified flag could be simply removed and continue assuming the states are backward sorted. The menu governor select function is simplified as the power is ordered. Actually the condition is always true with the current code. The cpuidle_play_dead function is also simplified by doing a reverse lookup on the array. The set_power_states function is removed as it does no make sense anymore. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/cpuidle.c | 17 ++++------------- drivers/cpuidle/driver.c | 25 ------------------------- drivers/cpuidle/governors/menu.c | 8 ++------ include/linux/cpuidle.h | 2 +- 4 files changed, 7 insertions(+), 45 deletions(-) diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 711dd83..f983262 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -69,24 +69,15 @@ int cpuidle_play_dead(void) { struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); - int i, dead_state = -1; - int power_usage = -1; + int i; if (!drv) return -ENODEV; /* Find lowest-power state that supports long-term idle */ - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { - struct cpuidle_state *s = &drv->states[i]; - - if (s->power_usage < power_usage && s->enter_dead) { - power_usage = s->power_usage; - dead_state = i; - } - } - - if (dead_state != -1) - return drv->states[dead_state].enter_dead(dev, dead_state); + for (i = drv->state_count; i >= CPUIDLE_DRIVER_STATE_START; i--) + if (drv->states[i].play_dead) + return drv->states[i].enter_dead(dev, i); return -ENODEV; } diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 3af841f..bb045f1 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -19,34 +19,9 @@ DEFINE_SPINLOCK(cpuidle_driver_lock); static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu); -static void set_power_states(struct cpuidle_driver *drv) -{ - int i; - - /* - * cpuidle driver should set the drv->power_specified bit - * before registering if the driver provides - * power_usage numbers. - * - * If power_specified is not set, - * we fill in power_usage with decreasing values as the - * cpuidle code has an implicit assumption that state Cn - * uses less power than C(n-1). - * - * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned - * an power value of -1. So we use -2, -3, etc, for other - * c-states. - */ - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) - drv->states[i].power_usage = -1 - i; -} - static void __cpuidle_driver_init(struct cpuidle_driver *drv) { drv->refcnt = 0; - - if (!drv->power_specified) - set_power_states(drv); } static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c index 2efee27..14eb11f 100644 --- a/drivers/cpuidle/governors/menu.c +++ b/drivers/cpuidle/governors/menu.c @@ -312,7 +312,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) { struct menu_device *data = &__get_cpu_var(menu_devices); int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY); - int power_usage = -1; int i; int multiplier; struct timespec t; @@ -383,11 +382,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) if (s->exit_latency * multiplier > data->predicted_us) continue; - if (s->power_usage < power_usage) { - power_usage = s->power_usage; - data->last_state_idx = i; - data->exit_us = s->exit_latency; - } + data->last_state_idx = i; + data->exit_us = s->exit_latency; } /* not deepest C-state chosen for low predicted residency */ diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 3711b34..24cd103 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -126,9 +126,9 @@ struct cpuidle_driver { struct module *owner; int refcnt; - unsigned int power_specified:1; /* set to 1 to use the core cpuidle time keeping (for all states). */ unsigned int en_core_tk_irqen:1; + /* states array must be ordered in decreasing power consumption */ struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count; int safe_state_index; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-11-12 20:26 ` [RFC] cpuidle - remove the power_specified field in the driver Daniel Lezcano @ 2012-11-12 21:09 ` Julius Werner 2012-11-12 22:08 ` Daniel Lezcano 2012-11-18 8:40 ` Francesco Lavra 1 sibling, 1 reply; 8+ messages in thread From: Julius Werner @ 2012-11-12 21:09 UTC (permalink / raw) To: Daniel Lezcano Cc: linux-pm, khilman, len.brown, Trinabh Gupta, deepthi, linux-kernel, rjw, akpm, Sameer Nanda, Lists Linaro-dev Thanks for moving this along, Daniel. I think this is the right approach... the cpuidle driver shouldn't be more complex than necessary. Note that you are starting your loop too high in cpuidle_play_dead... states[state_count] is not an actual state anymore, it should start at state_count - 1. Also, I think you can go ahead and do the same last-to-first loop transformation with immediate return in the menu governor, for an extra tiny bit of performance. On Mon, Nov 12, 2012 at 12:26 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > This patch follows the discussion about reinitializing the power usage > when a C-state is added/removed. > > https://lkml.org/lkml/2012/10/16/518 > > We realized the power usage field is never filled and when it is > filled for tegra, the power_specified flag is not set making all these > values to be resetted when the driver is initialized with the set_power_state > function. > > Julius and I feel this is over-engineered and the power_specified > flag could be simply removed and continue assuming the states are > backward sorted. > > The menu governor select function is simplified as the power is ordered. > Actually the condition is always true with the current code. > > The cpuidle_play_dead function is also simplified by doing a reverse lookup > on the array. > > The set_power_states function is removed as it does no make sense anymore. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpuidle/cpuidle.c | 17 ++++------------- > drivers/cpuidle/driver.c | 25 ------------------------- > drivers/cpuidle/governors/menu.c | 8 ++------ > include/linux/cpuidle.h | 2 +- > 4 files changed, 7 insertions(+), 45 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 711dd83..f983262 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -69,24 +69,15 @@ int cpuidle_play_dead(void) > { > struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); > struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); > - int i, dead_state = -1; > - int power_usage = -1; > + int i; > > if (!drv) > return -ENODEV; > > /* Find lowest-power state that supports long-term idle */ > - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { > - struct cpuidle_state *s = &drv->states[i]; > - > - if (s->power_usage < power_usage && s->enter_dead) { > - power_usage = s->power_usage; > - dead_state = i; > - } > - } > - > - if (dead_state != -1) > - return drv->states[dead_state].enter_dead(dev, dead_state); > + for (i = drv->state_count; i >= CPUIDLE_DRIVER_STATE_START; i--) > + if (drv->states[i].play_dead) > + return drv->states[i].enter_dead(dev, i); > > return -ENODEV; > } > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > index 3af841f..bb045f1 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -19,34 +19,9 @@ DEFINE_SPINLOCK(cpuidle_driver_lock); > static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); > static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu); > > -static void set_power_states(struct cpuidle_driver *drv) > -{ > - int i; > - > - /* > - * cpuidle driver should set the drv->power_specified bit > - * before registering if the driver provides > - * power_usage numbers. > - * > - * If power_specified is not set, > - * we fill in power_usage with decreasing values as the > - * cpuidle code has an implicit assumption that state Cn > - * uses less power than C(n-1). > - * > - * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned > - * an power value of -1. So we use -2, -3, etc, for other > - * c-states. > - */ > - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) > - drv->states[i].power_usage = -1 - i; > -} > - > static void __cpuidle_driver_init(struct cpuidle_driver *drv) > { > drv->refcnt = 0; > - > - if (!drv->power_specified) > - set_power_states(drv); > } > > static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) > diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c > index 2efee27..14eb11f 100644 > --- a/drivers/cpuidle/governors/menu.c > +++ b/drivers/cpuidle/governors/menu.c > @@ -312,7 +312,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) > { > struct menu_device *data = &__get_cpu_var(menu_devices); > int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY); > - int power_usage = -1; > int i; > int multiplier; > struct timespec t; > @@ -383,11 +382,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) > if (s->exit_latency * multiplier > data->predicted_us) > continue; > > - if (s->power_usage < power_usage) { > - power_usage = s->power_usage; > - data->last_state_idx = i; > - data->exit_us = s->exit_latency; > - } > + data->last_state_idx = i; > + data->exit_us = s->exit_latency; > } > > /* not deepest C-state chosen for low predicted residency */ > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index 3711b34..24cd103 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -126,9 +126,9 @@ struct cpuidle_driver { > struct module *owner; > int refcnt; > > - unsigned int power_specified:1; > /* set to 1 to use the core cpuidle time keeping (for all states). */ > unsigned int en_core_tk_irqen:1; > + /* states array must be ordered in decreasing power consumption */ > struct cpuidle_state states[CPUIDLE_STATE_MAX]; > int state_count; > int safe_state_index; > -- > 1.7.5.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-11-12 21:09 ` Julius Werner @ 2012-11-12 22:08 ` Daniel Lezcano 0 siblings, 0 replies; 8+ messages in thread From: Daniel Lezcano @ 2012-11-12 22:08 UTC (permalink / raw) To: Julius Werner Cc: linux-pm, khilman, len.brown, Trinabh Gupta, deepthi, linux-kernel, rjw, akpm, Sameer Nanda, Lists Linaro-dev On 11/12/2012 10:09 PM, Julius Werner wrote: > Thanks for moving this along, Daniel. I think this is the right > approach... the cpuidle driver shouldn't be more complex than > necessary. > > Note that you are starting your loop too high in cpuidle_play_dead... > states[state_count] is not an actual state anymore, it should start at > state_count - 1. Yep. Thanks for catching this. > Also, I think you can go ahead and do the same > last-to-first loop transformation with immediate return in the menu > governor, for an extra tiny bit of performance. Yes, that makes sense. Thanks for the review. -- Daniel > On Mon, Nov 12, 2012 at 12:26 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> This patch follows the discussion about reinitializing the power usage >> when a C-state is added/removed. >> >> https://lkml.org/lkml/2012/10/16/518 >> >> We realized the power usage field is never filled and when it is >> filled for tegra, the power_specified flag is not set making all these >> values to be resetted when the driver is initialized with the set_power_state >> function. >> >> Julius and I feel this is over-engineered and the power_specified >> flag could be simply removed and continue assuming the states are >> backward sorted. >> >> The menu governor select function is simplified as the power is ordered. >> Actually the condition is always true with the current code. >> >> The cpuidle_play_dead function is also simplified by doing a reverse lookup >> on the array. >> >> The set_power_states function is removed as it does no make sense anymore. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpuidle/cpuidle.c | 17 ++++------------- >> drivers/cpuidle/driver.c | 25 ------------------------- >> drivers/cpuidle/governors/menu.c | 8 ++------ >> include/linux/cpuidle.h | 2 +- >> 4 files changed, 7 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >> index 711dd83..f983262 100644 >> --- a/drivers/cpuidle/cpuidle.c >> +++ b/drivers/cpuidle/cpuidle.c >> @@ -69,24 +69,15 @@ int cpuidle_play_dead(void) >> { >> struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); >> struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); >> - int i, dead_state = -1; >> - int power_usage = -1; >> + int i; >> >> if (!drv) >> return -ENODEV; >> >> /* Find lowest-power state that supports long-term idle */ >> - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { >> - struct cpuidle_state *s = &drv->states[i]; >> - >> - if (s->power_usage < power_usage && s->enter_dead) { >> - power_usage = s->power_usage; >> - dead_state = i; >> - } >> - } >> - >> - if (dead_state != -1) >> - return drv->states[dead_state].enter_dead(dev, dead_state); >> + for (i = drv->state_count; i >= CPUIDLE_DRIVER_STATE_START; i--) >> + if (drv->states[i].play_dead) >> + return drv->states[i].enter_dead(dev, i); >> >> return -ENODEV; >> } >> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c >> index 3af841f..bb045f1 100644 >> --- a/drivers/cpuidle/driver.c >> +++ b/drivers/cpuidle/driver.c >> @@ -19,34 +19,9 @@ DEFINE_SPINLOCK(cpuidle_driver_lock); >> static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); >> static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu); >> >> -static void set_power_states(struct cpuidle_driver *drv) >> -{ >> - int i; >> - >> - /* >> - * cpuidle driver should set the drv->power_specified bit >> - * before registering if the driver provides >> - * power_usage numbers. >> - * >> - * If power_specified is not set, >> - * we fill in power_usage with decreasing values as the >> - * cpuidle code has an implicit assumption that state Cn >> - * uses less power than C(n-1). >> - * >> - * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned >> - * an power value of -1. So we use -2, -3, etc, for other >> - * c-states. >> - */ >> - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) >> - drv->states[i].power_usage = -1 - i; >> -} >> - >> static void __cpuidle_driver_init(struct cpuidle_driver *drv) >> { >> drv->refcnt = 0; >> - >> - if (!drv->power_specified) >> - set_power_states(drv); >> } >> >> static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) >> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c >> index 2efee27..14eb11f 100644 >> --- a/drivers/cpuidle/governors/menu.c >> +++ b/drivers/cpuidle/governors/menu.c >> @@ -312,7 +312,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) >> { >> struct menu_device *data = &__get_cpu_var(menu_devices); >> int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY); >> - int power_usage = -1; >> int i; >> int multiplier; >> struct timespec t; >> @@ -383,11 +382,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) >> if (s->exit_latency * multiplier > data->predicted_us) >> continue; >> >> - if (s->power_usage < power_usage) { >> - power_usage = s->power_usage; >> - data->last_state_idx = i; >> - data->exit_us = s->exit_latency; >> - } >> + data->last_state_idx = i; >> + data->exit_us = s->exit_latency; >> } >> >> /* not deepest C-state chosen for low predicted residency */ >> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h >> index 3711b34..24cd103 100644 >> --- a/include/linux/cpuidle.h >> +++ b/include/linux/cpuidle.h >> @@ -126,9 +126,9 @@ struct cpuidle_driver { >> struct module *owner; >> int refcnt; >> >> - unsigned int power_specified:1; >> /* set to 1 to use the core cpuidle time keeping (for all states). */ >> unsigned int en_core_tk_irqen:1; >> + /* states array must be ordered in decreasing power consumption */ >> struct cpuidle_state states[CPUIDLE_STATE_MAX]; >> int state_count; >> int safe_state_index; >> -- >> 1.7.5.4 >> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-11-12 20:26 ` [RFC] cpuidle - remove the power_specified field in the driver Daniel Lezcano 2012-11-12 21:09 ` Julius Werner @ 2012-11-18 8:40 ` Francesco Lavra 2012-11-18 9:17 ` Daniel Lezcano 1 sibling, 1 reply; 8+ messages in thread From: Francesco Lavra @ 2012-11-18 8:40 UTC (permalink / raw) To: Daniel Lezcano Cc: linux-pm, khilman, deepthi, g.trinabh, linaro-dev, len.brown, linux-kernel, rjw, jwerner, akpm, snanda Hi, On 11/12/2012 09:26 PM, Daniel Lezcano wrote: > This patch follows the discussion about reinitializing the power usage > when a C-state is added/removed. > > https://lkml.org/lkml/2012/10/16/518 > > We realized the power usage field is never filled and when it is > filled for tegra, the power_specified flag is not set making all these > values to be resetted when the driver is initialized with the set_power_state > function. > > Julius and I feel this is over-engineered and the power_specified > flag could be simply removed and continue assuming the states are > backward sorted. > > The menu governor select function is simplified as the power is ordered. > Actually the condition is always true with the current code. > > The cpuidle_play_dead function is also simplified by doing a reverse lookup > on the array. > > The set_power_states function is removed as it does no make sense anymore. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpuidle/cpuidle.c | 17 ++++------------- > drivers/cpuidle/driver.c | 25 ------------------------- > drivers/cpuidle/governors/menu.c | 8 ++------ > include/linux/cpuidle.h | 2 +- > 4 files changed, 7 insertions(+), 45 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 711dd83..f983262 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -69,24 +69,15 @@ int cpuidle_play_dead(void) > { > struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); > struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); > - int i, dead_state = -1; > - int power_usage = -1; > + int i; > > if (!drv) > return -ENODEV; > > /* Find lowest-power state that supports long-term idle */ > - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { > - struct cpuidle_state *s = &drv->states[i]; > - > - if (s->power_usage < power_usage && s->enter_dead) { > - power_usage = s->power_usage; > - dead_state = i; > - } > - } > - > - if (dead_state != -1) > - return drv->states[dead_state].enter_dead(dev, dead_state); > + for (i = drv->state_count; i >= CPUIDLE_DRIVER_STATE_START; i--) > + if (drv->states[i].play_dead) I guess you meant drv->states[i].enter_dead > + return drv->states[i].enter_dead(dev, i); -- Francesco ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-11-18 8:40 ` Francesco Lavra @ 2012-11-18 9:17 ` Daniel Lezcano 2012-12-10 19:09 ` Julius Werner 0 siblings, 1 reply; 8+ messages in thread From: Daniel Lezcano @ 2012-11-18 9:17 UTC (permalink / raw) To: Francesco Lavra Cc: linux-pm, khilman, deepthi, g.trinabh, linaro-dev, len.brown, linux-kernel, rjw, jwerner, akpm, snanda On 11/18/2012 09:40 AM, Francesco Lavra wrote: > Hi, > > On 11/12/2012 09:26 PM, Daniel Lezcano wrote: >> This patch follows the discussion about reinitializing the power usage >> when a C-state is added/removed. >> >> https://lkml.org/lkml/2012/10/16/518 >> >> We realized the power usage field is never filled and when it is >> filled for tegra, the power_specified flag is not set making all these >> values to be resetted when the driver is initialized with the set_power_state >> function. >> >> Julius and I feel this is over-engineered and the power_specified >> flag could be simply removed and continue assuming the states are >> backward sorted. >> >> The menu governor select function is simplified as the power is ordered. >> Actually the condition is always true with the current code. >> >> The cpuidle_play_dead function is also simplified by doing a reverse lookup >> on the array. >> >> The set_power_states function is removed as it does no make sense anymore. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpuidle/cpuidle.c | 17 ++++------------- >> drivers/cpuidle/driver.c | 25 ------------------------- >> drivers/cpuidle/governors/menu.c | 8 ++------ >> include/linux/cpuidle.h | 2 +- >> 4 files changed, 7 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >> index 711dd83..f983262 100644 >> --- a/drivers/cpuidle/cpuidle.c >> +++ b/drivers/cpuidle/cpuidle.c >> @@ -69,24 +69,15 @@ int cpuidle_play_dead(void) >> { >> struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); >> struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); >> - int i, dead_state = -1; >> - int power_usage = -1; >> + int i; >> >> if (!drv) >> return -ENODEV; >> >> /* Find lowest-power state that supports long-term idle */ >> - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { >> - struct cpuidle_state *s = &drv->states[i]; >> - >> - if (s->power_usage < power_usage && s->enter_dead) { >> - power_usage = s->power_usage; >> - dead_state = i; >> - } >> - } >> - >> - if (dead_state != -1) >> - return drv->states[dead_state].enter_dead(dev, dead_state); >> + for (i = drv->state_count; i >= CPUIDLE_DRIVER_STATE_START; i--) >> + if (drv->states[i].play_dead) > > I guess you meant drv->states[i].enter_dead Yep :) -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-11-18 9:17 ` Daniel Lezcano @ 2012-12-10 19:09 ` Julius Werner 2012-12-10 22:41 ` Rafael J. Wysocki 2012-12-11 9:46 ` Daniel Lezcano 0 siblings, 2 replies; 8+ messages in thread From: Julius Werner @ 2012-12-10 19:09 UTC (permalink / raw) To: Daniel Lezcano, Rafael J. Wysocki Cc: Francesco Lavra, linux-pm, Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev, len.brown, linux-kernel, Andrew Morton, Sameer Nanda Hi, What is the current status of this? Daniel, do you think you have got enough feedback to submit a definitive patch for this? Rafael, would you approve of such a change? The bug with dynamically added C-states that is tied to this still hurts the battery life for some users across all distros every day, so I think it would be valuable to get a consistent solution into the mainline soon before everyone has to roll their own. On 11/12/2012 09:26 PM, Daniel Lezcano wrote: > This patch follows the discussion about reinitializing the power usage > when a C-state is added/removed. > > https://lkml.org/lkml/2012/10/16/518 > > We realized the power usage field is never filled and when it is > filled for tegra, the power_specified flag is not set making all these > values to be resetted when the driver is initialized with the set_power_state > function. > > Julius and I feel this is over-engineered and the power_specified > flag could be simply removed and continue assuming the states are > backward sorted. > > The menu governor select function is simplified as the power is ordered. > Actually the condition is always true with the current code. > > The cpuidle_play_dead function is also simplified by doing a reverse lookup > on the array. > > The set_power_states function is removed as it does no make sense anymore. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpuidle/cpuidle.c | 17 ++++------------- > drivers/cpuidle/driver.c | 25 ------------------------- > drivers/cpuidle/governors/menu.c | 8 ++------ > include/linux/cpuidle.h | 2 +- > 4 files changed, 7 insertions(+), 45 deletions(-) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-12-10 19:09 ` Julius Werner @ 2012-12-10 22:41 ` Rafael J. Wysocki 2012-12-11 9:46 ` Daniel Lezcano 1 sibling, 0 replies; 8+ messages in thread From: Rafael J. Wysocki @ 2012-12-10 22:41 UTC (permalink / raw) To: Julius Werner Cc: Daniel Lezcano, Francesco Lavra, linux-pm, Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev, len.brown, linux-kernel, Andrew Morton, Sameer Nanda, Len Brown On Monday, December 10, 2012 11:09:58 AM Julius Werner wrote: > Hi, > > What is the current status of this? Daniel, do you think you have got > enough feedback to submit a definitive patch for this? Rafael, would > you approve of such a change? I need to talk to Len about that before I give you a reliable answer. Thanks, Rafael > The bug with dynamically added C-states that is tied to this still > hurts the battery life for some users across all distros every day, so > I think it would be valuable to get a consistent solution into the > mainline soon before everyone has to roll their own. > > On 11/12/2012 09:26 PM, Daniel Lezcano wrote: > > This patch follows the discussion about reinitializing the power usage > > when a C-state is added/removed. > > > > https://lkml.org/lkml/2012/10/16/518 > > > > We realized the power usage field is never filled and when it is > > filled for tegra, the power_specified flag is not set making all these > > values to be resetted when the driver is initialized with the set_power_state > > function. > > > > Julius and I feel this is over-engineered and the power_specified > > flag could be simply removed and continue assuming the states are > > backward sorted. > > > > The menu governor select function is simplified as the power is ordered. > > Actually the condition is always true with the current code. > > > > The cpuidle_play_dead function is also simplified by doing a reverse lookup > > on the array. > > > > The set_power_states function is removed as it does no make sense anymore. > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > --- > > drivers/cpuidle/cpuidle.c | 17 ++++------------- > > drivers/cpuidle/driver.c | 25 ------------------------- > > drivers/cpuidle/governors/menu.c | 8 ++------ > > include/linux/cpuidle.h | 2 +- > > 4 files changed, 7 insertions(+), 45 deletions(-) > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cpuidle - remove the power_specified field in the driver 2012-12-10 19:09 ` Julius Werner 2012-12-10 22:41 ` Rafael J. Wysocki @ 2012-12-11 9:46 ` Daniel Lezcano 1 sibling, 0 replies; 8+ messages in thread From: Daniel Lezcano @ 2012-12-11 9:46 UTC (permalink / raw) To: Julius Werner Cc: Daniel Lezcano, Rafael J. Wysocki, Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev, len.brown, linux-pm, linux-kernel, Andrew Morton, Sameer Nanda On 12/10/2012 08:09 PM, Julius Werner wrote: > Hi, > > What is the current status of this? Daniel, do you think you have got > enough feedback to submit a definitive patch for this? Yes, I have a definitive patch. I will resend it tomorrow. Thanks -- Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-12-11 9:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <50831CA3.2020602@linaro.org>
2012-11-12 20:26 ` [RFC] cpuidle - remove the power_specified field in the driver Daniel Lezcano
2012-11-12 21:09 ` Julius Werner
2012-11-12 22:08 ` Daniel Lezcano
2012-11-18 8:40 ` Francesco Lavra
2012-11-18 9:17 ` Daniel Lezcano
2012-12-10 19:09 ` Julius Werner
2012-12-10 22:41 ` Rafael J. Wysocki
2012-12-11 9:46 ` Daniel Lezcano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).