Re: [PATCH v2 1/2] thermal: power allocator: change the 'k_i' coefficient estimation

[Lukasz Luba]

On 10/9/20 2:05 PM, Ionela Voinescu wrote:
> On Thursday 08 Oct 2020 at 18:04:25 (+0100), Lukasz Luba wrote:
> > Intelligent Power Allocation (IPA) is built around the PID controller
> > concept. The initialization code tries to setup the environment based on
> > the information available in DT or estimate the value based on minimum
> > power reported by each of the cooling device. The estimation will have an
> > impact on the PID controller behaviour via the related 'k_po', 'k_pu',
> > 'k_i' coefficients and also on the power budget calculation.
> >
> > This change prevents the situation when 'k_i' is relatively big compared
> > to 'k_po' and 'k_pu' values. This might happen when the estimation for
> > 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are
> > small.
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   drivers/thermal/gov_power_allocator.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index e566806f1550..aa35aa6c561c 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -132,6 +132,7 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
> >   	int ret;
> >   	int switch_on_temp;
> >   	u32 temperature_threshold;
> > +	s32 k_i;
> >   
> >   	ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
> >   	if (ret)
> > @@ -157,8 +158,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
> >   		tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
> >   			temperature_threshold;
> >   
> > -	if (!tz->tzp->k_i || force)
> > -		tz->tzp->k_i = int_to_frac(10) / 1000;
> > +	if (!tz->tzp->k_i || force) {
> > +		k_i = tz->tzp->k_pu / 10;
> > +		tz->tzp->k_i = k_i > 0 ? k_i : 1;
> > +	}
>
> I spent some time to understand how smaller or bigger values here impact
> the stability of the output and its closeness to the control temperature
> so I could give you and informed review :).
>
> I did observed that if the k_i value has the same order of magnitude as
> k_p, the output oscillates more, so I do believe this is a good change
> to have.
>
> What I also observed is that a small k_d value could be very beneficial
> in quicker stabilising the oscillation and saw that it's recommended to
> have for temperature, or in general for systems with measurement lag.
>
> It's probably worth experimenting with some real systems first, but I
> wonder if setting k_d to 1 as well is a good default. The risk is that
> we will end up too conservative and not take advantage of some power
> budget that's actually still left on the table. In any case, this is a
> story for another time :).
>
> For these changes:
>
> Reviewed-by: Ionela Voinescu <ionela.voinescu@xxxxxxx>

Thank you for the review. I will add it to the v3, which I am going to
send soon.

Regards,
Lukasz

> Regards,
> Ionela.
>
> > +
> >   	/*
> >   	 * The default for k_d and integral_cutoff is 0, so we can
> >   	 * leave them as they are.
> > --
> > 2.17.1