[PATCH 0/2] ASoC: CPU/Codec connection cleanup - step1
Hi Mark Cc Pierre-Louis
I would like to post CPU/Codec connection cleanup patch-set, but it needs to be tested at many situation. Thus, I have separate it into small sub-patch-set, and this is step1 of it.
Current soc_get_playback_capture() is checking validation of CPU/Codec, but it is too complex, and unfortunately wrong when multi CPU/Codec case. This patch fixup and cleanup it.
Kuninori Morimoto (2): ASoC: soc-pcm.c: fixup validation check of multi CPU/Codec on soc_get_playback_capture() ASoC: soc-pcm.c: factorize CPU/Codec validation check on soc_get_playback_capture()
sound/soc/soc-pcm.c | 55 ++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 33 deletions(-)
Current soc_get_playback_capture() are checking validation of CPU/Codec like below
static int soc_get_playback_capture(...) { ... ^ if (dai_link->dynamic || dai_link->no_pcm) { (X) ... v } else { ^ ... | for_each_rtd_codec_dais(rtd, i, codec_dai) { | ... | if (snd_soc_dai_stream_valid(codec_dai, ...) && | snd_soc_dai_stream_valid(cpu_dai, ...)) (Y)(a) has_playback = 1; | if (snd_soc_dai_stream_valid(codec_dai, ...) && | snd_soc_dai_stream_valid(cpu_dai, ..)) | (b) has_capture = 1; | } v } ... }
(X) is for DPCM connection, (Y) is for Normal connection. In Normal connection (Y), it is handling CPU/Codec, and it will set has_playback/capture = 1 at (a)(b), but it means today is "at least one of CPU/Codec pair was valid" in multi CPU/Codec case.
This is wrong, it should be handled when "all CPU/Codec are valid". This patch fixup it.
Link: https://lore.kernel.org/r/87mt1ihhm3.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8c168dc553f6..a45c0cf0fa14 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2787,9 +2787,10 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, if (dai_link->dpcm_playback) { stream = SNDRV_PCM_STREAM_PLAYBACK;
+ has_playback = (dai_link->num_cpus > 0); for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - has_playback = 1; + if (!snd_soc_dai_stream_valid(cpu_dai, stream)) { + has_playback = 0; break; } } @@ -2803,9 +2804,10 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, if (dai_link->dpcm_capture) { stream = SNDRV_PCM_STREAM_CAPTURE;
+ has_capture = (dai_link->num_cpus > 0); for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - has_capture = 1; + if (!snd_soc_dai_stream_valid(cpu_dai, stream)) { + has_capture = 0; break; } } @@ -2824,6 +2826,8 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, int cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
+ has_playback = (dai_link->num_codecs > 0); + has_capture = (dai_link->num_codecs > 0); for_each_rtd_codec_dais(rtd, i, codec_dai) { if (dai_link->num_cpus == 1) { cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); @@ -2848,12 +2852,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, return -EINVAL; }
- if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) - has_playback = 1; - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) - has_capture = 1; + if (!(snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && + snd_soc_dai_stream_valid(cpu_dai, cpu_playback))) + has_playback = 0; + if (!(snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && + snd_soc_dai_stream_valid(cpu_dai, cpu_capture))) + has_capture = 0; } }
Hi Morimoto-san,
Current soc_get_playback_capture() are checking validation of CPU/Codec like below
static int soc_get_playback_capture(...) { ... ^ if (dai_link->dynamic || dai_link->no_pcm) { (X) ... v } else { ^ ... | for_each_rtd_codec_dais(rtd, i, codec_dai) { | ... | if (snd_soc_dai_stream_valid(codec_dai, ...) && | snd_soc_dai_stream_valid(cpu_dai, ...)) (Y)(a) has_playback = 1; | if (snd_soc_dai_stream_valid(codec_dai, ...) && | snd_soc_dai_stream_valid(cpu_dai, ..)) | (b) has_capture = 1; | } v } ... }
(X) is for DPCM connection, (Y) is for Normal connection. In Normal connection (Y), it is handling CPU/Codec, and it will set has_playback/capture = 1 at (a)(b), but it means today is "at least one of CPU/Codec pair was valid" in multi CPU/Codec case.
This is wrong, it should be handled when "all CPU/Codec are valid". This patch fixup it.
I knew this code looked familiar and sure enough we've been there before in 2020. This code was introduced by
4f8721542f7b ASoC: core: use less strict tests for dailink capabilities
the initial stricter tests caused a number of regressions reported by Jerome Brunet so we lowered the bar from "all dais" to "at least one dai" to be backwards-compatible.
I don't think we can revisit this without hitting the same sort of issues.
Regards, -Pierre
On Wed 04 Oct 2023 at 09:06, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
Hi Morimoto-san,
Current soc_get_playback_capture() are checking validation of CPU/Codec like below
static int soc_get_playback_capture(...) { ... ^ if (dai_link->dynamic || dai_link->no_pcm) { (X) ... v } else { ^ ... | for_each_rtd_codec_dais(rtd, i, codec_dai) { | ... | if (snd_soc_dai_stream_valid(codec_dai, ...) && | snd_soc_dai_stream_valid(cpu_dai, ...)) (Y)(a) has_playback = 1; | if (snd_soc_dai_stream_valid(codec_dai, ...) && | snd_soc_dai_stream_valid(cpu_dai, ..)) | (b) has_capture = 1; | } v } ... }
(X) is for DPCM connection, (Y) is for Normal connection. In Normal connection (Y), it is handling CPU/Codec, and it will set has_playback/capture = 1 at (a)(b), but it means today is "at least one of CPU/Codec pair was valid" in multi CPU/Codec case.
This is wrong, it should be handled when "all CPU/Codec are valid". This patch fixup it.
I knew this code looked familiar and sure enough we've been there before in 2020. This code was introduced by
4f8721542f7b ASoC: core: use less strict tests for dailink capabilities
the initial stricter tests caused a number of regressions reported by Jerome Brunet so we lowered the bar from "all dais" to "at least one dai" to be backwards-compatible.
I don't think we can revisit this without hitting the same sort of issues.
Good memory :)
Hi Morimoto-san,
Here is an example: 1 CPU - 1 dai link - 2 codecs: * 1 codec handles the playback and just that * the other does same capture
I have fair number of boards doing just that.
This is valid from the HW i2s/TDM point of view.
Going with 'all must be valid for the direction' makes this use case impossible. Each codec would disable the direction of the other one.
As long as there is component, at least one, capable of handling the stream direction then it is fine to do it.
Do you have an actual problem because/error because of this ?
Regards, -Pierre
Hi Jerome, Pierre-Louis
Thank you for your feedback
Here is an example: 1 CPU - 1 dai link - 2 codecs:
- 1 codec handles the playback and just that
- the other does same capture
(snip)
Going with 'all must be valid for the direction' makes this use case impossible. Each codec would disable the direction of the other one.
Ah..., OK, I see...
Do you have an actual problem because/error because of this ?
CPU/Codec N:M support is added on ASoC, but the code is hackish, so I want makes it more generic. In the same time, this DAI validation check which is related to it is too much complex for now.
I will re-consider around there.
Thank you for your help !!
Best regards --- Kuninori Morimoto
Current soc_get_playback_capture() is checking CPU/Codec validation. But it is using different operation for each 1:N, N:N, N:M connections, thus it is very complex.
Therefore, there was omission of check at 1:N, N:N, N:M connections. It was handles as "valid" eventhough it was just "at least one of CPU/Codec pair was valid", and off course it was wrong. It is fixed and handled as "valid" when "all CPUs and Codecs were valid" today.
Because it checks all CPUs/Codecs today, we no longer need to think about CPU/Codec connection pair style. This patch cleanup CPU/Codec validation check.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 43 ++++++++++++++----------------------------- 1 file changed, 14 insertions(+), 29 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index a45c0cf0fa14..1e7925b93689 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2820,43 +2820,28 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, } } } else { - struct snd_soc_dai *codec_dai; + struct snd_soc_dai *dai;
/* Adapt stream for codec2codec links */ int cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
- has_playback = (dai_link->num_codecs > 0); - has_capture = (dai_link->num_codecs > 0); - for_each_rtd_codec_dais(rtd, i, codec_dai) { - if (dai_link->num_cpus == 1) { - cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); - } else if (dai_link->num_cpus == dai_link->num_codecs) { - cpu_dai = snd_soc_rtd_to_cpu(rtd, i); - } else if (rtd->dai_link->num_codecs > rtd->dai_link->num_cpus) { - int cpu_id; - - if (!rtd->dai_link->codec_ch_maps) { - dev_err(rtd->card->dev, "%s: no codec channel mapping table provided\n", - __func__); - return -EINVAL; - } + has_playback = + has_capture = (dai_link->num_cpus > 0 && dai_link->num_codecs > 0);
- cpu_id = rtd->dai_link->codec_ch_maps[i].connected_cpu_id; - cpu_dai = snd_soc_rtd_to_cpu(rtd, cpu_id); - } else { - dev_err(rtd->card->dev, - "%s codec number %d < cpu number %d is not supported\n", - __func__, rtd->dai_link->num_codecs, - rtd->dai_link->num_cpus); - return -EINVAL; - } + /* CPU validation check */ + for_each_rtd_cpu_dais(rtd, i, dai) { + if (!snd_soc_dai_stream_valid(dai, cpu_playback)) + has_playback = 0; + if (!snd_soc_dai_stream_valid(dai, cpu_capture)) + has_capture = 0; + }
- if (!(snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && - snd_soc_dai_stream_valid(cpu_dai, cpu_playback))) + /* Codec validation check */ + for_each_rtd_codec_dais(rtd, i, dai) { + if (!snd_soc_dai_stream_valid(dai, SNDRV_PCM_STREAM_PLAYBACK)) has_playback = 0; - if (!(snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && - snd_soc_dai_stream_valid(cpu_dai, cpu_capture))) + if (!snd_soc_dai_stream_valid(dai, SNDRV_PCM_STREAM_CAPTURE)) has_capture = 0; } }
participants (3)
-
Jerome Brunet -
Kuninori Morimoto -
Pierre-Louis Bossart