[PATCH 1/3] drm/amdgpu: cleanup adjust_mc_addr handling v2

[PATCH 1/3] drm/amdgpu: cleanup adjust_mc_addr handling v2

[Christian König]

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

Rename adjust_mc_addr to get_vm_pde and check the address bits in one place.

v2: handle vcn as well, keep setting the valid bit manually,
    add a BUG_ON() for GMC v6, v7 and v8 as well.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h    |  5 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 26 +++++++++-----------------
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  |  6 ++----
 drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c  |  7 +++++++
 drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  |  9 ++++++++-
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  |  9 ++++++++-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 10 ++++++----
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c |  6 ++----
 drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c  | 12 ++++--------
 drivers/gpu/drm/amd/amdgpu/vce_v4_0.c  |  6 ++----
 drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c  |  5 ++---
 11 files changed, 53 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index fadeb55..bc089eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -309,8 +309,8 @@ struct amdgpu_gart_funcs {
 	/* set pte flags based per asic */
 	uint64_t (*get_vm_pte_flags)(struct amdgpu_device *adev,
 				     uint32_t flags);
-	/* adjust mc addr in fb for APU case */
-	u64 (*adjust_mc_addr)(struct amdgpu_device *adev, u64 addr);
+	/* get the pde for a given mc addr */
+	u64 (*get_vm_pde)(struct amdgpu_device *adev, u64 addr);
 	uint32_t (*get_invalidate_req)(unsigned int vm_id);
 };
 
@@ -1816,6 +1816,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
 #define amdgpu_asic_get_config_memsize(adev) (adev)->asic_funcs->get_config_memsize((adev))
 #define amdgpu_gart_flush_gpu_tlb(adev, vmid) (adev)->gart.gart_funcs->flush_gpu_tlb((adev), (vmid))
 #define amdgpu_gart_set_pte_pde(adev, pt, idx, addr, flags) (adev)->gart.gart_funcs->set_pte_pde((adev), (pt), (idx), (addr), (flags))
+#define amdgpu_gart_get_vm_pde(adev, addr) (adev)->gart.gart_funcs->get_vm_pde((adev), (addr))
 #define amdgpu_vm_copy_pte(adev, ib, pe, src, count) ((adev)->vm_manager.vm_pte_funcs->copy_pte((ib), (pe), (src), (count)))
 #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)))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 88420dc..344f943 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -682,16 +682,6 @@ static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
 	return false;
 }
 
