Re: [PATCH 3/4] vfio/igd: use PCI ID defines to detect IGD gen

["Corvin Köhne" <corvin.koehne@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5366 bytes --]

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


-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]