Skip to contentSkip to conversation

Display all P-states in HTML-report

Powertop NAK Igor Zhbanov 3 participants 5 messages Jul 11, 2012 – Jul 17, 2012

lore
Share & tools
lore

Igor Zhbanov

author
#0 lore raw

Hello!

On Arm processors there are more than 10 P-states. On my device there are 12 p-states with different frequency. But PowerTOP displays only 10. And it doesn't display states (11 and 12) where the phone spends most of the time.

I suppose that was because of console mode of PowerTOP, when there are limited number of lines on the console. But in the case of HTML-report this should not be a problem.

Here is the patch that counts the number of found P-states and displays all of them.

--8<--Cut here---------------------------------------------------------------- diff -purN -X dontdiff-powertop powertop-intel/config.h powertop/config.h

src/cpu/cpu.cpp +6 -1
diff -purN -X dontdiff-powertop powertop-intel/src/cpu/cpu.cpp powertop/src/cpu/cpu.cpp
@@ -618,6 +618,11 @@ void report_display_cpu_pstates(void)
  	unsigned int package, core, cpu;
  	int line;
  	class abstract_cpu *_package, * _core, * _cpu;
+	unsigned int i, pstates_num;
+
+	for (i = 0, pstates_num = 0; i<  all_cpus.size(); i++)
+		if (all_cpus[i]&&  all_cpus[i]->pstates.size()>  pstates_num)
+			pstates_num = all_cpus[i]->pstates.size();
 
  	if ((!reportout.csv_report)&&(!reportout.http_report))
  		return;

@@ -644,7 +649,7 @@ void report_display_cpu_pstates(void)
  			if (!_core)
  				continue;
 
