[igt-dev] [PATCH i-g-t v2 0/6] Improve logic to work with audio dependency on DRM driver

[igt-dev] [PATCH i-g-t v2 0/6] Improve logic to work with audio dependency on DRM driver

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

There are several IGT tests that require a DRM driver to unbind or unload.

However, depending on the hardware model, the audio driver (snd_hda_intel)
hooks into the DRM driver in order to work, and the audio PCI drivers and
ALSA core currently doesn't support unbind drivers that provide them access
to the hardware, without first unbinding the audio driver.

The real fix would be to improve the audio driver's bind/unbind logic, but
this will very likely require changes at the subsystem as well. There were
some discussions in the past at the ALSA subsystem:

	https://mailman.alsa-project.org/pipermail/alsa-devel/2021-September/190279.html

But this was not addressed yet.

It should be noticed that there are some daemons like alsactl, pulseaudio and
pipewire-pulse that constantly accesses the audio hardware. So, the only way to
unbind the audio driver is to first make such apps to stop using the audio
devices.

On other words, if, for instance, snd-hda-intel is bound into i915 driver,
those are the needed steps to unbind i915 driver:

- request pulseaudio or pipewire-pulse to stop using the audio devices;
- kill all other apps that are using /dev/snd devnodes;
- unbind/unload snd-hda-intel;
- unbind or unload i915 driver.

The patches on this series warrant that the above steps will be observed,
and unifies the logic which currently has two implementations: one at IGT
library and another one at core_hotunplug.

In the specific case of snd-hda and i915 driver, the actual binding between
them depends on the actual hardware architecture. In the past, only a few
CPU/GPU models would do such binding. As documented at sound/pci/hda/hda_intel,
it used to be:

	- Haswell;
	- Broadwell;
	- Baytrail;
	- Braswell.

But nowadays, on all devices with discrete graphics GPU, the snd_hda
driver needs to hook into the i915 driver, mostly for power management.
Also, newer integrated graphics also need to bind into i915, in order
to talk with the hardware registers.

As the binding between snd_hda and i915 depends on hardware model and
changes over time, the safest way is to know for sure when such binding
exists would be if the userspace tools would report, e. g.:

	$ lsmod|grep i915.*snd
	i915                 8167424  3 snd_hda_intel

	$ cat /proc/modules |grep i915.*snd
	i915 8167424 3 snd_hda_intel, Live 0x0000000000000000

With such knowledge, userspace can better decide if how to unbind the
DRM driver. The Kernel patches adding support for such feature can be
seen at:

	https://lore.kernel.org/intel-gfx/cover.1651348913.git.mchehab@kernel.org/

This series assumes that the above patch series (or a similar one) will
be there on Kernel 5.20 (this could be changed in the future if the patch
series take a longer time to be merged).

So, if Kernel is 5.20 and /proc/modules doesn't show any dependencies
between a snd driver and the DRM driver, IGT will simply unbind the DRM
driver directly, completely ignoring the audio drivers. If the dependency
exists, the logic shoud work for any audio and DRM driver - and even allow
recursive dependencies. So, it should work even for SOC/SOF audio drivers.

If Kernel is older, it will try to unload the audio first, but then the
logic is dependent on the drivers names. This series preserve the existing
dependency chain. e. g. it expects either snd_hda_intel or snd_hdmi_lpe_audio
driver, and assumes that those are likely bind to i915 driver.
Howver, as IGT doesn't know for sure if the such binding exists, it won't
fail the IGT test if the audio unload fails, as there's a chance that
the i915 unbind would work even when this fails.

---

v2:
  - rebased on the top of today's origin/master, to make CI happy.
    no functional changes.

Mauro Carvalho Chehab (6):
  tests/core_hotunplug: properly finish processes using audio devices
  lib/igt_kmod: always fill who when unloading audio driver
  lib/igt_kmod: improve audio unbind logic
  core_hotunplug: fix audio unbind logic
  lib/igt_kmod: make it less pedantic with audio driver removal
  lib/igt_kmod: properly handle pipewire-pulse

 lib/igt_aux.c          | 248 ++++++++++++++++++++++++++++++++++++++
 lib/igt_aux.h          |   3 +
 lib/igt_core.c         |   7 ++
 lib/igt_core.h         |   1 +
 lib/igt_kmod.c         | 264 +++++++++++++++++++++++++++++++++++++++--
 lib/igt_kmod.h         |   2 +
 tests/core_hotunplug.c |  55 +++------
 7 files changed, 531 insertions(+), 49 deletions(-)

-- 
2.35.1

[igt-dev] [PATCH i-g-t v2 1/6] tests/core_hotunplug: properly finish processes using audio devices

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

Before unloading or unbinding an audio driver, all processes
that are using it must be terminated. The current logic seeks
only for alsactl, but ignore other processes, including
pulseaudio.

Make the logic more general, extending it to any processes that
could have an open device under /dev/snd.

It should be noticed that some distros like Fedora and openSUSE
are now migrating from pulseaudio into pipewire-pulse. Right
now, there's no standard distribution-agnostic way to request
pipewire-pulse to stop using audio devices, but there's a new
patch upstream that will make things easier:

	https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/6ad6300ec657c88322a8cd6f3548261d3dc05359

Which should be available for pipewire-pulse versions 0.3.50 and
upper.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 lib/igt_aux.c          | 142 +++++++++++++++++++++++++++++++++++++++++
 lib/igt_aux.h          |   1 +
 lib/igt_kmod.c         |  38 +++++++----
 lib/igt_kmod.h         |   2 +
 tests/core_hotunplug.c |  11 +---
 5 files changed, 173 insertions(+), 21 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 03cc38c93e5c..4f1d88ed68dd 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -31,6 +31,7 @@
 #endif
 #include <stdio.h>
 #include <fcntl.h>
+#include <pwd.h>
 #include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <string.h>
@@ -1410,6 +1411,147 @@ igt_lsof(const char *dpath)
 	free(sanitized);
 }
 
