Revert "thermal: power allocator: change the 'k_*' always in estimate_pid_constants()"
Commit Message
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(-)
Comments
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.
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
@@ -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);