Re: [Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 11:31 AM Sumit Gupta <sumitg@xxxxxxxxxx> wrote:
>
> From: Srikar Srimath Tirumala <srikars@xxxxxxxxxx>
>
> Add support to configure the CPUFREQ reduction percentage and set the
> maximum number of throttling steps accordingly. Current implementation
> of processor_thermal performs software throttling in fixed steps of
> "20%" which can be too coarse for some platforms. Change that by adding
> new config to provide the reduction percentage.
>
> Signed-off-by: Srikar Srimath Tirumala <srikars@xxxxxxxxxx>
> Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
> ---
>  drivers/acpi/Kconfig             | 15 +++++++++++++++
>  drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
>  2 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 00dd309b6682..287cf58defbf 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -254,6 +254,21 @@ config ACPI_DOCK
>  config ACPI_CPU_FREQ_PSS
>         bool
>
> +config ACPI_CPU_FREQ_THERM_HAS_PARAMS
> +       bool "CPU frequency throttling control"
> +       depends on ACPI_PROCESSOR
> +
> +config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> +       int "Minimum throttle percentage for processor_thermal cooling device"
> +       depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
> +       default 20
> +       help
> +         The processor_thermal driver uses this config to calculate the
> +         percentage amount by which cpu frequency must be reduced for each
> +         cooling state. The config is also used to calculate the maximum number
> +         of throttling steps or cooling states supported by the driver. Value
> +         must be an unsigned integer in the range [1, 50].
> +

I don't think that the new Kconfig symbols are particularly useful.
At least they don't help the distro vendors that each would need to
pick up a specific value for their kernel anyway.

I also wonder how the users building their own kernels are supposed to
determine the values suitable for the target systems.

>  config ACPI_PROCESSOR_CSTATE
>         def_bool y
>         depends on ACPI_PROCESSOR
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index b7c6287eccca..ee443cc69b73 100644
> --- a/drivers/acpi/processor_thermal.c
> +++ b/drivers/acpi/processor_thermal.c
> @@ -25,8 +25,20 @@
>   * _any_ cpufreq driver and not only the acpi-cpufreq driver.
>   */
>
> -#define CPUFREQ_THERMAL_MIN_STEP 0
> -#define CPUFREQ_THERMAL_MAX_STEP 3
> +#define CPUFREQ_THERMAL_MIN_STEP       0
> +#ifdef CONFIG_ACPI_CPU_FREQ_THERM_HAS_PARAMS
> +#define CPUFREQ_THERMAL_PCTG           CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> +
> +/* Derive the MAX_STEP from minimum throttle percentage so that the reduction
> + * percentage does end up becoming negative. Also cap the MAX_STEP so that
> + * the CPU performance doesn't become 0.
> + */
> +#define CPUFREQ_THERMAL_MAX_STEP       ((100 / CPUFREQ_THERMAL_PCTG) - 1)
> +
> +#else
> +#define CPUFREQ_THERMAL_MAX_STEP       3
> +#define CPUFREQ_THERMAL_PCTG           20
> +#endif
>
>  static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
>
> @@ -113,7 +125,8 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
>                 if (!policy)
>                         return -EINVAL;
>
> -               max_freq = (policy->cpuinfo.max_freq * (100 - reduction_pctg(i) * 20)) / 100;
> +               max_freq = (policy->cpuinfo.max_freq *
> +                           (100 - reduction_pctg(i) * CPUFREQ_THERMAL_PCTG)) / 100;
>
>                 cpufreq_cpu_put(policy);
>
> --