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

[Christian König]

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 at amd.com>
>>> ---
>>> 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;
>>>   }
>>
>