-static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
-{
-	u64 addr = mc_addr;
-
-	if (adev->gart.gart_funcs->adjust_mc_addr)
-		addr = adev->gart.gart_funcs->adjust_mc_addr(adev, addr);
-
-	return addr;
-}
-
 bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
 				  struct amdgpu_job *job)
 {
@@ -1034,18 +1024,18 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 		    (count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
 
 			if (count) {
-				uint64_t pt_addr =
-					amdgpu_vm_adjust_mc_addr(adev, last_pt);
+				uint64_t entry;
 
+				entry = amdgpu_gart_get_vm_pde(adev, last_pt);
 				if (shadow)
 					amdgpu_vm_do_set_ptes(&params,
 							      last_shadow,
-							      pt_addr, count,
+							      entry, count,
 							      incr,
 							      AMDGPU_PTE_VALID);
 
 				amdgpu_vm_do_set_ptes(&params, last_pde,
-						      pt_addr, count, incr,
+						      entry, count, incr,
 						      AMDGPU_PTE_VALID);
 			}
 
@@ -1059,13 +1049,15 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 	}
 
 	if (count) {
-		uint64_t pt_addr = amdgpu_vm_adjust_mc_addr(adev, last_pt);
+		uint64_t entry;
+
+		entry = amdgpu_gart_get_vm_pde(adev, last_pt);
 
 		if (vm->root.bo->shadow)
-			amdgpu_vm_do_set_ptes(&params, last_shadow, pt_addr,
+			amdgpu_vm_do_set_ptes(&params, last_shadow, entry,
 					      count, incr, AMDGPU_PTE_VALID);
 
-		amdgpu_vm_do_set_ptes(&params, last_pde, pt_addr,
+		amdgpu_vm_do_set_ptes(&params, last_pde, entry,
 				      count, incr, AMDGPU_PTE_VALID);
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 6dc75d2..e08a232 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -3759,10 +3759,8 @@ static void gfx_v9_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
 	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
 	unsigned eng = ring->vm_inv_eng;
 
-	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
-	pd_addr = pd_addr | 0x1; /* valid bit */
-	/* now only use physical base address of PDE and valid */
-	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
+	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
+	pd_addr |= AMDGPU_PTE_VALID;
 
 	gfx_v9_0_write_data_to_reg(ring, usepfp, true,
 				   hub->ctx0_ptb_addr_lo32 + (2 * vm_id),
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 1e6263a..d5f3d873 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -395,6 +395,12 @@ static uint64_t gmc_v6_0_get_vm_pte_flags(struct amdgpu_device *adev,
 	return pte_flag;
 }
 
+static uint64_t gmc_v6_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
+{
+	BUG_ON(addr & 0xFFFFF0000000FFFULL);
+	return addr;
+}
+
 static void gmc_v6_0_set_fault_enable_default(struct amdgpu_device *adev,
 					      bool value)
 {
@@ -1127,6 +1133,7 @@ static const struct amdgpu_gart_funcs gmc_v6_0_gart_funcs = {
 	.flush_gpu_tlb = gmc_v6_0_gart_flush_gpu_tlb,
 	.set_pte_pde = gmc_v6_0_gart_set_pte_pde,
 	.set_prt = gmc_v6_0_set_prt,
+	.get_vm_pde = gmc_v6_0_get_vm_pde,
 	.get_vm_pte_flags = gmc_v6_0_get_vm_pte_flags
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 967505b..4f140e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -472,6 +472,12 @@ static uint64_t gmc_v7_0_get_vm_pte_flags(struct amdgpu_device *adev,
 	return pte_flag;
 }
 
+static uint64_t gmc_v7_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
+{
+	BUG_ON(addr & 0xFFFFF0000000FFFULL);
+	return addr;
+}
+
 /**
  * gmc_v8_0_set_fault_enable_default - update VM fault handling
  *
@@ -1293,7 +1299,8 @@ static const struct amdgpu_gart_funcs gmc_v7_0_gart_funcs = {
 	.flush_gpu_tlb = gmc_v7_0_gart_flush_gpu_tlb,
 	.set_pte_pde = gmc_v7_0_gart_set_pte_pde,
 	.set_prt = gmc_v7_0_set_prt,
-	.get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags
+	.get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags,
+	.get_vm_pde = gmc_v7_0_get_vm_pde
 };
 
 static const struct amdgpu_irq_src_funcs gmc_v7_0_irq_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 3b5ea0f..f05c034 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -656,6 +656,12 @@ static uint64_t gmc_v8_0_get_vm_pte_flags(struct amdgpu_device *adev,
 	return pte_flag;
 }
 
+static uint64_t gmc_v8_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
+{
+	BUG_ON(addr & 0xFFF0000000000FFFULL);
+	return addr;
+}
+
 /**
  * gmc_v8_0_set_fault_enable_default - update VM fault handling
  *
@@ -1612,7 +1618,8 @@ static const struct amdgpu_gart_funcs gmc_v8_0_gart_funcs = {
 	.flush_gpu_tlb = gmc_v8_0_gart_flush_gpu_tlb,
 	.set_pte_pde = gmc_v8_0_gart_set_pte_pde,
 	.set_prt = gmc_v8_0_set_prt,
-	.get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags
+	.get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags,
+	.get_vm_pde = gmc_v8_0_get_vm_pde
 };
 
 static const struct amdgpu_irq_src_funcs gmc_v8_0_irq_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 19e1027..047b1a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -358,17 +358,19 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
 	return pte_flag;
 }
 
-static u64 gmc_v9_0_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
+static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
 {
-	return adev->vm_manager.vram_base_offset + mc_addr - adev->mc.vram_start;
+	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
+	BUG_ON(addr & 0xFFFF00000000003FULL);
+	return addr;
 }
 
 static const struct amdgpu_gart_funcs gmc_v9_0_gart_funcs = {
 	.flush_gpu_tlb = gmc_v9_0_gart_flush_gpu_tlb,
 	.set_pte_pde = gmc_v9_0_gart_set_pte_pde,
-	.get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
-	.adjust_mc_addr = gmc_v9_0_adjust_mc_addr,
 	.get_invalidate_req = gmc_v9_0_get_invalidate_req,
+	.get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
+	.get_vm_pde = gmc_v9_0_get_vm_pde
 };
 
 static void gmc_v9_0_set_gart_funcs(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 91cf7e6..fb6f4b3 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -1143,10 +1143,8 @@ static void sdma_v4_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
 	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
 	unsigned eng = ring->vm_inv_eng;
 
-	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
-	pd_addr = pd_addr | 0x1; /* valid bit */
-	/* now only use physical base address of PDE and valid */
-	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
+	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
+	pd_addr |= AMDGPU_PTE_VALID;
 
 	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_SRBM_WRITE) |
 			  SDMA_PKT_SRBM_WRITE_HEADER_BYTE_EN(0xf));
diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
index 22f42f3..1997ec8 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
@@ -1316,10 +1316,8 @@ static void uvd_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
 	uint32_t data0, data1, mask;
 	unsigned eng = ring->vm_inv_eng;
 
-	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
-	pd_addr = pd_addr | 0x1; /* valid bit */
-	/* now only use physical base address of PDE and valid */
-	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
+	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
+	pd_addr |= AMDGPU_PTE_VALID;
 
 	data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
 	data1 = upper_32_bits(pd_addr);
@@ -1358,10 +1356,8 @@ static void uvd_v7_0_enc_ring_emit_vm_flush(struct amdgpu_ring *ring,
 	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
 	unsigned eng = ring->vm_inv_eng;
 
-	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
-	pd_addr = pd_addr | 0x1; /* valid bit */
-	/* now only use physical base address of PDE and valid */
-	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
+	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
+	pd_addr |= AMDGPU_PTE_VALID;
 
 	amdgpu_ring_write(ring, HEVC_ENC_CMD_REG_WRITE);
 	amdgpu_ring_write(ring,	(hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
index 07b2ac7..8dbd0b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
@@ -926,10 +926,8 @@ static void vce_v4_0_emit_vm_flush(struct amdgpu_ring *ring,
 	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
 	unsigned eng = ring->vm_inv_eng;
 
-	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
-	pd_addr = pd_addr | 0x1; /* valid bit */
-	/* now only use physical base address of PDE and valid */
-	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
+	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
+	pd_addr |= AMDGPU_PTE_VALID;
 
 	amdgpu_ring_write(ring, VCE_CMD_REG_WRITE);
 	amdgpu_ring_write(ring,	(hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
index 94104a9..bbbe62f 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
@@ -882,9 +882,8 @@ static void vcn_v1_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
 	uint32_t data0, data1, mask;
 	unsigned eng = ring->vm_inv_eng;
 
-	pd_addr = pd_addr | 0x1; /* valid bit */
-	/* now only use physical base address of PDE and valid */
-	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
+	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
+	pd_addr |= AMDGPU_PTE_VALID;
 
 	data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
 	data1 = upper_32_bits(pd_addr);
-- 
2.7.4

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

[PATCH 2/3] drm/amdgpu: add some extra VM error handling

[Christian König]

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

If updating the PDs fails we now invalidate all entries to try again later.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 344f943..b877f9f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1105,6 +1105,32 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 }
 
 /*
+ * amdgpu_vm_invalidate_level - mark all PD levels as invalid
+ *
+ * @parent: parent PD
+ *
+ * Mark all PD level as invalid after an error.
+ */
+static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
+{
+	unsigned pt_idx;
+
+	/*
+	 * Recurse into the subdirectories. This recursion is harmless because
+	 * we only have a maximum of 5 layers.
+	 */
+	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
+		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
+
+		if (!entry->bo)
+			continue;
+
+		entry->addr = ~0ULL;
+		amdgpu_vm_invalidate_level(entry);
+	}
+}
+
+/*
  * amdgpu_vm_update_directories - make sure that all directories are valid
  *
  * @adev: amdgpu_device pointer
@@ -1116,7 +1142,13 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 				 struct amdgpu_vm *vm)
 {
-	return amdgpu_vm_update_level(adev, vm, &vm->root, 0);
+	int r;
+
+	r = amdgpu_vm_update_level(adev, vm, &vm->root, 0);
+	if (r)
+		amdgpu_vm_invalidate_level(&vm->root);
+
+	return r;
 }
 
 /**
-- 
2.7.4

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

[PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Christian König]

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

I always wondered why this code uses the MC address. Now it came to me that
this is actually a bug and only works by coincident because we used to have
VRAM mapped at zero.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index e4c5d31..ba6eb73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
 	}
 
 	if (p->job->vm) {
-		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
+		p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
 
 		r = amdgpu_bo_vm_update_pte(p);
 		if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b877f9f3..7256fcc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 				return r;
 		}
 
-		pt = amdgpu_bo_gpu_offset(bo);
+		pt = bo->tbo.mem.start << PAGE_SHIFT;
 		if (parent->entries[pt_idx].addr == pt)
 			continue;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 047b1a7..63cb573 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
 
 static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
 {
-	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
+	addr = adev->vm_manager.vram_base_offset + addr;
 	BUG_ON(addr & 0xFFFF00000000003FULL);
 	return addr;
 }
-- 
2.7.4

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

Re: [PATCH 1/3] drm/amdgpu: cleanup adjust_mc_addr handling v2

[zhoucm1]

On 2017年05月15日 19:57, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Rename adjust_mc_addr to get_vm_pde and check the address bits in one place.
>
> v2: handle vcn as well, keep setting the valid bit manually,
>      add a BUG_ON() for GMC v6, v7 and v8 as well.
If that way, I didn't see how meaningful compared previous, especially 
for old asics, they don't need to add callbacks.
Renaming is ok to me, and previous name is fine as well.

Regards,
David Zhou
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h    |  5 +++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 26 +++++++++-----------------
>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  |  6 ++----
>   drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c  |  7 +++++++
>   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  |  9 ++++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  |  9 ++++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 10 ++++++----
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c |  6 ++----
>   drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c  | 12 ++++--------
>   drivers/gpu/drm/amd/amdgpu/vce_v4_0.c  |  6 ++----
>   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c  |  5 ++---
>   11 files changed, 53 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index fadeb55..bc089eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -309,8 +309,8 @@ struct amdgpu_gart_funcs {
>   	/* set pte flags based per asic */
>   	uint64_t (*get_vm_pte_flags)(struct amdgpu_device *adev,
>   				     uint32_t flags);
> -	/* adjust mc addr in fb for APU case */
> -	u64 (*adjust_mc_addr)(struct amdgpu_device *adev, u64 addr);
> +	/* get the pde for a given mc addr */
> +	u64 (*get_vm_pde)(struct amdgpu_device *adev, u64 addr);
>   	uint32_t (*get_invalidate_req)(unsigned int vm_id);
>   };
>   
> @@ -1816,6 +1816,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
>   #define amdgpu_asic_get_config_memsize(adev) (adev)->asic_funcs->get_config_memsize((adev))
>   #define amdgpu_gart_flush_gpu_tlb(adev, vmid) (adev)->gart.gart_funcs->flush_gpu_tlb((adev), (vmid))
>   #define amdgpu_gart_set_pte_pde(adev, pt, idx, addr, flags) (adev)->gart.gart_funcs->set_pte_pde((adev), (pt), (idx), (addr), (flags))
> +#define amdgpu_gart_get_vm_pde(adev, addr) (adev)->gart.gart_funcs->get_vm_pde((adev), (addr))
>   #define amdgpu_vm_copy_pte(adev, ib, pe, src, count) ((adev)->vm_manager.vm_pte_funcs->copy_pte((ib), (pe), (src), (count)))
>   #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)))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 88420dc..344f943 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -682,16 +682,6 @@ static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
>   	return false;
>   }
>   
> -static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
> -{
> -	u64 addr = mc_addr;
> -
> -	if (adev->gart.gart_funcs->adjust_mc_addr)
> -		addr = adev->gart.gart_funcs->adjust_mc_addr(adev, addr);
> -
> -	return addr;
> -}
> -
>   bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
>   				  struct amdgpu_job *job)
>   {
> @@ -1034,18 +1024,18 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   		    (count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
>   
>   			if (count) {
> -				uint64_t pt_addr =
> -					amdgpu_vm_adjust_mc_addr(adev, last_pt);
> +				uint64_t entry;
>   
> +				entry = amdgpu_gart_get_vm_pde(adev, last_pt);
>   				if (shadow)
>   					amdgpu_vm_do_set_ptes(&params,
>   							      last_shadow,
> -							      pt_addr, count,
> +							      entry, count,
>   							      incr,
>   							      AMDGPU_PTE_VALID);
>   
>   				amdgpu_vm_do_set_ptes(&params, last_pde,
> -						      pt_addr, count, incr,
> +						      entry, count, incr,
>   						      AMDGPU_PTE_VALID);
>   			}
>   
> @@ -1059,13 +1049,15 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   	}
>   
>   	if (count) {
> -		uint64_t pt_addr = amdgpu_vm_adjust_mc_addr(adev, last_pt);
> +		uint64_t entry;
> +
> +		entry = amdgpu_gart_get_vm_pde(adev, last_pt);
>   
>   		if (vm->root.bo->shadow)
> -			amdgpu_vm_do_set_ptes(&params, last_shadow, pt_addr,
> +			amdgpu_vm_do_set_ptes(&params, last_shadow, entry,
>   					      count, incr, AMDGPU_PTE_VALID);
>   
> -		amdgpu_vm_do_set_ptes(&params, last_pde, pt_addr,
> +		amdgpu_vm_do_set_ptes(&params, last_pde, entry,
>   				      count, incr, AMDGPU_PTE_VALID);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 6dc75d2..e08a232 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -3759,10 +3759,8 @@ static void gfx_v9_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	gfx_v9_0_write_data_to_reg(ring, usepfp, true,
>   				   hub->ctx0_ptb_addr_lo32 + (2 * vm_id),
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> index 1e6263a..d5f3d873 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> @@ -395,6 +395,12 @@ static uint64_t gmc_v6_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v6_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
> +{
> +	BUG_ON(addr & 0xFFFFF0000000FFFULL);
> +	return addr;
> +}
> +
>   static void gmc_v6_0_set_fault_enable_default(struct amdgpu_device *adev,
>   					      bool value)
>   {
> @@ -1127,6 +1133,7 @@ static const struct amdgpu_gart_funcs gmc_v6_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v6_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v6_0_gart_set_pte_pde,
>   	.set_prt = gmc_v6_0_set_prt,
> +	.get_vm_pde = gmc_v6_0_get_vm_pde,
>   	.get_vm_pte_flags = gmc_v6_0_get_vm_pte_flags
>   };
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index 967505b..4f140e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -472,6 +472,12 @@ static uint64_t gmc_v7_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v7_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
> +{
> +	BUG_ON(addr & 0xFFFFF0000000FFFULL);
> +	return addr;
> +}
> +
>   /**
>    * gmc_v8_0_set_fault_enable_default - update VM fault handling
>    *
> @@ -1293,7 +1299,8 @@ static const struct amdgpu_gart_funcs gmc_v7_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v7_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v7_0_gart_set_pte_pde,
>   	.set_prt = gmc_v7_0_set_prt,
> -	.get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags
> +	.get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v7_0_get_vm_pde
>   };
>   
>   static const struct amdgpu_irq_src_funcs gmc_v7_0_irq_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 3b5ea0f..f05c034 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -656,6 +656,12 @@ static uint64_t gmc_v8_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v8_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
> +{
> +	BUG_ON(addr & 0xFFF0000000000FFFULL);
> +	return addr;
> +}
> +
>   /**
>    * gmc_v8_0_set_fault_enable_default - update VM fault handling
>    *
> @@ -1612,7 +1618,8 @@ static const struct amdgpu_gart_funcs gmc_v8_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v8_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v8_0_gart_set_pte_pde,
>   	.set_prt = gmc_v8_0_set_prt,
> -	.get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags
> +	.get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v8_0_get_vm_pde
>   };
>   
>   static const struct amdgpu_irq_src_funcs gmc_v8_0_irq_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 19e1027..047b1a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -358,17 +358,19 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> -static u64 gmc_v9_0_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
> +static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>   {
> -	return adev->vm_manager.vram_base_offset + mc_addr - adev->mc.vram_start;
> +	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> +	BUG_ON(addr & 0xFFFF00000000003FULL);
> +	return addr;
>   }
>   
>   static const struct amdgpu_gart_funcs gmc_v9_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v9_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v9_0_gart_set_pte_pde,
> -	.get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
> -	.adjust_mc_addr = gmc_v9_0_adjust_mc_addr,
>   	.get_invalidate_req = gmc_v9_0_get_invalidate_req,
> +	.get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v9_0_get_vm_pde
>   };
>   
>   static void gmc_v9_0_set_gart_funcs(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 91cf7e6..fb6f4b3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -1143,10 +1143,8 @@ static void sdma_v4_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_SRBM_WRITE) |
>   			  SDMA_PKT_SRBM_WRITE_HEADER_BYTE_EN(0xf));
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> index 22f42f3..1997ec8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> @@ -1316,10 +1316,8 @@ static void uvd_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t data0, data1, mask;
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
>   	data1 = upper_32_bits(pd_addr);
> @@ -1358,10 +1356,8 @@ static void uvd_v7_0_enc_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	amdgpu_ring_write(ring, HEVC_ENC_CMD_REG_WRITE);
>   	amdgpu_ring_write(ring,	(hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> index 07b2ac7..8dbd0b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> @@ -926,10 +926,8 @@ static void vce_v4_0_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	amdgpu_ring_write(ring, VCE_CMD_REG_WRITE);
>   	amdgpu_ring_write(ring,	(hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 94104a9..bbbe62f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -882,9 +882,8 @@ static void vcn_v1_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t data0, data1, mask;
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
>   	data1 = upper_32_bits(pd_addr);

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

Re: [PATCH 1/3] drm/amdgpu: cleanup adjust_mc_addr handling v2

[zhoucm1]

On 2017年05月15日 19:57, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Rename adjust_mc_addr to get_vm_pde and check the address bits in one place.
>
> v2: handle vcn as well, keep setting the valid bit manually,
>      add a BUG_ON() for GMC v6, v7 and v8 as well.
If that way, I didn't see how meaningful compared previous, especially 
for old asics, they don't need to add callbacks.
Renaming is ok to me, and previous name is fine as well.

Regards,
David Zhou
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h    |  5 +++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 26 +++++++++-----------------
>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  |  6 ++----
>   drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c  |  7 +++++++
>   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  |  9 ++++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  |  9 ++++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 10 ++++++----
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c |  6 ++----
>   drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c  | 12 ++++--------
>   drivers/gpu/drm/amd/amdgpu/vce_v4_0.c  |  6 ++----
>   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c  |  5 ++---
>   11 files changed, 53 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index fadeb55..bc089eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -309,8 +309,8 @@ struct amdgpu_gart_funcs {
>   	/* set pte flags based per asic */
>   	uint64_t (*get_vm_pte_flags)(struct amdgpu_device *adev,
>   				     uint32_t flags);
> -	/* adjust mc addr in fb for APU case */
> -	u64 (*adjust_mc_addr)(struct amdgpu_device *adev, u64 addr);
> +	/* get the pde for a given mc addr */
> +	u64 (*get_vm_pde)(struct amdgpu_device *adev, u64 addr);
>   	uint32_t (*get_invalidate_req)(unsigned int vm_id);
>   };
>   
> @@ -1816,6 +1816,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)
>   #define amdgpu_asic_get_config_memsize(adev) (adev)->asic_funcs->get_config_memsize((adev))
>   #define amdgpu_gart_flush_gpu_tlb(adev, vmid) (adev)->gart.gart_funcs->flush_gpu_tlb((adev), (vmid))
>   #define amdgpu_gart_set_pte_pde(adev, pt, idx, addr, flags) (adev)->gart.gart_funcs->set_pte_pde((adev), (pt), (idx), (addr), (flags))
> +#define amdgpu_gart_get_vm_pde(adev, addr) (adev)->gart.gart_funcs->get_vm_pde((adev), (addr))
>   #define amdgpu_vm_copy_pte(adev, ib, pe, src, count) ((adev)->vm_manager.vm_pte_funcs->copy_pte((ib), (pe), (src), (count)))
>   #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)))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 88420dc..344f943 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -682,16 +682,6 @@ static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring)
>   	return false;
>   }
>   
> -static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
> -{
> -	u64 addr = mc_addr;
> -
> -	if (adev->gart.gart_funcs->adjust_mc_addr)
> -		addr = adev->gart.gart_funcs->adjust_mc_addr(adev, addr);
> -
> -	return addr;
> -}
> -
>   bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
>   				  struct amdgpu_job *job)
>   {
> @@ -1034,18 +1024,18 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   		    (count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
>   
>   			if (count) {
> -				uint64_t pt_addr =
> -					amdgpu_vm_adjust_mc_addr(adev, last_pt);
> +				uint64_t entry;
>   
> +				entry = amdgpu_gart_get_vm_pde(adev, last_pt);
>   				if (shadow)
>   					amdgpu_vm_do_set_ptes(&params,
>   							      last_shadow,
> -							      pt_addr, count,
> +							      entry, count,
>   							      incr,
>   							      AMDGPU_PTE_VALID);
>   
>   				amdgpu_vm_do_set_ptes(&params, last_pde,
> -						      pt_addr, count, incr,
> +						      entry, count, incr,
>   						      AMDGPU_PTE_VALID);
>   			}
>   
> @@ -1059,13 +1049,15 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   	}
>   
>   	if (count) {
> -		uint64_t pt_addr = amdgpu_vm_adjust_mc_addr(adev, last_pt);
> +		uint64_t entry;
> +
> +		entry = amdgpu_gart_get_vm_pde(adev, last_pt);
>   
>   		if (vm->root.bo->shadow)
> -			amdgpu_vm_do_set_ptes(&params, last_shadow, pt_addr,
> +			amdgpu_vm_do_set_ptes(&params, last_shadow, entry,
>   					      count, incr, AMDGPU_PTE_VALID);
>   
> -		amdgpu_vm_do_set_ptes(&params, last_pde, pt_addr,
> +		amdgpu_vm_do_set_ptes(&params, last_pde, entry,
>   				      count, incr, AMDGPU_PTE_VALID);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 6dc75d2..e08a232 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -3759,10 +3759,8 @@ static void gfx_v9_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	gfx_v9_0_write_data_to_reg(ring, usepfp, true,
>   				   hub->ctx0_ptb_addr_lo32 + (2 * vm_id),
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> index 1e6263a..d5f3d873 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> @@ -395,6 +395,12 @@ static uint64_t gmc_v6_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v6_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
> +{
> +	BUG_ON(addr & 0xFFFFF0000000FFFULL);
> +	return addr;
> +}
> +
>   static void gmc_v6_0_set_fault_enable_default(struct amdgpu_device *adev,
>   					      bool value)
>   {
> @@ -1127,6 +1133,7 @@ static const struct amdgpu_gart_funcs gmc_v6_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v6_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v6_0_gart_set_pte_pde,
>   	.set_prt = gmc_v6_0_set_prt,
> +	.get_vm_pde = gmc_v6_0_get_vm_pde,
>   	.get_vm_pte_flags = gmc_v6_0_get_vm_pte_flags
>   };
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index 967505b..4f140e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -472,6 +472,12 @@ static uint64_t gmc_v7_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v7_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
> +{
> +	BUG_ON(addr & 0xFFFFF0000000FFFULL);
> +	return addr;
> +}
> +
>   /**
>    * gmc_v8_0_set_fault_enable_default - update VM fault handling
>    *
> @@ -1293,7 +1299,8 @@ static const struct amdgpu_gart_funcs gmc_v7_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v7_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v7_0_gart_set_pte_pde,
>   	.set_prt = gmc_v7_0_set_prt,
> -	.get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags
> +	.get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v7_0_get_vm_pde
>   };
>   
>   static const struct amdgpu_irq_src_funcs gmc_v7_0_irq_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 3b5ea0f..f05c034 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -656,6 +656,12 @@ static uint64_t gmc_v8_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v8_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
> +{
> +	BUG_ON(addr & 0xFFF0000000000FFFULL);
> +	return addr;
> +}
> +
>   /**
>    * gmc_v8_0_set_fault_enable_default - update VM fault handling
>    *
> @@ -1612,7 +1618,8 @@ static const struct amdgpu_gart_funcs gmc_v8_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v8_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v8_0_gart_set_pte_pde,
>   	.set_prt = gmc_v8_0_set_prt,
> -	.get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags
> +	.get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v8_0_get_vm_pde
>   };
>   
>   static const struct amdgpu_irq_src_funcs gmc_v8_0_irq_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 19e1027..047b1a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -358,17 +358,19 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> -static u64 gmc_v9_0_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
> +static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>   {
> -	return adev->vm_manager.vram_base_offset + mc_addr - adev->mc.vram_start;
> +	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> +	BUG_ON(addr & 0xFFFF00000000003FULL);
> +	return addr;
>   }
>   
>   static const struct amdgpu_gart_funcs gmc_v9_0_gart_funcs = {
>   	.flush_gpu_tlb = gmc_v9_0_gart_flush_gpu_tlb,
>   	.set_pte_pde = gmc_v9_0_gart_set_pte_pde,
> -	.get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
> -	.adjust_mc_addr = gmc_v9_0_adjust_mc_addr,
>   	.get_invalidate_req = gmc_v9_0_get_invalidate_req,
> +	.get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v9_0_get_vm_pde
>   };
>   
>   static void gmc_v9_0_set_gart_funcs(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 91cf7e6..fb6f4b3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -1143,10 +1143,8 @@ static void sdma_v4_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_SRBM_WRITE) |
>   			  SDMA_PKT_SRBM_WRITE_HEADER_BYTE_EN(0xf));
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> index 22f42f3..1997ec8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> @@ -1316,10 +1316,8 @@ static void uvd_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t data0, data1, mask;
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
>   	data1 = upper_32_bits(pd_addr);
> @@ -1358,10 +1356,8 @@ static void uvd_v7_0_enc_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	amdgpu_ring_write(ring, HEVC_ENC_CMD_REG_WRITE);
>   	amdgpu_ring_write(ring,	(hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> index 07b2ac7..8dbd0b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> @@ -926,10 +926,8 @@ static void vce_v4_0_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev, pd_addr);
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	amdgpu_ring_write(ring, VCE_CMD_REG_WRITE);
>   	amdgpu_ring_write(ring,	(hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 94104a9..bbbe62f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -882,9 +882,8 @@ static void vcn_v1_0_dec_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	uint32_t data0, data1, mask;
>   	unsigned eng = ring->vm_inv_eng;
>   
> -	pd_addr = pd_addr | 0x1; /* valid bit */
> -	/* now only use physical base address of PDE and valid */
> -	BUG_ON(pd_addr & 0xFFFF00000000003EULL);
> +	pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
> +	pd_addr |= AMDGPU_PTE_VALID;
>   
>   	data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
>   	data1 = upper_32_bits(pd_addr);

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

Re: [PATCH 2/3] drm/amdgpu: add some extra VM error handling

[zhoucm1]

On 2017年05月15日 19:57, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> If updating the PDs fails we now invalidate all entries to try again later.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 +++++++++++++++++++++++++++++++++-
>   1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 344f943..b877f9f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1105,6 +1105,32 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   }
>   
>   /*
> + * amdgpu_vm_invalidate_level - mark all PD levels as invalid
> + *
> + * @parent: parent PD
> + *
> + * Mark all PD level as invalid after an error.
> + */
> +static void amdgpu_vm_invalidate_level(struct amdgpu_vm_pt *parent)
> +{
> +	unsigned pt_idx;
> +
> +	/*
> +	 * Recurse into the subdirectories. This recursion is harmless because
> +	 * we only have a maximum of 5 layers.
> +	 */
> +	for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
> +		struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
> +
> +		if (!entry->bo)
> +			continue;
> +
> +		entry->addr = ~0ULL;
> +		amdgpu_vm_invalidate_level(entry);
> +	}
> +}
> +
> +/*
>    * amdgpu_vm_update_directories - make sure that all directories are valid
>    *
>    * @adev: amdgpu_device pointer
> @@ -1116,7 +1142,13 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   				 struct amdgpu_vm *vm)
>   {
> -	return amdgpu_vm_update_level(adev, vm, &vm->root, 0);
> +	int r;
> +
> +	r = amdgpu_vm_update_level(adev, vm, &vm->root, 0);
> +	if (r)
> +		amdgpu_vm_invalidate_level(&vm->root);
> +
> +	return r;
>   }
>   
>   /**

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

Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[zhoucm1]

On 2017年05月15日 19:57, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> I always wondered why this code uses the MC address. Now it came to me that
> this is actually a bug and only works by coincident because we used to have
> VRAM mapped at zero.
adding Hawking to confirm this.

Regards,
David Zhou
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index e4c5d31..ba6eb73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   	}
>   
>   	if (p->job->vm) {
> -		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
> +		p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>   
>   		r = amdgpu_bo_vm_update_pte(p);
>   		if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b877f9f3..7256fcc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   				return r;
>   		}
>   
> -		pt = amdgpu_bo_gpu_offset(bo);
> +		pt = bo->tbo.mem.start << PAGE_SHIFT;
>   		if (parent->entries[pt_idx].addr == pt)
>   			continue;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 047b1a7..63cb573 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   
>   static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>   {
> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> +	addr = adev->vm_manager.vram_base_offset + addr;
>   	BUG_ON(addr & 0xFFFF00000000003FULL);
>   	return addr;
>   }

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

Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Zhang, Jerry (Junwei)]

On 05/15/2017 07:57 PM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> I always wondered why this code uses the MC address. Now it came to me that
> this is actually a bug and only works by coincident because we used to have
> VRAM mapped at zero.

Do you mean to use tbo address in general sw path and convert it into mc 
address before it going to the real IP (like get_vm_pde)?

Jerry

>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index e4c5d31..ba6eb73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   	}
>
>   	if (p->job->vm) {
> -		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
> +		p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>
>   		r = amdgpu_bo_vm_update_pte(p);
>   		if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b877f9f3..7256fcc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   				return r;
>   		}
>
> -		pt = amdgpu_bo_gpu_offset(bo);
> +		pt = bo->tbo.mem.start << PAGE_SHIFT;
>   		if (parent->entries[pt_idx].addr == pt)
>   			continue;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 047b1a7..63cb573 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
>
>   static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>   {
> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> +	addr = adev->vm_manager.vram_base_offset + addr;
>   	BUG_ON(addr & 0xFFFF00000000003FULL);
>   	return addr;
>   }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Zhang, Hawking]

>   static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>   {
> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> +	addr = adev->vm_manager.vram_base_offset + addr;

Please be noted that mv.vram_start is not 0, it is equal to the value from mmMC_VM_FB_LOCATION_BASE, which is initialized by vbios. Typically is 0xf400000000, mc address. Thus any addr here should be based on 0xf400000000. 

+ addr = adev->vm_manager.vram_base_offset + addr;

I don't think it is correct to add an absolute addr here. vram_base_offset is already the base physical address of vram (typically 0 for dGPU, while some non-zero value for APU), we need an relative address/offset to reflect the location of vram bo. 
addr - adev->mc.vram_start just calculate the relative address of the vram bo, or the offset from vram start point.

Anyway, the base physical address (vram_base_offset) should plus an offset to reflect the real physical address of vram bo.

Regards,
Hawking

-----Original Message-----
From: Zhou, David(ChunMing) 
Sent: Tuesday, May 16, 2017 10:45
To: Christian König <deathsimple@vodafone.de>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug



On 2017年05月15日 19:57, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> I always wondered why this code uses the MC address. Now it came to me 
> that this is actually a bug and only works by coincident because we 
> used to have VRAM mapped at zero.
adding Hawking to confirm this.

Regards,
David Zhou
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index e4c5d31..ba6eb73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>   	}
>   
>   	if (p->job->vm) {
> -		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
> +		p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>   
>   		r = amdgpu_bo_vm_update_pte(p);
>   		if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b877f9f3..7256fcc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   				return r;
>   		}
>   
> -		pt = amdgpu_bo_gpu_offset(bo);
> +		pt = bo->tbo.mem.start << PAGE_SHIFT;
>   		if (parent->entries[pt_idx].addr == pt)
>   			continue;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 047b1a7..63cb573 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct 
> amdgpu_device *adev,
>   
>   static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>   {
> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> +	addr = adev->vm_manager.vram_base_offset + addr;
>   	BUG_ON(addr & 0xFFFF00000000003FULL);
>   	return addr;
>   }

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

Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Christian König]

Am 16.05.2017 um 09:21 schrieb Zhang, Hawking:
>>    static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>    {
>> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
>> +	addr = adev->vm_manager.vram_base_offset + addr;
> Please be noted that mv.vram_start is not 0, it is equal to the value from mmMC_VM_FB_LOCATION_BASE, which is initialized by vbios. Typically is 0xf400000000, mc address. Thus any addr here should be based on 0xf400000000.
>
> + addr = adev->vm_manager.vram_base_offset + addr;
>
> I don't think it is correct to add an absolute addr here. vram_base_offset is already the base physical address of vram (typically 0 for dGPU, while some non-zero value for APU), we need an relative address/offset to reflect the location of vram bo.
> addr - adev->mc.vram_start just calculate the relative address of the vram bo, or the offset from vram start point.

Yes, completely agree. We used the MC address space to program the page 
directory address into the hardware instead of the relative 
address/offset to reflect the location of VRAM BO.

During GFX9 bringup somebody noticed that this in incorrect and doesn't 
work when mmMC_VM_FB_LOCATION_BASE is != 0.

The problem is that whoever did this didn't fixed the underlying bug and 
started to use the relative address/offset for programming the PD 
address, but rather added the code snippet " - adev->mc.vram_start" to 
just get GFX9 working.

Alex was wondering for quite a while why programming 
mmMC_VM_FB_LOCATION_BASE != 0 didn't worked correctly on GFX7 and GFX8 
and this is the explanation.

Please review,
Christian.

>
> Anyway, the base physical address (vram_base_offset) should plus an offset to reflect the real physical address of vram bo.
>
> Regards,
> Hawking
>
> -----Original Message-----
> From: Zhou, David(ChunMing)
> Sent: Tuesday, May 16, 2017 10:45
> To: Christian König <deathsimple@vodafone.de>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug
>
>
>
> On 2017年05月15日 19:57, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> I always wondered why this code uses the MC address. Now it came to me
>> that this is actually a bug and only works by coincident because we
>> used to have VRAM mapped at zero.
> adding Hawking to confirm this.
>
> Regards,
> David Zhou
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>>    3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index e4c5d31..ba6eb73 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>>    	}
>>    
>>    	if (p->job->vm) {
>> -		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
>> +		p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>>    
>>    		r = amdgpu_bo_vm_update_pte(p);
>>    		if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index b877f9f3..7256fcc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>>    				return r;
>>    		}
>>    
>> -		pt = amdgpu_bo_gpu_offset(bo);
>> +		pt = bo->tbo.mem.start << PAGE_SHIFT;
>>    		if (parent->entries[pt_idx].addr == pt)
>>    			continue;
>>    
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 047b1a7..63cb573 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct
>> amdgpu_device *adev,
>>    
>>    static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>    {
>> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
>> +	addr = adev->vm_manager.vram_base_offset + addr;
>>    	BUG_ON(addr & 0xFFFF00000000003FULL);
>>    	return addr;
>>    }


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

Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Christian König]

Am 16.05.2017 um 06:58 schrieb Zhang, Jerry (Junwei):
> On 05/15/2017 07:57 PM, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> I always wondered why this code uses the MC address. Now it came to 
>> me that
>> this is actually a bug and only works by coincident because we used 
>> to have
>> VRAM mapped at zero.
>
> Do you mean to use tbo address in general sw path and convert it into 
> mc address before it going to the real IP (like get_vm_pde)?

Yes, see when we need an address for the MC clients 
(GFX,SDMA,UVD,VCE,DCE etc...) we should use amdgpu_bo_gpu_offset().

But when we need an address for the MC itself then using the mapping 
applied by the MC is nonsense. We need to use the relative address 
inside VRAM instead (or the physical system memory address and set the 
system memory bit).

Christian.

>
> Jerry
>
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index e4c5d31..ba6eb73 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct 
>> amdgpu_device *adev,
>>       }
>>
>>       if (p->job->vm) {
>> -        p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
>> +        p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>>
>>           r = amdgpu_bo_vm_update_pte(p);
>>           if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index b877f9f3..7256fcc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct 
>> amdgpu_device *adev,
>>                   return r;
>>           }
>>
>> -        pt = amdgpu_bo_gpu_offset(bo);
>> +        pt = bo->tbo.mem.start << PAGE_SHIFT;
>>           if (parent->entries[pt_idx].addr == pt)
>>               continue;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 047b1a7..63cb573 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct 
>> amdgpu_device *adev,
>>
>>   static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>   {
>> -    addr = adev->vm_manager.vram_base_offset + addr - 
>> adev->mc.vram_start;
>> +    addr = adev->vm_manager.vram_base_offset + addr;
>>       BUG_ON(addr & 0xFFFF00000000003FULL);
>>       return addr;
>>   }
>>

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

RE: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Zhang, Hawking]

Hi Christian,

Basically we are on the same page that we should use relative address of vram bo to program hw.  

What I missed part is that you used the tbo.mem.start, the relative address, rather than tbo.offset, the absolute address, for the calculation. Based on that, I'm okay with the follow up logic: addr = adev->vm_manager.vram_base_offset + addr; However, I'm under impression that even with that fixed, and pre-GFX9 also changed to use FB_LOCATION_BASE programmed by bios as mc start, there is still some issue in VCE/UVD.etc. 

Have you verified the patch on pre-GFX9 adapter?

Regards,
Hawking

-----Original Message-----
From: Christian König [mailto:deathsimple@vodafone.de] 
Sent: Tuesday, May 16, 2017 15:49
To: Zhang, Hawking <Hawking.Zhang@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

Am 16.05.2017 um 09:21 schrieb Zhang, Hawking:
>>    static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>    {
>> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
>> +	addr = adev->vm_manager.vram_base_offset + addr;
> Please be noted that mv.vram_start is not 0, it is equal to the value from mmMC_VM_FB_LOCATION_BASE, which is initialized by vbios. Typically is 0xf400000000, mc address. Thus any addr here should be based on 0xf400000000.
>
> + addr = adev->vm_manager.vram_base_offset + addr;
>
> I don't think it is correct to add an absolute addr here. vram_base_offset is already the base physical address of vram (typically 0 for dGPU, while some non-zero value for APU), we need an relative address/offset to reflect the location of vram bo.
> addr - adev->mc.vram_start just calculate the relative address of the vram bo, or the offset from vram start point.

Yes, completely agree. We used the MC address space to program the page directory address into the hardware instead of the relative address/offset to reflect the location of VRAM BO.

During GFX9 bringup somebody noticed that this in incorrect and doesn't work when mmMC_VM_FB_LOCATION_BASE is != 0.

The problem is that whoever did this didn't fixed the underlying bug and started to use the relative address/offset for programming the PD address, but rather added the code snippet " - adev->mc.vram_start" to just get GFX9 working.

Alex was wondering for quite a while why programming mmMC_VM_FB_LOCATION_BASE != 0 didn't worked correctly on GFX7 and GFX8 and this is the explanation.

Please review,
Christian.

>
> Anyway, the base physical address (vram_base_offset) should plus an offset to reflect the real physical address of vram bo.
>
> Regards,
> Hawking
>
> -----Original Message-----
> From: Zhou, David(ChunMing)
> Sent: Tuesday, May 16, 2017 10:45
> To: Christian König <deathsimple@vodafone.de>; 
> amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug
>
>
>
> On 2017年05月15日 19:57, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> I always wondered why this code uses the MC address. Now it came to 
>> me that this is actually a bug and only works by coincident because 
>> we used to have VRAM mapped at zero.
> adding Hawking to confirm this.
>
> Regards,
> David Zhou
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>>    3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index e4c5d31..ba6eb73 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
>>    	}
>>    
>>    	if (p->job->vm) {
>> -		p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
>> +		p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>>    
>>    		r = amdgpu_bo_vm_update_pte(p);
>>    		if (r)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index b877f9f3..7256fcc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>>    				return r;
>>    		}
>>    
>> -		pt = amdgpu_bo_gpu_offset(bo);
>> +		pt = bo->tbo.mem.start << PAGE_SHIFT;
>>    		if (parent->entries[pt_idx].addr == pt)
>>    			continue;
>>    
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 047b1a7..63cb573 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct 
>> amdgpu_device *adev,
>>    
>>    static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>    {
>> -	addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
>> +	addr = adev->vm_manager.vram_base_offset + addr;
>>    	BUG_ON(addr & 0xFFFF00000000003FULL);
>>    	return addr;
>>    }


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

Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug

[Zhang, Jerry (Junwei)]

On 05/16/2017 03:51 PM, Christian König wrote:
> Am 16.05.2017 um 06:58 schrieb Zhang, Jerry (Junwei):
>> On 05/15/2017 07:57 PM, Christian König wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> I always wondered why this code uses the MC address. Now it came to me that
>>> this is actually a bug and only works by coincident because we used to have
>>> VRAM mapped at zero.
>>
>> Do you mean to use tbo address in general sw path and convert it into mc
>> address before it going to the real IP (like get_vm_pde)?
>
> Yes, see when we need an address for the MC clients (GFX,SDMA,UVD,VCE,DCE
> etc...) we should use amdgpu_bo_gpu_offset().
>
> But when we need an address for the MC itself then using the mapping applied by
> the MC is nonsense. We need to use the relative address inside VRAM instead (or
> the physical system memory address and set the system memory bit).

Sounds reasonable.

Shall we use below for pre-gfx9 as well?
addr = adev->vm_manager.vram_base_offset + addr;

Jery

>
> Christian.
>
>>
>> Jerry
>>
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index e4c5d31..ba6eb73 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device
>>> *adev,
>>>       }
>>>
>>>       if (p->job->vm) {
>>> -        p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
>>> +        p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>>>
>>>           r = amdgpu_bo_vm_update_pte(p);
>>>           if (r)
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index b877f9f3..7256fcc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device
>>> *adev,
>>>                   return r;
>>>           }
>>>
>>> -        pt = amdgpu_bo_gpu_offset(bo);
>>> +        pt = bo->tbo.mem.start << PAGE_SHIFT;
>>>           if (parent->entries[pt_idx].addr == pt)
>>>               continue;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> index 047b1a7..63cb573 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct
>>> amdgpu_device *adev,
>>>
>>>   static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>>   {
>>> -    addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
>>> +    addr = adev->vm_manager.vram_base_offset + addr;
>>>       BUG_ON(addr & 0xFFFF00000000003FULL);
>>>       return addr;
>>>   }
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx