Re: [PATCH] Add GPU_POWER sensors (v2)

2017-02-11 Thread Tom St Denis 

On 02/11/2017 08:29 AM, Kai Wasserbäch wrote:

Hey Tom,
Tom St Denis wrote on 11.02.2017 12:26:

Add the ability to sample GPU_POWER sensors.  Because
the sensors have a high latency we read them from a background
thread which means we've added the requirement for pthreads.

Signed-off-by: Tom St Denis 

(v2) Cleaned up CMake around pthreads
---
 CMakeLists.txt |  4 +++
 README |  6 ++--
 src/app/top.c  | 88 +-
 src/lib/CMakeLists.txt |  1 +
 src/lib/read_sensor.c  | 37 +
 src/umr.h  |  5 +++
 6 files changed, 123 insertions(+), 18 deletions(-)
 create mode 100644 src/lib/read_sensor.c

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ef78c97ad763..8d89445c39e3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
 # Add local repository for FindXXX.cmake modules.
 SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/" 
${CMAKE_MODULE_PATH})

+set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+find_package(Threads REQUIRED)
+
 find_package(Curses REQUIRED)
 include_directories(${CURSES_INCLUDE_DIRS})

@@ -31,6 +34,7 @@ include_directories(${LIBDRM_INCLUDE_DIR})
 set(REQUIRED_EXTERNAL_LIBS
   ${CURSES_LIBRARIES}
   ${PCIACCESS_LIBRARIES}
+  Threads::Threads
 )

 # Global setting: build everything position independent
diff --git a/README b/README
index 13cdac663d20..8a8de8485ac7 100644
--- a/README
+++ b/README
@@ -28,9 +28,9 @@ mailing list at:
 Building
 -

-To build umr you will need pciaccess, ncurses, and libdrm headers and
-libraries.  Which are available in both Fedora and Ubuntu (as well as
-other distributions).  To build umr:
+To build umr you will need pciaccess, ncurses, libdrm, and pthread
+headers and libraries.  Which are available in both Fedora and Ubuntu
+(as well as other distributions).  To build umr:


maybe just write "most distributions"? Since you're not giving package names I
don't really see a point in naming two distributions, especially where one is
just a derivative.


I can update that.  To be honest I do 99% of my testing with Fedora and 
Ubuntu is used quite a bit amongst AMDers.






 $ mkdir build && cd build/ && cmake ../
 $ make
