[PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Di Shen]

Commit <0952177f2a1f>(thermal/core/power_allocator: Update once
cooling devices when temp is low) adds a update flag to avoid
the thermal event is triggered when there is no need, and
thermal cdev would be update once when temperature is low.

But when the trips are writable, and switch_on_temp is set
to be a higher value, the cooling device state may not be
reset to 0, because last_temperature is smaller than the
switch_on_temp.

For example:
First:
switch_on_temp=70 control_temp=85;
Then userspace change the trip_temp:
switch_on_temp=45 control_temp=55 cur_temp=54

Then userspace reset the trip_temp:
switch_on_temp=70 control_temp=85 cur_temp=57 last_temp=54

At this time, the cooling device state should be reset to 0.
However, because cur_temp(57) < switch_on_temp(70)
last_temp(54) < switch_on_temp(70)  ---->  update = false,
update is false, the cooling device state can not be reset.

This patch adds a function thermal_cdev_needs_update() to
renew the update flag value only when the trips are writable,
so that thermal cdev->state can be reset after switch_on_temp
changed from low to high.

Fixes: <0952177f2a1f> (thermal/core/power_allocator: Update once cooling devices when temp is low)
Signed-off-by: Di Shen <di.shen@unisoc.com>

---
V3:
- Add fix tag.

V2:
- Compared to v1, do not revert.

- Add a variable(last_switch_on_temp) in power_allocator_params
  to record the last switch_on_temp value.

- Adds a function to renew the update flag and update the
  last_switch_on_temp when thermal trips are writable.

V1:
- Revert commit 0952177f2a1f.
---
---
 drivers/thermal/gov_power_allocator.c | 39 ++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 0eaf1527d3e3..c9e1f3b15f15 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -59,6 +59,8 @@ static inline s64 div_frac(s64 x, s64 y)
  *			governor switches on when this trip point is crossed.
  *			If the thermal zone only has one passive trip point,
  *			@trip_switch_on should be INVALID_TRIP.
+ * @last_switch_on_temp:Record the last switch_on_temp only when trips
+			are writable.
  * @trip_max_desired_temperature:	last passive trip point of the thermal
  *					zone.  The temperature we are
  *					controlling for.
@@ -70,6 +72,9 @@ struct power_allocator_params {
 	s64 err_integral;
 	s32 prev_err;
 	int trip_switch_on;
+#ifdef CONFIG_THERMAL_WRITABLE_TRIPS
+	int last_switch_on_temp;
+#endif
 	int trip_max_desired_temperature;
 	u32 sustainable_power;
 };
@@ -554,6 +559,25 @@ static void get_governor_trips(struct thermal_zone_device *tz,
 	}
 }
 
+#ifdef CONFIG_THERMAL_WRITABLE_TRIPS
+static bool thermal_cdev_needs_update(struct thermal_zone_device *tz, int switch_on_temp)
+{
+	bool update;
+	struct power_allocator_params *params = tz->governor_data;
+	int last_switch_on_temp = params->last_switch_on_temp;
+
+	update = (tz->last_temperature >= last_switch_on_temp);
+	params->last_switch_on_temp = switch_on_temp;
+
+	return update;
+}
+#else
+static inline bool thermal_cdev_needs_update(struct thermal_zone_device *tz, int switch_on_temp)
+{
+	return false;
+}
+#endif
+
 static void reset_pid_controller(struct power_allocator_params *params)
 {
 	params->err_integral = 0;
@@ -709,12 +733,15 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
 		return 0;
 
 	ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
-	if (!ret && (tz->temperature < trip.temperature)) {
-		update = (tz->last_temperature >= trip.temperature);
-		tz->passive = 0;
-		reset_pid_controller(params);
-		allow_maximum_power(tz, update);
-		return 0;
+	if (!ret) {
+		update = thermal_cdev_needs_update(tz, trip.temperature);
+		if (tz->temperature < trip.temperature) {
+			update |= (tz->last_temperature >= trip.temperature);
+			tz->passive = 0;
+			reset_pid_controller(params);
+			allow_maximum_power(tz, update);
+			return 0;
+		}
 	}
 
 	tz->passive = 1;
-- 
2.17.1

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Lukasz Luba]

On 3/20/23 09:56, Di Shen wrote:
> Commit <0952177f2a1f>(thermal/core/power_allocator: Update once
> cooling devices when temp is low) adds a update flag to avoid
> the thermal event is triggered when there is no need, and
> thermal cdev would be update once when temperature is low.
> 
> But when the trips are writable, and switch_on_temp is set
> to be a higher value, the cooling device state may not be
> reset to 0, because last_temperature is smaller than the
> switch_on_temp.
> 
> For example:
> First:
> switch_on_temp=70 control_temp=85;
> Then userspace change the trip_temp:
> switch_on_temp=45 control_temp=55 cur_temp=54
> 
> Then userspace reset the trip_temp:
> switch_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> 
> At this time, the cooling device state should be reset to 0.
> However, because cur_temp(57) < switch_on_temp(70)
> last_temp(54) < switch_on_temp(70)  ---->  update = false,
> update is false, the cooling device state can not be reset.
> 
> This patch adds a function thermal_cdev_needs_update() to
> renew the update flag value only when the trips are writable,
> so that thermal cdev->state can be reset after switch_on_temp
> changed from low to high.
> 
> Fixes: <0952177f2a1f> (thermal/core/power_allocator: Update once cooling devices when temp is low)
> Signed-off-by: Di Shen <di.shen@unisoc.com>
> 
> ---
> V3:
> - Add fix tag.
> 
> V2:
> - Compared to v1, do not revert.
> 
> - Add a variable(last_switch_on_temp) in power_allocator_params
>    to record the last switch_on_temp value.
> 
> - Adds a function to renew the update flag and update the
>    last_switch_on_temp when thermal trips are writable.
> 
> V1:
> - Revert commit 0952177f2a1f.
> ---
> ---
>   drivers/thermal/gov_power_allocator.c | 39 ++++++++++++++++++++++-----
>   1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 0eaf1527d3e3..c9e1f3b15f15 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -59,6 +59,8 @@ static inline s64 div_frac(s64 x, s64 y)
>    *			governor switches on when this trip point is crossed.
>    *			If the thermal zone only has one passive trip point,
>    *			@trip_switch_on should be INVALID_TRIP.
> + * @last_switch_on_temp:Record the last switch_on_temp only when trips
> +			are writable.
>    * @trip_max_desired_temperature:	last passive trip point of the thermal
>    *					zone.  The temperature we are
>    *					controlling for.
> @@ -70,6 +72,9 @@ struct power_allocator_params {
>   	s64 err_integral;
>   	s32 prev_err;
>   	int trip_switch_on;
> +#ifdef CONFIG_THERMAL_WRITABLE_TRIPS
> +	int last_switch_on_temp;
> +#endif
>   	int trip_max_desired_temperature;
>   	u32 sustainable_power;
>   };
> @@ -554,6 +559,25 @@ static void get_governor_trips(struct thermal_zone_device *tz,
>   	}
>   }
>   
> +#ifdef CONFIG_THERMAL_WRITABLE_TRIPS
> +static bool thermal_cdev_needs_update(struct thermal_zone_device *tz, int switch_on_temp)
> +{
> +	bool update;
> +	struct power_allocator_params *params = tz->governor_data;
> +	int last_switch_on_temp = params->last_switch_on_temp;
> +
> +	update = (tz->last_temperature >= last_switch_on_temp);
> +	params->last_switch_on_temp = switch_on_temp;
> +
> +	return update;
> +}
> +#else
> +static inline bool thermal_cdev_needs_update(struct thermal_zone_device *tz, int switch_on_temp)
> +{
> +	return false;
> +}
> +#endif
> +
>   static void reset_pid_controller(struct power_allocator_params *params)
>   {
>   	params->err_integral = 0;
> @@ -709,12 +733,15 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
>   		return 0;
>   
>   	ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
> -	if (!ret && (tz->temperature < trip.temperature)) {
> -		update = (tz->last_temperature >= trip.temperature);
> -		tz->passive = 0;
> -		reset_pid_controller(params);
> -		allow_maximum_power(tz, update);
> -		return 0;
> +	if (!ret) {
> +		update = thermal_cdev_needs_update(tz, trip.temperature);
> +		if (tz->temperature < trip.temperature) {
> +			update |= (tz->last_temperature >= trip.temperature);
> +			tz->passive = 0;
> +			reset_pid_controller(params);
> +			allow_maximum_power(tz, update);
> +			return 0;
> +		}
>   	}
>   
>   	tz->passive = 1;


Thanks for the patch. The code looks good. The initial value of
'last_switch_on_temp' would be set to 0. It won't harm us because it
will get the proper value later.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Di Shen]

Hi Lukasz,
Could you please apply this patch if there's no more comment? Thank you.

Best regards,
Di

On Mon, Mar 20, 2023 at 8:29 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 3/20/23 09:56, Di Shen wrote:
> > Commit <0952177f2a1f>(thermal/core/power_allocator: Update once
> > cooling devices when temp is low) adds a update flag to avoid
> > the thermal event is triggered when there is no need, and
> > thermal cdev would be update once when temperature is low.
> >
> > But when the trips are writable, and switch_on_temp is set
> > to be a higher value, the cooling device state may not be
> > reset to 0, because last_temperature is smaller than the
> > switch_on_temp.
> >
> > For example:
> > First:
> > switch_on_temp=70 control_temp=85;
> > Then userspace change the trip_temp:
> > switch_on_temp=45 control_temp=55 cur_temp=54
> >
> > Then userspace reset the trip_temp:
> > switch_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> >
> > At this time, the cooling device state should be reset to 0.
> > However, because cur_temp(57) < switch_on_temp(70)
> > last_temp(54) < switch_on_temp(70)  ---->  update = false,
> > update is false, the cooling device state can not be reset.
> >
> > This patch adds a function thermal_cdev_needs_update() to
> > renew the update flag value only when the trips are writable,
> > so that thermal cdev->state can be reset after switch_on_temp
> > changed from low to high.
> >
> > Fixes: <0952177f2a1f> (thermal/core/power_allocator: Update once cooling devices when temp is low)
> > Signed-off-by: Di Shen <di.shen@unisoc.com>
> >
> > ---
> > V3:
> > - Add fix tag.
> >
> > V2:
> > - Compared to v1, do not revert.
> >
> > - Add a variable(last_switch_on_temp) in power_allocator_params
> >    to record the last switch_on_temp value.
> >
> > - Adds a function to renew the update flag and update the
> >    last_switch_on_temp when thermal trips are writable.
> >
> > V1:
> > - Revert commit 0952177f2a1f.
> > ---
> > ---
> >   drivers/thermal/gov_power_allocator.c | 39 ++++++++++++++++++++++-----
> >   1 file changed, 33 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 0eaf1527d3e3..c9e1f3b15f15 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -59,6 +59,8 @@ static inline s64 div_frac(s64 x, s64 y)
> >    *                  governor switches on when this trip point is crossed.
> >    *                  If the thermal zone only has one passive trip point,
> >    *                  @trip_switch_on should be INVALID_TRIP.
> > + * @last_switch_on_temp:Record the last switch_on_temp only when trips
> > +                     are writable.
> >    * @trip_max_desired_temperature:   last passive trip point of the thermal
> >    *                                  zone.  The temperature we are
> >    *                                  controlling for.
> > @@ -70,6 +72,9 @@ struct power_allocator_params {
> >       s64 err_integral;
> >       s32 prev_err;
> >       int trip_switch_on;
> > +#ifdef CONFIG_THERMAL_WRITABLE_TRIPS
> > +     int last_switch_on_temp;
> > +#endif
> >       int trip_max_desired_temperature;
> >       u32 sustainable_power;
> >   };
> > @@ -554,6 +559,25 @@ static void get_governor_trips(struct thermal_zone_device *tz,
> >       }
> >   }
> >
> > +#ifdef CONFIG_THERMAL_WRITABLE_TRIPS
> > +static bool thermal_cdev_needs_update(struct thermal_zone_device *tz, int switch_on_temp)
> > +{
> > +     bool update;
> > +     struct power_allocator_params *params = tz->governor_data;
> > +     int last_switch_on_temp = params->last_switch_on_temp;
> > +
> > +     update = (tz->last_temperature >= last_switch_on_temp);
> > +     params->last_switch_on_temp = switch_on_temp;
> > +
> > +     return update;
> > +}
> > +#else
> > +static inline bool thermal_cdev_needs_update(struct thermal_zone_device *tz, int switch_on_temp)
> > +{
> > +     return false;
> > +}
> > +#endif
> > +
> >   static void reset_pid_controller(struct power_allocator_params *params)
> >   {
> >       params->err_integral = 0;
> > @@ -709,12 +733,15 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> >               return 0;
> >
> >       ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
> > -     if (!ret && (tz->temperature < trip.temperature)) {
> > -             update = (tz->last_temperature >= trip.temperature);
> > -             tz->passive = 0;
> > -             reset_pid_controller(params);
> > -             allow_maximum_power(tz, update);
> > -             return 0;
> > +     if (!ret) {
> > +             update = thermal_cdev_needs_update(tz, trip.temperature);
> > +             if (tz->temperature < trip.temperature) {
> > +                     update |= (tz->last_temperature >= trip.temperature);
> > +                     tz->passive = 0;
> > +                     reset_pid_controller(params);
> > +                     allow_maximum_power(tz, update);
> > +                     return 0;
> > +             }
> >       }
> >
> >       tz->passive = 1;
>
>
> Thanks for the patch. The code looks good. The initial value of
> 'last_switch_on_temp' would be set to 0. It won't harm us because it
> will get the proper value later.
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Daniel Lezcano]

On 10/04/2023 04:09, Di Shen wrote:
> Hi Lukasz,
> Could you please apply this patch if there's no more comment? Thank you.

Hi,

I take care of applying the patches. Give me some time to read the changes.

Thanks
   -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Lukasz Luba]

On 4/10/23 20:51, Daniel Lezcano wrote:
> On 10/04/2023 04:09, Di Shen wrote:
>> Hi Lukasz,
>> Could you please apply this patch if there's no more comment? Thank you.
> 
> Hi,
> 
> I take care of applying the patches. Give me some time to read the changes.
> 
> Thanks
>    -- Daniel
> 

Thank you Daniel!

Regards,
Lukasz

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Di Shen]

Thank you Daniel. Any comments would be appreciated!

Best regards,
Di


On Tue, Apr 11, 2023 at 3:51 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 10/04/2023 04:09, Di Shen wrote:
> > Hi Lukasz,
> > Could you please apply this patch if there's no more comment? Thank you.
>
> Hi,
>
> I take care of applying the patches. Give me some time to read the changes.
>
> Thanks
>    -- Daniel
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Daniel Lezcano]

On 13/04/2023 06:51, Di Shen wrote:
> Thank you Daniel. Any comments would be appreciated!

Still thinking about the changes...

For my understanding, why do you change the 'switch_on' trip point on 
the fly ?



> On Tue, Apr 11, 2023 at 3:51 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 10/04/2023 04:09, Di Shen wrote:
>>> Hi Lukasz,
>>> Could you please apply this patch if there's no more comment? Thank you.
>>
>> Hi,
>>
>> I take care of applying the patches. Give me some time to read the changes.
>>
>> Thanks
>>     -- Daniel
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Di Shen]

We have discussed this question in patch-v1:
https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm.com/

Simply put, we use the trip_temp in the Android System; set different
trip_temp for thermal control of different scenarios.

On Thu, Apr 13, 2023 at 3:37 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 13/04/2023 06:51, Di Shen wrote:
> > Thank you Daniel. Any comments would be appreciated!
>
> Still thinking about the changes...
>
> For my understanding, why do you change the 'switch_on' trip point on
> the fly ?
>
>
>
> > On Tue, Apr 11, 2023 at 3:51 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 10/04/2023 04:09, Di Shen wrote:
> >>> Hi Lukasz,
> >>> Could you please apply this patch if there's no more comment? Thank you.
> >>
> >> Hi,
> >>
> >> I take care of applying the patches. Give me some time to read the changes.
> >>
> >> Thanks
> >>     -- Daniel
> >>
> >> --
> >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> >>
> >> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> >> <http://twitter.com/#!/linaroorg> Twitter |
> >> <http://www.linaro.org/linaro-blog/> Blog
> >>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Daniel Lezcano]

On 13/04/2023 10:40, Di Shen wrote:
> We have discussed this question in patch-v1:
> https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm.com/
> 
> Simply put, we use the trip_temp in the Android System; set different
> trip_temp for thermal control of different scenarios.

The changes are dealing with the trip points and trying to detect the 
threshold. That part should be handled in the thermal core or thermal 
trip side, not in the governor.

AFAICT, if a trip point is changed, then the power allocator should be 
reset, including the cdev state.

It would be more convenient to add an ops to the governor ops structure 
to reset the governor and then call it when a trip point is changed in 
thermal_zone_set_trip()


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Lukasz Luba]

On 4/14/23 12:12, Daniel Lezcano wrote:
> On 13/04/2023 10:40, Di Shen wrote:
>> We have discussed this question in patch-v1:
>> https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm.com/
>>
>> Simply put, we use the trip_temp in the Android System; set different
>> trip_temp for thermal control of different scenarios.
> 
> The changes are dealing with the trip points and trying to detect the 
> threshold. That part should be handled in the thermal core or thermal 
> trip side, not in the governor.
> 
> AFAICT, if a trip point is changed, then the power allocator should be 
> reset, including the cdev state.
> 
> It would be more convenient to add an ops to the governor ops structure 
> to reset the governor and then call it when a trip point is changed in 
> thermal_zone_set_trip()
> 
> 

Sounds reasonable to have a proper API and fwk handling this corner
case scenario.
Although, if there is a need for a 'easy-to-backport' fix for IPA only,
I agree with this patch, since it's straight forward to put in some
Android kernel. We can later fix the framework to handle this properly.
Anyway, both ways are OK to me.

Regards,
Lukasz

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Daniel Lezcano]

On 14/04/2023 16:18, Lukasz Luba wrote:
> 
> 
> On 4/14/23 12:12, Daniel Lezcano wrote:
>> On 13/04/2023 10:40, Di Shen wrote:
>>> We have discussed this question in patch-v1:
>>> https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm.com/
>>>
>>> Simply put, we use the trip_temp in the Android System; set different
>>> trip_temp for thermal control of different scenarios.
>>
>> The changes are dealing with the trip points and trying to detect the 
>> threshold. That part should be handled in the thermal core or thermal 
>> trip side, not in the governor.
>>
>> AFAICT, if a trip point is changed, then the power allocator should be 
>> reset, including the cdev state.
>>
>> It would be more convenient to add an ops to the governor ops 
>> structure to reset the governor and then call it when a trip point is 
>> changed in thermal_zone_set_trip()
>>
>>
> 
> Sounds reasonable to have a proper API and fwk handling this corner
> case scenario.
> Although, if there is a need for a 'easy-to-backport' fix for IPA only,
> I agree with this patch, since it's straight forward to put in some
> Android kernel. We can later fix the framework to handle this properly.
> Anyway, both ways are OK to me.

Unfortunately, we can not do the maintenance of the Linux kernel based 
on an 'easy-to-backport' policy to Android.

This patch could be applied from-list to Android as a hotfix. But for 
Linux the fix should be more elaborated. One solution is to add a 
'reset' ops and call it from the trip point update function.

Did you double check the issue is not impacting the other governors too ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Lukasz Luba]

On 4/14/23 16:06, Daniel Lezcano wrote:
> On 14/04/2023 16:18, Lukasz Luba wrote:
>>
>>
>> On 4/14/23 12:12, Daniel Lezcano wrote:
>>> On 13/04/2023 10:40, Di Shen wrote:
>>>> We have discussed this question in patch-v1:
>>>> https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm.com/
>>>>
>>>> Simply put, we use the trip_temp in the Android System; set different
>>>> trip_temp for thermal control of different scenarios.
>>>
>>> The changes are dealing with the trip points and trying to detect the 
>>> threshold. That part should be handled in the thermal core or thermal 
>>> trip side, not in the governor.
>>>
>>> AFAICT, if a trip point is changed, then the power allocator should 
>>> be reset, including the cdev state.
>>>
>>> It would be more convenient to add an ops to the governor ops 
>>> structure to reset the governor and then call it when a trip point is 
>>> changed in thermal_zone_set_trip()
>>>
>>>
>>
>> Sounds reasonable to have a proper API and fwk handling this corner
>> case scenario.
>> Although, if there is a need for a 'easy-to-backport' fix for IPA only,
>> I agree with this patch, since it's straight forward to put in some
>> Android kernel. We can later fix the framework to handle this properly.
>> Anyway, both ways are OK to me.
> 
> Unfortunately, we can not do the maintenance of the Linux kernel based 
> on an 'easy-to-backport' policy to Android.
> 
> This patch could be applied from-list to Android as a hotfix. But for 
> Linux the fix should be more elaborated. One solution is to add a 
> 'reset' ops and call it from the trip point update function.

Fair enough.

> 
> Did you double check the issue is not impacting the other governors too ?

No, unfortunately, I haven't checked other governors.

Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset

[Di Shen]

On Fri, Apr 14, 2023 at 11:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 4/14/23 16:06, Daniel Lezcano wrote:
> > On 14/04/2023 16:18, Lukasz Luba wrote:
> >>
> >>
> >> On 4/14/23 12:12, Daniel Lezcano wrote:
> >>> On 13/04/2023 10:40, Di Shen wrote:
> >>>> We have discussed this question in patch-v1:
> >>>> https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm.com/
> >>>>
> >>>> Simply put, we use the trip_temp in the Android System; set different
> >>>> trip_temp for thermal control of different scenarios.
> >>>
> >>> The changes are dealing with the trip points and trying to detect the
> >>> threshold. That part should be handled in the thermal core or thermal
> >>> trip side, not in the governor.
> >>>
> >>> AFAICT, if a trip point is changed, then the power allocator should
> >>> be reset, including the cdev state.
> >>>
> >>> It would be more convenient to add an ops to the governor ops
> >>> structure to reset the governor and then call it when a trip point is
> >>> changed in thermal_zone_set_trip()
> >>>
> >>>
> >>
> >> Sounds reasonable to have a proper API and fwk handling this corner
> >> case scenario.
> >> Although, if there is a need for a 'easy-to-backport' fix for IPA only,
> >> I agree with this patch, since it's straight forward to put in some
> >> Android kernel. We can later fix the framework to handle this properly.
> >> Anyway, both ways are OK to me.
> >
> > Unfortunately, we can not do the maintenance of the Linux kernel based
> > on an 'easy-to-backport' policy to Android.
> >
> > This patch could be applied from-list to Android as a hotfix. But for
> > Linux the fix should be more elaborated. One solution is to add a
> > 'reset' ops and call it from the trip point update function.
>
> Fair enough.
>
> >
> > Did you double check the issue is not impacting the other governors too ?
>
> No, unfortunately, I haven't checked other governors.

Hi Lukasz and Daniel,
I rethought about this issue, and have tried three ways to solve it.
Finally, I realized that the root cause might be the cdev->state
update and notify. We should get back to take cdev->state into
account.

The three ways:
1.From trips updating perspective:
As your suggestion，add an ops function for thermal_governor
structure，define it in IPA governor, and call it when trips are
changed by userspace(sysfs node).

2.From cdev->state updating perspective:
For example, for gov_power_allocator there are two branches reached to
__thermal_cdev_update.

power_allocator_trottle
        |
allow_maximum_power()[gov_power_allocator.c]
        |
__thermal_cdev_update()[thermal_helpers.c]<<<<<<<(1)
        |
thermal_cdev_set_cur_state()
        |
cdev->ops->set_cur_state()
        |
thermal_notify_cdev_state_update()
        |
     .......


power_allocator_throttle()[gov_power_allocator.c]
        |
allocate_power()
        |
power_actor_set_power()
        |
__thermal_cdev_update()[thermal_helpers.c]<<<<<<(2)
        |
      ......

Add a variable last_state for thermal_cooling_device structure to
record the last target cooling state, and before
thermal_notify_cdev_state_update, determine whether the last_state is
equal to current state. If not equal, then
thermal_notify_cdev_state_update.

static void thermal_cdev_set_cur_state(struct thermal_cooling_device
*cdev,
~                                        unsigned long target)
{
        if (cdev->ops->set_cur_state(cdev, target))
                return;

~       if (cdev->last_state != target)
+               thermal_notify_cdev_state_update(cdev->id, target);
+
+       cdev->last_state = target;
+
        thermal_cooling_device_stats_update(cdev, target);
}

In this way, it will only update and notify when the state is changed
which means there's no need to use update flag to make sure it updates
cdev->state only once.

It can avoid a lot of unnecessary notifications, not only when the
temperature drops below the first trip point(at this situation
cdev->state is always 0) but also when the policy is working.

3.Similar to the second method, but an easier one.
Actually, in the set_cur_state ops of every cooling device, it has
already checked whether the last cooling state is equal to current
value or not, and returns 0. Like cpufreq cooling device:
static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
                                unsigned long state)
{
        //.......
        /* Request state should be less than max_level */
        if (state > cpufreq_cdev->max_level)
                return -EINVAL;

        /* Check if the old cooling action is same as new cooling
action */
        if (cpufreq_cdev->cpufreq_state == state)
                return 0; //return -EAGAIN;
}

What if return a non-zero value? 1 or -EAGAIN(means thy again)? Then
thermal_cdev_set_cur_state() in __thermal_cdev_update() can return
directly without update and notify.

I prefer method 3. Because there's no more new variable or function
compared to 1 and 2, and it can make code more brief.

Well, what do you think about the three ways? Look forward to your
comments. Thank you!

Best regards,
Di

[PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Di Shen]

When the thermal trip point is changed, the governor should
be reset so that the policy algorithm be updated to adapt to the
new trip point.

This patch adds an ops for thermal the governor structure to reset
the governor. The ops is called when the trip point is changed.
For power allocator, the parameters of pid controller and the states
of power cooling devices can be reset when the passive trip point
is changed.

Signed-off-by: Di Shen <di.shen@unisoc.com>

---
V4:
- Compared to V3, handle it in thermal core instead of in governor.

- Add an ops to the governor structure, and call it when a trip
  point is changed.

- Define reset ops for power allocator.

V3:
- Add fix tag.

V2:
- Compared to v1, do not revert.

- Add a variable(last_switch_on_temp) in power_allocator_params
  to record the last switch_on_temp value.

- Adds a function to renew the update flag and update the
  last_switch_on_temp when thermal trips are writable.

V1:
- Revert commit 0952177f2a1f.
---
---
 drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
 drivers/thermal/thermal_trip.c        |  6 ++++++
 include/linux/thermal.h               |  1 +
 3 files changed, 28 insertions(+)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 8642f1096b91..41d155adc616 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
 	return allocate_power(tz, trip.temperature);
 }
 
+static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
+{
+	int ret = 0;
+	struct thermal_trip trip;
+	struct power_allocator_params *params = tz->governor_data;
+
+	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
+	if (ret)
+		return ret;
+
+	/* Only need reset for passive trips */
+	if (trip.type != THERMAL_TRIP_PASSIVE)
+		return -EINVAL;
+
+	reset_pid_controller(params);
+	allow_maximum_power(tz, true);
+
+	return ret;
+}
+
 static struct thermal_governor thermal_gov_power_allocator = {
 	.name		= "power_allocator",
 	.bind_to_tz	= power_allocator_bind,
 	.unbind_from_tz	= power_allocator_unbind,
 	.throttle	= power_allocator_throttle,
+	.reset		= power_allocator_reset,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
index 907f3a4d7bc8..52eb768fada8 100644
--- a/drivers/thermal/thermal_trip.c
+++ b/drivers/thermal/thermal_trip.c
@@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
 	if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
 		tz->trips[trip_id] = *trip;
 
+	if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
+		ret = tz->governor->reset(tz, trip_id);
+		if (ret)
+			pr_warn_once("Failed to reset thermal governor\n");
+	}
+
 	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
 				      trip->temperature, trip->hysteresis);
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 87837094d549..155ce2291fa5 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -204,6 +204,7 @@ struct thermal_governor {
 	int (*bind_to_tz)(struct thermal_zone_device *tz);
 	void (*unbind_from_tz)(struct thermal_zone_device *tz);
 	int (*throttle)(struct thermal_zone_device *tz, int trip);
+	int (*reset)(struct thermal_zone_device *tz, int trip);
 	struct list_head	governor_list;
 };
 
-- 
2.17.1

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Rafael J. Wysocki]

On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@unisoc.com> wrote:
>
> When the thermal trip point is changed, the governor should
> be reset so that the policy algorithm be updated to adapt to the
> new trip point.
>
> This patch adds an ops for thermal the governor structure to reset
> the governor. The ops is called when the trip point is changed.
> For power allocator, the parameters of pid controller and the states
> of power cooling devices can be reset when the passive trip point
> is changed.
>
> Signed-off-by: Di Shen <di.shen@unisoc.com>
>
> ---
> V4:
> - Compared to V3, handle it in thermal core instead of in governor.
>
> - Add an ops to the governor structure, and call it when a trip
>   point is changed.
>
> - Define reset ops for power allocator.
>
> V3:
> - Add fix tag.
>
> V2:
> - Compared to v1, do not revert.
>
> - Add a variable(last_switch_on_temp) in power_allocator_params
>   to record the last switch_on_temp value.
>
> - Adds a function to renew the update flag and update the
>   last_switch_on_temp when thermal trips are writable.
>
> V1:
> - Revert commit 0952177f2a1f.
> ---
> ---
>  drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
>  drivers/thermal/thermal_trip.c        |  6 ++++++
>  include/linux/thermal.h               |  1 +
>  3 files changed, 28 insertions(+)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 8642f1096b91..41d155adc616 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
>         return allocate_power(tz, trip.temperature);
>  }
>
> +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> +{
> +       int ret = 0;
> +       struct thermal_trip trip;
> +       struct power_allocator_params *params = tz->governor_data;
> +
> +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> +       if (ret)
> +               return ret;
> +
> +       /* Only need reset for passive trips */
> +       if (trip.type != THERMAL_TRIP_PASSIVE)
> +               return -EINVAL;
> +
> +       reset_pid_controller(params);
> +       allow_maximum_power(tz, true);
> +
> +       return ret;
> +}
> +
>  static struct thermal_governor thermal_gov_power_allocator = {
>         .name           = "power_allocator",
>         .bind_to_tz     = power_allocator_bind,
>         .unbind_from_tz = power_allocator_unbind,
>         .throttle       = power_allocator_throttle,
> +       .reset          = power_allocator_reset,
>  };
>  THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> index 907f3a4d7bc8..52eb768fada8 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>         if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
>                 tz->trips[trip_id] = *trip;
>
> +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> +               ret = tz->governor->reset(tz, trip_id);
> +               if (ret)
> +                       pr_warn_once("Failed to reset thermal governor\n");

I'm not really sure if it is useful to print this message here.

First off, the governors may print more precise diagnostic messages if
they care.

Second, what is the sysadmin supposed to do in response to this message?

> +       }
> +
>         thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
>                                       trip->temperature, trip->hysteresis);
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 87837094d549..155ce2291fa5 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -204,6 +204,7 @@ struct thermal_governor {
>         int (*bind_to_tz)(struct thermal_zone_device *tz);
>         void (*unbind_from_tz)(struct thermal_zone_device *tz);
>         int (*throttle)(struct thermal_zone_device *tz, int trip);
> +       int (*reset)(struct thermal_zone_device *tz, int trip);
>         struct list_head        governor_list;
>  };
>
> --
> 2.17.1
>

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Rafael J. Wysocki]

On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@unisoc.com> wrote:
> >
> > When the thermal trip point is changed, the governor should
> > be reset so that the policy algorithm be updated to adapt to the
> > new trip point.
> >
> > This patch adds an ops for thermal the governor structure to reset
> > the governor. The ops is called when the trip point is changed.
> > For power allocator, the parameters of pid controller and the states
> > of power cooling devices can be reset when the passive trip point
> > is changed.
> >
> > Signed-off-by: Di Shen <di.shen@unisoc.com>
> >
> > ---
> > V4:
> > - Compared to V3, handle it in thermal core instead of in governor.
> >
> > - Add an ops to the governor structure, and call it when a trip
> >   point is changed.
> >
> > - Define reset ops for power allocator.
> >
> > V3:
> > - Add fix tag.
> >
> > V2:
> > - Compared to v1, do not revert.
> >
> > - Add a variable(last_switch_on_temp) in power_allocator_params
> >   to record the last switch_on_temp value.
> >
> > - Adds a function to renew the update flag and update the
> >   last_switch_on_temp when thermal trips are writable.
> >
> > V1:
> > - Revert commit 0952177f2a1f.
> > ---
> > ---
> >  drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> >  drivers/thermal/thermal_trip.c        |  6 ++++++
> >  include/linux/thermal.h               |  1 +
> >  3 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 8642f1096b91..41d155adc616 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> >         return allocate_power(tz, trip.temperature);
> >  }
> >
> > +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> > +{
> > +       int ret = 0;
> > +       struct thermal_trip trip;
> > +       struct power_allocator_params *params = tz->governor_data;
> > +
> > +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Only need reset for passive trips */
> > +       if (trip.type != THERMAL_TRIP_PASSIVE)
> > +               return -EINVAL;
> > +
> > +       reset_pid_controller(params);
> > +       allow_maximum_power(tz, true);
> > +
> > +       return ret;
> > +}
> > +
> >  static struct thermal_governor thermal_gov_power_allocator = {
> >         .name           = "power_allocator",
> >         .bind_to_tz     = power_allocator_bind,
> >         .unbind_from_tz = power_allocator_unbind,
> >         .throttle       = power_allocator_throttle,
> > +       .reset          = power_allocator_reset,
> >  };
> >  THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > index 907f3a4d7bc8..52eb768fada8 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> >         if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> >                 tz->trips[trip_id] = *trip;
> >
> > +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> > +               ret = tz->governor->reset(tz, trip_id);
> > +               if (ret)
> > +                       pr_warn_once("Failed to reset thermal governor\n");
>
> I'm not really sure if it is useful to print this message here.
>
> First off, the governors may print more precise diagnostic messages if
> they care.
>
> Second, what is the sysadmin supposed to do in response to this message?

In addition to the above, trip point temperatures may be updated in
other places too, for instance in response to notifications from
platform firmware and IMV this new callback should be also used in
those cases.  However, in those cases multiple trip points may change
at a time and the critical/hot trip point temperatures may be updated
too AFAICS.

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

Hi Rafael,


On 6/20/23 11:07, Rafael J. Wysocki wrote:
> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@unisoc.com> wrote:
>>>
>>> When the thermal trip point is changed, the governor should
>>> be reset so that the policy algorithm be updated to adapt to the
>>> new trip point.
>>>
>>> This patch adds an ops for thermal the governor structure to reset
>>> the governor. The ops is called when the trip point is changed.
>>> For power allocator, the parameters of pid controller and the states
>>> of power cooling devices can be reset when the passive trip point
>>> is changed.
>>>
>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
>>>
>>> ---
>>> V4:
>>> - Compared to V3, handle it in thermal core instead of in governor.
>>>
>>> - Add an ops to the governor structure, and call it when a trip
>>>    point is changed.
>>>
>>> - Define reset ops for power allocator.
>>>
>>> V3:
>>> - Add fix tag.
>>>
>>> V2:
>>> - Compared to v1, do not revert.
>>>
>>> - Add a variable(last_switch_on_temp) in power_allocator_params
>>>    to record the last switch_on_temp value.
>>>
>>> - Adds a function to renew the update flag and update the
>>>    last_switch_on_temp when thermal trips are writable.
>>>
>>> V1:
>>> - Revert commit 0952177f2a1f.
>>> ---
>>> ---
>>>   drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
>>>   drivers/thermal/thermal_trip.c        |  6 ++++++
>>>   include/linux/thermal.h               |  1 +
>>>   3 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>>> index 8642f1096b91..41d155adc616 100644
>>> --- a/drivers/thermal/gov_power_allocator.c
>>> +++ b/drivers/thermal/gov_power_allocator.c
>>> @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
>>>          return allocate_power(tz, trip.temperature);
>>>   }
>>>
>>> +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
>>> +{
>>> +       int ret = 0;
>>> +       struct thermal_trip trip;
>>> +       struct power_allocator_params *params = tz->governor_data;
>>> +
>>> +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       /* Only need reset for passive trips */
>>> +       if (trip.type != THERMAL_TRIP_PASSIVE)
>>> +               return -EINVAL;
>>> +
>>> +       reset_pid_controller(params);
>>> +       allow_maximum_power(tz, true);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   static struct thermal_governor thermal_gov_power_allocator = {
>>>          .name           = "power_allocator",
>>>          .bind_to_tz     = power_allocator_bind,
>>>          .unbind_from_tz = power_allocator_unbind,
>>>          .throttle       = power_allocator_throttle,
>>> +       .reset          = power_allocator_reset,
>>>   };
>>>   THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
>>> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
>>> index 907f3a4d7bc8..52eb768fada8 100644
>>> --- a/drivers/thermal/thermal_trip.c
>>> +++ b/drivers/thermal/thermal_trip.c
>>> @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>>>          if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
>>>                  tz->trips[trip_id] = *trip;
>>>
>>> +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
>>> +               ret = tz->governor->reset(tz, trip_id);
>>> +               if (ret)
>>> +                       pr_warn_once("Failed to reset thermal governor\n");
>>
>> I'm not really sure if it is useful to print this message here.
>>
>> First off, the governors may print more precise diagnostic messages if
>> they care.
>>
>> Second, what is the sysadmin supposed to do in response to this message?
> 
> In addition to the above, trip point temperatures may be updated in
> other places too, for instance in response to notifications from
> platform firmware and IMV this new callback should be also used in
> those cases.  However, in those cases multiple trip points may change
> at a time and the critical/hot trip point temperatures may be updated
> too AFAICS.

IIRC the critical/hot trip points are handled differently, not using the
governors. The governors' 'throttle' callback would be called only
after we pass the test of 'critical/hot' [1].

What Di is facing is in the issue under the bucket of
'handle_non_critical_trips()' when the governor just tries to
work on stale data - old trip temp.

For the 2nd case IIUC the code, we pass the 'trip.temperature'
and should be ready for what you said (modification of that value).
Furthermore, the critical/hot situation is handled w/o governor
assistance, so we should be safe:
tz->ops->critical(tz) or tz->ops->hot(tz) and not
tz->governor->throttle(tz, trip)

Would you agree Rafael?

Regards,
Lukasz

[1] 
https://elixir.bootlin.com/linux/v6.3/source/drivers/thermal/thermal_core.c#L370

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

Hi Di,

I have missed your v4 because it landed below your v3 thread.

On 6/19/23 07:35, Di Shen wrote:
> When the thermal trip point is changed, the governor should
> be reset so that the policy algorithm be updated to adapt to the
> new trip point.
> 
> This patch adds an ops for thermal the governor structure to reset

s/ops/callback

> the governor. The ops is called when the trip point is changed.
> For power allocator, the parameters of pid controller and the states
> of power cooling devices can be reset when the passive trip point
> is changed.
> 
> Signed-off-by: Di Shen <di.shen@unisoc.com>
> 
> ---
> V4:
> - Compared to V3, handle it in thermal core instead of in governor.
> 
> - Add an ops to the governor structure, and call it when a trip
>    point is changed.
> 
> - Define reset ops for power allocator.
> 
> V3:
> - Add fix tag.
> 
> V2:
> - Compared to v1, do not revert.
> 
> - Add a variable(last_switch_on_temp) in power_allocator_params
>    to record the last switch_on_temp value.
> 
> - Adds a function to renew the update flag and update the
>    last_switch_on_temp when thermal trips are writable.
> 
> V1:
> - Revert commit 0952177f2a1f.
> ---
> ---
>   drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
>   drivers/thermal/thermal_trip.c        |  6 ++++++
>   include/linux/thermal.h               |  1 +
>   3 files changed, 28 insertions(+)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 8642f1096b91..41d155adc616 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
>   	return allocate_power(tz, trip.temperature);
>   }
>   
> +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> +{
> +	int ret = 0;
> +	struct thermal_trip trip;
> +	struct power_allocator_params *params = tz->governor_data;
> +
> +	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> +	if (ret)
> +		return ret;
> +
> +	/* Only need reset for passive trips */
> +	if (trip.type != THERMAL_TRIP_PASSIVE)
> +		return -EINVAL;
> +
> +	reset_pid_controller(params);
> +	allow_maximum_power(tz, true);
> +
> +	return ret;
> +}
> +
>   static struct thermal_governor thermal_gov_power_allocator = {
>   	.name		= "power_allocator",
>   	.bind_to_tz	= power_allocator_bind,
>   	.unbind_from_tz	= power_allocator_unbind,
>   	.throttle	= power_allocator_throttle,
> +	.reset		= power_allocator_reset,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> index 907f3a4d7bc8..52eb768fada8 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>   	if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
>   		tz->trips[trip_id] = *trip;
>   
> +	if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> +		ret = tz->governor->reset(tz, trip_id);
> +		if (ret)
> +			pr_warn_once("Failed to reset thermal governor\n");
> +	}

I agree with Rafael. Maybe change that to debug print, so that can be
checked during the product testing. We cannot do much if that happens.

> +
>   	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
>   				      trip->temperature, trip->hysteresis);
>   
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 87837094d549..155ce2291fa5 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -204,6 +204,7 @@ struct thermal_governor {
>   	int (*bind_to_tz)(struct thermal_zone_device *tz);
>   	void (*unbind_from_tz)(struct thermal_zone_device *tz);
>   	int (*throttle)(struct thermal_zone_device *tz, int trip);
> +	int (*reset)(struct thermal_zone_device *tz, int trip);
>   	struct list_head	governor_list;
>   };
>   

That thermal_governor::reset() callback is what I had im mind while
giving you the feedback for the v1. Now it's much cleaner what is going
on and why.

Apart from some small bits, LGTM. Please adjust the comment in the patch
header and this debug print and you can add:

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Please send the next version as separate new thread.

Regards,
Lukasz

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Rafael J. Wysocki]

