Messages in this thread | | | Date | Thu, 12 Jan 2023 23:48:18 -0600 | Subject | Re: [PATCH v2 5/6] cpufreq: amd_pstate: Add guided mode control support via sysfs | From | Mario Limonciello <> |
| |
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,
| |