| Message ID | 20191030075141.1039-2-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [V6,1/3] cpuidle: play_idle: Make play_idle more flexible | expand |
On 08/11/2019 02:20, Rafael J. Wysocki wrote: > On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote: >> Currently, the play_idle function does not allow to tell which idle >> state we want to go. Improve this by passing the idle state as >> parameter to the function. >> >> Export cpuidle_find_deepest_state() symbol as it is used from the >> intel_powerclamp driver as a module. >> >> There is no functional changes, the cpuidle state is the deepest one. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> V6: >> - Change variable name 'state' -> 'index': >> https://lkml.org/lkml/2019/10/28/874 >> V4: >> - Add EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state) for the >> intel_powerclamp driver when this one is compiled as a module >> V3: >> - Add missing cpuidle.h header >> --- >> drivers/cpuidle/cpuidle.c | 1 + >> drivers/powercap/idle_inject.c | 4 +++- >> drivers/thermal/intel/intel_powerclamp.c | 4 +++- >> include/linux/cpu.h | 2 +- >> kernel/sched/idle.c | 4 ++-- >> 5 files changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >> index 18523ea6b11b..b871fc2e8e67 100644 >> --- a/drivers/cpuidle/cpuidle.c >> +++ b/drivers/cpuidle/cpuidle.c >> @@ -126,6 +126,7 @@ int cpuidle_find_deepest_state(void) >> >> return find_deepest_state(drv, dev, UINT_MAX, 0, false); >> } >> +EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state); > > That doesn't appear to be really necessary to me. > >> >> #ifdef CONFIG_SUSPEND >> static void enter_s2idle_proper(struct cpuidle_driver *drv, >> diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c >> index cd1270614cc6..233c878cbf46 100644 >> --- a/drivers/powercap/idle_inject.c >> +++ b/drivers/powercap/idle_inject.c >> @@ -38,6 +38,7 @@ >> #define pr_fmt(fmt) "ii_dev: " fmt >> >> #include <linux/cpu.h> >> +#include <linux/cpuidle.h> >> #include <linux/hrtimer.h> >> #include <linux/kthread.h> >> #include <linux/sched.h> >> @@ -138,7 +139,8 @@ static void idle_inject_fn(unsigned int cpu) >> */ >> iit->should_run = 0; >> >> - play_idle(READ_ONCE(ii_dev->idle_duration_us)); >> + play_idle(READ_ONCE(ii_dev->idle_duration_us), >> + cpuidle_find_deepest_state()); > > The next patch changes this again and I'm not sure why this intermediate > change is useful. > >> } >> >> /** >> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c >> index 53216dcbe173..b55786c169ae 100644 >> --- a/drivers/thermal/intel/intel_powerclamp.c >> +++ b/drivers/thermal/intel/intel_powerclamp.c >> @@ -29,6 +29,7 @@ >> #include <linux/delay.h> >> #include <linux/kthread.h> >> #include <linux/cpu.h> >> +#include <linux/cpuidle.h> >> #include <linux/thermal.h> >> #include <linux/slab.h> >> #include <linux/tick.h> >> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work) >> if (should_skip) >> goto balance; >> >> - play_idle(jiffies_to_usecs(w_data->duration_jiffies)); >> + play_idle(jiffies_to_usecs(w_data->duration_jiffies), >> + cpuidle_find_deepest_state()); > > I don't see a reason for changing the code here like this. > > What you really need is to have a way to set a limit on the idle > state exit latency for idle injection on ARM. Mmh, yes you are right. The idle state number is part of the internals of the cpuidle framework while the exit latency is an input (from user or kernel). > For that you can pass the exit latency limit to play_idle(), but then > you need to change powerclamp to pass UNIT_MAX or similar which is > ugly, or you can redefine cpuidle_use_deepest_state() to take the > exit latency limit as the arg (with 0 meaning use_deepest_state == false). Should it make sense to just get the resume latency in cpuidle_use_deepest_state() and pass the value to find_deepest_state()? It is the only code path where the constraint is not taken into account AFAICT. With this simple change, we can manage everything from the pm_qos API then and this series is no longer needed. > In the latter case, it would be quite straightforward to add an > exit_latency argument to cpuidle_find_deepest_state() and note that > find_deepest_state() takes a max_latency arg already, so that would be > a trivial change (hint!). -- <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
On Fri, Nov 8, 2019 at 11:47 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 08/11/2019 02:20, Rafael J. Wysocki wrote: > > On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote: > >> Currently, the play_idle function does not allow to tell which idle > >> state we want to go. Improve this by passing the idle state as > >> parameter to the function. > >> > >> Export cpuidle_find_deepest_state() symbol as it is used from the > >> intel_powerclamp driver as a module. > >> > >> There is no functional changes, the cpuidle state is the deepest one. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > >> --- > >> V6: > >> - Change variable name 'state' -> 'index': > >> https://lkml.org/lkml/2019/10/28/874 > >> V4: > >> - Add EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state) for the > >> intel_powerclamp driver when this one is compiled as a module > >> V3: > >> - Add missing cpuidle.h header > >> --- > >> drivers/cpuidle/cpuidle.c | 1 + > >> drivers/powercap/idle_inject.c | 4 +++- > >> drivers/thermal/intel/intel_powerclamp.c | 4 +++- > >> include/linux/cpu.h | 2 +- > >> kernel/sched/idle.c | 4 ++-- > >> 5 files changed, 10 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > >> index 18523ea6b11b..b871fc2e8e67 100644 > >> --- a/drivers/cpuidle/cpuidle.c > >> +++ b/drivers/cpuidle/cpuidle.c > >> @@ -126,6 +126,7 @@ int cpuidle_find_deepest_state(void) > >> > >> return find_deepest_state(drv, dev, UINT_MAX, 0, false); > >> } > >> +EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state); > > > > That doesn't appear to be really necessary to me. > > > >> > >> #ifdef CONFIG_SUSPEND > >> static void enter_s2idle_proper(struct cpuidle_driver *drv, > >> diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c > >> index cd1270614cc6..233c878cbf46 100644 > >> --- a/drivers/powercap/idle_inject.c > >> +++ b/drivers/powercap/idle_inject.c > >> @@ -38,6 +38,7 @@ > >> #define pr_fmt(fmt) "ii_dev: " fmt > >> > >> #include <linux/cpu.h> > >> +#include <linux/cpuidle.h> > >> #include <linux/hrtimer.h> > >> #include <linux/kthread.h> > >> #include <linux/sched.h> > >> @@ -138,7 +139,8 @@ static void idle_inject_fn(unsigned int cpu) > >> */ > >> iit->should_run = 0; > >> > >> - play_idle(READ_ONCE(ii_dev->idle_duration_us)); > >> + play_idle(READ_ONCE(ii_dev->idle_duration_us), > >> + cpuidle_find_deepest_state()); > > > > The next patch changes this again and I'm not sure why this intermediate > > change is useful. > > > >> } > >> > >> /** > >> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c > >> index 53216dcbe173..b55786c169ae 100644 > >> --- a/drivers/thermal/intel/intel_powerclamp.c > >> +++ b/drivers/thermal/intel/intel_powerclamp.c > >> @@ -29,6 +29,7 @@ > >> #include <linux/delay.h> > >> #include <linux/kthread.h> > >> #include <linux/cpu.h> > >> +#include <linux/cpuidle.h> > >> #include <linux/thermal.h> > >> #include <linux/slab.h> > >> #include <linux/tick.h> > >> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work) > >> if (should_skip) > >> goto balance; > >> > >> - play_idle(jiffies_to_usecs(w_data->duration_jiffies)); > >> + play_idle(jiffies_to_usecs(w_data->duration_jiffies), > >> + cpuidle_find_deepest_state()); > > > > I don't see a reason for changing the code here like this. > > > > What you really need is to have a way to set a limit on the idle > > state exit latency for idle injection on ARM. > > Mmh, yes you are right. The idle state number is part of the internals > of the cpuidle framework while the exit latency is an input (from user > or kernel). > > > For that you can pass the exit latency limit to play_idle(), but then > > you need to change powerclamp to pass UNIT_MAX or similar which is > > ugly, or you can redefine cpuidle_use_deepest_state() to take the > > exit latency limit as the arg (with 0 meaning use_deepest_state == false). > > Should it make sense to just get the resume latency in > cpuidle_use_deepest_state() and pass the value to find_deepest_state()? Yes, I would change cpuidle_use_deepest_state() to take the max exit latency as the arg (maybe with 0 meaning "don't use the deepest state only any more"). > It is the only code path where the constraint is not taken into account > AFAICT. > > With this simple change, we can manage everything from the pm_qos API > then and this series is no longer needed. OK
On 08/11/2019 11:56, Rafael J. Wysocki wrote: > On Fri, Nov 8, 2019 at 11:47 AM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 08/11/2019 02:20, Rafael J. Wysocki wrote: >>> On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote: >>>> Currently, the play_idle function does not allow to tell which idle >>>> state we want to go. Improve this by passing the idle state as >>>> parameter to the function. >>>> >>>> Export cpuidle_find_deepest_state() symbol as it is used from the >>>> intel_powerclamp driver as a module. >>>> >>>> There is no functional changes, the cpuidle state is the deepest one. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> >>>> --- >>>> V6: >>>> - Change variable name 'state' -> 'index': >>>> https://lkml.org/lkml/2019/10/28/874 >>>> V4: >>>> - Add EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state) for the >>>> intel_powerclamp driver when this one is compiled as a module >>>> V3: >>>> - Add missing cpuidle.h header >>>> --- >>>> drivers/cpuidle/cpuidle.c | 1 + >>>> drivers/powercap/idle_inject.c | 4 +++- >>>> drivers/thermal/intel/intel_powerclamp.c | 4 +++- >>>> include/linux/cpu.h | 2 +- >>>> kernel/sched/idle.c | 4 ++-- >>>> 5 files changed, 10 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >>>> index 18523ea6b11b..b871fc2e8e67 100644 >>>> --- a/drivers/cpuidle/cpuidle.c >>>> +++ b/drivers/cpuidle/cpuidle.c >>>> @@ -126,6 +126,7 @@ int cpuidle_find_deepest_state(void) >>>> >>>> return find_deepest_state(drv, dev, UINT_MAX, 0, false); >>>> } >>>> +EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state); >>> >>> That doesn't appear to be really necessary to me. >>> >>>> >>>> #ifdef CONFIG_SUSPEND >>>> static void enter_s2idle_proper(struct cpuidle_driver *drv, >>>> diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c >>>> index cd1270614cc6..233c878cbf46 100644 >>>> --- a/drivers/powercap/idle_inject.c >>>> +++ b/drivers/powercap/idle_inject.c >>>> @@ -38,6 +38,7 @@ >>>> #define pr_fmt(fmt) "ii_dev: " fmt >>>> >>>> #include <linux/cpu.h> >>>> +#include <linux/cpuidle.h> >>>> #include <linux/hrtimer.h> >>>> #include <linux/kthread.h> >>>> #include <linux/sched.h> >>>> @@ -138,7 +139,8 @@ static void idle_inject_fn(unsigned int cpu) >>>> */ >>>> iit->should_run = 0; >>>> >>>> - play_idle(READ_ONCE(ii_dev->idle_duration_us)); >>>> + play_idle(READ_ONCE(ii_dev->idle_duration_us), >>>> + cpuidle_find_deepest_state()); >>> >>> The next patch changes this again and I'm not sure why this intermediate >>> change is useful. >>> >>>> } >>>> >>>> /** >>>> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c >>>> index 53216dcbe173..b55786c169ae 100644 >>>> --- a/drivers/thermal/intel/intel_powerclamp.c >>>> +++ b/drivers/thermal/intel/intel_powerclamp.c >>>> @@ -29,6 +29,7 @@ >>>> #include <linux/delay.h> >>>> #include <linux/kthread.h> >>>> #include <linux/cpu.h> >>>> +#include <linux/cpuidle.h> >>>> #include <linux/thermal.h> >>>> #include <linux/slab.h> >>>> #include <linux/tick.h> >>>> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work) >>>> if (should_skip) >>>> goto balance; >>>> >>>> - play_idle(jiffies_to_usecs(w_data->duration_jiffies)); >>>> + play_idle(jiffies_to_usecs(w_data->duration_jiffies), >>>> + cpuidle_find_deepest_state()); >>> >>> I don't see a reason for changing the code here like this. >>> >>> What you really need is to have a way to set a limit on the idle >>> state exit latency for idle injection on ARM. >> >> Mmh, yes you are right. The idle state number is part of the internals >> of the cpuidle framework while the exit latency is an input (from user >> or kernel). >> >>> For that you can pass the exit latency limit to play_idle(), but then >>> you need to change powerclamp to pass UNIT_MAX or similar which is >>> ugly, or you can redefine cpuidle_use_deepest_state() to take the >>> exit latency limit as the arg (with 0 meaning use_deepest_state == false). >> >> Should it make sense to just get the resume latency in >> cpuidle_use_deepest_state() and pass the value to find_deepest_state()? > > Yes, I would change cpuidle_use_deepest_state() to take the max exit > latency as the arg (maybe with 0 meaning "don't use the deepest state > only any more"). Why not simply ? int cpuidle_find_deepest_state(struct cpuidle_driver *drv, struct cpuidle_device *dev) { int latency = cpuidle_governor_latency_req(dev->cpu); return find_deepest_state(drv, dev, latency_req, 0, false); } >> It is the only code path where the constraint is not taken into account >> AFAICT. >> >> With this simple change, we can manage everything from the pm_qos API >> then and this series is no longer needed. > > OK > -- <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
On Fri, Nov 8, 2019 at 2:33 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 08/11/2019 11:56, Rafael J. Wysocki wrote: > > On Fri, Nov 8, 2019 at 11:47 AM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> On 08/11/2019 02:20, Rafael J. Wysocki wrote: > >>> On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote: > >>>> Currently, the play_idle function does not allow to tell which idle > >>>> state we want to go. Improve this by passing the idle state as > >>>> parameter to the function. > >>>> > >>>> Export cpuidle_find_deepest_state() symbol as it is used from the > >>>> intel_powerclamp driver as a module. > >>>> > >>>> There is no functional changes, the cpuidle state is the deepest one. > >>>> > >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > >>>> --- > >>>> V6: > >>>> - Change variable name 'state' -> 'index': > >>>> https://lkml.org/lkml/2019/10/28/874 > >>>> V4: > >>>> - Add EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state) for the > >>>> intel_powerclamp driver when this one is compiled as a module > >>>> V3: > >>>> - Add missing cpuidle.h header > >>>> --- > >>>> drivers/cpuidle/cpuidle.c | 1 + > >>>> drivers/powercap/idle_inject.c | 4 +++- > >>>> drivers/thermal/intel/intel_powerclamp.c | 4 +++- > >>>> include/linux/cpu.h | 2 +- > >>>> kernel/sched/idle.c | 4 ++-- > >>>> 5 files changed, 10 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > >>>> index 18523ea6b11b..b871fc2e8e67 100644 > >>>> --- a/drivers/cpuidle/cpuidle.c > >>>> +++ b/drivers/cpuidle/cpuidle.c > >>>> @@ -126,6 +126,7 @@ int cpuidle_find_deepest_state(void) > >>>> > >>>> return find_deepest_state(drv, dev, UINT_MAX, 0, false); > >>>> } > >>>> +EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state); > >>> > >>> That doesn't appear to be really necessary to me. > >>> > >>>> > >>>> #ifdef CONFIG_SUSPEND > >>>> static void enter_s2idle_proper(struct cpuidle_driver *drv, > >>>> diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c > >>>> index cd1270614cc6..233c878cbf46 100644 > >>>> --- a/drivers/powercap/idle_inject.c > >>>> +++ b/drivers/powercap/idle_inject.c > >>>> @@ -38,6 +38,7 @@ > >>>> #define pr_fmt(fmt) "ii_dev: " fmt > >>>> > >>>> #include <linux/cpu.h> > >>>> +#include <linux/cpuidle.h> > >>>> #include <linux/hrtimer.h> > >>>> #include <linux/kthread.h> > >>>> #include <linux/sched.h> > >>>> @@ -138,7 +139,8 @@ static void idle_inject_fn(unsigned int cpu) > >>>> */ > >>>> iit->should_run = 0; > >>>> > >>>> - play_idle(READ_ONCE(ii_dev->idle_duration_us)); > >>>> + play_idle(READ_ONCE(ii_dev->idle_duration_us), > >>>> + cpuidle_find_deepest_state()); > >>> > >>> The next patch changes this again and I'm not sure why this intermediate > >>> change is useful. > >>> > >>>> } > >>>> > >>>> /** > >>>> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c > >>>> index 53216dcbe173..b55786c169ae 100644 > >>>> --- a/drivers/thermal/intel/intel_powerclamp.c > >>>> +++ b/drivers/thermal/intel/intel_powerclamp.c > >>>> @@ -29,6 +29,7 @@ > >>>> #include <linux/delay.h> > >>>> #include <linux/kthread.h> > >>>> #include <linux/cpu.h> > >>>> +#include <linux/cpuidle.h> > >>>> #include <linux/thermal.h> > >>>> #include <linux/slab.h> > >>>> #include <linux/tick.h> > >>>> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work) > >>>> if (should_skip) > >>>> goto balance; > >>>> > >>>> - play_idle(jiffies_to_usecs(w_data->duration_jiffies)); > >>>> + play_idle(jiffies_to_usecs(w_data->duration_jiffies), > >>>> + cpuidle_find_deepest_state()); > >>> > >>> I don't see a reason for changing the code here like this. > >>> > >>> What you really need is to have a way to set a limit on the idle > >>> state exit latency for idle injection on ARM. > >> > >> Mmh, yes you are right. The idle state number is part of the internals > >> of the cpuidle framework while the exit latency is an input (from user > >> or kernel). > >> > >>> For that you can pass the exit latency limit to play_idle(), but then > >>> you need to change powerclamp to pass UNIT_MAX or similar which is > >>> ugly, or you can redefine cpuidle_use_deepest_state() to take the > >>> exit latency limit as the arg (with 0 meaning use_deepest_state == false). > >> > >> Should it make sense to just get the resume latency in > >> cpuidle_use_deepest_state() and pass the value to find_deepest_state()? > > > > Yes, I would change cpuidle_use_deepest_state() to take the max exit > > latency as the arg (maybe with 0 meaning "don't use the deepest state > > only any more"). > > Why not simply ? > > int cpuidle_find_deepest_state(struct cpuidle_driver *drv, > struct cpuidle_device *dev) > { > int latency = cpuidle_governor_latency_req(dev->cpu); > > return find_deepest_state(drv, dev, latency_req, 0, false); > } Because, AFAICS, that doesn't work for powerclamp.
On 08/11/2019 02:20, Rafael J. Wysocki wrote: > On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote: >> Currently, the play_idle function does not allow to tell which idle >> state we want to go. Improve this by passing the idle state as >> parameter to the function. >> >> Export cpuidle_find_deepest_state() symbol as it is used from the >> intel_powerclamp driver as a module. [ ... ] >> /** >> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c >> index 53216dcbe173..b55786c169ae 100644 >> --- a/drivers/thermal/intel/intel_powerclamp.c >> +++ b/drivers/thermal/intel/intel_powerclamp.c >> @@ -29,6 +29,7 @@ >> #include <linux/delay.h> >> #include <linux/kthread.h> >> #include <linux/cpu.h> >> +#include <linux/cpuidle.h> >> #include <linux/thermal.h> >> #include <linux/slab.h> >> #include <linux/tick.h> >> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work) >> if (should_skip) >> goto balance; >> >> - play_idle(jiffies_to_usecs(w_data->duration_jiffies)); >> + play_idle(jiffies_to_usecs(w_data->duration_jiffies), >> + cpuidle_find_deepest_state()); > > I don't see a reason for changing the code here like this. > > What you really need is to have a way to set a limit on the idle > state exit latency for idle injection on ARM. > > For that you can pass the exit latency limit to play_idle(), but then > you need to change powerclamp to pass UNIT_MAX or similar which is > ugly, or you can redefine cpuidle_use_deepest_state() to take the > exit latency limit as the arg (with 0 meaning use_deepest_state == false). I'm confused with the "... ugly, *or* ...". In any case we have to specify a latency constraint to play_idle(), no? > In the latter case, it would be quite straightforward to add an > exit_latency argument to cpuidle_find_deepest_state() and note that > find_deepest_state() takes a max_latency arg already, so that would be > a trivial change (hint!). -- <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
On 05/12/2019 18:04, Martin Kepplinger wrote: > hi Daniel, > > Since there's been quite some changes in cpuidle recently, how's your plans > to move this patchset forward? I, at least, need it. This series was merged but instead of specifying an idle state, we specify an exit latency [1]. The cooling device itself is at V4 with some review tags. It should be merged soon [2]. -- Daniel [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c55b51a06b01d67a99457bb82a8c31081c7faa23 [2] https://lkml.org/lkml/2019/12/4/563 -- <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
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 18523ea6b11b..b871fc2e8e67 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -126,6 +126,7 @@ int cpuidle_find_deepest_state(void) return find_deepest_state(drv, dev, UINT_MAX, 0, false); } +EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state); #ifdef CONFIG_SUSPEND static void enter_s2idle_proper(struct cpuidle_driver *drv, diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c index cd1270614cc6..233c878cbf46 100644 --- a/drivers/powercap/idle_inject.c +++ b/drivers/powercap/idle_inject.c @@ -38,6 +38,7 @@ #define pr_fmt(fmt) "ii_dev: " fmt #include <linux/cpu.h> +#include <linux/cpuidle.h> #include <linux/hrtimer.h> #include <linux/kthread.h> #include <linux/sched.h> @@ -138,7 +139,8 @@ static void idle_inject_fn(unsigned int cpu) */ iit->should_run = 0; - play_idle(READ_ONCE(ii_dev->idle_duration_us)); + play_idle(READ_ONCE(ii_dev->idle_duration_us), + cpuidle_find_deepest_state()); } /** diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c index 53216dcbe173..b55786c169ae 100644 --- a/drivers/thermal/intel/intel_powerclamp.c +++ b/drivers/thermal/intel/intel_powerclamp.c @@ -29,6 +29,7 @@ #include <linux/delay.h> #include <linux/kthread.h> #include <linux/cpu.h> +#include <linux/cpuidle.h> #include <linux/thermal.h> #include <linux/slab.h> #include <linux/tick.h> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work) if (should_skip) goto balance; - play_idle(jiffies_to_usecs(w_data->duration_jiffies)); + play_idle(jiffies_to_usecs(w_data->duration_jiffies), + cpuidle_find_deepest_state()); balance: if (clamping && w_data->clamping && cpu_online(w_data->cpu)) diff --git a/include/linux/cpu.h b/include/linux/cpu.h index d0633ebdaa9c..6f5df0b3641a 100644 --- a/include/linux/cpu.h +++ b/include/linux/cpu.h @@ -179,7 +179,7 @@ void arch_cpu_idle_dead(void); int cpu_report_state(int cpu); int cpu_check_up_prepare(int cpu); void cpu_set_state_online(int cpu); -void play_idle(unsigned long duration_us); +void play_idle(unsigned long duration_us, int index); #ifdef CONFIG_HOTPLUG_CPU bool cpu_wait_death(unsigned int cpu, int seconds); diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c index fb9fc93f1497..eb9e93b37a97 100644 --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -314,7 +314,7 @@ static enum hrtimer_restart idle_inject_timer_fn(struct hrtimer *timer) return HRTIMER_NORESTART; } -void play_idle(unsigned long duration_us) +void play_idle(unsigned long duration_us, int index) { struct idle_timer it; @@ -331,7 +331,7 @@ void play_idle(unsigned long duration_us) rcu_sleep_check(); preempt_disable(); current->flags |= PF_IDLE; - cpuidle_use_state(cpuidle_find_deepest_state()); + cpuidle_use_state(index); it.done = 0; hrtimer_init_on_stack(&it.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);