Re: [PATCH] ASoC: Intel: sst: ipc command timeout

From: Amadeusz SÅawiÅski
Date: Thu Apr 23 2020 - 04:33:54 EST



On 4/21/2020 6:16 PM, Lu, Brent wrote:

Regs width difference between BDW and BYT comes from specification. BDW
has IPC registers which are 32 wide. This fact ain't exactly the reason to modify
sst_shim32_read64.

I'm sharing Amadeo's point of view. Your change should slow down execution a
bit - but that might be just what handlers needed to make everything work again.
Debug prints also slow down the execution what could have prevented you from
spotting the real problem.
Let's ignore the memcpy stuff for a moment - could you focus on elaborating
the scenario where such issue occurs? Your initial commit message also skips
important bits such as platform used when reproducing and so on, please add
them.

Thanks,
Czarek

This issue is reported to happen on BYT Chrome book (rambi) on Chrome-v4.4
branch after changing toolchain from clang10 to clang11.

The reproduce step is simple. Just run arecord multiple times (<10) then you will
see error message for hw_params ioctl failure.

$ arecord -D hw:1,0 -f S16_LE -c 2 /dev/null
$ ctrl+c

The error message suggests the stream commit (IPC_IA_ALLOC_STREAM, 0x20) IPC
command failed due to timeout but the msg id field of ipcd read after the timeout is
also 0x20. It seems to me that the command is success but the host driver does not
know it for some reason.

2020-03-15T23:02:06.614151+00:00 ERR kernel: [ 852.023766] baytrail-pcm-audio baytrail-pcm-audio: ipc: --message timeout-- ipcx 0x2220 isr 0xf0020 ipcd 0x2220 imrx 0x0
2020-03-15T23:02:06.614209+00:00 ERR kernel: [ 852.023796] baytrail-pcm-audio baytrail-pcm-audio: ipc: error stream commit failed
2020-03-15T23:02:06.614218+00:00 ERR kernel: [ 852.023813] Baytrail Audio: PCM: failed stream commit -110
2020-03-15T23:02:06.614225+00:00 ERR kernel: [ 852.023832] baytrail-pcm-audio baytrail-pcm-audio: ASoC: baytrail-pcm-audio hw params failed: -110

I add some messages to make the log easier to read. You can check the gerrit link if you
are interested:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2131507

In the test patch I read the SST_IPCD register in sst_byt_irq_thread() twice and found
the value could be different. The IPC_IA_FREE_STREAM(0x21) seems to be the last IPC
command sent in the pcm_release of previous arecord command.

[ 125.009724] sound pcmC1D0c: snd_pcm_release:
[ 125.009732] baytrail-pcm-audio baytrail-pcm-audio: ipc_tx_message: header 0x8000000000000221 wait 1
[ 125.009760] baytrail-pcm-audio baytrail-pcm-audio: byt_tx_msg: header 0x8000000000000226
[ 125.009803] baytrail-pcm-audio baytrail-pcm-audio: sst_byt_irq_thread: header 0x297
[ 125.009823] baytrail-pcm-audio baytrail-pcm-audio: byt_tx_msg: header 0x8000000000000221
[ 125.009872] baytrail-pcm-audio baytrail-pcm-audio: sst_byt_irq_thread: header 0x297
[ 125.010442] baytrail-pcm-audio baytrail-pcm-audio: sst_byt_irq_thread: header 0x80000000000c2226
[ 125.010456] baytrail-pcm-audio baytrail-pcm-audio: sst_ipc_reply_find_msg: not match, msg header 0x8000000000000221
[ 125.010511] baytrail-pcm-audio baytrail-pcm-audio: sst_byt_irq_thread: header 0x8000000000000221
[ 125.011355] baytrail-pcm-audio baytrail-pcm-audio: tx_wait_done: ipc success, header 0x8000000000000221
[ 125.380506] sound pcmC1D0c: snd_pcm_capture_open:
[ 125.380544] sound pcmC1D0c: snd_pcm_capture_ioctl1: INFO
[ 125.380554] sound pcmC1D0c: snd_pcm_capture_ioctl1: PVERSION
[ 125.380561] sound pcmC1D0c: snd_pcm_capture_ioctl1: TTSTAMP
[ 125.380581] sound pcmC1D0c: snd_pcm_mmap:
[ 125.380622] sound pcmC1D0c: snd_pcm_mmap:
[ 125.380648] sound pcmC1D0c: snd_pcm_capture_ioctl1: INFO
[ 125.380717] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380737] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380766] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380779] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380789] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380799] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380808] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_REFINE
[ 125.380815] sound pcmC1D0c: snd_pcm_capture_ioctl1: HW_PARAMS
[ 125.380839] baytrail-pcm-audio baytrail-pcm-audio: ipc_tx_message: header 0x8000000000642220 wait 1
[ 125.380854] baytrail-pcm-audio baytrail-pcm-audio: byt_tx_msg: header 0x8000000000642220
=> write IPC_IA_ALLOC_STREAM message (0x20) to SST_IPCX
[ 125.380931] baytrail-pcm-audio baytrail-pcm-audio: sst_byt_irq_thread: header 0x8000000000000221
=> read message reply from SST_IPCD and an IPC_IA_FREE_STREAM(0x21) is returned
[ 125.380942] baytrail-pcm-audio baytrail-pcm-audio: sst_byt_irq_thread: ipcd 0x8000000000602220
=> read SST_IPCD again, this time the message id is what we are expected (0x20)
[ 125.380948] baytrail-pcm-audio baytrail-pcm-audio: sst_ipc_reply_find_msg: not match, msg header 0x8000000000642220
[ 125.681171] baytrail-pcm-audio baytrail-pcm-audio: tx_wait_done: ipc timeout, header 0x8000000000642220
[ 125.681204] baytrail-pcm-audio baytrail-pcm-audio: ipc: --message timeout-- ipcx 0x2220 isr 0x50020 ipcd 0x2220 imrx 0x0
[ 125.681212] baytrail-pcm-audio baytrail-pcm-audio: ipc: error stream commit failed
[ 125.681217] Baytrail Audio: PCM: failed stream commit -110
[ 125.681223] baytrail-pcm-audio baytrail-pcm-audio: ASoC: baytrail-pcm-audio hw params failed: -110

Regards,
Brent


I've looked at the code and byt_is_dsp_busy seems suspicious to me.
Can you check if following change fixes problem for you:

diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
index 74274bd38f7a..34746fd871b0 100644
--- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
+++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
@@ -666,8 +666,8 @@ static bool byt_is_dsp_busy(struct sst_dsp *dsp)
{
u64 ipcx;

- ipcx = sst_dsp_shim_read_unlocked(dsp, SST_IPCX);
- return (ipcx & (SST_IPCX_BUSY | SST_IPCX_DONE));
+ ipcx = sst_dsp_shim_read64_unlocked(dsp, SST_IPCX);
+ return (ipcx & (SST_BYT_IPCX_BUSY | SST_BYT_IPCX_DONE));
}

int sst_byt_dsp_init(struct device *dev, struct sst_pdata *pdata)

We seem to treat SST_IPCX as 32 bit register instead of 64 one, which may explain wrong behaviour. (Specification says it is 64 bit register).

Thanks,
Amadeusz