Re: [PATCH 2/2] drm/xe/pat: Default XE_CACHE_NONE_COMPRESSION to invalid and assert on use

[Matthew Auld <matthew.auld@intel.com>]

On 06/04/2026 19:25, Xin Wang wrote:
> Initialize XE_CACHE_NONE_COMPRESSION PAT index to XE_PAT_INVALID_IDX by
> default, and add xe_assert() checks at the sites that use it for PTE
> encoding. This ensures platforms that don't support this cache mode fail
> loudly rather than silently encoding an invalid index into PTEs.
> 
> Suggested-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Xin Wang <x.wang@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_migrate.c | 9 +++++++--
>   drivers/gpu/drm/xe/xe_pat.c     | 1 +
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index fc918b4fba54..cad47866435d 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -30,6 +30,7 @@
>   #include "xe_lrc.h"
>   #include "xe_map.h"
>   #include "xe_mocs.h"
> +#include "xe_pat.h"
>   #include "xe_printk.h"
>   #include "xe_pt.h"
>   #include "xe_res_cursor.h"
> @@ -341,6 +342,7 @@ static void xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>   				DIV_ROUND_UP_ULL(actual_phy_size, SZ_1G);
>   			u64 pt31_ofs = xe_bo_size(bo) - XE_PAGE_SIZE;
>   
> +			xe_assert(xe, comp_pat_index != XE_PAT_INVALID_IDX);
>   			xe_assert(xe, actual_phy_size <= (MAX_NUM_PTE - IDENTITY_OFFSET -
>   							  IDENTITY_OFFSET / 2) * SZ_1G);
>   			xe_migrate_program_identity(xe, vm, bo, map_ofs, vram_offset,
> @@ -635,11 +637,14 @@ static void emit_pte(struct xe_migrate *m,
>   	u64 cur_ofs;
>   
>   	/* Indirect access needs compression enabled uncached PAT index */
> -	if (GRAPHICS_VERx100(xe) >= 2000)
> +	if (GRAPHICS_VERx100(xe) >= 2000) {
>   		pat_index = is_comp_pte ? xe->pat.idx[XE_CACHE_NONE_COMPRESSION] :
>   					  xe->pat.idx[XE_CACHE_WB];
> -	else
> +		if (is_comp_pte)
> +			xe_assert(xe, pat_index != XE_PAT_INVALID_IDX);
> +	} else {
>   		pat_index = xe->pat.idx[XE_CACHE_WB];
> +	}

To simplify the flow, maybe we just unconditionally stick the assert here?

But would it make sense to take this a step further and wrap all 
pat.idx[] accesses with a simple helper (like xe_cache_pat_idx()), and 
add the assert there? There are a fair few places directly accessing 
pat.idx, outside of xe_pat.

>   
>   	ptes = DIV_ROUND_UP(size, XE_PAGE_SIZE);
>   
> diff --git a/drivers/gpu/drm/xe/xe_pat.c b/drivers/gpu/drm/xe/xe_pat.c
> index 75aaae7b003d..fad5b5a5ed4a 100644
> --- a/drivers/gpu/drm/xe/xe_pat.c
> +++ b/drivers/gpu/drm/xe/xe_pat.c
> @@ -559,6 +559,7 @@ static const struct xe_pat_ops xe3p_xpc_pat_ops = {
>   void xe_pat_init_early(struct xe_device *xe)
>   {
>   	xe->pat.idx[XE_CACHE_WB_COMPRESSION] = XE_PAT_INVALID_IDX;
> +	xe->pat.idx[XE_CACHE_NONE_COMPRESSION] = XE_PAT_INVALID_IDX;
>   	if (GRAPHICS_VERx100(xe) == 3511) {
>   		xe->pat.ops = &xe3p_xpc_pat_ops;
>   		xe->pat.table = xe3p_xpc_pat_table;