Re: [PATCH v4 2/3] thermal: power allocator: refactor sustainable power estimation

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hey,

Mostly trivial nits (added in case you want to consider them for this code
or future changes).

On Tuesday 24 Nov 2020 at 16:10:24 (+0000), Lukasz Luba wrote:
> The sustainable power value might come from the Device Tree or can be
> estimated in run time. The sustainable power might be updated by the user
> via sysfs interface, which should trigger new estimation of PID
> coefficients. There is no need to estimate it 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.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 52 ++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 2e20085ed217..d7e4b9f6af60 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -63,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
>   * @trip_max_desired_temperature:	last passive trip point of the thermal
>   *					zone.  The temperature we are
>   *					controlling for.
> + * @sustainable_power:	Sustainable power (heat) that this thermal zone can
> + *			dissipate
>   */
>  struct power_allocator_params {
>  	bool allocated_tzp;
> @@ -70,6 +72,7 @@ struct power_allocator_params {
>  	s32 prev_err;
>  	int trip_switch_on;
>  	int trip_max_desired_temperature;
> +	u32 sustainable_power;
>  };
>  
>  /**
> @@ -118,10 +121,6 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>   *
>   * 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
> @@ -171,6 +170,42 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>  	 */
>  }
>  
> +/**
> + * get_sustainable_power() - Get the right sustainable power
                                    ^^^^^^^^^
Nit: I would not say there is a right sustainable power. I would remove
this.

> + * @tz:		thermal zone for which to estimate the constants
> + * @params:	parameters for the power allocator governor
> + * @control_temp:	target temperature for the power allocator governor
> + *
> + * This function is used for getting the proper sustainable power value based
> + * on variables which might be updated by the user sysfs interface. If that
                                          ^^
					  through
> + * happen the new value is going to be estimated and updated. It is also used

Nit: "If that happens, the new.."

> + * after thermal zone binding, where the initial values where set to 0.
                                                    ^^^^^^^^^^^^^^^^^^^^^
						    value could be 0.
> + */

Nit: I think the code is self explanatory so you might not need to go
into so many details in the description.

> +static u32 get_sustainable_power(struct thermal_zone_device *tz,
> +				 struct power_allocator_params *params,
> +				 int control_temp)
> +{
> +	u32 sustainable_power;
> +

Given that we call this every time the controller kicks in, it might
help to add unlikely to both conditions. I think the most likely
scenario is for our stored params->sustainable_power and
tz->tzp->sustainable_power to match.

> +	if (!tz->tzp->sustainable_power)
> +		sustainable_power = estimate_sustainable_power(tz);
> +	else
> +		sustainable_power = tz->tzp->sustainable_power;
> +
> +	/* 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,
> +				       true);
> +
> +		/* Do the estimation only once and make available in sysfs */
> +		tz->tzp->sustainable_power = sustainable_power;
> +		params->sustainable_power = sustainable_power;
> +	}
> +
> +	return sustainable_power;
> +}
> +
>  /**
>   * pid_controller() - PID controller
>   * @tz:	thermal zone we are operating in
> @@ -200,14 +235,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>  
>  	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);
> -	}
> +	sustainable_power = get_sustainable_power(tz, params, control_temp);
>  
>  	err = control_temp - tz->temperature;
>  	err = int_to_frac(err);
> -- 

The logic seems sane so:

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Thank you,
Ionela.

> 2.17.1
>