Re: [PATCH 4/5] thermal: devfreq_cooling: remove old power model and use EM

[Lukasz Luba]

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.