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

Matt Roper <[email protected]> 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 <[email protected]>
>> ---
>>  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.

--
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