[PATCH 4/4] drm/amd/pp: Implement set_power_profile_mode on smu7
Felix Kuehling
felix.kuehling at amd.com
Thu Jan 25 16:31:40 UTC 2018
On 2018-01-25 06:48 AM, Zhu, Rex wrote:
>>> That's a change in the pp_dpm_sclk/mclk interface. Currenly these only work in manual mode and fail or are ignored in auto mode. E.g. see this code in smu7_hwmgr.c:
>>> static int smu7_force_clock_level(struct pp_hwmgr *hwmgr,
> The attached patch can fix this issue.
> Thanks.
How do you unforce the clock levels with your patch? Currently switching
from "manual" back to "auto" will call smu7_unforce_dpm_levels. However,
if you are in "auto" mode all the time, that won't happen. So you're
still breaking the interface.
Regards,
Felix
>
>
> Best Regards
> Rex
> -----Original Message-----
> From: Kuehling, Felix
> Sent: Thursday, January 25, 2018 12:16 PM
> 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
>
> On 2018-01-24 09:28 PM, Zhu, Rex wrote:
>>> But then you're changing the semantics of how pp_dpm_sclk/mclk wok
>>> together with pp_dpm_force_performance_level.
>> Rex:The two sysfs are all for clock range setting.
>>
>>
>> pp_dpm_sclk/mclk/pcie can set sclk/mclk/pcie range separately.
>>
> Only in manual mode. If you change that, you're changing the interface and breaking existing tools.
>
>> and pp_dpm_force_performance_level, we can support low/high/umd-pstate
>> that set all the clocks range.
>>
>> (manual state, driver do nothing except set the flag)
>>
>>
>> No matter what state user enter in, driver can support to continue to
>> change the clock range through pp_dpm_sclk/mclk/pcie.
>>
>>
>> so in fact, don't need manual state for sepalately set the clock range.
>>
> That's a change in the pp_dpm_sclk/mclk interface. Currenly these only work in manual mode and fail or are ignored in auto mode. E.g. see this code in smu7_hwmgr.c:
>
> static int smu7_force_clock_level(struct pp_hwmgr *hwmgr,
> enum pp_clock_type type, uint32_t mask) {
> 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;
>
> Regards,
> Felix
>
>
>>> 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.
>>
>> Rex: As the old power profile interface can't support Vega.
>>
>> so add new interface and we are try to support old asics with new sysfs.
>>
>>
>>
>>> But the old pp_dpm_sclk/mclk and
>>> pp_dpm_force_performance_level affect code that's currently upstream
>>> and used by existing tools.
>>
>> Rex: Yes, we are trying to maintain the sysfs stable.
>> if we change the old sysfs code, we will try to not affect existing
>> tools.
>>
>> Best Regards
>> Rex
>>
>>
>> ----------------------------------------------------------------------
>> --
>> *From:* amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of
>> Felix Kuehling <felix.kuehling at amd.com>
>> *Sent:* Thursday, January 25, 2018 7:12 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
>>
>> 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.
>>
>> 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 Info Page - freedesktop.org
>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>> lists.freedesktop.org
>> Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the
>> following form. Use of all freedesktop.org lists is subject to our
>> Code of ...
>>
>>
>>
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> amd-gfx Info Page - freedesktop.org
>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>> lists.freedesktop.org
>> Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the
>> following form. Use of all freedesktop.org lists is subject to our
>> Code of ...
>>
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> amd-gfx Info Page - freedesktop.org
>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>> lists.freedesktop.org
>> Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the
>> following form. Use of all freedesktop.org lists is subject to our
>> Code of ...
>>
>>
>>
More information about the amd-gfx
mailing list