| Message ID | 20230323223343.587210-1-quic_bjorande@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: qcom-cpufreq-hw: Revert adding cpufreq qos | expand |
On 23.03.2023 23:33, Bjorn Andersson wrote: > The OSM/EPSS hardware controls the frequency of each CPU cluster based > on requests from the OS and various throttling events in the system. > While throttling is in effect the related dcvs interrupt will be kept > high. The purpose of the code handling this interrupt is to > continuously report the thermal pressure based on the throttled > frequency. > > The reasoning for adding QoS control to this mechanism is not entirely > clear, but the introduction of commit 'c4c0efb06f17 ("cpufreq: > qcom-cpufreq-hw: Add cpufreq qos for LMh")' causes the > scaling_max_frequncy to be set to the throttled frequency. On the next > iteration of polling, the throttled frequency is above or equal to the > newly requested frequency, so the polling is stopped. Oh wow.. That must have been fun to debug.. Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > > With cpufreq limiting the max frequency, the hardware no longer report a > throttling state and no further updates to thermal pressure or qos > state are made. > > The result of this is that scaling_max_frequency can only go down, and > the system becomes slower and slower every time a thermal throttling > event is reported by the hardware. > > Even if the logic could be improved, there is no reason for software to > limit the max freqency in response to the hardware limiting the max > frequency. At best software will follow the reported hardware state, but > typically it will cause slower backoff of the throttling. > > This reverts commit c4c0efb06f17fa4a37ad99e7752b18a5405c76dc. > > Fixes: c4c0efb06f17 ("cpufreq: qcom-cpufreq-hw: Add cpufreq qos for LMh") > Cc: stable@vger.kernel.org > Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 575a4461c25a..1503d315fa7e 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -14,7 +14,6 @@ > #include <linux/of_address.h> > #include <linux/of_platform.h> > #include <linux/pm_opp.h> > -#include <linux/pm_qos.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/units.h> > @@ -60,8 +59,6 @@ struct qcom_cpufreq_data { > struct clk_hw cpu_clk; > > bool per_core_dcvs; > - > - struct freq_qos_request throttle_freq_req; > }; > > static struct { > @@ -351,8 +348,6 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) > > throttled_freq = freq_hz / HZ_PER_KHZ; > > - freq_qos_update_request(&data->throttle_freq_req, throttled_freq); > - > /* Update thermal pressure (the boost frequencies are accepted) */ > arch_update_thermal_pressure(policy->related_cpus, throttled_freq); > > @@ -445,14 +440,6 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index) > if (data->throttle_irq < 0) > return data->throttle_irq; > > - ret = freq_qos_add_request(&policy->constraints, > - &data->throttle_freq_req, FREQ_QOS_MAX, > - FREQ_QOS_MAX_DEFAULT_VALUE); > - if (ret < 0) { > - dev_err(&pdev->dev, "Failed to add freq constraint (%d)\n", ret); > - return ret; > - } > - > data->cancel_throttle = false; > data->policy = policy; > > @@ -519,7 +506,6 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data) > if (data->throttle_irq <= 0) > return; > > - freq_qos_remove_request(&data->throttle_freq_req); > free_irq(data->throttle_irq, data); > } >
On 23-03-23, 15:33, Bjorn Andersson wrote: > The OSM/EPSS hardware controls the frequency of each CPU cluster based > on requests from the OS and various throttling events in the system. > While throttling is in effect the related dcvs interrupt will be kept > high. The purpose of the code handling this interrupt is to > continuously report the thermal pressure based on the throttled > frequency. > > The reasoning for adding QoS control to this mechanism is not entirely > clear, but the introduction of commit 'c4c0efb06f17 ("cpufreq: > qcom-cpufreq-hw: Add cpufreq qos for LMh")' causes the > scaling_max_frequncy to be set to the throttled frequency. On the next > iteration of polling, the throttled frequency is above or equal to the > newly requested frequency, so the polling is stopped. > > With cpufreq limiting the max frequency, the hardware no longer report a > throttling state and no further updates to thermal pressure or qos > state are made. > > The result of this is that scaling_max_frequency can only go down, and > the system becomes slower and slower every time a thermal throttling > event is reported by the hardware. > > Even if the logic could be improved, there is no reason for software to > limit the max freqency in response to the hardware limiting the max > frequency. At best software will follow the reported hardware state, but > typically it will cause slower backoff of the throttling. > > This reverts commit c4c0efb06f17fa4a37ad99e7752b18a5405c76dc. > > Fixes: c4c0efb06f17 ("cpufreq: qcom-cpufreq-hw: Add cpufreq qos for LMh") > Cc: stable@vger.kernel.org > Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 14 -------------- > 1 file changed, 14 deletions(-) Applied. Thanks.
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index 575a4461c25a..1503d315fa7e 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -14,7 +14,6 @@ #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/pm_opp.h> -#include <linux/pm_qos.h> #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/units.h> @@ -60,8 +59,6 @@ struct qcom_cpufreq_data { struct clk_hw cpu_clk; bool per_core_dcvs; - - struct freq_qos_request throttle_freq_req; }; static struct { @@ -351,8 +348,6 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) throttled_freq = freq_hz / HZ_PER_KHZ; - freq_qos_update_request(&data->throttle_freq_req, throttled_freq); - /* Update thermal pressure (the boost frequencies are accepted) */ arch_update_thermal_pressure(policy->related_cpus, throttled_freq); @@ -445,14 +440,6 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index) if (data->throttle_irq < 0) return data->throttle_irq; - ret = freq_qos_add_request(&policy->constraints, - &data->throttle_freq_req, FREQ_QOS_MAX, - FREQ_QOS_MAX_DEFAULT_VALUE); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to add freq constraint (%d)\n", ret); - return ret; - } - data->cancel_throttle = false; data->policy = policy; @@ -519,7 +506,6 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data) if (data->throttle_irq <= 0) return; - freq_qos_remove_request(&data->throttle_freq_req); free_irq(data->throttle_irq, data); }
The OSM/EPSS hardware controls the frequency of each CPU cluster based on requests from the OS and various throttling events in the system. While throttling is in effect the related dcvs interrupt will be kept high. The purpose of the code handling this interrupt is to continuously report the thermal pressure based on the throttled frequency. The reasoning for adding QoS control to this mechanism is not entirely clear, but the introduction of commit 'c4c0efb06f17 ("cpufreq: qcom-cpufreq-hw: Add cpufreq qos for LMh")' causes the scaling_max_frequncy to be set to the throttled frequency. On the next iteration of polling, the throttled frequency is above or equal to the newly requested frequency, so the polling is stopped. With cpufreq limiting the max frequency, the hardware no longer report a throttling state and no further updates to thermal pressure or qos state are made. The result of this is that scaling_max_frequency can only go down, and the system becomes slower and slower every time a thermal throttling event is reported by the hardware. Even if the logic could be improved, there is no reason for software to limit the max freqency in response to the hardware limiting the max frequency. At best software will follow the reported hardware state, but typically it will cause slower backoff of the throttling. This reverts commit c4c0efb06f17fa4a37ad99e7752b18a5405c76dc. Fixes: c4c0efb06f17 ("cpufreq: qcom-cpufreq-hw: Add cpufreq qos for LMh") Cc: stable@vger.kernel.org Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- drivers/cpufreq/qcom-cpufreq-hw.c | 14 -------------- 1 file changed, 14 deletions(-)