From: Martijn van Duren Subject: Re: azalia(4) testers wanted! To: tech@openbsd.org Cc: Alexandre Ratchov , Bryan Steele , thfr@openbsd.org, Andreas Bartelt Date: Thu, 16 May 2024 17:19:41 +0200 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); }