[alsa-devel] [PATCH 0/1] ALSA: hda: add fallback to polling to hdac_bus_get_response()

[alsa-devel] [PATCH 0/1] ALSA: hda: add fallback to polling to hdac_bus_get_response()

[Kai Vehmanen]

Hey all,

while debugging issues with some Intel platforms related to display
audio codec probe (see
https://lists.freedesktop.org/archives/intel-gfx/2019-October/214621.html ),
I found a discrepancy in behaviour between snd-hda-intel and SOF, despite
using the same snd-hda-codec-hdmi as the codec driver.

The specific problem I was debugging appears in a stress test
(designed to uncover the above display driver issue) where
driver-unload, s3-suspend, resume and driver-reload is done in a loop
and repeated for hundreds of iterations. When using SOF, I would get
occasional probe fail due to a missing HDA irq. The AZX snd_hda_intel
driver nicely survives this test. The explanation seems to be differences
in the hdac get_response() implementation.

While the specific issue could be solved with other means,
the git history shows a number of rare issues with HDA codecs
where polling has helped. It would seem best to align the logic
with the AZX driver implementation that has seen much more usage
over the years. This will benefit SOF and any other users of the HDAC
library.

Kai Vehmanen (1):
  ALSA: hda: add fallback to polling to hdac_bus_get_response()

 sound/hda/hdac_controller.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 1/1] ALSA: hda: add fallback to polling to hdac_bus_get_response()

[Kai Vehmanen]

The AZX controller implementation in azx_rirb_get_response()
implements logic to fallback to polling in case interrupt is
not received from HDA codec.

Port over this same logic to the generic snd_hdac_bus_get_response()
function, which is used by other HDAC clients such as SOF.

Without this fix, failures are observed in module reload
stress tests with the SOF driver, while test passes on same
hardware with the snd_hda_intel driver. Considering
the AZX implementation has been much more widely used and
there can be exceptions with other systems (and codecs), it
is best to align the implementation and use the time-proven
logic in all drivers.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/hda/hdac_controller.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index d3999e7b0705..994c1dd2eb2e 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -238,14 +238,18 @@ int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
 {
 	unsigned long timeout;
 	unsigned long loopcounter;
+	int do_poll = 0;
 
+ again:
 	timeout = jiffies + msecs_to_jiffies(1000);
 
 	for (loopcounter = 0;; loopcounter++) {
 		spin_lock_irq(&bus->reg_lock);
-		if (bus->polling_mode)
+		if (bus->polling_mode || do_poll)
 			snd_hdac_bus_update_rirb(bus);
 		if (!bus->rirb.cmds[addr]) {
+			if (!do_poll)
+				bus->poll_count = 0;
 			if (res)
 				*res = bus->rirb.res[addr]; /* the last value */
 			spin_unlock_irq(&bus->reg_lock);
@@ -262,6 +266,23 @@ int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
 		}
 	}
 
+	if (!bus->polling_mode && bus->poll_count < 2) {
+		dev_dbg(bus->dev,
+			"response timeout, polling the codec once: last cmd=0x%08x\n",
+			bus->last_cmd[addr]);
+		do_poll = 1;
+		bus->poll_count++;
+		goto again;
+	}
+
+	if (!bus->polling_mode) {
+		dev_warn(bus->dev,
+			 "response timeout, switching to polling mode: last cmd=0x%08x\n",
+			 bus->last_cmd[addr]);
+		bus->polling_mode = 1;
+		goto again;
+	}
+
 	return -EIO;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_bus_get_response);
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 0/1] ALSA: hda: add fallback to polling to hdac_bus_get_response()

[Takashi Iwai]

On Fri, 04 Oct 2019 16:35:26 +0200,
Kai Vehmanen wrote:
> 
> Hey all,
> 
> while debugging issues with some Intel platforms related to display
> audio codec probe (see
> https://lists.freedesktop.org/archives/intel-gfx/2019-October/214621.html ),
> I found a discrepancy in behaviour between snd-hda-intel and SOF, despite
> using the same snd-hda-codec-hdmi as the codec driver.
> 
> The specific problem I was debugging appears in a stress test
> (designed to uncover the above display driver issue) where
> driver-unload, s3-suspend, resume and driver-reload is done in a loop
> and repeated for hundreds of iterations. When using SOF, I would get
> occasional probe fail due to a missing HDA irq. The AZX snd_hda_intel
> driver nicely survives this test. The explanation seems to be differences
> in the hdac get_response() implementation.
> 
> While the specific issue could be solved with other means,
> the git history shows a number of rare issues with HDA codecs
> where polling has helped. It would seem best to align the logic
> with the AZX driver implementation that has seen much more usage
> over the years. This will benefit SOF and any other users of the HDAC
> library.

While it's OK to add the polling support in the core code, I suspect
that the main problem gets solved by setting the write_sync flag as
the commit 2756d9143aa5.  For SOF/SST, you may set the flag
unconditionally since they support only the new chipsets.

I've been traveling (still for the next week), so the further reply
may be delayed.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 0/1] ALSA: hda: add fallback to polling to hdac_bus_get_response()

[Kai Vehmanen]

Hi,

On Mon, 7 Oct 2019, Takashi Iwai wrote:

> > I found a discrepancy in behaviour between snd-hda-intel and SOF, despite
> > using the same snd-hda-codec-hdmi as the codec driver.
[...]
> 
> While it's OK to add the polling support in the core code, I suspect
> that the main problem gets solved by setting the write_sync flag as
> the commit 2756d9143aa5.  For SOF/SST, you may set the flag
> unconditionally since they support only the new chipsets.

I've been meaning to try that out, and indeed, if I set write-sync 
on SOF side, it helps in this specific ICL case as well. I'll push a patch 
via Pierre's SOF tree:
 https://github.com/thesofproject/linux/pull/1281 

... this makes the polling fallback patch less urgent, but probably better 
to apply that as well, just to align behaviour between different HDAC 
users.

I'll also check whether we could remove the older workaround for CFL and 
CNL that force polling mode on these platforms. It is highly likely these 
workarounds are no longer needed with sync-write set.

> I've been traveling (still for the next week), so the further reply
> may be delayed.

Ack, thanks. I'll do some more testing around these flows during 
the week.

Br, Kai 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel