* Re: [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function
@ 2020-09-17 11:15 zhuguangqing83
2020-09-17 12:33 ` Daniel Lezcano
0 siblings, 1 reply; 6+ messages in thread
From: zhuguangqing83 @ 2020-09-17 11:15 UTC (permalink / raw)
To: 'Daniel Lezcano',
amit.kachhap, viresh.kumar, javi.merino, rui.zhang, amitk,
zhuguangqing
Cc: linux-pm, linux-kernel
> > From: zhuguangqing <zhuguangqing@xiaomi.com>
> >
> > In the function cpuidle_cooling_set_cur_state(), if current_state is
> > not equal to state and both current_state and state are greater than
> > 0(scene 4 as follows), then maybe it should stop->start or restart
> > idle_inject.
>
> Sorry, I don't get it.
>
> It is an update of the state, why do we need to restart the idle
> injection ? The state change will be automatically take into account by
> the idle injection code at the new injection cycle.
>
Thanks for your comments.
For example, we call cpuidle_cooling_set_cur_state() twice, first time
the input parameter state is 20, second time the input parameter state
is 30.
In current code, in the second call of cpuidle_cooling_set_cur_state(),
current_state == 20, state == 30, then "if (current_state == 0 &&
state > 0)" is not satisfied, "else if (current_state > 0 && !state)"
is not satisfied either, so we just update idle_cdev->state to 30 and
idle_inject_set_duration to new injection cycle，but we do not call
idle injection code.
In the example mentioned above, we should call idle injection code. If
idle_inject_start() takes into account by the idle injection code at
the new injection cycle, then just calling idle_inject_start() is ok.
Otherwise, we need a restart or stop->start process to execute idle
injection code at the new state 30.
> > The scenes changed is as follows,
> >
> > scene current_state state action
> > 1 0 >0 start
> > 2 0 0 do nothing
> > 3 >0 0 stop
> > 4 >0 && !=state >0 stop->start or restart
> > 5 >0 && ==state >0 do nothing
> >
> > Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
> > ---
> > drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thermal/cpuidle_cooling.c
> b/drivers/thermal/cpuidle_cooling.c
> > index 78e3e8238116..868919ad3dda 100644
> > --- a/drivers/thermal/cpuidle_cooling.c
> > +++ b/drivers/thermal/cpuidle_cooling.c
> > @@ -113,7 +113,7 @@ static int cpuidle_cooling_get_cur_state(struct
> > thermal_cooling_device *cdev,
> > /**
> > * cpuidle_cooling_set_cur_state - Set the current cooling state
> > * @cdev: the thermal cooling device
> > - * @state: the target state
> > + * @state: the target state, max value is 100
> > *
> > * The function checks first if we are initiating the mitigation which
> > * in turn wakes up all the idle injection tasks belonging to the idle
> > @@ -130,6 +130,9 @@ static int cpuidle_cooling_set_cur_state(struct
> > thermal_cooling_device *cdev,
> > unsigned long current_state = idle_cdev->state;
> > unsigned int runtime_us, idle_duration_us;
> >
> > + if (state > 100 || current_state == state)
> > + return 0;
> > +
> > idle_cdev->state = state;
> >
> > idle_inject_get_duration(ii_dev, &runtime_us, &idle_duration_us);
> > @@ -140,8 +143,11 @@ static int cpuidle_cooling_set_cur_state(struct
> > thermal_cooling_device *cdev,
> >
> > if (current_state == 0 && state > 0) {
> > idle_inject_start(ii_dev);
> > - } else if (current_state > 0 && !state) {
> > + } else if (current_state > 0 && !state) {
> > idle_inject_stop(ii_dev);
> > + } else {
> > + idle_inject_stop(ii_dev);
> > + idle_inject_start(ii_dev);
> > }
> >
> > return 0;
> >
>
>
> --
> <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] 6+ messages in thread* Re: [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function
2020-09-17 11:15 [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function zhuguangqing83
@ 2020-09-17 12:33 ` Daniel Lezcano
2020-09-17 13:22 ` Daniel Lezcano
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Lezcano @ 2020-09-17 12:33 UTC (permalink / raw)
To: zhuguangqing83, amit.kachhap, viresh.kumar, javi.merino,
rui.zhang, amitk, zhuguangqing
Cc: linux-pm, linux-kernel
On 17/09/2020 13:15, zhuguangqing83 wrote:
>
>>> From: zhuguangqing <zhuguangqing@xiaomi.com>
>>>
>>> In the function cpuidle_cooling_set_cur_state(), if current_state is
>>> not equal to state and both current_state and state are greater than
>>> 0(scene 4 as follows), then maybe it should stop->start or restart
>>> idle_inject.
>>
>> Sorry, I don't get it.
>>
>> It is an update of the state, why do we need to restart the idle
>> injection ? The state change will be automatically take into account by
>> the idle injection code at the new injection cycle.
>>
>
> Thanks for your comments.
>
> For example, we call cpuidle_cooling_set_cur_state() twice, first time
> the input parameter state is 20, second time the input parameter state
> is 30.
>
> In current code, in the second call of cpuidle_cooling_set_cur_state(),
> current_state == 20, state == 30, then "if (current_state == 0 &&
> state > 0)" is not satisfied, "else if (current_state > 0 && !state)"
> is not satisfied either, so we just update idle_cdev->state to 30 and
> idle_inject_set_duration to new injection cycle，but we do not call
> idle injection code.
Ok, I think understand your question.
When the idle injection is started, a timer is periodically calling the
function play_idle_precise() with the idle duration. This one is updated
by idle_inject_set_duration().
So when the state is changed, that changes the idle duration. At the
next timer expiration, a few Milli seconds after, play_idle_precise()
will be called with the new idle duration which was updated by
idle_inject_set_duration().
There is no need to stop and start the idle injection at each update.
The new value is take into account automatically for the next cycle.
It does not really matter if the update is delayed. Restarting the idle
injection at each update will be worth in the cooling context than
waiting an idle cycle.
> In the example mentioned above, we should call idle injection code. If
> idle_inject_start() takes into account by the idle injection code at
> the new injection cycle, then just calling idle_inject_start() is ok.
> Otherwise, we need a restart or stop->start process to execute idle
> injection code at the new state 30.
>
>>> The scenes changed is as follows,
>>>
>>> scene current_state state action
>>> 1 0 >0 start
>>> 2 0 0 do nothing
>>> 3 >0 0 stop
>>> 4 >0 && !=state >0 stop->start or restart
>>> 5 >0 && ==state >0 do nothing
>>>
>>> Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
>>> ---
>>> drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/thermal/cpuidle_cooling.c
>> b/drivers/thermal/cpuidle_cooling.c
>>> index 78e3e8238116..868919ad3dda 100644
>>> --- a/drivers/thermal/cpuidle_cooling.c
>>> +++ b/drivers/thermal/cpuidle_cooling.c
>>> @@ -113,7 +113,7 @@ static int cpuidle_cooling_get_cur_state(struct
>>> thermal_cooling_device *cdev,
>>> /**
>>> * cpuidle_cooling_set_cur_state - Set the current cooling state
>>> * @cdev: the thermal cooling device
>>> - * @state: the target state
>>> + * @state: the target state, max value is 100
>>> *
>>> * The function checks first if we are initiating the mitigation which
>>> * in turn wakes up all the idle injection tasks belonging to the idle
>>> @@ -130,6 +130,9 @@ static int cpuidle_cooling_set_cur_state(struct
>>> thermal_cooling_device *cdev,
>>> unsigned long current_state = idle_cdev->state;
>>> unsigned int runtime_us, idle_duration_us;
>>>
>>> + if (state > 100 || current_state == state)
>>> + return 0;
>>> +
>>> idle_cdev->state = state;
>>>
>>> idle_inject_get_duration(ii_dev, &runtime_us, &idle_duration_us);
>>> @@ -140,8 +143,11 @@ static int cpuidle_cooling_set_cur_state(struct
>>> thermal_cooling_device *cdev,
>>>
>>> if (current_state == 0 && state > 0) {
>>> idle_inject_start(ii_dev);
>>> - } else if (current_state > 0 && !state) {
>>> + } else if (current_state > 0 && !state) {
>>> idle_inject_stop(ii_dev);
>>> + } else {
>>> + idle_inject_stop(ii_dev);
>>> + idle_inject_start(ii_dev);
>>> }
>>>
>>> return 0;
>>>
>>
>>
>> --
>> <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
>
--
<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] 6+ messages in thread
* Re: [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function
@ 2020-09-17 12:52 zhuguangqing83
0 siblings, 0 replies; 6+ messages in thread
From: zhuguangqing83 @ 2020-09-17 12:52 UTC (permalink / raw)
To: 'Daniel Lezcano',
amit.kachhap, viresh.kumar, javi.merino, rui.zhang, amitk,
zhuguangqing
Cc: linux-pm, linux-kernel
>
> On 17/09/2020 13:15, zhuguangqing83 wrote:
> >
> >>> From: zhuguangqing <zhuguangqing@xiaomi.com>
> >>>
> >>> In the function cpuidle_cooling_set_cur_state(), if current_state is
> >>> not equal to state and both current_state and state are greater than
> >>> 0(scene 4 as follows), then maybe it should stop->start or restart
> >>> idle_inject.
> >>
> >> Sorry, I don't get it.
> >>
> >> It is an update of the state, why do we need to restart the idle
> >> injection ? The state change will be automatically take into account by
> >> the idle injection code at the new injection cycle.
> >>
> >
> > Thanks for your comments.
> >
> > For example, we call cpuidle_cooling_set_cur_state() twice, first time
> > the input parameter state is 20, second time the input parameter state
> > is 30.
> >
> > In current code, in the second call of cpuidle_cooling_set_cur_state(),
> > current_state == 20, state == 30, then "if (current_state == 0 &&
> > state > 0)" is not satisfied, "else if (current_state > 0 && !state)"
> > is not satisfied either, so we just update idle_cdev->state to 30 and
> > idle_inject_set_duration to new injection cycle，but we do not call
> > idle injection code.
>
> Ok, I think understand your question.
>
> When the idle injection is started, a timer is periodically calling the
> function play_idle_precise() with the idle duration. This one is updated
> by idle_inject_set_duration().
>
> So when the state is changed, that changes the idle duration. At the
> next timer expiration, a few Milli seconds after, play_idle_precise()
> will be called with the new idle duration which was updated by
> idle_inject_set_duration().
>
> There is no need to stop and start the idle injection at each update.
>
> The new value is take into account automatically for the next cycle.
>
> It does not really matter if the update is delayed. Restarting the idle
> injection at each update will be worth in the cooling context than
> waiting an idle cycle.
>
Ok, got it. Thanks.
> > In the example mentioned above, we should call idle injection code. If
> > idle_inject_start() takes into account by the idle injection code at
> > the new injection cycle, then just calling idle_inject_start() is ok.
> > Otherwise, we need a restart or stop->start process to execute idle
> > injection code at the new state 30.
> >
> >>> The scenes changed is as follows,
> >>>
> >>> scene current_state state action
> >>> 1 0 >0 start
> >>> 2 0 0 do nothing
> >>> 3 >0 0 stop
> >>> 4 >0 && !=state >0 stop->start or restart
> >>> 5 >0 && ==state >0 do nothing
> >>>
> >>> Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
> >>> ---
> >>> drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
> >>> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/cpuidle_cooling.c
> >> b/drivers/thermal/cpuidle_cooling.c
> >>> index 78e3e8238116..868919ad3dda 100644
> >>> --- a/drivers/thermal/cpuidle_cooling.c
> >>> +++ b/drivers/thermal/cpuidle_cooling.c
> >>> @@ -113,7 +113,7 @@ static int cpuidle_cooling_get_cur_state(struct
> >>> thermal_cooling_device *cdev,
> >>> /**
> >>> * cpuidle_cooling_set_cur_state - Set the current cooling state
> >>> * @cdev: the thermal cooling device
> >>> - * @state: the target state
> >>> + * @state: the target state, max value is 100
> >>> *
> >>> * The function checks first if we are initiating the mitigation which
> >>> * in turn wakes up all the idle injection tasks belonging to the idle
> >>> @@ -130,6 +130,9 @@ static int cpuidle_cooling_set_cur_state(struct
> >>> thermal_cooling_device *cdev,
> >>> unsigned long current_state = idle_cdev->state;
> >>> unsigned int runtime_us, idle_duration_us;
> >>>
> >>> + if (state > 100 || current_state == state)
> >>> + return 0;
> >>> +
> >>> idle_cdev->state = state;
> >>>
> >>> idle_inject_get_duration(ii_dev, &runtime_us, &idle_duration_us);
> >>> @@ -140,8 +143,11 @@ static int cpuidle_cooling_set_cur_state(struct
> >>> thermal_cooling_device *cdev,
> >>>
> >>> if (current_state == 0 && state > 0) {
> >>> idle_inject_start(ii_dev);
> >>> - } else if (current_state > 0 && !state) {
> >>> + } else if (current_state > 0 && !state) {
> >>> idle_inject_stop(ii_dev);
> >>> + } else {
> >>> + idle_inject_stop(ii_dev);
> >>> + idle_inject_start(ii_dev);
> >>> }
> >>>
> >>> return 0;
> >>>
> >>
> >>
> >> --
> >> <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
> >
>
>
> --
> <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] 6+ messages in thread* [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function
@ 2020-09-17 6:00 zhuguangqing83
2020-09-17 7:05 ` Daniel Lezcano
0 siblings, 1 reply; 6+ messages in thread
From: zhuguangqing83 @ 2020-09-17 6:00 UTC (permalink / raw)
To: amit.kachhap, daniel.lezcano, viresh.kumar, javi.merino,
rui.zhang, amitk, zhuguangqing
Cc: linux-pm, linux-kernel
From: zhuguangqing <zhuguangqing@xiaomi.com>
In the function cpuidle_cooling_set_cur_state(), if current_state is
not equal to state and both current_state and state are greater than
0(scene 4 as follows), then maybe it should stop->start or restart
idle_inject.
The scenes changed is as follows,
scene current_state state action
1 0 >0 start
2 0 0 do nothing
3 >0 0 stop
4 >0 && !=state >0 stop->start or restart
5 >0 && ==state >0 do nothing
Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
---
drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/cpuidle_cooling.c b/drivers/thermal/cpuidle_cooling.c
index 78e3e8238116..868919ad3dda 100644
--- a/drivers/thermal/cpuidle_cooling.c
+++ b/drivers/thermal/cpuidle_cooling.c
@@ -113,7 +113,7 @@ static int cpuidle_cooling_get_cur_state(struct thermal_cooling_device *cdev,
/**
* cpuidle_cooling_set_cur_state - Set the current cooling state
* @cdev: the thermal cooling device
- * @state: the target state
+ * @state: the target state, max value is 100
*
* The function checks first if we are initiating the mitigation which
* in turn wakes up all the idle injection tasks belonging to the idle
@@ -130,6 +130,9 @@ static int cpuidle_cooling_set_cur_state(struct thermal_cooling_device *cdev,
unsigned long current_state = idle_cdev->state;
unsigned int runtime_us, idle_duration_us;
+ if (state > 100 || current_state == state)
+ return 0;
+
idle_cdev->state = state;
idle_inject_get_duration(ii_dev, &runtime_us, &idle_duration_us);
@@ -140,8 +143,11 @@ static int cpuidle_cooling_set_cur_state(struct thermal_cooling_device *cdev,
if (current_state == 0 && state > 0) {
idle_inject_start(ii_dev);
- } else if (current_state > 0 && !state) {
+ } else if (current_state > 0 && !state) {
idle_inject_stop(ii_dev);
+ } else {
+ idle_inject_stop(ii_dev);
+ idle_inject_start(ii_dev);
}
return 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function
2020-09-17 6:00 zhuguangqing83
@ 2020-09-17 7:05 ` Daniel Lezcano
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Lezcano @ 2020-09-17 7:05 UTC (permalink / raw)
To: zhuguangqing83, amit.kachhap, viresh.kumar, javi.merino,
rui.zhang, amitk, zhuguangqing
Cc: linux-pm, linux-kernel
On 17/09/2020 08:00, zhuguangqing83@gmail.com wrote:
> From: zhuguangqing <zhuguangqing@xiaomi.com>
>
> In the function cpuidle_cooling_set_cur_state(), if current_state is
> not equal to state and both current_state and state are greater than
> 0(scene 4 as follows), then maybe it should stop->start or restart
> idle_inject.
Sorry, I don't get it.
It is an update of the state, why do we need to restart the idle
injection ? The state change will be automatically take into account by
the idle injection code at the new injection cycle.
> The scenes changed is as follows,
>
> scene current_state state action
> 1 0 >0 start
> 2 0 0 do nothing
> 3 >0 0 stop
> 4 >0 && !=state >0 stop->start or restart
> 5 >0 && ==state >0 do nothing
>
> Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
> ---
> drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/cpuidle_cooling.c b/drivers/thermal/cpuidle_cooling.c
> index 78e3e8238116..868919ad3dda 100644
> --- a/drivers/thermal/cpuidle_cooling.c
> +++ b/drivers/thermal/cpuidle_cooling.c
> @@ -113,7 +113,7 @@ static int cpuidle_cooling_get_cur_state(struct thermal_cooling_device *cdev,
> /**
> * cpuidle_cooling_set_cur_state - Set the current cooling state
> * @cdev: the thermal cooling device
> - * @state: the target state
> + * @state: the target state, max value is 100
> *
> * The function checks first if we are initiating the mitigation which
> * in turn wakes up all the idle injection tasks belonging to the idle
> @@ -130,6 +130,9 @@ static int cpuidle_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> unsigned long current_state = idle_cdev->state;
> unsigned int runtime_us, idle_duration_us;
>
> + if (state > 100 || current_state == state)
> + return 0;
> +
> idle_cdev->state = state;
>
> idle_inject_get_duration(ii_dev, &runtime_us, &idle_duration_us);
> @@ -140,8 +143,11 @@ static int cpuidle_cooling_set_cur_state(struct thermal_cooling_device *cdev,
>
> if (current_state == 0 && state > 0) {
> idle_inject_start(ii_dev);
> - } else if (current_state > 0 && !state) {
> + } else if (current_state > 0 && !state) {
> idle_inject_stop(ii_dev);
> + } else {
> + idle_inject_stop(ii_dev);
> + idle_inject_start(ii_dev);
> }
>
> return 0;
>
--
<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] 6+ messages in thread
end of thread, other threads:[~2020-09-17 13:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 11:15 [PATCH] thermal/drivers/cpuidle_cooling: Change the set_cur_state function zhuguangqing83
2020-09-17 12:33 ` Daniel Lezcano
2020-09-17 13:22 ` Daniel Lezcano
-- strict thread matches above, loose matches on Subject: below --
2020-09-17 12:52 zhuguangqing83
2020-09-17 6:00 zhuguangqing83
2020-09-17 7:05 ` 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).