diff mbox series

cpufreq: Avoid unnecessary frequency updates due to mismatch

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

Commit Message

Viresh Kumar May 4, 2022, 8:21 a.m. UTC
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(+)

Comments

Vincent Guittot May 5, 2022, 7:28 a.m. UTC | #1
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
>
Viresh Kumar May 5, 2022, 7:44 a.m. UTC | #2
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.
Vincent Guittot May 5, 2022, 8:21 a.m. UTC | #3
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
Viresh Kumar May 5, 2022, 8:28 a.m. UTC | #4
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.
Vincent Guittot May 5, 2022, 9:40 a.m. UTC | #5
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
Viresh Kumar May 5, 2022, 9:45 a.m. UTC | #6
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.
Matthias Brugger May 5, 2022, 1:31 p.m. UTC | #7
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);
Rafael J. Wysocki May 6, 2022, 6:57 p.m. UTC | #8
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 mbox series

Patch

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);