Re: [PATCH] cpufreq: intel_pstate: Use the latest guaranteed freq during verify

[Rafael J. Wysocki]

On Thu, Dec 17, 2020 at 6:29 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Dec 17, 2020 at 6:09 PM Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 2020-12-17 at 16:24 +0100, Rafael J. Wysocki wrote:
> > > On Thu, Dec 17, 2020 at 4:21 PM Srinivas Pandruvada
> > > <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:

[cut]

> > > > > Well, would something like the patch below work?
> > > > >
> > > > > ---
> > > > >  drivers/cpufreq/intel_pstate.c |   16 +++++++++++++---
> > > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > > >
> > > > > Index: linux-pm/drivers/cpufreq/intel_pstate.c
> > > > > =================================================================
> > > > > ==
> > > > > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> > > > > +++ linux-pm/drivers/cpufreq/intel_pstate.c
> > > > > @@ -2207,9 +2207,9 @@ static void intel_pstate_update_perf_lim
> > > > >                                             unsigned int
> > > > > policy_min,
> > > > >                                             unsigned int
> > > > > policy_max)
> > > > >  {
> > > > > -       int max_freq = intel_pstate_get_max_freq(cpu);
> > > > >         int32_t max_policy_perf, min_policy_perf;
> > > > >         int max_state, turbo_max;
> > > > > +       int max_freq;
> > > > >
> > > > >         /*
> > > > >          * HWP needs some special consideration, because on BDX
> > > > > the
> > > > > @@ -2223,6 +2223,7 @@ static void intel_pstate_update_perf_lim
> > > > >                         cpu->pstate.max_pstate : cpu-
> > > > > > pstate.turbo_pstate;
> > > > >                 turbo_max = cpu->pstate.turbo_pstate;
> > > > >         }
> > > > > +       max_freq = max_state * cpu->pstate.scaling;
> > > > >
> > > > >         max_policy_perf = max_state * policy_max / max_freq;
> > > > >         if (policy_max == policy_min) {
> > > > > @@ -2325,9 +2326,18 @@ static void intel_pstate_adjust_policy_m
> > > > >  static void intel_pstate_verify_cpu_policy(struct cpudata *cpu,
> > > > >                                            struct
> > > > > cpufreq_policy_data
> > > > > *policy)
> > > > >  {
> > > > > +       int max_freq;
> > > > > +
> > > > >         update_turbo_state();
> > > > > -       cpufreq_verify_within_limits(policy, policy-
> > > > > > cpuinfo.min_freq,
> > > > > -
> > > > > intel_pstate_get_max_freq(cpu));
> > > > > +       if (hwp_active) {
> > > > > +               int max_state, turbo_max;
> > > > > +
> > > > > +               intel_pstate_get_hwp_max(cpu->cpu, &turbo_max,
> > > > > &max_state);
> > > > > +               max_freq = max_state * cpu->pstate.scaling;
> > > > > +       } else {
> > > > > +               max_freq = intel_pstate_get_max_freq(cpu);
> > > > > +       }
> > > > > +       cpufreq_verify_within_limits(policy, policy-
> > > > > > cpuinfo.min_freq, max_freq);
> > > > >
> > > > >         intel_pstate_adjust_policy_max(cpu, policy);
> > > > >  }
> > > > >
> > > > Should work.
> > > >  I will test this patch and let you know once I get the system.
> > >
> > > Please do, thank you!
> >
> > This works. Please check if you can submit a change for this.
>
> I can do that, but I'm going to borrow some changelog pieces from the
> $subject patch.
>
> Will submit shortly.

Well, this only fixes the setting of the policy max limit AFAICS, but
pstate.max_pstate is used in computations in some places, so it looks
like it needs to be updated every time HWP_CAP is read, or do I
confuse things?

And if pstate.max_pstate needs to be updated then, shouldn't
pstate.turbo_pstate be updated then too (because it may change too as
a result of ISS updates)?