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

[Zbigniew Kempczyński]

On Thu, May 16, 2024 at 08:49:19PM +0100, Adrián Larumbe wrote:
> On 13.05.2024 09:17, Zbigniew Kempczyński wrote:
> > On Fri, May 10, 2024 at 08:22:38PM +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 add the static IGT profiling library to gputop's list of build
> > > dependencies.
> > > 
> > > Cc: Tvrtko Ursulin <tursulin at ursulin.net>
> > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe at collabora.com>
> > > ---
> > >  tools/gputop.c    | 33 ++++++++++++++++++++++++++++++++-
> > >  tools/meson.build |  2 +-
> > >  2 files changed, 33 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/gputop.c b/tools/gputop.c
> > > index 5634fa58a0c7..7ae3d6411cf1 100644
> > > --- a/tools/gputop.c
> > > +++ b/tools/gputop.c
> > > @@ -29,6 +29,7 @@
> > >  #include "igt_core.h"
> > >  #include "igt_drm_clients.h"
> > >  #include "igt_drm_fdinfo.h"
> > > +#include "igt_profiling.h"
> > >  #include "drmtest.h"
> > >  
> > >  static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> > > @@ -358,10 +359,19 @@ static int parse_args(int argc, char * const argv[], struct gputop_args *args)
> > >  	return 1;
> > >  }
> > >  
> > > +static volatile bool stop_top;
> > > +
> > > +static void sigint_handler(int sig)
> > > +{
> > > +	(void) sig;
> > > +	stop_top = true;
> > > +}
> > > +
> > >  int main(int argc, char **argv)
> > >  {
> > >  	struct gputop_args args;
> > >  	unsigned int period_us;
> > > +	struct igt_profiled_device *profiled_devices = NULL;
> > >  	struct igt_drm_clients *clients = NULL;
> > >  	int con_w = -1, con_h = -1;
> > >  	int ret;
> > > @@ -380,9 +390,22 @@ int main(int argc, char **argv)
> > >  	if (!clients)
> > >  		exit(1);
> > >  
> > > +
> > > +	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);
> > > +			igt_devices_free_profiling(profiled_devices);
> > > +			profiled_devices = NULL;
> > > +		}
> > > +	}
> > > +
> > >  	igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
> > >  
> > > -	while (n != 0) {
> > > +	while ((n != 0) && !stop_top) {
> > >  		struct igt_drm_client *c, *prevc = NULL;
> > >  		int i, engine_w = 0, lines = 0;
> > >  
> > > @@ -410,9 +433,17 @@ int main(int argc, char **argv)
> > >  		usleep(period_us);
> > >  		if (n > 0)
> > >  			n--;
> > > +
> > > +		if (profiled_devices != NULL)
> > > +			igt_devices_update_original_profiling_state(profiled_devices);
> > 
> > Only reason in which calling this function here makes sense to me is
> > when external program which profiles some devices exits and switches
> > from 1 -> 0. Is sth I don't see here?
> 
> Yep, maybe another profiler that is checking the sysfs knob status, but could also
> be a second gputop instance runnig at the same time.

Now it makes sense, a-b from me.

--
Zbigniew

> 
> > --
> > Zbigniew
> > 
> > >  	}
> > >  
> > >  	igt_drm_clients_free(clients);
> > >  
> > > +	if (profiled_devices != NULL) {
> > > +		igt_devices_toggle_profiling(profiled_devices, false);
> > > +		igt_devices_free_profiling(profiled_devices);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > > diff --git a/tools/meson.build b/tools/meson.build
> > > index ac79d8b5840c..05138ee679cd 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_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
> > >  
> > >  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
> > >