| Message ID | 20201124161025.27694-1-lukasz.luba@arm.com |
|---|---|
| Headers | show |
| Series | Improve the estimations in Intelligent Power Allocation | expand |
Hi Daniel, On 11/24/20 4:10 PM, Lukasz Luba wrote: > 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 of sustainable power only once and avoid expensive > calculation every time the IPA is called. Do the estimation of PID constants > when there was user update via sysfs to sustainable power. > > The patch set should apply on top next-20201124 > > Changes: > v4: > - added new function get_sustainable_power() which handles use cases > when the value should be estimated again or simply returned > - added sustainable_power in the power_allocator_params to track if there > was a change to sustainable_power by the user via sysfs > - addressed Daniel's comments that sustainable power set via sysfs should > trigger PID coefficients estimation > - removed 'force' argument from estimate_pid_constants() and make it ready > for updates due to new value for sust. power from sysfs > - abandoned the design from v3 with a single function responsible for > estimation both sust. power and PID const. requested by Ionela > v3 [1]: > - changed estimate_pid_constants to estimate_tzp_constants and related comments > - estimate the PID coefficients always together with sust. power > - added print indicating that we are estimating sust. power and PID const. > - don't use local variable 'sustainable_power' > > Regards, > Lukasz Luba > > [1] https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/ > > Lukasz Luba (3): > thermal: power allocator: change the 'k_i' coefficient estimation > thermal: power allocator: refactor sustainable power estimation > thermal: power allocator: change the 'k_*' always in > estimate_pid_constants() > > drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++---------- > 1 file changed, 49 insertions(+), 27 deletions(-) > Gentle ping. This is a self contained change to only power allocator file. It addresses also your requirement regarding sustainable_power changed via sysfs. Could you take it please? It should apply smoothly in your tree. Regards, Lukasz
On 26/11/2020 13:49, Lukasz Luba wrote: > Hi Daniel, > > On 11/24/20 4:10 PM, Lukasz Luba wrote: >> 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 of sustainable power only once and avoid >> expensive >> calculation every time the IPA is called. Do the estimation of PID >> constants >> when there was user update via sysfs to sustainable power. >> >> The patch set should apply on top next-20201124 >> >> Changes: >> v4: >> - added new function get_sustainable_power() which handles use cases >> when the value should be estimated again or simply returned >> - added sustainable_power in the power_allocator_params to track if there >> was a change to sustainable_power by the user via sysfs >> - addressed Daniel's comments that sustainable power set via sysfs should >> trigger PID coefficients estimation >> - removed 'force' argument from estimate_pid_constants() and make it >> ready >> for updates due to new value for sust. power from sysfs >> - abandoned the design from v3 with a single function responsible for >> estimation both sust. power and PID const. requested by Ionela >> v3 [1]: >> - changed estimate_pid_constants to estimate_tzp_constants and related >> comments >> - estimate the PID coefficients always together with sust. power >> - added print indicating that we are estimating sust. power and PID >> const. >> - don't use local variable 'sustainable_power' >> >> Regards, >> Lukasz Luba >> >> [1] >> https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/ >> >> Lukasz Luba (3): >> thermal: power allocator: change the 'k_i' coefficient estimation >> thermal: power allocator: refactor sustainable power estimation >> thermal: power allocator: change the 'k_*' always in >> estimate_pid_constants() >> >> drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++---------- >> 1 file changed, 49 insertions(+), 27 deletions(-) >> > > Gentle ping. This is a self contained change to only power allocator > file. It addresses also your requirement regarding sustainable_power > changed via sysfs. > > Could you take it please? It should apply smoothly in your tree. Actually, I'm waiting for Ionela and Dietmar ack. -- <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
On 11/26/20 1:09 PM, Daniel Lezcano wrote: > On 26/11/2020 13:49, Lukasz Luba wrote: >> Hi Daniel, >> >> On 11/24/20 4:10 PM, Lukasz Luba wrote: >>> 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 of sustainable power only once and avoid >>> expensive >>> calculation every time the IPA is called. Do the estimation of PID >>> constants >>> when there was user update via sysfs to sustainable power. >>> >>> The patch set should apply on top next-20201124 >>> >>> Changes: >>> v4: >>> - added new function get_sustainable_power() which handles use cases >>> when the value should be estimated again or simply returned >>> - added sustainable_power in the power_allocator_params to track if there >>> was a change to sustainable_power by the user via sysfs >>> - addressed Daniel's comments that sustainable power set via sysfs should >>> trigger PID coefficients estimation >>> - removed 'force' argument from estimate_pid_constants() and make it >>> ready >>> for updates due to new value for sust. power from sysfs >>> - abandoned the design from v3 with a single function responsible for >>> estimation both sust. power and PID const. requested by Ionela >>> v3 [1]: >>> - changed estimate_pid_constants to estimate_tzp_constants and related >>> comments >>> - estimate the PID coefficients always together with sust. power >>> - added print indicating that we are estimating sust. power and PID >>> const. >>> - don't use local variable 'sustainable_power' >>> >>> Regards, >>> Lukasz Luba >>> >>> [1] >>> https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/ >>> >>> Lukasz Luba (3): >>> thermal: power allocator: change the 'k_i' coefficient estimation >>> thermal: power allocator: refactor sustainable power estimation >>> thermal: power allocator: change the 'k_*' always in >>> estimate_pid_constants() >>> >>> drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++---------- >>> 1 file changed, 49 insertions(+), 27 deletions(-) >>> >> >> Gentle ping. This is a self contained change to only power allocator >> file. It addresses also your requirement regarding sustainable_power >> changed via sysfs. >> >> Could you take it please? It should apply smoothly in your tree. > > Actually, I'm waiting for Ionela and Dietmar ack. > > Are they maintainers of this file that you need their ACKs? Maybe I should drop mine then.
On 26/11/2020 15:02, Lukasz Luba wrote: [ ... ] >>> changed via sysfs. >>> >>> Could you take it please? It should apply smoothly in your tree. >> >> Actually, I'm waiting for Ionela and Dietmar ack. >> >> > > Are they maintainers of this file that you need their ACKs? > Maybe I should drop mine then. Ok let me clarify :) In general when someone comments on the changes, I usually wait for the consensus before merging the patches. If the persons who commented before are unresponsive, depending on the context and my perception, I apply the changes or not. I'm giving the opportunity to Ionela to review the series again as she commented the previous version (and gave a reviewed-by). I thought also Dietmar commented the series but apparently I was wrong. As you stated, you are the maintainer of the subsystem, so if there are no acked-by or reviewed-by, the series will be applied anyway soon. Meanwhile, they are in the 'testing' branch of the tree, the antechamber of 'linux-next' and 'next' ;) -- <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
On 11/26/20 2:30 PM, Daniel Lezcano wrote: > On 26/11/2020 15:02, Lukasz Luba wrote: > > [ ... ] > >>>> changed via sysfs. >>>> >>>> Could you take it please? It should apply smoothly in your tree. >>> >>> Actually, I'm waiting for Ionela and Dietmar ack. >>> >>> >> >> Are they maintainers of this file that you need their ACKs? >> Maybe I should drop mine then. > > Ok let me clarify :) > > In general when someone comments on the changes, I usually wait for the > consensus before merging the patches. If the persons who commented > before are unresponsive, depending on the context and my perception, I > apply the changes or not. > > I'm giving the opportunity to Ionela to review the series again as she > commented the previous version (and gave a reviewed-by). I thought also > Dietmar commented the series but apparently I was wrong. > > As you stated, you are the maintainer of the subsystem, so if there are > no acked-by or reviewed-by, the series will be applied anyway soon. > > Meanwhile, they are in the 'testing' branch of the tree, the antechamber > of 'linux-next' and 'next' ;) > Thank you clarifying this and taking the patches into the testing branch. Regards, Lukasz
Hi Lukasz, On Tuesday 24 Nov 2020 at 16:10:23 (+0000), 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 b29e21c56a4f..2e20085ed217 100644 > --- a/drivers/thermal/gov_power_allocator.c > +++ b/drivers/thermal/gov_power_allocator.c > @@ -134,6 +134,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) > @@ -159,8 +160,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. > -- I see this patch did not change so: Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> > 2.17.1 >
On Tuesday 24 Nov 2020 at 16:10:25 (+0000), Lukasz Luba wrote: > The PID coefficients should be estimated again when there was a change to > sustainable power value made by user. This change removes unused argument > 'force' and makes the function ready for such updates. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > drivers/thermal/gov_power_allocator.c | 28 +++++++++------------------ > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c > index d7e4b9f6af60..7a4170a0b51f 100644 > --- a/drivers/thermal/gov_power_allocator.c > +++ b/drivers/thermal/gov_power_allocator.c > @@ -117,18 +117,13 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz) > * @sustainable_power: sustainable power for the thermal zone > * @trip_switch_on: trip point number for the switch on temperature > * @control_temp: target temperature for the power allocator governor > - * @force: whether to force the update of the constants > * > * This function is used to update the estimation of the PID > * controller constants in struct thermal_zone_parameters. > - * > - * If @force is not set, the values in the thermal zone's parameters > - * are preserved if they are not zero. If @force is set, the values > - * in thermal zone's parameters are overwritten. > */ > static void estimate_pid_constants(struct thermal_zone_device *tz, > u32 sustainable_power, int trip_switch_on, > - int control_temp, bool force) > + int control_temp) > { > int ret; > int switch_on_temp; > @@ -151,18 +146,14 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, > if (!temperature_threshold) > return; > > - if (!tz->tzp->k_po || force) > - tz->tzp->k_po = int_to_frac(sustainable_power) / > - temperature_threshold; > + tz->tzp->k_po = int_to_frac(sustainable_power) / > + temperature_threshold; > > - if (!tz->tzp->k_pu || force) > - tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / > - temperature_threshold; > + tz->tzp->k_pu = int_to_frac(2 * sustainable_power) / > + temperature_threshold; > > - if (!tz->tzp->k_i || force) { > - k_i = tz->tzp->k_pu / 10; > - tz->tzp->k_i = k_i > 0 ? k_i : 1; > - } > + 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 > @@ -195,8 +186,7 @@ static u32 get_sustainable_power(struct thermal_zone_device *tz, > /* Check if it's init value 0 or there was update via sysfs */ > if (sustainable_power != params->sustainable_power) { > estimate_pid_constants(tz, sustainable_power, > - params->trip_switch_on, control_temp, > - true); > + params->trip_switch_on, control_temp); > > /* Do the estimation only once and make available in sysfs */ > tz->tzp->sustainable_power = sustainable_power; > @@ -640,7 +630,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz) > if (!ret) > estimate_pid_constants(tz, tz->tzp->sustainable_power, > params->trip_switch_on, > - control_temp, false); > + control_temp); > } > > reset_pid_controller(params); > -- I was actually wondering why we still need force, while reading 2/3. It looks good! Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> > 2.17.1 >
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 of sustainable power only once and avoid expensive calculation every time the IPA is called. Do the estimation of PID constants when there was user update via sysfs to sustainable power. The patch set should apply on top next-20201124 Changes: v4: - added new function get_sustainable_power() which handles use cases when the value should be estimated again or simply returned - added sustainable_power in the power_allocator_params to track if there was a change to sustainable_power by the user via sysfs - addressed Daniel's comments that sustainable power set via sysfs should trigger PID coefficients estimation - removed 'force' argument from estimate_pid_constants() and make it ready for updates due to new value for sust. power from sysfs - abandoned the design from v3 with a single function responsible for estimation both sust. power and PID const. requested by Ionela v3 [1]: - changed estimate_pid_constants to estimate_tzp_constants and related comments - estimate the PID coefficients always together with sust. power - added print indicating that we are estimating sust. power and PID const. - don't use local variable 'sustainable_power' Regards, Lukasz Luba [1] https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/ Lukasz Luba (3): thermal: power allocator: change the 'k_i' coefficient estimation thermal: power allocator: refactor sustainable power estimation thermal: power allocator: change the 'k_*' always in estimate_pid_constants() drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++---------- 1 file changed, 49 insertions(+), 27 deletions(-)