drivers/thermal/gov_power_allocator.c | 28 ++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)
This reverts commit 90a996544946d1d4834ec2ec8add586edd905779.
The commit ensures that the pid constants are updated when
sustainable_power changes, but it makes it impossible for
the driver to set the pid constants when the sustainable_power
is not changed.
When the driver tries to register a thermal zone device by
thermal_zone_device_register_with_trips(const char *type,
struct thermal_trip *trips, int num_trips, int mask,
void *devdata, struct thermal_zone_device_ops *ops,
struct thermal_zone_params *tzp, int passive_delay,
int polling_delay)
and passes the private thermal_zone_params structure data,
thermal_zone_devcice_register_with_trips
|
thermal_set_governor
|
bind_to_tz
|
power_allocator_bind
|
estimate_pid_constants
the tzp->k_* will not be the data that driver have given,
but the data estimated by sustainable_power.
To make it possible for driver to add its own pid constants,
the 'force' flag is needed to indicate whether the tzp->k_*
should be estimated by sustainable_power or not.
Signed-off-by: Di Shen <di.shen@unisoc.com>
---
drivers/thermal/gov_power_allocator.c | 28 ++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 8642f1096b91..f1753291b827 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -116,13 +116,18 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
* @sustainable_power: sustainable power for the thermal zone
* @trip_switch_on: trip point number for the switch on temperature
* @control_temp: target temperature for the power allocator governor
+ * @force: whether to force the update of the constants
*
* This function is used to update the estimation of the PID
* controller constants in struct thermal_zone_parameters.
+ *
+ * If @force is not set, the values in the thermal zone's parameters
+ * are preserved if they are not zero. If @force is set, the values
+ * in thermal zone's parameters are overwritten.
*/
static void estimate_pid_constants(struct thermal_zone_device *tz,
u32 sustainable_power, int trip_switch_on,
- int control_temp)
+ int control_temp, bool force)
{
struct thermal_trip trip;
u32 temperature_threshold = control_temp;
@@ -144,14 +149,18 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
if (!temperature_threshold)
return;
- tz->tzp->k_po = int_to_frac(sustainable_power) /
- temperature_threshold;
+ if (!tz->tzp->k_po || force)
+ tz->tzp->k_po = int_to_frac(sustainable_power) /
+ temperature_threshold;
- tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
- temperature_threshold;
+ if (!tz->tzp->k_pu || force)
+ tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
+ temperature_threshold;
- k_i = tz->tzp->k_pu / 10;
- tz->tzp->k_i = k_i > 0 ? k_i : 1;
+ if (!tz->tzp->k_i || force) {
+ k_i = tz->tzp->k_pu / 10;
+ tz->tzp->k_i = k_i > 0 ? k_i : 1;
+ }
/*
* The default for k_d and integral_cutoff is 0, so we can
@@ -184,7 +193,8 @@ static u32 get_sustainable_power(struct thermal_zone_device *tz,
/* Check if it's init value 0 or there was update via sysfs */
if (sustainable_power != params->sustainable_power) {
estimate_pid_constants(tz, sustainable_power,
- params->trip_switch_on, control_temp);
+ params->trip_switch_on, control_temp,
+ true);
/* Do the estimation only once and make available in sysfs */
tz->tzp->sustainable_power = sustainable_power;
@@ -662,7 +672,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
if (!ret)
estimate_pid_constants(tz, tz->tzp->sustainable_power,
params->trip_switch_on,
- trip.temperature);
+ trip.temperature, false);
}
reset_pid_controller(params);
--
2.17.1Hi Di, On 7/12/23 09:48, Di Shen wrote: > This reverts commit 90a996544946d1d4834ec2ec8add586edd905779. > > The commit ensures that the pid constants are updated when > sustainable_power changes, but it makes it impossible for > the driver to set the pid constants when the sustainable_power > is not changed. > > When the driver tries to register a thermal zone device by > thermal_zone_device_register_with_trips(const char *type, > struct thermal_trip *trips, int num_trips, int mask, > void *devdata, struct thermal_zone_device_ops *ops, > struct thermal_zone_params *tzp, int passive_delay, > int polling_delay) > and passes the private thermal_zone_params structure data, > > thermal_zone_devcice_register_with_trips > | > thermal_set_governor > | > bind_to_tz > | > power_allocator_bind > | > estimate_pid_constants > > the tzp->k_* will not be the data that driver have given, > but the data estimated by sustainable_power. > > To make it possible for driver to add its own pid constants, That was dropped, the drivers shouldn't configure 'k_*' IPA parameters. There was also an ask to add those parameter values to the DT for setup - also not allowed. > the 'force' flag is needed to indicate whether the tzp->k_* > should be estimated by sustainable_power or not. We don't want to maintain many different ways of configurations, which can cause bugs in not tested corner cases. Please use the user-space to change those 'k_*' parameters. There are this dedicated and safe sysfs interfaces for each thermal zone. The phones that I have on my desk do the update of 'k_*' parameters via sysfs. They do this in different scenarios. You can try to derive best 'k_*' values for your workload scenarios and than save them in the config file. You can update in runtime from user-space when you switch to your scenario (e.g. camera, game, video call). Regards, Lukasz
Hi Lukasz, I'm happy to hear from you :) On Wed, Jul 19, 2023 at 4:50 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > Hi Di, > > On 7/12/23 09:48, Di Shen wrote: > > This reverts commit 90a996544946d1d4834ec2ec8add586edd905779. > > > > The commit ensures that the pid constants are updated when > > sustainable_power changes, but it makes it impossible for > > the driver to set the pid constants when the sustainable_power > > is not changed. > > > > When the driver tries to register a thermal zone device by > > thermal_zone_device_register_with_trips(const char *type, > > struct thermal_trip *trips, int num_trips, int mask, > > void *devdata, struct thermal_zone_device_ops *ops, > > struct thermal_zone_params *tzp, int passive_delay, > > int polling_delay) > > and passes the private thermal_zone_params structure data, > > > > thermal_zone_devcice_register_with_trips > > | > > thermal_set_governor > > | > > bind_to_tz > > | > > power_allocator_bind > > | > > estimate_pid_constants > > > > the tzp->k_* will not be the data that driver have given, > > but the data estimated by sustainable_power. > > > > To make it possible for driver to add its own pid constants, > > That was dropped, the drivers shouldn't configure 'k_*' IPA > parameters. There was also an ask to add those parameter > values to the DT for setup - also not allowed. > > > the 'force' flag is needed to indicate whether the tzp->k_* > > should be estimated by sustainable_power or not. > > We don't want to maintain many different ways of configurations, > which can cause bugs in not tested corner cases. > Ok, I understand. > Please use the user-space to change those 'k_*' parameters. > There are this dedicated and safe sysfs interfaces for each > thermal zone. > > The phones that I have on my desk do the update of 'k_*' parameters via > sysfs. They do this in different scenarios. You can try to derive > best 'k_*' values for your workload scenarios and than save > them in the config file. You can update in runtime from user-space > when you switch to your scenario (e.g. camera, game, video call). > Thank you for your kind suggestions, Lukasz. Now I totally understand. Thank you. > Regards, > Lukasz Best regards, Di
© 2016 - 2026 Red Hat, Inc.