| Message ID | 00747fe09746282ef4d99ffd2a4e58e592ba4f66.1569272883.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
Hi, On 19. 9. 24. 오전 6:10, Leonard Crestez wrote: > Switch the handling of min_freq and max_freq from sysfs to use the > dev_pm_qos_request interface. > > Since PM QoS handles frequencies as kHz this change reduces the > precision of min_freq and max_freq. This shouldn't introduce problems > because frequencies which are not an integer number of kHz are likely > not an integer number of Hz either. > > Try to ensure compatibility by rounding min values down and rounding > max values up. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++-------------- > include/linux/devfreq.h | 9 +++---- > 2 files changed, 36 insertions(+), 22 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 9887408f23bb..a00737e34d36 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq, > *min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value( > devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY)); > *max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value( > devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY)); > > - /* constraints from sysfs */ > - *min_freq = max(*min_freq, devfreq->min_freq); > - *max_freq = min(*max_freq, devfreq->max_freq); > - > /* constraints from OPP interface */ > *min_freq = max(*min_freq, devfreq->scaling_min_freq); > /* scaling_max_freq can be zero on error */ > if (devfreq->scaling_max_freq) > *max_freq = min(*max_freq, devfreq->scaling_max_freq); > @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev) > DEV_PM_QOS_MIN_FREQUENCY); > > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > + dev_pm_qos_remove_request(&devfreq->user_max_freq_req); > + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); > kfree(devfreq->time_in_state); > kfree(devfreq->trans_table); > mutex_destroy(&devfreq->lock); > kfree(devfreq); > } > @@ -747,18 +745,26 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->scaling_min_freq = find_available_min_freq(devfreq); > if (!devfreq->scaling_min_freq) { > err = -EINVAL; > goto err_dev; > } > - devfreq->min_freq = devfreq->scaling_min_freq; > > devfreq->scaling_max_freq = find_available_max_freq(devfreq); > if (!devfreq->scaling_max_freq) { > err = -EINVAL; > goto err_dev; > } > - devfreq->max_freq = devfreq->scaling_max_freq; > + > + /* PM QoS requests for min/max freq from sysfs */ The comment is important. But the devfreq_add_device() has usually not added the comment for each step. I'm not sure to add the some comments for only this. How about removing it? > + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req, > + DEV_PM_QOS_MIN_FREQUENCY, 0); > + if (err < 0) > + goto err_dev; > + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req, > + DEV_PM_QOS_MAX_FREQUENCY, S32_MAX); > + if (err < 0) > + goto err_dev; > > devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); > atomic_set(&devfreq->suspend_count, 0); > > devfreq->trans_table = kzalloc( > @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev, > err_dev: > /* > * Cleanup path for errors that happen before registration. > * Otherwise we rely on devfreq_dev_release. > */ > + if (dev_pm_qos_request_active(&devfreq->user_max_freq_req)) > + dev_pm_qos_remove_request(&devfreq->user_max_freq_req); > + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) > + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); > kfree(devfreq->time_in_state); > kfree(devfreq->trans_table); > kfree(devfreq); > err_out: > return ERR_PTR(err); > @@ -1387,14 +1397,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > > ret = sscanf(buf, "%lu", &value); > if (ret != 1) > return -EINVAL; > > - mutex_lock(&df->lock); > - df->min_freq = value; > - update_devfreq(df); > - mutex_unlock(&df->lock); > + /* round down to kHz for dev_pm_qos */ Please remove it. > + if (value) > + value = value / HZ_PER_KHZ; It doesn't be necessary. > + > + ret = dev_pm_qos_update_request(&df->user_min_freq_req, value); Change it as following: ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ)); > + if (ret < 0) > + return ret; > > return count; > } > > static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, > @@ -1419,19 +1432,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > > ret = sscanf(buf, "%lu", &value); > if (ret != 1) > return -EINVAL; > > - mutex_lock(&df->lock); > - > - /* Interpret zero as "don't care" */ > - if (!value) > - value = ULONG_MAX; > + /* round up to kHz for dev_pm_qos and interpret zero as "don't care" */ Please remove it. > + if (value) > + value = DIV_ROUND_UP(value, HZ_PER_KHZ); Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)' instead of 'value / HZ_PER_KHZ' in min_freq_store()? > + else > + value = S32_MAX;> > - df->max_freq = value; > - update_devfreq(df); > - mutex_unlock(&df->lock); > + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value); > + if (ret < 0) > + return ret; > > return count; > } > static DEVICE_ATTR_RW(min_freq); > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 8b92ccbd1962..3162eb9b0954 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -11,10 +11,11 @@ > #define __LINUX_DEVFREQ_H__ > > #include <linux/device.h> > #include <linux/notifier.h> > #include <linux/pm_opp.h> > +#include <linux/pm_qos.h> > > #define DEVFREQ_NAME_LEN 16 > > /* DEVFREQ governor name */ > #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand" > @@ -121,12 +122,12 @@ struct devfreq_dev_profile { > * devfreq.nb to the corresponding register notifier call chain. > * @work: delayed work for load monitoring. > * @previous_freq: previously configured frequency value. > * @data: Private data of the governor. The devfreq framework does not > * touch this. > - * @min_freq: Limit minimum frequency requested by user (0: none) > - * @max_freq: Limit maximum frequency requested by user (0: none) > + * @user_min_freq_req: PM QoS min frequency request from user (via sysfs) > + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) I think that 'user' prefix is not needed. You better to change it as following @min_freq_req: PM QoS minimum frequency request by user via sysfs @max_freq_req: PM QoS maximum frequency request by user via sysfs > * @scaling_min_freq: Limit minimum frequency requested by OPP interface > * @scaling_max_freq: Limit maximum frequency requested by OPP interface > * @stop_polling: devfreq polling status of a device. > * @suspend_freq: frequency of a device set during suspend phase. > * @resume_freq: frequency of a device set in resume phase. > @@ -161,12 +162,12 @@ struct devfreq { > unsigned long previous_freq; > struct devfreq_dev_status last_status; > > void *data; /* private data for governors */ > > - unsigned long min_freq; > - unsigned long max_freq; > + struct dev_pm_qos_request user_min_freq_req; > + struct dev_pm_qos_request user_max_freq_req; ditto. > unsigned long scaling_min_freq; > unsigned long scaling_max_freq; > bool stop_polling; > > unsigned long suspend_freq; >
On 2019-09-24 10:22 AM, Chanwoo Choi wrote: > Hi, > > On 19. 9. 24. 오전 6:10, Leonard Crestez wrote: >> Switch the handling of min_freq and max_freq from sysfs to use the >> dev_pm_qos_request interface. >> >> Since PM QoS handles frequencies as kHz this change reduces the >> precision of min_freq and max_freq. This shouldn't introduce problems >> because frequencies which are not an integer number of kHz are likely >> not an integer number of Hz either. >> >> Try to ensure compatibility by rounding min values down and rounding >> max values up. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++-------------- >> include/linux/devfreq.h | 9 +++---- >> 2 files changed, 36 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 9887408f23bb..a00737e34d36 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq, >> *min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value( >> devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY)); >> *max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value( >> devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY)); >> >> - /* constraints from sysfs */ >> - *min_freq = max(*min_freq, devfreq->min_freq); >> - *max_freq = min(*max_freq, devfreq->max_freq); >> - >> /* constraints from OPP interface */ >> *min_freq = max(*min_freq, devfreq->scaling_min_freq); >> /* scaling_max_freq can be zero on error */ >> if (devfreq->scaling_max_freq) >> *max_freq = min(*max_freq, devfreq->scaling_max_freq); >> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev) >> DEV_PM_QOS_MIN_FREQUENCY); >> >> if (devfreq->profile->exit) >> devfreq->profile->exit(devfreq->dev.parent); >> >> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req); >> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); >> kfree(devfreq->time_in_state); >> kfree(devfreq->trans_table); >> mutex_destroy(&devfreq->lock); >> kfree(devfreq); >> } >> @@ -747,18 +745,26 @@ struct devfreq *devfreq_add_device(struct device *dev, >> devfreq->scaling_min_freq = find_available_min_freq(devfreq); >> if (!devfreq->scaling_min_freq) { >> err = -EINVAL; >> goto err_dev; >> } >> - devfreq->min_freq = devfreq->scaling_min_freq; >> >> devfreq->scaling_max_freq = find_available_max_freq(devfreq); >> if (!devfreq->scaling_max_freq) { >> err = -EINVAL; >> goto err_dev; >> } >> - devfreq->max_freq = devfreq->scaling_max_freq; >> + >> + /* PM QoS requests for min/max freq from sysfs */ > > The comment is important. But the devfreq_add_device() has usually > not added the comment for each step. I'm not sure to add the some > comments for only this. How about removing it? OK. >> + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req, >> + DEV_PM_QOS_MIN_FREQUENCY, 0); >> + if (err < 0) >> + goto err_dev; >> + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req, >> + DEV_PM_QOS_MAX_FREQUENCY, S32_MAX); >> + if (err < 0) >> + goto err_dev; >> >> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >> atomic_set(&devfreq->suspend_count, 0); >> >> devfreq->trans_table = kzalloc( >> @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev, >> err_dev: >> /* >> * Cleanup path for errors that happen before registration. >> * Otherwise we rely on devfreq_dev_release. >> */ >> + if (dev_pm_qos_request_active(&devfreq->user_max_freq_req)) >> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req); >> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) >> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); >> kfree(devfreq->time_in_state); >> kfree(devfreq->trans_table); >> kfree(devfreq); >> err_out: >> return ERR_PTR(err); >> @@ -1387,14 +1397,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, >> >> ret = sscanf(buf, "%lu", &value); >> if (ret != 1) >> return -EINVAL; >> >> - mutex_lock(&df->lock); >> - df->min_freq = value; >> - update_devfreq(df); >> - mutex_unlock(&df->lock); >> + /* round down to kHz for dev_pm_qos */ > > Please remove it. > >> + if (value) >> + value = value / HZ_PER_KHZ; > > It doesn't be necessary. > >> + >> + ret = dev_pm_qos_update_request(&df->user_min_freq_req, value); > > Change it as following: > ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ)); OK >> + if (ret < 0) >> + return ret; >> >> return count; >> } >> >> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, >> @@ -1419,19 +1432,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >> >> ret = sscanf(buf, "%lu", &value); >> if (ret != 1) >> return -EINVAL; >> >> - mutex_lock(&df->lock); >> - >> - /* Interpret zero as "don't care" */ >> - if (!value) >> - value = ULONG_MAX; >> + /* round up to kHz for dev_pm_qos and interpret zero as "don't care" */ > > Please remove it. > >> + if (value) >> + value = DIV_ROUND_UP(value, HZ_PER_KHZ); > > Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)' > instead of 'value / HZ_PER_KHZ' in min_freq_store()? If an user request of max=666666666 Hz is converted to max=666666 kHz and then back to 666666000 Hz then the OPP with freq=2/3 gHz might be cut out. I deliberately rounded down for min and up for max to make the constraint interval "inclusive". Does this make sense? It seems like the path least likely to cause issues. >> + else >> + value = S32_MAX; >> - df->max_freq = value; >> - update_devfreq(df); >> - mutex_unlock(&df->lock); >> + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value); >> + if (ret < 0) >> + return ret; >> >> return count; >> } >> static DEVICE_ATTR_RW(min_freq); >> >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index 8b92ccbd1962..3162eb9b0954 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -11,10 +11,11 @@ >> #define __LINUX_DEVFREQ_H__ >> >> #include <linux/device.h> >> #include <linux/notifier.h> >> #include <linux/pm_opp.h> >> +#include <linux/pm_qos.h> >> >> #define DEVFREQ_NAME_LEN 16 >> >> /* DEVFREQ governor name */ >> #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand" >> @@ -121,12 +122,12 @@ struct devfreq_dev_profile { >> * devfreq.nb to the corresponding register notifier call chain. >> * @work: delayed work for load monitoring. >> * @previous_freq: previously configured frequency value. >> * @data: Private data of the governor. The devfreq framework does not >> * touch this. >> - * @min_freq: Limit minimum frequency requested by user (0: none) >> - * @max_freq: Limit maximum frequency requested by user (0: none) >> + * @user_min_freq_req: PM QoS min frequency request from user (via sysfs) >> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) > > I think that 'user' prefix is not needed. You better to change it as following Was requested here: https://patchwork.kernel.org/patch/11149505/#22891887 It makes sense to mention ths request is from userspace via sysfs. Other drivers can also make PM QoS frequency requests for their own reasons.
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 9887408f23bb..a00737e34d36 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq, *min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value( devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY)); *max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value( devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY)); - /* constraints from sysfs */ - *min_freq = max(*min_freq, devfreq->min_freq); - *max_freq = min(*max_freq, devfreq->max_freq); - /* constraints from OPP interface */ *min_freq = max(*min_freq, devfreq->scaling_min_freq); /* scaling_max_freq can be zero on error */ if (devfreq->scaling_max_freq) *max_freq = min(*max_freq, devfreq->scaling_max_freq); @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev) DEV_PM_QOS_MIN_FREQUENCY); if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); + dev_pm_qos_remove_request(&devfreq->user_max_freq_req); + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); kfree(devfreq->time_in_state); kfree(devfreq->trans_table); mutex_destroy(&devfreq->lock); kfree(devfreq); } @@ -747,18 +745,26 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->scaling_min_freq = find_available_min_freq(devfreq); if (!devfreq->scaling_min_freq) { err = -EINVAL; goto err_dev; } - devfreq->min_freq = devfreq->scaling_min_freq; devfreq->scaling_max_freq = find_available_max_freq(devfreq); if (!devfreq->scaling_max_freq) { err = -EINVAL; goto err_dev; } - devfreq->max_freq = devfreq->scaling_max_freq; + + /* PM QoS requests for min/max freq from sysfs */ + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req, + DEV_PM_QOS_MIN_FREQUENCY, 0); + if (err < 0) + goto err_dev; + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req, + DEV_PM_QOS_MAX_FREQUENCY, S32_MAX); + if (err < 0) + goto err_dev; devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); atomic_set(&devfreq->suspend_count, 0); devfreq->trans_table = kzalloc( @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev, err_dev: /* * Cleanup path for errors that happen before registration. * Otherwise we rely on devfreq_dev_release. */ + if (dev_pm_qos_request_active(&devfreq->user_max_freq_req)) + dev_pm_qos_remove_request(&devfreq->user_max_freq_req); + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); kfree(devfreq->time_in_state); kfree(devfreq->trans_table); kfree(devfreq); err_out: return ERR_PTR(err); @@ -1387,14 +1397,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, ret = sscanf(buf, "%lu", &value); if (ret != 1) return -EINVAL; - mutex_lock(&df->lock); - df->min_freq = value; - update_devfreq(df); - mutex_unlock(&df->lock); + /* round down to kHz for dev_pm_qos */ + if (value) + value = value / HZ_PER_KHZ; + + ret = dev_pm_qos_update_request(&df->user_min_freq_req, value); + if (ret < 0) + return ret; return count; } static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, @@ -1419,19 +1432,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, ret = sscanf(buf, "%lu", &value); if (ret != 1) return -EINVAL; - mutex_lock(&df->lock); - - /* Interpret zero as "don't care" */ - if (!value) - value = ULONG_MAX; + /* round up to kHz for dev_pm_qos and interpret zero as "don't care" */ + if (value) + value = DIV_ROUND_UP(value, HZ_PER_KHZ); + else + value = S32_MAX; - df->max_freq = value; - update_devfreq(df); - mutex_unlock(&df->lock); + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value); + if (ret < 0) + return ret; return count; } static DEVICE_ATTR_RW(min_freq); diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 8b92ccbd1962..3162eb9b0954 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -11,10 +11,11 @@ #define __LINUX_DEVFREQ_H__ #include <linux/device.h> #include <linux/notifier.h> #include <linux/pm_opp.h> +#include <linux/pm_qos.h> #define DEVFREQ_NAME_LEN 16 /* DEVFREQ governor name */ #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand" @@ -121,12 +122,12 @@ struct devfreq_dev_profile { * devfreq.nb to the corresponding register notifier call chain. * @work: delayed work for load monitoring. * @previous_freq: previously configured frequency value. * @data: Private data of the governor. The devfreq framework does not * touch this. - * @min_freq: Limit minimum frequency requested by user (0: none) - * @max_freq: Limit maximum frequency requested by user (0: none) + * @user_min_freq_req: PM QoS min frequency request from user (via sysfs) + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) * @scaling_min_freq: Limit minimum frequency requested by OPP interface * @scaling_max_freq: Limit maximum frequency requested by OPP interface * @stop_polling: devfreq polling status of a device. * @suspend_freq: frequency of a device set during suspend phase. * @resume_freq: frequency of a device set in resume phase. @@ -161,12 +162,12 @@ struct devfreq { unsigned long previous_freq; struct devfreq_dev_status last_status; void *data; /* private data for governors */ - unsigned long min_freq; - unsigned long max_freq; + struct dev_pm_qos_request user_min_freq_req; + struct dev_pm_qos_request user_max_freq_req; unsigned long scaling_min_freq; unsigned long scaling_max_freq; bool stop_polling; unsigned long suspend_freq;