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

[sashiko-bot@kernel.org]

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