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

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

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?

> +
> +/* 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()?

> -       .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?

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;
>  }