[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Tuesday, November 30, 2021 9:48 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 V2 11/17] drm/amd/pm: correct the usage for
> amdgpu_dpm_dispatch_task()
>
>
>
> 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@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 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.
[Quan, Evan] The situation here is
1. in some cases the amdgpu_dpm_dispatch_task() is included/integrated. E.g. amdgpu_dpm_set_mclk_od() amdgpu_dpm_set_sclk_od
2. in other cases the amdgpu_dpm_dispatch_task() is called separately . E.g. by amdgpu_set_pp_force_state() and amdgpu_set_pp_od_clk_voltage() from amdgpu_pm.c
They will make the thing that adds a unified lock protection on those amdgpu_dpm_xxx() APIs tricky. To resolve that, we either
1. separate the amdgpu_dpm_dispatch_task() from those APIs(amdgpu_dpm_set_mclk_od() amdgpu_dpm_set_sclk_od())
2. try to get amdgpu_dpm_dispatch_task() included also in amdgpu_set_pp_force_state() and amdgpu_set_pp_od_clk_voltage()
After some considerations, I believe 1 is the more proper way. As the current implementation of amdgpu_dpm_set_mclk_od() really combines two logics separately things together.
The amdgpu_dpm_dispatch_task() should be splitted out.
BR
Evan
>
> 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);
> >
