linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Lukasz Luba <lukasz.luba@arm.com>,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Cc: amitk@kernel.org, Dietmar.Eggemann@arm.com, ionela.voinescu@arm.com
Subject: Re: [PATCH v3 2/2] thermal: power allocator: change how estimation code is called
Date: Tue, 13 Oct 2020 18:41:57 +0200	[thread overview]
Message-ID: <371617d1-fb1c-8e7b-0a50-e3ea07a1f825@linaro.org> (raw)
In-Reply-To: <20201009135850.14727-3-lukasz.luba@arm.com>

On 09/10/2020 15:58, Lukasz Luba wrote:
> The sustainable power value might come from the Device Tree or can be
> estimated in run time. There is no need to estimate every time when the
> governor is called and temperature is high. Instead, store the estimated
> value and make it available via standard sysfs interface so it can be
> checked from the user-space. Re-invoke the estimation only in case the
> sustainable power was set to 0. Apart from that the PID coefficients
> are not going to be force updated thus can better handle sysfs settings.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---

[ ... ]

> -static void estimate_pid_constants(struct thermal_zone_device *tz,
> -				   u32 sustainable_power, int trip_switch_on,
> -				   int control_temp, bool force)
> +static void estimate_tzp_constants(struct thermal_zone_device *tz,
> +				   int trip_switch_on, int control_temp)
>  {
> -	int ret;
> -	int switch_on_temp;
>  	u32 temperature_threshold;
> +	int switch_on_temp;
> +	bool force = false;
> +	int ret;
>  	s32 k_i;
>  
> +	if (!tz->tzp->sustainable_power) {
> +		tz->tzp->sustainable_power = estimate_sustainable_power(tz);
> +		force = true;
> +		dev_info(&tz->device, "power_allocator: estimating sust. power and PID constants\n");
> +	}
> +
>  	ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
>  	if (ret)
>  		switch_on_temp = 0;
>  
>  	temperature_threshold = control_temp - switch_on_temp;
>  	/*
> -	 * estimate_pid_constants() tries to find appropriate default
> +	 * estimate_tzp_constants() tries to find appropriate default
>  	 * values for thermal zones that don't provide them. If a
>  	 * system integrator has configured a thermal zone with two
>  	 * passive trip points at the same temperature, that person
> @@ -151,11 +151,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>  		return;
>  
>  	if (!tz->tzp->k_po || force)
> -		tz->tzp->k_po = int_to_frac(sustainable_power) /
> +		tz->tzp->k_po = int_to_frac(tz->tzp->sustainable_power) /
>  			temperature_threshold;
>  
>  	if (!tz->tzp->k_pu || force)
> -		tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
> +		tz->tzp->k_pu = int_to_frac(2 * tz->tzp->sustainable_power) /
>  			temperature_threshold;
>  
>  	if (!tz->tzp->k_i || force) {
> @@ -193,19 +193,13 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>  {
>  	s64 p, i, d, power_range;
>  	s32 err, max_power_frac;
> -	u32 sustainable_power;
>  	struct power_allocator_params *params = tz->governor_data;
>  
>  	max_power_frac = int_to_frac(max_allocatable_power);
>  
> -	if (tz->tzp->sustainable_power) {
> -		sustainable_power = tz->tzp->sustainable_power;
> -	} else {
> -		sustainable_power = estimate_sustainable_power(tz);
> -		estimate_pid_constants(tz, sustainable_power,
> -				       params->trip_switch_on, control_temp,
> -				       true);
> -	}
> +	if (!tz->tzp->sustainable_power)
> +		estimate_tzp_constants(tz, params->trip_switch_on,
> +				       control_temp);

The changes in this patch are appropriate and make sense but they are
not done at the right place.

estimate_tzp_constants() must be called when sustainable_power is
updated via DT/init or sysfs.

Keeping a function to estimate the sustainable power and another one to
estimate the k_* separated would be more clear.

Actually the confusion is coming from when the pid constants are
computed, I suggest moving the initialization of k_* out of this
function and killing the 'force' test.


[ ... ]


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

  parent reply	other threads:[~2020-10-13 16:42 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-09 13:58 [PATCH v3 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 2/2] thermal: power allocator: change how estimation code is called Lukasz Luba
2020-10-09 14:23   ` Ionela Voinescu
2020-10-13 16:41   ` Daniel Lezcano [this message]
2020-10-29  9:24     ` Lukasz Luba

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=371617d1-fb1c-8e7b-0a50-e3ea07a1f825@linaro.org \
    --to=daniel.lezcano@linaro.org \
    --cc=Dietmar.Eggemann@arm.com \
    --cc=amitk@kernel.org \
    --cc=ionela.voinescu@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --subject='Re: [PATCH v3 2/2] thermal: power allocator: change how estimation code is called' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).