Re: [PATCH powertop] thermal: add window for thermal related infromation

On 12 March 2013 17:15, Sanjay Singh Rawat sanjay.rawat@linaro.org wrote:

add a window to display the stats of thermal, cooling zones and also information given by the hwmon sensor.

Nice window displaying thermal/hwmon sysfs information. Good enough to start with. Any plans to process sysfs information and display more useful stats like done in cpufreq and cpuidle stats? for instance how long a cooling device was active and temperature drop as a result.

Signed-off-by: Sanjay Singh Rawat sanjay.rawat@linaro.com

src/Makefile.am | 2 +- src/display.cpp | 1 + src/main.cpp | 3 +- src/measurement/thermal.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++ src/measurement/thermal.h | 1 + 5 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 src/measurement/thermal.cpp create mode 100644 src/measurement/thermal.h

diff --git a/src/Makefile.am b/src/Makefile.am index f60426a..335fb6c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -34,7 +34,7 @@ powertop_SOURCES = parameters/persistent.cpp parameters/learn.cpp parameters/par report/report-formatter-base.cpp report/report-formatter-base.h \ report/report-formatter-csv.cpp report/report-formatter-csv.h \ report/report-formatter-html.cpp report/report-formatter-html.h \

•           main.cpp css.h powertop.css cpu/intel_gpu.cpp
  

•           main.cpp css.h powertop.css cpu/intel_gpu.cpp measurement/thermal.h measurement/thermal.cpp
  
  
  

powertop_CXXFLAGS = -fno-omit-frame-pointer -fstack-protector -Wall -Wshadow -Wformat $(NCURSES_CFLAGS) $(PCIUTILS_CFLAGS) $(LIBNL_CFLAGS) $(GLIB2_CFLAGS) diff --git a/src/display.cpp b/src/display.cpp index c76ba27..adcf80d 100644 --- a/src/display.cpp +++ b/src/display.cpp @@ -71,6 +71,7 @@ void init_display(void) create_tab("Idle stats", _("Idle stats")); create_tab("Frequency stats", _("Frequency stats")); create_tab("Device stats", _("Device stats"));

•   create_tab("Thermal stats", _("Thermal stats"));
  
    display = 1;
  

} diff --git a/src/main.cpp b/src/main.cpp index e6036ae..e649919 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -51,7 +51,7 @@ #include "parameters/parameters.h" #include "calibrate/calibrate.h"

+#include "measurement/thermal.h" #include "tuning/tuning.h"

#include "display.h" @@ -214,6 +214,7 @@ void one_measurement(int seconds, char *workload) report_display_cpu_pstates(); report_process_update_display();

•   thermal_update_display();
    tuning_update_display();
  
    end_process_data();
  

diff --git a/src/measurement/thermal.cpp b/src/measurement/thermal.cpp new file mode 100644 index 0000000..c2c4c11 --- /dev/null +++ b/src/measurement/thermal.cpp @@ -0,0 +1,148 @@ +/*

• • This is part of PowerTOP
• • This program file is free software; you can redistribute it and/or modify it
• • under the terms of the GNU General Public License as published by the
• • Free Software Foundation; version 2 of the License.
• • This program is distributed in the hope that it will be useful, but WITHOUT
• • ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
• • FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
• • for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program in a file named COPYING; if not, write to the
• • Free Software Foundation, Inc,
• • 51 Franklin Street, Fifth Floor,
• • Boston, MA 02110-1301 USA
• • or just google for it.
• • getopt code is taken from "The GNU C Library" reference manual,
• • section 24.2 "Parsing program options using getopt"
• • http://www.gnu.org/s/libc/manual/html_node/Getopt-Long-Option-Example.html
• • Manual published under the terms of the Free Documentation License.
• */

+#include <stdlib.h> +#include <ncurses.h> +#include "../display.h" +#include "../lib.h"

Patch fails to compile for me. Need to include unistd.h for R_OK?

+enum attr_length {

•   SHORTT,
  

•   LONGT
  

+};

+static int get_thermal_attr(WINDOW *win, char *zone, const char *attr, int wide) +{

•    char buf[512];
  

•    int val;
  

•    string line;
  

•   if(!wide)
  

What happens when more enumerations are added?

•           wprintw(win,"%17s : ",attr);
  

•   else
  

•           wprintw(win,"%s : ",attr);
  

•    for(val=0 ; val<255 ; val++) {
  

Instead of using magic number here, why not dynamically detect available thermal zones and cooling devices?

•        sprintf(buf,"%s%d/%s",zone,val,attr);
  

•                   if (access(buf, R_OK) !=0) {
  

Please follow powertop existing code for coding standards.

•                           wprintw(win,"  *\n");
  

•                           return -1;
  

•           }
  

•            line = read_sysfs_string("%s", buf);
  

•           wprintw(win,"  %s  -",line.c_str());
  

•    }
  

•    wprintw(win,"\n");
  

•   return 0;
  

+}

