diff mbox series

[5/8] x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()

Message ID 20240826211358.2694603-6-superm1@kernel.org (mailing list archive)
State Superseded, archived
Delegated to: Mario Limonciello
Headers show
Series Adjustments for preferred core detection | expand

Commit Message

Mario Limonciello Aug. 26, 2024, 9:13 p.m. UTC
From: Mario Limonciello <mario.limonciello@amd.com>

AMD systems that support preferred cores will use "166" as their
numerator for max frequency calculations instead of "255".

Add a function for detecting preferred cores by looking at the
highest perf value on all cores.

If preferred cores are enabled return 166 and if disabled the
value in the highest perf register. As the function will be called
multiple times, cache the values for the boost numerator and if
preferred cores will be enabled in global variables.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/acpi/cppc.c  | 89 ++++++++++++++++++++++++++++++++----
 drivers/cpufreq/amd-pstate.c | 34 ++++++--------
 include/acpi/cppc_acpi.h     |  2 +
 3 files changed, 94 insertions(+), 31 deletions(-)

Comments

Gautham R. Shenoy Aug. 27, 2024, 3:43 p.m. UTC | #1
On Mon, Aug 26, 2024 at 04:13:55PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> AMD systems that support preferred cores will use "166" as their
> numerator for max frequency calculations instead of "255".
> 
> Add a function for detecting preferred cores by looking at the
> highest perf value on all cores.
> 
> If preferred cores are enabled return 166 and if disabled the
> value in the highest perf register. As the function will be called
> multiple times, cache the values for the boost numerator and if
> preferred cores will be enabled in global variables.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---

[..snip..]

>  /**
>   * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
>   * @cpu: CPU to get numerator for.
> @@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>   */
>  int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>  {
> -	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	bool prefcore;
> +	int ret;
>  
> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
> -		*numerator = 166;
> -		return 0;
> -	}
> +	ret = amd_detect_prefcore(&prefcore);
> +	if (ret)
> +		return ret;
>  
> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
> -		*numerator = 166;
> +	/* without preferred cores, return the highest perf register value */
> +	if (!prefcore) {
> +		*numerator = boost_numerator;
>  		return 0;
>  	}
> -	*numerator = 255;
> +	*numerator = CPPC_HIGHEST_PERF_PREFCORE;


Interesting. So even when the user boots a system that supports
preferred-cores with "amd_preferred=disable",
amd_get_boost_ratio_numerator() will return CPPC_HIGHEST_PERF_PREFCORE
as the call prefcore == true here.

I suppose that is as intended, since even though the user may not want
to use the preferred core logic for task-scheduling/load-balancing,
the numerator for the boost-ratio is purely dependent on the h/w
capability.

Is this understanding correct? If so, can this be included as a
comment in the code ?

The rest of the patch looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.



