ASoc / PCM recording-related regression between v5.4 and v5.5

ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

Hi,

I am trying to find the cause of a rather unusual regression between kernel v5.4 and v5.5, for an out-of-tree driver on the raspberrypi. Everything looks identical between v5.4 and 5.5, within accountable differences, even when I enable dynamic debugging and 

echo "file sound/* +p" > /sys/kernel/debug/dynamic_debug/control

for the whole of sound sub-system. It records fines against v5.4, but blocks on v5.5, and on ctrl-c, does

^CAborted by signal Interrupt...
arecord: pcm_read:2145: read error: Interrupted system call

I have gradually replaced the include/sound and sound/ directories between the two (the pi v5.4 and v5.5 kernels are separate branches off linux-stable, so one cannot do ordinary bisects), and narrowed it down to the following 13 commits from 5 people: 

4104faaeeda0e23a169c50e43f309ff7435087b1 ASoC: soc-core: disable route checks for legacy devices
4bf2e385aa59c2fae5f880aa25cfd2b470109093 ASoC: core: Init pcm runtime work early to avoid warnings
fb5126778333d289b2623a7e6260beeb1ac1b819 ASoC: core: add SND_SOC_BYTES_E
dc73d73aa7145f55412611f3eead1e85ae026785 ASoC: add control components management
509ba54fcfd1e45bceebe8ccea59dc496312f1a0 ASoC: soc.h: dobj is used only when SND_SOC_TOPOLOGY
e443c20593de9f8efd9b2935ed40eb0bbacce30b ASoC: soc-core: don't call snd_soc_dapm_new_dai_widgets() at snd_soc_register_dai()
5d07519703bc2f0bf19d33652401552a480d68b8 ASoC: soc-core: have legacy_dai_naming at snd_soc_register_dai()
e11381f38f34789b374880c4a149e25e8d7f0cfd ASoC: soc-core: add snd_soc_unregister_dai()
4baabbf932ed4f97df8e18cf546d39b7c2138020 ASoC: soc-dpcm: tidyup for_each_dpcm_xx() macro
2b544dd7b43b19fb55ea4fbb3e30b60eb20b7828 ASoC: soc-core: add for_each_rtd_components() and replace
33536a14879515949b065721cdb7fedb276d8e8a ASoC: soc-core: remove for_each_rtdcom_safe()
8ec241c495dde3d19a0459304298c2468c60182b ASoC: soc-core: add snd_soc_pcm_lib_ioctl()
b7c5bc45ee94a03a0dc45a862180e17db8ea8e9d ASoC: soc-core: merge soc_free_pcm_runtime() and soc_rtd_free()

I think it is overlay / devicetree related, since another audio device of the same family and similar hardware but different overlay works fine. As I already did 'echo "file sound/* +p" > /sys/kernel/debug/dynamic_debug/control' , I likely need some additional debug statement or enable debugging at another part to go further, despite it being a regression in 'include/sound/soc*.h' and 'sound/soc/soc-*'. Building the pi kernel cleanly - from 'make mrproper', copy .config then 'make oldconnfig' - takes about an hour, so switching between the two and compare takes about 3 hours, and quite slow.

I can possibly narrow the changes further from the 13 commits (9 of them from Kuninori Morimoto), but I think some more dev_dbg() is needed as the current ones in "sound/*" don't help; and it is worth bring it to alsa-devel since it is so unusual - all the dev_dbg() in kernel/sound/ and the driver side emits to the same messages, v5.4 can records while v5.5 blocks on stuck system call.

More details at
https://github.com/raspberrypi/linux/issues/4279
and it was initially at https://github.com/respeaker/seeed-voicecard/issues/290 . The "seeed-8mic-voicecard-overlay.dts" is the dts corresponds to the problematic device ;  The one with similar hardware but okay is "seeed-4mic-voicecard-overlay.dts" .

I am subscribed to alsa-devel actually, but receive it in digest form, and haven't opened the digests for a while...

Thanks a lot.

Hin-Tak

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Kuninori Morimoto]

Hi

