[PATCH 1/2] drm/amd/pp: Remove manual mode for power_dpm_force_performance_level
Zhu, Rex
Rex.Zhu at amd.com
Fri Jan 26 12:50:09 UTC 2018
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/20376f6a/attachment-0001.html>
More information about the amd-gfx
mailing list