| Message ID | 20190909145015.26317-3-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/3] cpuidle: play_idle: Make play_idle more flexible | expand |
On Mon, Sep 09, 2019 at 04:50:15PM +0200, Daniel Lezcano wrote: > Currently the idle injection framework only allows to inject the > deepest idle state available on the system. > > Give the opportunity to specify which idle state we want to inject by > adding a new function helper to set the state and use it when calling > play_idle(). > > There is no functional changes, the cpuidle state is the deepest one. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/powercap/idle_inject.c | 15 ++++++++++++++- > include/linux/idle_inject.h | 3 +++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c > index 9b18667b9f26..a612c425d74c 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> > @@ -65,6 +66,7 @@ struct idle_inject_thread { > */ > struct idle_inject_device { > struct hrtimer timer; > + int state; > unsigned int idle_duration_us; > unsigned int run_duration_us; > unsigned long int cpumask[0]; > @@ -139,7 +141,7 @@ static void idle_inject_fn(unsigned int cpu) > iit->should_run = 0; > > play_idle(READ_ONCE(ii_dev->idle_duration_us), > - cpuidle_find_deepest_state()); > + READ_ONCE(ii_dev->state)); > } > > /** > @@ -170,6 +172,16 @@ void idle_inject_get_duration(struct idle_inject_device *ii_dev, > *idle_duration_us = READ_ONCE(ii_dev->idle_duration_us); > } > > +/** > + * idle_inject_set_state - set the idle state to inject > + * @state: an integer for the idle state to inject > + */ > +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state) > +{ > + if (state >= CPUIDLE_STATE_NOUSE && state < CPUIDLE_STATE_MAX) > + WRITE_ONCE(ii_dev->state, state); > +} > + > /** > * idle_inject_start - start idle injections > * @ii_dev: idle injection control device structure > @@ -298,6 +310,7 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) > cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask); > hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > ii_dev->timer.function = idle_inject_timer_fn; > + ii_dev->state = cpuidle_find_deepest_state(); > > for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { > > diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h > index a445cd1a36c5..e2b26b9ccd34 100644 > --- a/include/linux/idle_inject.h > +++ b/include/linux/idle_inject.h > @@ -26,4 +26,7 @@ void idle_inject_set_duration(struct idle_inject_device *ii_dev, > void idle_inject_get_duration(struct idle_inject_device *ii_dev, > unsigned int *run_duration_us, > unsigned int *idle_duration_us); > + > +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state); > + The above function is not used in this patch and as such should be introduce as part of future work. Otherwise I agree that this patch does not carry any functional changes. Without function idle_inject_set_state(): Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> > #endif /* __IDLE_INJECT_H__ */ > -- > 2.17.1 >
Hi Mathieu, On 18/09/2019 21:35, Mathieu Poirier wrote: > On Mon, Sep 09, 2019 at 04:50:15PM +0200, Daniel Lezcano wrote: >> Currently the idle injection framework only allows to inject the >> deepest idle state available on the system. >> >> Give the opportunity to specify which idle state we want to inject by >> adding a new function helper to set the state and use it when calling >> play_idle(). >> >> There is no functional changes, the cpuidle state is the deepest one. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- [ ... ] >> +/** >> + * idle_inject_set_state - set the idle state to inject >> + * @state: an integer for the idle state to inject >> + */ >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state) >> +{ >> + if (state >= CPUIDLE_STATE_NOUSE && state < CPUIDLE_STATE_MAX) >> + WRITE_ONCE(ii_dev->state, state); >> +} >> + >> /** >> * idle_inject_start - start idle injections >> * @ii_dev: idle injection control device structure >> @@ -298,6 +310,7 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) >> cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask); >> hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); >> ii_dev->timer.function = idle_inject_timer_fn; >> + ii_dev->state = cpuidle_find_deepest_state(); >> >> for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { >> >> diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h >> index a445cd1a36c5..e2b26b9ccd34 100644 >> --- a/include/linux/idle_inject.h >> +++ b/include/linux/idle_inject.h >> @@ -26,4 +26,7 @@ void idle_inject_set_duration(struct idle_inject_device *ii_dev, >> void idle_inject_get_duration(struct idle_inject_device *ii_dev, >> unsigned int *run_duration_us, >> unsigned int *idle_duration_us); >> + >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state); >> + > > The above function is not used in this patch and as such should be introduce as > part of future work. Otherwise I agree that this patch does not carry any > functional changes. > > Without function idle_inject_set_state(): I was about to remove the function but actually it may not make sense as the idle_inject is a framework providing the different API to do the idle injection and the function is an helper to set the state value. It comes with the addition of the state number in the structure. Next patch is the idle cooling device and makes use of it. Can I still consider your acked-by valid?
On Tue, 24 Sep 2019 at 07:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > Hi Mathieu, > > On 18/09/2019 21:35, Mathieu Poirier wrote: > > On Mon, Sep 09, 2019 at 04:50:15PM +0200, Daniel Lezcano wrote: > >> Currently the idle injection framework only allows to inject the > >> deepest idle state available on the system. > >> > >> Give the opportunity to specify which idle state we want to inject by > >> adding a new function helper to set the state and use it when calling > >> play_idle(). > >> > >> There is no functional changes, the cpuidle state is the deepest one. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> --- > > [ ... ] > > >> +/** > >> + * idle_inject_set_state - set the idle state to inject > >> + * @state: an integer for the idle state to inject > >> + */ > >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state) > >> +{ > >> + if (state >= CPUIDLE_STATE_NOUSE && state < CPUIDLE_STATE_MAX) > >> + WRITE_ONCE(ii_dev->state, state); > >> +} > >> + > >> /** > >> * idle_inject_start - start idle injections > >> * @ii_dev: idle injection control device structure > >> @@ -298,6 +310,7 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) > >> cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask); > >> hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > >> ii_dev->timer.function = idle_inject_timer_fn; > >> + ii_dev->state = cpuidle_find_deepest_state(); > >> > >> for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { > >> > >> diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h > >> index a445cd1a36c5..e2b26b9ccd34 100644 > >> --- a/include/linux/idle_inject.h > >> +++ b/include/linux/idle_inject.h > >> @@ -26,4 +26,7 @@ void idle_inject_set_duration(struct idle_inject_device *ii_dev, > >> void idle_inject_get_duration(struct idle_inject_device *ii_dev, > >> unsigned int *run_duration_us, > >> unsigned int *idle_duration_us); > >> + > >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state); > >> + > > > > The above function is not used in this patch and as such should be introduce as > > part of future work. Otherwise I agree that this patch does not carry any > > functional changes. > > > > Without function idle_inject_set_state(): > > I was about to remove the function but actually it may not make sense as > the idle_inject is a framework providing the different API to do the > idle injection and the function is an helper to set the state value. It > comes with the addition of the state number in the structure. I agree with you but there is still no current user of the API. > > Next patch is the idle cooling device and makes use of it. Then why not simply introduce it then? > > Can I still consider your acked-by valid? Whether the API is introduced here or in the next patchset has no effect on the validity of the code and as such my approbation is still valid. Thanks, Mathieu > > -- > <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/powercap/idle_inject.c b/drivers/powercap/idle_inject.c index 9b18667b9f26..a612c425d74c 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> @@ -65,6 +66,7 @@ struct idle_inject_thread { */ struct idle_inject_device { struct hrtimer timer; + int state; unsigned int idle_duration_us; unsigned int run_duration_us; unsigned long int cpumask[0]; @@ -139,7 +141,7 @@ static void idle_inject_fn(unsigned int cpu) iit->should_run = 0; play_idle(READ_ONCE(ii_dev->idle_duration_us), - cpuidle_find_deepest_state()); + READ_ONCE(ii_dev->state)); } /** @@ -170,6 +172,16 @@ void idle_inject_get_duration(struct idle_inject_device *ii_dev, *idle_duration_us = READ_ONCE(ii_dev->idle_duration_us); } +/** + * idle_inject_set_state - set the idle state to inject + * @state: an integer for the idle state to inject + */ +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state) +{ + if (state >= CPUIDLE_STATE_NOUSE && state < CPUIDLE_STATE_MAX) + WRITE_ONCE(ii_dev->state, state); +} + /** * idle_inject_start - start idle injections * @ii_dev: idle injection control device structure @@ -298,6 +310,7 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask); hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); ii_dev->timer.function = idle_inject_timer_fn; + ii_dev->state = cpuidle_find_deepest_state(); for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h index a445cd1a36c5..e2b26b9ccd34 100644 --- a/include/linux/idle_inject.h +++ b/include/linux/idle_inject.h @@ -26,4 +26,7 @@ void idle_inject_set_duration(struct idle_inject_device *ii_dev, void idle_inject_get_duration(struct idle_inject_device *ii_dev, unsigned int *run_duration_us, unsigned int *idle_duration_us); + +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state); + #endif /* __IDLE_INJECT_H__ */
Currently the idle injection framework only allows to inject the deepest idle state available on the system. Give the opportunity to specify which idle state we want to inject by adding a new function helper to set the state and use it when calling play_idle(). There is no functional changes, the cpuidle state is the deepest one. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/powercap/idle_inject.c | 15 ++++++++++++++- include/linux/idle_inject.h | 3 +++ 2 files changed, 17 insertions(+), 1 deletion(-)