[PATCH] Add GPU_POWER sensors (v2)

Kai Wasserb├Ąch kai at dev.carbon-project.org
Sat Feb 11 13:29:37 UTC 2017


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.

>      $ 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.

> @@ -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);

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

case SENSOR_IDENTITY:
	counts[j] = value[x];
	break;
case SENSOR_D1000: // divide by 1000 (e.g. KHz => MHz)
	counts[j] = value[x] / 1000;
	break;
case SENSOR_D100: // divide by 100 (e.g. 10KHz => MHz)
	counts[j] = value[x] / 100;
	break;
case SENSOR_WATT:
	counts[j] = ((value[x] >> 8) * 1000);
	[...]

(same for the rest of SENSOR_WATT)

> +								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);
> 

With those changes, this patch is
  Reviewed-by: Kai Wasserb├Ąch <kai at dev.carbon-project.org>

Cheers,
Kai

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170211/f0f9b031/attachment.sig>


More information about the amd-gfx mailing list