On Mon, Feb 20, 2023 at 11:56 AM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> > Sent: Monday, February 13, 2023 15:11
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Deucher, Alexander
> > <Alexander.Deucher@xxxxxxx>
> > 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 <Alexander.Deucher@xxxxxxx>
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > v1->v2:
> > * Don't run any suspend code or resume code in this case
>
> Any feedback for this patch?
Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
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
