Re: [PATCH i-g-t v3 2/4] Add gputop functionality common to all drivers

[Riana Tauro <riana.tauro@intel.com>]

Hi Soham

Add description for each patch

On 2/28/2025 7:48 PM, Soham Purkait wrote:
> v2 : fix for refactoring GPUTOP into a
>       vendor-agnostic tool (Lucas)
> 
> v3 : Headers in alphabetical order (Kamil, Riana)
missing signed-off-by

use git commit -s
> 
> ---
>   tools/gputop/common_gputop.c | 79 ++++++++++++++++++++++++++++++++++++
>   tools/gputop/common_gputop.h | 58 ++++++++++++++++++++++++++
>   2 files changed, 137 insertions(+)
>   create mode 100644 tools/gputop/common_gputop.c
>   create mode 100644 tools/gputop/common_gputop.h
> 
> diff --git a/tools/gputop/common_gputop.c b/tools/gputop/common_gputop.c
> new file mode 100644
> index 000000000..578bd0a01
> --- /dev/null
> +++ b/tools/gputop/common_gputop.c
since this file has formatting and printing related functions.
different name?
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +#include <assert.h>
> +
> +#include "common_gputop.h"
> +
> +static const char * const bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> +
> +/* tr_pmu_name()
> + *
> + * Transliterate pci_slot_id to sysfs device name entry for GPU
> + * from type struct igt_device_card.
> + * GPU PCI ID   ("xxxx:yy:zz.z")       device = "xe_xxxx_yy_zz.z".
> + */
> +char *tr_pmu_name(const struct igt_device_card *card)
> +{
> +	int ret;
> +	const int bufsize = 16;
> +	char *buf, *device = NULL;
> +
> +	assert(card->pci_slot_name[0]);
> +
> +	device = malloc(bufsize);
> +	assert(device);
> +
> +	ret = snprintf(device, bufsize, "xe_%s", card->pci_slot_name);
this file is named common but has "xe" in this function

you can use below from lib/igt_perf.h

const char *xe_perf_device(int xe, char *buf, int buflen)
const char *i915_perf_device(int i915, char *buf, int buflen)

or add a common function in igt_perf and re-use

> +	assert(ret == (bufsize - 1));
> +
> +	buf = device;
> +	for (; *buf; buf++)
> +		if (*buf == ':')
> +			*buf = '_';
> +
> +	return device;
> +}
> +
> +void n_spaces(const unsigned int n)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < n; i++)
> +		putchar(' ');
> +}
> +
> +void print_percentage_bar(double percent, int max_len)
> +{
> +	int bar_len, i, len = max_len - 1;
> +	const int w = 8;
> +
> +	len -= printf("|%5.1f%% ", percent);
> +
> +	/* no space left for bars, do what we can */
> +	if (len < 0)
> +		len = 0;
> +
> +	bar_len = ceil(w * percent * len / 100.0);
> +	if (bar_len > w * len)
> +		bar_len = w * len;
> +
> +	for (i = bar_len; i >= w; i -= w)
> +		printf("%s", bars[w]);
> +	if (i)
> +		printf("%s", bars[i]);
> +
> +	len -= (bar_len + (w - 1)) / w;
> +	n_spaces(len);
> +
> +	putchar('|');
> +}
> +
> +int print_engines_footer(int lines, int con_w, int con_h)
> +{
> +	if (lines++ < con_h)
> +		printf("\n");
> +
> +	return lines;
> +}
> diff --git a/tools/gputop/common_gputop.h b/tools/gputop/common_gputop.h
> new file mode 100644
> index 000000000..4e6f632af
> --- /dev/null
> +++ b/tools/gputop/common_gputop.h
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef COMMON_GPUTOP_H
> +#define COMMON_GPUTOP_H
> +
> +#include <math.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include "igt_device_scan.h"
> +
> +#define ANSI_HEADER "\033[7m"
> +#define ANSI_RESET "\033[0m"
> +
> +/**
> + * struct gputop_device
> + *
> + * @driver_present: It is set if at least a
> + * single device found of the respective driver
> + * @len: Number of total device discovered
> + * of the respective driver
> + * @instances: pointer to the array of
> + * discovered instances of the devices
> + * of the same driver
> + */
> +struct gputop_device {
> +	bool driver_present;
> +	int len;
> +	void *instances;
> +};
> +
> +/**
> + * struct device_operations - Structure to hold function
> + * pointers for device specific operations for each individual driver.
> + * @populate_device_instances: Function to populate device instances.
> + * @discover_engines: Function to discover engines.
> + * @pmu_init: Function to initialize the PMU (Performance Monitoring Unit).
> + * @pmu_sample: Function to sample PMU data.
> + * @print_engines: Function to print engine business.
> + */
> +struct device_operations {
> +	void (*populate_device_instances)(struct gputop_device *dv);
%s/dv/dev or device
> +	void *(*discover_engines)(const void *obj);
> +	int (*pmu_init)(const void *obj);
> +	void (*pmu_sample)(const void *obj);
> +	int (*print_engines)(const void *obj, int lines, int w, int h);
> +};

Why not combine both? Both are device specific.

Thanks
Riana Tauro
> +
> +void print_percentage_bar(double percent, int max_len);
> +int print_engines_footer(int lines, int con_w, int con_h);
> +void n_spaces(const unsigned int n);
> +char *tr_pmu_name(const struct igt_device_card *card);
> +
> +#endif // COMMON_GPUTOP_H