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

[Christian König]

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?

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