| Message ID | 08ed1f9f76a6a1c401efd8f426bdeb9681c4b4e9.1710323410.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | AMD Pstate Fixes And Enhancements | expand |
Hello Perry, On Wed, Mar 13, 2024 at 05:59:13PM +0800, Perry Yuan wrote: > Address an untested error where the nominal_freq was returned in KHz > instead of the correct MHz units, this oversight led to a wrong > nominal_freq set and resued, it will cause the max frequency of core to > be initialized with a wrong frequency value. As I had mentioned in my review comment to v6 [1], cpudata->max_freq, cpudata->min_freq, cpudata->lowest_non_linear_freq are all in khz. With this patch, cpudata->nominal_freq will be in mhz. As Dhananjay confirmed [2], this patch breaks the reporting in /sys/devices/system/cpu/cpufreq/policyX/*_freq as some of them will be reported in mhz while some others in khz which breaks the expectation that all these sysfs values should be reported in khz. [cpufreq]# grep . *freq amd_pstate_lowest_nonlinear_freq:1804000 <----- in khz amd_pstate_max_freq:3514000 <----- in khz cpuinfo_max_freq:2151 <----- in mhz cpuinfo_min_freq:400000 <----- in khz scaling_cur_freq:2151 <----- in mhz scaling_max_freq:2151 <----- in mhz scaling_min_freq:2151 <----- in mhz [cpufreq]# pwd /sys/devices/system/cpu/cpu0/cpufreq What am I missing ? [1] https://lore.kernel.org/lkml/ZcRvoYZKdUEjBUHp@BLR-5CG11610CF.amd.com/) [2] https://lore.kernel.org/lkml/1aecf2fc-2ea4-46ec-aaf2-0dbbb11b5f8b@amd.com/ > > Cc: stable@vger.kernel.org > Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors") > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Perry Yuan <perry.yuan@amd.com> -- Thanks and Regards gautham. > --- > drivers/cpufreq/amd-pstate.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 2015c9fcc3c9..3faa895b77b7 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -647,8 +647,7 @@ static int amd_get_nominal_freq(struct amd_cpudata *cpudata) > if (ret) > return ret; > > - /* Switch to khz */ > - return cppc_perf.nominal_freq * 1000; > + return cppc_perf.nominal_freq; > } > > static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata) > -- > 2.34.1 >
[AMD Official Use Only - General] Hi Gautham > -----Original Message----- > From: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com> > Sent: Thursday, March 14, 2024 1:49 PM > To: Yuan, Perry <Perry.Yuan@amd.com> > Cc: rafael.j.wysocki@intel.com; Limonciello, Mario > <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>; > Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v7 1/6] cpufreq:amd-pstate: fix the nominal freq value set > > Hello Perry, > > On Wed, Mar 13, 2024 at 05:59:13PM +0800, Perry Yuan wrote: > > Address an untested error where the nominal_freq was returned in KHz > > instead of the correct MHz units, this oversight led to a wrong > > nominal_freq set and resued, it will cause the max frequency of core > > to be initialized with a wrong frequency value. > > As I had mentioned in my review comment to v6 [1], cpudata->max_freq, > cpudata->min_freq, cpudata->lowest_non_linear_freq are all in > khz. With this patch, cpudata->nominal_freq will be in mhz. > > As Dhananjay confirmed [2], this patch breaks the reporting in > /sys/devices/system/cpu/cpufreq/policyX/*_freq as some of them will be > reported in mhz while some others in khz which breaks the expectation that all > these sysfs values should be reported in khz. > > [cpufreq]# grep . *freq > amd_pstate_lowest_nonlinear_freq:1804000 <----- in khz > amd_pstate_max_freq:3514000 <----- in khz > cpuinfo_max_freq:2151 <----- in mhz > cpuinfo_min_freq:400000 <----- in khz > scaling_cur_freq:2151 <----- in mhz > scaling_max_freq:2151 <----- in mhz > scaling_min_freq:2151 <----- in mhz > [cpufreq]# pwd > /sys/devices/system/cpu/cpu0/cpufreq > > What am I missing ? https://lore.kernel.org/lkml/42a36c7f788e0fb77d4be7575aab9c937e1773de.1710322310.git.perry.yuan@amd.com/ Changes from v3: * fix the max frequency value to be KHz when cpb boost disabled(Gautham R. Shenoy) The previous problem has been resolved by the new patchset of cpb boost support + if (on) + policy->cpuinfo.max_freq = cpudata->max_freq; + else + policy->cpuinfo.max_freq = cpudata->nominal_freq * 1000; The frequency values of cpuinfo are correct on my system. amd_pstate_lowest_nonlinear_freq:1701000 amd_pstate_max_freq:3501000 cpuinfo_max_freq:3501000 cpuinfo_min_freq:400000 scaling_cur_freq:400000 scaling_max_freq:3501000 scaling_min_freq:400000 Perry. > > [1] https://lore.kernel.org/lkml/ZcRvoYZKdUEjBUHp@BLR- > 5CG11610CF.amd.com/) > [2] https://lore.kernel.org/lkml/1aecf2fc-2ea4-46ec-aaf2- > 0dbbb11b5f8b@amd.com/ > > > > > Cc: stable@vger.kernel.org > > Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State > > driver to support future processors") > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > -- > Thanks and Regards > gautham. > > > > --- > > drivers/cpufreq/amd-pstate.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c > > b/drivers/cpufreq/amd-pstate.c index 2015c9fcc3c9..3faa895b77b7 > 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -647,8 +647,7 @@ static int amd_get_nominal_freq(struct > amd_cpudata *cpudata) > > if (ret) > > return ret; > > > > - /* Switch to khz */ > > - return cppc_perf.nominal_freq * 1000; > > + return cppc_perf.nominal_freq; > > } > > > > static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata) > > -- > > 2.34.1 > >
Hello Perry, On Thu, Mar 14, 2024 at 11:39:20AM +0530, Yuan, Perry wrote: > [AMD Official Use Only - General] > > Hi Gautham > > > -----Original Message----- > > From: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com> > > Sent: Thursday, March 14, 2024 1:49 PM > > To: Yuan, Perry <Perry.Yuan@amd.com> > > Cc: rafael.j.wysocki@intel.com; Limonciello, Mario > > <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Huang, Ray > > <Ray.Huang@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>; > > Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer > > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH v7 1/6] cpufreq:amd-pstate: fix the nominal freq value set > > > > Hello Perry, > > > > On Wed, Mar 13, 2024 at 05:59:13PM +0800, Perry Yuan wrote: > > > Address an untested error where the nominal_freq was returned in KHz > > > instead of the correct MHz units, this oversight led to a wrong > > > nominal_freq set and resued, it will cause the max frequency of core > > > to be initialized with a wrong frequency value. What is still not clear from this commit log or the rest of the patch is, which part of the kernel code expects nominal_freq to be in MHz, when all the other freqs in cpudata are in KHz units. If nominal_freq is in KHz as it is currently, how does it cause the max frequency to be initialized to the wrong value ? Could you please elaborate this ? > > > > As I had mentioned in my review comment to v6 [1], cpudata->max_freq, > > cpudata->min_freq, cpudata->lowest_non_linear_freq are all in > > khz. With this patch, cpudata->nominal_freq will be in mhz. > > > > As Dhananjay confirmed [2], this patch breaks the reporting in > > /sys/devices/system/cpu/cpufreq/policyX/*_freq as some of them will be > > reported in mhz while some others in khz which breaks the expectation that all > > these sysfs values should be reported in khz. > > > > [cpufreq]# grep . *freq > > amd_pstate_lowest_nonlinear_freq:1804000 <----- in khz > > amd_pstate_max_freq:3514000 <----- in khz > > cpuinfo_max_freq:2151 <----- in mhz > > cpuinfo_min_freq:400000 <----- in khz > > scaling_cur_freq:2151 <----- in mhz > > scaling_max_freq:2151 <----- in mhz > > scaling_min_freq:2151 <----- in mhz > > [cpufreq]# pwd > > /sys/devices/system/cpu/cpu0/cpufreq > > > > What am I missing ? > > https://lore.kernel.org/lkml/42a36c7f788e0fb77d4be7575aab9c937e1773de.1710322310.git.perry.yuan@amd.com/ > Changes from v3: > * fix the max frequency value to be KHz when cpb boost disabled(Gautham R. Shenoy) This CPB boost change assumes that cpudata->nominal_freq is in Mhz which is not the case until this patch. So is the CPB patchset dependent on this patch ? -- Thanks and Regards gautham. > > The previous problem has been resolved by the new patchset of cpb boost support > > + if (on) > + policy->cpuinfo.max_freq = cpudata->max_freq; > + else > + policy->cpuinfo.max_freq = cpudata->nominal_freq * 1000; > > > The frequency values of cpuinfo are correct on my system. > > amd_pstate_lowest_nonlinear_freq:1701000 > amd_pstate_max_freq:3501000 > cpuinfo_max_freq:3501000 > cpuinfo_min_freq:400000 > scaling_cur_freq:400000 > scaling_max_freq:3501000 > scaling_min_freq:400000 > > Perry. > > > > > [1] https://lore.kernel.org/lkml/ZcRvoYZKdUEjBUHp@BLR- > > 5CG11610CF.amd.com/) > > [2] https://lore.kernel.org/lkml/1aecf2fc-2ea4-46ec-aaf2- > > 0dbbb11b5f8b@amd.com/ > > > > > > > > Cc: stable@vger.kernel.org > > > Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State > > > driver to support future processors") > > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > > > -- > > Thanks and Regards > > gautham. > > > > > > > --- > > > drivers/cpufreq/amd-pstate.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/cpufreq/amd-pstate.c > > > b/drivers/cpufreq/amd-pstate.c index 2015c9fcc3c9..3faa895b77b7 > > 100644 > > > --- a/drivers/cpufreq/amd-pstate.c > > > +++ b/drivers/cpufreq/amd-pstate.c > > > @@ -647,8 +647,7 @@ static int amd_get_nominal_freq(struct > > amd_cpudata *cpudata) > > > if (ret) > > > return ret; > > > > > > - /* Switch to khz */ > > > - return cppc_perf.nominal_freq * 1000; > > > + return cppc_perf.nominal_freq; > > > } > > > > > > static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata) > > > -- > > > 2.34.1 > > >
[AMD Official Use Only - General] > -----Original Message----- > From: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com> > Sent: Thursday, March 14, 2024 5:32 PM > To: Yuan, Perry <Perry.Yuan@amd.com> > Cc: rafael.j.wysocki@intel.com; Limonciello, Mario > <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>; Deucher, > Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li > (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v7 1/6] cpufreq:amd-pstate: fix the nominal freq value set > > Hello Perry, > > On Thu, Mar 14, 2024 at 11:39:20AM +0530, Yuan, Perry wrote: > > [AMD Official Use Only - General] > > > > Hi Gautham > > > > > -----Original Message----- > > > From: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com> > > > Sent: Thursday, March 14, 2024 1:49 PM > > > To: Yuan, Perry <Perry.Yuan@amd.com> > > > Cc: rafael.j.wysocki@intel.com; Limonciello, Mario > > > <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Huang, Ray > > > <Ray.Huang@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>; > > > Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer > > > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, > > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH v7 1/6] cpufreq:amd-pstate: fix the nominal freq > > > value set > > > > > > Hello Perry, > > > > > > On Wed, Mar 13, 2024 at 05:59:13PM +0800, Perry Yuan wrote: > > > > Address an untested error where the nominal_freq was returned in > > > > KHz instead of the correct MHz units, this oversight led to a > > > > wrong nominal_freq set and resued, it will cause the max frequency > > > > of core to be initialized with a wrong frequency value. > > What is still not clear from this commit log or the rest of the patch is, which part > of the kernel code expects nominal_freq to be in MHz, when all the other freqs in > cpudata are in KHz units. > > If nominal_freq is in KHz as it is currently, how does it cause the max frequency to > be initialized to the wrong value ? Could you please elaborate this ? OK, here is the story. Actually, the original capability values are Mhz like below, so the driver need to initialize the nominal_freq as the as-it-is value, then pstate driver will calculate the max frequency as needed. feedback_ctrs:ref:103751311076 del:87445442175 highest_perf:255 lowest_freq:400 lowest_nonlinear_perf:124 lowest_perf:30 nominal_freq:2801 nominal_perf:204 reference_perf:204 wraparound_time:18446744073709551615 The previous driver did not use the READ_ONCE(cpudata-> nominal_freq) at all. We initialize all the freq and perf values in the init functions like you suggested in the other patchset. if driver still use Khz, below code will have problem. nominal_freq = READ_ONCE(cpudata-> nominal_freq); lowest_nonlinear_freq = nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT; /* Switch to khz */ return lowest_nonlinear_freq * 1000; Now we can read READ_ONCE(cpudata-> nominal_freq) without reading the CPPC ACPI again. The nominal_freq must be in MHz as it is. Perry. > > > > > > > As I had mentioned in my review comment to v6 [1], > > > cpudata->max_freq, > > > cpudata->min_freq, cpudata->lowest_non_linear_freq are all in > > > khz. With this patch, cpudata->nominal_freq will be in mhz. > > > > > > As Dhananjay confirmed [2], this patch breaks the reporting in > > > /sys/devices/system/cpu/cpufreq/policyX/*_freq as some of them will > > > be reported in mhz while some others in khz which breaks the > > > expectation that all these sysfs values should be reported in khz. > > > > > > [cpufreq]# grep . *freq > > > amd_pstate_lowest_nonlinear_freq:1804000 <----- in khz > > > amd_pstate_max_freq:3514000 <----- in khz > > > cpuinfo_max_freq:2151 <----- in mhz > > > cpuinfo_min_freq:400000 <----- in khz > > > scaling_cur_freq:2151 <----- in mhz > > > scaling_max_freq:2151 <----- in mhz > > > scaling_min_freq:2151 <----- in mhz > > > [cpufreq]# pwd > > > /sys/devices/system/cpu/cpu0/cpufreq > > > > > > What am I missing ? > > > > https://lore.kernel.org/lkml/42a36c7f788e0fb77d4be7575aab9c937e1773de. > > 1710322310.git.perry.yuan@amd.com/ > > Changes from v3: > > * fix the max frequency value to be KHz when cpb boost > > disabled(Gautham R. Shenoy) > > This CPB boost change assumes that cpudata->nominal_freq is in Mhz which is > not the case until this patch. So is the CPB patchset dependent on this patch ? > > -- > Thanks and Regards > gautham. > > > > > The previous problem has been resolved by the new patchset of cpb > > boost support > > > > + if (on) > > + policy->cpuinfo.max_freq = cpudata->max_freq; > > + else > > + policy->cpuinfo.max_freq = cpudata->nominal_freq * > > + 1000; > > > > > > The frequency values of cpuinfo are correct on my system. > > > > amd_pstate_lowest_nonlinear_freq:1701000 > > amd_pstate_max_freq:3501000 > > cpuinfo_max_freq:3501000 > > cpuinfo_min_freq:400000 > > scaling_cur_freq:400000 > > scaling_max_freq:3501000 > > scaling_min_freq:400000 > > > > Perry. > > > > > > > > [1] https://lore.kernel.org/lkml/ZcRvoYZKdUEjBUHp@BLR- > > > 5CG11610CF.amd.com/) > > > [2] https://lore.kernel.org/lkml/1aecf2fc-2ea4-46ec-aaf2- > > > 0dbbb11b5f8b@amd.com/ > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD > > > > P-State driver to support future processors") > > > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > > > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > > > > > -- > > > Thanks and Regards > > > gautham. > > > > > > > > > > --- > > > > drivers/cpufreq/amd-pstate.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/cpufreq/amd-pstate.c > > > > b/drivers/cpufreq/amd-pstate.c index 2015c9fcc3c9..3faa895b77b7 > > > 100644 > > > > --- a/drivers/cpufreq/amd-pstate.c > > > > +++ b/drivers/cpufreq/amd-pstate.c > > > > @@ -647,8 +647,7 @@ static int amd_get_nominal_freq(struct > > > amd_cpudata *cpudata) > > > > if (ret) > > > > return ret; > > > > > > > > - /* Switch to khz */ > > > > - return cppc_perf.nominal_freq * 1000; > > > > + return cppc_perf.nominal_freq; > > > > } > > > > > > > > static int amd_get_lowest_nonlinear_freq(struct amd_cpudata > > > > *cpudata) > > > > -- > > > > 2.34.1 > > > >
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 2015c9fcc3c9..3faa895b77b7 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -647,8 +647,7 @@ static int amd_get_nominal_freq(struct amd_cpudata *cpudata) if (ret) return ret; - /* Switch to khz */ - return cppc_perf.nominal_freq * 1000; + return cppc_perf.nominal_freq; } static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
Address an untested error where the nominal_freq was returned in KHz instead of the correct MHz units, this oversight led to a wrong nominal_freq set and resued, it will cause the max frequency of core to be initialized with a wrong frequency value. Cc: stable@vger.kernel.org Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors") Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> Signed-off-by: Perry Yuan <perry.yuan@amd.com> --- drivers/cpufreq/amd-pstate.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)