On 09/10/2020 15:58, Lukasz Luba wrote:
> The sustainable power value might come from the Device Tree or can be
> estimated in run time. There is no need to estimate every time when the
> governor is called and temperature is high. Instead, store the estimated
> value and make it available via standard sysfs interface so it can be
> checked from the user-space. Re-invoke the estimation only in case the
> sustainable power was set to 0. Apart from that the PID coefficients
> are not going to be force updated thus can better handle sysfs settings.
>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
[ ... ]
> -static void estimate_pid_constants(struct thermal_zone_device *tz,
> - u32 sustainable_power, int trip_switch_on,
> - int control_temp, bool force)
> +static void estimate_tzp_constants(struct thermal_zone_device *tz,
> + int trip_switch_on, int control_temp)
> {
> - int ret;
> - int switch_on_temp;
> u32 temperature_threshold;
> + int switch_on_temp;
> + bool force = false;
> + int ret;
> s32 k_i;
>
> + if (!tz->tzp->sustainable_power) {
> + tz->tzp->sustainable_power = estimate_sustainable_power(tz);
> + force = true;
> + dev_info(&tz->device, "power_allocator: estimating sust. power
> and PID constants\n");
> + }
> +
> ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
> if (ret)
> switch_on_temp = 0;
>
> temperature_threshold = control_temp - switch_on_temp;
> /*
> - * estimate_pid_constants() tries to find appropriate default
> + * estimate_tzp_constants() tries to find appropriate default
> * values for thermal zones that don't provide them. If a
> * system integrator has configured a thermal zone with two
> * passive trip points at the same temperature, that person
> @@ -151,11 +151,11 @@ static void estimate_pid_constants(struct
> thermal_zone_device *tz,
> return;
>
> if (!tz->tzp->k_po || force)
> - tz->tzp->k_po = int_to_frac(sustainable_power) /
> + tz->tzp->k_po = int_to_frac(tz->tzp->sustainable_power) /
> temperature_threshold;
>
> if (!tz->tzp->k_pu || force)
> - tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
> + tz->tzp->k_pu = int_to_frac(2 * tz->tzp->sustainable_power) /
> temperature_threshold;
>
> if (!tz->tzp->k_i || force) {
> @@ -193,19 +193,13 @@ static u32 pid_controller(struct thermal_zone_device
> *tz,
> {
> s64 p, i, d, power_range;
> s32 err, max_power_frac;
> - u32 sustainable_power;
> struct power_allocator_params *params = tz->governor_data;
>
> max_power_frac = int_to_frac(max_allocatable_power);
>
> - if (tz->tzp->sustainable_power) {
> - sustainable_power = tz->tzp->sustainable_power;
> - } else {
> - sustainable_power = estimate_sustainable_power(tz);
> - estimate_pid_constants(tz, sustainable_power,
> - params->trip_switch_on, control_temp,
> - true);
> - }
> + if (!tz->tzp->sustainable_power)
> + estimate_tzp_constants(tz, params->trip_switch_on,
> + control_temp);
The changes in this patch are appropriate and make sense but they are
not done at the right place.
estimate_tzp_constants() must be called when sustainable_power is
updated via DT/init or sysfs.
Keeping a function to estimate the sustainable power and another one to
estimate the k_* separated would be more clear.
Actually the confusion is coming from when the pid constants are
computed, I suggest moving the initialization of k_* out of this
function and killing the 'force' test.
[ ... ]
--
<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
