All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/8] drm/amdgpu: remove some unused VM defines
@ 2019-03-19 12:44 Christian König
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

Not needed any more.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index f5c25c0ae367..8348804c46cd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -165,11 +165,6 @@ struct amdgpu_vm_pte_funcs {
 			    uint32_t incr, uint64_t flags);
 };
 
-#define AMDGPU_VM_FAULT(pasid, addr) (((u64)(pasid) << 48) | (addr))
-#define AMDGPU_VM_FAULT_PASID(fault) ((u64)(fault) >> 48)
-#define AMDGPU_VM_FAULT_ADDR(fault)  ((u64)(fault) & 0xfffffffff000ULL)
-
-
 struct amdgpu_task_info {
 	char	process_name[TASK_COMM_LEN];
 	char	task_name[TASK_COMM_LEN];
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/8] drm/amdgpu: always set and check dma addresses in the VM code
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
@ 2019-03-19 12:44   ` Christian König
  2019-03-19 12:44   ` [PATCH 3/8] drm/amdgpu: move and rename amdgpu_pte_update_params Christian König
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

Clean that up a bit and allow to always have the DMA addresses around.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c8f0e4ca05fb..2d61d74d32a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -84,6 +84,13 @@ struct amdgpu_pte_update_params {
 	 */
 	struct amdgpu_vm *vm;
 
+	/**
+	 * @pages_addr:
+	 *
+	 * DMA addresses to use for mapping
+	 */
+	dma_addr_t *pages_addr;
+
 	/**
 	 * @src: address where to copy page table entries from
 	 */
@@ -101,12 +108,6 @@ struct amdgpu_pte_update_params {
 		     struct amdgpu_bo *bo, uint64_t pe,
 		     uint64_t addr, unsigned count, uint32_t incr,
 		     uint64_t flags);
-	/**
-	 * @pages_addr:
-	 *
-	 * DMA addresses to use for mapping, used during VM update by CPU
-	 */
-	dma_addr_t *pages_addr;
 };
 
 /**
@@ -1573,7 +1574,7 @@ static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
 		max_frag = 31;
 
 	/* system pages are non continuously */
