| Message ID | cover.1560163748.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | cpufreq: Use QoS layer to manage freq-constraints | expand |
On Mon, Jun 10, 2019 at 04:21:32PM +0530, Viresh Kumar wrote: > In order to use the same set of routines to register notifiers for > different request types, update the existing > dev_pm_qos_{add|remove}_notifier() routines with an additional > parameter: request-type. > > For now, it only supports resume-latency request type. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Documentation/power/pm_qos_interface.txt | 10 ++++++---- > drivers/base/power/domain.c | 8 +++++--- > drivers/base/power/qos.c | 14 ++++++++++++-- > include/linux/pm_qos.h | 12 ++++++++---- > 4 files changed, 31 insertions(+), 13 deletions(-) My QoS background is nil, but this looks reasonable to me: Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Hi Viresh, On Mon, Jun 10, 2019 at 04:21:35PM +0530, Viresh Kumar wrote: > This registers the notifiers for min/max frequency constraints with the > PM QoS framework. The constraints are also taken into consideration in > cpufreq_set_policy(). > > This also relocates cpufreq_policy_put_kobj() as it is required to be > called from cpufreq_policy_alloc() now. > > No constraints are added until now though. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++------- > include/linux/cpufreq.h | 4 ++ > 2 files changed, 120 insertions(+), 23 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 85ff958e01f1..547d221b2ff2 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -26,6 +26,7 @@ > #include <linux/kernel_stat.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/pm_qos.h> > #include <linux/slab.h> > #include <linux/suspend.h> > #include <linux/syscore_ops.h> > @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work) > cpufreq_update_policy(cpu); > } > > +static void cpufreq_update_freq_work(struct work_struct *work) > +{ > + struct cpufreq_policy *policy = > + container_of(work, struct cpufreq_policy, req_work); > + struct cpufreq_policy new_policy = *policy; > + > + /* We should read constraint values from QoS layer */ > + new_policy.min = 0; > + new_policy.max = UINT_MAX; > + > + down_write(&policy->rwsem); > + > + if (!policy_is_inactive(policy)) > + cpufreq_set_policy(policy, &new_policy); > + > + up_write(&policy->rwsem); > +} > + > +static int cpufreq_update_freq(struct cpufreq_policy *policy) > +{ > + schedule_work(&policy->req_work); I think you need to add a cancel_work_sync() in cpufreq_policy_free() to make sure the work doesn't run after the policy has been freed. Otherwise it looks good to me. Cheers Matthias
Hi Viresh, On Mon, Jun 10, 2019 at 04:21:36PM +0530, Viresh Kumar wrote: > This implements QoS requests to manage userspace configuration of min > and max frequency. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++-------------------- > include/linux/cpufreq.h | 8 +--- > 2 files changed, 47 insertions(+), 53 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 547d221b2ff2..ff754981fcb4 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) > static ssize_t store_##file_name \ > (struct cpufreq_policy *policy, const char *buf, size_t count) \ > { \ > - int ret, temp; \ > - struct cpufreq_policy new_policy; \ > + unsigned long val; \ > + int ret; \ > \ > - memcpy(&new_policy, policy, sizeof(*policy)); \ > - new_policy.min = policy->user_policy.min; \ > - new_policy.max = policy->user_policy.max; \ > - \ > - ret = sscanf(buf, "%u", &new_policy.object); \ > + ret = sscanf(buf, "%lu", &val); \ > if (ret != 1) \ > return -EINVAL; \ > \ > - temp = new_policy.object; \ > - ret = cpufreq_set_policy(policy, &new_policy); \ > - if (!ret) \ > - policy->user_policy.object = temp; \ > - \ > - return ret ? ret : count; \ > + ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\ > + return ret && ret != 1 ? ret : count; \ nit: I wonder if return (ret >= 0) ? count : ret; would be clearer. Other than that: Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On 14-06-19, 10:14, Matthias Kaehlcke wrote: > Hi Viresh, > > On Mon, Jun 10, 2019 at 04:21:36PM +0530, Viresh Kumar wrote: > > This implements QoS requests to manage userspace configuration of min > > and max frequency. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++-------------------- > > include/linux/cpufreq.h | 8 +--- > > 2 files changed, 47 insertions(+), 53 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 547d221b2ff2..ff754981fcb4 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) > > static ssize_t store_##file_name \ > > (struct cpufreq_policy *policy, const char *buf, size_t count) \ > > { \ > > - int ret, temp; \ > > - struct cpufreq_policy new_policy; \ > > + unsigned long val; \ > > + int ret; \ > > \ > > - memcpy(&new_policy, policy, sizeof(*policy)); \ > > - new_policy.min = policy->user_policy.min; \ > > - new_policy.max = policy->user_policy.max; \ > > - \ > > - ret = sscanf(buf, "%u", &new_policy.object); \ > > + ret = sscanf(buf, "%lu", &val); \ > > if (ret != 1) \ > > return -EINVAL; \ > > \ > > - temp = new_policy.object; \ > > - ret = cpufreq_set_policy(policy, &new_policy); \ > > - if (!ret) \ > > - policy->user_policy.object = temp; \ > > - \ > > - return ret ? ret : count; \ > > + ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\ > > + return ret && ret != 1 ? ret : count; \ > > nit: I wonder if > > return (ret >= 0) ? count : ret; > > would be clearer. Done. Thanks. > Other than that: > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> -- viresh
On Mon, 10 Jun 2019 at 12:51, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > In order to use the same set of routines to register notifiers for > different request types, update the existing > dev_pm_qos_{add|remove}_notifier() routines with an additional > parameter: request-type. > > For now, it only supports resume-latency request type. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > Documentation/power/pm_qos_interface.txt | 10 ++++++---- > drivers/base/power/domain.c | 8 +++++--- > drivers/base/power/qos.c | 14 ++++++++++++-- > include/linux/pm_qos.h | 12 ++++++++---- > 4 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt > index 19c5f7b1a7ba..ec7d662d1707 100644 > --- a/Documentation/power/pm_qos_interface.txt > +++ b/Documentation/power/pm_qos_interface.txt > @@ -164,12 +164,14 @@ directory. > Notification mechanisms: > The per-device PM QoS framework has a per-device notification tree. > > -int dev_pm_qos_add_notifier(device, notifier): > -Adds a notification callback function for the device. > +int dev_pm_qos_add_notifier(device, notifier, type): > +Adds a notification callback function for the device for a particular request > +type. > + > The callback is called when the aggregated value of the device constraints list > -is changed (for resume latency device PM QoS only). > +is changed. > > -int dev_pm_qos_remove_notifier(device, notifier): > +int dev_pm_qos_remove_notifier(device, notifier, type): > Removes the notification callback function for the device. > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 33c30c1e6a30..b063bc41b0a9 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1536,7 +1536,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > if (ret) > genpd_free_dev_data(dev, gpd_data); > else > - dev_pm_qos_add_notifier(dev, &gpd_data->nb); > + dev_pm_qos_add_notifier(dev, &gpd_data->nb, > + DEV_PM_QOS_RESUME_LATENCY); > > return ret; > } > @@ -1569,7 +1570,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > > pdd = dev->power.subsys_data->domain_data; > gpd_data = to_gpd_data(pdd); > - dev_pm_qos_remove_notifier(dev, &gpd_data->nb); > + dev_pm_qos_remove_notifier(dev, &gpd_data->nb, > + DEV_PM_QOS_RESUME_LATENCY); > > genpd_lock(genpd); > > @@ -1597,7 +1599,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > > out: > genpd_unlock(genpd); > - dev_pm_qos_add_notifier(dev, &gpd_data->nb); > + dev_pm_qos_add_notifier(dev, &gpd_data->nb, DEV_PM_QOS_RESUME_LATENCY); > > return ret; > } > diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c > index 6c91f8df1d59..cfd463212513 100644 > --- a/drivers/base/power/qos.c > +++ b/drivers/base/power/qos.c > @@ -467,6 +467,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request); > * > * @dev: target device for the constraint > * @notifier: notifier block managed by caller. > + * @type: request type. > * > * Will register the notifier into a notification chain that gets called > * upon changes to the target value for the device. > @@ -474,10 +475,14 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request); > * If the device's constraints object doesn't exist when this routine is called, > * it will be created (or error code will be returned if that fails). > */ > -int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier) > +int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier, > + enum dev_pm_qos_req_type type) > { > int ret = 0; > > + if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY)) > + return -EINVAL; > + > mutex_lock(&dev_pm_qos_mtx); > > if (IS_ERR(dev->power.qos)) > @@ -500,15 +505,20 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier); > * > * @dev: target device for the constraint > * @notifier: notifier block to be removed. > + * @type: request type. > * > * Will remove the notifier from the notification chain that gets called > * upon changes to the target value. > */ > int dev_pm_qos_remove_notifier(struct device *dev, > - struct notifier_block *notifier) > + struct notifier_block *notifier, > + enum dev_pm_qos_req_type type) > { > int retval = 0; > > + if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY)) > + return -EINVAL; > + > mutex_lock(&dev_pm_qos_mtx); > > /* Silently return if the constraints object is not present. */ > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h > index 6ea1ae373d77..1f4d456e8fff 100644 > --- a/include/linux/pm_qos.h > +++ b/include/linux/pm_qos.h > @@ -146,9 +146,11 @@ int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req, > int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value); > int dev_pm_qos_remove_request(struct dev_pm_qos_request *req); > int dev_pm_qos_add_notifier(struct device *dev, > - struct notifier_block *notifier); > + struct notifier_block *notifier, > + enum dev_pm_qos_req_type type); > int dev_pm_qos_remove_notifier(struct device *dev, > - struct notifier_block *notifier); > + struct notifier_block *notifier, > + enum dev_pm_qos_req_type type); > void dev_pm_qos_constraints_init(struct device *dev); > void dev_pm_qos_constraints_destroy(struct device *dev); > int dev_pm_qos_add_ancestor_request(struct device *dev, > @@ -202,10 +204,12 @@ static inline int dev_pm_qos_update_request(struct dev_pm_qos_request *req, > static inline int dev_pm_qos_remove_request(struct dev_pm_qos_request *req) > { return 0; } > static inline int dev_pm_qos_add_notifier(struct device *dev, > - struct notifier_block *notifier) > + struct notifier_block *notifier, > + enum dev_pm_qos_req_type type); > { return 0; } > static inline int dev_pm_qos_remove_notifier(struct device *dev, > - struct notifier_block *notifier) > + struct notifier_block *notifier, > + enum dev_pm_qos_req_type type) > { return 0; } > static inline void dev_pm_qos_constraints_init(struct device *dev) > { > -- > 2.21.0.rc0.269.g1a574e7a288b >
On Mon, 10 Jun 2019 at 12:52, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > This patch introduces the min-frequency and max-frequency device > constraints, which will be used by the cpufreq core to begin with. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/qos.c | 103 +++++++++++++++++++++++++++++++++------ > include/linux/pm_qos.h | 18 +++++++ > 2 files changed, 107 insertions(+), 14 deletions(-) > > diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c > index 7bb88174371f..e977aef437a5 100644 > --- a/drivers/base/power/qos.c > +++ b/drivers/base/power/qos.c > @@ -151,6 +151,14 @@ static int apply_constraint(struct dev_pm_qos_request *req, > req->dev->power.set_latency_tolerance(req->dev, value); > } > break; > + case DEV_PM_QOS_MIN_FREQUENCY: > + ret = pm_qos_update_target(&qos->min_frequency, > + &req->data.pnode, action, value); > + break; > + case DEV_PM_QOS_MAX_FREQUENCY: > + ret = pm_qos_update_target(&qos->max_frequency, > + &req->data.pnode, action, value); > + break; > case DEV_PM_QOS_FLAGS: > ret = pm_qos_update_flags(&qos->flags, &req->data.flr, > action, value); > @@ -179,12 +187,11 @@ static int dev_pm_qos_constraints_allocate(struct device *dev) > if (!qos) > return -ENOMEM; > > - n = kzalloc(sizeof(*n), GFP_KERNEL); > + n = kzalloc(3 * sizeof(*n), GFP_KERNEL); > if (!n) { > kfree(qos); > return -ENOMEM; > } > - BLOCKING_INIT_NOTIFIER_HEAD(n); > > c = &qos->resume_latency; > plist_head_init(&c->list); > @@ -193,6 +200,7 @@ static int dev_pm_qos_constraints_allocate(struct device *dev) > c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; > c->type = PM_QOS_MIN; > c->notifiers = n; > + BLOCKING_INIT_NOTIFIER_HEAD(n); > > c = &qos->latency_tolerance; > plist_head_init(&c->list); > @@ -201,6 +209,24 @@ static int dev_pm_qos_constraints_allocate(struct device *dev) > c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT; > c->type = PM_QOS_MIN; > > + c = &qos->min_frequency; > + plist_head_init(&c->list); > + c->target_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; > + c->default_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; > + c->no_constraint_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; > + c->type = PM_QOS_MAX; > + c->notifiers = ++n; > + BLOCKING_INIT_NOTIFIER_HEAD(n); > + > + c = &qos->max_frequency; > + plist_head_init(&c->list); > + c->target_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; > + c->default_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; > + c->no_constraint_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; > + c->type = PM_QOS_MIN; > + c->notifiers = ++n; > + BLOCKING_INIT_NOTIFIER_HEAD(n); > + > INIT_LIST_HEAD(&qos->flags.list); > > spin_lock_irq(&dev->power.lock); > @@ -254,11 +280,25 @@ void dev_pm_qos_constraints_destroy(struct device *dev) > apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE); > memset(req, 0, sizeof(*req)); > } > + > c = &qos->latency_tolerance; > plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) { > apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE); > memset(req, 0, sizeof(*req)); > } > + > + c = &qos->min_frequency; > + plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) { > + apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE); > + memset(req, 0, sizeof(*req)); > + } > + > + c = &qos->max_frequency; > + plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) { > + apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE); > + memset(req, 0, sizeof(*req)); > + } > + > f = &qos->flags; > list_for_each_entry_safe(req, tmp, &f->list, data.flr.node) { > apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE); > @@ -370,6 +410,8 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req, > switch(req->type) { > case DEV_PM_QOS_RESUME_LATENCY: > case DEV_PM_QOS_LATENCY_TOLERANCE: > + case DEV_PM_QOS_MIN_FREQUENCY: > + case DEV_PM_QOS_MAX_FREQUENCY: > curr_value = req->data.pnode.prio; > break; > case DEV_PM_QOS_FLAGS: > @@ -482,9 +524,6 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier, > { > int ret = 0; > > - if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY)) > - return -EINVAL; > - > mutex_lock(&dev_pm_qos_mtx); > > if (IS_ERR(dev->power.qos)) > @@ -492,10 +531,28 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier, > else if (!dev->power.qos) > ret = dev_pm_qos_constraints_allocate(dev); > > - if (!ret) > + if (ret) > + goto unlock; > + > + switch (type) { > + case DEV_PM_QOS_RESUME_LATENCY: > ret = blocking_notifier_chain_register(dev->power.qos->resume_latency.notifiers, > notifier); > + break; > + case DEV_PM_QOS_MIN_FREQUENCY: > + ret = blocking_notifier_chain_register(dev->power.qos->min_frequency.notifiers, > + notifier); > + break; > + case DEV_PM_QOS_MAX_FREQUENCY: > + ret = blocking_notifier_chain_register(dev->power.qos->max_frequency.notifiers, > + notifier); > + break; > + default: > + WARN_ON(1); > + ret = -EINVAL; > + } > > +unlock: > mutex_unlock(&dev_pm_qos_mtx); > return ret; > } > @@ -516,20 +573,35 @@ int dev_pm_qos_remove_notifier(struct device *dev, > struct notifier_block *notifier, > enum dev_pm_qos_req_type type) > { > - int retval = 0; > - > - if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY)) > - return -EINVAL; > + int ret = 0; > > mutex_lock(&dev_pm_qos_mtx); > > /* Silently return if the constraints object is not present. */ > - if (!IS_ERR_OR_NULL(dev->power.qos)) > - retval = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers, > - notifier); > + if (IS_ERR_OR_NULL(dev->power.qos)) > + goto unlock; > > + switch (type) { > + case DEV_PM_QOS_RESUME_LATENCY: > + ret = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers, > + notifier); > + break; > + case DEV_PM_QOS_MIN_FREQUENCY: > + ret = blocking_notifier_chain_unregister(dev->power.qos->min_frequency.notifiers, > + notifier); > + break; > + case DEV_PM_QOS_MAX_FREQUENCY: > + ret = blocking_notifier_chain_unregister(dev->power.qos->max_frequency.notifiers, > + notifier); > + break; > + default: > + WARN_ON(1); > + ret = -EINVAL; > + } > + > +unlock: > mutex_unlock(&dev_pm_qos_mtx); > - return retval; > + return ret; > } > EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier); > > @@ -589,6 +661,9 @@ static void __dev_pm_qos_drop_user_request(struct device *dev, > req = dev->power.qos->flags_req; > dev->power.qos->flags_req = NULL; > break; > + default: > + WARN_ON(1); > + return; > } > __dev_pm_qos_remove_request(req); > kfree(req); > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h > index 55814d48c39c..3f994283a5ae 100644 > --- a/include/linux/pm_qos.h > +++ b/include/linux/pm_qos.h > @@ -40,6 +40,8 @@ enum pm_qos_flags_status { > #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY > #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS PM_QOS_LATENCY_ANY_NS > #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 0 > +#define PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE 0 > +#define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE (-1) > #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT (-1) > > #define PM_QOS_FLAG_NO_POWER_OFF (1 << 0) > @@ -58,6 +60,8 @@ struct pm_qos_flags_request { > enum dev_pm_qos_req_type { > DEV_PM_QOS_RESUME_LATENCY = 1, > DEV_PM_QOS_LATENCY_TOLERANCE, > + DEV_PM_QOS_MIN_FREQUENCY, > + DEV_PM_QOS_MAX_FREQUENCY, > DEV_PM_QOS_FLAGS, > }; > > @@ -99,10 +103,14 @@ struct pm_qos_flags { > struct dev_pm_qos { > struct pm_qos_constraints resume_latency; > struct pm_qos_constraints latency_tolerance; > + struct pm_qos_constraints min_frequency; > + struct pm_qos_constraints max_frequency; > struct pm_qos_flags flags; > struct dev_pm_qos_request *resume_latency_req; > struct dev_pm_qos_request *latency_tolerance_req; > struct dev_pm_qos_request *flags_req; > + struct dev_pm_qos_request *min_frequency_req; > + struct dev_pm_qos_request *max_frequency_req; > }; > > /* Action requested to pm_qos_update_target */ > @@ -185,6 +193,12 @@ static inline s32 dev_pm_qos_raw_read_value(struct device *dev, > case DEV_PM_QOS_RESUME_LATENCY: > return IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT > : pm_qos_read_value(&qos->resume_latency); > + case DEV_PM_QOS_MIN_FREQUENCY: > + return IS_ERR_OR_NULL(qos) ? PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE > + : pm_qos_read_value(&qos->min_frequency); > + case DEV_PM_QOS_MAX_FREQUENCY: > + return IS_ERR_OR_NULL(qos) ? PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE > + : pm_qos_read_value(&qos->max_frequency); > default: > WARN_ON(1); > return 0; > @@ -256,6 +270,10 @@ static inline s32 dev_pm_qos_raw_read_value(struct device *dev, > switch type { > case DEV_PM_QOS_RESUME_LATENCY: > return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; > + case DEV_PM_QOS_MIN_FREQUENCY: > + return PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; > + case DEV_PM_QOS_MAX_FREQUENCY: > + return PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; > default: > WARN_ON(1); > return 0; > -- > 2.21.0.rc0.269.g1a574e7a288b >
On Mon, 10 Jun 2019 at 12:52, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > This implements QoS requests to manage userspace configuration of min > and max frequency. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++-------------------- > include/linux/cpufreq.h | 8 +--- > 2 files changed, 47 insertions(+), 53 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 547d221b2ff2..ff754981fcb4 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) > static ssize_t store_##file_name \ > (struct cpufreq_policy *policy, const char *buf, size_t count) \ > { \ > - int ret, temp; \ > - struct cpufreq_policy new_policy; \ > + unsigned long val; \ > + int ret; \ > \ > - memcpy(&new_policy, policy, sizeof(*policy)); \ > - new_policy.min = policy->user_policy.min; \ > - new_policy.max = policy->user_policy.max; \ > - \ > - ret = sscanf(buf, "%u", &new_policy.object); \ > + ret = sscanf(buf, "%lu", &val); \ > if (ret != 1) \ > return -EINVAL; \ > \ > - temp = new_policy.object; \ > - ret = cpufreq_set_policy(policy, &new_policy); \ > - if (!ret) \ > - policy->user_policy.object = temp; \ > - \ > - return ret ? ret : count; \ > + ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\ > + return ret && ret != 1 ? ret : count; \ > } > > store_one(scaling_min_freq, min); > @@ -1133,10 +1125,6 @@ static void cpufreq_update_freq_work(struct work_struct *work) > container_of(work, struct cpufreq_policy, req_work); > struct cpufreq_policy new_policy = *policy; > > - /* We should read constraint values from QoS layer */ > - new_policy.min = 0; > - new_policy.max = UINT_MAX; > - > down_write(&policy->rwsem); > > if (!policy_is_inactive(policy)) > @@ -1243,6 +1231,12 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) > goto err_min_qos_notifier; > } > > + policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req), > + GFP_KERNEL); > + if (!policy->min_freq_req) > + goto err_max_qos_notifier; > + > + policy->max_freq_req = policy->min_freq_req + 1; > INIT_LIST_HEAD(&policy->policy_list); > init_rwsem(&policy->rwsem); > spin_lock_init(&policy->transition_lock); > @@ -1254,6 +1248,9 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) > policy->cpu = cpu; > return policy; > > +err_max_qos_notifier: > + dev_pm_qos_remove_notifier(dev, &policy->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > err_min_qos_notifier: > dev_pm_qos_remove_notifier(dev, &policy->nb_min, > DEV_PM_QOS_MIN_FREQUENCY); > @@ -1289,6 +1286,10 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy) > DEV_PM_QOS_MAX_FREQUENCY); > dev_pm_qos_remove_notifier(dev, &policy->nb_min, > DEV_PM_QOS_MIN_FREQUENCY); > + dev_pm_qos_remove_request(policy->max_freq_req); > + dev_pm_qos_remove_request(policy->min_freq_req); > + kfree(policy->min_freq_req); > + > cpufreq_policy_put_kobj(policy); > free_cpumask_var(policy->real_cpus); > free_cpumask_var(policy->related_cpus); > @@ -1366,16 +1367,30 @@ static int cpufreq_online(unsigned int cpu) > cpumask_and(policy->cpus, policy->cpus, cpu_online_mask); > > if (new_policy) { > - policy->user_policy.min = policy->min; > - policy->user_policy.max = policy->max; > + struct device *dev = get_cpu_device(cpu); > > for_each_cpu(j, policy->related_cpus) { > per_cpu(cpufreq_cpu_data, j) = policy; > add_cpu_dev_symlink(policy, j); > } > - } else { > - policy->min = policy->user_policy.min; > - policy->max = policy->user_policy.max; > + > + ret = dev_pm_qos_add_request(dev, policy->min_freq_req, > + DEV_PM_QOS_MIN_FREQUENCY, > + policy->min); > + if (ret < 0) { > + dev_err(dev, "Failed to add min-freq constraint (%d)\n", > + ret); > + goto out_destroy_policy; > + } > + > + ret = dev_pm_qos_add_request(dev, policy->max_freq_req, > + DEV_PM_QOS_MAX_FREQUENCY, > + policy->max); > + if (ret < 0) { > + dev_err(dev, "Failed to add max-freq constraint (%d)\n", > + ret); > + goto out_destroy_policy; > + } > } > > if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > @@ -2366,7 +2381,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy, > { > struct cpufreq_governor *old_gov; > struct device *cpu_dev = get_cpu_device(policy->cpu); > - unsigned long min, max; > int ret; > > pr_debug("setting new policy for CPU %u: %u - %u kHz\n", > @@ -2374,24 +2388,12 @@ int cpufreq_set_policy(struct cpufreq_policy *policy, > > memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo)); > > - /* > - * This check works well when we store new min/max freq attributes, > - * because new_policy is a copy of policy with one field updated. > - */ > - if (new_policy->min > new_policy->max) > - return -EINVAL; > - > /* > * PM QoS framework collects all the requests from users and provide us > * the final aggregated value here. > */ > - min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY); > - max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY); > - > - if (min > new_policy->min) > - new_policy->min = min; > - if (max < new_policy->max) > - new_policy->max = max; > + new_policy->min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY); > + new_policy->max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY); > > /* verify the cpu speed can be set within this limit */ > ret = cpufreq_driver->verify(new_policy); > @@ -2480,10 +2482,9 @@ int cpufreq_set_policy(struct cpufreq_policy *policy, > * @cpu: CPU to re-evaluate the policy for. > * > * Update the current frequency for the cpufreq policy of @cpu and use > - * cpufreq_set_policy() to re-apply the min and max limits saved in the > - * user_policy sub-structure of that policy, which triggers the evaluation > - * of policy notifiers and the cpufreq driver's ->verify() callback for the > - * policy in question, among other things. > + * cpufreq_set_policy() to re-apply the min and max limits, which triggers the > + * evaluation of policy notifiers and the cpufreq driver's ->verify() callback > + * for the policy in question, among other things. > */ > void cpufreq_update_policy(unsigned int cpu) > { > @@ -2503,8 +2504,6 @@ void cpufreq_update_policy(unsigned int cpu) > > pr_debug("updating policy for CPU %u\n", cpu); > memcpy(&new_policy, policy, sizeof(*policy)); > - new_policy.min = policy->user_policy.min; > - new_policy.max = policy->user_policy.max; > > cpufreq_set_policy(policy, &new_policy); > > @@ -2549,10 +2548,9 @@ static int cpufreq_boost_set_sw(int state) > break; > } > > - down_write(&policy->rwsem); > - policy->user_policy.max = policy->max; > - cpufreq_governor_limits(policy); > - up_write(&policy->rwsem); > + ret = dev_pm_qos_update_request(policy->max_freq_req, policy->max); > + if (ret) > + break; > } > > return ret; > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 0fe7678da9c2..6bbed9af4fd2 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -50,11 +50,6 @@ struct cpufreq_cpuinfo { > unsigned int transition_latency; > }; > > -struct cpufreq_user_policy { > - unsigned int min; /* in kHz */ > - unsigned int max; /* in kHz */ > -}; > - > struct cpufreq_policy { > /* CPUs sharing clock, require sw coordination */ > cpumask_var_t cpus; /* Online CPUs only */ > @@ -85,7 +80,8 @@ struct cpufreq_policy { > * called, but you're in IRQ context */ > struct work_struct req_work; > > - struct cpufreq_user_policy user_policy; > + struct dev_pm_qos_request *min_freq_req; > + struct dev_pm_qos_request *max_freq_req; > struct cpufreq_frequency_table *freq_table; > enum cpufreq_table_sorting freq_table_sorted; > > -- > 2.21.0.rc0.269.g1a574e7a288b >
On Monday, June 10, 2019 12:51:32 PM CEST Viresh Kumar wrote: > In order to use the same set of routines to register notifiers for > different request types, update the existing > dev_pm_qos_{add|remove}_notifier() routines with an additional > parameter: request-type. > > For now, it only supports resume-latency request type. It would be good to mention the broader rationale of this change in its changelog (that is, the frequency limits use case).
On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote: > This registers the notifiers for min/max frequency constraints with the > PM QoS framework. The constraints are also taken into consideration in > cpufreq_set_policy(). > > This also relocates cpufreq_policy_put_kobj() as it is required to be > called from cpufreq_policy_alloc() now. > > No constraints are added until now though. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++------- > include/linux/cpufreq.h | 4 ++ > 2 files changed, 120 insertions(+), 23 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 85ff958e01f1..547d221b2ff2 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -26,6 +26,7 @@ > #include <linux/kernel_stat.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/pm_qos.h> > #include <linux/slab.h> > #include <linux/suspend.h> > #include <linux/syscore_ops.h> > @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work) > cpufreq_update_policy(cpu); > } > > +static void cpufreq_update_freq_work(struct work_struct *work) > +{ > + struct cpufreq_policy *policy = > + container_of(work, struct cpufreq_policy, req_work); > + struct cpufreq_policy new_policy = *policy; > + > + /* We should read constraint values from QoS layer */ > + new_policy.min = 0; > + new_policy.max = UINT_MAX; > + > + down_write(&policy->rwsem); > + > + if (!policy_is_inactive(policy)) > + cpufreq_set_policy(policy, &new_policy); > + > + up_write(&policy->rwsem); > +} > + > +static int cpufreq_update_freq(struct cpufreq_policy *policy) > +{ > + schedule_work(&policy->req_work); > + return 0; > +} > + > +static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq, > + void *data) > +{ > + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min); > + > + return cpufreq_update_freq(policy); > +} > + > +static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq, > + void *data) > +{ > + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max); > + > + return cpufreq_update_freq(policy); > +} This is a bit convoluted. Two different notifiers are registered basically for the same thing. Any chance to use just one?
On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote: > This registers the notifiers for min/max frequency constraints with the > PM QoS framework. The constraints are also taken into consideration in > cpufreq_set_policy(). > > This also relocates cpufreq_policy_put_kobj() as it is required to be > called from cpufreq_policy_alloc() now. > > No constraints are added until now though. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++------- > include/linux/cpufreq.h | 4 ++ > 2 files changed, 120 insertions(+), 23 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 85ff958e01f1..547d221b2ff2 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -26,6 +26,7 @@ > #include <linux/kernel_stat.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/pm_qos.h> > #include <linux/slab.h> > #include <linux/suspend.h> > #include <linux/syscore_ops.h> > @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work) > cpufreq_update_policy(cpu); > } > > +static void cpufreq_update_freq_work(struct work_struct *work) > +{ > + struct cpufreq_policy *policy = > + container_of(work, struct cpufreq_policy, req_work); > + struct cpufreq_policy new_policy = *policy; > + > + /* We should read constraint values from QoS layer */ > + new_policy.min = 0; > + new_policy.max = UINT_MAX; > + > + down_write(&policy->rwsem); > + > + if (!policy_is_inactive(policy)) > + cpufreq_set_policy(policy, &new_policy); > + > + up_write(&policy->rwsem); > +} > + > +static int cpufreq_update_freq(struct cpufreq_policy *policy) > +{ > + schedule_work(&policy->req_work); > + return 0; > +} > + > +static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq, > + void *data) > +{ > + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min); > + > + return cpufreq_update_freq(policy); > +} > + > +static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq, > + void *data) > +{ > + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max); > + > + return cpufreq_update_freq(policy); > +} > + > +static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy) > +{ > + struct kobject *kobj; > + struct completion *cmp; > + > + down_write(&policy->rwsem); > + cpufreq_stats_free_table(policy); > + kobj = &policy->kobj; > + cmp = &policy->kobj_unregister; > + up_write(&policy->rwsem); > + kobject_put(kobj); > + > + /* > + * We need to make sure that the underlying kobj is > + * actually not referenced anymore by anybody before we > + * proceed with unloading. > + */ > + pr_debug("waiting for dropping of refcount\n"); > + wait_for_completion(cmp); > + pr_debug("wait complete\n"); > +} > + > static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) > { > struct cpufreq_policy *policy; > + struct device *dev = get_cpu_device(cpu); > int ret; > > + if (!dev) > + return NULL; > + > policy = kzalloc(sizeof(*policy), GFP_KERNEL); > if (!policy) > return NULL; > @@ -1147,7 +1214,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) > ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq, > cpufreq_global_kobject, "policy%u", cpu); > if (ret) { > - pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret); > + dev_err(dev, "%s: failed to init policy->kobj: %d\n", __func__, ret); > /* > * The entire policy object will be freed below, but the extra > * memory allocated for the kobject name needs to be freed by > @@ -1157,16 +1224,41 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) > goto err_free_real_cpus; > } > > + policy->nb_min.notifier_call = cpufreq_notifier_min; > + policy->nb_max.notifier_call = cpufreq_notifier_max; > + > + ret = dev_pm_qos_add_notifier(dev, &policy->nb_min, > + DEV_PM_QOS_MIN_FREQUENCY); > + if (ret) { > + dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n", > + ret, cpumask_pr_args(policy->cpus)); > + goto err_kobj_remove; > + } > + > + ret = dev_pm_qos_add_notifier(dev, &policy->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > + if (ret) { > + dev_err(dev, "Failed to register MAX QoS notifier: %d (%*pbl)\n", > + ret, cpumask_pr_args(policy->cpus)); > + goto err_min_qos_notifier; > + } > + > INIT_LIST_HEAD(&policy->policy_list); > init_rwsem(&policy->rwsem); > spin_lock_init(&policy->transition_lock); > init_waitqueue_head(&policy->transition_wait); > init_completion(&policy->kobj_unregister); > INIT_WORK(&policy->update, handle_update); > + INIT_WORK(&policy->req_work, cpufreq_update_freq_work); One more thing. handle_update() is very similar to cpufreq_update_freq_work(). Why are both of them needed?
On 18-06-19, 01:37, Rafael J. Wysocki wrote: > One more thing. > > handle_update() is very similar to cpufreq_update_freq_work(). > > Why are both of them needed? I probably did that because of locking required in cpufreq_update_freq_work() but maybe I can do better. Lemme try and come back on it. -- viresh
On Tuesday, June 18, 2019 1:25:22 PM CEST Viresh Kumar wrote: > On 18-06-19, 01:26, Rafael J. Wysocki wrote: > > On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote: > > > +static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq, > > > + void *data) > > > +{ > > > + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min); > > > + > > > + return cpufreq_update_freq(policy); > > > +} > > > + > > > +static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq, > > > + void *data) > > > +{ > > > + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max); > > > + > > > + return cpufreq_update_freq(policy); > > > +} > > > > This is a bit convoluted. > > > > Two different notifiers are registered basically for the same thing. > > > > Any chance to use just one? > > The way QoS is designed, it handles one value only at a time and we need two, > min/max. I thought a lot about it earlier and this is what I came up with :( > > You have any suggestions here ? In patch [3/5] you could point notifiers for both min and max freq to the same notifier head. Both of your notifiers end up calling cpufreq_update_policy() anyway.
On 19-06-19, 00:23, Rafael J. Wysocki wrote: > In patch [3/5] you could point notifiers for both min and max freq to the same > notifier head. Both of your notifiers end up calling cpufreq_update_policy() > anyway. I tried it and the changes in qos.c file look fine. But I don't like at all how cpufreq.c looks now. We only register for min-freq notifier now and that takes care of max as well. What could have been better is if we could have registered a freq-notifier instead of min/max, which isn't possible as well because of how qos framework works. Honestly, the cpufreq changes look hacky to me :( What do you say. -- viresh --- drivers/base/power/qos.c | 15 ++++++++------- drivers/cpufreq/cpufreq.c | 38 ++++++++------------------------------ include/linux/cpufreq.h | 3 +-- 3 files changed, 17 insertions(+), 39 deletions(-) diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c index cde2692b97f9..9bbf2d2a3376 100644 --- a/drivers/base/power/qos.c +++ b/drivers/base/power/qos.c @@ -202,20 +202,20 @@ static int dev_pm_qos_constraints_allocate(struct device *dev) if (!qos) return -ENOMEM; - n = kzalloc(3 * sizeof(*n), GFP_KERNEL); + n = kzalloc(2 * sizeof(*n), GFP_KERNEL); if (!n) { kfree(qos); return -ENOMEM; } + BLOCKING_INIT_NOTIFIER_HEAD(n); c = &qos->resume_latency; plist_head_init(&c->list); c->target_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE; c->default_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE; c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; c->type = PM_QOS_MIN; - c->notifiers = n; - BLOCKING_INIT_NOTIFIER_HEAD(n); + c->notifiers = n++; c = &qos->latency_tolerance; plist_head_init(&c->list); @@ -224,14 +224,16 @@ static int dev_pm_qos_constraints_allocate(struct device *dev) c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT; c->type = PM_QOS_MIN; + /* Same notifier head is used for both min/max frequency */ + BLOCKING_INIT_NOTIFIER_HEAD(n); + c = &qos->min_frequency; plist_head_init(&c->list); c->target_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; c->default_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; c->no_constraint_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE; c->type = PM_QOS_MAX; - c->notifiers = ++n; - BLOCKING_INIT_NOTIFIER_HEAD(n); + c->notifiers = n; c = &qos->max_frequency; plist_head_init(&c->list); @@ -239,8 +241,7 @@ static int dev_pm_qos_constraints_allocate(struct device *dev) c->default_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; c->no_constraint_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; c->type = PM_QOS_MIN; - c->notifiers = ++n; - BLOCKING_INIT_NOTIFIER_HEAD(n); + c->notifiers = n; INIT_LIST_HEAD(&qos->flags.list); diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1344e1b1307f..1605dba1327e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1139,19 +1139,10 @@ static int cpufreq_update_freq(struct cpufreq_policy *policy) return 0; } -static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq, +static int cpufreq_notifier_qos(struct notifier_block *nb, unsigned long freq, void *data) { - struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min); - - return cpufreq_update_freq(policy); -} - -static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq, - void *data) -{ - struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max); + struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_qos); return cpufreq_update_freq(policy); @@ -1214,10 +1205,10 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) goto err_free_real_cpus; } - policy->nb_min.notifier_call = cpufreq_notifier_min; - policy->nb_max.notifier_call = cpufreq_notifier_max; + policy->nb_qos.notifier_call = cpufreq_notifier_qos; - ret = dev_pm_qos_add_notifier(dev, &policy->nb_min, + /* Notifier for min frequency also takes care of max frequency notifier */ + ret = dev_pm_qos_add_notifier(dev, &policy->nb_qos, DEV_PM_QOS_MIN_FREQUENCY); if (ret) { dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n", @@ -1225,18 +1216,10 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) goto err_kobj_remove; } - ret = dev_pm_qos_add_notifier(dev, &policy->nb_max, - DEV_PM_QOS_MAX_FREQUENCY); - if (ret) { - dev_err(dev, "Failed to register MAX QoS notifier: %d (%*pbl)\n", - ret, cpumask_pr_args(policy->cpus)); - goto err_min_qos_notifier; - } - policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req), GFP_KERNEL); if (!policy->min_freq_req) - goto err_max_qos_notifier; + goto err_min_qos_notifier; policy->max_freq_req = policy->min_freq_req + 1; INIT_LIST_HEAD(&policy->policy_list); @@ -1250,11 +1233,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) policy->cpu = cpu; return policy; -err_max_qos_notifier: - dev_pm_qos_remove_notifier(dev, &policy->nb_max, - DEV_PM_QOS_MAX_FREQUENCY); err_min_qos_notifier: - dev_pm_qos_remove_notifier(dev, &policy->nb_min, + dev_pm_qos_remove_notifier(dev, &policy->nb_qos, DEV_PM_QOS_MIN_FREQUENCY); err_kobj_remove: cpufreq_policy_put_kobj(policy); @@ -1284,9 +1264,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy) per_cpu(cpufreq_cpu_data, cpu) = NULL; write_unlock_irqrestore(&cpufreq_driver_lock, flags); - dev_pm_qos_remove_notifier(dev, &policy->nb_max, - DEV_PM_QOS_MAX_FREQUENCY); - dev_pm_qos_remove_notifier(dev, &policy->nb_min, + dev_pm_qos_remove_notifier(dev, &policy->nb_qos, DEV_PM_QOS_MIN_FREQUENCY); cancel_work_sync(&policy->req_work); dev_pm_qos_remove_request(policy->max_freq_req); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 6bbed9af4fd2..2080d6490ed1 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -145,8 +145,7 @@ struct cpufreq_policy { /* Pointer to the cooling device if used for thermal mitigation */ struct thermal_cooling_device *cdev; - struct notifier_block nb_min; - struct notifier_block nb_max; + struct notifier_block nb_qos; }; struct cpufreq_freqs {
On Wed, Jun 19, 2019 at 8:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-06-19, 00:23, Rafael J. Wysocki wrote: > > In patch [3/5] you could point notifiers for both min and max freq to the same > > notifier head. Both of your notifiers end up calling cpufreq_update_policy() > > anyway. > > I tried it and the changes in qos.c file look fine. But I don't like at all how > cpufreq.c looks now. We only register for min-freq notifier now and that takes > care of max as well. What could have been better is if we could have registered > a freq-notifier instead of min/max, which isn't possible as well because of how > qos framework works. > > Honestly, the cpufreq changes look hacky to me :( > > What do you say. OK, leave it as is. That's not a big deal. It is slightly awkward, but oh well.
Hello, This patchset attempts to manage CPU frequency constraints using the PM QoS framework. It only does the basic stuff right now and moves the userspace constraints to use the QoS infrastructure. Todo: - Migrate all users to the QoS framework and get rid of cpufreq specific notifiers. - Make PM QoS learn about the relation of CPUs in a policy, so a single list of constraints is managed for all of them instead of per-cpu constraints. V2->V3: - Add a comment in cpufreq.c as suggested by Qais. - Rebased on latest pm/linux-next. V1->V2: - The previous version introduced a completely new framework, this one moves to PM QoS instead. - Lots of changes because of this. -- viresh Viresh Kumar (5): PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() PM / QOS: Pass request type to dev_pm_qos_read_value() PM / QoS: Add support for MIN/MAX frequency constraints cpufreq: Register notifiers with the PM QoS framework cpufreq: Add QoS requests for userspace constraints Documentation/power/pm_qos_interface.txt | 12 +- drivers/base/power/domain.c | 8 +- drivers/base/power/domain_governor.c | 4 +- drivers/base/power/qos.c | 115 +++++++++++-- drivers/base/power/runtime.c | 2 +- drivers/cpufreq/cpufreq.c | 203 ++++++++++++++++------- drivers/cpuidle/governor.c | 2 +- include/linux/cpufreq.h | 12 +- include/linux/pm_qos.h | 71 ++++++-- 9 files changed, 325 insertions(+), 104 deletions(-) -- 2.21.0.rc0.269.g1a574e7a288b