Re: [PATCH 3/4] drm/amdgpu: set the A+A VF SYSTEM bit in the GFX 12.1 helpers

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

On 11/9/26 17:26, Zhigang Luo wrote:
> gmc_v12_1_get_vm_pde() converts a VRAM MC address to a physical
> address and skips that conversion when AMDGPU_PTE_SYSTEM is already
> set. Set the bit after the translation so the address stays physical.
> 
> gmc_v12_1_get_vm_pte() covers the graphics and SVM PTE paths. SVM
> VRAM ranges call it with a NULL buffer object, so set the bit there
> before the NPA flags, which still clear it.
> 
> PDB0 is written directly and does not go through get_vm_pde(). Set
> AMDGPU_PTE_SYSTEM on the SPM VRAM entries and the GART PDE. NPA
> still requires PTE.S=0, so clear the bit in gmc_v12_1_get_npa_flags()
> and amdgpu_ualink_peer_remote_init().
> 
> Signed-off-by: Zhigang Luo <zhigang.luo@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    |  4 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c |  1 +
>  drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c     | 10 ++++++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 659e89e5e78f..f76da74abfec 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -1281,6 +1281,8 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	flags |= AMDGPU_PTE_VALID | AMDGPU_PTE_READABLE;
>  	flags |= AMDGPU_PTE_WRITEABLE;
>  	flags |= AMDGPU_PTE_SNOOPED;
> +	if (amdgpu_virt_vram_is_spm(adev))
> +		flags |= AMDGPU_PTE_SYSTEM;

Yeah again, hacks like that are a pretty big NO-GO.

We should probably start to use amdgpu_gmc_get_vm_pde() here as well. The reason we haven't done that previously is potentially because the address parameter is in the wrong address space.

Old HW generations expected the MC address space in the PDEs, so that is what amdgpu_gmc_get_vm_pde() takes as parameter. But newer generations expect the PA and because of this we call amdgpu_gmc_vram_mc2pa() in the get_vm_pde callback.

So that results in a PA->MC->PA conversion which doesn't make to much sense.

This needs to be restructured to completely support VRAM domain in system memory as well.

>  	flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
>  	flags |= AMDGPU_PDE_PTE_FLAG(adev);
>  
> @@ -1300,6 +1302,8 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	 * pointing to a 4K system page
>  	 */
>  	flags = AMDGPU_PTE_VALID;
> +	if (amdgpu_virt_vram_is_spm(adev))
> +		flags |= AMDGPU_PTE_SYSTEM;

Same here.

>  	flags |= AMDGPU_PTE_SNOOPED | AMDGPU_PDE_BFS_FLAG(adev, 0);
>  	/* Requires gart_ptb_gpu_pa to be 4K aligned */
>  	amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, gart_ptb_gpu_pa, flags);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> index 3bb426b33af9..11f022c56667 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> @@ -6171,6 +6171,7 @@ static int amdgpu_ualink_peer_remote_init(struct amdgpu_device *adev)
>  	flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, bo->tbo.resource);
>  	flags |= AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT_GFX12 | AMDGPU_PTE_BUS_ATOMICS;
>  	flags &= ~AMDGPU_PTE_VALID;
> +	flags &= ~AMDGPU_PTE_SYSTEM;

And that here actually breaks thing, clear NAK to that!

>  	/* PTE.X=0 turn off RPC checks for RBs, wptr and doorbell NPA address */
>  	flags &= ~AMDGPU_PTE_EXECUTABLE;
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c b/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c
> index 76391fca6c62..3bd86028f304 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c
> @@ -586,6 +586,12 @@ static void gmc_v12_1_get_vm_pde(struct amdgpu_device *adev, int level,
>  		*addr = adev->vm_manager.vram_base_offset + *addr -
>  			adev->gmc.vram_start;
>  
> +	/* SPM VRAM is reached at a system physical address. Set S after the
> +	 * translation above so that translation is not skipped.
> +	 */
> +	if (amdgpu_virt_vram_is_spm(adev))
> +		*flags |= AMDGPU_PTE_SYSTEM;
> +

That looks correct to me.

>  	if (*addr & ~adev->gmc.pte_addr_mask) {
>  		dev_err(adev->dev, "Invalid PDE address: %llx\n", *addr);
>  		*addr = 0;
> @@ -616,6 +622,7 @@ static void gmc_v12_1_get_npa_flags(struct amdgpu_device *adev,
>  	*flags |= AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT_GFX12 |
>  		   AMDGPU_PTE_BUS_ATOMICS;
>  	*flags &= ~AMDGPU_PTE_VALID;
> +	*flags &= ~AMDGPU_PTE_SYSTEM

That should be superflous.
;
>  	*flags &= ~AMDGPU_PTE_EXECUTABLE;
>  }
>  
> @@ -756,6 +763,9 @@ static void gmc_v12_1_get_vm_pte(struct amdgpu_device *adev,
>  		break;
>  	}
>  
> +	if (amdgpu_virt_vram_is_spm(adev))
> +		*flags |= AMDGPU_PTE_SYSTEM;
> +

Here you need to check if the destination is actually VRAM and not NPA.

Regards,
Christian.

>  	if (bo) {
>  		mem = bo->tbo.resource;
>  		if (mem && mem->mem_type == AMDGPU_PL_NPA) {