* [PATCH v3 1/5] thermal: introduce the Power Allocator governor
2015-03-02 17:17 [PATCH v3 0/5] Subject: The power allocator thermal governor Javi Merino
@ 2015-03-02 17:17 ` Javi Merino
2015-03-02 17:17 ` [PATCH v3 2/5] thermal: add trace events to the power allocator governor Javi Merino
` (4 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Javi Merino @ 2015-03-02 17:17 UTC (permalink / raw)
To: edubezval, rui.zhang
Cc: linux-pm, linux-kernel, punit.agrawal, lina.iyer, broonie, tixy,
Javi Merino
The power allocator governor is a thermal governor that controls system
and device power allocation to control temperature. Conceptually, the
implementation divides the sustainable power of a thermal zone among
all the heat sources in that zone.
This governor relies on "power actors", entities that represent heat
sources. They can report current and maximum power consumption and
can set a given maximum power consumption, usually via a cooling
device.
The governor uses a Proportional Integral Derivative (PID) controller
driven by the temperature of the thermal zone. The output of the
controller is a power budget that is then allocated to each power
actor that can have bearing on the temperature we are trying to
control. It decides how much power to give each cooling device based
on the performance they are requesting. The PID controller ensures
that the total power budget does not exceed the control temperature.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
Documentation/thermal/power_allocator.txt | 247 ++++++++++++++
drivers/thermal/Kconfig | 15 +
drivers/thermal/Makefile | 1 +
drivers/thermal/power_allocator.c | 520 ++++++++++++++++++++++++++++++
drivers/thermal/thermal_core.c | 9 +-
drivers/thermal/thermal_core.h | 8 +
include/linux/thermal.h | 37 ++-
7 files changed, 830 insertions(+), 7 deletions(-)
create mode 100644 Documentation/thermal/power_allocator.txt
create mode 100644 drivers/thermal/power_allocator.c
diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
new file mode 100644
index 000000000000..c3797b529991
--- /dev/null
+++ b/Documentation/thermal/power_allocator.txt
@@ -0,0 +1,247 @@
+Power allocator governor tunables
+=================================
+
+Trip points
+-----------
+
+The governor requires the following two passive trip points:
+
+1. "switch on" trip point: temperature above which the governor
+ control loop starts operating. This is the first passive trip
+ point of the thermal zone.
+
+2. "desired temperature" trip point: it should be higher than the
+ "switch on" trip point. This the target temperature the governor
+ is controlling for. This is the last passive trip point of the
+ thermal zone.
+
+PID Controller
+--------------
+
+The power allocator governor implements a
+Proportional-Integral-Derivative controller (PID controller) with
+temperature as the control input and power as the controlled output:
+
+ P_max = k_p * e + k_i * err_integral + k_d * diff_err + sustainable_power
+
+where
+ e = desired_temperature - current_temperature
+ err_integral is the sum of previous errors
+ diff_err = e - previous_error
+
+It is similar to the one depicted below:
+
+ k_d
+ |
+current_temp |
+ | v
+ | +----------+ +---+
+ | +----->| diff_err |-->| X |------+
+ | | +----------+ +---+ |
+ | | | tdp actor
+ | | k_i | | get_requested_power()
+ | | | | | | |
+ | | | | | | | ...
+ v | v v v v v
+ +---+ | +-------+ +---+ +---+ +---+ +----------+
+ | S |-------+----->| sum e |----->| X |--->| S |-->| S |-->|power |
+ +---+ | +-------+ +---+ +---+ +---+ |allocation|
+ ^ | ^ +----------+
+ | | | | |
+ | | +---+ | | |
+ | +------->| X |-------------------+ v v
+ | +---+ granted performance
+desired_temperature ^
+ |
+ |
+ k_po/k_pu
+
+Sustainable power
+-----------------
+
+An estimate of the sustainable dissipatable power (in mW) should be
+provided while registering the thermal zone. This estimates the
+sustained power that can be dissipated at the desired control
+temperature. This is the maximum sustained power for allocation at
+the desired maximum temperature. The actual sustained power can vary
+for a number of reasons. The closed loop controller will take care of
+variations such as environmental conditions, and some factors related
+to the speed-grade of the silicon. `sustainable_power` is therefore
+simply an estimate, and may be tuned to affect the aggressiveness of
+the thermal ramp. For reference, the sustainable power of a 4" phone
+is typically 2000mW, while on a 10" tablet is around 4500mW (may vary
+depending on screen size).
+
+If you are using device tree, do add it as a property of the
+thermal-zone. For example:
+
+ thermal-zones {
+ soc_thermal {
+ polling-delay = <1000>;
+ polling-delay-passive = <100>;
+ sustainable-power = <2500>;
+ ...
+
+Instead, if the thermal zone is registered from the platform code, pass a
+`thermal_zone_params` that has a `sustainable_power`. If no
+`thermal_zone_params` were being passed, then something like below
+will suffice:
+
+ static const struct thermal_zone_params tz_params = {
+ .sustainable_power = 3500,
+ };
+
+and then pass `tz_params` as the 5th parameter to
+`thermal_zone_device_register()`
+
+k_po and k_pu
+-------------
+
+The implementation of the PID controller in the power allocator
+thermal governor allows the configuration of two proportional term
+constants: `k_po` and `k_pu`. `k_po` is the proportional term
+constant during temperature overshoot periods (current temperature is
+above "desired temperature" trip point). Conversely, `k_pu` is the
+proportional term constant during temperature undershoot periods
+(current temperature below "desired temperature" trip point).
+
+These controls are intended as the primary mechanism for configuring
+the permitted thermal "ramp" of the system. For instance, a lower
+`k_pu` value will provide a slower ramp, at the cost of capping
+available capacity at a low temperature. On the other hand, a high
+value of `k_pu` will result in the governor granting very high power
+whilst temperature is low, and may lead to temperature overshooting.
+
+The default value for `k_pu` is:
+
+ 2 * sustainable_power / (desired_temperature - switch_on_temp)
+
+This means that at `switch_on_temp` the output of the controller's
+proportional term will be 2 * `sustainable_power`. The default value
+for `k_po` is:
+
+ sustainable_power / (desired_temperature - switch_on_temp)
+
+Focusing on the proportional and feed forward values of the PID
+controller equation we have:
+
+ P_max = k_p * e + sustainable_power
+
+The proportional term is proportional to the difference between the
+desired temperature and the current one. When the current temperature
+is the desired one, then the proportional component is zero and
+`P_max` = `sustainable_power`. That is, the system should operate in
+thermal equilibrium under constant load. `sustainable_power` is only
+an estimate, which is the reason for closed-loop control such as this.
+
+Expanding `k_pu` we get:
+ P_max = 2 * sustainable_power * (T_set - T) / (T_set - T_on) +
+ sustainable_power
+
+where
+ T_set is the desired temperature
+ T is the current temperature
+ T_on is the switch on temperature
+
+When the current temperature is the switch_on temperature, the above
+formula becomes:
+
+ P_max = 2 * sustainable_power * (T_set - T_on) / (T_set - T_on) +
+ sustainable_power = 2 * sustainable_power + sustainable_power =
+ 3 * sustainable_power
+
+Therefore, the proportional term alone linearly decreases power from
+3 * `sustainable_power` to `sustainable_power` as the temperature
+rises from the switch on temperature to the desired temperature.
+
+k_i and integral_cutoff
+-----------------------
+
+`k_i` configures the PID loop's integral term constant. This term
+allows the PID controller to compensate for long term drift and for
+the quantized nature of the output control: cooling devices can't set
+the exact power that the governor requests. When the temperature
+error is below `integral_cutoff`, errors are accumulated in the
+integral term. This term is then multiplied by `k_i` and the result
+added to the output of the controller. Typically `k_i` is set low (1
+or 2) and `integral_cutoff` is 0.
+
+k_d
+---
+
+`k_d` configures the PID loop's derivative term constant. It's
+recommended to leave it as the default: 0.
+
+Cooling device power API
+========================
+
+Cooling devices controlled by this governor must supply the additional
+"power" API in their `cooling_device_ops`. It consists on three ops:
+
+1. int get_requested_power(struct thermal_cooling_device *cdev,
+ struct thermal_zone_device *tz, u32 *power);
+@cdev: The `struct thermal_cooling_device` pointer
+@tz: thermal zone in which we are currently operating
+@power: pointer in which to store the calculated power
+
+`get_requested_power()` calculates the power requested by the device
+in milliwatts and stores it in @power . It should return 0 on
+success, -E* on failure. This is currently used by the power
+allocator governor to calculate how much power to give to each cooling
+device.
+
+2. int state2power(struct thermal_cooling_device *cdev, struct
+ thermal_zone_device *tz, unsigned long state, u32 *power);
+@cdev: The `struct thermal_cooling_device` pointer
+@tz: thermal zone in which we are currently operating
+@state: A cooling device state
+@power: pointer in which to store the equivalent power
+
+Convert cooling device state @state into power consumption in
+milliwatts and store it in @power. It should return 0 on success, -E*
+on failure. This is currently used by thermal core to calculate the
+maximum power that an actor can consume.
+
+3. int power2state(struct thermal_cooling_device *cdev, u32 power,
+ unsigned long *state);
+@cdev: The `struct thermal_cooling_device` pointer
+@power: power in milliwatts
+@state: pointer in which to store the resulting state
+
+Calculate a cooling device state that would make the device consume at
+most @power mW and store it in @state. It should return 0 on success,
+-E* on failure. This is currently used by the thermal core to convert
+a given power set by the power allocator governor to a state that the
+cooling device can set. It is a function because this conversion may
+depend on external factors that may change so this function should the
+best conversion given "current circumstances".
+
+Cooling device weights
+----------------------
+
+Weights are a mechanism to bias the allocation among cooling
+devices. They express the relative power efficiency of different
+cooling devices. Higher weight can be used to express higher power
+efficiency. Weighting is relative such that if each cooling device
+has a weight of one they are considered equal. This is particularly
+useful in heterogeneous systems where two cooling devices may perform
+the same kind of compute, but with different efficiency. For example,
+a system with two different types of processors.
+
+If the thermal zone is registered using
+`thermal_zone_device_register()` (i.e., platform code), then weights
+are passed as part of the thermal zone's `thermal_bind_parameters`.
+If the platform is registered using device tree, then they are passed
+as the `contribution` property of each map in the `cooling-maps` node.
+
+Limitations of the power allocator governor
+===========================================
+
+The power allocator governor's PID controller works best if there is a
+periodic tick. If you have a driver that calls
+`thermal_zone_device_update()` (or anything that ends up calling the
+governor's `throttle()` function) repetitively, the governor response
+won't be very good. Note that this is not particular to this
+governor, step-wise will also misbehave if you call its throttle()
+faster than the normal thermal framework tick (due to interrupts for
+example) as it will overreact.
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 30aee81e9f5b..a1b43eab0a70 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -71,6 +71,14 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
Select this if you want to let the user space manage the
platform thermals.
+config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
+ bool "power_allocator"
+ select THERMAL_GOV_POWER_ALLOCATOR
+ help
+ Select this if you want to control temperature based on
+ system and device power allocation. This governor can only
+ operate on cooling devices that implement the power API.
+
endchoice
config THERMAL_GOV_FAIR_SHARE
@@ -99,6 +107,13 @@ config THERMAL_GOV_USER_SPACE
help
Enable this to let the user space manage the platform thermals.
+config THERMAL_GOV_POWER_ALLOCATOR
+ bool "Power allocator thermal governor"
+ select THERMAL_POWER_ACTOR
+ help
+ Enable this to manage platform thermals by dynamically
+ allocating and limiting power to devices.
+
config CPU_THERMAL
bool "generic cpu cooling support"
depends on CPU_FREQ
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 1fe86652cfb6..b1783cf37ed2 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -14,6 +14,7 @@ thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += fair_share.o
thermal_sys-$(CONFIG_THERMAL_GOV_BANG_BANG) += gov_bang_bang.o
thermal_sys-$(CONFIG_THERMAL_GOV_STEP_WISE) += step_wise.o
thermal_sys-$(CONFIG_THERMAL_GOV_USER_SPACE) += user_space.o
+thermal_sys-$(CONFIG_THERMAL_GOV_POWER_ALLOCATOR) += power_allocator.o
# cpufreq cooling
thermal_sys-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
new file mode 100644
index 000000000000..67982d79b76c
--- /dev/null
+++ b/drivers/thermal/power_allocator.c
@@ -0,0 +1,520 @@
+/*
+ * A power allocator to manage temperature
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "Power allocator: " fmt
+
+#include <linux/rculist.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+#define FRAC_BITS 10
+#define int_to_frac(x) ((x) << FRAC_BITS)
+#define frac_to_int(x) ((x) >> FRAC_BITS)
+
+/**
+ * mul_frac() - multiply two fixed-point numbers
+ * @x: first multiplicand
+ * @y: second multiplicand
+ *
+ * Return: the result of multiplying two fixed-point numbers. The
+ * result is also a fixed-point number.
+ */
+static inline s64 mul_frac(s64 x, s64 y)
+{
+ return (x * y) >> FRAC_BITS;
+}
+
+/**
+ * div_frac() - divide two fixed-point numbers
+ * @x: the dividend
+ * @y: the divisor
+ *
+ * Return: the result of dividing two fixed-point numbers. The
+ * result is also a fixed-point number.
+ */
+static inline s64 div_frac(s64 x, s64 y)
+{
+ return div_s64(x << FRAC_BITS, y);
+}
+
+/**
+ * struct power_allocator_params - parameters for the power allocator governor
+ * @err_integral: accumulated error in the PID controller.
+ * @prev_err: error in the previous iteration of the PID controller.
+ * Used to calculate the derivative term.
+ * @trip_switch_on: first passive trip point of the thermal zone. The
+ * governor switches on when this trip point is crossed.
+ * @trip_max_desired_temperature: last passive trip point of the thermal
+ * zone. The temperature we are
+ * controlling for.
+ */
+struct power_allocator_params {
+ s64 err_integral;
+ s32 prev_err;
+ int trip_switch_on;
+ int trip_max_desired_temperature;
+};
+
+/**
+ * pid_controller() - PID controller
+ * @tz: thermal zone we are operating in
+ * @current_temp: the current temperature in millicelsius
+ * @control_temp: the target temperature in millicelsius
+ * @max_allocatable_power: maximum allocatable power for this thermal zone
+ *
+ * This PID controller increases the available power budget so that the
+ * temperature of the thermal zone gets as close as possible to
+ * @control_temp and limits the power if it exceeds it. k_po is the
+ * proportional term when we are overshooting, k_pu is the
+ * proportional term when we are undershooting. integral_cutoff is a
+ * threshold below which we stop accumulating the error. The
+ * accumulated error is only valid if the requested power will make
+ * the system warmer. If the system is mostly idle, there's no point
+ * in accumulating positive error.
+ *
+ * Return: The power budget for the next period.
+ */
+static u32 pid_controller(struct thermal_zone_device *tz,
+ unsigned long current_temp,
+ unsigned long control_temp,
+ u32 max_allocatable_power)
+{
+ s64 p, i, d, power_range;
+ s32 err, max_power_frac;
+ struct power_allocator_params *params = tz->governor_data;
+
+ max_power_frac = int_to_frac(max_allocatable_power);
+
+ err = ((s32)control_temp - (s32)current_temp);
+ err = int_to_frac(err);
+
+ /* Calculate the proportional term */
+ p = mul_frac(err < 0 ? tz->tzp->k_po : tz->tzp->k_pu, err);
+
+ /*
+ * Calculate the integral term
+ *
+ * if the error is less than cut off allow integration (but
+ * the integral is limited to max power)
+ */
+ i = mul_frac(tz->tzp->k_i, params->err_integral);
+
+ if (err < int_to_frac(tz->tzp->integral_cutoff)) {
+ s64 i_next = i + mul_frac(tz->tzp->k_i, err);
+
+ if (abs64(i_next) < max_power_frac) {
+ i = i_next;
+ params->err_integral += err;
+ }
+ }
+
+ /*
+ * Calculate the derivative term
+ *
+ * We do err - prev_err, so with a positive k_d, a decreasing
+ * error (i.e. driving closer to the line) results in less
+ * power being applied, slowing down the controller)
+ */
+ d = mul_frac(tz->tzp->k_d, err - params->prev_err);
+ d = div_frac(d, tz->passive_delay);
+ params->prev_err = err;
+
+ power_range = p + i + d;
+
+ /* feed-forward the known sustainable dissipatable power */
+ power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+
+ return clamp(power_range, (s64)0, (s64)max_allocatable_power);
+}
+
+/**
+ * divvy_up_power() - divvy the allocated power between the actors
+ * @req_power: each actor's requested power
+ * @max_power: each actor's maximum available power
+ * @num_actors: size of the @req_power, @max_power and @granted_power's array
+ * @total_req_power: sum of @req_power
+ * @power_range: total allocated power
+ * @granted_power: output array: each actor's granted power
+ * @extra_actor_power: an appropriately sized array to be used in the
+ * function as temporary storage of the extra power given
+ * to the actors
+ *
+ * This function divides the total allocated power (@power_range)
+ * fairly between the actors. It first tries to give each actor a
+ * share of the @power_range according to how much power it requested
+ * compared to the rest of the actors. For example, if only one actor
+ * requests power, then it receives all the @power_range. If
+ * three actors each requests 1mW, each receives a third of the
+ * @power_range.
+ *
+ * If any actor received more than their maximum power, then that
+ * surplus is re-divvied among the actors based on how far they are
+ * from their respective maximums.
+ *
+ * Granted power for each actor is written to @granted_power, which
+ * should've been allocated by the calling function.
+ */
+static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
+ u32 total_req_power, u32 power_range,
+ u32 *granted_power, u32 *extra_actor_power)
+{
+ u32 extra_power, capped_extra_power;
+ int i;
+
+ /*
+ * Prevent division by 0 if none of the actors request power.
+ */
+ if (!total_req_power)
+ total_req_power = 1;
+
+ capped_extra_power = 0;
+ extra_power = 0;
+ for (i = 0; i < num_actors; i++) {
+ u64 req_range = req_power[i] * power_range;
+
+ granted_power[i] = div_u64(req_range, total_req_power);
+
+ if (granted_power[i] > max_power[i]) {
+ extra_power += granted_power[i] - max_power[i];
+ granted_power[i] = max_power[i];
+ }
+
+ extra_actor_power[i] = max_power[i] - granted_power[i];
+ capped_extra_power += extra_actor_power[i];
+ }
+
+ if (!extra_power)
+ return;
+
+ /*
+ * Re-divvy the reclaimed extra among actors based on
+ * how far they are from the max
+ */
+ extra_power = min(extra_power, capped_extra_power);
+ if (capped_extra_power > 0)
+ for (i = 0; i < num_actors; i++)
+ granted_power[i] += (extra_actor_power[i] *
+ extra_power) / capped_extra_power;
+}
+
+static int allocate_power(struct thermal_zone_device *tz,
+ unsigned long current_temp,
+ unsigned long control_temp)
+{
+ struct thermal_instance *instance;
+ struct power_allocator_params *params = tz->governor_data;
+ u32 *req_power, *max_power, *granted_power, *extra_actor_power;
+ u32 total_req_power, max_allocatable_power;
+ u32 power_range;
+ int i, num_actors, total_weight, ret = 0;
+ int trip_max_desired_temperature = params->trip_max_desired_temperature;
+
+ mutex_lock(&tz->lock);
+
+ num_actors = 0;
+ total_weight = 0;
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ if ((instance->trip == trip_max_desired_temperature) &&
+ cdev_is_power_actor(instance->cdev)) {
+ num_actors++;
+ total_weight += instance->weight;
+ }
+ }
+
+ /*
+ * We need to allocate three arrays of the same size:
+ * req_power, max_power and granted_power. They are going to
+ * be needed until this function returns. Allocate them all
+ * in one go to simplify the allocation and deallocation
+ * logic.
+ */
+ BUILD_BUG_ON(sizeof(*req_power) != sizeof(*max_power));
+ BUILD_BUG_ON(sizeof(*req_power) != sizeof(*granted_power));
+ BUILD_BUG_ON(sizeof(*req_power) != sizeof(*extra_actor_power));
+ req_power = devm_kcalloc(&tz->device, num_actors * 4,
+ sizeof(*req_power), GFP_KERNEL);
+ if (!req_power) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ max_power = &req_power[num_actors];
+ granted_power = &req_power[2 * num_actors];
+ extra_actor_power = &req_power[3 * num_actors];
+
+ i = 0;
+ total_req_power = 0;
+ max_allocatable_power = 0;
+
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ int weight;
+ struct thermal_cooling_device *cdev = instance->cdev;
+
+ if (instance->trip != trip_max_desired_temperature)
+ continue;
+
+ if (!cdev_is_power_actor(cdev))
+ continue;
+
+ if (cdev->ops->get_requested_power(cdev, tz, &req_power[i]))
+ continue;
+
+ if (!total_weight)
+ weight = 1 << FRAC_BITS;
+ else
+ weight = instance->weight;
+
+ req_power[i] = frac_to_int(weight * req_power[i]);
+
+ if (power_actor_get_max_power(cdev, tz, &max_power[i]))
+ continue;
+
+ total_req_power += req_power[i];
+ max_allocatable_power += max_power[i];
+
+ i++;
+ }
+
+ power_range = pid_controller(tz, current_temp, control_temp,
+ max_allocatable_power);
+
+ divvy_up_power(req_power, max_power, num_actors, total_req_power,
+ power_range, granted_power, extra_actor_power);
+
+ i = 0;
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ if (instance->trip != trip_max_desired_temperature)
+ continue;
+
+ if (!cdev_is_power_actor(instance->cdev))
+ continue;
+
+ power_actor_set_power(instance->cdev, instance,
+ granted_power[i]);
+
+ i++;
+ }
+
+ devm_kfree(&tz->device, req_power);
+unlock:
+ mutex_unlock(&tz->lock);
+
+ return ret;
+}
+
+static int get_governor_trips(struct thermal_zone_device *tz,
+ struct power_allocator_params *params)
+{
+ int i, ret, last_passive;
+ bool found_first_passive;
+
+ found_first_passive = false;
+ last_passive = -1;
+ ret = -EINVAL;
+
+ for (i = 0; i < tz->trips; i++) {
+ enum thermal_trip_type type;
+
+ ret = tz->ops->get_trip_type(tz, i, &type);
+ if (ret)
+ return ret;
+
+ if (!found_first_passive) {
+ if (type == THERMAL_TRIP_PASSIVE) {
+ params->trip_switch_on = i;
+ found_first_passive = true;
+ }
+ } else if (type == THERMAL_TRIP_PASSIVE) {
+ last_passive = i;
+ } else {
+ break;
+ }
+ }
+
+ if (last_passive != -1) {
+ params->trip_max_desired_temperature = last_passive;
+ ret = 0;
+ } else {
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static void reset_pid_controller(struct power_allocator_params *params)
+{
+ params->err_integral = 0;
+ params->prev_err = 0;
+}
+
+static void allow_maximum_power(struct thermal_zone_device *tz)
+{
+ struct thermal_instance *instance;
+ struct power_allocator_params *params = tz->governor_data;
+
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ if ((instance->trip != params->trip_max_desired_temperature) ||
+ (!cdev_is_power_actor(instance->cdev)))
+ continue;
+
+ instance->target = 0;
+ instance->cdev->updated = false;
+ thermal_cdev_update(instance->cdev);
+ }
+}
+
+/**
+ * power_allocator_bind() - bind the power_allocator governor to a thermal zone
+ * @tz: thermal zone to bind it to
+ *
+ * Check that the thermal zone is valid for this governor, that is, it
+ * has two thermal trips. If so, initialize the PID controller
+ * parameters and bind it to the thermal zone.
+ *
+ * Return: 0 on success, -EINVAL if the trips were invalid or -ENOMEM
+ * if we ran out of memory.
+ */
+static int power_allocator_bind(struct thermal_zone_device *tz)
+{
+ int ret;
+ struct power_allocator_params *params;
+ unsigned long switch_on_temp, control_temp;
+ u32 temperature_threshold;
+
+ if (!tz->tzp || !tz->tzp->sustainable_power) {
+ dev_err(&tz->device,
+ "power_allocator: missing sustainable_power\n");
+ return -EINVAL;
+ }
+
+ params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
+ if (!params)
+ return -ENOMEM;
+
+ ret = get_governor_trips(tz, params);
+ if (ret) {
+ dev_err(&tz->device,
+ "thermal zone %s has wrong trip setup for power allocator\n",
+ tz->type);
+ goto free;
+ }
+
+ ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
+ &switch_on_temp);
+ if (ret)
+ goto free;
+
+ ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature,
+ &control_temp);
+ if (ret)
+ goto free;
+
+ temperature_threshold = control_temp - switch_on_temp;
+
+ tz->tzp->k_po = tz->tzp->k_po ?:
+ int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
+ tz->tzp->k_pu = tz->tzp->k_pu ?:
+ int_to_frac(2 * tz->tzp->sustainable_power) /
+ temperature_threshold;
+ tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
+ /*
+ * The default for k_d and integral_cutoff is 0, so we can
+ * leave them as they are.
+ */
+
+ reset_pid_controller(params);
+
+ tz->governor_data = params;
+
+ return 0;
+
+free:
+ devm_kfree(&tz->device, params);
+ return ret;
+}
+
+static void power_allocator_unbind(struct thermal_zone_device *tz)
+{
+ dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
+ devm_kfree(&tz->device, tz->governor_data);
+ tz->governor_data = NULL;
+}
+
+static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
+{
+ int ret;
+ unsigned long switch_on_temp, control_temp, current_temp;
+ struct power_allocator_params *params = tz->governor_data;
+
+ /*
+ * We get called for every trip point but we only need to do
+ * our calculations once
+ */
+ if (trip != params->trip_max_desired_temperature)
+ return 0;
+
+ ret = thermal_zone_get_temp(tz, ¤t_temp);
+ if (ret) {
+ dev_warn(&tz->device, "Failed to get temperature: %d\n", ret);
+ return ret;
+ }
+
+ ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
+ &switch_on_temp);
+ if (ret) {
+ dev_warn(&tz->device,
+ "Failed to get switch on temperature: %d\n", ret);
+ return ret;
+ }
+
+ if (current_temp < switch_on_temp) {
+ tz->passive = 0;
+ reset_pid_controller(params);
+ allow_maximum_power(tz);
+ return 0;
+ }
+
+ tz->passive = 1;
+
+ ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature,
+ &control_temp);
+ if (ret) {
+ dev_warn(&tz->device,
+ "Failed to get the maximum desired temperature: %d\n",
+ ret);
+ return ret;
+ }
+
+ return allocate_power(tz, current_temp, control_temp);
+}
+
+static struct thermal_governor thermal_gov_power_allocator = {
+ .name = "power_allocator",
+ .bind_to_tz = power_allocator_bind,
+ .unbind_from_tz = power_allocator_unbind,
+ .throttle = power_allocator_throttle,
+};
+
+int thermal_gov_power_allocator_register(void)
+{
+ return thermal_register_governor(&thermal_gov_power_allocator);
+}
+
+void thermal_gov_power_allocator_unregister(void)
+{
+ thermal_unregister_governor(&thermal_gov_power_allocator);
+}
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 82f1cca9c8ce..df9ba3bf55dc 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1617,7 +1617,7 @@ static void remove_trip_attrs(struct thermal_zone_device *tz)
struct thermal_zone_device *thermal_zone_device_register(const char *type,
int trips, int mask, void *devdata,
struct thermal_zone_device_ops *ops,
- const struct thermal_zone_params *tzp,
+ struct thermal_zone_params *tzp,
int passive_delay, int polling_delay)
{
struct thermal_zone_device *tz;
@@ -1969,7 +1969,11 @@ static int __init thermal_register_governors(void)
if (result)
return result;
- return thermal_gov_user_space_register();
+ result = thermal_gov_user_space_register();
+ if (result)
+ return result;
+
+ return thermal_gov_power_allocator_register();
}
static void thermal_unregister_governors(void)
@@ -1978,6 +1982,7 @@ static void thermal_unregister_governors(void)
thermal_gov_fair_share_unregister();
thermal_gov_bang_bang_unregister();
thermal_gov_user_space_unregister();
+ thermal_gov_power_allocator_unregister();
}
static int __init thermal_init(void)
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index faebe881f062..8a6624488cc5 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -88,6 +88,14 @@ static inline int thermal_gov_user_space_register(void) { return 0; }
static inline void thermal_gov_user_space_unregister(void) {}
#endif /* CONFIG_THERMAL_GOV_USER_SPACE */
+#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
+int thermal_gov_power_allocator_register(void);
+void thermal_gov_power_allocator_unregister(void);
+#else
+static inline int thermal_gov_power_allocator_register(void) { return 0; }
+static inline void thermal_gov_power_allocator_unregister(void) {}
+#endif /* CONFIG_THERMAL_GOV_POWER_ALLOCATOR */
+
/* device tree support */
#ifdef CONFIG_THERMAL_OF
int of_parse_thermal_zones(void);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index d69b1806f430..f8f503cd50a6 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -59,6 +59,8 @@
#define DEFAULT_THERMAL_GOVERNOR "fair_share"
#elif defined(CONFIG_THERMAL_DEFAULT_GOV_USER_SPACE)
#define DEFAULT_THERMAL_GOVERNOR "user_space"
+#elif defined(CONFIG_THERMAL_DEFAULT_GOV_POWER_ALLOCATOR)
+#define DEFAULT_THERMAL_GOVERNOR "power_allocator"
#endif
struct thermal_zone_device;
@@ -154,8 +156,7 @@ struct thermal_attr {
* @devdata: private pointer for device private data
* @trips: number of trip points the thermal zone supports
* @passive_delay: number of milliseconds to wait between polls when
- * performing passive cooling. Currenty only used by the
- * step-wise governor
+ * performing passive cooling.
* @polling_delay: number of milliseconds to wait between polls when
* checking whether trip points have been crossed (0 for
* interrupt driven systems)
@@ -165,7 +166,6 @@ struct thermal_attr {
* @last_temperature: previous temperature read
* @emul_temperature: emulated temperature when using CONFIG_THERMAL_EMULATION
* @passive: 1 if you've crossed a passive trip point, 0 otherwise.
- * Currenty only used by the step-wise governor.
* @forced_passive: If > 0, temperature at which to switch on all ACPI
* processor cooling devices. Currently only used by the
* step-wise governor.
@@ -197,7 +197,7 @@ struct thermal_zone_device {
int passive;
unsigned int forced_passive;
struct thermal_zone_device_ops *ops;
- const struct thermal_zone_params *tzp;
+ struct thermal_zone_params *tzp;
struct thermal_governor *governor;
void *governor_data;
struct list_head thermal_instances;
@@ -275,6 +275,33 @@ struct thermal_zone_params {
int num_tbps; /* Number of tbp entries */
struct thermal_bind_params *tbp;
+
+ /*
+ * Sustainable power (heat) that this thermal zone can dissipate in
+ * mW
+ */
+ u32 sustainable_power;
+
+ /*
+ * Proportional parameter of the PID controller when
+ * overshooting (i.e., when temperature is below the target)
+ */
+ s32 k_po;
+
+ /*
+ * Proportional parameter of the PID controller when
+ * undershooting
+ */
+ s32 k_pu;
+
+ /* Integral parameter of the PID controller */
+ s32 k_i;
+
+ /* Derivative parameter of the PID controller */
+ s32 k_d;
+
+ /* threshold below which the error is no longer accumulated */
+ s32 integral_cutoff;
};
struct thermal_genl_event {
@@ -349,7 +376,7 @@ int power_actor_set_power(struct thermal_cooling_device *,
struct thermal_instance *, u32);
struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
void *, struct thermal_zone_device_ops *,
- const struct thermal_zone_params *, int, int);
+ struct thermal_zone_params *, int, int);
void thermal_zone_device_unregister(struct thermal_zone_device *);
int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 2/5] thermal: add trace events to the power allocator governor
2015-03-02 17:17 [PATCH v3 0/5] Subject: The power allocator thermal governor Javi Merino
2015-03-02 17:17 ` [PATCH v3 1/5] thermal: introduce the Power Allocator governor Javi Merino
@ 2015-03-02 17:17 ` Javi Merino
2017-03-15 4:26 ` Viresh Kumar
2015-03-02 17:17 ` [PATCH v3 3/5] thermal: export thermal_zone_parameters to sysfs Javi Merino
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Javi Merino @ 2015-03-02 17:17 UTC (permalink / raw)
To: edubezval, rui.zhang
Cc: linux-pm, linux-kernel, punit.agrawal, lina.iyer, broonie, tixy,
Javi Merino, Frederic Weisbecker, Ingo Molnar
Add trace events for the power allocator governor and the power actor
interface of the cpu cooling device.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
drivers/thermal/cpu_cooling.c | 31 ++++++++-
drivers/thermal/power_allocator.c | 22 ++++++-
include/trace/events/thermal.h | 58 +++++++++++++++++
include/trace/events/thermal_power_allocator.h | 87 ++++++++++++++++++++++++++
4 files changed, 194 insertions(+), 4 deletions(-)
create mode 100644 include/trace/events/thermal_power_allocator.h
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ba23150c7bde..c4974144c787 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -31,6 +31,8 @@
#include <linux/cpu.h>
#include <linux/cpu_cooling.h>
+#include <trace/events/thermal.h>
+
/*
* Cooling state <-> CPUFreq frequency
*
@@ -588,12 +590,20 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
u32 *power)
{
unsigned long freq;
- int cpu, ret;
+ int i = 0, cpu, ret;
u32 static_power, dynamic_power, total_load = 0;
struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
+ u32 *load_cpu = NULL;
freq = cpufreq_quick_get(cpumask_any(&cpufreq_device->allowed_cpus));
+ if (trace_thermal_power_cpu_get_power_enabled()) {
+ u32 ncpus = cpumask_weight(&cpufreq_device->allowed_cpus);
+
+ load_cpu = devm_kcalloc(&cdev->device, ncpus, sizeof(*load_cpu),
+ GFP_KERNEL);
+ }
+
for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {
u32 load;
@@ -603,14 +613,29 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
load = 0;
total_load += load;
+ if (trace_thermal_power_cpu_limit_enabled() && load_cpu)
+ load_cpu[i] = load;
+
+ i++;
}
cpufreq_device->last_load = total_load;
dynamic_power = get_dynamic_power(cpufreq_device, freq);
ret = get_static_power(cpufreq_device, tz, freq, &static_power);
- if (ret)
+ if (ret) {
+ if (load_cpu)
+ devm_kfree(&cdev->device, load_cpu);
return ret;
+ }
+
+ if (load_cpu) {
+ trace_thermal_power_cpu_get_power(
+ &cpufreq_device->allowed_cpus,
+ freq, load_cpu, i, dynamic_power, static_power);
+
+ devm_kfree(&cdev->device, load_cpu);
+ }
*power = static_power + dynamic_power;
return 0;
@@ -718,6 +743,8 @@ static int cpufreq_power2state(struct thermal_cooling_device *cdev,
return -EINVAL;
}
+ trace_thermal_power_cpu_limit(&cpufreq_device->allowed_cpus,
+ target_freq, *state, power);
return 0;
}
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 67982d79b76c..3ca7530deac6 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -19,6 +19,9 @@
#include <linux/slab.h>
#include <linux/thermal.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/thermal_power_allocator.h>
+
#include "thermal_core.h"
#define FRAC_BITS 10
@@ -138,7 +141,14 @@ static u32 pid_controller(struct thermal_zone_device *tz,
/* feed-forward the known sustainable dissipatable power */
power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
- return clamp(power_range, (s64)0, (s64)max_allocatable_power);
+ power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
+
+ trace_thermal_power_allocator_pid(tz, frac_to_int(err),
+ frac_to_int(params->err_integral),
+ frac_to_int(p), frac_to_int(i),
+ frac_to_int(d), power_range);
+
+ return power_range;
}
/**
@@ -219,7 +229,7 @@ static int allocate_power(struct thermal_zone_device *tz,
struct power_allocator_params *params = tz->governor_data;
u32 *req_power, *max_power, *granted_power, *extra_actor_power;
u32 total_req_power, max_allocatable_power;
- u32 power_range;
+ u32 total_granted_power, power_range;
int i, num_actors, total_weight, ret = 0;
int trip_max_desired_temperature = params->trip_max_desired_temperature;
@@ -295,6 +305,7 @@ static int allocate_power(struct thermal_zone_device *tz,
divvy_up_power(req_power, max_power, num_actors, total_req_power,
power_range, granted_power, extra_actor_power);
+ total_granted_power = 0;
i = 0;
list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
if (instance->trip != trip_max_desired_temperature)
@@ -305,10 +316,17 @@ static int allocate_power(struct thermal_zone_device *tz,
power_actor_set_power(instance->cdev, instance,
granted_power[i]);
+ total_granted_power += granted_power[i];
i++;
}
+ trace_thermal_power_allocator(tz, req_power, total_req_power,
+ granted_power, total_granted_power,
+ num_actors, power_range,
+ max_allocatable_power, current_temp,
+ (s32)control_temp - (s32)current_temp);
+
devm_kfree(&tz->device, req_power);
unlock:
mutex_unlock(&tz->lock);
diff --git a/include/trace/events/thermal.h b/include/trace/events/thermal.h
index 0f4f95d63c03..8b1f80682b80 100644
--- a/include/trace/events/thermal.h
+++ b/include/trace/events/thermal.h
@@ -77,6 +77,64 @@ TRACE_EVENT(thermal_zone_trip,
__entry->trip_type)
);
+TRACE_EVENT(thermal_power_cpu_get_power,
+ TP_PROTO(const struct cpumask *cpus, unsigned long freq, u32 *load,
+ size_t load_len, u32 dynamic_power, u32 static_power),
+
+ TP_ARGS(cpus, freq, load, load_len, dynamic_power, static_power),
+
+ TP_STRUCT__entry(
+ __bitmask(cpumask, num_possible_cpus())
+ __field(unsigned long, freq )
+ __dynamic_array(u32, load, load_len)
+ __field(size_t, load_len )
+ __field(u32, dynamic_power )
+ __field(u32, static_power )
+ ),
+
+ TP_fast_assign(
+ __assign_bitmask(cpumask, cpumask_bits(cpus),
+ num_possible_cpus());
+ __entry->freq = freq;
+ memcpy(__get_dynamic_array(load), load,
+ load_len * sizeof(*load));
+ __entry->load_len = load_len;
+ __entry->dynamic_power = dynamic_power;
+ __entry->static_power = static_power;
+ ),
+
+ TP_printk("cpus=%s freq=%lu load={%s} dynamic_power=%d static_power=%d",
+ __get_bitmask(cpumask), __entry->freq,
+ __print_array(__get_dynamic_array(load), __entry->load_len, 4),
+ __entry->dynamic_power, __entry->static_power)
+);
+
+TRACE_EVENT(thermal_power_cpu_limit,
+ TP_PROTO(const struct cpumask *cpus, unsigned int freq,
+ unsigned long cdev_state, u32 power),
+
+ TP_ARGS(cpus, freq, cdev_state, power),
+
+ TP_STRUCT__entry(
+ __bitmask(cpumask, num_possible_cpus())
+ __field(unsigned int, freq )
+ __field(unsigned long, cdev_state)
+ __field(u32, power )
+ ),
+
+ TP_fast_assign(
+ __assign_bitmask(cpumask, cpumask_bits(cpus),
+ num_possible_cpus());
+ __entry->freq = freq;
+ __entry->cdev_state = cdev_state;
+ __entry->power = power;
+ ),
+
+ TP_printk("cpus=%s freq=%u cdev_state=%lu power=%u",
+ __get_bitmask(cpumask), __entry->freq, __entry->cdev_state,
+ __entry->power)
+);
+
#endif /* _TRACE_THERMAL_H */
/* This part must be outside protection */
diff --git a/include/trace/events/thermal_power_allocator.h b/include/trace/events/thermal_power_allocator.h
new file mode 100644
index 000000000000..12e1321c4e0c
--- /dev/null
+++ b/include/trace/events/thermal_power_allocator.h
@@ -0,0 +1,87 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM thermal_power_allocator
+
+#if !defined(_TRACE_THERMAL_POWER_ALLOCATOR_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_THERMAL_POWER_ALLOCATOR_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(thermal_power_allocator,
+ TP_PROTO(struct thermal_zone_device *tz, u32 *req_power,
+ u32 total_req_power, u32 *granted_power,
+ u32 total_granted_power, size_t num_actors,
+ u32 power_range, u32 max_allocatable_power,
+ unsigned long current_temp, s32 delta_temp),
+ TP_ARGS(tz, req_power, total_req_power, granted_power,
+ total_granted_power, num_actors, power_range,
+ max_allocatable_power, current_temp, delta_temp),
+ TP_STRUCT__entry(
+ __field(int, tz_id )
+ __dynamic_array(u32, req_power, num_actors )
+ __field(u32, total_req_power )
+ __dynamic_array(u32, granted_power, num_actors)
+ __field(u32, total_granted_power )
+ __field(size_t, num_actors )
+ __field(u32, power_range )
+ __field(u32, max_allocatable_power )
+ __field(unsigned long, current_temp )
+ __field(s32, delta_temp )
+ ),
+ TP_fast_assign(
+ __entry->tz_id = tz->id;
+ memcpy(__get_dynamic_array(req_power), req_power,
+ num_actors * sizeof(*req_power));
+ __entry->total_req_power = total_req_power;
+ memcpy(__get_dynamic_array(granted_power), granted_power,
+ num_actors * sizeof(*granted_power));
+ __entry->total_granted_power = total_granted_power;
+ __entry->num_actors = num_actors;
+ __entry->power_range = power_range;
+ __entry->max_allocatable_power = max_allocatable_power;
+ __entry->current_temp = current_temp;
+ __entry->delta_temp = delta_temp;
+ ),
+
+ TP_printk("thermal_zone_id=%d req_power={%s} total_req_power=%u granted_power={%s} total_granted_power=%u power_range=%u max_allocatable_power=%u current_temperature=%lu delta_temperature=%d",
+ __entry->tz_id,
+ __print_array(__get_dynamic_array(req_power),
+ __entry->num_actors, 4),
+ __entry->total_req_power,
+ __print_array(__get_dynamic_array(granted_power),
+ __entry->num_actors, 4),
+ __entry->total_granted_power, __entry->power_range,
+ __entry->max_allocatable_power, __entry->current_temp,
+ __entry->delta_temp)
+);
+
+TRACE_EVENT(thermal_power_allocator_pid,
+ TP_PROTO(struct thermal_zone_device *tz, s32 err, s32 err_integral,
+ s64 p, s64 i, s64 d, s32 output),
+ TP_ARGS(tz, err, err_integral, p, i, d, output),
+ TP_STRUCT__entry(
+ __field(int, tz_id )
+ __field(s32, err )
+ __field(s32, err_integral)
+ __field(s64, p )
+ __field(s64, i )
+ __field(s64, d )
+ __field(s32, output )
+ ),
+ TP_fast_assign(
+ __entry->tz_id = tz->id;
+ __entry->err = err;
+ __entry->err_integral = err_integral;
+ __entry->p = p;
+ __entry->i = i;
+ __entry->d = d;
+ __entry->output = output;
+ ),
+
+ TP_printk("thermal_zone_id=%d err=%d err_integral=%d p=%lld i=%lld d=%lld output=%d",
+ __entry->tz_id, __entry->err, __entry->err_integral,
+ __entry->p, __entry->i, __entry->d, __entry->output)
+);
+#endif /* _TRACE_THERMAL_POWER_ALLOCATOR_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 2/5] thermal: add trace events to the power allocator governor
2015-03-02 17:17 ` [PATCH v3 2/5] thermal: add trace events to the power allocator governor Javi Merino
@ 2017-03-15 4:26 ` Viresh Kumar
0 siblings, 0 replies; 21+ messages in thread
From: Viresh Kumar @ 2017-03-15 4:26 UTC (permalink / raw)
To: Javi Merino
Cc: Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst, Frederic Weisbecker, Ingo Molnar
Hi Javi,
Sorry for picking up an old thread, but i had a question for you.
On Mon, Mar 2, 2015 at 10:47 PM, Javi Merino <javi.merino@arm.com> wrote:
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> @@ -588,12 +590,20 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
> u32 *power)
> {
> unsigned long freq;
> - int cpu, ret;
> + int i = 0, cpu, ret;
> u32 static_power, dynamic_power, total_load = 0;
> struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
> + u32 *load_cpu = NULL;
>
> freq = cpufreq_quick_get(cpumask_any(&cpufreq_device->allowed_cpus));
>
> + if (trace_thermal_power_cpu_get_power_enabled()) {
You allocated load_cpu if cpu_get_power trace is enabled.
> + u32 ncpus = cpumask_weight(&cpufreq_device->allowed_cpus);
> +
> + load_cpu = devm_kcalloc(&cdev->device, ncpus, sizeof(*load_cpu),
> + GFP_KERNEL);
> + }
> +
> for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {
> u32 load;
>
> @@ -603,14 +613,29 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
> load = 0;
>
> total_load += load;
> + if (trace_thermal_power_cpu_limit_enabled() && load_cpu)
But you store values to it only if cpu_limit trace is also enabled,
otherwise it is all zeros.
Why?
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] thermal: export thermal_zone_parameters to sysfs
2015-03-02 17:17 [PATCH v3 0/5] Subject: The power allocator thermal governor Javi Merino
2015-03-02 17:17 ` [PATCH v3 1/5] thermal: introduce the Power Allocator governor Javi Merino
2015-03-02 17:17 ` [PATCH v3 2/5] thermal: add trace events to the power allocator governor Javi Merino
@ 2015-03-02 17:17 ` Javi Merino
2015-03-02 17:17 ` [PATCH v3 4/5] Revert "cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications" Javi Merino
` (2 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Javi Merino @ 2015-03-02 17:17 UTC (permalink / raw)
To: edubezval, rui.zhang
Cc: linux-pm, linux-kernel, punit.agrawal, lina.iyer, broonie, tixy,
Javi Merino
It's useful for tuning to be able to edit thermal_zone_parameters from
userspace. Export them to the thermal_zone sysfs so that they can be
easily changed.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
Documentation/thermal/sysfs-api.txt | 52 +++++++++++++++++++
drivers/thermal/thermal_core.c | 101 ++++++++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+)
diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index fc7dfe10778b..7d44d7f1a71b 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -184,6 +184,12 @@ Thermal zone device sys I/F, created once it's registered:
|---trip_point_[0-*]_type: Trip point type
|---trip_point_[0-*]_hyst: Hysteresis value for this trip point
|---emul_temp: Emulated temperature set node
+ |---sustainable_power: Sustainable dissipatable power
+ |---k_po: Proportional term during temperature overshoot
+ |---k_pu: Proportional term during temperature undershoot
+ |---k_i: PID's integral term in the power allocator gov
+ |---k_d: PID's derivative term in the power allocator
+ |---integral_cutoff: Offset above which errors are accumulated
Thermal cooling device sys I/F, created once it's registered:
/sys/class/thermal/cooling_device[0-*]:
@@ -307,6 +313,52 @@ emul_temp
because userland can easily disable the thermal policy by simply
flooding this sysfs node with low temperature values.
+sustainable_power
+ An estimate of the sustained power that can be dissipated by
+ the thermal zone. Used by the power allocator governor. For
+ more information see Documentation/thermal/power_allocator.txt
+ Unit: milliwatts
+ RW, Optional
+
+k_po
+ The proportional term of the power allocator governor's PID
+ controller during temperature overshoot. Temperature overshoot
+ is when the current temperature is above the "desired
+ temperature" trip point. For more information see
+ Documentation/thermal/power_allocator.txt
+ RW, Optional
+
+k_pu
+ The proportional term of the power allocator governor's PID
+ controller during temperature undershoot. Temperature undershoot
+ is when the current temperature is below the "desired
+ temperature" trip point. For more information see
+ Documentation/thermal/power_allocator.txt
+ RW, Optional
+
+k_i
+ The integral term of the power allocator governor's PID
+ controller. This term allows the PID controller to compensate
+ for long term drift. For more information see
+ Documentation/thermal/power_allocator.txt
+ RW, Optional
+
+k_d
+ The derivative term of the power allocator governor's PID
+ controller. For more information see
+ Documentation/thermal/power_allocator.txt
+ RW, Optional
+
+integral_cutoff
+ Temperature offset from the desired temperature trip point
+ above which the integral term of the power allocator
+ governor's PID controller starts accumulating errors. For
+ example, if integral_cutoff is 0, then the integral term only
+ accumulates error when temperature is above the desired
+ temperature trip point. For more information see
+ Documentation/thermal/power_allocator.txt
+ RW, Optional
+
*****************************
* Cooling device attributes *
*****************************
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index df9ba3bf55dc..38ce70cd0b08 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -873,6 +873,102 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
#endif/*CONFIG_THERMAL_EMULATION*/
+static ssize_t
+sustainable_power_show(struct device *dev, struct device_attribute *devattr,
+ char *buf)
+{
+ struct thermal_zone_device *tz = to_thermal_zone(dev);
+
+ if (tz->tzp)
+ return sprintf(buf, "%u\n", tz->tzp->sustainable_power);
+ else
+ return -EIO;
+}
+
+static ssize_t
+sustainable_power_store(struct device *dev, struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct thermal_zone_device *tz = to_thermal_zone(dev);
+ u32 sustainable_power;
+
+ if (!tz->tzp)
+ return -EIO;
+
+ if (kstrtou32(buf, 10, &sustainable_power))
+ return -EINVAL;
+
+ tz->tzp->sustainable_power = sustainable_power;
+
+ return count;
+}
+static DEVICE_ATTR(sustainable_power, S_IWUSR | S_IRUGO, sustainable_power_show,
+ sustainable_power_store);
+
+#define create_s32_tzp_attr(name) \
+ static ssize_t \
+ name##_show(struct device *dev, struct device_attribute *devattr, \
+ char *buf) \
+ { \
+ struct thermal_zone_device *tz = to_thermal_zone(dev); \
+ \
+ if (tz->tzp) \
+ return sprintf(buf, "%u\n", tz->tzp->name); \
+ else \
+ return -EIO; \
+ } \
+ \
+ static ssize_t \
+ name##_store(struct device *dev, struct device_attribute *devattr, \
+ const char *buf, size_t count) \
+ { \
+ struct thermal_zone_device *tz = to_thermal_zone(dev); \
+ s32 value; \
+ \
+ if (!tz->tzp) \
+ return -EIO; \
+ \
+ if (kstrtos32(buf, 10, &value)) \
+ return -EINVAL; \
+ \
+ tz->tzp->name = value; \
+ \
+ return count; \
+ } \
+ static DEVICE_ATTR(name, S_IWUSR | S_IRUGO, name##_show, name##_store)
+
+create_s32_tzp_attr(k_po);
+create_s32_tzp_attr(k_pu);
+create_s32_tzp_attr(k_i);
+create_s32_tzp_attr(k_d);
+create_s32_tzp_attr(integral_cutoff);
+#undef create_s32_tzp_attr
+
+static struct device_attribute *dev_tzp_attrs[] = {
+ &dev_attr_sustainable_power,
+ &dev_attr_k_po,
+ &dev_attr_k_pu,
+ &dev_attr_k_i,
+ &dev_attr_k_d,
+ &dev_attr_integral_cutoff,
+};
+
+static int create_tzp_attrs(struct device *dev)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(dev_tzp_attrs); i++) {
+ int ret;
+ struct device_attribute *dev_attr = dev_tzp_attrs[i];
+
+ ret = device_create_file(dev, dev_attr);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/**
* power_actor_get_max_power() - get the maximum power that a cdev can consume
* @cdev: pointer to &thermal_cooling_device
@@ -1712,6 +1808,11 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
if (result)
goto unregister;
+ /* Add thermal zone params */
+ result = create_tzp_attrs(&tz->device);
+ if (result)
+ goto unregister;
+
/* Update 'this' zone's governor information */
mutex_lock(&thermal_governor_lock);
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 4/5] Revert "cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications"
2015-03-02 17:17 [PATCH v3 0/5] Subject: The power allocator thermal governor Javi Merino
` (2 preceding siblings ...)
2015-03-02 17:17 ` [PATCH v3 3/5] thermal: export thermal_zone_parameters to sysfs Javi Merino
@ 2015-03-02 17:17 ` Javi Merino
2015-03-02 17:17 ` [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu Javi Merino
2015-03-02 17:28 ` [PATCH v3 0/5] Subject: The power allocator thermal governor Eduardo Valentin
5 siblings, 0 replies; 21+ messages in thread
From: Javi Merino @ 2015-03-02 17:17 UTC (permalink / raw)
To: edubezval, rui.zhang
Cc: linux-pm, linux-kernel, punit.agrawal, lina.iyer, broonie, tixy,
Javi Merino, Viresh Kumar, Rafael J. Wysocki
This reverts commit d9f354460db8 ("cpufreq: remove
CPUFREQ_UPDATE_POLICY_CPU notifications"). When the lead cpu of a group
of cpus managed by cpufreq is hotplugged out, the cpu device cached in
drivers/thermal/cpu_cooling.c needs to be updated accordingly.
Bring back the CPUFREQ_UPDATE_POLICY_CPU notifier so that we can react
to it.
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
Hi Viresh,
We just noticed that you removed this from v4.0-rc1 when we were
planning on using it, that's why are reverting the patch. The user is
introduced in the next patch: "[PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu".
drivers/cpufreq/cpufreq.c | 3 +++
include/linux/cpufreq.h | 5 +++--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 28e59a48b35f..215147050c3d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1095,6 +1095,9 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
policy->cpu = cpu;
up_write(&policy->rwsem);
+ blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+ CPUFREQ_UPDATE_POLICY_CPU, policy);
+
return 0;
}
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 2ee4888c1f47..7e1a389b4e92 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -368,8 +368,9 @@ static inline void cpufreq_resume(void) {}
#define CPUFREQ_INCOMPATIBLE (1)
#define CPUFREQ_NOTIFY (2)
#define CPUFREQ_START (3)
-#define CPUFREQ_CREATE_POLICY (4)
-#define CPUFREQ_REMOVE_POLICY (5)
+#define CPUFREQ_UPDATE_POLICY_CPU (4)
+#define CPUFREQ_CREATE_POLICY (5)
+#define CPUFREQ_REMOVE_POLICY (6)
#ifdef CONFIG_CPU_FREQ
int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-02 17:17 [PATCH v3 0/5] Subject: The power allocator thermal governor Javi Merino
` (3 preceding siblings ...)
2015-03-02 17:17 ` [PATCH v3 4/5] Revert "cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications" Javi Merino
@ 2015-03-02 17:17 ` Javi Merino
2015-03-03 4:03 ` Viresh Kumar
2015-03-02 17:28 ` [PATCH v3 0/5] Subject: The power allocator thermal governor Eduardo Valentin
5 siblings, 1 reply; 21+ messages in thread
From: Javi Merino @ 2015-03-02 17:17 UTC (permalink / raw)
To: edubezval, rui.zhang
Cc: linux-pm, linux-kernel, punit.agrawal, lina.iyer, broonie, tixy,
Kapileshwar Singh
From: Kapileshwar Singh <kapileshwar.singh@arm.com>
When cpufreq changes the policy cpu, we need to update our cached cpu
device accordingly.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
---
drivers/thermal/cpu_cooling.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index c4974144c787..e306d6bc3cf1 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -269,6 +269,9 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
mutex_unlock(&cooling_cpufreq_lock);
break;
+ case CPUFREQ_UPDATE_POLICY_CPU:
+ update_cpu_device(policy->cpu);
+ break;
case CPUFREQ_CREATE_POLICY:
update_cpu_device(policy->cpu);
break;
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-02 17:17 ` [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu Javi Merino
@ 2015-03-03 4:03 ` Viresh Kumar
2015-03-03 10:59 ` Kapileshwar Singh
0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2015-03-03 4:03 UTC (permalink / raw)
To: Javi Merino
Cc: Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, punit.agrawal, Lina Iyer,
Mark Brown, Jon Medhurst, Kapileshwar Singh
On Mon, Mar 2, 2015 at 10:47 PM, Javi Merino <javi.merino@arm.com> wrote:
> From: Kapileshwar Singh <kapileshwar.singh@arm.com>
>
> When cpufreq changes the policy cpu, we need to update our cached cpu
> device accordingly.
>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
> ---
> drivers/thermal/cpu_cooling.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index c4974144c787..e306d6bc3cf1 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -269,6 +269,9 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
> mutex_unlock(&cooling_cpufreq_lock);
> break;
>
> + case CPUFREQ_UPDATE_POLICY_CPU:
> + update_cpu_device(policy->cpu);
> + break;
> case CPUFREQ_CREATE_POLICY:
> update_cpu_device(policy->cpu);
> break;
First of all, I wasn't able to find 3/5 on LKML and I looked at 3/7
from an earlier
version to look at how update_cpu_device() looks like.
What I couldn't understand is why do you need to update things if policy->cpu
is changing ?
I am expecting a detailed answer here according to your design, and we may
be able to work out without such updates. Lets see..
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 4:03 ` Viresh Kumar
@ 2015-03-03 10:59 ` Kapileshwar Singh
2015-03-03 11:19 ` Viresh Kumar
0 siblings, 1 reply; 21+ messages in thread
From: Kapileshwar Singh @ 2015-03-03 10:59 UTC (permalink / raw)
To: Viresh Kumar, Javi Merino
Cc: Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
Hi Viresh!
On 03/03/15 04:03, Viresh Kumar wrote:
> On Mon, Mar 2, 2015 at 10:47 PM, Javi Merino <javi.merino@arm.com> wrote:
>> From: Kapileshwar Singh <kapileshwar.singh@arm.com>
>>
>> When cpufreq changes the policy cpu, we need to update our cached cpu
>> device accordingly.
>>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Signed-off-by: Kapileshwar Singh <kapileshwar.singh@arm.com>
>> ---
>> drivers/thermal/cpu_cooling.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> index c4974144c787..e306d6bc3cf1 100644
>> --- a/drivers/thermal/cpu_cooling.c
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -269,6 +269,9 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
>> mutex_unlock(&cooling_cpufreq_lock);
>> break;
>>
>> + case CPUFREQ_UPDATE_POLICY_CPU:
>> + update_cpu_device(policy->cpu);
>> + break;
>> case CPUFREQ_CREATE_POLICY:
>> update_cpu_device(policy->cpu);
>> break;
>
> First of all, I wasn't able to find 3/5 on LKML and I looked at 3/7
> from an earlier
> version to look at how update_cpu_device() looks like.
>
> What I couldn't understand is why do you need to update things if policy->cpu
> is changing ?
>
We store the device pointer of the lead CPU (policy CPU) in a cpufreq domain as a part of the
cpufreq_cooling_device data structure. There is one cpufreq_cooling_device per
cpufreq domain.
We need the device to find out the current OPP for the cpufreq_cooling_device for our static power calculation.
opp = opp_find_freq_exact(cpu_dev, freq_hz, true);
voltage = dev_pm_opp_get_voltage(opp);
The problem we are trying to solve here is:
When this lead CPU gets hotplugged out, the device pointer becomes stale and the policy
cpu for the cpufreq domain changes. We then store the new policy CPU's device pointer for the
in cpufreq_cooling_device on the reception of a notification from cpufreq. Being open to your
suggestions for any other possible ways to solve the problem..
Regards,
KP
> I am expecting a detailed answer here according to your design, and we may
> be able to work out without such updates. Lets see..
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 10:59 ` Kapileshwar Singh
@ 2015-03-03 11:19 ` Viresh Kumar
2015-03-03 11:41 ` Kapileshwar Singh
0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2015-03-03 11:19 UTC (permalink / raw)
To: Kapileshwar Singh
Cc: Javi Merino, Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
On 3 March 2015 at 16:29, Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
> We store the device pointer of the lead CPU (policy CPU) in a cpufreq domain as a part of the
> cpufreq_cooling_device data structure. There is one cpufreq_cooling_device per
> cpufreq domain.
>
> We need the device to find out the current OPP for the cpufreq_cooling_device for our static power calculation.
>
> opp = opp_find_freq_exact(cpu_dev, freq_hz, true);
> voltage = dev_pm_opp_get_voltage(opp);
>
>
> The problem we are trying to solve here is:
>
> When this lead CPU gets hotplugged out, the device pointer becomes stale and the policy
> cpu for the cpufreq domain changes. We then store the new policy CPU's device pointer for the
> in cpufreq_cooling_device on the reception of a notification from cpufreq. Being open to your
> suggestions for any other possible ways to solve the problem..
I would have loved that if life was that simple :)
So, the OPP library today isn't that perfect and so is this doing rounds [1].
The problem is the OPPs are initialized per device today and even if they
are shared by multiple CPUs, OPP library doesn't know about it.
So, if the policy->cpu goes away, OPP APIs on the new CPU will not work
as OPPs are only initialized for one CPU and not for others within the same
policy :)
The way cpufreq-dt is taking care of this is by saving cpu_dev of the first
CPU for which OPPs are initialized and always using that even if the CPU
goes away. And you need to do exactly that.
And please, do test such scenario before sending the patches again. As
it would have simply failed in this case, have you given it a try ..
Once my patchset [1] is applied, life would be very simple and we can
call OPP library for any CPU, but that is going to take some time.
--
viresh
[1] https://www.marc.info/?l=linaro-kernel&m=142364262800650&w=3
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 11:19 ` Viresh Kumar
@ 2015-03-03 11:41 ` Kapileshwar Singh
2015-03-03 13:07 ` Viresh Kumar
0 siblings, 1 reply; 21+ messages in thread
From: Kapileshwar Singh @ 2015-03-03 11:41 UTC (permalink / raw)
To: Viresh Kumar
Cc: Javi Merino, Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
On 03/03/15 11:19, Viresh Kumar wrote:
> On 3 March 2015 at 16:29, Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
>> We store the device pointer of the lead CPU (policy CPU) in a cpufreq domain as a part of the
>> cpufreq_cooling_device data structure. There is one cpufreq_cooling_device per
>> cpufreq domain.
>>
>> We need the device to find out the current OPP for the cpufreq_cooling_device for our static power calculation.
>>
>> opp = opp_find_freq_exact(cpu_dev, freq_hz, true);
>> voltage = dev_pm_opp_get_voltage(opp);
>>
>>
>> The problem we are trying to solve here is:
>>
>> When this lead CPU gets hotplugged out, the device pointer becomes stale and the policy
>> cpu for the cpufreq domain changes. We then store the new policy CPU's device pointer for the
>> in cpufreq_cooling_device on the reception of a notification from cpufreq. Being open to your
>> suggestions for any other possible ways to solve the problem..
>
> I would have loved that if life was that simple :)
>
> So, the OPP library today isn't that perfect and so is this doing rounds [1].
> The problem is the OPPs are initialized per device today and even if they
> are shared by multiple CPUs, OPP library doesn't know about it.
>
> So, if the policy->cpu goes away, OPP APIs on the new CPU will not work
> as OPPs are only initialized for one CPU and not for others within the same
> policy :)
>
> The way cpufreq-dt is taking care of this is by saving cpu_dev of the first
> CPU for which OPPs are initialized and always using that even if the CPU
> goes away. And you need to do exactly that.
>
> And please, do test such scenario before sending the patches again. As
> it would have simply failed in this case, have you given it a try ..
Yes I indeed tested the case where we cache the device pointer of the CPU for which the OPP's are populated.
When this CPU is hotplugged out, it invalidates the device pointer itself. Here are the error we get in dmesg:
..
<3>[67203.216774] opp_get_voltage: Invalid parameters
<3>[67203.326774] opp_get_voltage: Invalid parameters
<3>[67203.326774] opp_get_voltage: Invalid parameters
..
Which happens because:
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
{
..
tmp_opp = rcu_dereference(opp);
if (unlikely(IS_ERR_OR_NULL(tmp_opp)) || !tmp_opp->available)
pr_err("%s: Invalid parameters\n", __func__);
else
..
Which happens when
opp = dev_pm_opp_find_freq_exact(cpufreq_device->cpu_dev, freq_hz,
true);
returns a an erroneous or NULL OPP or the opp is unavailable (in the above condition)
Regards,
KP
>
> Once my patchset [1] is applied, life would be very simple and we can
> call OPP library for any CPU, but that is going to take some time.
>
> --
> viresh
>
> [1] https://www.marc.info/?l=linaro-kernel&m=142364262800650&w=3
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 11:41 ` Kapileshwar Singh
@ 2015-03-03 13:07 ` Viresh Kumar
2015-03-03 15:09 ` Kapileshwar Singh
0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2015-03-03 13:07 UTC (permalink / raw)
To: Kapileshwar Singh
Cc: Javi Merino, Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
On 3 March 2015 at 17:11, Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
> Yes I indeed tested the case where we cache the device pointer of the CPU for which the OPP's are populated.
> When this CPU is hotplugged out, it invalidates the device pointer itself. Here are the error we get in dmesg:
What do you mean by 'invalidates the device pointer' ? that cpu_dev is NULL ?
> <3>[67203.216774] opp_get_voltage: Invalid parameters
> <3>[67203.326774] opp_get_voltage: Invalid parameters
> <3>[67203.326774] opp_get_voltage: Invalid parameters
Have you handwritten them ? Why don't they precede with dev_pm_* ??
>
> Which happens because:
>
> unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
> {
> ..
> tmp_opp = rcu_dereference(opp);
> if (unlikely(IS_ERR_OR_NULL(tmp_opp)) || !tmp_opp->available)
> pr_err("%s: Invalid parameters\n", __func__);
This %s should print routine name ..
> else
> ..
>
> Which happens when
>
> opp = dev_pm_opp_find_freq_exact(cpufreq_device->cpu_dev, freq_hz,
> true);
>
> returns a an erroneous or NULL OPP or the opp is unavailable (in the above condition)
Please goto the depth of this thing, as I don't think it should happen.
Over that I was asking you if you have tested the solution Javi gave,
because OPPs
wouldn't have been initialized for other CPUs once policy->cpu goes down.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 13:07 ` Viresh Kumar
@ 2015-03-03 15:09 ` Kapileshwar Singh
2015-03-03 15:26 ` Sudeep Holla
2015-03-03 15:29 ` Viresh Kumar
0 siblings, 2 replies; 21+ messages in thread
From: Kapileshwar Singh @ 2015-03-03 15:09 UTC (permalink / raw)
To: Viresh Kumar
Cc: Javi Merino, Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
On 03/03/15 13:07, Viresh Kumar wrote:
> On 3 March 2015 at 17:11, Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
>> Yes I indeed tested the case where we cache the device pointer of the CPU for which the OPP's are populated.
>> When this CPU is hotplugged out, it invalidates the device pointer itself. Here are the error we get in dmesg:
>
> What do you mean by 'invalidates the device pointer' ? that cpu_dev is NULL ?
The cpu_dev is not NULL but we get an erroneous OPP back. We found the problem lies in the way we calculate the frequency for the cluster.
>> <3>[67203.216774] opp_get_voltage: Invalid parameters
>> <3>[67203.326774] opp_get_voltage: Invalid parameters
>> <3>[67203.326774] opp_get_voltage: Invalid parameters
>
> Have you handwritten them ? Why don't they precede with dev_pm_* ??
I have not handwritten them, It was from a Linaro 3.10 based kernel when I first noticed this issue but the same problem exists in mainline.
Apologies for this I sent you an older trace which I had saved when I found the bug. Here is the trace I get from mainline
[ 5680.135339] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.245528] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.355432] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.465521] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.575599] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.685817] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.795556] dev_pm_opp_get_voltage: Invalid parameters
[ 5680.905598] dev_pm_opp_get_voltage: Invalid parameters
>
>>
>> Which happens because:
>>
>> unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>> {
>> ..
>> tmp_opp = rcu_dereference(opp);
>> if (unlikely(IS_ERR_OR_NULL(tmp_opp)) || !tmp_opp->available)
>> pr_err("%s: Invalid parameters\n", __func__);
>
> This %s should print routine name ..
>
>> else
>> ..
>>
>> Which happens when
>>
>> opp = dev_pm_opp_find_freq_exact(cpufreq_device->cpu_dev, freq_hz,
>> true);
>>
>> returns a an erroneous or NULL OPP or the opp is unavailable (in the above condition)
>
Update: This returns an erroneous OPP
> Please goto the depth of this thing, as I don't think it should happen.
>
> Over that I was asking you if you have tested the solution Javi gave,
> because OPPs
> wouldn't have been initialized for other CPUs once policy->cpu goes down.
I did test this but we were working with the assumption that OPPs should be populated for all the CPUs and also that OPPs are lost for a hotplugged CPU which I see is not the case.
We have looked at this more closely and found that problem lies in:
freq = cpufreq_quick_get(cpumask_any(&cpufreq_device->allowed_cpus));
which returns a NULL frequency as we are not checking for online CPUs here. We shall come up with a fix for this. Many thanks for helping us with the investigation.
Regards,
KP
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 15:09 ` Kapileshwar Singh
@ 2015-03-03 15:26 ` Sudeep Holla
2015-03-03 15:30 ` Viresh Kumar
2015-03-03 15:29 ` Viresh Kumar
1 sibling, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2015-03-03 15:26 UTC (permalink / raw)
To: Kapileshwar Singh
Cc: Viresh Kumar, Javi Merino, Eduardo Valentin, Zhang Rui,
Linux PM list, linux-kernel@vger.kernel.org, Punit Agrawal,
Lina Iyer, Mark Brown, Jon Medhurst, Sudeep Holla
On Tue, Mar 3, 2015 at 3:09 PM, Kapileshwar Singh
<kapileshwar.singh@arm.com> wrote:
> On 03/03/15 13:07, Viresh Kumar wrote:
[...]
>> Please goto the depth of this thing, as I don't think it should happen.
>>
>> Over that I was asking you if you have tested the solution Javi gave,
>> because OPPs
>> wouldn't have been initialized for other CPUs once policy->cpu goes down.
> I did test this but we were working with the assumption that OPPs should be populated for all the CPUs and also that OPPs are lost for a hotplugged CPU which I see is not the case.
>
> We have looked at this more closely and found that problem lies in:
>
> freq = cpufreq_quick_get(cpumask_any(&cpufreq_device->allowed_cpus));
>
> which returns a NULL frequency as we are not checking for online CPUs here. We shall come up with a fix for this. Many thanks for helping us with the investigation.
>
You can use any_online_cpu(..) instead of cpumask_any IMO
Regards,
Sudeep
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 15:26 ` Sudeep Holla
@ 2015-03-03 15:30 ` Viresh Kumar
2015-03-03 15:33 ` Sudeep Holla
0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2015-03-03 15:30 UTC (permalink / raw)
To: Sudeep Holla
Cc: Kapileshwar Singh, Javi Merino, Eduardo Valentin, Zhang Rui,
Linux PM list, linux-kernel@vger.kernel.org, Punit Agrawal,
Lina Iyer, Mark Brown, Jon Medhurst
On 3 March 2015 at 20:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> You can use any_online_cpu(..) instead of cpumask_any IMO
What about policy->cpu ? :)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 15:30 ` Viresh Kumar
@ 2015-03-03 15:33 ` Sudeep Holla
0 siblings, 0 replies; 21+ messages in thread
From: Sudeep Holla @ 2015-03-03 15:33 UTC (permalink / raw)
To: Viresh Kumar
Cc: Sudeep Holla, Kapileshwar Singh, Javi Merino, Eduardo Valentin,
Zhang Rui, Linux PM list, linux-kernel@vger.kernel.org,
Punit Agrawal, Lina Iyer, Mark Brown, Jon Medhurst
On 03/03/15 15:30, Viresh Kumar wrote:
> On 3 March 2015 at 20:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> You can use any_online_cpu(..) instead of cpumask_any IMO
>
> What about policy->cpu ? :)
>
We can, was not sure if they have access to policy in their thermal
driver. Just the first thought seeing one line of the code posted :)
Regards,
Sudeep
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 15:09 ` Kapileshwar Singh
2015-03-03 15:26 ` Sudeep Holla
@ 2015-03-03 15:29 ` Viresh Kumar
2015-03-03 15:34 ` Kapileshwar Singh
1 sibling, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2015-03-03 15:29 UTC (permalink / raw)
To: Kapileshwar Singh
Cc: Javi Merino, Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
On 3 March 2015 at 20:39, Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
> I did test this but we were working with the assumption that OPPs should be populated for all the CPUs and also that OPPs are lost for a hotplugged CPU which I see is not the case.
Then what did you test? My point here is, even with the latest patches
that you have
sent, you wouldn't be able to get the OPPs once policy->cpu goes down. So, how
did this worked for you ?
> We have looked at this more closely and found that problem lies in:
>
> freq = cpufreq_quick_get(cpumask_any(&cpufreq_device->allowed_cpus));
>
> which returns a NULL frequency as we are not checking for online CPUs here. We shall come up with a fix for this. Many thanks for helping us with the investigation.
Right.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu
2015-03-03 15:29 ` Viresh Kumar
@ 2015-03-03 15:34 ` Kapileshwar Singh
0 siblings, 0 replies; 21+ messages in thread
From: Kapileshwar Singh @ 2015-03-03 15:34 UTC (permalink / raw)
To: Viresh Kumar
Cc: Javi Merino, Eduardo Valentin, Zhang Rui, Linux PM list,
linux-kernel@vger.kernel.org, Punit Agrawal, Lina Iyer,
Mark Brown, Jon Medhurst
On 03/03/15 15:29, Viresh Kumar wrote:
> On 3 March 2015 at 20:39, Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
>
>> I did test this but we were working with the assumption that OPPs should be populated for all the CPUs and also that OPPs are lost for a hotplugged CPU which I see is not the case.
>
> Then what did you test? My point here is, even with the latest patches
> that you have
> sent, you wouldn't be able to get the OPPs once policy->cpu goes down. So, how
> did this worked for you ?
We were basing our fix on possibility of having OPPs for all the CPUs and we incorrectly attributed the erroneous OPP we got from dev_pm_opp_find_freq_exact to the missing OPPs in the other CPUs.
>
>> We have looked at this more closely and found that problem lies in:
>>
>> freq = cpufreq_quick_get(cpumask_any(&cpufreq_device->allowed_cpus));
>>
>> which returns a NULL frequency as we are not checking for online CPUs here. We shall come up with a fix for this. Many thanks for helping us with the investigation.
>
> Right.
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/5] Subject: The power allocator thermal governor
2015-03-02 17:17 [PATCH v3 0/5] Subject: The power allocator thermal governor Javi Merino
` (4 preceding siblings ...)
2015-03-02 17:17 ` [PATCH v3 5/5] thermal: cpu_cooling: update the cpu device when cpufreq updates the policy cpu Javi Merino
@ 2015-03-02 17:28 ` Eduardo Valentin
2015-03-02 17:40 ` Javi Merino
5 siblings, 1 reply; 21+ messages in thread
From: Eduardo Valentin @ 2015-03-02 17:28 UTC (permalink / raw)
To: Javi Merino
Cc: rui.zhang, linux-pm, linux-kernel, punit.agrawal, lina.iyer,
broonie, tixy
[-- Attachment #1: Type: text/plain, Size: 5388 bytes --]
On Mon, Mar 02, 2015 at 05:17:18PM +0000, Javi Merino wrote:
> *** BLURB HERE ***
>
> Hi linux-pm,
>
> Introduce the power allocator governor, a thermal governor that
> allocates device power to control temperature. This series is based
> on branch "linus" of Eduardo's linux-soc-thermal tree:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
>
> Changes since v2:
> - Address Eduardo's review
> + Turn variable-size array in divvy_up_power() into a
> devm_kcalloc() as suggested by Eduardo
> + Remove #ifdeffery from thermal_core.c as suggested by Eduardo
> - Bring back cpufreq's CPUFREQ_UPDATE_POLICY_CPU notifier in order
> to update the cpu device in cpu_cooling.c when cpufreq changes
> the policy cpu.
Can you please elaborate a bit more on the issue you saw to decide to
bring this functionality back?
Can we keep the cpufreq changes as a separated thread? To me if the
issue happens to power allocator, it also happens to the other
governors. I don't want to block power allocator due to the cpufreq
chanes.
Besides, cpufreq changes should go via the proper cpufreq tree, not the
thermal tree.
>
> Changes since v1:
> - Removed a memory leak pointed out by Steve Rostedt
> - Relax the constraint of the trip points to be the first and last
> passive trip point as Lina Iyer suggested.
> - Update how the governor treats weights to mimic the behavior of
> fair share in v3 of the weight fixes series.
> - Don't cache the cpufreq_cooling_device and scan for it whenever
> we need it.
> - Consider the rate of changes in the derivative term of the PID
> controller
>
> Changes since RFC v6:
> - Addressed Eduardo's review
> + Pass the interval to the static power function as suggested by
> Eduardo
> + Make the cooling device ops return 0 or -E* and put the
> calculation in a parameter, like the rest of the cooling device
> ops
> + Documentation improvements
> - Use thermal_cdev_update() to change cooling device states
> - Add a patch to export the power allocator governor's tzp
> parameters to sysfs
>
> Changes since RFC v5:
> - Addressed Stephen's review of the trace patches.
> - Removed power actors and extended the cooling device interface
> instead.
> - Let platforms override the power allocator governor parameters in
> their thermal zone parameters
>
> Changes since RFC v4:
> - Add more tracing
> - Document some of the limitations of the power allocator governor
> - Export the power_actor API and move power_actor.h to include/linux
>
> Changes since RFC v3:
> - Use tz->passive to poll faster when the first trip point is hit.
> - Don't make a special directory for power_actors
> - Add a DT property for sustainable-power
> - Simplify the static power interface and pass the current thermal
> zone in every power_actor_ops to remove the controversial
> enum power_actor_types
> - Use locks with the actor_list list
> - Use cpufreq_get() to get the frequency of the cpu instead of
> using the notifiers.
> - Remove the prompt for THERMAL_POWER_ACTOR_CPU when configuring
> the kernel
>
> Changes since RFC v2:
> - Changed the PI controller into a PID controller
> - Added static power to the cpu power model
> - tz parameter max_dissipatable_power renamed to sustainable_power
> - Register the cpufreq cooling device as part of the
> power_cpu_actor registration.
>
> Changes since RFC v1:
> - Fixed finding cpufreq cooling devices in cpufreq_frequency_change()
> - Replaced the cooling device interface with a separate power actor
> API
> - Addressed most of Eduardo's comments
> - Incorporated ftrace support for bitmask to trace cpumasks
>
> Cheers,
> Javi & Punit
>
> Javi Merino (4):
> thermal: introduce the Power Allocator governor
> thermal: add trace events to the power allocator governor
> thermal: export thermal_zone_parameters to sysfs
> Revert "cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications"
>
> Kapileshwar Singh (1):
> thermal: cpu_cooling: update the cpu device when cpufreq updates the
> policy cpu
>
> Documentation/thermal/power_allocator.txt | 247 ++++++++++++
> Documentation/thermal/sysfs-api.txt | 52 +++
> drivers/cpufreq/cpufreq.c | 3 +
> drivers/thermal/Kconfig | 15 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/cpu_cooling.c | 34 +-
> drivers/thermal/power_allocator.c | 538 +++++++++++++++++++++++++
> drivers/thermal/thermal_core.c | 110 ++++-
> drivers/thermal/thermal_core.h | 8 +
> include/linux/cpufreq.h | 5 +-
> include/linux/thermal.h | 37 +-
> include/trace/events/thermal.h | 58 +++
> include/trace/events/thermal_power_allocator.h | 87 ++++
> 13 files changed, 1184 insertions(+), 11 deletions(-)
> create mode 100644 Documentation/thermal/power_allocator.txt
> create mode 100644 drivers/thermal/power_allocator.c
> create mode 100644 include/trace/events/thermal_power_allocator.h
>
> --
> 1.9.1
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 0/5] Subject: The power allocator thermal governor
2015-03-02 17:28 ` [PATCH v3 0/5] Subject: The power allocator thermal governor Eduardo Valentin
@ 2015-03-02 17:40 ` Javi Merino
2015-03-02 18:47 ` Eduardo Valentin
0 siblings, 1 reply; 21+ messages in thread
From: Javi Merino @ 2015-03-02 17:40 UTC (permalink / raw)
To: Eduardo Valentin
Cc: rui.zhang@intel.com, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org, Punit Agrawal, lina.iyer@linaro.org,
broonie@kernel.org, tixy@linaro.org, Kapileshwar Singh
On Mon, Mar 02, 2015 at 05:28:50PM +0000, Eduardo Valentin wrote:
> On Mon, Mar 02, 2015 at 05:17:18PM +0000, Javi Merino wrote:
> > *** BLURB HERE ***
> >
> > Hi linux-pm,
> >
> > Introduce the power allocator governor, a thermal governor that
> > allocates device power to control temperature. This series is based
> > on branch "linus" of Eduardo's linux-soc-thermal tree:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> >
> > Changes since v2:
> > - Address Eduardo's review
> > + Turn variable-size array in divvy_up_power() into a
> > devm_kcalloc() as suggested by Eduardo
> > + Remove #ifdeffery from thermal_core.c as suggested by Eduardo
> > - Bring back cpufreq's CPUFREQ_UPDATE_POLICY_CPU notifier in order
> > to update the cpu device in cpu_cooling.c when cpufreq changes
> > the policy cpu.
>
>
> Can you please elaborate a bit more on the issue you saw to decide to
> bring this functionality back?
I explained it in patch 5 ("thermal: cpu_cooling: update the cpu
device when cpufreq updates the policy cpu"): When cpufreq changes the
policy cpu, the cpufreq cooling device needs to update the cached cpu
device accordingly.
> Can we keep the cpufreq changes as a separated thread? To me if the
> issue happens to power allocator, it also happens to the other
> governors.
The cached cpu device was introduced in patch 98ea0816118f ("thermal:
cpu_cooling: implement the power cooling device API") in your tree.
I'm posting it here because it only affects doesn't affect other
governors.
> I don't want to block power allocator due to the cpufreq
> changes.
You don't have to. It's the last two patches precisely for that. You
can merge everything up to patch 3 without depending on cpufreq
changes.
> Besides, cpufreq changes should go via the proper cpufreq tree, not the
> thermal tree.
It's a change to cpu_cooling (patch 5) that needs a revert of cpufreq
(patch 4). The change to cpu_cooling depends on the patches already
in your branch. So you will have to carry this patch in your branch
with an Ack from the cpufreq maintainers.
Cheers,
Javi
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 0/5] Subject: The power allocator thermal governor
2015-03-02 17:40 ` Javi Merino
@ 2015-03-02 18:47 ` Eduardo Valentin
0 siblings, 0 replies; 21+ messages in thread
From: Eduardo Valentin @ 2015-03-02 18:47 UTC (permalink / raw)
To: Javi Merino
Cc: rui.zhang@intel.com, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org, Punit Agrawal, lina.iyer@linaro.org,
broonie@kernel.org, tixy@linaro.org, Kapileshwar Singh
[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]
On Mon, Mar 02, 2015 at 05:40:54PM +0000, Javi Merino wrote:
> On Mon, Mar 02, 2015 at 05:28:50PM +0000, Eduardo Valentin wrote:
> > On Mon, Mar 02, 2015 at 05:17:18PM +0000, Javi Merino wrote:
> > > *** BLURB HERE ***
> > >
> > > Hi linux-pm,
> > >
> > > Introduce the power allocator governor, a thermal governor that
> > > allocates device power to control temperature. This series is based
> > > on branch "linus" of Eduardo's linux-soc-thermal tree:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> > >
> > > Changes since v2:
> > > - Address Eduardo's review
> > > + Turn variable-size array in divvy_up_power() into a
> > > devm_kcalloc() as suggested by Eduardo
> > > + Remove #ifdeffery from thermal_core.c as suggested by Eduardo
> > > - Bring back cpufreq's CPUFREQ_UPDATE_POLICY_CPU notifier in order
> > > to update the cpu device in cpu_cooling.c when cpufreq changes
> > > the policy cpu.
> >
> >
> > Can you please elaborate a bit more on the issue you saw to decide to
> > bring this functionality back?
>
> I explained it in patch 5 ("thermal: cpu_cooling: update the cpu
> device when cpufreq updates the policy cpu"): When cpufreq changes the
> policy cpu, the cpufreq cooling device needs to update the cached cpu
> device accordingly.
>
> > Can we keep the cpufreq changes as a separated thread? To me if the
> > issue happens to power allocator, it also happens to the other
> > governors.
>
> The cached cpu device was introduced in patch 98ea0816118f ("thermal:
> cpu_cooling: implement the power cooling device API") in your tree.
> I'm posting it here because it only affects doesn't affect other
> governors.
>
> > I don't want to block power allocator due to the cpufreq
> > changes.
>
> You don't have to. It's the last two patches precisely for that. You
> can merge everything up to patch 3 without depending on cpufreq
> changes.
>
> > Besides, cpufreq changes should go via the proper cpufreq tree, not the
> > thermal tree.
>
> It's a change to cpu_cooling (patch 5) that needs a revert of cpufreq
> (patch 4). The change to cpu_cooling depends on the patches already
> in your branch. So you will have to carry this patch in your branch
> with an Ack from the cpufreq maintainers.
Yes, the patches in the thermal code depend on code in my tree.
However, changes in cpufreq are not depending on any code in my tree.
Unless there is a direct dependency, I would prefer to send them via the
correct tree.
I simple don't feel comfortable sending stuff that are really part of
what is described in MAINTAINERS. But if you get an ack from cpufreq,
then of course, I can consider it.
>
> Cheers,
> Javi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread