[PATCH v6 0/2] Add gputop support for sysfs profiling knob

[PATCH v6 0/2] Add gputop support for sysfs profiling knob

[Adrián Larumbe]

Some GPUs like Panfrost need a sysfs file to be toggled before the HW can
initiate the job accounting necessary to feed fdinfo with engine and cycle
data. This sysfs knob has to be disabled when the profiler is done, to save
power.

Changelog:
v6:
 - Deleted spurious line
 - Changed license identifier comment style in header file
 - Added some ack tags to th commits
v5:
 - Deleted unnecesary blank line and added Ack tags
v4:
 - Improve error handling *igt_devices_profiled
 - Changed name of some symbols to better reflect their semantics
 - Changed header with copyright notice
 - Added documentation for public functions in igt_profiling.c
v3:
 - Created separate lib_igt_profiling to avoid dynamic linking
 of gputop with igt_lib, which also meant isolating the functions
 therein from the rest of igt_lib.
 - Make gputop check the sysfs knob state at the end of every period
 in case other instances of itself or other profilers might have
 changed it, so that the knob can be returned to its original state.
v2:
 - Added header file guards around igt_profiling.h
 - Modified licensing information to comply with SPDX format
 - Sorted included header files in alphabetic order
 - Added volatile qualifier to gputop stop variable

Adrián Larumbe (2):
  lib: Add DRM driver sysfs profiling knob toggling functions
  tools/gputop: toggle sysfs profiling knob if available for device

 lib/igt_profiling.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_profiling.h |  22 ++++++
 lib/meson.build     |   8 ++
 tools/gputop.c      |  32 +++++++-
 tools/meson.build   |   2 +-
 5 files changed, 251 insertions(+), 2 deletions(-)
 create mode 100644 lib/igt_profiling.c
 create mode 100644 lib/igt_profiling.h


base-commit: a2ab0ec12ef447c96c67dc539813462b6b39f857
-- 
2.45.1

[PATCH v6 1/2] lib: Add DRM driver sysfs profiling knob toggling functions

[Adrián Larumbe]

Some DRM drivers need to have their accounting HW toggled on demand from
user space so that fdinfo's drm-engine and drm-cycles tags can be updated
upon job completion.

A profiler such as gputop should be able to check which DRM devices have
such a sysfs knob, record its original state, toggle-enable it and revert
this operation right before exiting.

Also create a new static library dependency for this family of functions so
that it can later be linked against gputop.

Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_profiling.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_profiling.h |  22 ++++++
 lib/meson.build     |   8 ++
 3 files changed, 219 insertions(+)
 create mode 100644 lib/igt_profiling.c
 create mode 100644 lib/igt_profiling.h

diff --git a/lib/igt_profiling.c b/lib/igt_profiling.c
new file mode 100644
index 000000000000..b615067422e7
--- /dev/null
+++ b/lib/igt_profiling.c
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Collabora Ltd.
+ *
+ * Author: Adrian Larumbe <adrian.larumbe@collabora.com>
+ *
+ */
+
+#include <unistd.h>
+#include <assert.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "igt_profiling.h"
+
+#define SYSFS_DRM	"/sys/class/drm"
+#define NUM_DEVICES	10
+
+/**
+ * igt_devices_profiled
+ *
+ * Gives us an array of igt_profiled_device structures, each of which contains
+ * the full path of the DRM device's sysfs profiling knob and its original
+ * state, so that it can be restored later on.
+ *
+ * Returns: NULL-terminated array of struct igt_profiled_device pointers, or
+ * NULL on failure.
+ */
+struct igt_profiled_device *igt_devices_profiled(void)
+{
+	struct igt_profiled_device *profiled_devices;
+	unsigned int devlist_len = NUM_DEVICES;
+	unsigned int i = 0;
+	struct dirent *entry;
+	DIR *dev_dir;
+
+	/* The return array will be resized in case there are too many devices */
+	profiled_devices = malloc(devlist_len * sizeof(struct igt_profiled_device));
+	if (!profiled_devices)
+		return NULL;
+
+	dev_dir = opendir(SYSFS_DRM);
+	if (!dev_dir)
+		goto end;
+
+	while ((entry = readdir(dev_dir)) != NULL) {
+		char path[PATH_MAX];
+		char orig_state;
+		int sysfs_fd;
+
+		/* All DRM device entries are symlinks to other paths within sysfs */
+		if (entry->d_type != DT_LNK)
+			continue;
+
+		/* We're only interested in render nodes */
+		if (strstr(entry->d_name, "render") != entry->d_name)
+			continue;
+
+		snprintf(path, sizeof(path), "%s/%s/device/%s",
+			 SYSFS_DRM, entry->d_name, "profiling");
+
+		if (access(path, F_OK))
+			continue;
+
+		sysfs_fd = open(path, O_RDONLY);
+		if (sysfs_fd == -1)
+			continue;
+
+		if (read(sysfs_fd, &orig_state, 1) <= 0) {
+			close(sysfs_fd);
+			continue;
+		}
+
+		if (i == (devlist_len - 1)) {
+			struct igt_profiled_device *new_profiled_devices;
+
+			devlist_len += NUM_DEVICES;
+			new_profiled_devices = realloc(profiled_devices, devlist_len);
+			if (!new_profiled_devices)
+				goto end;
+			profiled_devices = new_profiled_devices;
+		}
+
+		profiled_devices[i].syspath = strdup(path);
+		profiled_devices[i++].original_state = orig_state;
+
+		close(sysfs_fd);
+	}
+
+	if (i == 0)
+		goto end;
+	else
+		profiled_devices[i].syspath = NULL; /* Array terminator */
+
+	return profiled_devices;
+
+end:
+	free(profiled_devices);
+	return NULL;
+}
+
+/**
+ * igt_devices_configure_profiling
+ * @devices: NULL-terminated array of igt_profiled_device structures.
+ * @enable: If True, then enable profiling, otherwise restore to original state
+ *
+ * For every single device's profiling knob sysfs path in the NULL-terminated
+ * 'devices' array, set it to '1' if bool equals true. Otherwise set it to
+ * its original state at the time it was first probed in igt_devices_profiled
+ *
+ */
+void igt_devices_configure_profiling(struct igt_profiled_device *devices, bool enable)
+{
+	assert(devices);
+
+	for (unsigned int i = 0; devices[i].syspath; i++) {
+		int sysfs_fd = open(devices[i].syspath, O_WRONLY);
+
+		if (sysfs_fd < 0)
+			continue;
+
+		write(sysfs_fd, enable ? "1" : &devices[i].original_state, 1);
+		close(sysfs_fd);
+	}
+}
+
+/**
+ * igt_devices_configure_profiling
+ * @devices: NULL-terminated array of igt_profiled_device structures.
+ *
+ * For every single struct igt_profiled_device in the 'devices' array,
+ * free its duplicated syspath string, and then free the array itself.
+ *
+ */
+void igt_devices_free_profiling(struct igt_profiled_device *devices)
+{
+	assert(devices);
+
+	for (unsigned int i = 0; devices[i].syspath; i++)
+		free(devices[i].syspath);
+
+	free(devices);
+}
+
+/**
+ * igt_devices_configure_profiling
+ * @devices: NULL-terminated array of igt_profiled_device structures.
+ *
+ * For every single struct igt_profiled_device in the 'devices' array,
+ * check whether the sysfs profiling knob has changed its state since
+ * the last time its original state was registered, and then update it
+ * accordingly. This is usually a symptom that there are other profilers
+ * currently trying to toggle the sysfs knob, or perhaps more than one
+ * instance of the same profiler.
+ * The goal of this function is ensuring the sysfs knob is eventually
+ * restored to a coherent state, even though a small race window is
+ * possible. There's nothing we can do about this, so this function
+ * tries to mitigate that situation in a best-effort fashion.
+ *
+ */
+void igt_devices_update_original_profiling_state(struct igt_profiled_device *devices)
+{
+	assert(devices);
+
+	for (unsigned int i = 0; devices[i].syspath; i++) {
+		char new_state;
+		int sysfs_fd;
+
+		sysfs_fd = open(devices[i].syspath, O_RDWR);
+		if (sysfs_fd == -1)
+			continue;
+
+		if (!read(sysfs_fd, &new_state, 1)) {
+			close(sysfs_fd);
+			continue;
+		}
+
+		if (new_state == '0') {
+			write(sysfs_fd, "1", 1);
+			devices[i].original_state = new_state;
+		}
+
+		close(sysfs_fd);
+	}
+}
diff --git a/lib/igt_profiling.h b/lib/igt_profiling.h
new file mode 100644
index 000000000000..b063d54e12cb
--- /dev/null
+++ b/lib/igt_profiling.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Collabora Ltd.
+ *
+ * Author: Adrian Larumbe <adrian.larumbe@collabora.com>
+ *
+ */
+
+#ifndef IGT_PROFILING_H
+#define IGT_PROFILING_H
+
+struct igt_profiled_device {
+	char *syspath;
+	char original_state;
+};
+
+void igt_devices_configure_profiling(struct igt_profiled_device *devices, bool enable);
+struct igt_profiled_device *igt_devices_profiled(void);
+void igt_devices_free_profiling(struct igt_profiled_device *devices);
+void igt_devices_update_original_profiling_state(struct igt_profiled_device *devices);
+
+#endif /* IGT_PROFILING_H */
diff --git a/lib/meson.build b/lib/meson.build
index 7577bee9d35e..f711e60a736a 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -309,6 +309,14 @@ lib_igt_drm_fdinfo_build = static_library('igt_drm_fdinfo',
 
 lib_igt_drm_fdinfo = declare_dependency(link_with : lib_igt_drm_fdinfo_build,
 				  include_directories : inc)
+
+lib_igt_profiling_build = static_library('igt_profiling',
+	['igt_profiling.c'],
+	include_directories : inc)
+
+lib_igt_profiling = declare_dependency(link_with : lib_igt_profiling_build,
+				        include_directories : inc)
+
 i915_perf_files = [
   'igt_list.c',
   'i915/perf.c',
-- 
2.45.1

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

[Adrián Larumbe]

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@ursulin.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tools/gputop.c    | 32 +++++++++++++++++++++++++++++++-
 tools/meson.build |  2 +-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tools/gputop.c b/tools/gputop.c
index 6c7cf6927464..8842eebbd8e5 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"
 
 enum utilization_type {
@@ -391,10 +392,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;
@@ -413,9 +423,21 @@ int main(int argc, char **argv)
 	if (!clients)
 		exit(1);
 
+	profiled_devices = igt_devices_profiled();
+	if (profiled_devices != NULL) {
+		igt_devices_configure_profiling(profiled_devices, true);
+
+		if (signal(SIGINT, sigint_handler) == SIG_ERR) {
+			fprintf(stderr, "Failed to install signal handler!\n");
+			igt_devices_configure_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;
 
@@ -443,9 +465,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);
 	}
 
 	igt_drm_clients_free(clients);
 
+	if (profiled_devices != NULL) {
+		igt_devices_configure_profiling(profiled_devices, false);
+		igt_devices_free_profiling(profiled_devices);
+	}
+
 	return 0;
 }
diff --git a/tools/meson.build b/tools/meson.build
index c02c020d0b31..df26c4b95e3f 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -69,7 +69,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.45.1

✗ Fi.CI.BAT: failure for Add gputop support for sysfs profiling knob

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

== Series Details ==

Series: Add gputop support for sysfs profiling knob
URL   : https://patchwork.freedesktop.org/series/136242/
State : failure

== Summary ==

Series 136242 revision 1 was fully merged or fully failed: no git log



[-- Attachment #2: Type: text/html, Size: 704 bytes --]

Re: [PATCH v6 0/2] Add gputop support for sysfs profiling knob

[Kamil Konieczny]

Hi Adrián,
On 2024-07-18 at 14:58:08 +0100, Adrián Larumbe wrote:
> Some GPUs like Panfrost need a sysfs file to be toggled before the HW can
> initiate the job accounting necessary to feed fdinfo with engine and cycle
> data. This sysfs knob has to be disabled when the profiler is done, to save
> power.
> 
> Changelog:
> v6:
>  - Deleted spurious line
>  - Changed license identifier comment style in header file
>  - Added some ack tags to th commits

I already merged your patches with fixes and acks,
sorry for not letting you know.

Regards,
Kamil

> v5:
>  - Deleted unnecesary blank line and added Ack tags
> v4:
>  - Improve error handling *igt_devices_profiled
>  - Changed name of some symbols to better reflect their semantics
>  - Changed header with copyright notice
>  - Added documentation for public functions in igt_profiling.c
> v3:
>  - Created separate lib_igt_profiling to avoid dynamic linking
>  of gputop with igt_lib, which also meant isolating the functions
>  therein from the rest of igt_lib.
>  - Make gputop check the sysfs knob state at the end of every period
>  in case other instances of itself or other profilers might have
>  changed it, so that the knob can be returned to its original state.
> v2:
>  - Added header file guards around igt_profiling.h
>  - Modified licensing information to comply with SPDX format
>  - Sorted included header files in alphabetic order
>  - Added volatile qualifier to gputop stop variable
> 
> Adrián Larumbe (2):
>   lib: Add DRM driver sysfs profiling knob toggling functions
>   tools/gputop: toggle sysfs profiling knob if available for device
> 
>  lib/igt_profiling.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_profiling.h |  22 ++++++
>  lib/meson.build     |   8 ++
>  tools/gputop.c      |  32 +++++++-
>  tools/meson.build   |   2 +-
>  5 files changed, 251 insertions(+), 2 deletions(-)
>  create mode 100644 lib/igt_profiling.c
>  create mode 100644 lib/igt_profiling.h
> 
> 
> base-commit: a2ab0ec12ef447c96c67dc539813462b6b39f857
> -- 
> 2.45.1
> 

Re: [PATCH v6 0/2] Add gputop support for sysfs profiling knob

[Adrián Larumbe]

On 18.07.2024 19:29, Kamil Konieczny wrote:
> Hi Adrián,
> On 2024-07-18 at 14:58:08 +0100, Adrián Larumbe wrote:
> > Some GPUs like Panfrost need a sysfs file to be toggled before the HW can
> > initiate the job accounting necessary to feed fdinfo with engine and cycle
> > data. This sysfs knob has to be disabled when the profiler is done, to save
> > power.
> > 
> > Changelog:
> > v6:
> >  - Deleted spurious line
> >  - Changed license identifier comment style in header file
> >  - Added some ack tags to th commits
> 
> I already merged your patches with fixes and acks,
> sorry for not letting you know.

No worries, many thanks for the reviews !

> Regards,
> Kamil
> 
> > v5:
> >  - Deleted unnecesary blank line and added Ack tags
> > v4:
> >  - Improve error handling *igt_devices_profiled
> >  - Changed name of some symbols to better reflect their semantics
> >  - Changed header with copyright notice
> >  - Added documentation for public functions in igt_profiling.c
> > v3:
> >  - Created separate lib_igt_profiling to avoid dynamic linking
> >  of gputop with igt_lib, which also meant isolating the functions
> >  therein from the rest of igt_lib.
> >  - Make gputop check the sysfs knob state at the end of every period
> >  in case other instances of itself or other profilers might have
> >  changed it, so that the knob can be returned to its original state.
> > v2:
> >  - Added header file guards around igt_profiling.h
> >  - Modified licensing information to comply with SPDX format
> >  - Sorted included header files in alphabetic order
> >  - Added volatile qualifier to gputop stop variable
> > 
> > Adrián Larumbe (2):
> >   lib: Add DRM driver sysfs profiling knob toggling functions
> >   tools/gputop: toggle sysfs profiling knob if available for device
> > 
> >  lib/igt_profiling.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_profiling.h |  22 ++++++
> >  lib/meson.build     |   8 ++
> >  tools/gputop.c      |  32 +++++++-
> >  tools/meson.build   |   2 +-
> >  5 files changed, 251 insertions(+), 2 deletions(-)
> >  create mode 100644 lib/igt_profiling.c
> >  create mode 100644 lib/igt_profiling.h
> > 
> > 
> > base-commit: a2ab0ec12ef447c96c67dc539813462b6b39f857
> > -- 
> > 2.45.1
> > 

✗ Fi.CI.BAT: failure for Add gputop support for sysfs profiling knob (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

== Series Details ==

Series: Add gputop support for sysfs profiling knob (rev2)
URL   : https://patchwork.freedesktop.org/series/136242/
State : failure

== Summary ==

Series 136242 revision 2 was fully merged or fully failed: no git log



[-- Attachment #2: Type: text/html, Size: 711 bytes --]