Messages in this thread | | | Subject | Re: [RFC][PATCH 1/3] PM /devfreq: add user frequency limits into devfreq struct | From | Lukasz Luba <> | Date | Thu, 11 Feb 2021 22:27:19 +0000 |
| |
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
| |