[PATCH] drm/amdgpu: always reset the asic in suspend

[PATCH] drm/amdgpu: always reset the asic in suspend

[Alex Deucher]

If the platform suspend happens to fail and the power rail
is not turned off, the GPU will be in an unknown state on
resume, so reset the asic so that it will be in a known
good state on resume even if the platform suspend failed.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 1db76429a673..42af3d88e0ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2165,8 +2165,9 @@ static int amdgpu_pmops_suspend(struct device *dev)
 	adev->in_s3 = true;
 	r = amdgpu_device_suspend(drm_dev, true);
 	adev->in_s3 = false;
-
-	return r;
+	if (r)
+		return r;
+	return amdgpu_asic_reset(adev);
 }
 
 static int amdgpu_pmops_resume(struct device *dev)
-- 
2.31.1

Re: [PATCH] drm/amdgpu: always reset the asic in suspend

[Alex Deucher]

Actually, ignore this for now.  This will likely cause problems with S0ix.

Alex

On Fri, Nov 12, 2021 at 11:18 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> If the platform suspend happens to fail and the power rail
> is not turned off, the GPU will be in an unknown state on
> resume, so reset the asic so that it will be in a known
> good state on resume even if the platform suspend failed.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 1db76429a673..42af3d88e0ba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2165,8 +2165,9 @@ static int amdgpu_pmops_suspend(struct device *dev)
>         adev->in_s3 = true;
>         r = amdgpu_device_suspend(drm_dev, true);
>         adev->in_s3 = false;
> -
> -       return r;
> +       if (r)
> +               return r;
> +       return amdgpu_asic_reset(adev);
>  }
>
>  static int amdgpu_pmops_resume(struct device *dev)
> --
> 2.31.1
>

Re: [PATCH] drm/amdgpu: always reset the asic in suspend

[Christian König]

I was just about to write up my concern as well.

IIRC we used to have that and it didn't really worked that well and we 
switched to resetting the GPU on driver load instead if initializing it 
doesn't work of hand.

Christian.

Am 12.11.21 um 17:19 schrieb Alex Deucher:
> Actually, ignore this for now.  This will likely cause problems with S0ix.
>
> Alex
>
> On Fri, Nov 12, 2021 at 11:18 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>> If the platform suspend happens to fail and the power rail
>> is not turned off, the GPU will be in an unknown state on
>> resume, so reset the asic so that it will be in a known
>> good state on resume even if the platform suspend failed.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 1db76429a673..42af3d88e0ba 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2165,8 +2165,9 @@ static int amdgpu_pmops_suspend(struct device *dev)
>>          adev->in_s3 = true;
>>          r = amdgpu_device_suspend(drm_dev, true);
>>          adev->in_s3 = false;
>> -
>> -       return r;
>> +       if (r)
>> +               return r;
>> +       return amdgpu_asic_reset(adev);
>>   }
>>
>>   static int amdgpu_pmops_resume(struct device *dev)
>> --
>> 2.31.1
>>

Re: [PATCH] drm/amdgpu: always reset the asic in suspend

[Deucher, Alexander]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

[AMD Official Use Only]

Well, that handles the case of the GPU needing to be reset on driver (e.g., virtualization), but doesn't handle the interrupted suspend case (e.g., when suspend is unwound before the power rail was turned off).  We already so something similar for hibernate to deal with the multiple freeze and thaw cycles.

Alex

________________________________
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Monday, November 15, 2021 8:41 AM
To: Alex Deucher <alexdeucher@gmail.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdgpu: always reset the asic in suspend

I was just about to write up my concern as well.

IIRC we used to have that and it didn't really worked that well and we
switched to resetting the GPU on driver load instead if initializing it
doesn't work of hand.

Christian.

Am 12.11.21 um 17:19 schrieb Alex Deucher:
> Actually, ignore this for now.  This will likely cause problems with S0ix.
>
> Alex
>
> On Fri, Nov 12, 2021 at 11:18 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>> If the platform suspend happens to fail and the power rail
>> is not turned off, the GPU will be in an unknown state on
>> resume, so reset the asic so that it will be in a known
>> good state on resume even if the platform suspend failed.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 1db76429a673..42af3d88e0ba 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2165,8 +2165,9 @@ static int amdgpu_pmops_suspend(struct device *dev)
>>          adev->in_s3 = true;
>>          r = amdgpu_device_suspend(drm_dev, true);
>>          adev->in_s3 = false;
>> -
>> -       return r;
>> +       if (r)
>> +               return r;
>> +       return amdgpu_asic_reset(adev);
>>   }
>>
>>   static int amdgpu_pmops_resume(struct device *dev)
>> --
>> 2.31.1
>>


[-- Attachment #2: Type: text/html, Size: 4153 bytes --]