[PATCH V2 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: Tuesday, November 30, 2021 9:48 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 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 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.
[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);
> >