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 Fri, 2025-10-31 at 05:09 +0000, Wang, X wrote:
> 
> 
> > -----Original Message-----
> > From: Summers, Stuart <stuart.summers@intel.com>
> > Sent: Thursday, October 30, 2025 14:20
> > To: igt-dev@lists.freedesktop.org; Wang, X <x.wang@intel.com>
> > Subject: Re: [PATCH v8 1/2] lib/xe/xe_query: Get runtime xe device
> > graphics
> > version from GMD_ID
> > 
> > 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 */
> >     }
> > }
> Good point! Will change it in next version.
> 
> > 
> > > +               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.
> 
> The graphics version is for the MAIN GT, do you mean we should also
> save the media GT version somewhere? 

Right.. but also we don't have tests using this right now, so not worth
looking at a more generic structure here..

> 
> > 
> > > +                   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.
> 
> The xe_dev has been freed in the callback function delete_in_cache(),
> I think we should call the igt_map_remove(xe_ipver.map, &xe_dev-
> >ipver.devid,null) instead of it.
> Otherwise, the xe_dev->ipver.devid will become null. 

True.. I should have swapped the order there..:
if (find_in_cache_unlocked(xe_dev->ipver.devid))
    igt_map_remove(cache.map, &xe_dev->ipver.devid, NULL);
/* Should be called last since xe_dev is removed in delete_in_cache. */
if (find_in_cache_unlocked(fd))
    igt_map_remove(cache.map, &fd, delete_in_cache);

Just to maintain consistency in the alloc and dealloc routines. But
also this is going to need a comment as above for the reason you
mentioned.

Thanks,
Stuart

> 
> Thanks, 
> Xin
> 
> 
> > 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 */
>