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

[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 ?


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