[PATCH] drm/amd: Set s0i3/s3 in prepare() callback instead of suspend() callback

[Mario Limonciello]

On 2/6/2024 16:00, Deucher, Alexander wrote:
> [AMD Official Use Only - General]
> 
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Mario
>> Limonciello
>> Sent: Tuesday, February 6, 2024 4:32 PM
>> To: amd-gfx at lists.freedesktop.org
>> Cc: Limonciello, Mario <Mario.Limonciello at amd.com>; Jürg Billeter
>> <j at bitron.ch>
>> Subject: [PATCH] drm/amd: Set s0i3/s3 in prepare() callback instead of
>> suspend() callback
>>
>> commit 5095d5418193 ("drm/amd: Evict resources during PM ops prepare()
>> callback") intentionally moved the eviction of resources to earlier in the
>> suspend process, but this introduced a subtle change that it occurs before
>> adev->in_s0ix or adev->in_s3 are set. This meant that APUs actually started to
>> evict resources at suspend time as well.
>>
>> Move the s0i3/s3 setting flags into prepare() to ensure that they're set during
>> eviction. Drop the existing call to return 1 in this case because the suspend()
>> callback looks for the flags too.
>>
>> Reported-by: Jürg Billeter <j at bitron.ch>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-
>> /issues/3132#note_2271038
>> Fixes: 5095d5418193 ("drm/amd: Evict resources during PM ops prepare()
>> callback")
>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 ++++----------
>>   1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index b74f68a15802..190b2ee9e36b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2464,12 +2464,10 @@ static int amdgpu_pmops_prepare(struct device
>> *dev)
>>            pm_runtime_suspended(dev))
>>                return 1;
>>
>> -     /* if we will not support s3 or s2i for the device
>> -      *  then skip suspend
>> -      */
>> -     if (!amdgpu_acpi_is_s0ix_active(adev) &&
>> -         !amdgpu_acpi_is_s3_active(adev))
>> -             return 1;
>> +     if (amdgpu_acpi_is_s0ix_active(adev))
>> +             adev->in_s0ix = true;
>> +     else if (amdgpu_acpi_is_s3_active(adev))
>> +             adev->in_s3 = true;
>>
> 
> Will resume always get called to clear these after after prepare?  Will these ever get set and then not unset?

You're right; it doesn't clean up.

This is the call sequence:

suspend_devices_and_enter()
->dpm_suspend_start()
->->device_prepare()
->->->dpm_prepare()

Errors bubble up.  In suspend_devices_and_enter() errors goto 
Recover_platform label.  This calls platform_recover().

platform_recover() is for platform recovery not device recovery.
So this patch is incorrect.

Let me see if I can come up with another way to do this without having 
to revert 5095d5418193.

> 
> Alex
> 
>>        return amdgpu_device_prepare(drm_dev);  } @@ -2484,10 +2482,6
>> @@ static int amdgpu_pmops_suspend(struct device *dev)
>>        struct drm_device *drm_dev = dev_get_drvdata(dev);
>>        struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>
>> -     if (amdgpu_acpi_is_s0ix_active(adev))
>> -             adev->in_s0ix = true;
>> -     else if (amdgpu_acpi_is_s3_active(adev))
>> -             adev->in_s3 = true;
>>        if (!adev->in_s0ix && !adev->in_s3)
>>                return 0;
>>        return amdgpu_device_suspend(drm_dev, true);
>> --
>> 2.34.1
>