| Message ID | 20231220110339.1065505-2-lukasz.luba@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce runtime modifiable Energy Model | expand |
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 >
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!
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.
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
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 --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;
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(+)