Received-SPF: pass (google.com: domain of linux-kernel+bounces-72640-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Oleksandr Natalenko <oleksandr@natalenko.name>
To: rafael@kernel.org, Lucas Lee Jing Yi <lucasleeeeeeeee@gmail.com>
Cc: Perry.Yuan@amd.com, Xiaojian.Du@amd.com, alexander.deucher@amd.com,
 bp@alien8.de, deepak.sharma@amd.com, li.meng@amd.com,
 linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-kselftest@vger.kernel.org, linux-pm@vger.kernel.org,
 mario.limonciello@amd.com, nathan.fontenot@amd.com,
 rafael.j.wysocki@intel.com, ray.huang@amd.com, shimmer.huang@amd.com,
 skhan@linuxfoundation.org, viresh.kumar@linaro.org, x86@kernel.org,
 Lucas Lee Jing Yi <lucasleeeeeeeee@gmail.com>
Subject:
 Re: [PATCH] [PATCH] amd_pstate: fix erroneous highest_perf value on some CPUs
Date: Tue, 20 Feb 2024 10:02:48 +0100
Message-ID: <3868832.mvXUDI8C0e@natalenko.name>
In-Reply-To: <20240218161435.38312-2-lucasleeeeeeeee@gmail.com>
References:
 <CAJZ5v0hRk3tME7yeC+1r0RM4-oPPrnSu2=JCsOshBbJp_Nq2Hg@mail.gmail.com>
 <20240218161435.38312-1-lucasleeeeeeeee@gmail.com>
 <20240218161435.38312-2-lucasleeeeeeeee@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart13532597.O9o76ZdvQC";
 micalg="pgp-sha256"; protocol="application/pgp-signature"

--nextPart13532597.O9o76ZdvQC
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"; protected-headers="v1"
From: Oleksandr Natalenko <oleksandr@natalenko.name>
To: rafael@kernel.org, Lucas Lee Jing Yi <lucasleeeeeeeee@gmail.com>
Date: Tue, 20 Feb 2024 10:02:48 +0100
Message-ID: <3868832.mvXUDI8C0e@natalenko.name>
In-Reply-To: <20240218161435.38312-2-lucasleeeeeeeee@gmail.com>
MIME-Version: 1.0

Hello.

On ned=C4=9Ble 18. =C3=BAnora 2024 17:10:31 CET Lucas Lee Jing Yi wrote:
> On a Ryzen 7840HS the highest_perf value is 196, not 166 as AMD assumed.
> This leads to the advertised max clock speed to only be 4.35ghz instead o=
f 5.14ghz , leading to a large degradation in performance.
>=20
> Fix the broken assumption and revert back to the old logic for getting hi=
ghest_perf.
>=20
> TEST:
> Geekbench 6 Before Patch:
> Single Core:	2325 (-22%)!
> Multi Core:	11335 (-10%)
>=20
> Geekbench 6 AFTER Patch:
> Single Core:	2635
> Multi Core:	12487
>=20
> Signed-off-by: Lucas Lee Jing Yi <lucasleeeeeeeee@gmail.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>=20
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 08e112444c27..54df68773620 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -50,7 +50,6 @@
> =20
>  #define AMD_PSTATE_TRANSITION_LATENCY	20000
>  #define AMD_PSTATE_TRANSITION_DELAY	1000
> -#define AMD_PSTATE_PREFCORE_THRESHOLD	166
> =20
>  /*
>   * TODO: We need more time to fine tune processors with shared memory so=
lution
> @@ -299,15 +298,12 @@ static int pstate_init_perf(struct amd_cpudata *cpu=
data)
>  				     &cap1);
>  	if (ret)
>  		return ret;
> -
> -	/* For platforms that do not support the preferred core feature, the
> -	 * highest_pef may be configured with 166 or 255, to avoid max frequency
> -	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
> -	 * the default max perf.
> +=20
> +	/* Some CPUs have different highest_perf from others, it is safer=20
> +	 * to read it than to assume some erroneous value, leading to performan=
ce issues.
>  	 */
> -	if (cpudata->hw_prefcore)
> -		highest_perf =3D AMD_PSTATE_PREFCORE_THRESHOLD;
> -	else
> +	highest_perf =3D amd_get_highest_perf();
> +	if(highest_perf > AMD_CPPC_HIGHEST_PERF(cap1))
>  		highest_perf =3D AMD_CPPC_HIGHEST_PERF(cap1);
> =20
>  	WRITE_ONCE(cpudata->highest_perf, highest_perf);
> @@ -329,9 +325,11 @@ static int cppc_init_perf(struct amd_cpudata *cpudat=
a)
>  	if (ret)
>  		return ret;
> =20
> -	if (cpudata->hw_prefcore)
> -		highest_perf =3D AMD_PSTATE_PREFCORE_THRESHOLD;
> -	else
> +	/* Some CPUs have different highest_perf from others, it is safer=20
> +	 * to read it than to assume some erroneous value, leading to performan=
ce issues.
> +	 */
> +	highest_perf =3D amd_get_highest_perf();
> +	if(highest_perf > cppc_perf.highest_perf)
>  		highest_perf =3D cppc_perf.highest_perf;
> =20
>  	WRITE_ONCE(cpudata->highest_perf, highest_perf);
>=20

Please pay attention to trailing whitespaces, adding whitespaces to blank l=
ines, and whitespaces between `if` and opening `(`.

`scripts/checkpatch.pl` may help you with that.

Thank you.

=2D-=20
Oleksandr Natalenko (post-factum)
--nextPart13532597.O9o76ZdvQC
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEZUOOw5ESFLHZZtOKil/iNcg8M0sFAmXUargACgkQil/iNcg8
M0uKvBAA75ErCLUkoBjqYUXvYnxdCiWChCP3YbRF+TZpge2SRBrdHkP37tb0h5xj
/YidBER2zYHcqz5PKoflu54fCIQod0VkX33TQ8HkXjm4tytT75WKs6invTYQOMOD
wrZmWRw5AvsjKNxUGuhtg2sp3CJjigzk7E8JUdeE2L07Wo5//npN2T4uNZAzI3wA
MX6ACRYwwh8yY4iIRTv2PJZfZATo69iRnNbo7UlLAgKqQA+GOsF+jwio2d41O+ZE
5xUfZQDx8DV7Bg/nbhTR0gg0Il3SwsxBg5lIeeLxJch7Rex6tI/Qhj6M0QCPViEA
w+ZyB9yMNhwy5yHH0t3KiSBB3ONy7CI0Umw+xriblr0iWRhlDCktI4/PkGhu6QfB
CLNk2Xrh8zYVUuZ91YBkxE0HEWxRBlI8c5RdMvS2qYqjdBgvdR8n1ZRVfDioIUHc
GCrQSI3/bkfvZDgHJIqFJAQTwS2o4SLUHiNgbrB0V66V9RRvqTlWbqIdztXgWhKi
egIAdiyKMEckiymU9Cqh8Q34Voaf2+o7mOE3Hj1l5s2NCBxk4fWoYJP1uRRF+wZm
FRQ/DuT2EgDDynHHLybnLl5Fy2LAEFh5cdWdZum8aOuCmruEsG06srI6j8qJ1FLk
NjH/oCDfkMemqkVHUFmOO4W9MgY89I4UVQuZGaYDAg2vTDY8+0E=
=MiiV
-----END PGP SIGNATURE-----

--nextPart13532597.O9o76ZdvQC--