On Tue, Jun 20, 2023 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
>
> On 6/20/23 11:07, Rafael J. Wysocki wrote:
> > On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@unisoc.com> wrote:
> >>>
> >>> When the thermal trip point is changed, the governor should
> >>> be reset so that the policy algorithm be updated to adapt to the
> >>> new trip point.
> >>>
> >>> This patch adds an ops for thermal the governor structure to reset
> >>> the governor. The ops is called when the trip point is changed.
> >>> For power allocator, the parameters of pid controller and the states
> >>> of power cooling devices can be reset when the passive trip point
> >>> is changed.
> >>>
> >>> Signed-off-by: Di Shen <di.shen@unisoc.com>
> >>>
> >>> ---
> >>> V4:
> >>> - Compared to V3, handle it in thermal core instead of in governor.
> >>>
> >>> - Add an ops to the governor structure, and call it when a trip
> >>>    point is changed.
> >>>
> >>> - Define reset ops for power allocator.
> >>>
> >>> V3:
> >>> - Add fix tag.
> >>>
> >>> V2:
> >>> - Compared to v1, do not revert.
> >>>
> >>> - Add a variable(last_switch_on_temp) in power_allocator_params
> >>>    to record the last switch_on_temp value.
> >>>
> >>> - Adds a function to renew the update flag and update the
> >>>    last_switch_on_temp when thermal trips are writable.
> >>>
> >>> V1:
> >>> - Revert commit 0952177f2a1f.
> >>> ---
> >>> ---
> >>>   drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> >>>   drivers/thermal/thermal_trip.c        |  6 ++++++
> >>>   include/linux/thermal.h               |  1 +
> >>>   3 files changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> >>> index 8642f1096b91..41d155adc616 100644
> >>> --- a/drivers/thermal/gov_power_allocator.c
> >>> +++ b/drivers/thermal/gov_power_allocator.c
> >>> @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> >>>          return allocate_power(tz, trip.temperature);
> >>>   }
> >>>
> >>> +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> >>> +{
> >>> +       int ret = 0;
> >>> +       struct thermal_trip trip;
> >>> +       struct power_allocator_params *params = tz->governor_data;
> >>> +
> >>> +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> >>> +       if (ret)
> >>> +               return ret;
> >>> +
> >>> +       /* Only need reset for passive trips */
> >>> +       if (trip.type != THERMAL_TRIP_PASSIVE)
> >>> +               return -EINVAL;
> >>> +
> >>> +       reset_pid_controller(params);
> >>> +       allow_maximum_power(tz, true);
> >>> +
> >>> +       return ret;
> >>> +}
> >>> +
> >>>   static struct thermal_governor thermal_gov_power_allocator = {
> >>>          .name           = "power_allocator",
> >>>          .bind_to_tz     = power_allocator_bind,
> >>>          .unbind_from_tz = power_allocator_unbind,
> >>>          .throttle       = power_allocator_throttle,
> >>> +       .reset          = power_allocator_reset,
> >>>   };
> >>>   THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> >>> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> >>> index 907f3a4d7bc8..52eb768fada8 100644
> >>> --- a/drivers/thermal/thermal_trip.c
> >>> +++ b/drivers/thermal/thermal_trip.c
> >>> @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> >>>          if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> >>>                  tz->trips[trip_id] = *trip;
> >>>
> >>> +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> >>> +               ret = tz->governor->reset(tz, trip_id);
> >>> +               if (ret)
> >>> +                       pr_warn_once("Failed to reset thermal governor\n");
> >>
> >> I'm not really sure if it is useful to print this message here.
> >>
> >> First off, the governors may print more precise diagnostic messages if
> >> they care.
> >>
> >> Second, what is the sysadmin supposed to do in response to this message?
> >
> > In addition to the above, trip point temperatures may be updated in
> > other places too, for instance in response to notifications from
> > platform firmware and IMV this new callback should be also used in
> > those cases.  However, in those cases multiple trip points may change
> > at a time and the critical/hot trip point temperatures may be updated
> > too AFAICS.
>
> IIRC the critical/hot trip points are handled differently, not using the
> governors. The governors' 'throttle' callback would be called only
> after we pass the test of 'critical/hot' [1].

OK, but is it actually useful to return an error code from the
->reset() callback when passed a non-passive trip point?

> What Di is facing is in the issue under the bucket of
> 'handle_non_critical_trips()' when the governor just tries to
> work on stale data - old trip temp.

Well, fair enough, but what about the other governors?  Is this
problem limited to power_allocator?

> For the 2nd case IIUC the code, we pass the 'trip.temperature'
> and should be ready for what you said (modification of that value).

Generally speaking, it needs to be prepared for a simultaneous change
of multiple trip points (including active), in which case it may not
be useful to invoke the ->reset() callback for each of them
individually.

Moreover, Daniel wants trip points to be sorted by temperature
eventually and in that case indices may change overall even on one
trip point update.

> Furthermore, the critical/hot situation is handled w/o governor
> assistance, so we should be safe:
> tz->ops->critical(tz) or tz->ops->hot(tz) and not
> tz->governor->throttle(tz, trip)

That's fine.

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

On 6/20/23 11:39, Rafael J. Wysocki wrote:
> On Tue, Jun 20, 2023 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>>
>> On 6/20/23 11:07, Rafael J. Wysocki wrote:
>>> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>
>>>> On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@unisoc.com> wrote:
>>>>>
>>>>> When the thermal trip point is changed, the governor should
>>>>> be reset so that the policy algorithm be updated to adapt to the
>>>>> new trip point.
>>>>>
>>>>> This patch adds an ops for thermal the governor structure to reset
>>>>> the governor. The ops is called when the trip point is changed.
>>>>> For power allocator, the parameters of pid controller and the states
>>>>> of power cooling devices can be reset when the passive trip point
>>>>> is changed.
>>>>>
>>>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
>>>>>
>>>>> ---
>>>>> V4:
>>>>> - Compared to V3, handle it in thermal core instead of in governor.
>>>>>
>>>>> - Add an ops to the governor structure, and call it when a trip
>>>>>     point is changed.
>>>>>
>>>>> - Define reset ops for power allocator.
>>>>>
>>>>> V3:
>>>>> - Add fix tag.
>>>>>
>>>>> V2:
>>>>> - Compared to v1, do not revert.
>>>>>
>>>>> - Add a variable(last_switch_on_temp) in power_allocator_params
>>>>>     to record the last switch_on_temp value.
>>>>>
>>>>> - Adds a function to renew the update flag and update the
>>>>>     last_switch_on_temp when thermal trips are writable.
>>>>>
>>>>> V1:
>>>>> - Revert commit 0952177f2a1f.
>>>>> ---
>>>>> ---
>>>>>    drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
>>>>>    drivers/thermal/thermal_trip.c        |  6 ++++++
>>>>>    include/linux/thermal.h               |  1 +
>>>>>    3 files changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>>>>> index 8642f1096b91..41d155adc616 100644
>>>>> --- a/drivers/thermal/gov_power_allocator.c
>>>>> +++ b/drivers/thermal/gov_power_allocator.c
>>>>> @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
>>>>>           return allocate_power(tz, trip.temperature);
>>>>>    }
>>>>>
>>>>> +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
>>>>> +{
>>>>> +       int ret = 0;
>>>>> +       struct thermal_trip trip;
>>>>> +       struct power_allocator_params *params = tz->governor_data;
>>>>> +
>>>>> +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>> +       /* Only need reset for passive trips */
>>>>> +       if (trip.type != THERMAL_TRIP_PASSIVE)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       reset_pid_controller(params);
>>>>> +       allow_maximum_power(tz, true);
>>>>> +
>>>>> +       return ret;
>>>>> +}
>>>>> +
>>>>>    static struct thermal_governor thermal_gov_power_allocator = {
>>>>>           .name           = "power_allocator",
>>>>>           .bind_to_tz     = power_allocator_bind,
>>>>>           .unbind_from_tz = power_allocator_unbind,
>>>>>           .throttle       = power_allocator_throttle,
>>>>> +       .reset          = power_allocator_reset,
>>>>>    };
>>>>>    THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
>>>>> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
>>>>> index 907f3a4d7bc8..52eb768fada8 100644
>>>>> --- a/drivers/thermal/thermal_trip.c
>>>>> +++ b/drivers/thermal/thermal_trip.c
>>>>> @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>>>>>           if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
>>>>>                   tz->trips[trip_id] = *trip;
>>>>>
>>>>> +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
>>>>> +               ret = tz->governor->reset(tz, trip_id);
>>>>> +               if (ret)
>>>>> +                       pr_warn_once("Failed to reset thermal governor\n");
>>>>
>>>> I'm not really sure if it is useful to print this message here.
>>>>
>>>> First off, the governors may print more precise diagnostic messages if
>>>> they care.
>>>>
>>>> Second, what is the sysadmin supposed to do in response to this message?
>>>
>>> In addition to the above, trip point temperatures may be updated in
>>> other places too, for instance in response to notifications from
>>> platform firmware and IMV this new callback should be also used in
>>> those cases.  However, in those cases multiple trip points may change
>>> at a time and the critical/hot trip point temperatures may be updated
>>> too AFAICS.
>>
>> IIRC the critical/hot trip points are handled differently, not using the
>> governors. The governors' 'throttle' callback would be called only
>> after we pass the test of 'critical/hot' [1].
> 
> OK, but is it actually useful to return an error code from the
> ->reset() callback when passed a non-passive trip point?

It will depend on the governor code. In our case the setup code
w.r.t. trip types is quite confusing (to fit into many possible
configurations). The non-passive trip point would be only
possible to bind when there are not other passive trip points.
That's is a really corner case and probably never used on any
device. Therefore, IMO we can just bail out in such situation
when then someone tries to update such single non-passive
trip point (probably not aware what is doing with IPA?).

For the rest of the governors - it's up to them what they
report in case non-passive trip is updated...

> 
>> What Di is facing is in the issue under the bucket of
>> 'handle_non_critical_trips()' when the governor just tries to
>> work on stale data - old trip temp.
> 
> Well, fair enough, but what about the other governors?  Is this
> problem limited to power_allocator?

IIUC the core fwk code - non of the governors would be needed
to handle the critical/hot trips. For the rest of the trip types
I would say it's up to the governor. In our IPA case this stale
data is used for power budget estimation - quite fundamental
step. Therefore, the reset and start from scratch would make more
sense.

I think other governors don't try to 'estimate' such
abstract power headroom based on temperature - so probably
they don't have to even implement the 'reset()' callback
(I don't know their logic that well).

> 
>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
>> and should be ready for what you said (modification of that value).
> 
> Generally speaking, it needs to be prepared for a simultaneous change
> of multiple trip points (including active), in which case it may not
> be useful to invoke the ->reset() callback for each of them
> individually.

Although, that looks more cleaner IMO. Resetting one by one in
a temperature order would be easily maintainable, won't be?

> 
> Moreover, Daniel wants trip points to be sorted by temperature
> eventually and in that case indices may change overall even on one
> trip point update.

It's more complicated I think that this $subject, but very useful.


> 
>> Furthermore, the critical/hot situation is handled w/o governor
>> assistance, so we should be safe:
>> tz->ops->critical(tz) or tz->ops->hot(tz) and not
>> tz->governor->throttle(tz, trip)
> 
> That's fine.

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Di Shen]

