| Message ID | 20201009135850.14727-3-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | Improve the estimations in Intelligent Power Allocation | expand |
On Friday 09 Oct 2020 at 14:58:50 (+0100), 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 <lukasz.luba@arm.com> > --- > > v3: > - changed estimate_pid_constants to estimate_tzp_constants and related comments > - estimate the PID coefficients always together with sust. power > - added print indicating that we are estimating sust. power and PID const. > - don't use local variable 'sustainable_power' > > > drivers/thermal/gov_power_allocator.c | 65 ++++++++++++--------------- > 1 file changed, 29 insertions(+), 36 deletions(-) > > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c > index aa35aa6c561c..e92a8d3ca5d4 100644 > --- a/drivers/thermal/gov_power_allocator.c > +++ b/drivers/thermal/gov_power_allocator.c > @@ -96,6 +96,9 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz) > if (instance->trip != params->trip_max_desired_temperature) > continue; > > + if (!cdev_is_power_actor(cdev)) > + continue; > + > if (cdev->ops->state2power(cdev, tz, instance->upper, > &min_power)) > continue; > @@ -107,40 +110,37 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz) > } > > /** > - * estimate_pid_constants() - Estimate the constants for the PID controller > + * estimate_tzp_constants() - Estimate sustainable power and PID constants > * @tz: thermal zone for which to estimate the constants > - * @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. > - * Sustainable power is provided in case it was estimated. The > - * estimated sustainable_power should not be stored in the > - * thermal_zone_parameters so it has to be passed explicitly to this > - * function. > - * > - * 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. > + * This function is used to estimate the sustainable power and PID controller > + * constants in struct thermal_zone_parameters. These estimations will then be > + * available in the sysfs. > */ > -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); > > err = control_temp - tz->temperature; > err = int_to_frac(err); > @@ -244,7 +238,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, > power_range = p + i + d; > > /* feed-forward the known sustainable dissipatable power */ > - power_range = sustainable_power + frac_to_int(power_range); > + power_range = tz->tzp->sustainable_power + frac_to_int(power_range); > > power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power); > > @@ -603,20 +597,19 @@ static int power_allocator_bind(struct thermal_zone_device *tz) > > get_governor_trips(tz, params); > > + tz->governor_data = params; > + > if (tz->trips > 0) { > ret = tz->ops->get_trip_temp(tz, > params->trip_max_desired_temperature, > &control_temp); > if (!ret) > - estimate_pid_constants(tz, tz->tzp->sustainable_power, > - params->trip_switch_on, > - control_temp, false); > + estimate_tzp_constants(tz, params->trip_switch_on, > + control_temp); > } > > reset_pid_controller(params); > > - tz->governor_data = params; > - > return 0; > > free_params: > -- > 2.17.1 > Awesome! Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
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 <lukasz.luba@arm.com> > --- [ ... ] > -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. [ ... ]
Hi Daniel, On 10/13/20 5:41 PM, Daniel Lezcano wrote: > 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 <lukasz.luba@arm.com> >> --- > > [ ... ] > >> -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. > > > [ ... ] > > Thank you for the review. I will re-write the patch and address your suggestion. Regards, Lukasz
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c index aa35aa6c561c..e92a8d3ca5d4 100644 --- a/drivers/thermal/gov_power_allocator.c +++ b/drivers/thermal/gov_power_allocator.c @@ -96,6 +96,9 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz) if (instance->trip != params->trip_max_desired_temperature) continue; + if (!cdev_is_power_actor(cdev)) + continue; + if (cdev->ops->state2power(cdev, tz, instance->upper, &min_power)) continue; @@ -107,40 +110,37 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz) } /** - * estimate_pid_constants() - Estimate the constants for the PID controller + * estimate_tzp_constants() - Estimate sustainable power and PID constants * @tz: thermal zone for which to estimate the constants - * @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. - * Sustainable power is provided in case it was estimated. The - * estimated sustainable_power should not be stored in the - * thermal_zone_parameters so it has to be passed explicitly to this - * function. - * - * 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. + * This function is used to estimate the sustainable power and PID controller + * constants in struct thermal_zone_parameters. These estimations will then be + * available in the sysfs. */ -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); err = control_temp - tz->temperature; err = int_to_frac(err); @@ -244,7 +238,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, power_range = p + i + d; /* feed-forward the known sustainable dissipatable power */ - power_range = sustainable_power + frac_to_int(power_range); + power_range = tz->tzp->sustainable_power + frac_to_int(power_range); power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power); @@ -603,20 +597,19 @@ static int power_allocator_bind(struct thermal_zone_device *tz) get_governor_trips(tz, params); + tz->governor_data = params; + if (tz->trips > 0) { ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature, &control_temp); if (!ret) - estimate_pid_constants(tz, tz->tzp->sustainable_power, - params->trip_switch_on, - control_temp, false); + estimate_tzp_constants(tz, params->trip_switch_on, + control_temp); } reset_pid_controller(params); - tz->governor_data = params; - return 0; free_params:
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 <lukasz.luba@arm.com> --- v3: - changed estimate_pid_constants to estimate_tzp_constants and related comments - estimate the PID coefficients always together with sust. power - added print indicating that we are estimating sust. power and PID const. - don't use local variable 'sustainable_power' drivers/thermal/gov_power_allocator.c | 65 ++++++++++++--------------- 1 file changed, 29 insertions(+), 36 deletions(-)