Messages in this thread | | | Subject | Re: [PATCH 3/5] thermal: devfreq_cooling: add new registration functions with Energy Model | From | Lukasz Luba <> | Date | Thu, 22 Oct 2020 12:17:31 +0100 |
| |
On 10/7/20 1:07 PM, Ionela Voinescu wrote: > Hi Lukasz, > > On Monday 21 Sep 2020 at 13:20:05 (+0100), Lukasz Luba wrote: >> The Energy Model (EM) framework supports devices such as Devfreq. Create >> new registration functions which automatically register EM for the thermal >> devfreq_cooling devices. This patch prepares the code for coming changes >> which are going to replace old power model with the new EM. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> drivers/thermal/devfreq_cooling.c | 99 ++++++++++++++++++++++++++++++- >> include/linux/devfreq_cooling.h | 22 +++++++ >> 2 files changed, 120 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c >> index cf045bd4d16b..7e091e795284 100644 >> --- a/drivers/thermal/devfreq_cooling.c >> +++ b/drivers/thermal/devfreq_cooling.c >> @@ -50,6 +50,8 @@ static DEFINE_IDA(devfreq_ida); >> * @capped_state: index to cooling state with in dynamic power budget >> * @req_max_freq: PM QoS request for limiting the maximum frequency >> * of the devfreq device. >> + * @em: Energy Model which represents the associated Devfreq device > ^^^^^^^^^^^^^^^^ > for
I will change it.
>> + * @em_registered: Devfreq cooling registered the EM and should free it. >> */ >> struct devfreq_cooling_device { >> int id; >> @@ -63,6 +65,8 @@ struct devfreq_cooling_device { >> u32 res_util; >> int capped_state; >> struct dev_pm_qos_request req_max_freq; >> + struct em_perf_domain *em; >> + bool em_registered; >> }; >> >> static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev, >> @@ -586,22 +590,115 @@ struct thermal_cooling_device *devfreq_cooling_register(struct devfreq *df) >> } >> EXPORT_SYMBOL_GPL(devfreq_cooling_register); >> >> +/** >> + * devfreq_cooling_em_register_power() - Register devfreq cooling device with >> + * power information and attempt to register Energy Model (EM) > > It took me a while to understand the differences between devfreq > register functions and it left me with a nagging feeling that we don't > need all of them. Also, looking over the cpufreq cooling devices, they > keep their registering interfaces quite simple.
This was discussed in previous series, related to EM core changes. It was requested to have a helper registration function which would create EM automatically.
> > With the functions added by this patch, the devfreq cooling devices will have: > - old: > of_devfreq_cooling_register_power > of_devfreq_cooling_register > devfreq_cooling_register > devfreq_cooling_unregister > - new: > devfreq_cooling_em_register_power > devfreq_cooling_em_register > > My question is whether we actually need the two new > devfreq_cooling_em_register_power() and devfreq_cooling_em_register()?
It is just for consistency, with older scheme. It is only a wrapper, one line, with default NULL. This scheme is common in thermal and some other frameworks.
> > The power_ops and the em are dependent on one another, so could we > extend the of_devfreq_cooling_register_power() to do the additional em > registration. We only need a way to pass the em_cb and I think that > could fit nicely in devfreq_cooling_power.
No, they aren't 'dependent on one another'. The EM usage doesn't depend on presence of power_ops. Drivers might not support power_ops, but want the framework still use EM and do power estimation.
> > To be noted that I've reviewed these interfaces in the context of the > final state of devfreq_cooling.c, after the changes in 4/5. > >> + * @df: Pointer to devfreq device. >> + * @dfc_power: Pointer to devfreq_cooling_power. >> + * @em_cb: Callback functions providing the data of the EM >> + * >> + * Register a devfreq cooling device and attempt to register Energy Model. The >> + * available OPPs must be registered for the device. >> + * >> + * If @dfc_power is provided, the cooling device is registered with the >> + * power extensions. If @em_cb is provided it will be called for each OPP to >> + * calculate power value and cost. If @em_cb is not provided then simple Energy >> + * Model is going to be used, which requires "dynamic-power-coefficient" a >> + * devicetree property. >> + */ >> +struct thermal_cooling_device * >> +devfreq_cooling_em_register_power(struct devfreq *df, >> + struct devfreq_cooling_power *dfc_power, >> + struct em_data_callback *em_cb) >> +{ >> + struct thermal_cooling_device *cdev; >> + struct devfreq_cooling_device *dfc; >> + struct device_node *np = NULL; >> + struct device *dev; >> + int nr_opp, ret; >> + >> + if (IS_ERR_OR_NULL(df)) >> + return ERR_PTR(-EINVAL); >> + >> + dev = df->dev.parent; >> + >> + if (em_cb) { >> + nr_opp = dev_pm_opp_get_opp_count(dev); >> + if (nr_opp <= 0) { >> + dev_err(dev, "No valid OPPs found\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + ret = em_dev_register_perf_domain(dev, nr_opp, em_cb, NULL); >> + } else { >> + ret = dev_pm_opp_of_register_em(dev, NULL); >> + } >> + >> + if (ret) >> + dev_warn(dev, "Unable to register EM for devfreq cooling device (%d)\n", >> + ret); >> + >> + if (dev->of_node) >> + np = of_node_get(dev->of_node); >> + >> + cdev = of_devfreq_cooling_register_power(np, df, dfc_power); >> + >> + if (np) >> + of_node_put(np); >> + >> + if (IS_ERR_OR_NULL(cdev)) { >> + if (!ret) >> + em_dev_unregister_perf_domain(dev); >> + } else { >> + dfc = cdev->devdata; >> + dfc->em_registered = !ret; >> + } >> + >> + return cdev; >> +} >> +EXPORT_SYMBOL_GPL(devfreq_cooling_em_register_power); >> + >> +/** >> + * devfreq_cooling_em_register() - Register devfreq cooling device together >> + * with Energy Model. >> + * @df: Pointer to devfreq device. >> + * @em_cb: Callback functions providing the data of the Energy Model >> + * >> + * This function attempts to register Energy Model for devfreq device and then >> + * register the devfreq cooling device. >> + */ >> +struct thermal_cooling_device * >> +devfreq_cooling_em_register(struct devfreq *df, struct em_data_callback *em_cb) >> +{ >> + return devfreq_cooling_em_register_power(df, NULL, em_cb); >> +} >> +EXPORT_SYMBOL_GPL(devfreq_cooling_em_register); >> + >> /** >> * devfreq_cooling_unregister() - Unregister devfreq cooling device. >> * @cdev: Pointer to devfreq cooling device to unregister. >> + * >> + * Unregisters devfreq cooling device and related Energy Model if it was >> + * present. >> */ >> void devfreq_cooling_unregister(struct thermal_cooling_device *cdev) >> { >> struct devfreq_cooling_device *dfc; >> + struct device *dev; >> >> - if (!cdev) >> + if (IS_ERR_OR_NULL(cdev)) >> return; >> >> dfc = cdev->devdata; >> + dev = dfc->devfreq->dev.parent; >> >> thermal_cooling_device_unregister(dfc->cdev); >> ida_simple_remove(&devfreq_ida, dfc->id); >> dev_pm_qos_remove_request(&dfc->req_max_freq); >> + >> + if (dfc->em_registered) >> + em_dev_unregister_perf_domain(dev); > > Nit: Isn't it enough to check if dev->em_pd != NULL to be able to > unregister the perf_domain? That would remove the need for > dfc->em_registered.
The devfreq cooling may only unregister the EM if it has registered it. If any other code did the registration, it should unregister when it finished using it.
> > I suppose one could say that's using implementation details on how the > EM is built and stored and we should not rely on it, so it's up to you > if you want to change it. > > Kind regards, > Ionela. > >> + >> kfree(dfc->power_table); >> kfree(dfc->freq_table); >> >> diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h >> index 9df2dfca68dd..19868fb922f1 100644 >> --- a/include/linux/devfreq_cooling.h >> +++ b/include/linux/devfreq_cooling.h >> @@ -11,6 +11,7 @@ >> #define __DEVFREQ_COOLING_H__ >> >> #include <linux/devfreq.h> >> +#include <linux/energy_model.h> >> #include <linux/thermal.h> >> >> >> @@ -65,6 +66,13 @@ struct thermal_cooling_device * >> of_devfreq_cooling_register(struct device_node *np, struct devfreq *df); >> struct thermal_cooling_device *devfreq_cooling_register(struct devfreq *df); >> void devfreq_cooling_unregister(struct thermal_cooling_device *dfc); >> +struct thermal_cooling_device * >> +devfreq_cooling_em_register_power(struct devfreq *df, >> + struct devfreq_cooling_power *dfc_power, >> + struct em_data_callback *em_cb); >> +struct thermal_cooling_device * >> +devfreq_cooling_em_register(struct devfreq *df, >> + struct em_data_callback *em_cb); >> >> #else /* !CONFIG_DEVFREQ_THERMAL */ >> >> @@ -87,6 +95,20 @@ devfreq_cooling_register(struct devfreq *df) >> return ERR_PTR(-EINVAL); >> } >> >> +static inline struct thermal_cooling_device * >> +devfreq_cooling_em_register_power(struct devfreq *df, >> + struct devfreq_cooling_power *dfc_power, >> + struct em_data_callback *em_cb) >> +{ >> + return ERR_PTR(-EINVAL); >> +} >> + >> +static inline struct thermal_cooling_device * >> +devfreq_cooling_em_register(struct devfreq *df, struct em_data_callback *em_cb) >> +{ >> + return ERR_PTR(-EINVAL); >> +} >> + >> static inline void >> devfreq_cooling_unregister(struct thermal_cooling_device *dfc) >> { >> -- >> 2.17.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
| |