Hi again, I've noticed most of the code in powertop deals with sysfs directly, so I guess 'scanning' sysfs looking for an Intel GPU is the way to go. I've checked the latest version of xf86-video-intel (2.21.11) and it looks like Intel GPUs all have the same PCI vendor ID (0x8086). Below is a patch that looks for all items /sys/class/drm/card[0-9]+ and checks whether the vendor is Intel. It then checks whether it can find the power/rc6_residency_ms file like it did earlier and uses the first GPU that passes all these checks.

What it does not fix: - Situations where there may be multiple Intel GPUs. I don't know whether such systems exist, and in that case each GPU would need to be tied to a specific processor (socket). - Hardcoded paths used to retrieve backlight information. Side effects: - In the case that any video driver other than Intel's exposed a 'power/rc6_residency_ms' file, the patch below will not consider the device because it checks vendor ID. If that is not desired, the vendor ID check should be omitted. From 71b0cea0b9b5cbe9ad7f673375004928ae842d9f Mon Sep 17 00:00:00 2001 From: Niels Penneman <niels.penneman(a)elis.ugent.be&gt; Date: Thu, 4 Jul 2013 20:20:01 +0200 Subject: [PATCH] Probe for Intel GPU --- src/cpu/cpu.cpp | 60 ++++++++++++++++++++++++++++++++++++++++++++++----- src/cpu/intel_cpus.h | 2 ++ src/cpu/intel_gpu.cpp | 21 ++++++++++++------ 3 files changed, 72 insertions(+), 11 deletions(-) diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp index 5d0db28..7fe3f84 100644 --- a/src/cpu/cpu.cpp +++ b/src/cpu/cpu.cpp @@ -22,11 +22,13 @@ * Authors: * Arjan van de Ven <arjan(a)linux.intel.com&gt; */ +#include <cctype> #include <iostream> #include <fstream> #include <vector> #include <string.h> #include <stdlib.h> +#include <dirent.h> #include <ncurses.h> #include <unistd.h> #include "cpu.h" @@ -151,13 +153,14 @@ static class abstract_cpu * new_core(int core, int cpu, char * vendor, int famil return ret; } -static class abstract_cpu * new_i965_gpu(void) +static class abstract_cpu * new_i965_gpu(const char *gpu) { class abstract_cpu *ret = NULL; ret = new class i965_core; ret->childcount = 0; ret->set_type("GPU"); + sprintf(static_cast<class i965_core *>(ret)->sysfs_dir, "/sys/class/drm/%s/power/", gpu); return ret; } @@ -260,7 +263,7 @@ static void handle_one_cpu(unsigned int number, char *vendor, int family, int mo all_cpus[number] = cpu; } -static void handle_i965_gpu(void) +static void handle_i965_gpu(const char *gpu) { unsigned int core_number = 0; class abstract_cpu *package; @@ -274,11 +277,56 @@ static void handle_i965_gpu(void) package->children.resize(core_number + 1, NULL); if (!package->children[core_number]) { - package->children[core_number] = new_i965_gpu(); + package->children[core_number] = new_i965_gpu(gpu); package->childcount++; } } +static bool probe_i965_gpu(char *gpu) +{ + DIR *dir; + struct dirent *entry; + bool found = false; + + dir = opendir("/sys/class/drm"); + if (!dir) + return false; + while ((entry = readdir(dir))) { + ifstream file; + char filename[4096]; + char *p; + uint16_t vendor; + + /* Match only cardN entries (N=0,1,...) */ + if (strncmp(entry->d_name, "card", 4) != 0) + continue; + p = &entry->d_name[4]; + while (*p && std::isdigit(*p++)); + if (*p) + continue; + + /* Check for Intel vendor ID */ + sprintf(filename, "/sys/class/drm/%s/device/vendor", entry->d_name); + file.open(filename); + if (file) { + file >> hex >> vendor; + file.close(); + } + if (vendor != 0x8086) + continue; + + /* Check for one of the interesting files */ + sprintf(filename, "/sys/class/drm/%s/power/rc6_residency_ms", entry->d_name); + if (access(filename, R_OK) == 0) { + strcpy(gpu, entry->d_name); + found = true; + break; + } + } + closedir(dir); + return found; +} + void enumerate_cpus(void) { @@ -290,6 +338,8 @@ void enumerate_cpus(void) int family = 0; int model = 0; + char gpu[256]; + file.open("/proc/cpuinfo", ios::in); if (!file) @@ -350,8 +400,8 @@ void enumerate_cpus(void) file.close(); - if (access("/sys/class/drm/card0/power/rc6_residency_ms", R_OK) == 0) - handle_i965_gpu(); + if (probe_i965_gpu(gpu)) + handle_i965_gpu(gpu); perf_events = new perf_power_bundle(); diff --git a/src/cpu/intel_cpus.h b/src/cpu/intel_cpus.h index 64d74f2..32679c1 100644 --- a/src/cpu/intel_cpus.h +++ b/src/cpu/intel_cpus.h @@ -137,6 +137,8 @@ private: struct timeval after; public: + char sysfs_dir[512]; + virtual void measurement_start(void); virtual void measurement_end(void); virtual int can_collapse(void) { return 0;}; diff --git a/src/cpu/intel_gpu.cpp b/src/cpu/intel_gpu.cpp index e0f4ac2..a579e28 100644 --- a/src/cpu/intel_gpu.cpp +++ b/src/cpu/intel_gpu.cpp @@ -43,11 +43,15 @@ void i965_core::measurement_start(void) { ifstream file; + char filename[512]; gettimeofday(&before, NULL); - rc6_before = read_sysfs("/sys/class/drm/card0/power/rc6_residency_ms", NULL); - rc6p_before = read_sysfs("/sys/class/drm/card0/power/rc6p_residency_ms", NULL); - rc6pp_before = read_sysfs("/sys/class/drm/card0/power/rc6pp_residency_ms", NULL); + sprintf(filename, "%src6_residency_ms", sysfs_dir); + rc6_before = read_sysfs(filename, NULL); + sprintf(filename, "%src6p_residency_ms", sysfs_dir); + rc6p_before = read_sysfs(filename, NULL); + sprintf(filename, "%src6pp_residency_ms", sysfs_dir); + rc6pp_before = read_sysfs(filename, NULL); update_cstate("gpu c0", "Powered On", 0, 0, 1, 0); update_cstate("gpu rc6", "RC6", 0, rc6_before, 1, 1); @@ -101,11 +105,16 @@ char * i965_core::fill_cstate_line(int line_nr, char *buffer, const char *separa void i965_core::measurement_end(void) { + char filename[512]; + gettimeofday(&after, NULL); - rc6_after = read_sysfs("/sys/class/drm/card0/power/rc6_residency_ms", NULL); - rc6p_after = read_sysfs("/sys/class/drm/card0/power/rc6p_residency_ms", NULL); - rc6pp_after = read_sysfs("/sys/class/drm/card0/power/rc6pp_residency_ms", NULL); + sprintf(filename, "%src6_residency_ms", sysfs_dir); + rc6_after = read_sysfs(filename, NULL); + sprintf(filename, "%src6p_residency_ms", sysfs_dir); + rc6p_after = read_sysfs(filename, NULL); + sprintf(filename, "%src6pp_residency_ms", sysfs_dir); + rc6pp_after = read_sysfs(filename, NULL); } char * i965_core::fill_pstate_line(int line_nr, char *buffer) -- 1.8.1.5 Regards, On 07/04/2013 09:47 AM, Niels Penneman wrote: > Hi all, > > Currently powertop tries to find Intel GPU statistics in sysfs with a > hardcoded path; in src/cpu/cpu.cpp:353-354: > > if (access("/sys/class/drm/card0/power/rc6_residency_ms", R_OK) == 0) > handle_i965_gpu(); > > In systems with multiple GPUs it is not guaranteed that the Intel GPU is > labeled 'card0' in sysfs. In my system with both integrated Intel and > discrete AMD Radeon the Intel GPU is 'card1', and powertop will not > display GPU statistics. > > One possible solution would be to scan all /sys/class/drm/card[0-9]+ > folders and check the vendor & device IDs in 'device/vendor' and > 'device/device'. Perhaps there is a better solution, e.g. by examining > the PCI device tree. > > > Regards, > -- Niels Penneman Computer Systems Lab Electronics and Information Systems Department Ghent University

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

On (07/04/13 20:33), Niels Penneman wrote: > Hi again, > > I've noticed most of the code in powertop deals with sysfs directly, so > I guess 'scanning' sysfs looking for an Intel GPU is the way to go. I've > checked the latest version of xf86-video-intel (2.21.11) and it looks > like Intel GPUs all have the same PCI vendor ID (0x8086). Below is a > patch that looks for all items /sys/class/drm/card[0-9]+ and checks > whether the vendor is Intel. It then checks whether it can find the > power/rc6_residency_ms file like it did earlier and uses the first GPU > that passes all these checks. > > What it does not fix: > - Situations where there may be multiple Intel GPUs. I don't know > whether such systems exist, and in that case each GPU would need to be > tied to a specific processor (socket). > - Hardcoded paths used to retrieve backlight information. > > Side effects: > - In the case that any video driver other than Intel's exposed a > 'power/rc6_residency_ms' file, the patch below will not consider the > device because it checks vendor ID. If that is not desired, the vendor > ID check should be omitted. > thanks for your patch. I took a quick look, please see inlined. > From 71b0cea0b9b5cbe9ad7f673375004928ae842d9f Mon Sep 17 00:00:00 2001 > From: Niels Penneman <niels.penneman(a)elis.ugent.be&gt; > Date: Thu, 4 Jul 2013 20:20:01 +0200 > Subject: [PATCH] Probe for Intel GPU > > --- > src/cpu/cpu.cpp | 60 > ++++++++++++++++++++++++++++++++++++++++++++++----- > src/cpu/intel_cpus.h | 2 ++ > src/cpu/intel_gpu.cpp | 21 ++++++++++++------ > 3 files changed, 72 insertions(+), 11 deletions(-) > > diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp > index 5d0db28..7fe3f84 100644 > --- a/src/cpu/cpu.cpp > +++ b/src/cpu/cpu.cpp > @@ -22,11 +22,13 @@ > * Authors: > * Arjan van de Ven <arjan(a)linux.intel.com&gt; > */ > +#include <cctype> > #include <iostream> > #include <fstream> > #include <vector> > #include <string.h> > #include <stdlib.h> > +#include <dirent.h> > #include <ncurses.h> > #include <unistd.h> > #include "cpu.h" > @@ -151,13 +153,14 @@ static class abstract_cpu * new_core(int core, int > cpu, char * vendor, int famil > return ret; > } > > -static class abstract_cpu * new_i965_gpu(void) > +static class abstract_cpu * new_i965_gpu(const char *gpu) > { > class abstract_cpu *ret = NULL; > > ret = new class i965_core; > ret->childcount = 0; > ret->set_type("GPU"); > + sprintf(static_cast<class i965_core *>(ret)->sysfs_dir, > "/sys/class/drm/%s/power/", gpu); > > return ret; > } > @@ -260,7 +263,7 @@ static void handle_one_cpu(unsigned int number, char > *vendor, int family, int mo > all_cpus[number] = cpu; > } > > -static void handle_i965_gpu(void) > +static void handle_i965_gpu(const char *gpu) > { > unsigned int core_number = 0; > class abstract_cpu *package; > @@ -274,11 +277,56 @@ static void handle_i965_gpu(void) > package->children.resize(core_number + 1, NULL); > > if (!package->children[core_number]) { > - package->children[core_number] = new_i965_gpu(); > + package->children[core_number] = new_i965_gpu(gpu); > package->childcount++; > } > } > > +static bool probe_i965_gpu(char *gpu) > +{ > + DIR *dir; > + struct dirent *entry; > + bool found = false; > + > + dir = opendir("/sys/class/drm"); > + if (!dir) > + return false; > + while ((entry = readdir(dir))) { > + ifstream file; > + char filename[4096]; > + char *p; > + uint16_t vendor; > + > + /* Match only cardN entries (N=0,1,...) */ > + if (strncmp(entry->d_name, "card", 4) != 0) > + continue; > + p = &entry->d_name[4]; > + while (*p && std::isdigit(*p++)); > + if (*p) > + continue; > + > + /* Check for Intel vendor ID */ > + sprintf(filename, "/sys/class/drm/%s/device/vendor", > entry->d_name); > + file.open(filename); > + if (file) { > + file >> hex >> vendor; > + file.close(); > + } > + if (vendor != 0x8086) > + continue; > + > + /* Check for one of the interesting files */ > + sprintf(filename, "/sys/class/drm/%s/power/rc6_residency_ms", > entry->d_name); > + if (access(filename, R_OK) == 0) { > + strcpy(gpu, entry->d_name); > + found = true; > + break; > + } > + } > + closedir(dir); > + return found; > +} > + > > void enumerate_cpus(void) > { > @@ -290,6 +338,8 @@ void enumerate_cpus(void) > int family = 0; > int model = 0; > > + char gpu[256]; > + > file.open("/proc/cpuinfo", ios::in); > > if (!file) > @@ -350,8 +400,8 @@ void enumerate_cpus(void) > > file.close(); > > - if (access("/sys/class/drm/card0/power/rc6_residency_ms", R_OK) == 0) > - handle_i965_gpu(); > + if (probe_i965_gpu(gpu)) > + handle_i965_gpu(gpu); > > perf_events = new perf_power_bundle(); > > diff --git a/src/cpu/intel_cpus.h b/src/cpu/intel_cpus.h > index 64d74f2..32679c1 100644 > --- a/src/cpu/intel_cpus.h > +++ b/src/cpu/intel_cpus.h > @@ -137,6 +137,8 @@ private: > struct timeval after; > > public: > + char sysfs_dir[512]; > + do we really need to store 512 bytes instead of 4? as a public member? btw, why 512?

