Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

[Zhang Rui <rui.zhang@intel.com>]

On Sat, 2021-03-13 at 07:16 -0800, Doug Smythies wrote:
> On Fri, Mar 12, 2021 at 2:16 PM Len Brown <lenb@kernel.org> wrote:
> > 
> > Doug,
> > The offset works for control.
> > 
> > However, it is erroneous to use it for reporting of the actual
> > temperature, like I did in turbostat.
> 
> Agreed.
> I have been running with a correction for that for a while,
> and as discussed on Rui's thread.
> But this bit mask correction patch is still needed isn't it?
> for this:
> cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (90 C) (100 default -
> 10 offset)
> which should be this:
> cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (74 C) (100 default -
> 26 offset)
> 
> But yes, I do now see the field size is only 4 bits for some parts.

As this is CPU specific, and we don't know which is which for all the
CPUs, so it seems that we can have a white list for the ones that we
care and have been verified.

For the others, by default, we only show the raw value and default TCC
activation temperature, like

cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (100 default )

And this white list can be updated together with the one in the kernel
tcc_offset_cooling driver.

what do you think?

thanks,
rui



> 
> ... Doug
> 
> > Thus, I'm going to revert the patch that added it's use in
> > turbostat
> > for the Temperature column.
> > 
> > thanks,
> > -Len
> > 
> > On Fri, Mar 12, 2021 at 1:26 AM Doug Smythies <dsmythies@telus.net>
> > wrote:
> > > 
> > > Hi Len,
> > > 
> > > 
> > > thank you for your reply.
> > > 
> > > On Thu, Mar 11, 2021 at 3:19 PM Len Brown <lenb@kernel.org>
> > > wrote:
> > > > 
> > > > Thanks for the close read, Doug.
> > > > 
> > > > This field size actually varies from system to system,
> > > > but the reality is that the offset is never that big, and so
> > > > the
> > > > smaller mask is sufficient.
> > > 
> > > Disagree.
> > > 
> > > I want to use an offset of 26.
> > > 
> > > > Finally, this may all be moot, because there is discussion that
> > > > using
> > > > the offset this way is simply erroneous.
> > > 
> > > Disagree.
> > > It works great.
> > > As far as I know/recall I was the only person that responded to
> > > Rui's thread
> > > "thermal/intel: introduce tcc cooling driver" [1]
> > > And, I spent quite a bit of time doing so.
> > > However, I agree the response seems different between the two
> > > systems
> > > under test, Rui's and mine.
> > > 
> > > [1] https://marc.info/?l=linux-pm&m=161070345329806&w=2
> > > 
> > > >  stay tuned.
> > > 
> > > O.K.
> > > 
> > > ... Doug
> > > > 
> > > > -Len
> > > > 
> > > > 
> > > > On Sat, Jan 16, 2021 at 12:07 PM Doug Smythies <
> > > > doug.smythies@gmail.com> wrote:
> > > > > 
> > > > > The TCC offset mask is incorrect, resulting in
> > > > > incorrect target temperature calculations, if
> > > > > the offset is big enough to exceed the mask size.
> > > > > 
> > > > > Signed-off-by: Doug Smythies <dsmythies@telus.net>
> > > > > ---
> > > > >  tools/power/x86/turbostat/turbostat.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/power/x86/turbostat/turbostat.c
> > > > > b/tools/power/x86/turbostat/turbostat.c
> > > > > index 389ea5209a83..d7acdd4d16c4 100644
> > > > > --- a/tools/power/x86/turbostat/turbostat.c
> > > > > +++ b/tools/power/x86/turbostat/turbostat.c
> > > > > @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp()
> > > > > 
> > > > >         target_c = (msr >> 16) & 0xFF;
> > > > > 
> > > > > -       offset_c = (msr >> 24) & 0xF;
> > > > > +       offset_c = (msr >> 24) & 0x3F;
> > > > > 
> > > > >         tcc = target_c - offset_c;
> > > > > 
> > > > > --
> > > > > 2.25.1
> > > > > 
> > > > 
> > > > 
> > > > --
> > > > Len Brown, Intel Open Source Technology Center
> > 
> > 
> > 
> > --
> > Len Brown, Intel Open Source Technology Center