Date:   Thu, 16 Feb 2023 14:15:53 +0800
From:   Huang Rui <ray.huang@amd.com>
To:     "Karny, Wyes" <Wyes.Karny@amd.com>
Cc:     Rafael J Wysocki <rafael@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "Limonciello, Mario" <Mario.Limonciello@amd.com>,
        "Yuan, Perry" <Perry.Yuan@amd.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        "Shukla, Santosh" <Santosh.Shukla@amd.com>,
        Len Brown <lenb@kernel.org>,
        Robert Moore <robert.moore@intel.com>,
        Borislav Petkov <bp@alien8.de>,
        "Narayan, Ananth" <Ananth.Narayan@amd.com>,
        "Shenoy, Gautham Ranjal" <gautham.shenoy@amd.com>,
        Tor Vic <torvic9@mailbox.org>
Subject: Re: [PATCH v6 5/6] cpufreq: amd_pstate: Add guided mode control
 support via sysfs
Message-ID: <Y+3KGcHWPggR6371@amd.com>
References: <20230206172157.49887-1-wyes.karny@amd.com>
 <20230206172157.49887-6-wyes.karny@amd.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230206172157.49887-6-wyes.karny@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?maBgA7wELj3hPu2bq+B5fdhjojh195QNGxraRXcRlt3MCM22DdvB7ZClTh6r?=
 =?us-ascii?Q?BAdZ82RjREgdV89NhAp3Fwvmkw5C5Wm/e6/+gfeQEY2wpQ1ra6VozHYpkJxw?=
 =?us-ascii?Q?kItm8OwR429HFReJe6dpUsPtldpfcNsfvxgVuE2om3fDBP5C0eipCKKdJNH/?=
 =?us-ascii?Q?uuGVap0Cs0G0ucBpsc6No4rMOByczHoTr+BoJckaozJUniik6G0ax15VZDQ/?=
 =?us-ascii?Q?oq3KZN70hEotNOIod/NxD6DatzG8ZkSpG2ReEjV+WbEHsvupbihxZNQRYsE5?=
 =?us-ascii?Q?iHRJY0w01xSHra4ceN1OUnzQ6a2C1DXqwuvV1nIPQu5n/lvPsNcKmAmJt0Ou?=
 =?us-ascii?Q?+2wCZKdWNRNdQBh2QqBPcf6sltRxYU19e5q9vaC7dbTEGdPEAFVV/kaz2Af+?=
 =?us-ascii?Q?INISeReWzvVtN9t9DftyStOz7upeuDPrVYqttJhTfZDMvURcufe/ZzoQ8aOF?=
 =?us-ascii?Q?cXHxgSa1RjjSgHJtq6mj9XCK2mWeM3r4u3rYR15fJApM+drqN+fnqMhEm4pY?=
 =?us-ascii?Q?kYuv5+yIaJJlmrfof2HJjwcT0WbcvTrAeRlqJzLpfu+38nIz0UIfqBJmi6AJ?=
 =?us-ascii?Q?VrCvVxAsB8eNmT0F6vDYpoIYHlfY3fICwLNgx0NjWkR1ScHeG5a+PJAF/B/m?=
 =?us-ascii?Q?Em/0TfFQktKS5YUTOpD51irXF1KXckCS/6ExL5hsqCBWW79TstEOoCP+7Ljh?=
 =?us-ascii?Q?3plhFOZA4753AZLacWHQyOLVrSvoy+TDLBL7gkwqdKZw73LNNLf+eyzYE7iE?=
 =?us-ascii?Q?xpJ/1/SOr7X4vxhNwsA+CUija+bBcaizCLd7rt502tC8k/ACqITI1sippbQ1?=
 =?us-ascii?Q?Jqt9ZlSI8rAPnjzmwu5m9Gy9tOuKGf93WFlKAe4Tz1Wf62pqWXzaJiiu8gtK?=
 =?us-ascii?Q?Lqj96MCqqQlzzWi6T9Z1hf+yFAyqGNZc6IpIJVDyhzF6+9o8SrT9iLrXrZwx?=
 =?us-ascii?Q?bPWJgvl3uXzRXX3qsE5W2bSqjB061GirunCjK3pD5o4CpyIQAHp/w0k7+3xt?=
 =?us-ascii?Q?r2X4f6guZEwnHuqjcVGcwCj4yIaBAXBxnol1P6DDxYIYKzsziv/l42nMKJwg?=
 =?us-ascii?Q?Cz1SlKKsOvk41WbSxHMvgqAVXj3sFDpHmOvaJ3bvGZOwucU1gR++Wsrce20a?=
 =?us-ascii?Q?oe/8gWQhX0nMnvWOIiE5iVUJZGBIPPec36POL65JutVELWySAo+Vwj6nUhNn?=
 =?us-ascii?Q?Us8iAf0cBsFRZVM2UrixOxihs8v5GDl5DBgtn9DGaHbe4dsHiC8kr+qj4By3?=
 =?us-ascii?Q?allkwPWYnsxSaLNG0txf0JCQOPI0kix2WTFqrwgy3Y71ww59Gxydw2QZXbnR?=
 =?us-ascii?Q?ojvQg0e3ZPlGEVjOXZ80jUF2+KgQtp+db98pOEjRucSHO6cKSMgQYxoV5dDV?=
 =?us-ascii?Q?pjTNOv+MQihBSuIn4vcQn5mWtzJteD1lngbFTtWODOtwozahcpE9AYcoXTsU?=
 =?us-ascii?Q?txC43vftfzB78hDxlLcJHMmpsKO/BhgP9W+tlLPgkM0JNwI0ALrKNht1nJxs?=
 =?us-ascii?Q?vTP3U3ByLLAwKOOWuFQ7vz+wwB/lnxX6nZFaZSF3Hj8uB/EzvS3Mv4MNH/be?=
 =?us-ascii?Q?0Ar38WcSGfBjqLgeuT0kwqeFH8lcdbSGBksTv+9F?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e97e121-871c-431f-ae57-08db0fe551c1
X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB2504.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2023 06:16:19.8507
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7sFOMBfz8mhQ3gRQYh6oYYB/d7EH7c0STDwwydSWUbvxmLGSnmeQyYSF1YuTIBrDZyTccArbhAEyZ3hRoT4Hpw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR12MB5598
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Feb 07, 2023 at 01:21:56AM +0800, Karny, Wyes 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.
> 
> 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>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I suggest we can add mode change function into CPUPower tool to operate
this sysfs interface.

Acked-by: Huang Rui <ray.huang@amd.com>

> ---
>  drivers/cpufreq/amd-pstate.c | 149 +++++++++++++++++++++++++----------
>  1 file changed, 107 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6582c922ad3a..4e74f59804ae 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -106,6 +106,8 @@ static unsigned int epp_values[] = {
>  	[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_POWERSAVE,
>   };
>  
> +typedef int (*cppc_mode_transition_fn)(int);
> +
>  static inline int get_mode_idx_from_str(const char *str, size_t size)
>  {
>  	int i;
> @@ -838,6 +840,104 @@ 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;
> +}
> +
> +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)
> @@ -846,57 +946,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 0;
>  }
>  
>  static ssize_t show_status(struct kobject *kobj,
> -- 
> 2.34.1
>