| Message ID | 20250206121341.118337-4-corvin.koehne@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/igd: sync PCI IDs with i915 | expand |
On Thu, 6 Feb 2025 13:13:39 +0100 Corvin Köhne <corvin.koehne@gmail.com> wrote: > From: Corvin Köhne <c.koehne@beckhoff.com> > > We've recently imported the PCI ID list of knwon Intel GPU devices from > Linux. It allows us to properly match GPUs to their generation without > maintaining an own list of PCI IDs. > > Signed-off-by: Corvin Köhne <c.koehne@beckhoff.com> > --- > hw/vfio/igd.c | 77 ++++++++++++++++++++++++++++----------------------- > 1 file changed, 42 insertions(+), 35 deletions(-) > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > index 0740a5dd8c..e5d7006ce2 100644 > --- a/hw/vfio/igd.c > +++ b/hw/vfio/igd.c > @@ -18,6 +18,7 @@ > #include "hw/hw.h" > #include "hw/nvram/fw_cfg.h" > #include "pci.h" > +#include "standard-headers/drm/intel/pciids.h" > #include "trace.h" > > /* > @@ -51,6 +52,42 @@ > * headless setup is desired, the OpRegion gets in the way of that. > */ > > +struct igd_device { > + const uint32_t device_id; > + const int gen; > +}; > + > +#define IGD_DEVICE(_id, _gen) { \ > + .device_id = (_id), \ > + .gen = (_gen), \ > +} > + > +static const struct igd_device igd_devices[] = { > + INTEL_SNB_IDS(IGD_DEVICE, 6), > + INTEL_IVB_IDS(IGD_DEVICE, 6), > + INTEL_HSW_IDS(IGD_DEVICE, 7), > + INTEL_VLV_IDS(IGD_DEVICE, 7), > + INTEL_BDW_IDS(IGD_DEVICE, 8), > + INTEL_CHV_IDS(IGD_DEVICE, 8), > + INTEL_SKL_IDS(IGD_DEVICE, 9), > + INTEL_BXT_IDS(IGD_DEVICE, 9), > + INTEL_KBL_IDS(IGD_DEVICE, 9), > + INTEL_CFL_IDS(IGD_DEVICE, 9), > + INTEL_CML_IDS(IGD_DEVICE, 9), > + INTEL_GLK_IDS(IGD_DEVICE, 9), > + INTEL_ICL_IDS(IGD_DEVICE, 11), > + INTEL_EHL_IDS(IGD_DEVICE, 11), > + INTEL_JSL_IDS(IGD_DEVICE, 11), > + INTEL_TGL_IDS(IGD_DEVICE, 12), > + INTEL_RKL_IDS(IGD_DEVICE, 12), > + INTEL_ADLS_IDS(IGD_DEVICE, 12), > + INTEL_ADLP_IDS(IGD_DEVICE, 12), > + INTEL_ADLN_IDS(IGD_DEVICE, 12), > + INTEL_RPLS_IDS(IGD_DEVICE, 12), > + INTEL_RPLU_IDS(IGD_DEVICE, 12), > + INTEL_RPLP_IDS(IGD_DEVICE, 12), > +}; I agree with Connie's comment on the ordering and content of the first two patches. For these last two, I wish these actually made it substantially easier to synchronize with upstream. Based on the next patch, I think it still requires manually tracking/parsing internal code in the i915 driver to extract generation information. Is it possible that we could split the above into a separate file that's auto-generated from a script? For example maybe some scripting and C code that can instantiate the pciidlist array from i915_pci.c and regurgitate it into a device-id/generation table? Thanks, Alex > + > /* > * This presumes the device is already known to be an Intel VGA device, so we > * take liberties in which device ID bits match which generation. This should > @@ -60,42 +97,12 @@ > */ > static int igd_gen(VFIOPCIDevice *vdev) > { > - /* > - * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84 > - * and 0x5a85, match bit 11:1 here > - * Prefix 0x0a is taken by Haswell, this rule should be matched first. > - */ > - if ((vdev->device_id & 0xffe) == 0xa84) { > - return 9; > - } > + for (int i = 0; i < ARRAY_SIZE(igd_devices); i++) { > + if (igd_devices[i].device_id != vdev->device_id) { > + continue; > + } > > - switch (vdev->device_id & 0xff00) { > - case 0x0100: /* SandyBridge, IvyBridge */ > - return 6; > - case 0x0400: /* Haswell */ > - case 0x0a00: /* Haswell */ > - case 0x0c00: /* Haswell */ > - case 0x0d00: /* Haswell */ > - case 0x0f00: /* Valleyview/Bay Trail */ > - return 7; > - case 0x1600: /* Broadwell */ > - case 0x2200: /* Cherryview */ > - return 8; > - case 0x1900: /* Skylake */ > - case 0x3100: /* Gemini Lake */ > - case 0x5900: /* Kaby Lake */ > - case 0x3e00: /* Coffee Lake */ > - case 0x9B00: /* Comet Lake */ > - return 9; > - case 0x8A00: /* Ice Lake */ > - case 0x4500: /* Elkhart Lake */ > - case 0x4E00: /* Jasper Lake */ > - return 11; > - case 0x9A00: /* Tiger Lake */ > - case 0x4C00: /* Rocket Lake */ > - case 0x4600: /* Alder Lake */ > - case 0xA700: /* Raptor Lake */ > - return 12; > + return igd_devices[i].gen; > } > > /*
On Thu, 2025-02-06 at 14:26 -0700, Alex Williamson wrote: > On Thu, 6 Feb 2025 13:13:39 +0100 > Corvin Köhne <corvin.koehne@gmail.com> wrote: > > > From: Corvin Köhne <c.koehne@beckhoff.com> > > > > We've recently imported the PCI ID list of knwon Intel GPU devices from > > Linux. It allows us to properly match GPUs to their generation without > > maintaining an own list of PCI IDs. > > > > Signed-off-by: Corvin Köhne <c.koehne@beckhoff.com> > > --- > > hw/vfio/igd.c | 77 ++++++++++++++++++++++++++++----------------------- > > 1 file changed, 42 insertions(+), 35 deletions(-) > > > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > > index 0740a5dd8c..e5d7006ce2 100644 > > --- a/hw/vfio/igd.c > > +++ b/hw/vfio/igd.c > > @@ -18,6 +18,7 @@ > > #include "hw/hw.h" > > #include "hw/nvram/fw_cfg.h" > > #include "pci.h" > > +#include "standard-headers/drm/intel/pciids.h" > > #include "trace.h" > > > > /* > > @@ -51,6 +52,42 @@ > > * headless setup is desired, the OpRegion gets in the way of that. > > */ > > > > +struct igd_device { > > + const uint32_t device_id; > > + const int gen; > > +}; > > + > > +#define IGD_DEVICE(_id, _gen) { \ > > + .device_id = (_id), \ > > + .gen = (_gen), \ > > +} > > + > > +static const struct igd_device igd_devices[] = { > > + INTEL_SNB_IDS(IGD_DEVICE, 6), > > + INTEL_IVB_IDS(IGD_DEVICE, 6), > > + INTEL_HSW_IDS(IGD_DEVICE, 7), > > + INTEL_VLV_IDS(IGD_DEVICE, 7), > > + INTEL_BDW_IDS(IGD_DEVICE, 8), > > + INTEL_CHV_IDS(IGD_DEVICE, 8), > > + INTEL_SKL_IDS(IGD_DEVICE, 9), > > + INTEL_BXT_IDS(IGD_DEVICE, 9), > > + INTEL_KBL_IDS(IGD_DEVICE, 9), > > + INTEL_CFL_IDS(IGD_DEVICE, 9), > > + INTEL_CML_IDS(IGD_DEVICE, 9), > > + INTEL_GLK_IDS(IGD_DEVICE, 9), > > + INTEL_ICL_IDS(IGD_DEVICE, 11), > > + INTEL_EHL_IDS(IGD_DEVICE, 11), > > + INTEL_JSL_IDS(IGD_DEVICE, 11), > > + INTEL_TGL_IDS(IGD_DEVICE, 12), > > + INTEL_RKL_IDS(IGD_DEVICE, 12), > > + INTEL_ADLS_IDS(IGD_DEVICE, 12), > > + INTEL_ADLP_IDS(IGD_DEVICE, 12), > > + INTEL_ADLN_IDS(IGD_DEVICE, 12), > > + INTEL_RPLS_IDS(IGD_DEVICE, 12), > > + INTEL_RPLU_IDS(IGD_DEVICE, 12), > > + INTEL_RPLP_IDS(IGD_DEVICE, 12), > > +}; > > I agree with Connie's comment on the ordering and content of the first > two patches. > > For these last two, I wish these actually made it substantially easier > to synchronize with upstream. Based on the next patch, I think it > still requires manually tracking/parsing internal code in the i915 > driver to extract generation information. > > Is it possible that we could split the above into a separate file > that's auto-generated from a script? For example maybe some scripting > and C code that can instantiate the pciidlist array from i915_pci.c and > regurgitate it into a device-id/generation table? Thanks, > > Alex > Hi Alex, I took a closer look into i915 and it seems hard to parse. Upstream maintains a description for each generation, e.g. on AlderLake P [1] the generation is defined in the .info field of a struct, the .info field itself is defined somewhere else [2] and sets the .__runtime_defaults.ip.ver by another C macro [3]. Other platforms like GeminiLake set the .ip.ver directly in their description struct [4]. Nevertheless, we may not need this PCI ID mapping at all in the future. It looks like Intel added a new register to their GPU starting with MeteorLake [5]. We can read it to obtain the GPU generation [6]. I don't have a MeteorLake system available yet, so I can't test it. On my TigerLake system, the register returns zero. When it works as expected, we could refactor the igd_gen function to something like: static int igd_gen(VFIOPCIDevice vdev) { uint32_t gmd_id = vfio_region_read(&vdev->bars[0].region, GMD_ID_DISPLAY, 4); if (gmd_id != 0) { return (gmd_id & GMD_ID_ARCH_MASK) >> GMD_ID_ARCH_SHIFT; } // Fallback to PCI ID mapping. ... } [1] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1171 [2] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1128 [3] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1120 [4] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L829 [5] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1326-L1330 [6] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1432
On Fri, 2025-02-07 at 08:47 +0100, Corvin Köhne wrote: > On Thu, 2025-02-06 at 14:26 -0700, Alex Williamson wrote: > > On Thu, 6 Feb 2025 13:13:39 +0100 > > Corvin Köhne <corvin.koehne@gmail.com> wrote: > > > > > From: Corvin Köhne <c.koehne@beckhoff.com> > > > > > > We've recently imported the PCI ID list of knwon Intel GPU devices from > > > Linux. It allows us to properly match GPUs to their generation without > > > maintaining an own list of PCI IDs. > > > > > > Signed-off-by: Corvin Köhne <c.koehne@beckhoff.com> > > > --- > > > hw/vfio/igd.c | 77 ++++++++++++++++++++++++++++----------------------- > > > 1 file changed, 42 insertions(+), 35 deletions(-) > > > > > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > > > index 0740a5dd8c..e5d7006ce2 100644 > > > --- a/hw/vfio/igd.c > > > +++ b/hw/vfio/igd.c > > > @@ -18,6 +18,7 @@ > > > #include "hw/hw.h" > > > #include "hw/nvram/fw_cfg.h" > > > #include "pci.h" > > > +#include "standard-headers/drm/intel/pciids.h" > > > #include "trace.h" > > > > > > /* > > > @@ -51,6 +52,42 @@ > > > * headless setup is desired, the OpRegion gets in the way of that. > > > */ > > > > > > +struct igd_device { > > > + const uint32_t device_id; > > > + const int gen; > > > +}; > > > + > > > +#define IGD_DEVICE(_id, _gen) { \ > > > + .device_id = (_id), \ > > > + .gen = (_gen), \ > > > +} > > > + > > > +static const struct igd_device igd_devices[] = { > > > + INTEL_SNB_IDS(IGD_DEVICE, 6), > > > + INTEL_IVB_IDS(IGD_DEVICE, 6), > > > + INTEL_HSW_IDS(IGD_DEVICE, 7), > > > + INTEL_VLV_IDS(IGD_DEVICE, 7), > > > + INTEL_BDW_IDS(IGD_DEVICE, 8), > > > + INTEL_CHV_IDS(IGD_DEVICE, 8), > > > + INTEL_SKL_IDS(IGD_DEVICE, 9), > > > + INTEL_BXT_IDS(IGD_DEVICE, 9), > > > + INTEL_KBL_IDS(IGD_DEVICE, 9), > > > + INTEL_CFL_IDS(IGD_DEVICE, 9), > > > + INTEL_CML_IDS(IGD_DEVICE, 9), > > > + INTEL_GLK_IDS(IGD_DEVICE, 9), > > > + INTEL_ICL_IDS(IGD_DEVICE, 11), > > > + INTEL_EHL_IDS(IGD_DEVICE, 11), > > > + INTEL_JSL_IDS(IGD_DEVICE, 11), > > > + INTEL_TGL_IDS(IGD_DEVICE, 12), > > > + INTEL_RKL_IDS(IGD_DEVICE, 12), > > > + INTEL_ADLS_IDS(IGD_DEVICE, 12), > > > + INTEL_ADLP_IDS(IGD_DEVICE, 12), > > > + INTEL_ADLN_IDS(IGD_DEVICE, 12), > > > + INTEL_RPLS_IDS(IGD_DEVICE, 12), > > > + INTEL_RPLU_IDS(IGD_DEVICE, 12), > > > + INTEL_RPLP_IDS(IGD_DEVICE, 12), > > > +}; > > > > I agree with Connie's comment on the ordering and content of the first > > two patches. > > > > For these last two, I wish these actually made it substantially easier > > to synchronize with upstream. Based on the next patch, I think it > > still requires manually tracking/parsing internal code in the i915 > > driver to extract generation information. > > > > Is it possible that we could split the above into a separate file > > that's auto-generated from a script? For example maybe some scripting > > and C code that can instantiate the pciidlist array from i915_pci.c and > > regurgitate it into a device-id/generation table? Thanks, > > > > Alex > > > > Hi Alex, > > I took a closer look into i915 and it seems hard to parse. Upstream maintains > a > description for each generation, e.g. on AlderLake P [1] the generation is > defined in the .info field of a struct, the .info field itself is defined > somewhere else [2] and sets the .__runtime_defaults.ip.ver by another C macro > [3]. Other platforms like GeminiLake set the .ip.ver directly in their > description struct [4]. > > Nevertheless, we may not need this PCI ID mapping at all in the future. It > looks > like Intel added a new register to their GPU starting with MeteorLake [5]. We > can read it to obtain the GPU generation [6]. I don't have a MeteorLake system > available yet, so I can't test it. On my TigerLake system, the register > returns > zero. When it works as expected, we could refactor the igd_gen function to > something like: > > static int igd_gen(VFIOPCIDevice vdev) { > uint32_t gmd_id = vfio_region_read(&vdev->bars[0].region, GMD_ID_DISPLAY, > 4); > if (gmd_id != 0) { > return (gmd_id & GMD_ID_ARCH_MASK) >> GMD_ID_ARCH_SHIFT; > } > > // Fallback to PCI ID mapping. > ... > } > > [1] > https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1171 > [2] > https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1128 > [3] > https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1120 > [4] > https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L829 > [5] > https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1326-L1330 > [6] > https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/display/intel_display_device.c#L1432 > > I've missed that upstream maintains a second list [1]. Nevertheless, it looks still hard to parse. [1] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/i915/i915_pci.c
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index 0740a5dd8c..e5d7006ce2 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -18,6 +18,7 @@ #include "hw/hw.h" #include "hw/nvram/fw_cfg.h" #include "pci.h" +#include "standard-headers/drm/intel/pciids.h" #include "trace.h" /* @@ -51,6 +52,42 @@ * headless setup is desired, the OpRegion gets in the way of that. */ +struct igd_device { + const uint32_t device_id; + const int gen; +}; + +#define IGD_DEVICE(_id, _gen) { \ + .device_id = (_id), \ + .gen = (_gen), \ +} + +static const struct igd_device igd_devices[] = { + INTEL_SNB_IDS(IGD_DEVICE, 6), + INTEL_IVB_IDS(IGD_DEVICE, 6), + INTEL_HSW_IDS(IGD_DEVICE, 7), + INTEL_VLV_IDS(IGD_DEVICE, 7), + INTEL_BDW_IDS(IGD_DEVICE, 8), + INTEL_CHV_IDS(IGD_DEVICE, 8), + INTEL_SKL_IDS(IGD_DEVICE, 9), + INTEL_BXT_IDS(IGD_DEVICE, 9), + INTEL_KBL_IDS(IGD_DEVICE, 9), + INTEL_CFL_IDS(IGD_DEVICE, 9), + INTEL_CML_IDS(IGD_DEVICE, 9), + INTEL_GLK_IDS(IGD_DEVICE, 9), + INTEL_ICL_IDS(IGD_DEVICE, 11), + INTEL_EHL_IDS(IGD_DEVICE, 11), + INTEL_JSL_IDS(IGD_DEVICE, 11), + INTEL_TGL_IDS(IGD_DEVICE, 12), + INTEL_RKL_IDS(IGD_DEVICE, 12), + INTEL_ADLS_IDS(IGD_DEVICE, 12), + INTEL_ADLP_IDS(IGD_DEVICE, 12), + INTEL_ADLN_IDS(IGD_DEVICE, 12), + INTEL_RPLS_IDS(IGD_DEVICE, 12), + INTEL_RPLU_IDS(IGD_DEVICE, 12), + INTEL_RPLP_IDS(IGD_DEVICE, 12), +}; + /* * This presumes the device is already known to be an Intel VGA device, so we * take liberties in which device ID bits match which generation. This should @@ -60,42 +97,12 @@ */ static int igd_gen(VFIOPCIDevice *vdev) { - /* - * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84 - * and 0x5a85, match bit 11:1 here - * Prefix 0x0a is taken by Haswell, this rule should be matched first. - */ - if ((vdev->device_id & 0xffe) == 0xa84) { - return 9; - } + for (int i = 0; i < ARRAY_SIZE(igd_devices); i++) { + if (igd_devices[i].device_id != vdev->device_id) { + continue; + } - switch (vdev->device_id & 0xff00) { - case 0x0100: /* SandyBridge, IvyBridge */ - return 6; - case 0x0400: /* Haswell */ - case 0x0a00: /* Haswell */ - case 0x0c00: /* Haswell */ - case 0x0d00: /* Haswell */ - case 0x0f00: /* Valleyview/Bay Trail */ - return 7; - case 0x1600: /* Broadwell */ - case 0x2200: /* Cherryview */ - return 8; - case 0x1900: /* Skylake */ - case 0x3100: /* Gemini Lake */ - case 0x5900: /* Kaby Lake */ - case 0x3e00: /* Coffee Lake */ - case 0x9B00: /* Comet Lake */ - return 9; - case 0x8A00: /* Ice Lake */ - case 0x4500: /* Elkhart Lake */ - case 0x4E00: /* Jasper Lake */ - return 11; - case 0x9A00: /* Tiger Lake */ - case 0x4C00: /* Rocket Lake */ - case 0x4600: /* Alder Lake */ - case 0xA700: /* Raptor Lake */ - return 12; + return igd_devices[i].gen; } /*