On Thu, Feb 8, 2024 at 1:48 AM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> 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.
>
> Explicitly set s0ix or s3 in the prepare() stage, and unset them if the
> prepare() stage failed.
>
> 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>
Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
> ---
> v3->v4:
> * New function to set s0ix/s3 and explicitly unset in cleanup
> v2->v3:
> * Whitespace
> v1->v2:
> * Add and use new in_prepare member
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 15 +++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 +++++++++--
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 3d8a48f46b01..f6c38a974bae 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1547,9 +1547,11 @@ static inline int amdgpu_acpi_smart_shift_update(struct drm_device *dev,
> #if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
> bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev);
> bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev);
> +void amdgpu_choose_low_power_state(struct amdgpu_device *adev);
> #else
> static inline bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev) { return false; }
> static inline bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev) { return false; }
> +static void amdgpu_choose_low_power_state(struct amdgpu_device *adev) { }
> #endif
>
> #if defined(CONFIG_DRM_AMD_DC)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 2deebece810e..cc21ed67a330 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1519,4 +1519,19 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
> #endif /* CONFIG_AMD_PMC */
> }
>
> +/**
> + * amdgpu_choose_low_power_state
> + *
> + * @adev: amdgpu_device_pointer
> + *
> + * Choose the target low power state for the GPU
> + */
> +void amdgpu_choose_low_power_state(struct amdgpu_device *adev)
> +{
> + if (amdgpu_acpi_is_s0ix_active(adev))
> + adev->in_s0ix = true;
> + else if (amdgpu_acpi_is_s3_active(adev))
> + adev->in_s3 = true;
> +}
> +
> #endif /* CONFIG_SUSPEND */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 2bc460cb993d..dab03865c827 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4518,13 +4518,15 @@ int amdgpu_device_prepare(struct drm_device *dev)
> struct amdgpu_device *adev = drm_to_adev(dev);
> int i, r;
>
> + amdgpu_choose_low_power_state(adev);
> +
> if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> return 0;
>
> /* Evict the majority of BOs before starting suspend sequence */
> r = amdgpu_device_evict_resources(adev);
> if (r)
> - return r;
> + goto unprepare;
>
> for (i = 0; i < adev->num_ip_blocks; i++) {
> if (!adev->ip_blocks[i].status.valid)
> @@ -4533,10 +4535,15 @@ int amdgpu_device_prepare(struct drm_device *dev)
> continue;
> r = adev->ip_blocks[i].version->funcs->prepare_suspend((void *)adev);
> if (r)
> - return r;
> + goto unprepare;
> }
>
> return 0;
> +
> +unprepare:
> + adev->in_s0ix = adev->in_s3 = false;
> +
> + return r;
> }
>
> /**
> --
> 2.34.1
>
