| Message ID | 39e39a7d30c8ee6af81fb64670a330abeb87402e.1651652493.git.viresh.kumar@linaro.org |
|---|---|
| State | Accepted |
| Commit | f55ae08c89873e140c7cac2a7fa161d31a0d60cf |
| Headers | show |
| Series | cpufreq: Avoid unnecessary frequency updates due to mismatch | expand |
On Wed, 4 May 2022 at 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > For some platforms, the frequency returned by hardware may be slightly > different from what is provided in the frequency table. For example, Do you have more details ? Do you mean that between 2 consecutives reads you can get either 500Mhz or 499Mhz ? Or is it a fixed mismatch between the table and the freq returned by HW ? > hardware may return 499 MHz instead of 500 MHz. In such cases it is > better to avoid getting into unnecessary frequency updates, as we may > end up switching policy->cur between the two and sending unnecessary > pre/post update notifications, etc. > > This patch has chosen allows the hardware frequency and table frequency > to deviate by 1 MHz for now, we may want to increase it a bit later on > if someone still complains. > > Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 0d58b0f8f3af..233e8af48848 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -28,6 +28,7 @@ > #include <linux/suspend.h> > #include <linux/syscore_ops.h> > #include <linux/tick.h> > +#include <linux/units.h> > #include <trace/events/power.h> > > static LIST_HEAD(cpufreq_policy_list); > @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b > return new_freq; > > if (policy->cur != new_freq) { > + /* > + * For some platforms, the frequency returned by hardware may be > + * slightly different from what is provided in the frequency > + * table, for example hardware may return 499 MHz instead of 500 > + * MHz. In such cases it is better to avoid getting into > + * unnecessary frequency updates. > + */ > + if (abs(policy->cur - new_freq) < HZ_PER_MHZ) > + return policy->cur; > + > cpufreq_out_of_sync(policy, new_freq); > if (update) > schedule_work(&policy->update); > -- > 2.31.1.272.g89b43f80a514 >
On 05-05-22, 09:28, Vincent Guittot wrote: > On Wed, 4 May 2022 at 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > For some platforms, the frequency returned by hardware may be slightly > > different from what is provided in the frequency table. For example, > > Do you have more details ? This is where the problem was discussed. https://lore.kernel.org/lkml/20220422075239.16437-8-rex-bc.chen@mediatek.com/ > Do you mean that between 2 consecutives reads you can get either > 500Mhz or 499Mhz ? No, the hardware always returns something like 499,999,726 Hz, but the OPP table contains the value 500 MHz. The field policy->cur is set based on opp table eventually (target_index) and so contains 500MHz, almost always. But when cpufreq_get() is called, it finds the current freq is 499 MHz, instead of 500 MHz. And so the issue. > Or is it a fixed mismatch between the table and the freq returned by HW ? Yes.
On Thu, 5 May 2022 at 09:44, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 05-05-22, 09:28, Vincent Guittot wrote: > > On Wed, 4 May 2022 at 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > For some platforms, the frequency returned by hardware may be slightly > > > different from what is provided in the frequency table. For example, > > > > Do you have more details ? > > This is where the problem was discussed. > > https://lore.kernel.org/lkml/20220422075239.16437-8-rex-bc.chen@mediatek.com/ Thanks for the link > > > Do you mean that between 2 consecutives reads you can get either > > 500Mhz or 499Mhz ? > > No, the hardware always returns something like 499,999,726 Hz, but the Part of your problem is that cpufreq use khz whereas clock uses hz Would it be better to do something like below in cpufreq_generic_get (clk_get_rate(policy->clk) + 500) / 1000 so you round to closest instead of always floor rounding > OPP table contains the value 500 MHz. The field policy->cur is set > based on opp table eventually (target_index) and so contains 500MHz, > almost always. But when cpufreq_get() is called, it finds the current > freq is 499 MHz, instead of 500 MHz. And so the issue. > > > Or is it a fixed mismatch between the table and the freq returned by HW ? > > Yes. > > -- > viresh
On 05-05-22, 10:21, Vincent Guittot wrote: > Part of your problem is that cpufreq use khz whereas clock uses hz Not in this case at least as the value mentioned in OPP table DT is in Hz. > Would it be better to do something like below in cpufreq_generic_get > > (clk_get_rate(policy->clk) + 500) / 1000 > > so you round to closest instead of always floor rounding That would be a fine thing to do anyway, though I am not sure if it will fix the problem at hand. If the hardware returns 499,999,499 Hz, we will still have the problem.
On Thu, 5 May 2022 at 10:28, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 05-05-22, 10:21, Vincent Guittot wrote: > > Part of your problem is that cpufreq use khz whereas clock uses hz > > Not in this case at least as the value mentioned in OPP table DT is in > Hz. But dev_pm_opp_init_cpufreq_table make it kHz anyway > > > Would it be better to do something like below in cpufreq_generic_get > > > > (clk_get_rate(policy->clk) + 500) / 1000 > > > > so you round to closest instead of always floor rounding > > That would be a fine thing to do anyway, though I am not sure if it > will fix the problem at hand. > > If the hardware returns 499,999,499 Hz, we will still have the > problem. But in this case, cpufreq table should use 499,999Khz IMO. We already have OPP/cpufreq table being updated at boot with actual value. > > -- > viresh
On 05-05-22, 11:40, Vincent Guittot wrote: > On Thu, 5 May 2022 at 10:28, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 05-05-22, 10:21, Vincent Guittot wrote: > > > Part of your problem is that cpufreq use khz whereas clock uses hz > > > > Not in this case at least as the value mentioned in OPP table DT is in > > Hz. > > But dev_pm_opp_init_cpufreq_table make it kHz anyway Yes. > > > Would it be better to do something like below in cpufreq_generic_get > > > > > > (clk_get_rate(policy->clk) + 500) / 1000 > > > > > > so you round to closest instead of always floor rounding > > > > That would be a fine thing to do anyway, though I am not sure if it > > will fix the problem at hand. > > > > If the hardware returns 499,999,499 Hz, we will still have the > > problem. > > But in this case, cpufreq table should use 499,999Khz IMO. I did think about it earlier, but then left it. > We already > have OPP/cpufreq table being updated at boot with actual value. I don't think we update the frequency values there yet, but yes one way to fix it is via DT.
On 04/05/2022 10:21, Viresh Kumar wrote: > For some platforms, the frequency returned by hardware may be slightly > different from what is provided in the frequency table. For example, > hardware may return 499 MHz instead of 500 MHz. In such cases it is > better to avoid getting into unnecessary frequency updates, as we may > end up switching policy->cur between the two and sending unnecessary > pre/post update notifications, etc. > > This patch has chosen allows the hardware frequency and table frequency > to deviate by 1 MHz for now, we may want to increase it a bit later on > if someone still complains. > > Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/cpufreq/cpufreq.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 0d58b0f8f3af..233e8af48848 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -28,6 +28,7 @@ > #include <linux/suspend.h> > #include <linux/syscore_ops.h> > #include <linux/tick.h> > +#include <linux/units.h> > #include <trace/events/power.h> > > static LIST_HEAD(cpufreq_policy_list); > @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b > return new_freq; > > if (policy->cur != new_freq) { > + /* > + * For some platforms, the frequency returned by hardware may be > + * slightly different from what is provided in the frequency > + * table, for example hardware may return 499 MHz instead of 500 > + * MHz. In such cases it is better to avoid getting into > + * unnecessary frequency updates. > + */ > + if (abs(policy->cur - new_freq) < HZ_PER_MHZ) > + return policy->cur; > + > cpufreq_out_of_sync(policy, new_freq); > if (update) > schedule_work(&policy->update);
On Thu, May 5, 2022 at 3:32 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: > > > > On 04/05/2022 10:21, Viresh Kumar wrote: > > For some platforms, the frequency returned by hardware may be slightly > > different from what is provided in the frequency table. For example, > > hardware may return 499 MHz instead of 500 MHz. In such cases it is > > better to avoid getting into unnecessary frequency updates, as we may > > end up switching policy->cur between the two and sending unnecessary > > pre/post update notifications, etc. > > > > This patch has chosen allows the hardware frequency and table frequency > > to deviate by 1 MHz for now, we may want to increase it a bit later on > > if someone still complains. > > > > Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > > > --- > > drivers/cpufreq/cpufreq.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 0d58b0f8f3af..233e8af48848 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -28,6 +28,7 @@ > > #include <linux/suspend.h> > > #include <linux/syscore_ops.h> > > #include <linux/tick.h> > > +#include <linux/units.h> > > #include <trace/events/power.h> > > > > static LIST_HEAD(cpufreq_policy_list); > > @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b > > return new_freq; > > > > if (policy->cur != new_freq) { > > + /* > > + * For some platforms, the frequency returned by hardware may be > > + * slightly different from what is provided in the frequency > > + * table, for example hardware may return 499 MHz instead of 500 > > + * MHz. In such cases it is better to avoid getting into > > + * unnecessary frequency updates. > > + */ > > + if (abs(policy->cur - new_freq) < HZ_PER_MHZ) > > + return policy->cur; > > + > > cpufreq_out_of_sync(policy, new_freq); > > if (update) > > schedule_work(&policy->update); Applied as 5.19 material, thanks!
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 0d58b0f8f3af..233e8af48848 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -28,6 +28,7 @@ #include <linux/suspend.h> #include <linux/syscore_ops.h> #include <linux/tick.h> +#include <linux/units.h> #include <trace/events/power.h> static LIST_HEAD(cpufreq_policy_list); @@ -1708,6 +1709,16 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b return new_freq; if (policy->cur != new_freq) { + /* + * For some platforms, the frequency returned by hardware may be + * slightly different from what is provided in the frequency + * table, for example hardware may return 499 MHz instead of 500 + * MHz. In such cases it is better to avoid getting into + * unnecessary frequency updates. + */ + if (abs(policy->cur - new_freq) < HZ_PER_MHZ) + return policy->cur; + cpufreq_out_of_sync(policy, new_freq); if (update) schedule_work(&policy->update);
For some platforms, the frequency returned by hardware may be slightly different from what is provided in the frequency table. For example, hardware may return 499 MHz instead of 500 MHz. In such cases it is better to avoid getting into unnecessary frequency updates, as we may end up switching policy->cur between the two and sending unnecessary pre/post update notifications, etc. This patch has chosen allows the hardware frequency and table frequency to deviate by 1 MHz for now, we may want to increase it a bit later on if someone still complains. Reported-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 11 +++++++++++ 1 file changed, 11 insertions(+)