Re: [PATCH i-g-t v11 5/5] tools/gputop/gputop: Enable support for multiple GPUs and instances

["Purkait, Soham" <soham.purkait@intel.com>]

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@intel.com>
>>
>> Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@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 ?
>> +    {
>> +        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.

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.
>
>> +
>> +    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.
>
> 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')
>