On Tue, Jun 20, 2023 at 6:39 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Di,
>
> I have missed your v4 because it landed below your v3 thread.
>
> On 6/19/23 07:35, Di Shen wrote:
> > When the thermal trip point is changed, the governor should
> > be reset so that the policy algorithm be updated to adapt to the
> > new trip point.
> >
> > This patch adds an ops for thermal the governor structure to reset
>
> s/ops/callback
>
> > the governor. The ops is called when the trip point is changed.
> > For power allocator, the parameters of pid controller and the states
> > of power cooling devices can be reset when the passive trip point
> > is changed.
> >
> > Signed-off-by: Di Shen <di.shen@unisoc.com>
> >
> > ---
> > V4:
> > - Compared to V3, handle it in thermal core instead of in governor.
> >
> > - Add an ops to the governor structure, and call it when a trip
> >    point is changed.
> >
> > - Define reset ops for power allocator.
> >
> > V3:
> > - Add fix tag.
> >
> > V2:
> > - Compared to v1, do not revert.
> >
> > - Add a variable(last_switch_on_temp) in power_allocator_params
> >    to record the last switch_on_temp value.
> >
> > - Adds a function to renew the update flag and update the
> >    last_switch_on_temp when thermal trips are writable.
> >
> > V1:
> > - Revert commit 0952177f2a1f.
> > ---
> > ---
> >   drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> >   drivers/thermal/thermal_trip.c        |  6 ++++++
> >   include/linux/thermal.h               |  1 +
> >   3 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 8642f1096b91..41d155adc616 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> >       return allocate_power(tz, trip.temperature);
> >   }
> >
> > +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> > +{
> > +     int ret = 0;
> > +     struct thermal_trip trip;
> > +     struct power_allocator_params *params = tz->governor_data;
> > +
> > +     ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Only need reset for passive trips */
> > +     if (trip.type != THERMAL_TRIP_PASSIVE)
> > +             return -EINVAL;
> > +
> > +     reset_pid_controller(params);
> > +     allow_maximum_power(tz, true);
> > +
> > +     return ret;
> > +}
> > +
> >   static struct thermal_governor thermal_gov_power_allocator = {
> >       .name           = "power_allocator",
> >       .bind_to_tz     = power_allocator_bind,
> >       .unbind_from_tz = power_allocator_unbind,
> >       .throttle       = power_allocator_throttle,
> > +     .reset          = power_allocator_reset,
> >   };
> >   THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > index 907f3a4d7bc8..52eb768fada8 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> >       if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> >               tz->trips[trip_id] = *trip;
> >
> > +     if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> > +             ret = tz->governor->reset(tz, trip_id);
> > +             if (ret)
> > +                     pr_warn_once("Failed to reset thermal governor\n");
> > +     }
>
> I agree with Rafael. Maybe change that to debug print, so that can be
> checked during the product testing. We cannot do much if that happens.
>
Right.

> > +
> >       thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> >                                     trip->temperature, trip->hysteresis);
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 87837094d549..155ce2291fa5 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -204,6 +204,7 @@ struct thermal_governor {
> >       int (*bind_to_tz)(struct thermal_zone_device *tz);
> >       void (*unbind_from_tz)(struct thermal_zone_device *tz);
> >       int (*throttle)(struct thermal_zone_device *tz, int trip);
> > +     int (*reset)(struct thermal_zone_device *tz, int trip);
> >       struct list_head        governor_list;
> >   };
> >
>
> That thermal_governor::reset() callback is what I had im mind while
> giving you the feedback for the v1. Now it's much cleaner what is going
> on and why.
>

Yes, it is necessary to do something for the governor if the trip point
is changed, especially for the governors that their trips are strongly
related to
the policy.

> Apart from some small bits, LGTM. Please adjust the comment in the patch
> header and this debug print and you can add:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
>
> Please send the next version as separate new thread.
>
> Regards,
> Lukasz

Thank you Lukasz !
I couldn't agree with you more about your comments. What you have said is what
I want to express.

I'd love to send the next version. Thanks again.

Best regards,
Di

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Rafael J. Wysocki]

On Tue, Jun 20, 2023 at 1:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 6/20/23 11:39, Rafael J. Wysocki wrote:
> > On Tue, Jun 20, 2023 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >>
> >> On 6/20/23 11:07, Rafael J. Wysocki wrote:
> >>> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>>>
> >>>> On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@unisoc.com> wrote:
> >>>>>
> >>>>> When the thermal trip point is changed, the governor should
> >>>>> be reset so that the policy algorithm be updated to adapt to the
> >>>>> new trip point.
> >>>>>
> >>>>> This patch adds an ops for thermal the governor structure to reset
> >>>>> the governor. The ops is called when the trip point is changed.
> >>>>> For power allocator, the parameters of pid controller and the states
> >>>>> of power cooling devices can be reset when the passive trip point
> >>>>> is changed.
> >>>>>
> >>>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
> >>>>>
> >>>>> ---
> >>>>> V4:
> >>>>> - Compared to V3, handle it in thermal core instead of in governor.
> >>>>>
> >>>>> - Add an ops to the governor structure, and call it when a trip
> >>>>>     point is changed.
> >>>>>
> >>>>> - Define reset ops for power allocator.
> >>>>>
> >>>>> V3:
> >>>>> - Add fix tag.
> >>>>>
> >>>>> V2:
> >>>>> - Compared to v1, do not revert.
> >>>>>
> >>>>> - Add a variable(last_switch_on_temp) in power_allocator_params
> >>>>>     to record the last switch_on_temp value.
> >>>>>
> >>>>> - Adds a function to renew the update flag and update the
> >>>>>     last_switch_on_temp when thermal trips are writable.
> >>>>>
> >>>>> V1:
> >>>>> - Revert commit 0952177f2a1f.
> >>>>> ---
> >>>>> ---
> >>>>>    drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> >>>>>    drivers/thermal/thermal_trip.c        |  6 ++++++
> >>>>>    include/linux/thermal.h               |  1 +
> >>>>>    3 files changed, 28 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> >>>>> index 8642f1096b91..41d155adc616 100644
> >>>>> --- a/drivers/thermal/gov_power_allocator.c
> >>>>> +++ b/drivers/thermal/gov_power_allocator.c
> >>>>> @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> >>>>>           return allocate_power(tz, trip.temperature);
> >>>>>    }
> >>>>>
> >>>>> +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> >>>>> +{
> >>>>> +       int ret = 0;
> >>>>> +       struct thermal_trip trip;
> >>>>> +       struct power_allocator_params *params = tz->governor_data;
> >>>>> +
> >>>>> +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> >>>>> +       if (ret)
> >>>>> +               return ret;
> >>>>> +
> >>>>> +       /* Only need reset for passive trips */
> >>>>> +       if (trip.type != THERMAL_TRIP_PASSIVE)
> >>>>> +               return -EINVAL;
> >>>>> +
> >>>>> +       reset_pid_controller(params);
> >>>>> +       allow_maximum_power(tz, true);
> >>>>> +
> >>>>> +       return ret;
> >>>>> +}
> >>>>> +
> >>>>>    static struct thermal_governor thermal_gov_power_allocator = {
> >>>>>           .name           = "power_allocator",
> >>>>>           .bind_to_tz     = power_allocator_bind,
> >>>>>           .unbind_from_tz = power_allocator_unbind,
> >>>>>           .throttle       = power_allocator_throttle,
> >>>>> +       .reset          = power_allocator_reset,
> >>>>>    };
> >>>>>    THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> >>>>> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> >>>>> index 907f3a4d7bc8..52eb768fada8 100644
> >>>>> --- a/drivers/thermal/thermal_trip.c
> >>>>> +++ b/drivers/thermal/thermal_trip.c
> >>>>> @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> >>>>>           if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> >>>>>                   tz->trips[trip_id] = *trip;
> >>>>>
> >>>>> +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> >>>>> +               ret = tz->governor->reset(tz, trip_id);
> >>>>> +               if (ret)
> >>>>> +                       pr_warn_once("Failed to reset thermal governor\n");
> >>>>
> >>>> I'm not really sure if it is useful to print this message here.
> >>>>
> >>>> First off, the governors may print more precise diagnostic messages if
> >>>> they care.
> >>>>
> >>>> Second, what is the sysadmin supposed to do in response to this message?
> >>>
> >>> In addition to the above, trip point temperatures may be updated in
> >>> other places too, for instance in response to notifications from
> >>> platform firmware and IMV this new callback should be also used in
> >>> those cases.  However, in those cases multiple trip points may change
> >>> at a time and the critical/hot trip point temperatures may be updated
> >>> too AFAICS.
> >>
> >> IIRC the critical/hot trip points are handled differently, not using the
> >> governors. The governors' 'throttle' callback would be called only
> >> after we pass the test of 'critical/hot' [1].
> >
> > OK, but is it actually useful to return an error code from the
> > ->reset() callback when passed a non-passive trip point?
>
> It will depend on the governor code. In our case the setup code
> w.r.t. trip types is quite confusing (to fit into many possible
> configurations). The non-passive trip point would be only
> possible to bind when there are not other passive trip points.
> That's is a really corner case and probably never used on any
> device. Therefore, IMO we can just bail out in such situation
> when then someone tries to update such single non-passive
> trip point (probably not aware what is doing with IPA?).

Because this is up to the governor, the core has no clue what to do
with the return value from ->reset() and so there should be none.

As I said, governors can print whatever diagnostic messages they like
in that callback, but returning anything from it to the core is just
not useful IMV.

> For the rest of the governors - it's up to them what they
> report in case non-passive trip is updated...

Sure.

> >
> >> What Di is facing is in the issue under the bucket of
> >> 'handle_non_critical_trips()' when the governor just tries to
> >> work on stale data - old trip temp.
> >
> > Well, fair enough, but what about the other governors?  Is this
> > problem limited to power_allocator?
>
> IIUC the core fwk code - non of the governors would be needed
> to handle the critical/hot trips. For the rest of the trip types
> I would say it's up to the governor. In our IPA case this stale
> data is used for power budget estimation - quite fundamental
> step. Therefore, the reset and start from scratch would make more
> sense.
>
> I think other governors don't try to 'estimate' such
> abstract power headroom based on temperature - so probably
> they don't have to even implement the 'reset()' callback
> (I don't know their logic that well).

So there seems to be a claim that IPA is the only governor needing the
->reset() callback, but I have not seen any solid analysis confirming
that.  It very well may be the case, but then the changelog should
clearly explain why this is the case IMO.

> >
> >> For the 2nd case IIUC the code, we pass the 'trip.temperature'
> >> and should be ready for what you said (modification of that value).
> >
> > Generally speaking, it needs to be prepared for a simultaneous change
> > of multiple trip points (including active), in which case it may not
> > be useful to invoke the ->reset() callback for each of them
> > individually.
>
> Although, that looks more cleaner IMO. Resetting one by one in
> a temperature order would be easily maintainable, won't be?

I wouldn't call it maintainable really.

First of all, the trips may not be ordered.  There are no guarantees
whatsoever that they will be ordered, so the caller may have to
determine the temperature order in the first place.  This would be an
extra requirement that currently is not there.

Apart from this, I don't see any fundamental difference between the
case when trip points are updated via sysfs and when they are updated
by the driver.  The governor should reset itself in any of those cases
and even if one trip point changes, the temperature order of all of
them may change, so the governor reset mechanism should be able to
handle the case when multiple trip points are updated at the same
time.  While you may argue that this is theoretical, the ACPI spec
clearly states that this is allowed to happen, for example.

If you want a generic reset callback for governors, that's fine, but
make it generic and not specific to a particular use case.

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

On 6/22/23 19:27, Rafael J. Wysocki wrote:
> On Tue, Jun 20, 2023 at 1:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 6/20/23 11:39, Rafael J. Wysocki wrote:
>>> On Tue, Jun 20, 2023 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>>
>>>> On 6/20/23 11:07, Rafael J. Wysocki wrote:
>>>>> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:

[snip]

> 
> Because this is up to the governor, the core has no clue what to do
> with the return value from ->reset() and so there should be none.
> 
> As I said, governors can print whatever diagnostic messages they like
> in that callback, but returning anything from it to the core is just
> not useful IMV.
> 
>> For the rest of the governors - it's up to them what they
>> report in case non-passive trip is updated...
> 
> Sure.
> 
>>>
>>>> What Di is facing is in the issue under the bucket of
>>>> 'handle_non_critical_trips()' when the governor just tries to
>>>> work on stale data - old trip temp.
>>>
>>> Well, fair enough, but what about the other governors?  Is this
>>> problem limited to power_allocator?
>>
>> IIUC the core fwk code - non of the governors would be needed
>> to handle the critical/hot trips. For the rest of the trip types
>> I would say it's up to the governor. In our IPA case this stale
>> data is used for power budget estimation - quite fundamental
>> step. Therefore, the reset and start from scratch would make more
>> sense.
>>
>> I think other governors don't try to 'estimate' such
>> abstract power headroom based on temperature - so probably
>> they don't have to even implement the 'reset()' callback
>> (I don't know their logic that well).
> 
> So there seems to be a claim that IPA is the only governor needing the
> ->reset() callback, but I have not seen any solid analysis confirming
> that.  It very well may be the case, but then the changelog should
> clearly explain why this is the case IMO.

I agree, the patch header doesn't explain that properly. Here is the
explanation for this Intelligent Power Allocator (IPA):

The IPA controls temperature using PID mechanism. It's a closed
feedback loop. That algorithm can 'learn' from the 'observed'
in the past reaction for it's control decisions and accumulates that
information in the part called 'error integral'. Those accumulated
'error' gaps are the differences between the set target value and the
actually achieved value. In our case the target value is the target
temperature which is coming from the trip point. That part is then used
with the 'I' (of PID) component, so we can compensate for those
'learned' mistakes.
Now, when you change the target temperature value - all your previous
learned errors won't help you. That's why Intelligent Power Allocator
should reset previously accumulated history.

> 
>>>
>>>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
>>>> and should be ready for what you said (modification of that value).
>>>
>>> Generally speaking, it needs to be prepared for a simultaneous change
>>> of multiple trip points (including active), in which case it may not
>>> be useful to invoke the ->reset() callback for each of them
>>> individually.
>>
>> Although, that looks more cleaner IMO. Resetting one by one in
>> a temperature order would be easily maintainable, won't be?
> 
> I wouldn't call it maintainable really.
> 
> First of all, the trips may not be ordered.  There are no guarantees
> whatsoever that they will be ordered, so the caller may have to
> determine the temperature order in the first place.  This would be an
> extra requirement that currently is not there.
> 
> Apart from this, I don't see any fundamental difference between the
> case when trip points are updated via sysfs and when they are updated
> by the driver.  The governor should reset itself in any of those cases
> and even if one trip point changes, the temperature order of all of
> them may change, so the governor reset mechanism should be able to
> handle the case when multiple trip points are updated at the same
> time.  While you may argue that this is theoretical, the ACPI spec
> clearly states that this is allowed to happen, for example.
> 
> If you want a generic reset callback for governors, that's fine, but
> make it generic and not specific to a particular use case.

I think we agree here, but probably having slightly different
implementation in mind. Based on you explanation I think you
want simply this API:
void (*reset)(struct thermal_zone_device *tz);

1. no return value
2. no specific trip ID as argument

Do you agree?
IMO such implementation and API would also work for this IPA
purpose. Would that work for the ACPI use case as well?

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Rafael J. Wysocki]

On Fri, Jun 23, 2023 at 9:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 6/22/23 19:27, Rafael J. Wysocki wrote:
> > On Tue, Jun 20, 2023 at 1:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >>
> >>
> >> On 6/20/23 11:39, Rafael J. Wysocki wrote:
> >>> On Tue, Jun 20, 2023 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>
> >>>> Hi Rafael,
> >>>>
> >>>>
> >>>> On 6/20/23 11:07, Rafael J. Wysocki wrote:
> >>>>> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> [snip]
>
> >
> > Because this is up to the governor, the core has no clue what to do
> > with the return value from ->reset() and so there should be none.
> >
> > As I said, governors can print whatever diagnostic messages they like
> > in that callback, but returning anything from it to the core is just
> > not useful IMV.
> >
> >> For the rest of the governors - it's up to them what they
> >> report in case non-passive trip is updated...
> >
> > Sure.
> >
> >>>
> >>>> What Di is facing is in the issue under the bucket of
> >>>> 'handle_non_critical_trips()' when the governor just tries to
> >>>> work on stale data - old trip temp.
> >>>
> >>> Well, fair enough, but what about the other governors?  Is this
> >>> problem limited to power_allocator?
> >>
> >> IIUC the core fwk code - non of the governors would be needed
> >> to handle the critical/hot trips. For the rest of the trip types
> >> I would say it's up to the governor. In our IPA case this stale
> >> data is used for power budget estimation - quite fundamental
> >> step. Therefore, the reset and start from scratch would make more
> >> sense.
> >>
> >> I think other governors don't try to 'estimate' such
> >> abstract power headroom based on temperature - so probably
> >> they don't have to even implement the 'reset()' callback
> >> (I don't know their logic that well).
> >
> > So there seems to be a claim that IPA is the only governor needing the
> > ->reset() callback, but I have not seen any solid analysis confirming
> > that.  It very well may be the case, but then the changelog should
> > clearly explain why this is the case IMO.
>
> I agree, the patch header doesn't explain that properly. Here is the
> explanation for this Intelligent Power Allocator (IPA):
>
> The IPA controls temperature using PID mechanism. It's a closed
> feedback loop. That algorithm can 'learn' from the 'observed'
> in the past reaction for it's control decisions and accumulates that
> information in the part called 'error integral'. Those accumulated
> 'error' gaps are the differences between the set target value and the
> actually achieved value. In our case the target value is the target
> temperature which is coming from the trip point. That part is then used
> with the 'I' (of PID) component, so we can compensate for those
> 'learned' mistakes.
> Now, when you change the target temperature value - all your previous
> learned errors won't help you. That's why Intelligent Power Allocator
> should reset previously accumulated history.

Right.

And every other governor using information from the past for control
will have an analogous problem, won't it?

> >
> >>>
> >>>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
> >>>> and should be ready for what you said (modification of that value).
> >>>
> >>> Generally speaking, it needs to be prepared for a simultaneous change
> >>> of multiple trip points (including active), in which case it may not
> >>> be useful to invoke the ->reset() callback for each of them
> >>> individually.
> >>
> >> Although, that looks more cleaner IMO. Resetting one by one in
> >> a temperature order would be easily maintainable, won't be?
> >
> > I wouldn't call it maintainable really.
> >
> > First of all, the trips may not be ordered.  There are no guarantees
> > whatsoever that they will be ordered, so the caller may have to
> > determine the temperature order in the first place.  This would be an
> > extra requirement that currently is not there.
> >
> > Apart from this, I don't see any fundamental difference between the
> > case when trip points are updated via sysfs and when they are updated
> > by the driver.  The governor should reset itself in any of those cases
> > and even if one trip point changes, the temperature order of all of
> > them may change, so the governor reset mechanism should be able to
> > handle the case when multiple trip points are updated at the same
> > time.  While you may argue that this is theoretical, the ACPI spec
> > clearly states that this is allowed to happen, for example.
> >
> > If you want a generic reset callback for governors, that's fine, but
> > make it generic and not specific to a particular use case.
>
> I think we agree here, but probably having slightly different
> implementation in mind. Based on you explanation I think you
> want simply this API:
> void (*reset)(struct thermal_zone_device *tz);
>
> 1. no return value
> 2. no specific trip ID as argument
>
> Do you agree?

Yes, I do.

> IMO such implementation and API would also work for this IPA
> purpose. Would that work for the ACPI use case as well?

It would AFAICS.

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

On 6/23/23 17:55, Rafael J. Wysocki wrote:
> On Fri, Jun 23, 2023 at 9:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>

[snip]

>>
>> I agree, the patch header doesn't explain that properly. Here is the
>> explanation for this Intelligent Power Allocator (IPA):
>>
>> The IPA controls temperature using PID mechanism. It's a closed
>> feedback loop. That algorithm can 'learn' from the 'observed'
>> in the past reaction for it's control decisions and accumulates that
>> information in the part called 'error integral'. Those accumulated
>> 'error' gaps are the differences between the set target value and the
>> actually achieved value. In our case the target value is the target
>> temperature which is coming from the trip point. That part is then used
>> with the 'I' (of PID) component, so we can compensate for those
>> 'learned' mistakes.
>> Now, when you change the target temperature value - all your previous
>> learned errors won't help you. That's why Intelligent Power Allocator
>> should reset previously accumulated history.
> 
> Right.
> 
> And every other governor using information from the past for control
> will have an analogous problem, won't it?

Not necessarily, but to play safe I would go case-by-case and make
sure other governors are aligned to this new feature.

E.g. the bang-bang governor operates only on current temperature and
current trip value + trip hysteresis. The flow graph describes it [1].
The control (state of the fan: ON or OFF) of that governor could be
simply adjusted to the new reality -> new trip point temp. That would
just mean 'toggling' the fan if needed. There are only 2 'target'
states: 0 or 1 for the fan. You can images a situation when the
temperature doesn't change, but we manipulate the trip value for that
governor. The governor would react correctly always in such situation
w/o a need of a reset IMO.

> 
>>>
>>>>>
>>>>>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
>>>>>> and should be ready for what you said (modification of that value).
>>>>>
>>>>> Generally speaking, it needs to be prepared for a simultaneous change
>>>>> of multiple trip points (including active), in which case it may not
>>>>> be useful to invoke the ->reset() callback for each of them
>>>>> individually.
>>>>
>>>> Although, that looks more cleaner IMO. Resetting one by one in
>>>> a temperature order would be easily maintainable, won't be?
>>>
>>> I wouldn't call it maintainable really.
>>>
>>> First of all, the trips may not be ordered.  There are no guarantees
>>> whatsoever that they will be ordered, so the caller may have to
>>> determine the temperature order in the first place.  This would be an
>>> extra requirement that currently is not there.
>>>
>>> Apart from this, I don't see any fundamental difference between the
>>> case when trip points are updated via sysfs and when they are updated
>>> by the driver.  The governor should reset itself in any of those cases
>>> and even if one trip point changes, the temperature order of all of
>>> them may change, so the governor reset mechanism should be able to
>>> handle the case when multiple trip points are updated at the same
>>> time.  While you may argue that this is theoretical, the ACPI spec
>>> clearly states that this is allowed to happen, for example.
>>>
>>> If you want a generic reset callback for governors, that's fine, but
>>> make it generic and not specific to a particular use case.
>>
>> I think we agree here, but probably having slightly different
>> implementation in mind. Based on you explanation I think you
>> want simply this API:
>> void (*reset)(struct thermal_zone_device *tz);
>>
>> 1. no return value
>> 2. no specific trip ID as argument
>>
>> Do you agree?
> 
> Yes, I do.

OK, thanks.

Di could you implement that 'reset()' API according to this description,
please?

> 
>> IMO such implementation and API would also work for this IPA
>> purpose. Would that work for the ACPI use case as well?
> 
> It would AFAICS.

Thanks Rafael for the comments and the progress that we made :)

Regards,
Lukasz

[1] 
https://elixir.bootlin.com/linux/v6.3/source/drivers/thermal/gov_bang_bang.c#L80

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Di Shen]

