>1. You're breaking the semantics of the existing pp_dpm_sclk/mclk/pcie
> interfaces, which affects existing tools
Rex: I don't think the patch will affects existing tools.
User set "manual" to power_performance_level, and then change the clock range through pp_dpm_sclk/mclk/pcie.
with this patch, User dont need to set "manual" command, if still receive the manual command, driver just return sucess to user in order not break existing
tools.
>2. You're taking the clock limits out of the power profile.
> Automatically adjusting the minimum sclk/mclk is a requirement for
> the compute power profile
Rex: In vega10, under default comput mode(with busy_set_point/fps/use_rlc_busy/min_active_level set), just two performance levels left
(level0 and level7). and clock just switch between lowest and highest.
I am not sure in this case, driver still can set min sclk/mclk.
Best Regards
Rex
________________________________
From: Kuehling, Felix
Sent: Saturday, January 27, 2018 12:49 AM
To: Zhu, Rex; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/amd/pp: Remove manual mode for power_dpm_force_performance_level
Hi Rex,
I think I understand what you're trying to do. To summarize my concerns,
there are two reasons I'm against your plan:
1. You're breaking the semantics of the existing pp_dpm_sclk/mclk/pcie
interfaces, which affects existing tools
2. You're taking the clock limits out of the power profile.
Automatically adjusting the minimum sclk/mclk is a requirement for
the compute power profile
Regards,
Felix
On 2018-01-26 07:50 AM, Zhu, Rex wrote:
>
> Hi Felix,
>
>
> >That would make sense. But switching to manual mode would disable
> >profiles and automatic profile selection. That was one reason why I
> >objected to your plan to control profile clock limits using these files.
>
> Rex:
>
>
> I am not very clear the old logic of gfx/compute power profile switch.
>
>
> But with new sysfs,
>
>
>
> The logic is(those sysfs are independent)
>
> 1. configure uphyst/downhyst/min_ativity through power_profile_mode,
>
> 2. adjust clock range through pp_dpm_sclk/mclk/pcie.(once this
> sysffs was called, set the dpm level mode to unknown)
>
> 3. adjust power limit through pp_od_power_limit(maybe equal to
> disable power containment).
>
>
>
> In those functions, driver do not check the dpm level mode.
>
> the dpm level mode just used by power_dpm_force_performance_level
> functions.
>
>
> Best Regards
>
> Rex
>
>
>
>
>
> ------------------------------------------------------------------------
> *From:* Kuehling, Felix
> *Sent:* Friday, January 26, 2018 8:26 AM
> *To:* Zhu, Rex; amd-gfx at lists.freedesktop.org
> *Subject:* Re: [PATCH 1/2] drm/amd/pp: Remove manual mode for
> power_dpm_force_performance_level
>
> On 2018-01-25 07:07 PM, Zhu, Rex wrote:
> > I also think about this problem.
> > just think user should unforced clk level through pp dpm
> > sclk/mclk/pcie if they change the clock logic through those sysfs.
> >
> > The logic seems weird, As we supply many sysfs for adjust clock range.
> >
> > We can fix this problem by change current mode to manual mode after
> > user call pp dpm sclk/mclk/pcie.
> >
> > But another thinkï¼?if user change back the clk range through pp dpm clk.
> >
> > we are in manual mode, and user set auto mode, in fact, driver change
> > nothing.
>
> With profiles, switching back to auto mode would select the appropriate
> profile, which may have a different clock mask. For example for compute
> we enable only the highest two sclk levels.
>
> >
> > Comparatively speaking, better set manual mode after user call pp
> dpm clk.
>
> That would make sense. But switching to manual mode would disable
> profiles and automatic profile selection. That was one reason why I
> objected to your plan to control profile clock limits using these files.
>
> Regards,
> Felix
>
> > Thanks very much.
> >
> > Best Regards
> > Rex
> > ------------------------------------------------------------------------
> > *From:* Kuehling, Felix
> > *Sent:* Friday, January 26, 2018 12:55:19 AM
> > *To:* amd-gfx at lists.freedesktop.org; Zhu, Rex
> > *Subject:* Re: [PATCH 1/2] drm/amd/pp: Remove manual mode for
> > power_dpm_force_performance_level
> >
> > This patch breaks unforcing of clocks, which is currently done by
> > switching back from "manual" to "auto". By removing "manual" mode, you
> > remove the ability to unset forced clocks.
> >
> > Regards,
> > Felix
> >
> >
> > On 2018-01-25 06:26 AM, Rex Zhu wrote:
> > > Driver do not maintain manual mode for dpm_force_performance_level,
> > > User can set sclk/mclk/pcie range through
> > pp_dpm_sclk/pp_dpm_mclk/pp_dpm_pcie
> > > directly.
> > >
> > > In order to not break currently tools,
> > > when set "manual" to power_dpm_force_performance_level
> > > driver will do nothing and just return successful.
> > >
> > > Change-Id: Iaf672b9abc7fa57b765ceb7fa2fba6ad3e80c50b
> > > Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
> > > ---
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
> > > drivers/gpu/drm/amd/amdgpu/ci_dpm.c | 5 -----
> > > drivers/gpu/drm/amd/include/kgd_pp_interface.h | 15
> +++++++--------
> > > drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 4 ----
> > > drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c | 1 -
> > > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ------
> > > drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 6 ------
> > > 7 files changed, 8 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > index 1812009..66b4df0 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > @@ -152,7 +152,6 @@ static ssize_t
> > amdgpu_get_dpm_forced_performance_level(struct device *dev,
> > > (level == AMD_DPM_FORCED_LEVEL_AUTO) ? "auto" :
> > > (level == AMD_DPM_FORCED_LEVEL_LOW) ? "low" :
> > > (level == AMD_DPM_FORCED_LEVEL_HIGH) ? "high" :
> > > - (level == AMD_DPM_FORCED_LEVEL_MANUAL) ?
> > "manual" :
> > > (level ==
> > AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD) ? "profile_standard" :
> > > (level ==
> > AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK) ? "profile_min_sclk" :
> > > (level ==
> > AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK) ? "profile_min_mclk" :
> > > @@ -186,7 +185,7 @@ static ssize_t
> > amdgpu_set_dpm_forced_performance_level(struct device *dev,
> > > } else if (strncmp("auto", buf, strlen("auto")) == 0) {
> > > level = AMD_DPM_FORCED_LEVEL_AUTO;
> > > } else if (strncmp("manual", buf, strlen("manual")) == 0) {
> > > - level = AMD_DPM_FORCED_LEVEL_MANUAL;
> > > + pr_info("No need to set manual mode, Just go ahead\n");
> > > } else if (strncmp("profile_exit", buf,
> > strlen("profile_exit")) == 0) {
> > > level = AMD_DPM_FORCED_LEVEL_PROFILE_EXIT;
> > > } else if (strncmp("profile_standard", buf,
> > strlen("profile_standard")) == 0) {
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
> > b/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
> > > index ab45232..8ddc978 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/ci_dpm.c
> > > @@ -6639,11 +6639,6 @@ static int ci_dpm_force_clock_level(void
> *handle,
> > > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > struct ci_power_info *pi = ci_get_pi(adev);
> > >
> > > - if (adev->pm.dpm.forced_level & (AMD_DPM_FORCED_LEVEL_AUTO |
> > > - AMD_DPM_FORCED_LEVEL_LOW |
> > > - AMD_DPM_FORCED_LEVEL_HIGH))
> > > - return -EINVAL;
> > > -
> > > switch (type) {
> > > case PP_SCLK:
> > > if (!pi->sclk_dpm_key_disabled)
> > > diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > index b9aa9f4..3fab686 100644
> > > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > @@ -41,14 +41,13 @@ struct amd_vce_state {
> > >
> > > enum amd_dpm_forced_level {
> > > AMD_DPM_FORCED_LEVEL_AUTO = 0x1,
> > > - AMD_DPM_FORCED_LEVEL_MANUAL = 0x2,
> > > - AMD_DPM_FORCED_LEVEL_LOW = 0x4,
> > > - AMD_DPM_FORCED_LEVEL_HIGH = 0x8,
> > > - AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD = 0x10,
> > > - AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK = 0x20,
> > > - AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK = 0x40,
> > > - AMD_DPM_FORCED_LEVEL_PROFILE_PEAK = 0x80,
> > > - AMD_DPM_FORCED_LEVEL_PROFILE_EXIT = 0x100,
> > > + AMD_DPM_FORCED_LEVEL_LOW = 0x2,
> > > + AMD_DPM_FORCED_LEVEL_HIGH = 0x4,
> > > + AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD = 0x8,
> > > + AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK = 0x10,
> > > + AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK = 0x20,
> > > + AMD_DPM_FORCED_LEVEL_PROFILE_PEAK = 0x40,
> > > + AMD_DPM_FORCED_LEVEL_PROFILE_EXIT = 0x80,
> > > };
> > >
> > > enum amd_pm_state_type {
> > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > > index dec8dd9..60d280c 100644
> > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > > @@ -1250,7 +1250,6 @@ static int cz_dpm_force_dpm_level(struct
> > pp_hwmgr *hwmgr,
> > > case AMD_DPM_FORCED_LEVEL_AUTO:
> > > ret = cz_phm_unforce_dpm_levels(hwmgr);
> > > break;
> > > - case AMD_DPM_FORCED_LEVEL_MANUAL:
> > > case AMD_DPM_FORCED_LEVEL_PROFILE_EXIT:
> > > default:
> > > break;
> > > @@ -1558,9 +1557,6 @@ static int cz_get_dal_power_level(struct
> > pp_hwmgr *hwmgr,
> > > static int cz_force_clock_level(struct pp_hwmgr *hwmgr,
> > > enum pp_clock_type type, uint32_t mask)
> > > {
> > > - if (hwmgr->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL)
> > > - return -EINVAL;
> > > -
> > > switch (type) {
> > > case PP_SCLK:
> > > smum_send_msg_to_smc_with_parameter(hwmgr,
> > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> > > index 409a56b..eddcbcd 100644
> > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> > > @@ -605,7 +605,6 @@ static int rv_dpm_force_dpm_level(struct
> > pp_hwmgr *hwmgr,
> > >
> > PPSMC_MSG_SetSoftMaxFclkByFreq,
> > >
> > RAVEN_UMD_PSTATE_MIN_FCLK);
> > > break;
> > > - case AMD_DPM_FORCED_LEVEL_MANUAL:
> > > case AMD_DPM_FORCED_LEVEL_PROFILE_EXIT:
> > > default:
> > > break;
> > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > > index 13db75c..e3a8374 100644
> > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > > @@ -2798,7 +2798,6 @@ static int smu7_force_dpm_level(struct
> > pp_hwmgr *hwmgr,
> > > smu7_force_clock_level(hwmgr, PP_MCLK, 1<<mclk_mask);
> > > smu7_force_clock_level(hwmgr, PP_PCIE, 1<<pcie_mask);
> > > break;
> > > - case AMD_DPM_FORCED_LEVEL_MANUAL:
> > > case AMD_DPM_FORCED_LEVEL_PROFILE_EXIT:
> > > default:
> > > break;
> > > @@ -4311,11 +4310,6 @@ static int smu7_force_clock_level(struct
> > pp_hwmgr *hwmgr,
> > > {
> > > struct smu7_hwmgr *data = (struct smu7_hwmgr
> *)(hwmgr->backend);
> > >
> > > - if (hwmgr->request_dpm_level & (AMD_DPM_FORCED_LEVEL_AUTO |
> > > - AMD_DPM_FORCED_LEVEL_LOW |
> > > - AMD_DPM_FORCED_LEVEL_HIGH))
> > > - return -EINVAL;
> > > -
> > > switch (type) {
> > > case PP_SCLK:
> > > if (!data->sclk_dpm_key_disabled)
> > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> > > index 6b28896..828677e 100644
> > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> > > @@ -4241,7 +4241,6 @@ static int vega10_dpm_force_dpm_level(struct
> > pp_hwmgr *hwmgr,
> > > vega10_force_clock_level(hwmgr, PP_SCLK, 1<<sclk_mask);
> > > vega10_force_clock_level(hwmgr, PP_MCLK, 1<<mclk_mask);
> > > break;
> > > - case AMD_DPM_FORCED_LEVEL_MANUAL:
> > > case AMD_DPM_FORCED_LEVEL_PROFILE_EXIT:
> > > default:
> > > break;
> > > @@ -4500,11 +4499,6 @@ static int vega10_force_clock_level(struct
> > pp_hwmgr *hwmgr,
> > > {
> > > struct vega10_hwmgr *data = (struct vega10_hwmgr
> > *)(hwmgr->backend);
> > >
> > > - if (hwmgr->request_dpm_level & (AMD_DPM_FORCED_LEVEL_AUTO |
> > > - AMD_DPM_FORCED_LEVEL_LOW |
> > > - AMD_DPM_FORCED_LEVEL_HIGH))
> > > - return -EINVAL;
> > > -
> > > switch (type) {
> > > case PP_SCLK:
> > > data->smc_state_table.gfx_boot_level = mask ?
> > (ffs(mask) - 1) : 0;
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180126/dd165c9a/attachment-0001.html>
