Re: [igt-dev] [PATCH i-g-t v3 2/3] tools/intel_gpu_top: Add generation info in header

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 17/11/2020 16:34, Zbigniew Kempczyński wrote:
> In multi device world we may want to see generation of device we're
> tracking counters. Add generation number/codename to be more verbose.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>   lib/Makefile.am       |  4 +++-
>   lib/meson.build       |  1 +
>   tools/intel_gpu_top.c | 18 ++++++++++++++++--
>   3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index fecf34cd..c476eeab 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -118,7 +118,9 @@ libigt_device_scan_la_SOURCES = \
>   	igt_list.c		\
>   	igt_tools_stub.c	\
>   	igt_device_scan.c	\
> -	igt_device_scan.h
> +	igt_device_scan.h	\
> +	intel_device_info.c	\
> +	intel_chipset.h
>   
>   libigt_perf_la_SOURCES = \
>   	igt_perf.c	 \
> diff --git a/lib/meson.build b/lib/meson.build
> index 090a10cd..540facb2 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -182,6 +182,7 @@ lib_igt_device_scan_build = static_library('igt_device_scan',
>   	['igt_device_scan.c',
>   	'igt_list.c',
>   	'igt_tools_stub.c',
> +	'intel_device_info.c',
>   	],
>   	dependencies : scan_dep,
>   	include_directories : inc)
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 86de09aa..399416c3 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -22,6 +22,7 @@
>    */
>   
>   #include "igt_device_scan.h"
> +#include "intel_chipset.h"
>   
>   #include <stdio.h>
>   #include <sys/types.h>
> @@ -1028,9 +1029,12 @@ static bool print_groups(struct cnt_group **groups)
>   
>   static int
>   print_header(const struct igt_device_card *card,
> +	     const struct intel_device_info *info,
>   	     struct engines *engines, double t,
>   	     int lines, int con_w, int con_h, bool *consumed)
>   {
> +	static char *codename;
> +	unsigned int gen = info ? ffs(info->gen) : 0;
>   	struct pmu_counter fake_pmu = {
>   		.present = true,
>   		.val.cur = 1,
> @@ -1106,8 +1110,15 @@ print_header(const struct igt_device_card *card,
>   	if (output_mode == INTERACTIVE) {
>   		printf("\033[H\033[J");
>   
> +		if (!codename) {
> +			codename = info->codename ? strdup(info->codename) :
> +						    strdup("unknown");
> +			codename[0] = toupper(codename[0]);
> +		}
> +
>   		if (lines++ < con_h) {
> -			printf("intel-gpu-top: %s - ", card->card);
> +			printf("intel-gpu-top: %s (Gen%u) @ %s - ",
> +			       codename, gen, card->card);

Leaks codename and lets try and avoid code duplication. And print hex 
string if unknown - so same comments as in previous patch.

>   			if (!engines->discrete)
>   				printf("%s/%s MHz;  %s%% RC6; %s %s; %s irqs/s\n",
>   					freq_items[1].buf, freq_items[0].buf,
> @@ -1317,6 +1328,7 @@ int main(int argc, char **argv)
>   	bool list_device = false;
>   	char *pmu_device, *opt_device = NULL;
>   	struct igt_device_card card;
> +	const struct intel_device_info *info = NULL;
>   
>   	/* Parse options */
>   	while ((ch = getopt(argc, argv, "o:s:d:JLlh")) != -1) {
> @@ -1441,6 +1453,8 @@ int main(int argc, char **argv)
>   	ret = EXIT_SUCCESS;
>   
>   	pmu_sample(engines);
> +	if (card.pci_device)
> +		info = intel_get_device_info(card.pci_device);
>   
>   	while (!stop_top) {
>   		bool consumed = false;
> @@ -1463,7 +1477,7 @@ int main(int argc, char **argv)
>   			break;
>   
>   		while (!consumed) {
> -			lines = print_header(&card, engines,
> +			lines = print_header(&card, info, engines,
>   					     t, lines, con_w, con_h,
>   					     &consumed);
>   
> 

I would really prefer print_header would not need to rebuild the pretty 
string every time. And also that we try and limit adding new arguments 
to it. So I have this idea:

Add the pretty string to struct engines and build it once on probe. 
Probably change discover_engines to take the card and move the block 
which figures out the pmu_device into the function. It can then build 
the pretty string and store in engines->codename / engines->drm_card or 
something.

Since I am bikeshedding a lot I am happy to take over the intel_gpu_top 
patch at least. But if you feel these suggestions are solid also feel 
free to carry on yourself.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev