| Message ID | 20230113052141.2874296-6-wyes.karny@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | amd_pstate: Add guided autonomous mode support | expand |
On 1/12/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. > > 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 | 144 ++++++++++++++++++++++++++--------- > 1 file changed, 109 insertions(+), 35 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 20d78dad712d..4a2b559fd712 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; > @@ -806,55 +808,127 @@ static ssize_t amd_pstate_show_status(char *buf) > > static void amd_pstate_driver_cleanup(void) > { > + amd_pstate_enable(false); > + cppc_state = AMD_PSTATE_DISABLE; > default_pstate_driver = NULL; > } > > -static int amd_pstate_update_status(const char *buf, size_t size) > +static int amd_pstate_register_driver(int mode) > { > int ret; > - int mode_idx; > > - if (size > 7 || size < 3) > + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED) > + default_pstate_driver = &amd_pstate_driver; > + else if (mode == AMD_PSTATE_ACTIVE) > + default_pstate_driver = &amd_pstate_epp_driver; > + else > return -EINVAL; > - mode_idx = get_mode_idx_from_str(buf, size); > > - switch(mode_idx) { > - case AMD_PSTATE_DISABLE: > - if (!default_pstate_driver) > - return -EINVAL; > - if (cppc_state == AMD_PSTATE_ACTIVE) > - return -EBUSY; > - ret = cpufreq_unregister_driver(default_pstate_driver); > + ret = cpufreq_register_driver(default_pstate_driver); > + if (ret) { > amd_pstate_driver_cleanup(); > - break; > - case AMD_PSTATE_PASSIVE: > - if (default_pstate_driver) { > - if (default_pstate_driver == &amd_pstate_driver) > - return 0; > - cpufreq_unregister_driver(default_pstate_driver); > - cppc_state = AMD_PSTATE_PASSIVE; > - default_pstate_driver = &amd_pstate_driver; > - } > + return ret; > + } > > - ret = cpufreq_register_driver(default_pstate_driver); > - break; > - case AMD_PSTATE_ACTIVE: > - if (default_pstate_driver) { > - if (default_pstate_driver == &amd_pstate_epp_driver) > - return 0; > - cpufreq_unregister_driver(default_pstate_driver); > - default_pstate_driver = &amd_pstate_epp_driver; > - cppc_state = AMD_PSTATE_ACTIVE; > + cppc_state = mode; > + return 0; > +} > + > +static int amd_pstate_unregister_driver(int dummy) > +{ > + int ret; > + > + ret = cpufreq_unregister_driver(default_pstate_driver); > + > + if (ret) > + return ret; > + > + amd_pstate_driver_cleanup(); > + return 0; > +} > + > +static void amd_pstate_change_mode_without_dvr_change(int mode) > +{ > + int cpu = 0; > + > + cppc_state = mode; > + if (!boot_cpu_has(X86_FEATURE_CPPC)) { You can save some indentation by inverting the check and calling "return". > + if (cppc_state == AMD_PSTATE_PASSIVE) { > + for_each_present_cpu(cpu) { > + cppc_set_auto_sel(cpu, 0); > + } > + } else if (cppc_state == AMD_PSTATE_GUIDED) { > + for_each_present_cpu(cpu) { > + cppc_set_auto_sel(cpu, 1); > + } > } > + } > +} > > - ret = cpufreq_register_driver(default_pstate_driver); > - break; > - default: > - break; > - ret = -EINVAL; > +static int amd_pstate_change_driver_mode(int mode) > +{ > + int ret; > + > + if ((mode == AMD_PSTATE_GUIDED && cppc_state == AMD_PSTATE_PASSIVE) || > + (mode == AMD_PSTATE_PASSIVE && cppc_state == AMD_PSTATE_GUIDED)) { > + amd_pstate_change_mode_without_dvr_change(mode); > + return 0; This feels like you're duplicating your logic of your mode transition table. How about having the mode transition table call the function for changing modes without a driver change directly instead for those cases? > } > > - return 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_driver_mode, > + }, > + [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_driver_mode, > + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, > + [AMD_PSTATE_GUIDED] = NULL, > + }, > +}; > + > +static int amd_pstate_update_status(const char *buf, size_t size) > +{ > + int mode_idx; > + > + if (size > 7 || size < 3) > + return -EINVAL; The compiler typically inlines strlen. Perhaps to make this more obvious what these boundaries are maybe do this: size > strlen("passive") || size < strlen("off") > + mode_idx = get_mode_idx_from_str(buf, size); > + > + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX) > + return -EINVAL; > + > + if (mode_state_machine[cppc_state][mode_idx]) > + return mode_state_machine[cppc_state][mode_idx](mode_idx); > + > + return -EBUSY; > } > > static ssize_t show_status(struct kobject *kobj,
On 1/13/2023 11:18 AM, Mario Limonciello wrote: > On 1/12/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. >> >> 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 | 144 ++++++++++++++++++++++++++--------- >> 1 file changed, 109 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index 20d78dad712d..4a2b559fd712 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; >> @@ -806,55 +808,127 @@ static ssize_t amd_pstate_show_status(char *buf) >> static void amd_pstate_driver_cleanup(void) >> { >> + amd_pstate_enable(false); >> + cppc_state = AMD_PSTATE_DISABLE; >> default_pstate_driver = NULL; >> } >> -static int amd_pstate_update_status(const char *buf, size_t size) >> +static int amd_pstate_register_driver(int mode) >> { >> int ret; >> - int mode_idx; >> - if (size > 7 || size < 3) >> + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED) >> + default_pstate_driver = &amd_pstate_driver; >> + else if (mode == AMD_PSTATE_ACTIVE) >> + default_pstate_driver = &amd_pstate_epp_driver; >> + else >> return -EINVAL; >> - mode_idx = get_mode_idx_from_str(buf, size); >> - switch(mode_idx) { >> - case AMD_PSTATE_DISABLE: >> - if (!default_pstate_driver) >> - return -EINVAL; >> - if (cppc_state == AMD_PSTATE_ACTIVE) >> - return -EBUSY; >> - ret = cpufreq_unregister_driver(default_pstate_driver); >> + ret = cpufreq_register_driver(default_pstate_driver); >> + if (ret) { >> amd_pstate_driver_cleanup(); >> - break; >> - case AMD_PSTATE_PASSIVE: >> - if (default_pstate_driver) { >> - if (default_pstate_driver == &amd_pstate_driver) >> - return 0; >> - cpufreq_unregister_driver(default_pstate_driver); >> - cppc_state = AMD_PSTATE_PASSIVE; >> - default_pstate_driver = &amd_pstate_driver; >> - } >> + return ret; >> + } >> - ret = cpufreq_register_driver(default_pstate_driver); >> - break; >> - case AMD_PSTATE_ACTIVE: >> - if (default_pstate_driver) { >> - if (default_pstate_driver == &amd_pstate_epp_driver) >> - return 0; >> - cpufreq_unregister_driver(default_pstate_driver); >> - default_pstate_driver = &amd_pstate_epp_driver; >> - cppc_state = AMD_PSTATE_ACTIVE; >> + cppc_state = mode; >> + return 0; >> +} >> + >> +static int amd_pstate_unregister_driver(int dummy) >> +{ >> + int ret; >> + >> + ret = cpufreq_unregister_driver(default_pstate_driver); >> + >> + if (ret) >> + return ret; >> + >> + amd_pstate_driver_cleanup(); >> + return 0; >> +} >> + >> +static void amd_pstate_change_mode_without_dvr_change(int mode) >> +{ >> + int cpu = 0; >> + >> + cppc_state = mode; >> + if (!boot_cpu_has(X86_FEATURE_CPPC)) { > > You can save some indentation by inverting the check and calling "return Makes sense. Thanks! > >> + if (cppc_state == AMD_PSTATE_PASSIVE) { >> + for_each_present_cpu(cpu) { >> + cppc_set_auto_sel(cpu, 0); >> + } >> + } else if (cppc_state == AMD_PSTATE_GUIDED) { >> + for_each_present_cpu(cpu) { >> + cppc_set_auto_sel(cpu, 1); >> + } >> } >> + } >> +} >> - ret = cpufreq_register_driver(default_pstate_driver); >> - break; >> - default: >> - break; >> - ret = -EINVAL; >> +static int amd_pstate_change_driver_mode(int mode) >> +{ >> + int ret; >> + >> + if ((mode == AMD_PSTATE_GUIDED && cppc_state == AMD_PSTATE_PASSIVE) || >> + (mode == AMD_PSTATE_PASSIVE && cppc_state == AMD_PSTATE_GUIDED)) { >> + amd_pstate_change_mode_without_dvr_change(mode); >> + return 0; > > This feels like you're duplicating your logic of your mode transition table. How about having the mode transition table call the function for changing modes without a driver change directly instead for those cases? Makes sense. Will add this in the table. > >> } >> - return 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_driver_mode, >> + }, >> + [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_driver_mode, >> + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, >> + [AMD_PSTATE_GUIDED] = NULL, >> + }, >> +}; >> + >> +static int amd_pstate_update_status(const char *buf, size_t size) >> +{ >> + int mode_idx; >> + >> + if (size > 7 || size < 3) >> + return -EINVAL; > > The compiler typically inlines strlen. Perhaps to make this more obvious what these boundaries are maybe do this: > > size > strlen("passive") || size < strlen("off") > >> + mode_idx = get_mode_idx_from_str(buf, size); >> + >> + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX) >> + return -EINVAL; >> + >> + if (mode_state_machine[cppc_state][mode_idx]) >> + return mode_state_machine[cppc_state][mode_idx](mode_idx); >> + >> + return -EBUSY; >> } >> static ssize_t show_status(struct kobject *kobj, >
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 20d78dad712d..4a2b559fd712 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; @@ -806,55 +808,127 @@ static ssize_t amd_pstate_show_status(char *buf) static void amd_pstate_driver_cleanup(void) { + amd_pstate_enable(false); + cppc_state = AMD_PSTATE_DISABLE; default_pstate_driver = NULL; } -static int amd_pstate_update_status(const char *buf, size_t size) +static int amd_pstate_register_driver(int mode) { int ret; - int mode_idx; - if (size > 7 || size < 3) + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED) + default_pstate_driver = &amd_pstate_driver; + else if (mode == AMD_PSTATE_ACTIVE) + default_pstate_driver = &amd_pstate_epp_driver; + else return -EINVAL; - mode_idx = get_mode_idx_from_str(buf, size); - switch(mode_idx) { - case AMD_PSTATE_DISABLE: - if (!default_pstate_driver) - return -EINVAL; - if (cppc_state == AMD_PSTATE_ACTIVE) - return -EBUSY; - ret = cpufreq_unregister_driver(default_pstate_driver); + ret = cpufreq_register_driver(default_pstate_driver); + if (ret) { amd_pstate_driver_cleanup(); - break; - case AMD_PSTATE_PASSIVE: - if (default_pstate_driver) { - if (default_pstate_driver == &amd_pstate_driver) - return 0; - cpufreq_unregister_driver(default_pstate_driver); - cppc_state = AMD_PSTATE_PASSIVE; - default_pstate_driver = &amd_pstate_driver; - } + return ret; + } - ret = cpufreq_register_driver(default_pstate_driver); - break; - case AMD_PSTATE_ACTIVE: - if (default_pstate_driver) { - if (default_pstate_driver == &amd_pstate_epp_driver) - return 0; - cpufreq_unregister_driver(default_pstate_driver); - default_pstate_driver = &amd_pstate_epp_driver; - cppc_state = AMD_PSTATE_ACTIVE; + cppc_state = mode; + return 0; +} + +static int amd_pstate_unregister_driver(int dummy) +{ + int ret; + + ret = cpufreq_unregister_driver(default_pstate_driver); + + if (ret) + return ret; + + amd_pstate_driver_cleanup(); + return 0; +} + +static void amd_pstate_change_mode_without_dvr_change(int mode) +{ + int cpu = 0; + + cppc_state = mode; + if (!boot_cpu_has(X86_FEATURE_CPPC)) { + if (cppc_state == AMD_PSTATE_PASSIVE) { + for_each_present_cpu(cpu) { + cppc_set_auto_sel(cpu, 0); + } + } else if (cppc_state == AMD_PSTATE_GUIDED) { + for_each_present_cpu(cpu) { + cppc_set_auto_sel(cpu, 1); + } } + } +} - ret = cpufreq_register_driver(default_pstate_driver); - break; - default: - break; - ret = -EINVAL; +static int amd_pstate_change_driver_mode(int mode) +{ + int ret; + + if ((mode == AMD_PSTATE_GUIDED && cppc_state == AMD_PSTATE_PASSIVE) || + (mode == AMD_PSTATE_PASSIVE && cppc_state == AMD_PSTATE_GUIDED)) { + amd_pstate_change_mode_without_dvr_change(mode); + return 0; } - return 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_driver_mode, + }, + [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_driver_mode, + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode, + [AMD_PSTATE_GUIDED] = NULL, + }, +}; + +static int amd_pstate_update_status(const char *buf, size_t size) +{ + int mode_idx; + + if (size > 7 || size < 3) + return -EINVAL; + mode_idx = get_mode_idx_from_str(buf, size); + + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX) + return -EINVAL; + + if (mode_state_machine[cppc_state][mode_idx]) + return mode_state_machine[cppc_state][mode_idx](mode_idx); + + 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 | 144 ++++++++++++++++++++++++++--------- 1 file changed, 109 insertions(+), 35 deletions(-)