[PATCH 2/2] drm/amdgpu: cleanup PTE flag generation
Kuehling, Felix
Felix.Kuehling at amd.com
Tue Sep 3 20:28:48 UTC 2019
On 2019-09-02 10:57 a.m., Christian König wrote:
> Move the ASIC specific code into a new callback function.
NAK. I believe the BUG_ONs you're adding will trigger with ROCm on
Hawaii and Kaveri. See inline ...
ROCm user mode doesn't care that the page table doesn't have an
executable bit on those chips. If the HW makes all memory executable, we
should just ignore the flag.
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
> 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);
NAK. This is going to break ROCm on Kaveri and Hawaii.
Regards,
Felix
> + 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)
More information about the amd-gfx
mailing list