Re: [PATCH] drm/amd: Set s0i3/s3 in prepare() callback instead of suspend() callback
On 2/6/2024 16:00, Deucher, Alexander wrote:
[AMD Official Use Only - General]
-Original Message-
From: amd-gfx On Behalf Of Mario
Limonciello
Sent: Tuesday, February 6, 2024 4:32 PM
To: amd-gfx@lists.freedesktop.org
Cc: Limonciello, Mario ; Jürg Billeter
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
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
---
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
RE: [PATCH] drm/amd: Set s0i3/s3 in prepare() callback instead of suspend() callback
[AMD Official Use Only - General]
> -Original Message-
> From: amd-gfx On Behalf Of Mario
> Limonciello
> Sent: Tuesday, February 6, 2024 4:32 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Limonciello, Mario ; Jürg Billeter
>
> 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
> 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
> ---
> 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?
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
[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
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
---
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;
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
