lkml.org 
[lkml]   [2021]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC][PATCH 1/3] PM /devfreq: add user frequency limits into devfreq struct
From
Date


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

\
 
 \ /
  Last update: 2021-02-11 23:29    [W:0.107 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site