Re: [PATCH] cpufreq/cppc: changing highest_perf to nominal_perf in cppc_cpufreq_cpu_init()

[liwei (JK)]

在 2024/5/3 22:19, Pierre Gondois 写道:
> Hello Liwei,
> The change itself seems ok, but I'm not sure I understand what the
> issue is exactly.
>
> On 4/29/24 12:49, Viresh Kumar wrote:
> > CC'ing few folks who are working with the driver.
> >
> > On 28-04-24, 17:28, liwei wrote:
> > > When turning on turbo, if frequency configuration takes effect slowly,
> > > the updated policy->cur may be equal to the frequency configured in
> > > governor->limits(), performance governor will not adjust the frequency,
> > > configured frequency will remain at turbo-freq.
> > >
> > > Simplified call stack looks as follows:
> > > cpufreq_register_driver(&cppc_cpufreq_driver)
> > >     ...
> > >     cppc_cpufreq_cpu_init()
> > >         cppc_get_perf_caps()
> > >         policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
> > >             cppc_set_perf(highest_perf) // set highest_perf
> > >             policy->cur = cpufreq_driver->get() // if cur == policy->max
>
> During the driver initialization, we have:
> cppc_cpufreq_cpu_init()
> \-policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
> \-policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> \-cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
> \-cppc_set_perf(cpu, &cpu_data->perf_ctrls); // set freq to highest_perf
> so here:
> policy->max = nominal_freq
> policy->cur = highest_freq
>
>
> And then for the cpufreq framework:
> cpufreq_online()
> // IIUC there is some delay here, so policy->cur = nominal_freq ?
> // i.e. the freq. was requested to change to the highest_freq,
> // but the change is not effective yet ?
> \-policy->cur = cpufreq_driver->get(policy->cpu);
> \-cpufreq_init_policy()
>    \-cpufreq_set_policy()
>      \-cpufreq_start_governor()
>        \-cpufreq_verify_current_freq()
>          \-new_freq = cpufreq_driver->get(policy->cpu); // new_freq = 
> nominal_freq ?
>          \-if (policy->cur != new_freq)
>          \-  cpufreq_out_of_sync()
>            \- policy->cur = new_freq;
>      \-cpufreq_start_governor()
>        \-cpufreq_gov_performance_limits()
>          \-__cpufreq_driver_target(target_freq=policy->max) // with 
> policy->max = nominal_freq ?
>            \-if (target_freq == policy->cur)
>            \-  // do nothing
>
> I am not sure I understand when you are turning the turbo on with:
> # echo 1 > /sys/devices/system/cpu/cpufreq/boost
>
> Or do you mean that turbo is available but not turned on ?

Sorry, my description is not clear enough. The scenario described above 
is during the kernel initialization process, turbo is available but 
boost is not turned on.

I found this problem is to read 
/sys/devices/system/cpu/cpufreq/policyX/cpuinfo_cur_freq directly after 
OS startup, and found that some frequencies are in turbo state and 
/sys/devices/system/cpu/cpufreq/boost has not been modified, its value 
is still 0.

LiWei
> Regards,
> Pierre
>
> > >     cpufreq_init_policy()
> > >         ...
> > >         cpufreq_start_governor() // governor: performance
> > >             new_freq = cpufreq_driver->get() // if new_freq == 
> > > policy->max
> > >             if (policy->cur != new_freq)
> > >             cpufreq_out_of_sync(policy, new_freq)
> > >                 ...
> > >                 policy->cur = new_freq
> > >             ...
> > >             policy->governor->limits()
> > >                 __cpufreq_driver_target(policy->max)
> > >                     if (policy->cur==target)
> > >                     // generate error, keep set highest_perf
> > >                         ret
> > >                     cppc_set_perf(target)
> > >
> > > Fix this by changing highest_perf to nominal_perf in 
> > > cppc_cpufreq_cpu_init().
> > >
> > > Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with 
> > > CPPC")
> > > Signed-off-by: liwei <liwei728@xxxxxxxxxx>
> > > ---
> > >   drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cppc_cpufreq.c 
> > > b/drivers/cpufreq/cppc_cpufreq.c
> > > index 64420d9cfd1e..db04a82b8a97 100644
> > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > > @@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct 
> > > cpufreq_policy *policy)
> > >       if (caps->highest_perf > caps->nominal_perf)
> > >           boost_supported = true;
> > > -    /* Set policy->cur to max now. The governors will adjust later. */
> > > -    policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> > > -    cpu_data->perf_ctrls.desired_perf =  caps->highest_perf;
> > > +    /* Set policy->cur to norm now. */
> > > +    policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
> > > +    cpu_data->perf_ctrls.desired_perf =  caps->nominal_perf;
> > >       ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
> > >       if (ret) {
> > >           pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
> > > -             caps->highest_perf, cpu, ret);
> > > +             caps->nominal_perf, cpu, ret);
> > >           goto out;
> > >       }
> > > --
> > > 2.25.1