[PATCH V2 11/17] drm/amd/pm: correct the usage for amdgpu_dpm_dispatch_task()

[Lazar, Lijo]

On 11/30/2021 1:12 PM, Evan Quan wrote:
> We should avoid having multi-function APIs. It should be up to the caller
> to determine when or whether to call amdgpu_dpm_dispatch_task().
> 
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> Change-Id: I78ec4eb8ceb6e526a4734113d213d15a5fbaa8a4
> ---
>   drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 18 ++----------------
>   drivers/gpu/drm/amd/pm/amdgpu_pm.c  | 26 ++++++++++++++++++++++++--
>   2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index c6299e406848..8f0ae58f4292 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -558,8 +558,6 @@ void amdgpu_dpm_set_power_state(struct amdgpu_device *adev,
>   				enum amd_pm_state_type state)
>   {
>   	adev->pm.dpm.user_state = state;
> -
> -	amdgpu_dpm_dispatch_task(adev, AMD_PP_TASK_ENABLE_USER_STATE, &state);
>   }
>   
>   enum amd_dpm_forced_level amdgpu_dpm_get_performance_level(struct amdgpu_device *adev)
> @@ -727,13 +725,7 @@ int amdgpu_dpm_set_sclk_od(struct amdgpu_device *adev, uint32_t value)
>   	if (!pp_funcs->set_sclk_od)
>   		return -EOPNOTSUPP;
>   
> -	pp_funcs->set_sclk_od(adev->powerplay.pp_handle, value);
> -
> -	amdgpu_dpm_dispatch_task(adev,
> -				 AMD_PP_TASK_READJUST_POWER_STATE,
> -				 NULL);
> -
> -	return 0;
> +	return pp_funcs->set_sclk_od(adev->powerplay.pp_handle, value);
>   }
>   
>   int amdgpu_dpm_get_mclk_od(struct amdgpu_device *adev)
> @@ -753,13 +745,7 @@ int amdgpu_dpm_set_mclk_od(struct amdgpu_device *adev, uint32_t value)
>   	if (!pp_funcs->set_mclk_od)
>   		return -EOPNOTSUPP;
>   
> -	pp_funcs->set_mclk_od(adev->powerplay.pp_handle, value);
> -
> -	amdgpu_dpm_dispatch_task(adev,
> -				 AMD_PP_TASK_READJUST_POWER_STATE,
> -				 NULL);
> -
> -	return 0;
> +	return pp_funcs->set_mclk_od(adev->powerplay.pp_handle, value);
>   }
>   
>   int amdgpu_dpm_get_power_profile_mode(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index fa2f4e11e94e..89e1134d660f 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -187,6 +187,10 @@ static ssize_t amdgpu_set_power_dpm_state(struct device *dev,
>   
>   	amdgpu_dpm_set_power_state(adev, state);
>   
> +	amdgpu_dpm_dispatch_task(adev,
> +				 AMD_PP_TASK_ENABLE_USER_STATE,
> +				 &state);
> +

This is just the opposite of what has been done so far. The idea is to 
keep the logic inside dpm_* calls and not to keep the logic in 
amdgpu_pm. This does the reverse. I guess this patch can be dropped.

Thanks,
Lijo

>   	pm_runtime_mark_last_busy(ddev->dev);
>   	pm_runtime_put_autosuspend(ddev->dev);
>   
> @@ -1278,7 +1282,16 @@ static ssize_t amdgpu_set_pp_sclk_od(struct device *dev,
>   		return ret;
>   	}
>   
> -	amdgpu_dpm_set_sclk_od(adev, (uint32_t)value);
> +	ret = amdgpu_dpm_set_sclk_od(adev, (uint32_t)value);
> +	if (ret) {
> +		pm_runtime_mark_last_busy(ddev->dev);
> +		pm_runtime_put_autosuspend(ddev->dev);
> +		return ret;
> +	}
> +
> +	amdgpu_dpm_dispatch_task(adev,
> +				 AMD_PP_TASK_READJUST_POWER_STATE,
> +				 NULL);
>   
>   	pm_runtime_mark_last_busy(ddev->dev);
>   	pm_runtime_put_autosuspend(ddev->dev);
> @@ -1340,7 +1353,16 @@ static ssize_t amdgpu_set_pp_mclk_od(struct device *dev,
>   		return ret;
>   	}
>   
> -	amdgpu_dpm_set_mclk_od(adev, (uint32_t)value);
> +	ret = amdgpu_dpm_set_mclk_od(adev, (uint32_t)value);
> +	if (ret) {
> +		pm_runtime_mark_last_busy(ddev->dev);
> +		pm_runtime_put_autosuspend(ddev->dev);
> +		return ret;
> +	}
> +
> +	amdgpu_dpm_dispatch_task(adev,
> +				 AMD_PP_TASK_READJUST_POWER_STATE,
> +				 NULL);
>   
>   	pm_runtime_mark_last_busy(ddev->dev);
>   	pm_runtime_put_autosuspend(ddev->dev);
>