2021-11-16 00:06:07

by Thara Gopinath

[permalink] [raw]
Subject: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
cpufreq policy. When cpus support boost frequency and if boost is disabled
during boot up (which is the default), cpuinfo.max_freq does not reflect
boost frequency as the maximum supported frequency till boost is explicitly
enabled via sysfs interface later. This also means that policy reports two
different cpuinfo.max_freq before and after turning on boost. Fix this by
separating out setting of policy->max and cpuinfo.max_freq in
cpufreq_frequency_table_cpuinfo.

e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
cluster (cpus 4-7). After boot up (boost disabled),

1. cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
<- This is wrong because boost frequency is

2. echo 1 > /sys/devices/system/cpu/cpufreq/boost <- Enable boost cat
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
max freq reflects boost freq.

3. echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
Discrepancy with step 1 as in both cases boost is disabled.

Note that the other way to fix this is to set cpuinfo.max_freq in Soc
cpufreq driver during initialization. Fixing it in
cpufreq_frequency_table_cpuinfo seems more generic solution

Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/cpufreq/freq_table.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 67e56cf638ef..6784f94124df 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *pos;
unsigned int min_freq = ~0;
unsigned int max_freq = 0;
+ unsigned int cpuinfo_max_freq = 0;
unsigned int freq;

cpufreq_for_each_valid_entry(pos, table) {
freq = pos->frequency;

+ if (freq > cpuinfo_max_freq)
+ cpuinfo_max_freq = freq;
+
if (!cpufreq_boost_enabled()
&& (pos->flags & CPUFREQ_BOOST_FREQ))
continue;
@@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
* If the driver has set its own cpuinfo.max_freq above max_freq, leave
* it as is.
*/
- if (policy->cpuinfo.max_freq < max_freq)
- policy->max = policy->cpuinfo.max_freq = max_freq;
+ if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
+ policy->cpuinfo.max_freq = cpuinfo_max_freq;

if (policy->min == ~0)
return -EINVAL;
--
2.25.1



2021-11-16 04:25:53

by Steev Klimaszewski

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

Hi Thara,

On 11/15/21 1:50 PM, Thara Gopinath wrote:
> cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
> cpufreq policy. When cpus support boost frequency and if boost is disabled
> during boot up (which is the default), cpuinfo.max_freq does not reflect
> boost frequency as the maximum supported frequency till boost is explicitly
> enabled via sysfs interface later. This also means that policy reports two
> different cpuinfo.max_freq before and after turning on boost. Fix this by
> separating out setting of policy->max and cpuinfo.max_freq in
> cpufreq_frequency_table_cpuinfo.
>
> e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
> cluster (cpus 4-7). After boot up (boost disabled),
>
> 1. cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
> <- This is wrong because boost frequency is
>
> 2. echo 1 > /sys/devices/system/cpu/cpufreq/boost <- Enable boost cat
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
> max freq reflects boost freq.
>
> 3. echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
> Discrepancy with step 1 as in both cases boost is disabled.
>
> Note that the other way to fix this is to set cpuinfo.max_freq in Soc
> cpufreq driver during initialization. Fixing it in
> cpufreq_frequency_table_cpuinfo seems more generic solution
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> drivers/cpufreq/freq_table.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index 67e56cf638ef..6784f94124df 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> struct cpufreq_frequency_table *pos;
> unsigned int min_freq = ~0;
> unsigned int max_freq = 0;
> + unsigned int cpuinfo_max_freq = 0;
> unsigned int freq;
>
> cpufreq_for_each_valid_entry(pos, table) {
> freq = pos->frequency;
>
> + if (freq > cpuinfo_max_freq)
> + cpuinfo_max_freq = freq;
> +
> if (!cpufreq_boost_enabled()
> && (pos->flags & CPUFREQ_BOOST_FREQ))
> continue;
> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> * If the driver has set its own cpuinfo.max_freq above max_freq, leave
> * it as is.
> */
> - if (policy->cpuinfo.max_freq < max_freq)
> - policy->max = policy->cpuinfo.max_freq = max_freq;
> + if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
> + policy->cpuinfo.max_freq = cpuinfo_max_freq;
>
> if (policy->min == ~0)
> return -EINVAL;


Something still isn't quite right...

The setup is that I have an rc.local of

#!/bin/sh

echo 1 > /sys/devices/system/cpu/cpufreq/boost

exit 0


After booting and logging in:

steev@limitless:~$ cat
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 2499
<snip>
2649600 38
2745600 31
2841600 1473
2956800 0

After running a "cargo build --release" in an alacritty git checkout:

teev@limitless:~$ cat
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 11220
<snip>
2649600 41
2745600 35
2841600 3065
2956800 0


however...

If I then

steev@limitless:~$ echo 0 | sudo tee /sys/devices/system/cpu/cpufreq/boost
[sudo] password for steev:
0
steev@limitless:~$ echo 1 | sudo tee /sys/devices/system/cpu/cpufreq/boost
1

and run the build again...

steev@limitless:~$ cat
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 21386
<snip>
2649600 45
2745600 38
2841600 3326
2956800 4815

As a workaround, I attempted to jiggle it 1-0-1 in rc.local, however
that ends up giving

steev@limitless:~$ cat
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 2902
<snip>
2649600 36
2745600 36
2841600 6050
2956800 13

And it doesn't go up, I even tried adding a sleep of 1 second between
the echo 1/0/1 lines and while 2956800 goes up to 28 (but never uses it)
it seems like, unless I do it manually once I've logged in, which I'm
assuming is a lot slower than waiting 1 second between them, it's not
quite giving us 2956800 "easily".

If the email wasn't clear, please let me know! I tried to explain as
best I could what I am seeing here.

-- steev


2021-11-16 05:39:49

by Steev Klimaszewski

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.


On 11/15/21 7:23 PM, Steev Klimaszewski wrote:
> Hi Thara,
> <snip>
>
> And it doesn't go up, I even tried adding a sleep of 1 second between
> the echo 1/0/1 lines and while 2956800 goes up to 28 (but never uses
> it) it seems like, unless I do it manually once I've logged in, which
> I'm assuming is a lot slower than waiting 1 second between them, it's
> not quite giving us 2956800 "easily".
>
> If the email wasn't clear, please let me know! I tried to explain as
> best I could what I am seeing here.
>
> -- steev
>
So after more testing... *sometimes* it works, with the jiggle, but not
always.  I'm really not sure why though, so if there's any guidance, I'm
all for it!

-- steev


2021-11-16 05:46:26

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

On 15-11-21, 19:23, Steev Klimaszewski wrote:
> Hi Thara,
>
> On 11/15/21 1:50 PM, Thara Gopinath wrote:
> > cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
> > cpufreq policy. When cpus support boost frequency and if boost is disabled
> > during boot up (which is the default), cpuinfo.max_freq does not reflect
> > boost frequency as the maximum supported frequency till boost is explicitly
> > enabled via sysfs interface later. This also means that policy reports two
> > different cpuinfo.max_freq before and after turning on boost. Fix this by
> > separating out setting of policy->max and cpuinfo.max_freq in
> > cpufreq_frequency_table_cpuinfo.
> >
> > e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
> > cluster (cpus 4-7). After boot up (boost disabled),
> >
> > 1. cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
> > <- This is wrong because boost frequency is
> >
> > 2. echo 1 > /sys/devices/system/cpu/cpufreq/boost <- Enable boost cat
> > /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
> > max freq reflects boost freq.
> >
> > 3. echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
> > /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
> > Discrepancy with step 1 as in both cases boost is disabled.
> >
> > Note that the other way to fix this is to set cpuinfo.max_freq in Soc
> > cpufreq driver during initialization. Fixing it in
> > cpufreq_frequency_table_cpuinfo seems more generic solution
> >
> > Signed-off-by: Thara Gopinath <[email protected]>
> > ---
> > drivers/cpufreq/freq_table.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> > index 67e56cf638ef..6784f94124df 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> > struct cpufreq_frequency_table *pos;
> > unsigned int min_freq = ~0;
> > unsigned int max_freq = 0;
> > + unsigned int cpuinfo_max_freq = 0;
> > unsigned int freq;
> > cpufreq_for_each_valid_entry(pos, table) {
> > freq = pos->frequency;
> > + if (freq > cpuinfo_max_freq)
> > + cpuinfo_max_freq = freq;
> > +
> > if (!cpufreq_boost_enabled()
> > && (pos->flags & CPUFREQ_BOOST_FREQ))
> > continue;
> > @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> > * If the driver has set its own cpuinfo.max_freq above max_freq, leave
> > * it as is.
> > */
> > - if (policy->cpuinfo.max_freq < max_freq)
> > - policy->max = policy->cpuinfo.max_freq = max_freq;
> > + if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
> > + policy->cpuinfo.max_freq = cpuinfo_max_freq;

You need to keep the check of policy->max here and update policy->max,
else you will never run at boost freq. I think this is what Steev
reported as well ?

So basically something like this:

if (policy->max < max_freq)
policy->max = max_freq;

if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
policy->cpuinfo.max_freq = cpuinfo_max_freq;

--
viresh

2021-11-16 15:27:45

by Thara Gopinath

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.



On 11/15/21 10:59 PM, Viresh Kumar wrote:
> On 15-11-21, 19:23, Steev Klimaszewski wrote:
>> Hi Thara,
>>
>> On 11/15/21 1:50 PM, Thara Gopinath wrote:
>>> cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
>>> cpufreq policy. When cpus support boost frequency and if boost is disabled
>>> during boot up (which is the default), cpuinfo.max_freq does not reflect
>>> boost frequency as the maximum supported frequency till boost is explicitly
>>> enabled via sysfs interface later. This also means that policy reports two
>>> different cpuinfo.max_freq before and after turning on boost. Fix this by
>>> separating out setting of policy->max and cpuinfo.max_freq in
>>> cpufreq_frequency_table_cpuinfo.
>>>
>>> e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
>>> cluster (cpus 4-7). After boot up (boost disabled),
>>>
>>> 1. cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
>>> <- This is wrong because boost frequency is
>>>
>>> 2. echo 1 > /sys/devices/system/cpu/cpufreq/boost <- Enable boost cat
>>> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
>>> max freq reflects boost freq.
>>>
>>> 3. echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
>>> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200 <-
>>> Discrepancy with step 1 as in both cases boost is disabled.
>>>
>>> Note that the other way to fix this is to set cpuinfo.max_freq in Soc
>>> cpufreq driver during initialization. Fixing it in
>>> cpufreq_frequency_table_cpuinfo seems more generic solution
>>>
>>> Signed-off-by: Thara Gopinath <[email protected]>
>>> ---
>>> drivers/cpufreq/freq_table.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
>>> index 67e56cf638ef..6784f94124df 100644
>>> --- a/drivers/cpufreq/freq_table.c
>>> +++ b/drivers/cpufreq/freq_table.c
>>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
>>> struct cpufreq_frequency_table *pos;
>>> unsigned int min_freq = ~0;
>>> unsigned int max_freq = 0;
>>> + unsigned int cpuinfo_max_freq = 0;
>>> unsigned int freq;
>>> cpufreq_for_each_valid_entry(pos, table) {
>>> freq = pos->frequency;
>>> + if (freq > cpuinfo_max_freq)
>>> + cpuinfo_max_freq = freq;
>>> +
>>> if (!cpufreq_boost_enabled()
>>> && (pos->flags & CPUFREQ_BOOST_FREQ))
>>> continue;
>>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
>>> * If the driver has set its own cpuinfo.max_freq above max_freq, leave
>>> * it as is.
>>> */
>>> - if (policy->cpuinfo.max_freq < max_freq)
>>> - policy->max = policy->cpuinfo.max_freq = max_freq;
>>> + if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>>> + policy->cpuinfo.max_freq = cpuinfo_max_freq;
>
> You need to keep the check of policy->max here and update policy->max,
> else you will never run at boost freq. I think this is what Steev
> reported as well ?

Hi Viresh,
policy->max is unconditionally set to max_freq in the line before "if
(policy->cpuinfo.max_freq < max_freq)". So this is not the issue Steev
is reporting.
policy->max = max_freq


>
> So basically something like this:
>
> if (policy->max < max_freq)
> policy->max = max_freq;
>
> if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
> policy->cpuinfo.max_freq = cpuinfo_max_freq;
>

--
Warm Regards
Thara (She/Her/Hers)

2021-11-16 15:32:22

by Thara Gopinath

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

Hi Steev,

Thanks for testing this.

On 11/15/21 8:23 PM, Steev Klimaszewski wrote:

--- snip
>>
>> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
>> index 67e56cf638ef..6784f94124df 100644
>> --- a/drivers/cpufreq/freq_table.c
>> +++ b/drivers/cpufreq/freq_table.c
>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct
>> cpufreq_policy *policy,
>>       struct cpufreq_frequency_table *pos;
>>       unsigned int min_freq = ~0;
>>       unsigned int max_freq = 0;
>> +    unsigned int cpuinfo_max_freq = 0;
>>       unsigned int freq;
>>       cpufreq_for_each_valid_entry(pos, table) {
>>           freq = pos->frequency;
>> +        if (freq > cpuinfo_max_freq)
>> +            cpuinfo_max_freq = freq;
>> +
>>           if (!cpufreq_boost_enabled()
>>               && (pos->flags & CPUFREQ_BOOST_FREQ))
>>               continue;
>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct
>> cpufreq_policy *policy,
>>        * If the driver has set its own cpuinfo.max_freq above
>> max_freq, leave
>>        * it as is.
>>        */
>> -    if (policy->cpuinfo.max_freq < max_freq)
>> -        policy->max = policy->cpuinfo.max_freq = max_freq;
>> +    if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>> +        policy->cpuinfo.max_freq = cpuinfo_max_freq;
>>       if (policy->min == ~0)
>>           return -EINVAL;
>
>
> Something still isn't quite right...
>
> The setup is that I have an rc.local of
>
> #!/bin/sh
>
> echo 1 > /sys/devices/system/cpu/cpufreq/boost
>
> exit 0
>
>
> After booting and logging in:
>
> steev@limitless:~$ cat
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 2499
> <snip>
> 2649600 38
> 2745600 31
> 2841600 1473
> 2956800 0

Did you try debugging this ? As in did you read back boost and
cpuinfo_max_freq at this point to ensure that everything is as expected ?


--
Warm Regards
Thara (She/Her/Hers)
>
> After running a "cargo build --release" in an alacritty git checkout:
>
> teev@limitless:~$ cat
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 11220
> <snip>
> 2649600 41
> 2745600 35
> 2841600 3065
> 2956800 0
>
>
> however...
>
> If I then
>
> steev@limitless:~$ echo 0 | sudo tee /sys/devices/system/cpu/cpufreq/boost
> [sudo] password for steev:
> 0
> steev@limitless:~$ echo 1 | sudo tee /sys/devices/system/cpu/cpufreq/boost
> 1
>
> and run the build again...
>
> steev@limitless:~$ cat
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 21386
> <snip>
> 2649600 45
> 2745600 38
> 2841600 3326
> 2956800 4815
>
> As a workaround, I attempted to jiggle it 1-0-1 in rc.local, however
> that ends up giving
>
> steev@limitless:~$ cat
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 2902
> <snip>
> 2649600 36
> 2745600 36
> 2841600 6050
> 2956800 13
>
> And it doesn't go up, I even tried adding a sleep of 1 second between
> the echo 1/0/1 lines and while 2956800 goes up to 28 (but never uses it)
> it seems like, unless I do it manually once I've logged in, which I'm
> assuming is a lot slower than waiting 1 second between them, it's not
> quite giving us 2956800 "easily".
>
> If the email wasn't clear, please let me know! I tried to explain as
> best I could what I am seeing here.
>
> -- steev
>


2021-11-16 16:15:56

by Steev Klimaszewski

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.


On 11/16/21 9:31 AM, Thara Gopinath wrote:
> Hi Steev,
>
> Thanks for testing this.
>
> On 11/15/21 8:23 PM, Steev Klimaszewski wrote:
>
> --- snip
>>>
>>> diff --git a/drivers/cpufreq/freq_table.c
>>> b/drivers/cpufreq/freq_table.c
>>> index 67e56cf638ef..6784f94124df 100644
>>> --- a/drivers/cpufreq/freq_table.c
>>> +++ b/drivers/cpufreq/freq_table.c
>>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct
>>> cpufreq_policy *policy,
>>>       struct cpufreq_frequency_table *pos;
>>>       unsigned int min_freq = ~0;
>>>       unsigned int max_freq = 0;
>>> +    unsigned int cpuinfo_max_freq = 0;
>>>       unsigned int freq;
>>>       cpufreq_for_each_valid_entry(pos, table) {
>>>           freq = pos->frequency;
>>> +        if (freq > cpuinfo_max_freq)
>>> +            cpuinfo_max_freq = freq;
>>> +
>>>           if (!cpufreq_boost_enabled()
>>>               && (pos->flags & CPUFREQ_BOOST_FREQ))
>>>               continue;
>>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct
>>> cpufreq_policy *policy,
>>>        * If the driver has set its own cpuinfo.max_freq above
>>> max_freq, leave
>>>        * it as is.
>>>        */
>>> -    if (policy->cpuinfo.max_freq < max_freq)
>>> -        policy->max = policy->cpuinfo.max_freq = max_freq;
>>> +    if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>>> +        policy->cpuinfo.max_freq = cpuinfo_max_freq;
>>>       if (policy->min == ~0)
>>>           return -EINVAL;
>>
>>
>> Something still isn't quite right...
>>
>> The setup is that I have an rc.local of
>>
>> #!/bin/sh
>>
>> echo 1 > /sys/devices/system/cpu/cpufreq/boost
>>
>> exit 0
>>
>>
>> After booting and logging in:
>>
>> steev@limitless:~$ cat
>> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
>> 825600 2499
>> <snip>
>> 2649600 38
>> 2745600 31
>> 2841600 1473
>> 2956800 0
>
> Did you try debugging this ? As in did you read back boost and
> cpuinfo_max_freq at this point to ensure that everything is as expected ?
>
>
Hi Thara,

I did - sorry I forgot to mention that boost does show 1 for enabled and
cpuinfo_max_freq is set to 2956800.  However, scaling_max_freq is still
listed as 2841600 and scaling_available_frequencies still shows 2841600
as the max available. scaling_boost_freqencies does also list 2956800.

steev@limitless:~$ grep . /sys/devices/system/cpu/cpufreq/policy4/*
/sys/devices/system/cpu/cpufreq/policy4/affected_cpus:4 5 6 7
grep: /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_cur_freq:
Permission denied
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq:2956800
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_transition_latency:0
/sys/devices/system/cpu/cpufreq/policy4/related_cpus:4 5 6 7
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_frequencies:825600
902400 979200 1056000 1209600 1286400 1363200 1459200 1536000 1612800
1689600 1766400 1843200 1920000 1996800 2092800 2169600 2246400 2323200
2400000 2476800 2553600 2649600 2745600 2841600
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_governors:ondemand
conservative powersave userspace performance schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies:2956800
/sys/devices/system/cpu/cpufreq/policy4/scaling_cur_freq:1920000
/sys/devices/system/cpu/cpufreq/policy4/scaling_driver:qcom-cpufreq-hw
/sys/devices/system/cpu/cpufreq/policy4/scaling_governor:schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq:2841600
/sys/devices/system/cpu/cpufreq/policy4/scaling_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/scaling_setspeed:<unsupported>


2021-11-16 16:45:04

by Steev Klimaszewski

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.


On 11/16/21 10:15 AM, Steev Klimaszewski wrote:
>
> On 11/16/21 9:31 AM, Thara Gopinath wrote:
>> Hi Steev,
>>
>> Thanks for testing this.
>>
>> On 11/15/21 8:23 PM, Steev Klimaszewski wrote:
>>
>> --- snip
>>>>
>>>> diff --git a/drivers/cpufreq/freq_table.c
>>>> b/drivers/cpufreq/freq_table.c
>>>> index 67e56cf638ef..6784f94124df 100644
>>>> --- a/drivers/cpufreq/freq_table.c
>>>> +++ b/drivers/cpufreq/freq_table.c
>>>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct
>>>> cpufreq_policy *policy,
>>>>       struct cpufreq_frequency_table *pos;
>>>>       unsigned int min_freq = ~0;
>>>>       unsigned int max_freq = 0;
>>>> +    unsigned int cpuinfo_max_freq = 0;
>>>>       unsigned int freq;
>>>>       cpufreq_for_each_valid_entry(pos, table) {
>>>>           freq = pos->frequency;
>>>> +        if (freq > cpuinfo_max_freq)
>>>> +            cpuinfo_max_freq = freq;
>>>> +
>>>>           if (!cpufreq_boost_enabled()
>>>>               && (pos->flags & CPUFREQ_BOOST_FREQ))
>>>>               continue;
>>>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct
>>>> cpufreq_policy *policy,
>>>>        * If the driver has set its own cpuinfo.max_freq above
>>>> max_freq, leave
>>>>        * it as is.
>>>>        */
>>>> -    if (policy->cpuinfo.max_freq < max_freq)
>>>> -        policy->max = policy->cpuinfo.max_freq = max_freq;
>>>> +    if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>>>> +        policy->cpuinfo.max_freq = cpuinfo_max_freq;
>>>>       if (policy->min == ~0)
>>>>           return -EINVAL;
>>>
>>>
>>> Something still isn't quite right...
>>>
>>> The setup is that I have an rc.local of
>>>
>>> #!/bin/sh
>>>
>>> echo 1 > /sys/devices/system/cpu/cpufreq/boost
>>>
>>> exit 0
>>>
>>>
>>> After booting and logging in:
>>>
>>> steev@limitless:~$ cat
>>> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
>>> 825600 2499
>>> <snip>
>>> 2649600 38
>>> 2745600 31
>>> 2841600 1473
>>> 2956800 0
>>
>> Did you try debugging this ? As in did you read back boost and
>> cpuinfo_max_freq at this point to ensure that everything is as
>> expected ?
>>
>>
> Hi Thara,
>
> I did - sorry I forgot to mention that boost does show 1 for enabled
> and cpuinfo_max_freq is set to 2956800.  However, scaling_max_freq is
> still listed as 2841600 and scaling_available_frequencies still shows
> 2841600 as the max available. scaling_boost_freqencies does also list
> 2956800.
>
> steev@limitless:~$ grep . /sys/devices/system/cpu/cpufreq/policy4/*
> /sys/devices/system/cpu/cpufreq/policy4/affected_cpus:4 5 6 7
> grep: /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_cur_freq:
> Permission denied
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq:2956800
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_min_freq:825600
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_transition_latency:0
> /sys/devices/system/cpu/cpufreq/policy4/related_cpus:4 5 6 7
> /sys/devices/system/cpu/cpufreq/policy4/scaling_available_frequencies:825600
> 902400 979200 1056000 1209600 1286400 1363200 1459200 1536000 1612800
> 1689600 1766400 1843200 1920000 1996800 2092800 2169600 2246400
> 2323200 2400000 2476800 2553600 2649600 2745600 2841600
> /sys/devices/system/cpu/cpufreq/policy4/scaling_available_governors:ondemand
> conservative powersave userspace performance schedutil
> /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies:2956800
> /sys/devices/system/cpu/cpufreq/policy4/scaling_cur_freq:1920000
> /sys/devices/system/cpu/cpufreq/policy4/scaling_driver:qcom-cpufreq-hw
> /sys/devices/system/cpu/cpufreq/policy4/scaling_governor:schedutil
> /sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq:2841600
> /sys/devices/system/cpu/cpufreq/policy4/scaling_min_freq:825600
> /sys/devices/system/cpu/cpufreq/policy4/scaling_setspeed:<unsupported>
>
Once it does start working (e.g. I've run echo 0 to turn off boost, and
then echo 1 to turn it back one)

steev@limitless:~$ grep . /sys/devices/system/cpu/cpufreq/policy4/*
/sys/devices/system/cpu/cpufreq/policy4/affected_cpus:4 5 6 7
grep: /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_cur_freq:
Permission denied
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq:2956800
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_transition_latency:0
/sys/devices/system/cpu/cpufreq/policy4/related_cpus:4 5 6 7
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_frequencies:825600
902400 979200 1056000 1209600 1286400 1363200 1459200 1536000 1612800
1689600 1766400 1843200 1920000 1996800 2092800 2169600 2246400 2323200
2400000 2476800 2553600 2649600 2745600 2841600
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_governors:ondemand
conservative powersave userspace performance schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies:2956800
/sys/devices/system/cpu/cpufreq/policy4/scaling_cur_freq:1920000
/sys/devices/system/cpu/cpufreq/policy4/scaling_driver:qcom-cpufreq-hw
/sys/devices/system/cpu/cpufreq/policy4/scaling_governor:schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq:2956800
/sys/devices/system/cpu/cpufreq/policy4/scaling_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/scaling_setspeed:<unsupported>


Notice that the scaling_max_freq is now 2956800 instead of 2841600 when
it isn't working.

Sorry for forgetting and sending another mail :(

-- steev


2021-11-17 07:24:19

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

On 16-11-21, 10:27, Thara Gopinath wrote:
> policy->max is unconditionally set to max_freq in the line before "if
> (policy->cpuinfo.max_freq < max_freq)".

So the current code is not correct then :)

--
viresh