ASoC: soc-pcm: fixup soc_pcm_params_symmetry()

Commit Message

Kuninori Morimoto April 15, 2021, 1:56 a.m. UTC

From: Kuninori Morimoto <[e-mail]>

commit 3a9067211122 ("ASoC: soc-pcm: cleanup soc_pcm_params_symmetry()")
cleanups soc_pcm_params_symmetry() by addig new
__soc_pcm_params_symmetry() macro.

It checks symmetry first, and checks each DAI settings if symmetry
was true.
But original code checked

	symmetric_rate		: DAI_Link / CPU         (A)
	symmetric_channels	: DAI_Link / CPU / Codec (B)
	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)

	(A) was using for_each_rtd_cpu_dais()
	(B) was using for_each_rtd_dais()

Current code is using (B) for all symmetric_xxx. This is bug.

One note is that we can use .be_hw_params_fixup in DPCM case.
This means, BE settings might be fixuped/updated by FE.
It can be happen not only for rate, but for channels/sample_bits too.

And also, DPCM uses dummy-DAI which will be used for all DPCM
connectoin. ALSA SoC will clean DAI params (a) if it was last user (b).
But dummy-DAI is used from many place, it might not be cleaned.

	static int soc_pcm_hw_clean(...)
	{
		...
		for_each_rtd_dais(rtd, i, dai) {
			...
(b)			if (snd_soc_dai_active(dai) == 1)
(a)				soc_pcm_set_dai_params(dai, NULL);
			...
		}
		...
	}

The solution maybe
	(A1) Symmetric checks CPU only
	(A2) Symmetric checks both CPU / Codec, but ignores dummy-DAI

This patch selects A1.

For example, if Sound Card is exchanging all rate to 48kHz
by using .be_hw_params_fixup() on DPCM, below can be happen.
It is using 44100 Hz, but has mismatch between dummy-DAI which is keeping
48kHz from 1st aplay.

	# aplay 44100.wav
	# aplay 44100.wav
=>	[kernel] be.ak4613-hifi: ASoC: unmatched rate symmetry: 44100 - 48000
	[kernel] be.ak4613-hifi: ASoC: hw_params BE failed -22
	[kernel] fe.rsnd-dai.0: ASoC: hw_params BE failed -22
	aplay: set_params:1407: Unable to install hw params:
	ACCESS:  RW_INTERLEAVED
	FORMAT:  S16_LE
	SUBFORMAT:  STD
	SAMPLE_BITS: 16
	FRAME_BITS: 32
	CHANNELS: 2
	RATE: 44100
	PERIOD_TIME: (23219 23220)
	PERIOD_SIZE: 1024
	PERIOD_BYTES: 4096
	PERIODS: 4
	BUFFER_TIME: (92879 92880)
	BUFFER_SIZE: 4096
	BUFFER_BYTES: 16384
	TICK_TIME: 0

Fixes: 3a9067211122 ("ASoC: soc-pcm: cleanup soc_pcm_params_symmetry()")
Signed-off-by: Kuninori Morimoto <[e-mail]>
---
Hi Mark

This patch is needed for latest linus tree (= for v5.12).
Please let me know if you want (A2).

 sound/soc/soc-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kuninori Morimoto April 15, 2021, 6:24 a.m. UTC | #1

Hi Mark

> commit 3a9067211122 ("ASoC: soc-pcm: cleanup soc_pcm_params_symmetry()")
> cleanups soc_pcm_params_symmetry() by addig new
> __soc_pcm_params_symmetry() macro.
> 
> It checks symmetry first, and checks each DAI settings if symmetry
> was true.
> But original code checked
> 
> 	symmetric_rate		: DAI_Link / CPU         (A)
> 	symmetric_channels	: DAI_Link / CPU / Codec (B)
> 	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)
> 
> 	(A) was using for_each_rtd_cpu_dais()
> 	(B) was using for_each_rtd_dais()
> 
> Current code is using (B) for all symmetric_xxx. This is bug.

Oops ?
More older verion was

	symmetric_rate		: DAI_Link / CPU / Codec (B)
	symmetric_channels	: DAI_Link / CPU / Codec (B)
	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)

but commit c840f7698d26 ("ASoC: soc-pcm: Merge for_each_rtd_cpu/codec_dais()")
had bug, and code become

	symmetric_rate		: DAI_Link / CPU         (A)
	symmetric_channels	: DAI_Link / CPU / Codec (B)
	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)

And commit 3a9067211122 ("ASoC: soc-pcm: cleanup soc_pcm_params_symmetry()")
back again

	symmetric_rate		: DAI_Link / CPU / Codec (B)
	symmetric_channels	: DAI_Link / CPU / Codec (B)
	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)

Does this issue had been happen since older versoin ??

	# aplay 44100.wav
	# aplay 44100.wav
=>	[kernel] be.ak4613-hifi: ASoC: unmatched rate symmetry: 44100 - 48000
	[kernel] be.ak4613-hifi: ASoC: hw_params BE failed -22

If so, correct solution should be (A2)

	(A1) Symmetric checks CPU only
	(A2) Symmetric checks both CPU / Codec, but ignores dummy-DAI

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Mark Brown April 15, 2021, 3:37 p.m. UTC | #2

On Thu, Apr 15, 2021 at 03:24:01PM +0900, Kuninori Morimoto wrote:

> > It checks symmetry first, and checks each DAI settings if symmetry
> > was true.
> > But original code checked

> > 	symmetric_rate		: DAI_Link / CPU         (A)
> > 	symmetric_channels	: DAI_Link / CPU / Codec (B)
> > 	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)

> > 	(A) was using for_each_rtd_cpu_dais()
> > 	(B) was using for_each_rtd_dais()

> > Current code is using (B) for all symmetric_xxx. This is bug.

> Oops ?

> More older verion was

> 	symmetric_rate		: DAI_Link / CPU / Codec (B)
> 	symmetric_channels	: DAI_Link / CPU / Codec (B)
> 	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)

Right, this is what I'd expect.

> Does this issue had been happen since older versoin ??
> 
> 	# aplay 44100.wav
> 	# aplay 44100.wav
> =>	[kernel] be.ak4613-hifi: ASoC: unmatched rate symmetry: 44100 - 48000
> 	[kernel] be.ak4613-hifi: ASoC: hw_params BE failed -22

> If so, correct solution should be (A2)

> 	(A1) Symmetric checks CPU only
> 	(A2) Symmetric checks both CPU / Codec, but ignores dummy-DAI

I think this is something that gets confused by DPCM breaking
assumptions :/ .  Not sure if *ignoring* the dummy DAI is the best
option though, the general way the dummy works is that it has extremely
loose constraints so it ends up not restricting anything else it gets
paired with but then the dummy DAI might end up in multiple links.

Is it a case of needing a fixup function, or special handling if a fixup
function is there?

Kuninori Morimoto April 16, 2021, 12:15 a.m. UTC | #3

Hi Mark

Thank you for your feedback

> > 	symmetric_rate		: DAI_Link / CPU / Codec (B)
> > 	symmetric_channels	: DAI_Link / CPU / Codec (B)
> > 	symmetric_sample_bits	: DAI_Link / CPU / Codec (B)
> 
> Right, this is what I'd expect.

Yes, I agree

> > Does this issue had been happen since older versoin ??
> > 
> > 	# aplay 44100.wav
> > 	# aplay 44100.wav
> > =>	[kernel] be.ak4613-hifi: ASoC: unmatched rate symmetry: 44100 - 48000
> > 	[kernel] be.ak4613-hifi: ASoC: hw_params BE failed -22

I confirmed. Unfortunately there was copy miss (= bug) on
soc_pcm_params_symmetry() (= A) which didn't check Codec.
But is back by (B).

	A: v5.7:  commit c840f7698d26 ("ASoC: soc-pcm: Merge for_each_rtd_cpu/codec_dais()")
	B: v5.12: commit 3a9067211122 ("ASoC: soc-pcm: cleanup soc_pcm_params_symmetry()")

In total,
old - v5.6 (= Generation-1):

	symmetric_rate		: DAI_Link / CPU / Codec
	symmetric_channels	: DAI_Link / CPU / Codec
	symmetric_sample_bits	: DAI_Link / CPU / Codec

v5.7 - v5.11 (= Generation-2): (= because of bug by (A))

	symmetric_rate		: DAI_Link / CPU
	symmetric_channels	: DAI_Link / CPU / Codec
	symmetric_sample_bits	: DAI_Link / CPU / Codec

v5.12 - (= Generation-3): (= back by (B))

	symmetric_rate		: DAI_Link / CPU / Codec
	symmetric_channels	: DAI_Link / CPU / Codec
	symmetric_sample_bits	: DAI_Link / CPU / Codec

Because of these, Generation-1 / Generation-3 have DPCM issue
if it was..
	1) using .be_hw_params_fixup
	2) exchanged BE's rate

The issue is below. I didn't confirm but maybe same things happen
if .be_hw_params_fixup exchanged channels/sample_bits, I guess.

	# aplay 44100.wav
	# aplay 44100.wav
=>	[kernel] be.ak4613-hifi: ASoC: unmatched rate symmetry: 44100 - 48000
	[kernel] be.ak4613-hifi: ASoC: hw_params BE failed -22
	...

> I think this is something that gets confused by DPCM breaking
> assumptions :/ .  Not sure if *ignoring* the dummy DAI is the best
> option though, the general way the dummy works is that it has extremely
> loose constraints so it ends up not restricting anything else it gets
> paired with but then the dummy DAI might end up in multiple links.

Ignoring dummy-DAI is not so bad idea,
and it is possible to lose any constraints, I think.
soc_probe_component() is also ignoring it.

	static int soc_probe_component(...)
	{
		...
=>		if (!strcmp(component->name, "snd-soc-dummy"))
			return 0;
		...
	}

> Is it a case of needing a fixup function, or special handling if a fixup
> function is there?

Above issue will gone if soc_pcm_params_symmetry() didn't check
dummy-DAI's rate/channels/sample_bits.
I will post the patches.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Patch

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 02968a4e52b4..92e95e4aef9f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -388,7 +388,7 @@  static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
 
 #define __soc_pcm_params_symmetry(name)					\
 	symmetry = rtd->dai_link->symmetric_##name;			\
-	for_each_rtd_dais(rtd, i, dai)					\
+	for_each_rtd_cpu_dais(rtd, i, dai)					\
 		symmetry |= dai->driver->symmetric_##name;		\
 									\
 	if (symmetry)							\