Received-SPF: pass (google.com: domain of linux-kernel+bounces-45238-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <e64d07e6-b38e-456c-8f56-4a6287de8e65@amd.com>
Date: Tue, 30 Jan 2024 14:01:09 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH Resend 2/8] tools/power x86_energy_perf_policy: enable AMD
 pstate EPP profile switching on MSR based system
Content-Language: en-US
To: Perry Yuan <perry.yuan@amd.com>, rafael.j.wysocki@intel.com,
 Borislav.Petkov@amd.com, viresh.kumar@linaro.org, Ray.Huang@amd.com,
 gautham.shenoy@amd.com
Cc: Alexander.Deucher@amd.com, Xinmei.Huang@amd.com, Xiaojian.Du@amd.com,
 Li.Meng@amd.com, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <cover.1706592301.git.perry.yuan@amd.com>
 <ecd0310b117e6b68160de9c77517dfed2b8e530c.1706592301.git.perry.yuan@amd.com>
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <ecd0310b117e6b68160de9c77517dfed2b8e530c.1706592301.git.perry.yuan@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?aXMwbjBVcHFYMXE2T1B4RGtPTjcvWEhDQ1cvWjQ2ckY0TElWa25qV0hBR3Qx?=
 =?utf-8?B?MitiNlpaa0grc2hPZ1NRWmVYUVZmN25meUZNZnExbGZGZ2x6U0VWdEVHZkFK?=
 =?utf-8?B?Y2QzcWFibGFTeFE0WkUzeldIS3NwWlkyNkJGMWJ1em4wMmZmTUxuVUhUQTI3?=
 =?utf-8?B?YWU0MUF0Q0g0SDdKUkNlOCtmaHdNWTk5N3V5NmkzZ0NLVmdqY1MwQ1B0aHM3?=
 =?utf-8?B?SlYvajZEdzVQRFpqNDFYbnI1V0RjQUNmRENZRmI5dVg5TFB0Tk1kNytjNUdS?=
 =?utf-8?B?a3FKTTF5TG1yWkJPVXk2Vklha09KdnlXU2hldStaK0VSQXRYWEJQMytTaGxN?=
 =?utf-8?B?cUpVc2thaUVzYnp4SUI4d3R2N3lSWnFwYW80bXMrOXJiT2JESkxUS0VOajR2?=
 =?utf-8?B?ajJqeDdKMVZ4Y2xwTjV4dktTVXBaalpDSkZlNVlHRlBrWXUyWVoxaUQ4VDVu?=
 =?utf-8?B?L2UrN2dIT2NIbDg4U211YVRzT2FnOE1tdkxPYmFwcDJ2TEVHcW1HYmdvcmE5?=
 =?utf-8?B?K0dJQWp0dE1FN2RaVUlqV3AwNVRwWXVyZmVzU3hVem1Ob2FOZ2JYYUg1cGdr?=
 =?utf-8?B?MWp6SFVROUNFQWdXY2NlUFNOV1doTXdVaXlkemk3eEJJNHpOU2xhRUk5Qkky?=
 =?utf-8?B?T1RjcCtDZDg2WTJ4SlBhdk5xb2RnL2I2VGtZaHNaWUVwSlFpcWdjcnRWbjFN?=
 =?utf-8?B?ZzJHVytNS1d0YUhvTHJNWDE5TnlJMUlpTm5lWTk1T0JPYnJ0MExjZGcxZkdk?=
 =?utf-8?B?S3dJRU9iVGIwVlpzUkdsd3dQSWVhUHlBVVBWNjRYZGZBSkhMa2tLS1RodEtk?=
 =?utf-8?B?UnNPdW5CSnFzdVlsN1Z1OCtTbTRLdEtOYTZBK0ZDaVllUE13MmpJNFdwYWtM?=
 =?utf-8?B?Q3RBNHV1RkZxTkhoNDZzeG11aEMwWXRJei82N1IzYTY3RHZtTWxGb3dPY2VN?=
 =?utf-8?B?cnBRNjF0cGF3aDlzZzFXL2tWTGlWam9yRzdvaW5sdnBESTgyMkRubm90d0po?=
 =?utf-8?B?MXFvVlZIeVdBMythMlZBQVhzNkZvV2Zsa3hTUmF5QTYzS2tTSkt2Y2pEZ2Uv?=
 =?utf-8?B?Q2kvQ1ZQN3pSckZ5QnhpZGlMaVBNaFJnUXpsTGlYSENXWGtIcmFIcjdwUmxs?=
 =?utf-8?B?WlBIaUxYU1lxQ2VZMUlOdnZucXJuNFVTQUtQOVkxVFI0S3hCNnZoYVdDZGxx?=
 =?utf-8?B?dm8vM0cyOTVmUVpKM1BQRm9DYm5mSko2MnVheEZSR3Y5ZXUrQTlSWUdOUmRT?=
 =?utf-8?B?cXRqUGZmRmtPUkZjazNlNUdCd0JoS0RFQWJrQkFCcmtSblg2bTlnN01CVWtD?=
 =?utf-8?B?NFJqQkxQMmcrSkZ6VmZPbzE0SUJCZTlnZVJveEtpRFB2aFpvU0VZOG1DZmZj?=
 =?utf-8?B?Z09KS0pyL1JNOWhGQVBpTnhGMFJpOHJFS0hxeVg2c29nY0oyUjJNL3VndnlY?=
 =?utf-8?B?cUUrZHBTMFZ2VWhUbXdLcS9KbGwycDZJdEJPbUFUMW5haUVwNFZlUUhuOE1C?=
 =?utf-8?B?L0xRbmc2bFJUcndMRGI5cXhsa3lycFdpcU1kaHdwSXBZcnp2TDlRMGdobkFQ?=
 =?utf-8?B?c0MzRU5KQzB3T3FycWpJOWZqdE9POUxkNUVQNEtxSHU1TVYwQUJYVmR3NW42?=
 =?utf-8?B?Y2I2c1BBbDhNVFJ6K2xDTjhrSXMyTm1USzlBQTlGd0tLQjlmRFVpUzJpMGI2?=
 =?utf-8?B?SEpWTTRRbS91anlXczVLaGdrbmwyV05DVERMYmxYNGMyZnBNYlVJUWxBSHRi?=
 =?utf-8?B?TnplOFdCb3NIV2d3M3VqMk4xSDNUSGJsUEpmanBONnpJR3FWZHZxZXVWQWtL?=
 =?utf-8?B?cjVodWY2U0JXWHZwSVNBM3h0V2pTV1hYWm01L0dVZFBpRG50Vk5vNUgrd0RC?=
 =?utf-8?B?c2FlS3V4dmFBNUxab1lzVW1MaituamZlVWpicEtqS1BKQTVHZUhBVkJqUlo4?=
 =?utf-8?B?M0FBN2RtQUpqQWtvNTY2T3pYYmpNRjBMRzViZ1FSanhvZngxNDhtakIwa1h2?=
 =?utf-8?B?VXdKcDlCQWc3c05ldVRwRUNTbmR0OU9CZnRFeVgzUjRoZndvbEEzZzR2cE5j?=
 =?utf-8?B?Y2E0V0ZGbDJVYktoa2xySC93Qm1tTSszcXg4RXNQZ1dGTGFZaEE5Nythcm05?=
 =?utf-8?Q?Pml7RTVAzU4ADV4ZcP/jqIZbR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 712fbe47-763b-477f-8084-08dc21ce34a7
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2024 20:01:11.2217
 (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: tVb+aQybv8H2IYsa+dltd298uMWny9Nl6l6c+zvIUdFoRWDU4AZOR21Bv/+bnZCqfKDpuzF86orPbDMU9NocCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR12MB7111

On 1/29/2024 23:56, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
> 
> x86_energy_perf_policy adds AMD EPP profiles switch support,the EPP
> driver also has four EPP profiles implemented under power
> firmware autonomous mode.
> (performance, balance-performance, balance-power, power)
> User can choose the new profile to use with this utility support while
> `amd_pstate_epp` driver instance loaded for the MSR support systems.
> 
> for example:
> $ sudo x86_energy_perf_policy -d --hwp-epp balance-performance
> cpu0: old: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu0: HWP_CAP: low 13 eff 157 guar 0 high 231
> cpu0: new: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu1: old: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu1: HWP_CAP: low 13 eff 157 guar 0 high 236
> cpu1: new: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu2: old: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu2: HWP_CAP: low 13 eff 157 guar 0 high 211
> cpu2: new: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu3: old: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> cpu3: HWP_CAP: low 13 eff 157 guar 0 high 206
> cpu3: new: HWP_REQ: min 13 max 166 des 0 epp 128 window 0x9 (9*10^0us) use_pkg 0
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>   .../x86_energy_perf_policy.c                  | 69 +++++++++++++++----
>   1 file changed, 54 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c
> index 5daf1c2bb601..da9087873915 100644
> --- a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c
> +++ b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c
> @@ -902,14 +902,23 @@ void write_hwp_request(int cpu, struct msr_hwp_request *hwp_req, unsigned int ms
>   			hwp_req->hwp_desired, hwp_req->hwp_epp,
>   			hwp_req->hwp_window, hwp_req->hwp_use_pkg);
>   
> -	msr |= HWP_MIN_PERF(ratio_2_msr_perf(hwp_req->hwp_min));
> -	msr |= HWP_MAX_PERF(ratio_2_msr_perf(hwp_req->hwp_max));
> -	msr |= HWP_DESIRED_PERF(ratio_2_msr_perf(hwp_req->hwp_desired));
> -	msr |= HWP_ENERGY_PERF_PREFERENCE(hwp_req->hwp_epp);
> -	msr |= HWP_ACTIVITY_WINDOW(hwp_req->hwp_window);
> -	msr |= HWP_PACKAGE_CONTROL(hwp_req->hwp_use_pkg);
> -
> -	put_msr(cpu, msr_offset, msr);
> +	if (genuine_intel) {
> +		msr |= HWP_MIN_PERF(ratio_2_msr_perf(hwp_req->hwp_min));
> +		msr |= HWP_MAX_PERF(ratio_2_msr_perf(hwp_req->hwp_max));
> +		msr |= HWP_DESIRED_PERF(ratio_2_msr_perf(hwp_req->hwp_desired));
> +		msr |= HWP_ENERGY_PERF_PREFERENCE(hwp_req->hwp_epp);
> +		msr |= HWP_ACTIVITY_WINDOW(hwp_req->hwp_window);
> +		msr |= HWP_PACKAGE_CONTROL(hwp_req->hwp_use_pkg);
> +		put_msr(cpu, msr_offset, msr);
> +	} else if (authentic_amd) {
> +		/* AMD EPP need to set desired perf with zero */
> +		hwp_req->hwp_desired = 0;

Should this be blocked a level higher up?  Like exiting the tool the 
function call when calling parse_cmdline_hwp_desired() is used?

> +		msr |= AMD_CPPC_MIN_PERF(hwp_req->hwp_min);
> +		msr |= AMD_CPPC_MAX_PERF(hwp_req->hwp_max);
> +		msr |= AMD_CPPC_DES_PERF(hwp_req->hwp_desired);

Given the msr is already initialized to 0, this is unnecessary isn't it?

> +		msr |= AMD_CPPC_ENERGY_PERF_PREF(hwp_req->hwp_epp);
> +		amd_put_msr(cpu, msr_offset, (unsigned int)msr);
> +	}
>   }
>   
>   static int get_epb(int cpu)
> @@ -1157,8 +1166,12 @@ int update_hwp_request(int cpu)
>   {
>   	struct msr_hwp_request req;
>   	struct msr_hwp_cap cap;
> +	int msr_offset = 0;
>   
> -	int msr_offset = MSR_HWP_REQUEST;
> +	if (genuine_intel)
> +		msr_offset = MSR_HWP_REQUEST;
> +	else if (authentic_amd)
> +		msr_offset = MSR_AMD_CPPC_REQ;
>   
>   	read_hwp_request(cpu, &req, msr_offset);
>   	if (debug)
> @@ -1181,7 +1194,11 @@ int update_hwp_request(int cpu)
>   
>   	req.hwp_use_pkg = req_update.hwp_use_pkg;
>   
> -	read_hwp_cap(cpu, &cap, MSR_HWP_CAPABILITIES);
> +	if (genuine_intel)
> +		read_hwp_cap(cpu, &cap, MSR_HWP_CAPABILITIES);
> +	else if (authentic_amd)
> +		read_hwp_cap(cpu, &cap, MSR_AMD_CPPC_CAP1);
> +
>   	if (debug)
>   		print_hwp_cap(cpu, &cap, "");
>   
> @@ -1203,8 +1220,12 @@ int update_hwp_request_pkg(int pkg)
>   	struct msr_hwp_request req;
>   	struct msr_hwp_cap cap;
>   	int cpu = first_cpu_in_pkg[pkg];
> +	int msr_offset = 0;
>   
> -	int msr_offset = MSR_HWP_REQUEST_PKG;
> +	if (genuine_intel)
> +		msr_offset = MSR_HWP_REQUEST_PKG;
> +	else if (authentic_amd)
> +		msr_offset = MSR_AMD_CPPC_REQ;
>   
>   	read_hwp_request(cpu, &req, msr_offset);
>   	if (debug)
> @@ -1225,7 +1246,11 @@ int update_hwp_request_pkg(int pkg)
>   	if (update_hwp_epp)
>   		req.hwp_epp = req_update.hwp_epp;
>   
> -	read_hwp_cap(cpu, &cap, MSR_HWP_CAPABILITIES);
> +	if (genuine_intel)
> +		read_hwp_cap(cpu, &cap, MSR_HWP_CAPABILITIES);
> +	else if (authentic_amd)
> +		read_hwp_cap(cpu, &cap, MSR_AMD_CPPC_CAP1);
> +
>   	if (debug)
>   		print_hwp_cap(cpu, &cap, "");
>   
> @@ -1246,13 +1271,27 @@ int update_hwp_request_pkg(int pkg)
>   int enable_hwp_on_cpu(int cpu)
>   {
>   	unsigned long long msr;
> +	int ret;
> +
> +	if (genuine_intel) {
> +		get_msr(cpu, MSR_PM_ENABLE, &msr);
> +		put_msr(cpu, MSR_PM_ENABLE, 1);
> +	} else if (authentic_amd) {
> +		ret = amd_get_msr(cpu, MSR_AMD_CPPC_ENABLE, (unsigned long *)(&msr));
> +		if (ret < 0)
> +			errx(-1, "failed to get msr with return %d", ret);
>   
> -	get_msr(cpu, MSR_PM_ENABLE, &msr);
> -	put_msr(cpu, MSR_PM_ENABLE, 1);
> +		ret = amd_put_msr(cpu, MSR_AMD_CPPC_ENABLE, 1);
> +		if (ret < 0)
> +			errx(-1, "failed to put msr with return %d", ret);
> +	}
>   
> -	if (verbose)
> +	if (verbose && genuine_intel)
>   		printf("cpu%d: MSR_PM_ENABLE old: %d new: %d\n", cpu, (unsigned int) msr, 1);
>   
> +	if (verbose && authentic_amd)
> +		printf("cpu%d: MSR_AMD_CPPC_ENABLE old: %d new: %d\n", cpu, (unsigned int) msr, 1);
> +
>   	return 0;
>   }
>