From: Lukasz Luba <lukasz.luba@arm.com>
To: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
dri-devel@lists.freedesktop.org, amit.kucheria@verdurent.com,
airlied@linux.ie, daniel.lezcano@linaro.org,
steven.price@arm.com, alyssa.rosenzweig@collabora.com,
rui.zhang@intel.com, orjan.eide@arm.com
Subject: Re: [PATCH 3/5] thermal: devfreq_cooling: add new registration functions with Energy Model
Date: Thu, 22 Oct 2020 12:17:31 +0100 [thread overview]
Message-ID: <71cfae58-8ea5-c591-455b-d84420d8412a@arm.com> (raw)
In-Reply-To: <20201007120746.GA15063@arm.com>
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
next prev parent reply other threads:[~2020-10-22 11:17 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-21 12:20 [PATCH 0/5] Thermal devfreq cooling improvements with Energy Model Lukasz Luba
2020-09-21 12:20 ` [PATCH 1/5] thermal: devfreq_cooling: change tracing function and arguments Lukasz Luba
2020-09-21 12:20 ` [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status Lukasz Luba
2020-10-07 16:11 ` Ionela Voinescu
2020-10-22 10:55 ` Lukasz Luba
2020-12-01 10:36 ` Ionela Voinescu
2020-12-01 12:19 ` Lukasz Luba
2020-12-01 14:55 ` Ionela Voinescu
2020-10-14 14:34 ` Daniel Lezcano
2020-10-22 11:45 ` Lukasz Luba
2020-09-21 12:20 ` [PATCH 3/5] thermal: devfreq_cooling: add new registration functions with Energy Model Lukasz Luba
2020-10-07 12:07 ` Ionela Voinescu
2020-10-22 11:17 ` Lukasz Luba [this message]
2020-12-01 14:05 ` Ionela Voinescu
2020-12-01 14:37 ` Lukasz Luba
2020-12-01 15:02 ` Ionela Voinescu
2020-09-21 12:20 ` [PATCH 4/5] thermal: devfreq_cooling: remove old power model and use EM Lukasz Luba
2020-10-07 15:12 ` Ionela Voinescu
2020-10-22 11:26 ` Lukasz Luba
2020-09-21 12:20 ` [PATCH 5/5] drm/panfrost: Register devfreq cooling and attempt to add Energy Model Lukasz Luba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=71cfae58-8ea5-c591-455b-d84420d8412a@arm.com \
--to=lukasz.luba@arm.com \
--cc=airlied@linux.ie \
--cc=alyssa.rosenzweig@collabora.com \
--cc=amit.kucheria@verdurent.com \
--cc=daniel.lezcano@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=ionela.voinescu@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=orjan.eide@arm.com \
--cc=rui.zhang@intel.com \
--cc=steven.price@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).