| Message ID | d1a7585539ad2ced2bfcc9e232cf859b1ec9c71a.1560163748.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | cpufreq: Use QoS layer to manage freq-constraints | expand |
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>
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 >
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;
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(-)