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
