Re: [PATCH 3/4] vfio/igd: use PCI ID defines to detect IGD gen
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 wrote:
> >
> > > From: Corvin Köhne
> > >
> > > 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
> > > ---
> > > 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/gp
Re: [PATCH 3/4] vfio/igd: use PCI ID defines to detect IGD 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 wrote:
>
> > From: Corvin Köhne
> >
> > 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
> > ---
> > 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
--
Kind regards,
Corvin
> > +
> > /*
> > * 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 tak
Re: [PATCH 3/4] vfio/igd: use PCI ID defines to detect IGD gen
On Thu, 6 Feb 2025 13:13:39 +0100
Corvin Köhne wrote:
> From: Corvin Köhne
>
> 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
> ---
> 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;
> }
>
> /*
