[PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset

[PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset

[Lang Yu]

drm sched is stopped and SDMA mode is not available,
while CPU mode worked well in such a case.

Use case,
amdgpu_do_asic_reset
amdgpu_device_ip_late_init
umsch_mm_late_init
umsch_mm_test
amdgpu_vm_init

Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8af3f0fd3073..af53f9cfcc40 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2404,8 +2404,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	vm->is_compute_context = false;
 
-	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
-				    AMDGPU_VM_USE_CPU_FOR_GFX);
+	vm->use_cpu_for_update = !!(amdgpu_in_reset(adev) ||
+		adev->vm_manager.vm_update_mode & AMDGPU_VM_USE_CPU_FOR_GFX);
 
 	DRM_DEBUG_DRIVER("VM update mode is %s\n",
 			 vm->use_cpu_for_update ? "CPU" : "SDMA");
-- 
2.25.1

RE: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset

[Yu, Lang]

[AMD Official Use Only - General]

Please ignore this one.

If user space calls amdgpu_vm_init between gpu reset start and end(driver doesn't prevent user space from doing that),

this will change VM update mode and affect user space. That's not expected.

Will find another way to solve this.

Regards,
Lang

>-----Original Message-----
>From: Yu, Lang <Lang.Yu@amd.com>
>Sent: Monday, March 25, 2024 1:36 PM
>To: amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian
><Christian.Koenig@amd.com>; Yu, Lang <Lang.Yu@amd.com>
>Subject: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset
>
>drm sched is stopped and SDMA mode is not available, while CPU mode
>worked well in such a case.
>
>Use case,
>amdgpu_do_asic_reset
>amdgpu_device_ip_late_init
>umsch_mm_late_init
>umsch_mm_test
>amdgpu_vm_init
>
>Signed-off-by: Lang Yu <Lang.Yu@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>index 8af3f0fd3073..af53f9cfcc40 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>@@ -2404,8 +2404,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>struct amdgpu_vm *vm,
>
>       vm->is_compute_context = false;
>
>-      vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode
>&
>-                                  AMDGPU_VM_USE_CPU_FOR_GFX);
>+      vm->use_cpu_for_update = !!(amdgpu_in_reset(adev) ||
>+              adev->vm_manager.vm_update_mode &
>AMDGPU_VM_USE_CPU_FOR_GFX);
>
>       DRM_DEBUG_DRIVER("VM update mode is %s\n",
>                        vm->use_cpu_for_update ? "CPU" : "SDMA");
>--
>2.25.1

Re: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset

[Christian König]

Am 25.03.24 um 06:35 schrieb Lang Yu:
> drm sched is stopped and SDMA mode is not available,
> while CPU mode worked well in such a case.
>
> Use case,
> amdgpu_do_asic_reset
> amdgpu_device_ip_late_init
> umsch_mm_late_init
> umsch_mm_test
> amdgpu_vm_init

Well big NAK to that.

The VM updates should just be scheduled and applied as soon as the GPU 
reset is completed.

The problem is rather that a GPU reset should *never* create a VM to do 
a test. During GPU reset no memory allocation whatsoever is allowed.

That's why we only do IB and ring tests with a pre-allocated memory pool 
during a GPU reset.

If the umsch_mm_test abuses the VM tests like this then please remove 
that code immediately.

Regards,
Christian.

>
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8af3f0fd3073..af53f9cfcc40 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2404,8 +2404,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   	vm->is_compute_context = false;
>   
> -	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
> -				    AMDGPU_VM_USE_CPU_FOR_GFX);
> +	vm->use_cpu_for_update = !!(amdgpu_in_reset(adev) ||
> +		adev->vm_manager.vm_update_mode & AMDGPU_VM_USE_CPU_FOR_GFX);
>   
>   	DRM_DEBUG_DRIVER("VM update mode is %s\n",
>   			 vm->use_cpu_for_update ? "CPU" : "SDMA");

RE: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset

[Yu, Lang]

[AMD Official Use Only - General]

>-----Original Message-----
>From: Koenig, Christian <Christian.Koenig@amd.com>
>Sent: Friday, March 29, 2024 7:08 PM
>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>Subject: Re: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU
>reset
>
>Am 25.03.24 um 06:35 schrieb Lang Yu:
>> drm sched is stopped and SDMA mode is not available, while CPU mode
>> worked well in such a case.
>>
>> Use case,
>> amdgpu_do_asic_reset
>> amdgpu_device_ip_late_init
>> umsch_mm_late_init
>> umsch_mm_test
>> amdgpu_vm_init
>
>Well big NAK to that.
>
>The VM updates should just be scheduled and applied as soon as the GPU
>reset is completed.
>
>The problem is rather that a GPU reset should *never* create a VM to do a
>test. During GPU reset no memory allocation whatsoever is allowed.

But user space can still create a VM via open("/dev/dri/card0", ...) during GPU reset,
driver doesn't prevent user space from doing that. So is this reasonable? Thanks.

Regards,
Lang

>That's why we only do IB and ring tests with a pre-allocated memory pool
>during a GPU reset.
>
>If the umsch_mm_test abuses the VM tests like this then please remove that
>code immediately.
>
>Regards,
>Christian.
>
>>
>> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8af3f0fd3073..af53f9cfcc40 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2404,8 +2404,8 @@ int amdgpu_vm_init(struct amdgpu_device
>*adev,
>> struct amdgpu_vm *vm,
>>
>>      vm->is_compute_context = false;
>>
>> -    vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode
>&
>> -                                AMDGPU_VM_USE_CPU_FOR_GFX);
>> +    vm->use_cpu_for_update = !!(amdgpu_in_reset(adev) ||
>> +            adev->vm_manager.vm_update_mode &
>AMDGPU_VM_USE_CPU_FOR_GFX);
>>
>>      DRM_DEBUG_DRIVER("VM update mode is %s\n",
>>                       vm->use_cpu_for_update ? "CPU" : "SDMA");

Re: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU reset

[Christian König]

Am 02.04.24 um 10:47 schrieb Yu, Lang:
> [AMD Official Use Only - General]
>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, March 29, 2024 7:08 PM
>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 1/2] drm/amdgpu: use CPU to update VM during GPU
>> reset
>>
>> Am 25.03.24 um 06:35 schrieb Lang Yu:
>>> drm sched is stopped and SDMA mode is not available, while CPU mode
>>> worked well in such a case.
>>>
>>> Use case,
>>> amdgpu_do_asic_reset
>>> amdgpu_device_ip_late_init
>>> umsch_mm_late_init
>>> umsch_mm_test
>>> amdgpu_vm_init
>> Well big NAK to that.
>>
>> The VM updates should just be scheduled and applied as soon as the GPU
>> reset is completed.
>>
>> The problem is rather that a GPU reset should *never* create a VM to do a
>> test. During GPU reset no memory allocation whatsoever is allowed.
> But user space can still create a VM via open("/dev/dri/card0", ...) during GPU reset,
> driver doesn't prevent user space from doing that. So is this reasonable? Thanks.

Yes the UMD can still create VMs during reset, but this is completely 
unproblematic since all submissions will wait till after the reset 
before they start executing.

This includes both VM updates as well as userspace submissions.

Regards,
Christian.

>
> Regards,
> Lang
>
>> That's why we only do IB and ring tests with a pre-allocated memory pool
>> during a GPU reset.
>>
>> If the umsch_mm_test abuses the VM tests like this then please remove that
>> code immediately.
>>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 8af3f0fd3073..af53f9cfcc40 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2404,8 +2404,8 @@ int amdgpu_vm_init(struct amdgpu_device
>> *adev,
>>> struct amdgpu_vm *vm,
>>>
>>>       vm->is_compute_context = false;
>>>
>>> -    vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode
>> &
>>> -                                AMDGPU_VM_USE_CPU_FOR_GFX);
>>> +    vm->use_cpu_for_update = !!(amdgpu_in_reset(adev) ||
>>> +            adev->vm_manager.vm_update_mode &
>> AMDGPU_VM_USE_CPU_FOR_GFX);
>>>       DRM_DEBUG_DRIVER("VM update mode is %s\n",
>>>                        vm->use_cpu_for_update ? "CPU" : "SDMA");