On Sun, 2023-03-05 at 06:22 -0500, Bryan Steele wrote:
> On Sun, Mar 05, 2023 at 08:53:00AM +0100, Alexandre Ratchov wrote:
> > If you've an azalia(4) attaching as "AMD 17h/1xh HD Audio", please
> > test this diff and report regressions. Especially audio lock ups that
> > require reboot.
> > 
> > IIRC, MSI was disabled few years ago to "fix" such lockups, and now
> > this diff suggests we need MSI on certain boards.
> > 
> > Context and diff below:
> > 
> > 
> 
> No, this workaround is still needed. thfr@ and I tried to debug this years
> ago but could not determine the cause at the time.
> 
> This audio hang is still there on many systems, e.g: playback works for a
> breif time until the it hangs, and only a reboot will fix it. But we could
> never reproduce it with MSI disabled, so that was the best option we had.
> 
> -Bryan.
> 
This is a preliminary diff that passes my normal failures, but I haven't
run for an elaborate amount of time. The problem is that the INTSTS can
be set again by the controller without generating a new interrupt while
we're still in the interrupt handler. Somehow I only found the Linux
equivalent[0][1] when searching for my solution. The linux discussion
also states that it was discovered on "some intel platforms", which
matches my earlier statement about it also happening on my intel based
laptop, but it mostly goes unnoticed there. So far I haven't properly
tested this diff on my laptop since it occurs far and few in between.

I don't know if this behaviour is timing-based, related to the
interrupt mask, or maybe something else, so I do realise that this
method of fixing things is a bit racy. However it is my understanding of
what Linux does and does appear to fix my problems. The 10 retries is
taken from Linux, but I haven't seen my card needing more than 1 extra
iteration so far.

I removed the MSI disable statement entirely so that people with other
cards with the same symptoms can test it.

I'll keep my computer running for some time to see if I can eventually
hit the hang, but any feedback/tests is welcome.

martijn@

[0] https://lore.kernel.org/alsa-devel/1456126790-13480-1-git-send-email-libin.yang@linux.intel.com/
[1] linux/sound/pci/hda/hda_controller.c:azx_interrupt()

Index: azalia.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.286
diff -u -p -r1.286 azalia.c
--- azalia.c	6 Mar 2024 00:11:25 -0000	1.286
+++ azalia.c	16 May 2024 15:19:11 -0000
@@ -556,16 +556,6 @@ azalia_pci_attach(struct device *parent,
 		azalia_pci_write(sc->pc, sc->tag, ICH_PCI_MMC, reg);
 	}
 
-	/* disable MSI for AMD Summit Ridge/Raven Ridge HD Audio */
-	if (PCI_VENDOR(sc->pciid) == PCI_VENDOR_AMD) {
-		switch (PCI_PRODUCT(sc->pciid)) {
-		case PCI_PRODUCT_AMD_17_HDA:
-		case PCI_PRODUCT_AMD_17_1X_HDA:
-		case PCI_PRODUCT_AMD_HUDSON2_HDA:
-			pa->pa_flags &= ~PCI_FLAGS_MSI_ENABLED;
-		}
-	}
-
 	/* interrupt */
 	if (pci_intr_map_msi(pa, &ih) && pci_intr_map(pa, &ih)) {
 		printf(": can't map interrupt\n");
@@ -708,33 +698,38 @@ azalia_intr(void *v)
 {
 	azalia_t *az = v;
 	uint32_t intsts;
-	int ret = 0;
+	int ret = 0, retry;
 
 	mtx_enter(&audio_lock);
-	intsts = AZ_READ_4(az, INTSTS);
-	if (intsts == 0 || intsts == 0xffffffff) {
-		mtx_leave(&audio_lock);
-		return (ret);
-	}
-
-	AZ_WRITE_4(az, INTSTS, intsts);
-
-	if (intsts & az->pstream.intr_bit) {
-		azalia_stream_intr(&az->pstream);
-		ret = 1;
-	}
+	/*
+	 * INTSTS can get set during handling without sending a new interrupt.
+	 */
+	for (retry = 10; retry > 0; retry--) {
+		intsts = AZ_READ_4(az, INTSTS);
+		if (intsts == 0 || intsts == 0xffffffff)
+			goto done;
+
+		AZ_WRITE_4(az, INTSTS, intsts);
+
+		if (intsts & az->pstream.intr_bit) {
+			azalia_stream_intr(&az->pstream);
+			ret = 1;
+		}
 
-	if (intsts & az->rstream.intr_bit) {
-		azalia_stream_intr(&az->rstream);
-		ret = 1;
-	}
+		if (intsts & az->rstream.intr_bit) {
+			azalia_stream_intr(&az->rstream);
+			ret = 1;
+		}
 
-	if ((intsts & HDA_INTSTS_CIS) &&
-	    (AZ_READ_1(az, RIRBCTL) & HDA_RIRBCTL_RINTCTL) &&
-	    (AZ_READ_1(az, RIRBSTS) & HDA_RIRBSTS_RINTFL)) {
-		azalia_rirb_intr(az);
-		ret = 1;
+		if ((intsts & HDA_INTSTS_CIS) &&
+		    (AZ_READ_1(az, RIRBCTL) & HDA_RIRBCTL_RINTCTL) &&
+		    (AZ_READ_1(az, RIRBSTS) & HDA_RIRBSTS_RINTFL)) {
+			azalia_rirb_intr(az);
+			ret = 1;
+		}
 	}
+	printf("%s: Failed to clear INTSTS\n", XNAME(az));
+ done:
 	mtx_leave(&audio_lock);
 	return (ret);
 }