thermal/drivers/cpuidle_cooling: Change the set_cur_state function

PATCH zhuguangqing83 2 participants 3 messages Sep 17, 2020

Share & tools

zhuguangqing83

author

2020-09-17 11:19 UTC #0 lore raw

Quoted 8 lines

Quoted 7 lines
From: zhuguangqing <[email protected]>
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.

Quoted 6 lines

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.

Quoted 63 lines

Quoted 9 lines

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

Review

Quoted 6 lines

Signed-off-by zhuguangqing <[email protected]>
---
 drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/thermal/cpuidle_cooling.c

Quoted 37 lines

index 78e3e8238116..868919ad3dda 100644

drivers/thermal/cpuidle_cooling.c +1 -1

@@ -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

drivers/thermal/cpuidle_cooling.c +3 -0

@@ -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;

drivers/thermal/cpuidle_cooling.c +4 -1

 	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);
 	}

Quoted 2 lines

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

Daniel Lezcano

2020-09-17 12:33 UTC #1 lore raw

Quoted 21 lines · On 17/09/2020 13:15, zhuguangqing83 wrote:

Quoted 8 lines
Quoted 7 lines
From: zhuguangqing <[email protected]>
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.
Quoted 6 lines
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.

Quoted 6 lines

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.

Quoted 70 lines

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.
Quoted 63 lines
Quoted 9 lines
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
Review
Quoted 6 lines
Signed-off-by zhuguangqing <[email protected]>
---
 drivers/thermal/cpuidle_cooling.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/thermal/cpuidle_cooling.c
Quoted 37 lines
index 78e3e8238116..868919ad3dda 100644
drivers/thermal/cpuidle_cooling.c +1 -1
@@ -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
drivers/thermal/cpuidle_cooling.c +3 -0
@@ -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;
 
drivers/thermal/cpuidle_cooling.c +4 -1
 	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);
 	}
 
Quoted 2 lines
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

Signature · 5 lines

<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

Daniel Lezcano

2020-09-17 13:39 UTC #2 lore raw

On 17/09/2020 14:33, Daniel Lezcano wrote:

[ .. ]

Quoted 3 lines

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.

s/worth/worse/

Signature · 5 lines

<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