Re: [PATCH i-g-t v13 2/4] lib/intel_device_info: make device info cache process-wide

["Wang, X" <x.wang@intel.com>]

On 9/24/2026 23:22, Zbigniew Kempczyński wrote:
> On Wed, Sep 23, 2026 at 08:55:38PM -0700, Xin Wang wrote:
>> intel_get_device_info() caches the last looked-up entry in per-thread
>> variables that point into the static, read-only PCI-ID table. Such a
>> cache cannot carry data that is updated at runtime, e.g. the graphics
>> IP version reported by GMD_ID on Xe platforms.
>>
>> Replace it with a process-wide cache keyed by devid which holds a
>> mutable copy of the matching static table entry. The cache is protected
>> by a statically initialized mutex, the map is created lazily on the
>> first lookup so it does not depend on constructor ordering, and it is
>> torn down via igt_destructor. If a cache entry cannot be allocated,
>> fall back to the static table entry.
> Generally series looks correct, but I got few nits. Returning static
> entry is incorrect, especially when patch 4/4 drops rel field.
>
>> Also link igt_map.c into libigt_chipset to provide the map implementation.
>>
>> Signed-off-by: Xin Wang <x.wang@intel.com>
>> ---
>>   lib/intel_device_info.c | 106 ++++++++++++++++++++++++++++++++++------
>>   lib/meson.build         |   1 +
>>   2 files changed, 92 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c
>> index ae316bfcab..a7ba40ed2b 100644
>> --- a/lib/intel_device_info.c
>> +++ b/lib/intel_device_info.c
>> @@ -1,8 +1,11 @@
>>   #include "intel_chipset.h"
>>   #include "pciids.h"
>>   #include "i915_pciids_local.h"
>> +#include "igt_core.h"
>> +#include "igt_map.h"
>>   
>>   #include <ctype.h>
>> +#include <pthread.h>
>>   #include <strings.h> /* ffs() */
>>   
>>   static const struct intel_device_info intel_generic_info = {
>> @@ -716,6 +719,59 @@ static const struct pci_id_match intel_device_match[] = {
>>   
>>   #undef INTEL_PCI_ID_INIT
>>   
>> +/*
>> + * Process-wide cache of per-devid copies of the static PCI-ID table entries.
>> + * Entries can be updated at runtime, e.g. by xe_device_get() with the graphics
>> + * IP version reported by GMD_ID. The cache is keyed by PCI device ID, so all
>> + * devices sharing a device ID share one entry.
>> + *
>> + * The map is created lazily on first lookup, so lookups do not depend on
>> + * constructor ordering.
>> + */
>> +static struct {
>> +	pthread_mutex_t mutex;
>> +	struct igt_map *map;
>> +} devinfo_cache = {
>> +	.mutex = PTHREAD_MUTEX_INITIALIZER,
>> +};
>> +
>> +static void free_device_info(struct igt_map_entry *entry)
>> +{
>> +	free(entry->data);
>> +	free((void *)entry->key);
>> +}
>> +
>> +igt_destructor
>> +{
>> +	pthread_mutex_lock(&devinfo_cache.mutex);
>> +	igt_map_destroy(devinfo_cache.map, free_device_info);
>> +	devinfo_cache.map = NULL;
>> +	pthread_mutex_unlock(&devinfo_cache.mutex);
>> +}
> Imo this destructor path is not necessary here. There's no other
> cleanups than memory release what will happen during process exit
> anyway.
Hi Zbigniew,

Thank you for the review.

On the destructor: I understand that the OS reclaims this memory at
process exit. I still prefer explicit teardown here: this library
allocates the cache map, keys, and device-info copies, and the
destructor releases them when the library is finalized. This keeps
allocation and cleanup in the same library.

While running Xe tests under Valgrind, I noticed similar process-lifetime
allocations in other IGT libraries. I plan to examine those separately;
they are outside the scope of this series.

>> +
>> +/* Caller must hold devinfo_cache.mutex. */
>> +static struct intel_device_info *devinfo_cache_search(uint16_t devid)
>> +{
>> +	uint32_t key = devid;
>> +
>> +	if (!devinfo_cache.map)
>> +		return NULL;
>> +
>> +	return igt_map_search(devinfo_cache.map, &key);
>> +}
>> +
>> +static const struct intel_device_info *devinfo_table_lookup(uint16_t devid)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; intel_device_match[i].device_id != PCI_MATCH_ANY; i++) {
>> +		if (devid == intel_device_match[i].device_id)
>> +			break;
>> +	}
>> +
>> +	return (const struct intel_device_info *)intel_device_match[i].match_data;
>> +}
>> +
>>   /**
>>    * intel_get_device_info:
>>    * @devid: pci device id
>> @@ -727,24 +783,44 @@ static const struct pci_id_match intel_device_match[] = {
>>    */
>>   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;
>> -	int i;
>> -
>> -	if (cached_devid == devid)
>> -		goto out;
>> -
>> -	/* XXX Presort table and bsearch! */
>> -	for (i = 0; intel_device_match[i].device_id != PCI_MATCH_ANY; i++) {
>> -		if (devid == intel_device_match[i].device_id)
>> -			break;
>> +	const struct intel_device_info *table_info;
>> +	struct intel_device_info *info, *new_info;
>> +	uint32_t *new_key;
>> +
>> +	pthread_mutex_lock(&devinfo_cache.mutex);
>> +	info = devinfo_cache_search(devid);
>> +	pthread_mutex_unlock(&devinfo_cache.mutex);
>> +	if (info)
>> +		return info;
>> +
>> +	table_info = devinfo_table_lookup(devid);
>> +
>> +	new_key = malloc(sizeof(*new_key));
>> +	new_info = malloc(sizeof(*new_info));
> This part should report allocation failure, otherwise we use static
> entry and we even don't know about it. And with 4/4 patch it is useless
> anyway.

On the allocation-failure path: agreed. Returning the static entry can
silently report an incorrect graphics_rel after patch 4, and without a
cache entry the runtime GMD_ID version cannot be stored. I've updated
the local patch to report malloc, map creation, and insertion failures
to stderr and exit with failure. The mutex is released before reporting
an insertion failure. I'll include this change in the next revision.

Thanks
Xin
> --
> Zbigniew
>
>> +	if (new_key && new_info) {
>> +		*new_key = devid;
>> +		*new_info = *table_info;
>> +
>> +		pthread_mutex_lock(&devinfo_cache.mutex);
>> +		if (!devinfo_cache.map)
>> +			devinfo_cache.map = igt_map_create(igt_map_hash_32,
>> +							   igt_map_equal_32);
>> +		/* Another thread may have inserted while we were allocating. */
>> +		info = devinfo_cache_search(devid);
>> +		if (!info && devinfo_cache.map &&
>> +		    igt_map_insert(devinfo_cache.map, new_key, new_info)) {
>> +			info = new_info;
>> +			new_key = NULL;
>> +			new_info = NULL;
>> +		}
>> +		pthread_mutex_unlock(&devinfo_cache.mutex);
>>   	}
>>   
>> -	cached_devid = devid;
>> -	cache = (void *)intel_device_match[i].match_data;
>> +	free(new_info);
>> +	free(new_key);
>>   
>> -out:
>> -	return cache;
>> +	/* Without a cache entry, fall back to the static table entry. */
>> +	return info ?: table_info;
>>   }
>>   
>>   static bool char_eq(char c1, char c2)
>> diff --git a/lib/meson.build b/lib/meson.build
>> index 2ac6327587..7d1502ef48 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -397,6 +397,7 @@ igt_deps = [ lib_igt ] + lib_deps
>>   lin_igt_chipset_build = static_library('igt_chipset',
>>                                          ['intel_chipset.c',
>>   					'intel_device_info.c',
>> +					'igt_map.c',
>>   					'intel_cmds_info.c'],
>>                                          include_directories : inc)
>>   
>> -- 
>> 2.43.0
>>