| Message ID | 20240624213400.67773-6-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | AMD Pstate Driver Core Performance Boost | expand |
On 24-06-24, 16:34, Mario Limonciello wrote: > The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy > boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost > policy incorrectly when boost has been enabled by the platform firmware > initially even if a driver sets the policy up. > > This is because there are no discrete entries in the frequency table. > Update that code to only run when a frequency table is present. > > Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > Cc: Sibi Sankar <quic_sibis@quicinc.com> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Dhruva Gole <d-gole@ti.com> > Cc: Yipeng Zou <zouyipeng@huawei.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > drivers/cpufreq/cpufreq.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 1fdabb660231..32c119614710 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu) > } > > /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); > + if (policy->freq_table) > + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); I am not sure if I understand the problem properly here. Can you please explain a bit in detail ?
Hello Mario, Mario Limonciello <mario.limonciello@amd.com> writes: > The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy > boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost > policy incorrectly when boost has been enabled by the platform firmware > initially even if a driver sets the policy up. > > This is because there are no discrete entries in the frequency table. > Update that code to only run when a frequency table is present. Thanks for this fix. There are also drivers such as acpi-cpufreq which have a frequency table, but the boost Pstates are not advertised. Thus none of the table entries have CPUFREQ_BOOST_FREQ set. > > Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > Cc: Sibi Sankar <quic_sibis@quicinc.com> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Dhruva Gole <d-gole@ti.com> > Cc: Yipeng Zou <zouyipeng@huawei.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > drivers/cpufreq/cpufreq.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 1fdabb660231..32c119614710 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu) > } > > /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); > + if (policy->freq_table) > + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); > > /* > * The initialization has succeeded and the policy is > online. How about something like the following: --- diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 37f1cdf46d29..be5f4d3e9c1d 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -140,6 +140,7 @@ static int set_boost(struct cpufreq_policy *policy, int val) pr_debug("CPU %*pbl: Core Boosting %s.\n", cpumask_pr_args(policy->cpus), str_enabled_disabled(val)); + policy->boost_enabled = val; return 0; } diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c index 10e80d912b8d..f604389b9cd6 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -18,6 +18,10 @@ bool policy_has_boost_freq(struct cpufreq_policy *policy) { struct cpufreq_frequency_table *pos, *table = policy->freq_table; + /* The driver has explicitly advertised the boost-capabilities */ + if (policy->boost_enabled) + return true; + if (!table) return false; > -- > 2.43.0
On 6/25/2024 01:30, Viresh Kumar wrote: > On 24-06-24, 16:34, Mario Limonciello wrote: >> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy >> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost >> policy incorrectly when boost has been enabled by the platform firmware >> initially even if a driver sets the policy up. >> >> This is because there are no discrete entries in the frequency table. >> Update that code to only run when a frequency table is present. >> >> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()") >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> Cc: Sibi Sankar <quic_sibis@quicinc.com> >> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Cc: Dhruva Gole <d-gole@ti.com> >> Cc: Yipeng Zou <zouyipeng@huawei.com> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> drivers/cpufreq/cpufreq.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 1fdabb660231..32c119614710 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu) >> } >> >> /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ >> - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); >> + if (policy->freq_table) >> + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); > > I am not sure if I understand the problem properly here. Can you > please explain a bit in detail ? > The core issue is that there are drivers that have boost functionality but don't have a frequency table. As pointed out by Gautham there are also drivers that have a frequency table but don't advertise boost pstates (CPUFREQ_BOOST_FREQ isn't set on any frequency). So what happens is the driver may have choosen a policy to have boost enabled but when cpufreq_online() is called it gets "marked" disabled from this check introduced in f37a4d6b4a2c even though it's previously enabled.
On 6/25/2024 06:55, Gautham R.Shenoy wrote: > > Hello Mario, > > Mario Limonciello <mario.limonciello@amd.com> writes: > >> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy >> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost >> policy incorrectly when boost has been enabled by the platform firmware >> initially even if a driver sets the policy up. >> >> This is because there are no discrete entries in the frequency table. >> Update that code to only run when a frequency table is present. > > Thanks for this fix. > > > There are also drivers such as acpi-cpufreq which have a frequency > table, but the boost Pstates are not advertised. Thus none of the table > entries have CPUFREQ_BOOST_FREQ set. > > >> >> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()") >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> Cc: Sibi Sankar <quic_sibis@quicinc.com> >> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Cc: Dhruva Gole <d-gole@ti.com> >> Cc: Yipeng Zou <zouyipeng@huawei.com> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> drivers/cpufreq/cpufreq.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 1fdabb660231..32c119614710 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu) >> } >> >> /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ >> - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); >> + if (policy->freq_table) >> + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); >> >> /* >> * The initialization has succeeded and the policy is >> online. > > > How about something like the following: > > --- > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 37f1cdf46d29..be5f4d3e9c1d 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -140,6 +140,7 @@ static int set_boost(struct cpufreq_policy *policy, int val) > pr_debug("CPU %*pbl: Core Boosting %s.\n", > cpumask_pr_args(policy->cpus), str_enabled_disabled(val)); > > + policy->boost_enabled = val; > return 0; > } > > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c > index 10e80d912b8d..f604389b9cd6 100644 > --- a/drivers/cpufreq/freq_table.c > +++ b/drivers/cpufreq/freq_table.c > @@ -18,6 +18,10 @@ bool policy_has_boost_freq(struct cpufreq_policy *policy) > { > struct cpufreq_frequency_table *pos, *table = policy->freq_table; > > + /* The driver has explicitly advertised the boost-capabilities */ > + if (policy->boost_enabled) > + return true; > + > if (!table) > return false; > > > > >> -- >> 2.43.0 Yeah I think this works too. Let's see see what Viresh thinks.
On 25-06-24, 07:31, Mario Limonciello wrote: > The core issue is that there are drivers that have boost functionality but > don't have a frequency table. As pointed out by Gautham there are also > drivers that have a frequency table but don't advertise boost pstates > (CPUFREQ_BOOST_FREQ isn't set on any frequency). > > So what happens is the driver may have choosen a policy to have boost > enabled but when cpufreq_online() is called it gets "marked" disabled from > this check introduced in f37a4d6b4a2c even though it's previously enabled. And who was setting policy->boost_enabled to true before that ? Also how will your patch fix the problem ? I don't see any other code setting it too, unless request comes from sysfs, which would work even now I think.
On 6/25/2024 22:11, Viresh Kumar wrote: > On 25-06-24, 07:31, Mario Limonciello wrote: >> The core issue is that there are drivers that have boost functionality but >> don't have a frequency table. As pointed out by Gautham there are also >> drivers that have a frequency table but don't advertise boost pstates >> (CPUFREQ_BOOST_FREQ isn't set on any frequency). >> >> So what happens is the driver may have choosen a policy to have boost >> enabled but when cpufreq_online() is called it gets "marked" disabled from >> this check introduced in f37a4d6b4a2c even though it's previously enabled. > > And who was setting policy->boost_enabled to true before that ? Also > how will your patch fix the problem ? I don't see any other code > setting it too, unless request comes from sysfs, which would work even > now I think. > The earlier patches in this series do that with amd-pstate. Gautham had suggested a change to acpi-cpufreq for the same too. However I tested Gautham's suggestion (which is in this thread) and I think it's the better way to do it than what I did in this v14 patch.
On 25-06-24, 22:14, Mario Limonciello wrote: > The earlier patches in this series do that with amd-pstate. Gautham had > suggested a change to acpi-cpufreq for the same too. Ahh, since I wasn't cc'd, I missed that obvious part :) The right fix would be this then I guess: diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a45aac17c20f..9e5060b27864 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1431,7 +1431,8 @@ static int cpufreq_online(unsigned int cpu) } /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); + if (cpufreq_boost_enabled() && policy_has_boost_freq(policy)) + policy->boost_enabled = true; /* * The initialization has succeeded and the policy is online.
On 6/25/2024 22:17, Viresh Kumar wrote: > On 25-06-24, 22:14, Mario Limonciello wrote: >> The earlier patches in this series do that with amd-pstate. Gautham had >> suggested a change to acpi-cpufreq for the same too. > > Ahh, since I wasn't cc'd, I missed that obvious part :) > > The right fix would be this then I guess: > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index a45aac17c20f..9e5060b27864 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1431,7 +1431,8 @@ static int cpufreq_online(unsigned int cpu) > } > > /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); > + if (cpufreq_boost_enabled() && policy_has_boost_freq(policy)) > + policy->boost_enabled = true; > > /* > * The initialization has succeeded and the policy is online. > Yeah; that's effectively the same result as Gautham's suggestion. He had just said to change policy_has_boost_freq() for the same. I'll test it with yours and reconcile the better one to submit back out for v15. Thank you for your feedback.
On 25-06-24, 22:20, Mario Limonciello wrote: > Yeah; that's effectively the same result as Gautham's suggestion. He had > just said to change policy_has_boost_freq() for the same. I know. The problem is policy_has_boost_freq() (as its name suggests) needs to check if the policy supports boost freqs (in a generic way) and it is used at several other places and it would be wrong to hack that routine to fix this problem. All we want here is for the core to not touch boost_enabled if the policy->init() function has already done so. > I'll test it with yours and reconcile the better one to submit back out for > v15. You can send it separately to be honest, and it looks like a fix, so Rafael should be able to get it merged a bit sooner. Add a fixes tag too.
On 26-06-24, 08:55, Viresh Kumar wrote: > On 25-06-24, 22:20, Mario Limonciello wrote: > > Yeah; that's effectively the same result as Gautham's suggestion. He had > > just said to change policy_has_boost_freq() for the same. > > I know. The problem is policy_has_boost_freq() (as its name suggests) > needs to check if the policy supports boost freqs (in a generic way) > and it is used at several other places and it would be wrong to hack > that routine to fix this problem. > > All we want here is for the core to not touch boost_enabled if the > policy->init() function has already done so. > > > I'll test it with yours and reconcile the better one to submit back out for > > v15. > > You can send it separately to be honest, and it looks like a fix, so > Rafael should be able to get it merged a bit sooner. Add a fixes tag > too. And maybe send patch for acpi-cpufreq and any other platform that is broken too..
On 6/25/2024 22:27, Viresh Kumar wrote: > On 26-06-24, 08:55, Viresh Kumar wrote: >> On 25-06-24, 22:20, Mario Limonciello wrote: >>> Yeah; that's effectively the same result as Gautham's suggestion. He had >>> just said to change policy_has_boost_freq() for the same. >> >> I know. The problem is policy_has_boost_freq() (as its name suggests) >> needs to check if the policy supports boost freqs (in a generic way) >> and it is used at several other places and it would be wrong to hack >> that routine to fix this problem. >> >> All we want here is for the core to not touch boost_enabled if the >> policy->init() function has already done so. >> >>> I'll test it with yours and reconcile the better one to submit back out for >>> v15. >> >> You can send it separately to be honest, and it looks like a fix, so >> Rafael should be able to get it merged a bit sooner. Add a fixes tag >> too. > > And maybe send patch for acpi-cpufreq and any other platform that is > broken too.. > I can take it all through the amd-pstate tree. I'll put it at the front of the series. I think as long as we can get it merged before ~rc6 it should be fine for the 6.11 merge window.
On 25-06-24, 22:33, Mario Limonciello wrote: > I can take it all through the amd-pstate tree. Unless there is a dependency, we try to take the patches through the PM tree itself as there can be conflicting patches there otherwise. > I'll put it at the front of the series. > I think as long as we can get it merged before ~rc6 it should be fine for > the 6.11 merge window. Since this is a fix, it may get into 6.10 itself.
On 6/25/2024 22:44, Viresh Kumar wrote: > On 25-06-24, 22:33, Mario Limonciello wrote: >> I can take it all through the amd-pstate tree. > > Unless there is a dependency, we try to take the patches through the > PM tree itself as there can be conflicting patches there otherwise. Right; but amd-pstate merges to the PM tree. > >> I'll put it at the front of the series. >> I think as long as we can get it merged before ~rc6 it should be fine for >> the 6.11 merge window. > > Since this is a fix, it may get into 6.10 itself. > Good point. I'll do some testing with your suggestion and send those two as 6.10 material and then the rest of this series at v15 for 6.11 remaining material if we get it done in time.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1fdabb660231..32c119614710 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu) } /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); + if (policy->freq_table) + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); /* * The initialization has succeeded and the policy is online.
The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost policy incorrectly when boost has been enabled by the platform firmware initially even if a driver sets the policy up. This is because there are no discrete entries in the frequency table. Update that code to only run when a frequency table is present. Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- Cc: Sibi Sankar <quic_sibis@quicinc.com> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: Dhruva Gole <d-gole@ti.com> Cc: Yipeng Zou <zouyipeng@huawei.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> drivers/cpufreq/cpufreq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)