On 2/6/2024 16:00, Deucher, Alexander wrote:
[AMD Official Use Only - General]
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Mario
Limonciello
Sent: Tuesday, February 6, 2024 4:32 PM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Jürg Billeter
<j@xxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxx>
---
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
