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

[Adrián Larumbe]

On 18.03.2024 13:40, Kamil Konieczny wrote:
> Hi Adrián,
> On 2024-03-16 at 22:00:38 +0000, 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.
> > 
> > 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..d6885be6aa16 100644
> > --- a/tools/gputop.c
> > +++ b/tools/gputop.c
> > @@ -27,7 +27,9 @@
> >  #include <stdbool.h>
> >  
> >  #include "igt_drm_clients.h"
> > +#include "igt_device_scan.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 bool stop_top;
> --------- ^^^^^^
> Add 'volatile' here as compiler could optimise your loop into
> while(true).
> 
> > +
> > +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])
> ----------------------------- ^^^^^^^
> This will blow-up size of tool, also why did you removed
> lib_igt_drm_fdinfo?

I removed it because igt_drm_fdinfo.c is part of lib_sources, and since I decided to link gputop
dynamically against libigt, it didn't make any sense to have the same symbols provided by both
a static archive and a dynamic library (although I guess the linker will pick them up from whatever
shows up first in the command line).

As to the new tool size, I think it'd be smaller than before, because it's dynamically linked.
Although if this is unacceptable I could traverse the whole tree of dependencies and create a new
archive dependency for gputop that includes both igt_profiling.c, igt_sysfs.c and everything
they depend on.

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