[PATCH] Add GPU_POWER sensors (v2)

[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 <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