[PATCH v2 2/2] tools/gputop: toggle sysfs profiling knob if available for device

[Lucas De Marchi]

On Tue, Apr 16, 2024 at 08:49:46AM +0100, Tvrtko Ursulin wrote:
>
>On 15/04/2024 20:35, Adrián Larumbe wrote:
>>On 03.04.2024 19:22, Kamil Konieczny wrote:
>>>Hi Adrián,
>>>On 2024-04-02 at 23:27:45 +0100, Adrián Larumbe wrote:
>>>>For every DRM device that enables its job accounting HW from user space,
>>>>toggle it right before obtaining per-client fdinfo numbers.
>>>>
>>>>Make sure profiling status is returned to its original state before
>>>>exiting, by handling the SIGINT signal just like in intel_gpu_top.
>>>>
>>>>Also dynamically link gputop against igt lib instead of separate per-file
>>>>static libraries to avoid dependence cascade because of using igt_sysfs
>>>>functions.
>>>>
>>>>Cc: Tvrtko Ursulin <tursulin at ursulin.net>
>>>>Cc: Daniel Vetter <daniel at ffwll.ch>
>>>>Cc: Boris Brezillon <boris.brezillon at collabora.com>
>>>>Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>>>>Signed-off-by: Adrián Larumbe <adrian.larumbe at collabora.com>
>>>>---
>>>>  tools/gputop.c    | 30 +++++++++++++++++++++++++++++-
>>>>  tools/meson.build |  2 +-
>>>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>>diff --git a/tools/gputop.c b/tools/gputop.c
>>>>index 71e28f43ee4c..5b390cbdb3e7 100644
>>>>--- a/tools/gputop.c
>>>>+++ b/tools/gputop.c
>>>>@@ -26,8 +26,10 @@
>>>>  #include <sys/sysmacros.h>
>>>>  #include <stdbool.h>
>>>>+#include "igt_device_scan.h"
>>>>  #include "igt_drm_clients.h"
>>>>  #include "igt_drm_fdinfo.h"
>>>>+#include "igt_profiling.h"
>>>>  #include "drmtest.h"
>>>>  static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
>>>>@@ -243,9 +245,17 @@ static int client_cmp(const void *_a, const void *_b, void *unused)
>>>>  }
>>>>+static volatile bool stop_top;
>>>>+
>>>>+static void sigint_handler(int  sig)
>>>>+{
>>>>+	stop_top = true;
>>>>+}
>>>>+
>>>>  int main(int argc, char **argv)
>>>>  {
>>>>  	unsigned int period_us = 2e6;
>>>>+	struct igt_profiled_device *profiled_devices = NULL;
>>>>  	struct igt_drm_clients *clients = NULL;
>>>>  	int con_w = -1, con_h = -1;
>>>>@@ -253,9 +263,22 @@ int main(int argc, char **argv)
>>>>  	if (!clients)
>>>>  		exit(1);
>>>>+	igt_devices_scan(false);
>>>>+
>>>>+	profiled_devices = igt_devices_profiled();
>>>>+	if (profiled_devices != NULL)
>>>>+		igt_devices_toggle_profiling(profiled_devices, true);
>>>>+
>>>>+	if (signal(SIGINT, sigint_handler) == SIG_ERR) {
>>>>+		fprintf(stderr, "Failed to install signal handler!\n");
>>>>+		igt_devices_toggle_profiling(profiled_devices, false);
>>>>+		free(profiled_devices);
>>>>+		profiled_devices = NULL;
>>>>+	}
>>>>+
>>>>  	igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
>>>>-	for (;;) {
>>>>+	while (!stop_top) {
>>>>  		struct igt_drm_client *c, *prevc = NULL;
>>>>  		int i, engine_w = 0, lines = 0;
>>>>  		struct winsize ws;
>>>>@@ -293,5 +316,10 @@ int main(int argc, char **argv)
>>>>  		usleep(period_us);
>>>>  	}
>>>>+	if (profiled_devices != NULL) {
>>>>+		igt_devices_toggle_profiling(profiled_devices, false);
>>>>+		free(profiled_devices);
>>>>+	}
>>>>+
>>>>  	return 0;
>>>>  }
>>>>diff --git a/tools/meson.build b/tools/meson.build
>>>>index ac79d8b5840c..a126d6cad7ba 100644
>>>>--- a/tools/meson.build
>>>>+++ b/tools/meson.build
>>>>@@ -68,7 +68,7 @@ endif
>>>>  executable('gputop', 'gputop.c',
>>>>             install : true,
>>>>             install_rpath : bindir_rpathdir,
>>>>-           dependencies : [lib_igt_drm_clients,lib_igt_drm_fdinfo,math])
>>>>+           dependencies : [lib_igt, lib_igt_drm_clients, math])
>>>
>>>Why not creating lib_igt_profiling with only needed dependeces and
>>>adding it here? Before your patch:
>>>
>>>ldd build/tools/gputop
>>>
>>>linux-vdso.so.1 (0x00007ffc5e120000)
>>>libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007bf0a4200000)
>>>/lib64/ld-linux-x86-64.so.2 (0x00007bf0a452b000)
>>>
>>>After:
>>>
>>>ldd gputop |wc -l
>>>78
>>
>>I guess if dynamic library dependencies are seen as undesirable for gputop, then
>>I'll try to do as you suggested, but I suspect a hypothetical lib_igt_profiling
>>static library will contain most of the object files in igt_lib's lib_sources
>>list of files.
>
>It is correct that a goal so far was to avoid linking tools/ (outside 
>tests/ even) with igt_igt. But you use quite a lot of lib_igt stuff in 
>this work right? I am not sure it is easy to split it all out into 
>separate libraries. Question for IGT maintainers.

I think we need to consider what gputop is. I can think of 2 things:

1) reference implementation of the kernel interface

Then maybe hiding the real implementation across several igt libraries
is not a good thing.

2) a tool we really expect end users to use

Then maybe it's ok to depend on those additional libraries, but
ldd gputop |wc -l == 78  for what gputop does seem too much.

3) both (1) and (2)?

Lucas De Marchi

>
>Regards,
>
>Tvrtko
>
>>
>>>Regards,
>>>Kamil
>>>
>>>>  intel_l3_parity_src = [ 'intel_l3_parity.c', 'intel_l3_udev_listener.c' ]
>>>>  executable('intel_l3_parity', sources : intel_l3_parity_src,
>>>>-- 
>>>>2.44.0
>>>>
>>
>>Adrian Larumbe