| Message ID | 20201002122416.13659-3-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | Improve the estimations in Intelligent Power Allocation | expand |
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 >
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. >
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;
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(+)