Re: [Patch v3 2/2] ACPI: processor: reduce CPUFREQ thermal reduction pctg for Tegra241

[Sumit Gupta <sumitg@nvidia.com>]

On 06/10/23 21:22, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Fri, Oct 6, 2023 at 5:36 PM Sumit Gupta <sumitg@nvidia.com> wrote:
>>
>> From: Srikar Srimath Tirumala <srikars@nvidia.com>
>>
>> Current implementation of processor_thermal performs software throttling
>> in fixed steps of "20%" which can be too coarse for some platforms.
>> We observed some performance gain after reducing the throttle percentage.
>> Change the CPUFREQ thermal reduction percentage and maximum thermal steps
>> to be configurable. Also, update the default values of both for Nvidia
>> Tegra241 (Grace) SoC. The thermal reduction percentage is reduced to "5%"
>> and accordingly the maximum number of thermal steps are increased as they
>> are derived from the reduction percentage.
>>
>> Signed-off-by: Srikar Srimath Tirumala <srikars@nvidia.com>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/acpi/processor_thermal.c | 43 +++++++++++++++++++++++++++++---
>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
>> index b7c6287eccca..677ba8bc3fbc 100644
>> --- a/drivers/acpi/processor_thermal.c
>> +++ b/drivers/acpi/processor_thermal.c
>> @@ -26,7 +26,16 @@
>>    */
>>
>>   #define CPUFREQ_THERMAL_MIN_STEP 0
>> -#define CPUFREQ_THERMAL_MAX_STEP 3
>> +
>> +static int cpufreq_thermal_max_step __read_mostly = 3;
>> +
>> +/*
>> + * Minimum throttle percentage for processor_thermal cooling device.
>> + * The processor_thermal driver uses it to calculate the percentage amount by
>> + * which cpu frequency must be reduced for each cooling state. This is also used
>> + * to calculate the maximum number of throttling steps or cooling states.
>> + */
>> +static int cpufreq_thermal_pctg __read_mostly = 20;
>>
>>   static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
>>
>> @@ -71,7 +80,7 @@ static int cpufreq_get_max_state(unsigned int cpu)
>>          if (!cpu_has_cpufreq(cpu))
>>                  return 0;
>>
>> -       return CPUFREQ_THERMAL_MAX_STEP;
>> +       return cpufreq_thermal_max_step;
>>   }
>>
>>   static int cpufreq_get_cur_state(unsigned int cpu)
>> @@ -113,7 +122,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);
>>
>> @@ -126,10 +136,37 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
>>          return 0;
>>   }
>>
>> +#ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY
>> +#define SMCCC_SOC_ID_T241      0x036b0241
>> +
>> +static void acpi_thermal_cpufreq_config_nvidia(void)
>> +{
>> +       s32 soc_id = arm_smccc_get_soc_id_version();
>> +
>> +       /* Check JEP106 code for NVIDIA Tegra241 chip (036b:0241) */
>> +       if (soc_id < 0 || soc_id != SMCCC_SOC_ID_T241)
>> +               return;
>> +
>> +       /* Reduce the CPUFREQ Thermal reduction percentage to 5% */
>> +       cpufreq_thermal_pctg = 5;
>> +
>> +       /*
>> +        * Derive the MAX_STEP from minimum throttle percentage so that the reduction
>> +        * percentage doesn't end up becoming negative. Also, cap the MAX_STEP so that
>> +        * the CPU performance doesn't become 0.
>> +        */
>> +       cpufreq_thermal_max_step = (100 / cpufreq_thermal_pctg) - 1;
>> +}
> 
> Looks better now, but one more thing: This is introducing an
> ARM-specific piece of code into an otherwise generic file and there is
> drivers/acpi/arm64/ for ARM-specific code, so I would very much prefer
> this piece of code to go there.
> 
> Of course, it won't be able to modify the static variables directly
> then, but what if instead it defines functions to return the
> appropriate values?
> 
> The variables in question could be initialized with the help of those
> functions then.
> 

Hi Rafael,

Thank you for the review!

Have done the suggested change and sent v4[1].
Please suggest if it looks fine now (or) needs any further change.

[1] https://lore.kernel.org/lkml/20231009171839.12267-1-sumitg@nvidia.com/

Best Regards,
Sumit Gupta

>> +#else
>> +static inline void acpi_thermal_cpufreq_config_nvidia(void) {}
>> +#endif
>> +
>>   void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
>>   {
>>          unsigned int cpu;
>>
>> +       acpi_thermal_cpufreq_config_nvidia();
>> +
>>          for_each_cpu(cpu, policy->related_cpus) {
>>                  struct acpi_processor *pr = per_cpu(processors, cpu);
>>                  int ret;
>> --
>> 2.17.1
>>