diff mbox series

[1/2] OPP: Add API to update EM after adjustment of voltage for OPPs

Message ID 20231220110339.1065505-2-lukasz.luba@arm.com
State New
Headers show
Series Introduce runtime modifiable Energy Model | expand

Commit Message

Lukasz Luba Dec. 20, 2023, 11:03 a.m. UTC
There are device drivers which can modify voltage values for OPPs. It
could be due to the chip binning and those drivers have specific chip
knowledge about this. This adjustment can happen after Energy Model is
registered, thus EM can have stale data about power.

Introduce new API function which can be used by device driver which
adjusted the voltage for OPPs. The implementation takes care about
calculating needed internal details in the new EM table ('cost' field).
It plugs in the new EM table to the framework so other subsystems would
use the correct data.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++
 2 files changed, 75 insertions(+)

Comments

Xuewen Yan Dec. 21, 2023, 7:28 a.m. UTC | #1
On Wed, Dec 20, 2023 at 7:02 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> There are device drivers which can modify voltage values for OPPs. It
> could be due to the chip binning and those drivers have specific chip
> knowledge about this. This adjustment can happen after Energy Model is
> registered, thus EM can have stale data about power.
>
> Introduce new API function which can be used by device driver which
> adjusted the voltage for OPPs. The implementation takes care about
> calculating needed internal details in the new EM table ('cost' field).
> It plugs in the new EM table to the framework so other subsystems would
> use the correct data.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  6 ++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 81fa27599d58..992434c0b711 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
> +
> +/**
> + * dev_pm_opp_of_update_em() - Update Energy Model with new power values
> + * @dev                : Device for which an Energy Model has to be registered
> + *
> + * This uses the "dynamic-power-coefficient" devicetree property to calculate
> + * power values for EM. It uses the new adjusted voltage values known for OPPs
> + * which have changed after boot.
> + */
> +int dev_pm_opp_of_update_em(struct device *dev)
> +{
> +       struct em_perf_table __rcu *runtime_table;
> +       struct em_perf_state *table, *new_table;
> +       struct em_perf_domain *pd;
> +       int ret, table_size, i;
> +
> +       if (IS_ERR_OR_NULL(dev))
> +               return -EINVAL;
> +
> +       pd = em_pd_get(dev);
> +       if (!pd) {
> +               dev_warn(dev, "Couldn't find Energy Model %d\n", ret);
> +               return -EINVAL;
> +       }
> +
> +       runtime_table = em_allocate_table(pd);
> +       if (!runtime_table) {
> +               dev_warn(dev, "new EM allocation failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       new_table = runtime_table->state;
> +
> +       table = em_get_table(pd);
> +       /* Initialize data based on older EM table */
> +       table_size = sizeof(struct em_perf_state) * pd->nr_perf_states;
> +       memcpy(new_table, table, table_size);
> +
> +       em_put_table();
> +
> +       /* Update power values which might change due to new voltage in OPPs */
> +       for (i = 0; i < pd->nr_perf_states; i++) {
> +               unsigned long freq = new_table[i].frequency;
> +               unsigned long power;
> +
> +               ret = _get_power(dev, &power, &freq);
> +               if (ret)
> +                       goto failed;

Need we use the EM_SET_ACTIVE_POWER_CB(em_cb, _get_power) and call
em_cb->active_power?

> +
> +               new_table[i].power = power;
> +       }
> +
> +       ret = em_dev_compute_costs(dev, new_table, pd->nr_perf_states);
> +       if (ret)
> +               goto failed;
> +
> +       ret = em_dev_update_perf_domain(dev, runtime_table);
> +       if (ret)
> +               goto failed;
> +
> +       return 0;
> +
> +failed:
> +       dev_warn(dev, "EM update failed %d\n", ret);
> +       em_free_table(runtime_table);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_of_update_em);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index ccd97bcef269..b3ab117890fc 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -464,6 +464,7 @@ struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
>  int of_get_required_opp_performance_state(struct device_node *np, int index);
>  int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table);
>  int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus);
> +int dev_pm_opp_of_update_em(struct device *dev);
>  static inline void dev_pm_opp_of_unregister_em(struct device *dev)
>  {
>         em_dev_unregister_perf_domain(dev);
> @@ -527,6 +528,11 @@ static inline void dev_pm_opp_of_unregister_em(struct device *dev)
>  {
>  }
>
> +static inline int dev_pm_opp_of_update_em(struct device *dev)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  static inline int of_get_required_opp_performance_state(struct device_node *np, int index)
>  {
>         return -EOPNOTSUPP;
> --
> 2.25.1
>
Lukasz Luba Dec. 21, 2023, 8:04 a.m. UTC | #2
On 12/21/23 07:28, Xuewen Yan wrote:
> On Wed, Dec 20, 2023 at 7:02 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> There are device drivers which can modify voltage values for OPPs. It
>> could be due to the chip binning and those drivers have specific chip
>> knowledge about this. This adjustment can happen after Energy Model is
>> registered, thus EM can have stale data about power.
>>
>> Introduce new API function which can be used by device driver which
>> adjusted the voltage for OPPs. The implementation takes care about
>> calculating needed internal details in the new EM table ('cost' field).
>> It plugs in the new EM table to the framework so other subsystems would
>> use the correct data.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pm_opp.h |  6 ++++
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index 81fa27599d58..992434c0b711 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>>          return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
>> +
>> +/**
>> + * dev_pm_opp_of_update_em() - Update Energy Model with new power values
>> + * @dev                : Device for which an Energy Model has to be registered
>> + *
>> + * This uses the "dynamic-power-coefficient" devicetree property to calculate
>> + * power values for EM. It uses the new adjusted voltage values known for OPPs
>> + * which have changed after boot.
>> + */
>> +int dev_pm_opp_of_update_em(struct device *dev)
>> +{
>> +       struct em_perf_table __rcu *runtime_table;
>> +       struct em_perf_state *table, *new_table;
>> +       struct em_perf_domain *pd;
>> +       int ret, table_size, i;
>> +
>> +       if (IS_ERR_OR_NULL(dev))
>> +               return -EINVAL;
>> +
>> +       pd = em_pd_get(dev);
>> +       if (!pd) {
>> +               dev_warn(dev, "Couldn't find Energy Model %d\n", ret);
>> +               return -EINVAL;
>> +       }
>> +
>> +       runtime_table = em_allocate_table(pd);
>> +       if (!runtime_table) {
>> +               dev_warn(dev, "new EM allocation failed\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       new_table = runtime_table->state;
>> +
>> +       table = em_get_table(pd);
>> +       /* Initialize data based on older EM table */
>> +       table_size = sizeof(struct em_perf_state) * pd->nr_perf_states;
>> +       memcpy(new_table, table, table_size);
>> +
>> +       em_put_table();
>> +
>> +       /* Update power values which might change due to new voltage in OPPs */
>> +       for (i = 0; i < pd->nr_perf_states; i++) {
>> +               unsigned long freq = new_table[i].frequency;
>> +               unsigned long power;
>> +
>> +               ret = _get_power(dev, &power, &freq);
>> +               if (ret)
>> +                       goto failed;
> 
> Need we use the EM_SET_ACTIVE_POWER_CB(em_cb, _get_power) and call
> em_cb->active_power?
> 

No, not in this case. It's not like registration of EM, when there
is a need to also pass the callback function. As you can see this code
operates locally and the call _get_power() just simply gets the
power in straight way. Later the whole 'runtime_table' is passed to the
EM framework to 'swap' the pointers under RCU.

Thanks Xuewen for having a look at this!
Viresh Kumar Dec. 26, 2023, 5:12 a.m. UTC | #3
On 20-12-23, 11:03, Lukasz Luba wrote:
> There are device drivers which can modify voltage values for OPPs. It
> could be due to the chip binning and those drivers have specific chip
> knowledge about this. This adjustment can happen after Energy Model is
> registered, thus EM can have stale data about power.
> 
> Introduce new API function which can be used by device driver which
> adjusted the voltage for OPPs. The implementation takes care about
> calculating needed internal details in the new EM table ('cost' field).
> It plugs in the new EM table to the framework so other subsystems would
> use the correct data.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  6 ++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 81fa27599d58..992434c0b711 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
> +
> +/**
> + * dev_pm_opp_of_update_em() - Update Energy Model with new power values
> + * @dev		: Device for which an Energy Model has to be registered
> + *
> + * This uses the "dynamic-power-coefficient" devicetree property to calculate
> + * power values for EM. It uses the new adjusted voltage values known for OPPs
> + * which have changed after boot.
> + */
> +int dev_pm_opp_of_update_em(struct device *dev)

I don't see anything OPP or OF related in this function, I don't think it needs
to be part of the OPP core. You just want to reuse _get_power() I guess, which
can be exported then.

This should really be part of the EM core instead.
Lukasz Luba Jan. 4, 2024, 10:38 a.m. UTC | #4
Hi Viresh,

On 12/26/23 05:12, Viresh Kumar wrote:
> On 20-12-23, 11:03, Lukasz Luba wrote:
>> There are device drivers which can modify voltage values for OPPs. It
>> could be due to the chip binning and those drivers have specific chip
>> knowledge about this. This adjustment can happen after Energy Model is
>> registered, thus EM can have stale data about power.
>>
>> Introduce new API function which can be used by device driver which
>> adjusted the voltage for OPPs. The implementation takes care about
>> calculating needed internal details in the new EM table ('cost' field).
>> It plugs in the new EM table to the framework so other subsystems would
>> use the correct data.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pm_opp.h |  6 ++++
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index 81fa27599d58..992434c0b711 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
>> +
>> +/**
>> + * dev_pm_opp_of_update_em() - Update Energy Model with new power values
>> + * @dev		: Device for which an Energy Model has to be registered
>> + *
>> + * This uses the "dynamic-power-coefficient" devicetree property to calculate
>> + * power values for EM. It uses the new adjusted voltage values known for OPPs
>> + * which have changed after boot.
>> + */
>> +int dev_pm_opp_of_update_em(struct device *dev)
> 
> I don't see anything OPP or OF related in this function, I don't think it needs
> to be part of the OPP core. You just want to reuse _get_power() I guess, which
> can be exported then.
> 
> This should really be part of the EM core instead.
> 

Thank you for having a look at this. OK, that makes sense.
When I finish the EM runtime modification core features and get them
merged, I'll continue to work on this patch set. I'll try to follow
your comment here and export that function (with a different name
probably).

Regards,
Lukasz
Dietmar Eggemann Jan. 4, 2024, 5:11 p.m. UTC | #5
On 04/01/2024 11:38, Lukasz Luba wrote:
> Hi Viresh,
> 
> On 12/26/23 05:12, Viresh Kumar wrote:
>> On 20-12-23, 11:03, Lukasz Luba wrote:
>>> There are device drivers which can modify voltage values for OPPs. It
>>> could be due to the chip binning and those drivers have specific chip
>>> knowledge about this. This adjustment can happen after Energy Model is
>>> registered, thus EM can have stale data about power.
>>>
>>> Introduce new API function which can be used by device driver which
>>> adjusted the voltage for OPPs. The implementation takes care about
>>> calculating needed internal details in the new EM table ('cost' field).
>>> It plugs in the new EM table to the framework so other subsystems would
>>> use the correct data.
>>>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>> ---
>>>   drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/pm_opp.h |  6 ++++
>>>   2 files changed, 75 insertions(+)
>>>
>>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>>> index 81fa27599d58..992434c0b711 100644
>>> --- a/drivers/opp/of.c
>>> +++ b/drivers/opp/of.c
>>> @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device
>>> *dev, struct cpumask *cpus)
>>>       return ret;
>>>   }
>>>   EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
>>> +
>>> +/**
>>> + * dev_pm_opp_of_update_em() - Update Energy Model with new power
>>> values
>>> + * @dev        : Device for which an Energy Model has to be registered
>>> + *
>>> + * This uses the "dynamic-power-coefficient" devicetree property to
>>> calculate
>>> + * power values for EM. It uses the new adjusted voltage values
>>> known for OPPs
>>> + * which have changed after boot.
>>> + */
>>> +int dev_pm_opp_of_update_em(struct device *dev)
>>
>> I don't see anything OPP or OF related in this function, I don't think
>> it needs
>> to be part of the OPP core. You just want to reuse _get_power() I
>> guess, which
>> can be exported then.
>>
>> This should really be part of the EM core instead.
>>
> 
> Thank you for having a look at this. OK, that makes sense.
> When I finish the EM runtime modification core features and get them
> merged, I'll continue to work on this patch set. I'll try to follow
> your comment here and export that function (with a different name
> probably).

Just to make sure: If this is the case then you could also add
em_dev_compute_costs() with this new patch instead providing it with the
'Introduce runtime modifiable Energy Model' patch-set?

This would keep dev_pm_opp_of_update_em() and em_dev_compute_costs()
together. IIRC, all the other new EM interfaces you already use with
your 'modifiable EM' use case: '[PATCH v5 14/23] PM: EM: Support late
CPUs booting and capacity adjustment'.
Lukasz Luba Jan. 4, 2024, 5:27 p.m. UTC | #6
On 1/4/24 17:11, Dietmar Eggemann wrote:
> On 04/01/2024 11:38, Lukasz Luba wrote:
>> Hi Viresh,
>>
>> On 12/26/23 05:12, Viresh Kumar wrote:
>>> On 20-12-23, 11:03, Lukasz Luba wrote:
>>>> There are device drivers which can modify voltage values for OPPs. It
>>>> could be due to the chip binning and those drivers have specific chip
>>>> knowledge about this. This adjustment can happen after Energy Model is
>>>> registered, thus EM can have stale data about power.
>>>>
>>>> Introduce new API function which can be used by device driver which
>>>> adjusted the voltage for OPPs. The implementation takes care about
>>>> calculating needed internal details in the new EM table ('cost' field).
>>>> It plugs in the new EM table to the framework so other subsystems would
>>>> use the correct data.
>>>>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>>    drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/pm_opp.h |  6 ++++
>>>>    2 files changed, 75 insertions(+)
>>>>
>>>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>>>> index 81fa27599d58..992434c0b711 100644
>>>> --- a/drivers/opp/of.c
>>>> +++ b/drivers/opp/of.c
>>>> @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device
>>>> *dev, struct cpumask *cpus)
>>>>        return ret;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
>>>> +
>>>> +/**
>>>> + * dev_pm_opp_of_update_em() - Update Energy Model with new power
>>>> values
>>>> + * @dev        : Device for which an Energy Model has to be registered
>>>> + *
>>>> + * This uses the "dynamic-power-coefficient" devicetree property to
>>>> calculate
>>>> + * power values for EM. It uses the new adjusted voltage values
>>>> known for OPPs
>>>> + * which have changed after boot.
>>>> + */
>>>> +int dev_pm_opp_of_update_em(struct device *dev)
>>>
>>> I don't see anything OPP or OF related in this function, I don't think
>>> it needs
>>> to be part of the OPP core. You just want to reuse _get_power() I
>>> guess, which
>>> can be exported then.
>>>
>>> This should really be part of the EM core instead.
>>>
>>
>> Thank you for having a look at this. OK, that makes sense.
>> When I finish the EM runtime modification core features and get them
>> merged, I'll continue to work on this patch set. I'll try to follow
>> your comment here and export that function (with a different name
>> probably).
> 
> Just to make sure: If this is the case then you could also add
> em_dev_compute_costs() with this new patch instead providing it with the
> 'Introduce runtime modifiable Energy Model' patch-set?

You're referring to the patch 22/23 [1]. Yes, it could be skipped,
but both will go in the same merge window, so not big difference.
I tend to agree that patch 22/23 could belong to this $subject.

As soon as Rafael will merge the core runtime patches, I will
push this small one from this $subject. So it will be in a few
days delay (assuming I would get an Ack from Marek or Krzysztof
for the Exynos patch).

> 
> This would keep dev_pm_opp_of_update_em() and em_dev_compute_costs()
> together. IIRC, all the other new EM interfaces you already use with
> your 'modifiable EM' use case: '[PATCH v5 14/23] PM: EM: Support late
> CPUs booting and capacity adjustment'.
> 
> 
> 

Yes, correct, the rest of API is used (mainly from thermal/dtmp).

[1] 
https://lore.kernel.org/lkml/20240104171553.2080674-23-lukasz.luba@arm.com/
diff mbox series

Patch

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 81fa27599d58..992434c0b711 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1596,3 +1596,72 @@  int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
+
+/**
+ * dev_pm_opp_of_update_em() - Update Energy Model with new power values
+ * @dev		: Device for which an Energy Model has to be registered
+ *
+ * This uses the "dynamic-power-coefficient" devicetree property to calculate
+ * power values for EM. It uses the new adjusted voltage values known for OPPs
+ * which have changed after boot.
+ */
+int dev_pm_opp_of_update_em(struct device *dev)
+{
+	struct em_perf_table __rcu *runtime_table;
+	struct em_perf_state *table, *new_table;
+	struct em_perf_domain *pd;
+	int ret, table_size, i;
+
+	if (IS_ERR_OR_NULL(dev))
+		return -EINVAL;
+
+	pd = em_pd_get(dev);
+	if (!pd) {
+		dev_warn(dev, "Couldn't find Energy Model %d\n", ret);
+		return -EINVAL;
+	}
+
+	runtime_table = em_allocate_table(pd);
+	if (!runtime_table) {
+		dev_warn(dev, "new EM allocation failed\n");
+		return -ENOMEM;
+	}
+
+	new_table = runtime_table->state;
+
+	table = em_get_table(pd);
+	/* Initialize data based on older EM table */
+	table_size = sizeof(struct em_perf_state) * pd->nr_perf_states;
+	memcpy(new_table, table, table_size);
+
+	em_put_table();
+
+	/* Update power values which might change due to new voltage in OPPs */
+	for (i = 0; i < pd->nr_perf_states; i++) {
+		unsigned long freq = new_table[i].frequency;
+		unsigned long power;
+
+		ret = _get_power(dev, &power, &freq);
+		if (ret)
+			goto failed;
+
+		new_table[i].power = power;
+	}
+
+	ret = em_dev_compute_costs(dev, new_table, pd->nr_perf_states);
+	if (ret)
+		goto failed;
+
+	ret = em_dev_update_perf_domain(dev, runtime_table);
+	if (ret)
+		goto failed;
+
+	return 0;
+
+failed:
+	dev_warn(dev, "EM update failed %d\n", ret);
+	em_free_table(runtime_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_of_update_em);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index ccd97bcef269..b3ab117890fc 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -464,6 +464,7 @@  struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
 int of_get_required_opp_performance_state(struct device_node *np, int index);
 int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table);
 int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus);
+int dev_pm_opp_of_update_em(struct device *dev);
 static inline void dev_pm_opp_of_unregister_em(struct device *dev)
 {
 	em_dev_unregister_perf_domain(dev);
@@ -527,6 +528,11 @@  static inline void dev_pm_opp_of_unregister_em(struct device *dev)
 {
 }
 
+static inline int dev_pm_opp_of_update_em(struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int of_get_required_opp_performance_state(struct device_node *np, int index)
 {
 	return -EOPNOTSUPP;