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

[Tvrtko Ursulin <tursulin@ursulin.net>]

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@ursulin.net>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@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.

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