[AMD Official Use Only - General]
> -----Original Message-----
> From: Alex Deucher <alexdeucher@xxxxxxxxx>
> Sent: Monday, February 20, 2023 11:10
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>
> 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
> <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>
>
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
