* [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask @ 2021-01-16 17:07 Doug Smythies 2021-03-11 23:18 ` Len Brown 0 siblings, 1 reply; 6+ messages in thread From: Doug Smythies @ 2021-01-16 17:07 UTC (permalink / raw) To: lenb; +Cc: dsmythies, linux-kernel, linux-pm 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask 2021-01-16 17:07 [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask Doug Smythies @ 2021-03-11 23:18 ` Len Brown 2021-03-12 6:26 ` Doug Smythies 0 siblings, 1 reply; 6+ messages in thread From: Len Brown @ 2021-03-11 23:18 UTC (permalink / raw) To: Doug Smythies; +Cc: Doug Smythies, Linux Kernel Mailing List, Linux PM list 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. Finally, this may all be moot, because there is discussion that using the offset this way is simply erroneous. stay tuned. -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask 2021-03-11 23:18 ` Len Brown @ 2021-03-12 6:26 ` Doug Smythies 2021-03-12 22:15 ` Len Brown 0 siblings, 1 reply; 6+ messages in thread From: Doug Smythies @ 2021-03-12 6:26 UTC (permalink / raw) To: Len Brown; +Cc: Linux Kernel Mailing List, Linux PM list, dsmythies 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask 2021-03-12 6:26 ` Doug Smythies @ 2021-03-12 22:15 ` Len Brown 2021-03-13 15:16 ` Doug Smythies 0 siblings, 1 reply; 6+ messages in thread From: Len Brown @ 2021-03-12 22:15 UTC (permalink / raw) To: Doug Smythies; +Cc: Linux Kernel Mailing List, Linux PM list Doug, The offset works for control. However, it is erroneous to use it for reporting of the actual temperature, like I did in turbostat. 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask 2021-03-12 22:15 ` Len Brown @ 2021-03-13 15:16 ` Doug Smythies 2021-04-11 14:09 ` Zhang Rui 0 siblings, 1 reply; 6+ messages in thread From: Doug Smythies @ 2021-03-13 15:16 UTC (permalink / raw) To: Len Brown; +Cc: Linux Kernel Mailing List, Linux PM list, dsmythies 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. ... 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask 2021-03-13 15:16 ` Doug Smythies @ 2021-04-11 14:09 ` Zhang Rui 0 siblings, 0 replies; 6+ messages in thread From: Zhang Rui @ 2021-04-11 14:09 UTC (permalink / raw) To: Doug Smythies, Len Brown; +Cc: Linux Kernel Mailing List, Linux PM list 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-11 14:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-16 17:07 [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask Doug Smythies 2021-03-11 23:18 ` Len Brown 2021-03-12 6:26 ` Doug Smythies 2021-03-12 22:15 ` Len Brown 2021-03-13 15:16 ` Doug Smythies 2021-04-11 14:09 ` Zhang Rui
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.