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

From: Lukasz Luba
Date: Thu Feb 11 2021 - 17:28:39 EST




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