Re: [PATCH v8 1/2] lib/xe/xe_query: Get runtime xe device graphics version from GMD_ID

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

On Mon, 2025-10-20 at 23:12 +0000, Xin Wang wrote:
> This allows IGT to query the exact IP version for xe platforms.
> 
> Key changes:
> - Add xe_device_ipver field to xe_device structure
> - set the graphics versions based on the GMD_ID
> - Cache device ipver in global map indexed by devid for efficient
> lookup
> - Implement xe_ipver_cache_lookup() to retrieve cached ipver by devid
> - Clean up cached device ipver when xe_device is released
> 
> V2:
> - add new struct xe_device_ipver to hold the ipver info
> - separate cache map to eliminate collision (Roper, Matthew D)
> - changed function name to xe_ipver_cache_lookup() to avoid
>   confusion (Roper, Matthew D)
> 
> Signed-off-by: Xin Wang <x.wang@intel.com>
> ---
>  lib/intel_chipset.h |  6 +++++
>  lib/xe/xe_query.c   | 53
> ++++++++++++++++++++++++++++++++++++++++++++-
>  lib/xe/xe_query.h   |  4 ++++
>  3 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
> index 2f6bf788a..2dd214413 100644
> --- a/lib/intel_chipset.h
> +++ b/lib/intel_chipset.h
> @@ -98,6 +98,12 @@ struct intel_device_info {
>         const char *codename;
>  };
>  
> +struct xe_device_ipver {
> +       uint32_t devid;
> +       uint16_t graphics_ver;
> +       uint16_t graphics_rel;
> +};
> +
>  const struct intel_device_info *intel_get_device_info(uint16_t
> devid) __attribute__((pure));
>  
>  const struct intel_cmds_info *intel_get_cmds_info(uint16_t devid)
> __attribute__((pure));
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index a89e0b980..5d8accd47 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -319,6 +319,22 @@ static struct xe_device_cache {
>         struct igt_map *map;
>  } cache;
>  
> +static struct xe_ipver_cache {
> +       pthread_mutex_t mutex;
> +       struct igt_map *map;
> +} xe_ipver;
> +
> +struct xe_device_ipver *xe_ipver_cache_lookup(uint32_t devid)
> +{
> +       struct xe_device_ipver *ipver;
> +
> +       pthread_mutex_lock(&xe_ipver.mutex);
> +       ipver = igt_map_search(xe_ipver.map, &devid);
> +       pthread_mutex_unlock(&xe_ipver.mutex);
> +
> +       return ipver;
> +}
> +
>  static struct xe_device *find_in_cache_unlocked(int fd)
>  {
>         return igt_map_search(cache.map, &fd);
> @@ -379,6 +395,23 @@ struct xe_device *xe_device_get(int fd)
>         for (int gt = 0; gt < xe_dev->gt_list->num_gt; gt++)
>                 xe_dev->gt_mask |= (1ull << xe_dev->gt_list-
> >gt_list[gt].gt_id);
>  
> +       /*
> +       * Set graphics_ver and graphics_rel based on the main GT's
> GMD_ID.
> +       * We should use the hardcoded value for the none GMD_ID

/s/none /non-/

> platforms (ip_ver_major == 0)
> +       */
> +       xe_dev->ipver.devid = 0;
> +       for (int gt = 0; gt < xe_dev->gt_list->num_gt; gt++)

Please add brackets here. Otherwise it's too easy to add extra code
below the if() {} which would then be out of scope, so:
for () {
    if () {
         /* multi-line code */
    }
}

> +               if (xe_dev->gt_list->gt_list[gt].type ==
> DRM_XE_QUERY_GT_TYPE_MAIN &&

We talked offline about how we might be moving to an FD-based approach
here rather than a GMD ID based approach for platform support of
features. Otherwise I'd recommend doing this across all of the GTs.

> +                   xe_dev->gt_list->gt_list[gt].ip_ver_major) {
> +                       igt_debug("Setting graphics_ver to %u and
> graphics_rel to %u\n",
> +                                 xe_dev->gt_list-
> >gt_list[gt].ip_ver_major,
> +                                 xe_dev->gt_list-
> >gt_list[gt].ip_ver_minor);
> +                       xe_dev->ipver.graphics_ver = xe_dev->gt_list-
> >gt_list[gt].ip_ver_major;
> +                       xe_dev->ipver.graphics_rel = xe_dev->gt_list-
> >gt_list[gt].ip_ver_minor;
> +                       xe_dev->ipver.devid = xe_dev->dev_id;
> +                       break;
> +               }
> +
>         /* Tile IDs may be non-consecutive; keep a mask of valid IDs
> */
>         for (int gt = 0; gt < xe_dev->gt_list->num_gt; gt++)
>                 xe_dev->tile_mask |= (1ull << xe_dev->gt_list-
> >gt_list[gt].tile_id);
> @@ -413,6 +446,11 @@ struct xe_device *xe_device_get(int fd)
>         prev = find_in_cache_unlocked(fd);
>         if (!prev) {
>                 igt_map_insert(cache.map, &xe_dev->fd, xe_dev);
> +               if (xe_dev->ipver.devid) {
> +                       pthread_mutex_lock(&xe_ipver.mutex);
> +                       igt_map_insert(xe_ipver.map, &xe_dev-
> >ipver.devid, &xe_dev->ipver);
> +                       pthread_mutex_unlock(&xe_ipver.mutex);
> +               }
>         } else {
>                 xe_device_free(xe_dev);
>                 xe_dev = prev;
> @@ -424,7 +462,15 @@ struct xe_device *xe_device_get(int fd)
>  
>  static void delete_in_cache(struct igt_map_entry *entry)
>  {
> -       xe_device_free((struct xe_device *)entry->data);
> +       struct xe_device *xe_dev = (struct xe_device *)entry->data;
> +
> +       if (xe_dev->ipver.devid) {
> +               pthread_mutex_lock(&xe_ipver.mutex);
> +               igt_map_remove(xe_ipver.map, &xe_dev->ipver.devid,
> NULL);

I feel like this should be done in the outer layer, xe_device_put(),
rather than here. So xe_device_put() becomes:
if (find_in_cache_unlocked(fd)) {
    igt_map_remove(cache.map, &fd, delete_in_cache);
    igt_map_remove(cache.map, &xe_dev->ipver.devid, NULL);
}

This also follows the other init/alloc locations where we touch
xe_ipver.map.

Thanks,
Stuart

> +               pthread_mutex_unlock(&xe_ipver.mutex);
> +       }
> +
> +       xe_device_free(xe_dev);
>  }
>  
>  /**
> @@ -474,13 +520,18 @@ static void xe_device_destroy_cache(void)
>         pthread_mutex_lock(&cache.cache_mutex);
>         igt_map_destroy(cache.map, delete_in_cache);
>         pthread_mutex_unlock(&cache.cache_mutex);
> +       pthread_mutex_lock(&xe_ipver.mutex);
> +       igt_map_destroy(xe_ipver.map, NULL);
> +       pthread_mutex_unlock(&xe_ipver.mutex);
>  }
>  
>  static void xe_device_cache_init(void)
>  {
>         pthread_mutex_init(&cache.cache_mutex, NULL);
> +       pthread_mutex_init(&xe_ipver.mutex, NULL);
>         xe_device_destroy_cache();
>         cache.map = igt_map_create(igt_map_hash_32,
> igt_map_equal_32);
> +       xe_ipver.map = igt_map_create(igt_map_hash_32,
> igt_map_equal_32);
>  }
>  
>  #define xe_dev_FN(_NAME, _FIELD, _TYPE) \
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index 715b64e2f..4e8ba0372 100644
> --- a/lib/xe/xe_query.h
> +++ b/lib/xe/xe_query.h
> @@ -74,6 +74,9 @@ struct xe_device {
>  
>         /** @dev_id: Device id of xe device */
>         uint16_t dev_id;
> +
> +       /** @ipver: Device ip version */
> +       struct xe_device_ipver ipver;
>  };
>  
>  #define xe_for_each_engine(__fd, __hwe) \
> @@ -140,6 +143,7 @@ int xe_query_pxp_status(int fd);
>  int xe_wait_for_pxp_init(int fd);
>  
>  struct xe_device *xe_device_get(int fd);
> +struct xe_device_ipver *xe_ipver_cache_lookup(uint32_t devid);
>  void xe_device_put(int fd);
>  
>  #endif /* XE_QUERY_H */