Re: [RFC][PATCH 1/3] PM /devfreq: add user frequency limits into devfreq struct

[Lukasz Luba <lukasz.luba@arm.com>]

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