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

[igt-dev] [PATCH i-g-t v3 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.

---

v3:
 - patch 6 was improved to better handle pipewire-pulse case.
   Also added on its log the tests made on a RKL-S machine with
   Fedora 35 and wireplumber installed.
 - Minor changes on patches, addressing Andi's review from v2.

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          | 282 +++++++++++++++++++++++++++++++++++++++++
 lib/igt_aux.h          |   3 +
 lib/igt_core.c         |   7 +
 lib/igt_core.h         |   1 +
 lib/igt_kmod.c         | 279 +++++++++++++++++++++++++++++++++++++---
 lib/igt_kmod.h         |   2 +
 tests/core_hotunplug.c |  55 +++-----
 7 files changed, 573 insertions(+), 56 deletions(-)

-- 
2.35.1

[igt-dev] [PATCH i-g-t v3 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          | 157 +++++++++++++++++++++++++++++++++++++++++
 lib/igt_aux.h          |   1 +
 lib/igt_kmod.c         |  53 ++++++++------
 lib/igt_kmod.h         |   2 +
 tests/core_hotunplug.c |  11 +--
 5 files changed, 196 insertions(+), 28 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 03cc38c93e5c..f56b5a06f100 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,162 @@ 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);
+
+		system("for i in $(pacmd list-sources|grep module:|cut -d : -f 2); do pactl unload-module $i; done");
+	}
+	igt_waitchildren();
+}
+
+/**
+ * __igt_lsof_audio_and_kill_proc() - check if a given process is using an
+ *	audio device. If so, stop or prevent them to use such devices.
+ *
+ * @proc_info: process struct, as returned by readproc()
+ * @proc_path: path of the process under procfs
+ *
+ * No processes can be using an audio device by the time it gets removed.
+ * This function checks if a process is using an audio device from /dev/snd.
+ * If so, it will check:
+ * 	- if the process is pulseaudio, it can't be killed, as systemd will
+ * 	  respawn it. So, instead, send a request for it to stop bind the
+ * 	  audio devices.
+ * If the check fails, it means that the process can simply be killed.
+ */
+static int
+__igt_lsof_audio_and_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 cause 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_audio_and_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..133d19048a9b 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 EACCES;
+
+			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,27 +432,19 @@ 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)) {
-			ret = igt_kmod_unload(*m, 0);
-			if (ret) {
-				if (who)
-					*who = *m;
-				return ret;
-			}
+		if (!igt_kmod_is_loaded(*m))
+			continue;
+
+		ret = igt_kmod_unload(*m, 0);
+		if (ret) {
+			if (who)
+				*who = *m;
+			return ret;
 		}
 	}
 
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 v3 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.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
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 f56b5a06f100..0d90ebb5b6ec 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 133d19048a9b..d282be70f102 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 EACCES;
 
 			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 v3 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 d282be70f102..ca74c602ce66 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)
+{
+	const char *drm_driver = "i915";
+	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, drm_driver))
+			break;
+
+	if (i == num_mod) {
+		ret = 0;
+		goto ret;
+	}
+
+	/* Recursively remove all drivers that depend on 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 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()) {
+				ret = EACCES;
+				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 v3 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.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
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 v3 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 the dependency
between the audio and DRM drivers, skip the core hotunplug
test if it fails to unload the audio driver, as this 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         | 33 +++++++++++++++++++++++++++++----
 tests/core_hotunplug.c |  7 ++-----
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index ca74c602ce66..5a1d1658b367 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -399,17 +399,35 @@ 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 EACCES;
+
+			ret = igt_lsof_kill_audio_processes();
+
+			if (ret) {
+				igt_warn("Could not stop %d audio process(es)\n", ret);
+				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;
@@ -599,6 +617,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 v3 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.

Tested on RKL-S with Fedora 35 and wireplumber:

	./build/tests/core_hotunplug --run unbind-rebind
	IGT-Version: 1.26-gf8b43420 (x86_64) (Linux: 5.18.0-rc4-drm-11b90fad0e81+ x86_64)
	Starting subtest: unbind-rebind
	process 699 (alsactl) is using audio device. Should be terminated.
	process 3557 (pipewire) is using audio device. Should be terminated.
	process 4491 (pipewire-pulse) is using audio device. Should be requested to stop using them.
	Preventing pipewire-pulse to use the audio drivers
	Device Audio0 can not be acquired: Success
	reserve acquired
	Timed out waiting for children
	Unloaded audio driver snd_hda_intel
	Realoading snd_hda_intel
	Subtest unbind-rebind: SUCCESS (2.478s)

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

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 0d90ebb5b6ec..b508f7ab7533 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -1435,12 +1435,114 @@ static void pulseaudio_unload_module(proc_t *proc_info)
 	igt_waitchildren();
 }
 
+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);
+}
+
 /**
  * __igt_lsof_audio_and_kill_proc() - check if a given process is using an
  *	audio device. If so, stop or prevent them to use such devices.
  *
  * @proc_info: process struct, as returned by readproc()
  * @proc_path: path of the process under procfs
+ * @pipewire_pulse_pid: PID of pipewire-pulse process
  *
  * No processes can be using an audio device by the time it gets removed.
  * This function checks if a process is using an audio device from /dev/snd.
@@ -1448,10 +1550,15 @@ static void pulseaudio_unload_module(proc_t *proc_info)
  * 	- if the process is pulseaudio, it can't be killed, as systemd will
  * 	  respawn it. So, instead, send a request for it to stop bind the
  * 	  audio devices.
+ *	- if the process is pipewire-pulse, it can't be killed, as systemd will
+ *	  respawn it. So, instead, the caller should call pw-reserve, remove
+ *	  the kernel driver and then stop pw-reserve. On such case, this
+ *	  function returns the PID of pipewire-pulse, but won't touch it.
  * If the check fails, it means that the process can simply be killed.
  */
 static int
-__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
+__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path,
+			       int *pipewire_pulse_pid)
 {
 	const char *audio_dev = "/dev/snd/";
 	char path[PATH_MAX * 2];
@@ -1460,8 +1567,43 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
 	char *fd_lnk;
 	int fail = 0;
 	ssize_t read;
+	DIR *dp;
 
-	DIR *dp = opendir(proc_path);
+	/*
+	 * 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 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.
+	 */
+	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;
+		return 0;
+	}
+	/*
+	 * pipewire-pulse itself doesn't hook into a /dev/snd device. Instead,
+	 * the actual binding happens at the Pipewire Session Manager, e.g.
+	 * either wireplumber or pipewire media-session.
+	 *
+	 * Just killing such processes won't produce any effect, as systemd
+	 * will respawn them. So, just ignore here, they'll honor pw-reserve,
+	 * when the time comes.
+	 */
+	if (!strcmp(proc_info->cmd, "pipewire-media-session"))
+		return 0;
+	if (!strcmp(proc_info->cmd, "wireplumber"))
+		return 0;
+
+	dp = opendir(proc_path);
 	igt_assert(dp);
 
 	while ((d = readdir(dp))) {
@@ -1497,24 +1639,6 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
 			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);
@@ -1544,7 +1668,7 @@ __igt_lsof_audio_and_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;
@@ -1553,12 +1677,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_audio_and_kill_proc(proc_info, path);
+			fail += __igt_lsof_audio_and_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 5a1d1658b367..5d5a23cad837 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",
@@ -411,8 +412,7 @@ static int igt_always_unload_audio_driver(const char **who)
 			if (who)
 				*who = *m;
 
-			ret = igt_lsof_kill_audio_processes();
-
+			ret = igt_lsof_kill_audio_processes(&pipewire_pulse_pid);
 			if (ret) {
 				igt_warn("Could not stop %d audio process(es)\n", ret);
 				igt_kmod_list_loaded();
@@ -420,8 +420,12 @@ static int igt_always_unload_audio_driver(const char **who)
 				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();
@@ -566,6 +570,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;
@@ -608,12 +613,19 @@ int igt_audio_driver_unload(const char **who)
 		 * first, in order to make it possible to unload the driver
 		 */
 		if (strstr(mod[pos].name, "snd")) {
-			if (igt_lsof_kill_audio_processes()) {
+			if (igt_lsof_kill_audio_processes(&pipewire_pulse_pid)) {
 				ret = EACCES;
 				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: success for Improve logic to work with audio dependency on DRM driver

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from IGT_6468 -> IGTPW_7052
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (42 -> 43)
------------------------------

  Additional (3): bat-dg2-8 bat-dg2-9 bat-jsl-1 
  Missing    (2): fi-bxt-dsi fi-bsw-cyan 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-snb-2600:        NOTRUN -> [SKIP][1] ([fdo#109271] / [fdo#111827])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/fi-snb-2600/igt@kms_chamelium@common-hpd-after-suspend.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [INCOMPLETE][2] ([i915#3921]) -> [PASS][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6468/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#5171]: https://gitlab.freedesktop.org/drm/intel/issues/5171
  [i915#5174]: https://gitlab.freedesktop.org/drm/intel/issues/5174
  [i915#5181]: https://gitlab.freedesktop.org/drm/intel/issues/5181
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5602]: https://gitlab.freedesktop.org/drm/intel/issues/5602
  [i915#5606]: https://gitlab.freedesktop.org/drm/intel/issues/5606
  [i915#5616]: https://gitlab.freedesktop.org/drm/intel/issues/5616
  [i915#5703]: https://gitlab.freedesktop.org/drm/intel/issues/5703
  [i915#5775]: https://gitlab.freedesktop.org/drm/intel/issues/5775
  [i915#5917]: https://gitlab.freedesktop.org/drm/intel/issues/5917


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

  * CI: CI-20190529 -> None
  * IGT: IGT_6468 -> IGTPW_7052

  CI-20190529: 20190529
  CI_DRM_11616: 65a5fe9ac96c60bd6dfcc44a0bb8d584912ea53d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_7052: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/index.html
  IGT_6468: cffa5fffe9acddf49565b4caeeb5e3355ff2ea44 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

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

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

[igt-dev] ✓ Fi.CI.IGT: success for Improve logic to work with audio dependency on DRM driver

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from IGT_6468_full -> IGTPW_7052_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (9 -> 8)
------------------------------

  Missing    (1): shard-rkl 

New tests
---------

  New tests have been introduced between IGT_6468_full and IGTPW_7052_full:

### New IGT tests (2) ###

  * igt@kms_plane_scaling@upscale-with-modifier-20x20@pipe-b-hdmi-a-1-upscale-with-modifier:
    - Statuses : 1 pass(s)
    - Exec time: [0.21] s

  * igt@kms_plane_scaling@upscale-with-modifier-20x20@pipe-d-hdmi-a-1-upscale-with-modifier:
    - Statuses : 1 pass(s)
    - Exec time: [0.24] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_flush@basic-batch-kernel-default-uc:
    - shard-snb:          [PASS][1] -> [SKIP][2] ([fdo#109271]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6468/shard-snb5/igt@gem_exec_flush@basic-batch-kernel-default-uc.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-snb6/igt@gem_exec_flush@basic-batch-kernel-default-uc.html

  * igt@i915_hangman@engine-engine-hang:
    - shard-snb:          NOTRUN -> [SKIP][3] ([fdo#109271]) +99 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-snb7/igt@i915_hangman@engine-engine-hang.html

  * igt@kms_chamelium@hdmi-hpd-enable-disable-mode:
    - shard-snb:          NOTRUN -> [SKIP][4] ([fdo#109271] / [fdo#111827]) +3 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-snb6/igt@kms_chamelium@hdmi-hpd-enable-disable-mode.html

  * igt@syncobj_timeline@transfer-timeline-point:
    - shard-snb:          NOTRUN -> [DMESG-FAIL][5] ([i915#5098])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-snb5/igt@syncobj_timeline@transfer-timeline-point.html

  
#### Possible fixes ####

  * igt@gem_eio@in-flight-contexts-10ms:
    - shard-snb:          [FAIL][6] ([i915#4409]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6468/shard-snb6/igt@gem_eio@in-flight-contexts-10ms.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-snb5/igt@gem_eio@in-flight-contexts-10ms.html

  * igt@gem_eio@unwedge-stress:
    - {shard-tglu}:       [TIMEOUT][8] ([i915#3063]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6468/shard-tglu-8/igt@gem_eio@unwedge-stress.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-tglu-6/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_flush@basic-uc-rw-default:
    - shard-snb:          [SKIP][10] ([fdo#109271]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6468/shard-snb6/igt@gem_exec_flush@basic-uc-rw-default.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/shard-snb2/igt@gem_exec_flush@basic-uc-rw-default.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1063]: https://gitlab.freedesktop.org/drm/intel/issues/1063
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#1257]: https://gitlab.freedesktop.org/drm/intel/issues/1257
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2530]: https://gitlab.freedesktop.org/drm/intel/issues/2530
  [i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2994]: https://gitlab.freedesktop.org/drm/intel/issues/2994
  [i915#3063]: https://gitlab.freedesktop.org/drm/intel/issues/3063
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3319]: https://gitlab.freedesktop.org/drm/intel/issues/3319
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
  [i915#3825]: https://gitlab.freedesktop.org/drm/intel/issues/3825
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3966]: https://gitlab.freedesktop.org/drm/intel/issues/3966
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
  [i915#4409]: https://gitlab.freedesktop.org/drm/intel/issues/4409
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4877]: https://gitlab.freedesktop.org/drm/intel/issues/4877
  [i915#5076]: https://gitlab.freedesktop.org/drm/intel/issues/5076
  [i915#5098]: https://gitlab.freedesktop.org/drm/intel/issues/5098
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5182]: https://gitlab.freedesktop.org/drm/intel/issues/5182
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5287]: https://gitlab.freedesktop.org/drm/intel/issues/5287
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5614]: https://gitlab.freedesktop.org/drm/intel/issues/5614
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658


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

  * CI: CI-20190529 -> None
  * IGT: IGT_6468 -> IGTPW_7052

  CI-20190529: 20190529
  CI_DRM_11616: 65a5fe9ac96c60bd6dfcc44a0bb8d584912ea53d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_7052: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7052/index.html
  IGT_6468: cffa5fffe9acddf49565b4caeeb5e3355ff2ea44 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

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

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

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

[Lucas De Marchi]

On Fri, May 06, 2022 at 01:48:24PM +0200, Mauro Carvalho Chehab wrote:
>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.

this is for running tests... I don't think it's really expected to have
pulseaudio or pipewire running. We should basically be doing
systemctl disable/mask as a configuration step.

And if we want to handle that, why don't we simply systemctl stop them
instead of having specific commands for each of them to unbind?

yeah, the fallback to simply kill process with open handles to sound
device is nice

Lucas De Marchi

>
>Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>---
> lib/igt_aux.c          | 157 +++++++++++++++++++++++++++++++++++++++++
> lib/igt_aux.h          |   1 +
> lib/igt_kmod.c         |  53 ++++++++------
> lib/igt_kmod.h         |   2 +
> tests/core_hotunplug.c |  11 +--
> 5 files changed, 196 insertions(+), 28 deletions(-)
>
>diff --git a/lib/igt_aux.c b/lib/igt_aux.c
>index 03cc38c93e5c..f56b5a06f100 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,162 @@ 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);
>+
>+		system("for i in $(pacmd list-sources|grep module:|cut -d : -f 2); do pactl unload-module $i; done");
>+	}
>+	igt_waitchildren();
>+}
>+
>+/**
>+ * __igt_lsof_audio_and_kill_proc() - check if a given process is using an
>+ *	audio device. If so, stop or prevent them to use such devices.
>+ *
>+ * @proc_info: process struct, as returned by readproc()
>+ * @proc_path: path of the process under procfs
>+ *
>+ * No processes can be using an audio device by the time it gets removed.
>+ * This function checks if a process is using an audio device from /dev/snd.
>+ * If so, it will check:
>+ * 	- if the process is pulseaudio, it can't be killed, as systemd will
>+ * 	  respawn it. So, instead, send a request for it to stop bind the
>+ * 	  audio devices.
>+ * If the check fails, it means that the process can simply be killed.
>+ */
>+static int
>+__igt_lsof_audio_and_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 cause 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_audio_and_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..133d19048a9b 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 EACCES;
>+
>+			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,27 +432,19 @@ 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)) {
>-			ret = igt_kmod_unload(*m, 0);
>-			if (ret) {
>-				if (who)
>-					*who = *m;
>-				return ret;
>-			}
>+		if (!igt_kmod_is_loaded(*m))
>+			continue;
>+
>+		ret = igt_kmod_unload(*m, 0);
>+		if (ret) {
>+			if (who)
>+				*who = *m;
>+			return ret;
> 		}
> 	}
>
>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
>

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

[Lucas De Marchi]

On Fri, May 06, 2022 at 01:48:26PM +0200, Mauro Carvalho Chehab wrote:
>+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;
>+	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))

comparing specific kernel version is not very good practice as features
are moved from one kernel to the other, backported, etc.

if at all, we should test if something is present in the kernel. If we
can't, then we either add support for that if we can enough.
For IGT we can probably simply take the approach of landing this only
when the corresponding kernel support is merged.

What we could merge today is: always unload audio drivers.

Lucas De Marchi

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

[Mauro Carvalho Chehab]

On Wed, 11 May 2022 10:03:26 -0700
Lucas De Marchi <lucas.demarchi@intel.com> wrote:

> On Fri, May 06, 2022 at 01:48:24PM +0200, Mauro Carvalho Chehab wrote:
> >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.  
> 
> this is for running tests... I don't think it's really expected to have
> pulseaudio or pipewire running. We should basically be doing
> systemctl disable/mask as a configuration step.

That pretty much depends on where this runs: pipewire-pulse is new.
Systems like Fedora and the upcoming RHEL 9 have it loaded by default. Distros
with older stacks usually came with pulseaudio, also enabled by default.

See, a special-purpose machine that was carefully prepared for CI
testing can be configured to have pulseaudio/pipewire-pulse not installed
or disabled, but if those are running, they should stop using the
audio device before removing the device driver.

> And if we want to handle that, why don't we simply systemctl stop them
> instead of having specific commands for each of them to unbind?

Because it doesn't work. On Fedora 35:

	$ sudo systemctl stop pipewire-pulse
	Failed to stop pipewire-pulse.service: Unit pipewire-pulse.service not loaded.
	$ ps ax|grep pulse
	   2337 ?        S<sl 120:47 /usr/bin/pipewire-pulse
	$ find /etc/systemd/|grep pulse
	/etc/systemd/user/sockets.target.wants/pipewire-pulse.socket

On Ubuntu 20.04:

	$ sudo systemctl stop pulseaudio
	Failed to stop pulseaudio.service: Unit pulseaudio.service not loaded.
	$ ps ax|grep pulse
	   1469 ?        S<sl   0:00 /usr/bin/pulseaudio --daemonize=no --log-target=journal
	   2061 ?        Ssl    0:00 /usr/bin/pulseaudio --daemonize=no --log-target=journal

	$ find /etc/systemd/|grep pulse
	/etc/systemd/user/default.target.wants/pulseaudio.service
	/etc/systemd/user/sockets.target.wants/pulseaudio.socket

Basically, systemctl uses this "socket" kind of service, which is
triggered by audio device detection.

The only way to stop pulseaudio to use an audio device is to call its
API and send a request for it to release the audio device. The same
is true for pipewire-pulse.

> yeah, the fallback to simply kill process with open handles to sound
> device is nice

This is not a fallback... it is to kill other processes like alsactl,
which is started as a daemon on several distributions.

> 
> Lucas De Marchi
> 
> >
> >Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> >---
> > lib/igt_aux.c          | 157 +++++++++++++++++++++++++++++++++++++++++
> > lib/igt_aux.h          |   1 +
> > lib/igt_kmod.c         |  53 ++++++++------
> > lib/igt_kmod.h         |   2 +
> > tests/core_hotunplug.c |  11 +--
> > 5 files changed, 196 insertions(+), 28 deletions(-)
> >
> >diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> >index 03cc38c93e5c..f56b5a06f100 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,162 @@ 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);
> >+
> >+		system("for i in $(pacmd list-sources|grep module:|cut -d : -f 2); do pactl unload-module $i; done");
> >+	}
> >+	igt_waitchildren();
> >+}
> >+
> >+/**
> >+ * __igt_lsof_audio_and_kill_proc() - check if a given process is using an
> >+ *	audio device. If so, stop or prevent them to use such devices.
> >+ *
> >+ * @proc_info: process struct, as returned by readproc()
> >+ * @proc_path: path of the process under procfs
> >+ *
> >+ * No processes can be using an audio device by the time it gets removed.
> >+ * This function checks if a process is using an audio device from /dev/snd.
> >+ * If so, it will check:
> >+ * 	- if the process is pulseaudio, it can't be killed, as systemd will
> >+ * 	  respawn it. So, instead, send a request for it to stop bind the
> >+ * 	  audio devices.
> >+ * If the check fails, it means that the process can simply be killed.
> >+ */
> >+static int
> >+__igt_lsof_audio_and_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 cause 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_audio_and_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..133d19048a9b 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 EACCES;
> >+
> >+			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,27 +432,19 @@ 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)) {
> >-			ret = igt_kmod_unload(*m, 0);
> >-			if (ret) {
> >-				if (who)
> >-					*who = *m;
> >-				return ret;
> >-			}
> >+		if (!igt_kmod_is_loaded(*m))
> >+			continue;
> >+
> >+		ret = igt_kmod_unload(*m, 0);
> >+		if (ret) {
> >+			if (who)
> >+				*who = *m;
> >+			return ret;
> > 		}
> > 	}
> >
> >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
> >  

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

[Mauro Carvalho Chehab]

On Wed, 11 May 2022 10:09:13 -0700
Lucas De Marchi <lucas.demarchi@intel.com> wrote:

> On Fri, May 06, 2022 at 01:48:26PM +0200, Mauro Carvalho Chehab wrote:
> >+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;
> >+	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))  
> 
> comparing specific kernel version is not very good practice as features
> are moved from one kernel to the other, backported, etc.

Yes, but, in this very specific case, I don't see much troubles, as the
logic doesn't rely on it, using the information as only a hint if lsmod
dependencies are reliable or not.

See, the Kernel patches I'm proposing actually fix a long-standing
Kernel bug, where lsmod and /proc/modules doesn't properly show 
dependencies when module refcount is incremented on non-trivial ways.

The approach I took on IGT is that, assuming that this gets merged on
5.20.0, we can trust that, on all newer kernels, lsmod will properly
show the dependencies.

Ok, older Kernels may have this backported or not, but, as there's no
reliable way to know for sure, IGT will fall back to a logic that
will always try to unload snd-hda-intel, even when this is not needed[1].

> if at all, we should test if something is present in the kernel. 

I can't see any way to verify if the fixes for such bug are present
or not on a given kernel. Ok, we might artificially increment i915 version
to indicate that, but there's not a single change on i915 driver, as the
real fix happens inside kernel/modules. 

[1] On machines where snd-hda-intel doesn't bind into i915, as
    trying to unload it could fail, this could cause issues at the
    IGT test - and even on future tests, in case of OOPSes during
    removal.

> If we
> can't, then we either add support for that if we can enough.
> For IGT we can probably simply take the approach of landing this only
> when the corresponding kernel support is merged.
> 
> What we could merge today is: always unload audio drivers.

I would prefer to have the full code there, as there are a fundamental
difference between the two cases.

See:

Case 1) snd-hda-intel doesn't bind into i915

On such case if IGT can be sure about that (e. g. after the Kernel fixes), 
IGT will simply not touch snd-hda-intel. So, if something bad happens 
during i915 unbind/unload, the IGT test should fail.

Case 2) snd-hda-intel binds into i915

In this case, if snd-hda-intel unload fails, the IGT test should be
skipped, as trying to unload/unbind/rebind i915 will fail, but not 
be due to i915 troubles. So, it won't make sense to report an IGT
error on such case.

On other words:

a) if lsmod is reliable:

   - for case 1, don't touch snd-hda-intel;
   - for case 2, failures to remove snd-hda-intel will call igt_skip();
   - failures on i915 unbind/rebind (or unload/reload) will call
     igt_error().

b) if lsmod is not reliable (buggy kernels, like current upstream),
   as IGT doesn't know if it is case 1 or 2, it will:

   - always try to unload snd-hda-intel. If it fails, just print some
     messages that will help debugging troubles;
   - always try to unload or unbind i915. Failures on i915 unbind/rebind 
     (or unload/reload) will call igt_error().

I would prefer to merge the above logic altogether in order to have the
full solution in IGT - even if we place, instead, something like:


	int igt_audio_driver_unload(const char **who)
	{
...
	/*
	 * FIXME: update this once the Kernel fixes for lsmod gets merged
	 */
	const bool is_lsmod_reliable = false;
...
	/*
	 * 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 (!is_lsmod_reliable)
		return igt_always_unload_audio_driver(who);

And then, once this gets merged upstream, replace the static var
with:

	const bool is_lsmod_reliable = linux_kernel_version() >= LINUX_VERSION(5, 20, 0)

(or something similar)

Regards,
Mauro

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

[Lucas De Marchi]

On Thu, May 12, 2022 at 08:49:50AM +0200, Mauro Carvalho Chehab wrote:
>On Wed, 11 May 2022 10:03:26 -0700
>Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>> On Fri, May 06, 2022 at 01:48:24PM +0200, Mauro Carvalho Chehab wrote:
>> >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.
>>
>> this is for running tests... I don't think it's really expected to have
>> pulseaudio or pipewire running. We should basically be doing
>> systemctl disable/mask as a configuration step.
>
>That pretty much depends on where this runs: pipewire-pulse is new.
>Systems like Fedora and the upcoming RHEL 9 have it loaded by default. Distros
>with older stacks usually came with pulseaudio, also enabled by default.
>
>See, a special-purpose machine that was carefully prepared for CI
>testing can be configured to have pulseaudio/pipewire-pulse not installed
>or disabled, but if those are running, they should stop using the
>audio device before removing the device driver.
>
>> And if we want to handle that, why don't we simply systemctl stop them
>> instead of having specific commands for each of them to unbind?
>
>Because it doesn't work. On Fedora 35:
>
>	$ sudo systemctl stop pipewire-pulse
>	Failed to stop pipewire-pulse.service: Unit pipewire-pulse.service not loaded.

it's on user session, not system.

	systemctl --user disable --now pipewire-pulse.socket
	systemctl --user disable --now pipewire-pulse.service

My worry is that we have to keep adding logic for specific components.
You'd probably already have to add support for wireplumber if you follow
this logic you will also probably have to find a way to stop wireplumber
that is the new pipewire's session manager being adopted by distros.


>	$ ps ax|grep pulse
>	   2337 ?        S<sl 120:47 /usr/bin/pipewire-pulse
>	$ find /etc/systemd/|grep pulse
>	/etc/systemd/user/sockets.target.wants/pipewire-pulse.socket
>
>On Ubuntu 20.04:
>
>	$ sudo systemctl stop pulseaudio
>	Failed to stop pulseaudio.service: Unit pulseaudio.service not loaded.
>	$ ps ax|grep pulse
>	   1469 ?        S<sl   0:00 /usr/bin/pulseaudio --daemonize=no --log-target=journal
>	   2061 ?        Ssl    0:00 /usr/bin/pulseaudio --daemonize=no --log-target=journal
>
>	$ find /etc/systemd/|grep pulse
>	/etc/systemd/user/default.target.wants/pulseaudio.service
>	/etc/systemd/user/sockets.target.wants/pulseaudio.socket

same here...         ^^^^

I don't really care much, just think not talking with each component
directly will be lower maintenance long term and fewer chance for bugs.


Lucas De Marchi

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

[Mauro Carvalho Chehab]

On Mon, 16 May 2022 00:24:44 -0700
Lucas De Marchi <lucas.demarchi@intel.com> wrote:

> On Thu, May 12, 2022 at 08:49:50AM +0200, Mauro Carvalho Chehab wrote:
> >On Wed, 11 May 2022 10:03:26 -0700
> >Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >  
> >> On Fri, May 06, 2022 at 01:48:24PM +0200, Mauro Carvalho Chehab wrote:  
> >> >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.  
> >>
> >> this is for running tests... I don't think it's really expected to have
> >> pulseaudio or pipewire running. We should basically be doing
> >> systemctl disable/mask as a configuration step.  
> >
> >That pretty much depends on where this runs: pipewire-pulse is new.
> >Systems like Fedora and the upcoming RHEL 9 have it loaded by default. Distros
> >with older stacks usually came with pulseaudio, also enabled by default.
> >
> >See, a special-purpose machine that was carefully prepared for CI
> >testing can be configured to have pulseaudio/pipewire-pulse not installed
> >or disabled, but if those are running, they should stop using the
> >audio device before removing the device driver.
> >  
> >> And if we want to handle that, why don't we simply systemctl stop them
> >> instead of having specific commands for each of them to unbind?  
> >
> >Because it doesn't work. On Fedora 35:
> >
> >	$ sudo systemctl stop pipewire-pulse
> >	Failed to stop pipewire-pulse.service: Unit pipewire-pulse.service not loaded.  
> 
> it's on user session, not system.
> 
> 	systemctl --user disable --now pipewire-pulse.socket
> 	systemctl --user disable --now pipewire-pulse.service

Yeah, true.

> 
> My worry is that we have to keep adding logic for specific components.
> You'd probably already have to add support for wireplumber if you follow
> this logic you will also probably have to find a way to stop wireplumber
> that is the new pipewire's session manager being adopted by distros.

This patch series properly covers pipewire-pulse. It doesn't need
anything specific to different types of pipewire session manager
(wireplumber or pipewire media-session), as it uses pipewire reserve 
API logic that should work with any session manager.

Btw, I tested this series on Fedora 35 with pipewire-pulse/wireplumber
started as --user, and it properly unloaded snd-hda-intel.

I'd expect that we won't need to change anything for quite a while,
as I can't foresee yet another replacement for puseaudio.


I agree with you that the best is to disable pipewire/pulseaudio
via systemctl, but in case one forgets to do it on some system, this 
logic will still make snd-hda-intel unload work.

Besides that, I had already some bad experiences in the past with
systemd issues. Better to not rely only on it to do the right thing.

-

Btw, in long term, the best would be to actually change snd-hda-audio
for it to allow removing/reinserting i915 driver without issues
(but this may require changes at ALSA core).

> 
> 
> >	$ ps ax|grep pulse
> >	   2337 ?        S<sl 120:47 /usr/bin/pipewire-pulse
> >	$ find /etc/systemd/|grep pulse
> >	/etc/systemd/user/sockets.target.wants/pipewire-pulse.socket
> >
> >On Ubuntu 20.04:
> >
> >	$ sudo systemctl stop pulseaudio
> >	Failed to stop pulseaudio.service: Unit pulseaudio.service not loaded.
> >	$ ps ax|grep pulse
> >	   1469 ?        S<sl   0:00 /usr/bin/pulseaudio --daemonize=no --log-target=journal
> >	   2061 ?        Ssl    0:00 /usr/bin/pulseaudio --daemonize=no --log-target=journal
> >
> >	$ find /etc/systemd/|grep pulse
> >	/etc/systemd/user/default.target.wants/pulseaudio.service
> >	/etc/systemd/user/sockets.target.wants/pulseaudio.socket  
> 
> same here...         ^^^^
> 
> I don't really care much, just think not talking with each component
> directly will be lower maintenance long term and fewer chance for bugs.

I actually think just the opposite: relying that all machines in the
world that would run IGT are properly customized to not run any audio
daemons is risky and has a high chance of maintenance and bugs.

IGT should always check it before trying to unload snd-hda-intel, as
any process that would keep a /dev/snd device opened will prevent the
module unload, thus may cause issues while doing i915 unbind/rebind
(or unload/reload).

Regards,
Mauro

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

[Andi Shyti]

Hi Mauro,

> +	/* Recursively remove all drivers that depend on drm_driver */
> +	for (j = 0; j < mod[i].num_required; j++) {
> +		pos = mod[i].required_by[j];
> +		if (who)
> +			*who = strdup(mod[pos].name);

how many who are we "strdupping"? Are they freed anywhere?

Andi

> +		/*
> +		 * If a sound driver depends on 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()) {
> +				ret = EACCES;
> +				goto ret;
> +			}
> +		}
> +		ret = igt_unload_driver(mod, num_mod, pos);
> +	}
> +
> +ret:
> +	free_module_ref(mod, num_mod);
> +
> +	return ret;
> +}

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

[Andi Shyti]

Hi Mauro,

On Fri, May 06, 2022 at 01:48:24PM +0200, Mauro Carvalho Chehab wrote:
> 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>

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

Andi

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

[Andi Shyti]

Hi Mauro,

> +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 exit...

> +	}

... and waitchild().

Like in patch 1 :)

> +}
> +
> +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);

why do we need the sigterm? I guess here you need to modify
kill_children so that it kills with a signal. Otherwise this
SIGTERM is useless.

>  /**
>   * __igt_lsof_audio_and_kill_proc() - check if a given process is using an
>   *	audio device. If so, stop or prevent them to use such devices.
>   *
>   * @proc_info: process struct, as returned by readproc()
>   * @proc_path: path of the process under procfs
> + * @pipewire_pulse_pid: PID of pipewire-pulse process
>   *
>   * No processes can be using an audio device by the time it gets removed.
>   * This function checks if a process is using an audio device from /dev/snd.
> @@ -1448,10 +1550,15 @@ static void pulseaudio_unload_module(proc_t *proc_info)
>   * 	- if the process is pulseaudio, it can't be killed, as systemd will
>   * 	  respawn it. So, instead, send a request for it to stop bind the
>   * 	  audio devices.
> + *	- if the process is pipewire-pulse, it can't be killed, as systemd will
> + *	  respawn it. So, instead, the caller should call pw-reserve, remove
> + *	  the kernel driver and then stop pw-reserve. On such case, this
> + *	  function returns the PID of pipewire-pulse, but won't touch it.
>   * If the check fails, it means that the process can simply be killed.
>   */
>  static int
> -__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> +__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path,
> +			       int *pipewire_pulse_pid)

Still I think that this is becoming messy. We are carrying this
pipewire_pulse_pid from function to function with a possible
value of '0'.

Anyway, I will not insist on this.

[...]

> +void igt_killchildren(int signal)
> +{
> +	igt_info("Timed out waiting for children\n");
> +
> +	kill_children();
> +}

As I said before, we don't really need for int signal. And I
think this should in a different patch as this is a change in
igt_core?

Andi

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

[Mauro Carvalho Chehab]

Hi Andi,

On Mon, 16 May 2022 14:30:58 +0200
Andi Shyti <andi.shyti@linux.intel.com> wrote:

> Hi Mauro,
> 
> > +	/* Recursively remove all drivers that depend on drm_driver */
> > +	for (j = 0; j < mod[i].num_required; j++) {
> > +		pos = mod[i].required_by[j];
> > +		if (who)
> > +			*who = strdup(mod[pos].name);  
> 
> how many who are we "strdupping"?

Currently, with i915, there's just one module that will actually be
removed here - either snd_hda_intel or snd_hdmi_lpe_audio (on Atom).

If a SOC/SOF driver would end use this interface, then maybe up to
a dozen modules might need to be unloaded, depending on codecs and
other ancillary drivers. For instance, those are the modules I have
here on a personal machine with uses sof/soc:

	$ lsmod|grep snd
	snd_seq_dummy          16384  0
	snd_hrtimer            16384  1
	snd_soc_sof_es8336     20480  0
	snd_soc_intel_hda_dsp_common    20480  1 snd_soc_sof_es8336
	snd_hda_codec_hdmi     73728  0
	snd_hda_codec_realtek   163840  0
	snd_hda_codec_generic    98304  1 snd_hda_codec_realtek
	snd_soc_dmic           16384  0
	snd_sof_pci_intel_cnl    16384  0
	snd_sof_intel_hda_common   110592  1 snd_sof_pci_intel_cnl
	soundwire_intel        45056  1 snd_sof_intel_hda_common
	snd_sof_intel_hda      20480  1 snd_sof_intel_hda_common
	snd_sof_pci            20480  2 snd_sof_intel_hda_common,snd_sof_pci_intel_cnl
	snd_sof_xtensa_dsp     16384  1 snd_sof_intel_hda_common
	snd_sof               167936  2 snd_sof_pci,snd_sof_intel_hda_common
	snd_soc_skl           176128  0
	snd_soc_hdac_hda       24576  2 snd_sof_intel_hda_common,snd_soc_skl
	snd_hda_ext_core       36864  4 snd_sof_intel_hda_common,snd_soc_hdac_hda,snd_soc_skl,snd_sof_intel_hda
	snd_soc_sst_ipc        20480  1 snd_soc_skl
	snd_soc_sst_dsp        36864  1 snd_soc_skl
	snd_soc_acpi_intel_match    61440  3 snd_sof_intel_hda_common,snd_soc_skl,snd_sof_pci_intel_cnl
	snd_soc_acpi           16384  3 snd_soc_acpi_intel_match,snd_sof_intel_hda_common,snd_soc_skl
	snd_hda_intel          57344  0
	snd_intel_dspcfg       32768  3 snd_hda_intel,snd_sof_intel_hda_common,snd_soc_skl
	snd_intel_sdw_acpi     20480  2 snd_sof_intel_hda_common,snd_intel_dspcfg
	ledtrig_audio          16384  3 snd_hda_codec_generic,huawei_wmi,snd_sof
	snd_hda_codec         172032  6 snd_hda_codec_generic,snd_hda_codec_hdmi,snd_hda_intel,snd_hda_codec_realtek,snd_soc_intel_hda_dsp_common,snd_soc_hdac_hda
	snd_hda_core          110592  11 snd_hda_codec_generic,snd_hda_codec_hdmi,snd_hda_intel,snd_hda_ext_core,snd_hda_codec,snd_hda_codec_realtek,snd_soc_intel_hda_dsp_common,snd_sof_intel_hda_common,snd_soc_hdac_hda,snd_soc_skl,snd_sof_intel_hda
	snd_soc_es8316         53248  0
	snd_hwdep              16384  1 snd_hda_codec
	snd_soc_core          348160  8 soundwire_intel,snd_sof,snd_sof_intel_hda_common,snd_soc_hdac_hda,snd_soc_sof_es8336,snd_soc_skl,snd_soc_es8316,snd_soc_dmic
	snd_compress           28672  1 snd_soc_core
	ac97_bus               16384  1 snd_soc_core
	snd_pcm_dmaengine      16384  1 snd_soc_core
	snd_seq                90112  7 snd_seq_dummy
	snd_seq_device         16384  1 snd_seq
	snd_pcm               151552  12 snd_hda_codec_hdmi,snd_hda_intel,snd_hda_codec,soundwire_intel,snd_sof,snd_sof_intel_hda_common,snd_compress,snd_soc_core,snd_soc_skl,snd_soc_es8316,snd_hda_core,snd_pcm_dmaengine
	snd_timer              49152  3 snd_seq,snd_hrtimer,snd_pcm
	snd                   114688  16 snd_hda_codec_generic,snd_seq,snd_seq_device,snd_hda_codec_hdmi,snd_hwdep,snd_hda_intel,snd_hda_codec,snd_hda_codec_realtek,snd_sof,snd_timer,snd_compress,snd_soc_core,snd_soc_sof_es8336,snd_pcm
	soundcore              16384  1 snd

but I dunno if some drivers in this chain is bound to i915.

In any case, I opted to make the logic there generic enough to cover complex cases with a module dependency chain.

> Are they freed anywhere?

Currently, they aren't freed. I can change the logic to free it, if there is
already an allocated pointer, e. g. something like:

	if (who) {
		if (*who)
			free(*who);
		*who = strdup(mod[pos].name);
	}

With that, there will be either a single one small string allocated or
a pointer to a static const char.

> > +		/*
> > +		 * If a sound driver depends on 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()) {
> > +				ret = EACCES;
> > +				goto ret;
> > +			}
> > +		}
> > +		ret = igt_unload_driver(mod, num_mod, pos);
> > +	}
> > +
> > +ret:
> > +	free_module_ref(mod, num_mod);
> > +
> > +	return ret;
> > +}  

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

[Mauro Carvalho Chehab]

On Mon, 16 May 2022 14:49:21 +0200
Andi Shyti <andi.shyti@linux.intel.com> wrote:

> Hi Mauro,
> 
> > +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 exit...
> 
> > +	}  
> 
> ... and waitchild().
> 
> Like in patch 1 :)

OK!

> 
> > +}
> > +
> > +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);  
> 
> why do we need the sigterm? I guess here you need to modify
> kill_children so that it kills with a signal. Otherwise this
> SIGTERM is useless.

I guess we can just use kill_children() here, as either SIGKILL or
SIGTERM would do the job. There's no special reason why it would
need SIGTERM. I would prefer to let pw reserve to terminate normally,
but I can't see why SIGKILL won't equally work.

I'll run some tests to be sure.

> 
> >  /**
> >   * __igt_lsof_audio_and_kill_proc() - check if a given process is using an
> >   *	audio device. If so, stop or prevent them to use such devices.
> >   *
> >   * @proc_info: process struct, as returned by readproc()
> >   * @proc_path: path of the process under procfs
> > + * @pipewire_pulse_pid: PID of pipewire-pulse process
> >   *
> >   * No processes can be using an audio device by the time it gets removed.
> >   * This function checks if a process is using an audio device from /dev/snd.
> > @@ -1448,10 +1550,15 @@ static void pulseaudio_unload_module(proc_t *proc_info)
> >   * 	- if the process is pulseaudio, it can't be killed, as systemd will
> >   * 	  respawn it. So, instead, send a request for it to stop bind the
> >   * 	  audio devices.
> > + *	- if the process is pipewire-pulse, it can't be killed, as systemd will
> > + *	  respawn it. So, instead, the caller should call pw-reserve, remove
> > + *	  the kernel driver and then stop pw-reserve. On such case, this
> > + *	  function returns the PID of pipewire-pulse, but won't touch it.
> >   * If the check fails, it means that the process can simply be killed.
> >   */
> >  static int
> > -__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path)
> > +__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path,
> > +			       int *pipewire_pulse_pid)  
> 
> Still I think that this is becoming messy. We are carrying this
> pipewire_pulse_pid from function to function with a possible
> value of '0'.

we might store pipewire_pulse_pid on some static var, but I don't like
very much to use static vars. We could also re-check the PID, but storing
it saves us a couple of CPU cycles.

> 
> Anyway, I will not insist on this.
> 
> [...]
> 
> > +void igt_killchildren(int signal)
> > +{
> > +	igt_info("Timed out waiting for children\n");
> > +
> > +	kill_children();
> > +}  
> 
> As I said before, we don't really need for int signal. And I
> think this should in a different patch as this is a change in
> igt_core?

Heh, true. I guess I had a different implementation, that degraded
into this ;-)

I'll clean it up.

Regards,
Mauro

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

[Mauro Carvalho Chehab]

On Mon, 16 May 2022 15:35:14 +0200
Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:

> On Mon, 16 May 2022 14:49:21 +0200
> Andi Shyti <andi.shyti@linux.intel.com> wrote:
> 
> > Hi Mauro,
> >   
> > > +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 exit...
> >   
> > > +	}    
> > 
> > ... and waitchild().
> > 
> > Like in patch 1 :)  
> 
> OK!

Actually, no!

See, this is what happens if it calls waitchild():

	IGT-Version: 1.26-g30915d22 (x86_64) (Linux: 5.18.0-rc7-drm-ad75b5b819c9+ x86_64)
	Starting subtest: unbind-rebind
	process 12096 (alsactl) is using audio device. Should be terminated.
	process 12801 (pipewire-pulse) is using audio device. Should be requested to stop using them.
	Preventing pipewire-pulse to use the audio drivers
	Device Audio0 can not be acquired: Success
	reserve acquired
	<wait forever, as pw-reserve will run forever>

By using exit(system()) there, everything works like a charm:

	IGT-Version: 1.26-gac8dabf6 (x86_64) (Linux: 5.18.0-rc7-drm-ad75b5b819c9+ x86_64)
	Starting subtest: unbind-rebind
	process 12801 (pipewire-pulse) is using audio device. Should be requested to stop using them.
	Preventing pipewire-pulse to use the audio drivers
	Device Audio0 can not be acquired: Success
	reserve acquired
	Unloaded audio driver snd_hda_intel
	Realoading snd_hda_intel
	Subtest unbind-rebind: SUCCESS (2.755s)


See, what we want is to run pw-reserve in backgroud, then remove
snd_hda_intel and finally kill it with SIGTERM.

Regards,
Mauro