+static int get_thermal_zone_info(WINDOW *win) +{

•   char linebuf[1024];
  

•   string line;
  

•   sprintf(linebuf,"/sys/class/thermal/thermal_zone");
  

•   wprintw(win,"\n[Zone : Thermal]----------------------------------------------\n");
  

•   get_thermal_attr(win, linebuf, "type",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp",SHORTT);
  

•   get_thermal_attr(win, linebuf, "mode",SHORTT);
  

•   get_thermal_attr(win, linebuf, "policy",SHORTT);
  

•   get_thermal_attr(win, linebuf, "passive",SHORTT);
  

•   //todo: currently assuming only 3 trip points
  

Again, why not dynamically detect available trip points to avoid displaying unsupported parameters?

•   get_thermal_attr(win, linebuf, "trip_point_0_temp",LONGT);
  

•   get_thermal_attr(win, linebuf, "trip_point_0_type",LONGT);
  

•   get_thermal_attr(win, linebuf, "trip_point_1_temp",LONGT);
  

•   get_thermal_attr(win, linebuf, "trip_point_1_type",LONGT);
  

•   get_thermal_attr(win, linebuf, "trip_point_2_temp",LONGT);
  

•   get_thermal_attr(win, linebuf, "trip_point_2_type",LONGT);
  

•   return 0;
  

+}

+static int get_cooling_zone_info(WINDOW *win) +{

•    char linebuf[1024];
  

•    string line;
  

•   sprintf(linebuf,"/sys/class/thermal/cooling_device");
  

•    wprintw(win,"\n[Zone : Cooling]----------------------------------------------\n");
  

•   get_thermal_attr(win, linebuf, "type",SHORTT);
  

•   get_thermal_attr(win, linebuf, "cur_state",SHORTT);
  

•   get_thermal_attr(win, linebuf, "max_state",SHORTT);
  

•   return 0;
  

+}

+static int get_hwmon_info(WINDOW *win) +{

•   char linebuf[1024];
  

•    string line;
  

•   sprintf(linebuf,"/sys/class/hwmon/hwmon");
  

•   wprintw(win,"\n[HWMON info]--------------------------------------------------\n");
  

•   get_thermal_attr(win, linebuf, "name",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp1_input",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp1_crit",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp1_max",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp2_input",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp2_crit",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp2_max",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp3_input",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp3_crit",SHORTT);
  

•   get_thermal_attr(win, linebuf, "temp3_max",SHORTT);
  

•   get_thermal_attr(win, linebuf, "fan1_input",SHORTT);
  

•   get_thermal_attr(win, linebuf, "fan2_input",SHORTT);
  

•   get_thermal_attr(win, linebuf, "fan3_input",SHORTT);
  

•   //todo: add more hwmon info
  

Ditto.

•   return 0;
  

+}

+void thermal_update_display(void) +{

•   WINDOW *win;
  

•   win = get_ncurses_win("Thermal stats");
  

•    if (!win) {
  

•           printf("error: Window Thermal stats not found\n");
  

•            return;
  

•   }
  

•   wclear(win);
  

•   wmove(win, 1,0);
  

•   wprintw(win,"           *** Thermal window ***\n\n");
  

The current window tab says it's a thermal window. So this line can be removed.

•   wprintw(win,"[Note: '*' means no-more/no parameters]\n\n");
  

Is it really required to mark end of parameters?

•   wprintw(win,"[Params    :       Per-Devices-Info(1->n)]\n");
  

•   wprintw(win,"    |                      |\n");
  

•   wprintw(win,"    |                      |\n");
  

•   wprintw(win,"    V                      V");
  

•   get_cooling_zone_info(win);
  

•   get_thermal_zone_info(win);
  

•   get_hwmon_info(win);
  

+} diff --git a/src/measurement/thermal.h b/src/measurement/thermal.h new file mode 100644 index 0000000..104e968 --- /dev/null +++ b/src/measurement/thermal.h @@ -0,0 +1 @@

+extern void thermal_update_display(void);

1.7.9.5

---

linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev