Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <7417968.Ghue05m4RV@kreacher> <2233690.N3OVLkotou@kreacher> <20201106100712.u336gbtblaxr2cit@vireshk-i7>
In-Reply-To: <20201106100712.u336gbtblaxr2cit@vireshk-i7>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 6 Nov 2020 18:02:45 +0100
Message-ID: <CAJZ5v0gT07K-oPa0=f8+Fq6tevqZJ8iWYjtf9YDNUJw1GJEBBA@mail.gmail.com>
Subject: Re: [PATCH 1/2] cpufreq: Introduce target min and max frequency hints
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Zhang Rui <rui.zhang@intel.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Nov 6, 2020 at 11:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 05-11-20, 19:23, Rafael J. Wysocki wrote:
> > Index: linux-pm/include/linux/cpufreq.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpufreq.h
> > +++ linux-pm/include/linux/cpufreq.h
> > @@ -63,6 +63,8 @@ struct cpufreq_policy {
> >
> >       unsigned int            min;    /* in kHz */
> >       unsigned int            max;    /* in kHz */
> > +     unsigned int            target_min; /* in kHz */
> > +     unsigned int            target_max; /* in kHz */
> >       unsigned int            cur;    /* in kHz, only needed if cpufreq
> >                                        * governors are used */
> >       unsigned int            suspend_freq; /* freq to set during suspend */
>
> Rafael, honestly speaking I didn't like this patch very much.

So what's the concern, specifically?

> We need to fix a very specific problem with the intel-pstate driver when it is
> used with powersave/performance governor to make sure the hard limits
> are enforced. And this is something which no one else may face as
> well.

Well, I predict that the CPPC driver will face this problem too at one point.

As well as any other driver which doesn't select OPPs directly for
that matter, at least to some extent (note that intel_pstate in the
"passive" mode without HWP has it too, but since there is no way to
enforce the target max in that case, it is not relevant).

> What about doing something like this instead in the intel_pstate
> driver only to get this fixed ?
>
>         if (!strcmp(policy->governor->name, "powersave") ||
>             !strcmp(policy->governor->name, "performance"))
>                 hard-limit-to-be-enforced;
>
> This would be a much simpler and contained approach IMHO.

I obviously prefer to do it the way I did in this series, because it
is more general and it is based on the governor telling the driver
what is needed instead of the driver trying to figure out what the
governor is and guessing what may be needed because of that.

But if you have a very specific technical concern regarding my
approach, I can do it the other way too.

Cheers!