[Intel-gfx] [PATCH i-g-t 0/6] Assorted intel_gpu_top improvements

[Intel-gfx] [PATCH i-g-t 0/6] Assorted intel_gpu_top improvements

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Mostly by popular demand from
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/109, although for
some of these changes I'd like some second opinions. For instance is it user
friendly to change the default aggregation mode for stdout and JSON output?

Plus a GPU selection fix to prepare for Intel VPU being a DRM device, making
intel_gpu_top not try to monitor it.

And finally some memory freeing fixes.

Tvrtko Ursulin (6):
  intel_gpu_top: Fix man page formatting
  intel_gpu_top: Automatically enclose JSON output into brackets
  intel_gpu_top: Add command line switch to start in physical engine
    mode
  intel_gpu_top: Aggregate engine classes in all output modes
  intel_gpu_top: Fix cleanup on old kernels / unsupported GPU
  lib/igt_device_scan: Improve Intel discrete GPU selection

 lib/igt_device_scan.c |  7 +++--
 man/intel_gpu_top.rst | 60 +++++++++++++++++++++----------------------
 tools/intel_gpu_top.c | 37 +++++++++++++++++++-------
 3 files changed, 62 insertions(+), 42 deletions(-)

-- 
2.34.1

[Intel-gfx] [PATCH i-g-t 1/6] intel_gpu_top: Fix man page formatting

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

New lines are not respected when rst2man generates the page so try to work
around that by followin advice from the Internet.

v2:
 * Improve some wording.
 * Tidy -o option description.
 * Update dates.
 * Convert the filter list to table.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com> # v1
---
 man/intel_gpu_top.rst | 55 ++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/man/intel_gpu_top.rst b/man/intel_gpu_top.rst
index 748c7740c800..4417bcff0d5b 100644
--- a/man/intel_gpu_top.rst
+++ b/man/intel_gpu_top.rst
@@ -7,9 +7,9 @@ Display a top-like summary of Intel GPU usage
 ---------------------------------------------
 .. include:: defs.rst
 :Author: IGT Developers <igt-dev@lists.freedesktop.org>
-:Date: 2020-03-18
+:Date: 2023-01-27
 :Version: |PACKAGE_STRING|
-:Copyright: 2009,2011,2012,2016,2018,2019,2020 Intel Corporation
+:Copyright: 2009,2011,2012,2016,2018,2019,2020,2023 Intel Corporation
 :Manual section: |MANUAL_SECTION|
 :Manual group: |MANUAL_GROUP|
 
@@ -23,7 +23,7 @@ DESCRIPTION
 
 **intel_gpu_top** is a tool to display usage information on Intel GPU's.
 
-The tool gathers data using perf performance counters (PMU) exposed by i915 and other platform drivers like RAPL (power) and Uncore IMC (memory bandwidth).
+The tool presents data collected from performance counters (PMU), exposed by i915 and other platform drivers like RAPL (power) and Uncore IMC (memory bandwidth).
 
 OPTIONS
 =======
@@ -37,49 +37,44 @@ OPTIONS
 -l
     List plain text data.
 
--o <file path | ->
-    Output to the specified file instead of standard output.
-    '-' can also be specified to explicitly select standard output.
+-o <file>, or -o -
+    Output to the specified file instead of standard output. '-' can also be specified to explicitly select standard output.
 
 -s <ms>
     Refresh period in milliseconds.
+
 -L
-    List available GPUs on the platform.
+    List available GPUs on the system.
+
 -d
-    Select a specific GPU using supported filter.
+    Select a specific GPU using one of the supported filters.
 
 RUNTIME CONTROL
 ===============
 
 Supported keys:
 
-    'q'    Exit from the tool.
-    'h'    Show interactive help.
-    '1'    Toggle between aggregated engine class and physical engine mode.
-    'n'    Toggle display of numeric client busyness overlay.
-    's'    Toggle between sort modes (runtime, total runtime, pid, client id).
-    'i'    Toggle display of clients which used no GPU time.
-    'H'    Toggle between per PID aggregation and individual clients.
+|
+|    'q'    Exit from the tool.
+|    'h'    Show interactive help.
+|    '1'    Toggle between aggregated by engine class and physical engine mode.
+|    'n'    Toggle display of numeric client busyness overlay.
+|    's'    Toggle between sort modes (runtime, total runtime, pid, client id).
+|    'i'    Toggle display of clients which used no GPU time.
+|    'H'    Toggle between per PID aggregation and individual clients.
 
 DEVICE SELECTION
 ================
 
