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 <tom.stdenis at amd.com>
> ---
> 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, &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 },
> };
this hulk seems unrelated to this patch?
With those fixes,
Reviewed-by: Edward O'Callaghan <funfunctor at folklore1984.net>
>
> @@ -227,6 +224,10 @@ static struct umr_bitfield stat_vi_sensor_bits[] = {
> { "GFX_MCLK", AMDGPU_PP_SENSOR_GFX_MCLK, SENSOR_D100|(SENSOR_MHZ<<4), &umr_bitfield_default },
> { "GPU_LOAD", AMDGPU_PP_SENSOR_GPU_LOAD, SENSOR_PERCENT<<4, &umr_bitfield_default },
> { "GPU_TEMP", AMDGPU_PP_SENSOR_GPU_TEMP, SENSOR_D1000|(SENSOR_TEMP<<4), &umr_bitfield_default },
> + { "VDDC", AMDGPU_PP_SENSOR_GPU_POWER, SENSOR_WATT|(SENSOR_POWER<<4), &umr_bitfield_default },
> + { "VDDCI", AMDGPU_PP_SENSOR_GPU_POWER, SENSOR_WATT|(SENSOR_POWER<<4), &umr_bitfield_default },
> + { "MAX_GPU", AMDGPU_PP_SENSOR_GPU_POWER, SENSOR_WATT|(SENSOR_POWER<<4), &umr_bitfield_default },
> + { "AVG_GPU", AMDGPU_PP_SENSOR_GPU_POWER, SENSOR_WATT|(SENSOR_POWER<<4), &umr_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(&asic, AMDGPU_PP_SENSOR_GPU_POWER, gpu_power_data, &size);
> + close(asic.fd.sensors);
> + return NULL;
> +}
>
> static unsigned long last_fence_emitted, last_fence_signaled, fence_signal_count, fence_emit_count;
> static void analyze_fence_info(struct umr_asic *asic)
> @@ -428,6 +444,9 @@ static void print_sensors(struct umr_bitfield *bits, uint64_t *counts)
> case SENSOR_TEMP:
> printw("%5d C ", counts[i]);
> break;
> + case SENSOR_POWER:
> + printw("%3d.%02d W ", counts[i]/100, counts[i]%100);
> + break;
> };
> if ((++print_j & (top_options.wide ? 3 : 1)) != 0)
> printw(" |");
> @@ -496,8 +515,8 @@ static void parse_bits(struct umr_asic *asic, uint32_t addr, struct umr_bitfield
>
> static void parse_sensors(struct umr_asic *asic, uint32_t addr, struct umr_bitfield *bits, uint64_t *counts, uint32_t *mask, uint32_t *cmp, uint64_t addr_mask)
> {
> - int j;
> - int32_t value;
> + int j, size, x;
> + uint32_t value[16];
>
> (void)addr;
> (void)mask;
> @@ -507,13 +526,34 @@ static void parse_sensors(struct umr_asic *asic, uint32_t addr, struct umr_bitfi
> if (asic->fd.sensors < 0)
> return;
>
> - for (j = 0; bits[j].regname; j++) {
> - lseek(asic->fd.sensors, bits[j].start * 4, SEEK_SET);
> - read(asic->fd.sensors, &value, 4);
> - switch (bits[j].stop & 0x0F) {
> - case SENSOR_IDENTITY: counts[j] = value; break; // identity
> - case SENSOR_D1000: counts[j] = value/1000; break; // divide by 1000 (e.g. KHz => MHz)
> - case SENSOR_D100: counts[j] = value/100; break; // divide by 100 (e.g. 10KHz => MHz)
> + for (j = 0; bits[j].regname; ) {
> + size = 4;
> + if (bits[j].start == AMDGPU_PP_SENSOR_GPU_POWER || !umr_read_sensor(asic, bits[j].start, &value[0], &size)) {
> + x = 0;
> + if (bits[j].start == AMDGPU_PP_SENSOR_GPU_POWER) {
> + size = 4 * sizeof(uint32_t);
> + memcpy(value, gpu_power_data, size);
> + }
> +
> + while (size) {
> + switch (bits[j].stop & 0x0F) {
> + case SENSOR_IDENTITY: counts[j] = value[x]; break; // identity
> + case SENSOR_D1000: counts[j] = value[x]/1000; break; // divide by 1000 (e.g. KHz => MHz)
> + case SENSOR_D100: counts[j] = value[x]/100; break; // divide by 100 (e.g. 10KHz => MHz)
> + case SENSOR_WATT: counts[j] = ((value[x]>>8)*1000);
> + if ((value[x]&0xFF) < 100)
> + counts[j] += (value[x]&0xFF)*10;
> + else
> + counts[j] += value[x];
> + counts[j] /= 10; // convert to centiwatts since we don't need 3 digits of excess precision
> + break;
> + }
> + size -= 4;
> + ++j;
> + ++x;
> + }
> + } else {
> + ++j;
> }
> }
> }
> @@ -818,12 +858,13 @@ static void toggle_logger(void)
>
> void umr_top(struct umr_asic *asic)
> {
> - int i, j, k;
> + int i, j, k, use_thread;
> struct timespec req;
> uint32_t rep;
> time_t tt;
> uint64_t ts;
> char hostname[64] = { 0 };
> + pthread_t sensor_thread;
>
> if (getenv("HOSTNAME")) strcpy(hostname, getenv("HOSTNAME"));
>
> @@ -844,6 +885,18 @@ void umr_top(struct umr_asic *asic)
> if (stat_counters[i].is_sensor == 0)
> grab_bits(stat_counters[i].name, asic, stat_counters[i].bits, &stat_counters[i].addr);
>
> + sensor_thread_quit = 0;
> + use_thread = 0;
> +
> + // start thread to grab sensor data for VI
> + if (asic->family == FAMILY_VI) {
> + if (pthread_create(&sensor_thread, NULL, vi_sensor_thread, asic)) {
> + fprintf(stderr, "[ERROR] Cannot create vi_sensor_thread\n");
> + return;
> + }
> + use_thread = 1;
> + }
> +
> initscr();
> start_color();
> cbreak();
> @@ -973,4 +1026,9 @@ void umr_top(struct umr_asic *asic)
> refresh();
> }
> endwin();
> +
> + if (use_thread) {
> + sensor_thread_quit = 1;
> + pthread_join(sensor_thread, NULL);
> + }
> }
> diff --git a/src/lib/CMakeLists.txt b/src/lib/CMakeLists.txt
> index 46c75d61acb4..a5f0bda38689 100644
> --- a/src/lib/CMakeLists.txt
> +++ b/src/lib/CMakeLists.txt
> @@ -14,6 +14,7 @@ add_library(umrcore STATIC
> find_reg.c
> mmio.c
> query_drm.c
> + read_sensor.c
> read_sgpr.c
> read_vram.c
> ring_decode.c
> diff --git a/src/lib/read_sensor.c b/src/lib/read_sensor.c
> new file mode 100644
> index 000000000000..c4907f83208f
> --- /dev/null
> +++ b/src/lib/read_sensor.c
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright 2017 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Tom St Denis <tom.stdenis at amd.com>
> + *
> + */
> +#include "umr.h"
> +
> +int umr_read_sensor(struct umr_asic *asic, int sensor, void *dst, int *size)
> +{
> + int r;
> + lseek(asic->fd.sensors, sensor*4, SEEK_SET);
> + r = read(asic->fd.sensors, dst, *size);
> + if (r != *size) {
> + return -1;
> + }
> + *size = r;
> + return 0;
> +}
> diff --git a/src/umr.h b/src/umr.h
> index f2bce13d104b..34e8809eb415 100644
> --- a/src/umr.h
> +++ b/src/umr.h
> @@ -31,6 +31,7 @@
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <pciaccess.h>
> +#include <pthread.h>
>
> /* sourced from amd_powerplay.h from the kernel */
> enum amd_pp_sensors {
> @@ -43,6 +44,9 @@ enum amd_pp_sensors {
> AMDGPU_PP_SENSOR_GPU_LOAD,
> AMDGPU_PP_SENSOR_GFX_MCLK,
> AMDGPU_PP_SENSOR_GPU_TEMP,
> + AMDGPU_PP_SENSOR_VCE_POWER,
> + AMDGPU_PP_SENSOR_UVD_POWER,
> + AMDGPU_PP_SENSOR_GPU_POWER,
> };
>
> enum chipfamily {
> @@ -409,6 +413,7 @@ void umr_enumerate_devices(void);
> int umr_get_wave_status(struct umr_asic *asic, unsigned se, unsigned sh, unsigned cu, unsigned simd, unsigned wave, struct umr_wave_status *ws);
> int umr_get_wave_sq_info(struct umr_asic *asic, unsigned se, unsigned sh, unsigned cu, struct umr_wave_status *ws);
> int umr_read_sgprs(struct umr_asic *asic, struct umr_wave_status *ws, uint32_t *dst);
> +int umr_read_sensor(struct umr_asic *asic, int sensor, void *dst, int *size);
>
> /* mmio helpers */
> uint32_t umr_find_reg(struct umr_asic *asic, char *regname);
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170211/7013e36e/attachment-0001.sig>
