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, Aug 18, 2026 at 03:25:34PM -0300, Gustavo Sousa wrote:
>> 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.
>
> But using a void pointer with casting doesn't really address that
> properly.  If we want the internal details of 'struct intel_pat_cache'
> to be something that only intel_pat code can access, then we make sure
> that the structure definition is inside the .c file (or inside a
> "private" header if the intel_pat unit spans multiple files). Everything
> outside of that unit will just see "struct intel_pat_cache;" on the line
> you removed here, which declares it as an opaque structure for them.
> They can pass around pointers to that freely, but can't access its
> internals since they don't have the definition.

Yeah.  I agree that we don't need to make the member a void pointer in
order to keep the details hidden.  I guess the void pointer is actually
useful when we don't know the type of the data that it will be pointing
to, but here it is a different matter and using the proper type is
better.  Thanks!

I moved it back to be a pointer to struct intel_cache_pat.  In the same
spirit, I also changed the patch for cached workaround data to also be a
pointer to struct xe_wa_cache.

--
Gustavo Sousa

>
>
> Matt
>
>> 
>> --
>> 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
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation