Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Mario Limonciello]

On 6/27/2024 09:47, Gautham R.Shenoy wrote:
> Mario Limonciello <mario.limonciello@xxxxxxx> writes:
>
> > On 6/27/2024 00:12, Gautham R.Shenoy wrote:
>
> [..snip..]
> > > > -	return CPPC_HIGHEST_PERF_MAX;
> > > > +	/*
> > > > +	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
> > > > +	 * the highest performance level is set to 196.
> > > > +	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
> > > > +	 */
> > > > +	if (cpu_feature_enabled(X86_FEATURE_ZEN4)) {
> > > > +		switch (c->x86_model) {
> > > > +		case 0x70 ... 0x7f:
> > > > +			return CPPC_HIGHEST_PERF_PERFORMANCE;
> > > > +		default:
> > > > +			return CPPC_HIGHEST_PERF_DEFAULT;
> > >                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > Should this be CPPC_HIGHEST_PERF_MAX ?
> > >
> > > Without this patchset, this function returns 255 on Genoa (0x10-0x1f)
> > > and Bergamo (0xa0-0xaf) systems. This patchset changes the return value
> > > to 166.
> > >
> > > The acpi-cpufreq driver computes the max frequency based on the
> > > boost-ratio, which is the ratio of the highest_perf (returned by this
> > > function) to the nominal_perf.
> > >
> > > So assuming a nominal_freq of 2000Mhz, nominal_perf of 159.
> > >
> > > Previously the max_perf = (2000*255/159) ~ 3200Mhz
> > > With this patch max_perf = (2000*166/159) ~ 2100Mhz.
> > >
> > > Am I missing something ?
> >
> > Yeah; this is exactly what I'm worried about.
> >
> > How does Bergamo handle amd-pstate?  It should probably explode there
> > too.
>
> So amd-pstate driver calls amd_pstate_highest_perf_set() only when
> hw_prefcore is set.
>
> Thus for Genoa and Bergamo, since hw_prefcore is false, the highest_perf
> is extracted from the MSR_AMD_CPPC_CAP1. See this fragment in
> pstate_init_perf()
>
>
> 	/* For platforms that do not support the preferred core feature, the
> 	 * highest_pef may be configured with 166 or 255, to avoid max frequency
> 	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
> 	 * the default max perf.
> 	 */
> 	if (cpudata->hw_prefcore)
> 		highest_perf = amd_pstate_highest_perf_set(cpudata);
> 	else
> 		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
>
> Hence it doesn't blow up on amd-pstate. So it looks like it would be
> better if the prefcore check is in the amd_get_highest_perf() function
> so that it can be invoked from both acpi-cpufreq and amd-pstate drivers.

Ah; yes this makes more sense then.  I'll work on a modified series 
during next kernel cycle.