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

[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.

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