> virtual void measurement_start(void); > virtual void measurement_end(void); > virtual int can_collapse(void) { return 0;}; > diff --git a/src/cpu/intel_gpu.cpp b/src/cpu/intel_gpu.cpp > index e0f4ac2..a579e28 100644 > --- a/src/cpu/intel_gpu.cpp > +++ b/src/cpu/intel_gpu.cpp > @@ -43,11 +43,15 @@ > void i965_core::measurement_start(void) > { > ifstream file; > + char filename[512]; > 4K

> gettimeofday(&before, NULL); > - rc6_before = > read_sysfs("/sys/class/drm/card0/power/rc6_residency_ms", NULL); > - rc6p_before = > read_sysfs("/sys/class/drm/card0/power/rc6p_residency_ms", NULL); > - rc6pp_before = > read_sysfs("/sys/class/drm/card0/power/rc6pp_residency_ms", NULL); > + sprintf(filename, "%src6_residency_ms", sysfs_dir); > + rc6_before = read_sysfs(filename, NULL); > + sprintf(filename, "%src6p_residency_ms", sysfs_dir); > + rc6p_before = read_sysfs(filename, NULL); > + sprintf(filename, "%src6pp_residency_ms", sysfs_dir); > + rc6pp_before = read_sysfs(filename, NULL); or we can sprintf gpu number? /sys/class/drm/card%d/power/rc6_residency_ms > update_cstate("gpu c0", "Powered On", 0, 0, 1, 0); > update_cstate("gpu rc6", "RC6", 0, rc6_before, 1, 1); > @@ -101,11 +105,16 @@ char * i965_core::fill_cstate_line(int line_nr, > char *buffer, const char *separa > > void i965_core::measurement_end(void) > { > + char filename[512]; > + > gettimeofday(&after, NULL); > > - rc6_after = > read_sysfs("/sys/class/drm/card0/power/rc6_residency_ms", NULL); > - rc6p_after = > read_sysfs("/sys/class/drm/card0/power/rc6p_residency_ms", NULL); > - rc6pp_after = > read_sysfs("/sys/class/drm/card0/power/rc6pp_residency_ms", NULL); > + sprintf(filename, "%src6_residency_ms", sysfs_dir); > + rc6_after = read_sysfs(filename, NULL); > + sprintf(filename, "%src6p_residency_ms", sysfs_dir); > + rc6p_after = read_sysfs(filename, NULL); > + sprintf(filename, "%src6pp_residency_ms", sysfs_dir); > + rc6pp_after = read_sysfs(filename, NULL); > } > same as above. -ss > > char * i965_core::fill_pstate_line(int line_nr, char *buffer) > -- > 1.8.1.5 > > > > > Regards, > > > On 07/04/2013 09:47 AM, Niels Penneman wrote: >> Hi all, >> >> Currently powertop tries to find Intel GPU statistics in sysfs with a >> hardcoded path; in src/cpu/cpu.cpp:353-354: >> >> if (access("/sys/class/drm/card0/power/rc6_residency_ms", R_OK) == 0) >> handle_i965_gpu(); >> >> In systems with multiple GPUs it is not guaranteed that the Intel GPU is >> labeled 'card0' in sysfs. In my system with both integrated Intel and >> discrete AMD Radeon the Intel GPU is 'card1', and powertop will not >> display GPU statistics. >> >> One possible solution would be to scan all /sys/class/drm/card[0-9]+ >> folders and check the vendor & device IDs in 'device/vendor' and >> 'device/device'. Perhaps there is a better solution, e.g. by examining >> the PCI device tree. >> >> >> Regards, >> > -- > Niels Penneman > Computer Systems Lab > Electronics and Information Systems Department > Ghent University > > _______________________________________________ > PowerTop mailing list > PowerTop(a)lists.01.org > https://lists.01.org/mailman/listinfo/powertop

On (07/08/13 14:30), Niels Penneman wrote: [..] > >> + > > do we really need to store 512 bytes instead of 4? as a public member? > > > > btw, why 512? > > Indeed, we can store GPU number instead. I looked at how paths were > stored throughout powertop and found buffer sizes of 256 for filenames, > so I doubled it to 512 when adding '/sys/class/drm' etc. GPU number is > better though, I'll edit the patch. > thanks. [..] > >> + char filename[512]; > >> > > 4K > Did you mean buffer size should be 4k ? It would be helpful if powertop > sources actually had constants for this. > better to use standard ones. linux/limits.h [..] #define NAME_MAX 255 /* # chars in a file name */ #define PATH_MAX 4096 /* # chars in a path name including nul [..] -ss