Re: [PATCH v3 2/2] thermal: power allocator: change how estimation code is called

[Lukasz Luba]

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.l...@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