diff mbox series

ASoC: soc-pcm: fixup soc_pcm_params_symmetry()

Message ID 87a6q0z4xt.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series ASoC: soc-pcm: fixup soc_pcm_params_symmetry() | expand

Commit Message

Kuninori Morimoto April 15, 2021, 1:56 a.m. UTC
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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 <kuninori.morimoto.gx@renesas.com>
---
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 16, 2021, 12:15 a.m. UTC | #1
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
diff mbox series

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)							\