Hello Auke, Could you please look at this patch and provide your feedback ? Thanks!
Regards, Amit Arora On Fri, Aug 13, 2010 at 12:55 PM, Amit Arora <[email protected]> wrote: > Background: > Currently, PowerTOP has hard coded values for the number of C and P > states supported. Hence it doesn't work well on systems which may have > more states than these hard coded ones. This is specially true for some > of the ARM SoCs which may have as many as 8 to 9 C-states. > > Suggested fix: > This patch defines MAX values of C and P states for allocating enough > memory for data structures statically. But, uses actual values of the > MAX C and P states for display (using, maxcstate and maxpstate > variables). > > Testing done: > This patch has been tested on a few x86 desktops, x86_64 servers, a > laptop and a couple of ARM boards (OMAP3 and imx53). > > Signed-off-by: Amit Arora <[email protected]> > --- > cpufreqstats.c | 15 ++++++++------- > display.c | 24 +++++++++++++++++------- > powertop.c | 56 > +++++++++++++++++++++++++++++++++++--------------------- > powertop.h | 16 ++++++++++++++-- > 4 files changed, 74 insertions(+), 37 deletions(-) > > diff --git a/cpufreqstats.c b/cpufreqstats.c > index d10b047..6e948cd 100644 > --- a/cpufreqstats.c > +++ b/cpufreqstats.c > @@ -37,12 +37,12 @@ struct cpufreqdata { > uint64_t count; > }; > > -struct cpufreqdata freqs[16]; > -struct cpufreqdata oldfreqs[16]; > +struct cpufreqdata freqs[MAX_NUM_PSTATES]; > +struct cpufreqdata oldfreqs[MAX_NUM_PSTATES]; > > -struct cpufreqdata delta[16]; > +struct cpufreqdata delta[MAX_NUM_PSTATES]; > > -char cpufreqstrings[6][80]; > +char cpufreqstrings[MAX_NUM_PSTATES+1][80]; > int topfreq = -1; > > static void zap(void) > @@ -114,7 +114,7 @@ void do_cpufreq_stats(void) > memset(&cpufreqstrings, 0, sizeof(cpufreqstrings)); > sprintf(cpufreqstrings[0], _("P-states (frequencies)\n")); > > - for (ret = 0; ret<16; ret++) > + for (ret = 0; ret < MAX_NUM_PSTATES; ret++) > freqs[ret].count = 0; > > dir = opendir("/sys/devices/system/cpu"); > @@ -153,7 +153,7 @@ void do_cpufreq_stats(void) > if (f && maxfreq < i) > maxfreq = i; > i++; > - if (i>15) > + if (i >= MAX_NUM_PSTATES) > break; > } > fclose(file); > @@ -161,7 +161,7 @@ void do_cpufreq_stats(void) > > closedir(dir); > > - for (ret = 0; ret < 16; ret++) { > + for (ret = 0; ret < MAX_NUM_PSTATES; ret++) { > delta[ret].count = freqs[ret].count - oldfreqs[ret].count; > total_time += delta[ret].count; > delta[ret].frequency = freqs[ret].frequency; > @@ -176,6 +176,7 @@ void do_cpufreq_stats(void) > qsort(&delta, maxfreq+1, sizeof(struct cpufreqdata), sort_by_count); > if (maxfreq>4) > maxfreq=4; > + maxpstate = maxfreq; > qsort(&delta, maxfreq+1, sizeof(struct cpufreqdata), sort_by_freq); > > if (delta[0].frequency == delta[1].frequency + 1000) > diff --git a/display.c b/display.c > index 79732ab..df23622 100644 > --- a/display.c > +++ b/display.c > @@ -91,15 +91,20 @@ int maxwidth = 200; > > void setup_windows(void) > { > + int yline = (maxcstate >= maxpstate) ? maxcstate+1 : maxpstate; > + > + /* number of states is one more than the MAX state! */ > + yline++; > + > getmaxyx(stdscr, maxy, maxx); > > zap_windows(); > > title_bar_window = subwin(stdscr, 1, maxx, 0, 0); > - cstate_window = subwin(stdscr, 7, maxx, 2, 0); > - wakeup_window = subwin(stdscr, 1, maxx, 9, 0); > - battery_power_window = subwin(stdscr, 2, maxx, 10, 0); > - timerstat_window = subwin(stdscr, maxy-16, maxx, 12, 0); > + cstate_window = subwin(stdscr, (yline + 3), maxx, 2, 0); > + wakeup_window = subwin(stdscr, 1, maxx, (yline + 4), 0); > + battery_power_window = subwin(stdscr, 2, maxx, (yline + 5), 0); > + timerstat_window = subwin(stdscr, maxy-16, maxx, (yline + 7), 0); > maxtimerstats = maxy-16 -2; > maxwidth = maxx - 18; > suggestion_window = subwin(stdscr, 3, maxx, maxy-4, 0); > @@ -164,20 +169,25 @@ void show_title_bar(void) > void show_cstates(void) > { > int i, count = 0; > + int maxcstatelines = (maxcstate >= maxpstate) ? maxcstate+1 : > maxpstate; > + > + /* Number of states is one more than the MAX state! */ > + maxcstatelines++; > + > werase(cstate_window); > > - for (i=0; i < 10; i++) { > + for (i = 0; i <= maxcstatelines; i++) { > if (i == topcstate+1) > wattron(cstate_window, A_BOLD); > else > wattroff(cstate_window, A_BOLD); > - if (strlen(cstate_lines[i]) && count <= 6) { > + if (strlen(cstate_lines[i]) && count <= maxcstatelines) { > print(cstate_window, count, 0, "%s", cstate_lines[i]); > count++; > } > } > > - for (i=0; i<6; i++) { > + for (i = 0; i <= (maxpstate+1); i++) { > if (i == topfreq+1) > wattron(cstate_window, A_BOLD); > else > diff --git a/powertop.c b/powertop.c > index 74eb328..16e8ab0 100644 > --- a/powertop.c > +++ b/powertop.c > @@ -41,9 +41,9 @@ > #include "powertop.h" > > > -uint64_t start_usage[8], start_duration[8]; > -uint64_t last_usage[8], last_duration[8]; > -char cnames[8][16]; > +uint64_t start_usage[MAX_NUM_CSTATES], start_duration[MAX_NUM_CSTATES]; > +uint64_t last_usage[MAX_NUM_CSTATES], last_duration[MAX_NUM_CSTATES]; > +char cnames[MAX_NUM_CSTATES][16]; > > double ticktime = 15.0; > > @@ -51,7 +51,8 @@ int interrupt_0, total_interrupt; > > int showpids = 0; > > -static int maxcstate = 0; > +int maxcstate; > +int maxpstate; > int topcstate = 0; > > int dump = 0; > @@ -450,6 +451,10 @@ static void read_data_cpuidle(uint64_t * usage, uint64_t > * duration) > sprintf(cnames[clevel], "%s\t", "C1 > halt"); > } > } > + > + if (clevel > maxcstate) > + maxcstate = clevel; > + > sprintf(filename + len, "/%s/usage", entry->d_name); > file = fopen(filename, "r"); > if (!file) > @@ -477,9 +482,6 @@ static void read_data_cpuidle(uint64_t * usage, uint64_t > * duration) > duration[clevel] += 1+strtoull(line, NULL, 10); > > clevel++; > - if (clevel > maxcstate) > - maxcstate = clevel; > - > } > closedir(dir); > > @@ -824,7 +826,7 @@ void print_battery_sysfs(void) > show_acpi_power_line(rate, cap, prev_bat_cap - cap, time(NULL) - > prev_bat_time); > } > > -char cstate_lines[12][200]; > +char cstate_lines[MAX_CSTATE_LINES][200]; > > void usage() > { > @@ -848,7 +850,7 @@ int main(int argc, char **argv) > char line[1024]; > int ncursesinited=0; > FILE *file = NULL; > - uint64_t cur_usage[8], cur_duration[8]; > + uint64_t cur_usage[MAX_NUM_CSTATES], cur_duration[MAX_NUM_CSTATES]; > double wakeups_per_second = 0; > > setlocale (LC_ALL, ""); > @@ -957,25 +959,22 @@ int main(int argc, char **argv) > do_proc_irq(); > read_data(&cur_usage[0], &cur_duration[0]); > > + if (maxcstate >= MAX_NUM_CSTATES) { > + printf("Actual CSTATES (%d) > MAX_NUM_CSTATES > (%d)!\n", > + maxcstate+1, MAX_NUM_CSTATES); > + exit(1); > + } > + > totalticks = 0; > totalevents = 0; > - for (i = 0; i < 8; i++) > + for (i = 0; i <= maxcstate; i++) > if (cur_usage[i]) { > totalticks += cur_duration[i] - > last_duration[i]; > totalevents += cur_usage[i] - last_usage[i]; > } > > - if (!dump) { > - if (!ncursesinited) { > - initialize_curses(); > - ncursesinited++; > - } > - setup_windows(); > - show_title_bar(); > - } > - > memset(&cstate_lines, 0, sizeof(cstate_lines)); > - topcstate = -4; > + topcstate = -(maxcstate); > if (totalevents == 0 && maxcstate <= 1) { > sprintf(cstate_lines[5],_("< Detailed C-state > information is not available.>\n")); > } else { > @@ -989,7 +988,7 @@ int main(int argc, char **argv) > sprintf(cstate_lines[1], _("C0 (cpu running) > (%4.1f%%)\n"), percentage); > if (percentage > 50) > topcstate = 0; > - for (i = 0; i < 8; i++) > + for (i = 0; i <= maxcstate; i++) > if (cur_usage[i]) { > sleept = (cur_duration[i] - > last_duration[i]) / (cur_usage[i] - last_usage[i] > > + 0.1) / FREQ; > @@ -1010,6 +1009,21 @@ int main(int argc, char **argv) > } > } > do_cpufreq_stats(); > + > + if (maxpstate > MAX_NUM_PSTATES) { > + printf("Actual PSTATES (%d) > MAX_NUM_PSTATES > (%d)!\n", > + maxpstate, MAX_NUM_PSTATES); > + exit(1); > + } > + > + if (!dump) { > + if (!ncursesinited) { > + initialize_curses(); > + ncursesinited++; > + } > + setup_windows(); > + show_title_bar(); > + } > show_cstates(); > /* now the timer_stats info */ > memset(line, 0, sizeof(line)); > diff --git a/powertop.h b/powertop.h > index d8f8182..22814d5 100644 > --- a/powertop.h > +++ b/powertop.h > @@ -28,6 +28,16 @@ > > #include <libintl.h> > > +/* > + * Define the MAX values for [CP]STATES "big enough". > + * Defining it too less will result in missing some C/P state information. > + * Defining the values too high will result in memory wastage. > + */ > +#define MAX_NUM_CSTATES 10 > +#define MAX_NUM_PSTATES 16 > + > +#define MAX_CSTATE_LINES (MAX_NUM_CSTATES + 3) > + > struct line { > char *string; > int count; > @@ -41,6 +51,8 @@ extern struct line *lines; > extern int linehead; > extern int linesize; > extern int linectotal; > +extern int maxcstate; > +extern int maxpstate; > > extern double displaytime; > > @@ -70,8 +82,8 @@ void devicepm_activity_hint(void); > > > > -extern char cstate_lines[12][200]; > -extern char cpufreqstrings[6][80]; > +extern char cstate_lines[MAX_CSTATE_LINES][200]; > +extern char cpufreqstrings[MAX_NUM_PSTATES+1][80]; > > extern int topcstate; > extern int topfreq; > -- > 1.7.0.4 > > _______________________________________________ Power mailing list [email protected] http://www.bughost.org/mailman/listinfo/power