>  
>  	return 0;
>  }
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f470b5700db58..ec32c830abc1d 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
>  
>  static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>  {
> -	int ret, prio;
> -	u32 highest_perf;
> -
> -	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
> -	if (ret)
> +	/* user disabled or not detected */
> +	if (!amd_pstate_prefcore)
>  		return;
>  
>  	cpudata->hw_prefcore = true;
> -	/* check if CPPC preferred core feature is enabled*/
> -	if (highest_perf < CPPC_MAX_PERF)
> -		prio = (int)highest_perf;
> -	else {
> -		pr_debug("AMD CPPC preferred core is unsupported!\n");
> -		cpudata->hw_prefcore = false;
> -		return;
> -	}
> -
> -	if (!amd_pstate_prefcore)
> -		return;
>  
>  	/*
>  	 * The priorities can be set regardless of whether or not
>  	 * sched_set_itmt_support(true) has been called and it is valid to
>  	 * update them at any time after it has been called.
>  	 */
> -	sched_set_itmt_core_prio(prio, cpudata->cpu);
> +	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->highest_perf), cpudata->cpu);
>  
>  	schedule_work(&sched_prefcore_work);
>  }
> @@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>  
>  	cpudata->cpu = policy->cpu;
>  
> -	amd_pstate_init_prefcore(cpudata);
> -
>  	ret = amd_pstate_init_perf(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
>  
> +	amd_pstate_init_prefcore(cpudata);
> +
>  	ret = amd_pstate_init_freq(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
> @@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	cpudata->cpu = policy->cpu;
>  	cpudata->epp_policy = 0;
>  
> -	amd_pstate_init_prefcore(cpudata);
> -
>  	ret = amd_pstate_init_perf(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
>  
> +	amd_pstate_init_prefcore(cpudata);
> +
>  	ret = amd_pstate_init_freq(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
> @@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)
>  		static_call_update(amd_pstate_update_perf, cppc_update_perf);
>  	}
>  
> +	if (amd_pstate_prefcore) {
> +		ret = amd_detect_prefcore(&amd_pstate_prefcore);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* enable amd pstate feature */
>  	ret = amd_pstate_enable(true);
>  	if (ret) {
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 2246ce0630362..1d79320a23490 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,10 +137,12 @@ struct cppc_cpudata {
>  };
>  
>  #ifdef CONFIG_CPU_SUP_AMD
> +extern int amd_detect_prefcore(bool *detected);
>  extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>  extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>  #else /* !CONFIG_CPU_SUP_AMD */
>  static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
> +static inline int amd_detect_prefcore(bool *detected) { return -ENODEV; }
>  static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
>  #endif /* !CONFIG_CPU_SUP_AMD */
>  
> -- 
> 2.43.0
>
Mario Limonciello Aug. 27, 2024, 7 p.m. UTC | #2
On 8/27/2024 10:43, Gautham R. Shenoy wrote:
> 
> On Mon, Aug 26, 2024 at 04:13:55PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> AMD systems that support preferred cores will use "166" as their
>> numerator for max frequency calculations instead of "255".
>>
>> Add a function for detecting preferred cores by looking at the
>> highest perf value on all cores.
>>
>> If preferred cores are enabled return 166 and if disabled the
>> value in the highest perf register. As the function will be called
>> multiple times, cache the values for the boost numerator and if
>> preferred cores will be enabled in global variables.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
> 
> [..snip..]
> 
>>   /**
>>    * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
>>    * @cpu: CPU to get numerator for.
>> @@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>>    */
>>   int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>>   {
>> -	struct cpuinfo_x86 *c = &boot_cpu_data;
>> +	bool prefcore;
>> +	int ret;
>>   
>> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
>> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
>> -		*numerator = 166;
>> -		return 0;
>> -	}
>> +	ret = amd_detect_prefcore(&prefcore);
>> +	if (ret)
>> +		return ret;
>>   
>> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
>> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
>> -		*numerator = 166;
>> +	/* without preferred cores, return the highest perf register value */
>> +	if (!prefcore) {
>> +		*numerator = boost_numerator;
>>   		return 0;
>>   	}
>> -	*numerator = 255;
>> +	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
> 
> 
> Interesting. So even when the user boots a system that supports
> preferred-cores with "amd_preferred=disable",
> amd_get_boost_ratio_numerator() will return CPPC_HIGHEST_PERF_PREFCORE
> as the call prefcore == true here.
> 
> I suppose that is as intended, since even though the user may not want
> to use the preferred core logic for task-scheduling/load-balancing,
> the numerator for the boost-ratio is purely dependent on the h/w
> capability.
> 
> Is this understanding correct? If so, can this be included as a
> comment in the code ?
> 

Yup, you got it all right.  I'll fold some of this into the function 
comment for v2.

> The rest of the patch looks good to me.
> 
> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> 
> --
> Thanks and Regards
> gautham.
> 
> 
> 
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index f470b5700db58..ec32c830abc1d 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
>>   
>>   static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>>   {
>> -	int ret, prio;
>> -	u32 highest_perf;
>> -
>> -	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
>> -	if (ret)
>> +	/* user disabled or not detected */
>> +	if (!amd_pstate_prefcore)
>>   		return;
>>   
>>   	cpudata->hw_prefcore = true;
>> -	/* check if CPPC preferred core feature is enabled*/
>> -	if (highest_perf < CPPC_MAX_PERF)
>> -		prio = (int)highest_perf;
>> -	else {
>> -		pr_debug("AMD CPPC preferred core is unsupported!\n");
>> -		cpudata->hw_prefcore = false;
>> -		return;
>> -	}
>> -
>> -	if (!amd_pstate_prefcore)
>> -		return;
>>   
>>   	/*
>>   	 * The priorities can be set regardless of whether or not
>>   	 * sched_set_itmt_support(true) has been called and it is valid to
>>   	 * update them at any time after it has been called.
>>   	 */
>> -	sched_set_itmt_core_prio(prio, cpudata->cpu);
>> +	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->highest_perf), cpudata->cpu);
>>   
>>   	schedule_work(&sched_prefcore_work);
>>   }
>> @@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>   
>>   	cpudata->cpu = policy->cpu;
>>   
>> -	amd_pstate_init_prefcore(cpudata);
>> -
>>   	ret = amd_pstate_init_perf(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>>   
>> +	amd_pstate_init_prefcore(cpudata);
>> +
>>   	ret = amd_pstate_init_freq(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>> @@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>>   	cpudata->cpu = policy->cpu;
>>   	cpudata->epp_policy = 0;
>>   
>> -	amd_pstate_init_prefcore(cpudata);
>> -
>>   	ret = amd_pstate_init_perf(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>>   
>> +	amd_pstate_init_prefcore(cpudata);
>> +
>>   	ret = amd_pstate_init_freq(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>> @@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)
>>   		static_call_update(amd_pstate_update_perf, cppc_update_perf);
>>   	}
>>   
>> +	if (amd_pstate_prefcore) {
>> +		ret = amd_detect_prefcore(&amd_pstate_prefcore);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	/* enable amd pstate feature */
>>   	ret = amd_pstate_enable(true);
>>   	if (ret) {
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 2246ce0630362..1d79320a23490 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -137,10 +137,12 @@ struct cppc_cpudata {
>>   };
>>   
>>   #ifdef CONFIG_CPU_SUP_AMD
>> +extern int amd_detect_prefcore(bool *detected);
>>   extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>>   extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>>   #else /* !CONFIG_CPU_SUP_AMD */
>>   static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
>> +static inline int amd_detect_prefcore(bool *detected) { return -ENODEV; }
>>   static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
>>   #endif /* !CONFIG_CPU_SUP_AMD */
>>   
>> -- 
>> 2.43.0
>>
diff mbox series

Patch

diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index 5a6c01a1b0d95..729b35e84f5eb 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -9,6 +9,16 @@ 
 #include <asm/processor.h>
 #include <asm/topology.h>
 
+#define CPPC_HIGHEST_PERF_PREFCORE	166
+
+enum amd_pref_core {
+	AMD_PREF_CORE_UNKNOWN = 0,
+	AMD_PREF_CORE_SUPPORTED,
+	AMD_PREF_CORE_UNSUPPORTED,
+};
+static enum amd_pref_core amd_pref_core_detected;
+static u64 boost_numerator;
+
 /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
 
 bool cpc_supported_by_cpu(void)
@@ -149,6 +159,66 @@  int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf)
 }
 EXPORT_SYMBOL_GPL(amd_get_highest_perf);
 
+/**
+ * amd_detect_prefcore: Detect if CPUs in the system support preferred cores
+ * @detected: Output variable for the result of the detection.
+ *
+ * Determine whether CPUs in the system support preferred cores. On systems
+ * that support preferred cores, different highest perf values will be found
+ * on different cores. On other systems, the highest perf value will be the
+ * same on all cores.
+ *
+ * The result of the detection will be stored in the 'detected' parameter.
+ *
+ * Return: 0 for success, negative error code otherwise
+ */
+int amd_detect_prefcore(bool *detected)
+{
+	int cpu, count = 0;
+	u64 highest_perf[2] = {0};
+
+	if (WARN_ON(!detected))
+		return -EINVAL;
+
+	switch (amd_pref_core_detected) {
+	case AMD_PREF_CORE_SUPPORTED:
+		*detected = true;
+		return 0;
+	case AMD_PREF_CORE_UNSUPPORTED:
+		*detected = false;
+		return 0;
+	default:
+		break;
+	}
+
+	for_each_present_cpu(cpu) {
+		u32 tmp;
+		int ret;
+
+		ret = amd_get_highest_perf(cpu, &tmp);
+		if (ret)
+			return ret;
+
+		if (!count || (count == 1 && tmp != highest_perf[0]))
+			highest_perf[count++] = tmp;
+
+		if (count == 2)
+			break;
+	}
+
+	*detected = (count == 2);
+	boost_numerator = highest_perf[0];
+
+	amd_pref_core_detected = *detected ? AMD_PREF_CORE_SUPPORTED :
+					     AMD_PREF_CORE_UNSUPPORTED;
+
+	pr_debug("AMD CPPC preferred core is %ssupported (highest perf: 0x%llx)\n",
+		 *detected ? "" : "un", highest_perf[0]);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(amd_detect_prefcore);
+
 /**
  * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
  * @cpu: CPU to get numerator for.
@@ -162,20 +232,19 @@  EXPORT_SYMBOL_GPL(amd_get_highest_perf);
  */
 int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
 {
-	struct cpuinfo_x86 *c = &boot_cpu_data;
+	bool prefcore;
+	int ret;
 
-	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
-			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
-		*numerator = 166;
-		return 0;
-	}
+	ret = amd_detect_prefcore(&prefcore);
+	if (ret)
+		return ret;
 
-	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
-			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
-		*numerator = 166;
+	/* without preferred cores, return the highest perf register value */
+	if (!prefcore) {
+		*numerator = boost_numerator;
 		return 0;
 	}
-	*numerator = 255;
+	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
 
 	return 0;
 }
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index f470b5700db58..ec32c830abc1d 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -807,32 +807,18 @@  static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
 
 static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
 {
-	int ret, prio;
-	u32 highest_perf;
-
-	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
-	if (ret)
+	/* user disabled or not detected */
+	if (!amd_pstate_prefcore)
 		return;
 
 	cpudata->hw_prefcore = true;
-	/* check if CPPC preferred core feature is enabled*/
-	if (highest_perf < CPPC_MAX_PERF)
-		prio = (int)highest_perf;
-	else {
-		pr_debug("AMD CPPC preferred core is unsupported!\n");
-		cpudata->hw_prefcore = false;
-		return;
-	}
-
-	if (!amd_pstate_prefcore)
-		return;
 
 	/*
 	 * The priorities can be set regardless of whether or not
 	 * sched_set_itmt_support(true) has been called and it is valid to
 	 * update them at any time after it has been called.
 	 */
-	sched_set_itmt_core_prio(prio, cpudata->cpu);
+	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->highest_perf), cpudata->cpu);
 
 	schedule_work(&sched_prefcore_work);
 }
@@ -998,12 +984,12 @@  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 
 	cpudata->cpu = policy->cpu;
 
-	amd_pstate_init_prefcore(cpudata);
-
 	ret = amd_pstate_init_perf(cpudata);
 	if (ret)
 		goto free_cpudata1;
 
+	amd_pstate_init_prefcore(cpudata);
+
 	ret = amd_pstate_init_freq(cpudata);
 	if (ret)
 		goto free_cpudata1;
@@ -1453,12 +1439,12 @@  static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	cpudata->cpu = policy->cpu;
 	cpudata->epp_policy = 0;
 
-	amd_pstate_init_prefcore(cpudata);
-
 	ret = amd_pstate_init_perf(cpudata);
 	if (ret)
 		goto free_cpudata1;
 
+	amd_pstate_init_prefcore(cpudata);
+
 	ret = amd_pstate_init_freq(cpudata);
 	if (ret)
 		goto free_cpudata1;
@@ -1903,6 +1889,12 @@  static int __init amd_pstate_init(void)
 		static_call_update(amd_pstate_update_perf, cppc_update_perf);
 	}
 
+	if (amd_pstate_prefcore) {
+		ret = amd_detect_prefcore(&amd_pstate_prefcore);
+		if (ret)
+			return ret;
+	}
+
 	/* enable amd pstate feature */
 	ret = amd_pstate_enable(true);
 	if (ret) {
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 2246ce0630362..1d79320a23490 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -137,10 +137,12 @@  struct cppc_cpudata {
 };
 
 #ifdef CONFIG_CPU_SUP_AMD
+extern int amd_detect_prefcore(bool *detected);
 extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
 extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
 #else /* !CONFIG_CPU_SUP_AMD */
 static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
+static inline int amd_detect_prefcore(bool *detected) { return -ENODEV; }
 static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
 #endif /* !CONFIG_CPU_SUP_AMD */