diff mbox

[v3,5/8] PM / devfreq: Get the available next frequency on update_devfreq()

Message ID 1507691364-3899-6-git-send-email-cw00.choi@samsung.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Chanwoo Choi Oct. 11, 2017, 3:09 a.m. UTC
The update_devfreq() considers only user frequency (min_freq/max_freq)
and the next target_freq provided by the governor. But, the commit
a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
OPP as a cooling device. In result, the update_devfreq() have to
consider the 'opp->available' status in order to decicde the next freq
by the devfreq_recommended_opp().

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

MyungJoo Ham Oct. 11, 2017, 11:30 a.m. UTC | #1
> The update_devfreq() considers only user frequency (min_freq/max_freq)
> and the next target_freq provided by the governor. But, the commit
> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
> OPP as a cooling device. In result, the update_devfreq() have to
> consider the 'opp->available' status in order to decicde the next freq
> by the devfreq_recommended_opp().
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1c4b377cacfb..3b9662ffe603 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>  int update_devfreq(struct devfreq *devfreq)
>  {
>  	struct devfreq_freqs freqs;
> +	struct dev_pm_opp *opp;
>  	unsigned long freq, cur_freq;
>  	int err = 0;
>  	u32 flags = 0;
> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>  		return err;
>  
>  	/*
> -	 * Adjust the frequency with user freq and QoS.
> +	 * Adjust the frequency with user freq, QoS and available freq.
>  	 *
>  	 * List from the highest priority
>  	 * max_freq
> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>  		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>  	}
>  
> +	opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +	freq = dev_pm_opp_get_freq(opp);
> +	dev_pm_opp_put(opp);
> +

Is this really necessary?

devfreq_recommended_opp is going to be called by the device driver
invoked by devfreq->profile->target() function anyway.

We are now going to call devfreq_recommended_opp twice in this context.

>  	if (devfreq->profile->get_cur_freq)
>  		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>  	else
> --
Chanwoo Choi Oct. 11, 2017, 1:33 p.m. UTC | #2
On Wed, Oct 11, 2017 at 8:30 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> The update_devfreq() considers only user frequency (min_freq/max_freq)
>> and the next target_freq provided by the governor. But, the commit
>> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
>> OPP as a cooling device. In result, the update_devfreq() have to
>> consider the 'opp->available' status in order to decicde the next freq
>> by the devfreq_recommended_opp().
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 1c4b377cacfb..3b9662ffe603 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>  int update_devfreq(struct devfreq *devfreq)
>>  {
>>       struct devfreq_freqs freqs;
>> +     struct dev_pm_opp *opp;
>>       unsigned long freq, cur_freq;
>>       int err = 0;
>>       u32 flags = 0;
>> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>>               return err;
>>
>>       /*
>> -      * Adjust the frequency with user freq and QoS.
>> +      * Adjust the frequency with user freq, QoS and available freq.
>>        *
>>        * List from the highest priority
>>        * max_freq
>> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>>               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>       }
>>
>> +     opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
>> +     if (IS_ERR(opp))
>> +             return PTR_ERR(opp);
>> +     freq = dev_pm_opp_get_freq(opp);
>> +     dev_pm_opp_put(opp);
>> +
>
> Is this really necessary?

The requirement is due to devfreq_cooling device using
dev_pm_opp_disable/enable().

I added the detailed explanation on cover letter as following:
If this code is not included, the notifiee using TRANSITION_NOTIFIER
receives the wrong next target_freq. On the cpufreq, cpufreq doesn't
use the 'dev_pm_opp_disable/enable()' function and then there is no
the same issue on cpufreq.

[Cover letter's description about this patch]
For example,
- devfreq's min_freq is 100Mhz and max_freq is 700Mhz.
- OPP disabled 500/600/700Mhz due to devfreq-cooling.c.
- simple_ondemand govenor decided the next target_freq (600Mhz)
|----------|-------------------------------------------------------------|
|Freq(MHz) |100     |200    |300    |400     |500     |600      |70 0    |
|Devfreq   |min_freq|       |       |        |        |         |max_freq|
|OPP avail |enabled |enabled|enabled|enabled |Disabled| Disabled|Disabled|
|Ondmenad  |        |       |       |        |        |next_freq|        |
|------------------------------------------------------------------------|

In result,
- Before this patch, target_freq is 600Mhz
  and TRANSITION_NOTIFIER sends the next_freq is 600Mhz to the notifiee.
- After this patch, target_freq is 400Mhz because 500/600 were disabled by OPP.
  and TRANSITION_NOTIFIER sends the next_freq is 400Mhz to the notifiee.
--------------

>
> devfreq_recommended_opp is going to be called by the device driver
> invoked by devfreq->profile->target() function anyway.
>
> We are now going to call devfreq_recommended_opp twice in this context.
>
>>       if (devfreq->profile->get_cur_freq)
>>               devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>>       else
>> --

Right. The devfreq_recommended_opp() is called twice.
I wish there was a better way.
Chanwoo Choi Oct. 13, 2017, 6:45 a.m. UTC | #3
Hi,

On 2017년 10월 11일 22:33, Chanwoo Choi wrote:
> On Wed, Oct 11, 2017 at 8:30 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>> The update_devfreq() considers only user frequency (min_freq/max_freq)
>>> and the next target_freq provided by the governor. But, the commit
>>> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
>>> OPP as a cooling device. In result, the update_devfreq() have to
>>> consider the 'opp->available' status in order to decicde the next freq
>>> by the devfreq_recommended_opp().
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/devfreq.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 1c4b377cacfb..3b9662ffe603 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>>  int update_devfreq(struct devfreq *devfreq)
>>>  {
>>>       struct devfreq_freqs freqs;
>>> +     struct dev_pm_opp *opp;
>>>       unsigned long freq, cur_freq;
>>>       int err = 0;
>>>       u32 flags = 0;
>>> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>>>               return err;
>>>
>>>       /*
>>> -      * Adjust the frequency with user freq and QoS.
>>> +      * Adjust the frequency with user freq, QoS and available freq.
>>>        *
>>>        * List from the highest priority
>>>        * max_freq
>>> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>>>               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>>       }
>>>
>>> +     opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
>>> +     if (IS_ERR(opp))
>>> +             return PTR_ERR(opp);
>>> +     freq = dev_pm_opp_get_freq(opp);
>>> +     dev_pm_opp_put(opp);
>>> +
>>
>> Is this really necessary?
> 
> The requirement is due to devfreq_cooling device using
> dev_pm_opp_disable/enable().

I got the better solution. If struct devfreq contains the 'scaling_min/max_freq'
variable, this issue could be fixed. I'll update it with scaling_min/max_freq'
variables on v4.

> 
> I added the detailed explanation on cover letter as following:
> If this code is not included, the notifiee using TRANSITION_NOTIFIER
> receives the wrong next target_freq. On the cpufreq, cpufreq doesn't
> use the 'dev_pm_opp_disable/enable()' function and then there is no
> the same issue on cpufreq.
> 
> [Cover letter's description about this patch]
> For example,
> - devfreq's min_freq is 100Mhz and max_freq is 700Mhz.
> - OPP disabled 500/600/700Mhz due to devfreq-cooling.c.
> - simple_ondemand govenor decided the next target_freq (600Mhz)
> |----------|-------------------------------------------------------------|
> |Freq(MHz) |100     |200    |300    |400     |500     |600      |70 0    |
> |Devfreq   |min_freq|       |       |        |        |         |max_freq|
> |OPP avail |enabled |enabled|enabled|enabled |Disabled| Disabled|Disabled|
> |Ondmenad  |        |       |       |        |        |next_freq|        |
> |------------------------------------------------------------------------|
> 
> In result,
> - Before this patch, target_freq is 600Mhz
>   and TRANSITION_NOTIFIER sends the next_freq is 600Mhz to the notifiee.
> - After this patch, target_freq is 400Mhz because 500/600 were disabled by OPP.
>   and TRANSITION_NOTIFIER sends the next_freq is 400Mhz to the notifiee.
> --------------
> 
>>
>> devfreq_recommended_opp is going to be called by the device driver
>> invoked by devfreq->profile->target() function anyway.
>>
>> We are now going to call devfreq_recommended_opp twice in this context.
>>
>>>       if (devfreq->profile->get_cur_freq)
>>>               devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>>>       else
>>> --
> 
> Right. The devfreq_recommended_opp() is called twice.
> I wish there was a better way.
>
diff mbox

Patch

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1c4b377cacfb..3b9662ffe603 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -255,6 +255,7 @@  static int devfreq_notify_transition(struct devfreq *devfreq,
 int update_devfreq(struct devfreq *devfreq)
 {
 	struct devfreq_freqs freqs;
+	struct dev_pm_opp *opp;
 	unsigned long freq, cur_freq;
 	int err = 0;
 	u32 flags = 0;
@@ -273,7 +274,7 @@  int update_devfreq(struct devfreq *devfreq)
 		return err;
 
 	/*
-	 * Adjust the frequency with user freq and QoS.
+	 * Adjust the frequency with user freq, QoS and available freq.
 	 *
 	 * List from the highest priority
 	 * max_freq
@@ -289,6 +290,12 @@  int update_devfreq(struct devfreq *devfreq)
 		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
 	}
 
+	opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+	freq = dev_pm_opp_get_freq(opp);
+	dev_pm_opp_put(opp);
+
 	if (devfreq->profile->get_cur_freq)
 		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
 	else