+static void pulseaudio_unload_module(proc_t *proc_info)
+{
+	char xdg_dir[PATH_MAX];
+	const char *homedir;
+	struct passwd *pw;
+
+	igt_fork(child, 1) {
+		pw = getpwuid(proc_info->euid);
+		homedir = pw->pw_dir;
+		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
+
+		igt_info("Ask pulseaudio to stop using audio device\n");
+
+		setgid(proc_info->egid);
+		setuid(proc_info->euid);
+		clearenv();
+		setenv("HOME", homedir, 1);
+		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
+
+		exit(system("for i in $(pacmd list-sources|grep module:|cut -d : -f 2); do pactl unload-module $i; done"));
+	}
+}
+
+static int
+__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
+{
+	const char *audio_dev = "/dev/snd/";
+	char path[PATH_MAX * 2];
+	struct dirent *d;
+	struct stat st;
+	char *fd_lnk;
+	int fail = 0;
+	ssize_t read;
+
+	DIR *dp = opendir(proc_path);
+	igt_assert(dp);
+
+	while ((d = readdir(dp))) {
+		if (*d->d_name == '.')
+			continue;
+
+		memset(path, 0, sizeof(path));
+		snprintf(path, sizeof(path), "%s/%s", proc_path, d->d_name);
+
+		if (lstat(path, &st) == -1)
+			continue;
+
+		fd_lnk = malloc(st.st_size + 1);
+
+		igt_assert((read = readlink(path, fd_lnk, st.st_size + 1)));
+		fd_lnk[read] = '\0';
+
+		if (strncmp(audio_dev, fd_lnk, strlen(audio_dev))) {
+			free(fd_lnk);
+			continue;
+		}
+
+		free(fd_lnk);
+
+		/*
+		 * In order to avoid racing against pa/systemd, ensure that
+		 * pulseaudio will close all audio files. This should be
+		 * enough to unbind audio modules and won't case race issues
+		 * with systemd trying to reload it.
+		 */
+		if (!strcmp(proc_info->cmd, "pulseaudio")) {
+			pulseaudio_unload_module(proc_info);
+			break;
+		}
+
+		/*
+		 * FIXME: terminating pipewire-pulse is not that easy, as
+		 * pipewire there's no standard way up to pipewire version
+		 * 0.3.49. Just trying to kill pipewire will start a race
+		 * between IGT and systemd. If IGT wins, the audio driver
+		 * will be unloaded before systemd tries to reload it, but
+		 * if systemd wins, the audio device will be re-opened and
+		 * used before IGT has a chance to remove the audio driver.
+		 * Pipewire version 0.3.50 should bring a standard way:
+		 *
+		 * 1) start a thread running:
+		 *	 pw-reserve -n Audio0 -r
+		 * 2) unload/unbind the the audio driver(s);
+		 * 3) stop the pw-reserve thread.
+		 *
+		 * We should add support for it once distros start shipping it.
+		 */
+
+		/* For all other processes, just kill them */
+		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
+				proc_info->tid, proc_info->cmd);
+
+		if (kill(proc_info->tid, SIGTERM) < 0) {
+			igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n",
+				proc_info->cmd, proc_info->tid);
+			if (kill(proc_info->tid, SIGABRT) < 0) {
+				fail++;
+				igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n",
+					proc_info->cmd, proc_info->tid);
+			}
+		}
+
+		break;
+	}
+
+	closedir(dp);
+	return fail;
+}
+
+/*
+ * This function identifies each process running on the machine that is
+ * opening an audio device and tries to stop it.
+ *
+ * Special care should be taken with pipewire and pipewire-pulse, as those
+ * daemons are respanned if they got killed.
+ */
+int
+igt_lsof_kill_audio_processes(void)
+{
+	char path[PATH_MAX];
+	proc_t *proc_info;
+	PROCTAB *proc;
+	int fail = 0;
+
+	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
+	igt_assert(proc != NULL);
+
+	while ((proc_info = readproc(proc, NULL))) {
+		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1) {
+			fail++;
+		} else {
+			fail += __igt_lsof_kill_proc(proc_info, path);
+		}
+
+		freeproc(proc_info);
+	}
+	closeproc(proc);
+
+	return fail;
+}
+
 static struct igt_siglatency {
 	timer_t timer;
 	struct timespec target;
diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index 9f2588aeca90..bb96d1afb777 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -306,6 +306,7 @@ bool igt_allow_unlimited_files(void);
 int igt_is_process_running(const char *comm);
 int igt_terminate_process(int sig, const char *comm);
 void igt_lsof(const char *dpath);
+int igt_lsof_kill_audio_processes(void);
 
 #define igt_hweight(x) \
 	__builtin_choose_expr(sizeof(x) == 8, \
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index f252535d5a3a..87a59245f699 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -389,7 +389,7 @@ igt_i915_driver_load(const char *opts)
 	return 0;
 }
 
-int __igt_i915_driver_unload(const char **who)
+int igt_audio_driver_unload(const char **who)
 {
 	int ret;
 	const char *sound[] = {
@@ -398,6 +398,27 @@ int __igt_i915_driver_unload(const char **who)
 		NULL,
 	};
 
+	for (const char **m = sound; *m; m++) {
+		if (igt_kmod_is_loaded(*m)) {
+			if (igt_lsof_kill_audio_processes())
+				return 1;
+
+			kick_snd_hda_intel();
+			ret = igt_kmod_unload(*m, 0);
+			if (ret) {
+				if (who)
+					*who = *m;
+				return ret;
+			}
+		}
+	}
+	return 0;
+}
+
+int __igt_i915_driver_unload(const char **who)
+{
+	int ret;
+
 	const char *aux[] = {
 		/* gen5: ips uses symbol_get() so only a soft module dependency */
 		"intel_ips",
@@ -411,18 +432,9 @@ int __igt_i915_driver_unload(const char **who)
 	/* unbind vt */
 	bind_fbcon(false);
 
-	for (const char **m = sound; *m; m++) {
-		if (igt_kmod_is_loaded(*m)) {
-			igt_terminate_process(SIGTERM, "alsactl");
-			kick_snd_hda_intel();
-			ret = igt_kmod_unload(*m, 0);
-			if (ret) {
-				if (who)
-					*who = *m;
-				return ret;
-			}
-		}
-	}
+	ret = igt_audio_driver_unload(who);
+	if (ret)
+		return ret;
 
 	for (const char **m = aux; *m; m++) {
 		if (igt_kmod_is_loaded(*m)) {
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index 0898122b3efe..15f0be31e8e4 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -36,6 +36,8 @@ bool igt_kmod_has_param(const char *mod_name, const char *param);
 int igt_kmod_load(const char *mod_name, const char *opts);
 int igt_kmod_unload(const char *mod_name, unsigned int flags);
 
+int igt_audio_driver_unload(const char **whom);
+
 int igt_i915_driver_load(const char *opts);
 int igt_i915_driver_unload(void);
 int __igt_i915_driver_unload(const char **whom);
diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index 02eae19e1e16..18e37ec8feba 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -152,19 +152,14 @@ static void driver_unbind(struct hotunplug *priv, const char *prefix,
 	 * safest and easiest way out.
 	 */
 	if (priv->snd_unload) {
-		igt_terminate_process(SIGTERM, "alsactl");
-
-		/* unbind snd_hda_intel */
-		kick_snd_hda_intel();
-
-		if (igt_kmod_unload("snd_hda_intel", 0)) {
+		if (igt_audio_driver_unload(NULL)) {
 			priv->snd_unload = false;
-			igt_warn("Could not unload snd_hda_intel\n");
+			igt_warn("Could not unload audio driver\n");
 			igt_kmod_list_loaded();
 			igt_lsof("/dev/snd");
 			igt_skip("Audio is in use, skipping\n");
 		} else {
-			igt_info("Preventively unloaded snd_hda_intel\n");
+			igt_info("Preventively unloaded audio driver\n");
 		}
 	}
 
-- 
2.35.1

[igt-dev] [PATCH i-g-t v2 2/6] lib/igt_kmod: always fill who when unloading audio driver

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

As we'll use this information at core_hotunplug to announce
when an audio module is unloaded, fill it even if return code
is zero.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 lib/igt_aux.c  | 2 +-
 lib/igt_kmod.c | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 4f1d88ed68dd..15fe87839567 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -1422,7 +1422,7 @@ static void pulseaudio_unload_module(proc_t *proc_info)
 		homedir = pw->pw_dir;
 		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
 
-		igt_info("Ask pulseaudio to stop using audio device\n");
+		igt_info("Request pulseaudio to stop using audio device\n");
 
 		setgid(proc_info->egid);
 		setuid(proc_info->euid);
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 87a59245f699..716e03f426c9 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -400,16 +400,15 @@ int igt_audio_driver_unload(const char **who)
 
 	for (const char **m = sound; *m; m++) {
 		if (igt_kmod_is_loaded(*m)) {
+			if (who)
+				*who = *m;
 			if (igt_lsof_kill_audio_processes())
 				return 1;
 
 			kick_snd_hda_intel();
 			ret = igt_kmod_unload(*m, 0);
-			if (ret) {
-				if (who)
-					*who = *m;
+			if (ret)
 				return ret;
-			}
 		}
 	}
 	return 0;
-- 
2.35.1

[igt-dev] [PATCH i-g-t v2 3/6] lib/igt_kmod: improve audio unbind logic

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

The dependencies between audio and DRM drivers are not trivial.

On several CPUs, the logic inside snd-hda-intel, for instance, tries to
hook into i915 driver, via drm_audio_component logic. That also happens
when there's no runtime PM.

When the audio driver is bound into i915, removing or unbinding i915
without first removing the audio driver produce Kernel errors.

So, the audio driver(s) should be removed first, and this can only
happen after pulseaudio, pipewire-pulse, audioctl and any other
userspace program stops using it.

This is more prune to failures. So, the best is to only try to stop
the audio driver when it is known to have dependencies on the video
driver.

Before an upcoming Kernel patch, there's no way to detect if the
audio driver required a DRM one. So, the safest way is to always
remove the audio drivers that are known to cause issues.

After the new Kernel, the logic can be more selective, only removing
the audio driver if /proc/modules shows dependencies with the DRM
driver.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 lib/igt_kmod.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 191 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 716e03f426c9..ab98b1cdefa5 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -24,6 +24,7 @@
 #include <ctype.h>
 #include <signal.h>
 #include <errno.h>
+#include <sys/utsname.h>
 
 #include "igt_aux.h"
 #include "igt_core.h"
@@ -389,7 +390,7 @@ igt_i915_driver_load(const char *opts)
 	return 0;
 }
 
-int igt_audio_driver_unload(const char **who)
+static int igt_always_unload_audio_driver(const char **who)
 {
 	int ret;
 	const char *sound[] = {
@@ -414,6 +415,195 @@ int igt_audio_driver_unload(const char **who)
 	return 0;
 }
 
+struct module_ref {
+	char *name;
+	unsigned long mem;
+	unsigned int ref_count;
+	unsigned int num_required;
+	unsigned int *required_by;
+};
+
+static struct module_ref *read_module_dependencies(unsigned int *num_mod)
+{
+	FILE *fp = fopen("/proc/modules", "r");
+	struct module_ref *mod = NULL;
+	size_t line_buf_size = 0;
+	char *required_by, *p;
+	unsigned n_mod = 0;
+	unsigned num_req;
+	char *line = NULL;
+	int len = 0;
+	int i, ret;
+
+	igt_assert(fp);
+
+	while ((len = getline(&line, &line_buf_size, fp)) > 0) {
+		mod = realloc(mod, (n_mod + 1) * sizeof(*mod));
+		igt_assert(mod);
+		mod[n_mod].required_by = NULL;
+
+		p = strtok(line, " ");
+		mod[n_mod].name = strdup(p);
+
+		p = strtok(NULL, " ");
+		ret = sscanf(p, "%lu", &mod[n_mod].mem);
+		igt_assert(ret == 1);
+
+		p = strtok(NULL, " ");
+		ret = sscanf(p, "%u", &mod[n_mod].ref_count);
+		igt_assert(ret == 1);
+
+		num_req = 0;
+		required_by = strtok(NULL, " ");
+		if (strcmp(required_by, "-")) {
+			p = strtok(required_by, ",");
+			while (p) {
+				for (i = 0; i < n_mod; i++) {
+					if (!strcmp(p, mod[i].name))
+						break;
+				}
+				igt_assert(i < n_mod);
+
+				mod[n_mod].required_by = realloc(mod[n_mod].required_by,
+								 (num_req + 1) * sizeof(unsigned int));
+				mod[n_mod].required_by[num_req] = i;
+				num_req++;
+				p = strtok(NULL, ",");
+			}
+		}
+		mod[n_mod].num_required = num_req;
+		n_mod++;
+	}
+	free(line);
+	fclose(fp);
+
+	*num_mod = n_mod;
+	return mod;
+}
+
+static void free_module_ref(struct module_ref *mod, unsigned int num_mod)
+{
+	int i;
+
+	for (i = 0; i < num_mod; i++) {
+		free(mod[i].name);
+		free(mod[i].required_by);
+	}
+	free(mod);
+}
+
+static int igt_unload_driver(struct module_ref *mod, unsigned int num_mod,
+			     unsigned int pos)
+{
+	int ret, i;
+
+	/* Recursively remove depending modules, if any */
+	for (i = 0; i < mod[pos].num_required; i++) {
+		ret = igt_unload_driver(mod, num_mod,
+					mod[num_mod].required_by[i]);
+		if (ret)
+			return ret;
+	}
+
+	return igt_kmod_unload(mod[pos].name, 0);
+}
+
+#define LINUX_VERSION(major, minor, patch)			\
+		     ((major) << 16 | (minor) << 8 | (patch))
+
+
+static int linux_kernel_version(void)
+{
+	struct utsname utsname;
+	int ver[3] = { 0 };
+	int i = 0;
+	int n = 0;
+	char *p;
+
+	if (uname(&utsname))
+		return 0;
+
+	p = utsname.release;
+
+	while (*p && i < 3) {
+		if (isdigit(*p)) {
+			n = n * 10 + (*p - '0');
+			p++;
+			continue;
+		}
+		if (n > 255)
+			n = 255;
+		ver[i] = n;
+		i++;
+		n = 0;
+
+		if (*p != '.')
+			break;
+		p++;
+	}
+
+	return LINUX_VERSION(ver[0], ver[1], ver[2]);
+}
+
+int igt_audio_driver_unload(const char **who)
+{
+	unsigned int num_mod, i, j;
+	struct module_ref *mod;
+	int pos = -1;
+	int ret = 0;
+
+	/*
+	 * On older Kernels, there's no way to check if the audio driver
+	 * binds into the DRM one. So, always remove audio drivers that
+	 * might be binding.
+	 */
+	if (linux_kernel_version() < LINUX_VERSION(5, 20, 0))
+		return igt_always_unload_audio_driver(who);
+
+	/*
+	 * Newer Kernels gained support for showing the dependencies between
+	 * audio and DRM drivers via /proc/modules and lsmod. Use it to
+	 * detect if removing the audio driver is needed, properly unloading
+	 * any audio processes that could be using the audio driver and
+	 * handling module dependencies. Such solution should be generic
+	 * enough to work with newer SOC/SOF drivers if ever needed.
+	 */
+
+	mod = read_module_dependencies(&num_mod);
+
+	for (i = 0; i < num_mod; i++) {
+		if (!strcmp(mod[i].name, "i915")) {
+			break;
+		}
+	}
+
+	if (i == num_mod)
+		return 0;
+
+	/* Recursively remove all drivers that depend on i915 */
+	for (j = 0; j < mod[i].num_required; j++) {
+		pos = mod[i].required_by[j];
+		if (who)
+			*who = strdup(mod[pos].name);
+		/*
+		 * If a sound driver depends on i915, kill audio processes
+		 * first, in order to make it possible to unload the driver
+		 */
+		if (strstr(mod[pos].name, "snd")) {
+			if (igt_lsof_kill_audio_processes()) {
+				ret = 1;
+				goto ret;
+			}
+		}
+		ret = igt_unload_driver(mod, num_mod, pos);
+	}
+
+ret:
+	free_module_ref(mod, num_mod);
+
+	return ret;
+}
+
 int __igt_i915_driver_unload(const char **who)
 {
 	int ret;
-- 
2.35.1

[igt-dev] [PATCH i-g-t v2 4/6] core_hotunplug: fix audio unbind logic

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

The current audio unbind logic is wrong: it expects the audio
driver to depend on i915 only on Haswell, Broadwell and DG1.

That doesn't match the Kernel driver, where snd-hda-audio binds
on i915 also on Skylake, Braswell and whenever it needs to use
pm runtime from the DRM driver.

Now that lib/igt-kmod has gained improved support for audio
unbind, update core_hotunplug to benefit from the newer logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 tests/core_hotunplug.c | 53 ++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index 18e37ec8feba..7c68b336e67b 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -54,7 +54,7 @@ struct hotunplug {
 	const char *failure;
 	bool need_healthcheck;
 	bool has_intel_perf;
-	bool snd_unload;
+ 	const char *snd_driver;
 };
 
 /* Helpers */
@@ -140,27 +140,22 @@ static void prepare(struct hotunplug *priv)
 static void driver_unbind(struct hotunplug *priv, const char *prefix,
 			  int timeout)
 {
-	/**
-	 * FIXME: Unbinding the i915 driver on affected platforms with
-	 * audio results in a kernel WARN on "i915 raw-wakerefs=1
-	 * wakelocks=1 on cleanup". The below CI friendly user level
-	 * workaround to unload and de-couple audio from IGT testing,
-	 * prevents the warning from appearing. Drop this hack as soon
-	 * as this is fixed in the kernel. unbind/re-bind validation
-	 * on audio side is not robust and we could have potential
-	 * failures blocking display CI, currently this seems to the
-	 * safest and easiest way out.
+	/*
+	 * FIXME: on some devices, the audio driver (snd_hda_intel)
+	 * binds into the i915 driver. On such hardware, kernel warnings
+	 * and errors may happen if i915 is unbind/removed before removing
+	 * first the audio driver.
+	 * So, add a logic that unloads the audio driver before trying to
+	 * unbind i915 driver, reloading it when binding again.
 	 */
-	if (priv->snd_unload) {
-		if (igt_audio_driver_unload(NULL)) {
-			priv->snd_unload = false;
-			igt_warn("Could not unload audio driver\n");
-			igt_kmod_list_loaded();
-			igt_lsof("/dev/snd");
-			igt_skip("Audio is in use, skipping\n");
-		} else {
-			igt_info("Preventively unloaded audio driver\n");
-		}
+	if (igt_audio_driver_unload(&priv->snd_driver)) {
+		if (priv->snd_driver)
+			igt_warn("Could not unload audio driver %s\n", priv->snd_driver);
+		igt_kmod_list_loaded();
+		igt_lsof("/dev/snd");
+		igt_skip("Audio is in use, skipping\n");
+	} else if (priv->snd_driver) {
+		igt_info("Unloaded audio driver %s\n", priv->snd_driver);
 	}
 
 	local_debug("%sunbinding the driver from the device\n", prefix);
@@ -192,8 +187,11 @@ static void driver_bind(struct hotunplug *priv, int timeout)
 				F_OK, 0),
 		      "Rebound device not present (%s)!\n", priv->dev_bus_addr);
 
-	if (priv->snd_unload)
-		igt_kmod_load("snd_hda_intel", NULL);
+	if (priv->snd_driver) {
+		igt_info("Realoading %s\n", priv->snd_driver);
+		igt_kmod_load(priv->snd_driver, NULL);
+		priv->snd_driver = NULL;
+	}
 }
 
 /* Remove (virtually unplug) the device from its bus */
@@ -606,7 +604,7 @@ igt_main
 		.failure	= NULL,
 		.need_healthcheck = true,
 		.has_intel_perf = false,
-		.snd_unload	= false,
+		.snd_driver	= NULL,
 	};
 
 	igt_fixture {
@@ -616,13 +614,6 @@ igt_main
 		igt_skip_on_f(fd_drm < 0, "No known DRM device found\n");
 
 		if (is_i915_device(fd_drm)) {
-			/*
-			 * Audio driver does not support hot unplug via DRM
-			 * audio component framework, so driver must be unloaded
-			 * explicitly for i915 unbind tests.
-			 */
-			priv.snd_unload = true;
-
 			gem_quiescent_gpu(fd_drm);
 			igt_require_gem(fd_drm);
 
-- 
2.35.1

[igt-dev] [PATCH i-g-t v2 5/6] lib/igt_kmod: make it less pedantic with audio driver removal

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

Current Linux Kernel don't report if the audio driver binds
into the DRM driver. As this is CPU specific, allow audio
driver unload fail without skipping the IGT tests on legacy
Kernels, as this may not be mandatory.

On new kernels where lsmod will properly display de dependency
between the audio and DRM drivers, skip the core hotunplug
test if it fails to unload the audio driver, as this is is
unrelated to the DRM driver - and it could simply because there
are some userspace code using the audio device while the IGT
test is running.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 lib/igt_kmod.c         | 40 ++++++++++++++++++++++++++++++++--------
 tests/core_hotunplug.c |  7 ++-----
 2 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index ab98b1cdefa5..d1d0662f3706 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -399,17 +399,32 @@ static int igt_always_unload_audio_driver(const char **who)
 		NULL,
 	};
 
+	/*
+	 * With old Kernels, the dependencies between audio and DRM drivers
+	 * are not shown. So, it may not be mandatory to remove the audio
+	 * driver before unload/unbind the DRM one. So, let's print warnings,
+	 * but return 0 on errors, as, if the dependency is mandatory, this
+	 * will be detected later when trying to unbind/unload the DRM driver.
+	 */
 	for (const char **m = sound; *m; m++) {
 		if (igt_kmod_is_loaded(*m)) {
 			if (who)
 				*who = *m;
-			if (igt_lsof_kill_audio_processes())
-				return 1;
+			if (igt_lsof_kill_audio_processes()) {
+				igt_warn("Could not stop audio process(es)\n");
+				igt_kmod_list_loaded();
+				igt_lsof("/dev/snd");
+				return 0;
+			}
 
 			kick_snd_hda_intel();
 			ret = igt_kmod_unload(*m, 0);
-			if (ret)
-				return ret;
+			if (ret) {
+				igt_warn("Could not unload audio driver %s\n", *m);
+				igt_kmod_list_loaded();
+				igt_lsof("/dev/snd");
+				return 0;
+			}
 		}
 	}
 	return 0;
@@ -547,6 +562,7 @@ static int linux_kernel_version(void)
 
 int igt_audio_driver_unload(const char **who)
 {
+	const char *drm_driver = "i915";
 	unsigned int num_mod, i, j;
 	struct module_ref *mod;
 	int pos = -1;
@@ -572,7 +588,7 @@ int igt_audio_driver_unload(const char **who)
 	mod = read_module_dependencies(&num_mod);
 
 	for (i = 0; i < num_mod; i++) {
-		if (!strcmp(mod[i].name, "i915")) {
+		if (!strcmp(mod[i].name, drm_driver)) {
 			break;
 		}
 	}
@@ -580,14 +596,15 @@ int igt_audio_driver_unload(const char **who)
 	if (i == num_mod)
 		return 0;
 
-	/* Recursively remove all drivers that depend on i915 */
+	/* Recursively remove all drivers that depend on the drm driver */
 	for (j = 0; j < mod[i].num_required; j++) {
 		pos = mod[i].required_by[j];
 		if (who)
 			*who = strdup(mod[pos].name);
 		/*
-		 * If a sound driver depends on i915, kill audio processes
-		 * first, in order to make it possible to unload the driver
+		 * If a sound driver depends on the drm driver, kill audio
+		 * processes first, in order to make it possible to unload
+		 * the driver.
 		 */
 		if (strstr(mod[pos].name, "snd")) {
 			if (igt_lsof_kill_audio_processes()) {
@@ -599,6 +616,13 @@ int igt_audio_driver_unload(const char **who)
 	}
 
 ret:
+	if (ret) {
+		igt_warn("Couldn't unload %s, which is using the %s driver\n",
+			 mod[pos].name, drm_driver);
+		igt_kmod_list_loaded();
+		igt_lsof("/dev/snd");
+	}
+
 	free_module_ref(mod, num_mod);
 
 	return ret;
diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index 7c68b336e67b..3888c660d4ac 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -149,11 +149,8 @@ static void driver_unbind(struct hotunplug *priv, const char *prefix,
 	 * unbind i915 driver, reloading it when binding again.
 	 */
 	if (igt_audio_driver_unload(&priv->snd_driver)) {
-		if (priv->snd_driver)
-			igt_warn("Could not unload audio driver %s\n", priv->snd_driver);
-		igt_kmod_list_loaded();
-		igt_lsof("/dev/snd");
-		igt_skip("Audio is in use, skipping\n");
+		igt_skip("Audio driver %s in use, skipping test\n",
+			 priv->snd_driver);
 	} else if (priv->snd_driver) {
 		igt_info("Unloaded audio driver %s\n", priv->snd_driver);
 	}
-- 
2.35.1

[igt-dev] [PATCH i-g-t v2 6/6] lib/igt_kmod: properly handle pipewire-pulse

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

Newer distributions like Fedora and openSUSE thumbweed are now
coming with pipewire-pulse instead of pulseaudio - either as
default or as an optional audio stack.

That adds a new requirement when unloading sound drivers,
which, in turn, is needed when the DRM driver is bound into
it.

Add the needed logic to work properly in case pipewire-pulse
is detected.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 lib/igt_aux.c  | 122 +++++++++++++++++++++++++++++++++++++++++++++----
 lib/igt_aux.h  |   4 +-
 lib/igt_core.c |   7 +++
 lib/igt_core.h |   1 +
 lib/igt_kmod.c |  17 ++++++-
 5 files changed, 140 insertions(+), 11 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 15fe87839567..a0da1d042ab4 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -1434,8 +1434,109 @@ static void pulseaudio_unload_module(proc_t *proc_info)
 	}
 }
 
+static void pipewire_reserve_wait(int pipewire_pulse_pid)
+{
+	char xdg_dir[PATH_MAX];
+	const char *homedir;
+	struct passwd *pw;
+	proc_t *proc_info;
+	PROCTAB *proc;
+
+	igt_fork(child, 1) {
+		igt_info("Preventing pipewire-pulse to use the audio drivers\n");
+
+		proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
+		igt_assert(proc != NULL);
+
+		while ((proc_info = readproc(proc, NULL))) {
+			if (pipewire_pulse_pid == proc_info->tid)
+				break;
+			freeproc(proc_info);
+		}
+		closeproc(proc);
+
+		/* Sanity check: if it can't find the process, it means it has gone */
+		if (pipewire_pulse_pid != proc_info->tid)
+			exit(0);
+
+		pw = getpwuid(proc_info->euid);
+		homedir = pw->pw_dir;
+		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
+		setgid(proc_info->egid);
+		setuid(proc_info->euid);
+		clearenv();
+		setenv("HOME", homedir, 1);
+		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
+		freeproc(proc_info);
+
+		exit(system("pw-reserve -n Audio0 -r"));
+	}
+}
+
+static int pipewire_pw_reserve_pid = 0;
+
+/* Maximum time waiting for pw-reserve to start running */
+#define PIPEWIRE_RESERVE_MAX_TIME 1000 /* milisseconds */
+
+int pipewire_pulse_start_reserve(int pipewire_pulse_pid)
+{
+	bool is_pw_reserve_running = false;
+	proc_t *proc_info;
+	int attempts = 0;
+	PROCTAB *proc;
+
+	if (!pipewire_pulse_pid)
+		return 0;
+
+	pipewire_reserve_wait(pipewire_pulse_pid);
+
+	/*
+	 * Note: using pw-reserve to stop using audio only works with
+	 * pipewire version 0.3.50 or upper.
+	 */
+	for (attempts = 0; attempts < PIPEWIRE_RESERVE_MAX_TIME; attempts++) {
+		usleep(1000);
+		proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
+		igt_assert(proc != NULL);
+
+		while ((proc_info = readproc(proc, NULL))) {
+			if (!strcmp(proc_info->cmd, "pw-reserve")) {
+				is_pw_reserve_running = true;
+				pipewire_pw_reserve_pid = proc_info->tid;
+				freeproc(proc_info);
+				break;
+			}
+			freeproc(proc_info);
+		}
+		closeproc(proc);
+		if (is_pw_reserve_running)
+			break;
+	}
+	if (!is_pw_reserve_running) {
+		igt_warn("Failed to remove audio drivers from pipewire\n");
+		return 1;
+	}
+	/* Let's grant some time for pw_reserve to notify pipewire via D-BUS */
+	usleep(50000);
+
+	/*
+	 * pw-reserve is running, and should have stopped using the audio
+	 * drivers. We can now remove the driver.
+	 */
+
+	return 0;
+}
+
+void pipewire_pulse_stop_reserve(int pipewire_pulse_pid)
+{
+	if (!pipewire_pulse_pid)
+		return;
+
+	igt_killchildren(SIGTERM);
+}
+
 static int
-__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
+__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path, int *pipewire_pulse_pid)
 {
 	const char *audio_dev = "/dev/snd/";
 	char path[PATH_MAX * 2];
@@ -1482,9 +1583,9 @@ __igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
 		}
 
 		/*
-		 * FIXME: terminating pipewire-pulse is not that easy, as
-		 * pipewire there's no standard way up to pipewire version
-		 * 0.3.49. Just trying to kill pipewire will start a race
+		 * Terminating pipewire-pulse require an special procedure,
+		 * which is only available at version 0.3.50 and upper.
+		 * Just trying to kill pipewire will start a race
 		 * between IGT and systemd. If IGT wins, the audio driver
 		 * will be unloaded before systemd tries to reload it, but
 		 * if systemd wins, the audio device will be re-opened and
@@ -1495,9 +1596,13 @@ __igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
 		 *	 pw-reserve -n Audio0 -r
 		 * 2) unload/unbind the the audio driver(s);
 		 * 3) stop the pw-reserve thread.
-		 *
-		 * We should add support for it once distros start shipping it.
 		 */
+		if (!strcmp(proc_info->cmd, "pipewire-pulse")) {
+			igt_info("process %d (%s) is using audio device. Should be requested to stop using them.\n",
+				 proc_info->tid, proc_info->cmd);
+			*pipewire_pulse_pid = proc_info->tid;
+			break;
+		}
 
 		/* For all other processes, just kill them */
 		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
@@ -1528,7 +1633,7 @@ __igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
  * daemons are respanned if they got killed.
  */
 int
-igt_lsof_kill_audio_processes(void)
+igt_lsof_kill_audio_processes(int *pipewire_pulse_pid)
 {
 	char path[PATH_MAX];
 	proc_t *proc_info;
@@ -1537,12 +1642,13 @@ igt_lsof_kill_audio_processes(void)
 
 	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
 	igt_assert(proc != NULL);
+	*pipewire_pulse_pid = 0;
 
 	while ((proc_info = readproc(proc, NULL))) {
 		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1) {
 			fail++;
 		} else {
-			fail += __igt_lsof_kill_proc(proc_info, path);
+			fail += __igt_lsof_kill_proc(proc_info, path, pipewire_pulse_pid);
 		}
 
 		freeproc(proc_info);
diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index bb96d1afb777..d8b05088f0ba 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -306,7 +306,9 @@ bool igt_allow_unlimited_files(void);
 int igt_is_process_running(const char *comm);
 int igt_terminate_process(int sig, const char *comm);
 void igt_lsof(const char *dpath);
-int igt_lsof_kill_audio_processes(void);
+int igt_lsof_kill_audio_processes(int *pipewire_pulse_pid);
+int pipewire_pulse_start_reserve(int pipewire_pulse_pid);
+void pipewire_pulse_stop_reserve(int pipewire_pulse_pid);
 
 #define igt_hweight(x) \
 	__builtin_choose_expr(sizeof(x) == 8, \
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 6dad3c84858f..10b3e37656bd 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -2427,6 +2427,13 @@ static void igt_alarm_killchildren(int signal)
 	kill_children();
 }
 
+void igt_killchildren(int signal)
+{
+	igt_info("Timed out waiting for children\n");
+
+	kill_children();
+}
+
 /**
  * igt_waitchildren_timeout:
  * @seconds: timeout in seconds to wait
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 78dc6202ced4..c62a31e016af 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -1103,6 +1103,7 @@ bool __igt_fork(void);
 int __igt_waitchildren(void);
 void igt_waitchildren(void);
 void igt_waitchildren_timeout(int seconds, const char *reason);
+void igt_killchildren(int signal);
 
 /**
  * igt_helper_process:
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index d1d0662f3706..5a6af050c272 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -392,6 +392,7 @@ igt_i915_driver_load(const char *opts)
 
 static int igt_always_unload_audio_driver(const char **who)
 {
+	int pipewire_pulse_pid;
 	int ret;
 	const char *sound[] = {
 		"snd_hda_intel",
@@ -410,15 +411,19 @@ static int igt_always_unload_audio_driver(const char **who)
 		if (igt_kmod_is_loaded(*m)) {
 			if (who)
 				*who = *m;
-			if (igt_lsof_kill_audio_processes()) {
+			if (igt_lsof_kill_audio_processes(&pipewire_pulse_pid)) {
 				igt_warn("Could not stop audio process(es)\n");
 				igt_kmod_list_loaded();
 				igt_lsof("/dev/snd");
 				return 0;
 			}
 
+			ret = pipewire_pulse_start_reserve(pipewire_pulse_pid);
+			if (ret)
+				igt_warn("Failed to notify pipewire_pulse\n");
 			kick_snd_hda_intel();
 			ret = igt_kmod_unload(*m, 0);
+			pipewire_pulse_stop_reserve(pipewire_pulse_pid);
 			if (ret) {
 				igt_warn("Could not unload audio driver %s\n", *m);
 				igt_kmod_list_loaded();
@@ -564,6 +569,7 @@ int igt_audio_driver_unload(const char **who)
 {
 	const char *drm_driver = "i915";
 	unsigned int num_mod, i, j;
+	int pipewire_pulse_pid = 0;
 	struct module_ref *mod;
 	int pos = -1;
 	int ret = 0;
@@ -607,12 +613,19 @@ int igt_audio_driver_unload(const char **who)
 		 * the driver.
 		 */
 		if (strstr(mod[pos].name, "snd")) {
-			if (igt_lsof_kill_audio_processes()) {
+			if (igt_lsof_kill_audio_processes(&pipewire_pulse_pid)) {
 				ret = 1;
 				goto ret;
 			}
 		}
+
+		ret = pipewire_pulse_start_reserve(pipewire_pulse_pid);
+		if (ret)
+			igt_warn("Failed to notify pipewire_pulse\n");
 		ret = igt_unload_driver(mod, num_mod, pos);
+		pipewire_pulse_stop_reserve(pipewire_pulse_pid);
+		if (ret)
+			break;
 	}
 
 ret:
-- 
2.35.1

[igt-dev] ✗ Fi.CI.BAT: failure for Improve logic to work with audio dependency on DRM driver

[Patchwork]

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

== Series Details ==

Series: Improve logic to work with audio dependency on DRM driver
URL   : https://patchwork.freedesktop.org/series/103530/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11601 -> IGTPW_7037
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_7037 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_7037, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/index.html

Participating hosts (44 -> 43)
------------------------------

  Additional (2): bat-rpls-1 bat-adlm-1 
  Missing    (3): fi-bsw-cyan bat-jsl-2 fi-icl-u2 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_7037:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gtt:
    - bat-adlp-4:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/bat-adlp-4/igt@i915_selftest@live@gtt.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/bat-adlp-4/igt@i915_selftest@live@gtt.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gt_mocs:
    - {bat-adlm-1}:       NOTRUN -> [INCOMPLETE][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/bat-adlm-1/igt@i915_selftest@live@gt_mocs.html

  
Known issues
------------

  Here are the changes found in IGTPW_7037 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@hangcheck:
    - fi-bdw-5557u:       NOTRUN -> [INCOMPLETE][4] ([i915#3921])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/fi-bdw-5557u/igt@i915_selftest@live@hangcheck.html
    - fi-snb-2600:        [PASS][5] -> [INCOMPLETE][6] ([i915#3921])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  * igt@kms_busy@basic@flip:
    - bat-adlp-4:         [PASS][7] -> [DMESG-WARN][8] ([i915#3576]) +2 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/bat-adlp-4/igt@kms_busy@basic@flip.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/bat-adlp-4/igt@kms_busy@basic@flip.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-cfl-8109u:       [DMESG-WARN][9] ([i915#62]) -> [PASS][10] +81 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/fi-cfl-8109u/igt@i915_pm_rpm@module-reload.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/fi-cfl-8109u/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@client:
    - {bat-dg2-9}:        [DMESG-FAIL][11] ([i915#5879]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/bat-dg2-9/igt@i915_selftest@live@client.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/bat-dg2-9/igt@i915_selftest@live@client.html

  * igt@i915_selftest@live@memory_region:
    - fi-cfl-8109u:       [DMESG-WARN][13] -> [PASS][14] +34 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/fi-cfl-8109u/igt@i915_selftest@live@memory_region.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/fi-cfl-8109u/igt@i915_selftest@live@memory_region.html

  * igt@i915_selftest@live@mman:
    - fi-bdw-5557u:       [INCOMPLETE][15] ([i915#5704]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/fi-bdw-5557u/igt@i915_selftest@live@mman.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/fi-bdw-5557u/igt@i915_selftest@live@mman.html

  * igt@kms_flip@basic-flip-vs-modeset@b-edp1:
    - {bat-adlp-6}:       [DMESG-WARN][17] ([i915#3576]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/bat-adlp-6/igt@kms_flip@basic-flip-vs-modeset@b-edp1.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/bat-adlp-6/igt@kms_flip@basic-flip-vs-modeset@b-edp1.html
    - bat-adlp-4:         [DMESG-WARN][19] ([i915#3576]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11601/bat-adlp-4/igt@kms_flip@basic-flip-vs-modeset@b-edp1.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/bat-adlp-4/igt@kms_flip@basic-flip-vs-modeset@b-edp1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5704]: https://gitlab.freedesktop.org/drm/intel/issues/5704
  [i915#5828]: https://gitlab.freedesktop.org/drm/intel/issues/5828
  [i915#5879]: https://gitlab.freedesktop.org/drm/intel/issues/5879
  [i915#5885]: https://gitlab.freedesktop.org/drm/intel/issues/5885
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_6465 -> IGTPW_7037

  CI-20190529: 20190529
  CI_DRM_11601: 387569698a0a6c4fefffe315446e51f169f52e7b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_7037: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/index.html
  IGT_6465: f6bb4399881a806fbff75ce3df89b60286d55917 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7037/index.html

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

Re: [igt-dev] [PATCH i-g-t v2 1/6] tests/core_hotunplug: properly finish processes using audio devices

[Andi Shyti]

Hi Mauro,

[...]

> +static void pulseaudio_unload_module(proc_t *proc_info)
> +{
> +	char xdg_dir[PATH_MAX];
> +	const char *homedir;
> +	struct passwd *pw;
> +
> +	igt_fork(child, 1) {
> +		pw = getpwuid(proc_info->euid);
> +		homedir = pw->pw_dir;
> +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> +
> +		igt_info("Ask pulseaudio to stop using audio device\n");
> +
> +		setgid(proc_info->egid);
> +		setuid(proc_info->euid);
> +		clearenv();
> +		setenv("HOME", homedir, 1);
> +		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> +
> +		exit(system("for i in $(pacmd list-sources|grep module:|cut -d : -f 2); do pactl unload-module $i; done"));

you shouldn't need for the exit()

> +	}

do we need for igt_waitchildren() at the end?

> +}
> +
> +static int
> +__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
> +{
> +	const char *audio_dev = "/dev/snd/";
> +	char path[PATH_MAX * 2];
> +	struct dirent *d;
> +	struct stat st;
> +	char *fd_lnk;
> +	int fail = 0;
> +	ssize_t read;

nitpick: shall we move some of the variables above inside the
while loop?

> +	DIR *dp = opendir(proc_path);
> +	igt_assert(dp);
> +
> +	while ((d = readdir(dp))) {
> +		if (*d->d_name == '.')
> +			continue;

do we need to consider ".." as well? "by-path"?

> +		memset(path, 0, sizeof(path));
> +		snprintf(path, sizeof(path), "%s/%s", proc_path, d->d_name);
> +
> +		if (lstat(path, &st) == -1)
> +			continue;

should this be treated as an unlikely error?

> +		fd_lnk = malloc(st.st_size + 1);
> +
> +		igt_assert((read = readlink(path, fd_lnk, st.st_size + 1)));
> +		fd_lnk[read] = '\0';

are we sure that in /dev/snd we have only links? In my PC I have
only character interfaces.

> +		if (strncmp(audio_dev, fd_lnk, strlen(audio_dev))) {
> +			free(fd_lnk);
> +			continue;
> +		}
> +
> +		free(fd_lnk);
> +
> +		/*
> +		 * In order to avoid racing against pa/systemd, ensure that
> +		 * pulseaudio will close all audio files. This should be
> +		 * enough to unbind audio modules and won't case race issues

/case/cause/

> +		 * with systemd trying to reload it.
> +		 */
> +		if (!strcmp(proc_info->cmd, "pulseaudio")) {
> +			pulseaudio_unload_module(proc_info);
> +			break;
> +		}
> +
> +		/*
> +		 * FIXME: terminating pipewire-pulse is not that easy, as
> +		 * pipewire there's no standard way up to pipewire version
> +		 * 0.3.49. Just trying to kill pipewire will start a race
> +		 * between IGT and systemd. If IGT wins, the audio driver
> +		 * will be unloaded before systemd tries to reload it, but
> +		 * if systemd wins, the audio device will be re-opened and
> +		 * used before IGT has a chance to remove the audio driver.
> +		 * Pipewire version 0.3.50 should bring a standard way:
> +		 *
> +		 * 1) start a thread running:
> +		 *	 pw-reserve -n Audio0 -r
> +		 * 2) unload/unbind the the audio driver(s);
> +		 * 3) stop the pw-reserve thread.
> +		 *
> +		 * We should add support for it once distros start shipping it.
> +		 */

thanks for the explanation!

> +		/* For all other processes, just kill them */
> +		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
> +				proc_info->tid, proc_info->cmd);
> +
> +		if (kill(proc_info->tid, SIGTERM) < 0) {
> +			igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n",
> +				proc_info->cmd, proc_info->tid);
> +			if (kill(proc_info->tid, SIGABRT) < 0) {
> +				fail++;
> +				igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n",
> +					proc_info->cmd, proc_info->tid);
> +			}
> +		}
> +
> +		break;
> +	}
> +
> +	closedir(dp);
> +	return fail;
> +}
> +
> +/*
> + * This function identifies each process running on the machine that is
> + * opening an audio device and tries to stop it.
> + *
> + * Special care should be taken with pipewire and pipewire-pulse, as those
> + * daemons are respanned if they got killed.
> + */
> +int
> +igt_lsof_kill_audio_processes(void)
> +{
> +	char path[PATH_MAX];
> +	proc_t *proc_info;
> +	PROCTAB *proc;
> +	int fail = 0;
> +
> +	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);

you could eventually make an igt_openproc() with the assert as
this is a pattern that it's repeated several times. Up to you.

> +	igt_assert(proc != NULL);
> +
> +	while ((proc_info = readproc(proc, NULL))) {
> +		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1) {
> +			fail++;

we are counting failures but using it more as a boolean. Is there
any use of fail?

> +		} else {
> +			fail += __igt_lsof_kill_proc(proc_info, path);
> +		}

brackets not needed here

> +		freeproc(proc_info);
> +	}
> +	closeproc(proc);
> +
> +	return fail;
> +}
> +
>  static struct igt_siglatency {
>  	timer_t timer;
>  	struct timespec target;
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index 9f2588aeca90..bb96d1afb777 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -306,6 +306,7 @@ bool igt_allow_unlimited_files(void);
>  int igt_is_process_running(const char *comm);
>  int igt_terminate_process(int sig, const char *comm);
>  void igt_lsof(const char *dpath);
> +int igt_lsof_kill_audio_processes(void);
>  
>  #define igt_hweight(x) \
>  	__builtin_choose_expr(sizeof(x) == 8, \
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index f252535d5a3a..87a59245f699 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -389,7 +389,7 @@ igt_i915_driver_load(const char *opts)
>  	return 0;
>  }
>  
> -int __igt_i915_driver_unload(const char **who)
> +int igt_audio_driver_unload(const char **who)
>  {
>  	int ret;
>  	const char *sound[] = {
> @@ -398,6 +398,27 @@ int __igt_i915_driver_unload(const char **who)
>  		NULL,
>  	};
>  
> +	for (const char **m = sound; *m; m++) {
> +		if (igt_kmod_is_loaded(*m)) {

little insignificant nit: with

	if (!igt_kmod_is_loaded(*m))
		continue;

you save one level of indentation.

> +			if (igt_lsof_kill_audio_processes())
> +				return 1;

mmhhh... I don't like the return 1 because makes this function a
hybrid boolean/error function and we can't rely anyomore on the
return value. Please identify a proper errno to return.

> +
> +			kick_snd_hda_intel();
> +			ret = igt_kmod_unload(*m, 0);
> +			if (ret) {
> +				if (who)
> +					*who = *m;
> +				return ret;
> +			}
> +		}
> +	}
> +	return 0;
> +}

[...]

> @@ -152,19 +152,14 @@ static void driver_unbind(struct hotunplug *priv, const char *prefix,
>  	 * safest and easiest way out.
>  	 */
>  	if (priv->snd_unload) {
> -		igt_terminate_process(SIGTERM, "alsactl");
> -
> -		/* unbind snd_hda_intel */
> -		kick_snd_hda_intel();
> -
> -		if (igt_kmod_unload("snd_hda_intel", 0)) {
> +		if (igt_audio_driver_unload(NULL)) {

why give NULL insteadd of a proper string? We can provide a
better log.

>  			priv->snd_unload = false;
> -			igt_warn("Could not unload snd_hda_intel\n");
> +			igt_warn("Could not unload audio driver\n");
>  			igt_kmod_list_loaded();
>  			igt_lsof("/dev/snd");
>  			igt_skip("Audio is in use, skipping\n");
>  		} else {
> -			igt_info("Preventively unloaded snd_hda_intel\n");
> +			igt_info("Preventively unloaded audio driver\n");
>  		}
>  	}

Andi

Re: [igt-dev] [PATCH i-g-t v2 2/6] lib/igt_kmod: always fill who when unloading audio driver

[Andi Shyti]

Hi Mauro,

On Wed, May 04, 2022 at 11:59:00AM +0200, Mauro Carvalho Chehab wrote:
> From: Mauro Carvalho Chehab <mchehab@kernel.org>
> 
> As we'll use this information at core_hotunplug to announce
> when an audio module is unloaded, fill it even if return code
> is zero.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
>  lib/igt_aux.c  | 2 +-
>  lib/igt_kmod.c | 7 +++----
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 4f1d88ed68dd..15fe87839567 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -1422,7 +1422,7 @@ static void pulseaudio_unload_module(proc_t *proc_info)
>  		homedir = pw->pw_dir;
>  		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
>  
> -		igt_info("Ask pulseaudio to stop using audio device\n");
> +		igt_info("Request pulseaudio to stop using audio device\n");
>  
>  		setgid(proc_info->egid);
>  		setuid(proc_info->euid);
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 87a59245f699..716e03f426c9 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -400,16 +400,15 @@ int igt_audio_driver_unload(const char **who)
>  
>  	for (const char **m = sound; *m; m++) {
>  		if (igt_kmod_is_loaded(*m)) {
> +			if (who)
> +				*who = *m;
>  			if (igt_lsof_kill_audio_processes())
>  				return 1;
>  
>  			kick_snd_hda_intel();
>  			ret = igt_kmod_unload(*m, 0);
> -			if (ret) {
> -				if (who)
> -					*who = *m;
> +			if (ret)
>  				return ret;
> -			}

this answers one of my comments from the previous patch :)

It could have been squashed with the previous... but it doesn't
really matter:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

>  		}
>  	}
>  	return 0;
> -- 
> 2.35.1

Re: [igt-dev] [PATCH i-g-t v2 3/6] lib/igt_kmod: improve audio unbind logic

[Andi Shyti]

Hi Mauro,

> +static struct module_ref *read_module_dependencies(unsigned int *num_mod)
> +{
> +	FILE *fp = fopen("/proc/modules", "r");
> +	struct module_ref *mod = NULL;
> +	size_t line_buf_size = 0;
> +	char *required_by, *p;
> +	unsigned n_mod = 0;
> +	unsigned num_req;
> +	char *line = NULL;
> +	int len = 0;
> +	int i, ret;
> +
> +	igt_assert(fp);
> +
> +	while ((len = getline(&line, &line_buf_size, fp)) > 0) {
> +		mod = realloc(mod, (n_mod + 1) * sizeof(*mod));
> +		igt_assert(mod);
> +		mod[n_mod].required_by = NULL;
> +
> +		p = strtok(line, " ");
> +		mod[n_mod].name = strdup(p);
> +
> +		p = strtok(NULL, " ");
> +		ret = sscanf(p, "%lu", &mod[n_mod].mem);
> +		igt_assert(ret == 1);
> +
> +		p = strtok(NULL, " ");
> +		ret = sscanf(p, "%u", &mod[n_mod].ref_count);
> +		igt_assert(ret == 1);
> +
> +		num_req = 0;
> +		required_by = strtok(NULL, " ");
> +		if (strcmp(required_by, "-")) {
> +			p = strtok(required_by, ",");
> +			while (p) {
> +				for (i = 0; i < n_mod; i++) {
> +					if (!strcmp(p, mod[i].name))
> +						break;
> +				}

brackets not needed here

> +				igt_assert(i < n_mod);
> +
> +				mod[n_mod].required_by = realloc(mod[n_mod].required_by,
> +								 (num_req + 1) * sizeof(unsigned int));
> +				mod[n_mod].required_by[num_req] = i;
> +				num_req++;
> +				p = strtok(NULL, ",");
> +			}
> +		}
> +		mod[n_mod].num_required = num_req;
> +		n_mod++;
> +	}
> +	free(line);
> +	fclose(fp);
> +
> +	*num_mod = n_mod;
> +	return mod;
> +}

it looks correct, haven't spotted any error in this function, but
I admit that string parsing can be complex. I trust you have
tested it properly. I will check it again, though.

[...]

> +#define LINUX_VERSION(major, minor, patch)			\
> +		     ((major) << 16 | (minor) << 8 | (patch))

I can't believe that the glibc is not providing anything similar.

> +
> +

you have an extra line here

> +static int linux_kernel_version(void)
> +{
> +	struct utsname utsname;
> +	int ver[3] = { 0 };

[...]

> +
> +int igt_audio_driver_unload(const char **who)

ha! this confused me because I start reading from the bottom and
I didn't notice the above

  -int igt_audio_driver_unload(const char **who)
  +static int igt_always_unload_audio_driver(const char **who)

> +{
> +	unsigned int num_mod, i, j;
> +	struct module_ref *mod;
> +	int pos = -1;
> +	int ret = 0;
> +
> +	/*
> +	 * On older Kernels, there's no way to check if the audio driver
> +	 * binds into the DRM one. So, always remove audio drivers that
> +	 * might be binding.
> +	 */
> +	if (linux_kernel_version() < LINUX_VERSION(5, 20, 0))
> +		return igt_always_unload_audio_driver(who);

... so that this one becomes what you did in patch 1.

> +
> +	/*
> +	 * Newer Kernels gained support for showing the dependencies between
> +	 * audio and DRM drivers via /proc/modules and lsmod. Use it to
> +	 * detect if removing the audio driver is needed, properly unloading
> +	 * any audio processes that could be using the audio driver and
> +	 * handling module dependencies. Such solution should be generic
> +	 * enough to work with newer SOC/SOF drivers if ever needed.
> +	 */
> +
> +	mod = read_module_dependencies(&num_mod);
> +
> +	for (i = 0; i < num_mod; i++) {
> +		if (!strcmp(mod[i].name, "i915")) {
> +			break;
> +		}
> +	}

no need for brackets here.

> +
> +	if (i == num_mod)
> +		return 0;
> +
> +	/* Recursively remove all drivers that depend on i915 */
> +	for (j = 0; j < mod[i].num_required; j++) {
> +		pos = mod[i].required_by[j];
> +		if (who)
> +			*who = strdup(mod[pos].name);
> +		/*
> +		 * If a sound driver depends on i915, kill audio processes
> +		 * first, in order to make it possible to unload the driver
> +		 */
> +		if (strstr(mod[pos].name, "snd")) {
> +			if (igt_lsof_kill_audio_processes()) {
> +				ret = 1;

is this a boolean function? Shall we choose some proper errnos?

> +				goto ret;

do we need to free who here...

> +			}
> +		}
> +		ret = igt_unload_driver(mod, num_mod, pos);

... and here?

And here... do we need to handle the error?

> +	}
> +
> +ret:
> +	free_module_ref(mod, num_mod);
> +
> +	return ret;
> +}

Thanks,
Andi

Re: [igt-dev] [PATCH i-g-t v2 4/6] core_hotunplug: fix audio unbind logic

[Andi Shyti]

Hi Mauro,

On Wed, May 04, 2022 at 11:59:02AM +0200, Mauro Carvalho Chehab wrote:
> From: Mauro Carvalho Chehab <mchehab@kernel.org>
> 
> The current audio unbind logic is wrong: it expects the audio
> driver to depend on i915 only on Haswell, Broadwell and DG1.
> 
> That doesn't match the Kernel driver, where snd-hda-audio binds
> on i915 also on Skylake, Braswell and whenever it needs to use
> pm runtime from the DRM driver.
> 
> Now that lib/igt-kmod has gained improved support for audio
> unbind, update core_hotunplug to benefit from the newer logic.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [igt-dev] [PATCH i-g-t v2 1/6] tests/core_hotunplug: properly finish processes using audio devices

[Andi Shyti]

> > @@ -152,19 +152,14 @@ static void driver_unbind(struct hotunplug *priv, const char *prefix,
> >  	 * safest and easiest way out.
> >  	 */
> >  	if (priv->snd_unload) {
> > -		igt_terminate_process(SIGTERM, "alsactl");
> > -
> > -		/* unbind snd_hda_intel */
> > -		kick_snd_hda_intel();
> > -
> > -		if (igt_kmod_unload("snd_hda_intel", 0)) {
> > +		if (igt_audio_driver_unload(NULL)) {
> 
> why give NULL insteadd of a proper string? We can provide a
> better log.

please ignore, I see that later you changed this part.

Andi

Re: [igt-dev] [PATCH i-g-t v2 5/6] lib/igt_kmod: make it less pedantic with audio driver removal

[Andi Shyti]

Hi Mauro,

> Current Linux Kernel don't report if the audio driver binds
> into the DRM driver. As this is CPU specific, allow audio
> driver unload fail without skipping the IGT tests on legacy
> Kernels, as this may not be mandatory.
> 
> On new kernels where lsmod will properly display de dependency

/de/the/

> between the audio and DRM drivers, skip the core hotunplug
> test if it fails to unload the audio driver, as this is is

/is//

[...]

>  int igt_audio_driver_unload(const char **who)
>  {
> +	const char *drm_driver = "i915";
>  	unsigned int num_mod, i, j;
>  	struct module_ref *mod;
>  	int pos = -1;
> @@ -572,7 +588,7 @@ int igt_audio_driver_unload(const char **who)
>  	mod = read_module_dependencies(&num_mod);
>  
>  	for (i = 0; i < num_mod; i++) {
> -		if (!strcmp(mod[i].name, "i915")) {
> +		if (!strcmp(mod[i].name, drm_driver)) {
>  			break;
>  		}
>  	}
> @@ -580,14 +596,15 @@ int igt_audio_driver_unload(const char **who)
>  	if (i == num_mod)
>  		return 0;
>  
> -	/* Recursively remove all drivers that depend on i915 */
> +	/* Recursively remove all drivers that depend on the drm driver */
>  	for (j = 0; j < mod[i].num_required; j++) {
>  		pos = mod[i].required_by[j];
>  		if (who)
>  			*who = strdup(mod[pos].name);
>  		/*
> -		 * If a sound driver depends on i915, kill audio processes
> -		 * first, in order to make it possible to unload the driver
> +		 * If a sound driver depends on the drm driver, kill audio
> +		 * processes first, in order to make it possible to unload
> +		 * the driver.

Why isn't i915 --> drm_driver done in patch 3?

[...]

Looks good, though,

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Andi

Re: [igt-dev] [PATCH i-g-t v2 6/6] lib/igt_kmod: properly handle pipewire-pulse

[Andi Shyti]

Hi Mauro,

> Newer distributions like Fedora and openSUSE thumbweed are now
> coming with pipewire-pulse instead of pulseaudio - either as
> default or as an optional audio stack.
> 
> That adds a new requirement when unloading sound drivers,
> which, in turn, is needed when the DRM driver is bound into
> it.
> 
> Add the needed logic to work properly in case pipewire-pulse
> is detected.

stubborn processes that don't want to do what the user tells them
to do :)

> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
>  lib/igt_aux.c  | 122 +++++++++++++++++++++++++++++++++++++++++++++----
>  lib/igt_aux.h  |   4 +-
>  lib/igt_core.c |   7 +++
>  lib/igt_core.h |   1 +
>  lib/igt_kmod.c |  17 ++++++-
>  5 files changed, 140 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 15fe87839567..a0da1d042ab4 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -1434,8 +1434,109 @@ static void pulseaudio_unload_module(proc_t *proc_info)
>  	}
>  }
>  
> +static void pipewire_reserve_wait(int pipewire_pulse_pid)
> +{
> +	char xdg_dir[PATH_MAX];
> +	const char *homedir;
> +	struct passwd *pw;
> +	proc_t *proc_info;
> +	PROCTAB *proc;
> +
> +	igt_fork(child, 1) {
> +		igt_info("Preventing pipewire-pulse to use the audio drivers\n");
> +
> +		proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> +		igt_assert(proc != NULL);
> +
> +		while ((proc_info = readproc(proc, NULL))) {
> +			if (pipewire_pulse_pid == proc_info->tid)
> +				break;
> +			freeproc(proc_info);
> +		}
> +		closeproc(proc);
> +
> +		/* Sanity check: if it can't find the process, it means it has gone */
> +		if (pipewire_pulse_pid != proc_info->tid)
> +			exit(0);
> +
> +		pw = getpwuid(proc_info->euid);
> +		homedir = pw->pw_dir;
> +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> +		setgid(proc_info->egid);
> +		setuid(proc_info->euid);
> +		clearenv();
> +		setenv("HOME", homedir, 1);
> +		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> +		freeproc(proc_info);
> +
> +		exit(system("pw-reserve -n Audio0 -r"));

no need for exit

> +	}

we need waitchild, I guess.

> +}
> +
> +static int pipewire_pw_reserve_pid = 0;
> +
> +/* Maximum time waiting for pw-reserve to start running */
> +#define PIPEWIRE_RESERVE_MAX_TIME 1000 /* milisseconds */
> +
> +int pipewire_pulse_start_reserve(int pipewire_pulse_pid)
> +{
> +	bool is_pw_reserve_running = false;
> +	proc_t *proc_info;
> +	int attempts = 0;
> +	PROCTAB *proc;
> +
> +	if (!pipewire_pulse_pid)
> +		return 0;
> +
> +	pipewire_reserve_wait(pipewire_pulse_pid);
> +
> +	/*
> +	 * Note: using pw-reserve to stop using audio only works with
> +	 * pipewire version 0.3.50 or upper.
> +	 */
> +	for (attempts = 0; attempts < PIPEWIRE_RESERVE_MAX_TIME; attempts++) {
> +		usleep(1000);
> +		proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> +		igt_assert(proc != NULL);
> +
> +		while ((proc_info = readproc(proc, NULL))) {
> +			if (!strcmp(proc_info->cmd, "pw-reserve")) {
> +				is_pw_reserve_running = true;
> +				pipewire_pw_reserve_pid = proc_info->tid;
> +				freeproc(proc_info);
> +				break;
> +			}
> +			freeproc(proc_info);
> +		}
> +		closeproc(proc);
> +		if (is_pw_reserve_running)
> +			break;
> +	}
> +	if (!is_pw_reserve_running) {
> +		igt_warn("Failed to remove audio drivers from pipewire\n");
> +		return 1;
> +	}
> +	/* Let's grant some time for pw_reserve to notify pipewire via D-BUS */
> +	usleep(50000);
> +
> +	/*
> +	 * pw-reserve is running, and should have stopped using the audio
> +	 * drivers. We can now remove the driver.
> +	 */
> +
> +	return 0;
> +}
> +
> +void pipewire_pulse_stop_reserve(int pipewire_pulse_pid)
> +{
> +	if (!pipewire_pulse_pid)
> +		return;
> +
> +	igt_killchildren(SIGTERM);
> +}
> +
>  static int
> -__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
> +__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path, int *pipewire_pulse_pid)

here we lose some clarity as in 6 patches these functions are
slightly changing their meaning.

__igt_lsof_kill_proc() by the name is supposed to kill a process
using a resource. But in the meantime, cunningly, is also telling
if the process killed is pipewire. But this meaning is intrinsic
in the parameter.

It would be nicer to have a better understanding of the function
in a first glance. I dare a new person, who doesn't really know
what's going on, to understand the meaning of the "int
*pipewire_pulse_pid".

Perhaps, for the sake of clarity, we can add some extra function
that tells immediately if the sound user is pipewire or others.

[...]

Andi

Re: [igt-dev] [PATCH i-g-t v2 6/6] lib/igt_kmod: properly handle pipewire-pulse

[Mauro Carvalho Chehab]

On Thu, 5 May 2022 20:18:51 +0200
Andi Shyti <andi.shyti@linux.intel.com> wrote:

> Hi Mauro,
> 
> > Newer distributions like Fedora and openSUSE thumbweed are now
> > coming with pipewire-pulse instead of pulseaudio - either as
> > default or as an optional audio stack.
> > 
> > That adds a new requirement when unloading sound drivers,
> > which, in turn, is needed when the DRM driver is bound into
> > it.
> > 
> > Add the needed logic to work properly in case pipewire-pulse
> > is detected.  
> 
> stubborn processes that don't want to do what the user tells them
> to do :)

heh, yes!

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > ---
> >  lib/igt_aux.c  | 122 +++++++++++++++++++++++++++++++++++++++++++++----
> >  lib/igt_aux.h  |   4 +-
> >  lib/igt_core.c |   7 +++
> >  lib/igt_core.h |   1 +
> >  lib/igt_kmod.c |  17 ++++++-
> >  5 files changed, 140 insertions(+), 11 deletions(-)
> > 
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index 15fe87839567..a0da1d042ab4 100644
> > --- a/lib/igt_aux.c
> > +++ b/lib/igt_aux.c
> > @@ -1434,8 +1434,109 @@ static void pulseaudio_unload_module(proc_t *proc_info)
> >  	}
> >  }
> >  
> > +static void pipewire_reserve_wait(int pipewire_pulse_pid)
> > +{
> > +	char xdg_dir[PATH_MAX];
> > +	const char *homedir;
> > +	struct passwd *pw;
> > +	proc_t *proc_info;
> > +	PROCTAB *proc;
> > +
> > +	igt_fork(child, 1) {
> > +		igt_info("Preventing pipewire-pulse to use the audio drivers\n");
> > +
> > +		proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> > +		igt_assert(proc != NULL);
> > +
> > +		while ((proc_info = readproc(proc, NULL))) {
> > +			if (pipewire_pulse_pid == proc_info->tid)
> > +				break;
> > +			freeproc(proc_info);
> > +		}
> > +		closeproc(proc);
> > +
> > +		/* Sanity check: if it can't find the process, it means it has gone */
> > +		if (pipewire_pulse_pid != proc_info->tid)
> > +			exit(0);
> > +
> > +		pw = getpwuid(proc_info->euid);
> > +		homedir = pw->pw_dir;
> > +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> > +		setgid(proc_info->egid);
> > +		setuid(proc_info->euid);
> > +		clearenv();
> > +		setenv("HOME", homedir, 1);
> > +		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> > +		freeproc(proc_info);
> > +
> > +		exit(system("pw-reserve -n Audio0 -r"));  
> 
> no need for exit
> 
> > +	}  
> 
> we need waitchild, I guess.

I guess not. Assuming that there's just the *pulse* process running,
this is what happens:

1) for pulseaudio:

We need to call some pa-specific processes and wait for their exit.
After the proccess exit, remove the alsa device.

2) for pipewire-pulse:

We need to call pw-reserve, wait for it to send a pipewire-specific
signaling via DBUS and, while pw-reserve is still running, remove
the alsa device, as, at the moment pw-reserve exits, it will send
another message via DBUS that will make pipewire to re-bind at the
audio devices.

So, in this particular case, what the logic is doing is waiting for
the pa-reserve process to appear, then give some time for the DBUS
message to arrive pipewire-pulse.

> 
> > +}
> > +
> > +static int pipewire_pw_reserve_pid = 0;
> > +
> > +/* Maximum time waiting for pw-reserve to start running */
> > +#define PIPEWIRE_RESERVE_MAX_TIME 1000 /* milisseconds */
> > +
> > +int pipewire_pulse_start_reserve(int pipewire_pulse_pid)
> > +{
> > +	bool is_pw_reserve_running = false;
> > +	proc_t *proc_info;
> > +	int attempts = 0;
> > +	PROCTAB *proc;
> > +
> > +	if (!pipewire_pulse_pid)
> > +		return 0;
> > +
> > +	pipewire_reserve_wait(pipewire_pulse_pid);
> > +
> > +	/*
> > +	 * Note: using pw-reserve to stop using audio only works with
> > +	 * pipewire version 0.3.50 or upper.
> > +	 */
> > +	for (attempts = 0; attempts < PIPEWIRE_RESERVE_MAX_TIME; attempts++) {
> > +		usleep(1000);
> > +		proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);
> > +		igt_assert(proc != NULL);
> > +
> > +		while ((proc_info = readproc(proc, NULL))) {
> > +			if (!strcmp(proc_info->cmd, "pw-reserve")) {
> > +				is_pw_reserve_running = true;
> > +				pipewire_pw_reserve_pid = proc_info->tid;
> > +				freeproc(proc_info);
> > +				break;
> > +			}
> > +			freeproc(proc_info);
> > +		}
> > +		closeproc(proc);
> > +		if (is_pw_reserve_running)
> > +			break;
> > +	}
> > +	if (!is_pw_reserve_running) {
> > +		igt_warn("Failed to remove audio drivers from pipewire\n");
> > +		return 1;
> > +	}
> > +	/* Let's grant some time for pw_reserve to notify pipewire via D-BUS */
> > +	usleep(50000);
> > +
> > +	/*
> > +	 * pw-reserve is running, and should have stopped using the audio
> > +	 * drivers. We can now remove the driver.
> > +	 */
> > +
> > +	return 0;
> > +}
> > +
> > +void pipewire_pulse_stop_reserve(int pipewire_pulse_pid)
> > +{
> > +	if (!pipewire_pulse_pid)
> > +		return;
> > +
> > +	igt_killchildren(SIGTERM);
> > +}
> > +
> >  static int
> > -__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
> > +__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path, int *pipewire_pulse_pid)  
> 
> here we lose some clarity as in 6 patches these functions are
> slightly changing their meaning.
> 
> __igt_lsof_kill_proc() by the name is supposed to kill a process
> using a resource. But in the meantime, cunningly, is also telling
> if the process killed is pipewire. But this meaning is intrinsic
> in the parameter.

Yeah, the function name is not precise. it should be something like
__igt_lsof_audio_helper_and_kill_proc().

Perhaps I could add some documentation before it explaining what
this is supposed to do.

Originally, it was designed to kill audio processes and stop pulseaudio
to bind audio devices, but this doesn't work with pipewire-pulse.

As explained earlier, pipewire-pulse requires something similar to
this bash script snippet:

	pw-reserve -n Audio0 -r &
	child_pid=$!
	usleep 100000 		# give some time for the process to start, send a DBUS msg and wait for pipewire-pulse to handle the msg
	rmmod snd_hda_audio
	kill $child_pid

(instead of usleep, we could run lsof on a loop waiting for it to
unbind from /dev/snd, until some timeout, but this would increase
complexity and could still fail, so I opted to just use usleep on
this series, as this should likely be enough for IGT)

> It would be nicer to have a better understanding of the function
> in a first glance. I dare a new person, who doesn't really know
> what's going on, to understand the meaning of the "int
> *pipewire_pulse_pid".
> 
> Perhaps, for the sake of clarity, we can add some extra function
> that tells immediately if the sound user is pipewire or others.

I guess a proper description of the function should be enough.
What do we use on IGT? kerneldoc?

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t v2 1/6] tests/core_hotunplug: properly finish processes using audio devices

[Mauro Carvalho Chehab]

On Thu, 5 May 2022 17:54:09 +0200
Andi Shyti <andi.shyti@linux.intel.com> wrote:

> Hi Mauro,
> 
> [...]
> 
> > +static void pulseaudio_unload_module(proc_t *proc_info)
> > +{
> > +	char xdg_dir[PATH_MAX];
> > +	const char *homedir;
> > +	struct passwd *pw;
> > +
> > +	igt_fork(child, 1) {
> > +		pw = getpwuid(proc_info->euid);
> > +		homedir = pw->pw_dir;
> > +		snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid);
> > +
> > +		igt_info("Ask pulseaudio to stop using audio device\n");
> > +
> > +		setgid(proc_info->egid);
> > +		setuid(proc_info->euid);
> > +		clearenv();
> > +		setenv("HOME", homedir, 1);
> > +		setenv("XDG_RUNTIME_DIR",xdg_dir, 1);
> > +
> > +		exit(system("for i in $(pacmd list-sources|grep module:|cut -d : -f 2); do pactl unload-module $i; done"));  
> 
> you shouldn't need for the exit()

Ok.

> 
> > +	}  
> 
> do we need for igt_waitchildren() at the end?

Yes.

> 
> > +}
> > +
> > +static int
> > +__igt_lsof_kill_proc(proc_t *proc_info, char *proc_path)
> > +{
> > +	const char *audio_dev = "/dev/snd/";
> > +	char path[PATH_MAX * 2];
> > +	struct dirent *d;
> > +	struct stat st;
> > +	char *fd_lnk;
> > +	int fail = 0;
> > +	ssize_t read;  
> 
> nitpick: shall we move some of the variables above inside the
> while loop?

I actually like more to place the vars at the beginning, in reverse
Christmas tree order ;-)

All the above vars belong to the loop (except for fail). 

> 
> > +	DIR *dp = opendir(proc_path);
> > +	igt_assert(dp);
> > +
> > +	while ((d = readdir(dp))) {
> > +		if (*d->d_name == '.')
> > +			continue;  
> 
> do we need to consider ".." as well? "by-path"?

Look it better: it is actually picking both "." and "..", as it is
actually ignoring everything that starts with a dot ;-)

> 
> > +		memset(path, 0, sizeof(path));
> > +		snprintf(path, sizeof(path), "%s/%s", proc_path, d->d_name);
> > +
> > +		if (lstat(path, &st) == -1)
> > +			continue;  
> 
> should this be treated as an unlikely error?

Good point. This is what __igt_lsof_fds(). I just copied it without
any changes, as an error here would break the next line:

> > +		fd_lnk = malloc(st.st_size + 1);

Yet, if the Kernel is producing badly formed procfs entries, I
bet the system will be unreliable enough to compete booting ;-)

> > +
> > +		igt_assert((read = readlink(path, fd_lnk, st.st_size + 1)));
> > +		fd_lnk[read] = '\0';  
> 
> are we sure that in /dev/snd we have only links? In my PC I have
> only character interfaces.

No. This is not looking inside /dev/snd... the logic is actually reading
the contents of /proc/<pid>/fd. This is what is shown here on Fedora 35
for alsactl, for instance:

	$ tree /proc/1052/fd
	/proc/1052/fd
	├── 0 -> /dev/null
	├── 1 -> socket:[29993]
	├── 2 -> socket:[29993]
	├── 3 -> socket:[29997]
	└── 4 -> /dev/snd/controlC0

Everything there are symlinks.

> > +		if (strncmp(audio_dev, fd_lnk, strlen(audio_dev))) {
> > +			free(fd_lnk);
> > +			continue;
> > +		}
> > +
> > +		free(fd_lnk);
> > +
> > +		/*
> > +		 * In order to avoid racing against pa/systemd, ensure that
> > +		 * pulseaudio will close all audio files. This should be
> > +		 * enough to unbind audio modules and won't case race issues  
> 
> /case/cause/
> 
> > +		 * with systemd trying to reload it.
> > +		 */
> > +		if (!strcmp(proc_info->cmd, "pulseaudio")) {
> > +			pulseaudio_unload_module(proc_info);
> > +			break;
> > +		}
> > +
> > +		/*
> > +		 * FIXME: terminating pipewire-pulse is not that easy, as
> > +		 * pipewire there's no standard way up to pipewire version
> > +		 * 0.3.49. Just trying to kill pipewire will start a race
> > +		 * between IGT and systemd. If IGT wins, the audio driver
> > +		 * will be unloaded before systemd tries to reload it, but
> > +		 * if systemd wins, the audio device will be re-opened and
> > +		 * used before IGT has a chance to remove the audio driver.
> > +		 * Pipewire version 0.3.50 should bring a standard way:
> > +		 *
> > +		 * 1) start a thread running:
> > +		 *	 pw-reserve -n Audio0 -r
> > +		 * 2) unload/unbind the the audio driver(s);
> > +		 * 3) stop the pw-reserve thread.
> > +		 *
> > +		 * We should add support for it once distros start shipping it.
> > +		 */  
> 
> thanks for the explanation!
> 
> > +		/* For all other processes, just kill them */
> > +		igt_info("process %d (%s) is using audio device. Should be terminated.\n",
> > +				proc_info->tid, proc_info->cmd);
> > +
> > +		if (kill(proc_info->tid, SIGTERM) < 0) {
> > +			igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n",
> > +				proc_info->cmd, proc_info->tid);
> > +			if (kill(proc_info->tid, SIGABRT) < 0) {
> > +				fail++;
> > +				igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n",
> > +					proc_info->cmd, proc_info->tid);
> > +			}
> > +		}
> > +
> > +		break;
> > +	}
> > +
> > +	closedir(dp);
> > +	return fail;
> > +}
> > +
> > +/*
> > + * This function identifies each process running on the machine that is
> > + * opening an audio device and tries to stop it.
> > + *
> > + * Special care should be taken with pipewire and pipewire-pulse, as those
> > + * daemons are respanned if they got killed.
> > + */
> > +int
> > +igt_lsof_kill_audio_processes(void)
> > +{
> > +	char path[PATH_MAX];
> > +	proc_t *proc_info;
> > +	PROCTAB *proc;
> > +	int fail = 0;
> > +
> > +	proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG);  
> 
> you could eventually make an igt_openproc() with the assert as
> this is a pattern that it's repeated several times. Up to you.

Ok.

> 
> > +	igt_assert(proc != NULL);
> > +
> > +	while ((proc_info = readproc(proc, NULL))) {
> > +		if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1) {
> > +			fail++;  
> 
> we are counting failures but using it more as a boolean. Is there
> any use of fail?

Currently no. It might be interesting to print the number of failures
in case something bad happens there. That's why I opted to use
a number.
> 
> > +		} else {
> > +			fail += __igt_lsof_kill_proc(proc_info, path);
> > +		}  
> 
> brackets not needed here

Will drop. I had some printfs in the middle during my tests. Forgot
removing the brackets afterwards ;-)

> > +		freeproc(proc_info);
> > +	}
> > +	closeproc(proc);
> > +
> > +	return fail;
> > +}
> > +
> >  static struct igt_siglatency {
> >  	timer_t timer;
> >  	struct timespec target;
> > diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> > index 9f2588aeca90..bb96d1afb777 100644
> > --- a/lib/igt_aux.h
> > +++ b/lib/igt_aux.h
> > @@ -306,6 +306,7 @@ bool igt_allow_unlimited_files(void);
> >  int igt_is_process_running(const char *comm);
> >  int igt_terminate_process(int sig, const char *comm);
> >  void igt_lsof(const char *dpath);
> > +int igt_lsof_kill_audio_processes(void);
> >  
> >  #define igt_hweight(x) \
> >  	__builtin_choose_expr(sizeof(x) == 8, \
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index f252535d5a3a..87a59245f699 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -389,7 +389,7 @@ igt_i915_driver_load(const char *opts)
> >  	return 0;
> >  }
> >  
> > -int __igt_i915_driver_unload(const char **who)
> > +int igt_audio_driver_unload(const char **who)
> >  {
> >  	int ret;
> >  	const char *sound[] = {
> > @@ -398,6 +398,27 @@ int __igt_i915_driver_unload(const char **who)
> >  		NULL,
> >  	};
> >  
> > +	for (const char **m = sound; *m; m++) {
> > +		if (igt_kmod_is_loaded(*m)) {  
> 
> little insignificant nit: with
> 
> 	if (!igt_kmod_is_loaded(*m))
> 		continue;
> 
> you save one level of indentation.
> 
> > +			if (igt_lsof_kill_audio_processes())
> > +				return 1;  
> 
> mmhhh... I don't like the return 1 because makes this function a
> hybrid boolean/error function and we can't rely anyomore on the
> return value. Please identify a proper errno to return.


> 
> > +
> > +			kick_snd_hda_intel();
> > +			ret = igt_kmod_unload(*m, 0);
> > +			if (ret) {
> > +				if (who)
> > +					*who = *m;
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +	return 0;
> > +}  
> 
> [...]
> 
> > @@ -152,19 +152,14 @@ static void driver_unbind(struct hotunplug *priv, const char *prefix,
> >  	 * safest and easiest way out.
> >  	 */
> >  	if (priv->snd_unload) {
> > -		igt_terminate_process(SIGTERM, "alsactl");
> > -
> > -		/* unbind snd_hda_intel */
> > -		kick_snd_hda_intel();
> > -
> > -		if (igt_kmod_unload("snd_hda_intel", 0)) {
> > +		if (igt_audio_driver_unload(NULL)) {  
> 
> why give NULL insteadd of a proper string? We can provide a
> better log.

As you noticed, I opted to do this on another patch, in order to
properly address some const char* warnings.

> 
> >  			priv->snd_unload = false;
> > -			igt_warn("Could not unload snd_hda_intel\n");
> > +			igt_warn("Could not unload audio driver\n");
> >  			igt_kmod_list_loaded();
> >  			igt_lsof("/dev/snd");
> >  			igt_skip("Audio is in use, skipping\n");
> >  		} else {
> > -			igt_info("Preventively unloaded snd_hda_intel\n");
> > +			igt_info("Preventively unloaded audio driver\n");
> >  		}
> >  	}  
> 
> Andi

Re: [igt-dev] [PATCH i-g-t v2 3/6] lib/igt_kmod: improve audio unbind logic

[Mauro Carvalho Chehab]

On Thu, 5 May 2022 18:44:43 +0200
Andi Shyti <andi.shyti@linux.intel.com> wrote:

> Hi Mauro,
> 
> > +static struct module_ref *read_module_dependencies(unsigned int *num_mod)
> > +{
> > +	FILE *fp = fopen("/proc/modules", "r");
> > +	struct module_ref *mod = NULL;
> > +	size_t line_buf_size = 0;
> > +	char *required_by, *p;
> > +	unsigned n_mod = 0;
> > +	unsigned num_req;
> > +	char *line = NULL;
> > +	int len = 0;
> > +	int i, ret;
> > +
> > +	igt_assert(fp);
> > +
> > +	while ((len = getline(&line, &line_buf_size, fp)) > 0) {
> > +		mod = realloc(mod, (n_mod + 1) * sizeof(*mod));
> > +		igt_assert(mod);
> > +		mod[n_mod].required_by = NULL;
> > +
> > +		p = strtok(line, " ");
> > +		mod[n_mod].name = strdup(p);
> > +
> > +		p = strtok(NULL, " ");
> > +		ret = sscanf(p, "%lu", &mod[n_mod].mem);
> > +		igt_assert(ret == 1);
> > +
> > +		p = strtok(NULL, " ");
> > +		ret = sscanf(p, "%u", &mod[n_mod].ref_count);
> > +		igt_assert(ret == 1);
> > +
> > +		num_req = 0;
> > +		required_by = strtok(NULL, " ");
> > +		if (strcmp(required_by, "-")) {
> > +			p = strtok(required_by, ",");
> > +			while (p) {
> > +				for (i = 0; i < n_mod; i++) {
> > +					if (!strcmp(p, mod[i].name))
> > +						break;
> > +				}  
> 
> brackets not needed here
> 
> > +				igt_assert(i < n_mod);
> > +
> > +				mod[n_mod].required_by = realloc(mod[n_mod].required_by,
> > +								 (num_req + 1) * sizeof(unsigned int));
> > +				mod[n_mod].required_by[num_req] = i;
> > +				num_req++;
> > +				p = strtok(NULL, ",");
> > +			}
> > +		}
> > +		mod[n_mod].num_required = num_req;
> > +		n_mod++;
> > +	}
> > +	free(line);
> > +	fclose(fp);
> > +
> > +	*num_mod = n_mod;
> > +	return mod;
> > +}  
> 
> it looks correct, haven't spotted any error in this function, but
> I admit that string parsing can be complex. I trust you have
> tested it properly. I will check it again, though.

Yeah, this was tested.

Basically, /proc/modules produce:

	i2c_hid_acpi 16384 0 - Live 0x0000000000000000
	typec 65536 1 typec_ucsi, Live 0x0000000000000000
	wmi 32768 3 hp_wmi,wmi_bmof,intel_wmi_thunderbolt, Live 0x0000000000000000


So, the third parameter (module dependencies) is either "-" or a
comma-separated list mod modules, ending with a comma.

I opted to use strtok here to get the 4 parameters:

	- module name;
	- module size (unused, but need to parse it anyway);
	- refcount;
	- module dependencies.

As strtok(NULL, delimiter) continues the search, without requiring
any extra logic. 

Then, I use strtok() again with "," to handle module dependencies
at the 4th parameter, if the parameter is not equal to "-" (meaning
no dependencies).

Btw, to test it, I applied my Kernel patches and commented the
Kernel version fallback check.

> 
> [...]
> 
> > +#define LINUX_VERSION(major, minor, patch)			\
> > +		     ((major) << 16 | (minor) << 8 | (patch))  
> 
> I can't believe that the glibc is not providing anything similar.

Fedora has a macro like that (KERNEL_VERSION) under:
	/usr/include/linux/version.h

but I'm not 100% sure if this would exist on all distros. The
Fedora one has things like:

	#define RHEL_MAJOR 9
	#define RHEL_MINOR 99

So, IMO the best is to just define it.

> 
> > +
> > +  
> 
> you have an extra line here

Ok, will drop.

> 
> > +static int linux_kernel_version(void)
> > +{
> > +	struct utsname utsname;
> > +	int ver[3] = { 0 };  
> 
> [...]
> 
> > +
> > +int igt_audio_driver_unload(const char **who)  
> 
> ha! this confused me because I start reading from the bottom and
> I didn't notice the above
> 
>   -int igt_audio_driver_unload(const char **who)
>   +static int igt_always_unload_audio_driver(const char **who)
> 
> > +{
> > +	unsigned int num_mod, i, j;
> > +	struct module_ref *mod;
> > +	int pos = -1;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * On older Kernels, there's no way to check if the audio driver
> > +	 * binds into the DRM one. So, always remove audio drivers that
> > +	 * might be binding.
> > +	 */
> > +	if (linux_kernel_version() < LINUX_VERSION(5, 20, 0))
> > +		return igt_always_unload_audio_driver(who);  
> 
> ... so that this one becomes what you did in patch 1.

Yes. On current Kernels, there's no way to know if snd_hda_intel
is using i915. So, it will use a somewhat hacky logic there,
that will always unload snd_hda_intel, even if not needed, and
will even ignore recursive dependencies (if any).

This should hopefully work fine with current hardware tested
on our CI.

Once the Kernel patches reach upstream, IGT will use the newer
way, which is independent on the audio module names and will
handle more complex cases where the removal of a `snd_driver_foo`
driver would require some other Kernel drivers first.

This should work fine with the newer approach where SOF/SOC are
being used for newer CPUs, and could require more complex unload
logic.

> > +	/*
> > +	 * Newer Kernels gained support for showing the dependencies between
> > +	 * audio and DRM drivers via /proc/modules and lsmod. Use it to
> > +	 * detect if removing the audio driver is needed, properly unloading
> > +	 * any audio processes that could be using the audio driver and
> > +	 * handling module dependencies. Such solution should be generic
> > +	 * enough to work with newer SOC/SOF drivers if ever needed.
> > +	 */
> > +
> > +	mod = read_module_dependencies(&num_mod);
> > +
> > +	for (i = 0; i < num_mod; i++) {
> > +		if (!strcmp(mod[i].name, "i915")) {
> > +			break;
> > +		}
> > +	}  
> 
> no need for brackets here.

I'll drop.

> 
> > +
> > +	if (i == num_mod)
> > +		return 0;
> > +
> > +	/* Recursively remove all drivers that depend on i915 */
> > +	for (j = 0; j < mod[i].num_required; j++) {
> > +		pos = mod[i].required_by[j];
> > +		if (who)
> > +			*who = strdup(mod[pos].name);
> > +		/*
> > +		 * If a sound driver depends on i915, kill audio processes
> > +		 * first, in order to make it possible to unload the driver
> > +		 */
> > +		if (strstr(mod[pos].name, "snd")) {
> > +			if (igt_lsof_kill_audio_processes()) {
> > +				ret = 1;  
> 
> is this a boolean function? Shall we choose some proper errnos?
> 
> > +				goto ret;  
> 
> do we need to free who here...

The structs that handle the module references should be freed.

> 
> > +			}
> > +		}
> > +		ret = igt_unload_driver(mod, num_mod, pos);  
> 
> ... and here?
> 
> And here... do we need to handle the error?

yeah, a goto is missing here - and also at the trivial case where there's no
DRM driver that might have snd holders are loaded.

> 
> > +	}
> > +
> > +ret:
> > +	free_module_ref(mod, num_mod);
> > +
> > +	return ret;
> > +}  
> 
> Thanks,
> Andi