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

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 12/3/2021 8:35 AM, 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@xxxxxxx>
> 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 6d9db2e2cbd3..32bf1247fb60 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -554,8 +554,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)
> @@ -723,13 +721,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)
> @@ -749,13 +741,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);
> +
>   	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);

amdgpu_set_pp_sclk_od has a verbatim API like amdgpu_dpm_set_sclk_od and 
one would expect that to handle everything required to set the clock.

If locking is the problem, then it should be handled differently. This 
kind of mixing is not the right way.

Thanks,
Lijo

> +	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);