Re: [PATCH i-g-t 08/10] lib/intel_pat: Encapsulate management of xe_device's pat_cache

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

Matt Roper <matthew.d.roper@intel.com> writes:

> On Tue, Jul 21, 2026 at 04:00:01PM -0300, Gustavo Sousa wrote:
>> Currently xe_device_get() manages the PAT cached information
>> information by itself, by doing the necessary memory
>> allocation/deallocation and calling xe_get_pat_sw_config() to
>> initialize the cached information.
>> 
>> Such management is arguably better suited to be implemented as part of
>> the intel_pat module and encapsulated such that xe_query doesn't need
>> to know the details.
>> 
>> We will introduce changes that will require an extra logic for
>> initializing the cache and having such encapsulation makes it much
>> easier to implement.  Since we will touch this area, take this
>> opportunity to move the existing management to intel_pat.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  lib/intel_pat.c   | 27 ++++++++++++++++++++++++++-
>>  lib/intel_pat.h   |  5 +++++
>>  lib/xe/xe_query.c |  9 ++-------
>>  lib/xe/xe_query.h |  4 +---
>>  4 files changed, 34 insertions(+), 11 deletions(-)
>> 
>> diff --git a/lib/intel_pat.c b/lib/intel_pat.c
>> index 2f35bb77249f..14935e3c4692 100644
>> --- a/lib/intel_pat.c
>> +++ b/lib/intel_pat.c
>> @@ -144,6 +144,31 @@ int32_t xe_get_pat_hw_config(int drm_fd, struct intel_pat_cache *xe_pat_cache, i
>>  	return xe_get_pat_config(drm_fd, xe_pat_cache, gt, PAT_HW_CONFIG);
>>  }
>>  
>> +void intel_pat_build_xe_cache(struct xe_device *xe_dev)
>> +{
>> +	struct intel_pat_cache *pat_cache;
>> +
>> +	pat_cache = calloc(1, sizeof(*pat_cache));
>> +	igt_assert(pat_cache);
>> +
>> +	if (igt_debug_on(xe_get_pat_sw_config(xe_dev->fd, pat_cache, 0) <= 0)) {
>> +		free(pat_cache);
>> +		return;
>> +	}
>> +
>> +	xe_dev->pat_cache = pat_cache;
>> +}
>> +
>> +void intel_pat_free_xe_cache(struct xe_device *xe_dev)
>> +{
>> +	if (!xe_dev->pat_cache)
>> +		return;
>> +
>> +	free(xe_dev->pat_cache);
>> +
>> +	xe_dev->pat_cache = NULL;
>> +}
>> +
>>  /*
>>   * Hardcoded PAT indices for Xe platforms, used as a fallback when the
>>   * kernel doesn't expose gt0/pat_sw_config in debugfs.
>> @@ -217,7 +242,7 @@ static void intel_get_pat_idx(int fd, struct intel_pat_cache *pat)
>>  		struct xe_device *xe_dev = xe_device_get(fd);
>>  
>>  		if (xe_dev->pat_cache) {
>> -			*pat = *xe_dev->pat_cache;
>> +			*pat = *((struct intel_pat_cache *)xe_dev->pat_cache);
>>  		} else if (xe_pat_fallback(fd, pat)) {
>>  			igt_info("PAT sw_config debugfs not available, "
>>  				 "using hardcoded fallback\n");
>> diff --git a/lib/intel_pat.h b/lib/intel_pat.h
>> index a31b60e86e2a..fc9ef9b650bf 100644
>> --- a/lib/intel_pat.h
>> +++ b/lib/intel_pat.h
>> @@ -13,6 +13,8 @@
>>  #define XE_PAT_IDX_INVALID ((uint8_t)-2) /* no such PAT index on this platform */
>>  #define XE_PAT_MAX_ENTRIES 32
>>  
>> +struct xe_device;
>> +
>>  struct xe_pat_entry {
>>  	uint32_t pat;
>>  	bool rsvd;
>> @@ -40,4 +42,7 @@ uint8_t intel_get_pat_idx_uc_comp(int fd);
>>  int32_t xe_get_pat_sw_config(int drm_fd, struct intel_pat_cache *xe_pat_cache, int gt);
>>  int32_t xe_get_pat_hw_config(int drm_fd, struct intel_pat_cache *xe_pat_cache, int gt);
>>  
>> +void intel_pat_build_xe_cache(struct xe_device *xe_dev);
>> +void intel_pat_free_xe_cache(struct xe_device *xe_dev);
>> +
>>  #endif /* INTEL_PAT_H */
>> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
>> index ea095b207534..aa81db45a41a 100644
>> --- a/lib/xe/xe_query.c
>> +++ b/lib/xe/xe_query.c
>> @@ -377,8 +377,8 @@ static void xe_device_free(struct xe_device *xe_dev)
>>  	free(xe_dev->mem_regions);
>>  	free(xe_dev->vram_size);
>>  	free(xe_dev->eu_stall);
>> -	free(xe_dev->pat_cache);
>>  
>> +	intel_pat_free_xe_cache(xe_dev);
>>  	xe_wa_free_cache(xe_dev);
>>  
>>  	free(xe_dev);
>> @@ -458,12 +458,7 @@ struct xe_device *xe_device_get(int fd)
>>  	 * should be extended to cache PAT entries by platform version/
>>  	 * revision instead.
>>  	 */
>> -	xe_dev->pat_cache = calloc(1, sizeof(*xe_dev->pat_cache));
>> -	igt_assert(xe_dev->pat_cache);
>> -	if (xe_get_pat_sw_config(xe_dev->fd, xe_dev->pat_cache, 0) <= 0) {
>> -		free(xe_dev->pat_cache);
>> -		xe_dev->pat_cache = NULL;
>> -	}
>> +	intel_pat_build_xe_cache(xe_dev);
>>  
>>  	/* We may get here from multiple threads, use first cached xe_dev */
>>  	pthread_mutex_lock(&cache.cache_mutex);
>> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
>> index f70476945dee..82f72314f072 100644
>> --- a/lib/xe/xe_query.h
>> +++ b/lib/xe/xe_query.h
>> @@ -20,8 +20,6 @@
>>  #define XE_DEFAULT_ALIGNMENT           SZ_4K
>>  #define XE_DEFAULT_ALIGNMENT_64K       SZ_64K
>>  
>> -struct intel_pat_cache;
>> -
>>  struct xe_device {
>>  	/** @fd: xe fd */
>>  	int fd;
>> @@ -78,7 +76,7 @@ struct xe_device {
>>  	uint16_t dev_id;
>>  
>>  	/** @pat_cache: cached PAT index configuration, NULL if not yet populated */
>> -	struct intel_pat_cache *pat_cache;
>> +	void *pat_cache;
>
> Is there a need to change this to a void pointer?  It seems like we can
> just leave this as-is and avoid the cast earlier.

The rationale here is that intel_pat is the only unit supposed to know
the details of the cached information, so I think it is best to make
this a void pointer to make it explicit and also to keep other units
from using it in the future without going through the proper functions
provided by intel_pat.

--
Gustavo Sousa

>
>
> Matt
>
>>  
>>  	/** @wa_cache: cached data for xe_wa() and related functions. */
>>  	void *wa_cache;
>> 
>> -- 
>> 2.55.0
>> 
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation