[PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Shashank Sharma]

From: Christian König <christian.koenig@amd.com>

The problem is that when (for example) 4k pages are replaced
with a single 2M page we need to wait for change to be flushed
out by invalidating the TLB before the PT can be freed.

Solve this by moving the TLB flush into a DMA-fence object which
can be used to delay the freeing of the PT BOs until it is signaled.

V2: (Shashank)
    - rebase
    - set dma_fence_error only in case of error
    - add tlb_flush fence only when PT/PD BO is locked (Felix)
    - use vm->pasid when f is NULL (Mukul)

V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
    - move the misplaced fence_create call to the end (Philip)

V5: - free the f->dependency properly (Christian)

Cc: Christian Koenig <christian.koenig@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
 .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 ++++++++++++++++++
 4 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index fa26a4e3a99d..91ab4cf29b5b 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o amdgpu_kms.o \
 	amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
 	atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
 	atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
-	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o amdgpu_pll.o \
+	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_vm_tlb_fence.o \
+	amdgpu_ib.o amdgpu_pll.o \
 	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
 	amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o \
 	amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 0960e0a665d3..310aae6fb49b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	r = vm->update_funcs->commit(&params, fence);
 
+	/* Prepare a TLB flush fence to be attached to PTs */
+	if (!unlocked && params.needs_flush && vm->is_compute_context) {
+		amdgpu_vm_tlb_fence_create(adev, vm, fence);
+
+		/* Makes sure no PD/PT is freed before the flush */
+		dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
+				   DMA_RESV_USAGE_BOOKKEEP);
+	}
+
 error_unlock:
 	amdgpu_vm_eviction_unlock(vm);
 	drm_dev_exit(idx);
@@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	mutex_init(&vm->eviction_lock);
 	vm->evicting = false;
+	vm->tlb_fence_context = dma_fence_context_alloc(1);
 
 	r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
 				false, &root, xcp_id);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 64b3f69efa57..298f604b8e5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -341,6 +341,7 @@ struct amdgpu_vm {
 	atomic64_t		tlb_seq;
 	uint64_t		tlb_seq_va;
 	uint64_t		*tlb_seq_cpu_addr;
+	uint64_t		tlb_fence_context;
 
 	atomic64_t		kfd_last_flushed_seq;
 
@@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
 				  uint64_t addr,
 				  uint32_t status,
 				  unsigned int vmhub);
+void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
+				 struct amdgpu_vm *vm,
+				 struct dma_fence **fence);
 
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
new file mode 100644
index 000000000000..51cddfa3f1e8
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/dma-fence.h>
+#include <linux/workqueue.h>
+
+#include "amdgpu.h"
+#include "amdgpu_vm.h"
+#include "amdgpu_gmc.h"
+
+struct amdgpu_tlb_fence {
+	struct dma_fence	base;
+	struct amdgpu_device	*adev;
+	struct dma_fence	*dependency;
+	struct work_struct	work;
+	spinlock_t		lock;
+	uint16_t		pasid;
+
+};
+
+static const char *amdgpu_tlb_fence_get_driver_name(struct dma_fence *fence)
+{
+	return "amdgpu tlb fence";
+}
+
+static const char *amdgpu_tlb_fence_get_timeline_name(struct dma_fence *f)
+{
+	return "amdgpu tlb timeline";
+}
+
+static void amdgpu_tlb_fence_work(struct work_struct *work)
+{
+	struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
+	int r;
+
+	if (f->dependency) {
+		dma_fence_wait(f->dependency, false);
+		dma_fence_put(f->dependency);
+		f->dependency = NULL;
+	}
+
+	r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
+	if (r) {
+		dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
+			f->pasid);
+		dma_fence_set_error(&f->base, r);
+	}
+
+	dma_fence_signal(&f->base);
+	dma_fence_put(&f->base);
+}
+
+static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
+	.use_64bit_seqno = true,
+	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
+	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
+};
+
+void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+				struct dma_fence **fence)
+{
+	struct amdgpu_tlb_fence *f;
+
+	f = kmalloc(sizeof(*f), GFP_KERNEL);
+	if (!f) {
+		/*
+		 * We can't fail since the PDEs and PTEs are already updated, so
+		 * just block for the dependency and execute the TLB flush
+		 */
+		if (*fence)
+			dma_fence_wait(*fence, false);
+
+		amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
+		*fence = dma_fence_get_stub();
+		return;
+	}
+
+	f->adev = adev;
+	f->dependency = *fence;
+	f->pasid = vm->pasid;
+	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
+	spin_lock_init(&f->lock);
+
+	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
+		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
+
+	/* TODO: We probably need a separate wq here */
+	dma_fence_get(&f->base);
+	schedule_work(&f->work);
+
+	*fence = &f->base;
+}
-- 
2.43.2

[PATCH v5 2/2] drm/amdgpu: sync page table freeing with tlb flush

[Shashank Sharma]

The idea behind this patch is to delay the freeing of PT entry objects
until the TLB flush is done.

This patch:
- Adds a tlb_flush_waitlist in amdgpu_vm_update_params which will keep the
  objects that need to be freed after tlb_flush.
- Adds PT entries in this list in amdgpu_vm_ptes_update after finding
  the PT entry.
- Changes functionality of amdgpu_vm_pt_free_dfs from (df_search + free)
  to simply freeing of the BOs, also renames it to
  amdgpu_vm_pt_free_list to reflect this same.
- Exports function amdgpu_vm_pt_free_list to be called directly.
- Calls amdgpu_vm_pt_free_list directly from amdgpu_vm_update_range.

V2: rebase
V4: Addressed review comments from Christian
    - add only locked PTEs entries in TLB flush waitlist.
    - do not create a separate function for list flush.
    - do not create a new lock for TLB flush.
    - there is no need to wait on tlb_flush_fence exclusively.

V5: Addressed review comments from Christian
    - change the amdgpu_vm_pt_free_dfs's functionality to simple freeing
      of the objects and rename it.
    - add all the PTE objects in params->tlb_flush_waitlist
    - let amdgpu_vm_pt_free_root handle the freeing of BOs independently
    - call amdgpu_vm_pt_free_list directly

Cc: Christian König <Christian.Koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Felix Kuehling <felix.kuehling@amd.com>
Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    |  4 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h    |  7 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 53 +++++++++++++----------
 3 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 310aae6fb49b..b8a6e534cd81 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -905,6 +905,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	params.unlocked = unlocked;
 	params.needs_flush = flush_tlb;
 	params.allow_override = allow_override;
+	INIT_LIST_HEAD(&params.tlb_flush_waitlist);
 
 	/* Implicitly sync to command submissions in the same VM before
 	 * unmapping. Sync to moving fences before mapping.
@@ -997,6 +998,9 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 				   DMA_RESV_USAGE_BOOKKEEP);
 	}
 
+	if (params.needs_flush)
+		amdgpu_vm_pt_free_list(adev, &params);
+
 error_unlock:
 	amdgpu_vm_eviction_unlock(vm);
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 298f604b8e5f..b81ca460b210 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -265,6 +265,11 @@ struct amdgpu_vm_update_params {
 	 * to be overridden for NUMA local memory.
 	 */
 	bool allow_override;
+
+	/**
+	 * @tlb_flush_waitlist: temporary storage for BOs until tlb_flush
+	 */
+	struct list_head tlb_flush_waitlist;
 };
 
 struct amdgpu_vm_update_funcs {
@@ -545,6 +550,8 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
 			  uint64_t start, uint64_t end,
 			  uint64_t dst, uint64_t flags);
 void amdgpu_vm_pt_free_work(struct work_struct *work);
+void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
+			    struct amdgpu_vm_update_params *params);
 
 #if defined(CONFIG_DEBUG_FS)
 void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm *vm, struct seq_file *m);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index 95dc0afdaffb..d3afc9d34b71 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -679,40 +679,30 @@ void amdgpu_vm_pt_free_work(struct work_struct *work)
 }
 
 /**
- * amdgpu_vm_pt_free_dfs - free PD/PT levels
+ * amdgpu_vm_pt_free_list - free PD/PT levels
  *
  * @adev: amdgpu device structure
- * @vm: amdgpu vm structure
- * @start: optional cursor where to start freeing PDs/PTs
- * @unlocked: vm resv unlock status
+ * @params: see amdgpu_vm_update_params definition
  *
- * Free the page directory or page table level and all sub levels.
+ * Free the page directory objects saved in the flush list
  */
-static void amdgpu_vm_pt_free_dfs(struct amdgpu_device *adev,
-				  struct amdgpu_vm *vm,
-				  struct amdgpu_vm_pt_cursor *start,
-				  bool unlocked)
+void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
+			    struct amdgpu_vm_update_params *params)
 {
-	struct amdgpu_vm_pt_cursor cursor;
-	struct amdgpu_vm_bo_base *entry;
+	struct amdgpu_vm_bo_base *entry, *next;
+	struct amdgpu_vm *vm = params->vm;
+	bool unlocked = params->unlocked;
 
 	if (unlocked) {
 		spin_lock(&vm->status_lock);
-		for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
-			list_move(&entry->vm_status, &vm->pt_freed);
-
-		if (start)
-			list_move(&start->entry->vm_status, &vm->pt_freed);
+		list_splice_init(&vm->pt_freed, &params->tlb_flush_waitlist);
 		spin_unlock(&vm->status_lock);
 		schedule_work(&vm->pt_free_work);
 		return;
 	}
 
-	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
+	list_for_each_entry_safe(entry, next, &params->tlb_flush_waitlist, vm_status)
 		amdgpu_vm_pt_free(entry);
-
-	if (start)
-		amdgpu_vm_pt_free(start->entry);
 }
 
 /**
@@ -724,7 +714,11 @@ static void amdgpu_vm_pt_free_dfs(struct amdgpu_device *adev,
  */
 void amdgpu_vm_pt_free_root(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 {
-	amdgpu_vm_pt_free_dfs(adev, vm, NULL, false);
+	struct amdgpu_vm_pt_cursor cursor;
+	struct amdgpu_vm_bo_base *entry;
+
+	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, NULL, cursor, entry)
+		amdgpu_vm_pt_free(entry);
 }
 
 /**
@@ -1056,10 +1050,21 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
 			while (cursor.pfn < frag_start) {
 				/* Make sure previous mapping is freed */
 				if (cursor.entry->bo) {
+					struct amdgpu_vm_pt_cursor seek;
+					struct amdgpu_vm_bo_base *entry;
 					params->needs_flush = true;
-					amdgpu_vm_pt_free_dfs(adev, params->vm,
-							      &cursor,
-							      params->unlocked);
+
+					spin_lock(&params->vm->status_lock);
+					for_each_amdgpu_vm_pt_dfs_safe(adev, params->vm, &cursor,
+								       seek, entry) {
+						list_move(&entry->vm_status,
+							  &params->tlb_flush_waitlist);
+					}
+
+					/* enter start node now */
+					list_move(&cursor.entry->vm_status,
+						  &params->tlb_flush_waitlist);
+					spin_unlock(&params->vm->status_lock);
 				}
 				amdgpu_vm_pt_next(adev, &cursor);
 			}
-- 
2.43.2

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Felix Kuehling]

On 2024-03-06 09:41, Shashank Sharma wrote:
> From: Christian König <christian.koenig@amd.com>
>
> The problem is that when (for example) 4k pages are replaced
> with a single 2M page we need to wait for change to be flushed
> out by invalidating the TLB before the PT can be freed.
>
> Solve this by moving the TLB flush into a DMA-fence object which
> can be used to delay the freeing of the PT BOs until it is signaled.
>
> V2: (Shashank)
>      - rebase
>      - set dma_fence_error only in case of error
>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>      - use vm->pasid when f is NULL (Mukul)
>
> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>      - move the misplaced fence_create call to the end (Philip)
>
> V5: - free the f->dependency properly (Christian)
>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 ++++++++++++++++++
>   4 files changed, 128 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index fa26a4e3a99d..91ab4cf29b5b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o amdgpu_kms.o \
>   	amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>   	atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>   	atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
> -	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o amdgpu_pll.o \
> +	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_vm_tlb_fence.o \
> +	amdgpu_ib.o amdgpu_pll.o \
>   	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>   	amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o \
>   	amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 0960e0a665d3..310aae6fb49b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   	r = vm->update_funcs->commit(&params, fence);
>   
> +	/* Prepare a TLB flush fence to be attached to PTs */
> +	if (!unlocked && params.needs_flush && vm->is_compute_context) {
> +		amdgpu_vm_tlb_fence_create(adev, vm, fence);

This schedules a TLB flush after "fence" signals and replaces "fence" 
with a new one that will signal after the TLB flush is done. That part I 
understand.

I'm not sure why this only applies to compute contexts.


> +
> +		/* Makes sure no PD/PT is freed before the flush */
> +		dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
> +				   DMA_RESV_USAGE_BOOKKEEP);

But what's the point of adding the fence to the page table reservation? 
This is after the BOs have already been freed. Maybe it would make more 
sense to move this into the next patch, where the freeing is done after 
this point.

Regards,
   Felix


> +	}
> +
>   error_unlock:
>   	amdgpu_vm_eviction_unlock(vm);
>   	drm_dev_exit(idx);
> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   	mutex_init(&vm->eviction_lock);
>   	vm->evicting = false;
> +	vm->tlb_fence_context = dma_fence_context_alloc(1);
>   
>   	r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>   				false, &root, xcp_id);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 64b3f69efa57..298f604b8e5f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>   	atomic64_t		tlb_seq;
>   	uint64_t		tlb_seq_va;
>   	uint64_t		*tlb_seq_cpu_addr;
> +	uint64_t		tlb_fence_context;
>   
>   	atomic64_t		kfd_last_flushed_seq;
>   
> @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>   				  uint64_t addr,
>   				  uint32_t status,
>   				  unsigned int vmhub);
> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
> +				 struct amdgpu_vm *vm,
> +				 struct dma_fence **fence);
>   
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> new file mode 100644
> index 000000000000..51cddfa3f1e8
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/dma-fence.h>
> +#include <linux/workqueue.h>
> +
> +#include "amdgpu.h"
> +#include "amdgpu_vm.h"
> +#include "amdgpu_gmc.h"
> +
> +struct amdgpu_tlb_fence {
> +	struct dma_fence	base;
> +	struct amdgpu_device	*adev;
> +	struct dma_fence	*dependency;
> +	struct work_struct	work;
> +	spinlock_t		lock;
> +	uint16_t		pasid;
> +
> +};
> +
> +static const char *amdgpu_tlb_fence_get_driver_name(struct dma_fence *fence)
> +{
> +	return "amdgpu tlb fence";
> +}
> +
> +static const char *amdgpu_tlb_fence_get_timeline_name(struct dma_fence *f)
> +{
> +	return "amdgpu tlb timeline";
> +}
> +
> +static void amdgpu_tlb_fence_work(struct work_struct *work)
> +{
> +	struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
> +	int r;
> +
> +	if (f->dependency) {
> +		dma_fence_wait(f->dependency, false);
> +		dma_fence_put(f->dependency);
> +		f->dependency = NULL;
> +	}
> +
> +	r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
> +	if (r) {
> +		dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
> +			f->pasid);
> +		dma_fence_set_error(&f->base, r);
> +	}
> +
> +	dma_fence_signal(&f->base);
> +	dma_fence_put(&f->base);
> +}
> +
> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
> +	.use_64bit_seqno = true,
> +	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
> +	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
> +};
> +
> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +				struct dma_fence **fence)
> +{
> +	struct amdgpu_tlb_fence *f;
> +
> +	f = kmalloc(sizeof(*f), GFP_KERNEL);
> +	if (!f) {
> +		/*
> +		 * We can't fail since the PDEs and PTEs are already updated, so
> +		 * just block for the dependency and execute the TLB flush
> +		 */
> +		if (*fence)
> +			dma_fence_wait(*fence, false);
> +
> +		amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
> +		*fence = dma_fence_get_stub();
> +		return;
> +	}
> +
> +	f->adev = adev;
> +	f->dependency = *fence;
> +	f->pasid = vm->pasid;
> +	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
> +	spin_lock_init(&f->lock);
> +
> +	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
> +		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
> +
> +	/* TODO: We probably need a separate wq here */
> +	dma_fence_get(&f->base);
> +	schedule_work(&f->work);
> +
> +	*fence = &f->base;
> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Sharma, Shashank]

On 07/03/2024 00:54, Felix Kuehling wrote:
>
> On 2024-03-06 09:41, Shashank Sharma wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> The problem is that when (for example) 4k pages are replaced
>> with a single 2M page we need to wait for change to be flushed
>> out by invalidating the TLB before the PT can be freed.
>>
>> Solve this by moving the TLB flush into a DMA-fence object which
>> can be used to delay the freeing of the PT BOs until it is signaled.
>>
>> V2: (Shashank)
>>      - rebase
>>      - set dma_fence_error only in case of error
>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>      - use vm->pasid when f is NULL (Mukul)
>>
>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>      - move the misplaced fence_create call to the end (Philip)
>>
>> V5: - free the f->dependency properly (Christian)
>>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>> Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 ++++++++++++++++++
>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>> index fa26a4e3a99d..91ab4cf29b5b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o 
>> amdgpu_kms.o \
>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>> amdgpu_pll.o \
>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_vm_tlb_fence.o \
>> +    amdgpu_ib.o amdgpu_pll.o \
>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>> amdgpu_virt.o \
>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 0960e0a665d3..310aae6fb49b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm,
>>         r = vm->update_funcs->commit(&params, fence);
>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>
> This schedules a TLB flush after "fence" signals and replaces "fence" 
> with a new one that will signal after the TLB flush is done. That part 
> I understand.
>
> I'm not sure why this only applies to compute contexts.
>
>
>> +
>> +        /* Makes sure no PD/PT is freed before the flush */
>> +        dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>> +                   DMA_RESV_USAGE_BOOKKEEP);
>
> But what's the point of adding the fence to the page table 
> reservation? This is after the BOs have already been freed. Maybe it 
> would make more sense to move this into the next patch, where the 
> freeing is done after this point.

To make it easier for code review, the split of the patches is like:
- one patch introduces function creating tlb_flush_fence and uses it

- the second patch does the rework and movement of freeing of the buffer 
after the patch attach.

If we move this change into next patch, in this patch we will just 
create the fence, where one can argue why create the fence if no one is 
using it.

May be, we can make 'changes in freeing of buffers' as first patch in 
sequence, and make this second patch in the series, so that you know the 
background of changes better.

- Shashank

>
> Regards,
>   Felix
>
>
>> +    }
>> +
>>   error_unlock:
>>       amdgpu_vm_eviction_unlock(vm);
>>       drm_dev_exit(idx);
>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm,
>>         mutex_init(&vm->eviction_lock);
>>       vm->evicting = false;
>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>         r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>                   false, &root, xcp_id);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 64b3f69efa57..298f604b8e5f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>       atomic64_t        tlb_seq;
>>       uint64_t        tlb_seq_va;
>>       uint64_t        *tlb_seq_cpu_addr;
>> +    uint64_t        tlb_fence_context;
>>         atomic64_t        kfd_last_flushed_seq;
>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>> amdgpu_device *adev,
>>                     uint64_t addr,
>>                     uint32_t status,
>>                     unsigned int vmhub);
>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>> +                 struct amdgpu_vm *vm,
>> +                 struct dma_fence **fence);
>>     #endif
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> new file mode 100644
>> index 000000000000..51cddfa3f1e8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be 
>> included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>> DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>> OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>> USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <linux/dma-fence.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "amdgpu.h"
>> +#include "amdgpu_vm.h"
>> +#include "amdgpu_gmc.h"
>> +
>> +struct amdgpu_tlb_fence {
>> +    struct dma_fence    base;
>> +    struct amdgpu_device    *adev;
>> +    struct dma_fence    *dependency;
>> +    struct work_struct    work;
>> +    spinlock_t        lock;
>> +    uint16_t        pasid;
>> +
>> +};
>> +
>> +static const char *amdgpu_tlb_fence_get_driver_name(struct dma_fence 
>> *fence)
>> +{
>> +    return "amdgpu tlb fence";
>> +}
>> +
>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>> dma_fence *f)
>> +{
>> +    return "amdgpu tlb timeline";
>> +}
>> +
>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>> +{
>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
>> +    int r;
>> +
>> +    if (f->dependency) {
>> +        dma_fence_wait(f->dependency, false);
>> +        dma_fence_put(f->dependency);
>> +        f->dependency = NULL;
>> +    }
>> +
>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
>> +    if (r) {
>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>> +            f->pasid);
>> +        dma_fence_set_error(&f->base, r);
>> +    }
>> +
>> +    dma_fence_signal(&f->base);
>> +    dma_fence_put(&f->base);
>> +}
>> +
>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>> +    .use_64bit_seqno = true,
>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>> +};
>> +
>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct 
>> amdgpu_vm *vm,
>> +                struct dma_fence **fence)
>> +{
>> +    struct amdgpu_tlb_fence *f;
>> +
>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>> +    if (!f) {
>> +        /*
>> +         * We can't fail since the PDEs and PTEs are already 
>> updated, so
>> +         * just block for the dependency and execute the TLB flush
>> +         */
>> +        if (*fence)
>> +            dma_fence_wait(*fence, false);
>> +
>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>> +        *fence = dma_fence_get_stub();
>> +        return;
>> +    }
>> +
>> +    f->adev = adev;
>> +    f->dependency = *fence;
>> +    f->pasid = vm->pasid;
>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>> +    spin_lock_init(&f->lock);
>> +
>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>> +
>> +    /* TODO: We probably need a separate wq here */
>> +    dma_fence_get(&f->base);
>> +    schedule_work(&f->work);
>> +
>> +    *fence = &f->base;
>> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Philip Yang]

[-- Attachment #1: Type: text/html, Size: 9984 bytes --]

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Felix Kuehling]

On 2024-03-07 1:39, Sharma, Shashank wrote:
>
> On 07/03/2024 00:54, Felix Kuehling wrote:
>>
>> On 2024-03-06 09:41, Shashank Sharma wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> The problem is that when (for example) 4k pages are replaced
>>> with a single 2M page we need to wait for change to be flushed
>>> out by invalidating the TLB before the PT can be freed.
>>>
>>> Solve this by moving the TLB flush into a DMA-fence object which
>>> can be used to delay the freeing of the PT BOs until it is signaled.
>>>
>>> V2: (Shashank)
>>>      - rebase
>>>      - set dma_fence_error only in case of error
>>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>>      - use vm->pasid when f is NULL (Mukul)
>>>
>>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>>      - move the misplaced fence_create call to the end (Philip)
>>>
>>> V5: - free the f->dependency properly (Christian)
>>>
>>> Cc: Christian Koenig <christian.koenig@amd.com>
>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 
>>> ++++++++++++++++++
>>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> index fa26a4e3a99d..91ab4cf29b5b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o 
>>> amdgpu_kms.o \
>>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>>> amdgpu_pll.o \
>>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o 
>>> amdgpu_vm_tlb_fence.o \
>>> +    amdgpu_ib.o amdgpu_pll.o \
>>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>>> amdgpu_virt.o \
>>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 0960e0a665d3..310aae6fb49b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device 
>>> *adev, struct amdgpu_vm *vm,
>>>         r = vm->update_funcs->commit(&params, fence);
>>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>>
>> This schedules a TLB flush after "fence" signals and replaces "fence" 
>> with a new one that will signal after the TLB flush is done. That 
>> part I understand.
>>
>> I'm not sure why this only applies to compute contexts.
>>
>>
>>> +
>>> +        /* Makes sure no PD/PT is freed before the flush */
>>> +        dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>> +                   DMA_RESV_USAGE_BOOKKEEP);
>>
>> But what's the point of adding the fence to the page table 
>> reservation? This is after the BOs have already been freed. Maybe it 
>> would make more sense to move this into the next patch, where the 
>> freeing is done after this point.
>
> To make it easier for code review, the split of the patches is like:
> - one patch introduces function creating tlb_flush_fence and uses it
>
> - the second patch does the rework and movement of freeing of the 
> buffer after the patch attach.
>
> If we move this change into next patch, in this patch we will just 
> create the fence, where one can argue why create the fence if no one 
> is using it.
>
> May be, we can make 'changes in freeing of buffers' as first patch in 
> sequence, and make this second patch in the series, so that you know 
> the background of changes better.

Sure. I don't think it's super important. I was just trying to 
understand how the two patches fit together. I think it makes sense now. 
I discussed this also with Philip offline. We think there may be an 
easier way to solve the "wait for TLB flush before freeing BOs" thing, 
but I believe using the new TLB flush fence is architecturally cleaner, 
and that fence will be useful to solve some other issues that are either 
still lingering, or currently have only some ugly workarounds. I'll need 
to dig through the code and my memory to remember the details.

I'm still not sure whether the creation of the TLB flush fence should be 
limited to compute contexts, but I'm happy to get them at least there 
for now. The series is

Acked-by: Felix Kuehling <felix.kuehling@amd.com>

Regards,
   Felix


>
> - Shashank
>
>>
>> Regards,
>>   Felix
>>
>>
>>> +    }
>>> +
>>>   error_unlock:
>>>       amdgpu_vm_eviction_unlock(vm);
>>>       drm_dev_exit(idx);
>>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>>> struct amdgpu_vm *vm,
>>>         mutex_init(&vm->eviction_lock);
>>>       vm->evicting = false;
>>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>>         r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>>                   false, &root, xcp_id);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 64b3f69efa57..298f604b8e5f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>>       atomic64_t        tlb_seq;
>>>       uint64_t        tlb_seq_va;
>>>       uint64_t        *tlb_seq_cpu_addr;
>>> +    uint64_t        tlb_fence_context;
>>>         atomic64_t        kfd_last_flushed_seq;
>>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>>> amdgpu_device *adev,
>>>                     uint64_t addr,
>>>                     uint32_t status,
>>>                     unsigned int vmhub);
>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>>> +                 struct amdgpu_vm *vm,
>>> +                 struct dma_fence **fence);
>>>     #endif
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>> new file mode 100644
>>> index 000000000000..51cddfa3f1e8
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>> @@ -0,0 +1,112 @@
>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>> +/*
>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to 
>>> whom the
>>> + * Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be 
>>> included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>>> EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>>> DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>>> OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>> USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#include <linux/dma-fence.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#include "amdgpu.h"
>>> +#include "amdgpu_vm.h"
>>> +#include "amdgpu_gmc.h"
>>> +
>>> +struct amdgpu_tlb_fence {
>>> +    struct dma_fence    base;
>>> +    struct amdgpu_device    *adev;
>>> +    struct dma_fence    *dependency;
>>> +    struct work_struct    work;
>>> +    spinlock_t        lock;
>>> +    uint16_t        pasid;
>>> +
>>> +};
>>> +
>>> +static const char *amdgpu_tlb_fence_get_driver_name(struct 
>>> dma_fence *fence)
>>> +{
>>> +    return "amdgpu tlb fence";
>>> +}
>>> +
>>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>>> dma_fence *f)
>>> +{
>>> +    return "amdgpu tlb timeline";
>>> +}
>>> +
>>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>>> +{
>>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
>>> +    int r;
>>> +
>>> +    if (f->dependency) {
>>> +        dma_fence_wait(f->dependency, false);
>>> +        dma_fence_put(f->dependency);
>>> +        f->dependency = NULL;
>>> +    }
>>> +
>>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
>>> +    if (r) {
>>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>>> +            f->pasid);
>>> +        dma_fence_set_error(&f->base, r);
>>> +    }
>>> +
>>> +    dma_fence_signal(&f->base);
>>> +    dma_fence_put(&f->base);
>>> +}
>>> +
>>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>>> +    .use_64bit_seqno = true,
>>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>> +};
>>> +
>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct 
>>> amdgpu_vm *vm,
>>> +                struct dma_fence **fence)
>>> +{
>>> +    struct amdgpu_tlb_fence *f;
>>> +
>>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>>> +    if (!f) {
>>> +        /*
>>> +         * We can't fail since the PDEs and PTEs are already 
>>> updated, so
>>> +         * just block for the dependency and execute the TLB flush
>>> +         */
>>> +        if (*fence)
>>> +            dma_fence_wait(*fence, false);
>>> +
>>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>>> +        *fence = dma_fence_get_stub();
>>> +        return;
>>> +    }
>>> +
>>> +    f->adev = adev;
>>> +    f->dependency = *fence;
>>> +    f->pasid = vm->pasid;
>>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>> +    spin_lock_init(&f->lock);
>>> +
>>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>> +
>>> +    /* TODO: We probably need a separate wq here */
>>> +    dma_fence_get(&f->base);
>>> +    schedule_work(&f->work);
>>> +
>>> +    *fence = &f->base;
>>> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Christian König]

Am 07.03.24 um 23:44 schrieb Felix Kuehling:
> On 2024-03-07 1:39, Sharma, Shashank wrote:
>>
>> On 07/03/2024 00:54, Felix Kuehling wrote:
>>>
>>> On 2024-03-06 09:41, Shashank Sharma wrote:
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> The problem is that when (for example) 4k pages are replaced
>>>> with a single 2M page we need to wait for change to be flushed
>>>> out by invalidating the TLB before the PT can be freed.
>>>>
>>>> Solve this by moving the TLB flush into a DMA-fence object which
>>>> can be used to delay the freeing of the PT BOs until it is signaled.
>>>>
>>>> V2: (Shashank)
>>>>      - rebase
>>>>      - set dma_fence_error only in case of error
>>>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>>>      - use vm->pasid when f is NULL (Mukul)
>>>>
>>>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>>>      - move the misplaced fence_create call to the end (Philip)
>>>>
>>>> V5: - free the f->dependency properly (Christian)
>>>>
>>>> Cc: Christian Koenig <christian.koenig@amd.com>
>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 
>>>> ++++++++++++++++++
>>>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> index fa26a4e3a99d..91ab4cf29b5b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o 
>>>> amdgpu_kms.o \
>>>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>>>> amdgpu_pll.o \
>>>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o 
>>>> amdgpu_vm_tlb_fence.o \
>>>> +    amdgpu_ib.o amdgpu_pll.o \
>>>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>>>> amdgpu_virt.o \
>>>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 0960e0a665d3..310aae6fb49b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>         r = vm->update_funcs->commit(&params, fence);
>>>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>>>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>>>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>>>
>>> This schedules a TLB flush after "fence" signals and replaces 
>>> "fence" with a new one that will signal after the TLB flush is done. 
>>> That part I understand.
>>>
>>> I'm not sure why this only applies to compute contexts.
>>>
>>>
>>>> +
>>>> +        /* Makes sure no PD/PT is freed before the flush */
>>>> + dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>>> +                   DMA_RESV_USAGE_BOOKKEEP);
>>>
>>> But what's the point of adding the fence to the page table 
>>> reservation? This is after the BOs have already been freed. Maybe it 
>>> would make more sense to move this into the next patch, where the 
>>> freeing is done after this point.
>>
>> To make it easier for code review, the split of the patches is like:
>> - one patch introduces function creating tlb_flush_fence and uses it
>>
>> - the second patch does the rework and movement of freeing of the 
>> buffer after the patch attach.
>>
>> If we move this change into next patch, in this patch we will just 
>> create the fence, where one can argue why create the fence if no one 
>> is using it.
>>
>> May be, we can make 'changes in freeing of buffers' as first patch in 
>> sequence, and make this second patch in the series, so that you know 
>> the background of changes better.
>
> Sure. I don't think it's super important. I was just trying to 
> understand how the two patches fit together. I think it makes sense 
> now. I discussed this also with Philip offline. We think there may be 
> an easier way to solve the "wait for TLB flush before freeing BOs" 
> thing, but I believe using the new TLB flush fence is architecturally 
> cleaner, and that fence will be useful to solve some other issues that 
> are either still lingering, or currently have only some ugly 
> workarounds. I'll need to dig through the code and my memory to 
> remember the details.
>
> I'm still not sure whether the creation of the TLB flush fence should 
> be limited to compute contexts, but I'm happy to get them at least 
> there for now.

Shashank is working on this because we need the TLB flush fence for GFX 
user queues as well. So limiting it to compute VMs is just the first 
step to solve the KFD problem and MES will come later on.

> The series is
>
> Acked-by: Felix Kuehling <felix.kuehling@amd.com>

Thanks,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> - Shashank
>>
>>>
>>> Regards,
>>>   Felix
>>>
>>>
>>>> +    }
>>>> +
>>>>   error_unlock:
>>>>       amdgpu_vm_eviction_unlock(vm);
>>>>       drm_dev_exit(idx);
>>>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>> *adev, struct amdgpu_vm *vm,
>>>>         mutex_init(&vm->eviction_lock);
>>>>       vm->evicting = false;
>>>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>>>         r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>>>                   false, &root, xcp_id);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index 64b3f69efa57..298f604b8e5f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>>>       atomic64_t        tlb_seq;
>>>>       uint64_t        tlb_seq_va;
>>>>       uint64_t        *tlb_seq_cpu_addr;
>>>> +    uint64_t        tlb_fence_context;
>>>>         atomic64_t        kfd_last_flushed_seq;
>>>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>>>> amdgpu_device *adev,
>>>>                     uint64_t addr,
>>>>                     uint32_t status,
>>>>                     unsigned int vmhub);
>>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>>>> +                 struct amdgpu_vm *vm,
>>>> +                 struct dma_fence **fence);
>>>>     #endif
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>> new file mode 100644
>>>> index 000000000000..51cddfa3f1e8
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>> @@ -0,0 +1,112 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>>> +/*
>>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person 
>>>> obtaining a
>>>> + * copy of this software and associated documentation files (the 
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without 
>>>> limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>>> sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to 
>>>> whom the
>>>> + * Software is furnished to do so, subject to the following 
>>>> conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be 
>>>> included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO 
>>>> EVENT SHALL
>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>>>> DAMAGES OR
>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>>>> OTHERWISE,
>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>>> USE OR
>>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>>> + */
>>>> +
>>>> +#include <linux/dma-fence.h>
>>>> +#include <linux/workqueue.h>
>>>> +
>>>> +#include "amdgpu.h"
>>>> +#include "amdgpu_vm.h"
>>>> +#include "amdgpu_gmc.h"
>>>> +
>>>> +struct amdgpu_tlb_fence {
>>>> +    struct dma_fence    base;
>>>> +    struct amdgpu_device    *adev;
>>>> +    struct dma_fence    *dependency;
>>>> +    struct work_struct    work;
>>>> +    spinlock_t        lock;
>>>> +    uint16_t        pasid;
>>>> +
>>>> +};
>>>> +
>>>> +static const char *amdgpu_tlb_fence_get_driver_name(struct 
>>>> dma_fence *fence)
>>>> +{
>>>> +    return "amdgpu tlb fence";
>>>> +}
>>>> +
>>>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>>>> dma_fence *f)
>>>> +{
>>>> +    return "amdgpu tlb timeline";
>>>> +}
>>>> +
>>>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>>>> +{
>>>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), 
>>>> work);
>>>> +    int r;
>>>> +
>>>> +    if (f->dependency) {
>>>> +        dma_fence_wait(f->dependency, false);
>>>> +        dma_fence_put(f->dependency);
>>>> +        f->dependency = NULL;
>>>> +    }
>>>> +
>>>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 
>>>> 0);
>>>> +    if (r) {
>>>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>>>> +            f->pasid);
>>>> +        dma_fence_set_error(&f->base, r);
>>>> +    }
>>>> +
>>>> +    dma_fence_signal(&f->base);
>>>> +    dma_fence_put(&f->base);
>>>> +}
>>>> +
>>>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>>>> +    .use_64bit_seqno = true,
>>>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>>> +};
>>>> +
>>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct 
>>>> amdgpu_vm *vm,
>>>> +                struct dma_fence **fence)
>>>> +{
>>>> +    struct amdgpu_tlb_fence *f;
>>>> +
>>>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>>>> +    if (!f) {
>>>> +        /*
>>>> +         * We can't fail since the PDEs and PTEs are already 
>>>> updated, so
>>>> +         * just block for the dependency and execute the TLB flush
>>>> +         */
>>>> +        if (*fence)
>>>> +            dma_fence_wait(*fence, false);
>>>> +
>>>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>>>> +        *fence = dma_fence_get_stub();
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    f->adev = adev;
>>>> +    f->dependency = *fence;
>>>> +    f->pasid = vm->pasid;
>>>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>>> +    spin_lock_init(&f->lock);
>>>> +
>>>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>>>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>>> +
>>>> +    /* TODO: We probably need a separate wq here */
>>>> +    dma_fence_get(&f->base);
>>>> +    schedule_work(&f->work);
>>>> +
>>>> +    *fence = &f->base;
>>>> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Sharma, Shashank]

On 07/03/2024 20:22, Philip Yang wrote:
>
>
> On 2024-03-06 09:41, Shashank Sharma wrote:
>> From: Christian König<christian.koenig@amd.com>
>>
>> The problem is that when (for example) 4k pages are replaced
>> with a single 2M page we need to wait for change to be flushed
>> out by invalidating the TLB before the PT can be freed.
>>
>> Solve this by moving the TLB flush into a DMA-fence object which
>> can be used to delay the freeing of the PT BOs until it is signaled.
>>
>> V2: (Shashank)
>>      - rebase
>>      - set dma_fence_error only in case of error
>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>      - use vm->pasid when f is NULL (Mukul)
>>
>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>      - move the misplaced fence_create call to the end (Philip)
>>
>> V5: - free the f->dependency properly (Christian)
>>
>> Cc: Christian Koenig<christian.koenig@amd.com>
>> Cc: Felix Kuehling<Felix.Kuehling@amd.com>
>> Cc: Rajneesh Bhardwaj<rajneesh.bhardwaj@amd.com>
>> Cc: Alex Deucher<alexander.deucher@amd.com>
>> Reviewed-by: Shashank Sharma<shashank.sharma@amd.com>
>> Signed-off-by: Christian König<christian.koenig@amd.com>
>> Signed-off-by: Shashank Sharma<shashank.sharma@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 ++++++++++++++++++
>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
>> index fa26a4e3a99d..91ab4cf29b5b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o amdgpu_kms.o \
>>   	amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>   	atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>   	atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>> -	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o amdgpu_pll.o \
>> +	amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_vm_tlb_fence.o \
>> +	amdgpu_ib.o amdgpu_pll.o \
>>   	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>   	amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o \
>>   	amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 0960e0a665d3..310aae6fb49b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>   
>>   	r = vm->update_funcs->commit(&params, fence);
>>   
>> +	/* Prepare a TLB flush fence to be attached to PTs */
>> +	if (!unlocked && params.needs_flush && vm->is_compute_context) {
>> +		amdgpu_vm_tlb_fence_create(adev, vm, fence);
>> +
>> +		/* Makes sure no PD/PT is freed before the flush */
>> +		dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>> +				   DMA_RESV_USAGE_BOOKKEEP);
>> +	}
>> +
>>   error_unlock:
>>   	amdgpu_vm_eviction_unlock(vm);
>>   	drm_dev_exit(idx);
>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>   
>>   	mutex_init(&vm->eviction_lock);
>>   	vm->evicting = false;
>> +	vm->tlb_fence_context = dma_fence_context_alloc(1);
>>   
>>   	r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>   				false, &root, xcp_id);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 64b3f69efa57..298f604b8e5f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>   	atomic64_t		tlb_seq;
>>   	uint64_t		tlb_seq_va;
>>   	uint64_t		*tlb_seq_cpu_addr;
>> +	uint64_t		tlb_fence_context;
>>   
>>   	atomic64_t		kfd_last_flushed_seq;
>>   
>> @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>>   				  uint64_t addr,
>>   				  uint32_t status,
>>   				  unsigned int vmhub);
>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>> +				 struct amdgpu_vm *vm,
>> +				 struct dma_fence **fence);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> new file mode 100644
>> index 000000000000..51cddfa3f1e8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <linux/dma-fence.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "amdgpu.h"
>> +#include "amdgpu_vm.h"
>> +#include "amdgpu_gmc.h"
>> +
>> +struct amdgpu_tlb_fence {
>> +	struct dma_fence	base;
>> +	struct amdgpu_device	*adev;
>> +	struct dma_fence	*dependency;
>> +	struct work_struct	work;
>> +	spinlock_t		lock;
>> +	uint16_t		pasid;
>> +
>> +};
>> +
>> +static const char *amdgpu_tlb_fence_get_driver_name(struct dma_fence *fence)
>> +{
>> +	return "amdgpu tlb fence";
>> +}
>> +
>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct dma_fence *f)
>> +{
>> +	return "amdgpu tlb timeline";
>> +}
>> +
>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>> +{
>> +	struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
>> +	int r;
>> +
>> +	if (f->dependency) {
>> +		dma_fence_wait(f->dependency, false);
>> +		dma_fence_put(f->dependency);
>> +		f->dependency = NULL;
>> +	}
>> +
>> +	r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
>
> To flush all XCCs, as this is a corner case, we could start with this 
> to make it correct for SPX mode for now, with extra flush for other modes.
>
>     int num_xcc = f->adev->gfx.xcc_mask ? 
> NUM_XCC(f->adev->gfx.xcc_mask) : 1;
>     uint32_t xcc_mask = GENMASK(num_xcc - 1, 0);
>     int i;
>
>     for_each_inst(i, xcc_mask)
>          r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 
> TLB_FLUSH_LEGACY, true, i);

Thanks for this input, Philip.

@Christian, your feedback for this ?

- Shashank

>
> Regards,
> Philip
>> +	if (r) {
>> +		dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>> +			f->pasid);
>> +		dma_fence_set_error(&f->base, r);
>> +	}
>> +
>> +	dma_fence_signal(&f->base);
>> +	dma_fence_put(&f->base);
>> +}
>> +
>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>> +	.use_64bit_seqno = true,
>> +	.get_driver_name = amdgpu_tlb_fence_get_driver_name,
>> +	.get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>> +};
>> +
>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> +				struct dma_fence **fence)
>> +{
>> +	struct amdgpu_tlb_fence *f;
>> +
>> +	f = kmalloc(sizeof(*f), GFP_KERNEL);
>> +	if (!f) {
>> +		/*
>> +		 * We can't fail since the PDEs and PTEs are already updated, so
>> +		 * just block for the dependency and execute the TLB flush
>> +		 */
>> +		if (*fence)
>> +			dma_fence_wait(*fence, false);
>> +
>> +		amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>> +		*fence = dma_fence_get_stub();
>> +		return;
>> +	}
>> +
>> +	f->adev = adev;
>> +	f->dependency = *fence;
>> +	f->pasid = vm->pasid;
>> +	INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>> +	spin_lock_init(&f->lock);
>> +
>> +	dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>> +		       vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>> +
>> +	/* TODO: We probably need a separate wq here */
>> +	dma_fence_get(&f->base);
>> +	schedule_work(&f->work);
>> +
>> +	*fence = &f->base;
>> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Bhardwaj, Rajneesh]

Acked-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>

On 3/11/2024 10:37 AM, Sharma, Shashank wrote:
>
> On 07/03/2024 20:22, Philip Yang wrote:
>>
>>
>> On 2024-03-06 09:41, Shashank Sharma wrote:
>>> From: Christian König<christian.koenig@amd.com>
>>>
>>> The problem is that when (for example) 4k pages are replaced
>>> with a single 2M page we need to wait for change to be flushed
>>> out by invalidating the TLB before the PT can be freed.
>>>
>>> Solve this by moving the TLB flush into a DMA-fence object which
>>> can be used to delay the freeing of the PT BOs until it is signaled.
>>>
>>> V2: (Shashank)
>>>      - rebase
>>>      - set dma_fence_error only in case of error
>>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>>      - use vm->pasid when f is NULL (Mukul)
>>>
>>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>>      - move the misplaced fence_create call to the end (Philip)
>>>
>>> V5: - free the f->dependency properly (Christian)
>>>
>>> Cc: Christian Koenig<christian.koenig@amd.com>
>>> Cc: Felix Kuehling<Felix.Kuehling@amd.com>
>>> Cc: Rajneesh Bhardwaj<rajneesh.bhardwaj@amd.com>
>>> Cc: Alex Deucher<alexander.deucher@amd.com>
>>> Reviewed-by: Shashank Sharma<shashank.sharma@amd.com>
>>> Signed-off-by: Christian König<christian.koenig@amd.com>
>>> Signed-off-by: Shashank Sharma<shashank.sharma@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 
>>> ++++++++++++++++++
>>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> index fa26a4e3a99d..91ab4cf29b5b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o 
>>> amdgpu_kms.o \
>>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>>> amdgpu_pll.o \
>>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o 
>>> amdgpu_vm_tlb_fence.o \
>>> +    amdgpu_ib.o amdgpu_pll.o \
>>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>>> amdgpu_virt.o \
>>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 0960e0a665d3..310aae6fb49b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device 
>>> *adev, struct amdgpu_vm *vm,
>>>         r = vm->update_funcs->commit(&params, fence);
>>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>>> +
>>> +        /* Makes sure no PD/PT is freed before the flush */
>>> +        dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>> +                   DMA_RESV_USAGE_BOOKKEEP);
>>> +    }
>>> +
>>>   error_unlock:
>>>       amdgpu_vm_eviction_unlock(vm);
>>>       drm_dev_exit(idx);
>>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>>> struct amdgpu_vm *vm,
>>>         mutex_init(&vm->eviction_lock);
>>>       vm->evicting = false;
>>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>>         r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>>                   false, &root, xcp_id);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 64b3f69efa57..298f604b8e5f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>>       atomic64_t        tlb_seq;
>>>       uint64_t        tlb_seq_va;
>>>       uint64_t        *tlb_seq_cpu_addr;
>>> +    uint64_t        tlb_fence_context;
>>>         atomic64_t        kfd_last_flushed_seq;
>>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>>> amdgpu_device *adev,
>>>                     uint64_t addr,
>>>                     uint32_t status,
>>>                     unsigned int vmhub);
>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>>> +                 struct amdgpu_vm *vm,
>>> +                 struct dma_fence **fence);
>>>     #endif
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>> new file mode 100644
>>> index 000000000000..51cddfa3f1e8
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>> @@ -0,0 +1,112 @@
>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>> +/*
>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to 
>>> whom the
>>> + * Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be 
>>> included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>>> EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>>> DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>>> OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>> USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#include <linux/dma-fence.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#include "amdgpu.h"
>>> +#include "amdgpu_vm.h"
>>> +#include "amdgpu_gmc.h"
>>> +
>>> +struct amdgpu_tlb_fence {
>>> +    struct dma_fence    base;
>>> +    struct amdgpu_device    *adev;
>>> +    struct dma_fence    *dependency;
>>> +    struct work_struct    work;
>>> +    spinlock_t        lock;
>>> +    uint16_t        pasid;
>>> +
>>> +};
>>> +
>>> +static const char *amdgpu_tlb_fence_get_driver_name(struct 
>>> dma_fence *fence)
>>> +{
>>> +    return "amdgpu tlb fence";
>>> +}
>>> +
>>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>>> dma_fence *f)
>>> +{
>>> +    return "amdgpu tlb timeline";
>>> +}
>>> +
>>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>>> +{
>>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
>>> +    int r;
>>> +
>>> +    if (f->dependency) {
>>> +        dma_fence_wait(f->dependency, false);
>>> +        dma_fence_put(f->dependency);
>>> +        f->dependency = NULL;
>>> +    }
>>> +
>>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
>>
>> To flush all XCCs, as this is a corner case, we could start with this 
>> to make it correct for SPX mode for now, with extra flush for other 
>> modes.
>>
>>     int num_xcc = f->adev->gfx.xcc_mask ? 
>> NUM_XCC(f->adev->gfx.xcc_mask) : 1;
>>     uint32_t xcc_mask = GENMASK(num_xcc - 1, 0);
>>     int i;
>>
>>     for_each_inst(i, xcc_mask)
>>          r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 
>> TLB_FLUSH_LEGACY, true, i);
>
> Thanks for this input, Philip.
>
> @Christian, your feedback for this ?
>
> - Shashank
>
>>
>> Regards,
>> Philip
>>> +    if (r) {
>>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>>> +            f->pasid);
>>> +        dma_fence_set_error(&f->base, r);
>>> +    }
>>> +
>>> +    dma_fence_signal(&f->base);
>>> +    dma_fence_put(&f->base);
>>> +}
>>> +
>>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>>> +    .use_64bit_seqno = true,
>>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>> +};
>>> +
>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct 
>>> amdgpu_vm *vm,
>>> +                struct dma_fence **fence)
>>> +{
>>> +    struct amdgpu_tlb_fence *f;
>>> +
>>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>>> +    if (!f) {
>>> +        /*
>>> +         * We can't fail since the PDEs and PTEs are already 
>>> updated, so
>>> +         * just block for the dependency and execute the TLB flush
>>> +         */
>>> +        if (*fence)
>>> +            dma_fence_wait(*fence, false);
>>> +
>>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>>> +        *fence = dma_fence_get_stub();
>>> +        return;
>>> +    }
>>> +
>>> +    f->adev = adev;
>>> +    f->dependency = *fence;
>>> +    f->pasid = vm->pasid;
>>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>> +    spin_lock_init(&f->lock);
>>> +
>>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>> +
>>> +    /* TODO: We probably need a separate wq here */
>>> +    dma_fence_get(&f->base);
>>> +    schedule_work(&f->work);
>>> +
>>> +    *fence = &f->base;
>>> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Christian König]

Am 11.03.24 um 15:37 schrieb Sharma, Shashank:
>
> On 07/03/2024 20:22, Philip Yang wrote:
>>
>>
>> On 2024-03-06 09:41, Shashank Sharma wrote:
>>> From: Christian König<christian.koenig@amd.com>
>>>
>>> The problem is that when (for example) 4k pages are replaced
>>> with a single 2M page we need to wait for change to be flushed
>>> out by invalidating the TLB before the PT can be freed.
>>>
>>> Solve this by moving the TLB flush into a DMA-fence object which
>>> can be used to delay the freeing of the PT BOs until it is signaled.
>>>
>>> V2: (Shashank)
>>>      - rebase
>>>      - set dma_fence_error only in case of error
>>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>>      - use vm->pasid when f is NULL (Mukul)
>>>
>>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>>      - move the misplaced fence_create call to the end (Philip)
>>>
>>> V5: - free the f->dependency properly (Christian)
>>>
>>> Cc: Christian Koenig<christian.koenig@amd.com>
>>> Cc: Felix Kuehling<Felix.Kuehling@amd.com>
>>> Cc: Rajneesh Bhardwaj<rajneesh.bhardwaj@amd.com>
>>> Cc: Alex Deucher<alexander.deucher@amd.com>
>>> Reviewed-by: Shashank Sharma<shashank.sharma@amd.com>
>>> Signed-off-by: Christian König<christian.koenig@amd.com>
>>> Signed-off-by: Shashank Sharma<shashank.sharma@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 
>>> ++++++++++++++++++
>>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> index fa26a4e3a99d..91ab4cf29b5b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o 
>>> amdgpu_kms.o \
>>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>>> amdgpu_pll.o \
>>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o 
>>> amdgpu_vm_tlb_fence.o \
>>> +    amdgpu_ib.o amdgpu_pll.o \
>>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>>> amdgpu_virt.o \
>>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 0960e0a665d3..310aae6fb49b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct amdgpu_device 
>>> *adev, struct amdgpu_vm *vm,
>>>         r = vm->update_funcs->commit(&params, fence);
>>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>>> +
>>> +        /* Makes sure no PD/PT is freed before the flush */
>>> +        dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>> +                   DMA_RESV_USAGE_BOOKKEEP);
>>> +    }
>>> +
>>>   error_unlock:
>>>       amdgpu_vm_eviction_unlock(vm);
>>>       drm_dev_exit(idx);
>>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>>> struct amdgpu_vm *vm,
>>>         mutex_init(&vm->eviction_lock);
>>>       vm->evicting = false;
>>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>>         r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>>                   false, &root, xcp_id);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 64b3f69efa57..298f604b8e5f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>>       atomic64_t        tlb_seq;
>>>       uint64_t        tlb_seq_va;
>>>       uint64_t        *tlb_seq_cpu_addr;
>>> +    uint64_t        tlb_fence_context;
>>>         atomic64_t        kfd_last_flushed_seq;
>>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>>> amdgpu_device *adev,
>>>                     uint64_t addr,
>>>                     uint32_t status,
>>>                     unsigned int vmhub);
>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>>> +                 struct amdgpu_vm *vm,
>>> +                 struct dma_fence **fence);
>>>     #endif
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>> new file mode 100644
>>> index 000000000000..51cddfa3f1e8
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>> @@ -0,0 +1,112 @@
>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>> +/*
>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to 
>>> whom the
>>> + * Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be 
>>> included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>>> EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>>> DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>>> OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>> USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#include <linux/dma-fence.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#include "amdgpu.h"
>>> +#include "amdgpu_vm.h"
>>> +#include "amdgpu_gmc.h"
>>> +
>>> +struct amdgpu_tlb_fence {
>>> +    struct dma_fence    base;
>>> +    struct amdgpu_device    *adev;
>>> +    struct dma_fence    *dependency;
>>> +    struct work_struct    work;
>>> +    spinlock_t        lock;
>>> +    uint16_t        pasid;
>>> +
>>> +};
>>> +
>>> +static const char *amdgpu_tlb_fence_get_driver_name(struct 
>>> dma_fence *fence)
>>> +{
>>> +    return "amdgpu tlb fence";
>>> +}
>>> +
>>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>>> dma_fence *f)
>>> +{
>>> +    return "amdgpu tlb timeline";
>>> +}
>>> +
>>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>>> +{
>>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), work);
>>> +    int r;
>>> +
>>> +    if (f->dependency) {
>>> +        dma_fence_wait(f->dependency, false);
>>> +        dma_fence_put(f->dependency);
>>> +        f->dependency = NULL;
>>> +    }
>>> +
>>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 0);
>>
>> To flush all XCCs, as this is a corner case, we could start with this 
>> to make it correct for SPX mode for now, with extra flush for other 
>> modes.
>>
>>     int num_xcc = f->adev->gfx.xcc_mask ? 
>> NUM_XCC(f->adev->gfx.xcc_mask) : 1;
>>     uint32_t xcc_mask = GENMASK(num_xcc - 1, 0);
>>     int i;
>>
>>     for_each_inst(i, xcc_mask)
>>          r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 
>> TLB_FLUSH_LEGACY, true, i);
>
> Thanks for this input, Philip.
>
> @Christian, your feedback for this ?

IIRC Felix and I talked about that before. In theory each VM should only 
clear one XCC, but in practice that won't work.

I suggest to just make it another parameter and maybe separate fence 
allocation from actually arming it.

Christian.

>
> - Shashank
>
>>
>> Regards,
>> Philip
>>> +    if (r) {
>>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>>> +            f->pasid);
>>> +        dma_fence_set_error(&f->base, r);
>>> +    }
>>> +
>>> +    dma_fence_signal(&f->base);
>>> +    dma_fence_put(&f->base);
>>> +}
>>> +
>>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>>> +    .use_64bit_seqno = true,
>>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>> +};
>>> +
>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct 
>>> amdgpu_vm *vm,
>>> +                struct dma_fence **fence)
>>> +{
>>> +    struct amdgpu_tlb_fence *f;
>>> +
>>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>>> +    if (!f) {
>>> +        /*
>>> +         * We can't fail since the PDEs and PTEs are already 
>>> updated, so
>>> +         * just block for the dependency and execute the TLB flush
>>> +         */
>>> +        if (*fence)
>>> +            dma_fence_wait(*fence, false);
>>> +
>>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>>> +        *fence = dma_fence_get_stub();
>>> +        return;
>>> +    }
>>> +
>>> +    f->adev = adev;
>>> +    f->dependency = *fence;
>>> +    f->pasid = vm->pasid;
>>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>> +    spin_lock_init(&f->lock);
>>> +
>>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>> +
>>> +    /* TODO: We probably need a separate wq here */
>>> +    dma_fence_get(&f->base);
>>> +    schedule_work(&f->work);
>>> +
>>> +    *fence = &f->base;
>>> +}

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Sharma, Shashank]

On 12/03/2024 09:31, Christian König wrote:
> Am 11.03.24 um 15:37 schrieb Sharma, Shashank:
>>
>> On 07/03/2024 20:22, Philip Yang wrote:
>>>
>>>
>>> On 2024-03-06 09:41, Shashank Sharma wrote:
>>>> From: Christian König<christian.koenig@amd.com>
>>>>
>>>> The problem is that when (for example) 4k pages are replaced
>>>> with a single 2M page we need to wait for change to be flushed
>>>> out by invalidating the TLB before the PT can be freed.
>>>>
>>>> Solve this by moving the TLB flush into a DMA-fence object which
>>>> can be used to delay the freeing of the PT BOs until it is signaled.
>>>>
>>>> V2: (Shashank)
>>>>      - rebase
>>>>      - set dma_fence_error only in case of error
>>>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>>>      - use vm->pasid when f is NULL (Mukul)
>>>>
>>>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>>>      - move the misplaced fence_create call to the end (Philip)
>>>>
>>>> V5: - free the f->dependency properly (Christian)
>>>>
>>>> Cc: Christian Koenig<christian.koenig@amd.com>
>>>> Cc: Felix Kuehling<Felix.Kuehling@amd.com>
>>>> Cc: Rajneesh Bhardwaj<rajneesh.bhardwaj@amd.com>
>>>> Cc: Alex Deucher<alexander.deucher@amd.com>
>>>> Reviewed-by: Shashank Sharma<shashank.sharma@amd.com>
>>>> Signed-off-by: Christian König<christian.koenig@amd.com>
>>>> Signed-off-by: Shashank Sharma<shashank.sharma@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 
>>>> ++++++++++++++++++
>>>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> index fa26a4e3a99d..91ab4cf29b5b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o amdgpu_doorbell_mgr.o 
>>>> amdgpu_kms.o \
>>>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>>>> amdgpu_pll.o \
>>>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o 
>>>> amdgpu_vm_tlb_fence.o \
>>>> +    amdgpu_ib.o amdgpu_pll.o \
>>>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>>>> amdgpu_virt.o \
>>>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 0960e0a665d3..310aae6fb49b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>         r = vm->update_funcs->commit(&params, fence);
>>>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>>>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>>>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>>>> +
>>>> +        /* Makes sure no PD/PT is freed before the flush */
>>>> + dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>>> +                   DMA_RESV_USAGE_BOOKKEEP);
>>>> +    }
>>>> +
>>>>   error_unlock:
>>>>       amdgpu_vm_eviction_unlock(vm);
>>>>       drm_dev_exit(idx);
>>>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>> *adev, struct amdgpu_vm *vm,
>>>>         mutex_init(&vm->eviction_lock);
>>>>       vm->evicting = false;
>>>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>>>         r = amdgpu_vm_pt_create(adev, vm, adev->vm_manager.root_level,
>>>>                   false, &root, xcp_id);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index 64b3f69efa57..298f604b8e5f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>>>       atomic64_t        tlb_seq;
>>>>       uint64_t        tlb_seq_va;
>>>>       uint64_t        *tlb_seq_cpu_addr;
>>>> +    uint64_t        tlb_fence_context;
>>>>         atomic64_t        kfd_last_flushed_seq;
>>>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>>>> amdgpu_device *adev,
>>>>                     uint64_t addr,
>>>>                     uint32_t status,
>>>>                     unsigned int vmhub);
>>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>>>> +                 struct amdgpu_vm *vm,
>>>> +                 struct dma_fence **fence);
>>>>     #endif
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>> new file mode 100644
>>>> index 000000000000..51cddfa3f1e8
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>> @@ -0,0 +1,112 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>>> +/*
>>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person 
>>>> obtaining a
>>>> + * copy of this software and associated documentation files (the 
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without 
>>>> limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>>> sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to 
>>>> whom the
>>>> + * Software is furnished to do so, subject to the following 
>>>> conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be 
>>>> included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO 
>>>> EVENT SHALL
>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>>>> DAMAGES OR
>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>>>> OTHERWISE,
>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>>> USE OR
>>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>>> + */
>>>> +
>>>> +#include <linux/dma-fence.h>
>>>> +#include <linux/workqueue.h>
>>>> +
>>>> +#include "amdgpu.h"
>>>> +#include "amdgpu_vm.h"
>>>> +#include "amdgpu_gmc.h"
>>>> +
>>>> +struct amdgpu_tlb_fence {
>>>> +    struct dma_fence    base;
>>>> +    struct amdgpu_device    *adev;
>>>> +    struct dma_fence    *dependency;
>>>> +    struct work_struct    work;
>>>> +    spinlock_t        lock;
>>>> +    uint16_t        pasid;
>>>> +
>>>> +};
>>>> +
>>>> +static const char *amdgpu_tlb_fence_get_driver_name(struct 
>>>> dma_fence *fence)
>>>> +{
>>>> +    return "amdgpu tlb fence";
>>>> +}
>>>> +
>>>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>>>> dma_fence *f)
>>>> +{
>>>> +    return "amdgpu tlb timeline";
>>>> +}
>>>> +
>>>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>>>> +{
>>>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), 
>>>> work);
>>>> +    int r;
>>>> +
>>>> +    if (f->dependency) {
>>>> +        dma_fence_wait(f->dependency, false);
>>>> +        dma_fence_put(f->dependency);
>>>> +        f->dependency = NULL;
>>>> +    }
>>>> +
>>>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, true, 
>>>> 0);
>>>
>>> To flush all XCCs, as this is a corner case, we could start with 
>>> this to make it correct for SPX mode for now, with extra flush for 
>>> other modes.
>>>
>>>     int num_xcc = f->adev->gfx.xcc_mask ? 
>>> NUM_XCC(f->adev->gfx.xcc_mask) : 1;
>>>     uint32_t xcc_mask = GENMASK(num_xcc - 1, 0);
>>>     int i;
>>>
>>>     for_each_inst(i, xcc_mask)
>>>          r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 
>>> TLB_FLUSH_LEGACY, true, i);
>>
>> Thanks for this input, Philip.
>>
>> @Christian, your feedback for this ?
>
> IIRC Felix and I talked about that before. In theory each VM should 
> only clear one XCC, but in practice that won't work.
>
> I suggest to just make it another parameter and maybe separate fence 
> allocation from actually arming it.


Based on the recent MI300 validation feedback, it seems the current 
patch is good enough to handle the problem. Should we go ahead and merge 
the current series now and keep this for a follow-up patch ?

- Shashank

>
> Christian.
>
>>
>> - Shashank
>>
>>>
>>> Regards,
>>> Philip
>>>> +    if (r) {
>>>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>>>> +            f->pasid);
>>>> +        dma_fence_set_error(&f->base, r);
>>>> +    }
>>>> +
>>>> +    dma_fence_signal(&f->base);
>>>> +    dma_fence_put(&f->base);
>>>> +}
>>>> +
>>>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>>>> +    .use_64bit_seqno = true,
>>>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>>> +};
>>>> +
>>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct 
>>>> amdgpu_vm *vm,
>>>> +                struct dma_fence **fence)
>>>> +{
>>>> +    struct amdgpu_tlb_fence *f;
>>>> +
>>>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>>>> +    if (!f) {
>>>> +        /*
>>>> +         * We can't fail since the PDEs and PTEs are already 
>>>> updated, so
>>>> +         * just block for the dependency and execute the TLB flush
>>>> +         */
>>>> +        if (*fence)
>>>> +            dma_fence_wait(*fence, false);
>>>> +
>>>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>>>> +        *fence = dma_fence_get_stub();
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    f->adev = adev;
>>>> +    f->dependency = *fence;
>>>> +    f->pasid = vm->pasid;
>>>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>>> +    spin_lock_init(&f->lock);
>>>> +
>>>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>>>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>>> +
>>>> +    /* TODO: We probably need a separate wq here */
>>>> +    dma_fence_get(&f->base);
>>>> +    schedule_work(&f->work);
>>>> +
>>>> +    *fence = &f->base;
>>>> +}
>

Re: [PATCH v5 1/2] drm/amdgpu: implement TLB flush fence

[Christian König]

Am 12.03.24 um 10:35 schrieb Sharma, Shashank:
>
> On 12/03/2024 09:31, Christian König wrote:
>> Am 11.03.24 um 15:37 schrieb Sharma, Shashank:
>>>
>>> On 07/03/2024 20:22, Philip Yang wrote:
>>>>
>>>>
>>>> On 2024-03-06 09:41, Shashank Sharma wrote:
>>>>> From: Christian König<christian.koenig@amd.com>
>>>>>
>>>>> The problem is that when (for example) 4k pages are replaced
>>>>> with a single 2M page we need to wait for change to be flushed
>>>>> out by invalidating the TLB before the PT can be freed.
>>>>>
>>>>> Solve this by moving the TLB flush into a DMA-fence object which
>>>>> can be used to delay the freeing of the PT BOs until it is signaled.
>>>>>
>>>>> V2: (Shashank)
>>>>>      - rebase
>>>>>      - set dma_fence_error only in case of error
>>>>>      - add tlb_flush fence only when PT/PD BO is locked (Felix)
>>>>>      - use vm->pasid when f is NULL (Mukul)
>>>>>
>>>>> V4: - add a wait for (f->dependency) in tlb_fence_work (Christian)
>>>>>      - move the misplaced fence_create call to the end (Philip)
>>>>>
>>>>> V5: - free the f->dependency properly (Christian)
>>>>>
>>>>> Cc: Christian Koenig<christian.koenig@amd.com>
>>>>> Cc: Felix Kuehling<Felix.Kuehling@amd.com>
>>>>> Cc: Rajneesh Bhardwaj<rajneesh.bhardwaj@amd.com>
>>>>> Cc: Alex Deucher<alexander.deucher@amd.com>
>>>>> Reviewed-by: Shashank Sharma<shashank.sharma@amd.com>
>>>>> Signed-off-by: Christian König<christian.koenig@amd.com>
>>>>> Signed-off-by: Shashank Sharma<shashank.sharma@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/Makefile           |   3 +-
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  10 ++
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   4 +
>>>>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c  | 112 
>>>>> ++++++++++++++++++
>>>>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
>>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>> index fa26a4e3a99d..91ab4cf29b5b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>> @@ -70,7 +70,8 @@ amdgpu-y += amdgpu_device.o 
>>>>> amdgpu_doorbell_mgr.o amdgpu_kms.o \
>>>>>       amdgpu_cs.o amdgpu_bios.o amdgpu_benchmark.o \
>>>>>       atombios_dp.o amdgpu_afmt.o amdgpu_trace_points.o \
>>>>>       atombios_encoders.o amdgpu_sa.o atombios_i2c.o \
>>>>> -    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o amdgpu_ib.o 
>>>>> amdgpu_pll.o \
>>>>> +    amdgpu_dma_buf.o amdgpu_vm.o amdgpu_vm_pt.o 
>>>>> amdgpu_vm_tlb_fence.o \
>>>>> +    amdgpu_ib.o amdgpu_pll.o \
>>>>>       amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
>>>>>       amdgpu_gtt_mgr.o amdgpu_preempt_mgr.o amdgpu_vram_mgr.o 
>>>>> amdgpu_virt.o \
>>>>>       amdgpu_atomfirmware.o amdgpu_vf_error.o amdgpu_sched.o \
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 0960e0a665d3..310aae6fb49b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -988,6 +988,15 @@ int amdgpu_vm_update_range(struct 
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>         r = vm->update_funcs->commit(&params, fence);
>>>>>   +    /* Prepare a TLB flush fence to be attached to PTs */
>>>>> +    if (!unlocked && params.needs_flush && vm->is_compute_context) {
>>>>> +        amdgpu_vm_tlb_fence_create(adev, vm, fence);
>>>>> +
>>>>> +        /* Makes sure no PD/PT is freed before the flush */
>>>>> + dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>>>> +                   DMA_RESV_USAGE_BOOKKEEP);
>>>>> +    }
>>>>> +
>>>>>   error_unlock:
>>>>>       amdgpu_vm_eviction_unlock(vm);
>>>>>       drm_dev_exit(idx);
>>>>> @@ -2237,6 +2246,7 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>>> *adev, struct amdgpu_vm *vm,
>>>>>         mutex_init(&vm->eviction_lock);
>>>>>       vm->evicting = false;
>>>>> +    vm->tlb_fence_context = dma_fence_context_alloc(1);
>>>>>         r = amdgpu_vm_pt_create(adev, vm, 
>>>>> adev->vm_manager.root_level,
>>>>>                   false, &root, xcp_id);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index 64b3f69efa57..298f604b8e5f 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -341,6 +341,7 @@ struct amdgpu_vm {
>>>>>       atomic64_t        tlb_seq;
>>>>>       uint64_t        tlb_seq_va;
>>>>>       uint64_t        *tlb_seq_cpu_addr;
>>>>> +    uint64_t        tlb_fence_context;
>>>>>         atomic64_t        kfd_last_flushed_seq;
>>>>>   @@ -594,5 +595,8 @@ void amdgpu_vm_update_fault_cache(struct 
>>>>> amdgpu_device *adev,
>>>>>                     uint64_t addr,
>>>>>                     uint32_t status,
>>>>>                     unsigned int vmhub);
>>>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
>>>>> +                 struct amdgpu_vm *vm,
>>>>> +                 struct dma_fence **fence);
>>>>>     #endif
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>>> new file mode 100644
>>>>> index 000000000000..51cddfa3f1e8
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
>>>>> @@ -0,0 +1,112 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>>>> +/*
>>>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>>>> + *
>>>>> + * Permission is hereby granted, free of charge, to any person 
>>>>> obtaining a
>>>>> + * copy of this software and associated documentation files (the 
>>>>> "Software"),
>>>>> + * to deal in the Software without restriction, including without 
>>>>> limitation
>>>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>>>> sublicense,
>>>>> + * and/or sell copies of the Software, and to permit persons to 
>>>>> whom the
>>>>> + * Software is furnished to do so, subject to the following 
>>>>> conditions:
>>>>> + *
>>>>> + * The above copyright notice and this permission notice shall be 
>>>>> included in
>>>>> + * all copies or substantial portions of the Software.
>>>>> + *
>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY 
>>>>> KIND, EXPRESS OR
>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>>>> MERCHANTABILITY,
>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO 
>>>>> EVENT SHALL
>>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>>>>> DAMAGES OR
>>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
>>>>> OTHERWISE,
>>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>>>> USE OR
>>>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>>>> + */
>>>>> +
>>>>> +#include <linux/dma-fence.h>
>>>>> +#include <linux/workqueue.h>
>>>>> +
>>>>> +#include "amdgpu.h"
>>>>> +#include "amdgpu_vm.h"
>>>>> +#include "amdgpu_gmc.h"
>>>>> +
>>>>> +struct amdgpu_tlb_fence {
>>>>> +    struct dma_fence    base;
>>>>> +    struct amdgpu_device    *adev;
>>>>> +    struct dma_fence    *dependency;
>>>>> +    struct work_struct    work;
>>>>> +    spinlock_t        lock;
>>>>> +    uint16_t        pasid;
>>>>> +
>>>>> +};
>>>>> +
>>>>> +static const char *amdgpu_tlb_fence_get_driver_name(struct 
>>>>> dma_fence *fence)
>>>>> +{
>>>>> +    return "amdgpu tlb fence";
>>>>> +}
>>>>> +
>>>>> +static const char *amdgpu_tlb_fence_get_timeline_name(struct 
>>>>> dma_fence *f)
>>>>> +{
>>>>> +    return "amdgpu tlb timeline";
>>>>> +}
>>>>> +
>>>>> +static void amdgpu_tlb_fence_work(struct work_struct *work)
>>>>> +{
>>>>> +    struct amdgpu_tlb_fence *f = container_of(work, typeof(*f), 
>>>>> work);
>>>>> +    int r;
>>>>> +
>>>>> +    if (f->dependency) {
>>>>> +        dma_fence_wait(f->dependency, false);
>>>>> +        dma_fence_put(f->dependency);
>>>>> +        f->dependency = NULL;
>>>>> +    }
>>>>> +
>>>>> +    r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 2, 
>>>>> true, 0);
>>>>
>>>> To flush all XCCs, as this is a corner case, we could start with 
>>>> this to make it correct for SPX mode for now, with extra flush for 
>>>> other modes.
>>>>
>>>>     int num_xcc = f->adev->gfx.xcc_mask ? 
>>>> NUM_XCC(f->adev->gfx.xcc_mask) : 1;
>>>>     uint32_t xcc_mask = GENMASK(num_xcc - 1, 0);
>>>>     int i;
>>>>
>>>>     for_each_inst(i, xcc_mask)
>>>>          r = amdgpu_gmc_flush_gpu_tlb_pasid(f->adev, f->pasid, 
>>>> TLB_FLUSH_LEGACY, true, i);
>>>
>>> Thanks for this input, Philip.
>>>
>>> @Christian, your feedback for this ?
>>
>> IIRC Felix and I talked about that before. In theory each VM should 
>> only clear one XCC, but in practice that won't work.
>>
>> I suggest to just make it another parameter and maybe separate fence 
>> allocation from actually arming it.
>
>
> Based on the recent MI300 validation feedback, it seems the current 
> patch is good enough to handle the problem. Should we go ahead and 
> merge the current series now and keep this for a follow-up patch ?

Yeah, works for me.

Regards,
Christian.

>
> - Shashank
>
>>
>> Christian.
>>
>>>
>>> - Shashank
>>>
>>>>
>>>> Regards,
>>>> Philip
>>>>> +    if (r) {
>>>>> +        dev_err(f->adev->dev, "TLB flush failed for PASID %d.\n",
>>>>> +            f->pasid);
>>>>> +        dma_fence_set_error(&f->base, r);
>>>>> +    }
>>>>> +
>>>>> +    dma_fence_signal(&f->base);
>>>>> +    dma_fence_put(&f->base);
>>>>> +}
>>>>> +
>>>>> +static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
>>>>> +    .use_64bit_seqno = true,
>>>>> +    .get_driver_name = amdgpu_tlb_fence_get_driver_name,
>>>>> +    .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
>>>>> +};
>>>>> +
>>>>> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, 
>>>>> struct amdgpu_vm *vm,
>>>>> +                struct dma_fence **fence)
>>>>> +{
>>>>> +    struct amdgpu_tlb_fence *f;
>>>>> +
>>>>> +    f = kmalloc(sizeof(*f), GFP_KERNEL);
>>>>> +    if (!f) {
>>>>> +        /*
>>>>> +         * We can't fail since the PDEs and PTEs are already 
>>>>> updated, so
>>>>> +         * just block for the dependency and execute the TLB flush
>>>>> +         */
>>>>> +        if (*fence)
>>>>> +            dma_fence_wait(*fence, false);
>>>>> +
>>>>> +        amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
>>>>> +        *fence = dma_fence_get_stub();
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    f->adev = adev;
>>>>> +    f->dependency = *fence;
>>>>> +    f->pasid = vm->pasid;
>>>>> +    INIT_WORK(&f->work, amdgpu_tlb_fence_work);
>>>>> +    spin_lock_init(&f->lock);
>>>>> +
>>>>> +    dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock,
>>>>> +               vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
>>>>> +
>>>>> +    /* TODO: We probably need a separate wq here */
>>>>> +    dma_fence_get(&f->base);
>>>>> +    schedule_work(&f->work);
>>>>> +
>>>>> +    *fence = &f->base;
>>>>> +}
>>