On Thu, 20 Mar 2025, Mario Limonciello wrote: > On 3/19/2025 09:03, Ilpo Järvinen wrote: > > On Tue, 18 Feb 2025, Mario Limonciello wrote: > > > > > From: Perry Yuan <Perry.Yuan@xxxxxxx> > > > > > > The AMD Heterogeneous core design and Hardware Feedback Interface (HFI) > > > provide behavioral classification and a dynamically updated ranking table > > > for the scheduler to use when choosing cores for tasks. > > > > > > There are two CPU core types defined: `Classic Core` and `Dense Core`. > > > "Classic" cores are the standard performance cores, while "Dense" cores > > > are optimized for area and efficiency. > > > > > > Heterogeneous compute refers to CPU implementations that are comprised > > > of more than one architectural class, each with two capabilities. This > > > means each CPU reports two separate capabilities: "perf" and "eff". > > > > > > Each capability lists all core ranking numbers between 0 and 255, where > > > a higher number represents a higher capability. > > > > > > Heterogeneous systems can also extend to more than two architectural > > > classes. > > > > > > The purpose of the scheduling feedback mechanism is to provide information > > > to the operating system scheduler in real time, allowing the scheduler to > > > direct threads to the optimal core during task scheduling. > > > > > > All core ranking data are provided by the PMFW via a shared memory ranking > > > table, which the driver reads and uses to update core capabilities to the > > > scheduler. When the hardware updates the table, it generates a platform > > > interrupt to notify the OS to read the new ranking table. > > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 > > > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx> > > > Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> > > > Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx> > > > Co-developed-by: Mario Limonciello <mario.limonciello@xxxxxxx> > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > > > --- > > > v8: > > > * s,devm_kzalloc,devm_kcalloc, > > > * fold newlines from patch 5 into this patch > > > * Drop ->cpu member, push to later patch > > > * s,for_each_present_cpu,for_each_possible_cpu, > > > v7: > > > * Adjust Kconfig to 80 characters > > > --- > > > drivers/platform/x86/amd/Kconfig | 1 + > > > drivers/platform/x86/amd/Makefile | 1 + > > > drivers/platform/x86/amd/hfi/Kconfig | 20 ++++ > > > drivers/platform/x86/amd/hfi/Makefile | 7 ++ > > > drivers/platform/x86/amd/hfi/hfi.c | 162 ++++++++++++++++++++++++++ > > > 5 files changed, 191 insertions(+) > > > create mode 100644 drivers/platform/x86/amd/hfi/Kconfig > > > create mode 100644 drivers/platform/x86/amd/hfi/Makefile > > > create mode 100644 drivers/platform/x86/amd/hfi/hfi.c > > > > > > diff --git a/drivers/platform/x86/amd/Kconfig > > > b/drivers/platform/x86/amd/Kconfig > > > index c3e086ea64fc6..589d61ebf726b 100644 > > > --- a/drivers/platform/x86/amd/Kconfig > > > +++ b/drivers/platform/x86/amd/Kconfig > > > @@ -6,6 +6,7 @@ > > > source "drivers/platform/x86/amd/hsmp/Kconfig" > > > source "drivers/platform/x86/amd/pmf/Kconfig" > > > source "drivers/platform/x86/amd/pmc/Kconfig" > > > +source "drivers/platform/x86/amd/hfi/Kconfig" > > > config AMD_3D_VCACHE > > > tristate "AMD 3D V-Cache Performance Optimizer Driver" > > > diff --git a/drivers/platform/x86/amd/Makefile > > > b/drivers/platform/x86/amd/Makefile > > > index 56f62fc9c97b4..c50e93c3334cf 100644 > > > --- a/drivers/platform/x86/amd/Makefile > > > +++ b/drivers/platform/x86/amd/Makefile > > > @@ -10,3 +10,4 @@ obj-$(CONFIG_AMD_PMC) += pmc/ > > > obj-$(CONFIG_AMD_HSMP) += hsmp/ > > > obj-$(CONFIG_AMD_PMF) += pmf/ > > > obj-$(CONFIG_AMD_WBRF) += wbrf.o > > > +obj-$(CONFIG_AMD_HFI) += hfi/ > > > diff --git a/drivers/platform/x86/amd/hfi/Kconfig > > > b/drivers/platform/x86/amd/hfi/Kconfig > > > new file mode 100644 > > > index 0000000000000..532939eb08a6a > > > --- /dev/null > > > +++ b/drivers/platform/x86/amd/hfi/Kconfig > > > @@ -0,0 +1,20 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only > > > +# > > > +# AMD Hardware Feedback Interface Driver > > > +# > > > + > > > +config AMD_HFI > > > + bool "AMD Hetero Core Hardware Feedback Driver" > > > + depends on ACPI > > > + depends on CPU_SUP_AMD > > > + help > > > + Select this option to enable the AMD Heterogeneous Core Hardware > > > + Feedback Interface. If selected, hardware provides runtime thread > > > + classification guidance to the operating system on the performance > > > and > > > + energy efficiency capabilities of each heterogeneous CPU core. These > > > + capabilities may vary due to the inherent differences in the core > > > types > > > + and can also change as a result of variations in the operating > > > + conditions of the system such as power and thermal limits. If > > > selected, > > > > This says the capabilities can change but metadata is only read and scores > > updated during probe? > > > > > + the kernel relays updates in heterogeneous CPUs' capabilities to > > > + userspace, allowing for more optimal task scheduling and resource > > > + allocation, leveraging the diverse set of cores available. > > > > How are the capabilities communicated to userspace as mentioned here? I'm > > asking this because I only noted debugfs interface, and that commit > > claimed the debug fs interface was to troubleshoot scheduler issues. > > This is one of those cases that the split into multiple parts shows. Thinking > through I feel it makes more sense to adjust for now and then we can change it > again on the next part. > > > > > > diff --git a/drivers/platform/x86/amd/hfi/Makefile > > > b/drivers/platform/x86/amd/hfi/Makefile > > > new file mode 100644 > > > index 0000000000000..672c6ac106e95 > > > --- /dev/null > > > +++ b/drivers/platform/x86/amd/hfi/Makefile > > > @@ -0,0 +1,7 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# > > > +# AMD Hardware Feedback Interface Driver > > > +# > > > + > > > +obj-$(CONFIG_AMD_HFI) += amd_hfi.o > > > +amd_hfi-objs := hfi.o > > > diff --git a/drivers/platform/x86/amd/hfi/hfi.c > > > b/drivers/platform/x86/amd/hfi/hfi.c > > > new file mode 100644 > > > index 0000000000000..426f7e520b76c > > > --- /dev/null > > > +++ b/drivers/platform/x86/amd/hfi/hfi.c > > > @@ -0,0 +1,162 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * AMD Hardware Feedback Interface Driver > > > + * > > > + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved. > > > > 2025 ? > > Ack. > > Heh, earlier versions started in 2024! > > > > + * > > > + * Authors: Perry Yuan <Perry.Yuan@xxxxxxx> > > > + * Mario Limonciello <mario.limonciello@xxxxxxx> > > > + */ > > > + > > > +#define pr_fmt(fmt) "amd-hfi: " fmt > > > + > > > +#include <linux/acpi.h> > > > +#include <linux/cpu.h> > > > +#include <linux/cpumask.h> > > > +#include <linux/gfp.h> > > > +#include <linux/init.h> > > > +#include <linux/io.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/mutex.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/smp.h> > > > + > > > +#define AMD_HFI_DRIVER "amd_hfi" > > > + > > > +#define AMD_HETERO_CPUID_27 0x80000027 > > > + > > > +static struct platform_device *device; > > > + > > > +struct amd_hfi_data { > > > + const char *name; > > > + struct device *dev; > > > + struct mutex lock; > > > > Please mention what this protects. > > As a comment at the end like this, right? > > struct mutex lock; /* lock the foo */ Yes. -- i. > > > > > > +}; > > > + > > > +struct amd_hfi_classes { > > > + u32 perf; > > > + u32 eff; > > > +}; > > > + > > > +/** > > > + * struct amd_hfi_cpuinfo - HFI workload class info per CPU > > > + * @cpu: cpu index > > > + * @class_index: workload class ID index > > > + * @nr_class: max number of workload class supported > > > + * @amd_hfi_classes: current cpu workload class ranking data > > > + * > > > + * Parameters of a logical processor linked with hardware feedback class > > > > missing . > > Ack > > > > > > + */ > > > +struct amd_hfi_cpuinfo { > > > + int cpu; > > > + s16 class_index; > > > + u8 nr_class; > > > + struct amd_hfi_classes *amd_hfi_classes; > > > +}; > > > + > > > +static DEFINE_PER_CPU(struct amd_hfi_cpuinfo, amd_hfi_cpuinfo) = > > > {.class_index = -1}; > > > + > > > +static int amd_hfi_alloc_class_data(struct platform_device *pdev) > > > +{ > > > + struct amd_hfi_cpuinfo *hfi_cpuinfo; > > > + struct device *dev = &pdev->dev; > > > + int idx; > > > + int nr_class_id; > > > + > > > + nr_class_id = cpuid_eax(AMD_HETERO_CPUID_27); > > > + if (nr_class_id < 0 || nr_class_id > 255) { > > > > Is the signed type correct for this? > > > > > + dev_err(dev, "failed to get number of supported classes: > > > %d\n", > > > + nr_class_id); > > > > I'd reword the error message as the number of classes was just too > > large / outside the allowed range. > > OK. > > > > > > + return -EINVAL; > > > + } > > > + > > > + for_each_possible_cpu(idx) { > > > + struct amd_hfi_classes *classes; > > > + > > > + classes = devm_kcalloc(dev, > > > + nr_class_id, > > > + sizeof(struct amd_hfi_classes), > > > + GFP_KERNEL); > > > + if (!classes) > > > + return -ENOMEM; > > > + hfi_cpuinfo = per_cpu_ptr(&amd_hfi_cpuinfo, idx); > > > + hfi_cpuinfo->amd_hfi_classes = classes; > > > + hfi_cpuinfo->nr_class = nr_class_id; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static const struct acpi_device_id amd_hfi_platform_match[] = { > > > + {"AMDI0104", 0}, > > > + { } > > > +}; > > > +MODULE_DEVICE_TABLE(acpi, amd_hfi_platform_match); > > > + > > > +static int amd_hfi_probe(struct platform_device *pdev) > > > +{ > > > + struct amd_hfi_data *amd_hfi_data; > > > + int ret; > > > + > > > + if (!acpi_match_device(amd_hfi_platform_match, &pdev->dev)) > > > + return -ENODEV; > > > + > > > + amd_hfi_data = devm_kzalloc(&pdev->dev, sizeof(*amd_hfi_data), > > > GFP_KERNEL); > > > + if (!amd_hfi_data) > > > + return -ENOMEM; > > > + > > > + amd_hfi_data->dev = &pdev->dev; > > > + ret = devm_mutex_init(&pdev->dev, &amd_hfi_data->lock); > > > + if (ret) > > > + return ret; > > > + platform_set_drvdata(pdev, amd_hfi_data); > > > + > > > + ret = amd_hfi_alloc_class_data(pdev); > > > + if (ret) > > > + return ret; > > > + > > > + return 0; > > > +} > > > + > > > +static struct platform_driver amd_hfi_driver = { > > > + .driver = { > > > + .name = AMD_HFI_DRIVER, > > > + .owner = THIS_MODULE, > > > + .acpi_match_table = ACPI_PTR(amd_hfi_platform_match), > > > + }, > > > + .probe = amd_hfi_probe, > > > +}; > > > + > > > +static int __init amd_hfi_init(void) > > > +{ > > > + int ret; > > > + > > > + if (acpi_disabled || > > > + !cpu_feature_enabled(X86_FEATURE_AMD_HETEROGENEOUS_CORES) || > > > + !cpu_feature_enabled(X86_FEATURE_AMD_WORKLOAD_CLASS)) > > > + return -ENODEV; > > > + > > > + device = platform_device_register_simple(AMD_HFI_DRIVER, -1, NULL, 0); > > > + if (IS_ERR(device)) { > > > + pr_err("unable to register HFI platform device\n"); > > > + return PTR_ERR(device); > > > + } > > > + > > > + ret = platform_driver_register(&amd_hfi_driver); > > > + if (ret) > > > + pr_err("failed to register HFI driver\n"); > > > + > > > + return ret; > > > +} > > > + > > > +static __exit void amd_hfi_exit(void) > > > +{ > > > + platform_device_unregister(device); > > > + platform_driver_unregister(&amd_hfi_driver); > > > > Why are these not in the opposite order than in init? > > Oversight. Will fix it. > > > > > > +} > > > +module_init(amd_hfi_init); > > > +module_exit(amd_hfi_exit); > > > + > > > +MODULE_LICENSE("GPL"); > > > +MODULE_DESCRIPTION("AMD Hardware Feedback Interface Driver"); > > > > > >
