| Message ID | 1913649.CQOukoFCf9@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: core: Redesign the governor interface | expand |
On 10/04/2024 18:12, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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 <rafael.j.wysocki@intel.com> > --- 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 = { > > >
On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 10/04/2024 18:12, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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 <rafael.j.wysocki@intel.com> > > --- > > 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 19:54, Rafael J. Wysocki wrote: > On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 10/04/2024 18:12, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> 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 <rafael.j.wysocki@intel.com> >>> --- >> >> 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 ?
On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 23/04/2024 19:54, Rafael J. Wysocki wrote: > > On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> On 10/04/2024 18:12, Rafael J. Wysocki wrote: > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> > >>> 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 <rafael.j.wysocki@intel.com> > >>> --- > >> > >> 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.
On 23/04/2024 20:05, Rafael J. Wysocki wrote: > On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 23/04/2024 19:54, Rafael J. Wysocki wrote: >>> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> >>>> On 10/04/2024 18:12, Rafael J. Wysocki wrote: >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>> >>>>> 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 <rafael.j.wysocki@intel.com> >>>>> --- >>>> >>>> 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
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 = {