-User can select specific GPU for performance monitoring on platform where multiple GPUs are available.
-A GPU can be selected by sysfs path, drm node or using various PCI sub filters.
-
-Filter types: ::
-
-    ---
-    filter   syntax
-    ---
-    sys      sys:/sys/devices/pci0000:00/0000:00:02.0
-             find device by its sysfs path
-
-    drm      drm:/dev/dri/* path
-             find drm device by /dev/dri/* node
+On systems where multiple GPUs are present it is possible to select a specific GPU to be monitored. A GPU can be selected by sysfs path, drm device node or using various PCI sub filters.
 
-    pci      pci:[vendor=%04x/name][,device=%04x][,card=%d]
-             vendor is hex number or vendor name
+==========  ====================================================== ======================
+**Filter**  **Syntax**                                             **GPU selection criteria**
+==========  ====================================================== ======================
+sys          | ``sys:/sys/devices/pci0000:00/0000:00:02.0``        Select using the sysfs path.
+drm          | ``drm:/dev/dri/<node>``                             Select using the /dev/dri/\* device node.
+pci          | ``pci:[vendor=%04x/name][,device=%04x][,card=%d]``  Select using the PCI addrress. Vendor is hexadecinal number or vendor name.
+==========  ====================================================== ======================
 
 JSON OUTPUT
 ===========
-- 
2.34.1

Re: [Intel-gfx] [PATCH i-g-t 1/6] intel_gpu_top: Fix man page formatting

[Kamil Konieczny]

Hi Tvrtko,

On 2023-01-27 at 11:12:36 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> New lines are not respected when rst2man generates the page so try to work
> around that by followin advice from the Internet.
> 
> v2:
>  * Improve some wording.
>  * Tidy -o option description.
>  * Update dates.
>  * Convert the filter list to table.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com> # v1
--------------------------------------------------------------- ^^^^^
Remove this addition at end,
s/ # v1//

Man page looks much better so you can keep my r-b,
thank you for your effort,

Regards,
Kamil

> ---
>  man/intel_gpu_top.rst | 55 ++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 30 deletions(-)
> 
> diff --git a/man/intel_gpu_top.rst b/man/intel_gpu_top.rst
> index 748c7740c800..4417bcff0d5b 100644
> --- a/man/intel_gpu_top.rst
> +++ b/man/intel_gpu_top.rst
> @@ -7,9 +7,9 @@ Display a top-like summary of Intel GPU usage
>  ---------------------------------------------
>  .. include:: defs.rst
>  :Author: IGT Developers <igt-dev@lists.freedesktop.org>
> -:Date: 2020-03-18
> +:Date: 2023-01-27
>  :Version: |PACKAGE_STRING|
> -:Copyright: 2009,2011,2012,2016,2018,2019,2020 Intel Corporation
> +:Copyright: 2009,2011,2012,2016,2018,2019,2020,2023 Intel Corporation
>  :Manual section: |MANUAL_SECTION|
>  :Manual group: |MANUAL_GROUP|
>  
> @@ -23,7 +23,7 @@ DESCRIPTION
>  
>  **intel_gpu_top** is a tool to display usage information on Intel GPU's.
>  
> -The tool gathers data using perf performance counters (PMU) exposed by i915 and other platform drivers like RAPL (power) and Uncore IMC (memory bandwidth).
> +The tool presents data collected from performance counters (PMU), exposed by i915 and other platform drivers like RAPL (power) and Uncore IMC (memory bandwidth).
>  
>  OPTIONS
>  =======
> @@ -37,49 +37,44 @@ OPTIONS
>  -l
>      List plain text data.
>  
> --o <file path | ->
> -    Output to the specified file instead of standard output.
> -    '-' can also be specified to explicitly select standard output.
> +-o <file>, or -o -
> +    Output to the specified file instead of standard output. '-' can also be specified to explicitly select standard output.
>  
>  -s <ms>
>      Refresh period in milliseconds.
> +
>  -L
> -    List available GPUs on the platform.
> +    List available GPUs on the system.
> +
>  -d
> -    Select a specific GPU using supported filter.
> +    Select a specific GPU using one of the supported filters.
>  
>  RUNTIME CONTROL
>  ===============
>  
>  Supported keys:
>  
> -    'q'    Exit from the tool.
> -    'h'    Show interactive help.
> -    '1'    Toggle between aggregated engine class and physical engine mode.
> -    'n'    Toggle display of numeric client busyness overlay.
> -    's'    Toggle between sort modes (runtime, total runtime, pid, client id).
> -    'i'    Toggle display of clients which used no GPU time.
> -    'H'    Toggle between per PID aggregation and individual clients.
> +|
> +|    'q'    Exit from the tool.
> +|    'h'    Show interactive help.
> +|    '1'    Toggle between aggregated by engine class and physical engine mode.
> +|    'n'    Toggle display of numeric client busyness overlay.
> +|    's'    Toggle between sort modes (runtime, total runtime, pid, client id).
> +|    'i'    Toggle display of clients which used no GPU time.
> +|    'H'    Toggle between per PID aggregation and individual clients.
>  
>  DEVICE SELECTION
>  ================
>  
> -User can select specific GPU for performance monitoring on platform where multiple GPUs are available.
> -A GPU can be selected by sysfs path, drm node or using various PCI sub filters.
> -
> -Filter types: ::
> -
> -    ---
> -    filter   syntax
> -    ---
> -    sys      sys:/sys/devices/pci0000:00/0000:00:02.0
> -             find device by its sysfs path
> -
> -    drm      drm:/dev/dri/* path
> -             find drm device by /dev/dri/* node
> +On systems where multiple GPUs are present it is possible to select a specific GPU to be monitored. A GPU can be selected by sysfs path, drm device node or using various PCI sub filters.
>  
> -    pci      pci:[vendor=%04x/name][,device=%04x][,card=%d]
> -             vendor is hex number or vendor name
> +==========  ====================================================== ======================
> +**Filter**  **Syntax**                                             **GPU selection criteria**
> +==========  ====================================================== ======================
> +sys          | ``sys:/sys/devices/pci0000:00/0000:00:02.0``        Select using the sysfs path.
> +drm          | ``drm:/dev/dri/<node>``                             Select using the /dev/dri/\* device node.
> +pci          | ``pci:[vendor=%04x/name][,device=%04x][,card=%d]``  Select using the PCI addrress. Vendor is hexadecinal number or vendor name.
> +==========  ====================================================== ======================
>  
>  JSON OUTPUT
>  ===========
> -- 
> 2.34.1
> 

[Intel-gfx] [PATCH i-g-t 2/6] intel_gpu_top: Automatically enclose JSON output into brackets

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Parsers need the whole output enclosed into square brackets so every
period sample becomes an array element.

So far we have been suggesting this in the man page but we can trivially
make the tool output that itself.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Eero Tamminen <eero.t.tamminen@intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 man/intel_gpu_top.rst | 2 +-
 tools/intel_gpu_top.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/man/intel_gpu_top.rst b/man/intel_gpu_top.rst
index 4417bcff0d5b..caf0a9f9432c 100644
--- a/man/intel_gpu_top.rst
+++ b/man/intel_gpu_top.rst
@@ -79,7 +79,7 @@ pci          | ``pci:[vendor=%04x/name][,device=%04x][,card=%d]``  Select using
 JSON OUTPUT
 ===========
 
-To parse the JSON as output by the tool the consumer should wrap its entirety into square brackets ([ ]). This will make each sample point a JSON array element and will avoid "Multiple root elements" JSON validation error.
+JSON output will be correctly terminated when the tool cleanly exits, otherwise one square bracket needs to be added before parsing.
 
 LIMITATIONS
 ===========
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 6de8a164fcff..c4d98de4fe31 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -2597,6 +2597,9 @@ int main(int argc, char **argv)
 	scan_clients(clients, false);
 	codename = igt_device_get_pretty_name(&card, false);
 
+	if (output_mode == JSON)
+		printf("[\n");
+
 	while (!stop_top) {
 		struct clients *disp_clients;
 		bool consumed = false;
@@ -2683,6 +2686,9 @@ int main(int argc, char **argv)
 			usleep(period_us);
 	}
 
+	if (output_mode == JSON)
+		printf("]\n");
+
 	if (clients)
 		free_clients(clients);
 
-- 
2.34.1

[Intel-gfx] [PATCH i-g-t 3/6] intel_gpu_top: Add command line switch to start in physical engine mode

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Default mode is to aggreate engines per class but some users would prefer
to be able to start in physical engine mode too.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 man/intel_gpu_top.rst | 3 +++
 tools/intel_gpu_top.c | 9 +++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/man/intel_gpu_top.rst b/man/intel_gpu_top.rst
index caf0a9f9432c..69834756b81e 100644
--- a/man/intel_gpu_top.rst
+++ b/man/intel_gpu_top.rst
@@ -49,6 +49,9 @@ OPTIONS
 -d
     Select a specific GPU using one of the supported filters.
 
+-p
+   Default to showing physical engines instead of aggregated classes.
+
 RUNTIME CONTROL
 ===============
 
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index c4d98de4fe31..e91b47baf72b 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -1268,6 +1268,7 @@ usage(const char *appname)
 		"\t[-s <ms>]       Refresh period in milliseconds (default %ums).\n"
 		"\t[-L]            List all cards.\n"
 		"\t[-d <device>]   Device filter, please check manual page for more details.\n"
+		"\t[-p]            Default to showing physical engines instead of classes.\n"
 		"\n",
 		appname, DEFAULT_PERIOD_MS);
 	igt_device_print_filter_types();
@@ -2446,6 +2447,7 @@ int main(int argc, char **argv)
 {
 	unsigned int period_us = DEFAULT_PERIOD_MS * 1000;
 	struct clients *clients = NULL;
+	bool physical_engines = false;
 	int con_w = -1, con_h = -1;
 	char *output_path = NULL;
 	struct engines *engines;
@@ -2456,7 +2458,7 @@ int main(int argc, char **argv)
 	char *codename = NULL;
 
 	/* Parse options */
-	while ((ch = getopt(argc, argv, "o:s:d:JLlh")) != -1) {
+	while ((ch = getopt(argc, argv, "o:s:d:pJLlh")) != -1) {
 		switch (ch) {
 		case 'o':
 			output_path = optarg;
@@ -2467,6 +2469,9 @@ int main(int argc, char **argv)
 		case 'd':
 			opt_device = strdup(optarg);
 			break;
+		case 'p':
+			physical_engines = true;
+			break;
 		case 'J':
 			output_mode = JSON;
 			break;
@@ -2508,7 +2513,7 @@ int main(int argc, char **argv)
 	case INTERACTIVE:
 		pops = &term_pops;
 		interactive_stdin();
-		class_view = true;
+		class_view = !physical_engines;
 		break;
 	case STDOUT:
 		pops = &stdout_pops;
-- 
2.34.1

[Intel-gfx] [PATCH i-g-t 4/6] intel_gpu_top: Aggregate engine classes in all output modes

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Use the same default for stdout and JSON output modes as it is for
interactive.

Previously added command line switch can be used to go back to showing all
physical engines.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tools/intel_gpu_top.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index e91b47baf72b..7aa233570463 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -2509,11 +2509,12 @@ int main(int argc, char **argv)
 	if (signal(SIGINT, sigint_handler) == SIG_ERR)
 		fprintf(stderr, "Failed to install signal handler!\n");
 
+	class_view = !physical_engines;
+
 	switch (output_mode) {
 	case INTERACTIVE:
 		pops = &term_pops;
 		interactive_stdin();
-		class_view = !physical_engines;
 		break;
 	case STDOUT:
 		pops = &stdout_pops;
-- 
2.34.1

[Intel-gfx] [PATCH i-g-t 5/6] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Avoid trying to dereference null engines on exit when there are either
none which are supported, or kernel does not have i915 PMU support.

Also fix a memory leak on the same failure path just so Valgrind runs are
quite.

v2:
 * Fix a memory leak in the same failure mode too.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Nirmoy Das <nirmoy.das@intel.com> # v1
---
 tools/intel_gpu_top.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 7aa233570463..0a1de41b3374 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -340,7 +340,7 @@ static struct engines *discover_engines(char *device)
 
 	d = opendir(sysfs_root);
 	if (!d)
-		return NULL;
+		goto err;
 
 	while ((dent = readdir(d)) != NULL) {
 		const char *endswith = "-busy";
@@ -423,10 +423,8 @@ static struct engines *discover_engines(char *device)
 	}
 
 	if (ret) {
-		free(engines);
 		errno = ret;
-
-		return NULL;
+		goto err;
 	}
 
 	qsort(engine_ptr(engines, 0), engines->num_engines,
@@ -435,6 +433,11 @@ static struct engines *discover_engines(char *device)
 	engines->root = d;
 
 	return engines;
+
+err:
+	free(engines);
+
+	return NULL;
 }
 
 static void free_engines(struct engines *engines)
@@ -448,6 +451,9 @@ static void free_engines(struct engines *engines)
 	};
 	unsigned int i;
 
+	if (!engines)
+		return;
+
 	for (pmu = &free_list[0]; *pmu; pmu++) {
 		if ((*pmu)->present)
 			free((char *)(*pmu)->units);
@@ -2568,7 +2574,7 @@ int main(int argc, char **argv)
 			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
 			strerror(errno));
 		ret = EXIT_FAILURE;
-		goto err;
+		goto err_engines;
 	}
 
 	ret = pmu_init(engines);
@@ -2585,7 +2591,7 @@ int main(int argc, char **argv)
 "More information can be found at 'Perf events and tool security' document:\n"
 "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
 		ret = EXIT_FAILURE;
-		goto err;
+		goto err_pmu;
 	}
 
 	ret = EXIT_SUCCESS;
@@ -2699,8 +2705,9 @@ int main(int argc, char **argv)
 		free_clients(clients);
 
 	free(codename);
-err:
+err_pmu:
 	free_engines(engines);
+err_engines:
 	free(pmu_device);
 exit:
 	igt_devices_free();
-- 
2.34.1

Re: [Intel-gfx] [PATCH i-g-t 5/6] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

[Kamil Konieczny]

Hi Tvrtko,

On 2023-01-27 at 11:12:40 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Avoid trying to dereference null engines on exit when there are either
> none which are supported, or kernel does not have i915 PMU support.
> 
> Also fix a memory leak on the same failure path just so Valgrind runs are
> quite.
> 
> v2:
>  * Fix a memory leak in the same failure mode too.

Please rebase, patch do not apply.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Acked-by: Nirmoy Das <nirmoy.das@intel.com> # v1
-------------------------------------------- ^^^^^
Delete this.

Rest looks good,

Regards,
Kamil

> ---
>  tools/intel_gpu_top.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 7aa233570463..0a1de41b3374 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -340,7 +340,7 @@ static struct engines *discover_engines(char *device)
>  
>  	d = opendir(sysfs_root);
>  	if (!d)
> -		return NULL;
> +		goto err;
>  
>  	while ((dent = readdir(d)) != NULL) {
>  		const char *endswith = "-busy";
> @@ -423,10 +423,8 @@ static struct engines *discover_engines(char *device)
>  	}
>  
>  	if (ret) {
> -		free(engines);
>  		errno = ret;
> -
> -		return NULL;
> +		goto err;
>  	}
>  
>  	qsort(engine_ptr(engines, 0), engines->num_engines,
> @@ -435,6 +433,11 @@ static struct engines *discover_engines(char *device)
>  	engines->root = d;
>  
>  	return engines;
> +
> +err:
> +	free(engines);
> +
> +	return NULL;
>  }
>  
>  static void free_engines(struct engines *engines)
> @@ -448,6 +451,9 @@ static void free_engines(struct engines *engines)
>  	};
>  	unsigned int i;
>  
> +	if (!engines)
> +		return;
> +
>  	for (pmu = &free_list[0]; *pmu; pmu++) {
>  		if ((*pmu)->present)
>  			free((char *)(*pmu)->units);
> @@ -2568,7 +2574,7 @@ int main(int argc, char **argv)
>  			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
>  			strerror(errno));
>  		ret = EXIT_FAILURE;
> -		goto err;
> +		goto err_engines;
>  	}
>  
>  	ret = pmu_init(engines);
> @@ -2585,7 +2591,7 @@ int main(int argc, char **argv)
>  "More information can be found at 'Perf events and tool security' document:\n"
>  "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>  		ret = EXIT_FAILURE;
> -		goto err;
> +		goto err_pmu;
>  	}
>  
>  	ret = EXIT_SUCCESS;
> @@ -2699,8 +2705,9 @@ int main(int argc, char **argv)
>  		free_clients(clients);
>  
>  	free(codename);
> -err:
> +err_pmu:
>  	free_engines(engines);
> +err_engines:
>  	free(pmu_device);
>  exit:
>  	igt_devices_free();
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 5/6] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

[Tvrtko Ursulin]

On 27/01/2023 16:10, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> On 2023-01-27 at 11:12:40 +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Avoid trying to dereference null engines on exit when there are either
>> none which are supported, or kernel does not have i915 PMU support.
>>
>> Also fix a memory leak on the same failure path just so Valgrind runs are
>> quite.
>>
>> v2:
>>   * Fix a memory leak in the same failure mode too.
> 
> Please rebase, patch do not apply.

Hm how, CI applied it fine. Maybe you mean as standalone? There is the 
same patch here: 
https://patchwork.freedesktop.org/patch/519751/?series=113096&rev=2

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Acked-by: Nirmoy Das <nirmoy.das@intel.com> # v1
> -------------------------------------------- ^^^^^
> Delete this.

I can do that only if Nirmoy agrees. ;)

Regards,

Tvrtko

> Rest looks good,
> 
> Regards,
> Kamil
> 
>> ---
>>   tools/intel_gpu_top.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>> index 7aa233570463..0a1de41b3374 100644
>> --- a/tools/intel_gpu_top.c
>> +++ b/tools/intel_gpu_top.c
>> @@ -340,7 +340,7 @@ static struct engines *discover_engines(char *device)
>>   
>>   	d = opendir(sysfs_root);
>>   	if (!d)
>> -		return NULL;
>> +		goto err;
>>   
>>   	while ((dent = readdir(d)) != NULL) {
>>   		const char *endswith = "-busy";
>> @@ -423,10 +423,8 @@ static struct engines *discover_engines(char *device)
>>   	}
>>   
>>   	if (ret) {
>> -		free(engines);
>>   		errno = ret;
>> -
>> -		return NULL;
>> +		goto err;
>>   	}
>>   
>>   	qsort(engine_ptr(engines, 0), engines->num_engines,
>> @@ -435,6 +433,11 @@ static struct engines *discover_engines(char *device)
>>   	engines->root = d;
>>   
>>   	return engines;
>> +
>> +err:
>> +	free(engines);
>> +
>> +	return NULL;
>>   }
>>   
>>   static void free_engines(struct engines *engines)
>> @@ -448,6 +451,9 @@ static void free_engines(struct engines *engines)
>>   	};
>>   	unsigned int i;
>>   
>> +	if (!engines)
>> +		return;
>> +
>>   	for (pmu = &free_list[0]; *pmu; pmu++) {
>>   		if ((*pmu)->present)
>>   			free((char *)(*pmu)->units);
>> @@ -2568,7 +2574,7 @@ int main(int argc, char **argv)
>>   			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
>>   			strerror(errno));
>>   		ret = EXIT_FAILURE;
>> -		goto err;
>> +		goto err_engines;
>>   	}
>>   
>>   	ret = pmu_init(engines);
>> @@ -2585,7 +2591,7 @@ int main(int argc, char **argv)
>>   "More information can be found at 'Perf events and tool security' document:\n"
>>   "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>>   		ret = EXIT_FAILURE;
>> -		goto err;
>> +		goto err_pmu;
>>   	}
>>   
>>   	ret = EXIT_SUCCESS;
>> @@ -2699,8 +2705,9 @@ int main(int argc, char **argv)
>>   		free_clients(clients);
>>   
>>   	free(codename);
>> -err:
>> +err_pmu:
>>   	free_engines(engines);
>> +err_engines:
>>   	free(pmu_device);
>>   exit:
>>   	igt_devices_free();
>> -- 
>> 2.34.1
>>

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 5/6] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

[Das, Nirmoy]

On 1/30/2023 11:55 AM, Tvrtko Ursulin wrote:
>
> On 27/01/2023 16:10, Kamil Konieczny wrote:
>> Hi Tvrtko,
>>
>> On 2023-01-27 at 11:12:40 +0000, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Avoid trying to dereference null engines on exit when there are either
>>> none which are supported, or kernel does not have i915 PMU support.
>>>
>>> Also fix a memory leak on the same failure path just so Valgrind 
>>> runs are
>>> quite.
>>>
>>> v2:
>>>   * Fix a memory leak in the same failure mode too.
>>
>> Please rebase, patch do not apply.
>
> Hm how, CI applied it fine. Maybe you mean as standalone? There is the 
> same patch here: 
> https://patchwork.freedesktop.org/patch/519751/?series=113096&rev=2
>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Acked-by: Nirmoy Das <nirmoy.das@intel.com> # v1
>> -------------------------------------------- ^^^^^
>> Delete this.
>
> I can do that only if Nirmoy agrees. ;)


Sorry I missed this, my a-b stays as it is with the new version of this 
patch.


Regards,

Nirmoy

>
> Regards,
>
> Tvrtko
>
>> Rest looks good,
>>
>> Regards,
>> Kamil
>>
>>> ---
>>>   tools/intel_gpu_top.c | 21 ++++++++++++++-------
>>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>>> index 7aa233570463..0a1de41b3374 100644
>>> --- a/tools/intel_gpu_top.c
>>> +++ b/tools/intel_gpu_top.c
>>> @@ -340,7 +340,7 @@ static struct engines *discover_engines(char 
>>> *device)
>>>         d = opendir(sysfs_root);
>>>       if (!d)
>>> -        return NULL;
>>> +        goto err;
>>>         while ((dent = readdir(d)) != NULL) {
>>>           const char *endswith = "-busy";
>>> @@ -423,10 +423,8 @@ static struct engines *discover_engines(char 
>>> *device)
>>>       }
>>>         if (ret) {
>>> -        free(engines);
>>>           errno = ret;
>>> -
>>> -        return NULL;
>>> +        goto err;
>>>       }
>>>         qsort(engine_ptr(engines, 0), engines->num_engines,
>>> @@ -435,6 +433,11 @@ static struct engines *discover_engines(char 
>>> *device)
>>>       engines->root = d;
>>>         return engines;
>>> +
>>> +err:
>>> +    free(engines);
>>> +
>>> +    return NULL;
>>>   }
>>>     static void free_engines(struct engines *engines)
>>> @@ -448,6 +451,9 @@ static void free_engines(struct engines *engines)
>>>       };
>>>       unsigned int i;
>>>   +    if (!engines)
>>> +        return;
>>> +
>>>       for (pmu = &free_list[0]; *pmu; pmu++) {
>>>           if ((*pmu)->present)
>>>               free((char *)(*pmu)->units);
>>> @@ -2568,7 +2574,7 @@ int main(int argc, char **argv)
>>>               "Failed to detect engines! (%s)\n(Kernel 4.16 or newer 
>>> is required for i915 PMU support.)\n",
>>>               strerror(errno));
>>>           ret = EXIT_FAILURE;
>>> -        goto err;
>>> +        goto err_engines;
>>>       }
>>>         ret = pmu_init(engines);
>>> @@ -2585,7 +2591,7 @@ int main(int argc, char **argv)
>>>   "More information can be found at 'Perf events and tool security' 
>>> document:\n"
>>> "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>>>           ret = EXIT_FAILURE;
>>> -        goto err;
>>> +        goto err_pmu;
>>>       }
>>>         ret = EXIT_SUCCESS;
>>> @@ -2699,8 +2705,9 @@ int main(int argc, char **argv)
>>>           free_clients(clients);
>>>         free(codename);
>>> -err:
>>> +err_pmu:
>>>       free_engines(engines);
>>> +err_engines:
>>>       free(pmu_device);
>>>   exit:
>>>       igt_devices_free();
>>> -- 
>>> 2.34.1
>>>

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 5/6] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

[Kamil Konieczny]

Hi,

On 2023-01-30 at 10:55:42 +0000, Tvrtko Ursulin wrote:
> 
> On 27/01/2023 16:10, Kamil Konieczny wrote:
> > Hi Tvrtko,
> > 
> > On 2023-01-27 at 11:12:40 +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > Avoid trying to dereference null engines on exit when there are either
> > > none which are supported, or kernel does not have i915 PMU support.
> > > 
> > > Also fix a memory leak on the same failure path just so Valgrind runs are
> > > quite.
> > > 
> > > v2:
> > >   * Fix a memory leak in the same failure mode too.
> > 
> > Please rebase, patch do not apply.
> 
> Hm how, CI applied it fine. Maybe you mean as standalone? There is the same
> patch here:
> https://patchwork.freedesktop.org/patch/519751/?series=113096&rev=2
> 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Acked-by: Nirmoy Das <nirmoy.das@intel.com> # v1
> > -------------------------------------------- ^^^^^
> > Delete this.
> 
> I can do that only if Nirmoy agrees. ;)
> 
> Regards,
> 
> Tvrtko

It is already too late, that was merged some time ago and got
into git history so nothing can be done now.

Regards,
Kamil