-			for (line = LEVEL_HEADER; line<  10; line++) {
+			for (line = LEVEL_HEADER; line<  (int)pstates_num; line++) {
  				int first = 1;
 
  				if (!_package->has_pstate_level(line))

--8<--------------------------------------------------------------------------

The same issue may affect the function report_display_cpu_cstates() but I'm not sure that CPUs with more than 10 C-states exists.

The same code is in the function impl_w_display_cpu_states(), but that function is used in console mode only.

Thank you.

Signature · 9 lines
Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

diff -purN -X dontdiff-powertop powertop-intel/config.h powertop/config.h

src/cpu/cpu.cpp +6 -1
diff -purN -X dontdiff-powertop powertop-intel/src/cpu/cpu.cpp powertop/src/cpu/cpu.cpp
@@ -618,6 +618,11 @@ void report_display_cpu_pstates(void)
 	unsigned int package, core, cpu;
 	int line;
 	class abstract_cpu *_package, * _core, * _cpu;
+	unsigned int i, pstates_num;
+
+	for (i = 0, pstates_num = 0; i < all_cpus.size(); i++)
+		if (all_cpus[i] && all_cpus[i]->pstates.size() > pstates_num)
+			pstates_num = all_cpus[i]->pstates.size();
 
 	if ((!reportout.csv_report)&&(!reportout.http_report))
 		return;

@@ -644,7 +649,7 @@ void report_display_cpu_pstates(void)
 			if (!_core)
 				continue;
 
-			for (line = LEVEL_HEADER; line < 10; line++) {
+			for (line = LEVEL_HEADER; line < (int)pstates_num; line++) {
 				int first = 1;
 
 				if (!_package->has_pstate_level(line))
author
#1 lore raw

This is the formatted version of the patch:

From 66d0f194250d94afcf83eb7712af6f50c6ad8ec4 Mon Sep 17 00:00:00 2001
From: Igor Zhbanov<i.zhbanov(a)samsung.com>
Date: Wed, 11 Jul 2012 12:35:31 +0400
Subject: [PATCH] Display all P-states in HTML-report

On Arm processors there are more than 10 P-states. On my device there are 12 p-states with different frequency. But PowerTOP displays only 10. And it doesn't display states (11 and 12) where the phone spends most of the time.

I suppose that was because of console mode of PowerTOP, when there are limited number of lines on the console. But in the case of HTML-report this should not be a problem.

This patch that counts the number of found P-states and displays all of them.

The same issue may affect the function report_display_cpu_cstates() but I'm not sure that CPUs with more than 10 C-states exists.

The same code is in the function impl_w_display_cpu_states(), but that function is used in console mode only where lines limit is reasonable.

---
  src/cpu/cpu.cpp |    7 ++++++-
  1 files changed, 6 insertions(+), 1 deletions(-)
src/cpu/cpu.cpp +6 -1
index 4096835..c3dd6a5 100644
@@ -588,6 +588,11 @@ void report_display_cpu_pstates(void)
  	unsigned int package, core, cpu;
  	int line;
  	class abstract_cpu *_package, * _core, * _cpu;
+	unsigned int i, pstates_num;
+
+	for (i = 0, pstates_num = 0; i<  all_cpus.size(); i++)
+		if (all_cpus[i]&&  all_cpus[i]->pstates.size()>  pstates_num)
+			pstates_num = all_cpus[i]->pstates.size();
 
  	if ((!reportout.csv_report)&&(!reportout.http_report))
  		return;

@@ -614,7 +619,7 @@ void report_display_cpu_pstates(void)
  			if (!_core)
  				continue;
 
-			for (line = LEVEL_HEADER; line<  10; line++) {
+			for (line = LEVEL_HEADER; line<  (int)pstates_num; line++) {
  				int first = 1;
 
  				if (!_package->has_pstate_level(line))
Signature · 1 line
1.7.5.4
Quoted 56 lines · Igor Zhbanov

Hello!

On Arm processors there are more than 10 P-states. On my device there are 12
p-states with different frequency. But PowerTOP displays only 10. And it doesn't display states (11 and 12) where the phone spends most of the time.

I suppose that was because of console mode of PowerTOP, when there are limited
number of lines on the console. But in the case of HTML-report this should
not be a problem.

Here is the patch that counts the number of found P-states and displays all
of them.

--8<--Cut here---------------------------------------------------------------- diff -purN -X dontdiff-powertop powertop-intel/config.h powertop/config.h diff -purN -X dontdiff-powertop powertop-intel/src/cpu/cpu.cpp powertop/src/cpu/cpu.cpp

src/cpu/cpu.cpp 2012-07-05 18:43:19.704809327 +0400 renamed from src/cpu/cpu.cpp 2012-07-05 13:06:16.000000000 +0400 +6 -2
@@ -618,6 +618,11 @@ void report_display_cpu_pstates(void)
     unsigned int package, core, cpu;
     int line;
     class abstract_cpu *_package, * _core, * _cpu;
+    unsigned int i, pstates_num;
+
+    for (i = 0, pstates_num = 0; i<  all_cpus.size(); i++)
+        if (all_cpus[i]&&  all_cpus[i]->pstates.size()>  pstates_num)
+            pstates_num = all_cpus[i]->pstates.size();
 
     if ((!reportout.csv_report)&&(!reportout.http_report))
         return;

@@ -644,7 +649,7 @@ void report_display_cpu_pstates(void)
             if (!_core)
                 continue;
 
-            for (line = LEVEL_HEADER; line<  10; line++) {
+            for (line = LEVEL_HEADER; line<  (int)pstates_num; line++) {
                 int first = 1;
 
                 if (!_package->has_pstate_level(line))
--8<--------------------------------------------------------------------------

The same issue may affect the function report_display_cpu_cstates() but I'm not sure that CPUs with more than 10 C-states exists.

The same code is in the function impl_w_display_cpu_states(), but that function is used in console mode only.

Thank you.

Signature · 9 lines
Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation
NAK
#2 lore raw

src/cpu/cpu.cpp void report_display_cpu_pstates(void) · line 595

Quoted 7 lines · Igor Zhbanov
src/cpu/cpu.cpp +5 -0
+	unsigned int i, pstates_num;
+
+	for (i = 0, pstates_num = 0; i<  all_cpus.size(); i++)
+		if (all_cpus[i]&&  all_cpus[i]->pstates.size()>  pstates_num)
+			pstates_num = all_cpus[i]->pstates.size();

nack

That will not work correctly, I'm afraid.

pstates_num initialized with stack garbage and is free to have value (e.g. 0x7fffffff) which will never satisfy "all_cpus[i]->pstates.size()> pstates_num" condition. In that case our loop will spin "0x7fffffff - actual_pstates_number" useless cycles.

BTW, could you please put white space before operators and remove extra one after?

E.g.

    "all_cpus[i] && all_cpus[i]->pstates.size() > pstates_num"

vs.

    "all_cpus[i]&&  all_cpus[i]->pstates.size()>  pstates_num"
               ^^^^^^                          ^^^^^

	-ss
#3 lore raw
In reply to Igor Zhbanov

src/cpu/cpu.cpp void report_display_cpu_pstates(void) · line 595

Quoted 7 lines · Igor Zhbanov
src/cpu/cpu.cpp +5 -0
+	unsigned int i, pstates_num;
+
+	for (i = 0, pstates_num = 0; i<  all_cpus.size(); i++)
+		if (all_cpus[i]&&  all_cpus[i]->pstates.size()>  pstates_num)
+			pstates_num = all_cpus[i]->pstates.size();

OH, SILLY ME! My bad, sorry!
You initialize pstates_num within for loop (which is confusing).

Sorry for noise.

	-ss
#4 lore raw
In reply to Igor Zhbanov
Quoted 55 lines · Igor Zhbanov

Hello!

On Arm processors there are more than 10 P-states. On my device there are 12
p-states with different frequency. But PowerTOP displays only 10. And it doesn't display states (11 and 12) where the phone spends most of the time.

I suppose that was because of console mode of PowerTOP, when there are limited
number of lines on the console. But in the case of HTML-report this should
not be a problem.

Here is the patch that counts the number of found P-states and displays all
of them.

--8<--Cut here---------------------------------------------------------------- diff -purN -X dontdiff-powertop powertop-intel/config.h powertop/config.h diff -purN -X dontdiff-powertop powertop-intel/src/cpu/cpu.cpp powertop/src/cpu/cpu.cpp

src/cpu/cpu.cpp 2012-07-05 18:43:19.704809327 +0400 renamed from src/cpu/cpu.cpp 2012-07-05 13:06:16.000000000 +0400 +6 -2
@@ -618,6 +618,11 @@ void report_display_cpu_pstates(void)
     unsigned int package, core, cpu;
     int line;
     class abstract_cpu *_package, * _core, * _cpu;
+    unsigned int i, pstates_num;
+
+    for (i = 0, pstates_num = 0; i<  all_cpus.size(); i++)
+        if (all_cpus[i]&& all_cpus[i]->pstates.size()>  pstates_num)
+            pstates_num = all_cpus[i]->pstates.size();
 
     if ((!reportout.csv_report)&&(!reportout.http_report))
         return;

@@ -644,7 +649,7 @@ void report_display_cpu_pstates(void)
             if (!_core)
                 continue;
 
-            for (line = LEVEL_HEADER; line<  10; line++) {
+            for (line = LEVEL_HEADER; line<  (int)pstates_num; line++) {
                 int first = 1;
 
                 if (!_package->has_pstate_level(line))
--8<--------------------------------------------------------------------------

The same issue may affect the function report_display_cpu_cstates() but I'm not sure that CPUs with more than 10 C-states exists.

The same code is in the function impl_w_display_cpu_states(), but that function is used in console mode only.

Quoted 8 lines

Thank you.

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop

Your patch has been merged.
Thank you,