Re: [PATCH i-g-t 04/10] lib/xe: Cache workaround information in xe_device

[Gustavo Sousa <gustavo.sousa@intel.com>]

Gustavo Sousa <gustavo.sousa@intel.com> writes:

> Matt Roper <matthew.d.roper@intel.com> writes:
>
>> On Tue, Jul 21, 2026 at 03:59:57PM -0300, Gustavo Sousa wrote:
>>> One existing inconvenience with xe_wa() is that it needs to dump from
>>> debugfs every time it is called.  A more concerning issue is that the
>>> dump will fail if xe_wa() is called from a context without the
>>> required privileges.
>>> 
>>> We currently have only one user of xe_wa(), but shortly we will have
>>> another user that will need to check for a certain workaround in a
>>> non-root user context (in test cases that use igt_drop_root()).
>>> 
>>> Let's resolve those issues by ensuring that xe_device_get() caches the
>>> workaround information.  Tests that need to check for workarounds in
>>> underprivileged context will need to make sure to call xe_device_get()
>>> before dropping privileges.
>>> 
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  lib/xe/xe_query.c |  6 ++++++
>>>  lib/xe/xe_query.h |  9 +++++++--
>>>  lib/xe/xe_wa.c    | 37 ++++++++++++++++++++++++++++---------
>>>  lib/xe/xe_wa.h    |  5 +++++
>>>  4 files changed, 46 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
>>> index 68e60ddc75a2..ea095b207534 100644
>>> --- a/lib/xe/xe_query.c
>>> +++ b/lib/xe/xe_query.c
>>> @@ -28,6 +28,7 @@
>>>  
>>>  #include "xe_query.h"
>>>  #include "xe_ioctl.h"
>>> +#include "xe_wa.h"
>>>  
>>>  /**
>>>   * xe_query_device_may_fail:
>>> @@ -377,6 +378,9 @@ static void xe_device_free(struct xe_device *xe_dev)
>>>  	free(xe_dev->vram_size);
>>>  	free(xe_dev->eu_stall);
>>>  	free(xe_dev->pat_cache);
>>> +
>>> +	xe_wa_free_cache(xe_dev);
>>> +
>>>  	free(xe_dev);
>>>  }
>>>  
>>> @@ -441,6 +445,8 @@ struct xe_device *xe_device_get(int fd)
>>>  	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_regions);
>>>  	xe_dev->has_vram = __mem_has_vram(xe_dev->mem_regions);
>>>  
>>> +	xe_wa_build_cache(xe_dev);
>>> +
>>>  	/*
>>>  	 * Populate the PAT cache while we still have sufficient privileges
>>>  	 * to read debugfs.  Forked children that inherit this xe_device
>>> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
>>> index 59330d80fd1b..f70476945dee 100644
>>> --- a/lib/xe/xe_query.h
>>> +++ b/lib/xe/xe_query.h
>>> @@ -80,6 +80,9 @@ struct xe_device {
>>>  	/** @pat_cache: cached PAT index configuration, NULL if not yet populated */
>>>  	struct intel_pat_cache *pat_cache;
>>>  
>>> +	/** @wa_cache: cached data for xe_wa() and related functions. */
>>> +	void *wa_cache;
>>> +
>>>  	/**
>>>  	 * @multi_lrc_mask: bitmask of engine classes supporting multi-LRC.
>>>  	 * UINT16_MAX if not available (older kernel).
>>> @@ -99,10 +102,12 @@ struct xe_device {
>>>  #define xe_for_each_engine_class(__class) \
>>>  	for (__class = 0; __class < DRM_XE_ENGINE_CLASS_COMPUTE + 1; \
>>>  	     ++__class)
>>> -#define xe_for_each_gt(__fd, __gt) \
>>> -	for (uint64_t igt_unique(__mask) = xe_device_get(__fd)->gt_mask; \
>>> +#define xe_for_each_gt_from_mask(__gt_mask, __gt) \
>>
>> The gt_mask changes in this commit don't seem necessary/related to the
>> rest of the patch?  We still pass xe_dev->gt_mask to
>> get_wa_debugfs_dumps() so it doesn't seem like there's any change of
>> behavior related to this.
>
> The difference here is that xe_for_each_gt_from_mask() will not attempt
> to do a xe_device_get() to get the mask.  Now that we are caching the wa
> data as part of xe_device_get(), using xe_for_each_gt() would cause an
> infinite recursion.

I added a note in the v2's commit message to clarify the need for the
new xe_for_each_gt_from_mask() macro.

Based on your feedback to patch #08, another change I did for v2 is to
have a type for the cache: struct xe_wa_cache.

--
Gustavo Sousa

>
> --
> Gustavo Sousa
>
>>
>>
>> Matt
>>
>>> +	for (uint64_t igt_unique(__mask) = __gt_mask; \
>>>  	     __gt = ffsll(igt_unique(__mask)) - 1, igt_unique(__mask) != 0; \
>>>  	     igt_unique(__mask) &= ~(1ull << __gt))
>>> +#define xe_for_each_gt(__fd, __gt) \
>>> +	xe_for_each_gt_from_mask(xe_device_get(__fd)->gt_mask, __gt)
>>>  #define xe_for_each_tile(__fd, __tile) \
>>>  	for (uint64_t igt_unique(__mask) = xe_device_get(__fd)->tile_mask; \
>>>  	     __tile = ffsll(igt_unique(__mask)) - 1, igt_unique(__mask) != 0; \
>>> diff --git a/lib/xe/xe_wa.c b/lib/xe/xe_wa.c
>>> index e4e1b0bb7972..0f4d2edc0795 100644
>>> --- a/lib/xe/xe_wa.c
>>> +++ b/lib/xe/xe_wa.c
>>> @@ -22,14 +22,14 @@ static void free_wa_debugfs_dumps(char **dumps)
>>>  	free(dumps);
>>>  }
>>>  
>>> -static char **get_wa_debugfs_dumps(int fd)
>>> +static char **get_wa_debugfs_dumps(int fd, uint64_t gt_mask)
>>>  {
>>>  	char **dumps;
>>>  	int gt;
>>>  	int debugfs_fd;
>>>  	int count = 1; /* Device workarounds */
>>>  
>>> -	xe_for_each_gt(fd, gt)
>>> +	xe_for_each_gt_from_mask(gt_mask, gt)
>>>  		count++;
>>>  
>>>  	dumps = calloc(count + 1, sizeof(*dumps));
>>> @@ -45,7 +45,7 @@ static char **get_wa_debugfs_dumps(int fd)
>>>  	if (!(dumps[count++] = igt_sysfs_get(debugfs_fd, "workarounds")))
>>>  		goto err;
>>>  
>>> -	xe_for_each_gt(fd, gt) {
>>> +	xe_for_each_gt_from_mask(gt_mask, gt) {
>>>  		char name[32];
>>>  
>>>  		snprintf(name, sizeof(name), "gt%d/workarounds", gt);
>>> @@ -112,17 +112,36 @@ static bool debugfs_dump_has_wa(char *dump, const char *wa)
>>>   */
>>>  int xe_wa(int fd, const char *wa)
>>>  {
>>> -	char **dumps = get_wa_debugfs_dumps(fd);
>>> -	int ret = 0;
>>> +	char **dumps = xe_device_get(fd)->wa_cache;
>>>  
>>>  	if (igt_warn_on(!dumps))
>>>  		return -1;
>>>  
>>>  	for (char **dump = dumps; *dump; dump++)
>>> -		if ((ret = debugfs_dump_has_wa(*dump, wa)))
>>> -			break;
>>> +		if (debugfs_dump_has_wa(*dump, wa))
>>> +			return 1;
>>>  
>>> -	free_wa_debugfs_dumps(dumps);
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * xe_wa_build_cache: Build cached data for xe_wa().
>>> + * @xe_dev: Xe device where the cache will be stashed.
>>> + */
>>> +void xe_wa_build_cache(struct xe_device *xe_dev)
>>> +{
>>> +	xe_dev->wa_cache = get_wa_debugfs_dumps(xe_dev->fd, xe_dev->gt_mask);
>>> +}
>>> +
>>> +/**
>>> + * xe_wa_free_cache: Free cached data that was built with xe_wa_build_cache().
>>> + * @xe_dev: Xe device where the cached data is stashed.
>>> + */
>>> +void xe_wa_free_cache(struct xe_device *xe_dev)
>>> +{
>>> +	if (!xe_dev->wa_cache)
>>> +		return;
>>>  
>>> -	return ret;
>>> +	free_wa_debugfs_dumps(xe_dev->wa_cache);
>>> +	xe_dev->wa_cache = NULL;
>>>  }
>>> diff --git a/lib/xe/xe_wa.h b/lib/xe/xe_wa.h
>>> index 4ff897196545..f0a826553df8 100644
>>> --- a/lib/xe/xe_wa.h
>>> +++ b/lib/xe/xe_wa.h
>>> @@ -6,6 +6,11 @@
>>>  #ifndef XE_WA_H
>>>  #define XE_WA_H
>>>  
>>> +struct xe_device;
>>> +
>>>  int xe_wa(int fd, const char *wa);
>>>  
>>> +void xe_wa_build_cache(struct xe_device *xe_dev);
>>> +void xe_wa_free_cache(struct xe_device *xe_dev);
>>> +
>>>  #endif /* XE_WA_H */
>>> 
>>> -- 
>>> 2.55.0
>>> 
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation

>  LocalWords:  xe struct wa