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

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

[willy]

From: Willy VanSickle <vansicklewilly@gmail.com>
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@gmail.com>
---
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;

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The fix for spurious dGPU resets on s2idle resume is incomplete, leaving identically flawed logic in soc21.c and soc24.c.
--

commit 5105966b236eb1d3339925f9a22bee109b13037a
Author: Willy VanSickle <vansicklewilly@gmail.com>

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

This commit fixes a spurious mode1 reset issue on dGPUs resuming from
s2idle. It updates soc15_need_reset_on_resume() to verify that the
suspend state was actually PM_SUSPEND_MEM and not resumed via firmware.

> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index 3d4573d937422..bec63957f3221 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -592,7 +592,12 @@ static bool soc15_need_reset_on_resume(struct amdgpu_device *adev)
>  	 * 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;

[Severity: High]
This is a pre-existing issue, but does this incomplete fix leave the same
spurious reset bug in soc21.c and soc24.c?

Looking at drivers/gpu/drm/amd/amdgpu/soc21.c in
soc21_need_reset_on_resume():

	if (!(adev->flags & AMD_IS_APU) && adev->in_s3) {
		sol_reg1 = RREG32_SOC15(MP0, 0, regMP0_SMN_C2PMSG_81);
		msleep(100);
		sol_reg2 = RREG32_SOC15(MP0, 0, regMP0_SMN_C2PMSG_81);

		return (sol_reg1 != sol_reg2);
	}

And identically in drivers/gpu/drm/amd/amdgpu/soc24.c in
soc24_need_reset_on_resume():

	if (!(adev->flags & AMD_IS_APU) && adev->in_s3) {
		sol_reg1 = RREG32_SOC15(MP0, 0, regMPASP_SMN_C2PMSG_81);
		msleep(100);
		sol_reg2 = RREG32_SOC15(MP0, 0, regMPASP_SMN_C2PMSG_81);

		return (sol_reg1 != sol_reg2);
	}

It appears adev->in_s3 will incorrectly evaluate to true on s2idle resumes
for dGPUs here as well, causing an unnecessary 100ms delay and a subsequent
spurious ASIC reset. Should the PM_SUSPEND_MEM and pm_resume_via_firmware()
checks be applied to these files too?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010001a074b60857-4cb0239e-c529-4069-ba4e-e347cfe379a4-000000@email.amazonses.com?part=1