On Mon, Nov 16, 2020 at 05:26:52PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> In the previous patch we switched the lsgpu output to a short and user
> friendly format but some users will need a shorthand for getting other
> types of device selection filters than the defaut drm.
>
> Add some command line switches to enable this:
>
> $ lsgpu
> card0 8086:193B drm:/dev/dri/card0
> └─renderD128 drm:/dev/dri/renderD128
>
> $ lsgpu --sysfs
> card0 8086:193B sys:/sys/devices/pci0000:00/0000:00:02.0/drm/card0
> └─renderD128 sys:/sys/devices/pci0000:00/0000:00:02.0/drm/renderD128
>
> $ lsgpu --pci
> card0 8086:193B pci:vendor=8086,device=193B,card=0
> └─renderD128
>
> v2:
> * Fix pci filter format.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Suggested-by: Zbigniew Kempczyński <zbigniew.kempczynski@xxxxxxxxx>
> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> ---
> lib/igt_device_scan.c | 83 ++++++++++++++++++++++++++++++++-----------
> lib/igt_device_scan.h | 15 ++++++--
> tools/intel_gpu_top.c | 6 +++-
> tools/lsgpu.c | 32 +++++++++++++----
> 4 files changed, 106 insertions(+), 30 deletions(-)
>
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index ecb8db297f86..e97f7163ba70 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -777,7 +777,9 @@ static bool __check_empty(struct igt_list_head *view)
> return false;
> }
>
> -static void igt_devs_print_simple(struct igt_list_head *view)
> +static void
> +igt_devs_print_simple(struct igt_list_head *view,
> + const struct igt_devices_print_format *fmt)
> {
> struct igt_device *dev;
>
> @@ -821,7 +823,38 @@ __find_pci(struct igt_list_head *view, const char *drm)
> return NULL;
> }
>
> -static void igt_devs_print_user(struct igt_list_head *view)
> +static void __print_filter(char *buf, int len,
> + const struct igt_devices_print_format *fmt,
> + struct igt_device *dev,
> + bool render)
> +{
> + int ret;
> +
> + switch (fmt->option) {
> + case IGT_PRINT_DRM:
> + ret = snprintf(buf, len, "drm:%s",
> + render ? dev->drm_render : dev->drm_card);
> + igt_assert(ret < len);
> + break;
> + case IGT_PRINT_SYSFS:
> + ret = snprintf(buf, len, "sys:%s", dev->syspath);
> + igt_assert(ret < len);
> + break;
> + case IGT_PRINT_PCI:
> + if (!render) {
> + ret = snprintf(buf, len,
> + "pci:vendor=%s,device=%s,card=%d",
> + dev->vendor, dev->device,
> + dev->gpu_index);
> + igt_assert(ret < len);
> + }
> + break;
> + };
You could leave single assert after the switch but this is minor nit.
> +}
> +
> +static void
> +igt_devs_print_user(struct igt_list_head *view,
> + const struct igt_devices_print_format *fmt)
> {
> struct igt_device *dev;
>
> @@ -834,7 +867,6 @@ static void igt_devs_print_user(struct igt_list_head *view)
> struct igt_device *dev2;
> char filter[256];
> char *drm_name;
> - int ret;
>
> if (!is_drm_subsystem(dev))
> continue;
> @@ -845,16 +877,21 @@ static void igt_devs_print_user(struct igt_list_head *view)
> if (!drm_name || !*++drm_name)
> continue;
>
> - ret = snprintf(filter, sizeof(filter), "drm:%s", dev->drm_card);
> - igt_assert(ret < sizeof(filter));
> -
> pci_dev = __find_pci(view, dev->drm_card);
> - if (pci_dev)
> +
> + if (fmt->option == IGT_PRINT_PCI && !pci_dev)
> + continue;
> +
> + if (pci_dev) {
> + __print_filter(filter, sizeof(filter), fmt, pci_dev,
> + false);
> printf("%-24s%4s:%4s %s\n",
> drm_name, pci_dev->vendor, pci_dev->device,
> filter);
> - else
> + } else {
> + __print_filter(filter, sizeof(filter), fmt, dev, false);
> printf("%-24s %s\n", drm_name, filter);
> + }
>
> num_children = 0;
> igt_list_for_each_entry(dev2, view, link) {
> @@ -877,13 +914,15 @@ static void igt_devs_print_user(struct igt_list_head *view)
> if (!drm_name || !*++drm_name)
> continue;
>
> - ret = snprintf(filter, sizeof(filter), "drm:%s",
> - dev2->drm_render);
> - igt_assert(ret < sizeof(filter));
> -
> - printf("%s%-22s %s\n",
> - (++i == num_children) ? "└─" : "├─",
> - drm_name, filter);
> + printf("%s%-22s",
> + (++i == num_children) ? "└─" : "├─", drm_name);
> + if (fmt->option != IGT_PRINT_PCI) {
> + __print_filter(filter, sizeof(filter), fmt,
> + dev2, true);
> + printf(" %s\n", filter);
> + } else {
> + printf("\n");
> + }
> }
> }
> }
> @@ -908,7 +947,10 @@ static void print_ht(GHashTable *ht)
> g_list_free(keys);
> }
>
> -static void igt_devs_print_detail(struct igt_list_head *view)
> +static void
> +igt_devs_print_detail(struct igt_list_head *view,
> + const struct igt_devices_print_format *fmt)
> +
> {
> struct igt_device *dev;
>
> @@ -932,7 +974,8 @@ static void igt_devs_print_detail(struct igt_list_head *view)
> }
>
> static struct print_func {
> - void (*prn)(struct igt_list_head *view);
> + void (*prn)(struct igt_list_head *view,
> + const struct igt_devices_print_format *);
> } print_functions[] = {
> [IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple },
> [IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail },
> @@ -941,15 +984,15 @@ static struct print_func {
>
> /**
> * igt_devices_print
> - * @printtype: IGT_PRINT_SIMPLE or IGT_PRINT_DETAIL
> + * @fmt: Print format as specified by struct igt_devices_print_format
> *
> * Function can be used by external tool to print device array in simple
> * or detailed form. This function is added here to avoid exposing
> * internal implementation data structures.
> */
> -void igt_devices_print(enum igt_devices_print_type printtype)
> +void igt_devices_print(const struct igt_devices_print_format *fmt)
> {
> - print_functions[printtype].prn(&igt_devs.filtered);
> + print_functions[fmt->type].prn(&igt_devs.filtered, fmt);
> }
>
> /**
> diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> index 9822c22cb69c..bb4be72345fb 100644
> --- a/lib/igt_device_scan.h
> +++ b/lib/igt_device_scan.h
> @@ -35,11 +35,22 @@
> #include <unistd.h>
>
> enum igt_devices_print_type {
> - IGT_PRINT_SIMPLE,
> + IGT_PRINT_SIMPLE = 0,
> IGT_PRINT_DETAIL,
> IGT_PRINT_USER, /* End user friendly. */
> };
>
> +enum igt_devices_print_option {
> + IGT_PRINT_DRM = 0,
> + IGT_PRINT_SYSFS,
> + IGT_PRINT_PCI,
> +};
> +
> +struct igt_devices_print_format {
> + enum igt_devices_print_type type;
> + enum igt_devices_print_option option;
> +};
> +
> #define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
> #define PCI_SLOT_NAME_SIZE 12
> struct igt_device_card {
> @@ -51,7 +62,7 @@ struct igt_device_card {
>
> void igt_devices_scan(bool force);
>
> -void igt_devices_print(enum igt_devices_print_type printtype);
> +void igt_devices_print(const struct igt_devices_print_format *fmt);
> void igt_devices_print_vendors(void);
> void igt_device_print_filter_types(void);
>
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 5230472d2af4..07f88d555dc8 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -1387,7 +1387,11 @@ int main(int argc, char **argv)
> igt_devices_scan(false);
>
> if (list_device) {
> - igt_devices_print(IGT_PRINT_USER);
> + struct igt_devices_print_format fmt = {
> + .type = IGT_PRINT_USER
> + };
> +
> + igt_devices_print(&fmt);
> goto exit;
> }
>
> diff --git a/tools/lsgpu.c b/tools/lsgpu.c
> index 3b234b73361a..169ab0c29e50 100644
> --- a/tools/lsgpu.c
> +++ b/tools/lsgpu.c
> @@ -91,7 +91,11 @@ static const char *usage_str =
> " -v, --list-vendors List recognized vendors\n"
> " -l, --list-filter-types List registered device filters types\n"
> " -d, --device filter Device filter, can be given multiple times\n"
> - " -h, --help Show this help message and exit\n";
> + " -h, --help Show this help message and exit\n"
> + "\nOptions valid for default print out mode only:\n"
> + " --drm Show DRM filters (default) for each device\n"
> + " --sysfs Show sysfs filters for each device\n"
> + " --pci Show PCI filters for each device\n";
>
> static void test_device_open(struct igt_device_card *card)
> {
> @@ -153,6 +157,9 @@ static char *get_device_from_rc(void)
> int main(int argc, char *argv[])
> {
> static struct option long_options[] = {
> + {"drm", no_argument, NULL, 0},
> + {"sysfs", no_argument, NULL, 1},
> + {"pci", no_argument, NULL, 2},
> {"print-simple", no_argument, NULL, OPT_PRINT_SIMPLE},
> {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL},
> {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS},
> @@ -163,17 +170,19 @@ int main(int argc, char *argv[])
> };
> int c, index = 0;
> char *env_device = NULL, *opt_device = NULL, *rc_device = NULL;
> - enum igt_devices_print_type printtype = IGT_PRINT_USER;
> + struct igt_devices_print_format fmt = {
> + .type = IGT_PRINT_USER,
> + };
>
> while ((c = getopt_long(argc, argv, "spvld:h",
> long_options, &index)) != -1) {
> switch(c) {
>
> case OPT_PRINT_SIMPLE:
> - printtype = IGT_PRINT_SIMPLE;
> + fmt.type = IGT_PRINT_SIMPLE;
> break;
> case OPT_PRINT_DETAIL:
> - printtype = IGT_PRINT_DETAIL;
> + fmt.type = IGT_PRINT_DETAIL;
> break;
> case OPT_LIST_VENDORS:
> g_show_vendors = true;
> @@ -187,6 +196,15 @@ int main(int argc, char *argv[])
> case OPT_HELP:
> g_help = true;
> break;
> + case 0:
> + fmt.option = IGT_PRINT_DRM;
> + break;
> + case 1:
> + fmt.option = IGT_PRINT_SYSFS;
> + break;
> + case 2:
> + fmt.option = IGT_PRINT_PCI;
> + break;
> }
> }
>
> @@ -239,14 +257,14 @@ int main(int argc, char *argv[])
> printf("Device detail:\n");
> print_card(&card);
> test_device_open(&card);
> - if (printtype == IGT_PRINT_DETAIL) {
> + if (fmt.type == IGT_PRINT_DETAIL) {
> printf("\n");
> - igt_devices_print(printtype);
> + igt_devices_print(&fmt);
> }
> printf("-------------------------------------------\n");
>
> } else {
> - igt_devices_print(printtype);
> + igt_devices_print(&fmt);
> }
>
> free(rc_device);
> --
> 2.25.1
>
Ok, haven't tracked other issues and LGTM:
Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@xxxxxxxxx>
--
Zbigniew
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
