[PATCH] Add GPU_POWER sensors (v2)

Tom St Denis tom.stdenis at amd.com
Sat Feb 11 17:38:10 UTC 2017


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 <tom.stdenis at amd.com>
>>
>> (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, &umr_bitfield_default },
>>  	 { "OUTSTANDING_WRITE", 255, 255, &umr_bitfield_default },
>> -//	 { "OUTSTANDING_ATOMIC", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_HUB_RDREQ", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_HUB_RDRET", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_HUB_WRREQ", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_HUB_WRRET", 255, 255, &umr_bitfield_default },
>> -//	 { "OUTSTANDING_HUB_ATOMIC_REQ", 255, 255, &umr_bitfield_default },
>> -//	 { "OUTSTANDING_HUB_ATOMIC_RET", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_RPB_READ", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_RPB_WRITE", 255, 255, &umr_bitfield_default },
>> -//	 { "OUTSTANDING_RPB_ATOMIC", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_MCD_READ", 255, 255, &umr_bitfield_default },
>>  	 { "OUTSTANDING_MCD_WRITE", 255, 255, &umr_bitfield_default },
>> -//	 { "OUTSTANDING_MCD_ATOMIC", 255, 255, &umr_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


More information about the amd-gfx mailing list