Re: [PATCH v4 3/3] drm/amd: Drop unneeded functions to check if s3/s0ix active

[Mario Limonciello <mario.limonciello@xxxxxxx>]

On 2/8/2024 00:54, Christian König wrote:
> Am 08.02.24 um 06:52 schrieb Mario Limonciello:
> > amdgpu_acpi_is_s0ix_active() and amdgpu_acpi_is_s0ix_active() aren't
> > needed to be checked multiple times in a suspend cycle.  Checking and
> > setting up policy one time in the prepare() callback is sufficient.
>
> Mhm, looking at amdgpu_acpi_is_s3_active() I think we should not cache 
> that in a variable in the first place.
>
> Just calling the function all the time to check the state should be 
> sufficient, or do we then run into any state transition problems?

I think calling to check it each time is perfectly fine, it should never 
change once the sequence starts until it's over.

If the first 2 patches look OK, I'd like to get those merged so we can 
fix the regressions.  I'll do another series that can drop the cached 
parameters.

> Regards,
> Christian.
>
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > v4: New patch
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h      |  4 ----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |  7 +++----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 17 ++---------------
> >   3 files changed, 5 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index f6c38a974bae..53823539eba5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1545,12 +1545,8 @@ static inline int 
> > amdgpu_acpi_smart_shift_update(struct drm_device *dev,
> >   #endif
> >   #if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
> > -bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev);
> > -bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev);
> >   void amdgpu_choose_low_power_state(struct amdgpu_device *adev);
> >   #else
> > -static inline bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device 
> > *adev) { return false; }
> > -static inline bool amdgpu_acpi_is_s3_active(struct amdgpu_device 
> > *adev) { return false; }
> >   static void amdgpu_choose_low_power_state(struct amdgpu_device 
> > *adev) { }
> >   #endif
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > index cc21ed67a330..1d58728f8c3f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > @@ -1366,8 +1366,7 @@ bool amdgpu_acpi_should_gpu_reset(struct 
> > amdgpu_device *adev)
> >           adev->gfx.imu.funcs) /* Not need to do mode2 reset for IMU 
> > enabled APUs */
> >           return false;
> > -    if ((adev->flags & AMD_IS_APU) &&
> > -        amdgpu_acpi_is_s3_active(adev))
> > +    if ((adev->flags & AMD_IS_APU) && adev->in_s3)
> >           return false;
> >       if (amdgpu_sriov_vf(adev))
> > @@ -1472,7 +1471,7 @@ void amdgpu_acpi_release(void)
> >    *
> >    * returns true if supported, false if not.
> >    */
> > -bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> > +static inline bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> >   {
> >       return !(adev->flags & AMD_IS_APU) ||
> >           (pm_suspend_target_state == PM_SUSPEND_MEM);
> > @@ -1485,7 +1484,7 @@ bool amdgpu_acpi_is_s3_active(struct 
> > amdgpu_device *adev)
> >    *
> >    * returns true if supported, false if not.
> >    */
> > -bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
> > +static inline bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device 
> > *adev)
> >   {
> >       if (!(adev->flags & AMD_IS_APU) ||
> >           (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 971acf01bea6..2bc4c5bb9b5a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -2456,13 +2456,6 @@ 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;
> > -
> >       return amdgpu_device_prepare(drm_dev);
> >   }
> > @@ -2476,10 +2469,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);
> > @@ -2510,10 +2499,8 @@ static int amdgpu_pmops_resume(struct device *dev)
> >           adev->no_hw_access = true;
> >       r = amdgpu_device_resume(drm_dev, true);
> > -    if (amdgpu_acpi_is_s0ix_active(adev))
> > -        adev->in_s0ix = false;
> > -    else
> > -        adev->in_s3 = false;
> > +    adev->in_s0ix = adev->in_s3 = false;
> > +
> >       return r;
> >   }