| Message ID | 87zg0jwdnz.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| Headers | show |
| Series | ASoC: makes CPU/Codec channel connection map more generic | expand |
On Mon 16 Oct 2023 at 01:37, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Mark > Cc Bard, Pierre-Louis, Jerome, DT-ML > > This is v4 patch-set. > > Current ASoC is supporting CPU/Codec = N:M (N < M) connection by using > ch_map idea. This patch-set expands it that all connection uses this idea, > and no longer N < M limit [1]. > > Link: https://lore.kernel.org/r/87fs6wuszr.wl-kuninori.morimoto.gx@renesas.com [1] > > This patch is tested on Audio-Graph-Card2 with sample dtsi, > but needs Tested-by, at least from Intel. Checked for no regression on the Amlogic axg-card with DPCM and codec-to-codec links. Also checked no regression for multi-codec links with codecs doing playback only and capture-only on the same link. Looks good. Tested-by: Jerome Brunet <jbrunet@baylibre.com> Thanks for the notification Kuninori-san. > > v3 -> v4 > - add Jerome on To > - add "description" on "ch-maps" > > v2 -> v3 > - tidyup comment > - use more clear connection image on DT > - "ch_maps" -> "ch-maps" on DT > - Add DT maintainer on "To:" for all patches > > v1 -> v2 > - makes CPU/Codec connection relation clear on comment/sample > - fixup type "connction" -> "connection" > - makes error message clear > > Kuninori Morimoto (4): > ASoC: makes CPU/Codec channel connection map more generic > ASoC: audio-graph-card2: add CPU:Codec = N:M support > ASoC: audio-graph-card2-custom-sample: add CPU/Codec = N:M sample > dt-bindings: audio-graph-port: add ch-maps property > > .../bindings/sound/audio-graph-port.yaml | 8 +- > include/sound/soc.h | 66 ++++++++- > .../audio-graph-card2-custom-sample.dtsi | 138 +++++++++++++++--- > sound/soc/generic/audio-graph-card2.c | 29 ++++ > sound/soc/intel/boards/sof_sdw.c | 14 +- > sound/soc/soc-core.c | 85 +++++++++++ > sound/soc/soc-dapm.c | 47 +++--- > sound/soc/soc-pcm.c | 73 ++++----- > 8 files changed, 368 insertions(+), 92 deletions(-)
Hi Jerome > > This is v4 patch-set. > > > > Current ASoC is supporting CPU/Codec = N:M (N < M) connection by using > > ch_map idea. This patch-set expands it that all connection uses this idea, > > and no longer N < M limit [1]. (snip) > Checked for no regression on the Amlogic axg-card with DPCM and codec-to-codec > links. Also checked no regression for multi-codec links with codecs > doing playback only and capture-only on the same link. Thank you for your test ! Best regards --- Kuninori Morimoto
Hi Pierre-Louis Thank you for your test. > > /* > > * construct cpu channel mask by combining ch_mask of each > > * codec which maps to the cpu. > > + * see > > + * soc.h :: [dai_link->ch_maps Image sample] > > */ > > - for_each_rtd_codec_dais(rtd, j, codec_dai) { > > - if (rtd->dai_link->codec_ch_maps[j].connected_cpu_id == i) > > - ch_mask |= rtd->dai_link->codec_ch_maps[j].ch_mask; > > + if (rtd->dai_link->num_cpus >= rtd->dai_link->num_codecs) { > > + /* .ch_map is from CPU */ > > + ch_mask = rtd->dai_link->ch_maps[i].ch_mask; > > ... and for a FE dailink there's no ch_maps so this results in a kernel > oops. Hmm... this is strange... New snd_soc_compensate_connection_map() will add default ch_maps for all dai_link... Oh, is it using topology or something which doesn't call snd_soc_bind_card() ? If so could you please try to call snd_soc_compensate_connection_map() ? (I guess it is using soc_tplg_fe_link_create() ?) If it could solve your issue, v5 will handle it. Thank you for your help !! Best regards --- Kuninori Morimoto
On 10/17/23 18:03, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for your test. > >>> /* >>> * construct cpu channel mask by combining ch_mask of each >>> * codec which maps to the cpu. >>> + * see >>> + * soc.h :: [dai_link->ch_maps Image sample] >>> */ >>> - for_each_rtd_codec_dais(rtd, j, codec_dai) { >>> - if (rtd->dai_link->codec_ch_maps[j].connected_cpu_id == i) >>> - ch_mask |= rtd->dai_link->codec_ch_maps[j].ch_mask; >>> + if (rtd->dai_link->num_cpus >= rtd->dai_link->num_codecs) { >>> + /* .ch_map is from CPU */ >>> + ch_mask = rtd->dai_link->ch_maps[i].ch_mask; >> >> ... and for a FE dailink there's no ch_maps so this results in a kernel >> oops. > > Hmm... this is strange... > > New snd_soc_compensate_connection_map() will add default ch_maps for all > dai_link... > > Oh, is it using topology or something which doesn't call > snd_soc_bind_card() ? If so could you please try to call > snd_soc_compensate_connection_map() ? > (I guess it is using soc_tplg_fe_link_create() ?) > > If it could solve your issue, v5 will handle it. Sorry, not following what the suggestion is. Yes all our solutions are based on the topology, and I don't really understand the benefit of a ch_map for an FE? the codec_dai is a dummy one...
Hi Pierre-Louis Thank you for your feedback > > Oh, is it using topology or something which doesn't call > > snd_soc_bind_card() ? If so could you please try to call > > snd_soc_compensate_connection_map() ? > > (I guess it is using soc_tplg_fe_link_create() ?) > > > > If it could solve your issue, v5 will handle it. > > Sorry, not following what the suggestion is. > > Yes all our solutions are based on the topology, and I don't really > understand the benefit of a ch_map for an FE? the codec_dai is a dummy > one... This is my personal opinion, but the code can be simple if all case can be handled in the same way. For example this case, we don't need to care about FE or if (!ch_maps) if all dai_link has ch_maps. Complex case/pattern can be bug entry, IMO. ASoC is already very complex system... And because Codec is dummy, there is no effect for FE if all case handles same way. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis, Bard > > Yes all our solutions are based on the topology, and I don't really > > understand the benefit of a ch_map for an FE? the codec_dai is a dummy > > one... > > This is my personal opinion, but the code can be simple > if all case can be handled in the same way. > > For example this case, we don't need to care about FE or if (!ch_maps) > if all dai_link has ch_maps. Complex case/pattern can be bug entry, IMO. > ASoC is already very complex system... > > And because Codec is dummy, there is no effect for FE if all case > handles same way. I will post renew [1/4] patch after this patch (not yet v5). Could you please test it ? If my understanding was correct, it should work correctly on your topology. Thank you for your help !! Best regards --- Kuninori Morimoto
On 10/18/23 21:04, Kuninori Morimoto wrote: > Current ASoC CPU:Codec = N:M connection is using connection mapping idea, > but it is used for CPU < Codec case only. We want to use it for any case. > > By this patch, not only N:M connection, but all existing connection > (1:1, 1:N, N:N) will use same connection mapping. > Because it will use default mapping, no conversion patch is needed > to exising CPU/Codec drivers. > > More over, CPU:Codec = M:N (M > N) also supported in the same time. > > Link: https://lore.kernel.org/r/87fs6wuszr.wl-kuninori.morimoto.gx@renesas.com > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> No issues detected with this patch by the Intel CI (other than the usual suspend-resume timeouts that have nothing to do with this patch), see https://github.com/thesofproject/linux/pull/4632 Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Thanks Morimoto-san! > + 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; > } > + } > + /* .ch_map is from Codec */ > + else { > + for_each_rtd_codec_dais(rtd, i, codec_dai) { > + cpu_dai = snd_soc_rtd_to_cpu(rtd, dai_link->ch_maps[i].connected_node); > + > + 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)) while we're at it, can we also clean-up the weird extra spaces - unless they were intentional? > + 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 = 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; > + } > } > } >
Hi Pierre-Louis > No issues detected with this patch by the Intel CI (other than the usual > suspend-resume timeouts that have nothing to do with this patch), see (snip) > Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Thank you for your test Best regards --- Kuninori Morimoto