| Message ID | 20240624213400.67773-6-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | AMD Pstate Driver Core Performance Boost | expand |
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 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 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 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 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 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(-)