Re: [RESEND][PATCH v2 3/4] PM: EM: Add em_dev_update_chip_binning()

[Lukasz Luba <lukasz.luba@arm.com>]

On 3/26/24 10:09, Dietmar Eggemann wrote:
> On 22/03/2024 12:08, Lukasz Luba wrote:
> 
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index 6960dd7393b2d..f7f7ae34ec552 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -808,3 +808,54 @@ static void em_update_workfn(struct work_struct *work)
>>   {
>>   	em_check_capacity_update();
>>   }
>> +
>> +/**
>> + * em_dev_update_chip_binning() - Update Energy Model with new values after
> 
> s/with new values// ... IMHO this should be obvious ?

Make sense

> 
>> + *			the new voltage information is present in the OPPs.
>> + * @dev		: Device for which the Energy Model has to be updated.
>> + *
>> + * This function allows to update easily the EM with new values available in
>> + * the OPP framework and DT. It can be used after the chip has been properly
>> + * verified by device drivers and the voltages adjusted for the 'chip binning'.
>> + * It uses the "dynamic-power-coefficient" DT property to calculate the power
>> + * values for EM. For power calculation it uses the new adjusted voltage
>> + * values known for OPPs, which might be changed after boot.
> 
> The last two sentences describe what dev_pm_opp_calc_power() is doing.
> Maybe this can be made clearer here?

Or I can just remove this, since it's too detailed description.

> 
>> + */
>> +int em_dev_update_chip_binning(struct device *dev)
> 
> This is the old dev_pm_opp_of_update_em() right?

Yes, it is similar.

> 
>> +{
>> +	struct em_perf_table __rcu *em_table;
>> +	struct em_perf_domain *pd;
>> +	int i, ret;
>> +
>> +	if (IS_ERR_OR_NULL(dev))
>> +		return -EINVAL;
> 
> When do you use if '(IS_ERR_OR_NULL(dev))' and when 'if(!dev)' for EM
> interface functions?

Sometimes IS_ERR_OR_NULL is used, especially for API function other
that register function.

> 
>> +	pd = em_pd_get(dev);
>> +	if (!pd) {
>> +		dev_warn(dev, "Couldn't find Energy Model\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	em_table = em_table_dup(pd);
>> +	if (!em_table) {
>> +		dev_warn(dev, "EM: allocation failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* Update power values which might change due to new voltage in OPPs */
>> +	for (i = 0; i < pd->nr_perf_states; i++) {
>> +		unsigned long freq = em_table->state[i].frequency;
>> +		unsigned long power;
>> +
>> +		ret = dev_pm_opp_calc_power(dev, &power, &freq);
>> +		if (ret) {
>> +			em_table_free(em_table);
>> +			return ret;
>> +		}
>> +
>> +		em_table->state[i].power = power;
>> +	}
>> +
>> +	return em_recalc_and_update(dev, pd, em_table);
>> +}
>> +EXPORT_SYMBOL_GPL(em_dev_update_chip_binning);
> 
> In the previous version of 'chip-binning' you were using the new EM
> interface em_dev_compute_costs() (1) which is now replaced by
> em_recalc_and_update() -> em_compute_costs().
> 
> https://lkml.kernel.org/r/20231220110339.1065505-2-lukasz.luba@arm.com
> 
> Which leaves (1) still unused.
> 
> That was why my concern back then that we shouldn't introduce EM
> interfaces without a user:
> 
> https://lkml.kernel.org/r/8fc499cf-fca1-4465-bff7-a93dfd36f3c8@arm.com
> 
> What happens now with em_dev_compute_costs()?
> 

For now it's not used, but modules which will create new EMs
with custom power values will use it. When such a module have
e.g. 5 EMs for one PD and only switches on one of them, then
this em_dev_compute_costs() will be used at setup for those
5 EMs. Later it won't be used.
I don't wanted to combine the registration of new EM with
the compute cost, because that will create overhead in the
switching to new EM code path. Now we have only ~3us, which
was the main goal.

When our scmi-cpufreq get the support for EM update this
compute cost will be used there.