Re: [PATCH] cpufreq: intel_pstate: Fix scaling max/min limits with Turbo 3.0

[Rafael J. Wysocki]

On Fri, Jun 15, 2018 at 11:12 PM, Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> When scaling max/min settings are changed, internally they are converted
> to a ratio using the max turbo 1 core turbo frequency. This works fine
> when 1 core max is same irrespective of the core. But under Turbo 3.0,
> this will not be the case. For example:
> Core 0: max turbo pstate: 43 (4.3GHz)
> Core 1: max turbo pstate: 45 (4.5GHz)
> In this case 1 core turbo ratio will be maximum of all, so it will be
> 45 (4.5GHz). Suppose scaling max is set to 4GHz (ratio 40) for all cores
> ,then on core one it will be
>  = max_state * policy->max / max_freq;
>  = 43 * (4000000/4500000) = 38 (3.8GHz)
>  = 38
> which is 200MHz less than the desired.
> On core2, it will be correctly set to ratio 40 (4GHz). Same holds true
> for scaling min frequency limit. So this requires usage of correct turbo
> max frequency for core one, which in this case is 4.3GHz. So we need to
> adjust per CPU cpu->pstate.turbo_freq using the maximum HWP ratio of that
> core.
>
> This change uses the HWP capability of a core to adjust max turbo
> frequency. But since Broadwell HWP doesn't use ratios in the HWP
> capabilities, we have use legacy max 1 core turbo ratio. This is not a
> problem as the HWP capabilities don't differ among cores in Broadwell.
> So we need to check for non Broadwell CPU model for applying this change.
>
> While doing this change also optimize usage of x86_match_cpu(). Currently
> using, different x86_cpu_id structure values for each feature. We can
> consolidate, so that we don't have to introduce one more here. Added HWP
> feature flag to each CPU model, instead of a new definition.

I'd prefer a fix that can be propagated to -stable and then the
optimization as a separate patch.

>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
>  drivers/cpufreq/intel_pstate.c | 43 ++++++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 1de5ec8..26d1fc7 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -308,6 +308,12 @@ static struct global_params global;
>  static DEFINE_MUTEX(intel_pstate_driver_lock);
>  static DEFINE_MUTEX(intel_pstate_limits_lock);
>
> +#define INTEL_PSTATE_HWP_FEATURE_DISABLE_EE    0x01
> +#define INTEL_PSTATE_HWP_PERF_BOOST            0x02
> +#define INTEL_PSTATE_HWP_BROADWELL             0x04
> +
> +static u32 intel_pstate_hwp_features;

Why isn't it a member of strict cpudata?

> +
>  #ifdef CONFIG_ACPI
>
>  static bool intel_pstate_get_ppc_enable_status(void)
> @@ -1413,7 +1419,16 @@ static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
>         cpu->pstate.turbo_pstate = pstate_funcs.get_turbo();
>         cpu->pstate.scaling = pstate_funcs.get_scaling();
>         cpu->pstate.max_freq = cpu->pstate.max_pstate * cpu->pstate.scaling;
> -       cpu->pstate.turbo_freq = cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> +
> +       if (hwp_active &&
> +           !(intel_pstate_hwp_features & INTEL_PSTATE_HWP_BROADWELL)) {
> +               unsigned int phy_max, current_max;
> +
> +               intel_pstate_get_hwp_max(cpu->cpu, &phy_max, &current_max);
> +               cpu->pstate.turbo_freq = phy_max * cpu->pstate.scaling;
> +       } else {
> +               cpu->pstate.turbo_freq = cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> +       }
>
>         if (pstate_funcs.get_aperf_mperf_shift)
>                 cpu->aperf_mperf_shift = pstate_funcs.get_aperf_mperf_shift();
> @@ -1789,14 +1804,16 @@ static const struct x86_cpu_id intel_pstate_cpu_oob_ids[] __initconst = {
>         {}
>  };
>
> -static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[] = {
> -       ICPU(INTEL_FAM6_KABYLAKE_DESKTOP, core_funcs),
> -       {}
> -};
> -
> -static const struct x86_cpu_id intel_pstate_hwp_boost_ids[] = {
> -       ICPU(INTEL_FAM6_SKYLAKE_X, core_funcs),
> -       ICPU(INTEL_FAM6_SKYLAKE_DESKTOP, core_funcs),
> +#define ICPU1(model, feature) \
> +       { X86_VENDOR_INTEL, 6, model, X86_FEATURE_APERFMPERF,\
> +        feature}
> +
> +static const struct x86_cpu_id intel_pstate_hwp_feature_ids[] = {
> +       ICPU1(INTEL_FAM6_BROADWELL_X, INTEL_PSTATE_HWP_BROADWELL),
> +       ICPU1(INTEL_FAM6_BROADWELL_XEON_D, INTEL_PSTATE_HWP_BROADWELL),
> +       ICPU1(INTEL_FAM6_KABYLAKE_DESKTOP, INTEL_PSTATE_HWP_FEATURE_DISABLE_EE),
> +       ICPU1(INTEL_FAM6_SKYLAKE_DESKTOP, INTEL_PSTATE_HWP_PERF_BOOST),
> +       ICPU1(INTEL_FAM6_SKYLAKE_X, INTEL_PSTATE_HWP_PERF_BOOST),
>         {}
>  };
>
> @@ -1825,14 +1842,16 @@ static int intel_pstate_init_cpu(unsigned int cpunum)
>         if (hwp_active) {
>                 const struct x86_cpu_id *id;
>
> -               id = x86_match_cpu(intel_pstate_cpu_ee_disable_ids);
> +               id = x86_match_cpu(intel_pstate_hwp_feature_ids);
>                 if (id)
> +                       intel_pstate_hwp_features = id->driver_data;
> +
> +               if (intel_pstate_hwp_features & INTEL_PSTATE_HWP_FEATURE_DISABLE_EE)
>                         intel_pstate_disable_ee(cpunum);
>
>                 intel_pstate_hwp_enable(cpu);
>
> -               id = x86_match_cpu(intel_pstate_hwp_boost_ids);
> -               if (id)
> +               if (intel_pstate_hwp_features & INTEL_PSTATE_HWP_PERF_BOOST)
>                         hwp_boost = true;
>         }
>
> --
> 2.7.5
>