-	if (params->src) {
+	if (params->pages_addr) {
 		*frag = 0;
 		*frag_end = end;
 		return;
@@ -1753,16 +1754,13 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
 	params.vm = vm;
+	params.pages_addr = pages_addr;
 
 	/* sync to everything except eviction fences on unmapping */
 	if (!(flags & AMDGPU_PTE_VALID))
 		owner = AMDGPU_FENCE_OWNER_KFD;
 
 	if (vm->use_cpu_for_update) {
-		/* params.src is used as flag to indicate system Memory */
-		if (pages_addr)
-			params.src = ~0;
-
 		/* Wait for PT BOs to be idle. PTs share the same resv. object
 		 * as the root PD BO
 		 */
@@ -1778,7 +1776,6 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 		}
 
 		params.func = amdgpu_vm_cpu_set_ptes;
-		params.pages_addr = pages_addr;
 		return amdgpu_vm_update_ptes(&params, start, last + 1,
 					     addr, flags);
 	}
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/8] drm/amdgpu: move and rename amdgpu_pte_update_params
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
  2019-03-19 12:44   ` [PATCH 2/8] drm/amdgpu: always set and check dma addresses in the VM code Christian König
@ 2019-03-19 12:44   ` Christian König
  2019-03-19 12:44   ` [PATCH 4/8] drm/amdgpu: reserve less memory for PDE updates Christian König
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

Move the update parameter into the VM header and rename them.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 ++++++--------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 45 ++++++++++++++++
 2 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 2d61d74d32a8..560658406e8b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -65,51 +65,6 @@ INTERVAL_TREE_DEFINE(struct amdgpu_bo_va_mapping, rb, uint64_t, __subtree_last,
 #undef START
 #undef LAST
 
-/**
- * struct amdgpu_pte_update_params - Local structure
- *
- * Encapsulate some VM table update parameters to reduce
- * the number of function parameters
- *
- */
-struct amdgpu_pte_update_params {
-
-	/**
-	 * @adev: amdgpu device we do this update for
-	 */
-	struct amdgpu_device *adev;
-
-	/**
-	 * @vm: optional amdgpu_vm we do this update for
-	 */
-	struct amdgpu_vm *vm;
-
-	/**
-	 * @pages_addr:
-	 *
-	 * DMA addresses to use for mapping
-	 */
-	dma_addr_t *pages_addr;
-
-	/**
-	 * @src: address where to copy page table entries from
-	 */
-	uint64_t src;
-
-	/**
-	 * @ib: indirect buffer to fill with commands
-	 */
-	struct amdgpu_ib *ib;
-
-	/**
-	 * @func: Function which actually does the update
-	 */
-	void (*func)(struct amdgpu_pte_update_params *params,
-		     struct amdgpu_bo *bo, uint64_t pe,
-		     uint64_t addr, unsigned count, uint32_t incr,
-		     uint64_t flags);
-};
-
 /**
  * struct amdgpu_prt_cb - Helper to disable partial resident texture feature from a fence callback
  */
@@ -1219,7 +1174,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 /**
  * amdgpu_vm_do_set_ptes - helper to call the right asic function
  *
- * @params: see amdgpu_pte_update_params definition
+ * @params: see amdgpu_vm_update_params definition
  * @bo: PD/PT to update
  * @pe: addr of the page entry
  * @addr: dst addr to write into pe
@@ -1230,7 +1185,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
  * Traces the parameters and calls the right asic functions
  * to setup the page table using the DMA.
  */
-static void amdgpu_vm_do_set_ptes(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_do_set_ptes(struct amdgpu_vm_update_params *params,
 				  struct amdgpu_bo *bo,
 				  uint64_t pe, uint64_t addr,
 				  unsigned count, uint32_t incr,
@@ -1252,7 +1207,7 @@ static void amdgpu_vm_do_set_ptes(struct amdgpu_pte_update_params *params,
 /**
  * amdgpu_vm_do_copy_ptes - copy the PTEs from the GART
  *
- * @params: see amdgpu_pte_update_params definition
+ * @params: see amdgpu_vm_update_params definition
  * @bo: PD/PT to update
  * @pe: addr of the page entry
  * @addr: dst addr to write into pe
@@ -1262,7 +1217,7 @@ static void amdgpu_vm_do_set_ptes(struct amdgpu_pte_update_params *params,
  *
  * Traces the parameters and calls the DMA function to copy the PTEs.
  */
-static void amdgpu_vm_do_copy_ptes(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_do_copy_ptes(struct amdgpu_vm_update_params *params,
 				   struct amdgpu_bo *bo,
 				   uint64_t pe, uint64_t addr,
 				   unsigned count, uint32_t incr,
@@ -1306,7 +1261,7 @@ static uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr)
 /**
  * amdgpu_vm_cpu_set_ptes - helper to update page tables via CPU
  *
- * @params: see amdgpu_pte_update_params definition
+ * @params: see amdgpu_vm_update_params definition
  * @bo: PD/PT to update
  * @pe: kmap addr of the page entry
  * @addr: dst addr to write into pe
@@ -1316,7 +1271,7 @@ static uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr)
  *
  * Write count number of PT/PD entries directly.
  */
-static void amdgpu_vm_cpu_set_ptes(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_cpu_set_ptes(struct amdgpu_vm_update_params *params,
 				   struct amdgpu_bo *bo,
 				   uint64_t pe, uint64_t addr,
 				   unsigned count, uint32_t incr,
@@ -1344,7 +1299,7 @@ static void amdgpu_vm_cpu_set_ptes(struct amdgpu_pte_update_params *params,
  *
  * Calls the update function for both the given BO as well as its shadow.
  */
-static void amdgpu_vm_update_func(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_update_func(struct amdgpu_vm_update_params *params,
 				  struct amdgpu_bo *bo,
 				  uint64_t pe, uint64_t addr,
 				  unsigned count, uint32_t incr,
@@ -1365,7 +1320,7 @@ static void amdgpu_vm_update_func(struct amdgpu_pte_update_params *params,
  *
  * Makes sure the requested entry in parent is up to date.
  */
-static void amdgpu_vm_update_pde(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_update_pde(struct amdgpu_vm_update_params *params,
 				 struct amdgpu_vm *vm,
 				 struct amdgpu_vm_pt *parent,
 				 struct amdgpu_vm_pt *entry)
@@ -1416,7 +1371,7 @@ static void amdgpu_vm_invalidate_pds(struct amdgpu_device *adev,
 int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 				 struct amdgpu_vm *vm)
 {
-	struct amdgpu_pte_update_params params;
+	struct amdgpu_vm_update_params params;
 	struct amdgpu_job *job;
 	unsigned ndw = 0;
 	int r = 0;
@@ -1507,7 +1462,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
  *
  * Make sure to set the right flags for the PTEs at the desired level.
  */
-static void amdgpu_vm_update_flags(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_update_flags(struct amdgpu_vm_update_params *params,
 				   struct amdgpu_bo *bo, unsigned level,
 				   uint64_t pe, uint64_t addr,
 				   unsigned count, uint32_t incr,
@@ -1532,7 +1487,7 @@ static void amdgpu_vm_update_flags(struct amdgpu_pte_update_params *params,
 /**
  * amdgpu_vm_fragment - get fragment for PTEs
  *
- * @params: see amdgpu_pte_update_params definition
+ * @params: see amdgpu_vm_update_params definition
  * @start: first PTE to handle
  * @end: last PTE to handle
  * @flags: hw mapping flags
@@ -1541,7 +1496,7 @@ static void amdgpu_vm_update_flags(struct amdgpu_pte_update_params *params,
  *
  * Returns the first possible fragment for the start and end address.
  */
-static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
+static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
 			       uint64_t start, uint64_t end, uint64_t flags,
 			       unsigned int *frag, uint64_t *frag_end)
 {
@@ -1593,7 +1548,7 @@ static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
 /**
  * amdgpu_vm_update_ptes - make sure that page tables are valid
  *
- * @params: see amdgpu_pte_update_params definition
+ * @params: see amdgpu_vm_update_params definition
  * @start: start of GPU address range
  * @end: end of GPU address range
  * @dst: destination address to map to, the next dst inside the function
@@ -1604,7 +1559,7 @@ static void amdgpu_vm_fragment(struct amdgpu_pte_update_params *params,
  * Returns:
  * 0 for success, -EINVAL for failure.
  */
-static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
+static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 				 uint64_t start, uint64_t end,
 				 uint64_t dst, uint64_t flags)
 {
@@ -1747,7 +1702,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	void *owner = AMDGPU_FENCE_OWNER_VM;
 	unsigned nptes, ncmds, ndw;
 	struct amdgpu_job *job;
-	struct amdgpu_pte_update_params params;
+	struct amdgpu_vm_update_params params;
 	struct dma_fence *f = NULL;
 	int r;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 8348804c46cd..6df4d9e382ac 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -172,6 +172,51 @@ struct amdgpu_task_info {
 	pid_t	tgid;
 };
 
+/**
+ * struct amdgpu_vm_update_params
+ *
+ * Encapsulate some VM table update parameters to reduce
+ * the number of function parameters
+ *
+ */
+struct amdgpu_vm_update_params {
+
+	/**
+	 * @adev: amdgpu device we do this update for
+	 */
+	struct amdgpu_device *adev;
+
+	/**
+	 * @vm: optional amdgpu_vm we do this update for
+	 */
+	struct amdgpu_vm *vm;
+
+	/**
+	 * @pages_addr:
+	 *
+	 * DMA addresses to use for mapping
+	 */
+	dma_addr_t *pages_addr;
+
+	/**
+	 * @src: address where to copy page table entries from
+	 */
+	uint64_t src;
+
+	/**
+	 * @ib: indirect buffer to fill with commands
+	 */
+	struct amdgpu_ib *ib;
+
+	/**
+	 * @func: Function which actually does the update
+	 */
+	void (*func)(struct amdgpu_vm_update_params *params,
+		     struct amdgpu_bo *bo, uint64_t pe,
+		     uint64_t addr, unsigned count, uint32_t incr,
+		     uint64_t flags);
+};
+
 struct amdgpu_vm {
 	/* tree of virtual addresses mapped */
 	struct rb_root_cached	va;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/8] drm/amdgpu: reserve less memory for PDE updates
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
  2019-03-19 12:44   ` [PATCH 2/8] drm/amdgpu: always set and check dma addresses in the VM code Christian König
  2019-03-19 12:44   ` [PATCH 3/8] drm/amdgpu: move and rename amdgpu_pte_update_params Christian König
@ 2019-03-19 12:44   ` Christian König
  2019-03-19 12:44   ` [PATCH 5/8] drm/amdgpu: new VM update backends Christian König
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

Allocating 16KB was way to much, just use 2KB as a start for now.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 560658406e8b..e83ad0548801 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1391,7 +1391,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 
 		params.func = amdgpu_vm_cpu_set_ptes;
 	} else {
-		ndw = 512 * 8;
+		ndw = 512;
 		r = amdgpu_job_alloc_with_ib(adev, ndw * 4, &job);
 		if (r)
 			return r;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/8] drm/amdgpu: new VM update backends
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
                     ` (2 preceding siblings ...)
  2019-03-19 12:44   ` [PATCH 4/8] drm/amdgpu: reserve less memory for PDE updates Christian König
@ 2019-03-19 12:44   ` Christian König
       [not found]     ` <20190319124411.1563-5-christian.koenig-5C7GfCeVMHo@public.gmane.org>
  2019-03-19 12:44   ` [PATCH 6/8] drm/amdgpu: use the new VM backend for PDEs Christian König
                     ` (2 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

Separate out all functions for SDMA and CPU based page table
updates into separate backends.

This way we can keep most of the complexity of those from the
core VM code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Makefile         |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  30 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 116 +++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 248 ++++++++++++++++++++
 5 files changed, 401 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6039944abb71..7d539ba6400d 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -53,7 +53,8 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
 	amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \
 	amdgpu_gtt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o amdgpu_atomfirmware.o \
 	amdgpu_vf_error.o amdgpu_sched.o amdgpu_debugfs.o amdgpu_ids.o \
-	amdgpu_gmc.o amdgpu_xgmi.o amdgpu_csa.o amdgpu_ras.o
+	amdgpu_gmc.o amdgpu_xgmi.o amdgpu_csa.o amdgpu_ras.o amdgpu_vm_cpu.o \
+	amdgpu_vm_sdma.o
 
 # add asic specific block
 amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index e83ad0548801..86e12acb8493 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1243,7 +1243,7 @@ static void amdgpu_vm_do_copy_ptes(struct amdgpu_vm_update_params *params,
  * Returns:
  * The pointer for the page table entry.
  */
-static uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr)
+uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr)
 {
 	uint64_t result;
 
@@ -2975,6 +2975,11 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			 vm->use_cpu_for_update ? "CPU" : "SDMA");
 	WARN_ONCE((vm->use_cpu_for_update && !amdgpu_gmc_vram_full_visible(&adev->gmc)),
 		  "CPU update of VM recommended only for large BAR system\n");
+
+	if (vm->use_cpu_for_update)
+		vm->update_funcs = &amdgpu_vm_cpu_funcs;
+	else
+		vm->update_funcs = &amdgpu_vm_sdma_funcs;
 	vm->last_update = NULL;
 
 	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, &bp);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 6df4d9e382ac..a99b4caba13c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -203,11 +203,21 @@ struct amdgpu_vm_update_params {
 	 */
 	uint64_t src;
 
+	/**
+	 * @job: job to used for hw submission
+	 */
+	struct amdgpu_job *job;
+
 	/**
 	 * @ib: indirect buffer to fill with commands
 	 */
 	struct amdgpu_ib *ib;
 
+	/**
+	 * @num_dw_left: number of dw left for the IB
+	 */
+	unsigned int num_dw_left;
+
 	/**
 	 * @func: Function which actually does the update
 	 */
@@ -217,6 +227,17 @@ struct amdgpu_vm_update_params {
 		     uint64_t flags);
 };
 
+struct amdgpu_vm_update_funcs {
+
+	int (*prepare)(struct amdgpu_vm_update_params *p, void * owner,
+		       struct dma_fence *exclusive);
+	int (*update)(struct amdgpu_vm_update_params *p,
+		      struct amdgpu_bo *bo, uint64_t pe, uint64_t addr,
+		      unsigned count, uint32_t incr, uint64_t flags);
+	int (*commit)(struct amdgpu_vm_update_params *p,
+		      struct dma_fence **fence);
+};
+
 struct amdgpu_vm {
 	/* tree of virtual addresses mapped */
 	struct rb_root_cached	va;
@@ -252,7 +273,10 @@ struct amdgpu_vm {
 	struct amdgpu_vmid	*reserved_vmid[AMDGPU_MAX_VMHUBS];
 
 	/* Flag to indicate if VM tables are updated by CPU or GPU (SDMA) */
-	bool                    use_cpu_for_update;
+	bool					use_cpu_for_update;
+
+	/* Functions to use for VM table updates */
+	const struct amdgpu_vm_update_funcs	*update_funcs;
 
 	/* Flag to indicate ATS support from PTE for GFX9 */
 	bool			pte_support_ats;
@@ -319,6 +343,9 @@ struct amdgpu_vm_manager {
 #define amdgpu_vm_write_pte(adev, ib, pe, value, count, incr) ((adev)->vm_manager.vm_pte_funcs->write_pte((ib), (pe), (value), (count), (incr)))
 #define amdgpu_vm_set_pte_pde(adev, ib, pe, addr, count, incr, flags) ((adev)->vm_manager.vm_pte_funcs->set_pte_pde((ib), (pe), (addr), (count), (incr), (flags)))
 
+extern const struct amdgpu_vm_update_funcs amdgpu_vm_cpu_funcs;
+extern const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs;
+
 void amdgpu_vm_manager_init(struct amdgpu_device *adev);
 void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
 
@@ -348,6 +375,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			bool clear);
 void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 			     struct amdgpu_bo *bo, bool evicted);
+uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
 struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 				       struct amdgpu_bo *bo);
 struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
new file mode 100644
index 000000000000..9d53982021de
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
@@ -0,0 +1,116 @@
+/*
+ * Copyright 2019 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 "amdgpu_vm.h"
+#include "amdgpu_object.h"
+#include "amdgpu_trace.h"
+
+/**
+ * amdgpu_vm_cpu_prepare - prepare page table update with the CPU
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @owner: owner we need to sync to
+ * @exclusive: exclusive move fence we need to sync to
+ *
+ * Returns:
+ * Negativ errno, 0 for success.
+ */
+static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, void *owner,
+				 struct dma_fence *exclusive)
+{
+	int r;
+
+	/* Wait for PT BOs to be idle. PTs share the same resv. object
+	 * as the root PD BO
+	 */
+	r = amdgpu_bo_sync_wait(p->vm->root.base.bo, owner, true);
+	if (unlikely(r))
+		return r;
+
+	/* Wait for any BO move to be completed */
+	if (exclusive) {
+		r = dma_fence_wait(exclusive, true);
+		if (unlikely(r))
+			return r;
+	}
+
+	return 0;
+}
+
+/**
+ * amdgpu_vm_cpu_update - helper to update page tables via CPU
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @bo: PD/PT to update
+ * @pe: kmap addr of the page entry
+ * @addr: dst addr to write into pe
+ * @count: number of page entries to update
+ * @incr: increase next addr by incr bytes
+ * @flags: hw access flags
+ *
+ * Write count number of PT/PD entries directly.
+ */
+static int amdgpu_vm_cpu_update(struct amdgpu_vm_update_params *p,
+				struct amdgpu_bo *bo, uint64_t pe,
+				uint64_t addr, unsigned count, uint32_t incr,
+				uint64_t flags)
+{
+	unsigned int i;
+	uint64_t value;
+
+	pe += (unsigned long)amdgpu_bo_kptr(bo);
+
+	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
+
+	for (i = 0; i < count; i++) {
+		value = p->pages_addr ?
+			amdgpu_vm_map_gart(p->pages_addr, addr) :
+			addr;
+		amdgpu_gmc_set_pte_pde(p->adev, (void *)(uintptr_t)pe,
+				       i, value, flags);
+		addr += incr;
+	}
+	return 0;
+}
+
+/**
+ * amdgpu_vm_cpu_commit - commit page table update to the HW
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @fence: unused
+ *
+ * Make sure that the hardware sees the page table updates.
+ */
+static int amdgpu_vm_cpu_commit(struct amdgpu_vm_update_params *p,
+				struct dma_fence **fence)
+{
+	/* Flush HDP */
+	mb();
+	amdgpu_asic_flush_hdp(p->adev, NULL);
+	return 0;
+}
+
+const struct amdgpu_vm_update_funcs amdgpu_vm_cpu_funcs = {
+	.prepare = amdgpu_vm_cpu_prepare,
+	.update = amdgpu_vm_cpu_update,
+	.commit = amdgpu_vm_cpu_commit
+};
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
new file mode 100644
index 000000000000..e4bacdb44c68
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -0,0 +1,248 @@
+/*
+ * Copyright 2019 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 "amdgpu_vm.h"
+#include "amdgpu_job.h"
+#include "amdgpu_object.h"
+#include "amdgpu_trace.h"
+
+#define AMDGPU_VM_SDMA_MIN_NUM_DW	256u
+#define AMDGPU_VM_SDMA_MAX_NUM_DW	(16u * 1024u)
+
+/**
+ * amdgpu_vm_sdma_prepare - prepare SDMA command submission
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @owner: owner we need to sync to
+ * @exclusive: exclusive move fence we need to sync to
+ *
+ * Returns:
+ * Negativ errno, 0 for success.
+ */
+static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
+				  void *owner, struct dma_fence *exclusive)
+{
+	struct amdgpu_bo *root = p->vm->root.base.bo;
+	unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
+	int r;
+
+	r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.resv,
+			     owner, false);
+	if (r)
+		return r;
+
+	p->num_dw_left = ndw;
+	p->ib = &p->job->ibs[0];
+	return 0;
+}
+
+/**
+ * amdgpu_vm_sdma_commit - commit SDMA command submission
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @fence: resulting fence
+ *
+ * Returns:
+ * Negativ errno, 0 for success.
+ */
+static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
+				 struct dma_fence **fence)
+{
+	struct amdgpu_bo *root = p->vm->root.base.bo;
+	struct amdgpu_ring *ring;
+	struct dma_fence *f;
+	int r;
+
+	ring = container_of(p->vm->entity.rq->sched, struct amdgpu_ring, sched);
+
+	WARN_ON(p->ib->length_dw == 0);
+	amdgpu_ring_pad_ib(ring, p->ib);
+	WARN_ON(p->ib->length_dw > p->num_dw_left);
+	r = amdgpu_job_submit(p->job, &p->vm->entity,
+			      AMDGPU_FENCE_OWNER_VM, &f);
+	if (r)
+		goto error;
+
+	amdgpu_bo_fence(root, f, true);
+	if (fence)
+		swap(*fence, f);
+	dma_fence_put(f);
+	return 0;
+
+error:
+	amdgpu_job_free(p->job);
+	return r;
+}
+
+
+/**
+ * amdgpu_vm_sdma_copy_ptes - copy the PTEs from mapping
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @bo: PD/PT to update
+ * @pe: addr of the page entry
+ * @count: number of page entries to copy
+ *
+ * Traces the parameters and calls the DMA function to copy the PTEs.
+ */
+static void amdgpu_vm_sdma_copy_ptes(struct amdgpu_vm_update_params *p,
+				     struct amdgpu_bo *bo, uint64_t pe,
+				     unsigned count)
+{
+	uint64_t src = p->ib->gpu_addr;
+
+	src += p->num_dw_left * 4;
+
+	pe += amdgpu_bo_gpu_offset(bo);
+	trace_amdgpu_vm_copy_ptes(pe, src, count);
+
+	amdgpu_vm_copy_pte(p->adev, p->ib, pe, src, count);
+}
+
+/**
+ * amdgpu_vm_sdma_set_ptes - helper to call the right asic function
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @bo: PD/PT to update
+ * @pe: addr of the page entry
+ * @addr: dst addr to write into pe
+ * @count: number of page entries to update
+ * @incr: increase next addr by incr bytes
+ * @flags: hw access flags
+ *
+ * Traces the parameters and calls the right asic functions
+ * to setup the page table using the DMA.
+ */
+static void amdgpu_vm_sdma_set_ptes(struct amdgpu_vm_update_params *p,
+				    struct amdgpu_bo *bo, uint64_t pe,
+				    uint64_t addr, unsigned count,
+				    uint32_t incr, uint64_t flags)
+{
+	pe += amdgpu_bo_gpu_offset(bo);
+	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
+	if (count < 3) {
+		amdgpu_vm_write_pte(p->adev, p->ib, pe, addr | flags,
+				    count, incr);
+	} else {
+		amdgpu_vm_set_pte_pde(p->adev, p->ib, pe, addr,
+				      count, incr, flags);
+	}
+}
+
+/**
+ * amdgpu_vm_sdma_update - execute VM update
+ *
+ * @p: see amdgpu_vm_update_params definition
+ * @bo: PD/PT to update
+ * @pe: addr of the page entry
+ * @addr: dst addr to write into pe
+ * @count: number of page entries to update
+ * @incr: increase next addr by incr bytes
+ * @flags: hw access flags
+ *
+ * Reserve space in the IB, setup mapping buffer on demand and write commands to
+ * the IB.
+ */
+static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
+				 struct amdgpu_bo *bo, uint64_t pe,
+				 uint64_t addr, unsigned count, uint32_t incr,
+				 uint64_t flags)
+{
+	unsigned int i, ndw, nptes;
+	uint64_t *pte;
+	int r;
+
+	do {
+		ndw = p->num_dw_left;
+		ndw -= p->ib->length_dw;
+
+		if (ndw < 32) {
+			r = amdgpu_vm_sdma_commit(p, NULL);
+			if (r)
+				return r;
+
+			/* estimate how many dw we need */
+			ndw = 32;
+			if (p->pages_addr)
+				ndw += count * 2;
+			ndw = max(ndw, AMDGPU_VM_SDMA_MIN_NUM_DW);
+			ndw = min(ndw, AMDGPU_VM_SDMA_MAX_NUM_DW);
+
+			r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
+			if (r)
+				return r;
+
+			p->num_dw_left = ndw;
+			p->ib = &p->job->ibs[0];
+		}
+
+		if (!p->pages_addr) {
+			/* set page commands needed */
+			if (bo->shadow)
+				amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
+							count, incr, flags);
+			amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
+						incr, flags);
+			return 0;
+		}
+
+		/* copy commands needed */
+		ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
+			(bo->shadow ? 2 : 1);
+
+		/* for padding */
+		ndw -= 7;
+
+		nptes = min(count, ndw / 2);
+
+		/* Put the PTEs at the end of the IB. */
+		p->num_dw_left -= nptes * 2;
+		pte = (uint64_t *)&(p->ib->ptr[p->num_dw_left]);
+		for (i = 0; i < nptes; ++i, addr += incr) {
+			pte[i] = amdgpu_vm_map_gart(p->pages_addr, addr);
+			pte[i] |= flags;
+		}
+
+		if (bo->shadow)
+			amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
+		amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
+
+		pe += nptes * 8;
+		count -= nptes;
+	} while (count);
+
+	return 0;
+}
+
+const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs = {
+	.prepare = amdgpu_vm_sdma_prepare,
+	.update = amdgpu_vm_sdma_update,
+	.commit = amdgpu_vm_sdma_commit
+};
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/8] drm/amdgpu: use the new VM backend for PDEs
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
                     ` (3 preceding siblings ...)
  2019-03-19 12:44   ` [PATCH 5/8] drm/amdgpu: new VM update backends Christian König
@ 2019-03-19 12:44   ` Christian König
  2019-03-19 12:44   ` [PATCH 7/8] drm/amdgpu: use the new VM backend for PTEs Christian König
  2019-03-19 12:44   ` [PATCH 8/8] drm/amdgpu: use the new VM backend for clears Christian König
  6 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

