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

[Lukasz Luba]

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@xxxxxxx>
> > ---
>
> [ ... ]
>
> > -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