On Fri, Jun 23, 2023 at 4:10 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 6/22/23 19:27, Rafael J. Wysocki wrote:
> > On Tue, Jun 20, 2023 at 1:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >>
> >>
> >> On 6/20/23 11:39, Rafael J. Wysocki wrote:
> >>> On Tue, Jun 20, 2023 at 12:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>
> >>>> Hi Rafael,
> >>>>
> >>>>
> >>>> On 6/20/23 11:07, Rafael J. Wysocki wrote:
> >>>>> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> [snip]
>
> >
> > Because this is up to the governor, the core has no clue what to do
> > with the return value from ->reset() and so there should be none.
> >
> > As I said, governors can print whatever diagnostic messages they like
> > in that callback, but returning anything from it to the core is just
> > not useful IMV.
> >
> >> For the rest of the governors - it's up to them what they
> >> report in case non-passive trip is updated...
> >
> > Sure.
> >
> >>>
> >>>> What Di is facing is in the issue under the bucket of
> >>>> 'handle_non_critical_trips()' when the governor just tries to
> >>>> work on stale data - old trip temp.
> >>>
> >>> Well, fair enough, but what about the other governors?  Is this
> >>> problem limited to power_allocator?
> >>
> >> IIUC the core fwk code - non of the governors would be needed
> >> to handle the critical/hot trips. For the rest of the trip types
> >> I would say it's up to the governor. In our IPA case this stale
> >> data is used for power budget estimation - quite fundamental
> >> step. Therefore, the reset and start from scratch would make more
> >> sense.
> >>
> >> I think other governors don't try to 'estimate' such
> >> abstract power headroom based on temperature - so probably
> >> they don't have to even implement the 'reset()' callback
> >> (I don't know their logic that well).
> >
> > So there seems to be a claim that IPA is the only governor needing the
> > ->reset() callback, but I have not seen any solid analysis confirming
> > that.  It very well may be the case, but then the changelog should
> > clearly explain why this is the case IMO.
>
> I agree, the patch header doesn't explain that properly. Here is the
> explanation for this Intelligent Power Allocator (IPA):
>
> The IPA controls temperature using PID mechanism. It's a closed
> feedback loop. That algorithm can 'learn' from the 'observed'
> in the past reaction for it's control decisions and accumulates that
> information in the part called 'error integral'. Those accumulated
> 'error' gaps are the differences between the set target value and the
> actually achieved value. In our case the target value is the target
> temperature which is coming from the trip point. That part is then used
> with the 'I' (of PID) component, so we can compensate for those
> 'learned' mistakes.
> Now, when you change the target temperature value - all your previous
> learned errors won't help you. That's why Intelligent Power Allocator
> should reset previously accumulated history.
>

Yes, THAT's the point!
Maybe I need to write the commit message in more detail.

> >
> >>>
> >>>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
> >>>> and should be ready for what you said (modification of that value).
> >>>
> >>> Generally speaking, it needs to be prepared for a simultaneous change
> >>> of multiple trip points (including active), in which case it may not
> >>> be useful to invoke the ->reset() callback for each of them
> >>> individually.
> >>
> >> Although, that looks more cleaner IMO. Resetting one by one in
> >> a temperature order would be easily maintainable, won't be?
> >
> > I wouldn't call it maintainable really.
> >
> > First of all, the trips may not be ordered.  There are no guarantees
> > whatsoever that they will be ordered, so the caller may have to
> > determine the temperature order in the first place.  This would be an
> > extra requirement that currently is not there.
> >
> > Apart from this, I don't see any fundamental difference between the
> > case when trip points are updated via sysfs and when they are updated
> > by the driver.  The governor should reset itself in any of those cases
> > and even if one trip point changes, the temperature order of all of
> > them may change, so the governor reset mechanism should be able to
> > handle the case when multiple trip points are updated at the same
> > time.  While you may argue that this is theoretical, the ACPI spec
> > clearly states that this is allowed to happen, for example.
> >
> > If you want a generic reset callback for governors, that's fine, but
> > make it generic and not specific to a particular use case.
>
> I think we agree here, but probably having slightly different
> implementation in mind. Based on you explanation I think you
> want simply this API:
> void (*reset)(struct thermal_zone_device *tz);
>
> 1. no return value
> 2. no specific trip ID as argument
>
> Do you agree?
> IMO such implementation and API would also work for this IPA
> purpose. Would that work for the ACPI use case as well?

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Di Shen]

On Sat, Jun 24, 2023 at 1:54 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 6/23/23 17:55, Rafael J. Wysocki wrote:
> > On Fri, Jun 23, 2023 at 9:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >>
> >>
>
> [snip]
>
> >>
> >> I agree, the patch header doesn't explain that properly. Here is the
> >> explanation for this Intelligent Power Allocator (IPA):
> >>
> >> The IPA controls temperature using PID mechanism. It's a closed
> >> feedback loop. That algorithm can 'learn' from the 'observed'
> >> in the past reaction for it's control decisions and accumulates that
> >> information in the part called 'error integral'. Those accumulated
> >> 'error' gaps are the differences between the set target value and the
> >> actually achieved value. In our case the target value is the target
> >> temperature which is coming from the trip point. That part is then used
> >> with the 'I' (of PID) component, so we can compensate for those
> >> 'learned' mistakes.
> >> Now, when you change the target temperature value - all your previous
> >> learned errors won't help you. That's why Intelligent Power Allocator
> >> should reset previously accumulated history.
> >
> > Right.
> >
> > And every other governor using information from the past for control
> > will have an analogous problem, won't it?
>
> Not necessarily, but to play safe I would go case-by-case and make
> sure other governors are aligned to this new feature.
>
> E.g. the bang-bang governor operates only on current temperature and
> current trip value + trip hysteresis. The flow graph describes it [1].
> The control (state of the fan: ON or OFF) of that governor could be
> simply adjusted to the new reality -> new trip point temp. That would
> just mean 'toggling' the fan if needed. There are only 2 'target'
> states: 0 or 1 for the fan. You can images a situation when the
> temperature doesn't change, but we manipulate the trip value for that
> governor. The governor would react correctly always in such situation
> w/o a need of a reset IMO.
>
> >
> >>>
> >>>>>
> >>>>>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
> >>>>>> and should be ready for what you said (modification of that value).
> >>>>>
> >>>>> Generally speaking, it needs to be prepared for a simultaneous change
> >>>>> of multiple trip points (including active), in which case it may not
> >>>>> be useful to invoke the ->reset() callback for each of them
> >>>>> individually.
> >>>>
> >>>> Although, that looks more cleaner IMO. Resetting one by one in
> >>>> a temperature order would be easily maintainable, won't be?
> >>>
> >>> I wouldn't call it maintainable really.
> >>>
> >>> First of all, the trips may not be ordered.  There are no guarantees
> >>> whatsoever that they will be ordered, so the caller may have to
> >>> determine the temperature order in the first place.  This would be an
> >>> extra requirement that currently is not there.
> >>>
> >>> Apart from this, I don't see any fundamental difference between the
> >>> case when trip points are updated via sysfs and when they are updated
> >>> by the driver.  The governor should reset itself in any of those cases
> >>> and even if one trip point changes, the temperature order of all of
> >>> them may change, so the governor reset mechanism should be able to
> >>> handle the case when multiple trip points are updated at the same
> >>> time.  While you may argue that this is theoretical, the ACPI spec
> >>> clearly states that this is allowed to happen, for example.
> >>>
> >>> If you want a generic reset callback for governors, that's fine, but
> >>> make it generic and not specific to a particular use case.
> >>
> >> I think we agree here, but probably having slightly different
> >> implementation in mind. Based on you explanation I think you
> >> want simply this API:
> >> void (*reset)(struct thermal_zone_device *tz);
> >>
> >> 1. no return value
> >> 2. no specific trip ID as argument
> >>
> >> Do you agree?
> >
> > Yes, I do.
>
> OK, thanks.
>
> Di could you implement that 'reset()' API according to this description,
> please?
>
Yes, happy to do that.

> >
> >> IMO such implementation and API would also work for this IPA
> >> purpose. Would that work for the ACPI use case as well?
> >
> > It would AFAICS.
>
> Thanks Rafael for the comments and the progress that we made :)
>
> Regards,
> Lukasz
>
> [1]
> https://elixir.bootlin.com/linux/v6.3/source/drivers/thermal/gov_bang_bang.c#L80

Thanks Lukas and Rafeal for the comments. I will send the next version later.

Best regards,
Di

Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

On 6/25/23 09:39, Di Shen wrote:
> On Fri, Jun 23, 2023 at 4:10 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 6/22/23 19:27, Rafael J. Wysocki wrote:

[snip]

>>>
>>> So there seems to be a claim that IPA is the only governor needing the
>>> ->reset() callback, but I have not seen any solid analysis confirming
>>> that.  It very well may be the case, but then the changelog should
>>> clearly explain why this is the case IMO.
>>
>> I agree, the patch header doesn't explain that properly. Here is the
>> explanation for this Intelligent Power Allocator (IPA):
>>
>> The IPA controls temperature using PID mechanism. It's a closed
>> feedback loop. That algorithm can 'learn' from the 'observed'
>> in the past reaction for it's control decisions and accumulates that
>> information in the part called 'error integral'. Those accumulated
>> 'error' gaps are the differences between the set target value and the
>> actually achieved value. In our case the target value is the target
>> temperature which is coming from the trip point. That part is then used
>> with the 'I' (of PID) component, so we can compensate for those
>> 'learned' mistakes.
>> Now, when you change the target temperature value - all your previous
>> learned errors won't help you. That's why Intelligent Power Allocator
>> should reset previously accumulated history.
>>
> 
> Yes, THAT's the point!
> Maybe I need to write the commit message in more detail.
> 

Yes, please extend that description.

Regards,
Lukasz