Re: [PATCH 2/2] drm/amdgpu: cleanup PTE flag generation

Christian König <ckoenig.leichtzumerken@xxxxxxxxx> · Wed, 4 Sep 2019 09:59:29 +0200

the  mapping  structure still keep the  UAPI flag and  all the HW specific PTE setting is in the
last step before real set to HW ?

I thought about that approach as well, but that would unfortunately not 
work correctly.

The problem is that we have some mapping flags which don't have an 
userspace representation.

Additional to that I want to avoid leaking the UAPI defines into the VM 
subsystem.

We shouldn't have created the UAPI defines in the first place and should 
have used the hardware flags directly.

Regards,
Christian.

Am 03.09.19 um 23:32 schrieb Liu, Shaoyun:
Maybe  it's not necessary to separate the mtype from the get_vm_pte
function , so we just need one  asic specific functions (get_vm_pte) .

What make me confusing originally is  the  the  logic  to setup the  PTE
flag.   We first map the  UAPI flags to HW specific PTE flag , save it
into mapping  structure  and  in amdgpu_bo_split_mapping adjust the
flag again before set the value to HW . Is it possible we just have one
place to set the asic specific PTE flag ,  ex, the  mapping  structure
still keep the  UAPI flag and  all the HW specific PTE setting is in the
last step before real set to HW ?

Regards

shaoyun.liu

On 2019-09-02 10:57 a.m., Christian König wrote:
Move the ASIC specific code into a new callback function.

Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  5 +++++
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 29 ++-----------------------
   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  | 22 ++++++++++++++++++-
   drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c   |  9 ++++++++
   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c   | 11 +++++++++-
   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c   | 13 ++++++++++-
   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 24 +++++++++++++++++++-
   7 files changed, 82 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index d5e4574afbc2..d3be51ba6349 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -104,6 +104,10 @@ struct amdgpu_gmc_funcs {
   	/* get the pde for a given mc addr */
   	void (*get_vm_pde)(struct amdgpu_device *adev, int level,
   			   u64 *dst, u64 *flags);
+	/* get the pte flags to use for a BO VA mapping */
+	void (*get_vm_pte)(struct amdgpu_device *adev,
+			   struct amdgpu_bo_va_mapping *mapping,
+			   uint64_t *flags);
   };
   
   struct amdgpu_xgmi {
@@ -185,6 +189,7 @@ struct amdgpu_gmc {
   #define amdgpu_gmc_emit_pasid_mapping(r, vmid, pasid) (r)->adev->gmc.gmc_funcs->emit_pasid_mapping((r), (vmid), (pasid))
   #define amdgpu_gmc_map_mtype(adev, flags) (adev)->gmc.gmc_funcs->map_mtype((adev),(flags))
   #define amdgpu_gmc_get_vm_pde(adev, level, dst, flags) (adev)->gmc.gmc_funcs->get_vm_pde((adev), (level), (dst), (flags))
+#define amdgpu_gmc_get_vm_pte(adev, mapping, flags) (adev)->gmc.gmc_funcs->get_vm_pte((adev), (mapping), (flags))
   
   /**
    * amdgpu_gmc_vram_full_visible - Check if full VRAM is visible through the BAR
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 2cb82b229802..b285ab25146d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1571,33 +1571,8 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
   	if (!(mapping->flags & AMDGPU_PTE_WRITEABLE))
   		flags &= ~AMDGPU_PTE_WRITEABLE;
   
-	if (adev->asic_type >= CHIP_TONGA) {
-		flags &= ~AMDGPU_PTE_EXECUTABLE;
-		flags |= mapping->flags & AMDGPU_PTE_EXECUTABLE;
-	}
-
-	if (adev->asic_type >= CHIP_NAVI10) {
-		flags &= ~AMDGPU_PTE_MTYPE_NV10_MASK;
-		flags |= (mapping->flags & AMDGPU_PTE_MTYPE_NV10_MASK);
-	} else {
-		flags &= ~AMDGPU_PTE_MTYPE_VG10_MASK;
-		flags |= (mapping->flags & AMDGPU_PTE_MTYPE_VG10_MASK);
-	}
-
-	if ((mapping->flags & AMDGPU_PTE_PRT) &&
-	    (adev->asic_type >= CHIP_VEGA10)) {
-		flags |= AMDGPU_PTE_PRT;
-		if (adev->asic_type >= CHIP_NAVI10) {
-			flags |= AMDGPU_PTE_SNOOPED;
-			flags |= AMDGPU_PTE_LOG;
-			flags |= AMDGPU_PTE_SYSTEM;
-		}
-		flags &= ~AMDGPU_PTE_VALID;
-	}
-	if (adev->asic_type == CHIP_ARCTURUS &&
-	    !(flags & AMDGPU_PTE_SYSTEM) &&
-	    mapping->bo_va->is_xgmi)
-		flags |= AMDGPU_PTE_SNOOPED;
+	/* Apply ASIC specific mapping flags */
+	amdgpu_gmc_get_vm_pte(adev, mapping, &flags);
   
   	trace_amdgpu_vm_bo_update(mapping);
   
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index 7eb0ba87fef9..1a8d8f528b01 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -440,12 +440,32 @@ static void gmc_v10_0_get_vm_pde(struct amdgpu_device *adev, int level,
   	}
   }
   
+static void gmc_v10_0_get_vm_pte(struct amdgpu_device *adev,
+				 struct amdgpu_bo_va_mapping *mapping,
+				 uint64_t *flags)
+{
+	*flags &= ~AMDGPU_PTE_EXECUTABLE;
+	*flags |= mapping->flags & AMDGPU_PTE_EXECUTABLE;
+
+	*flags &= ~AMDGPU_PTE_MTYPE_NV10_MASK;
+	*flags |= (mapping->flags & AMDGPU_PTE_MTYPE_NV10_MASK);
+
+	if (mapping->flags & AMDGPU_PTE_PRT) {
+		*flags |= AMDGPU_PTE_PRT;
+		*flags |= AMDGPU_PTE_SNOOPED;
+		*flags |= AMDGPU_PTE_LOG;
+		*flags |= AMDGPU_PTE_SYSTEM;
+		*flags &= ~AMDGPU_PTE_VALID;
+	}
+}
+
   static const struct amdgpu_gmc_funcs gmc_v10_0_gmc_funcs = {
   	.flush_gpu_tlb = gmc_v10_0_flush_gpu_tlb,
   	.emit_flush_gpu_tlb = gmc_v10_0_emit_flush_gpu_tlb,
   	.emit_pasid_mapping = gmc_v10_0_emit_pasid_mapping,
   	.map_mtype = gmc_v10_0_map_mtype,
-	.get_vm_pde = gmc_v10_0_get_vm_pde
+	.get_vm_pde = gmc_v10_0_get_vm_pde,
+	.get_vm_pte = gmc_v10_0_get_vm_pte
   };
   
   static void gmc_v10_0_set_gmc_funcs(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 2e305b21db69..ce591c995691 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -389,6 +389,14 @@ static void gmc_v6_0_get_vm_pde(struct amdgpu_device *adev, int level,
   	BUG_ON(*addr & 0xFFFFFF0000000FFFULL);
   }
   
+static void gmc_v6_0_get_vm_pte(struct amdgpu_device *adev,
+				struct amdgpu_bo_va_mapping *mapping,
+				uint64_t *flags)
+{
+	BUG_ON(*flags & AMDGPU_PTE_EXECUTABLE);
+	BUG_ON(*flags & AMDGPU_PTE_PRT);
+}
+
   static void gmc_v6_0_set_fault_enable_default(struct amdgpu_device *adev,
   					      bool value)
   {
@@ -1144,6 +1152,7 @@ static const struct amdgpu_gmc_funcs gmc_v6_0_gmc_funcs = {
   	.emit_flush_gpu_tlb = gmc_v6_0_emit_flush_gpu_tlb,
   	.set_prt = gmc_v6_0_set_prt,
   	.get_vm_pde = gmc_v6_0_get_vm_pde,
+	.get_vm_pte = gmc_v6_0_get_vm_pte,
   };
   
   static const struct amdgpu_irq_src_funcs gmc_v6_0_irq_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 3b77421234a7..e3f53fbfcd8f 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -466,6 +466,14 @@ static void gmc_v7_0_get_vm_pde(struct amdgpu_device *adev, int level,
   	BUG_ON(*addr & 0xFFFFFF0000000FFFULL);
   }
   
+static void gmc_v7_0_get_vm_pte(struct amdgpu_device *adev,
+				struct amdgpu_bo_va_mapping *mapping,
+				uint64_t *flags)
+{
+	BUG_ON(*flags & AMDGPU_PTE_EXECUTABLE);
+	BUG_ON(*flags & AMDGPU_PTE_PRT);
+}
+
   /**
    * gmc_v8_0_set_fault_enable_default - update VM fault handling
    *
@@ -1339,7 +1347,8 @@ static const struct amdgpu_gmc_funcs gmc_v7_0_gmc_funcs = {
   	.emit_flush_gpu_tlb = gmc_v7_0_emit_flush_gpu_tlb,
   	.emit_pasid_mapping = gmc_v7_0_emit_pasid_mapping,
   	.set_prt = gmc_v7_0_set_prt,
-	.get_vm_pde = gmc_v7_0_get_vm_pde
+	.get_vm_pde = gmc_v7_0_get_vm_pde,
+	.get_vm_pte = gmc_v7_0_get_vm_pte
   };
   
   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 58444aa0af05..256d1faacada 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -689,6 +689,16 @@ static void gmc_v8_0_get_vm_pde(struct amdgpu_device *adev, int level,
   	BUG_ON(*addr & 0xFFFFFF0000000FFFULL);
   }
   
+static void gmc_v8_0_get_vm_pte(struct amdgpu_device *adev,
+				struct amdgpu_bo_va_mapping *mapping,
+				uint64_t *flags)
+{
+	BUG_ON(*flags & AMDGPU_PTE_PRT);
+
+	*flags &= ~AMDGPU_PTE_EXECUTABLE;
+	*flags |= mapping->flags & AMDGPU_PTE_EXECUTABLE;
+}
+
   /**
    * gmc_v8_0_set_fault_enable_default - update VM fault handling
    *
@@ -1705,7 +1715,8 @@ static const struct amdgpu_gmc_funcs gmc_v8_0_gmc_funcs = {
   	.emit_flush_gpu_tlb = gmc_v8_0_emit_flush_gpu_tlb,
   	.emit_pasid_mapping = gmc_v8_0_emit_pasid_mapping,
   	.set_prt = gmc_v8_0_set_prt,
-	.get_vm_pde = gmc_v8_0_get_vm_pde
+	.get_vm_pde = gmc_v8_0_get_vm_pde,
+	.get_vm_pte = gmc_v8_0_get_vm_pte
   };
   
   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 22660e1005db..b3d2d70e84c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -625,12 +625,34 @@ static void gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, int level,
   	}
   }
   
+static void gmc_v9_0_get_vm_pte(struct amdgpu_device *adev,
+				struct amdgpu_bo_va_mapping *mapping,
+				uint64_t *flags)
+{
+	*flags &= ~AMDGPU_PTE_EXECUTABLE;
+	*flags |= mapping->flags & AMDGPU_PTE_EXECUTABLE;
+
+	*flags &= ~AMDGPU_PTE_MTYPE_VG10_MASK;
+	*flags |= mapping->flags & AMDGPU_PTE_MTYPE_VG10_MASK;
+
+	if (mapping->flags & AMDGPU_PTE_PRT) {
+		*flags |= AMDGPU_PTE_PRT;
+		*flags &= ~AMDGPU_PTE_VALID;
+	}
+
+	if (adev->asic_type == CHIP_ARCTURUS &&
+	    !(*flags & AMDGPU_PTE_SYSTEM) &&
+	    mapping->bo_va->is_xgmi)
+		*flags |= AMDGPU_PTE_SNOOPED;
+}
+
   static const struct amdgpu_gmc_funcs gmc_v9_0_gmc_funcs = {
   	.flush_gpu_tlb = gmc_v9_0_flush_gpu_tlb,
   	.emit_flush_gpu_tlb = gmc_v9_0_emit_flush_gpu_tlb,
   	.emit_pasid_mapping = gmc_v9_0_emit_pasid_mapping,
   	.map_mtype = gmc_v9_0_map_mtype,
-	.get_vm_pde = gmc_v9_0_get_vm_pde
+	.get_vm_pde = gmc_v9_0_get_vm_pde,
+	.get_vm_pte = gmc_v9_0_get_vm_pte
   };
   
   static void gmc_v9_0_set_gmc_funcs(struct amdgpu_device *adev)

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







