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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Thursday, December 9, 2021 8:37 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; Feng, Kenneth <Kenneth.Feng at amd.com>
> Subject: Re: [PATCH V4 11/17] drm/amd/pm: correct the usage for
> amdgpu_dpm_dispatch_task()
> 
> 
> 
> 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 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 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.
[Quan, Evan] With patch2 updated, it was found the tricky set_pp_sclk/mclk_od implementation prevent further optimization.
So, let's just drop this patch.

BR
Evan
> 
> 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);
> >