> I am trying to find the cause of a rather unusual regression between kernel v5.4 and v5.5,
> for an out-of-tree driver on the raspberrypi.

I think out-of-tree code needs update.
We merged snd_pcm_ops into component between v5.4 - v5.5

	e2cb4a14541dba3587bb78e0f62da27a0e1ad399
    	("ASoC: soc-core: merge snd_pcm_ops member to component driver")

And all drivers which used it was updated.
For example,

	1fddf424b3c49a475ca7c23662f515b53f884172
	("ASoC: mediatek: remove snd_pcm_ops")

And snd_pcm_ops is no longer used on ALSA SoC

	e9067bb502787869dabe385727baff233024097b
	("ASoC: soc-component: remove snd_pcm_ops from component driver")

If your out-of-tree is using it, and not yet updated,
some issue can be happen, I think.


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 On Monday, 19 April 2021, 00:02:09 BST, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi

> > I am trying to find the cause of a rather unusual regression between kernel v5.4 and v5.5,
> > for an out-of-tree driver on the raspberrypi.

> I think out-of-tree code needs update.
> We merged snd_pcm_ops into component between v5.4 - v5.5

>    e2cb4a14541dba3587bb78e0f62da27a0e1ad399
>       ("ASoC: soc-core: merge snd_pcm_ops member to component driver")

> And all drivers which used it was updated.
> For example,

>    1fddf424b3c49a475ca7c23662f515b53f884172
>    ("ASoC: mediatek: remove snd_pcm_ops")

> And snd_pcm_ops is no longer used on ALSA SoC

>    e9067bb502787869dabe385727baff233024097b
>    ("ASoC: soc-component: remove snd_pcm_ops from component driver")

> If your out-of-tree is using it, and not yet updated,
> some issue can be happen, I think.

Thanks for the reply. It appears I made a mistake in my earlier posting - my 'git diff' of what files matter was correct, but the commit listing / authors with git log was missing some \* and were way too small. The actual number of possible commits is rather in the 100+ and involving about 20+ authors. That said, thanks to your suggestions, I have found one error - and a problematic area in the current out-of-tree driver code:

it was not setting .non_legacy_dai_naming - this was a 4.16 change . The similar hardware but working sibling device has one single ADC codec, while the broken device has two such ADC codec plus a 3rd DAC codec; I see that this flag started to matter in v5.6 for codec2codec links . I don't know if this qualifies as a codec 2 codec link - it is running two 4-mic ADC in parallel to read from 6 mics...

Anyway, I tried putting that flag in, but it has not fix problem with 5.10, so I am back to trimming the difference between v5.4 and v5.5 slowly.

At the moment I think I need to look at the v4.16 series involving .non_legacy_dai_naming again; while trimming the difference between v5.4 and v5.5 slowly.

(the out-of-tree driver is not my code - I am just the owner of one such device... the vendor is not very responsive, so I have been doing most of the porting since 4.19...)

Thanks a lot.

Hin-Tak

  

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Kuninori Morimoto]

Hi Hin-Tak


> it was not setting .non_legacy_dai_naming - this was a 4.16 change . The similar hardware but working
> sibling device has one single ADC codec, while the broken device has two such ADC codec plus a 3rd DAC
> codec; I see that this flag started to matter in v5.6 for codec2codec links . I don't know if this
> qualifies as a codec 2 codec link - it is running two 4-mic ADC in parallel to read from 6 mics...
> 
> Anyway, I tried putting that flag in, but it has not fix problem with 5.10, so I am back to trimming
> the difference between v5.4 and v5.5 slowly.

v5.3 has other big change on ALSA SoC for each driver.
Out-of-tree might get this effect.

	ASoC: xxxx: use modern dai_link style

For example,
05ab66178cb27ee795aa458b43818d2caa2d3953
("ASoC: mediatek: mt8173-rt5650-rt5676: use modern dai_link style")

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 

    On Tuesday, 20 April 2021, 06:35:51 GMT+8, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:
> v5.3 has other big change on ALSA SoC for each driver.
> Out-of-tree might get this effect.

>     ASoC: xxxx: use modern dai_link style

> For example,
> 05ab66178cb27ee795aa45b43818d2caa2d3953
> ("ASoC: mediatek: mt8173-rt5650-rt5676: use modern dai_link style")

I am bisecting down to 5 asoc related commits now, and currently building "ASoC: pcm_dmaengine: Extra snd_dmaengine_pcm_refine_runtime_hwpararms". The breakage is between "...remove snd_soc_soc_rtdcom_del_all()" and "Merge branch asoc-5.4 into asoc-5.5". It takes about an hour to do a clean build.
I think it is either that hwparams commit or the next one, "use different sequence..."?
I shall be able to tell you for sure in a few more hours / next day.
Any thoughts on those two?
Regards,Hin-Tak  

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 Kuninori Morimoto: Apologies - I have finished bisecting, and flip the problematic commit back and forth twice to confirm. It is not one of yours! Sorry about that.

Hi Peter:

I found the commit of a rather unusual regression between kernel v5.4 and v5.5, for an out-of-tree driver on the raspberrypi. Everything looks identical between v5.4 and 5.5, within accountable differences, even with debugging in sound/ +p and the driver side. 5.4 records fine, 5.5 stucks on recording. The commit is one of yours:

commit 4378f1fbe924054a09ff0d4e39e1a581b9245252
Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
Date: Fri Sep 27 10:16:46 2019 +0300

 ASoC: soc-pcm: Use different sequence for start/stop trigger
 
 On stream stop currently we stop the DMA first followed by the CPU DAI.
 This can cause underflow (playback) or overflow (capture) on the DAI side
 as the DMA is no longer feeding data while the DAI is still active.
 It can be observed easily if the DAI side does not have FIFO (or it is
 disabled) to survive the time while the DMA is stopped, but still can
 happen on relatively slow CPUs when relatively high sampling rate is used:
 the FIFO is drained between the time the DMA is stopped and the DAI is
 stopped.

 The problem was initially at https://github.com/respeaker/seeed-voicecard/issues/290 
then over to https://github.com/raspberrypi/linux/issues/4279 and finally here. The "seeed-8mic-voicecard-overlay.dts" is the dts corresponds to the problematic device ; The one with similar hardware but okay is "seeed-4mic-voicecard-overlay.dts" .

The Raspberry Pi distribution raspbian jumped directly from 5.4.x to 5.10.x at the beginning of February. Considering the change has been in since the v5.5 merge window, I guess I'd like some help to correct / workaround on the out-of-tree driver side? And probably some new dev_err() message in the kernel for problematic driver helping to diagnose similar problems in the future would be nice. 

And everybody: apologies about the mistakes in the initial posting.

Regards
Hin-Tak
  

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 Hi,

I agree with Peter's change. Raspbian - the raspberry pi distribution - is currently shipping v5.10.x (jumping from v5.4.x in February), which has changed a lot since v5.5.x. Nonetheless, as a proof of concept, I reverted the idea of Peter's change in v.5.10.x, with this:

<diff-start>
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 91bf33958..20077dd8c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1042,7 +1042,7 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 case SNDRV_PCM_TRIGGER_START:
 case SNDRV_PCM_TRIGGER_RESUME:
 case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
- ret = snd_soc_link_trigger(substream, cmd);
+ ret = snd_soc_pcm_dai_trigger(substream, cmd);
 if (ret < 0)
 break;
 
@@ -1050,8 +1050,9 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 if (ret < 0)
 break;
 
- ret = snd_soc_pcm_dai_trigger(substream, cmd);
+ ret = snd_soc_link_trigger(substream, cmd);
 break;
+
 case SNDRV_PCM_TRIGGER_STOP:
 case SNDRV_PCM_TRIGGER_SUSPEND:
 case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
<diff-end>

and was able to restore the functionality of Seeed Studio's respeaker driver against v5.10.x. That it relies on the previous behavior is a bit broken. I think I'd like some dev_dbg() inside soc-pcm.c, and perhaps some help in modifying the out-of-tree audio device driver to cope?

Thanks a lot.

Regards,
Hin-Tak 

On Tuesday, 20 April 2021, 15:19:58 BST, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:

> Hi Peter:

> I found the commit of a rather unusual regression between kernel v5.4 and v5.5, for an out-of-tree driver on the 
> raspberrypi. Everything looks identical between v5.4 and 5.5, within accountable differences, even with debugging in > sound/ +p and the driver side. 5.4 records fine, 5.5 stucks on recording. The commit is one of yours:

> commit 4378f1fbe924054a09ff0d4e39e1a581b9245252
> Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Date: Fri Sep 27 10:16:46 2019 +0300

> ASoC: soc-pcm: Use different sequence for start/stop trigger

...

> The problem was initially at https://github.com/respeaker/seeed-voicecard/issues/290 
> then over to https://github.com/raspberrypi/linux/issues/4279 and finally here. The "seeed-8mic-voicecard-
> overlay.dts" is the dts corresponds to the problematic device ; The one with similar hardware but okay is "seeed-
> 4mic-voicecard-overlay.dts" .

...
> The Raspberry Pi distribution raspbian jumped directly from 5.4.x to 5.10.x at the beginning of February. Considering 
> the change has been in since the v5.5 merge window, I guess I'd like some help to correct / workaround on the 
> out-of-tree driver side? And probably some new dev_err() message in the kernel for problematic driver helping to 
> diagnose similar problems in the future would be nice. 

  

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Kuninori Morimoto]

Hi Hin-Tak

> I am bisecting down to 5 asoc related commits now, and currently building "ASoC: pcm_dmaengine: Extra snd_dmaengine_pcm_refine_runtime_hwpararms". The breakage is between
> "...remove snd_soc_soc_rtdcom_del_all()" and "Merge branch asoc-5.4 into asoc-5.5". It takes about an hour to do a clean build.
> 
> I think it is either that hwparams commit or the next one, "use different sequence..."?
> 
> I shall be able to tell you for sure in a few more hours / next day.
> 
> Any thoughts on those two?

After "ASoC: soc-core: remove snd_soc_rtdcom_del_all()" patch (= A),
it will merge snd_pcm_ops to component driver (= B),
and many drivers switch to use it (= C).
After that, original snd_pcm_ops will be removed (= D).
If your out-of-tree is still using it, this can help you ?

(A)	353e16bf60458fae5927cf04ff668fc152fff465
	("ASoC: soc-core: remove snd_soc_rtdcom_del_all()")

(B)	e2cb4a14541dba3587bb78e0f62da27a0e1ad399
	("ASoC: soc-core: merge snd_pcm_ops member to component driver")

(C)	9dcefa7232d3fd5a7141454849d46ed24c7af867
	("ASoC: sh: rsnd: remove snd_pcm_ops")

(D)	e9067bb502787869dabe385727baff233024097b
	("ASoC: soc-component: remove snd_pcm_ops from component driver")


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Péter Ujfalusi]

Hi,

On 4/20/21 7:04 PM, Hin-Tak Leung wrote:
>  Hi,
> 
> I agree with Peter's change. Raspbian - the raspberry pi distribution - is currently shipping v5.10.x (jumping from v5.4.x in February), which has changed a lot since v5.5.x. Nonetheless, as a proof of concept, I reverted the idea of Peter's change in v.5.10.x, with this:
> 
> <diff-start>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 91bf33958..20077dd8c 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1042,7 +1042,7 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>  case SNDRV_PCM_TRIGGER_START:
>  case SNDRV_PCM_TRIGGER_RESUME:
>  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> - ret = snd_soc_link_trigger(substream, cmd);
> + ret = snd_soc_pcm_dai_trigger(substream, cmd);
>  if (ret < 0)
>  break;
>  
> @@ -1050,8 +1050,9 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>  if (ret < 0)
>  break;
>  
> - ret = snd_soc_pcm_dai_trigger(substream, cmd);
> + ret = snd_soc_link_trigger(substream, cmd);
>  break;
> +
>  case SNDRV_PCM_TRIGGER_STOP:
>  case SNDRV_PCM_TRIGGER_SUSPEND:
>  case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> <diff-end>
> 
> and was able to restore the functionality of Seeed Studio's respeaker driver against v5.10.x. That it relies on the previous behavior is a bit broken. I think I'd like some dev_dbg() inside soc-pcm.c, and perhaps some help in modifying the out-of-tree audio device driver to cope?


