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

[Lukasz Luba]

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