Re: [PATCH 10/14] drm/amdgpu/vm: Use init PTE flags and NOALLOC in amdgpu_vm_handle_fault()

["Timur Kristóf" <timur.kristof@gmail.com>]

On 2026. július 2., csütörtök 14:18:45 közép-európai nyári idő Christian König 
wrote:
> On 7/2/26 13:28, Timur Kristóf wrote:
> > On 2026. július 2., csütörtök 12:22:23 közép-európai nyári idő Christian
> > König> 
> > wrote:
> >> On 7/1/26 18:17, Timur Kristóf wrote:
> >>> PTE_IS_PTE seems necessary for handling retry faults on GFX12.
> >>> 
> >>> For reference see:
> >>> amdgpu_vm_pte_update_flags() that explains the problem
> >>> svm_range_get_pte_flags() that uses the flag on GFX12
> >>> 
> >>> Also add NOALLOC on GFX10.3+ as we don't need to allocate
> >>> the fault handling PTE in the infinity cache (MALL).
> >>> 
> >>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
> >>>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index
> >>> 32719f31b6c9..a915d061085f
> >>> 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>> @@ -3044,7 +3044,8 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device
> >>> *adev, u32 pasid,>
> >>> 
> >>>  	}
> >>>  	
> >>>  	addr /= AMDGPU_GPU_PAGE_SIZE;
> >>> 
> >>> -	flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED |
> >>> +	flags = adev->gmc.init_pte_flags |
> >>> +		AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED |
> >>> 
> >>>  		AMDGPU_PTE_SYSTEM;
> >> 
> >> That doesn't make sense the PTE flags should be adjusted to the device
> >> specific flags by the callbacks.
> > 
> > Can you please elaborate on what is it that doesn't make sense here and
> > why, and what you recommend to do instead?
> 
> I had to dig up what the problem here is as well. We use the ASIC specific
> AMDGPU_PTE_* flags directly.
> 
> Instead we should use the AMDGPU_VM_PAGE_* flags and then call
> amdgpu_gmc_get_vm_pte(adev, vm, NULL, in_flags, &out_flags) to translate
> the AMDGPU_VM_PAGE_* flags into the ASIC specific ones.
> 
> This will automatically add flags like AMDGPU_PTE_TF and AMDGPU_PTE_IS_PTE
> etc...

As far as I understand, the init_pte_flags was added specifically for this 
purpose, and that's what SVM uses too.

Are you suggesting to call to amdgpu_gmc_get_vm_pte() here?
As far as I see the implementation of that function for Navi 4 is 
gmc_v12_0_get_vm_pte() which doesn't set the PTE_IS_PTE flag so it wouldn't 
resolve the issue with the fault that this commit is fixing.

> >>>  	if (is_compute_context) {
> >>> 
> >>> @@ -3054,11 +3055,14 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device
> >>> *adev, u32 pasid,>
> >>> 
> >>>  		flags = AMDGPU_VM_NORETRY_FLAGS;
> >>>  		value = 0;
> >>>  	
> >>>  	} else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
> >>> 
> >>> +		/* Don't allocate this PTE in the MALL */
> >>> +		if (amdgpu_ip_version(adev, GC_HWIP, 0) >=
> > 
> > IP_VERSION(10, 3, 0))
> > 
> >>> +			flags |= AMDGPU_PTE_NOALLOC;
> >>> +
> >> 
> >> This doesn't make sense either, mall allocation for the dummy page should
> >> be perfectly fine.
> > 
> > I think we shouldn't waste space for this PTE in the MALL.
> 
> Hui? The PTE doesn't enter the MALL, it is the dummy page which enters the
> MALL and since it is only 4k it basically doesn't waste much space.
> 

Sure I can remove this line if you feel that strongly about it.
Do we gain any benefit from allowing this page in the MALL?


> 
> >>>  		/* Redirect the access to the dummy page */
> >>>  		value = adev->dummy_page_addr;
> >>>  		flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE |
> >>>  		
> >>>  			AMDGPU_PTE_WRITEABLE;
> >>> 
> >>> -
> >>> 
> >>>  	} else {
> >>>  	
> >>>  		/* Let the hw retry silently on the PTE */
> >>>  		value = 0;