> 
> > Rest looks good,
> > 
> > Regards,
> > Kamil
> > 
> > > ---
> > >   tools/intel_gpu_top.c | 21 ++++++++++++++-------
> > >   1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> > > index 7aa233570463..0a1de41b3374 100644
> > > --- a/tools/intel_gpu_top.c
> > > +++ b/tools/intel_gpu_top.c
> > > @@ -340,7 +340,7 @@ static struct engines *discover_engines(char *device)
> > >   	d = opendir(sysfs_root);
> > >   	if (!d)
> > > -		return NULL;
> > > +		goto err;
> > >   	while ((dent = readdir(d)) != NULL) {
> > >   		const char *endswith = "-busy";
> > > @@ -423,10 +423,8 @@ static struct engines *discover_engines(char *device)
> > >   	}
> > >   	if (ret) {
> > > -		free(engines);
> > >   		errno = ret;
> > > -
> > > -		return NULL;
> > > +		goto err;
> > >   	}
> > >   	qsort(engine_ptr(engines, 0), engines->num_engines,
> > > @@ -435,6 +433,11 @@ static struct engines *discover_engines(char *device)
> > >   	engines->root = d;
> > >   	return engines;
> > > +
> > > +err:
> > > +	free(engines);
> > > +
> > > +	return NULL;
> > >   }
> > >   static void free_engines(struct engines *engines)
> > > @@ -448,6 +451,9 @@ static void free_engines(struct engines *engines)
> > >   	};
> > >   	unsigned int i;
> > > +	if (!engines)
> > > +		return;
> > > +
> > >   	for (pmu = &free_list[0]; *pmu; pmu++) {
> > >   		if ((*pmu)->present)
> > >   			free((char *)(*pmu)->units);
> > > @@ -2568,7 +2574,7 @@ int main(int argc, char **argv)
> > >   			"Failed to detect engines! (%s)\n(Kernel 4.16 or newer is required for i915 PMU support.)\n",
> > >   			strerror(errno));
> > >   		ret = EXIT_FAILURE;
> > > -		goto err;
> > > +		goto err_engines;
> > >   	}
> > >   	ret = pmu_init(engines);
> > > @@ -2585,7 +2591,7 @@ int main(int argc, char **argv)
> > >   "More information can be found at 'Perf events and tool security' document:\n"
> > >   "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
> > >   		ret = EXIT_FAILURE;
> > > -		goto err;
> > > +		goto err_pmu;
> > >   	}
> > >   	ret = EXIT_SUCCESS;
> > > @@ -2699,8 +2705,9 @@ int main(int argc, char **argv)
> > >   		free_clients(clients);
> > >   	free(codename);
> > > -err:
> > > +err_pmu:
> > >   	free_engines(engines);
> > > +err_engines:
> > >   	free(pmu_device);
> > >   exit:
> > >   	igt_devices_free();
> > > -- 
> > > 2.34.1
> > > 

[Intel-gfx] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Now that DRM subsystem can contain PCI cards with the vendor set to Intel
but they are not Intel GPUs, we need a better selection logic than looking
at the vendor. Use the driver name instead.

Caveat that the driver key was on a blacklist so far, and although I can't
imagine it can be slow to probe, this is something to double check.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 lib/igt_device_scan.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index ed128d24dd10..8b767eed202d 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -237,6 +237,7 @@ struct igt_device {
 	char *vendor;
 	char *device;
 	char *pci_slot_name;
+	char *driver;
 	int gpu_index; /* For more than one GPU with same vendor and device. */
 
 	char *codename; /* For grouping by codename */
@@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
 				      "resource3", "resource4", "resource5",
 				      "resource0_wc", "resource1_wc", "resource2_wc",
 				      "resource3_wc", "resource4_wc", "resource5_wc",
-				      "driver",
 				      "uevent", NULL};
 	const char *key;
 	int i = 0;
@@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
 		get_pci_vendor_device(idev, &vendor, &device);
 		idev->codename = __pci_codename(vendor, device);
 		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
+		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
+		igt_assert(idev->driver);
 	}
 
 	return idev;
@@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
 
 	igt_list_for_each_entry(dev, &igt_devs.all, link) {
 
-		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
+		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
 			continue;
 
 		cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
@@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
 	free(dev->drm_render);
 	free(dev->vendor);
 	free(dev->device);
+	free(dev->driver);
 	free(dev->pci_slot_name);
 	g_hash_table_destroy(dev->attrs_ht);
 	g_hash_table_destroy(dev->props_ht);
-- 
2.34.1

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Petri Latvala]

On Fri, Jan 27, 2023 at 11:12:41AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Now that DRM subsystem can contain PCI cards with the vendor set to Intel
> but they are not Intel GPUs, we need a better selection logic than looking
> at the vendor. Use the driver name instead.
> 
> Caveat that the driver key was on a blacklist so far, and although I can't
> imagine it can be slow to probe, this is something to double check.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>  lib/igt_device_scan.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index ed128d24dd10..8b767eed202d 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -237,6 +237,7 @@ struct igt_device {
>  	char *vendor;
>  	char *device;
>  	char *pci_slot_name;
> +	char *driver;
>  	int gpu_index; /* For more than one GPU with same vendor and device. */
>  
>  	char *codename; /* For grouping by codename */
> @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
>  				      "resource3", "resource4", "resource5",
>  				      "resource0_wc", "resource1_wc", "resource2_wc",
>  				      "resource3_wc", "resource4_wc", "resource5_wc",
> -				      "driver",
>  				      "uevent", NULL};
>  	const char *key;
>  	int i = 0;
> @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
>  		get_pci_vendor_device(idev, &vendor, &device);
>  		idev->codename = __pci_codename(vendor, device);
>  		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
> +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
> +		igt_assert(idev->driver);
>  	}
>  
>  	return idev;
> @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
>  
>  	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>  
> -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))

Is this the time to prepare for that other driver as well?


-- 
Petri Latvala


>  			continue;
>  
>  		cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
>  	free(dev->drm_render);
>  	free(dev->vendor);
>  	free(dev->device);
> +	free(dev->driver);
>  	free(dev->pci_slot_name);
>  	g_hash_table_destroy(dev->attrs_ht);
>  	g_hash_table_destroy(dev->props_ht);
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Tvrtko Ursulin]

On 27/01/2023 11:39, Petri Latvala wrote:
> On Fri, Jan 27, 2023 at 11:12:41AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Now that DRM subsystem can contain PCI cards with the vendor set to Intel
>> but they are not Intel GPUs, we need a better selection logic than looking
>> at the vendor. Use the driver name instead.
>>
>> Caveat that the driver key was on a blacklist so far, and although I can't
>> imagine it can be slow to probe, this is something to double check.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>> ---
>>   lib/igt_device_scan.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index ed128d24dd10..8b767eed202d 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -237,6 +237,7 @@ struct igt_device {
>>   	char *vendor;
>>   	char *device;
>>   	char *pci_slot_name;
>> +	char *driver;
>>   	int gpu_index; /* For more than one GPU with same vendor and device. */
>>   
>>   	char *codename; /* For grouping by codename */
>> @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
>>   				      "resource3", "resource4", "resource5",
>>   				      "resource0_wc", "resource1_wc", "resource2_wc",
>>   				      "resource3_wc", "resource4_wc", "resource5_wc",
>> -				      "driver",
>>   				      "uevent", NULL};
>>   	const char *key;
>>   	int i = 0;
>> @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
>>   		get_pci_vendor_device(idev, &vendor, &device);
>>   		idev->codename = __pci_codename(vendor, device);
>>   		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
>> +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
>> +		igt_assert(idev->driver);
>>   	}
>>   
>>   	return idev;
>> @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
>>   
>>   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>>   
>> -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
>> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
> 
> Is this the time to prepare for that other driver as well?

Ha, didn't think of that TBH, but AFAICT this patch will work for that 
case too, no?

Regards,

Tvrtko

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Petri Latvala]

On Fri, Jan 27, 2023 at 11:53:38AM +0000, Tvrtko Ursulin wrote:
> 
> On 27/01/2023 11:39, Petri Latvala wrote:
> > On Fri, Jan 27, 2023 at 11:12:41AM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > Now that DRM subsystem can contain PCI cards with the vendor set to Intel
> > > but they are not Intel GPUs, we need a better selection logic than looking
> > > at the vendor. Use the driver name instead.
> > > 
> > > Caveat that the driver key was on a blacklist so far, and although I can't
> > > imagine it can be slow to probe, this is something to double check.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > > ---
> > >   lib/igt_device_scan.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > > index ed128d24dd10..8b767eed202d 100644
> > > --- a/lib/igt_device_scan.c
> > > +++ b/lib/igt_device_scan.c
> > > @@ -237,6 +237,7 @@ struct igt_device {
> > >   	char *vendor;
> > >   	char *device;
> > >   	char *pci_slot_name;
> > > +	char *driver;
> > >   	int gpu_index; /* For more than one GPU with same vendor and device. */
> > >   	char *codename; /* For grouping by codename */
> > > @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
> > >   				      "resource3", "resource4", "resource5",
> > >   				      "resource0_wc", "resource1_wc", "resource2_wc",
> > >   				      "resource3_wc", "resource4_wc", "resource5_wc",
> > > -				      "driver",
> > >   				      "uevent", NULL};
> > >   	const char *key;
> > >   	int i = 0;
> > > @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
> > >   		get_pci_vendor_device(idev, &vendor, &device);
> > >   		idev->codename = __pci_codename(vendor, device);
> > >   		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
> > > +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
> > > +		igt_assert(idev->driver);
> > >   	}
> > >   	return idev;
> > > @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
> > >   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> > > -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> > > +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
> > 
> > Is this the time to prepare for that other driver as well?
> 
> Ha, didn't think of that TBH, but AFAICT this patch will work for that case
> too, no?

Ah, now that I read the surrounding code...

Indeed the function is for finding i915 devices in particular, used by
gem_wsim and intel_gpu_top. Having that function find devices driven
by xe might lead to incompatibilities that need to be resolved or at
least compatibility fully understood, which is not the case for either
at this time I assume. In other words, out of scope for this patch.


-- 
Petri Latvala

Re: [Intel-gfx] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Kamil Konieczny]

Hi Tvrtko,

On 2023-01-27 at 11:12:41 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Now that DRM subsystem can contain PCI cards with the vendor set to Intel
> but they are not Intel GPUs, we need a better selection logic than looking
> at the vendor. Use the driver name instead.
> 
> Caveat that the driver key was on a blacklist so far, and although I can't
> imagine it can be slow to probe, this is something to double check.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

Please send this as separate patch, not in this series.

> ---
>  lib/igt_device_scan.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index ed128d24dd10..8b767eed202d 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -237,6 +237,7 @@ struct igt_device {
>  	char *vendor;
>  	char *device;
>  	char *pci_slot_name;
> +	char *driver;
>  	int gpu_index; /* For more than one GPU with same vendor and device. */
>  
>  	char *codename; /* For grouping by codename */
> @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
>  				      "resource3", "resource4", "resource5",
>  				      "resource0_wc", "resource1_wc", "resource2_wc",
>  				      "resource3_wc", "resource4_wc", "resource5_wc",
> -				      "driver",
>  				      "uevent", NULL};
>  	const char *key;
>  	int i = 0;
> @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
>  		get_pci_vendor_device(idev, &vendor, &device);
>  		idev->codename = __pci_codename(vendor, device);
>  		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
> +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
> +		igt_assert(idev->driver);
>  	}
>  
>  	return idev;
> @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
>  
>  	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>  
> -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))

Put the comment here why it can be problematic to relay on driver name.

Regards,
Kamil

>  			continue;
>  
>  		cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
>  	free(dev->drm_render);
>  	free(dev->vendor);
>  	free(dev->device);
> +	free(dev->driver);
>  	free(dev->pci_slot_name);
>  	g_hash_table_destroy(dev->attrs_ht);
>  	g_hash_table_destroy(dev->props_ht);
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Tvrtko Ursulin]

On 27/01/2023 16:17, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> On 2023-01-27 at 11:12:41 +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Now that DRM subsystem can contain PCI cards with the vendor set to Intel
>> but they are not Intel GPUs, we need a better selection logic than looking
>> at the vendor. Use the driver name instead.
>>
>> Caveat that the driver key was on a blacklist so far, and although I can't
>> imagine it can be slow to probe, this is something to double check.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> 
> Please send this as separate patch, not in this series.

Yeah I was lazy and wanting to save time so okay.

