Re: [Intel-gfx] [RFC 1/3] intel_gpu_top: User friendly device listing

["Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>]

On Tue, Nov 10, 2020 at 11:19:46AM +0000, Tvrtko Ursulin wrote:
> 
> On 10/11/2020 11:13, Zbigniew Kempczyński wrote:
> > On Mon, Nov 09, 2020 at 10:48:09AM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > Adding a new device selection print type suitable for user-facing
> > > use cases like intel_gpu_top -L and later lsgpu.
> > > 
> > > Instead of:
> > > 
> > > sys:/sys/devices/pci0000:00/0000:00:02.0/drm/card0
> > >      subsystem       : drm
> > >      drm card        : /dev/dri/card0
> > >      parent          : sys:/sys/devices/pci0000:00/0000:00:02.0
> > > 
> > > sys:/sys/devices/pci0000:00/0000:00:02.0/drm/renderD128
> > >      subsystem       : drm
> > >      drm render      : /dev/dri/renderD128
> > >      parent          : sys:/sys/devices/pci0000:00/0000:00:02.0
> > > 
> > > sys:/sys/devices/pci0000:00/0000:00:02.0
> > >      subsystem       : pci
> > >      drm card        : /dev/dri/card0
> > >      drm render      : /dev/dri/renderD128
> > >      vendor          : 8086
> > >      device          : 193B
> > > 
> > > New format looks like:
> > > 
> > > card0                   8086:193B    drm:/dev/dri/card0
> > > └─renderD128                         drm:/dev/dri/renderD128
> > > 
> > > Advantages are more compact, more readable, one entry per GPU, shorter
> > > string to copy and paste to intel_gpu_top -d, or respective usage.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > > ---
> > >   lib/igt_device_scan.c | 109 +++++++++++++++++++++++++++++++++++++-----
> > >   lib/igt_device_scan.h |   1 +
> > >   tools/intel_gpu_top.c |   3 +-
> > >   3 files changed, 100 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > > index c581a31ae55e..e66ccdc25aeb 100644
> > > --- a/lib/igt_device_scan.c
> > > +++ b/lib/igt_device_scan.c
> > > @@ -735,18 +735,26 @@ static inline void _pr_simple2(const char *k, const char *v1, const char *v2)
> > >   	printf("    %-16s: %s:%s\n", k, v1, v2);
> > >   }
> > > -static void igt_devs_print_simple(struct igt_list_head *view)
> > > +static bool __check_empty(struct igt_list_head *view)
> > >   {
> > > -	struct igt_device *dev;
> > > -
> > >   	if (!view)
> > > -		return;
> > > +		return true;
> > >   	if (igt_list_empty(view)) {
> > >   		printf("No GPU devices found\n");
> > > -		return;
> > > +		return true;
> > >   	}
> > > +	return false;
> > > +}
> > > +
> > > +static void igt_devs_print_simple(struct igt_list_head *view)
> > > +{
> > > +	struct igt_device *dev;
> > > +
> > > +	if (__check_empty(view))
> > > +		return;
> > > +
> > >   	igt_list_for_each_entry(dev, view, link) {
> > >   		printf("sys:%s\n", dev->syspath);
> > >   		if (dev->subsystem)
> > > @@ -768,6 +776,89 @@ static void igt_devs_print_simple(struct igt_list_head *view)
> > >   	}
> > >   }
> > > +static struct igt_device *
> > > +__find_pci(struct igt_list_head *view, const char *drm)
> > > +{
> > > +	struct igt_device *dev;
> > > +
> > > +	igt_list_for_each_entry(dev, view, link) {
> > > +		if (!is_pci_subsystem(dev) || !dev->drm_card)
> > > +			continue;
> > > +
> > > +		if (!strcmp(dev->drm_card, drm))
> > > +			return dev;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +static void igt_devs_print_user(struct igt_list_head *view)
> > > +{
> > > +	struct igt_device *dev;
> > > +
> > > +	if (__check_empty(view))
> > > +		return;
> > > +
> > > +	igt_list_for_each_entry(dev, view, link) {
> > > +		unsigned int i, num_children;
> > > +		struct igt_device *pci_dev;
> > > +		struct igt_device *dev2;
> > > +		char filter[64];
> > > +		char *drm_name;
> > > +		int ret;
> > > +
> > > +		if (!is_drm_subsystem(dev))
> > > +			continue;
> > > +		if (!dev->drm_card || dev->drm_render)
> > > +			continue;
> > > +
> > > +		drm_name = rindex(dev->drm_card, '/');
> > > +		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)
> > > +			printf("%-24s%4s:%4s    %s\n",
> > > +			       drm_name, pci_dev->vendor, pci_dev->device,
> > > +			       filter);
> > > +		else
> > > +			printf("%-24s             %s\n", drm_name, filter);
> > > +
> > > +		num_children = 0;
> > > +		igt_list_for_each_entry(dev2, view, link) {
> > > +			if (!is_drm_subsystem(dev2) || !dev2->drm_render)
> > > +				continue;
> > > +			if (strcmp(dev2->parent->syspath, dev->parent->syspath))
> > > +				continue;
> > > +
> > > +			num_children++;
> > > +		}
> > > +
> > > +		i = 0;
> > > +		igt_list_for_each_entry(dev2, view, link) {
> > > +			if (!is_drm_subsystem(dev2) || !dev2->drm_render)
> > > +				continue;
> > > +			if (strcmp(dev2->parent->syspath, dev->parent->syspath))
> > > +				continue;
> > > +
> > > +			drm_name = rindex(dev2->drm_render, '/');
> > > +			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);
> > > +		}
> > > +	}
> > > +}
> > > +
> > >   static inline void _print_key_value(const char* k, const char *v)
> > >   {
> > >   	printf("%-32s: %s\n", k, v);
> > > @@ -792,14 +883,9 @@ static void igt_devs_print_detail(struct igt_list_head *view)
> > >   {
> > >   	struct igt_device *dev;
> > > -	if (!view)
> > > +	if (__check_empty(view))
> > >   		return;
> > > -	if (igt_list_empty(view)) {
> > > -		printf("No GPU devices found\n");
> > > -		return;
> > > -	}
> > > -
> > >   	igt_list_for_each_entry(dev, view, link) {
> > >   		printf("========== %s:%s ==========\n",
> > >   		       dev->subsystem, dev->syspath);
> > > @@ -821,6 +907,7 @@ static struct print_func {
> > >   } print_functions[] = {
> > >   	[IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple },
> > >   	[IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail },
> > > +	[IGT_PRINT_USER] = { .prn = igt_devs_print_user },
> > >   };
> > >   /**
> > > diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> > > index 99daee0c52d6..9822c22cb69c 100644
> > > --- a/lib/igt_device_scan.h
> > > +++ b/lib/igt_device_scan.h
> > > @@ -37,6 +37,7 @@
> > >   enum igt_devices_print_type {
> > >   	IGT_PRINT_SIMPLE,
> > >   	IGT_PRINT_DETAIL,
> > > +	IGT_PRINT_USER, /* End user friendly. */
> > >   };
> > >   #define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
> > > diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> > > index 298defa4e6ed..5230472d2af4 100644
> > > --- a/tools/intel_gpu_top.c
> > > +++ b/tools/intel_gpu_top.c
> > > @@ -1313,7 +1313,6 @@ int main(int argc, char **argv)
> > >   	unsigned int i;
> > >   	int ret = 0, ch;
> > >   	bool list_device = false;
> > > -	enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE;
> > >   	char *pmu_device, *opt_device = NULL;
> > >   	struct igt_device_card card;
> > > @@ -1388,7 +1387,7 @@ int main(int argc, char **argv)
> > >   	igt_devices_scan(false);
> > >   	if (list_device) {
> > > -		igt_devices_print(printtype);
> > > +		igt_devices_print(IGT_PRINT_USER);
> > 
> > I would add at least possibility to use simple view to suggest
> > how to use pci/sys filter. With USER print format we see only
> 
> You mean the blurb printed out by igt_device_print_filter_types (currently
> printed as part of help text) or something else?

Yes. We know lsgpu and intel_gpu_top selection is same and you're
also printing filter syntax on 'intel_gpu_top -h'. But without 
using 'lsgpu -s' you won't guess how to specify sys:... filter.

> 
> > drm paths. But I won't insist for that, using drm selection
> > is ok for me.
> 
> Good point actually, intel_gpu_top should probably default to "--pci"
> listing type since it monitors GPUs with no notion of DRM master/render.
> 
> > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> 
> Thanks! Let me know if you agree with "--pci" by default for intel_gpu_top
> and if it is okay to keep the r-b?

If it is not problem you could provide --pci and --sysfs too to be compliant
with lsgpu switches. But there are minor nits and you can keep r-b with 
the code as it is.  

> 
> Regards,
> 
> Tvrtko

--
Zbigniew
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx