[PATCH i-g-t v11 5/5] tools/gputop/gputop: Enable support for multiple GPUs and instances
Riana Tauro
riana.tauro at intel.com
Wed Jun 11 07:24:32 UTC 2025
Hi Soham
On 6/11/2025 12:41 PM, Purkait, Soham wrote:
>
> On 05-06-2025 22:27, Riana Tauro wrote:
>> Hi Soham
>>
>> Code looks good. Have one question and few minor comments below
>>
>> On 5/22/2025 9:14 PM, Soham Purkait wrote:
>>> Introduce vendor-agnostic support for
>>> handling multiple GPUs and instances
>>> in gputop. Improve the tool's adaptability
>>> to various GPU configurations.
>>>
>>> v2 : Fix for refactoring GPUTOP into a
>>> vendor-agnostic tool. (Lucas)
>>>
>>> v3 : New year included in copyright. (Kamil, Riana)
>>> Removed caps in function name. (Riana)
>>> Struct for driver specific operations. (Riana)
>>> Headers in alphabetical order. (Kamil, Riana)
>>>
>>> v4 : Commit description and signed-off included.
>>>
>>> v5 : Fix for proper resource cleanup. (Riana)
>>> Use "dev_type" enum for card_type. (Krzysztof)
>>> Add new filter to return collection
>>> of matching devices. (Zbigniew)
>>>
>>> v6 : Use device filter to populate the array of
>>> cards for all supported drivers. (Zbigniew)
>>>
>>> v7 : Use filter to find all the cards. (Zbigniew)
>>>
>>> v8 : Removed 'drivers' array parameter from
>>> card match function. (Zbigniew)
>>>
>>> v10 : Resolved 'populate_devices' call with
>>> pci subsystem filtering. (Zbigniew)
>>>
>>> v11 : Add space after /* and before */ for better
>>> readability. (Zbigniew)
>>> Comments wrapped at 75/100. (Riana)
>>>
>>> Signed-off-by: Soham Purkait <soham.purkait at intel.com>
>>>
>>> Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>>> ---
>>> tools/{ => gputop}/gputop.c | 223 +++++++++++++++++++++++++++++-------
>>> tools/gputop/meson.build | 6 +
>>> tools/meson.build | 6 +-
>>> 3 files changed, 191 insertions(+), 44 deletions(-)
>>> rename tools/{ => gputop}/gputop.c (66%)
>>> create mode 100644 tools/gputop/meson.build
>>>
>>> diff --git a/tools/gputop.c b/tools/gputop/gputop.c
>>> similarity index 66%
>>> rename from tools/gputop.c
>>> rename to tools/gputop/gputop.c
>>> index 43b01f566..fc3ccb83b 100644
>>> --- a/tools/gputop.c
>>> +++ b/tools/gputop/gputop.c
>>> @@ -1,6 +1,6 @@
>>> // SPDX-License-Identifier: MIT
>>> /*
>>> - * Copyright © 2023 Intel Corporation
>>> + * Copyright © 2023-2025 Intel Corporation
>>> */
>>> #include <assert.h>
>>> @@ -14,66 +14,145 @@
>>> #include <math.h>
>>> #include <poll.h>
>>> #include <signal.h>
>>> +#include <stdbool.h>
>>> #include <stdint.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> +#include <sys/sysmacros.h>
>>
>> alphabetical
>>
>>> #include <sys/ioctl.h>
>>> #include <sys/stat.h>
>>> #include <sys/types.h>
>>> -#include <unistd.h>
>>> #include <termios.h>
>>> -#include <sys/sysmacros.h>
>>> -#include <stdbool.h>
>>> +#include <unistd.h>
>>> +#include "drmtest.h"
>>> #include "igt_core.h"
>>> #include "igt_drm_clients.h"
>>> #include "igt_drm_fdinfo.h"
>>> +#include "igt_perf.h"
>>> #include "igt_profiling.h"
>>> -#include "drmtest.h"
>>> +#include "xe_gputop.h"
>>> +#include "xe/xe_query.h"
>>> +
>>> +/**
>>> + * Supported Drivers
>>> + *
>>> + * Adhere to the following requirements when implementing support
>>> for the
>>> + * new driver:
>>> + * @drivers: Update drivers[] with driver string.
>>> + * @total_count: Update NUM_DRIVER with the total number of
>>> supported drivers.
>>> + * @operations: Update the respective operations of the new driver:
>>> + * gputop_init,
>>> + * discover_engines,
>>> + * pmu_init,
>>> + * pmu_sample,
>>> + * print_engines,
>>> + * clean_up
>>> + * @devices: Update devices[] array of type "struct gputop_device"
>>> with the
>>> + * initial values.
>>> + */
>>> +static const char * const drivers[] = {
>>> + "xe",
>>> + /* Keep the last one as NULL */
>>> + NULL
>>> +};
>>> +
>>> +/**
>>> + * Number of supported drivers needs to be adjusted as per the
>>> length of
>>> + * the drivers[] array.
>>> + */
>>> +#define NUM_DRIVER 1
>>> +
>>> +/**
>>> + * Supported operations on driver instances. Update the oprs[] array
>>> for
>>> + * each individual driver specific function. Maintain the sequence
>>> as per
>>> + * drivers[] array.
>>> + */
>>> +struct device_operations oprs[NUM_DRIVER] = {
>>
>> Replace oprs with ops.. generally use ops for operations
> Isn't ops more sound like options ?
Some examples like fops, pm_ops in kernel are for operations
Trivial comment upto you
>>> + {
>>> + xe_gputop_init,
>>> + xe_populate_engines,
>>> + xe_pmu_init,
>>> + xe_pmu_sample,
>>> + xe_print_engines,
>>> + xe_clean_up
>>> + }
>>> +};
>>> +
>>> +/*
>>> + * devices[] array of type struct gputop_device
>>> + */
>>> +struct gputop_device devices[] = {
>>
>> I must have commented here before. As i see this is one entry per
>> driver right? why is it named device?
>
> Actually this is keeping track of the devices for each driver, so the name.
yeah it tracks device count per driver. The array entries are per driver
not device. Each entry is not a gputop_device as the name suggests
>
> I have updated the description for better readability.
> >>
>>> + {false, 0, NULL}
>>> +};
>>> enum utilization_type {
>>> UTILIZATION_TYPE_ENGINE_TIME,
>>> UTILIZATION_TYPE_TOTAL_CYCLES,
>>> };
>>> -static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊",
>>> "▉", "█" };
>>> -
>>> -#define ANSI_HEADER "\033[7m"
>>> -#define ANSI_RESET "\033[0m"
>>> -
>>> -static void n_spaces(const unsigned int n)
>>> +static void gputop_clean_up(void)
>>> {
>>> - unsigned int i;
>>> -
>>> - for (i = 0; i < n; i++)
>>> - putchar(' ');
>>> + for (int i = 0; drivers[i]; i++) {
>>> + oprs[i].clean_up(devices[i].instances, devices[i].len);
>>> + free(devices[i].instances);
>>> + devices[i].driver_present = false;
>>> + devices[i].len = 0;
>>> + }
>>> }
>>> -static void print_percentage_bar(double percent, int max_len)
>>> +static int find_driver(struct igt_device_card *card)
>>> {
>>> - 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 (int i = 0; drivers[i]; i++) {
>>> + if (strcmp(drivers[i], card->driver) == 0)
>>> + return i;
>>> + }
>>> + return -1;
>>> +}
>>> - for (i = bar_len; i >= w; i -= w)
>>> - printf("%s", bars[w]);
>>> - if (i)
>>> - printf("%s", bars[i]);
>>> +/*
>>> + * If filter is not NULL i will be ignored.
>>> + */
>>> +static int populate_device_instances(const char *filter)
>>> +{
>>> + struct igt_device_card *cards = NULL;
>>> + struct igt_device_card *card_inplace = NULL;
>>> + struct gputop_device *dev = NULL;
>>> + int driver_no;
>>> + int count, final_count = 0;
>>
>> %s/final_count/card_count
> Here count and final_count both are keeping track of the cards but
> final_count is the count of supported filtered devices, so the name.
Trivial comment . upto you
>>
>>> +
>>> + count = igt_device_card_match_all(filter, &cards);
>>> + for (int j = 0; j < count; j++) {
>>> + if (strcmp((cards + j)->subsystem, "pci") != 0)
>>> + continue;
>>> - len -= (bar_len + (w - 1)) / w;
>>> - n_spaces(len);
>>> + driver_no = find_driver(cards + j);
>>> + if (driver_no < 0)
>>> + continue;
>>> - putchar('|');
>>> + dev = devices + driver_no;
>>> + if (!dev->driver_present)
>>> + dev->driver_present = true;
>>> + dev->len++;
>>> + dev->instances = realloc(dev->instances,
>>> + dev->len * sizeof(struct xe_gputop));
>>> + if (!dev->instances) {
>>> + fprintf(stderr,
>>> + "Device instance realloc failed (%s)\n",
>>> + strerror(errno));
>>> + exit(EXIT_FAILURE);
>>> + }
>>> + card_inplace = (struct igt_device_card *)
>>> + calloc(1, sizeof(struct igt_device_card));
>>> + memcpy(card_inplace, cards + j, sizeof(struct
>>> igt_device_card));
>>> + oprs[driver_no].gputop_init((struct xe_gputop *)(dev-
>>> >instances + dev->len - 1),
>>> + card_inplace);
>>> + final_count++;
>>> + }
>>> + if (count)
>>> + free(cards);
>>> + return final_count;
>>> }
>>> static int
>>> @@ -333,6 +412,7 @@ static void clrscr(void)
>>> struct gputop_args {
>>> long n_iter;
>>> unsigned long delay_usec;
>>> + char *device;
>>> };
>>> static void help(void)
>>> @@ -343,16 +423,18 @@ static void help(void)
>>> "\t-h, --help show this help\n"
>>> "\t-d, --delay =SEC[.TENTHS] iterative delay as SECS
>>> [.TENTHS]\n"
>>> "\t-n, --iterations =NUMBER number of executions\n"
>>> + "\t-D, --device Device filter\n"
>>> , program_invocation_short_name);
>>> }
>>> static int parse_args(int argc, char * const argv[], struct
>>> gputop_args *args)
>>> {
>>> - static const char cmdopts_s[] = "hn:d:";
>>> + static const char cmdopts_s[] = "hn:d:D:";
>>> static const struct option cmdopts[] = {
>>> {"help", no_argument, 0, 'h'},
>>> {"delay", required_argument, 0, 'd'},
>>> {"iterations", required_argument, 0, 'n'},
>>> + {"device", required_argument, 0, 'D'},
>>> { }
>>> };
>>> @@ -360,6 +442,7 @@ static int parse_args(int argc, char * const
>>> argv[], struct gputop_args *args)
>>> memset(args, 0, sizeof(*args));
>>> args->n_iter = -1;
>>> args->delay_usec = 2 * USEC_PER_SEC;
>>> + args->device = NULL;
>>> for (;;) {
>>> int c, idx = 0;
>>> @@ -383,6 +466,9 @@ static int parse_args(int argc, char * const
>>> argv[], struct gputop_args *args)
>>> return -1;
>>> }
>>> break;
>>> + case 'D':
>>> + args->device = optarg;
>>> + break;
>>> case 'h':
>>> help();
>>> return 0;
>>> @@ -422,6 +508,52 @@ int main(int argc, char **argv)
>>> n = args.n_iter;
>>> period_us = args.delay_usec;
>>> + if (!populate_device_instances(args.device ? args.device
>>> + : "device:subsystem=pci,card=all")) {
>>> + printf("No device found.\n");
>>> + gputop_clean_up();
>>> + exit(1);
>>> + }
>>> +
>>> + for (int i = 0; drivers[i]; i++) {
>>> + if (devices[i].driver_present) {
>>
>> if not present continue
> It will be at the end of the outer loop if not present so it eventually
> continues.
The comment was to reduce the indentation and improve readability
Thanks
Riana
>>>> Thanks
>> Riana
>>
>>> + for (int j = 0; j < devices[i].len; j++) {
>>> + if (!oprs[i].init_engines(devices[i].instances + j)) {
>>> + fprintf(stderr,
>>> + "Failed to initialize engines! (%s)\n",
>>> + strerror(errno));
>>> + gputop_clean_up();
>>> + return EXIT_FAILURE;
>>> + }
>>> + ret = oprs[i].pmu_init(devices[i].instances + j);
>>> +
>>> + if (ret) {
>>> + fprintf(stderr,
>>> + "Failed to initialize PMU! (%s)\n",
>>> + strerror(errno));
>>> + if (errno == EACCES && geteuid())
>>> + fprintf(stderr,
>>> + "\n"
>>> + "When running as a normal user
>>> CAP_PERFMON is required to access performance\n"
>>> + "monitoring. See \"man 7 capabilities\",
>>> \"man 8 setcap\", or contact your\n"
>>> + "distribution vendor for assistance.\n"
>>> + "\n"
>>> + "More information can be found at 'Perf
>>> events and tool security' document:\n"
>>> + "https://www.kernel.org/doc/html/latest/admin-guide/perf-
>>> security.html\n");
>>> +
>>> + igt_devices_free();
>>> + gputop_clean_up();
>>> + return EXIT_FAILURE;
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>> + for (int i = 0; drivers[i]; i++) {
>>> + for (int j = 0; devices[i].driver_present && j <
>>> devices[i].len; j++)
>>> + oprs[i].pmu_sample(devices[i].instances + j);
>>> + }
>>> +
>>> clients = igt_drm_clients_init(NULL);
>>> if (!clients)
>>> exit(1);
>>> @@ -442,14 +574,27 @@ int main(int argc, char **argv)
>>> while ((n != 0) && !stop_top) {
>>> struct igt_drm_client *c, *prevc = NULL;
>>> - int i, engine_w = 0, lines = 0;
>>> + int k, engine_w = 0, lines = 0;
>>> igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
>>> +
>>> + for (int i = 0; drivers[i]; i++) {
>>> + for (int j = 0; devices[i].driver_present && j <
>>> devices[i].len; j++)
>>> + oprs[i].pmu_sample(devices[i].instances + j);
>>> + }
>>> +
>>> igt_drm_clients_sort(clients, client_cmp);
>>> update_console_size(&con_w, &con_h);
>>> clrscr();
>>> + for (int i = 0; drivers[i]; i++) {
>>> + for (int j = 0; devices[i].driver_present && j <
>>> devices[i].len; j++) {
>>> + lines = oprs[i].print_engines(devices[i].instances + j,
>>> + lines, con_w, con_h);
>>> + }
>>> + }
>>> +
>>> if (!clients->num_clients) {
>>> const char *msg = " (No GPU clients yet. Start workload
>>> to see stats)";
>>> @@ -457,7 +602,7 @@ int main(int argc, char **argv)
>>> (int)(con_w - strlen(msg) - 1), msg);
>>> }
>>> - igt_for_each_drm_client(clients, c, i) {
>>> + igt_for_each_drm_client(clients, c, k) {
>>> assert(c->status != IGT_DRM_CLIENT_PROBE);
>>> if (c->status != IGT_DRM_CLIENT_ALIVE)
>>> break; /* Active clients are first in the array. */
>>> @@ -481,11 +626,11 @@ int main(int argc, char **argv)
>>> }
>>> igt_drm_clients_free(clients);
>>> + gputop_clean_up();
>>> if (profiled_devices != NULL) {
>>> igt_devices_configure_profiling(profiled_devices, false);
>>> igt_devices_free_profiling(profiled_devices);
>>> }
>>> -
>>> return 0;
>>> }
>>> diff --git a/tools/gputop/meson.build b/tools/gputop/meson.build
>>> new file mode 100644
>>> index 000000000..4766d8496
>>> --- /dev/null
>>> +++ b/tools/gputop/meson.build
>>> @@ -0,0 +1,6 @@
>>> +gputop_src = [ 'gputop.c', 'utils.c', 'xe_gputop.c']
>>> +executable('gputop', sources : gputop_src,
>>> + install : true,
>>> + install_rpath : bindir_rpathdir,
>>> + dependencies :
>>> [igt_deps,lib_igt_perf,lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math],
>>> + install: true)
>>> diff --git a/tools/meson.build b/tools/meson.build
>>> index de866c392..8002f707d 100644
>>> --- a/tools/meson.build
>>> +++ b/tools/meson.build
>>> @@ -69,11 +69,6 @@ if libudev.found()
>>> install : true)
>>> endif
>>> -executable('gputop', 'gputop.c',
>>> - install : true,
>>> - install_rpath : bindir_rpathdir,
>>> - dependencies :
>>> [lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
>>> -
>>> intel_l3_parity_src = [ 'intel_l3_parity.c',
>>> 'intel_l3_udev_listener.c' ]
>>> executable('intel_l3_parity', sources : intel_l3_parity_src,
>>> dependencies : tool_deps,
>>> @@ -122,3 +117,4 @@ endif
>>> subdir('i915-perf')
>>> subdir('xe-perf')
>>> subdir('null_state_gen')
>>> +subdir('gputop')
>>
More information about the igt-dev
mailing list