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

From: Thara Gopinath <thara.gopinath@linaro.org>
To: Steev Klimaszewski <steev@kali.org>,
	rafael@kernel.org, viresh.kumar@linaro.org,
	bjorn.andersson@linaro.org
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.
Date: Tue, 16 Nov 2021 10:31:59 -0500	[thread overview]
Message-ID: <1ceb5a9b-817b-a9ef-c378-be3bd0f7ff17@linaro.org> (raw)
In-Reply-To: <5ae2c644-4743-c62c-b17c-96945a0e6a01@kali.org>

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
> 