Re: [RFT][PATCH 0/2] cpufreq: intel_pstate: Handle _PPC updates on global turbo disable/enable

[Srinivas Pandruvada]

On Sun, 2019-03-03 at 22:51 +0100, Rafael J. Wysocki wrote:
> On Sun, Mar 3, 2019 at 10:20 PM Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > 
> > On Sun, 2019-03-03 at 18:03 +0100, Rafael J. Wysocki wrote:
> > > On Fri, Mar 1, 2019 at 6:39 PM Srinivas Pandruvada
> > > <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > > > 
> > > > On Fri, 2019-03-01 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > > Hi All,
> > > > > 
> > > > > This is how I would fix the issue reported in BZ 200759 (see
> > > > > thisdev
> > > > > patch series
> > > > > from Yu too: 
> > > > > https://marc.info/?l=linux-pm&m=155137672924029&w=2)
> > > > > .
> > > > > 
> > > > > Patch [1/2] causes intel_pstate to update all policies if it
> > > > > gets
> > > > > a
> > > > > _PPC change
> > > > > notification and sees a global turbo disable/enable change.
> > > > > 
> > > > > Patch [2/2] makes it update cpuinfo.max_freq for all policies
> > > > > in
> > > > > those cases.
> > > > > 
> > > > > The patches here have not been tested yet, so testing would
> > > > > be
> > > > > much
> > > > > appreciated.
> > > > > 
> > > > > Of course, comments are welcome too!
> > > > 
> > > > This is the only platform, someone reported such issue.
> > > 
> > > I don't think this matters.
> > > 
> > > First, it doesn't mean that no other problems have this problem.
> > > 
> > > Second, the current handling of
> > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> > > changes in intel_pstate is not sufficient if the changes is from
> > > set
> > > to unset anyway.
> > 
> > Any platform with HWP, you can't even notify of this change. So any
> > platform beyond SKL is not useful.
> 
> Do you mean that HWP platforms don't supply _PPS (as a rule) and so
> they don't send _PPC notifications?  Is there anything they do
> instead
> of it?
There are other methods like PL1 budget limit for such cases. FW can
just change the config TDP level.

> 
> That's fair enough, but the point is that the driver doesn'dev_t do
> the
> right thing even if the platform does send a _PPC notification.
_PPC notification is to indicate levels in _PSS not to disable/enable
turbo via IA32_MISC_*. The platform could have just set _PPC to 1 or to
TAR-1. Here _PPC is sent for somthing more than just changing _PSS max
level. Do we have bug in if _PPC just changes performance level?

> 
> > Fixing is always good :-)
> 
> Well, I can only agree with that ...