[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Thursday, December 9, 2021 8:37 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Feng, Kenneth <Kenneth.Feng@xxxxxxx>
> 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@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.
[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);
> >
