[PATCH] drm/amdgpu: only treat a real S3 as a suspend abort on resume

[willy]

From: Willy VanSickle <vansicklewilly@xxxxxxxxx>
Subject: [PATCH] drm/amdgpu: only treat a real S3 as a suspend abort on resume

soc15_need_reset_on_resume() resets the ASIC when
`adev->in_s3 && !pm_resume_via_firmware()`, to recover from an aborted
S3 suspend. Since commit 38e8ca3e4b6d ("amdgpu/soc15: enable asic
reset for dGPU in case of suspend abort") this applies to dGPUs as
well.

Two things make that check fire on every suspend-to-idle resume of a
dGPU, not just on aborts:

 - amdgpu_acpi_is_s3_active() returns true for any non-APU regardless
   of the sleep state actually used, so in_s3 is set on s2idle too.
 - pm_resume_via_firmware() is only true when platform firmware
   resumed the system (ACPI S3). s2idle never sets it.

So on an s2idle platform every dGPU resume is misread as an S3 abort
and gets a mode1 reset it did not need. On most parts the spurious
reset is survivable. On a MacBookPro15,3 (Radeon Pro Vega 20, PSP
v11) the PSP goes into suspend with a failed TA unload and the mode1
issued on that state leaves the bootloader unresponsive:

  amdgpu 0000:03:00.0: S3 suspend abort case, let's reset ASIC.
  amdgpu 0000:03:00.0: [drm] psp mode1 reset succeed
  amdgpu 0000:03:00.0: PSP is resuming...
  amdgpu 0000:03:00.0: psp reg (0x16063) wait timed out, mask: 80000000,
    read: 0 exp: 80000000
  amdgpu 0000:03:00.0: PSP load sys drv failed!
  amdgpu 0000:03:00.0: resume of IP block <psp> failed -62

The check was introduced by commit 58a8c756fc4c ("drm/amdgpu: correct the
S3 abort check condition") for APUs, on the basis that the PM core sets
PM_SUSPEND_FLAG_FW_RESUME when a real S3 completes, so its absence on
resume means the S3 was aborted. That holds for S3. It does not hold
for s2idle, where firmware is never involved, and commit 38e8ca3e4b6d
("amdgpu/soc15: enable asic reset for dGPU in case of suspend abort")
dropped the APU gate without adding a check on the sleep state.

The driver already caches the sleep state it suspended with in
adev->last_suspend_state. Use it: only a suspend whose target was
PM_SUSPEND_MEM and which firmware did not resume can be an S3 abort.

With this the Vega 20 resumes cleanly from s2idle (PSP up, all rings
back) on 7.2.2 + t2linux patches; without it every resume fails as
above.

Fixes: 38e8ca3e4b6d ("amdgpu/soc15: enable asic reset for dGPU in case of suspend abort")
Signed-off-by: Willy VanSickle <vansicklewilly@xxxxxxxxx>
---
Tested on a MacBookPro15,3 (Radeon Pro Vega 20, PSP v11) running 7.2.2
with the t2linux patch set, s2idle only. Verified twice: without the gate
every resume fails with psp -62; with it the PSP and all rings come back.

 drivers/gpu/drm/amd/amdgpu/soc15.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -591,7 +591,12 @@
 	 * 1) S3 suspend aborted in the normal S3 suspend
 	 * 2) S3 suspend aborted in performing pm core test.
 	 */
-	if (adev->in_s3 && !pm_resume_via_firmware())
+	/* Only a real S3 (mem) that firmware did not resume is an abort.
+	 * s2idle never resumes via firmware, so without this gate every
+	 * s2idle resume of a dGPU is misread as an abort and mode1-reset.
+	 */
+	if (adev->in_s3 && !pm_resume_via_firmware() &&
+	    adev->last_suspend_state == PM_SUSPEND_MEM)
 		return true;
 	else
 		return false;