[igt-dev] [RFC 1/3] intel_gpu_top: User friendly device listing

[Tvrtko Ursulin]

On 10/11/2020 11:46, Zbigniew Kempczyński wrote:

[snip]

>>>> 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.

My thinking is that typical intel_gpu_top user wouldn't want/need to 
concern themselves with different filters but just copy&paste what was 
listed from intel_gpu_top -L. In which case..

>>
>>> 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 at 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.

... I did not see the value of supporting --drm/--pci/--sysfs from 
intel_gpu_top.

So "--pci" by default should "JustWork" and make end users happy. As 
long as the help text is clear enough what needs to be copy&pasted, 
which may need improvement. If I am not missing something else.

Regards,

Tvrtko