| Message ID | 20230131052141.96475-6-wyes.karny@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | amd_pstate: Add guided autonomous mode support | expand |
On 1/30/23 23:21, Wyes Karny wrote: > amd_pstate driver's `status` sysfs entry helps to control the driver's > mode dynamically by user. After the addition of guided mode the > combinations of mode transitions have been increased (16 combinations). > Therefore optimise the amd_pstate_update_status function by implementing > a state transition table. > > There are 4 states amd_pstate supports, namely: 'disable', 'passive', > 'active', and 'guided'. The transition from any state to any other > state is possible after this change. Only if the state requested matches > with the current state then -EBUSY value is returned. I realized this after I finished reviewing doc patch, but you probably want to explain -EBUSY return code in documentation patch too. > > Sysfs interface: > > To disable amd_pstate driver: > # echo disable > /sys/devices/system/cpu/amd_pstate/status > > To enable passive mode: > # echo passive > /sys/devices/system/cpu/amd_pstate/status > > To change mode to active: > # echo active > /sys/devices/system/cpu/amd_pstate/status > > To change mode to guided: > # echo guided > /sys/devices/system/cpu/amd_pstate/status > > Signed-off-by: Wyes Karny <wyes.karny@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 150 +++++++++++++++++++++++++---------- > 1 file changed, 108 insertions(+), 42 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 48ab4684c3a5..6c522dec6967 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -65,6 +65,8 @@ static struct cpufreq_driver amd_pstate_epp_driver; > static int cppc_state = AMD_PSTATE_DISABLE; > struct kobject *amd_pstate_kobj; > > +typedef int (*cppc_mode_transition_fn)(int); > + > static inline int get_mode_idx_from_str(const char *str, size_t size) > { > int i; > @@ -797,6 +799,105 @@ static ssize_t show_energy_performance_preference( > return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]); > } > > +static void amd_pstate_driver_cleanup(void) > +{ > + amd_pstate_enable(false); > + cppc_state = AMD_PSTATE_DISABLE; > + current_pstate_driver = NULL; > +} > + > +static int amd_pstate_register_driver(int mode) > +{ > + int ret; > + > + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED) > + current_pstate_driver = &amd_pstate_driver; > + else if (mode == AMD_PSTATE_ACTIVE) > + current_pstate_driver = &amd_pstate_epp_driver; > + else > + return -EINVAL; > + > + cppc_state = mode; > + ret = cpufreq_register_driver(current_pstate_driver); > + if (ret) { > + amd_pstate_driver_cleanup(); > + return ret; > + } > + return 0; > +} > + > +static int amd_pstate_unregister_driver(int dummy) > +{ > + int ret; > + > + ret = cpufreq_unregister_driver(current_pstate_driver); > + > + if (ret) > + return ret; > + > + amd_pstate_driver_cleanup(); > + return 0; > +} > + > +static int amd_pstate_change_mode_without_dvr_change(int mode) > +{ > + int cpu = 0; > + > + cppc_state = mode; > + > + if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE) > + return 0; > + > + for_each_present_cpu(cpu) { > + cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1); > + } > + > + return 0; > +} > + > +static int amd_pstate_change_driver_mode(int mode) > +{ > + int ret; > + > + ret = amd_pstate_unregister_driver(0); > + if (ret) > + return ret; > + > + ret = amd_pstate_register_driver(mode); > + if (ret) > + return ret; > + > + return 0; > +} > + > +/* Mode transition table */ This seems to be a pointless comment to me. > +cppc_mode_transition_fn mode_state_machine[AMD_PSTATE_MAX][AMD_PSTATE_MAX] = { > + [AMD_PSTATE_DISABLE] = { > + [AMD_PSTATE_DISABLE] = NULL, > + [AMD_PSTATE_PASSIVE] = amd_pstate_register_driver, > + [AMD_PSTATE_ACTIVE] = amd_pstate_register_driver, > + [AMD_PSTATE_GUIDED] = amd_pstate_register_driver, > + }, > + [AMD_PSTATE_PASSIVE] = { > + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, > + [AMD_PSTATE_PASSIVE] = NULL, > + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, > + [AMD_PSTATE_GUIDED] = amd_pstate_change_mode_without_dvr_change, > + }, > + [AMD_PSTATE_ACTIVE] = { > + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, > + [AMD_PSTATE_PASSIVE] = amd_pstate_change_driver_mode, > + [AMD_PSTATE_ACTIVE] = NULL, > + [AMD_PSTATE_GUIDED] = amd_pstate_change_driver_mode, > + }, > + [AMD_PSTATE_GUIDED] = { > + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, > + [AMD_PSTATE_PASSIVE] = amd_pstate_change_mode_without_dvr_change, > + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, > + [AMD_PSTATE_GUIDED] = NULL, > + }, > +}; > + > static ssize_t amd_pstate_show_status(char *buf) > { > if (!current_pstate_driver) > @@ -805,57 +906,22 @@ static ssize_t amd_pstate_show_status(char *buf) > return sysfs_emit(buf, "%s\n", amd_pstate_mode_string[cppc_state]); > } > > -static void amd_pstate_driver_cleanup(void) > -{ > - current_pstate_driver = NULL; > -} > - > static int amd_pstate_update_status(const char *buf, size_t size) > { > - int ret; > int mode_idx; > > - if (size > 7 || size < 6) > + if (size > strlen("passive") || size < strlen("active")) > return -EINVAL; > - mode_idx = get_mode_idx_from_str(buf, size); > > - switch(mode_idx) { > - case AMD_PSTATE_DISABLE: > - if (!current_pstate_driver) > - return -EINVAL; > - if (cppc_state == AMD_PSTATE_ACTIVE) > - return -EBUSY; > - ret = cpufreq_unregister_driver(current_pstate_driver); > - amd_pstate_driver_cleanup(); > - break; > - case AMD_PSTATE_PASSIVE: > - if (current_pstate_driver) { > - if (current_pstate_driver == &amd_pstate_driver) > - return 0; > - cpufreq_unregister_driver(current_pstate_driver); > - cppc_state = AMD_PSTATE_PASSIVE; > - current_pstate_driver = &amd_pstate_driver; > - } > + mode_idx = get_mode_idx_from_str(buf, size); > > - ret = cpufreq_register_driver(current_pstate_driver); > - break; > - case AMD_PSTATE_ACTIVE: > - if (current_pstate_driver) { > - if (current_pstate_driver == &amd_pstate_epp_driver) > - return 0; > - cpufreq_unregister_driver(current_pstate_driver); > - current_pstate_driver = &amd_pstate_epp_driver; > - cppc_state = AMD_PSTATE_ACTIVE; > - } > + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX) > + return -EINVAL; > > - ret = cpufreq_register_driver(current_pstate_driver); > - break; > - default: > - ret = -EINVAL; > - break; > - } > + if (mode_state_machine[cppc_state][mode_idx]) > + return mode_state_machine[cppc_state][mode_idx](mode_idx); > > - return ret; > + return -EBUSY; > } > > static ssize_t show_status(struct kobject *kobj, With one nit fixed, Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
On 31 Jan 07:45, Mario Limonciello wrote: > On 1/30/23 23:21, Wyes Karny wrote: > > amd_pstate driver's `status` sysfs entry helps to control the driver's > > mode dynamically by user. After the addition of guided mode the > > combinations of mode transitions have been increased (16 combinations). > > Therefore optimise the amd_pstate_update_status function by implementing > > a state transition table. > > > > There are 4 states amd_pstate supports, namely: 'disable', 'passive', > > 'active', and 'guided'. The transition from any state to any other > > state is possible after this change. Only if the state requested matches > > with the current state then -EBUSY value is returned. > > I realized this after I finished reviewing doc patch, but you probably want > to explain -EBUSY return code in documentation patch too. Yes, I'll add this to documentation. Thanks! > > > > > Sysfs interface: > > > > To disable amd_pstate driver: > > # echo disable > /sys/devices/system/cpu/amd_pstate/status > > > > To enable passive mode: > > # echo passive > /sys/devices/system/cpu/amd_pstate/status > > > > To change mode to active: > > # echo active > /sys/devices/system/cpu/amd_pstate/status > > > > To change mode to guided: > > # echo guided > /sys/devices/system/cpu/amd_pstate/status > > > > Signed-off-by: Wyes Karny <wyes.karny@amd.com> > > --- > > drivers/cpufreq/amd-pstate.c | 150 +++++++++++++++++++++++++---------- > > 1 file changed, 108 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > > index 48ab4684c3a5..6c522dec6967 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -65,6 +65,8 @@ static struct cpufreq_driver amd_pstate_epp_driver; > > static int cppc_state = AMD_PSTATE_DISABLE; > > struct kobject *amd_pstate_kobj; > > +typedef int (*cppc_mode_transition_fn)(int); > > + > > static inline int get_mode_idx_from_str(const char *str, size_t size) > > { > > int i; > > @@ -797,6 +799,105 @@ static ssize_t show_energy_performance_preference( > > return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]); > > } > > +static void amd_pstate_driver_cleanup(void) > > +{ > > + amd_pstate_enable(false); > > + cppc_state = AMD_PSTATE_DISABLE; > > + current_pstate_driver = NULL; > > +} > > + > > +static int amd_pstate_register_driver(int mode) > > +{ > > + int ret; > > + > > + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED) > > + current_pstate_driver = &amd_pstate_driver; > > + else if (mode == AMD_PSTATE_ACTIVE) > > + current_pstate_driver = &amd_pstate_epp_driver; > > + else > > + return -EINVAL; > > + > > + cppc_state = mode; > > + ret = cpufreq_register_driver(current_pstate_driver); > > + if (ret) { > > + amd_pstate_driver_cleanup(); > > + return ret; > > + } > > + return 0; > > +} > > + > > +static int amd_pstate_unregister_driver(int dummy) > > +{ > > + int ret; > > + > > + ret = cpufreq_unregister_driver(current_pstate_driver); > > + > > + if (ret) > > + return ret; > > + > > + amd_pstate_driver_cleanup(); > > + return 0; > > +} > > + > > +static int amd_pstate_change_mode_without_dvr_change(int mode) > > +{ > > + int cpu = 0; > > + > > + cppc_state = mode; > > + > > + if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE) > > + return 0; > > + > > + for_each_present_cpu(cpu) { > > + cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1); > > + } > > + > > + return 0; > > +} > > + > > +static int amd_pstate_change_driver_mode(int mode) > > +{ > > + int ret; > > + > > + ret = amd_pstate_unregister_driver(0); > > + if (ret) > > + return ret; > > + > > + ret = amd_pstate_register_driver(mode); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +/* Mode transition table */ > > This seems to be a pointless comment to me. > > > +cppc_mode_transition_fn mode_state_machine[AMD_PSTATE_MAX][AMD_PSTATE_MAX] = { > > + [AMD_PSTATE_DISABLE] = { > > + [AMD_PSTATE_DISABLE] = NULL, > > + [AMD_PSTATE_PASSIVE] = amd_pstate_register_driver, > > + [AMD_PSTATE_ACTIVE] = amd_pstate_register_driver, > > + [AMD_PSTATE_GUIDED] = amd_pstate_register_driver, > > + }, > > + [AMD_PSTATE_PASSIVE] = { > > + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, > > + [AMD_PSTATE_PASSIVE] = NULL, > > + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, > > + [AMD_PSTATE_GUIDED] = amd_pstate_change_mode_without_dvr_change, > > + }, > > + [AMD_PSTATE_ACTIVE] = { > > + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, > > + [AMD_PSTATE_PASSIVE] = amd_pstate_change_driver_mode, > > + [AMD_PSTATE_ACTIVE] = NULL, > > + [AMD_PSTATE_GUIDED] = amd_pstate_change_driver_mode, > > + }, > > + [AMD_PSTATE_GUIDED] = { > > + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, > > + [AMD_PSTATE_PASSIVE] = amd_pstate_change_mode_without_dvr_change, > > + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, > > + [AMD_PSTATE_GUIDED] = NULL, > > + }, > > +}; > > + > > static ssize_t amd_pstate_show_status(char *buf) > > { > > if (!current_pstate_driver) > > @@ -805,57 +906,22 @@ static ssize_t amd_pstate_show_status(char *buf) > > return sysfs_emit(buf, "%s\n", amd_pstate_mode_string[cppc_state]); > > } > > -static void amd_pstate_driver_cleanup(void) > > -{ > > - current_pstate_driver = NULL; > > -} > > - > > static int amd_pstate_update_status(const char *buf, size_t size) > > { > > - int ret; > > int mode_idx; > > - if (size > 7 || size < 6) > > + if (size > strlen("passive") || size < strlen("active")) > > return -EINVAL; > > - mode_idx = get_mode_idx_from_str(buf, size); > > - switch(mode_idx) { > > - case AMD_PSTATE_DISABLE: > > - if (!current_pstate_driver) > > - return -EINVAL; > > - if (cppc_state == AMD_PSTATE_ACTIVE) > > - return -EBUSY; > > - ret = cpufreq_unregister_driver(current_pstate_driver); > > - amd_pstate_driver_cleanup(); > > - break; > > - case AMD_PSTATE_PASSIVE: > > - if (current_pstate_driver) { > > - if (current_pstate_driver == &amd_pstate_driver) > > - return 0; > > - cpufreq_unregister_driver(current_pstate_driver); > > - cppc_state = AMD_PSTATE_PASSIVE; > > - current_pstate_driver = &amd_pstate_driver; > > - } > > + mode_idx = get_mode_idx_from_str(buf, size); > > - ret = cpufreq_register_driver(current_pstate_driver); > > - break; > > - case AMD_PSTATE_ACTIVE: > > - if (current_pstate_driver) { > > - if (current_pstate_driver == &amd_pstate_epp_driver) > > - return 0; > > - cpufreq_unregister_driver(current_pstate_driver); > > - current_pstate_driver = &amd_pstate_epp_driver; > > - cppc_state = AMD_PSTATE_ACTIVE; > > - } > > + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX) > > + return -EINVAL; > > - ret = cpufreq_register_driver(current_pstate_driver); > > - break; > > - default: > > - ret = -EINVAL; > > - break; > > - } > > + if (mode_state_machine[cppc_state][mode_idx]) > > + return mode_state_machine[cppc_state][mode_idx](mode_idx); > > - return ret; > > + return -EBUSY; > > } > > static ssize_t show_status(struct kobject *kobj, > > With one nit fixed, > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
On 1/30/23 23:21, Wyes Karny wrote: > Only if the state requested matches > with the current state then -EBUSY value is returned. This differs from the behavior of intel_pstate, where writing the current state to the status file always succeeds. Why not do the same thing here?
Hi Russell, On 02 Feb 01:41, Russell Haley wrote: > > > On 1/30/23 23:21, Wyes Karny wrote: > > > Only if the state requested matches > > with the current state then -EBUSY value is returned. > > This differs from the behavior of intel_pstate, where writing the > current state to the status file always succeeds. > > Why not do the same thing here? Make sense. Will change this to return 0. Thanks for reviewing. Thanks, Wyes
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 48ab4684c3a5..6c522dec6967 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -65,6 +65,8 @@ static struct cpufreq_driver amd_pstate_epp_driver; static int cppc_state = AMD_PSTATE_DISABLE; struct kobject *amd_pstate_kobj; +typedef int (*cppc_mode_transition_fn)(int); + static inline int get_mode_idx_from_str(const char *str, size_t size) { int i; @@ -797,6 +799,105 @@ static ssize_t show_energy_performance_preference( return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]); } +static void amd_pstate_driver_cleanup(void) +{ + amd_pstate_enable(false); + cppc_state = AMD_PSTATE_DISABLE; + current_pstate_driver = NULL; +} + +static int amd_pstate_register_driver(int mode) +{ + int ret; + + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED) + current_pstate_driver = &amd_pstate_driver; + else if (mode == AMD_PSTATE_ACTIVE) + current_pstate_driver = &amd_pstate_epp_driver; + else + return -EINVAL; + + cppc_state = mode; + ret = cpufreq_register_driver(current_pstate_driver); + if (ret) { + amd_pstate_driver_cleanup(); + return ret; + } + return 0; +} + +static int amd_pstate_unregister_driver(int dummy) +{ + int ret; + + ret = cpufreq_unregister_driver(current_pstate_driver); + + if (ret) + return ret; + + amd_pstate_driver_cleanup(); + return 0; +} + +static int amd_pstate_change_mode_without_dvr_change(int mode) +{ + int cpu = 0; + + cppc_state = mode; + + if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE) + return 0; + + for_each_present_cpu(cpu) { + cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1); + } + + return 0; +} + +static int amd_pstate_change_driver_mode(int mode) +{ + int ret; + + ret = amd_pstate_unregister_driver(0); + if (ret) + return ret; + + ret = amd_pstate_register_driver(mode); + if (ret) + return ret; + + return 0; +} + +/* Mode transition table */ +cppc_mode_transition_fn mode_state_machine[AMD_PSTATE_MAX][AMD_PSTATE_MAX] = { + [AMD_PSTATE_DISABLE] = { + [AMD_PSTATE_DISABLE] = NULL, + [AMD_PSTATE_PASSIVE] = amd_pstate_register_driver, + [AMD_PSTATE_ACTIVE] = amd_pstate_register_driver, + [AMD_PSTATE_GUIDED] = amd_pstate_register_driver, + }, + [AMD_PSTATE_PASSIVE] = { + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, + [AMD_PSTATE_PASSIVE] = NULL, + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, + [AMD_PSTATE_GUIDED] = amd_pstate_change_mode_without_dvr_change, + }, + [AMD_PSTATE_ACTIVE] = { + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, + [AMD_PSTATE_PASSIVE] = amd_pstate_change_driver_mode, + [AMD_PSTATE_ACTIVE] = NULL, + [AMD_PSTATE_GUIDED] = amd_pstate_change_driver_mode, + }, + [AMD_PSTATE_GUIDED] = { + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver, + [AMD_PSTATE_PASSIVE] = amd_pstate_change_mode_without_dvr_change, + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, + [AMD_PSTATE_GUIDED] = NULL, + }, +}; + static ssize_t amd_pstate_show_status(char *buf) { if (!current_pstate_driver) @@ -805,57 +906,22 @@ static ssize_t amd_pstate_show_status(char *buf) return sysfs_emit(buf, "%s\n", amd_pstate_mode_string[cppc_state]); } -static void amd_pstate_driver_cleanup(void) -{ - current_pstate_driver = NULL; -} - static int amd_pstate_update_status(const char *buf, size_t size) { - int ret; int mode_idx; - if (size > 7 || size < 6) + if (size > strlen("passive") || size < strlen("active")) return -EINVAL; - mode_idx = get_mode_idx_from_str(buf, size); - switch(mode_idx) { - case AMD_PSTATE_DISABLE: - if (!current_pstate_driver) - return -EINVAL; - if (cppc_state == AMD_PSTATE_ACTIVE) - return -EBUSY; - ret = cpufreq_unregister_driver(current_pstate_driver); - amd_pstate_driver_cleanup(); - break; - case AMD_PSTATE_PASSIVE: - if (current_pstate_driver) { - if (current_pstate_driver == &amd_pstate_driver) - return 0; - cpufreq_unregister_driver(current_pstate_driver); - cppc_state = AMD_PSTATE_PASSIVE; - current_pstate_driver = &amd_pstate_driver; - } + mode_idx = get_mode_idx_from_str(buf, size); - ret = cpufreq_register_driver(current_pstate_driver); - break; - case AMD_PSTATE_ACTIVE: - if (current_pstate_driver) { - if (current_pstate_driver == &amd_pstate_epp_driver) - return 0; - cpufreq_unregister_driver(current_pstate_driver); - current_pstate_driver = &amd_pstate_epp_driver; - cppc_state = AMD_PSTATE_ACTIVE; - } + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX) + return -EINVAL; - ret = cpufreq_register_driver(current_pstate_driver); - break; - default: - ret = -EINVAL; - break; - } + if (mode_state_machine[cppc_state][mode_idx]) + return mode_state_machine[cppc_state][mode_idx](mode_idx); - return ret; + return -EBUSY; } static ssize_t show_status(struct kobject *kobj,
amd_pstate driver's `status` sysfs entry helps to control the driver's mode dynamically by user. After the addition of guided mode the combinations of mode transitions have been increased (16 combinations). Therefore optimise the amd_pstate_update_status function by implementing a state transition table. There are 4 states amd_pstate supports, namely: 'disable', 'passive', 'active', and 'guided'. The transition from any state to any other state is possible after this change. Only if the state requested matches with the current state then -EBUSY value is returned. Sysfs interface: To disable amd_pstate driver: # echo disable > /sys/devices/system/cpu/amd_pstate/status To enable passive mode: # echo passive > /sys/devices/system/cpu/amd_pstate/status To change mode to active: # echo active > /sys/devices/system/cpu/amd_pstate/status To change mode to guided: # echo guided > /sys/devices/system/cpu/amd_pstate/status Signed-off-by: Wyes Karny <wyes.karny@amd.com> --- drivers/cpufreq/amd-pstate.c | 150 +++++++++++++++++++++++++---------- 1 file changed, 108 insertions(+), 42 deletions(-)