RE: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
[AMD Official Use Only - General]
> -Original Message-
> From: Alex Deucher
> Sent: Monday, February 20, 2023 11:10
> To: Limonciello, Mario
> Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander
>
> Subject: Re: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
>
> On Mon, Feb 20, 2023 at 11:56 AM Limonciello, Mario
> wrote:
> >
> > [Public]
> >
> >
> >
> > > -Original Message-
> > > From: Limonciello, Mario
> > > Sent: Monday, February 13, 2023 15:11
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Limonciello, Mario ; Deucher,
> Alexander
> > >
> > > Subject: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
> > >
> > > APUs before Raven didn't support s0ix. As we just relieved some
> > > of the safety checks for s0ix to improve power consumption on
> > > APUs that support it but that are missing BIOS support a new
> > > blind spot was introduced that a user could "try" to run s0ix.
> > >
> > > Plug this hole so that if users try to run s0ix on anything older
> > > than Raven it will just skip suspend of the GPU.
> > >
> > > Fixes: cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > > Suggested-by: Alexander Deucher
> > > Signed-off-by: Mario Limonciello
> > > ---
> > > v1->v2:
> > > * Don't run any suspend code or resume code in this case
> >
> > Any feedback for this patch?
>
> Reviewed-by: Alex Deucher
>
Thanks.
> I think for S0ix and dGPUs, we probably need some additional work as
> well. If the user tries s2idle and the platform doesn't actually
> support s0ix (i.e., doesn't actually turn off the power rails), we
> should be using the runtime suspend routines for BACO/BOCO rather than
> the S3 suspend routines.
OK - I'll review the framework code for that case and see what makes sense.
>
> Alex
>
>
> >
> > > ---
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 3 +++
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 ++-
> > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > index fa7375b97fd47..25e902077caf6 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > @@ -1073,6 +1073,9 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > > amdgpu_device *adev)
> > > (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> > > return false;
> > >
> > > + if (adev->asic_type < CHIP_RAVEN)
> > > + return false;
> > > +
> > > /*
> > >* If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is
> > > generally
> > >* risky to do any special firmware-related preparations for
> > > entering
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > index 6c2fe50b528e0..1f6d93dc3d72b 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > @@ -2414,8 +2414,10 @@ static int amdgpu_pmops_suspend(struct
> device
> > > *dev)
> > >
> > > if (amdgpu_acpi_is_s0ix_active(adev))
> > > adev->in_s0ix = true;
> > > - else
> > > + 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);
> > > }
> > >
> > > @@ -2436,6 +2438,9 @@ static int amdgpu_pmops_resume(struct device
> > > *dev)
> > > struct amdgpu_device *adev = drm_to_adev(drm_dev);
> > > int r;
> > >
> > > + if (!adev->in_s0ix && !adev->in_s3)
> > > + return 0;
> > > +
> > > /* Avoids registers access if device is physically gone */
> > > if (!pci_device_is_present(adev->pdev))
> > > adev->no_hw_access = true;
> > > --
> > > 2.25.1
Re: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
On Mon, Feb 20, 2023 at 11:56 AM Limonciello, Mario
wrote:
>
> [Public]
>
>
>
> > -Original Message-
> > From: Limonciello, Mario
> > Sent: Monday, February 13, 2023 15:11
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Limonciello, Mario ; Deucher, Alexander
> >
> > Subject: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
> >
> > APUs before Raven didn't support s0ix. As we just relieved some
> > of the safety checks for s0ix to improve power consumption on
> > APUs that support it but that are missing BIOS support a new
> > blind spot was introduced that a user could "try" to run s0ix.
> >
> > Plug this hole so that if users try to run s0ix on anything older
> > than Raven it will just skip suspend of the GPU.
> >
> > Fixes: cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > Suggested-by: Alexander Deucher
> > Signed-off-by: Mario Limonciello
> > ---
> > v1->v2:
> > * Don't run any suspend code or resume code in this case
>
> Any feedback for this patch?
Reviewed-by: Alex Deucher
I think for S0ix and dGPUs, we probably need some additional work as
well. If the user tries s2idle and the platform doesn't actually
support s0ix (i.e., doesn't actually turn off the power rails), we
should be using the runtime suspend routines for BACO/BOCO rather than
the S3 suspend routines.
Alex
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 3 +++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 ++-
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > index fa7375b97fd47..25e902077caf6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > @@ -1073,6 +1073,9 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > amdgpu_device *adev)
> > (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> > return false;
> >
> > + if (adev->asic_type < CHIP_RAVEN)
> > + return false;
> > +
> > /*
> >* If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is
> > generally
> >* risky to do any special firmware-related preparations for entering
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 6c2fe50b528e0..1f6d93dc3d72b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -2414,8 +2414,10 @@ static int amdgpu_pmops_suspend(struct device
> > *dev)
> >
> > if (amdgpu_acpi_is_s0ix_active(adev))
> > adev->in_s0ix = true;
> > - else
> > + 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);
> > }
> >
> > @@ -2436,6 +2438,9 @@ static int amdgpu_pmops_resume(struct device
> > *dev)
> > struct amdgpu_device *adev = drm_to_adev(drm_dev);
> > int r;
> >
> > + if (!adev->in_s0ix && !adev->in_s3)
> > + return 0;
> > +
> > /* Avoids registers access if device is physically gone */
> > if (!pci_device_is_present(adev->pdev))
> > adev->no_hw_access = true;
> > --
> > 2.25.1
RE: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
[Public]
> -Original Message-
> From: Limonciello, Mario
> Sent: Monday, February 13, 2023 15:11
> To: amd-gfx@lists.freedesktop.org
> Cc: Limonciello, Mario ; Deucher, Alexander
>
> Subject: [PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
>
> APUs before Raven didn't support s0ix. As we just relieved some
> of the safety checks for s0ix to improve power consumption on
> APUs that support it but that are missing BIOS support a new
> blind spot was introduced that a user could "try" to run s0ix.
>
> Plug this hole so that if users try to run s0ix on anything older
> than Raven it will just skip suspend of the GPU.
>
> Fixes: cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> Suggested-by: Alexander Deucher
> Signed-off-by: Mario Limonciello
> ---
> v1->v2:
> * Don't run any suspend code or resume code in this case
Any feedback for this patch?
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 3 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 ++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index fa7375b97fd47..25e902077caf6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1073,6 +1073,9 @@ bool amdgpu_acpi_is_s0ix_active(struct
> amdgpu_device *adev)
> (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> return false;
>
> + if (adev->asic_type < CHIP_RAVEN)
> + return false;
> +
> /*
>* If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is
> generally
>* risky to do any special firmware-related preparations for entering
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 6c2fe50b528e0..1f6d93dc3d72b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2414,8 +2414,10 @@ static int amdgpu_pmops_suspend(struct device
> *dev)
>
> if (amdgpu_acpi_is_s0ix_active(adev))
> adev->in_s0ix = true;
> - else
> + 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);
> }
>
> @@ -2436,6 +2438,9 @@ static int amdgpu_pmops_resume(struct device
> *dev)
> struct amdgpu_device *adev = drm_to_adev(drm_dev);
> int r;
>
> + if (!adev->in_s0ix && !adev->in_s3)
> + return 0;
> +
> /* Avoids registers access if device is physically gone */
> if (!pci_device_is_present(adev->pdev))
> adev->no_hw_access = true;
> --
> 2.25.1
[PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven
APUs before Raven didn't support s0ix. As we just relieved some
of the safety checks for s0ix to improve power consumption on
APUs that support it but that are missing BIOS support a new
blind spot was introduced that a user could "try" to run s0ix.
Plug this hole so that if users try to run s0ix on anything older
than Raven it will just skip suspend of the GPU.
Fixes: cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
Suggested-by: Alexander Deucher
Signed-off-by: Mario Limonciello
---
v1->v2:
* Don't run any suspend code or resume code in this case
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 3 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 ++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index fa7375b97fd47..25e902077caf6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1073,6 +1073,9 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device
*adev)
(pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
return false;
+ if (adev->asic_type < CHIP_RAVEN)
+ return false;
+
/*
* If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is generally
* risky to do any special firmware-related preparations for entering
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 6c2fe50b528e0..1f6d93dc3d72b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2414,8 +2414,10 @@ static int amdgpu_pmops_suspend(struct device *dev)
if (amdgpu_acpi_is_s0ix_active(adev))
adev->in_s0ix = true;
- else
+ 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);
}
@@ -2436,6 +2438,9 @@ static int amdgpu_pmops_resume(struct device *dev)
struct amdgpu_device *adev = drm_to_adev(drm_dev);
int r;
+ if (!adev->in_s0ix && !adev->in_s3)
+ return 0;
+
/* Avoids registers access if device is physically gone */
if (!pci_device_is_present(adev->pdev))
adev->no_hw_access = true;
--
2.25.1
