Re: [PATCH v3 3/3] drm/xe/pat: Introduce xe_cache_pat_idx() helper

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

On 4/15/2026 02:12, Matthew Auld wrote:
> On 14/04/2026 22:14, Xin Wang wrote:
>> Add an inline helper xe_cache_pat_idx() that wraps pat.idx[] read
>> accesses with xe_assert() validation, catching any attempt to use
>> an invalid PAT index at runtime.
>>
>> Suggested-by: Matthew Auld <matthew.auld@intel.com>
>> Signed-off-by: Xin Wang <x.wang@intel.com>
>> ---
>>   drivers/gpu/drm/xe/display/xe_fb_pin.c | 11 ++++++-----
>>   drivers/gpu/drm/xe/tests/xe_migrate.c  |  3 ++-
>>   drivers/gpu/drm/xe/xe_ggtt.c           |  7 ++++---
>>   drivers/gpu/drm/xe/xe_migrate.c        | 15 ++++++++-------
>>   drivers/gpu/drm/xe/xe_pat.h            | 24 +++++++++++++++++++++++-
>>   drivers/gpu/drm/xe/xe_pt.c             |  3 ++-
>>   drivers/gpu/drm/xe/xe_vm.c             |  6 +++---
>>   7 files changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c 
>> b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> index e45a1e7a4670..d670a3cf1b84 100644
>> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> @@ -14,6 +14,7 @@
>>   #include "xe_device.h"
>>   #include "xe_display_vma.h"
>>   #include "xe_ggtt.h"
>> +#include "xe_pat.h"
>>   #include "xe_pm.h"
>>   #include "xe_vram_types.h"
>>   @@ -24,7 +25,7 @@ write_dpt_rotated(struct xe_bo *bo, struct 
>> iosys_map *map, u32 *dpt_ofs, u32 bo_
>>       struct xe_device *xe = xe_bo_device(bo);
>>       struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>>       u32 column, row;
>> -    u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo, 
>> xe->pat.idx[XE_CACHE_NONE]);
>> +    u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo, 
>> xe_cache_pat_idx(xe, XE_CACHE_NONE));
>>         /* TODO: Maybe rewrite so we can traverse the bo addresses 
>> sequentially,
>>        * by writing dpt/ggtt in a different order?
>> @@ -64,7 +65,7 @@ write_dpt_remapped_linear(struct xe_bo *bo, struct 
>> iosys_map *map,
>>       struct xe_device *xe = xe_bo_device(bo);
>>       struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>>       const u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo,
>> -                         xe->pat.idx[XE_CACHE_NONE]);
>> +                         xe_cache_pat_idx(xe, XE_CACHE_NONE));
>>       unsigned int offset = plane->offset * XE_PAGE_SIZE;
>>       unsigned int size = plane->size;
>>   @@ -87,7 +88,7 @@ write_dpt_remapped_tiled(struct xe_bo *bo, struct 
>> iosys_map *map,
>>       struct xe_device *xe = xe_bo_device(bo);
>>       struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>>       const u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo,
>> -                         xe->pat.idx[XE_CACHE_NONE]);
>> +                         xe_cache_pat_idx(xe, XE_CACHE_NONE));
>>       unsigned int offset, column, row;
>>         for (row = 0; row < plane->height; row++) {
>> @@ -190,7 +191,7 @@ static int __xe_pin_fb_vma_dpt(const struct 
>> intel_framebuffer *fb,
>>           return PTR_ERR(dpt);
>>         if (view->type == I915_GTT_VIEW_NORMAL) {
>> -        u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo, 
>> xe->pat.idx[XE_CACHE_NONE]);
>> +        u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo, 
>> xe_cache_pat_idx(xe, XE_CACHE_NONE));
>>           u32 x;
>>             for (x = 0; x < size / XE_PAGE_SIZE; x++) {
>> @@ -306,7 +307,7 @@ static int __xe_pin_fb_vma_ggtt(const struct 
>> intel_framebuffer *fb,
>>           /* display uses tiles instead of bytes here, so convert it 
>> back.. */
>>           size = intel_rotation_info_size(&view->rotated) * 
>> XE_PAGE_SIZE;
>>   -    pte = xe_ggtt_encode_pte_flags(ggtt, bo, 
>> xe->pat.idx[XE_CACHE_NONE]);
>> +    pte = xe_ggtt_encode_pte_flags(ggtt, bo, xe_cache_pat_idx(xe, 
>> XE_CACHE_NONE));
>>       vma->node = xe_ggtt_insert_node_transform(ggtt, bo, pte,
>>                             ALIGN(size, align), align,
>>                             view->type == I915_GTT_VIEW_NORMAL ?
>> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c 
>> b/drivers/gpu/drm/xe/tests/xe_migrate.c
>> index 34e2f0f4631f..50a97705e0ac 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
>> @@ -9,6 +9,7 @@
>>   #include "tests/xe_kunit_helpers.h"
>>   #include "tests/xe_pci_test.h"
>>   +#include "xe_pat.h"
>>   #include "xe_pci.h"
>>   #include "xe_pm.h"
>>   @@ -246,7 +247,7 @@ static void xe_migrate_sanity_test(struct 
>> xe_migrate *m, struct kunit *test,
>>       /* First part of the test, are we updating our pagetable bo 
>> with a new entry? */
>>       xe_map_wr(xe, &bo->vmap, XE_PAGE_SIZE * (NUM_KERNEL_PDE - 1), u64,
>>             0xdeaddeadbeefbeef);
>> -    expected = m->q->vm->pt_ops->pte_encode_bo(pt, 0, 
>> xe->pat.idx[XE_CACHE_WB], 0);
>> +    expected = m->q->vm->pt_ops->pte_encode_bo(pt, 0, 
>> xe_cache_pat_idx(xe, XE_CACHE_WB), 0);
>>       if (m->q->vm->flags & XE_VM_FLAG_64K)
>>           expected |= XE_PTE_PS64;
>>       if (xe_bo_is_vram(pt))
>> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>> index 3552fa3cac4b..a351c578b170 100644
>> --- a/drivers/gpu/drm/xe/xe_ggtt.c
>> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
>> @@ -24,6 +24,7 @@
>>   #include "xe_gt_types.h"
>>   #include "xe_map.h"
>>   #include "xe_mmio.h"
>> +#include "xe_pat.h"
>>   #include "xe_pm.h"
>>   #include "xe_res_cursor.h"
>>   #include "xe_sriov.h"
>> @@ -258,7 +259,7 @@ static u64 xe_ggtt_get_pte(struct xe_ggtt *ggtt, 
>> u64 addr)
>>     static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
>>   {
>> -    u16 pat_index = tile_to_xe(ggtt->tile)->pat.idx[XE_CACHE_WB];
>> +    u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile), 
>> XE_CACHE_WB);
>>       u64 end = start + size - 1;
>>       u64 scratch_pte;
>>   @@ -723,7 +724,7 @@ static void xe_ggtt_map_bo(struct xe_ggtt 
>> *ggtt, struct xe_ggtt_node *node,
>>   void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo)
>>   {
>>       u16 cache_mode = bo->flags & XE_BO_FLAG_NEEDS_UC ? 
>> XE_CACHE_NONE : XE_CACHE_WB;
>> -    u16 pat_index = tile_to_xe(ggtt->tile)->pat.idx[cache_mode];
>> +    u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile), 
>> cache_mode);
>>       u64 pte;
>>         mutex_lock(&ggtt->lock);
>> @@ -840,7 +841,7 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt 
>> *ggtt, struct xe_bo *bo,
>>           bo->ggtt_node[tile_id] = NULL;
>>       } else {
>>           u16 cache_mode = bo->flags & XE_BO_FLAG_NEEDS_UC ? 
>> XE_CACHE_NONE : XE_CACHE_WB;
>> -        u16 pat_index = tile_to_xe(ggtt->tile)->pat.idx[cache_mode];
>> +        u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile), 
>> cache_mode);
>>           u64 pte = ggtt->pt_ops->pte_encode_flags(bo, pat_index);
>>             xe_ggtt_map_bo(ggtt, bo->ggtt_node[tile_id], bo, pte);
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c 
>> b/drivers/gpu/drm/xe/xe_migrate.c
>> index 5fdc89ed5256..a87fbc1e9fb1 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -31,6 +31,7 @@
>>   #include "xe_map.h"
>>   #include "xe_mem_pool.h"
>>   #include "xe_mocs.h"
>> +#include "xe_pat.h"
>>   #include "xe_printk.h"
>>   #include "xe_pt.h"
>>   #include "xe_res_cursor.h"
>> @@ -217,7 +218,7 @@ static void xe_migrate_prepare_vm(struct xe_tile 
>> *tile, struct xe_migrate *m,
>>                     struct xe_vm *vm, u32 *ofs)
>>   {
>>       struct xe_device *xe = tile_to_xe(tile);
>> -    u16 pat_index = xe->pat.idx[XE_CACHE_WB];
>> +    u16 pat_index = xe_cache_pat_idx(xe, XE_CACHE_WB);
>>       u8 id = tile->id;
>>       u32 num_entries = NUM_PT_SLOTS, num_level = 
>> vm->pt_root[id]->level;
>>   #define VRAM_IDENTITY_MAP_COUNT    2
>> @@ -337,7 +338,7 @@ static void xe_migrate_prepare_vm(struct xe_tile 
>> *tile, struct xe_migrate *m,
>>            * if flat ccs is enabled.
>>            */
>>           if (GRAPHICS_VER(xe) >= 20 && xe_device_has_flat_ccs(xe)) {
>> -            u16 comp_pat_index = 
>> xe->pat.idx[XE_CACHE_NONE_COMPRESSION];
>> +            u16 comp_pat_index = xe_cache_pat_idx(xe, 
>> XE_CACHE_NONE_COMPRESSION);
>>               u64 vram_offset = IDENTITY_OFFSET +
>>                   DIV_ROUND_UP_ULL(actual_phy_size, SZ_1G);
>>               u64 pt31_ofs = xe_bo_size(bo) - XE_PAGE_SIZE;
>> @@ -637,10 +638,10 @@ static void emit_pte(struct xe_migrate *m,
>>         /* Indirect access needs compression enabled uncached PAT 
>> index */
>>       if (GRAPHICS_VERx100(xe) >= 2000)
>> -        pat_index = is_comp_pte ? 
>> xe->pat.idx[XE_CACHE_NONE_COMPRESSION] :
>> -                      xe->pat.idx[XE_CACHE_WB];
>> +        pat_index = is_comp_pte ? xe_cache_pat_idx(xe, 
>> XE_CACHE_NONE_COMPRESSION) :
>> +                      xe_cache_pat_idx(xe, XE_CACHE_WB);
>>       else
>> -        pat_index = xe->pat.idx[XE_CACHE_WB];
>> +        pat_index = xe_cache_pat_idx(xe, XE_CACHE_WB);
>>         ptes = DIV_ROUND_UP(size, XE_PAGE_SIZE);
>>   @@ -1876,7 +1877,7 @@ __xe_migrate_update_pgtables(struct 
>> xe_migrate *m,
>>         /* For sysmem PTE's, need to map them in our hole.. */
>>       if (!IS_DGFX(xe)) {
>> -        u16 pat_index = xe->pat.idx[XE_CACHE_WB];
>> +        u16 pat_index = xe_cache_pat_idx(xe, XE_CACHE_WB);
>>           u32 ptes, ofs;
>>             ppgtt_ofs = NUM_KERNEL_PDE - 1;
>> @@ -2098,7 +2099,7 @@ static void build_pt_update_batch_sram(struct 
>> xe_migrate *m,
>>                          struct drm_pagemap_addr *sram_addr,
>>                          u32 size, int level)
>>   {
>> -    u16 pat_index = tile_to_xe(m->tile)->pat.idx[XE_CACHE_WB];
>> +    u16 pat_index = xe_cache_pat_idx(tile_to_xe(m->tile), XE_CACHE_WB);
>>       u64 gpu_page_size = 0x1ull << xe_pt_shift(level);
>>       u32 ptes;
>>       int i = 0;
>> diff --git a/drivers/gpu/drm/xe/xe_pat.h b/drivers/gpu/drm/xe/xe_pat.h
>> index a1e287c08f57..338fda95e63f 100644
>> --- a/drivers/gpu/drm/xe/xe_pat.h
>> +++ b/drivers/gpu/drm/xe/xe_pat.h
>> @@ -8,8 +8,11 @@
>>     #include <linux/types.h>
>>   +#include "xe_assert.h"
>> +#include "xe_device_types.h"
>> +#include "xe_pt_types.h"
>> +
>>   struct drm_printer;
>> -struct xe_device;
>>   struct xe_gt;
>>     #define XE_PAT_INVALID_IDX    U16_MAX
>> @@ -82,4 +85,23 @@ bool xe_pat_index_get_comp_en(struct xe_device 
>> *xe, u16 pat_index);
>>    */
>>   u16 xe_pat_index_get_l3_policy(struct xe_device *xe, u16 pat_index);
>>   +/**
>> + * xe_cache_pat_idx - Get the PAT index for a given cache level
>> + * @xe: xe device
>> + * @cache_mode: cache level to look up
>> + *
>> + * Assert that @cache_mode is within bounds and that the PAT index has
>> + * been configured for the requested cache level, then return it.
>> + *
>> + * Return: PAT index corresponding to @cache_mode
>> + */
>> +static inline u16 xe_cache_pat_idx(struct xe_device *xe,
>> +                   enum xe_cache_level cache_mode)
>> +{
>> +    xe_assert(xe, cache_mode < __XE_CACHE_LEVEL_COUNT);
>> +    xe_assert(xe, xe->pat.idx[cache_mode] != XE_PAT_INVALID_IDX);
>> +
>> +    return xe->pat.idx[cache_mode];
>> +}
>
> Oh, actually maybe better to move this to the .c file, unless there is 
> a strong reason not to? Especially if we have to add a bunch of extra 
> includes in the .h.
>
My initial idea was to use an inline function to display the line number
and filename at the location of the call when the assert occurs, but it
seems that this doesn't achieve the desired result. I plan to use a
macro to implement this helper, which should be more reasonable.

Xin