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

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 08.02.24 um 16:04 schrieb Mario Limonciello:
> 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.

Feel free to add my Acked-by to the series, but for ACPI stuff I would 
wait for Alex to take a look as well.

Regards,
Christian.

> > 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;
> > >   }