| Message ID | 20210126104001.20361-2-lukasz.luba@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | New thermal interface allowing IPA to get max power | expand |
Hi Lukasz,
When accessing the max_freq and min_freq at devfreq-cooling.c,
even if can access 'user_max_freq' and 'lock' by using the 'devfreq' instance,
I think that the direct access of variables (lock/user_max_freq/user_min_freq)
of struct devfreq are not good.
Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER'
notification with following changes of 'struct devfreq_freq'?
Also, need to add codes into devfreq_set_target() for initializing
'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE
notification.
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 147a229056d2..d5726592d362 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -207,6 +207,8 @@ struct devfreq {
struct devfreq_freqs {
unsigned long old;
unsigned long new;
+ unsigned long new_max_freq;
+ unsigned long new_min_freq;
};
And I think that new 'user_min_freq'/'user_max_freq' are not necessary.
You can get the current max_freq/min_freq by using the following steps:
get_freq_range(devfreq, &min_freq, &max_freq);
dev_pm_opp_find_freq_floor(pdev, &min_freq);
dev_pm_opp_find_freq_floor(pdev, &max_freq);
So that you can get the 'max_freq/min_freq' and then
initialize the 'freqs.new_max_freq and freqs.new_min_freq'
with them as following:
in devfreq_set_target()
get_freq_range(devfreq, &min_freq, &max_freq);
dev_pm_opp_find_freq_floor(pdev, &min_freq);
dev_pm_opp_find_freq_floor(pdev, &max_freq);
freqs.new_max_freq = min_freq;
freqs.new_max_freq = max_freq;
devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
On 1/26/21 7:39 PM, Lukasz Luba wrote:
> The new fields inside devfreq struct allow to check the frequency limits
> set by the user via sysfs. These limits are important for thermal governor
> Intelligent Power Allocation (IPA) which needs to know the maximum allowed
> power consumption of the device.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
> drivers/devfreq/devfreq.c | 41 +++++++++++++++++++++++++++++++++++----
> include/linux/devfreq.h | 4 ++++
> 2 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 94cc25fd68da..e985a76e5ff3 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -843,6 +843,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
> goto err_dev;
> }
>
> + devfreq->user_min_freq = devfreq->scaling_min_freq;
> + devfreq->user_max_freq = devfreq->scaling_max_freq;
> +
> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> atomic_set(&devfreq->suspend_count, 0);
>
> @@ -1513,6 +1516,8 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> struct devfreq *df = to_devfreq(dev);
> + struct device *pdev = df->dev.parent;
> + struct dev_pm_opp *opp;
> unsigned long value;
> int ret;
>
> @@ -1533,6 +1538,19 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> if (ret < 0)
> return ret;
>
> + if (!value)
> + value = df->scaling_min_freq;
> +
> + opp = dev_pm_opp_find_freq_ceil(pdev, &value);
> + if (IS_ERR(opp))
> + return count;
> +
> + dev_pm_opp_put(opp);
> +
> + mutex_lock(&df->lock);
> + df->user_min_freq = value;
> + mutex_unlock(&df->lock);
> +
> return count;
> }
>
> @@ -1554,7 +1572,9 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> struct devfreq *df = to_devfreq(dev);
> - unsigned long value;
> + struct device *pdev = df->dev.parent;
> + unsigned long value, value_khz;
> + struct dev_pm_opp *opp;
> int ret;
>
> /*
> @@ -1579,14 +1599,27 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> * A value of zero means "no limit".
> */
> if (value)
> - value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> + value_khz = DIV_ROUND_UP(value, HZ_PER_KHZ);
> else
> - value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
> + value_khz = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
>
> - ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value_khz);
> if (ret < 0)
> return ret;
>
> + if (!value)
> + value = df->scaling_max_freq;
> +
> + opp = dev_pm_opp_find_freq_floor(pdev, &value);
> + if (IS_ERR(opp))
> + return count;
> +
> + dev_pm_opp_put(opp);
> +
> + mutex_lock(&df->lock);
> + df->user_max_freq = value;
> + mutex_unlock(&df->lock);
> +
> return count;
> }
>
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index b6d3bae1c74d..147a229056d2 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -147,6 +147,8 @@ struct devfreq_stats {
> * touch this.
> * @user_min_freq_req: PM QoS minimum frequency request from user (via sysfs)
> * @user_max_freq_req: PM QoS maximum frequency request from user (via sysfs)
> + * @user_min_freq: User's minimum frequency
> + * @user_max_freq: User's maximum frequency
> * @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.
> @@ -185,6 +187,8 @@ struct devfreq {
> struct dev_pm_qos_request user_max_freq_req;
> unsigned long scaling_min_freq;
> unsigned long scaling_max_freq;
> + unsigned long user_min_freq;
> + unsigned long user_max_freq;
> bool stop_polling;
>
> unsigned long suspend_freq;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Chanwoo, Thank you for looking at this. On 2/3/21 10:11 AM, Chanwoo Choi wrote: > Hi Lukasz, > > When accessing the max_freq and min_freq at devfreq-cooling.c, > even if can access 'user_max_freq' and 'lock' by using the 'devfreq' instance, > I think that the direct access of variables (lock/user_max_freq/user_min_freq) > of struct devfreq are not good. > > Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER' > notification with following changes of 'struct devfreq_freq'? I like the idea with devfreq notification. I will have to go through the code to check that possibility. > Also, need to add codes into devfreq_set_target() for initializing > 'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE > notification. > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 147a229056d2..d5726592d362 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -207,6 +207,8 @@ struct devfreq { > struct devfreq_freqs { > unsigned long old; > unsigned long new; > + unsigned long new_max_freq; > + unsigned long new_min_freq; > }; > > > And I think that new 'user_min_freq'/'user_max_freq' are not necessary. > You can get the current max_freq/min_freq by using the following steps: > > get_freq_range(devfreq, &min_freq, &max_freq); > dev_pm_opp_find_freq_floor(pdev, &min_freq); > dev_pm_opp_find_freq_floor(pdev, &max_freq); > > So that you can get the 'max_freq/min_freq' and then > initialize the 'freqs.new_max_freq and freqs.new_min_freq' > with them as following: > > in devfreq_set_target() > get_freq_range(devfreq, &min_freq, &max_freq); > dev_pm_opp_find_freq_floor(pdev, &min_freq); > dev_pm_opp_find_freq_floor(pdev, &max_freq); > freqs.new_max_freq = min_freq; > freqs.new_max_freq = max_freq; > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); I will plumb it in and check that option. My concern is that function get_freq_range() would give me the max_freq value from PM QoS, which might be my thermal limit - lower that user_max_freq. Then I still need I've been playing with PM QoS notifications because I thought it would be possible to be notified in thermal for all new set values - even from devfreq sysfs user max_freq write, which has value higher that the current limit set by thermal governor. Unfortunately PM QoS doesn't send that information by design. PM QoS also by desing won't allow me to check first two limits in the plist - which would be thermal and user sysfs max_freq. I will experiment with this notifications and share the results. That you for your comments. Regards, Lukasz
Hi Chanwoo, On 2/3/21 10:21 AM, Lukasz Luba wrote: > Hi Chanwoo, > > Thank you for looking at this. > > On 2/3/21 10:11 AM, Chanwoo Choi wrote: >> Hi Lukasz, >> >> When accessing the max_freq and min_freq at devfreq-cooling.c, >> even if can access 'user_max_freq' and 'lock' by using the 'devfreq' >> instance, >> I think that the direct access of variables >> (lock/user_max_freq/user_min_freq) >> of struct devfreq are not good. >> >> Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER' >> notification with following changes of 'struct devfreq_freq'? > > I like the idea with devfreq notification. I will have to go through the > code to check that possibility. > >> Also, need to add codes into devfreq_set_target() for initializing >> 'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE >> notification. >> >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index 147a229056d2..d5726592d362 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -207,6 +207,8 @@ struct devfreq { >> struct devfreq_freqs { >> unsigned long old; >> unsigned long new; >> + unsigned long new_max_freq; >> + unsigned long new_min_freq; >> }; >> >> >> And I think that new 'user_min_freq'/'user_max_freq' are not necessary. >> You can get the current max_freq/min_freq by using the following steps: >> >> get_freq_range(devfreq, &min_freq, &max_freq); >> dev_pm_opp_find_freq_floor(pdev, &min_freq); >> dev_pm_opp_find_freq_floor(pdev, &max_freq); >> >> So that you can get the 'max_freq/min_freq' and then >> initialize the 'freqs.new_max_freq and freqs.new_min_freq' >> with them as following: >> >> in devfreq_set_target() >> get_freq_range(devfreq, &min_freq, &max_freq); >> dev_pm_opp_find_freq_floor(pdev, &min_freq); >> dev_pm_opp_find_freq_floor(pdev, &max_freq); >> freqs.new_max_freq = min_freq; >> freqs.new_max_freq = max_freq; >> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > > I will plumb it in and check that option. My concern is that function > get_freq_range() would give me the max_freq value from PM QoS, which > might be my thermal limit - lower that user_max_freq. Then I still > need > > I've been playing with PM QoS notifications because I thought it would > be possible to be notified in thermal for all new set values - even from > devfreq sysfs user max_freq write, which has value higher that the > current limit set by thermal governor. Unfortunately PM QoS doesn't > send that information by design. PM QoS also by desing won't allow > me to check first two limits in the plist - which would be thermal > and user sysfs max_freq. > > I will experiment with this notifications and share the results. > That you for your comments. I have experimented with your proposal. Unfortunately, the value stored in the pm_qos which is read by get_freq_range() is not the user max freq. It's the value from thermal devfreq cooling when that one is lower. Which is OK in the overall design, but not for my IPA use case. What comes to my mind is two options: 1) this patch proposal, with simple solution of two new variables protected by mutex, which would maintain user stored values 2) add a new notification chain in devfreq to notify about new user written value, to which devfreq cooling would register; that would allow devfreq cooling to get that value instantly and store locally What do you think Chanwoo? Regards, Lukasz
On 2/11/21 11:07 AM, Lukasz Luba wrote: > Hi Chanwoo, > > On 2/3/21 10:21 AM, Lukasz Luba wrote: >> Hi Chanwoo, >> >> Thank you for looking at this. >> >> On 2/3/21 10:11 AM, Chanwoo Choi wrote: >>> Hi Lukasz, >>> >>> When accessing the max_freq and min_freq at devfreq-cooling.c, >>> even if can access 'user_max_freq' and 'lock' by using the 'devfreq' >>> instance, >>> I think that the direct access of variables >>> (lock/user_max_freq/user_min_freq) >>> of struct devfreq are not good. >>> >>> Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER' >>> notification with following changes of 'struct devfreq_freq'? >> >> I like the idea with devfreq notification. I will have to go through the >> code to check that possibility. >> >>> Also, need to add codes into devfreq_set_target() for initializing >>> 'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE >>> notification. >>> >>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>> index 147a229056d2..d5726592d362 100644 >>> --- a/include/linux/devfreq.h >>> +++ b/include/linux/devfreq.h >>> @@ -207,6 +207,8 @@ struct devfreq { >>> struct devfreq_freqs { >>> unsigned long old; >>> unsigned long new; >>> + unsigned long new_max_freq; >>> + unsigned long new_min_freq; >>> }; >>> >>> >>> And I think that new 'user_min_freq'/'user_max_freq' are not necessary. >>> You can get the current max_freq/min_freq by using the following steps: >>> >>> get_freq_range(devfreq, &min_freq, &max_freq); >>> dev_pm_opp_find_freq_floor(pdev, &min_freq); >>> dev_pm_opp_find_freq_floor(pdev, &max_freq); >>> >>> So that you can get the 'max_freq/min_freq' and then >>> initialize the 'freqs.new_max_freq and freqs.new_min_freq' >>> with them as following: >>> >>> in devfreq_set_target() >>> get_freq_range(devfreq, &min_freq, &max_freq); >>> dev_pm_opp_find_freq_floor(pdev, &min_freq); >>> dev_pm_opp_find_freq_floor(pdev, &max_freq); >>> freqs.new_max_freq = min_freq; >>> freqs.new_max_freq = max_freq; >>> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> >> I will plumb it in and check that option. My concern is that function >> get_freq_range() would give me the max_freq value from PM QoS, which >> might be my thermal limit - lower that user_max_freq. Then I still >> need >> >> I've been playing with PM QoS notifications because I thought it would >> be possible to be notified in thermal for all new set values - even from >> devfreq sysfs user max_freq write, which has value higher that the >> current limit set by thermal governor. Unfortunately PM QoS doesn't >> send that information by design. PM QoS also by desing won't allow >> me to check first two limits in the plist - which would be thermal >> and user sysfs max_freq. >> >> I will experiment with this notifications and share the results. >> That you for your comments. > > I have experimented with your proposal. Unfortunately, the value stored > in the pm_qos which is read by get_freq_range() is not the user max > freq. It's the value from thermal devfreq cooling when that one is > lower. Which is OK in the overall design, but not for my IPA use case. > > What comes to my mind is two options: > 1) this patch proposal, with simple solution of two new variables > protected by mutex, which would maintain user stored values > 2) add a new notification chain in devfreq to notify about new > user written value, to which devfreq cooling would register; that > would allow devfreq cooling to get that value instantly and store > locally 3) How about new define for existing notification chain: #define DEVFREQ_USER_CHANGE (2) Then a modified devfreq_notify_transition() would get: @@ -339,6 +339,10 @@ static int devfreq_notify_transition(struct devfreq *devfreq, srcu_notifier_call_chain(&devfreq->transition_notifier_list, DEVFREQ_POSTCHANGE, freqs); break; + case DEVFREQ_USER_CHANGE: + srcu_notifier_call_chain(&devfreq->transition_notifier_list, + DEVFREQ_USER_CHANGE, freqs); + break; default: return -EINVAL; } If that is present, I can plumb your suggestion with: struct devfreq_freq { + unsigned long new_max_freq; + unsigned long new_min_freq; and populate them with values in the max_freq_store() by adding at the end: freqs.new_max_freq = max_freq; mutex_lock(); devfreq_notify_transition(devfreq, &freqs, DEVFREQ_USER_CHANGE); mutex_unlock(); I would handle this notification in devfreq cooling and keep the value there, for future IPA checks. If you agree, I can send next version of the patch set. > > What do you think Chanwoo? > > Regards, > Lukasz
Hi Lukasz, On Fri, Feb 12, 2021 at 7:28 AM Lukasz Luba <lukasz.luba@arm.com> wrote: > > > > On 2/11/21 11:07 AM, Lukasz Luba wrote: > > Hi Chanwoo, > > > > On 2/3/21 10:21 AM, Lukasz Luba wrote: > >> Hi Chanwoo, > >> > >> Thank you for looking at this. > >> > >> On 2/3/21 10:11 AM, Chanwoo Choi wrote: > >>> Hi Lukasz, > >>> > >>> When accessing the max_freq and min_freq at devfreq-cooling.c, > >>> even if can access 'user_max_freq' and 'lock' by using the 'devfreq' > >>> instance, > >>> I think that the direct access of variables > >>> (lock/user_max_freq/user_min_freq) > >>> of struct devfreq are not good. > >>> > >>> Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER' > >>> notification with following changes of 'struct devfreq_freq'? > >> > >> I like the idea with devfreq notification. I will have to go through the > >> code to check that possibility. > >> > >>> Also, need to add codes into devfreq_set_target() for initializing > >>> 'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE > >>> notification. > >>> > >>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > >>> index 147a229056d2..d5726592d362 100644 > >>> --- a/include/linux/devfreq.h > >>> +++ b/include/linux/devfreq.h > >>> @@ -207,6 +207,8 @@ struct devfreq { > >>> struct devfreq_freqs { > >>> unsigned long old; > >>> unsigned long new; > >>> + unsigned long new_max_freq; > >>> + unsigned long new_min_freq; > >>> }; > >>> > >>> > >>> And I think that new 'user_min_freq'/'user_max_freq' are not necessary. > >>> You can get the current max_freq/min_freq by using the following steps: > >>> > >>> get_freq_range(devfreq, &min_freq, &max_freq); > >>> dev_pm_opp_find_freq_floor(pdev, &min_freq); > >>> dev_pm_opp_find_freq_floor(pdev, &max_freq); > >>> > >>> So that you can get the 'max_freq/min_freq' and then > >>> initialize the 'freqs.new_max_freq and freqs.new_min_freq' > >>> with them as following: > >>> > >>> in devfreq_set_target() > >>> get_freq_range(devfreq, &min_freq, &max_freq); > >>> dev_pm_opp_find_freq_floor(pdev, &min_freq); > >>> dev_pm_opp_find_freq_floor(pdev, &max_freq); > >>> freqs.new_max_freq = min_freq; > >>> freqs.new_max_freq = max_freq; > >>> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > >> > >> I will plumb it in and check that option. My concern is that function > >> get_freq_range() would give me the max_freq value from PM QoS, which > >> might be my thermal limit - lower that user_max_freq. Then I still > >> need > >> > >> I've been playing with PM QoS notifications because I thought it would > >> be possible to be notified in thermal for all new set values - even from > >> devfreq sysfs user max_freq write, which has value higher that the > >> current limit set by thermal governor. Unfortunately PM QoS doesn't > >> send that information by design. PM QoS also by desing won't allow > >> me to check first two limits in the plist - which would be thermal > >> and user sysfs max_freq. > >> > >> I will experiment with this notifications and share the results. > >> That you for your comments. > > > > I have experimented with your proposal. Unfortunately, the value stored > > in the pm_qos which is read by get_freq_range() is not the user max > > freq. It's the value from thermal devfreq cooling when that one is > > lower. Which is OK in the overall design, but not for my IPA use case. > > > > What comes to my mind is two options: > > 1) this patch proposal, with simple solution of two new variables > > protected by mutex, which would maintain user stored values > > 2) add a new notification chain in devfreq to notify about new > > user written value, to which devfreq cooling would register; that > > would allow devfreq cooling to get that value instantly and store > > locally > > 3) How about new define for existing notification chain: > #define DEVFREQ_USER_CHANGE (2) I think that if we add the notification with specific actor like user change or OPP change or others, it is not proper. But, we can add the notification for min or max frequency change timing. Because the devfreq already has the notification for current frequency like DEVFREQ_PRECHANGE, DEVFREQ_POSTCHANGE. Maybe, we can add the following notification for min/max_freq. The following min_freq and max_freq values will be calculated by get_freq_range(). DEVFREQ_MIN_FREQ_PRECHANGE DEVFREQ_MIN_FREQ_POSTCHANGE DEVFREQ_MAX_FREQ_PRECHANGE DEVFREQ_MAX_FREQ_POSTCHANGE > > Then a modified devfreq_notify_transition() would get: > @@ -339,6 +339,10 @@ static int devfreq_notify_transition(struct devfreq > *devfreq, > > srcu_notifier_call_chain(&devfreq->transition_notifier_list, > DEVFREQ_POSTCHANGE, freqs); > break; > + case DEVFREQ_USER_CHANGE: > + srcu_notifier_call_chain(&devfreq->transition_notifier_list, > + DEVFREQ_USER_CHANGE, freqs); > + break; > default: > return -EINVAL; > } > > If that is present, I can plumb your suggestion with: > struct devfreq_freq { > + unsigned long new_max_freq; > + unsigned long new_min_freq; > > and populate them with values in the max_freq_store() by adding at the > end: > > freqs.new_max_freq = max_freq; > mutex_lock(); > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_USER_CHANGE); > mutex_unlock(); > > I would handle this notification in devfreq cooling and keep the > value there, for future IPA checks. > > If you agree, I can send next version of the patch set. > > > > > What do you think Chanwoo? I thought that your suggestion to expose the user input for min/max_freq. But, these values are not valid for the public user. Actually, the devfreq core handles these values only internally without any explicit access from outside. I'm not sure that it is right or not to expose the internal value of devfreq struct. Until now, I think that it is not proper to show the interval value outside. Because the devfreq subsystem only provides the min_freq and max_freq which reflect the all requirement of user input/cooling policy/OPP instead of user_min_freq, user_max_freq. If we provide the user_min_freq, user_max_freq via DEVFREQ notification, we have to make the new sysfs attributes for user_min_freq and user_max_freq to show the value to the user. But, it seems that it is not nice. Actually, I have no other idea how to support your feature. We try to find the more proper method. -- Best Regards, Chanwoo Choi
Hi Chanwoo, On 2/15/21 3:00 PM, Chanwoo Choi wrote: > Hi Lukasz, > > On Fri, Feb 12, 2021 at 7:28 AM Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> >> >> On 2/11/21 11:07 AM, Lukasz Luba wrote: >>> Hi Chanwoo, >>> >>> On 2/3/21 10:21 AM, Lukasz Luba wrote: >>>> Hi Chanwoo, >>>> >>>> Thank you for looking at this. >>>> >>>> On 2/3/21 10:11 AM, Chanwoo Choi wrote: >>>>> Hi Lukasz, >>>>> >>>>> When accessing the max_freq and min_freq at devfreq-cooling.c, >>>>> even if can access 'user_max_freq' and 'lock' by using the 'devfreq' >>>>> instance, >>>>> I think that the direct access of variables >>>>> (lock/user_max_freq/user_min_freq) >>>>> of struct devfreq are not good. >>>>> >>>>> Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER' >>>>> notification with following changes of 'struct devfreq_freq'? >>>> >>>> I like the idea with devfreq notification. I will have to go through the >>>> code to check that possibility. >>>> >>>>> Also, need to add codes into devfreq_set_target() for initializing >>>>> 'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE >>>>> notification. >>>>> >>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>> index 147a229056d2..d5726592d362 100644 >>>>> --- a/include/linux/devfreq.h >>>>> +++ b/include/linux/devfreq.h >>>>> @@ -207,6 +207,8 @@ struct devfreq { >>>>> struct devfreq_freqs { >>>>> unsigned long old; >>>>> unsigned long new; >>>>> + unsigned long new_max_freq; >>>>> + unsigned long new_min_freq; >>>>> }; >>>>> >>>>> >>>>> And I think that new 'user_min_freq'/'user_max_freq' are not necessary. >>>>> You can get the current max_freq/min_freq by using the following steps: >>>>> >>>>> get_freq_range(devfreq, &min_freq, &max_freq); >>>>> dev_pm_opp_find_freq_floor(pdev, &min_freq); >>>>> dev_pm_opp_find_freq_floor(pdev, &max_freq); >>>>> >>>>> So that you can get the 'max_freq/min_freq' and then >>>>> initialize the 'freqs.new_max_freq and freqs.new_min_freq' >>>>> with them as following: >>>>> >>>>> in devfreq_set_target() >>>>> get_freq_range(devfreq, &min_freq, &max_freq); >>>>> dev_pm_opp_find_freq_floor(pdev, &min_freq); >>>>> dev_pm_opp_find_freq_floor(pdev, &max_freq); >>>>> freqs.new_max_freq = min_freq; >>>>> freqs.new_max_freq = max_freq; >>>>> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >>>> >>>> I will plumb it in and check that option. My concern is that function >>>> get_freq_range() would give me the max_freq value from PM QoS, which >>>> might be my thermal limit - lower that user_max_freq. Then I still >>>> need >>>> >>>> I've been playing with PM QoS notifications because I thought it would >>>> be possible to be notified in thermal for all new set values - even from >>>> devfreq sysfs user max_freq write, which has value higher that the >>>> current limit set by thermal governor. Unfortunately PM QoS doesn't >>>> send that information by design. PM QoS also by desing won't allow >>>> me to check first two limits in the plist - which would be thermal >>>> and user sysfs max_freq. >>>> >>>> I will experiment with this notifications and share the results. >>>> That you for your comments. >>> >>> I have experimented with your proposal. Unfortunately, the value stored >>> in the pm_qos which is read by get_freq_range() is not the user max >>> freq. It's the value from thermal devfreq cooling when that one is >>> lower. Which is OK in the overall design, but not for my IPA use case. >>> >>> What comes to my mind is two options: >>> 1) this patch proposal, with simple solution of two new variables >>> protected by mutex, which would maintain user stored values >>> 2) add a new notification chain in devfreq to notify about new >>> user written value, to which devfreq cooling would register; that >>> would allow devfreq cooling to get that value instantly and store >>> locally >> >> 3) How about new define for existing notification chain: >> #define DEVFREQ_USER_CHANGE (2) > > I think that if we add the notification with specific actor like user change > or OPP change or others, it is not proper. But, we can add the notification > for min or max frequency change timing. Because the devfreq already has > the notification for current frequency like DEVFREQ_PRECHANGE, > DEVFREQ_POSTCHANGE. > > Maybe, we can add the following notification for min/max_freq. > The following min_freq and max_freq values will be calculated by > get_freq_range(). > DEVFREQ_MIN_FREQ_PRECHANGE > DEVFREQ_MIN_FREQ_POSTCHANGE > DEVFREQ_MAX_FREQ_PRECHANGE > DEVFREQ_MAX_FREQ_POSTCHANGE Would it be then possible to pass the user max freq value written via sysfs? Something like in the example below, when writing into max sysfs: 1) starting in max_freq_store() freqs.new_max_freq = max_freq; devfreq_notify_transition(devfreq, &freqs, DEVFREQ_MAX_FREQ_PRECHANGE); dev_pm_qos_update_request() 2)then after a while in devfreq_set_target() get_freq_range(devfreq, &min_freq, &max_freq); dev_pm_opp_find_freq_floor(pdev, &min_freq); dev_pm_opp_find_freq_floor(pdev, &max_freq); freqs.new_min_freq = min_freq; freqs.new_max_freq = max_freq; devfreq_notify_transition(devfreq, &freqs, DEVFREQ_MAX_FREQ_POSTCHANGE); This 2nd part is called after the PM QoS has changed that limit, so might be missing (in case value was higher that current), but thermal would know about that, so no worries. > > >> >> Then a modified devfreq_notify_transition() would get: >> @@ -339,6 +339,10 @@ static int devfreq_notify_transition(struct devfreq >> *devfreq, >> >> srcu_notifier_call_chain(&devfreq->transition_notifier_list, >> DEVFREQ_POSTCHANGE, freqs); >> break; >> + case DEVFREQ_USER_CHANGE: >> + srcu_notifier_call_chain(&devfreq->transition_notifier_list, >> + DEVFREQ_USER_CHANGE, freqs); >> + break; >> default: >> return -EINVAL; >> } >> >> If that is present, I can plumb your suggestion with: >> struct devfreq_freq { >> + unsigned long new_max_freq; >> + unsigned long new_min_freq; >> >> and populate them with values in the max_freq_store() by adding at the >> end: >> >> freqs.new_max_freq = max_freq; >> mutex_lock(); >> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_USER_CHANGE); >> mutex_unlock(); >> >> I would handle this notification in devfreq cooling and keep the >> value there, for future IPA checks. >> >> If you agree, I can send next version of the patch set. >> >>> >>> What do you think Chanwoo? > > I thought that your suggestion to expose the user input for min/max_freq. > But, these values are not valid for the public user. Actually, the devfreq core > handles these values only internally without any explicit access from outside. > > I'm not sure that it is right or not to expose the internal value of > devfreq struct. > Until now, I think that it is not proper to show the interval value outside. > > Because the devfreq subsystem only provides the min_freq and max_freq > which reflect the all requirement of user input/cooling policy/OPP > instead of user_min_freq, user_max_freq. > > If we provide the user_min_freq, user_max_freq via DEVFREQ notification, > we have to make the new sysfs attributes for user_min_freq and user_max_freq > to show the value to the user. But, it seems that it is not nice. I would say we don't have to expose it. Let's take a closer look into an example. The main problem is with GPUs. The middleware is aware of the OPPs in the GPU. If the middleware wants to switch into different power-performance mode e.g. power-saving, it writes into this sysfs the max allowed freq. IPA does not know about it and makes wrong decisions. As you said, the sysfs read operation combines all: user input/cooling policy/OPP, but that's not a problem for this aware middleware. So it can stay as is. The only addition would be this 'notification about user attempt of reducing max device speed' internally inside the kernel, for those subsystems which are interested in it. > > Actually, I have no other idea how to support your feature. > We try to find the more proper method. > Thank you for coming back with your comments. I know it's not an easy feature but I hope we can find a solution. Regards, Lukasz
On 2/16/21 7:41 PM, Lukasz Luba wrote: > Hi Chanwoo, > > On 2/15/21 3:00 PM, Chanwoo Choi wrote: >> Hi Lukasz, >> >> On Fri, Feb 12, 2021 at 7:28 AM Lukasz Luba <lukasz.luba@arm.com> wrote: >>> >>> >>> >>> On 2/11/21 11:07 AM, Lukasz Luba wrote: >>>> Hi Chanwoo, >>>> >>>> On 2/3/21 10:21 AM, Lukasz Luba wrote: >>>>> Hi Chanwoo, >>>>> >>>>> Thank you for looking at this. >>>>> >>>>> On 2/3/21 10:11 AM, Chanwoo Choi wrote: >>>>>> Hi Lukasz, >>>>>> >>>>>> When accessing the max_freq and min_freq at devfreq-cooling.c, >>>>>> even if can access 'user_max_freq' and 'lock' by using the 'devfreq' >>>>>> instance, >>>>>> I think that the direct access of variables >>>>>> (lock/user_max_freq/user_min_freq) >>>>>> of struct devfreq are not good. >>>>>> >>>>>> Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER' >>>>>> notification with following changes of 'struct devfreq_freq'? >>>>> >>>>> I like the idea with devfreq notification. I will have to go through the >>>>> code to check that possibility. >>>>> >>>>>> Also, need to add codes into devfreq_set_target() for initializing >>>>>> 'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE >>>>>> notification. >>>>>> >>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>>> index 147a229056d2..d5726592d362 100644 >>>>>> --- a/include/linux/devfreq.h >>>>>> +++ b/include/linux/devfreq.h >>>>>> @@ -207,6 +207,8 @@ struct devfreq { >>>>>> struct devfreq_freqs { >>>>>> unsigned long old; >>>>>> unsigned long new; >>>>>> + unsigned long new_max_freq; >>>>>> + unsigned long new_min_freq; >>>>>> }; >>>>>> >>>>>> >>>>>> And I think that new 'user_min_freq'/'user_max_freq' are not necessary. >>>>>> You can get the current max_freq/min_freq by using the following steps: >>>>>> >>>>>> get_freq_range(devfreq, &min_freq, &max_freq); >>>>>> dev_pm_opp_find_freq_floor(pdev, &min_freq); >>>>>> dev_pm_opp_find_freq_floor(pdev, &max_freq); >>>>>> >>>>>> So that you can get the 'max_freq/min_freq' and then >>>>>> initialize the 'freqs.new_max_freq and freqs.new_min_freq' >>>>>> with them as following: >>>>>> >>>>>> in devfreq_set_target() >>>>>> get_freq_range(devfreq, &min_freq, &max_freq); >>>>>> dev_pm_opp_find_freq_floor(pdev, &min_freq); >>>>>> dev_pm_opp_find_freq_floor(pdev, &max_freq); >>>>>> freqs.new_max_freq = min_freq; >>>>>> freqs.new_max_freq = max_freq; >>>>>> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >>>>> >>>>> I will plumb it in and check that option. My concern is that function >>>>> get_freq_range() would give me the max_freq value from PM QoS, which >>>>> might be my thermal limit - lower that user_max_freq. Then I still >>>>> need >>>>> >>>>> I've been playing with PM QoS notifications because I thought it would >>>>> be possible to be notified in thermal for all new set values - even from >>>>> devfreq sysfs user max_freq write, which has value higher that the >>>>> current limit set by thermal governor. Unfortunately PM QoS doesn't >>>>> send that information by design. PM QoS also by desing won't allow >>>>> me to check first two limits in the plist - which would be thermal >>>>> and user sysfs max_freq. >>>>> >>>>> I will experiment with this notifications and share the results. >>>>> That you for your comments. >>>> >>>> I have experimented with your proposal. Unfortunately, the value stored >>>> in the pm_qos which is read by get_freq_range() is not the user max >>>> freq. It's the value from thermal devfreq cooling when that one is >>>> lower. Which is OK in the overall design, but not for my IPA use case. >>>> >>>> What comes to my mind is two options: >>>> 1) this patch proposal, with simple solution of two new variables >>>> protected by mutex, which would maintain user stored values >>>> 2) add a new notification chain in devfreq to notify about new >>>> user written value, to which devfreq cooling would register; that >>>> would allow devfreq cooling to get that value instantly and store >>>> locally >>> >>> 3) How about new define for existing notification chain: >>> #define DEVFREQ_USER_CHANGE (2) >> >> I think that if we add the notification with specific actor like user change >> or OPP change or others, it is not proper. But, we can add the notification >> for min or max frequency change timing. Because the devfreq already has >> the notification for current frequency like DEVFREQ_PRECHANGE, >> DEVFREQ_POSTCHANGE. >> >> Maybe, we can add the following notification for min/max_freq. >> The following min_freq and max_freq values will be calculated by >> get_freq_range(). >> DEVFREQ_MIN_FREQ_PRECHANGE >> DEVFREQ_MIN_FREQ_POSTCHANGE >> DEVFREQ_MAX_FREQ_PRECHANGE >> DEVFREQ_MAX_FREQ_POSTCHANGE > > Would it be then possible to pass the user max freq value written via > sysfs? Something like in the example below, when writing into max sysfs: > > 1) starting in max_freq_store() > freqs.new_max_freq = max_freq; > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_MAX_FREQ_PRECHANGE); > dev_pm_qos_update_request() When we use the PRECHANGE and POSTCHANGE notification, we should keep the consistent value. When PRECHANGE, notify the previous min/max frequency containing the user input/cooling policy/OPP. When POSTCHANGE, notify the new min/max frequency containing the user input/cooling policy/OPP. But, in case of your suggestion, DEVFREQ_MAX_FREQ_PRECHANGE considers only user input without cooling policy/opp. > > 2)then after a while in devfreq_set_target() > get_freq_range(devfreq, &min_freq, &max_freq); > dev_pm_opp_find_freq_floor(pdev, &min_freq); > dev_pm_opp_find_freq_floor(pdev, &max_freq); > freqs.new_min_freq = min_freq; > freqs.new_max_freq = max_freq; > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_MAX_FREQ_POSTCHANGE); > > This 2nd part is called after the PM QoS has changed that limit, > so might be missing (in case value was higher that current), > but thermal would know about that, so no worries. It doesn't focus on only thermal. We need to consider all potential user of max_freq notification. In the devfreq subsystem like devfreq governor, we might use the user min/max_freq without any restrictions. But, in this case, devfreq provides the min/max_freq to outside subsystem/drivers like devfreq-cooling.c of thermal. IMHO, it is difficult to agree this approach. If devfreq provides the various min/max_freq value to outside of devfreq, it makes the confusion to understand the meaning of min/max_freq. Actually, the other user doesn't need to know the user input for min/max_freq. > >> >> >>> >>> Then a modified devfreq_notify_transition() would get: >>> @@ -339,6 +339,10 @@ static int devfreq_notify_transition(struct devfreq >>> *devfreq, >>> >>> srcu_notifier_call_chain(&devfreq->transition_notifier_list, >>> DEVFREQ_POSTCHANGE, freqs); >>> break; >>> + case DEVFREQ_USER_CHANGE: >>> + srcu_notifier_call_chain(&devfreq->transition_notifier_list, >>> + DEVFREQ_USER_CHANGE, freqs); >>> + break; >>> default: >>> return -EINVAL; >>> } >>> >>> If that is present, I can plumb your suggestion with: >>> struct devfreq_freq { >>> + unsigned long new_max_freq; >>> + unsigned long new_min_freq; >>> >>> and populate them with values in the max_freq_store() by adding at the >>> end: >>> >>> freqs.new_max_freq = max_freq; >>> mutex_lock(); >>> devfreq_notify_transition(devfreq, &freqs, DEVFREQ_USER_CHANGE); >>> mutex_unlock(); >>> >>> I would handle this notification in devfreq cooling and keep the >>> value there, for future IPA checks. >>> >>> If you agree, I can send next version of the patch set. >>> >>>> >>>> What do you think Chanwoo? >> >> I thought that your suggestion to expose the user input for min/max_freq. >> But, these values are not valid for the public user. Actually, the devfreq core >> handles these values only internally without any explicit access from outside. >> >> I'm not sure that it is right or not to expose the internal value of >> devfreq struct. >> Until now, I think that it is not proper to show the interval value outside. >> >> Because the devfreq subsystem only provides the min_freq and max_freq >> which reflect the all requirement of user input/cooling policy/OPP >> instead of user_min_freq, user_max_freq. >> >> If we provide the user_min_freq, user_max_freq via DEVFREQ notification, >> we have to make the new sysfs attributes for user_min_freq and user_max_freq >> to show the value to the user. But, it seems that it is not nice. > > I would say we don't have to expose it. Let's take a closer look into > an example. The main problem is with GPUs. The middleware is aware of > the OPPs in the GPU. If the middleware wants to switch into different > power-performance mode e.g. power-saving, it writes into this sysfs > the max allowed freq. IPA does not know about it and makes wrong > decisions. As you said, the sysfs read operation combines all: > user input/cooling policy/OPP, but that's not a problem for this aware > middleware. So it can stay as is. > The only addition would be this 'notification about user attempt of > reducing max device speed' internally inside the kernel, for those > subsystems which are interested in it. As I commented on above, I'm not sure to provide the multiple min/max_freq to outside of devfreq subsytem. Instead, it is ok to use user min/max_freq inside of devfreq subsystem. Unfortunately, I didn't suggests the good solution. It is very important changes. So that I want to consider the all users of devfreq. > >> >> Actually, I have no other idea how to support your feature. >> We try to find the more proper method. >> > > Thank you for coming back with your comments. I know it's not > an easy feature but I hope we can find a solution. > > Regards, > Lukasz > > -- Best Regards, Chanwoo Choi Samsung Electronics
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 94cc25fd68da..e985a76e5ff3 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -843,6 +843,9 @@ struct devfreq *devfreq_add_device(struct device *dev, goto err_dev; } + devfreq->user_min_freq = devfreq->scaling_min_freq; + devfreq->user_max_freq = devfreq->scaling_max_freq; + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); atomic_set(&devfreq->suspend_count, 0); @@ -1513,6 +1516,8 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct devfreq *df = to_devfreq(dev); + struct device *pdev = df->dev.parent; + struct dev_pm_opp *opp; unsigned long value; int ret; @@ -1533,6 +1538,19 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, if (ret < 0) return ret; + if (!value) + value = df->scaling_min_freq; + + opp = dev_pm_opp_find_freq_ceil(pdev, &value); + if (IS_ERR(opp)) + return count; + + dev_pm_opp_put(opp); + + mutex_lock(&df->lock); + df->user_min_freq = value; + mutex_unlock(&df->lock); + return count; } @@ -1554,7 +1572,9 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct devfreq *df = to_devfreq(dev); - unsigned long value; + struct device *pdev = df->dev.parent; + unsigned long value, value_khz; + struct dev_pm_opp *opp; int ret; /* @@ -1579,14 +1599,27 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, * A value of zero means "no limit". */ if (value) - value = DIV_ROUND_UP(value, HZ_PER_KHZ); + value_khz = DIV_ROUND_UP(value, HZ_PER_KHZ); else - value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; + value_khz = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE; - ret = dev_pm_qos_update_request(&df->user_max_freq_req, value); + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value_khz); if (ret < 0) return ret; + if (!value) + value = df->scaling_max_freq; + + opp = dev_pm_opp_find_freq_floor(pdev, &value); + if (IS_ERR(opp)) + return count; + + dev_pm_opp_put(opp); + + mutex_lock(&df->lock); + df->user_max_freq = value; + mutex_unlock(&df->lock); + return count; } diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index b6d3bae1c74d..147a229056d2 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -147,6 +147,8 @@ struct devfreq_stats { * touch this. * @user_min_freq_req: PM QoS minimum frequency request from user (via sysfs) * @user_max_freq_req: PM QoS maximum frequency request from user (via sysfs) + * @user_min_freq: User's minimum frequency + * @user_max_freq: User's maximum frequency * @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. @@ -185,6 +187,8 @@ struct devfreq { struct dev_pm_qos_request user_max_freq_req; unsigned long scaling_min_freq; unsigned long scaling_max_freq; + unsigned long user_min_freq; + unsigned long user_max_freq; bool stop_polling; unsigned long suspend_freq;
The new fields inside devfreq struct allow to check the frequency limits set by the user via sysfs. These limits are important for thermal governor Intelligent Power Allocation (IPA) which needs to know the maximum allowed power consumption of the device. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- drivers/devfreq/devfreq.c | 41 +++++++++++++++++++++++++++++++++++---- include/linux/devfreq.h | 4 ++++ 2 files changed, 41 insertions(+), 4 deletions(-)