On 10/7/20 4:12 PM, Ionela Voinescu wrote:
Hi Lukasz,
On Monday 21 Sep 2020 at 13:20:06 (+0100), Lukasz Luba wrote:
[..]
/**
- * freq_get_state() - get the cooling state corresponding to a frequency
+ * freq_get_state() - get the performance index corresponding to a frequency
If we change the meaning of the return value, I think the function needs
a name change as well.
Also, we do treat this as a cooling state when we do validation and
compare it to THERMAL_CSTATE_INVALID, but it's not actually a cooling
state (it's max_state - state). It does create confusion if we name
"state" both a performance index and a cooling state.
Given that the only user is devfreq_cooling_get_requested_power(),
might be good to collapse freq_get_state() in that function and rename
the "state" variable in there to "em_perf_idx".
I will have a look into this.
* @dfc: Pointer to devfreq cooling device
- * @freq: frequency in Hz
+ * @freq: frequency in kHz
*
- * Return: the cooling state associated with the @freq, or
+ * Return: the performance index associated with the @freq, or
* THERMAL_CSTATE_INVALID if it wasn't found.
*/
static unsigned long
@@ -128,8 +130,8 @@ freq_get_state(struct devfreq_cooling_device *dfc, unsigned
long freq)
{
int i;
- for (i = 0; i < dfc->freq_table_size; i++) {
- if (dfc->freq_table[i] == freq)
+ for (i = 0; i <= dfc->max_state; i++) {
+ if (dfc->em->table[i].frequency == freq)
return i;
}
@@ -164,71 +166,15 @@ static unsigned long get_voltage(struct devfreq *df, unsigned long freq)
return voltage;
}
-/**
- * get_static_power() - calculate the static power
- * @dfc: Pointer to devfreq cooling device
- * @freq: Frequency in Hz
- *
- * Calculate the static power in milliwatts using the supplied
- * get_static_power(). The current voltage is calculated using the
- * OPP library. If no get_static_power() was supplied, assume the
- * static power is negligible.
- */
-static unsigned long
-get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
+static void dfc_em_get_requested_power(struct em_perf_domain *em,
+ struct devfreq_dev_status *status,
+ u32 *power, int em_perf_idx)
Is there a reason for not directly returning the power value in this
function? Also, this only does a few arithmetic operations and it's only
called in one place. Is it worth to have this in a separate function?
Good question, maybe I will just put this code where it's called.
[..]
@@ -345,11 +279,8 @@ static int devfreq_cooling_power2state(struct
thermal_cooling_device *cdev,
struct devfreq_cooling_device *dfc = cdev->devdata;
struct devfreq *df = dfc->devfreq;
struct devfreq_dev_status status;
- unsigned long busy_time;
+ u32 est_power = power;
Nit: You could use power directly and remove est_power as well.
unsigned long freq;
- s32 dyn_power;
- u32 static_power;
- s32 est_power;
int i;
mutex_lock(&df->lock);
@@ -358,31 +289,26 @@ static int devfreq_cooling_power2state(struct
thermal_cooling_device *cdev,
freq = status.current_frequency;
- if (dfc->power_ops->get_real_power) {
+ if (dfc->power_ops && dfc->power_ops->get_real_power) {
/* Scale for resource utilization */
est_power = power * dfc->res_util;
est_power /= SCALE_ERROR_MITIGATION;
} else {
- static_power = get_static_power(dfc, freq);
-
- dyn_power = power - static_power;
- dyn_power = dyn_power > 0 ? dyn_power : 0;
-
- /* Scale dynamic power for utilization */
- busy_time = status.busy_time ?: 1;
- est_power = (dyn_power * status.total_time) / busy_time;
+ _normalize_load(&status);
+ est_power *= status.total_time;
+ est_power /= status.busy_time;
}
/*
* Find the first cooling state that is within the power
- * budget for dynamic power.
+ * budget. The EM power table is sorted ascending.
*/
- for (i = 0; i < dfc->freq_table_size - 1; i++)
- if (est_power >= dfc->power_table[i])
+ for (i = dfc->max_state; i > 0; i--)
+ if (est_power >= dfc->em->table[i].power)
break;
- *state = i;
- dfc->capped_state = i;
+ *state = dfc->max_state - i;
+ dfc->capped_state = *state;
trace_thermal_power_devfreq_limit(cdev, freq, *state, power);
return 0;
}
[..]
/**
@@ -503,7 +381,7 @@ of_devfreq_cooling_register_power(struct device_node *np,
struct devfreq *df,
struct thermal_cooling_device *cdev;
struct devfreq_cooling_device *dfc;
char dev_name[THERMAL_NAME_LENGTH];
- int err;
+ int err, num_opps;
dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
if (!dfc)
@@ -511,28 +389,45 @@ of_devfreq_cooling_register_power(struct device_node *np,
struct devfreq *df,
dfc->devfreq = df;
- if (dfc_power) {
- dfc->power_ops = dfc_power;
-
+ dfc->em = em_pd_get(df->dev.parent);
+ if (dfc->em) {
devfreq_cooling_ops.get_requested_power =
devfreq_cooling_get_requested_power;
devfreq_cooling_ops.state2power = devfreq_cooling_state2power;
devfreq_cooling_ops.power2state = devfreq_cooling_power2state;
+
+ dfc->power_ops = dfc_power;
+
+ num_opps = em_pd_nr_perf_states(dfc->em);
+ } else {
+ /* Backward compatibility for drivers which do not use IPA */
+ dev_dbg(df->dev.parent, "missing EM for cooling device\n");
+
+ num_opps = dev_pm_opp_get_opp_count(df->dev.parent);
+
+ err = devfreq_cooling_gen_tables(dfc, num_opps);
+ if (err)
+ goto free_dfc;
}
- err = devfreq_cooling_gen_tables(dfc);
- if (err)
+ if (num_opps <= 0) {
+ err = -EINVAL;
goto free_dfc;
+ }
+
+ /* max_state is an index, not a counter */
Nit: Might be more clear to replace "index" with cooling state. Then
knowledge about cooling states would make this more clear.
Similar comment is in cpufreq_cooling.c. The 'index' here means the last
valid index in the array.
Thank you for the review comments for all patches.
Regards,
Lukasz
Regards,
Ionela.