And remove the existing code when it is unused.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 +++++---------------------
 1 file changed, 14 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 86e12acb8493..735a32b8a596 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1320,10 +1320,10 @@ static void amdgpu_vm_update_func(struct amdgpu_vm_update_params *params,
  *
  * Makes sure the requested entry in parent is up to date.
  */
-static void amdgpu_vm_update_pde(struct amdgpu_vm_update_params *params,
-				 struct amdgpu_vm *vm,
-				 struct amdgpu_vm_pt *parent,
-				 struct amdgpu_vm_pt *entry)
+static int amdgpu_vm_update_pde(struct amdgpu_vm_update_params *params,
+				struct amdgpu_vm *vm,
+				struct amdgpu_vm_pt *parent,
+				struct amdgpu_vm_pt *entry)
 {
 	struct amdgpu_bo *bo = parent->base.bo, *pbo;
 	uint64_t pde, pt, flags;
@@ -1335,7 +1335,7 @@ static void amdgpu_vm_update_pde(struct amdgpu_vm_update_params *params,
 	level += params->adev->vm_manager.root_level;
 	amdgpu_gmc_get_pde_for_bo(entry->base.bo, level, &pt, &flags);
 	pde = (entry - parent->entries) * 8;
-	amdgpu_vm_update_func(params, bo, pde, pt, 1, 0, flags);
+	return vm->update_funcs->update(params, bo, pde, pt, 1, 0, flags);
 }
 
 /*
@@ -1372,33 +1372,18 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 				 struct amdgpu_vm *vm)
 {
 	struct amdgpu_vm_update_params params;
-	struct amdgpu_job *job;
-	unsigned ndw = 0;
-	int r = 0;
+	int r;
 
 	if (list_empty(&vm->relocated))
 		return 0;
 
-restart:
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
+	params.vm = vm;
 
-	if (vm->use_cpu_for_update) {
-		r = amdgpu_bo_sync_wait(vm->root.base.bo,
-					AMDGPU_FENCE_OWNER_VM, true);
-		if (unlikely(r))
-			return r;
-
-		params.func = amdgpu_vm_cpu_set_ptes;
-	} else {
-		ndw = 512;
-		r = amdgpu_job_alloc_with_ib(adev, ndw * 4, &job);
-		if (r)
-			return r;
-
-		params.ib = &job->ibs[0];
-		params.func = amdgpu_vm_do_set_ptes;
-	}
+	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_VM, NULL);
+	if (r)
+		return r;
 
 	while (!list_empty(&vm->relocated)) {
 		struct amdgpu_vm_pt *pt, *entry;
@@ -1411,49 +1396,18 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 		if (!pt)
 			continue;
 
-		amdgpu_vm_update_pde(&params, vm, pt, entry);
-
-		if (!vm->use_cpu_for_update &&
-		    (ndw - params.ib->length_dw) < 32)
-			break;
-	}
-
-	if (vm->use_cpu_for_update) {
-		/* Flush HDP */
-		mb();
-		amdgpu_asic_flush_hdp(adev, NULL);
-	} else if (params.ib->length_dw == 0) {
-		amdgpu_job_free(job);
-	} else {
-		struct amdgpu_bo *root = vm->root.base.bo;
-		struct amdgpu_ring *ring;
-		struct dma_fence *fence;
-
-		ring = container_of(vm->entity.rq->sched, struct amdgpu_ring,
-				    sched);
-
-		amdgpu_ring_pad_ib(ring, params.ib);
-		amdgpu_sync_resv(adev, &job->sync, root->tbo.resv,
-				 AMDGPU_FENCE_OWNER_VM, false);
-		WARN_ON(params.ib->length_dw > ndw);
-		r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_VM,
-				      &fence);
+		r = amdgpu_vm_update_pde(&params, vm, pt, entry);
 		if (r)
 			goto error;
-
-		amdgpu_bo_fence(root, fence, true);
-		dma_fence_put(vm->last_update);
-		vm->last_update = fence;
 	}
 
-	if (!list_empty(&vm->relocated))
-		goto restart;
-
+	r = vm->update_funcs->commit(&params, &vm->last_update);
+	if (r)
+		goto error;
 	return 0;
 
 error:
 	amdgpu_vm_invalidate_pds(adev, vm);
-	amdgpu_job_free(job);
 	return r;
 }
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 7/8] drm/amdgpu: use the new VM backend for PTEs
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
                     ` (4 preceding siblings ...)
  2019-03-19 12:44   ` [PATCH 6/8] drm/amdgpu: use the new VM backend for PDEs Christian König
@ 2019-03-19 12:44   ` Christian König
  2019-03-19 12:44   ` [PATCH 8/8] drm/amdgpu: use the new VM backend for clears Christian König
  6 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

And remove the existing code when it is unused.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 229 +------------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  13 --
 2 files changed, 6 insertions(+), 236 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 735a32b8a596..729da1c486cd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1171,66 +1171,6 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 	return NULL;
 }
 
-/**
- * amdgpu_vm_do_set_ptes - helper to call the right asic function
- *
- * @params: see amdgpu_vm_update_params definition
- * @bo: PD/PT to update
- * @pe: addr of the page entry
- * @addr: dst addr to write into pe
- * @count: number of page entries to update
- * @incr: increase next addr by incr bytes
- * @flags: hw access flags
- *
- * Traces the parameters and calls the right asic functions
- * to setup the page table using the DMA.
- */
-static void amdgpu_vm_do_set_ptes(struct amdgpu_vm_update_params *params,
-				  struct amdgpu_bo *bo,
-				  uint64_t pe, uint64_t addr,
-				  unsigned count, uint32_t incr,
-				  uint64_t flags)
-{
-	pe += amdgpu_bo_gpu_offset(bo);
-	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
-
-	if (count < 3) {
-		amdgpu_vm_write_pte(params->adev, params->ib, pe,
-				    addr | flags, count, incr);
-
-	} else {
-		amdgpu_vm_set_pte_pde(params->adev, params->ib, pe, addr,
-				      count, incr, flags);
-	}
-}
-
-/**
- * amdgpu_vm_do_copy_ptes - copy the PTEs from the GART
- *
- * @params: see amdgpu_vm_update_params definition
- * @bo: PD/PT to update
- * @pe: addr of the page entry
- * @addr: dst addr to write into pe
- * @count: number of page entries to update
- * @incr: increase next addr by incr bytes
- * @flags: hw access flags
- *
- * Traces the parameters and calls the DMA function to copy the PTEs.
- */
-static void amdgpu_vm_do_copy_ptes(struct amdgpu_vm_update_params *params,
-				   struct amdgpu_bo *bo,
-				   uint64_t pe, uint64_t addr,
-				   unsigned count, uint32_t incr,
-				   uint64_t flags)
-{
-	uint64_t src = (params->src + (addr >> 12) * 8);
-
-	pe += amdgpu_bo_gpu_offset(bo);
-	trace_amdgpu_vm_copy_ptes(pe, src, count);
-
-	amdgpu_vm_copy_pte(params->adev, params->ib, pe, src, count);
-}
-
 /**
  * amdgpu_vm_map_gart - Resolve gart mapping of addr
  *
@@ -1258,58 +1198,6 @@ uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr)
 	return result;
 }
 
-/**
- * amdgpu_vm_cpu_set_ptes - helper to update page tables via CPU
- *
- * @params: see amdgpu_vm_update_params definition
- * @bo: PD/PT to update
- * @pe: kmap addr of the page entry
- * @addr: dst addr to write into pe
- * @count: number of page entries to update
- * @incr: increase next addr by incr bytes
- * @flags: hw access flags
- *
- * Write count number of PT/PD entries directly.
- */
-static void amdgpu_vm_cpu_set_ptes(struct amdgpu_vm_update_params *params,
-				   struct amdgpu_bo *bo,
-				   uint64_t pe, uint64_t addr,
-				   unsigned count, uint32_t incr,
-				   uint64_t flags)
-{
-	unsigned int i;
-	uint64_t value;
-
-	pe += (unsigned long)amdgpu_bo_kptr(bo);
-
-	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
-
-	for (i = 0; i < count; i++) {
-		value = params->pages_addr ?
-			amdgpu_vm_map_gart(params->pages_addr, addr) :
-			addr;
-		amdgpu_gmc_set_pte_pde(params->adev, (void *)(uintptr_t)pe,
-				       i, value, flags);
-		addr += incr;
-	}
-}
-
-/**
- * amdgpu_vm_update_func - helper to call update function
- *
- * Calls the update function for both the given BO as well as its shadow.
- */
-static void amdgpu_vm_update_func(struct amdgpu_vm_update_params *params,
-				  struct amdgpu_bo *bo,
-				  uint64_t pe, uint64_t addr,
-				  unsigned count, uint32_t incr,
-				  uint64_t flags)
-{
-	if (bo->shadow)
-		params->func(params, bo->shadow, pe, addr, count, incr, flags);
-	params->func(params, bo, pe, addr, count, incr, flags);
-}
-
 /*
  * amdgpu_vm_update_pde - update a single level in the hierarchy
  *
@@ -1435,7 +1323,8 @@ static void amdgpu_vm_update_flags(struct amdgpu_vm_update_params *params,
 		flags |= AMDGPU_PTE_EXECUTABLE;
 	}
 
-	amdgpu_vm_update_func(params, bo, pe, addr, count, incr, flags);
+	params->vm->update_funcs->update(params, bo, pe, addr, count, incr,
+					 flags);
 }
 
 /**
@@ -1652,12 +1541,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 				       uint64_t flags, uint64_t addr,
 				       struct dma_fence **fence)
 {
-	struct amdgpu_ring *ring;
-	void *owner = AMDGPU_FENCE_OWNER_VM;
-	unsigned nptes, ncmds, ndw;
-	struct amdgpu_job *job;
 	struct amdgpu_vm_update_params params;
-	struct dma_fence *f = NULL;
+	void *owner = AMDGPU_FENCE_OWNER_VM;
 	int r;
 
 	memset(&params, 0, sizeof(params));
@@ -1669,116 +1554,15 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (!(flags & AMDGPU_PTE_VALID))
 		owner = AMDGPU_FENCE_OWNER_KFD;
 
-	if (vm->use_cpu_for_update) {
-		/* Wait for PT BOs to be idle. PTs share the same resv. object
-		 * as the root PD BO
-		 */
-		r = amdgpu_bo_sync_wait(vm->root.base.bo, owner, true);
-		if (unlikely(r))
-			return r;
-
-		/* Wait for any BO move to be completed */
-		if (exclusive) {
-			r = dma_fence_wait(exclusive, true);
-			if (unlikely(r))
-				return r;
-		}
-
-		params.func = amdgpu_vm_cpu_set_ptes;
-		return amdgpu_vm_update_ptes(&params, start, last + 1,
-					     addr, flags);
-	}
-
-	ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched);
-
-	nptes = last - start + 1;
-
-	/*
-	 * reserve space for two commands every (1 << BLOCK_SIZE)
-	 *  entries or 2k dwords (whatever is smaller)
-	 */
-	ncmds = ((nptes >> min(adev->vm_manager.block_size, 11u)) + 1);
-
-	/* The second command is for the shadow pagetables. */
-	if (vm->root.base.bo->shadow)
-		ncmds *= 2;
-
-	/* padding, etc. */
-	ndw = 64;
-
-	if (pages_addr) {
-		/* copy commands needed */
-		ndw += ncmds * adev->vm_manager.vm_pte_funcs->copy_pte_num_dw;
-
-		/* and also PTEs */
-		ndw += nptes * 2;
-
-		params.func = amdgpu_vm_do_copy_ptes;
-
-	} else {
-		/* set page commands needed */
-		ndw += ncmds * 10;
-
-		/* extra commands for begin/end fragments */
-		ncmds = 2 * adev->vm_manager.fragment_size;
-		if (vm->root.base.bo->shadow)
-			ncmds *= 2;
-
-		ndw += 10 * ncmds;
-
-		params.func = amdgpu_vm_do_set_ptes;
-	}
-
-	r = amdgpu_job_alloc_with_ib(adev, ndw * 4, &job);
+	r = vm->update_funcs->prepare(&params, owner, exclusive);
 	if (r)
 		return r;
 
-	params.ib = &job->ibs[0];
-
-	if (pages_addr) {
-		uint64_t *pte;
-		unsigned i;
-
-		/* Put the PTEs at the end of the IB. */
-		i = ndw - nptes * 2;
-		pte= (uint64_t *)&(job->ibs->ptr[i]);
-		params.src = job->ibs->gpu_addr + i * 4;
-
-		for (i = 0; i < nptes; ++i) {
-			pte[i] = amdgpu_vm_map_gart(pages_addr, addr + i *
-						    AMDGPU_GPU_PAGE_SIZE);
-			pte[i] |= flags;
-		}
-		addr = 0;
-	}
-
-	r = amdgpu_sync_fence(adev, &job->sync, exclusive, false);
-	if (r)
-		goto error_free;
-
-	r = amdgpu_sync_resv(adev, &job->sync, vm->root.base.bo->tbo.resv,
-			     owner, false);
-	if (r)
-		goto error_free;
-
 	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
 	if (r)
-		goto error_free;
-
-	amdgpu_ring_pad_ib(ring, params.ib);
-	WARN_ON(params.ib->length_dw > ndw);
-	r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_VM, &f);
-	if (r)
-		goto error_free;
-
-	amdgpu_bo_fence(vm->root.base.bo, f, true);
-	dma_fence_put(*fence);
-	*fence = f;
-	return 0;
+		return r;
 
-error_free:
-	amdgpu_job_free(job);
-	return r;
+	return vm->update_funcs->commit(&params, fence);
 }
 
 /**
@@ -1861,7 +1645,6 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
 		if (pages_addr) {
 			uint64_t count;
 
-			max_entries = min(max_entries, 16ull * 1024ull);
 			for (count = 1;
 			     count < max_entries / AMDGPU_GPU_PAGES_IN_CPU_PAGE;
 			     ++count) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index a99b4caba13c..520122be798b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -198,11 +198,6 @@ struct amdgpu_vm_update_params {
 	 */
 	dma_addr_t *pages_addr;
 
-	/**
-	 * @src: address where to copy page table entries from
-	 */
-	uint64_t src;
-
 	/**
 	 * @job: job to used for hw submission
 	 */
@@ -217,14 +212,6 @@ struct amdgpu_vm_update_params {
 	 * @num_dw_left: number of dw left for the IB
 	 */
 	unsigned int num_dw_left;
-
-	/**
-	 * @func: Function which actually does the update
-	 */
-	void (*func)(struct amdgpu_vm_update_params *params,
-		     struct amdgpu_bo *bo, uint64_t pe,
-		     uint64_t addr, unsigned count, uint32_t incr,
-		     uint64_t flags);
 };
 
 struct amdgpu_vm_update_funcs {
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 8/8] drm/amdgpu: use the new VM backend for clears
       [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
                     ` (5 preceding siblings ...)
  2019-03-19 12:44   ` [PATCH 7/8] drm/amdgpu: use the new VM backend for PTEs Christian König
@ 2019-03-19 12:44   ` Christian König
       [not found]     ` <20190319124411.1563-8-christian.koenig-5C7GfCeVMHo@public.gmane.org>
  6 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2019-03-19 12:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

And remove the existing code when it is unused.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 89 +++++++++-----------------
 1 file changed, 32 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 729da1c486cd..af1a7020c3ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -711,11 +711,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 {
 	struct ttm_operation_ctx ctx = { true, false };
 	unsigned level = adev->vm_manager.root_level;
+	struct amdgpu_vm_update_params params;
 	struct amdgpu_bo *ancestor = bo;
-	struct dma_fence *fence = NULL;
 	unsigned entries, ats_entries;
-	struct amdgpu_ring *ring;
-	struct amdgpu_job *job;
 	uint64_t addr;
 	int r;
 
@@ -750,8 +748,6 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 		}
 	}
 
-	ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched);
-
 	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
 	if (r)
 		return r;
@@ -772,60 +768,45 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 
 	}
 
-	r = amdgpu_job_alloc_with_ib(adev, 64, &job);
+	memset(&params, 0, sizeof(params));
+	params.adev = adev;
+	params.vm = vm;
+
+	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_KFD, NULL);
 	if (r)
 		return r;
 
-	do {
-		addr = amdgpu_bo_gpu_offset(bo);
-		if (ats_entries) {
-			uint64_t ats_value;
-
-			ats_value = AMDGPU_PTE_DEFAULT_ATC;
-			if (level != AMDGPU_VM_PTB)
-				ats_value |= AMDGPU_PDE_PTE;
-
-			amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
-					      ats_entries, 0, ats_value);
-			addr += ats_entries * 8;
-		}
-
-		if (entries) {
-			uint64_t value = 0;
-
-			/* Workaround for fault priority problem on GMC9 */
-			if (level == AMDGPU_VM_PTB &&
-			    adev->asic_type >= CHIP_VEGA10)
-				value = AMDGPU_PTE_EXECUTABLE;
-
-			amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
-					      entries, 0, value);
-		}
+	addr = 0;
+	if (ats_entries) {
+		uint64_t ats_value;
 
-		bo = bo->shadow;
-	} while (bo);
+		ats_value = AMDGPU_PTE_DEFAULT_ATC;
+		if (level != AMDGPU_VM_PTB)
+			ats_value |= AMDGPU_PDE_PTE;
 
-	amdgpu_ring_pad_ib(ring, &job->ibs[0]);
+		r = vm->update_funcs->update(&params, bo, addr, 0, ats_entries,
+					     0, ats_value);
+		if (r)
+			return r;
 
-	WARN_ON(job->ibs[0].length_dw > 64);
-	r = amdgpu_sync_resv(adev, &job->sync, vm->root.base.bo->tbo.resv,
-			     AMDGPU_FENCE_OWNER_KFD, false);
-	if (r)
-		goto error_free;
+		addr += ats_entries * 8;
+	}
 
-	r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_UNDEFINED,
-			      &fence);
-	if (r)
-		goto error_free;
+	if (entries) {
+		uint64_t value = 0;
 
-	amdgpu_bo_fence(vm->root.base.bo, fence, true);
-	dma_fence_put(fence);
+		/* Workaround for fault priority problem on GMC9 */
+		if (level == AMDGPU_VM_PTB &&
+		    adev->asic_type >= CHIP_VEGA10)
+			value = AMDGPU_PTE_EXECUTABLE;
 
-	return 0;
+		r = vm->update_funcs->update(&params, bo, addr, 0, entries,
+					     0, value);
+		if (r)
+			return r;
+	}
 
-error_free:
-	amdgpu_job_free(job);
-	return r;
+	return vm->update_funcs->commit(&params, NULL);
 }
 
 /**
@@ -913,7 +894,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 	if (r)
 		goto error_free_pt;
 
-	return 1;
+	return 0;
 
 error_free_pt:
 	amdgpu_bo_unref(&pt->shadow);
@@ -1421,12 +1402,10 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 		unsigned shift, parent_shift, mask;
 		uint64_t incr, entry_end, pe_start;
 		struct amdgpu_bo *pt;
-		bool need_to_sync;
 
 		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
-		if (r < 0)
+		if (r)
 			return r;
-		need_to_sync = (r && params->vm->use_cpu_for_update);
 
 		pt = cursor.entry->base.bo;
 
@@ -1474,10 +1453,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 		entry_end += cursor.pfn & ~(entry_end - 1);
 		entry_end = min(entry_end, end);
 
-		if (need_to_sync)
-			r = amdgpu_bo_sync_wait(params->vm->root.base.bo,
-						AMDGPU_FENCE_OWNER_VM, true);
-
 		do {
 			uint64_t upd_end = min(entry_end, frag_end);
 			unsigned nptes = (upd_end - frag_start) >> shift;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 8/8] drm/amdgpu: use the new VM backend for clears
       [not found]     ` <20190319124411.1563-8-christian.koenig-5C7GfCeVMHo@public.gmane.org>
@ 2019-03-20  4:27       ` zhoucm1
  0 siblings, 0 replies; 12+ messages in thread
From: zhoucm1 @ 2019-03-20  4:27 UTC (permalink / raw)
  To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

patch#2 and patch#4 are Ached-by: Chunming Zhou <david1.zhou@amd.com>

patch#1, #3, #5~#8 are Reviewed-by: Chunming Zhou <david1.zhou@amd.com>



On 2019年03月19日 20:44, Christian König wrote:
> And remove the existing code when it is unused.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 89 +++++++++-----------------
>   1 file changed, 32 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 729da1c486cd..af1a7020c3ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -711,11 +711,9 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   {
>   	struct ttm_operation_ctx ctx = { true, false };
>   	unsigned level = adev->vm_manager.root_level;
> +	struct amdgpu_vm_update_params params;
>   	struct amdgpu_bo *ancestor = bo;
> -	struct dma_fence *fence = NULL;
>   	unsigned entries, ats_entries;
> -	struct amdgpu_ring *ring;
> -	struct amdgpu_job *job;
>   	uint64_t addr;
>   	int r;
>   
> @@ -750,8 +748,6 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   		}
>   	}
>   
> -	ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched);
> -
>   	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>   	if (r)
>   		return r;
> @@ -772,60 +768,45 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   
>   	}
>   
> -	r = amdgpu_job_alloc_with_ib(adev, 64, &job);
> +	memset(&params, 0, sizeof(params));
> +	params.adev = adev;
> +	params.vm = vm;
> +
> +	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_KFD, NULL);
>   	if (r)
>   		return r;
>   
> -	do {
> -		addr = amdgpu_bo_gpu_offset(bo);
> -		if (ats_entries) {
> -			uint64_t ats_value;
> -
> -			ats_value = AMDGPU_PTE_DEFAULT_ATC;
> -			if (level != AMDGPU_VM_PTB)
> -				ats_value |= AMDGPU_PDE_PTE;
> -
> -			amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
> -					      ats_entries, 0, ats_value);
> -			addr += ats_entries * 8;
> -		}
> -
> -		if (entries) {
> -			uint64_t value = 0;
> -
> -			/* Workaround for fault priority problem on GMC9 */
> -			if (level == AMDGPU_VM_PTB &&
> -			    adev->asic_type >= CHIP_VEGA10)
> -				value = AMDGPU_PTE_EXECUTABLE;
> -
> -			amdgpu_vm_set_pte_pde(adev, &job->ibs[0], addr, 0,
> -					      entries, 0, value);
> -		}
> +	addr = 0;
> +	if (ats_entries) {
> +		uint64_t ats_value;
>   
> -		bo = bo->shadow;
> -	} while (bo);
> +		ats_value = AMDGPU_PTE_DEFAULT_ATC;
> +		if (level != AMDGPU_VM_PTB)
> +			ats_value |= AMDGPU_PDE_PTE;
>   
> -	amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> +		r = vm->update_funcs->update(&params, bo, addr, 0, ats_entries,
> +					     0, ats_value);
> +		if (r)
> +			return r;
>   
> -	WARN_ON(job->ibs[0].length_dw > 64);
> -	r = amdgpu_sync_resv(adev, &job->sync, vm->root.base.bo->tbo.resv,
> -			     AMDGPU_FENCE_OWNER_KFD, false);
> -	if (r)
> -		goto error_free;
> +		addr += ats_entries * 8;
> +	}
>   
> -	r = amdgpu_job_submit(job, &vm->entity, AMDGPU_FENCE_OWNER_UNDEFINED,
> -			      &fence);
> -	if (r)
> -		goto error_free;
> +	if (entries) {
> +		uint64_t value = 0;
>   
> -	amdgpu_bo_fence(vm->root.base.bo, fence, true);
> -	dma_fence_put(fence);
> +		/* Workaround for fault priority problem on GMC9 */
> +		if (level == AMDGPU_VM_PTB &&
> +		    adev->asic_type >= CHIP_VEGA10)
> +			value = AMDGPU_PTE_EXECUTABLE;
>   
> -	return 0;
> +		r = vm->update_funcs->update(&params, bo, addr, 0, entries,
> +					     0, value);
> +		if (r)
> +			return r;
> +	}
>   
> -error_free:
> -	amdgpu_job_free(job);
> -	return r;
> +	return vm->update_funcs->commit(&params, NULL);
>   }
>   
>   /**
> @@ -913,7 +894,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   	if (r)
>   		goto error_free_pt;
>   
> -	return 1;
> +	return 0;
>   
>   error_free_pt:
>   	amdgpu_bo_unref(&pt->shadow);
> @@ -1421,12 +1402,10 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   		unsigned shift, parent_shift, mask;
>   		uint64_t incr, entry_end, pe_start;
>   		struct amdgpu_bo *pt;
> -		bool need_to_sync;
>   
>   		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
> -		if (r < 0)
> +		if (r)
>   			return r;
> -		need_to_sync = (r && params->vm->use_cpu_for_update);
>   
>   		pt = cursor.entry->base.bo;
>   
> @@ -1474,10 +1453,6 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   		entry_end += cursor.pfn & ~(entry_end - 1);
>   		entry_end = min(entry_end, end);
>   
> -		if (need_to_sync)
> -			r = amdgpu_bo_sync_wait(params->vm->root.base.bo,
> -						AMDGPU_FENCE_OWNER_VM, true);
> -
>   		do {
>   			uint64_t upd_end = min(entry_end, frag_end);
>   			unsigned nptes = (upd_end - frag_start) >> shift;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/8] drm/amdgpu: new VM update backends
       [not found]     ` <20190319124411.1563-5-christian.koenig-5C7GfCeVMHo@public.gmane.org>
@ 2019-03-20 11:57       ` Kuehling, Felix
       [not found]         ` <153c002a-5454-ad17-bd3b-03685867844d-5C7GfCeVMHo@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Kuehling, Felix @ 2019-03-20 11:57 UTC (permalink / raw)
  To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	Russell, Kent

As far as I can tell, the whole series is a small cleanup and big 
refactor to enable CPU clearing of PTs without a lot of ugliness or code 
duplication. It looks good to me. I haven't reviewed all the moved SDMA 
update code to make sure it all works correctly, but at least the 
prepare and commit functions look sane to me.

For this patch I have a suggestion (inline) to remove params->ib, which 
seems redundant with params->job. That would also ripple through the 
remaining patches.

Other than that, the series is Reviewed-by: Felix Kuehling 
<Felix.Kuehling@amd.com>

[+Kent], Look out for this patch series in an upcoming merge to 
amd-kfd-staging. I don't think it'll cause conflicts, but has a risk of 
regressions (like all big amdgpu_vm changes IME).

Regards,
   Felix

On 3/19/2019 8:44 AM, Christian König wrote:
> Separate out all functions for SDMA and CPU based page table
> updates into separate backends.
>
> This way we can keep most of the complexity of those from the
> core VM code.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/Makefile         |   3 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |   7 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  30 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 116 +++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 248 ++++++++++++++++++++
>   5 files changed, 401 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
[snip]
> +/**
> + * amdgpu_vm_sdma_prepare - prepare SDMA command submission
> + *
> + * @p: see amdgpu_vm_update_params definition
> + * @owner: owner we need to sync to
> + * @exclusive: exclusive move fence we need to sync to
> + *
> + * Returns:
> + * Negativ errno, 0 for success.
> + */
> +static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
> +				  void *owner, struct dma_fence *exclusive)
> +{
> +	struct amdgpu_bo *root = p->vm->root.base.bo;
> +	unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
> +	int r;
> +
> +	r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
> +	if (r)
> +		return r;
> +
> +	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
> +	if (r)
> +		return r;
> +
> +	r = amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.resv,
> +			     owner, false);
> +	if (r)
> +		return r;
> +
> +	p->num_dw_left = ndw;
> +	p->ib = &p->job->ibs[0];

With p->job added, do we still need p->ib? We could just use 
&p->job->ibs[0] directly, which should perform the same or be more 
efficient since it's just a constant offset from p->job.


> +	return 0;
> +}
> +
> +/**
> + * amdgpu_vm_sdma_commit - commit SDMA command submission
> + *
> + * @p: see amdgpu_vm_update_params definition
> + * @fence: resulting fence
> + *
> + * Returns:
> + * Negativ errno, 0 for success.
> + */
> +static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
> +				 struct dma_fence **fence)
> +{
> +	struct amdgpu_bo *root = p->vm->root.base.bo;
> +	struct amdgpu_ring *ring;
> +	struct dma_fence *f;
> +	int r;
> +
> +	ring = container_of(p->vm->entity.rq->sched, struct amdgpu_ring, sched);
> +
> +	WARN_ON(p->ib->length_dw == 0);
> +	amdgpu_ring_pad_ib(ring, p->ib);
> +	WARN_ON(p->ib->length_dw > p->num_dw_left);
> +	r = amdgpu_job_submit(p->job, &p->vm->entity,
> +			      AMDGPU_FENCE_OWNER_VM, &f);
> +	if (r)
> +		goto error;
> +
> +	amdgpu_bo_fence(root, f, true);
> +	if (fence)
> +		swap(*fence, f);
> +	dma_fence_put(f);
> +	return 0;
> +
> +error:
> +	amdgpu_job_free(p->job);
> +	return r;
> +}
> +
> +
> +/**
> + * amdgpu_vm_sdma_copy_ptes - copy the PTEs from mapping
> + *
> + * @p: see amdgpu_vm_update_params definition
> + * @bo: PD/PT to update
> + * @pe: addr of the page entry
> + * @count: number of page entries to copy
> + *
> + * Traces the parameters and calls the DMA function to copy the PTEs.
> + */
> +static void amdgpu_vm_sdma_copy_ptes(struct amdgpu_vm_update_params *p,
> +				     struct amdgpu_bo *bo, uint64_t pe,
> +				     unsigned count)
> +{
> +	uint64_t src = p->ib->gpu_addr;
> +
> +	src += p->num_dw_left * 4;
> +
> +	pe += amdgpu_bo_gpu_offset(bo);
> +	trace_amdgpu_vm_copy_ptes(pe, src, count);
> +
> +	amdgpu_vm_copy_pte(p->adev, p->ib, pe, src, count);
> +}
> +
> +/**
> + * amdgpu_vm_sdma_set_ptes - helper to call the right asic function
> + *
> + * @p: see amdgpu_vm_update_params definition
> + * @bo: PD/PT to update
> + * @pe: addr of the page entry
> + * @addr: dst addr to write into pe
> + * @count: number of page entries to update
> + * @incr: increase next addr by incr bytes
> + * @flags: hw access flags
> + *
> + * Traces the parameters and calls the right asic functions
> + * to setup the page table using the DMA.
> + */
> +static void amdgpu_vm_sdma_set_ptes(struct amdgpu_vm_update_params *p,
> +				    struct amdgpu_bo *bo, uint64_t pe,
> +				    uint64_t addr, unsigned count,
> +				    uint32_t incr, uint64_t flags)
> +{
> +	pe += amdgpu_bo_gpu_offset(bo);
> +	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
> +	if (count < 3) {
> +		amdgpu_vm_write_pte(p->adev, p->ib, pe, addr | flags,
> +				    count, incr);
> +	} else {
> +		amdgpu_vm_set_pte_pde(p->adev, p->ib, pe, addr,
> +				      count, incr, flags);
> +	}
> +}
> +
> +/**
> + * amdgpu_vm_sdma_update - execute VM update
> + *
> + * @p: see amdgpu_vm_update_params definition
> + * @bo: PD/PT to update
> + * @pe: addr of the page entry
> + * @addr: dst addr to write into pe
> + * @count: number of page entries to update
> + * @incr: increase next addr by incr bytes
> + * @flags: hw access flags
> + *
> + * Reserve space in the IB, setup mapping buffer on demand and write commands to
> + * the IB.
> + */
> +static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
> +				 struct amdgpu_bo *bo, uint64_t pe,
> +				 uint64_t addr, unsigned count, uint32_t incr,
> +				 uint64_t flags)
> +{
> +	unsigned int i, ndw, nptes;
> +	uint64_t *pte;
> +	int r;
> +
> +	do {
> +		ndw = p->num_dw_left;
> +		ndw -= p->ib->length_dw;
> +
> +		if (ndw < 32) {
> +			r = amdgpu_vm_sdma_commit(p, NULL);
> +			if (r)
> +				return r;
> +
> +			/* estimate how many dw we need */
> +			ndw = 32;
> +			if (p->pages_addr)
> +				ndw += count * 2;
> +			ndw = max(ndw, AMDGPU_VM_SDMA_MIN_NUM_DW);
> +			ndw = min(ndw, AMDGPU_VM_SDMA_MAX_NUM_DW);
> +
> +			r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
> +			if (r)
> +				return r;
> +
> +			p->num_dw_left = ndw;
> +			p->ib = &p->job->ibs[0];
> +		}
> +
> +		if (!p->pages_addr) {
> +			/* set page commands needed */
> +			if (bo->shadow)
> +				amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
> +							count, incr, flags);
> +			amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
> +						incr, flags);
> +			return 0;
> +		}
> +
> +		/* copy commands needed */
> +		ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
> +			(bo->shadow ? 2 : 1);
> +
> +		/* for padding */
> +		ndw -= 7;
> +
> +		nptes = min(count, ndw / 2);
> +
> +		/* Put the PTEs at the end of the IB. */
> +		p->num_dw_left -= nptes * 2;
> +		pte = (uint64_t *)&(p->ib->ptr[p->num_dw_left]);
> +		for (i = 0; i < nptes; ++i, addr += incr) {
> +			pte[i] = amdgpu_vm_map_gart(p->pages_addr, addr);
> +			pte[i] |= flags;
> +		}
> +
> +		if (bo->shadow)
> +			amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
> +		amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
> +
> +		pe += nptes * 8;
> +		count -= nptes;
> +	} while (count);
> +
> +	return 0;
> +}
> +
> +const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs = {
> +	.prepare = amdgpu_vm_sdma_prepare,
> +	.update = amdgpu_vm_sdma_update,
> +	.commit = amdgpu_vm_sdma_commit
> +};
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/8] drm/amdgpu: new VM update backends
       [not found]         ` <153c002a-5454-ad17-bd3b-03685867844d-5C7GfCeVMHo@public.gmane.org>
@ 2019-03-25 11:38           ` Christian König
       [not found]             ` <11141c5e-155f-79dc-b5bb-00da3b8608ce-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2019-03-25 11:38 UTC (permalink / raw)
  To: Kuehling, Felix, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Russell, Kent

Am 20.03.19 um 12:57 schrieb Kuehling, Felix:
> As far as I can tell, the whole series is a small cleanup and big
> refactor to enable CPU clearing of PTs without a lot of ugliness or code
> duplication.

It's a bit more than that. Key point is that I can now easily add a 
parameter for direct submission during page fault handling :)

Christian.

> It looks good to me. I haven't reviewed all the moved SDMA
> update code to make sure it all works correctly, but at least the
> prepare and commit functions look sane to me.
>
> For this patch I have a suggestion (inline) to remove params->ib, which
> seems redundant with params->job. That would also ripple through the
> remaining patches.
>
> Other than that, the series is Reviewed-by: Felix Kuehling
> <Felix.Kuehling@amd.com>
>
> [+Kent], Look out for this patch series in an upcoming merge to
> amd-kfd-staging. I don't think it'll cause conflicts, but has a risk of
> regressions (like all big amdgpu_vm changes IME).
>
> Regards,
>     Felix
>
> On 3/19/2019 8:44 AM, Christian König wrote:
>> Separate out all functions for SDMA and CPU based page table
>> updates into separate backends.
>>
>> This way we can keep most of the complexity of those from the
>> core VM code.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/Makefile         |   3 +-
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |   7 +-
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  30 ++-
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 116 +++++++++
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 248 ++++++++++++++++++++
>>    5 files changed, 401 insertions(+), 3 deletions(-)
>>    create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>>    create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> [snip]
>> +/**
>> + * amdgpu_vm_sdma_prepare - prepare SDMA command submission
>> + *
>> + * @p: see amdgpu_vm_update_params definition
>> + * @owner: owner we need to sync to
>> + * @exclusive: exclusive move fence we need to sync to
>> + *
>> + * Returns:
>> + * Negativ errno, 0 for success.
>> + */
>> +static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>> +				  void *owner, struct dma_fence *exclusive)
>> +{
>> +	struct amdgpu_bo *root = p->vm->root.base.bo;
>> +	unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
>> +	int r;
>> +
>> +	r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
>> +	if (r)
>> +		return r;
>> +
>> +	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
>> +	if (r)
>> +		return r;
>> +
>> +	r = amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.resv,
>> +			     owner, false);
>> +	if (r)
>> +		return r;
>> +
>> +	p->num_dw_left = ndw;
>> +	p->ib = &p->job->ibs[0];
> With p->job added, do we still need p->ib? We could just use
> &p->job->ibs[0] directly, which should perform the same or be more
> efficient since it's just a constant offset from p->job.
>
>
>> +	return 0;
>> +}
>> +
>> +/**
>> + * amdgpu_vm_sdma_commit - commit SDMA command submission
>> + *
>> + * @p: see amdgpu_vm_update_params definition
>> + * @fence: resulting fence
>> + *
>> + * Returns:
>> + * Negativ errno, 0 for success.
>> + */
>> +static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>> +				 struct dma_fence **fence)
>> +{
>> +	struct amdgpu_bo *root = p->vm->root.base.bo;
>> +	struct amdgpu_ring *ring;
>> +	struct dma_fence *f;
>> +	int r;
>> +
>> +	ring = container_of(p->vm->entity.rq->sched, struct amdgpu_ring, sched);
>> +
>> +	WARN_ON(p->ib->length_dw == 0);
>> +	amdgpu_ring_pad_ib(ring, p->ib);
>> +	WARN_ON(p->ib->length_dw > p->num_dw_left);
>> +	r = amdgpu_job_submit(p->job, &p->vm->entity,
>> +			      AMDGPU_FENCE_OWNER_VM, &f);
>> +	if (r)
>> +		goto error;
>> +
>> +	amdgpu_bo_fence(root, f, true);
>> +	if (fence)
>> +		swap(*fence, f);
>> +	dma_fence_put(f);
>> +	return 0;
>> +
>> +error:
>> +	amdgpu_job_free(p->job);
>> +	return r;
>> +}
>> +
>> +
>> +/**
>> + * amdgpu_vm_sdma_copy_ptes - copy the PTEs from mapping
>> + *
>> + * @p: see amdgpu_vm_update_params definition
>> + * @bo: PD/PT to update
>> + * @pe: addr of the page entry
>> + * @count: number of page entries to copy
>> + *
>> + * Traces the parameters and calls the DMA function to copy the PTEs.
>> + */
>> +static void amdgpu_vm_sdma_copy_ptes(struct amdgpu_vm_update_params *p,
>> +				     struct amdgpu_bo *bo, uint64_t pe,
>> +				     unsigned count)
>> +{
>> +	uint64_t src = p->ib->gpu_addr;
>> +
>> +	src += p->num_dw_left * 4;
>> +
>> +	pe += amdgpu_bo_gpu_offset(bo);
>> +	trace_amdgpu_vm_copy_ptes(pe, src, count);
>> +
>> +	amdgpu_vm_copy_pte(p->adev, p->ib, pe, src, count);
>> +}
>> +
>> +/**
>> + * amdgpu_vm_sdma_set_ptes - helper to call the right asic function
>> + *
>> + * @p: see amdgpu_vm_update_params definition
>> + * @bo: PD/PT to update
>> + * @pe: addr of the page entry
>> + * @addr: dst addr to write into pe
>> + * @count: number of page entries to update
>> + * @incr: increase next addr by incr bytes
>> + * @flags: hw access flags
>> + *
>> + * Traces the parameters and calls the right asic functions
>> + * to setup the page table using the DMA.
>> + */
>> +static void amdgpu_vm_sdma_set_ptes(struct amdgpu_vm_update_params *p,
>> +				    struct amdgpu_bo *bo, uint64_t pe,
>> +				    uint64_t addr, unsigned count,
>> +				    uint32_t incr, uint64_t flags)
>> +{
>> +	pe += amdgpu_bo_gpu_offset(bo);
>> +	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
>> +	if (count < 3) {
>> +		amdgpu_vm_write_pte(p->adev, p->ib, pe, addr | flags,
>> +				    count, incr);
>> +	} else {
>> +		amdgpu_vm_set_pte_pde(p->adev, p->ib, pe, addr,
>> +				      count, incr, flags);
>> +	}
>> +}
>> +
>> +/**
>> + * amdgpu_vm_sdma_update - execute VM update
>> + *
>> + * @p: see amdgpu_vm_update_params definition
>> + * @bo: PD/PT to update
>> + * @pe: addr of the page entry
>> + * @addr: dst addr to write into pe
>> + * @count: number of page entries to update
>> + * @incr: increase next addr by incr bytes
>> + * @flags: hw access flags
>> + *
>> + * Reserve space in the IB, setup mapping buffer on demand and write commands to
>> + * the IB.
>> + */
>> +static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
>> +				 struct amdgpu_bo *bo, uint64_t pe,
>> +				 uint64_t addr, unsigned count, uint32_t incr,
>> +				 uint64_t flags)
>> +{
>> +	unsigned int i, ndw, nptes;
>> +	uint64_t *pte;
>> +	int r;
>> +
>> +	do {
>> +		ndw = p->num_dw_left;
>> +		ndw -= p->ib->length_dw;
>> +
>> +		if (ndw < 32) {
>> +			r = amdgpu_vm_sdma_commit(p, NULL);
>> +			if (r)
>> +				return r;
>> +
>> +			/* estimate how many dw we need */
>> +			ndw = 32;
>> +			if (p->pages_addr)
>> +				ndw += count * 2;
>> +			ndw = max(ndw, AMDGPU_VM_SDMA_MIN_NUM_DW);
>> +			ndw = min(ndw, AMDGPU_VM_SDMA_MAX_NUM_DW);
>> +
>> +			r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
>> +			if (r)
>> +				return r;
>> +
>> +			p->num_dw_left = ndw;
>> +			p->ib = &p->job->ibs[0];
>> +		}
>> +
>> +		if (!p->pages_addr) {
>> +			/* set page commands needed */
>> +			if (bo->shadow)
>> +				amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
>> +							count, incr, flags);
>> +			amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
>> +						incr, flags);
>> +			return 0;
>> +		}
>> +
>> +		/* copy commands needed */
>> +		ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
>> +			(bo->shadow ? 2 : 1);
>> +
>> +		/* for padding */
>> +		ndw -= 7;
>> +
>> +		nptes = min(count, ndw / 2);
>> +
>> +		/* Put the PTEs at the end of the IB. */
>> +		p->num_dw_left -= nptes * 2;
>> +		pte = (uint64_t *)&(p->ib->ptr[p->num_dw_left]);
>> +		for (i = 0; i < nptes; ++i, addr += incr) {
>> +			pte[i] = amdgpu_vm_map_gart(p->pages_addr, addr);
>> +			pte[i] |= flags;
>> +		}
>> +
>> +		if (bo->shadow)
>> +			amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
>> +		amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
>> +
>> +		pe += nptes * 8;
>> +		count -= nptes;
>> +	} while (count);
>> +
>> +	return 0;
>> +}
>> +
>> +const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs = {
>> +	.prepare = amdgpu_vm_sdma_prepare,
>> +	.update = amdgpu_vm_sdma_update,
>> +	.commit = amdgpu_vm_sdma_commit
>> +};

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/8] drm/amdgpu: new VM update backends
       [not found]             ` <11141c5e-155f-79dc-b5bb-00da3b8608ce-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2019-03-25 20:53               ` Kuehling, Felix
  0 siblings, 0 replies; 12+ messages in thread
From: Kuehling, Felix @ 2019-03-25 20:53 UTC (permalink / raw)
  To: Koenig, Christian, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	Russell, Kent

On 2019-03-25 7:38 a.m., Christian König wrote:
> Am 20.03.19 um 12:57 schrieb Kuehling, Felix:
>> As far as I can tell, the whole series is a small cleanup and big
>> refactor to enable CPU clearing of PTs without a lot of ugliness or code
>> duplication.
>
> It's a bit more than that. Key point is that I can now easily add a 
> parameter for direct submission during page fault handling :)

You mean for working around problems with CPU page table updates from 
page faults, to force all such updates through SDMA?

Regards,
   Felix


>
> Christian.
>
>> It looks good to me. I haven't reviewed all the moved SDMA
>> update code to make sure it all works correctly, but at least the
>> prepare and commit functions look sane to me.
>>
>> For this patch I have a suggestion (inline) to remove params->ib, which
>> seems redundant with params->job. That would also ripple through the
>> remaining patches.
>>
>> Other than that, the series is Reviewed-by: Felix Kuehling
>> <Felix.Kuehling@amd.com>
>>
>> [+Kent], Look out for this patch series in an upcoming merge to
>> amd-kfd-staging. I don't think it'll cause conflicts, but has a risk of
>> regressions (like all big amdgpu_vm changes IME).
>>
>> Regards,
>>     Felix
>>
>> On 3/19/2019 8:44 AM, Christian König wrote:
>>> Separate out all functions for SDMA and CPU based page table
>>> updates into separate backends.
>>>
>>> This way we can keep most of the complexity of those from the
>>> core VM code.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/Makefile         |   3 +-
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |   7 +-
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  30 ++-
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 116 +++++++++
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 248 
>>> ++++++++++++++++++++
>>>    5 files changed, 401 insertions(+), 3 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>>>    create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> [snip]
>>> +/**
>>> + * amdgpu_vm_sdma_prepare - prepare SDMA command submission
>>> + *
>>> + * @p: see amdgpu_vm_update_params definition
>>> + * @owner: owner we need to sync to
>>> + * @exclusive: exclusive move fence we need to sync to
>>> + *
>>> + * Returns:
>>> + * Negativ errno, 0 for success.
>>> + */
>>> +static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>>> +                  void *owner, struct dma_fence *exclusive)
>>> +{
>>> +    struct amdgpu_bo *root = p->vm->root.base.bo;
>>> +    unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
>>> +    int r;
>>> +
>>> +    r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
>>> +    if (r)
>>> +        return r;
>>> +
>>> +    r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
>>> +    if (r)
>>> +        return r;
>>> +
>>> +    r = amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.resv,
>>> +                 owner, false);
>>> +    if (r)
>>> +        return r;
>>> +
>>> +    p->num_dw_left = ndw;
>>> +    p->ib = &p->job->ibs[0];
>> With p->job added, do we still need p->ib? We could just use
>> &p->job->ibs[0] directly, which should perform the same or be more
>> efficient since it's just a constant offset from p->job.
>>
>>
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * amdgpu_vm_sdma_commit - commit SDMA command submission
>>> + *
>>> + * @p: see amdgpu_vm_update_params definition
>>> + * @fence: resulting fence
>>> + *
>>> + * Returns:
>>> + * Negativ errno, 0 for success.
>>> + */
>>> +static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>>> +                 struct dma_fence **fence)
>>> +{
>>> +    struct amdgpu_bo *root = p->vm->root.base.bo;
>>> +    struct amdgpu_ring *ring;
>>> +    struct dma_fence *f;
>>> +    int r;
>>> +
>>> +    ring = container_of(p->vm->entity.rq->sched, struct 
>>> amdgpu_ring, sched);
>>> +
>>> +    WARN_ON(p->ib->length_dw == 0);
>>> +    amdgpu_ring_pad_ib(ring, p->ib);
>>> +    WARN_ON(p->ib->length_dw > p->num_dw_left);
>>> +    r = amdgpu_job_submit(p->job, &p->vm->entity,
>>> +                  AMDGPU_FENCE_OWNER_VM, &f);
>>> +    if (r)
>>> +        goto error;
>>> +
>>> +    amdgpu_bo_fence(root, f, true);
>>> +    if (fence)
>>> +        swap(*fence, f);
>>> +    dma_fence_put(f);
>>> +    return 0;
>>> +
>>> +error:
>>> +    amdgpu_job_free(p->job);
>>> +    return r;
>>> +}
>>> +
>>> +
>>> +/**
>>> + * amdgpu_vm_sdma_copy_ptes - copy the PTEs from mapping
>>> + *
>>> + * @p: see amdgpu_vm_update_params definition
>>> + * @bo: PD/PT to update
>>> + * @pe: addr of the page entry
>>> + * @count: number of page entries to copy
>>> + *
>>> + * Traces the parameters and calls the DMA function to copy the PTEs.
>>> + */
>>> +static void amdgpu_vm_sdma_copy_ptes(struct amdgpu_vm_update_params 
>>> *p,
>>> +                     struct amdgpu_bo *bo, uint64_t pe,
>>> +                     unsigned count)
>>> +{
>>> +    uint64_t src = p->ib->gpu_addr;
>>> +
>>> +    src += p->num_dw_left * 4;
>>> +
>>> +    pe += amdgpu_bo_gpu_offset(bo);
>>> +    trace_amdgpu_vm_copy_ptes(pe, src, count);
>>> +
>>> +    amdgpu_vm_copy_pte(p->adev, p->ib, pe, src, count);
>>> +}
>>> +
>>> +/**
>>> + * amdgpu_vm_sdma_set_ptes - helper to call the right asic function
>>> + *
>>> + * @p: see amdgpu_vm_update_params definition
>>> + * @bo: PD/PT to update
>>> + * @pe: addr of the page entry
>>> + * @addr: dst addr to write into pe
>>> + * @count: number of page entries to update
>>> + * @incr: increase next addr by incr bytes
>>> + * @flags: hw access flags
>>> + *
>>> + * Traces the parameters and calls the right asic functions
>>> + * to setup the page table using the DMA.
>>> + */
>>> +static void amdgpu_vm_sdma_set_ptes(struct amdgpu_vm_update_params *p,
>>> +                    struct amdgpu_bo *bo, uint64_t pe,
>>> +                    uint64_t addr, unsigned count,
>>> +                    uint32_t incr, uint64_t flags)
>>> +{
>>> +    pe += amdgpu_bo_gpu_offset(bo);
>>> +    trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags);
>>> +    if (count < 3) {
>>> +        amdgpu_vm_write_pte(p->adev, p->ib, pe, addr | flags,
>>> +                    count, incr);
>>> +    } else {
>>> +        amdgpu_vm_set_pte_pde(p->adev, p->ib, pe, addr,
>>> +                      count, incr, flags);
>>> +    }
>>> +}
>>> +
>>> +/**
>>> + * amdgpu_vm_sdma_update - execute VM update
>>> + *
>>> + * @p: see amdgpu_vm_update_params definition
>>> + * @bo: PD/PT to update
>>> + * @pe: addr of the page entry
>>> + * @addr: dst addr to write into pe
>>> + * @count: number of page entries to update
>>> + * @incr: increase next addr by incr bytes
>>> + * @flags: hw access flags
>>> + *
>>> + * Reserve space in the IB, setup mapping buffer on demand and 
>>> write commands to
>>> + * the IB.
>>> + */
>>> +static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
>>> +                 struct amdgpu_bo *bo, uint64_t pe,
>>> +                 uint64_t addr, unsigned count, uint32_t incr,
>>> +                 uint64_t flags)
>>> +{
>>> +    unsigned int i, ndw, nptes;
>>> +    uint64_t *pte;
>>> +    int r;
>>> +
>>> +    do {
>>> +        ndw = p->num_dw_left;
>>> +        ndw -= p->ib->length_dw;
>>> +
>>> +        if (ndw < 32) {
>>> +            r = amdgpu_vm_sdma_commit(p, NULL);
>>> +            if (r)
>>> +                return r;
>>> +
>>> +            /* estimate how many dw we need */
>>> +            ndw = 32;
>>> +            if (p->pages_addr)
>>> +                ndw += count * 2;
>>> +            ndw = max(ndw, AMDGPU_VM_SDMA_MIN_NUM_DW);
>>> +            ndw = min(ndw, AMDGPU_VM_SDMA_MAX_NUM_DW);
>>> +
>>> +            r = amdgpu_job_alloc_with_ib(p->adev, ndw * 4, &p->job);
>>> +            if (r)
>>> +                return r;
>>> +
>>> +            p->num_dw_left = ndw;
>>> +            p->ib = &p->job->ibs[0];
>>> +        }
>>> +
>>> +        if (!p->pages_addr) {
>>> +            /* set page commands needed */
>>> +            if (bo->shadow)
>>> +                amdgpu_vm_sdma_set_ptes(p, bo->shadow, pe, addr,
>>> +                            count, incr, flags);
>>> +            amdgpu_vm_sdma_set_ptes(p, bo, pe, addr, count,
>>> +                        incr, flags);
>>> +            return 0;
>>> +        }
>>> +
>>> +        /* copy commands needed */
>>> +        ndw -= p->adev->vm_manager.vm_pte_funcs->copy_pte_num_dw *
>>> +            (bo->shadow ? 2 : 1);
>>> +
>>> +        /* for padding */
>>> +        ndw -= 7;
>>> +
>>> +        nptes = min(count, ndw / 2);
>>> +
>>> +        /* Put the PTEs at the end of the IB. */
>>> +        p->num_dw_left -= nptes * 2;
>>> +        pte = (uint64_t *)&(p->ib->ptr[p->num_dw_left]);
>>> +        for (i = 0; i < nptes; ++i, addr += incr) {
>>> +            pte[i] = amdgpu_vm_map_gart(p->pages_addr, addr);
>>> +            pte[i] |= flags;
>>> +        }
>>> +
>>> +        if (bo->shadow)
>>> +            amdgpu_vm_sdma_copy_ptes(p, bo->shadow, pe, nptes);
>>> +        amdgpu_vm_sdma_copy_ptes(p, bo, pe, nptes);
>>> +
>>> +        pe += nptes * 8;
>>> +        count -= nptes;
>>> +    } while (count);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs = {
>>> +    .prepare = amdgpu_vm_sdma_prepare,
>>> +    .update = amdgpu_vm_sdma_update,
>>> +    .commit = amdgpu_vm_sdma_commit
>>> +};
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-03-25 20:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-19 12:44 [PATCH 1/8] drm/amdgpu: remove some unused VM defines Christian König
     [not found] ` <20190319124411.1563-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
2019-03-19 12:44   ` [PATCH 2/8] drm/amdgpu: always set and check dma addresses in the VM code Christian König
2019-03-19 12:44   ` [PATCH 3/8] drm/amdgpu: move and rename amdgpu_pte_update_params Christian König
2019-03-19 12:44   ` [PATCH 4/8] drm/amdgpu: reserve less memory for PDE updates Christian König
2019-03-19 12:44   ` [PATCH 5/8] drm/amdgpu: new VM update backends Christian König
     [not found]     ` <20190319124411.1563-5-christian.koenig-5C7GfCeVMHo@public.gmane.org>
2019-03-20 11:57       ` Kuehling, Felix
     [not found]         ` <153c002a-5454-ad17-bd3b-03685867844d-5C7GfCeVMHo@public.gmane.org>
2019-03-25 11:38           ` Christian König
     [not found]             ` <11141c5e-155f-79dc-b5bb-00da3b8608ce-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2019-03-25 20:53               ` Kuehling, Felix
2019-03-19 12:44   ` [PATCH 6/8] drm/amdgpu: use the new VM backend for PDEs Christian König
2019-03-19 12:44   ` [PATCH 7/8] drm/amdgpu: use the new VM backend for PTEs Christian König
2019-03-19 12:44   ` [PATCH 8/8] drm/amdgpu: use the new VM backend for clears Christian König
     [not found]     ` <20190319124411.1563-8-christian.koenig-5C7GfCeVMHo@public.gmane.org>
2019-03-20  4:27       ` zhoucm1

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.