[PATCH umr] read all sensors in a thread

[Tom St Denis]

On 27/03/17 09:41 AM, Tom St Denis wrote:
> Eventually the GPU_POWER sensor will be rated at around
> 50Hz but that's still too slow to read from the main loop of
> umr's --top so we move all sensor operations to a thread.
>
> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
> ---
>  src/app/top.c | 110 +++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 55 insertions(+), 55 deletions(-)
>
> diff --git a/src/app/top.c b/src/app/top.c
> index 74ae5dfbb98a..dedad93f029c 100644
> --- a/src/app/top.c
> +++ b/src/app/top.c
> @@ -279,17 +279,35 @@ 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)
> +static volatile uint32_t gpu_power_data[32];
> +static volatile struct umr_bitfield *sensor_bits = NULL;
> +static void *gpu_sensor_thread(void *data)
>  {
>  	struct umr_asic asic = *((struct umr_asic*)data);
> -	int size = sizeof(gpu_power_data);
> +	int size, rem, off, x;
>  	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);
> +	while (!sensor_thread_quit) {
> +		rem = sizeof gpu_power_data;
> +		off = 0;
> +		for (x = 0; sensor_bits[x].regname; ) {
> +			switch (sensor_bits[x].start) {
> +				case AMDGPU_PP_SENSOR_GPU_POWER:
> +					size = 16;
> +					break;
> +				default:
> +					size = 4;
> +					break;
> +			}
> +			if (size < rem)

Not that this will manifest yet but that should be size <= rem.  Assume 
this is fixed locally when reviewing please.

Tom


> +				umr_read_sensor(&asic, sensor_bits[x].start, (uint32_t*)&gpu_power_data[off], &size);
> +			off += size / 4;
> +			rem -= size;
> +			x   += size / 4;
> +		}
> +	}
>  	close(asic.fd.sensors);
>  	return NULL;
>  }
> @@ -536,8 +554,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, size, x;
> -	uint32_t value[16];
> +	int j, x;
> +	uint32_t value;
>
>  	(void)addr;
>  	(void)mask;
> @@ -547,42 +565,29 @@ 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; ) {
> -		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;
> -					case SENSOR_D1000:
> -						counts[j] = value[x] / 1000;
> -						break;
> -					case SENSOR_D100:
> -						counts[j] = value[x] / 100;
> -						break;
> -					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;
> +	for (x = j = 0; bits[j].regname; ) {
> +		value = gpu_power_data[x];
> +		switch (bits[j].stop & 0x0F) {
> +			case SENSOR_IDENTITY:
> +				counts[j] = value;
> +				break;
> +			case SENSOR_D1000:
> +				counts[j] = value / 1000;
> +				break;
> +			case SENSOR_D100:
> +				counts[j] = value / 100;
> +				break;
> +			case SENSOR_WATT:
> +				counts[j] = ((value >> 8) * 1000);
> +				if ((value & 0xFF) < 100)
> +					counts[j] += (value & 0xFF) * 10;
> +				else
> +					counts[j] += value;
> +				counts[j] /= 10; // convert to centiwatts since we don't need 3 digits of excess precision
> +				break;
>  		}
> +		++j;
> +		++x;
>  	}
>  }
>
> @@ -805,6 +810,7 @@ static void top_build_vi_program(struct umr_asic *asic)
>  		// SI
>  		ENTRY_SENSOR(i++, "GFX_SCLK", &stat_si_sensor_bits[0], &top_options.vi.sensors, "Sensors");
>  	}
> +	sensor_bits = stat_counters[i-1].bits;
>
>  	// More GFX bits
>  	ENTRY(i++, "mmTA_STATUS", &stat_ta_bits[0], &top_options.vi.ta, "TA");
> @@ -899,7 +905,7 @@ static void toggle_logger(void)
>
>  void umr_top(struct umr_asic *asic)
>  {
> -	int i, j, k, use_thread;
> +	int i, j, k;
>  	struct timespec req;
>  	uint32_t rep;
>  	time_t tt;
> @@ -927,15 +933,11 @@ void umr_top(struct umr_asic *asic)
>  			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;
> +	// start thread to grab sensor data
> +	if (pthread_create(&sensor_thread, NULL, gpu_sensor_thread, asic)) {
> +		fprintf(stderr, "[ERROR] Cannot create gpu_sensor_thread\n");
> +		return;
>  	}
>
>  	initscr();
> @@ -1068,8 +1070,6 @@ void umr_top(struct umr_asic *asic)
>  	}
>  	endwin();
>
> -	if (use_thread) {
> -		sensor_thread_quit = 1;
> -		pthread_join(sensor_thread, NULL);
> -	}
> +	sensor_thread_quit = 1;
> +	pthread_join(sensor_thread, NULL);
>  }
>