[Intel-xe] [RFC 5/5] drm/xe/uapi: support pat_index selection with vm_bind
Zhang, Carl
carl.zhang at intel.com
Tue Sep 5 09:12:35 UTC 2023
> -----Original Message-----
> From: Auld, Matthew <matthew.auld at intel.com>
> Sent: Monday, September 4, 2023 5:24 PM
>
> On 01/09/2023 10:34, Zhang, Carl wrote:
> >
> >
> >> -----Original Message-----
> >> From: Auld, Matthew <matthew.auld at intel.com>
> >> Sent: Thursday, August 31, 2023 6:44 PM
> >>
> >> On 31/08/2023 09:24, Zhang, Carl wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Auld, Matthew <matthew.auld at intel.com>
> >>>> Sent: Thursday, August 31, 2023 12:02 AM
> >>>>
> >>>> On 30/08/2023 16:27, Zhang, Carl wrote:
> >>>>> Several questions:
> >>>>> 1. the pat_index from vm_bind will override the setting from
> bo_create?
> >>>>> How to keep the value from bo_create unchanged?
> >>>>
> >>>> You only set the pat_index during vm_bind. At gem_create you just
> >>>> need tell the kernel what CPU side caching mode to use for system
> >>>> memory (wb/wc/uc), plus the expected GPU coherency mode of the
> >>>> pat_index. The current expectation is that the coherency mode of
> >>>> the pat_index should match the one at gem_create.
> >>>>
> >>> It means that we set the incomplete pat_index at gem_create
> >>> (coherency
> >>> only) And set another part at vm_bind (gpu cache) Why we separate
> >>> them? Why not set a full value at vm_bind?
> >>
> >> You always give the full pat_index at vm_bind. It's platform specific
> >> but the BSpec for the most part tells you what the coherency mode of the
> pat_index is.
> >>
> >> For example if you look at the MTL pat_index table it tells you the
> >> coh_mode:
> >>
> >> pat_index 0-2 = coh_mode "No snoop" = COH_NONE
> >> pat_index 3 = coh_mode "1way" = COH_1WAY
> >> pat_index 4 = coh_mode "2way" = COH_2WAY
> >>
> >> On future platforms the table is larger and also encodes stuff like
> >> compression, but KMD really only cares about the coh_mode, since that
> >> will restrict the allowed CPU smem_caching values that userspace can
> select.
> >>
> >> From KMD pov this matters since we always give userspace zeroed
> >> memory, which is useful for normal applications but is also needed for
> security reasons.
> >> However the actual clearing might result in the writes only being
> >> visible in the CPU cache (not flushed to main memory), and if the GPU
> >> doesn't snoop the CPU cache it can read directly from main memory,
> >> which effectively bypasses the clearing. To prevent that the KMD
> >> needs to know how userspace will map the memory via the GPU i.e what
> >> is the coh_mode for the pat_index. The gist is that if you select
> >> COH_NONE then it needs to clflush the pages before giving them to
> >> userspace, which is done by forcing you to use uc/wc for the
> >> smem_caching (the kernel will internally issue cflush on x86 when
> >> marking the pages as uc/wc). And if you select wb there is no flushing, but
> here you need to use 1way or 2way.
> >>
> >
> > It sounds like a perf optimization, if no considering the perf, you
> > could always call Clflush to flush the cpu cache to main memory before
> > giving it to user space
>
> I think the other thing is that the KMD is no longer doing any manual flushing,
> but instead the core kernel will now do that for us when marking the pages as
> wc/uc on x86.
>
> >
> >
> >> AFAIK separating them is just to prevent userspace from mixing
> >> different coherency modes for the same object by having it immutable
> >> (you can still use different pat_index but the coherency must be
> compatible).
> >> Also at creation time we can determine if the smem_caching is
> >> compatible with the coh_mode.
> >>
> >> Note that it might be that we ditch the COH_2WAY and just have
> >> COH_NONE and COH_AT_LEAST_1WAY, in which case this might be slightly
> different.
> >>
> >>>
> >>>>> 2. no UC/WB/WC definition (CPU cachable) in
> >>>>> drm_xe_gem_mmap_offset,
> >>>> will it be handled by KMD automatically?
> >>>>> For example: if set 1-way, it means GPU could snoop CPU cache, we
> >>>>> could use WB in mmap offset If it is COHERENCY_NONE, we could
> >>>>> only use
> >>>> UC, all these logic is handled by KMD automatically?
> >>>>
> >>>> Yes, the mmap will use the same CPU side caching mode that you set
> >>>> at gem_create with smem_caching (wb/wc/uc).
> >>>>
> >>> It means it could not be changed after gem_create. Sometimes, UMD
> >>> doesn’t know Whether the data need to be accessed by CPU at
> >>> gem_create . only application Knows the purpose of allocation & the
> >>> surface usage. So, may need change UMD api to let applications set
> >>> whether the
> >> resource is "accessible", or it will hurt the perf.
> >>
> >> Right, the smem_caching is immutable.
> >>
> >>>
> >>>>> 3. about " For imported dma-buf (from a different device) the
> >>>>> coherency mode is also implicit and must also be either 1WAY or
> 2WAY"
> >>>>> it means it must be 1way or 2way, and UMD need not to set it?
> >>>>
> >>>> If you are going to vm_bind it you need to supply the pat_index and
> >>>> in this case it needs to be either 1way or 2way. The object comes
> >>>> from a different device so we might not have smem_caching/coh_mode
> >>>> like we do for native objects.
> >>>>
> >>> How about the GPU cache setting of imported dma-buf?
> >>> Is there possible conflict between dma-buf producer and consumer?
> >>> For example:
> >>> In producer side , one bo is set GPU L3 cache, also CPU cacheable,
> >>> it should be 2Way In client side, we know the coherency should be
> >>> 1-way or 2-way, if it suppose no GPU cache Used. It just set 1-way.
> >>> It may cause some problems?
> >>
> >> If the producer and consumer are the same device, then I think the
> >> original gem_create.coh_mode is considered, since it's effectively
> >> still a native object. In which case the normal rules apply.
> >
> > But the consumer does not know the coh_mode, so , it is possible that
> > it set will set Another value, then it will failed.
> > And because consumer does not know it is 1-way or 2-way, consumer
> > could not Set the gpu cache correctly. Because if it is 1-way, cpu
> > could not snoop gpu cache
> >
> >>
> >> If it comes from a different device then we don't know what
> >> gem_create.coh_mode is (or if that even exists) since we don't have
> >> the usual native object, but just some opaque dma-buf object. In this
> >> case it must be at least 1way or 2way.
> >>
> >> From KMD pov we only care about userspace not being able to bypass
> >> the clearing. There might still be other coherency issues though.
> >>
> >>>
> >>> Also , when a bo is created, UMD does not know whether it will be
> exported.
> >>> It maybe be set as COHERENCY_NONE, right?
> >>
> >> Yeah, you can use whatever you want. Just that your smem_caching will
> >> need to be wc/uc if using COH_NONE. On the importer side, if it comes
> >> from the same device then it's just a normal native object and the normal
> rules apply.
> >> Currently this would mean also using pat_index with COH_NONE. If it
> >> is imported from a different device then you need to use a pat_index
> >> with 1way or 2way, since KMD doesn't really know how the object is
> >> mapped on the CPU or if the CPU caches are dirty (potentially with
> clearing).
> >>
> >> Is this going to be problematic?
> >>
> > If imported surfaces is from same device, how consumer know it is
> > original coh_non Or 1-way or 2-way?
> >
> > And it also means producer and consumer could have different pat_index for
> same bo.
>
> Ok, if KMD instead allows 1way or 2way for dma-buf when it's external or
> from the same device? Would that help here? i.e you can always select 1way
> or 2way, even if it came from the same device and was coh_none?
>
> Other option is you could call the vm_bind ioctl to check if the coh_mode is
> supported by checking if it returns an error? But maybe that is too nasy.
>
Still a bit confuse, I create a bo with coh_none, then export it to a PRIME fd.
In consumer side, it does not know it is coh_none, and will call vm_bind to set pat_index.
You mean it will return a error when I set different coh value though vm_bind?
And try coh_non, 1-way, 2-way, then get the correct one? TBH, it is ugly.
My concern is: if it is coh_non or 1-way, it means I could not use GPU cache if the bo also is CPU accessible.
Because CPU could not snoop GPU cache.
So, GPU cache specified by pat_index also will be rejected?
Could we add some query interface to query the CPU cache and coherency setting of a bo?
>
> >
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Auld, Matthew <matthew.auld at intel.com>
> >>>>>> Sent: Wednesday, August 30, 2023 7:28 PM
> >>>>>> To: Roper, Matthew D <matthew.d.roper at intel.com>
> >>>>>> Cc: intel-xe at lists.freedesktop.org; Mishra, Pallavi
> >>>> <pallavi.mishra at intel.com>;
> >>>>>> Thomas Hellström <thomas.hellstrom at linux.intel.com>; Joonas
> >>>>>> Lahtinen <joonas.lahtinen at linux.intel.com>; De Marchi, Lucas
> >>>>>> <lucas.demarchi at intel.com>; Souza, Jose <jose.souza at intel.com>;
> >>>> Hazubski,
> >>>>>> Filip <filip.hazubski at intel.com>; Zhang, Carl
> >>>>>> <carl.zhang at intel.com>; Yu,
> >>>> Effie
> >>>>>> <effie.yu at intel.com>
> >>>>>> Subject: Re: [RFC 5/5] drm/xe/uapi: support pat_index selection
> >>>>>> with
> >>>> vm_bind
> >>>>>>
> >>>>>> On 29/08/2023 22:36, Matt Roper wrote:
> >>>>>>> On Tue, Aug 29, 2023 at 05:28:46PM +0100, Matthew Auld wrote:
> >>>>>>>> Allow userspace to directly control the pat_index for a given
> >>>>>>>> vm binding. This should allow directly controlling the
> >>>>>>>> coherency, caching and potentially other stuff in the future for the
> ppGTT binding.
> >>>>>>>>
> >>>>>>>> The exact meaning behind the pat_index is very platform
> >>>>>>>> specific (see BSpec or PRMs) but effectively maps to some
> >>>>>>>> predefined memory attributes. From the KMD pov we only care
> >>>>>>>> about the coherency that is provided by the pat_index, which
> >>>>>>>> falls into either
> >> NONE, 1WAY or 2WAY.
> >>>>>>>> The vm_bind coherency mode for the given pat_index needs to
> >>>>>>>> match
> >>>> the
> >>>>>>>> given coh_mode that was set at object creation. For platforms
> >>>>>>>> that lack
> >>>>>>>
> >>>>>>> Is it actually important to match the coherency mode? I think
> >>>>>>> one of the main goals was to know up front if userspace might be
> >>>>>>> using a non-snooping PAT setting that would let it bypass the
> >>>>>>> CPU cache (and potentially read old, stale data from a different
> >>>>>>> process if the buffer's clear value is still sitting in cache
> >>>>>>> and hasn't landed in memory yet).
> >>>>>>>
> >>>>>>> If that's the only concern, then I think it should still be fine
> >>>>>>> to map with a non-matching PAT as long as it's more coherent
> >>>>>>> than the one specified at creation, right? E.g., if the buffer
> >>>>>>> was created with 1-way coherency, it would be fine to map it
> >>>>>>> with 2-way because userspace still can't use that to observe the
> >>>>>>> previous contents of the buffer. Or
> >>>>>>
> >>>>>> Yeah, I guess we could in theory do something that.
> >>>>>>
> >>>>>>> if the buffer was created with "non-coherent" then we've already
> >>>>>>> done the necessary clflushing in kernel before handing to buffer
> >>>>>>> over to userspace to ensure the clear value landed in memory, so
> >>>>>>> any valid PAT index should be safe (from a security POV) after
> >>>>>>> that, right? Any other problems that arise from mismatched
> >>>>>>> coherency would just be contained to the app possibly shooting
> >>>>>>> itself in the foot, which isn't really our concern.
> >>>>>>
> >>>>>> That is also my understanding, at least from the KMD security pov.
> >>>>>> If you allocate as wb then you must use at least 1way, since
> >>>>>> there is no flushing
> >>>> for
> >>>>>> clearing or swap-in. For uc/wc you could in theory use whatever you
> want.
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> the explicit coherency mode, we treat UC/WT/WC as NONE and
> WB
> >>>>>>>> as
> >>>>>> 2WAY.
> >>>>>>>>
> >>>>>>>> For userptr mappings we lack a corresponding gem object, so the
> >>>> expected
> >>>>>>>> coherency mode is instead implicit and must fall into either
> >>>>>>>> 1WAY or 2WAY. Trying to use NONE will be rejected by the
> >>>>>>>> kernel. For imported dma-buf (from a different device) the
> >>>>>>>> coherency mode is also implicit and must also be either 1WAY or
> 2WAY.
> >>>>>>>>
> >>>>>>>> As part of adding pat_index support with vm_bind we also need
> >>>>>>>> stop
> >>>> using
> >>>>>>>> xe_cache_level and instead use the pat_index in various places.
> >>>>>>>> We still make use of xe_cache_level, but only as a convenience
> >>>>>>>> for kernel internal objectsi (internally it maps to some
> >>>>>>>> reasonable pat_index). For
> >>>>>>>
> >>>>>>> Maybe we should kill xe_cache_level completely and just assign
> >>>>>>> xe_gt->pat_cached / xe_gt->pat_uncached at init that can be used
> >>>>>>> in appropriate places, similar to what we do with MOCS (gt-
> >>>>> mocs.uc_index,
> >>>>>>> gt->mocs.wb_index)?
> >>>>>>
> >>>>>> OK, seems reasonable to me.
> >>>>>>
> >>>>>>>
> >>>>>>>> now this is just a 1:1 conversion of the existing code, however
> >>>>>>>> for platforms like MTL+ we might need to give more control
> >>>>>>>> through
> >>>> bo_create
> >>>>>>>> or stop using WB on the CPU side if we need CPU access.
> >>>>>>>>
> >>>>>>>> Bspec: 45101, 44235 #xe
> >>>>>>>> Bspec: 70552, 71582, 59400 #xe2
> >>>>>>>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> >>>>>>>> Cc: Pallavi Mishra <pallavi.mishra at intel.com>
> >>>>>>>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> >>>>>>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>>>>>>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> >>>>>>>> Cc: Matt Roper <matthew.d.roper at intel.com>
> >>>>>>>> Cc: José Roberto de Souza <jose.souza at intel.com>
> >>>>>>>> Cc: Filip Hazubski <filip.hazubski at intel.com>
> >>>>>>>> Cc: Carl Zhang <carl.zhang at intel.com>
> >>>>>>>> Cc: Effie Yu <effie.yu at intel.com>
> >>>>>>>> ---
> >>>>>>>> drivers/gpu/drm/i915/gt/intel_gtt.h | 2 +-
> >>>>>>>> drivers/gpu/drm/xe/tests/xe_migrate.c | 2 +-
> >>>>>>>> drivers/gpu/drm/xe/xe_ggtt.c | 7 ++-
> >>>>>>>> drivers/gpu/drm/xe/xe_ggtt_types.h | 2 +-
> >>>>>>>> drivers/gpu/drm/xe/xe_migrate.c | 14 ++---
> >>>>>>>> drivers/gpu/drm/xe/xe_pt.c | 32 +++++-------
> >>>>>>>> drivers/gpu/drm/xe/xe_pt.h | 6 +--
> >>>>>>>> drivers/gpu/drm/xe/xe_vm.c | 73
> +++++++++++++++++++++----
> >> -
> >>>> -
> >>>>>>>> drivers/gpu/drm/xe/xe_vm_types.h | 13 +++--
> >>>>>>>> include/uapi/drm/xe_drm.h | 41 ++++++++++++++-
> >>>>>>>> 10 files changed, 134 insertions(+), 58 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> >>>>>> b/drivers/gpu/drm/i915/gt/intel_gtt.h
> >>>>>>>> index 4d6296cdbcfd..bb4c182048c3 100644
> >>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> >>>>>>>> @@ -302,7 +302,7 @@ struct i915_address_space {
> >>>>>>>> (*alloc_scratch_dma)(struct i915_address_space
> *vm,
> >> int
> >>>>>>>> sz);
> >>>>>>>>
> >>>>>>>> u64 (*pte_encode)(dma_addr_t addr,
> >>>>>>>> - unsigned int pat_index,
> >>>>>>>> + u32 pat_index,
> >>>>>>>> u32 flags); /* Create a valid PTE */
> >>>>>>>> #define PTE_READ_ONLY BIT(0)
> >>>>>>>> #define PTE_LM BIT(1)
> >>>>>>>> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c
> >>>>>> b/drivers/gpu/drm/xe/tests/xe_migrate.c
> >>>>>>>> index 5c8d5e78d9bc..7a128fd20a29 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
> >>>>>>>> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
> >>>>>>>> @@ -301,7 +301,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 = xe_pte_encode(pt, 0, XE_CACHE_WB, 0);
> >>>>>>>> + expected = xe_pte_encode(pt, 0, xe_pat_get_index(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 209fa053d9fb..4134c26150a5 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> >>>>>>>> @@ -41,7 +41,8 @@ u64 xe_ggtt_pte_encode(struct xe_bo *bo,
> u64
> >>>>>> bo_offset)
> >>>>>>>> pte |= XE_GGTT_PTE_DM;
> >>>>>>>>
> >>>>>>>> if ((ggtt->pat_encode).pte_encode)
> >>>>>>>> - pte = (ggtt->pat_encode).pte_encode(xe, pte,
> >>>>>> XE_CACHE_WB_1_WAY);
> >>>>>>>> + pte = (ggtt->pat_encode).pte_encode(xe, pte,
> >>>>>>>> +
> >> xe_pat_get_index(xe,
> >>>>>> XE_CACHE_WB_1_WAY));
> >>>>>>>>
> >>>>>>>> return pte;
> >>>>>>>> }
> >>>>>>>> @@ -102,10 +103,8 @@ static void primelockdep(struct xe_ggtt
> *ggtt)
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> static u64 xelpg_ggtt_pte_encode_pat(struct xe_device *xe,
> >>>>>>>> u64
> >>>> pte_pat,
> >>>>>>>> - enum xe_cache_level
> >> cache)
> >>>>>>>> + u32 pat_index)
> >>>>>>>> {
> >>>>>>>> - u32 pat_index = xe_pat_get_index(xe, cache);
> >>>>>>>> -
> >>>>>>>> pte_pat &= ~(XELPG_GGTT_PTE_PAT_MASK);
> >>>>>>>>
> >>>>>>>> if (pat_index & BIT(0))
> >>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h
> >>>>>> b/drivers/gpu/drm/xe/xe_ggtt_types.h
> >>>>>>>> index 7e55fac1a8a9..0bc40cb072e3 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_ggtt_types.h
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
> >>>>>>>> @@ -31,7 +31,7 @@ struct xe_ggtt {
> >>>>>>>>
> >>>>>>>> struct {
> >>>>>>>> u64 (*pte_encode)(struct xe_device *xe, u64 pte_pat,
> >>>>>>>> - enum xe_cache_level
> >> cache);
> >>>>>>>> + u32 pat_index);
> >>>>>>>> } pat_encode;
> >>>>>>>> };
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c
> >>>>>> b/drivers/gpu/drm/xe/xe_migrate.c
> >>>>>>>> index a782ea282cb6..54585e98452a 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_migrate.c
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> >>>>>>>> @@ -24,6 +24,7 @@
> >>>>>>>> #include "xe_lrc.h"
> >>>>>>>> #include "xe_map.h"
> >>>>>>>> #include "xe_mocs.h"
> >>>>>>>> +#include "xe_pat.h"
> >>>>>>>> #include "xe_pt.h"
> >>>>>>>> #include "xe_res_cursor.h"
> >>>>>>>> #include "xe_sched_job.h"
> >>>>>>>> @@ -162,6 +163,7 @@ static int xe_migrate_prepare_vm(struct
> >>>>>>>> xe_tile
> >>>> *tile,
> >>>>>> struct xe_migrate *m,
> >>>>>>>> u32 num_entries = NUM_PT_SLOTS, num_level = vm-
> >>> pt_root[id]-
> >>>>>>> level;
> >>>>>>>> u32 map_ofs, level, i;
> >>>>>>>> struct xe_bo *bo, *batch = tile->mem.kernel_bb_pool->bo;
> >>>>>>>> + u32 pat_index = xe_pat_get_index(xe, XE_CACHE_WB);
> >>>>>>>> u64 entry;
> >>>>>>>> int ret;
> >>>>>>>>
> >>>>>>>> @@ -189,14 +191,14 @@ static int xe_migrate_prepare_vm(struct
> >>>> xe_tile
> >>>>>> *tile, struct xe_migrate *m,
> >>>>>>>> return ret;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - entry = xe_pde_encode(bo, bo->size - XE_PAGE_SIZE,
> >>>> XE_CACHE_WB);
> >>>>>>>> + entry = xe_pde_encode(bo, bo->size - XE_PAGE_SIZE,
> >> pat_index);
> >>>>>>>> xe_pt_write(xe, &vm->pt_root[id]->bo->vmap, 0, entry);
> >>>>>>>>
> >>>>>>>> map_ofs = (num_entries - num_level) * XE_PAGE_SIZE;
> >>>>>>>>
> >>>>>>>> /* Map the entire BO in our level 0 pt */
> >>>>>>>> for (i = 0, level = 0; i < num_entries; level++) {
> >>>>>>>> - entry = xe_pte_encode(bo, i * XE_PAGE_SIZE,
> >> XE_CACHE_WB,
> >>>>>> 0);
> >>>>>>>> + entry = xe_pte_encode(bo, i * XE_PAGE_SIZE,
> >> pat_index, 0);
> >>>>>>>>
> >>>>>>>> xe_map_wr(xe, &bo->vmap, map_ofs + level * 8, u64,
> >> entry);
> >>>>>>>>
> >>>>>>>> @@ -214,7 +216,7 @@ static int xe_migrate_prepare_vm(struct
> >>>>>>>> xe_tile
> >>>> *tile,
> >>>>>> struct xe_migrate *m,
> >>>>>>>> for (i = 0; i < batch->size;
> >>>>>>>> i += vm->flags & XE_VM_FLAG_64K ?
> >> XE_64K_PAGE_SIZE :
> >>>>>>>> XE_PAGE_SIZE) {
> >>>>>>>> - entry = xe_pte_encode(batch, i,
> >> XE_CACHE_WB, 0);
> >>>>>>>> + entry = xe_pte_encode(batch, i, pat_index, 0);
> >>>>>>>>
> >>>>>>>> xe_map_wr(xe, &bo->vmap, map_ofs + level
> *
> >> 8, u64,
> >>>>>>>> entry);
> >>>>>>>> @@ -239,7 +241,7 @@ static int xe_migrate_prepare_vm(struct
> >>>>>>>> xe_tile
> >>>> *tile,
> >>>>>> struct xe_migrate *m,
> >>>>>>>> flags = XE_PDE_64K;
> >>>>>>>>
> >>>>>>>> entry = xe_pde_encode(bo, map_ofs + (level - 1) *
> >>>>>>>> - XE_PAGE_SIZE,
> >> XE_CACHE_WB);
> >>>>>>>> + XE_PAGE_SIZE, pat_index);
> >>>>>>>> xe_map_wr(xe, &bo->vmap, map_ofs +
> XE_PAGE_SIZE
> >> * level,
> >>>>>> u64,
> >>>>>>>> entry | flags);
> >>>>>>>> }
> >>>>>>>> @@ -247,7 +249,7 @@ static int xe_migrate_prepare_vm(struct
> >>>>>>>> xe_tile
> >>>> *tile,
> >>>>>> struct xe_migrate *m,
> >>>>>>>> /* Write PDE's that point to our BO. */
> >>>>>>>> for (i = 0; i < num_entries - num_level; i++) {
> >>>>>>>> entry = xe_pde_encode(bo, i * XE_PAGE_SIZE,
> >>>>>>>> - XE_CACHE_WB);
> >>>>>>>> + pat_index);
> >>>>>>>>
> >>>>>>>> xe_map_wr(xe, &bo->vmap, map_ofs +
> XE_PAGE_SIZE
> >> +
> >>>>>>>> (i + 1) * 8, u64, entry); @@ -1256,7 +1258,7
> >> @@
> >>>>>>>> xe_migrate_update_pgtables(struct
> >>>> xe_migrate
> >>>>>> *m,
> >>>>>>>>
> >>>>>>>> XE_WARN_ON(pt_bo->size != SZ_4K);
> >>>>>>>>
> >>>>>>>> - addr = xe_pte_encode(pt_bo, 0,
> >> XE_CACHE_WB, 0);
> >>>>>>>> + addr = xe_pte_encode(pt_bo, 0,
> >> xe_pat_get_index(xe,
> >>>>>> XE_CACHE_WB), 0);
> >>>>>>>> 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 64713f400d94..019af2920078
> >>>>>>>> 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_pt.c
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_pt.c
> >>>>>>>> @@ -10,6 +10,7 @@
> >>>>>>>> #include "xe_gt.h"
> >>>>>>>> #include "xe_gt_tlb_invalidation.h"
> >>>>>>>> #include "xe_migrate.h"
> >>>>>>>> +#include "xe_pat.h"
> >>>>>>>> #include "xe_pt_types.h"
> >>>>>>>> #include "xe_pt_walk.h"
> >>>>>>>> #include "xe_res_cursor.h"
> >>>>>>>> @@ -57,24 +58,22 @@ static struct xe_pt *xe_pt_entry(struct
> >>>>>>>> xe_pt_dir
> >>>>>> *pt_dir, unsigned int index)
> >>>>>>>> *
> >>>>>>>> * Return: An encoded page directory entry. No errors.
> >>>>>>>> */
> >>>>>>>> -u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
> >>>>>>>> - const enum xe_cache_level cache)
> >>>>>>>> +u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset, u32
> >>>>>>>> +pat_index)
> >>>>>>>> {
> >>>>>>>> u64 pde;
> >>>>>>>> struct xe_vm *vm = bo->vm;
> >>>>>>>> struct xe_device *xe = vm->xe;
> >>>>>>>>
> >>>>>>>> -
> >>>>>>>> pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
> >>>>>>>> pde |= XE_PAGE_PRESENT | XE_PAGE_RW;
> >>>>>>>>
> >>>>>>>> if ((vm->pat_encode).pde_encode)
> >>>>>>>> - pde = (vm->pat_encode).pde_encode(xe, pde, cache);
> >>>>>>>> + pde = (vm->pat_encode).pde_encode(xe, pde,
> >> pat_index);
> >>>>>>>>
> >>>>>>>> return pde;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> -static u64 __pte_encode(u64 pte, enum xe_cache_level cache,
> >>>>>>>> +static u64 __pte_encode(u64 pte, u32 pat_index,
> >>>>>>>> struct xe_vma *vma, u32 pt_level)
> >>>>>>>> {
> >>>>>>>> struct xe_vm *vm = xe_vma_vm(vma); @@ -89,7 +88,7 @@
> >> static
> >>>>>>>> u64 __pte_encode(u64 pte, enum
> >>>>>> xe_cache_level cache,
> >>>>>>>> pte |= XE_PTE_NULL;
> >>>>>>>>
> >>>>>>>> if ((vm->pat_encode).pte_encode)
> >>>>>>>> - pte = (vm->pat_encode).pte_encode(xe, pte, cache);
> >>>>>>>> + pte = (vm->pat_encode).pte_encode(xe, pte,
> >> pat_index);
> >>>>>>>>
> >>>>>>>> if (pt_level == 1)
> >>>>>>>> pte |= XE_PDE_PS_2M;
> >>>>>>>> @@ -112,7 +111,7 @@ static u64 __pte_encode(u64 pte, enum
> >>>>>> xe_cache_level cache,
> >>>>>>>> *
> >>>>>>>> * Return: An encoded page-table entry. No errors.
> >>>>>>>> */
> >>>>>>>> -u64 xe_pte_encode(struct xe_bo *bo, u64 offset, enum
> >>>>>>>> xe_cache_level
> >>>>>> cache,
> >>>>>>>> +u64 xe_pte_encode(struct xe_bo *bo, u64 offset, u32 pat_index,
> >>>>>>>> u32 pt_level)
> >>>>>>>> {
> >>>>>>>> u64 pte;
> >>>>>>>> @@ -121,7 +120,7 @@ u64 xe_pte_encode(struct xe_bo *bo, u64
> >>>> offset,
> >>>>>> enum xe_cache_level cache,
> >>>>>>>> if (xe_bo_is_vram(bo) || xe_bo_is_stolen_devmem(bo))
> >>>>>>>> pte |= XE_PPGTT_PTE_DM;
> >>>>>>>>
> >>>>>>>> - return __pte_encode(pte, cache, NULL, pt_level);
> >>>>>>>> + return __pte_encode(pte, pat_index, NULL, pt_level);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct
> >>>>>>>> xe_vm *vm, @@ -134,12 +133,12 @@ static u64
> >>>>>>>> __xe_pt_empty_pte(struct xe_tile
> >>>> *tile,
> >>>>>> struct xe_vm *vm,
> >>>>>>>>
> >>>>>>>> if (level == 0) {
> >>>>>>>> u64 empty = xe_pte_encode(vm->scratch_bo[id], 0,
> >>>>>>>> - XE_CACHE_WB, 0);
> >>>>>>>> + xe_pat_get_index(vm->xe,
> >>>>>> XE_CACHE_WB), 0);
> >>>>>>>>
> >>>>>>>> return empty;
> >>>>>>>> } else {
> >>>>>>>> return xe_pde_encode(vm->scratch_pt[id][level - 1]-
> >>> bo, 0,
> >>>>>>>> - XE_CACHE_WB);
> >>>>>>>> + xe_pat_get_index(vm->xe,
> >>>>>> XE_CACHE_WB));
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> @@ -368,8 +367,6 @@ struct xe_pt_stage_bind_walk {
> >>>>>>>> struct xe_vm *vm;
> >>>>>>>> /** @tile: The tile we're building for. */
> >>>>>>>> struct xe_tile *tile;
> >>>>>>>> - /** @cache: Desired cache level for the ptes */
> >>>>>>>> - enum xe_cache_level cache;
> >>>>>>>> /** @default_pte: PTE flag only template. No address is
> >> associated */
> >>>>>>>> u64 default_pte;
> >>>>>>>> /** @dma_offset: DMA offset to add to the PTE. */ @@
> >>>>>>>> -604,7
> >>>>>>>> +601,7 @@ xe_pt_stage_bind_entry(struct xe_ptw *parent,
> >>>>>> pgoff_t offset,
> >>>>>>>>
> >>>>>>>> pte = __pte_encode(is_null ? 0 :
> >>>>>>>> xe_res_dma(curs) + xe_walk-
> >>> dma_offset,
> >>>>>>>> - xe_walk->cache, xe_walk->vma,
> >> level);
> >>>>>>>> + xe_walk->vma->pat_index,
> xe_walk-
> >>> vma,
> >>>>>> level);
> >>>>>>>> pte |= xe_walk->default_pte;
> >>>>>>>>
> >>>>>>>> /*
> >>>>>>>> @@ -669,7 +666,7 @@ xe_pt_stage_bind_entry(struct xe_ptw
> >>>>>>>> *parent,
> >>>>>> pgoff_t offset,
> >>>>>>>> xe_child->is_compact = true;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - pte = xe_pde_encode(xe_child->bo, 0, xe_walk-
> >cache)
> >> |
> >>>> flags;
> >>>>>>>> + pte = xe_pde_encode(xe_child->bo, 0, xe_walk->vma-
> >>>>>>> pat_index) | flags;
> >>>>>>>> ret = xe_pt_insert_entry(xe_walk, xe_parent, offset,
> >> xe_child,
> >>>>>>>> pte);
> >>>>>>>> }
> >>>>>>>> @@ -730,13 +727,8 @@ xe_pt_stage_bind(struct xe_tile *tile,
> >>>>>>>> struct
> >>>> xe_vma
> >>>>>> *vma,
> >>>>>>>> if (vma && vma->gpuva.flags &
> >> XE_VMA_ATOMIC_PTE_BIT)
> >>>>>>>> xe_walk.default_pte |=
> >> XE_USM_PPGTT_PTE_AE;
> >>>>>>>> xe_walk.dma_offset = vram_region_gpu_offset(bo-
> >>>>>>> ttm.resource);
> >>>>>>>> - xe_walk.cache = XE_CACHE_WB;
> >>>>>>>> - } else {
> >>>>>>>> - if (!xe_vma_has_no_bo(vma) && bo->flags &
> >>>>>> XE_BO_SCANOUT_BIT)
> >>>>>>>> - xe_walk.cache = XE_CACHE_WT;
> >>>>>>>> - else
> >>>>>>>> - xe_walk.cache = XE_CACHE_WB;
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> if (!xe_vma_has_no_bo(vma) && xe_bo_is_stolen(bo))
> >>>>>>>> xe_walk.dma_offset =
> >>>>>> xe_ttm_stolen_gpu_offset(xe_bo_device(bo));
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_pt.h
> >>>>>>>> b/drivers/gpu/drm/xe/xe_pt.h index 01be7ab08f87..1d433a5a96b4
> >>>>>>>> 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_pt.h
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_pt.h
> >>>>>>>> @@ -45,10 +45,8 @@ __xe_pt_unbind_vma(struct xe_tile *tile,
> >>>>>>>> struct
> >>>>>> xe_vma *vma, struct xe_exec_queu
> >>>>>>>>
> >>>>>>>> bool xe_pt_zap_ptes(struct xe_tile *tile, struct xe_vma
> >>>>>>>> *vma);
> >>>>>>>>
> >>>>>>>> -u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
> >>>>>>>> - const enum xe_cache_level level);
> >>>>>>>> +u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset, u32
> >>>>>>>> +pat_index);
> >>>>>>>>
> >>>>>>>> -u64 xe_pte_encode(struct xe_bo *bo, u64 offset, enum
> >>>>>>>> xe_cache_level
> >>>>>> cache,
> >>>>>>>> - u32 pt_level);
> >>>>>>>> +u64 xe_pte_encode(struct xe_bo *bo, u64 offset, u32 pat_index,
> >>>>>>>> +u32
> >>>>>> pt_level);
> >>>>>>>>
> >>>>>>>> #endif
> >>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_vm.c
> >>>>>>>> b/drivers/gpu/drm/xe/xe_vm.c index
> 7eeeed0411f3..34603a7e84b0
> >>>>>>>> 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_vm.c
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_vm.c
> >>>>>>>> @@ -6,6 +6,7 @@
> >>>>>>>> #include "xe_vm.h"
> >>>>>>>>
> >>>>>>>> #include <linux/dma-fence-array.h>
> >>>>>>>> +#include <linux/nospec.h>
> >>>>>>>>
> >>>>>>>> #include <drm/drm_print.h>
> >>>>>>>> #include <drm/ttm/ttm_execbuf_util.h> @@ -874,7 +875,8 @@
> >>>>>>>> static struct xe_vma *xe_vma_create(struct
> >>>> xe_vm
> >>>>>> *vm,
> >>>>>>>> u64 start, u64 end,
> >>>>>>>> bool read_only,
> >>>>>>>> bool is_null,
> >>>>>>>> - u8 tile_mask)
> >>>>>>>> + u8 tile_mask,
> >>>>>>>> + u32 pat_index)
> >>>>>>>> {
> >>>>>>>> struct xe_vma *vma;
> >>>>>>>> struct xe_tile *tile;
> >>>>>>>> @@ -913,6 +915,8 @@ static struct xe_vma
> *xe_vma_create(struct
> >>>> xe_vm
> >>>>>> *vm,
> >>>>>>>> vma->tile_mask |= 0x1 << id;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> + vma->pat_index = pat_index;
> >>>>>>>> +
> >>>>>>>> if (vm->xe->info.platform == XE_PVC)
> >>>>>>>> vma->gpuva.flags |= XE_VMA_ATOMIC_PTE_BIT;
> >>>>>>>>
> >>>>>>>> @@ -1194,10 +1198,8 @@ static void
> xe_vma_op_work_func(struct
> >>>>>> work_struct *w);
> >>>>>>>> static void vm_destroy_work_func(struct work_struct *w);
> >>>>>>>>
> >>>>>>>> static u64 xelp_ppgtt_pde_encode_pat(struct xe_device *xe,
> >>>>>>>> u64
> >>>> pde_pat,
> >>>>>>>> - enum xe_cache_level
> >> cache)
> >>>>>>>> + u32 pat_index)
> >>>>>>>> {
> >>>>>>>> - u32 pat_index = xe_pat_get_index(xe, cache);
> >>>>>>>> -
> >>>>>>>> pde_pat &= ~(XELP_PDE_PAT_MASK);
> >>>>>>>>
> >>>>>>>> if (pat_index & BIT(0))
> >>>>>>>> @@ -1213,10 +1215,8 @@ static u64
> >>>> xelp_ppgtt_pde_encode_pat(struct
> >>>>>> xe_device *xe, u64 pde_pat,
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> static u64 xelp_ppgtt_pte_encode_pat(struct xe_device *xe,
> >>>>>>>> u64
> >>>> pte_pat,
> >>>>>>>> - enum xe_cache_level
> >> cache)
> >>>>>>>> + u32 pat_index)
> >>>>>>>> {
> >>>>>>>> - u32 pat_index = xe_pat_get_index(xe, cache);
> >>>>>>>> -
> >>>>>>>> pte_pat &= ~(XELP_PTE_PAT_MASK);
> >>>>>>>>
> >>>>>>>> if (pat_index & BIT(0))
> >>>>>>>> @@ -1622,7 +1622,7 @@ struct xe_vm *xe_vm_lookup(struct
> xe_file
> >>>> *xef,
> >>>>>> u32 id)
> >>>>>>>> u64 xe_vm_pdp4_descriptor(struct xe_vm *vm, struct xe_tile
> *tile)
> >>>>>>>> {
> >>>>>>>> return xe_pde_encode(vm->pt_root[tile->id]->bo, 0,
> >>>>>>>> - XE_CACHE_WB);
> >>>>>>>> + xe_pat_get_index(vm->xe,
> >> XE_CACHE_WB));
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> static struct dma_fence *
> >>>>>>>> @@ -2311,7 +2311,7 @@ static void print_op(struct xe_device
> >>>>>>>> *xe,
> >>>> struct
> >>>>>> drm_gpuva_op *op)
> >>>>>>>> static struct drm_gpuva_ops *
> >>>>>>>> vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
> >>>>>>>> u64 bo_offset_or_userptr, u64 addr, u64
> >> range,
> >>>>>>>> - u32 operation, u8 tile_mask, u32 region)
> >>>>>>>> + u32 operation, u8 tile_mask, u32 region, u32
> >>>>>> pat_index)
> >>>>>>>> {
> >>>>>>>> struct drm_gem_object *obj = bo ? &bo->ttm.base : NULL;
> >>>>>>>> struct ww_acquire_ctx ww; @@ -2339,6 +2339,7 @@
> >>>>>>>> vm_bind_ioctl_ops_create(struct xe_vm
> >> *vm,
> >>>>>> struct xe_bo *bo,
> >>>>>>>> struct xe_vma_op *op =
> >> gpuva_op_to_vma_op(__op);
> >>>>>>>>
> >>>>>>>> op->tile_mask = tile_mask;
> >>>>>>>> + op->pat_index = pat_index;
> >>>>>>>> op->map.immediate =
> >>>>>>>> operation &
> >>>> XE_VM_BIND_FLAG_IMMEDIATE;
> >>>>>>>> op->map.read_only =
> >>>>>>>> @@ -2366,6 +2367,7 @@ vm_bind_ioctl_ops_create(struct xe_vm
> >> *vm,
> >>>>>> struct xe_bo *bo,
> >>>>>>>> struct xe_vma_op *op =
> >> gpuva_op_to_vma_op(__op);
> >>>>>>>>
> >>>>>>>> op->tile_mask = tile_mask;
> >>>>>>>> + op->pat_index = pat_index;
> >>>>>>>> op->prefetch.region = region;
> >>>>>>>> }
> >>>>>>>> break;
> >>>>>>>> @@ -2408,7 +2410,8 @@ vm_bind_ioctl_ops_create(struct xe_vm
> >> *vm,
> >>>>>> struct xe_bo *bo,
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> static struct xe_vma *new_vma(struct xe_vm *vm, struct
> >>>>>> drm_gpuva_op_map *op,
> >>>>>>>> - u8 tile_mask, bool read_only, bool is_null)
> >>>>>>>> + u8 tile_mask, bool read_only, bool is_null,
> >>>>>>>> + u32 pat_index)
> >>>>>>>> {
> >>>>>>>> struct xe_bo *bo = op->gem.obj ? gem_to_xe_bo(op-
> >>> gem.obj) :
> >>>> NULL;
> >>>>>>>> struct xe_vma *vma;
> >>>>>>>> @@ -2425,7 +2428,7 @@ static struct xe_vma *new_vma(struct
> >>>>>>>> xe_vm
> >>>>>> *vm, struct drm_gpuva_op_map *op,
> >>>>>>>> vma = xe_vma_create(vm, bo, op->gem.offset,
> >>>>>>>> op->va.addr, op->va.addr +
> >>>>>>>> op->va.range - 1, read_only, is_null,
> >>>>>>>> - tile_mask);
> >>>>>>>> + tile_mask, pat_index);
> >>>>>>>> if (bo)
> >>>>>>>> xe_bo_unlock(bo, &ww);
> >>>>>>>>
> >>>>>>>> @@ -2539,7 +2542,7 @@ static int
> vm_bind_ioctl_ops_parse(struct
> >>>> xe_vm
> >>>>>> *vm, struct xe_exec_queue *q,
> >>>>>>>>
> >>>>>>>> vma = new_vma(vm, &op-
> >base.map,
> >>>>>>>> op->tile_mask, op-
> >>>>> map.read_only,
> >>>>>>>> - op->map.is_null);
> >>>>>>>> + op->map.is_null, op-
> >>> pat_index);
> >>>>>>>> if (IS_ERR(vma)) {
> >>>>>>>> err = PTR_ERR(vma);
> >>>>>>>> goto free_fence;
> >>>>>>>> @@ -2567,7 +2570,7 @@ static int
> vm_bind_ioctl_ops_parse(struct
> >>>> xe_vm
> >>>>>> *vm, struct xe_exec_queue *q,
> >>>>>>>>
> >>>>>>>> vma = new_vma(vm, op-
> >>>>>>> base.remap.prev,
> >>>>>>>> op->tile_mask,
> >> read_only,
> >>>>>>>> - is_null);
> >>>>>>>> + is_null, op-
> >>> pat_index);
> >>>>>>>> if (IS_ERR(vma)) {
> >>>>>>>> err = PTR_ERR(vma);
> >>>>>>>> goto free_fence;
> >>>>>>>> @@ -2603,7 +2606,7 @@ static int
> vm_bind_ioctl_ops_parse(struct
> >>>> xe_vm
> >>>>>> *vm, struct xe_exec_queue *q,
> >>>>>>>>
> >>>>>>>> vma = new_vma(vm, op-
> >>>>>>> base.remap.next,
> >>>>>>>> op->tile_mask,
> >> read_only,
> >>>>>>>> - is_null);
> >>>>>>>> + is_null, op-
> >>> pat_index);
> >>>>>>>> if (IS_ERR(vma)) {
> >>>>>>>> err = PTR_ERR(vma);
> >>>>>>>> goto free_fence;
> >>>>>>>> @@ -3158,8 +3161,14 @@ static int
> >>>>>>>> vm_bind_ioctl_check_args(struct
> >>>>>> xe_device *xe,
> >>>>>>>> u32 obj = (*bind_ops)[i].obj;
> >>>>>>>> u64 obj_offset = (*bind_ops)[i].obj_offset;
> >>>>>>>> u32 region = (*bind_ops)[i].region;
> >>>>>>>> + u32 pat_index = (*bind_ops)[i].pat_index;
> >>>>>>>> bool is_null = op & XE_VM_BIND_FLAG_NULL;
> >>>>>>>>
> >>>>>>>> + if (XE_IOCTL_DBG(xe, pat_index >= xe-
> >>>>>>> info.pat_table_n_entries)) {
> >>>>>>>> + err = -EINVAL;
> >>>>>>>> + goto free_bind_ops;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> if (i == 0) {
> >>>>>>>> *async = !!(op &
> XE_VM_BIND_FLAG_ASYNC);
> >>>>>>>> } else if (XE_IOCTL_DBG(xe, !*async) || @@ -3346,8
> >> +3355,25
> >>>>>>>> @@ int xe_vm_bind_ioctl(struct drm_device
> >>>> *dev,
> >>>>>> void *data, struct drm_file *file)
> >>>>>>>> struct drm_gem_object *gem_obj;
> >>>>>>>> u64 range = bind_ops[i].range;
> >>>>>>>> u64 addr = bind_ops[i].addr;
> >>>>>>>> + u32 op = bind_ops[i].op;
> >>>>>>>> u32 obj = bind_ops[i].obj;
> >>>>>>>> u64 obj_offset = bind_ops[i].obj_offset;
> >>>>>>>> + u32 pat_index = bind_ops[i].pat_index;
> >>>>>>>> + u16 coh_mode;
> >>>>>>>> +
> >>>>>>>> + pat_index = array_index_nospec(pat_index,
> >>>>>>>> + xe-
> >>> info.pat_table_n_entries);
> >>>>>>>> + coh_mode = xe_pat_index_get_coh_mode(xe,
> >> pat_index);
> >>>>>>>> + if (XE_IOCTL_DBG(xe, !coh_mode)) {
> >>>>>>>
> >>>>>>> Assuming we drop the unusable entries from the TGL table, this
> >>>>>>> should be impossible, right? Any index that makes it past the
> >>>>>>> n_entries check at the top of the function should have a valid,
> >>>>>>> non-zero
> >> coh_mode value.
> >>>>>>> So this should probably be an assertion (to highlight a KMD bug)
> >>>>>>> rather than just a silent uapi failure return.
> >>>>>>
> >>>>>> Makes sense.
> >>>>>>
> >>>>>>>
> >>>>>>>> + err = -EINVAL;
> >>>>>>>> + goto put_obj;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + if (XE_IOCTL_DBG(xe, VM_BIND_OP(op) ==
> >>>>>> XE_VM_BIND_OP_MAP_USERPTR &&
> >>>>>>>> + coh_mode ==
> >> XE_GEM_COHERENCY_NONE))
> >>>> {
> >>>>>>>> + err = -EINVAL;
> >>>>>>>> + goto put_obj;
> >>>>>>>> + }
> >>>>>>>>
> >>>>>>>> if (!obj)
> >>>>>>>> continue;
> >>>>>>>> @@ -3375,6 +3401,22 @@ int xe_vm_bind_ioctl(struct
> drm_device
> >>>> *dev,
> >>>>>> void *data, struct drm_file *file)
> >>>>>>>> goto put_obj;
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> + if (bos[i]->coh_mode) {
> >>>>>>>> + if (XE_IOCTL_DBG(xe, bos[i]->coh_mode !=
> >>>>>> coh_mode)) {
> >>>>>>>> + err = -EINVAL;
> >>>>>>>> + goto put_obj;
> >>>>>>>> + }
> >>>>>>>> + } else if (XE_IOCTL_DBG(xe, coh_mode ==
> >>>>>> XE_GEM_COHERENCY_NONE)) {
> >>>>>>>> + /*
> >>>>>>>> + * Imported dma-buf from a different device
> >> should
> >>>>>>>> + * require 1way or 2way coherency since we
> >> don't
> >>>>>> know
> >>>>>>>> + * how it was mapped on CPU. Just assume is
> it
> >>>>>>>> + * potentially cached on CPU side.
> >>>>>>>> + */
> >>>>>>>> + err = -EINVAL;
> >>>>>>>> + goto put_obj;
> >>>>>>>> + }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> if (args->num_syncs) {
> >>>>>>>> @@ -3412,10 +3454,11 @@ int xe_vm_bind_ioctl(struct
> drm_device
> >>>> *dev,
> >>>>>> void *data, struct drm_file *file)
> >>>>>>>> u64 obj_offset = bind_ops[i].obj_offset;
> >>>>>>>> u8 tile_mask = bind_ops[i].tile_mask;
> >>>>>>>> u32 region = bind_ops[i].region;
> >>>>>>>> + u32 pat_index = bind_ops[i].pat_index;
> >>>>>>>>
> >>>>>>>> ops[i] = vm_bind_ioctl_ops_create(vm, bos[i],
> >> obj_offset,
> >>>>>>>> addr, range, op,
> >> tile_mask,
> >>>>>>>> - region);
> >>>>>>>> + region, pat_index);
> >>>>>>>> if (IS_ERR(ops[i])) {
> >>>>>>>> err = PTR_ERR(ops[i]);
> >>>>>>>> ops[i] = NULL;
> >>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> >>>>>> b/drivers/gpu/drm/xe/xe_vm_types.h
> >>>>>>>> index 83a1f87b6537..508679ed3c74 100644
> >>>>>>>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> >>>>>>>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> >>>>>>>> @@ -111,6 +111,11 @@ struct xe_vma {
> >>>>>>>> */
> >>>>>>>> u8 tile_present;
> >>>>>>>>
> >>>>>>>> + /**
> >>>>>>>> + * @pat_index: The pat index to use when encoding the PTEs
> >> for
> >>>>>>>> +this
> >>>>>> vma.
> >>>>>>>> + */
> >>>>>>>> + u32 pat_index;
> >>>>>>>> +
> >>>>>>>> struct {
> >>>>>>>> struct list_head rebind_link;
> >>>>>>>> } notifier;
> >>>>>>>> @@ -338,10 +343,8 @@ struct xe_vm {
> >>>>>>>> bool batch_invalidate_tlb;
> >>>>>>>>
> >>>>>>>> struct {
> >>>>>>>> - u64 (*pte_encode)(struct xe_device *xe, u64 pte_pat,
> >>>>>>>> - enum xe_cache_level
> >> cache);
> >>>>>>>> - u64 (*pde_encode)(struct xe_device *xe, u64
> pde_pat,
> >>>>>>>> - enum xe_cache_level
> >> cache);
> >>>>>>>> + u64 (*pte_encode)(struct xe_device *xe, u64 pte_pat,
> >> u32
> >>>>>> pat_index);
> >>>>>>>> + u64 (*pde_encode)(struct xe_device *xe, u64
> pde_pat,
> >> u32
> >>>>>> pat_index);
> >>>>>>>> } pat_encode;
> >>>>>>>> };
> >>>>>>>>
> >>>>>>>> @@ -417,6 +420,8 @@ struct xe_vma_op {
> >>>>>>>> struct async_op_fence *fence;
> >>>>>>>> /** @tile_mask: gt mask for this operation */
> >>>>>>>> u8 tile_mask;
> >>>>>>>> + /** @pat_index: The pat index to use for this operation. */
> >>>>>>>> + u32 pat_index;
> >>>>>>>> /** @flags: operation flags */
> >>>>>>>> enum xe_vma_op_flags flags;
> >>>>>>>>
> >>>>>>>> diff --git a/include/uapi/drm/xe_drm.h
> >>>>>>>> b/include/uapi/drm/xe_drm.h index 64bc66d4b550..0c15b6f32447
> >>>>>>>> 100644
> >>>>>>>> --- a/include/uapi/drm/xe_drm.h
> >>>>>>>> +++ b/include/uapi/drm/xe_drm.h
> >>>>>>>> @@ -600,8 +600,45 @@ struct drm_xe_vm_bind_op {
> >>>>>>>> */
> >>>>>>>> __u32 obj;
> >>>>>>>>
> >>>>>>>> - /** @pad: MBZ */
> >>>>>>>> - __u32 pad;
> >>>>>>>> + /**
> >>>>>>>> + * @pat_index: The platform defined @pat_index to use for
> >> this
> >>>>>> mapping.
> >>>>>>>> + * The index basically maps to some predefined memory
> >> attributes,
> >>>>>>>> + * including things like caching, coherency and likely other
> >>>>>>>> +stuff
> >> in
> >>>>>>>> + * the future. The exact meaning of the pat_index is
> >>>>>>>> +platform specific
> >>>>>>>
> >>>>>>> BTW, "other stuff in the future" already includes compression on
> >>>>>>> Xe2, we just haven't landed the patches for the Xe2 table yet.
> >>>>>>
> >>>>>> Ok, good to know.
> >>>>>>
> >>>>>>>
> >>>>>>>> + * and defined in the Bspec and PRMs. When the KMD sets
> up
> >> the
> >>>>>> binding
> >>>>>>>> + * the index here is encoded into the ppGTT PTE.
> >>>>>>>> + *
> >>>>>>>> + * For coherency the @pat_index needs to match the
> >>>>>>>> + * drm_xe_gem_create.coh_mode, so either
> >>>>>> XE_GEM_COHERENCY_NONE,
> >>>>>>>> + * XE_GEM_COHERENCY_1WAY or
> >> XE_GEM_COHERENCY_2WAY. The
> >>>>>> KMD will extract
> >>>>>>>> + * the coherency mode from the @pat_index and reject if
> >> there is a
> >>>>>>>> + * mismatch (see note below for pre-MTL platforms).
> >>>>>>>> + *
> >>>>>>>> + * Note: On pre-MTL platforms there is only a caching mode
> >> and no
> >>>>>>>> + * explicit coherency mode, but on such hardware there is
> >> always a
> >>>>>>>> + * shared-LLC (or is dgpu) so all GT memory accesses are
> >>>>>>>> +coherent
> >>>> with
> >>>>>>>> + * CPU caches even with the caching mode set as uncached.
> >> It's
> >>>>>>>> +only
> >>>>>> the
> >>>>>>>> + * display engine that is incoherent (on dgpu it must be in
> >> VRAM which
> >>>>>>>> + * is always mapped as WC on the CPU). However to keep the
> >> uapi
> >>>>>> somewhat
> >>>>>>>> + * consistent with newer platforms the KMD groups the
> >> different
> >>>> cache
> >>>>>>>> + * levels into the following coherency buckets on all pre-MTL
> >>>> platforms:
> >>>>>>>> + *
> >>>>>>>> + * ppGTT UC -> XE_GEM_COHERENCY_NONE
> >>>>>>>> + * ppGTT WC -> XE_GEM_COHERENCY_NONE
> >>>>>>>> + * ppGTT WT -> XE_GEM_COHERENCY_NONE
> >>>>>>>> + * ppGTT WB -> XE_GEM_COHERENCY_2WAY
> >>>>>>>
> >>>>>>> As noted on the previous patch, it seems like 2-way is
> >>>>>>> appropriate for LLC platforms, but 1-way might be a more
> >>>>>>> accurate description of dGPU behavior.
> >>>>>>>
> >>>>>>>> + *
> >>>>>>>> + * In practice UC/WC/WT should only ever used for scanout
> >>>>>>>> +surfaces
> >>>> on
> >>>>>>>> + * such platforms since it is only the display engine that is
> >> actually
> >>>>>>>> + * incoherent. Everything else should typically use WB given
> >>>>>>>> +that we
> >>>>>>>
> >>>>>>> What if we're sharing our buffers with some other (non-GPU)
> >>>>>>> device? Are there cases where that other device wouldn't be
> >>>>>>> coherent with the LLC, so we'd want to use one of these?
> >>>>>>
> >>>>>> Yeah, I guess there might be cases like that. I'll reword.
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> Matt
> >>>>>>>
> >>>>>>>> + * have a shared-LLC. On MTL+ this completely changes (also
> >>>>>> potentially
> >>>>>>>> + * no shared-LLC) and the HW defines the coherency mode as
> >> part
> >>>>>>>> +of
> >>>>>> the
> >>>>>>>> + * @pat_index.
> >>>>>>>> + *
> >>>>>>>> + * Note: For userptr and externally imported dma-buf the
> >> kernel
> >>>>>> expects
> >>>>>>>> + * either 1WAY or 2WAY for the @pat_index.
> >>>>>>>> + */
> >>>>>>>> + __u32 pat_index;
> >>>>>>>>
> >>>>>>>> union {
> >>>>>>>> /**
> >>>>>>>> --
> >>>>>>>> 2.41.0
> >>>>>>>>
> >>>>>>>
More information about the Intel-xe
mailing list