Re: [PATCH v2 2/2] thermal: power allocator: change how estimation code is called

[Lukasz Luba <lukasz.luba@arm.com>]

Hi Ionela,

On 10/9/20 12:19 PM, Ionela Voinescu wrote:
> Hi Lukasz,
> 
> On Thursday 08 Oct 2020 at 18:04:26 (+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>
>> ---
>>   drivers/thermal/gov_power_allocator.c | 56 +++++++++++++--------------
>>   1 file changed, 26 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index aa35aa6c561c..1ad8d9c2685f 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;
>> @@ -109,31 +112,28 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>>   /**
>>    * estimate_pid_constants() - Estimate the constants for the PID controller
>        ^^^^^^^^^^^^^^^^^^^^^^
>        estimate_tzp_constants()?
> 
> When called in pid_controller() it feels strange that we check for
> sustainable_power, then we call estimate_pid_constants() and then we
> magically have an non-zero sustainable_power. Therefore, it would be
> good to change the name to indicate it's not only the PID constants that
> are estimated.

I agree, I will rename it.

> 
>>    * @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.
> 
> How about replacing this with:
> 
> """
>   * 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 sysfs.
> """

Yes, that change is required since the function name will also change.

> 
>> - * 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.
>> + * Sustainable power is going to be estimated in case it is 0.
>>    */
>>   static void estimate_pid_constants(struct thermal_zone_device *tz,
>> -				   u32 sustainable_power, int trip_switch_on,
>> -				   int control_temp, bool force)
>> +				   int trip_switch_on, int control_temp)
>>   {
>> -	int ret;
>> -	int switch_on_temp;
>> +	u32 sustainable_power = tz->tzp->sustainable_power;
>>   	u32 temperature_threshold;
>> +	int switch_on_temp;
>> +	int ret;
>>   	s32 k_i;
>>   
>> +	if (!sustainable_power) {
>> +		sustainable_power = estimate_sustainable_power(tz);
>> +		/* Make the estimation available in sysfs */
> 
> I would remove this comment from here. The reason is that this is not a
> special case. This will happen for all the tzp parameters set below.
> That's why I suggested adding this to the overall function comment above.

Yes, function comment will handle this better.

> 
>> +		tz->tzp->sustainable_power = sustainable_power;
>> +	}
>> +
>>   	ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
>>   	if (ret)
>>   		switch_on_temp = 0;
>> @@ -150,15 +150,15 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>>   	if (!temperature_threshold)
>>   		return;
>>   
>> -	if (!tz->tzp->k_po || force)
>> +	if (!tz->tzp->k_po)
>>   		tz->tzp->k_po = int_to_frac(sustainable_power) /
>>   			temperature_threshold;
>>   
>> -	if (!tz->tzp->k_pu || force)
>> +	if (!tz->tzp->k_pu)
>>   		tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
>>   			temperature_threshold;
>>   
>> -	if (!tz->tzp->k_i || force) {
>> +	if (!tz->tzp->k_i) {
>>   		k_i = tz->tzp->k_pu / 10;
>>   		tz->tzp->k_i = k_i > 0 ? k_i : 1;
>>   	}
> 
> (Possibly judgement call)
> 
> I agree we don't need the force argument to this function, but I would
> still keep an internal force variable (default false) to be set to true
> when we estimate and set the sustainable power.

Yes, make sense, I will set locally.

> 
> The reason for this is that there is no guarantee that when
> sustainable_power is found to be 0 and estimated, we'll then find all of
> the PID constants 0 as well in order to set them to a sane default.
> Basically my worry is that we'll end up with a combination of PID
> constants and sustainable power (some estimated and some not) that is not
> quite sane.
> 
> But I understand a potential usecase in which a user might want to set
> it's own PID constants while wanting an estimated sustainable_power.
> But for this do you think it might be worth just having a pr_info
> message saying that "Sustainable power is 0; will estimate sustainable
> power and PID constants."? For this the user would only have to know
> that they need to set the sustainable_power to 0 first and then
> populate its own PID constants if they want to.

Yes, the print message is good in this case.

> 
>> @@ -198,14 +198,11 @@ 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);
>> -	}
>> +	if (!tz->tzp->sustainable_power)
>> +		estimate_pid_constants(tz, params->trip_switch_on,
>> +				       control_temp);
>> +
>> +	sustainable_power = tz->tzp->sustainable_power;
>>   
> 
> (Nit)
> 
> This is only used once below in:
> power_range = sustainable_power + frac_to_int(power_range);
> 
> I think we can use tz->tzp->sustainable_power directly there and
> completely remove sustainable_power.

I had the feeling that it won't fit in 80line there but it does.
I will change it.

Thank you for the review.

Lukasz

> 
> Thank you,
> Ionela.
> 
>>   	err = control_temp - tz->temperature;
>>   	err = int_to_frac(err);
>> @@ -603,20 +600,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_pid_constants(tz, params->trip_switch_on,
>> +					       control_temp);
>>   	}
>>   
>>   	reset_pid_controller(params);
>>   
>> -	tz->governor_data = params;
>> -
>>   	return 0;
>>   
>>   free_params:
>> -- 
>> 2.17.1
>>