Re: [PATCH 07/11] platform/x86/: hfi: init per-cpu scores for each class

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 27 Aug 2024, Perry Yuan wrote:

> From: Perry Yuan <Perry.Yuan@amd.com>
> 
> Initialize per cpu score `amd_hfi_ipcc_scores` which store energy score
> and performance score data for each class.
> 
> `Classic core` and `Dense core` are ranked according to those values as
> energy efficiency capability or performance capability.
> OS scheduler will pick cores from the ranking list on each class ID for
> the thread which provide the class id got from hardware feedback
> interface.
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>  drivers/platform/x86/amd/hfi/hfi.c | 65 ++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/hfi/hfi.c b/drivers/platform/x86/amd/hfi/hfi.c
> index 50f6ca9c148a..cd5f2b708ebf 100644
> --- a/drivers/platform/x86/amd/hfi/hfi.c
> +++ b/drivers/platform/x86/amd/hfi/hfi.c
> @@ -111,6 +111,7 @@ struct amd_hfi_data {
>  	raw_spinlock_t		table_lock;
>  	struct acpi_subtable_header	*pcct_entry;
>  	struct amd_hfi_metadata		*hfi_meta;
> +	raw_spinlock_t		hfi_data_lock;
>  };
>  
>  /**
> @@ -130,6 +131,7 @@ struct amd_hfi_classes {
>   * struct amd_hfi_cpuinfo - HFI workload class info per CPU
>   * @cpu:		cpu index
>   * @cpus:		cpu mask of cpus
> + * @apic_id:		apic id of the current cpu
>   * @class_index:	workload class ID index
>   * @nr_classa:		max number of workload class supported
>   * @amd_hfi_classes:	current cpu workload class ranking data
> @@ -139,6 +141,7 @@ struct amd_hfi_classes {
>  struct amd_hfi_cpuinfo {
>  	int		cpu;
>  	cpumask_var_t	cpus;
> +	u32		apic_id;
>  	s16		class_index;
>  	u8		nr_class;
>  	struct amd_hfi_classes	*amd_hfi_classes;
> @@ -146,6 +149,12 @@ struct amd_hfi_cpuinfo {
>  
>  static DEFINE_PER_CPU(struct amd_hfi_cpuinfo, amd_hfi_cpuinfo) = {.class_index = -1};
>  
> +static DEFINE_MUTEX(hfi_cpuinfo_lock);
> +static int __percpu *amd_hfi_ipcc_scores;
> +
> +static int amd_set_hfi_ipcc_score(struct amd_hfi_cpuinfo *info, int cpu);
> +static int update_hfi_ipcc_scores(struct amd_hfi_data *amd_hfi_data);

Looks unnecessary forward declarations. In general, we try to arrange code 
so that forward declarations are not required.

> +
>  static int find_cpu_index_by_apicid(unsigned int target_apicid)
>  {
>  	int cpu_index;
> @@ -293,6 +302,40 @@ static void amd_hfi_remove(struct platform_device *pdev)
>  
>  	mutex_destroy(&dev->lock);
>  }
> +
> +static int amd_set_hfi_ipcc_score(struct amd_hfi_cpuinfo *hfi_cpuinfo, int cpu)
> +{
> +	int i, *hfi_scores;
> +	u8 nr_classes = hfi_cpuinfo->nr_class;

Reverse xmas tree order.

> +
> +	hfi_scores = per_cpu_ptr(amd_hfi_ipcc_scores, cpu);
> +	if (!hfi_scores)
> +		return -ENODEV;
> +
> +	for (i = 0;  i < nr_classes; i++)

Extra space.

> +		WRITE_ONCE(hfi_scores[i], hfi_cpuinfo->amd_hfi_classes[i].perf);
> +
> +	return 0;
> +}
> +
> +static int update_hfi_ipcc_scores(struct amd_hfi_data *amd_hfi_data)
> +{
> +	int cpu;
> +	int ret;
> +
> +	raw_spin_lock_irq(&amd_hfi_data->hfi_data_lock);

Again, this is called from .probe so why you need a raw spinlock???

> +	for_each_online_cpu(cpu) {
> +		struct amd_hfi_cpuinfo *hfi_cpuinfo = per_cpu_ptr(&amd_hfi_cpuinfo, cpu);
> +
> +		ret = amd_set_hfi_ipcc_score(hfi_cpuinfo, cpu);
> +		if (ret)
> +			return ret;
> +	}
> +	raw_spin_unlock_irq(&amd_hfi_data->hfi_data_lock);
> +
> +	return 0;
> +}
> +
>  static int amd_hfi_metadata_parser(struct platform_device *pdev,
>  		struct amd_hfi_data *amd_hfi_data)
>  {
> @@ -344,6 +387,19 @@ static int amd_hfi_metadata_parser(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +static int alloc_amd_hfi_ipcc_scores(struct amd_hfi_data *amd_hfi_data)
> +{
> +	struct amd_hfi_metadata *hfi_meta = amd_hfi_data->hfi_meta;
> +
> +	amd_hfi_ipcc_scores = __alloc_percpu(sizeof(*amd_hfi_ipcc_scores) *
> +			hfi_meta->n_classes,
> +			sizeof(*amd_hfi_ipcc_scores));

Align these continuation lines better.

-- 
 i.

> +	if (WARN_ON(!amd_hfi_ipcc_scores))
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static const struct acpi_device_id amd_hfi_platform_match[] = {
>  	{ "AMDI0104", 0},
>  	{ }
> @@ -385,6 +441,7 @@ static int amd_hfi_probe(struct platform_device *pdev)
>  	amd_hfi_data->dhandle = dhandle;
>  
>  	raw_spin_lock_init(&amd_hfi_data->table_lock);
> +	raw_spin_lock_init(&amd_hfi_data->hfi_data_lock);
>  	mutex_init(&amd_hfi_data->lock);
>  
>  	platform_set_drvdata(pdev, amd_hfi_data);
> @@ -402,6 +459,14 @@ static int amd_hfi_probe(struct platform_device *pdev)
>  
>  	amd_hfi_data->hfi_meta->dynamic_rank_feature =
>  					cpuid_ebx(AMD_HETERO_CPUID_27) & 0xF;
> +
> +	if (alloc_amd_hfi_ipcc_scores(amd_hfi_data))
> +		goto err_exit;
> +
> +	ret = update_hfi_ipcc_scores(amd_hfi_data);
> +	if (ret)
> +		goto err_exit;
> +
>  	dev_dbg(&pdev->dev, "%s driver registered.\n", pdev->name);
>  
>  	return 0;
>