Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6d3f24a4-ae70-49eb-6e41-86baa1db6bed@linaro.org>
Date:   Mon, 10 Jul 2023 10:55:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V5] thermal/core/power_allocator: reset thermal governor
 when trip point is changed
Content-Language: en-US
To:     Di Shen <di.shen@unisoc.com>, lukasz.luba@arm.com,
        rafael@kernel.org
Cc:     amitk@kernel.org, rui.zhang@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, xuewen.yan@unisoc.com,
        jeson.gao@unisoc.com, orsonzhai@gmail.com, zhanglyra@gmail.com
References: <20230710033234.28641-1-di.shen@unisoc.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20230710033234.28641-1-di.shen@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 10/07/2023 05:32, Di Shen wrote:
> When the thermal trip point is changed, the governor should
> be reset so that the policy algorithm can be updated to adapt
> to the new trip point.
> 
> 1.Thermal governor is working for the passive trip point or active
> trip point. Therefore, when the trip point is changed it should
> reset the governor only for passic/active trip points.
> 
> 2.For "power_allocator" governor reset, the parameters of pid
> controller and the states of power cooling devices should be reset.
> 
> 2.1
> The IPA controls temperature using PID mechanism. It is a closed-
> loop feedback monitoring system. It uses the gap between target
> temperature and current temperature which says "error" as the
> input of the PID controller:
> 
> err = desired_temperature - current_temperature
> P_max =
> k_p * err + k_i * err_integral + k_d * diff_err + sustainable_power
> 
> That algorithm can 'learn' from the 'observed' in the past reaction
> for it's control decisions and accumulates that information in the
> I(Integral) part so that it can conpensate for those 'learned'
> mistakes.
> 
> Based on the above, the most important is the desired temperature
> comes from the trip point. When the trip point is changed, all the
> previous learned errors won't help for the IPA. So the pid parameters
> should be reset for IPA governor reset.
> 
> 2.2
> Other wise, the cooling devices should also be reset when the trip
> point is changed.
> 
> This patch adds an ops for the thermal_governor structure to reset
> the governor and give the 'reset' function definition for power
> allocator. The ops is called when the trip points are changed via
> sysfs interface.
> 
> Signed-off-by: Di Shen <di.shen@unisoc.com>
> ---
>   drivers/thermal/gov_power_allocator.c | 9 +++++++++
>   drivers/thermal/thermal_trip.c        | 5 +++++
>   include/linux/thermal.h               | 3 +++
>   3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 8642f1096b91..8d17a10671e4 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -729,10 +729,19 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
>   	return allocate_power(tz, trip.temperature);
>   }
>   
> +static void power_allocator_reset(struct thermal_zone_device *tz)
> +{
> +	struct power_allocator_params *params = tz->governor_data;
> +
> +	reset_pid_controller(params);
> +	allow_maximum_power(tz, true);

Do you really want to allow the maximum power? What about if the trip 
temperature is decreased ?

You want maximum power only if the mitigation ends.

> +}
> +
>   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..13bbe029c6ab 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -173,6 +173,11 @@ 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;
>   
> +	/* Reset the governor only when the trip type is active or passive. */
> +	ret = (trip->type == THERMAL_TRIP_PASSIVE || trip->type == THERMAL_TRIP_ACTIVE);

Actually we have the trip points:

ACTIVE, PASSIVE, HOT and CRITICAL

The last two ones should not be writable.

Instead of this test, it would be cleaner to only make the ACTIVE and 
PASSIVE trip point writable when the CONFIG_THERMAL_WRITABLE_TRIPS 
option is set. Consequently, other trip points won't be writable and 
this test can go away as set_trip will be protected by a RO sysfs file 
property.

> +	if (ret && t.temperature != trip->temperature && tz->governor && tz->governor->reset)

The temperature test is duplicated because it is already done in the 
block before.

> +		tz->governor->reset(tz);
> +
>   	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..d27d053319bf 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -197,6 +197,8 @@ struct thermal_zone_device {
>    *			thermal zone.
>    * @throttle:	callback called for every trip point even if temperature is
>    *		below the trip point temperature
> + * @reset:	callback called for governor reset
> + *
>    * @governor_list:	node in thermal_governor_list (in thermal_core.c)
>    */
>   struct thermal_governor {
> @@ -204,6 +206,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);
> +	void (*reset)(struct thermal_zone_device *tz);
>   	struct list_head	governor_list;
>   };
>   

-- 
<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