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

["Wang, X" <x.wang@intel.com>]

On 4/7/2026 02:48, Matthew Auld wrote:
> 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?
>
Yeah, I can modify this to simplify 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.
>
Agree on wrapping pat.idx[] access with a helper for
consistency.
But I don't think an assert inside the helper is the
right place. For UC/WT/WB, all platforms initialize
them so the assert would never fire — it's dead code.
For compression variants, xe_bo.c:3763 intentionally
reads the INVALID value as a capability check, so an
assert there would break that. Better to keep the
assert at the callsite like we do here in xe_migrate.c,
where we have the right context to know a valid index
is required.
What do you think?

Thanks
Xin
>>         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;
>