Re: [PATCH v8 2/2] lib/intel_device_info: Query runtime xe device graphics versions

["Summers, Stuart" <stuart.summers@intel.com>]

On Fri, 2025-10-31 at 05:25 +0000, Wang, X wrote:
> 
> 
> > -----Original Message-----
> > From: Summers, Stuart <stuart.summers@intel.com>
> > Sent: Thursday, October 30, 2025 13:51
> > To: igt-dev@lists.freedesktop.org; Wang, X <x.wang@intel.com>
> > Subject: Re: [PATCH v8 2/2] lib/intel_device_info: Query runtime xe
> > device
> > graphics versions
> > 
> > On Mon, 2025-10-20 at 23:12 +0000, Xin Wang wrote:
> > > For platforms with graphics_ver >= 20, query the runtime xe
> > > device ver
> > > instead of relying solely on hardcoded values from the PCI device
> > > table.
> > > This enables accurate IP minor version (graphics_rel) detection
> > > for
> > > platforms like Xe2 where different steppings have different IP
> > > versions.
> > > 
> > > Implementation details:
> > > - Use weak symbol linkage for xe_ipver_cache_lookup() to handle
> > > static
> > >   library compilation (libigt_chipset.a, libigt_device_scan.a)
> > > without
> > >   xe_query.c dependencies which are used for i915 tools
> > > (i915_perf and
> > >   intel_gpu_top)
> > > - Provide a weak stub that returns NULL when xe_query is not
> > > linked
> > > - For Gen20+ platforms, prefer runtime xe device versions over
> > > static
> > > data
> > > - Fall back to PCI table if xe device info is unavailable
> > > - Reset cache on query failure to allow retry
> > > 
> > > Remove hardcoded graphics_rel from static table entries for xe
> > > devices
> > > as they will be populated at runtime from GMD_ID.
> > > 
> > > This unifies device info handling between i915 and xe drivers,
> > > enabling:
> > > - Platform-specific workarounds based on accurate IP versions
> > > - Consistent device info API across both drivers
> > 
> > I'd personally drop this reference to unification of interfaces.
> > The
> > implementations of the query interfaces between xe and i915 are
> > already
> > different and could diverge even further over time, so this isn't a
> > true
> > unification anyway.
> > 
> > > 
> > > Signed-off-by: Xin Wang <x.wang@intel.com>
> > > ---
> > >  lib/intel_device_info.c | 26 +++++++++++++++++++++++---
> > >  1 file changed, 23 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c
> > > index
> > > a853f9ab4..87b1069be 100644
> > > --- a/lib/intel_device_info.c
> > > +++ b/lib/intel_device_info.c
> > > @@ -3,6 +3,16 @@
> > >  #include "i915_pciids_local.h"
> > > 
> > >  #include <strings.h> /* ffs() */
> > > +#include <stddef.h>
> > > +#include <string.h>
> > 
> > Why do you need string.h here?
> 
> +#include <stddef.h>   <-- NULL define
> +#include <string.h>    <-- memcpy()
> 
> The compiler will report error without these 2 headers.

Ah sorry I had missed that... yeah makes sense!

> > 
> > > +
> > > +/* Weak symbol stub - will be overridden if xe_query.c is linked
> > > */
> > > +struct xe_device_ipver *xe_ipver_cache_lookup(uint32_t devid)
> > > __attribute__((weak));
> > > +
> > > +struct xe_device_ipver *xe_ipver_cache_lookup(uint32_t devid) {
> > > +       return NULL;
> > > +}
> > > 
> > >  static const struct intel_device_info intel_generic_info = {
> > >         .graphics_ver = 0,
> > > @@ -505,7 +515,6 @@ static const struct intel_device_info
> > > intel_pontevecchio_info = {
> > > 
> > >  static const struct intel_device_info intel_lunarlake_info = {
> > >         .graphics_ver = 20,
> > 
> > Sorry if I'm coming in late here... why don't we do this for
> > graphics_ver also?
> > Then below you can just either check if graphics_ver == 0 or use
> > intel_gen()?
> 
> Some IGT tools don’t actually open the xe device but still need to
> access intel_get_device_info(). In those tools, the minor version
> isn’t relevant.
> The major version doesn’t change across different devids within the
> same intel_device_info struct, so keeping it here won’t cause any
> issues.

Ok and I guess this also goes with the below comment...

> 
> > 
> > > -       .graphics_rel = 4,
> > >         .display_ver = 20,
> > >         .has_4tile = true,
> > >         .has_flatccs = true,
> > > @@ -517,7 +526,6 @@ static const struct intel_device_info
> > > intel_lunarlake_info = {
> > > 
> > >  static const struct intel_device_info intel_battlemage_info = {
> > >         .graphics_ver = 20,
> > > -       .graphics_rel = 1,
> > >         .display_ver = 14,
> > >         .has_4tile = true,
> > >         .has_flatccs = true,
> > > @@ -529,7 +537,6 @@ static const struct intel_device_info
> > > intel_battlemage_info = {
> > > 
> > >  static const struct intel_device_info intel_pantherlake_info = {
> > >         .graphics_ver = 30,
> > > -       .graphics_rel = 0,
> > >         .display_ver = 30,
> > >         .has_4tile = true,
> > >         .has_flatccs = true,
> > > @@ -675,6 +682,8 @@ const struct intel_device_info
> > > *intel_get_device_info(uint16_t devid)
> > >  {
> > >         static __thread const struct intel_device_info *cache =
> > > &intel_generic_info;
> > >         static __thread uint16_t cached_devid;
> > > +       static __thread struct intel_device_info xe_dev_info;
> > > +       struct xe_device_ipver *ipver;
> > >         int i;
> > > 
> > >         if (cached_devid == devid)
> > > @@ -689,6 +698,17 @@ const struct intel_device_info
> > > *intel_get_device_info(uint16_t devid)
> > >         cached_devid = devid;
> > >         cache = (void *)intel_device_match[i].match_data;
> > > 
> > > +       if (cache->graphics_ver >= 20) {
> > > +               ipver = xe_ipver_cache_lookup(devid);
> > > +               if (ipver && ipver->devid == devid) {
> > 
> > What is the expectation if we don't link in xe_query.c? Seems like
> > it would be
> > better to just make this a hard requirement? Or since there is a
> > dependency
> > here on igt_device_get()/put() we still want this backup if we
> > don't care about
> > the versions?
> > 
> Since this file gets linked as a standalone lib in IGT, some i915
> tools already depend on it. 
> Forcing xe_query.c into the link would require a lot of changes, and
> those tools don’t benefit
> from xe_query.c anyway. I’d prefer not to touch i915 tools that have
> been stable and working for years.

Yeah this is a good point. While I like the idea of having a consistent
interface, especially since we might end up moving fully to an FD-based
implementation, maybe better here to just make this work without a huge
impact across a lot of tests and tools.

Thanks,
Stuart

> 
> 
> > Thanks,
> > Stuart
> > 
> > 
> > > +                       memcpy(&xe_dev_info, cache, sizeof(struct
> > > intel_device_info));
> > > +                       xe_dev_info.graphics_ver = ipver-
> > > > graphics_ver;
> > > +                       xe_dev_info.graphics_rel = ipver-
> > > > graphics_rel;
> > > +                       cache = &xe_dev_info;
> > > +               } else {
> > > +                       cached_devid = 0;
> > > +               }
> > > +       }
> > >  out:
> > >         return cache;
> > >  }
>