diff --git a/src/app/top.c b/src/app/top.c
index b081515a5b40..60f629d247f3 100644
--- a/src/app/top.c
+++ b/src/app/top.c
@@ -54,6 +54,7 @@ enum sensor_maps {
SENSOR_IDENTITY=0, // x = x
SENSOR_D1000,// x = x/1000
SENSOR_D100,// x = x/100
+   SENSOR_WATT,
 };

 enum sensor_print {
@@ -61,6 +62,7 @@ enum sensor_print {
SENSOR_MHZ,
SENSOR_PERCENT,
SENSOR_TEMP,
+   SENSOR_POWER,
 };

 enum drm_print {
@@ -171,19 +173,14 @@ static struct umr_bitfield stat_uvd_pgfsm7_bits[] = {
 static struct umr_bitfield stat_mc_hub_bits[] = {
 { "OUTSTANDING_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDRET", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRRET", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_RPB_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_MCD_ATOMIC", 255, 255, _bitfield_default },
 { NULL, 0, 0, NULL },
 };


I would agree with Edward and rather have these in a separate clean-up patch.


It's interesting that you note this.


Could you put the assignment and break on their own lines? Would increase
readability IMHO. Also maybe add spaces between variables, operators and
constants? I'm imagining something like


Because of this.

I'm not disagreeing with your formatting suggestion it just seems at 
this point we're building a deluxe bike shed.  I already own a bike shed 
(which my daughter calls a mini house) and don't need another. :-)


If I revert the hunk about the removed comment lines I cannot logically 
include your formatting changes since they're not material to the 
functional changes introduced by this commit.	


A reasonable compromise is to split this into two patches one which 
cleans up formatting first (and removes the commented lines) and the 2nd 
which introduces the new sensors.



Tom
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org

Re: [PATCH] Add GPU_POWER sensors (v2)

2017-02-11 Thread Kai Wasserbäch 
Hey Tom,
Tom St Denis wrote on 11.02.2017 12:26:
> Add the ability to sample GPU_POWER sensors.  Because
> the sensors have a high latency we read them from a background
> thread which means we've added the requirement for pthreads.
> 
> Signed-off-by: Tom St Denis 
> 
> (v2) Cleaned up CMake around pthreads
> ---
>  CMakeLists.txt |  4 +++
>  README |  6 ++--
>  src/app/top.c  | 88 
> +-
>  src/lib/CMakeLists.txt |  1 +
>  src/lib/read_sensor.c  | 37 +
>  src/umr.h  |  5 +++
>  6 files changed, 123 insertions(+), 18 deletions(-)
>  create mode 100644 src/lib/read_sensor.c
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index ef78c97ad763..8d89445c39e3 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
>  # Add local repository for FindXXX.cmake modules.
>  SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/" 
> ${CMAKE_MODULE_PATH})
>  
> +set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
> +find_package(Threads REQUIRED)
> +
>  find_package(Curses REQUIRED)
>  include_directories(${CURSES_INCLUDE_DIRS})
>  
> @@ -31,6 +34,7 @@ include_directories(${LIBDRM_INCLUDE_DIR})
>  set(REQUIRED_EXTERNAL_LIBS
>${CURSES_LIBRARIES}
>${PCIACCESS_LIBRARIES}
> +  Threads::Threads
>  )
>  
>  # Global setting: build everything position independent
> diff --git a/README b/README
> index 13cdac663d20..8a8de8485ac7 100644
> --- a/README
> +++ b/README
> @@ -28,9 +28,9 @@ mailing list at:
>  Building
>  -
>  
> -To build umr you will need pciaccess, ncurses, and libdrm headers and
> -libraries.  Which are available in both Fedora and Ubuntu (as well as
> -other distributions).  To build umr:
> +To build umr you will need pciaccess, ncurses, libdrm, and pthread 
> +headers and libraries.  Which are available in both Fedora and Ubuntu 
> +(as well as other distributions).  To build umr:

maybe just write "most distributions"? Since you're not giving package names I
don't really see a point in naming two distributions, especially where one is
just a derivative.

>  $ mkdir build && cd build/ && cmake ../
>  $ make
> diff --git a/src/app/top.c b/src/app/top.c
> index b081515a5b40..60f629d247f3 100644
> --- a/src/app/top.c
> +++ b/src/app/top.c
> @@ -54,6 +54,7 @@ enum sensor_maps {
>   SENSOR_IDENTITY=0, // x = x
>   SENSOR_D1000,// x = x/1000
>   SENSOR_D100,// x = x/100
> + SENSOR_WATT,
>  };
>  
>  enum sensor_print {
> @@ -61,6 +62,7 @@ enum sensor_print {
>   SENSOR_MHZ,
>   SENSOR_PERCENT,
>   SENSOR_TEMP,
> + SENSOR_POWER,
>  };
>  
>  enum drm_print {
> @@ -171,19 +173,14 @@ static struct umr_bitfield stat_uvd_pgfsm7_bits[] = {
>  static struct umr_bitfield stat_mc_hub_bits[] = {
>{ "OUTSTANDING_READ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_WRITE", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_ATOMIC", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_RDREQ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_RDRET", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_WRREQ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_WRRET", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, _bitfield_default },
>{ "OUTSTANDING_RPB_READ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_RPB_WRITE", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_RPB_ATOMIC", 255, 255, _bitfield_default },
>{ "OUTSTANDING_MCD_READ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_MCD_WRITE", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_MCD_ATOMIC", 255, 255, _bitfield_default },
>{ NULL, 0, 0, NULL },
>  };

I would agree with Edward and rather have these in a separate clean-up patch.

> @@ -227,6 +224,10 @@ static struct umr_bitfield stat_vi_sensor_bits[] = {
>   { "GFX_MCLK", AMDGPU_PP_SENSOR_GFX_MCLK, SENSOR_D100|(SENSOR_MHZ<<4), 
> _bitfield_default },
>   { "GPU_LOAD", AMDGPU_PP_SENSOR_GPU_LOAD, SENSOR_PERCENT<<4, 
> _bitfield_default },
>   { "GPU_TEMP", AMDGPU_PP_SENSOR_GPU_TEMP, SENSOR_D1000|(SENSOR_TEMP<<4), 
> _bitfield_default },
> + { "VDDC", AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
> + { "VDDCI",AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
> + { "MAX_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
> + { "AVG_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
>   { NULL, 0, 0, NULL },
>  };
>  
> @@ -256,6 +257,21 @@ static struct umr_bitfield stat_drm_bits[] = {
>  
>  static FILE *logfile = NULL;
>  
> +static volatile int sensor_thread_quit = 0;
> +static

Re: [PATCH] Add GPU_POWER sensors

2017-02-11 Thread Tom St Denis 

On 11/02/17 05:56 AM, Kai Wasserbäch wrote:

Hey Tom,
Tom St Denis wrote on 11.02.2017 02:02:

On 02/10/2017 07:25 PM, Edward O'Callaghan wrote:

Hey Tom,

On 02/11/2017 05:10 AM, Tom St Denis wrote:

Add the ability to sample GPU_POWER sensors.  Because
the sensors have a high latency we read them from a background
thread which means we've added the requirement for pthreads.

Signed-off-by: Tom St Denis 
---
 CMakeLists.txt |  5 ++-
 README |  6 ++--
 src/app/top.c  | 88 +-
 src/lib/CMakeLists.txt |  1 +
 src/lib/read_sensor.c  | 37 +
 src/umr.h  |  5 +++
 6 files changed, 123 insertions(+), 19 deletions(-)
 create mode 100644 src/lib/read_sensor.c

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ef78c97ad763..7b771d01919b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
 # Add local repository for FindXXX.cmake modules.
 SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/"
${CMAKE_MODULE_PATH})

+find_package(Threads REQUIRED)
+include_directories(${THREADS_INCLUDE_DIRS})

Do you need this include_directories() line?


+
 find_package(Curses REQUIRED)
 include_directories(${CURSES_INCLUDE_DIRS})

@@ -37,7 +40,7 @@ set(REQUIRED_EXTERNAL_LIBS
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)

 # CFLAGS += -Wall -W -O2 -g3 -Isrc/ -DPIC -fPIC
-set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -W -O2 -g3")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pthread -Wall -W -O2 -g3")

You don't really want to have your linkage flags here, I think your
looking for ${CMAKE_THREAD_LIBS_INIT} to go into the
REQUIRED_EXTERNAL_LIBS list.


How does it "go into"?  Simple '+='?

Can you just send a quick patch I can squash into this?  The less I know about
cmake the more room I have in my head for useful things :-) hehehe.


1. You might want to set
  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
at before the find_package() for Threads
2. Linking usually happens through target_link_*() calls, where you would add
Threads::Threads (special thing created by FindThreads.cmake) to the list of
your other variables.

See  for details on
how FindThreads.cmake works. In your case you might want to set
THREADS_PREFER_PTHREAD_FLAG as well.


Thanks.  I've sent v2 to the list.

Cheers,
Tom
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] Add GPU_POWER sensors (v2)

2017-02-11 Thread Tom St Denis 
Add the ability to sample GPU_POWER sensors.  Because
the sensors have a high latency we read them from a background
thread which means we've added the requirement for pthreads.

Signed-off-by: Tom St Denis 

(v2) Cleaned up CMake around pthreads
---
 CMakeLists.txt |  4 +++
 README |  6 ++--
 src/app/top.c  | 88 +-
 src/lib/CMakeLists.txt |  1 +
 src/lib/read_sensor.c  | 37 +
 src/umr.h  |  5 +++
 6 files changed, 123 insertions(+), 18 deletions(-)
 create mode 100644 src/lib/read_sensor.c

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ef78c97ad763..8d89445c39e3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
 # Add local repository for FindXXX.cmake modules.
 SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/" 
${CMAKE_MODULE_PATH})
 
+set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+find_package(Threads REQUIRED)
+
 find_package(Curses REQUIRED)
 include_directories(${CURSES_INCLUDE_DIRS})
 
@@ -31,6 +34,7 @@ include_directories(${LIBDRM_INCLUDE_DIR})
 set(REQUIRED_EXTERNAL_LIBS
   ${CURSES_LIBRARIES}
   ${PCIACCESS_LIBRARIES}
+  Threads::Threads
 )
 
 # Global setting: build everything position independent
diff --git a/README b/README
index 13cdac663d20..8a8de8485ac7 100644
--- a/README
+++ b/README
@@ -28,9 +28,9 @@ mailing list at:
 Building
 -
 
-To build umr you will need pciaccess, ncurses, and libdrm headers and
-libraries.  Which are available in both Fedora and Ubuntu (as well as
-other distributions).  To build umr:
+To build umr you will need pciaccess, ncurses, libdrm, and pthread 
+headers and libraries.  Which are available in both Fedora and Ubuntu 
+(as well as other distributions).  To build umr:
 
 $ mkdir build && cd build/ && cmake ../
 $ make
diff --git a/src/app/top.c b/src/app/top.c
index b081515a5b40..60f629d247f3 100644
--- a/src/app/top.c
+++ b/src/app/top.c
@@ -54,6 +54,7 @@ enum sensor_maps {
SENSOR_IDENTITY=0, // x = x
SENSOR_D1000,// x = x/1000
SENSOR_D100,// x = x/100
+   SENSOR_WATT,
 };
 
 enum sensor_print {
@@ -61,6 +62,7 @@ enum sensor_print {
SENSOR_MHZ,
SENSOR_PERCENT,
SENSOR_TEMP,
+   SENSOR_POWER,
 };
 
 enum drm_print {
@@ -171,19 +173,14 @@ static struct umr_bitfield stat_uvd_pgfsm7_bits[] = {
 static struct umr_bitfield stat_mc_hub_bits[] = {
 { "OUTSTANDING_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDRET", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRRET", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_RPB_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_MCD_ATOMIC", 255, 255, _bitfield_default },
 { NULL, 0, 0, NULL },
 };
 
@@ -227,6 +224,10 @@ static struct umr_bitfield stat_vi_sensor_bits[] = {
{ "GFX_MCLK", AMDGPU_PP_SENSOR_GFX_MCLK, SENSOR_D100|(SENSOR_MHZ<<4), 
_bitfield_default },
{ "GPU_LOAD", AMDGPU_PP_SENSOR_GPU_LOAD, SENSOR_PERCENT<<4, 
_bitfield_default },
{ "GPU_TEMP", AMDGPU_PP_SENSOR_GPU_TEMP, SENSOR_D1000|(SENSOR_TEMP<<4), 
_bitfield_default },
+   { "VDDC", AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
+   { "VDDCI",AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
+   { "MAX_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
+   { "AVG_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
{ NULL, 0, 0, NULL },
 };
 
@@ -256,6 +257,21 @@ static struct umr_bitfield stat_drm_bits[] = {
 
 static FILE *logfile = NULL;
 
+static volatile int sensor_thread_quit = 0;
+static uint32_t gpu_power_data[4];
+static void *vi_sensor_thread(void *data)
+{
+   struct umr_asic asic = *((struct umr_asic*)data);
+   int size = sizeof(gpu_power_data);
+   char fname[128];
+
+   snprintf(fname, sizeof(fname)-1, 
"/sys/kernel/debug/dri/%d/amdgpu_sensors", asic.instance);
+   asic.fd.sensors = open(fname, O_RDWR);
+   while (!sensor_thread_quit)
+   umr_read_sensor(, AMDGPU_PP_SENSOR_GPU_POWER, 
gpu_power_data, );
+

Re: [PATCH] Add GPU_POWER sensors

2017-02-11 Thread Kai Wasserbäch 
Hey Tom,
Tom St Denis wrote on 11.02.2017 02:02:
> On 02/10/2017 07:25 PM, Edward O'Callaghan wrote:
>> Hey Tom,
>>
>> On 02/11/2017 05:10 AM, Tom St Denis wrote:
>>> Add the ability to sample GPU_POWER sensors.  Because
>>> the sensors have a high latency we read them from a background
>>> thread which means we've added the requirement for pthreads.
>>>
>>> Signed-off-by: Tom St Denis 
>>> ---
>>>  CMakeLists.txt |  5 ++-
>>>  README |  6 ++--
>>>  src/app/top.c  | 88 
>>> +-
>>>  src/lib/CMakeLists.txt |  1 +
>>>  src/lib/read_sensor.c  | 37 +
>>>  src/umr.h  |  5 +++
>>>  6 files changed, 123 insertions(+), 19 deletions(-)
>>>  create mode 100644 src/lib/read_sensor.c
>>>
>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>> index ef78c97ad763..7b771d01919b 100644
>>> --- a/CMakeLists.txt
>>> +++ b/CMakeLists.txt
>>> @@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
>>>  # Add local repository for FindXXX.cmake modules.
>>>  SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/"
>>> ${CMAKE_MODULE_PATH})
>>>
>>> +find_package(Threads REQUIRED)
>>> +include_directories(${THREADS_INCLUDE_DIRS})
>> Do you need this include_directories() line?
>>
>>> +
>>>  find_package(Curses REQUIRED)
>>>  include_directories(${CURSES_INCLUDE_DIRS})
>>>
>>> @@ -37,7 +40,7 @@ set(REQUIRED_EXTERNAL_LIBS
>>>  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
>>>
>>>  # CFLAGS += -Wall -W -O2 -g3 -Isrc/ -DPIC -fPIC
>>> -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -W -O2 -g3")
>>> +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pthread -Wall -W -O2 -g3")
>> You don't really want to have your linkage flags here, I think your
>> looking for ${CMAKE_THREAD_LIBS_INIT} to go into the
>> REQUIRED_EXTERNAL_LIBS list.
> 
> How does it "go into"?  Simple '+='?
> 
> Can you just send a quick patch I can squash into this?  The less I know about
> cmake the more room I have in my head for useful things :-) hehehe.

1. You might want to set
  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
at before the find_package() for Threads
2. Linking usually happens through target_link_*() calls, where you would add
Threads::Threads (special thing created by FindThreads.cmake) to the list of
your other variables.

See  for details on
how FindThreads.cmake works. In your case you might want to set
THREADS_PREFER_PTHREAD_FLAG as well.

Cheers,
Kai



signature.asc
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Add GPU_POWER sensors

2017-02-10 Thread Tom St Denis 

On 02/10/2017 07:25 PM, Edward O'Callaghan wrote:

Hey Tom,

On 02/11/2017 05:10 AM, Tom St Denis wrote:

Add the ability to sample GPU_POWER sensors.  Because
the sensors have a high latency we read them from a background
thread which means we've added the requirement for pthreads.

Signed-off-by: Tom St Denis 
---
 CMakeLists.txt |  5 ++-
 README |  6 ++--
 src/app/top.c  | 88 +-
 src/lib/CMakeLists.txt |  1 +
 src/lib/read_sensor.c  | 37 +
 src/umr.h  |  5 +++
 6 files changed, 123 insertions(+), 19 deletions(-)
 create mode 100644 src/lib/read_sensor.c

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ef78c97ad763..7b771d01919b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
 # Add local repository for FindXXX.cmake modules.
 SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/" 
${CMAKE_MODULE_PATH})

+find_package(Threads REQUIRED)
+include_directories(${THREADS_INCLUDE_DIRS})

Do you need this include_directories() line?


+
 find_package(Curses REQUIRED)
 include_directories(${CURSES_INCLUDE_DIRS})

@@ -37,7 +40,7 @@ set(REQUIRED_EXTERNAL_LIBS
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)

 # CFLAGS += -Wall -W -O2 -g3 -Isrc/ -DPIC -fPIC
-set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -W -O2 -g3")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pthread -Wall -W -O2 -g3")

You don't really want to have your linkage flags here, I think your
looking for ${CMAKE_THREAD_LIBS_INIT} to go into the
REQUIRED_EXTERNAL_LIBS list.


How does it "go into"?  Simple '+='?

Can you just send a quick patch I can squash into this?  The less I know 
about cmake the more room I have in my head for useful things :-) hehehe.



 enum drm_print {
@@ -171,19 +173,14 @@ static struct umr_bitfield stat_uvd_pgfsm7_bits[] = {
 static struct umr_bitfield stat_mc_hub_bits[] = {
 { "OUTSTANDING_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDRET", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRRET", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_RPB_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_MCD_ATOMIC", 255, 255, _bitfield_default },
 { NULL, 0, 0, NULL },
 };

this hulk seems unrelated to this patch?


It is, but since there's no functional change I figured I'd just squash 
that in as house keeping.


Cheers,
tom
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Add GPU_POWER sensors

2017-02-10 Thread Edward O'Callaghan 
Hey Tom,

On 02/11/2017 05:10 AM, Tom St Denis wrote:
> Add the ability to sample GPU_POWER sensors.  Because
> the sensors have a high latency we read them from a background
> thread which means we've added the requirement for pthreads.
> 
> Signed-off-by: Tom St Denis 
> ---
>  CMakeLists.txt |  5 ++-
>  README |  6 ++--
>  src/app/top.c  | 88 
> +-
>  src/lib/CMakeLists.txt |  1 +
>  src/lib/read_sensor.c  | 37 +
>  src/umr.h  |  5 +++
>  6 files changed, 123 insertions(+), 19 deletions(-)
>  create mode 100644 src/lib/read_sensor.c
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index ef78c97ad763..7b771d01919b 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
>  # Add local repository for FindXXX.cmake modules.
>  SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/" 
> ${CMAKE_MODULE_PATH})
>  
> +find_package(Threads REQUIRED)
> +include_directories(${THREADS_INCLUDE_DIRS})
Do you need this include_directories() line?

> +
>  find_package(Curses REQUIRED)
>  include_directories(${CURSES_INCLUDE_DIRS})
>  
> @@ -37,7 +40,7 @@ set(REQUIRED_EXTERNAL_LIBS
>  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
>  
>  # CFLAGS += -Wall -W -O2 -g3 -Isrc/ -DPIC -fPIC
> -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -W -O2 -g3")
> +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pthread -Wall -W -O2 -g3")
You don't really want to have your linkage flags here, I think your
looking for ${CMAKE_THREAD_LIBS_INIT} to go into the
REQUIRED_EXTERNAL_LIBS list.

>  
>  add_subdirectory(src)
>  add_subdirectory(doc)
> diff --git a/README b/README
> index 13cdac663d20..8a8de8485ac7 100644
> --- a/README
> +++ b/README
> @@ -28,9 +28,9 @@ mailing list at:
>  Building
>  -
>  
> -To build umr you will need pciaccess, ncurses, and libdrm headers and
> -libraries.  Which are available in both Fedora and Ubuntu (as well as
> -other distributions).  To build umr:
> +To build umr you will need pciaccess, ncurses, libdrm, and pthread 
> +headers and libraries.  Which are available in both Fedora and Ubuntu 
> +(as well as other distributions).  To build umr:
>  
>  $ mkdir build && cd build/ && cmake ../
>  $ make
> diff --git a/src/app/top.c b/src/app/top.c
> index b081515a5b40..60f629d247f3 100644
> --- a/src/app/top.c
> +++ b/src/app/top.c
> @@ -54,6 +54,7 @@ enum sensor_maps {
>   SENSOR_IDENTITY=0, // x = x
>   SENSOR_D1000,// x = x/1000
>   SENSOR_D100,// x = x/100
> + SENSOR_WATT,
>  };
>  
>  enum sensor_print {
> @@ -61,6 +62,7 @@ enum sensor_print {
>   SENSOR_MHZ,
>   SENSOR_PERCENT,
>   SENSOR_TEMP,
> + SENSOR_POWER,
>  };
>  
>  enum drm_print {
> @@ -171,19 +173,14 @@ static struct umr_bitfield stat_uvd_pgfsm7_bits[] = {
>  static struct umr_bitfield stat_mc_hub_bits[] = {
>{ "OUTSTANDING_READ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_WRITE", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_ATOMIC", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_RDREQ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_RDRET", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_WRREQ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_HUB_WRRET", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, _bitfield_default },
>{ "OUTSTANDING_RPB_READ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_RPB_WRITE", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_RPB_ATOMIC", 255, 255, _bitfield_default },
>{ "OUTSTANDING_MCD_READ", 255, 255, _bitfield_default },
>{ "OUTSTANDING_MCD_WRITE", 255, 255, _bitfield_default },
> -//{ "OUTSTANDING_MCD_ATOMIC", 255, 255, _bitfield_default },
>{ NULL, 0, 0, NULL },
>  };
this hulk seems unrelated to this patch?


With those fixes,
Reviewed-by: Edward O'Callaghan 

>  
> @@ -227,6 +224,10 @@ static struct umr_bitfield stat_vi_sensor_bits[] = {
>   { "GFX_MCLK", AMDGPU_PP_SENSOR_GFX_MCLK, SENSOR_D100|(SENSOR_MHZ<<4), 
> _bitfield_default },
>   { "GPU_LOAD", AMDGPU_PP_SENSOR_GPU_LOAD, SENSOR_PERCENT<<4, 
> _bitfield_default },
>   { "GPU_TEMP", AMDGPU_PP_SENSOR_GPU_TEMP, SENSOR_D1000|(SENSOR_TEMP<<4), 
> _bitfield_default },
> + { "VDDC", AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
> + { "VDDCI",AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
> + { "MAX_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
> + { "AVG_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
> SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
>   { NULL, 0, 0, NULL },
>  };
>  
> @@ -256,6 +257,21 @@ static struct

[PATCH] Add GPU_POWER sensors

2017-02-10 Thread Tom St Denis 
Add the ability to sample GPU_POWER sensors.  Because
the sensors have a high latency we read them from a background
thread which means we've added the requirement for pthreads.

Signed-off-by: Tom St Denis 
---
 CMakeLists.txt |  5 ++-
 README |  6 ++--
 src/app/top.c  | 88 +-
 src/lib/CMakeLists.txt |  1 +
 src/lib/read_sensor.c  | 37 +
 src/umr.h  |  5 +++
 6 files changed, 123 insertions(+), 19 deletions(-)
 create mode 100644 src/lib/read_sensor.c

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ef78c97ad763..7b771d01919b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,9 @@ add_definitions(-DUMR_BUILD_REV=\"${GIT_REV}\")
 # Add local repository for FindXXX.cmake modules.
 SET(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules/" 
${CMAKE_MODULE_PATH})
 
+find_package(Threads REQUIRED)
+include_directories(${THREADS_INCLUDE_DIRS})
+
 find_package(Curses REQUIRED)
 include_directories(${CURSES_INCLUDE_DIRS})
 
@@ -37,7 +40,7 @@ set(REQUIRED_EXTERNAL_LIBS
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 
 # CFLAGS += -Wall -W -O2 -g3 -Isrc/ -DPIC -fPIC
-set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -W -O2 -g3")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pthread -Wall -W -O2 -g3")
 
 add_subdirectory(src)
 add_subdirectory(doc)
diff --git a/README b/README
index 13cdac663d20..8a8de8485ac7 100644
--- a/README
+++ b/README
@@ -28,9 +28,9 @@ mailing list at:
 Building
 -
 
-To build umr you will need pciaccess, ncurses, and libdrm headers and
-libraries.  Which are available in both Fedora and Ubuntu (as well as
-other distributions).  To build umr:
+To build umr you will need pciaccess, ncurses, libdrm, and pthread 
+headers and libraries.  Which are available in both Fedora and Ubuntu 
+(as well as other distributions).  To build umr:
 
 $ mkdir build && cd build/ && cmake ../
 $ make
diff --git a/src/app/top.c b/src/app/top.c
index b081515a5b40..60f629d247f3 100644
--- a/src/app/top.c
+++ b/src/app/top.c
@@ -54,6 +54,7 @@ enum sensor_maps {
SENSOR_IDENTITY=0, // x = x
SENSOR_D1000,// x = x/1000
SENSOR_D100,// x = x/100
+   SENSOR_WATT,
 };
 
 enum sensor_print {
@@ -61,6 +62,7 @@ enum sensor_print {
SENSOR_MHZ,
SENSOR_PERCENT,
SENSOR_TEMP,
+   SENSOR_POWER,
 };
 
 enum drm_print {
@@ -171,19 +173,14 @@ static struct umr_bitfield stat_uvd_pgfsm7_bits[] = {
 static struct umr_bitfield stat_mc_hub_bits[] = {
 { "OUTSTANDING_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_RDRET", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRREQ", 255, 255, _bitfield_default },
 { "OUTSTANDING_HUB_WRRET", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_RPB_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_RPB_ATOMIC", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_READ", 255, 255, _bitfield_default },
 { "OUTSTANDING_MCD_WRITE", 255, 255, _bitfield_default },
-//  { "OUTSTANDING_MCD_ATOMIC", 255, 255, _bitfield_default },
 { NULL, 0, 0, NULL },
 };
 
@@ -227,6 +224,10 @@ static struct umr_bitfield stat_vi_sensor_bits[] = {
{ "GFX_MCLK", AMDGPU_PP_SENSOR_GFX_MCLK, SENSOR_D100|(SENSOR_MHZ<<4), 
_bitfield_default },
{ "GPU_LOAD", AMDGPU_PP_SENSOR_GPU_LOAD, SENSOR_PERCENT<<4, 
_bitfield_default },
{ "GPU_TEMP", AMDGPU_PP_SENSOR_GPU_TEMP, SENSOR_D1000|(SENSOR_TEMP<<4), 
_bitfield_default },
+   { "VDDC", AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
+   { "VDDCI",AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
+   { "MAX_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
+   { "AVG_GPU",  AMDGPU_PP_SENSOR_GPU_POWER, 
SENSOR_WATT|(SENSOR_POWER<<4), _bitfield_default },
{ NULL, 0, 0, NULL },
 };
 
@@ -256,6 +257,21 @@ static struct umr_bitfield stat_drm_bits[] = {
 
 static FILE *logfile = NULL;
 
+static volatile int sensor_thread_quit = 0;
+static uint32_t gpu_power_data[4];
+static void *vi_sensor_thread(void *data)
+{
+   struct umr_asic asic = *((struct umr_asic*)data);
+   int size = sizeof(gpu_power_data);
+   char fname[128];
+
+   snprintf(fname, sizeof(fname)-1, 
"/sys/kernel/debug/dri/%d/amdgpu_sensors", asic.instance);
+   asic.fd.sensors = open(fname, O_RDWR);
+   while (!sensor_thread_quit)
+   umr_read_sensor(,