| Message ID | 87h6n6g69d.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: CPU/Codec connection cleanup - step1 | expand |
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
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; } }
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(-)