[PATCH] drm/amdgpu: Update invalid PTE flag setting

[PATCH] drm/amdgpu: Update invalid PTE flag setting

[Mukul Joshi]

Update the invalid PTE flag setting to ensure, in addition
to transitioning the retry fault to a no-retry fault, it
also causes the wavefront to enter the trap handler. With the
current setting, it only transitions to a no-retry fault.

Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index af6f26a97fc5..5df4f7bb241f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2488,7 +2488,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 		/* Intentionally setting invalid PTE flag
 		 * combination to force a no-retry-fault
 		 */
-		flags = AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT;
+		flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SYSTEM | AMDGPU_PTE_PRT;
 		value = 0;
 	} else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
 		/* Redirect the access to the dummy page */
-- 
2.35.1

Re: [PATCH] drm/amdgpu: Update invalid PTE flag setting

[Felix Kuehling]

On 2023-04-04 17:59, Mukul Joshi wrote:
> Update the invalid PTE flag setting to ensure, in addition
> to transitioning the retry fault to a no-retry fault, it
> also causes the wavefront to enter the trap handler. With the
> current setting, it only transitions to a no-retry fault.
>
> Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index af6f26a97fc5..5df4f7bb241f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2488,7 +2488,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   		/* Intentionally setting invalid PTE flag
>   		 * combination to force a no-retry-fault
>   		 */
> -		flags = AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT;
> +		flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SYSTEM | AMDGPU_PTE_PRT;
>   		value = 0;
>   	} else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
>   		/* Redirect the access to the dummy page */

Re: [PATCH] drm/amdgpu: Update invalid PTE flag setting

[Christian König]

Am 04.04.23 um 23:59 schrieb Mukul Joshi:
> Update the invalid PTE flag setting to ensure, in addition
> to transitioning the retry fault to a no-retry fault, it
> also causes the wavefront to enter the trap handler. With the
> current setting, it only transitions to a no-retry fault.

Well that was actually the intended result. Why should it enter the trap 
handler here?

>
> Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index af6f26a97fc5..5df4f7bb241f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2488,7 +2488,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   		/* Intentionally setting invalid PTE flag
>   		 * combination to force a no-retry-fault
>   		 */
> -		flags = AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT;
> +		flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SYSTEM | AMDGPU_PTE_PRT;

As far as I can see this is actually a valid combination and would not 
have the desired result.

Regards,
Christian.

>   		value = 0;
>   	} else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
>   		/* Redirect the access to the dummy page */

Re: [PATCH] drm/amdgpu: Update invalid PTE flag setting

[Felix Kuehling]

Am 2023-04-06 um 10:36 schrieb Christian König:
> Am 04.04.23 um 23:59 schrieb Mukul Joshi:
>> Update the invalid PTE flag setting to ensure, in addition
>> to transitioning the retry fault to a no-retry fault, it
>> also causes the wavefront to enter the trap handler. With the
>> current setting, it only transitions to a no-retry fault.
>
> Well that was actually the intended result. Why should it enter the 
> trap handler here?

We need the trap handler for reporting the fault to the debugger. The VM 
fault interrupt itself doesn't provide sufficient information to find 
the wave and the program counter that triggered the fault. It also 
doesn't work well when several waves are faulting on the same address.

Regards,
   Felix


>
>>
>> Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index af6f26a97fc5..5df4f7bb241f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2488,7 +2488,7 @@ bool amdgpu_vm_handle_fault(struct 
>> amdgpu_device *adev, u32 pasid,
>>           /* Intentionally setting invalid PTE flag
>>            * combination to force a no-retry-fault
>>            */
>> -        flags = AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT;
>> +        flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SYSTEM | AMDGPU_PTE_PRT;
>
> As far as I can see this is actually a valid combination and would not 
> have the desired result.
>
> Regards,
> Christian.
>
>>           value = 0;
>>       } else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
>>           /* Redirect the access to the dummy page */
>

Re: [PATCH] drm/amdgpu: Update invalid PTE flag setting

[Christian König]

Am 06.04.23 um 17:01 schrieb Felix Kuehling:
> Am 2023-04-06 um 10:36 schrieb Christian König:
>> Am 04.04.23 um 23:59 schrieb Mukul Joshi:
>>> Update the invalid PTE flag setting to ensure, in addition
>>> to transitioning the retry fault to a no-retry fault, it
>>> also causes the wavefront to enter the trap handler. With the
>>> current setting, it only transitions to a no-retry fault.
>>
>> Well that was actually the intended result. Why should it enter the 
>> trap handler here?
>
> We need the trap handler for reporting the fault to the debugger. The 
> VM fault interrupt itself doesn't provide sufficient information to 
> find the wave and the program counter that triggered the fault. It 
> also doesn't work well when several waves are faulting on the same 
> address.

Interesting. But then we still have the problem that VSP set is actually 
a valid combination when this is applied to a non PTB.

So that really doesn't look like a good idea to me.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>>>
>>> Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index af6f26a97fc5..5df4f7bb241f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2488,7 +2488,7 @@ bool amdgpu_vm_handle_fault(struct 
>>> amdgpu_device *adev, u32 pasid,
>>>           /* Intentionally setting invalid PTE flag
>>>            * combination to force a no-retry-fault
>>>            */
>>> -        flags = AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT;
>>> +        flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SYSTEM | AMDGPU_PTE_PRT;
>>
>> As far as I can see this is actually a valid combination and would 
>> not have the desired result.
>>
>> Regards,
>> Christian.
>>
>>>           value = 0;
>>>       } else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
>>>           /* Redirect the access to the dummy page */
>>