[Intel-xe] [PATCH] drm/xe: Fix and clarify PAT Index selection.
Lucas De Marchi
lucas.demarchi at intel.com
Fri Jul 28 21:52:08 UTC 2023
On Fri, Jul 28, 2023 at 02:26:42PM -0700, Matt Roper wrote:
>On Thu, Jul 27, 2023 at 05:37:45PM -0400, Rodrigo Vivi wrote:
>> PAT selection here in the Xe driver had inherited a lot
>> of history from i915 and ended up with some strange cases
>> like doing logical 'or' with 0, or even unnecessarily selecting
>> pat_index = 7, instead of simply opting for the 0.
>>
>> XXX: More work needs to be done in MTL and PVC for the right
>> selection, starting from the fact that pat_index = 2 is now the
>> uncached and pat_index = 3 is WB with some kind of coherence.
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_bo.h | 24 ++++++++++++++++++++----
>> drivers/gpu/drm/xe/xe_migrate.c | 4 ++--
>> drivers/gpu/drm/xe/xe_pt.c | 10 +++++-----
>> 3 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
>> index b29750a47d23..8c80c0784010 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.h
>> +++ b/drivers/gpu/drm/xe/xe_bo.h
>> @@ -37,10 +37,26 @@
>> #define XE_BO_INTERNAL_TEST BIT(30)
>> #define XE_BO_INTERNAL_64K BIT(31)
>>
>> -#define PPAT_UNCACHED GENMASK_ULL(4, 3)
>> -#define PPAT_CACHED_PDE 0
>> -#define PPAT_CACHED BIT_ULL(7)
>> -#define PPAT_DISPLAY_ELLC BIT_ULL(4)
>> +/*
>> + * PAT Index Mechanism
>> + *
>> + * Some bits encoded in the page table entries and directories,
>> + * are put together to form the "PAT_INDEX". The value of
>> + * PAT Index points to some cache options such as:
>> + *
>> + * PAT_INDEX = 0 --> WB Cache
>> + * PAT_INDEX = 1 --> WC Cache
>> + * PAT_INDEX = 2 --> WT Cache
>> + * PAT_INDEX = 3 --> Uncached
>> + * (The full per-platform table can be found at xe_pat.c)
>> + *
>> + * The bits that form the PAT index for PPGTT are:
>> + * PTE - [62][ 7][4][3]
>> + * PDE/PDP - [62][12][4][3]
>> + */
>> +#define PPAT_INDEX (BIT_ULL(62) | BIT_ULL(7) | BIT_ULL(4) | BIT_ULL(3))
>> +#define PPAT_WT BIT_ULL(4) /* PAT_INDEX = 2 */
>> +#define PPAT_UNCACHED GENMASK_ULL(4, 3) /* PAT_INDEX = 3 */
>
>Which index corresponds to what behavior is platform-specific and will
>likely change with every major hardware change. We should eliminate
>things like PPAT_WT or PPAT_UNCACHED completely. If there's a
>platform-specific PAT index which is used internally by the driver in a
>bunch of places (e.g., "give me fully uncached behavior") we should
>cache that index in something like xe_gt->pat_uc. It can't be a
>driver-wide constant.
>
>Generally the PAT selection is either going to come from userspace (and
>winds up getting passed all the way down to the pte/pde encoding
>functions), or we're working with some kernel-allocated buffer (like a
>GuC CTB or a golden LRC) and the exact characteristics that make sense
>need to be selected on a platform-by-platform basis at init time.
>
>>
>> #define XE_PTE_SHIFT 12
>> #define XE_PAGE_SIZE (1 << XE_PTE_SHIFT)
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>> index aad76a6a8094..5d33ee1aa3a0 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -257,7 +257,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>
>> level = 2;
>> ofs = map_ofs + XE_PAGE_SIZE * level + 256 * 8;
>> - flags = XE_PAGE_RW | XE_PAGE_PRESENT | PPAT_CACHED |
>> + flags = XE_PAGE_RW | XE_PAGE_PRESENT |
>
>Although a constant PPAT_CACHED isn't correct, it also isn't correct to
>just leave the PAT index as 0 either since that will result in different
>behavior on different platforms.
>
>> XE_PPGTT_PTE_DM | XE_PDPE_PS_1G;
>>
>> /*
>> @@ -465,7 +465,7 @@ static void emit_pte(struct xe_migrate *m,
>> addr += vram_region_gpu_offset(bo->ttm.resource);
>> addr |= XE_PPGTT_PTE_DM;
>> }
>> - addr |= PPAT_CACHED | XE_PAGE_PRESENT | XE_PAGE_RW;
>> + addr |= XE_PAGE_PRESENT | XE_PAGE_RW;
>> bb->cs[bb->len++] = lower_32_bits(addr);
>> bb->cs[bb->len++] = upper_32_bits(addr);
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index 3129fdb0551c..20f2d1139e8c 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -65,10 +65,10 @@ u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
>> pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
>> pde |= XE_PAGE_PRESENT | XE_PAGE_RW;
>>
>> - /* FIXME: I don't think the PPAT handling is correct for MTL */
>> + /* FIXME: the PPAT handling is incorrect for MTL and PVC */
>>
>> if (level != XE_CACHE_NONE)
>
>The important thing to accomplish for these encoding functions is to
>eliminate these symbolic values like XE_CACHE_NONE completely.
humn... kind of. Let me point people in this thread to the other one I
was reviewing:
https://patchwork.freedesktop.org/series/120614/ and
https://patchwork.freedesktop.org/series/121354/
I think the XE_CACHE_NONE (or maybe a better name, UC) would be the
platform-agnostic to select the behavior desired by *kernel* uses of the
indexes.
so everywhere where it's needed (either for ppgtt, but mostly ggtt) we'd
have:
pat_idx = xe_pat_get_index(xe, cache);
where cache is one of the platform-agnostic values. I'm using xe here
rather than gt since AFAIK this would be a per device behavior, not per
gt.
xe_pat.c implements xe_pat_get_index(). Basically during init it caches
the values of each platform-agnostic values. More or less like we
already do for mocs, but could just be an array.
pte |= vm->pte_encode_pat(xe, pat_idx);
This vfunc is what gets that index and returns the right bits to encode
in the pte. The bits are already different per platform and if using
ggtt vs ppgtt. That's why I think it deserves to be a vfunc we can set
during init.
>Eventually we should just take the PAT index as a parameter and encode
>it directly without trying to interpret what it means. The upper layers
>of the code (and/or userspace) are the ones responsible for selecting
>which index is actually appropriate.
I don't think we should try to interpret what comes from userspace
neither. But for kernel uses we need something like what we have for
mocs, to get the desired behavior.
Lucas De Marchi
>
>
>Matt
>
>> - pde |= PPAT_CACHED_PDE;
>> + pde &= ~PPAT_INDEX;
>> else
>> pde |= PPAT_UNCACHED;
>>
>> @@ -86,17 +86,17 @@ static u64 __pte_encode(u64 pte, enum xe_cache_level cache,
>> if (unlikely(vma && xe_vma_is_null(vma)))
>> pte |= XE_PTE_NULL;
>>
>> - /* FIXME: I don't think the PPAT handling is correct for MTL */
>> + /* FIXME: the PPAT handling is incorrect for MTL and PVC */
>>
>> switch (cache) {
>> case XE_CACHE_NONE:
>> pte |= PPAT_UNCACHED;
>> break;
>> case XE_CACHE_WT:
>> - pte |= PPAT_DISPLAY_ELLC;
>> + pte |= PPAT_WT;
>> break;
>> default:
>> - pte |= PPAT_CACHED;
>> + pte &= ~PPAT_INDEX;
>> break;
>> }
>>
>> --
>> 2.41.0
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
More information about the Intel-xe
mailing list