Re: [PATCH 2/5] drm/amdgpu: Support new arcturus mtype

["Kuehling, Felix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>]

On 2019-08-06 22:31, Zeng, Oak wrote:
> Arcturus repurposed mtype WC to RW. Modify gmc functions
> to support the new mtype
>
> Change-Id: Idc338e5386a57020f45262025e2664ab4ba9f291
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c     | 61 ++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/amd/include/vega10_enum.h |  7 ++++
>   2 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index a2aa35e..89064d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -599,6 +599,50 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct amdgpu_device *adev,
>   	return pte_flag;
>   }
>   
> +static uint64_t gmc_v9_0_arcturus_get_vm_pte_flags(struct amdgpu_device *adev,
> +						uint32_t flags)
> +
> +{
> +	uint64_t pte_flag = 0;
> +
> +	if (flags & AMDGPU_VM_PAGE_EXECUTABLE)
> +		pte_flag |= AMDGPU_PTE_EXECUTABLE;
> +	if (flags & AMDGPU_VM_PAGE_READABLE)
> +		pte_flag |= AMDGPU_PTE_READABLE;
> +	if (flags & AMDGPU_VM_PAGE_WRITEABLE)
> +		pte_flag |= AMDGPU_PTE_WRITEABLE;
> +
> +	if (adev->gmc.zfb_size > 0)
> +		pte_flag |= AMDGPU_PTE_SYSTEM;
> +
> +	switch (flags & AMDGPU_VM_MTYPE_MASK) {
> +	case AMDGPU_VM_MTYPE_DEFAULT:
> +		pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_ARCTURUS_NC);
> +		break;
> +	case AMDGPU_VM_MTYPE_NC:
> +		pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_ARCTURUS_NC);
> +		break;
> +	case AMDGPU_VM_MTYPE_RW:
> +		pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_ARCTURUS_RW);
> +		break;
> +	case AMDGPU_VM_MTYPE_CC:
> +		pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_ARCTURUS_CC);
> +		break;
> +	case AMDGPU_VM_MTYPE_UC:
> +		pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_ARCTURUS_UC);
> +		break;
> +	default:
> +		pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_ARCTURUS_NC);
> +		break;
> +	}
> +
> +	if (flags & AMDGPU_VM_PAGE_PRT)
> +		pte_flag |= AMDGPU_PTE_PRT;
> +
> +	return pte_flag;
> +}

Adding a whole new function for Arcturus is overkill. Just add MTYPE_RW 
to the existing function.

         switch(flags & AMDGPU_VM_MTYPE_MASK) {
         ...
         case AMDGPU_VM_MTYPE_WC:
                 pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_WC);
                 break;
         case AMDGPU_VM_MTYPE_RW:
                 pte_flag |= AMDGPU_PTE_MTYPE_VG10(MTYPE_RW);
                 break;
         ...
         }

The fact that WC isn't valid on Arcturus and RW isn't valid on other 
Vega GPUs could be handled in higher level code.


> +
> +
>   static void gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, int level,
>   				uint64_t *addr, uint64_t *flags)
>   {
> @@ -631,9 +675,24 @@ static const struct amdgpu_gmc_funcs gmc_v9_0_gmc_funcs = {
>   	.get_vm_pde = gmc_v9_0_get_vm_pde
>   };
>   
> +static const struct amdgpu_gmc_funcs gmc_v9_0_arcturus_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,
> +	.get_vm_pte_flags = gmc_v9_0_arcturus_get_vm_pte_flags,
> +	.get_vm_pde = gmc_v9_0_get_vm_pde
> +};
> +
>   static void gmc_v9_0_set_gmc_funcs(struct amdgpu_device *adev)
>   {
> -	adev->gmc.gmc_funcs = &gmc_v9_0_gmc_funcs;
> +	switch (adev->asic_type) {
> +	case CHIP_ARCTURUS:
> +		adev->gmc.gmc_funcs = &gmc_v9_0_arcturus_gmc_funcs;
> +		break;
> +	default:
> +		adev->gmc.gmc_funcs = &gmc_v9_0_gmc_funcs;
> +		break;
> +	}
>   }
>   
>   static void gmc_v9_0_set_umc_funcs(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/include/vega10_enum.h b/drivers/gpu/drm/amd/include/vega10_enum.h
> index c14ba65..0d6d97d 100644
> --- a/drivers/gpu/drm/amd/include/vega10_enum.h
> +++ b/drivers/gpu/drm/amd/include/vega10_enum.h
> @@ -1041,6 +1041,13 @@ MTYPE_CC                                 = 0x00000002,
>   MTYPE_UC                                 = 0x00000003,
>   } MTYPE;
>   
> +typedef enum MTYPE_ARCTURUS {
> +MTYPE_ARCTURUS_NC                                 = 0x00000000,
> +MTYPE_ARCTURUS_RW                                 = 0x00000001,
> +MTYPE_ARCTURUS_CC                                 = 0x00000002,
> +MTYPE_ARCTURUS_UC                                 = 0x00000003,
> +} MTYPE_ARCTURUS;

This is overkill. Most of the MTYPEs are the same on Arcturus. You can 
just define MTYPE_RW as an alias of MTYPE_WC:

typedef enum MTYPE {
MTYPE_NC                                 = 0x00000000,
MTYPE_WC                                 = 0x00000001,
MTYPE_RW                                 = 0x00000001, /* Arcturus */
MTYPE_CC                                 = 0x00000002,
MTYPE_UC                                 = 0x00000003,
} MTYPE;

Regards,
   Felix


> +
>   /*
>    * RMI_CID enum
>    */
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx