Re: [RFC i-g-t] intel_gpu_top: User friendly device listing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 16, 2020 at 10:11:04AM +0100, Tvrtko Ursulin wrote:
> 
> On 16/10/2020 05:07, Zbigniew Kempczyński wrote:
> > On Thu, Oct 15, 2020 at 09:09:02AM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 15/10/2020 05:36, Zbigniew Kempczyński wrote:
> >>> On Wed, Oct 14, 2020 at 11:48:53AM +0100, Tvrtko Ursulin wrote:
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>>
> >>>> Adding a new device selection print type suitable for user-facing
> >>>> use cases like intel_gpu_top -L and potentially 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.
> >>>
> >>> Looks nice and more intuitive.
> >>
> >> I wasn't sure about duplication of card/render name and again in filter
> >> string, but I have no better ideas at the moment.
> >>
> >> Maybe one day some could replace the left column with some card names from
> >> some database but for now above is good enough.
> >>
> >>> 1. Add -s switch to handle simple print (current default in lsgpu)
> >>
> >> I wanted to suggest -v (verbose) or -d (details) but both are taken in
> >> lsgpu. -s sounds like silent or in any case not obvious what "simple" means.
> >> Could use just long form --verbose or --details?
> > 
> > -v is vendor list we support in pci vendor ids, not verbose. If you have
> > resistence to use -s just use -i (simple). Sometimes I need to go to
> > device directory in /sys so I use lsgpu simple output with directory name.
> 
> I don't think "simple" as a term should be user facing because it is more complex than the mode I am proposing. So cannot really be called simple in my mind.

As lsgpu had two views they were called simple and detailed. I got no
better names previously.

> 
> To be clear I am working under the assumption intel_gpu_top and lsgpu will be getting more and more users and as such emphasis on making them less of a developer tools should be increased. (Former had an user bug submitted in Fedora, which was one of the triggers.)
> 
> I do agree sysfs path is an interesting addition, or alternate mode. Maybe user printout should have selectable modes of drm or sysfs?
> 
> $ lsgpu --drm --filter (filter adds the drm: prefix, can be default)
> 
> card0                   8086:193B    drm:/dev/dri/card0
> └─renderD128                         drm:/dev/dri/renderD128

I would assume --drm as default so 

$ lsgpu 

would be enough for above (drm) view.

> 
> $ lsgpu --sysfs
> 
> card0                   8086:193B    /sys/devices/pci0000:00/0000:00:02.0/drm/card0
> └─renderD128                         /sys/devices/pci0000:00/0000:00:02.0/drm/renderD128
> 

Ack.

> $ lsgpu --device
> 
> card0                   8086:193B    /sys/devices/pci0000:00/0000:00:02.0
> └─renderD128                         

Ack.

> 
> $ lsgpu --verbose (or --details)
> 
> ... the current "simple" output ...

I think we can reach consensuns with:
--verbose (simple view)
--detail (detailed view with properties)
--user (default view as you propose)

--
Zbigniew

> 
> None of my complications from above do not fit directly into the current print type enum. I'll play with it time permitting and send another RFC.
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > --
> > Zbigniew
> > 
> > 
> >>
> >>> 2. Change to user format as default.
> >>
> >> Yep.
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>>
> >>> --
> >>> Zbigniew
> >>>
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> >>>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@xxxxxxxxx>
> >>>> ---
> >>>>    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 f4d43c733314..ce0ea61dda50 100644
> >>>> --- a/lib/igt_device_scan.c
> >>>> +++ b/lib/igt_device_scan.c
> >>>> @@ -695,18 +695,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)
> >>>> @@ -728,6 +736,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);
> >>>> @@ -752,14 +843,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);
> >>>> @@ -781,6 +867,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 bd937d22752c..9e13ed9db406 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 79a936ffbe1a..b984edc656c7 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);
> >>>>    		goto exit;
> >>>>    	}
> >>>> -- 
> >>>> 2.25.1
> >>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux