[PATCH v2] drm/amd: Don't allow s0ix on APUs older than Raven

[Limonciello, Mario]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Monday, February 20, 2023 11:10
> To: Limonciello, Mario <Mario.Limonciello at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; Deucher, Alexander
> <Alexander.Deucher at amd.com>
> 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 at amd.com> wrote:
> >
> > [Public]
> >
> >
> >
> > > -----Original Message-----
> > > From: Limonciello, Mario <Mario.Limonciello at amd.com>
> > > Sent: Monday, February 13, 2023 15:11
> > > To: amd-gfx at lists.freedesktop.org
> > > Cc: Limonciello, Mario <Mario.Limonciello at amd.com>; Deucher,
> Alexander
> > > <Alexander.Deucher at amd.com>
> > > 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 at amd.com>
> > > Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> > > ---
> > > 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 at amd.com>
> 

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