* [PATCH 0/2] Improve the estimations in Intelligent Power Allocation @ 2020-10-02 12:24 Lukasz Luba 2020-10-02 12:24 ` [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba 2020-10-02 12:24 ` [PATCH 2/2] thermal: power allocator: estimate sustainable power only once Lukasz Luba 0 siblings, 2 replies; 10+ messages in thread From: Lukasz Luba @ 2020-10-02 12:24 UTC (permalink / raw) To: linux-kernel, linux-pm Cc: daniel.lezcano, amitk, Dietmar.Eggemann, lukasz.luba Hi all, The Intelligent Power Allocation (IPA) estimates the needed coefficients for internal algorithm. It can also estimate the sustainable power value when the DT has not provided one. Fix the 'k_i' coefficient which might be to big related to the other values, when the sustainable power is in an abstract scale. Do the estimation only once and avoid expensive calculation every time the IPA is called. Regards, Lukasz Luba Lukasz Luba (2): thermal: power allocator: change the 'k_i' coefficient estimation thermal: power allocator: estimate sustainable power only once drivers/thermal/gov_power_allocator.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation 2020-10-02 12:24 [PATCH 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba @ 2020-10-02 12:24 ` Lukasz Luba 2020-10-13 10:21 ` Daniel Lezcano 2020-10-02 12:24 ` [PATCH 2/2] thermal: power allocator: estimate sustainable power only once Lukasz Luba 1 sibling, 1 reply; 10+ messages in thread From: Lukasz Luba @ 2020-10-02 12:24 UTC (permalink / raw) To: linux-kernel, linux-pm Cc: daniel.lezcano, amitk, Dietmar.Eggemann, lukasz.luba Intelligent Power Allocation (IPA) is built around the PID controller concept. The initialization code tries to setup the environment based on the information available in DT or estimate the value based on minimum power reported by each of the cooling device. The estimation will have an impact on the PID controller behaviour via the related 'k_po', 'k_pu', 'k_i' coefficients and also on the power budget calculation. This change prevents the situation when 'k_i' is relatively big compared to 'k_po' and 'k_pu' values. This might happen when the estimation for 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are small. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- drivers/thermal/gov_power_allocator.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c index 5cb518d8f156..f69fafe486a5 100644 --- a/drivers/thermal/gov_power_allocator.c +++ b/drivers/thermal/gov_power_allocator.c @@ -131,6 +131,7 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, int ret; int switch_on_temp; u32 temperature_threshold; + s32 k_i; ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp); if (ret) @@ -156,8 +157,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / temperature_threshold; - if (!tz->tzp->k_i || force) - tz->tzp->k_i = int_to_frac(10) / 1000; + if (!tz->tzp->k_i || force) { + k_i = tz->tzp->k_pu / 10; + tz->tzp->k_i = k_i > 0 ? k_i : 1; + } + /* * The default for k_d and integral_cutoff is 0, so we can * leave them as they are. -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation 2020-10-02 12:24 ` [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba @ 2020-10-13 10:21 ` Daniel Lezcano 2020-10-13 10:59 ` Lukasz Luba 0 siblings, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2020-10-13 10:21 UTC (permalink / raw) To: Lukasz Luba, linux-kernel, linux-pm; +Cc: amitk, Dietmar.Eggemann Hi Lukasz, On 02/10/2020 14:24, Lukasz Luba wrote: > Intelligent Power Allocation (IPA) is built around the PID controller > concept. The initialization code tries to setup the environment based on > the information available in DT or estimate the value based on minimum > power reported by each of the cooling device. The estimation will have an > impact on the PID controller behaviour via the related 'k_po', 'k_pu', > 'k_i' coefficients and also on the power budget calculation. > > This change prevents the situation when 'k_i' is relatively big compared > to 'k_po' and 'k_pu' values. This might happen when the estimation for > 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are > small. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > drivers/thermal/gov_power_allocator.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c > index 5cb518d8f156..f69fafe486a5 100644 > --- a/drivers/thermal/gov_power_allocator.c > +++ b/drivers/thermal/gov_power_allocator.c > @@ -131,6 +131,7 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, > int ret; > int switch_on_temp; > u32 temperature_threshold; > + s32 k_i; > > ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp); > if (ret) > @@ -156,8 +157,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, > tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / > temperature_threshold; > > - if (!tz->tzp->k_i || force) > - tz->tzp->k_i = int_to_frac(10) / 1000; > + if (!tz->tzp->k_i || force) { > + k_i = tz->tzp->k_pu / 10; > + tz->tzp->k_i = k_i > 0 ? k_i : 1; > + } I do not understand the rational behind this change. Do you have some values to share describing what would be the impact of this change? Depending on the thermal behavior of a board, these coefficients could be very different, no ? -- <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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation 2020-10-13 10:21 ` Daniel Lezcano @ 2020-10-13 10:59 ` Lukasz Luba 2020-10-13 11:22 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Lukasz Luba @ 2020-10-13 10:59 UTC (permalink / raw) To: Daniel Lezcano, linux-kernel, linux-pm; +Cc: amitk, Dietmar.Eggemann Hi Daniel, On 10/13/20 11:21 AM, Daniel Lezcano wrote: > > Hi Lukasz, > > On 02/10/2020 14:24, Lukasz Luba wrote: >> Intelligent Power Allocation (IPA) is built around the PID controller >> concept. The initialization code tries to setup the environment based on >> the information available in DT or estimate the value based on minimum >> power reported by each of the cooling device. The estimation will have an >> impact on the PID controller behaviour via the related 'k_po', 'k_pu', >> 'k_i' coefficients and also on the power budget calculation. >> >> This change prevents the situation when 'k_i' is relatively big compared >> to 'k_po' and 'k_pu' values. This might happen when the estimation for >> 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are >> small. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> drivers/thermal/gov_power_allocator.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c >> index 5cb518d8f156..f69fafe486a5 100644 >> --- a/drivers/thermal/gov_power_allocator.c >> +++ b/drivers/thermal/gov_power_allocator.c >> @@ -131,6 +131,7 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, >> int ret; >> int switch_on_temp; >> u32 temperature_threshold; >> + s32 k_i; >> >> ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp); >> if (ret) >> @@ -156,8 +157,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, >> tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / >> temperature_threshold; >> >> - if (!tz->tzp->k_i || force) >> - tz->tzp->k_i = int_to_frac(10) / 1000; >> + if (!tz->tzp->k_i || force) { >> + k_i = tz->tzp->k_pu / 10; >> + tz->tzp->k_i = k_i > 0 ? k_i : 1; >> + } > > I do not understand the rational behind this change. This is the unfortunate impact of the EM abstract scale of power values. IPA didn't have to deal with it, because we always had milli-Watts. Because the EM allows the bogoWatts and some vendors already have them I have to re-evaluate the IPA. > > Do you have some values to share describing what would be the impact of > this change? Yes, here is an example: EM has 3 devices with abstract scale power values, where minimum power is 25 and max is 200. The minimum power is used by estimate_sustainable_power() as a sum of all devices' min power. Sustainable power is going to be estimated to 75. Then in the code we have 'temperature_threshold' which is in milli-Celcius, thus 15degC is 15000. We estimate 'k_po' according to: int_to_frac(sustainable_power) / temperature_threshold; which is: (75 << 10) / 15000 = ~75000 / 15000 = 5 <-- 'k_po' then k_pu: ((2*75) << 10) / 15000 = ~150000 / 15000 = 10 Then the old 'k_i' is just hard-coded 10, which is the same order of magnitude to what is in 'k_pu'. It should be 1 order of magnitude smaller than 'k_pu'. I did some experiments and the bigger 'k_i' slows down a lot the rising temp. That's why this change. It was OK to have k_i=10 when we were in milliWatts world, when the min power value was bigger, thus 'k_pu' was also bigger than our hard-coded 'k_i'. > > Depending on the thermal behavior of a board, these coefficients could > be very different, no ? > Yes, I strongly believe that vendor engineers will make experiments with these values and not go with default. Then they will store the k_pu, k_po, k_i via sysfs interface, with also sustainable_power. But I have to also fix the hard-coded k_i in the estimation. As described above, when we have small power values from abstract scale, the k_i stays too big. Regards, Lukasz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation 2020-10-13 10:59 ` Lukasz Luba @ 2020-10-13 11:22 ` Daniel Lezcano 2020-10-13 12:04 ` Lukasz Luba 0 siblings, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2020-10-13 11:22 UTC (permalink / raw) To: Lukasz Luba, linux-kernel, linux-pm; +Cc: amitk, Dietmar.Eggemann On 13/10/2020 12:59, Lukasz Luba wrote: > Hi Daniel, > > On 10/13/20 11:21 AM, Daniel Lezcano wrote: >> >> Hi Lukasz, >> >> On 02/10/2020 14:24, Lukasz Luba wrote: >>> Intelligent Power Allocation (IPA) is built around the PID controller >>> concept. The initialization code tries to setup the environment based on >>> the information available in DT or estimate the value based on minimum >>> power reported by each of the cooling device. The estimation will >>> have an >>> impact on the PID controller behaviour via the related 'k_po', 'k_pu', >>> 'k_i' coefficients and also on the power budget calculation. >>> >>> This change prevents the situation when 'k_i' is relatively big compared >>> to 'k_po' and 'k_pu' values. This might happen when the estimation for >>> 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are >>> small. >>> >>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >>> --- >>> drivers/thermal/gov_power_allocator.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/thermal/gov_power_allocator.c >>> b/drivers/thermal/gov_power_allocator.c >>> index 5cb518d8f156..f69fafe486a5 100644 >>> --- a/drivers/thermal/gov_power_allocator.c >>> +++ b/drivers/thermal/gov_power_allocator.c >>> @@ -131,6 +131,7 @@ static void estimate_pid_constants(struct >>> thermal_zone_device *tz, >>> int ret; >>> int switch_on_temp; >>> u32 temperature_threshold; >>> + s32 k_i; >>> ret = tz->ops->get_trip_temp(tz, trip_switch_on, >>> &switch_on_temp); >>> if (ret) >>> @@ -156,8 +157,11 @@ static void estimate_pid_constants(struct >>> thermal_zone_device *tz, >>> tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / >>> temperature_threshold; >>> - if (!tz->tzp->k_i || force) >>> - tz->tzp->k_i = int_to_frac(10) / 1000; >>> + if (!tz->tzp->k_i || force) { >>> + k_i = tz->tzp->k_pu / 10; >>> + tz->tzp->k_i = k_i > 0 ? k_i : 1; >>> + } >> >> I do not understand the rational behind this change. > > This is the unfortunate impact of the EM abstract scale of power values. > IPA didn't have to deal with it, because we always had milli-Watts. > Because the EM allows the bogoWatts and some vendors already have > them I have to re-evaluate the IPA. > >> >> Do you have some values to share describing what would be the impact of >> this change? > > Yes, here is an example: > EM has 3 devices with abstract scale power values, where minimum power > is 25 and max is 200. The minimum power is used by > estimate_sustainable_power() > as a sum of all devices' min power. Sustainable power is going to be > estimated to 75. > > Then in the code we have 'temperature_threshold' which is in > milli-Celcius, thus 15degC is 15000. > > We estimate 'k_po' according to: > int_to_frac(sustainable_power) / temperature_threshold; > > which is: > (75 << 10) / 15000 = ~75000 / 15000 = 5 <-- 'k_po' > > then k_pu: > ((2*75) << 10) / 15000 = ~150000 / 15000 = 10 > > Then the old 'k_i' is just hard-coded 10, which is > the same order of magnitude to what is in 'k_pu'. > It should be 1 order of magnitude smaller than 'k_pu'. > > I did some experiments and the bigger 'k_i' slows down a lot > the rising temp. That's why this change. > > It was OK to have k_i=10 when we were in milliWatts world, > when the min power value was bigger, thus 'k_pu' was also bigger > than our hard-coded 'k_i'. > >> >> Depending on the thermal behavior of a board, these coefficients could >> be very different, no ? >> > > Yes, I strongly believe that vendor engineers will make experiments with > these values and not go with default. Then they will store the k_pu, > k_po, k_i via sysfs interface, with also sustainable_power. IMHO it is the opposite. For what I've seen, the IPA is not used or the k_* are misunderstood, thus not changed. The PID regulation loop technique is not quite used and known by everyone. > But I have to also fix the hard-coded k_i in the estimation. As > described above, when we have small power values from abstract scale, > the k_i stays too big. May be it is preferable to adjust the k_* dynamically given the undershot and overshot results? And then add a set of less opaque parameters for the user, like the time or watts, no? -- <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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation 2020-10-13 11:22 ` Daniel Lezcano @ 2020-10-13 12:04 ` Lukasz Luba 2020-10-13 15:56 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Lukasz Luba @ 2020-10-13 12:04 UTC (permalink / raw) To: Daniel Lezcano, linux-kernel, linux-pm; +Cc: amitk, Dietmar.Eggemann On 10/13/20 12:22 PM, Daniel Lezcano wrote: > On 13/10/2020 12:59, Lukasz Luba wrote: >> Hi Daniel, >> >> On 10/13/20 11:21 AM, Daniel Lezcano wrote: >>> >>> Hi Lukasz, >>> >>> On 02/10/2020 14:24, Lukasz Luba wrote: >>>> Intelligent Power Allocation (IPA) is built around the PID controller >>>> concept. The initialization code tries to setup the environment based on >>>> the information available in DT or estimate the value based on minimum >>>> power reported by each of the cooling device. The estimation will >>>> have an >>>> impact on the PID controller behaviour via the related 'k_po', 'k_pu', >>>> 'k_i' coefficients and also on the power budget calculation. >>>> >>>> This change prevents the situation when 'k_i' is relatively big compared >>>> to 'k_po' and 'k_pu' values. This might happen when the estimation for >>>> 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are >>>> small. >>>> >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >>>> --- >>>> drivers/thermal/gov_power_allocator.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/thermal/gov_power_allocator.c >>>> b/drivers/thermal/gov_power_allocator.c >>>> index 5cb518d8f156..f69fafe486a5 100644 >>>> --- a/drivers/thermal/gov_power_allocator.c >>>> +++ b/drivers/thermal/gov_power_allocator.c >>>> @@ -131,6 +131,7 @@ static void estimate_pid_constants(struct >>>> thermal_zone_device *tz, >>>> int ret; >>>> int switch_on_temp; >>>> u32 temperature_threshold; >>>> + s32 k_i; >>>> ret = tz->ops->get_trip_temp(tz, trip_switch_on, >>>> &switch_on_temp); >>>> if (ret) >>>> @@ -156,8 +157,11 @@ static void estimate_pid_constants(struct >>>> thermal_zone_device *tz, >>>> tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / >>>> temperature_threshold; >>>> - if (!tz->tzp->k_i || force) >>>> - tz->tzp->k_i = int_to_frac(10) / 1000; >>>> + if (!tz->tzp->k_i || force) { >>>> + k_i = tz->tzp->k_pu / 10; >>>> + tz->tzp->k_i = k_i > 0 ? k_i : 1; >>>> + } >>> >>> I do not understand the rational behind this change. >> >> This is the unfortunate impact of the EM abstract scale of power values. >> IPA didn't have to deal with it, because we always had milli-Watts. >> Because the EM allows the bogoWatts and some vendors already have >> them I have to re-evaluate the IPA. >> >>> >>> Do you have some values to share describing what would be the impact of >>> this change? >> >> Yes, here is an example: >> EM has 3 devices with abstract scale power values, where minimum power >> is 25 and max is 200. The minimum power is used by >> estimate_sustainable_power() >> as a sum of all devices' min power. Sustainable power is going to be >> estimated to 75. >> >> Then in the code we have 'temperature_threshold' which is in >> milli-Celcius, thus 15degC is 15000. >> >> We estimate 'k_po' according to: >> int_to_frac(sustainable_power) / temperature_threshold; >> >> which is: >> (75 << 10) / 15000 = ~75000 / 15000 = 5 <-- 'k_po' >> >> then k_pu: >> ((2*75) << 10) / 15000 = ~150000 / 15000 = 10 >> >> Then the old 'k_i' is just hard-coded 10, which is >> the same order of magnitude to what is in 'k_pu'. >> It should be 1 order of magnitude smaller than 'k_pu'. >> >> I did some experiments and the bigger 'k_i' slows down a lot >> the rising temp. That's why this change. >> >> It was OK to have k_i=10 when we were in milliWatts world, >> when the min power value was bigger, thus 'k_pu' was also bigger >> than our hard-coded 'k_i'. >> >>> >>> Depending on the thermal behavior of a board, these coefficients could >>> be very different, no ? >>> >> >> Yes, I strongly believe that vendor engineers will make experiments with >> these values and not go with default. Then they will store the k_pu, >> k_po, k_i via sysfs interface, with also sustainable_power. > > IMHO it is the opposite. For what I've seen, the IPA is not used or the > k_* are misunderstood, thus not changed. The PID regulation loop > technique is not quite used and known by everyone. There is quite a few DT entries of 'sustainable-power' so I assumed it is known, but you might be right. > >> But I have to also fix the hard-coded k_i in the estimation. As >> described above, when we have small power values from abstract scale, >> the k_i stays too big. > > May be it is preferable to adjust the k_* dynamically given the > undershot and overshot results? And then add a set of less opaque > parameters for the user, like the time or watts, no? > Hmmmm, this is interesting, I haven't thought about it. Thank you for this idea. That would require a re-design of current IPA. IPA trying to figure out better k_* values... I will discuss it internally. It would take time, definitely more than the proposed small fix addressing abstract scale and hard-coded 'k_i'. Do you think that this fix can be applied and then I can experiment on what you suggested? There is v3 reviewed by Ionela [1]. Thank you for your comments. Regards, Lukasz [1] https://lore.kernel.org/linux-pm/20201009135850.14727-1-lukasz.luba@arm.com/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation 2020-10-13 12:04 ` Lukasz Luba @ 2020-10-13 15:56 ` Daniel Lezcano 0 siblings, 0 replies; 10+ messages in thread From: Daniel Lezcano @ 2020-10-13 15:56 UTC (permalink / raw) To: Lukasz Luba, linux-kernel, linux-pm; +Cc: amitk, Dietmar.Eggemann On 13/10/2020 14:04, Lukasz Luba wrote: > > > On 10/13/20 12:22 PM, Daniel Lezcano wrote: >> On 13/10/2020 12:59, Lukasz Luba wrote: >>> Hi Daniel, >>> >>> On 10/13/20 11:21 AM, Daniel Lezcano wrote: >>>> >>>> Hi Lukasz, >>>> >>>> On 02/10/2020 14:24, Lukasz Luba wrote: >>>>> Intelligent Power Allocation (IPA) is built around the PID controller >>>>> concept. The initialization code tries to setup the environment >>>>> based on >>>>> the information available in DT or estimate the value based on minimum >>>>> power reported by each of the cooling device. The estimation will >>>>> have an >>>>> impact on the PID controller behaviour via the related 'k_po', 'k_pu', >>>>> 'k_i' coefficients and also on the power budget calculation. >>>>> >>>>> This change prevents the situation when 'k_i' is relatively big >>>>> compared >>>>> to 'k_po' and 'k_pu' values. This might happen when the estimation for >>>>> 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are >>>>> small. >>>>> >>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >>>>> --- [ ... ] >>> Yes, I strongly believe that vendor engineers will make experiments with >>> these values and not go with default. Then they will store the k_pu, >>> k_po, k_i via sysfs interface, with also sustainable_power. >> >> IMHO it is the opposite. For what I've seen, the IPA is not used or the >> k_* are misunderstood, thus not changed. The PID regulation loop >> technique is not quite used and known by everyone. > > There is quite a few DT entries of 'sustainable-power' so I assumed > it is known, but you might be right. Yes, and if you do not count the Linaro contributions, there are even less entries. That may imply the sustainable power is estimated in most of the case if the vendors are specifying the ipa governor. This series may change the default behavior, but I guess this is not a problem without the right k_* in any case. >>> But I have to also fix the hard-coded k_i in the estimation. As >>> described above, when we have small power values from abstract scale, >>> the k_i stays too big. >> >> May be it is preferable to adjust the k_* dynamically given the >> undershot and overshot results? And then add a set of less opaque >> parameters for the user, like the time or watts, no? >> > > Hmmmm, this is interesting, I haven't thought about it. Thank you > for this idea. > That would require a re-design of current IPA. IPA trying to figure > out better k_* values... I will discuss it internally. [ ... ] > It would take time, definitely more than the proposed small fix > addressing abstract scale and hard-coded 'k_i'. > Do you think that this fix can be applied and then I can experiment > on what you suggested? Yes, sure. Let me review the patch 2/2. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] thermal: power allocator: estimate sustainable power only once 2020-10-02 12:24 [PATCH 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba 2020-10-02 12:24 ` [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba @ 2020-10-02 12:24 ` Lukasz Luba 2020-10-08 10:14 ` Ionela Voinescu 1 sibling, 1 reply; 10+ messages in thread From: Lukasz Luba @ 2020-10-02 12:24 UTC (permalink / raw) To: linux-kernel, linux-pm Cc: daniel.lezcano, amitk, Dietmar.Eggemann, lukasz.luba 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. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- drivers/thermal/gov_power_allocator.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c index f69fafe486a5..dd59085f38f5 100644 --- a/drivers/thermal/gov_power_allocator.c +++ b/drivers/thermal/gov_power_allocator.c @@ -204,6 +204,8 @@ static u32 pid_controller(struct thermal_zone_device *tz, estimate_pid_constants(tz, sustainable_power, params->trip_switch_on, control_temp, true); + /* Do the estimation only once and make available in sysfs */ + tz->tzp->sustainable_power = sustainable_power; } err = control_temp - tz->temperature; -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] thermal: power allocator: estimate sustainable power only once 2020-10-02 12:24 ` [PATCH 2/2] thermal: power allocator: estimate sustainable power only once Lukasz Luba @ 2020-10-08 10:14 ` Ionela Voinescu 2020-10-08 12:34 ` Lukasz Luba 0 siblings, 1 reply; 10+ messages in thread From: Ionela Voinescu @ 2020-10-08 10:14 UTC (permalink / raw) To: Lukasz Luba Cc: linux-kernel, linux-pm, daniel.lezcano, amitk, Dietmar.Eggemann Hi Lukasz, On Friday 02 Oct 2020 at 13:24:16 (+0100), 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. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > drivers/thermal/gov_power_allocator.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c > index f69fafe486a5..dd59085f38f5 100644 > --- a/drivers/thermal/gov_power_allocator.c > +++ b/drivers/thermal/gov_power_allocator.c > @@ -204,6 +204,8 @@ static u32 pid_controller(struct thermal_zone_device *tz, > estimate_pid_constants(tz, sustainable_power, > params->trip_switch_on, control_temp, > true); > + /* Do the estimation only once and make available in sysfs */ > + tz->tzp->sustainable_power = sustainable_power; After looking over the code, it does seems mostly useless to do the estimation every time the controller kicks in. But I have two comments in this regard: - The estimation is dependent on the temperature we control for which can be changed from sysfs. While I don't see that as a big worry, (sustainable power is an estimation anyway), it might be worth a more detailed comment on why we don't expect this to be a problem, or what we expect the consequences of computing sustainable power only once could be. - In the function comment for estimate_pid_constants() there is a mention of sustainable power: """ * Sustainable power is provided in case it was estimated. The * estimated sustainable_power should not be stored in the * thermal_zone_parameters so it has to be passed explicitly to this * function. """ If we are going to compute the sustainable power estimation only once, this comment should be removed, the estimated value should be added to the trip point parameters before estimate_pid_constants(), and the sustainable_power argument should be removed. Otherwise we end up with conflicting information in the code. Regards, Ionela. > } > > err = control_temp - tz->temperature; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] thermal: power allocator: estimate sustainable power only once 2020-10-08 10:14 ` Ionela Voinescu @ 2020-10-08 12:34 ` Lukasz Luba 0 siblings, 0 replies; 10+ messages in thread From: Lukasz Luba @ 2020-10-08 12:34 UTC (permalink / raw) To: Ionela Voinescu Cc: linux-kernel, linux-pm, daniel.lezcano, amitk, Dietmar.Eggemann Hi Ionela, On 10/8/20 11:14 AM, Ionela Voinescu wrote: > Hi Lukasz, > > On Friday 02 Oct 2020 at 13:24:16 (+0100), 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. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> drivers/thermal/gov_power_allocator.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c >> index f69fafe486a5..dd59085f38f5 100644 >> --- a/drivers/thermal/gov_power_allocator.c >> +++ b/drivers/thermal/gov_power_allocator.c >> @@ -204,6 +204,8 @@ static u32 pid_controller(struct thermal_zone_device *tz, >> estimate_pid_constants(tz, sustainable_power, >> params->trip_switch_on, control_temp, >> true); >> + /* Do the estimation only once and make available in sysfs */ >> + tz->tzp->sustainable_power = sustainable_power; > > After looking over the code, it does seems mostly useless to do the > estimation every time the controller kicks in. > > But I have two comments in this regard: > > - The estimation is dependent on the temperature we control for which > can be changed from sysfs. While I don't see that as a big worry, > (sustainable power is an estimation anyway), it might be worth a > more detailed comment on why we don't expect this to be a problem, > or what we expect the consequences of computing sustainable power > only once could be. The problem is that we don't expose the estimated value in the sysfs. This is the case when there was no DT entry with 'sustainable-power'. If someone is going to write the values via sysfs, we assume he/she knows the consequences and also what other values to write and where, to make it working optimally. > > - In the function comment for estimate_pid_constants() there is a > mention of sustainable power: > """ > * Sustainable power is provided in case it was estimated. The > * estimated sustainable_power should not be stored in the > * thermal_zone_parameters so it has to be passed explicitly to this > * function. > """ Good catch, that comment left. I will remove it. > If we are going to compute the sustainable power estimation only once, > this comment should be removed, the estimated value should be added to > the trip point parameters before estimate_pid_constants(), and the > sustainable_power argument should be removed. > Otherwise we end up with conflicting information in the code. We can also call estimate_sustainable_power() inside the estimate_pid_constants() if sust. power was 0, then set the tz->tzp->sustainable_power = sustainable_power Thank you for your comments. Regards, Lukasz > > Regards, > Ionela. > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-13 15:56 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-02 12:24 [PATCH 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba 2020-10-02 12:24 ` [PATCH 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba 2020-10-13 10:21 ` Daniel Lezcano 2020-10-13 10:59 ` Lukasz Luba 2020-10-13 11:22 ` Daniel Lezcano 2020-10-13 12:04 ` Lukasz Luba 2020-10-13 15:56 ` Daniel Lezcano 2020-10-02 12:24 ` [PATCH 2/2] thermal: power allocator: estimate sustainable power only once Lukasz Luba 2020-10-08 10:14 ` Ionela Voinescu 2020-10-08 12:34 ` Lukasz Luba
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).