>> ---
>>   lib/igt_device_scan.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index ed128d24dd10..8b767eed202d 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -237,6 +237,7 @@ struct igt_device {
>>   	char *vendor;
>>   	char *device;
>>   	char *pci_slot_name;
>> +	char *driver;
>>   	int gpu_index; /* For more than one GPU with same vendor and device. */
>>   
>>   	char *codename; /* For grouping by codename */
>> @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
>>   				      "resource3", "resource4", "resource5",
>>   				      "resource0_wc", "resource1_wc", "resource2_wc",
>>   				      "resource3_wc", "resource4_wc", "resource5_wc",
>> -				      "driver",
>>   				      "uevent", NULL};
>>   	const char *key;
>>   	int i = 0;
>> @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
>>   		get_pci_vendor_device(idev, &vendor, &device);
>>   		idev->codename = __pci_codename(vendor, device);
>>   		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
>> +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
>> +		igt_assert(idev->driver);
>>   	}
>>   
>>   	return idev;
>> @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
>>   
>>   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>>   
>> -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
>> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
> 
> Put the comment here why it can be problematic to relay on driver name.

Function name being __find_first_*i915*_card is IMO enough so it feels 
any comment to the same effect would be redundant.

Hm if anything igt_device_find_integrated_card should be renamed..

Regards,

Tvrtko

> 
> Regards,
> Kamil
> 
>>   			continue;
>>   
>>   		cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
>> @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
>>   	free(dev->drm_render);
>>   	free(dev->vendor);
>>   	free(dev->device);
>> +	free(dev->driver);
>>   	free(dev->pci_slot_name);
>>   	g_hash_table_destroy(dev->attrs_ht);
>>   	g_hash_table_destroy(dev->props_ht);
>> -- 
>> 2.34.1
>>

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Kamil Konieczny]

Hi,

On 2023-01-30 at 11:04:07 +0000, Tvrtko Ursulin wrote:
> 
> On 27/01/2023 16:17, Kamil Konieczny wrote:
> > Hi Tvrtko,
> > 
> > On 2023-01-27 at 11:12:41 +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > Now that DRM subsystem can contain PCI cards with the vendor set to Intel
> > > but they are not Intel GPUs, we need a better selection logic than looking
> > > at the vendor. Use the driver name instead.
> > > 
> > > Caveat that the driver key was on a blacklist so far, and although I can't
> > > imagine it can be slow to probe, this is something to double check.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > 
> > Please send this as separate patch, not in this series.
> 
> Yeah I was lazy and wanting to save time so okay.
> 

Well maybe next time, I already merged your series without 5/6,
that one were merged some time ago.

Regards,
Kamil

> > > ---
> > >   lib/igt_device_scan.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > > index ed128d24dd10..8b767eed202d 100644
> > > --- a/lib/igt_device_scan.c
> > > +++ b/lib/igt_device_scan.c
> > > @@ -237,6 +237,7 @@ struct igt_device {
> > >   	char *vendor;
> > >   	char *device;
> > >   	char *pci_slot_name;
> > > +	char *driver;
> > >   	int gpu_index; /* For more than one GPU with same vendor and device. */
> > >   	char *codename; /* For grouping by codename */
> > > @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
> > >   				      "resource3", "resource4", "resource5",
> > >   				      "resource0_wc", "resource1_wc", "resource2_wc",
> > >   				      "resource3_wc", "resource4_wc", "resource5_wc",
> > > -				      "driver",
> > >   				      "uevent", NULL};
> > >   	const char *key;
> > >   	int i = 0;
> > > @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
> > >   		get_pci_vendor_device(idev, &vendor, &device);
> > >   		idev->codename = __pci_codename(vendor, device);
> > >   		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
> > > +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
> > > +		igt_assert(idev->driver);
> > >   	}
> > >   	return idev;
> > > @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
> > >   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> > > -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> > > +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
> > 
> > Put the comment here why it can be problematic to relay on driver name.
> 
> Function name being __find_first_*i915*_card is IMO enough so it feels any
> comment to the same effect would be redundant.
> 
> Hm if anything igt_device_find_integrated_card should be renamed..
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Regards,
> > Kamil
> > 
> > >   			continue;
> > >   		cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> > > @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
> > >   	free(dev->drm_render);
> > >   	free(dev->vendor);
> > >   	free(dev->device);
> > > +	free(dev->driver);
> > >   	free(dev->pci_slot_name);
> > >   	g_hash_table_destroy(dev->attrs_ht);
> > >   	g_hash_table_destroy(dev->props_ht);
> > > -- 
> > > 2.34.1
> > > 

Re: [Intel-gfx] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Zbigniew Kempczyński]

On Fri, Jan 27, 2023 at 11:12:41AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Now that DRM subsystem can contain PCI cards with the vendor set to Intel
> but they are not Intel GPUs, we need a better selection logic than looking
> at the vendor. Use the driver name instead.
> 
> Caveat that the driver key was on a blacklist so far, and although I can't
> imagine it can be slow to probe, this is something to double check.

I don't remember why driver was added to the list. But you're right, driver
instead of vendor gives clear situation what drm device we're working on.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>  lib/igt_device_scan.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index ed128d24dd10..8b767eed202d 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -237,6 +237,7 @@ struct igt_device {
>  	char *vendor;
>  	char *device;
>  	char *pci_slot_name;
> +	char *driver;
>  	int gpu_index; /* For more than one GPU with same vendor and device. */
>  
>  	char *codename; /* For grouping by codename */
> @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
>  				      "resource3", "resource4", "resource5",
>  				      "resource0_wc", "resource1_wc", "resource2_wc",
>  				      "resource3_wc", "resource4_wc", "resource5_wc",
> -				      "driver",
>  				      "uevent", NULL};
>  	const char *key;
>  	int i = 0;
> @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
>  		get_pci_vendor_device(idev, &vendor, &device);
>  		idev->codename = __pci_codename(vendor, device);
>  		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
> +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
> +		igt_assert(idev->driver);
>  	}
>  
>  	return idev;
> @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
>  
>  	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>  
> -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))

We can easily add finding xe card in the future similar to the above. From me:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

--
Zbigniew

>  			continue;
>  
>  		cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
>  	free(dev->drm_render);
>  	free(dev->vendor);
>  	free(dev->device);
> +	free(dev->driver);
>  	free(dev->pci_slot_name);
>  	g_hash_table_destroy(dev->attrs_ht);
>  	g_hash_table_destroy(dev->props_ht);
> -- 
> 2.34.1
>