[PATCH] drm/xe: Replace the scratch pages with NULL PTEs
Lucas De Marchi
lucas.demarchi at intel.com
Thu Dec 7 17:39:50 UTC 2023
On Thu, Dec 07, 2023 at 06:10:56PM +0100, Thomas Hellström wrote:
>
>On 12/7/23 17:42, Lucas De Marchi wrote:
>>On Thu, Dec 07, 2023 at 05:25:01PM +0100, Thomas Hellström wrote:
>>>+ intel-xe cc.
>>>
>>>
>>>On 12/7/23 17:17, Lucas De Marchi wrote:
>>>>On Thu, Dec 07, 2023 at 04:44:23PM +0100, Thomas Hellström wrote:
>>>>>Replace the scratch pages with NULL PTEs that return 0 when
>>>>>reading and
>>>>>do nothing when writing.
>>>>
>>>>but don't we want to keep scratch so we can check when things go wrong
>>>>and writes go where they shouldn't?
>>>
>>>The argument against that was that apps that incorrectly writes to
>>>scratch and then reads back the same value wouldn't notice that it
>>>did something wrong, and the case you mention would to some extent
>>>be captured by not having a scratch page at all and check for
>>>pagefaults?
>>
>>maybe... But if we have a null pte where we had scratch mapped, aren't we
>>actually masking the pagefault? What about platforms where we don't have
>>pagefault? Relying on pagefault alone I think would actually be: use a
>>null pte to account for HW prefetch where applicable, but don't use it
>>elsewhere, and only if we support pagefault on that platform. But I
>>may be misunderstanding to what degree this patch is replacing scratch
>>since I see you kept xe_vm_has_scratch().
>
>With the current uAPI we're by default setting unmapped PTEs to 0, but
>UMD can at VM creation request scratch PTEs instead, they are then
>resolving to a single RW page.
ok, I missed that this was opt-in and default behavior is not to map
anything else
>
>What this patch does (if it weren't buggy) is to change the latter to
>point to the NULL PTEs that read 0 and writes nothing, mainly for the
>purpose of (buggy?) 3D workloads that expect reading 0 if they access
>an unmapped area....
>
>But with the default (no scrach) behaviour any HW prefetch outside a
>mapped area would cause, I figure, at least an unrecoverable GPU
>pagefault or IOMMU pagefault, so I figure UMDs are typically expected
>to over-allocate to account for that?
yeah... afaik they are overallocating to account for prefetch. At leat
we do that in igt and I know mesa had to change that for xe2 too.. But I'm
not really happy with that since it keeps changing per engine type and
per platform. Maybe we should just add a null pte at the end of a
mapping to account for that?
Acked-by: Lucas De Marchi <lucas.demarchi at intel.com> on the direction
you are going here.
thanks
Lucas De Marchi
>
>/Thomas
>
>
>>
>>Lucas De Marchi
>>
>>
>>>
>>>I admit I haven't been following the discussions around scratch
>>>pages long enough to know the historical uses for it. Cc'ing
>>>Joonas for further discussion.
>>>
>>>/Thomas
>>>
>>>
>>>>
>>>>Lucas De Marchi
>>>>
>>>>>
>>>>>Cc: Brian Welty <brian.welty at intel.com>
>>>>>Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>>>>---
>>>>>drivers/gpu/drm/xe/xe_pt.c | 68 ++++----------------------------
>>>>>drivers/gpu/drm/xe/xe_pt.h | 3 --
>>>>>drivers/gpu/drm/xe/xe_vm.c | 45 +--------------------
>>>>>drivers/gpu/drm/xe/xe_vm.h | 5 +++
>>>>>drivers/gpu/drm/xe/xe_vm_types.h | 2 -
>>>>>5 files changed, 14 insertions(+), 109 deletions(-)
>>>>>
>>>>>diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>>>>>index 35bd7940a571..ce2846d56b04 100644
>>>>>--- a/drivers/gpu/drm/xe/xe_pt.c
>>>>>+++ b/drivers/gpu/drm/xe/xe_pt.c
>>>>>@@ -50,17 +50,14 @@ static struct xe_pt *xe_pt_entry(struct
>>>>>xe_pt_dir *pt_dir, unsigned int index)
>>>>>static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct xe_vm *vm,
>>>>> unsigned int level)
>>>>>{
>>>>>- u16 pat_index = tile_to_xe(tile)->pat.idx[XE_CACHE_WB];
>>>>>- u8 id = tile->id;
>>>>>+ struct xe_device *xe = tile_to_xe(tile);
>>>>>+ u16 pat_index = xe->pat.idx[XE_CACHE_WB];
>>>>>
>>>>>- if (!vm->scratch_bo[id])
>>>>>+ if (!xe_vm_has_scratch(vm))
>>>>> return 0;
>>>>>
>>>>>- if (level > 0)
>>>>>- return
>>>>>vm->pt_ops->pde_encode_bo(vm->scratch_pt[id][level - 1]->bo,
>>>>>- 0, pat_index);
>>>>>-
>>>>>- return vm->pt_ops->pte_encode_bo(vm->scratch_bo[id], 0,
>>>>>pat_index, 0);
>>>>>+ return vm->pt_ops->pte_encode_addr(xe, 0, pat_index,
>>>>>level, IS_DGFX(xe),
>>>>>+ 0) | XE_PTE_NULL;
>>>>>}
>>>>>
>>>>>/**
>>>>>@@ -135,7 +132,7 @@ void xe_pt_populate_empty(struct xe_tile
>>>>>*tile, struct xe_vm *vm,
>>>>> u64 empty;
>>>>> int i;
>>>>>
>>>>>- if (!vm->scratch_bo[tile->id]) {
>>>>>+ if (!xe_vm_has_scratch(vm)) {
>>>>> /*
>>>>> * FIXME: Some memory is allocated already allocated to zero?
>>>>> * Find out which memory that is and avoid this memset...
>>>>>@@ -194,57 +191,6 @@ void xe_pt_destroy(struct xe_pt *pt, u32
>>>>>flags, struct llist_head *deferred)
>>>>> kfree(pt);
>>>>>}
>>>>>
>>>>>-/**
>>>>>- * xe_pt_create_scratch() - Setup a scratch memory pagetable
>>>>>tree for the
>>>>>- * given tile and vm.
>>>>>- * @xe: xe device.
>>>>>- * @tile: tile to set up for.
>>>>>- * @vm: vm to set up for.
>>>>>- *
>>>>>- * Sets up a pagetable tree with one page-table per level and
>>>>>a single
>>>>>- * leaf bo. All pagetable entries point to the single page-table or,
>>>>>- * for L0, the single bo one level below.
>>>>>- *
>>>>>- * Return: 0 on success, negative error code on error.
>>>>>- */
>>>>>-int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
>>>>>- struct xe_vm *vm)
>>>>>-{
>>>>>- u8 id = tile->id;
>>>>>- unsigned int flags;
>>>>>- int i;
>>>>>-
>>>>>- /*
>>>>>- * So we don't need to worry about 64K TLB hints when
>>>>>dealing with
>>>>>- * scratch entires, rather keep the scratch page in
>>>>>system memory on
>>>>>- * platforms where 64K pages are needed for VRAM.
>>>>>- */
>>>>>- flags = XE_BO_CREATE_PINNED_BIT;
>>>>>- if (vm->flags & XE_VM_FLAG_64K)
>>>>>- flags |= XE_BO_CREATE_SYSTEM_BIT;
>>>>>- else
>>>>>- flags |= XE_BO_CREATE_VRAM_IF_DGFX(tile);
>>>>>-
>>>>>- vm->scratch_bo[id] = xe_bo_create_pin_map(xe, tile, vm, SZ_4K,
>>>>>- ttm_bo_type_kernel,
>>>>>- flags);
>>>>>- if (IS_ERR(vm->scratch_bo[id]))
>>>>>- return PTR_ERR(vm->scratch_bo[id]);
>>>>>-
>>>>>- xe_map_memset(vm->xe, &vm->scratch_bo[id]->vmap, 0, 0,
>>>>>- vm->scratch_bo[id]->size);
>>>>>-
>>>>>- for (i = 0; i < vm->pt_root[id]->level; i++) {
>>>>>- vm->scratch_pt[id][i] = xe_pt_create(vm, tile, i);
>>>>>- if (IS_ERR(vm->scratch_pt[id][i]))
>>>>>- return PTR_ERR(vm->scratch_pt[id][i]);
>>>>>-
>>>>>- xe_pt_populate_empty(tile, vm, vm->scratch_pt[id][i]);
>>>>>- }
>>>>>-
>>>>>- return 0;
>>>>>-}
>>>>>-
>>>>>/**
>>>>> * DOC: Pagetable building
>>>>> *
>>>>>@@ -1286,7 +1232,7 @@ __xe_pt_bind_vma(struct xe_tile *tile,
>>>>>struct xe_vma *vma, struct xe_exec_queue
>>>>> * it needs to be done here.
>>>>> */
>>>>> if ((rebind && !xe_vm_in_lr_mode(vm) &&
>>>>>!vm->batch_invalidate_tlb) ||
>>>>>- (!rebind && vm->scratch_bo[tile->id] &&
>>>>>xe_vm_in_preempt_fence_mode(vm))) {
>>>>>+ (!rebind && xe_vm_has_scratch(vm) &&
>>>>>xe_vm_in_preempt_fence_mode(vm))) {
>>>>> ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
>>>>> if (!ifence)
>>>>> return ERR_PTR(-ENOMEM);
>>>>>diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
>>>>>index d5460e58dbbf..6f7359ac55bd 100644
>>>>>--- a/drivers/gpu/drm/xe/xe_pt.h
>>>>>+++ b/drivers/gpu/drm/xe/xe_pt.h
>>>>>@@ -26,9 +26,6 @@ unsigned int xe_pt_shift(unsigned int level);
>>>>>struct xe_pt *xe_pt_create(struct xe_vm *vm, struct xe_tile *tile,
>>>>> unsigned int level);
>>>>>
>>>>>-int xe_pt_create_scratch(struct xe_device *xe, struct xe_tile *tile,
>>>>>- struct xe_vm *vm);
>>>>>-
>>>>>void xe_pt_populate_empty(struct xe_tile *tile, struct xe_vm *vm,
>>>>> struct xe_pt *pt);
>>>>>
>>>>>diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>>>>>index e09050f16f07..868b9a2a5d02 100644
>>>>>--- a/drivers/gpu/drm/xe/xe_vm.c
>>>>>+++ b/drivers/gpu/drm/xe/xe_vm.c
>>>>>@@ -1332,7 +1332,7 @@ static void vm_destroy_work_func(struct
>>>>>work_struct *w);
>>>>>struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
>>>>>{
>>>>> struct xe_vm *vm;
>>>>>- int err, i = 0, number_tiles = 0;
>>>>>+ int err, number_tiles = 0;
>>>>> struct xe_tile *tile;
>>>>> u8 id;
>>>>>
>>>>>@@ -1397,17 +1397,8 @@ struct xe_vm *xe_vm_create(struct
>>>>>xe_device *xe, u32 flags)
>>>>> }
>>>>> }
>>>>>
>>>>>- if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
>>>>>- for_each_tile(tile, xe, id) {
>>>>>- if (!vm->pt_root[id])
>>>>>- continue;
>>>>>-
>>>>>- err = xe_pt_create_scratch(xe, tile, vm);
>>>>>- if (err)
>>>>>- goto err_scratch_pt;
>>>>>- }
>>>>>+ if (xe_vm_has_scratch(vm))
>>>>> vm->batch_invalidate_tlb = true;
>>>>>- }
>>>>>
>>>>> if (flags & XE_VM_FLAG_LR_MODE) {
>>>>> INIT_WORK(&vm->preempt.rebind_work, preempt_rebind_work_func);
>>>>>@@ -1415,7 +1406,6 @@ struct xe_vm *xe_vm_create(struct
>>>>>xe_device *xe, u32 flags)
>>>>> vm->batch_invalidate_tlb = false;
>>>>> }
>>>>>
>>>>>- /* Fill pt_root after allocating scratch tables */
>>>>> for_each_tile(tile, xe, id) {
>>>>> if (!vm->pt_root[id])
>>>>> continue;
>>>>>@@ -1465,19 +1455,6 @@ struct xe_vm *xe_vm_create(struct
>>>>>xe_device *xe, u32 flags)
>>>>>
>>>>> return vm;
>>>>>
>>>>>-err_scratch_pt:
>>>>>- for_each_tile(tile, xe, id) {
>>>>>- if (!vm->pt_root[id])
>>>>>- continue;
>>>>>-
>>>>>- i = vm->pt_root[id]->level;
>>>>>- while (i)
>>>>>- if (vm->scratch_pt[id][--i])
>>>>>- xe_pt_destroy(vm->scratch_pt[id][i],
>>>>>- vm->flags, NULL);
>>>>>- xe_bo_unpin(vm->scratch_bo[id]);
>>>>>- xe_bo_put(vm->scratch_bo[id]);
>>>>>- }
>>>>>err_destroy_root:
>>>>> for_each_tile(tile, xe, id) {
>>>>> if (vm->pt_root[id])
>>>>>@@ -1556,24 +1533,6 @@ void xe_vm_close_and_put(struct xe_vm *vm)
>>>>> vma->gpuva.flags |= XE_VMA_DESTROYED;
>>>>> }
>>>>>
>>>>>- /*
>>>>>- * All vm operations will add shared fences to resv.
>>>>>- * The only exception is eviction for a shared object,
>>>>>- * but even so, the unbind when evicted would still
>>>>>- * install a fence to resv. Hence it's safe to
>>>>>- * destroy the pagetables immediately.
>>>>>- */
>>>>>- for_each_tile(tile, xe, id) {
>>>>>- if (vm->scratch_bo[id]) {
>>>>>- u32 i;
>>>>>-
>>>>>- xe_bo_unpin(vm->scratch_bo[id]);
>>>>>- xe_bo_put(vm->scratch_bo[id]);
>>>>>- for (i = 0; i < vm->pt_root[id]->level; i++)
>>>>>- xe_pt_destroy(vm->scratch_pt[id][i], vm->flags,
>>>>>- NULL);
>>>>>- }
>>>>>- }
>>>>> xe_vm_unlock(vm);
>>>>>
>>>>> /*
>>>>>diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
>>>>>index 9a0ae19c47b7..b92d8f54011d 100644
>>>>>--- a/drivers/gpu/drm/xe/xe_vm.h
>>>>>+++ b/drivers/gpu/drm/xe/xe_vm.h
>>>>>@@ -61,6 +61,11 @@ static inline bool
>>>>>xe_vm_is_closed_or_banned(struct xe_vm *vm)
>>>>> return xe_vm_is_closed(vm) || xe_vm_is_banned(vm);
>>>>>}
>>>>>
>>>>>+static inline bool xe_vm_has_scratch(const struct xe_vm *vm)
>>>>>+{
>>>>>+ return vm->flags & XE_VM_FLAG_SCRATCH_PAGE;
>>>>>+}
>>>>>+
>>>>>struct xe_vma *
>>>>>xe_vm_find_overlapping_vma(struct xe_vm *vm, u64 start, u64 range);
>>>>>
>>>>>diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
>>>>>b/drivers/gpu/drm/xe/xe_vm_types.h
>>>>>index 23abdfd8622f..a5762e99e6aa 100644
>>>>>--- a/drivers/gpu/drm/xe/xe_vm_types.h
>>>>>+++ b/drivers/gpu/drm/xe/xe_vm_types.h
>>>>>@@ -158,8 +158,6 @@ struct xe_vm {
>>>>> u64 size;
>>>>>
>>>>> struct xe_pt *pt_root[XE_MAX_TILES_PER_DEVICE];
>>>>>- struct xe_bo *scratch_bo[XE_MAX_TILES_PER_DEVICE];
>>>>>- struct xe_pt
>>>>>*scratch_pt[XE_MAX_TILES_PER_DEVICE][XE_VM_MAX_LEVEL];
>>>>>
>>>>> /**
>>>>> * @flags: flags for this VM, statically setup a creation
>>>>>time aside
>>>>>-- 2.42.0
>>>>>
More information about the Intel-xe
mailing list