> I disagree. the clock range is not independent of the profile. > Profiles correspond to use cases. Different use cases have different > clock requirements. For compute we want the clocks to be high. For > video you may have requirements for minimum mclks to ensure smooth > playback. For power saving you may want to limit maximum clocks, etc. > So setting the clock range independent of the profile in "auto" mode > does not make sense to me. >>I agree with that. The clocks should be part of the profile, at least as far as the automatic profile selection in the driver is concerned. >>The whole point of it is so that the driver can select a profile dynamically for a specific use case without requiring manual interaction from the user. For manually selecting profiles I >>can see Rex's point (you may want independent knobs for testing different parameters), but I think we should be consistent in the interface otherwise it's confusing to users. Rex: I also agree that clock should be part of the profile. My point is driver supply independent knobs to user. (pp_dpm_sclk/mclk force_dpm_level for select clock range.) And I just want to supply a new sysfs that user can configure dpm parameters(smu7: uphyst/downhyst/min_activity vega10: RLC,ent_point.etc). And as the parameters maybe not easy to understand for users, we supply some modes(VR, COMPUTE ) as example. So the problem maybe we should not name new sysfs as "power_profile_mode ". It is easy to add min sclk/mclk in the new sysfs, I just think it repeated with other sysfs. Best Regards Rex -----Original Message----- From: Alex Deucher [mailto:alexdeucher@xxxxxxxxx] Sent: Thursday, January 25, 2018 7:42 AM To: Kuehling, Felix Cc: Zhu, Rex; Huang, JinHuiEric; amd-gfx at lists.freedesktop.org Subject: Re: [PATCH 4/4] drm/amd/pp: Implement set_power_profile_mode on smu7 On Wed, Jan 24, 2018 at 6:12 PM, Felix Kuehling <felix.kuehling at amd.com> wrote: > On 2018-01-24 06:05 PM, Zhu, Rex wrote: >> Hi Felix, >> >> The logic of gfx/computer profile setting works in such way as you >> said,need under â??autoâ?? state. >> But in new sysfs of power profile setting, we do not check the >> performance Level. >> >> And The â??manualâ?? state is a confusing flagï¼?when user set manual >> stateï¼?and then change the clk range through pp-dpm-sclk/mclkï¼?the dpm >> still works automatically in new clock range, I think we can remove >> this flag. > > But then you're changing the semantics of how pp_dpm_sclk/mclk wok > together with pp_dpm_force_performance_level. > > Sysfs interfaces should be maintained stable. I'm OK with breaking the > old power profile stuff, because it only affects KFD which isn't > upstream yet. But the old pp_dpm_sclk/mclk and > pp_dpm_force_performance_level affect code that's currently upstream > and used by existing tools. > >> >> So My idea is >> in the sysfs of pp dpm sclk/mclk, user can set clock range. >> In the sysfs of power profile state, user can configure the >> parameters smu/driver exposed. They are independent. > > I disagree. the clock range is not independent of the profile. > Profiles correspond to use cases. Different use cases have different > clock requirements. For compute we want the clocks to be high. For > video you may have requirements for minimum mclks to ensure smooth > playback. For power saving you may want to limit maximum clocks, etc. > So setting the clock range independent of the profile in "auto" mode > does not make sense to me. I agree with that. The clocks should be part of the profile, at least as far as the automatic profile selection in the driver is concerned. The whole point of it is so that the driver can select a profile dynamically for a specific use case without requiring manual interaction from the user. For manually selecting profiles I can see Rex's point (you may want independent knobs for testing different parameters), but I think we should be consistent in the interface otherwise it's confusing to users. Alex > > Regards, > Felix > >> >> >> >> >> >> >> Best Regards >> Rex >> --------------------------------------------------------------------- >> --- >> *From:* Kuehling, Felix >> *Sent:* Thursday, January 25, 2018 5:41:43 AM >> *To:* Zhu, Rex; Huang, JinHuiEric; amd-gfx at lists.freedesktop.org >> *Subject:* Re: [PATCH 4/4] drm/amd/pp: Implement >> set_power_profile_mode on smu7 >> >> Hi Rex, >> >> As I understand it (the way power profiles currently work), >> pp_dpm_sclk/mclk only apply if pp_dpm_force_performance_level is set >> to "manual". Power profiles and automatic switching between profiles >> only happens when pp_dpm_force_performance_level is set to "auto". >> >> This means pp_dpm_sclk/mclk don't apply when profiles are in effect. >> Also, there would be no way to set different minimum clocks for >> different profiles. >> >> I think minimum clocks should be part of the profiles. >> >> Regards, >> Felix >> >> >> On 2018-01-24 03:13 PM, Zhu, Rex wrote: >> > Hi Eric, >> > >> > We have sysfs pp-dpm-sclk/mclk to set min dpm level >> > >> > Best Regards >> > Rex >> > ------------------------------------------------------------------- >> > ----- >> > *From:* amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf >> > of Eric Huang <jinhuieric.huang at amd.com> >> > *Sent:* Thursday, January 25, 2018 12:04:55 AM >> > *To:* amd-gfx at lists.freedesktop.org >> > *Subject:* Re: [PATCH 4/4] drm/amd/pp: Implement >> > set_power_profile_mode on smu7 >> > >> > We have min_sclk and min_mclk in previous power profile parameters >> > for VI, which are similar with min_active_level for Vega10. How to >> > implement these parameters? >> > >> > Regards, >> > Eric >> > >> > On 2018-01-24 04:37 AM, Rex Zhu wrote: >> > > User can set smu7 profile pamameters through sysfs >> > > >> > > echo "0/1/2/3/4">pp_power_profile_mode to select >> > > 3D_FULL_SCREEN/POWER_SAVING/VIDEO/VR/COMPUTE >> > > mode. >> > > echo "5 * * * * * * * *">pp_power_profile_mode to config custom >> > > mode. >> > > "5 * * * * * * * *" mean "CUSTOM enable_sclk SCLK_UP_HYST >> > > SCLK_DOWN_HYST SCLK_ACTIVE_LEVEL enable_mclk MCLK_UP_HYST >> > > MCLK_DOWN_HYST MCLK_ACTIVE_LEVEL" >> > > >> > > Change-Id: Ic6d6f37363bc81ab17051285f6ace847edf725de >> > > Signed-off-by: Rex Zhu <Rex.Zhu at amd.com> >> > > --- >> > > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 49 >> > +++++++++++++++++++++++- >> > > 1 file changed, 48 insertions(+), 1 deletion(-) >> > > >> > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> > b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> > > index 9f6afd5..13db75c 100644 >> > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> > > @@ -5036,7 +5036,54 @@ static int >> > > smu7_get_power_profile_mode(struct >> > pp_hwmgr *hwmgr, char *buf) >> > > >> > > static int smu7_set_power_profile_mode(struct pp_hwmgr *hwmgr, >> > long *input, uint32_t size) >> > > { >> > > - /* To Do */ >> > > + struct smu7_hwmgr *data = (struct smu7_hwmgr *)(hwmgr->backend); >> > > + struct profile_mode_setting tmp; >> > > + >> > > + hwmgr->power_profile_mode = input[size]; >> > > + >> > > + switch (hwmgr->power_profile_mode) { >> > > + case PP_SMC_POWER_PROFILE_CUSTOM: >> > > + if (size < 8) >> > > + return -EINVAL; >> > > + >> > > + data->custom_profile_setting.bupdate_sclk = input[0]; >> > > + data->custom_profile_setting.sclk_up_hyst = input[1]; >> > > + data->custom_profile_setting.sclk_down_hyst = input[2]; >> > > + data->custom_profile_setting.sclk_activity = input[3]; >> > > + data->custom_profile_setting.bupdate_mclk = input[4]; >> > > + data->custom_profile_setting.mclk_up_hyst = input[5]; >> > > + data->custom_profile_setting.mclk_down_hyst = input[6]; >> > > + data->custom_profile_setting.mclk_activity = input[7]; >> > > + if (!smum_update_dpm_settings(hwmgr, >> > &data->custom_profile_setting)) >> > > + memcpy(&data->current_profile_setting, >> > &data->custom_profile_setting, sizeof(struct >> > profile_mode_setting)); >> > > + break; >> > > + case PP_SMC_POWER_PROFILE_FULLSCREEN3D: >> > > + case PP_SMC_POWER_PROFILE_POWERSAVING: >> > > + case PP_SMC_POWER_PROFILE_VIDEO: >> > > + case PP_SMC_POWER_PROFILE_VR: >> > > + case PP_SMC_POWER_PROFILE_COMPUTE: >> > > + memcpy(&tmp, >> > &smu7_profiling[hwmgr->power_profile_mode], sizeof(struct >> > profile_mode_setting)); >> > > + if (!smum_update_dpm_settings(hwmgr, &tmp)) { >> > > + if (tmp.bupdate_sclk) { >> > > + >> > data->current_profile_setting.bupdate_sclk = tmp.bupdate_sclk; >> > > + >> > data->current_profile_setting.sclk_up_hyst = tmp.sclk_up_hyst; >> > > + >> > data->current_profile_setting.sclk_down_hyst = tmp.sclk_down_hyst; >> > > + >> > data->current_profile_setting.sclk_activity = tmp.sclk_activity; >> > > + } >> > > + if (tmp.bupdate_mclk) { >> > > + >> > data->current_profile_setting.bupdate_mclk = tmp.bupdate_mclk; >> > > + >> > data->current_profile_setting.mclk_up_hyst = tmp.mclk_up_hyst; >> > > + >> > data->current_profile_setting.mclk_down_hyst = tmp.mclk_down_hyst; >> > > + >> > data->current_profile_setting.mclk_activity = tmp.mclk_activity; >> > > + } >> > > + } >> > > + break; >> > > + case PP_SMC_POWER_PROFILE_AUTO: /* TO DO auto wattman >> > > + feature >> > not implement */ >> > > + return 0; >> > > + default: >> > > + return -EINVAL; >> > > + } >> > > + >> > > return 0; >> > > } >> > > >> > >> > _______________________________________________ >> > amd-gfx mailing list >> > amd-gfx at lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > >> > >> > _______________________________________________ >> > amd-gfx mailing list >> > amd-gfx at lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
