From: Rafael J. Wysocki <[email protected]>
Notice that the passive field in struct thermal_zone_device is not
used by the Power Allocator governor itself and so the ordering of
its updates with respect to allow_maximum_power() or allocate_power()
does not matter.
Accordingly, make power_allocator_manage() update that field right
before returning, which allows the current value of it to be passed
directly to allow_maximum_power() without using the additional update
variable that can be dropped.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/thermal/gov_power_allocator.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -747,21 +747,18 @@ static void power_allocator_manage(struc
{
struct power_allocator_params *params = tz->governor_data;
const struct thermal_trip *trip = params->trip_switch_on;
- bool update;
lockdep_assert_held(&tz->lock);
if (trip && tz->temperature < trip->temperature) {
- update = tz->passive;
- tz->passive = 0;
reset_pid_controller(params);
- allow_maximum_power(tz, update);
+ allow_maximum_power(tz, tz->passive);
+ tz->passive = 0;
return;
}
- tz->passive = 1;
-
allocate_power(tz, params->trip_max->temperature);
+ tz->passive = 1;
}
static struct thermal_governor thermal_gov_power_allocator = {
On 4/10/24 17:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Notice that the passive field in struct thermal_zone_device is not
> used by the Power Allocator governor itself and so the ordering of
> its updates with respect to allow_maximum_power() or allocate_power()
> does not matter.
>
> Accordingly, make power_allocator_manage() update that field right
> before returning, which allows the current value of it to be passed
> directly to allow_maximum_power() without using the additional update
> variable that can be dropped.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/thermal/gov_power_allocator.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -747,21 +747,18 @@ static void power_allocator_manage(struc
> {
> struct power_allocator_params *params = tz->governor_data;
> const struct thermal_trip *trip = params->trip_switch_on;
> - bool update;
>
> lockdep_assert_held(&tz->lock);
>
> if (trip && tz->temperature < trip->temperature) {
> - update = tz->passive;
> - tz->passive = 0;
> reset_pid_controller(params);
> - allow_maximum_power(tz, update);
> + allow_maximum_power(tz, tz->passive);
> + tz->passive = 0;
> return;
> }
>
> - tz->passive = 1;
> -
> allocate_power(tz, params->trip_max->temperature);
> + tz->passive = 1;
> }
>
> static struct thermal_governor thermal_gov_power_allocator = {
>
>
>
LGTM
Reviewed-by: Lukasz Luba <[email protected]>
On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Notice that the passive field in struct thermal_zone_device is not
> used by the Power Allocator governor itself and so the ordering of
> its updates with respect to allow_maximum_power() or allocate_power()
> does not matter.
>
> Accordingly, make power_allocator_manage() update that field right
> before returning, which allows the current value of it to be passed
> directly to allow_maximum_power() without using the additional update
> variable that can be dropped.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
The step_wise and the power allocator are changing the tz->passive
values, so telling the core to start and stop the passive mitigation timer.
It looks strange that a plugin controls the core internal and not the
opposite.
I'm wondering if it would not make sense to have the following ops:
.start
.stop
start is called when the first trip point is crossed the way up
stop is called when the first trip point is crossed the way down
- The core is responsible to start and stop the passive mitigation timer.
- the governors do no longer us tz->passive
The reset of the governor can happen at start or stop, as well as the
device cooling states.
> drivers/thermal/gov_power_allocator.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -747,21 +747,18 @@ static void power_allocator_manage(struc
> {
> struct power_allocator_params *params = tz->governor_data;
> const struct thermal_trip *trip = params->trip_switch_on;
> - bool update;
>
> lockdep_assert_held(&tz->lock);
>
> if (trip && tz->temperature < trip->temperature) {
> - update = tz->passive;
> - tz->passive = 0;
> reset_pid_controller(params);
> - allow_maximum_power(tz, update);
> + allow_maximum_power(tz, tz->passive);
> + tz->passive = 0;
> return;
> }
>
> - tz->passive = 1;
> -
> allocate_power(tz, params->trip_max->temperature);
> + tz->passive = 1;
> }
>
> static struct thermal_governor thermal_gov_power_allocator = {
>
>
>
--
<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 Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Notice that the passive field in struct thermal_zone_device is not
> > used by the Power Allocator governor itself and so the ordering of
> > its updates with respect to allow_maximum_power() or allocate_power()
> > does not matter.
> >
> > Accordingly, make power_allocator_manage() update that field right
> > before returning, which allows the current value of it to be passed
> > directly to allow_maximum_power() without using the additional update
> > variable that can be dropped.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
>
> The step_wise and the power allocator are changing the tz->passive
> values, so telling the core to start and stop the passive mitigation timer.
>
> It looks strange that a plugin controls the core internal and not the
> opposite.
>
> I'm wondering if it would not make sense to have the following ops:
>
> .start
> .stop
>
> .start is called when the first trip point is crossed the way up
> .stop is called when the first trip point is crossed the way down
>
> - The core is responsible to start and stop the passive mitigation timer.
>
> - the governors do no longer us tz->passive
>
> The reset of the governor can happen at start or stop, as well as the
> device cooling states.
I have a patch that simply increments tz->passive when a passive trip
point is passed on the way up and decrements it when a passive trip
point is crossed on the way down. It appears to work reasonably well.
On 23/04/2024 20:05, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 23/04/2024 19:54, Rafael J. Wysocki wrote:
>>> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
>>> <[email protected]> wrote:
>>>>
>>>> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <[email protected]>
>>>>>
>>>>> Notice that the passive field in struct thermal_zone_device is not
>>>>> used by the Power Allocator governor itself and so the ordering of
>>>>> its updates with respect to allow_maximum_power() or allocate_power()
>>>>> does not matter.
>>>>>
>>>>> Accordingly, make power_allocator_manage() update that field right
>>>>> before returning, which allows the current value of it to be passed
>>>>> directly to allow_maximum_power() without using the additional update
>>>>> variable that can be dropped.
>>>>>
>>>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>>>> ---
>>>>
>>>> The step_wise and the power allocator are changing the tz->passive
>>>> values, so telling the core to start and stop the passive mitigation timer.
>>>>
>>>> It looks strange that a plugin controls the core internal and not the
>>>> opposite.
>>>>
>>>> I'm wondering if it would not make sense to have the following ops:
>>>>
>>>> .start
>>>> .stop
>>>>
>>>> .start is called when the first trip point is crossed the way up
>>>> .stop is called when the first trip point is crossed the way down
>>>>
>>>> - The core is responsible to start and stop the passive mitigation timer.
>>>>
>>>> - the governors do no longer us tz->passive
>>>>
>>>> The reset of the governor can happen at start or stop, as well as the
>>>> device cooling states.
>>>
>>> I have a patch that simply increments tz->passive when a passive trip
>>> point is passed on the way up and decrements it when a passive trip
>>> point is crossed on the way down. It appears to work reasonably well.
>>
>> Does it make the governors getting ride of it ? Or at least not changing
>> its value ?
>
> Not yet, but I'm going to update it this way. The governors should
> not mess up with tz->passive IMV.
+1
--
<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 23/04/2024 19:54, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> Notice that the passive field in struct thermal_zone_device is not
>>> used by the Power Allocator governor itself and so the ordering of
>>> its updates with respect to allow_maximum_power() or allocate_power()
>>> does not matter.
>>>
>>> Accordingly, make power_allocator_manage() update that field right
>>> before returning, which allows the current value of it to be passed
>>> directly to allow_maximum_power() without using the additional update
>>> variable that can be dropped.
>>>
>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>> ---
>>
>> The step_wise and the power allocator are changing the tz->passive
>> values, so telling the core to start and stop the passive mitigation timer.
>>
>> It looks strange that a plugin controls the core internal and not the
>> opposite.
>>
>> I'm wondering if it would not make sense to have the following ops:
>>
>> .start
>> .stop
>>
>> .start is called when the first trip point is crossed the way up
>> .stop is called when the first trip point is crossed the way down
>>
>> - The core is responsible to start and stop the passive mitigation timer.
>>
>> - the governors do no longer us tz->passive
>>
>> The reset of the governor can happen at start or stop, as well as the
>> device cooling states.
>
> I have a patch that simply increments tz->passive when a passive trip
> point is passed on the way up and decrements it when a passive trip
> point is crossed on the way down. It appears to work reasonably well.
Does it make the governors getting ride of it ? Or at least not changing
its value ?
--
<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 Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 23/04/2024 19:54, Rafael J. Wysocki wrote:
> > On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <[email protected]>
> >>>
> >>> Notice that the passive field in struct thermal_zone_device is not
> >>> used by the Power Allocator governor itself and so the ordering of
> >>> its updates with respect to allow_maximum_power() or allocate_power()
> >>> does not matter.
> >>>
> >>> Accordingly, make power_allocator_manage() update that field right
> >>> before returning, which allows the current value of it to be passed
> >>> directly to allow_maximum_power() without using the additional update
> >>> variable that can be dropped.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <[email protected]>
> >>> ---
> >>
> >> The step_wise and the power allocator are changing the tz->passive
> >> values, so telling the core to start and stop the passive mitigation timer.
> >>
> >> It looks strange that a plugin controls the core internal and not the
> >> opposite.
> >>
> >> I'm wondering if it would not make sense to have the following ops:
> >>
> >> .start
> >> .stop
> >>
> >> .start is called when the first trip point is crossed the way up
> >> .stop is called when the first trip point is crossed the way down
> >>
> >> - The core is responsible to start and stop the passive mitigation timer.
> >>
> >> - the governors do no longer us tz->passive
> >>
> >> The reset of the governor can happen at start or stop, as well as the
> >> device cooling states.
> >
> > I have a patch that simply increments tz->passive when a passive trip
> > point is passed on the way up and decrements it when a passive trip
> > point is crossed on the way down. It appears to work reasonably well.
>
> Does it make the governors getting ride of it ? Or at least not changing
> its value ?
Not yet, but I'm going to update it this way. The governors should
not mess up with tz->passive IMV.