Re: [PATCH 1/2] x86: intel_pstate: Fix wrong definition of Disable Energy Efficiency Optimization bit

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

On Mon, 2019-04-15 at 21:27 +0300, Liran Alon wrote:
> > On 15 Apr 2019, at 21:22, Srinivas Pandruvada <
> > srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Mon, 2019-04-15 at 21:13 +0300, Liran Alon wrote:
> > > > On 15 Apr 2019, at 21:03, Srinivas Pandruvada <
> > > > srinivas.pandruvada@linux.intel.com> wrote:
> > > > 
> > > > On Mon, 2019-04-15 at 11:32 +0300, Liran Alon wrote:
> > > > > > On 15 Apr 2019, at 5:00, Srinivas Pandruvada <
> > > > > > srinivas.pandruvada@linux.intel.com> wrote:
> > > > > > 
> > > > > > On Sun, 2019-04-14 at 23:48 +0300, Liran Alon wrote:
> > > > > > > Bit definition can be found in Intel SDM Section 2.16
> > > > > > > MSRS IN
> > > > > > > THE
> > > > > > > 6TH
> > > > > > > GENERATION, 7TH GENERATION AND 8TH GENERATION
> > > > > > > INTEL® CORE™ PROCESSORS, INTEL® XEON® PROCESSOR SCALABLE
> > > > > > > FAMILY, AND FUTURE INTEL® CORE™ PROCESSORS.
> > > > > > > 
> > > > > > > Definition of all Skylake MSR_POWER_CTL bits can also be
> > > > > > > found at
> > > > > > > EDK2
> > > > > > > source at UefiCpuPkg/Include/Register/Msr/SkylakeMsr.h
> > > > > > > union
> > > > > > > MSR_SKYLAKE_POWER_CTL_REGISTER.
> > > > > > > 
> > > > > > > Fixes: 6e978b22efa1 ("cpufreq: intel_pstate: Disable
> > > > > > > energy
> > > > > > > efficiency optimization")
> > > > > > 
> > > > > > What are you trying to address? This bit 19 has a special
> > > > > > meaning
> > > > > > when
> > > > > > system is in HWP mode. So this is correct.
> > > > > > 
> > > > > > Bit 20 has a different meaning depending on legacy or in
> > > > > > HWP
> > > > > > mode.
> > > > > > 
> > > > > > Thanks,
> > > > > > Srinivas
> > > > > > 
> > > > > 
> > > > > Maybe I’m misinterpreting Intel SDM, but it seems to me that
> > > > > bit
> > > > > 19
> > > > > in MSR_POWER_CTL is always "Disable Race to Halt Optimization
> > > > > (R/W)”
> > > > > while bit 20 is the "Disable Energy Efficiency Optimization
> > > > > (R/W)”.
> > > > > 
> > > > > I didn’t find a place in Intel SDM where it is discussed that
> > > > > bit
> > > > > 19
> > > > > have a special meaning when system is in HWP mode.
> > > > > Can you point me to relevant place in Intel SDM?
> > > > > 
> > > > 
> > > > SDM doesn't describe the algorithms. This is a feature of Intel
> > > > Speed
> > > > Shift Technology aka HWP. Both bits target disabling some
> > > > energy
> > > > efficiency features of the processor. I wish there are some
> > > > better
> > > > names of these bits. Ideas is to pick the best for a platform
> > > > based
> > > > on
> > > > the performance needs. Here based on the experiments, setting
> > > > bit
> > > > 19
> > > > gave the required performance on Kaby Lake desktops.
> > > > 
> > > > So unless you found some performance/power issue with setting
> > > > of
> > > > bit 19
> > > > vs bit 20, on Kaby Lake based platforms, we shouldn't change
> > > > (may
> > > > be
> > > > rename as per SDM definition).
> > > > 
> > > > Thanks,
> > > > Srinivas
> > > 
> > > I haven’t found any performance/power issue.
> > > 
> > > The name of the bit, the function names, prints and comments just
> > > seems to refer to bit 20 and not bit 19.
> > > If the code intention is to manipulate "Disable Race to Halt
> > > Optimization” bit instead of "Disable Energy Efficiency
> > > Optimization”
> > > bit,
> > > code should be renamed appropriately.
> > > Is this code intention?
> > 
> > The code change was done before SDM documented this feature, so it
> > is
> > not matching.
> > 
> > Thanks,
> > Srinivas
> > 
> 
> So should I submit a v2 patch that renames these appropriately by
> updated SDM?
Sure.

Thanks,
Srinivas

> 
> -Liran
> 
>