[PATCH] ASoC: soc-pcm: fixup soc_pcm_params_symmetry()
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(-)
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) \
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
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?
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
participants (2)
-
Kuninori Morimoto
-
Mark Brown