Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230629194509.4094455-1-srinivas.pandruvada@linux.intel.com>
In-Reply-To: <20230629194509.4094455-1-srinivas.pandruvada@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 30 Jun 2023 10:59:20 +0200
Message-ID: <CAJZ5v0ju6ffS3yWD97h1r0A1AzxoX+xLiXEN7YhGotD8SJv13Q@mail.gmail.com>
Subject: Re: [PATCH v2] cpufreq: intel_pstate: Fix scaling for hybrid capable
 system with disabled E-cores
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     rafael@kernel.org, lenb@kernel.org, viresh.kumar@linaro.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Thu, Jun 29, 2023 at 9:45 PM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Some system BIOS configuration may provide option to disable E-cores.
> As part of this change, CPUID feature for hybrid (Leaf 7 sub leaf 0,
> EDX[15] = 0) may not be set. But HWP performance limits will still be
> using a scaling factor like any other hybrid enabled system.
>
> The current check for applying scaling factor will fail when hybrid
> CPUID feature is not set. Only way to make sure that scaling should be
> applied by checking CPPC nominal frequency and nominal performance. If
> CPPC nominal frequency and nominal performance is defined and nominal
> frequency is not in multiples of 100MHz of nominal performance, then use
> hybrid scaling factor.
>
> The above check will fail for non hybrid capable systems as they don't
> publish nominal frequency field in CPPC, so this function can be used
> for all HWP systems without additional cpu model check.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v2:
> Compile errors reported by kernel test robot and Rafael for the case
> when CONFIG_ACPI is not defined
>
>  drivers/cpufreq/intel_pstate.c | 58 ++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 2548ec92faa2..7e18999be46a 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -302,6 +302,13 @@ static bool hwp_forced __read_mostly;
>
>  static struct cpufreq_driver *intel_pstate_driver __read_mostly;
>
> +#define HYBRID_SCALING_FACTOR  78741
> +
> +static inline int core_get_scaling(void)
> +{
> +       return 100000;
> +}
> +
>  #ifdef CONFIG_ACPI
>  static bool acpi_ppc;
>  #endif
> @@ -400,6 +407,25 @@ static int intel_pstate_get_cppc_guaranteed(int cpu)
>
>         return cppc_perf.nominal_perf;
>  }
> +
> +static int intel_pstate_cppc_get_scaling(int cpu)
> +{
> +       struct cppc_perf_caps cppc_perf;
> +       int ret;
> +
> +       ret = cppc_get_perf_caps(cpu, &cppc_perf);
> +
> +       /*
> +        * Check if nominal frequency is multiples of 100 MHz, if
> +        * not return hybrid scaling factor.
> +        */
> +       if (!ret && cppc_perf.nominal_perf && cppc_perf.nominal_freq &&
> +           (cppc_perf.nominal_perf * 100 != cppc_perf.nominal_freq))
> +               return HYBRID_SCALING_FACTOR;
> +
> +       return core_get_scaling();
> +}
> +
>  #else /* CONFIG_ACPI_CPPC_LIB */
>  static inline void intel_pstate_set_itmt_prio(int cpu)
>  {
> @@ -492,6 +518,11 @@ static inline int intel_pstate_get_cppc_guaranteed(int cpu)
>  {
>         return -ENOTSUPP;
>  }
> +
> +static int intel_pstate_cppc_get_scaling(int cpu)
> +{
> +       return core_get_scaling();
> +}
>  #endif /* CONFIG_ACPI_CPPC_LIB */
>
>  /**
> @@ -1895,11 +1926,6 @@ static int core_get_turbo_pstate(int cpu)
>         return ret;
>  }
>
> -static inline int core_get_scaling(void)
> -{
> -       return 100000;
> -}
> -
>  static u64 core_get_val(struct cpudata *cpudata, int pstate)
>  {
>         u64 val;
> @@ -1936,16 +1962,29 @@ static void hybrid_get_type(void *data)
>         *cpu_type = get_this_hybrid_cpu_type();
>  }
>
> -static int hybrid_get_cpu_scaling(int cpu)
> +static int hwp_get_cpu_scaling(int cpu)
>  {
>         u8 cpu_type = 0;
>
>         smp_call_function_single(cpu, hybrid_get_type, &cpu_type, 1);
>         /* P-cores have a smaller perf level-to-freqency scaling factor. */
>         if (cpu_type == 0x40)
> -               return 78741;
> +               return HYBRID_SCALING_FACTOR;
>
> -       return core_get_scaling();
> +       /* Use default core scaling for E-cores */
> +       if (cpu_type == 0x20)
> +               return core_get_scaling();
> +
> +       /*
> +        * If reached here, it means that, this system is either non
> +        * hybrid system (like Tiger Lake) or hybrid capable system (like
> +        * Alder Lake or Raptor Lake) with no E cores (CPUID for hybrid
> +        * support is 0).
> +        * All non hybrid systems, don't publish nominal_frequency
> +        * field (means nominal frequency = 0), In that case
> +        * the legacy core scaling is used.
> +        */
> +       return intel_pstate_cppc_get_scaling(cpu);
>  }
>
>  static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate)
> @@ -3393,8 +3432,7 @@ static int __init intel_pstate_init(void)
>                         if (!default_driver)
>                                 default_driver = &intel_pstate;
>
> -                       if (boot_cpu_has(X86_FEATURE_HYBRID_CPU))
> -                               pstate_funcs.get_cpu_scaling = hybrid_get_cpu_scaling;
> +                       pstate_funcs.get_cpu_scaling = hwp_get_cpu_scaling;
>
>                         goto hwp_cpu_matched;
>                 }
> --

Applied as 6.5-rc material with some adjustments.  Please check the
bleeding-edge branch.

Thanks!