From: Lukasz Luba <lukasz.luba@arm.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>,
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: Thu, 29 Oct 2020 09:24:01 +0000 [thread overview]
Message-ID: <38aa72f6-25fd-b7a5-07c0-9db7f0233479@arm.com> (raw)
In-Reply-To: <371617d1-fb1c-8e7b-0a50-e3ea07a1f825@linaro.org>
Hi Daniel,
On 10/13/20 5:41 PM, Daniel Lezcano wrote:
> On 09/10/2020 15:58, Lukasz Luba wrote:
>> The sustainable power value might come from the Device Tree or can be
>> estimated in run time. There is no need to estimate every time when the
>> governor is called and temperature is high. Instead, store the estimated
>> value and make it available via standard sysfs interface so it can be
>> checked from the user-space. Re-invoke the estimation only in case the
>> sustainable power was set to 0. Apart from that the PID coefficients
>> are not going to be force updated thus can better handle sysfs settings.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@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.
>
>
> [ ... ]
>
>
Thank you for the review. I will re-write the patch
and address your suggestion.
Regards,
Lukasz
prev parent reply other threads:[~2020-10-29 9:24 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
2020-10-29 9:24 ` Lukasz Luba [this message]
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=38aa72f6-25fd-b7a5-07c0-9db7f0233479@arm.com \
--to=lukasz.luba@arm.com \
--cc=Dietmar.Eggemann@arm.com \
--cc=amitk@kernel.org \
--cc=daniel.lezcano@linaro.org \
--cc=ionela.voinescu@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).