If you start the DAI first and later the DMA then it works again?
I'm still behind of the patch which introduced different sequences for
start and stop.

From my commit the start sequence is:
dai_link, DMA, CPU DAI then the codec

Is it make any difference if you try:
dai_link, DMA, codec then the CPU DAI

But the codec is usually not handled in trigger, it can not be atomic
most of the time.

Can you point me to the out of tree driver and the related dts files?
Can it be reproduced with rpbi w/o any hat? I have one hifiberry but
that is playback only and if I'm not mistaken the 3.5 on the board also?


> 
> Thanks a lot.
> 
> Regards,
> Hin-Tak 
> 
> On Tuesday, 20 April 2021, 15:19:58 BST, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:
> 
>> Hi Peter:
> 
>> I found the commit of a rather unusual regression between kernel v5.4 and v5.5, for an out-of-tree driver on the 
>> raspberrypi. Everything looks identical between v5.4 and 5.5, within accountable differences, even with debugging in > sound/ +p and the driver side. 5.4 records fine, 5.5 stucks on recording. The commit is one of yours:
> 
>> commit 4378f1fbe924054a09ff0d4e39e1a581b9245252
>> Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Date: Fri Sep 27 10:16:46 2019 +0300
> 
>> ASoC: soc-pcm: Use different sequence for start/stop trigger
> 
> ...
> 
>> The problem was initially at https://github.com/respeaker/seeed-voicecard/issues/290 
>> then over to https://github.com/raspberrypi/linux/issues/4279 and finally here. The "seeed-8mic-voicecard-
>> overlay.dts" is the dts corresponds to the problematic device ; The one with similar hardware but okay is "seeed-
>> 4mic-voicecard-overlay.dts" .
> 
> ...
>> The Raspberry Pi distribution raspbian jumped directly from 5.4.x to 5.10.x at the beginning of February. Considering 
>> the change has been in since the v5.5 merge window, I guess I'd like some help to correct / workaround on the 
>> out-of-tree driver side? And probably some new dev_err() message in the kernel for problematic driver helping to 
>> diagnose similar problems in the future would be nice. 
> 
>   
> 

-- 
Péter

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 On Thursday, 22 April 2021, 15:01:02 BST, Péter Ujfalusi <peter.ujfalusi@gmail.com> wrote:

> If you start the DAI first and later the DMA then it works again?
> I'm still behind of the patch which introduced different sequences for
> start and stop.

I am with you on your change too - I think your change makes sense; it is the vendor's driver that needs correcting.

> From my commit the start sequence is:
> dai_link, DMA, CPU DAI then the codec

> Is it make any difference if you try:
> dai_link, DMA, codec then the CPU DAI

I'll give this a try soon. I am just the owner of one such device, and did the porting from kernel 4.19 to v.5.4, and now troubleshooting to v5.10. The vendor has been quite absent... and I feel I perhaps spend more time on this venture than I am happy with... Anyway - 

> But the codec is usually not handled in trigger, it can not be atomic
> most of the time.

I suspect the vendor did something not quite right in the area to make it work under the previous wrong trigger order too. More below about spinlock and atomic context.

> Can you point me to the out of tree driver and the related dts files?
> Can it be reproduced with rpbi w/o any hat? I have one hifiberry but
> that is playback only and if I'm not mistaken the 3.5 on the board also?

For your purpose you can see my fork (which includes v5.4 -> v5.10 migration change, unlike the vendor's), https://github.com/HinTak/seeed-voicecard . 

The dts concerned is: https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-8mic-voicecard-overlay.dts

The card trigger code is in: https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-voicecard.c

while the codec trigger code is in
https://github.com/HinTak/seeed-voicecard/blob/v5.9/ac108.c
and also https://github.com/HinTak/seeed-voicecard/blob/v5.9/ac101.c ! 

Note that the routine ac101_trigger() is not used, although the ac101 codec itself is involved.
There is a spinlock in ac108_trigger(), which I have long suspected to be incorrect, and has been known to cause the pi to kernel panic if built with PREEMPT ; Now I think it is there to make it work under the previous wrong sequence.

There is another device of the same family - which records okay regardless of the trigger sequence https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-4mic-voicecard-overlay.dts .

The problematic device has 8 mics, using two ac108 for 2 x 4-ADC and the ac101 for DAC loopback (and as clock master?). The 4-mics record-okay device has just one ac108 (and without a ac101) to record 4-channels.

As I understand it, to record / playback for 8 channels on the pi, the I2S on the pi transfer data as stereo at 4x the frequency, and the codecs try to start and stop in such a way to avoid channel shifts during the 8-channels to 4x stereo change.

I can reboot between 5.4 and 5.10 quite easily (the pi does not have a boot loader per se - you just overwrite the kernel image in the first fat32 partitition...). At the moment I am thinking of inserting some pr_info() into seeed-voicecard.c:seeed_voice_card_trigger() and ac108.c:ac108_trigger() and perhaps also in general in ac101.c to see how the driver behaves differently under v5.4 and v5.10 . And while doing that, have some thoughts about getting rid of that problematic spinlock in ac108.c:ac108_trigger() when the code get re-arranged.

The problem is with trying to record, so I assume you cannot reproduce the problem if you do not have the hardware. However, I could certainly benefit from some suggestions and guidance on what to change and how to change the current code.

I am fairly okay with kernel development (in a few other different areas, file systems and wireless mostly), but audio is new to me.

Thanks a lot for any ideas / suggestions you might want to give on modfying the two trigger functions and getting rid of that spinlock!

Regards,
Hin-Tak  

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 

On Friday, 23 April 2021, 12:22:11 BST, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:

> On Thursday, 22 April 2021, 15:01:02 BST, Péter Ujfalusi <peter.ujfalusi@gmail.com> wrote:

> > Can you point me to the out of tree driver and the related dts files?
> > Can it be reproduced with rpbi w/o any hat? I have one hifiberry but
> > that is playback only and if I'm not mistaken the 3.5 on the board also?

> For your purpose you can see my fork (which includes v5.4 -> v5.10 migration change, unlike the vendor's), 
> https://github.com/HinTak/seeed-voicecard . 

> The dts concerned is: https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-8mic-voicecard-overlay.dts

> The card trigger code is in: https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-voicecard.c

> while the codec trigger code is in
> https://github.com/HinTak/seeed-voicecard/blob/v5.9/ac108.c
> and also https://github.com/HinTak/seeed-voicecard/blob/v5.9/ac101.c ! 

> Note that the routine ac101_trigger() is not used, although the ac101 codec itself is involved.
> There is a spinlock in ac108_trigger(), which I have long suspected to be incorrect, and has been known to cause the > pi to kernel panic if built with PREEMPT ; Now I think it is there to make it work under the previous wrong sequence.

> There is another device of the same family - which records okay regardless of the trigger sequence 
> https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-4mic-voicecard-overlay.dts .

> The problematic device has 8 mics, using two ac108 for 2 x 4-ADC and the ac101 for DAC loopback (and as clock
> master?). The 4-mics record-okay device has just one ac108 (and without a ac101) to record 4-channels.

I have gone ahead and inserted a number of pr_info()'s to all the triggers (and compare the order of calls in 5.4 and 5.10) ; there was a mistake in my comment above: ac101_trigger() is used! it is just not set up as a call-back in the component struct.

If I understand it correctly, the 8-mics device was designed to do record and playback simultaneously; the master clock is really on the DAC ac101 codec; and ac101_trigger() needed to be called before the card trigger code seeed_voice_card_trigger() calling _set_clock().

In 5.4, we had ac108_trigger() calling ac101_trigger() , then seeed_voice_card_trigger() calling _set_clock(), which works. 
in 5.10 (and 5.5+), seeed_voice_card_trigger() calling _set_clock(), then ac108_trigger() calling ac101_trigger() , which does not.

So I made this rather ugly change to the driver code, and it gives me the right behavior:

<diff-begin>
diff --git a/Makefile b/Makefile
index b9de7f4..1aa0949 100644
--- a/Makefile
+++ b/Makefile
@@ -14,7 +14,7 @@ ifneq ($(KERNELRELEASE),)
 
 snd-soc-wm8960-objs := wm8960.o
 snd-soc-ac108-objs := ac108.o ac101.o
-snd-soc-seeed-voicecard-objs := seeed-voicecard.o
+snd-soc-seeed-voicecard-objs := seeed-voicecard.o ac101.o
 
 
 obj-m += snd-soc-wm8960.o
diff --git a/ac108.c b/ac108.c
index 5fe5176..c755741 100644
--- a/ac108.c
+++ b/ac108.c
@@ -1060,7 +1060,7 @@ static int ac108_trigger(struct snd_pcm_substream *substream, int cmd,
 
 spin_lock_irqsave(&ac10x->lock, flags);
 
- if (ac10x->i2c101 && _MASTER_MULTI_CODEC == _MASTER_AC101) {
+ if (!cmd && ac10x->i2c101 && _MASTER_MULTI_CODEC == _MASTER_AC101) {
 ac101_trigger(substream, cmd, dai);
 if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 goto __ret;
diff --git a/seeed-voicecard.c b/seeed-voicecard.c
index 23dfb15..244dc6e 100644
--- a/seeed-voicecard.c
+++ b/seeed-voicecard.c
@@ -197,6 +197,7 @@ static int seeed_voice_card_trigger(struct snd_pcm_substream *substream, int cmd
 {
 struct snd_soc_pcm_runtime *rtd = substream->private_data;
 struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
+ struct ac10x_priv *ac10x = snd_soc_dai_get_drvdata(dai);
 struct seeed_card_data *priv = snd_soc_card_get_drvdata(rtd->card);
 #if CONFIG_AC10X_TRIG_LOCK
 unsigned long flags;
@@ -216,6 +217,9 @@ static int seeed_voice_card_trigger(struct snd_pcm_substream *substream, int cmd
 /* I know it will degrades performance, but I have no choice */
 spin_lock_irqsave(&priv->lock, flags);
 #endif
+ if (cmd && ac10x->i2c101 && _MASTER_MULTI_CODEC == _MASTER_AC101) {
+ ac101_trigger(substream, cmd, dai);
+ }
 if (_set_clock[SNDRV_PCM_STREAM_CAPTURE]) _set_clock[SNDRV_PCM_STREAM_CAPTURE](1);
 if (_set_clock[SNDRV_PCM_STREAM_PLAYBACK]) _set_clock[SNDRV_PCM_STREAM_PLAYBACK](1);
 #if CONFIG_AC10X_TRIG_LOCK
<diff-ends>

It is certainly not ideal for the card driver to be linked directly and calling codec routines... Is there a better way of doing this, perhaps by re-writing the dts in https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-8mic-voicecard-overlay.dts ? 

While we are at it, the spinlock in ac108.c (just above the changed line) seems wrong; can somebody have a look and advise on what better way to do it?

Regards,
Hin-Tak  

Re: ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

 Hi all,

I have come up with a second way of fixing the regression (the first one was a few commits before that):

https://github.com/HinTak/seeed-voicecard/commit/19067f3333d7759bdf626e62bc09a7deae4627a1

As far as I understand it, the breakage came from the somewhat unsual design of the SoC device: it has both recording / ADC and a playback / DAC codecs. (a pair of ADC ones, actually) to do real-time echo cancellation / noise filtering applications. The recording codecs use the playback codec's clock; and the playback codec's trigger needs to be called, before the clock can start.

I am not entirely happy with the rest of the vendor's code in terms of what code is where of event sequences, but I'll leave that for now. 

Would appreciate comments, etc.