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

["Vodapalli, Ravi Kumar" <ravi.kumar.vodapalli@intel.com>]

On 12/12/2025 10:19 PM, Wang, X wrote:
>
>> -----Original Message-----
>> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of
>> Vodapalli, Ravi Kumar
>> Sent: Friday, December 12, 2025 07:56
>> To: igt-dev@lists.freedesktop.org
>> Subject: Re: [PATCH v8 2/2] lib/intel_device_info: Query runtime xe device
>> graphics versionsth
>>
>>
>>
>> On 11/18/2025 11:41 AM, Wang, X wrote:
>>>> -----Original Message-----
>>>> From: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>>> Sent: Friday, November 7, 2025 11:48
>>>> To: Wang, X <x.wang@intel.com>
>>>> Cc: igt-dev@lists.freedesktop.org; Summers, Stuart
>>>> <stuart.summers@intel.com>
>>>> Subject: Re: [PATCH v8 2/2] lib/intel_device_info: Query runtime xe
>>>> device graphics versions
>>>>
>>>> Hi Xin,
>>>> On 2025-10-20 at 23:12:53 +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
>>>> +Cc: "Stuart Summers" <stuart.summers@intel.com>
>>>>> Signed-off-by: Xin Wang <x.wang@intel.com>
Reviewed-by: Ravi Kumar V  <ravi.kumar.vodapalli@intel.com>

Regards,
Ravi Kumar V
>>>>> ---
>>>>>    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>
>>>>> +
>>>>> +/* 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;
>>>>> +}
>>>>>
>>>> This seems wrong, you cannot cache by devid, either you already have
>>>> it or
>>> Why can't cache the ipver with the devid ??  We are not expecting to read the
>> register every time, so we must cache the value somewhere.
>>>> not. Also devid depends on register(s) reading and you can access
>>>> that only when you have fd already opened.
>>>>
>>>>>    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,
>>>>> -	.graphics_rel = 4,
>>> In fact the purpose of the patch is to fix the .graphics_rel error on some
>> devices. And we already have the correct value so we can safely remove it.
>>> I keep the  .graphics_ver here is for some of the tools may need to
>>> check the graphics ver without opening the xe devices. For this kind of use
>> case the tool is not interested in the .graphics_rel.
>>>> This also seems wrong, please keep old values for compatibility, here
>>>> and below.
>>>>
>>>> Regards,
>>>> Kamil
>>>>
>>>>>    	.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) {
>>> xe_device_get(fd) (in xe_query.c) records the tuple (graphics_ver,
>> graphics_rel, devid) into xe_ipver.map once it has fetched
>> DRM_XE_QUERY_GT_LIST/CONFIG. The key is the PCI devid, the value is the IP
>> version we just read from GMD_ID.
>>> Every Xe use‑case calls xe_device_get(fd) when opening the device: the
>>> common drm_open_driver{,_another}() path already does this
>>> automatically (see lib/drmtest.c
>>>
>>>
>>>>> +		ipver = xe_ipver_cache_lookup(devid);
>>> xe_ipver_cache_lookup() is only a map lookup. If a given build(i915 only
>> tools and tests) doesn’t link in xe_query.c, the weak stub returns NULL and
>> intel_get_device_info() will not go to the if (cache->graphics_ver >= 20) { ...}
>> part in the intel_get_device_info() function so we still make the code
>> compatible with i915 devices.
>>>>> +		if (ipver && ipver->devid == devid) {
>>>>> +			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;
>> cache variable is a constant type we cannot update it, did the code compiled
>> without error.
>>
> cache is declared as const struct intel_device_info *cache, so the pointee is const, but the pointer itself is mutable.
> Reassigning cache = &xe_dev_info; is allowed; what’s forbidden is mutating the fields through cache.
> The code builds cleanly (no warnings or errors).
>
> Xin
>> Regards,
>> Ravi Kumar V
>>
>>>>> +		} else {
>>>>> +			cached_devid = 0;
>>> This function will be called before the drm_open_driver() so we should
>> remove the cache here.
>>> codename_intel()  is calling intel_get_device_info() at very early
>>> time in igt_device_scan.c
>>>>> +		}
>>>>> +	}
>>>>>    out:
>>>>>    	return cache;
>>>>>    }
>>>>> --
>>>>> 2.43.0
>>>>>