[PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[Shikang Fan]

- evict_resource is taking too long causing sriov full access mode timeout.
  So, add an extra evict_resource in the beginning as an early evict.
- Move the original evict_resource after ip_suspend2.

Signed-off-by: Shikang Fan <shikang.fan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 818fa72c670d..8b7db87cffd9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4111,6 +4111,10 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 
 	adev->in_suspend = true;
 
+	r = amdgpu_device_evict_resources(adev);
+	if (r)
+		return r;
+
 	if (amdgpu_sriov_vf(adev)) {
 		amdgpu_virt_fini_data_exchange(adev);
 		r = amdgpu_virt_request_full_gpu(adev, false);
@@ -4135,14 +4139,14 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 	if (!adev->in_s0ix)
 		amdgpu_amdkfd_suspend(adev, adev->in_runpm);
 
-	r = amdgpu_device_evict_resources(adev);
-	if (r)
-		return r;
-
 	amdgpu_fence_driver_hw_fini(adev);
 
 	amdgpu_device_ip_suspend_phase2(adev);
 
+	r = amdgpu_device_evict_resources(adev);
+	if (r)
+		return r;
+
 	if (amdgpu_sriov_vf(adev))
 		amdgpu_virt_release_full_gpu(adev, false);
 
-- 
2.25.1

Re: [PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[Christian König]

Am 08.12.22 um 10:25 schrieb Shikang Fan:
> - evict_resource is taking too long causing sriov full access mode timeout.
>    So, add an extra evict_resource in the beginning as an early evict.
> - Move the original evict_resource after ip_suspend2.
>
> Signed-off-by: Shikang Fan <shikang.fan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 818fa72c670d..8b7db87cffd9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4111,6 +4111,10 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>   
>   	adev->in_suspend = true;
>   
> +	r = amdgpu_device_evict_resources(adev);
> +	if (r)
> +		return r;
> +
>   	if (amdgpu_sriov_vf(adev)) {
>   		amdgpu_virt_fini_data_exchange(adev);
>   		r = amdgpu_virt_request_full_gpu(adev, false);
> @@ -4135,14 +4139,14 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>   	if (!adev->in_s0ix)
>   		amdgpu_amdkfd_suspend(adev, adev->in_runpm);
>   
> -	r = amdgpu_device_evict_resources(adev);
> -	if (r)
> -		return r;
> -
>   	amdgpu_fence_driver_hw_fini(adev);
>   
>   	amdgpu_device_ip_suspend_phase2(adev);
>   
> +	r = amdgpu_device_evict_resources(adev);
> +	if (r)
> +		return r;
> +

As noted internally please keep this evict resources call where it was.

It makes sense to evict the BOs which were previously pinned by display 
with the SDMA engine.

Only the final eviction of BOs for fw etc.. should be done with the CPU.

I suggest to also add a comment to each call explaining why we need it.

Regards,
Christian.

>   	if (amdgpu_sriov_vf(adev))
>   		amdgpu_virt_release_full_gpu(adev, false);
>   

RE: [PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[Fan, Shikang]

[AMD Official Use Only - General]

Hi Christian, 
http://gerrit-git.amd.com/c/brahma/ec/linux/+/620522 In this patch I saw that there was originally two evict in the suspend and this patch the second one (the one after ip_suspend2) were removed. I am a little bit confused on this, should I just keep the way it is and just add an extra evict in the beginning?

Thanks,
Shikang.

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Thursday, December 8, 2022 5:30 PM
To: Fan, Shikang <Shikang.Fan@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

Am 08.12.22 um 10:25 schrieb Shikang Fan:
> - evict_resource is taking too long causing sriov full access mode timeout.
>    So, add an extra evict_resource in the beginning as an early evict.
> - Move the original evict_resource after ip_suspend2.
>
> Signed-off-by: Shikang Fan <shikang.fan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 818fa72c670d..8b7db87cffd9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4111,6 +4111,10 @@ int amdgpu_device_suspend(struct drm_device 
> *dev, bool fbcon)
>   
>   	adev->in_suspend = true;
>   
> +	r = amdgpu_device_evict_resources(adev);
> +	if (r)
> +		return r;
> +
>   	if (amdgpu_sriov_vf(adev)) {
>   		amdgpu_virt_fini_data_exchange(adev);
>   		r = amdgpu_virt_request_full_gpu(adev, false); @@ -4135,14 
> +4139,14 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>   	if (!adev->in_s0ix)
>   		amdgpu_amdkfd_suspend(adev, adev->in_runpm);
>   
> -	r = amdgpu_device_evict_resources(adev);
> -	if (r)
> -		return r;
> -
>   	amdgpu_fence_driver_hw_fini(adev);
>   
>   	amdgpu_device_ip_suspend_phase2(adev);
>   
> +	r = amdgpu_device_evict_resources(adev);
> +	if (r)
> +		return r;
> +

As noted internally please keep this evict resources call where it was.

It makes sense to evict the BOs which were previously pinned by display with the SDMA engine.

Only the final eviction of BOs for fw etc.. should be done with the CPU.

I suggest to also add a comment to each call explaining why we need it.

Regards,
Christian.

>   	if (amdgpu_sriov_vf(adev))
>   		amdgpu_virt_release_full_gpu(adev, false);
>   

Re: [PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[Christian König]

Hi Shikang,

good point.

The double checking this the second eviction was removed by Nirmoy 
because we didn't needed it any more. The GART table and fw images are 
now just uploaded to VRAM again after resume. I didn't had that info in 
my mind any more, so sorry my fault :)

Adding and extra eviction before grabbing full access makes sense 
because we can then move the majority of the buffers out of VRAM without 
worrying about any timeouts.

What's left are the display BOs which can only be evicted after phase1 
is completed (because that turn of the display engine). But we should 
still do this before phase2 because we want to use the hw accelerated 
DMA for this.

Otherwise we will run into trouble because a) CPU accesses are not 
always possible and b) take way more time than doing it with the DMA.

I suggest to just add the extra eviction with a comment above like /* 
Evict the majority of BOs before grabbing the full access */.

Regards,
Christian.

Am 08.12.22 um 10:58 schrieb Fan, Shikang:
> [AMD Official Use Only - General]
>
> Hi Christian,
> http://gerrit-git.amd.com/c/brahma/ec/linux/+/620522 In this patch I saw that there was originally two evict in the suspend and this patch the second one (the one after ip_suspend2) were removed. I am a little bit confused on this, should I just keep the way it is and just add an extra evict in the beginning?
>
> Thanks,
> Shikang.
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Thursday, December 8, 2022 5:30 PM
> To: Fan, Shikang <Shikang.Fan@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.
>
> Am 08.12.22 um 10:25 schrieb Shikang Fan:
>> - evict_resource is taking too long causing sriov full access mode timeout.
>>     So, add an extra evict_resource in the beginning as an early evict.
>> - Move the original evict_resource after ip_suspend2.
>>
>> Signed-off-by: Shikang Fan <shikang.fan@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 ++++++++----
>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 818fa72c670d..8b7db87cffd9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -4111,6 +4111,10 @@ int amdgpu_device_suspend(struct drm_device
>> *dev, bool fbcon)
>>    
>>    	adev->in_suspend = true;
>>    
>> +	r = amdgpu_device_evict_resources(adev);
>> +	if (r)
>> +		return r;
>> +
>>    	if (amdgpu_sriov_vf(adev)) {
>>    		amdgpu_virt_fini_data_exchange(adev);
>>    		r = amdgpu_virt_request_full_gpu(adev, false); @@ -4135,14
>> +4139,14 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>>    	if (!adev->in_s0ix)
>>    		amdgpu_amdkfd_suspend(adev, adev->in_runpm);
>>    
>> -	r = amdgpu_device_evict_resources(adev);
>> -	if (r)
>> -		return r;
>> -
>>    	amdgpu_fence_driver_hw_fini(adev);
>>    
>>    	amdgpu_device_ip_suspend_phase2(adev);
>>    
>> +	r = amdgpu_device_evict_resources(adev);
>> +	if (r)
>> +		return r;
>> +
> As noted internally please keep this evict resources call where it was.
>
> It makes sense to evict the BOs which were previously pinned by display with the SDMA engine.
>
> Only the final eviction of BOs for fw etc.. should be done with the CPU.
>
> I suggest to also add a comment to each call explaining why we need it.
>
> Regards,
> Christian.
>
>>    	if (amdgpu_sriov_vf(adev))
>>    		amdgpu_virt_release_full_gpu(adev, false);
>>    

[PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[Shikang Fan]

- evict_resource is taking too long causing sriov full access mode timeout.
  So, add an extra evict_resource in the beginning as an early evict.

Signed-off-by: Shikang Fan <shikang.fan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 818fa72c670d..b76e8fdfd266 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4111,6 +4111,11 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
 
 	adev->in_suspend = true;
 
+	/* Evict the majority of BOs before grabbing the full access */
+	r = amdgpu_device_evict_resources(adev);
+	if (r)
+		return r;
+
 	if (amdgpu_sriov_vf(adev)) {
 		amdgpu_virt_fini_data_exchange(adev);
 		r = amdgpu_virt_request_full_gpu(adev, false);
-- 
2.25.1

Re: [PATCH] drm/amdgpu: Add an extra evict_resource call during device_suspend.

[Christian König]

Am 08.12.22 um 12:55 schrieb Shikang Fan:
> - evict_resource is taking too long causing sriov full access mode timeout.
>    So, add an extra evict_resource in the beginning as an early evict.
>
> Signed-off-by: Shikang Fan <shikang.fan@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 818fa72c670d..b76e8fdfd266 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4111,6 +4111,11 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
>   
>   	adev->in_suspend = true;
>   
> +	/* Evict the majority of BOs before grabbing the full access */
> +	r = amdgpu_device_evict_resources(adev);
> +	if (r)
> +		return r;
> +
>   	if (amdgpu_sriov_vf(adev)) {
>   		amdgpu_virt_fini_data_exchange(adev);
>   		r = amdgpu_virt_request_full_gpu(adev, false);