| Message ID | 20230607031242.1032060-2-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ac950278b0872c87bcef6153fd9c119265c8ba83 |
| Headers | show |
| Series | ASoC: add N cpus to M codecs dai link support | expand |
Hi Bard > Currently, ASoC supports dailinks with the following mappings: > 1 cpu DAI to N codec DAIs > N cpu DAIs to N codec DAIs > But the mapping between N cpu DAIs and M codec DAIs is not supported. > The reason is that we didn't have a mechanism to map cpu and codec DAIs > > This patch suggests a new snd_soc_dai_link_codec_ch_map struct in > struct snd_soc_dai_link{} which provides codec DAI to cpu DAI mapping > information used to implement N cpu DAIs to M codec DAIs > support. > > When a dailink contains two or more cpu DAIs, we should set channel > number of cpus based on its channel mask. The new struct also provides > channel mask information for each codec and we can construct the cpu > channel mask by combining all codec channel masks which map to the cpu. > > The N:M mapping is however restricted to the N <= M case due to physical > restrictions on a time-multiplexed bus such as I2S/TDM, AC97, SoundWire > and HDaudio. I like CPU:Codec = N:M support ! OTOH, I have interesting to ASoC code cleanup too. So this is meddlesome from me. > +struct snd_soc_dai_link_codec_ch_map { > + unsigned int connected_cpu_id; > + unsigned int ch_mask; > +}; If my understanding was correct, this map is used only for N:M mapping, but we want to use it for all cases. In such case, the code can be more flexible and more clean. see below. > @@ -1041,13 +1045,32 @@ static int __soc_pcm_hw_params(struct snd_soc_pcm_runtime *rtd, > if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) > continue; > > - ret = snd_soc_dai_hw_params(cpu_dai, substream, params); > + /* copy params for each cpu */ > + cpu_params = *params; > + > + if (!rtd->dai_link->codec_ch_maps) > + goto hw_params; > + /* > + * construct cpu channel mask by combining ch_mask of each > + * codec which maps to the cpu. > + */ > + 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; > + } Can we re-use snd_soc_dai_tdm_mask_get() for this purpose instead of .ch_mask ? May be we want to rename it and/or want to have new xxx_mask on dai->stream[]. I'm asking because it is natural to reuse existing data and/or have variable on similar place instead of on new structure. Maybe I'm not 100% well understanding about CPU:Codec = N:M connection, but if we can assume like below, we can use it on all cases not only for N:M case. We can have connection map on dai_link which is for from M side (here N <= M). The image is like this. CPU0 <---> Codec2 CPU1 <-+-> Codec0 \-> Codec1 .num_cpus = 2; .num_codecs = 3; .map = [1, 1, 0]; // From Codec point of view. // sizeof(map) = num_codecs, because 3 > 2; In this rule, we can also handle CPU > Codec case, too. CPU0 <---> Codec1 CPU1 <-+-> Codec0 CPU2 <-/ .num_cpus = 3; .num_codecs = 2; .map = [1, 0, 0]; // From CPU point of view. We can use this idea for existing connection, too. 1:1 case CPU0 <-> Codec0 N:N case CPU0 <---> Codec0 CPU1 <---> Codec1 1:N case CPU0 <-+-> Codec0 \-> Codec1 default_map1 = [0, 1, 2, 3,...]; default_map2 = [0, 0, 0, 0,...]; if (!dai_link->map) { if (dai_link->num_cpus == dai_link->num_codecs) dai_link->map = default_map1; /* for 1:1, N:N */ else dai_link->map = default_map2; /* for 1:N */ } We can handle more flexible N:N connection as Richard said fixup N:N case CPU0 <---> Codec2 CPU1 <---> Codec1 CPU2 <---> Codec0 .num_cpus = 3; .num_codecs = 3; .map = [2, 1, 0]; // From CPU point of view. with is new .map, we can handle existing 1:1, N:N, 1:N, and new N:M (and M:N) connection with same code. This is just meddlesome from me, but I hope you can think about this. Thank you for your help !! Best regards --- Kuninori Morimoto
> -----Original Message----- > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Sent: Tuesday, June 13, 2023 8:05 AM > To: Bard Liao <yung-chuan.liao@linux.intel.com> > Cc: broonie@kernel.org; tiwai@suse.de; alsa-devel@alsa-project.org; pierre- > louis.bossart@linux.intel.com; Liao, Bard <bard.liao@intel.com> > Subject: Re: [PATCH 1/2] ASoC: add N cpus to M codecs dai link support > > > Hi Bard > > > Currently, ASoC supports dailinks with the following mappings: > > 1 cpu DAI to N codec DAIs > > N cpu DAIs to N codec DAIs > > But the mapping between N cpu DAIs and M codec DAIs is not supported. > > The reason is that we didn't have a mechanism to map cpu and codec DAIs > > > > This patch suggests a new snd_soc_dai_link_codec_ch_map struct in > > struct snd_soc_dai_link{} which provides codec DAI to cpu DAI mapping > > information used to implement N cpu DAIs to M codec DAIs > > support. > > > > When a dailink contains two or more cpu DAIs, we should set channel > > number of cpus based on its channel mask. The new struct also provides > > channel mask information for each codec and we can construct the cpu > > channel mask by combining all codec channel masks which map to the cpu. > > > > The N:M mapping is however restricted to the N <= M case due to physical > > restrictions on a time-multiplexed bus such as I2S/TDM, AC97, SoundWire > > and HDaudio. > > I like CPU:Codec = N:M support ! > OTOH, I have interesting to ASoC code cleanup too. > So this is meddlesome from me. > > > +struct snd_soc_dai_link_codec_ch_map { > > + unsigned int connected_cpu_id; > > + unsigned int ch_mask; > > +}; > > If my understanding was correct, this map is used only for N:M mapping, > but we want to use it for all cases. Thanks Morimoto, I hope so, too. > In such case, the code can be more flexible and more clean. > see below. > > > @@ -1041,13 +1045,32 @@ static int __soc_pcm_hw_params(struct > snd_soc_pcm_runtime *rtd, > > if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) > > continue; > > > > - ret = snd_soc_dai_hw_params(cpu_dai, substream, params); > > + /* copy params for each cpu */ > > + cpu_params = *params; > > + > > + if (!rtd->dai_link->codec_ch_maps) > > + goto hw_params; > > + /* > > + * construct cpu channel mask by combining ch_mask of each > > + * codec which maps to the cpu. > > + */ > > + 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; > > + } > > Can we re-use snd_soc_dai_tdm_mask_get() for this purpose instead > of .ch_mask ? > May be we want to rename it and/or want to have new xxx_mask on dai- > >stream[]. The reason that we didn't use tdm_mask is because the N:M cases is not a notion of tdm. But, it should work for the N:M cases if we rename it and add a new map as you said. > I'm asking because it is natural to reuse existing data and/or have variable on > similar place instead of on new structure. > > > Maybe I'm not 100% well understanding about CPU:Codec = N:M connection, > but if we can assume like below, we can use it on all cases not only for N:M > case. > > We can have connection map on dai_link which is for from M side (here N <= > M). > The image is like this. > > CPU0 <---> Codec2 > CPU1 <-+-> Codec0 > \-> Codec1 > > .num_cpus = 2; > .num_codecs = 3; > .map = [1, 1, 0]; // From Codec point of view. > // sizeof(map) = num_codecs, because 3 > 2; > > In this rule, we can also handle CPU > Codec case, too. > > CPU0 <---> Codec1 > CPU1 <-+-> Codec0 > CPU2 <-/ > > .num_cpus = 3; > .num_codecs = 2; > .map = [1, 0, 0]; // From CPU point of view. > > We can use this idea for existing connection, too. > > 1:1 case > CPU0 <-> Codec0 > > N:N case > CPU0 <---> Codec0 > CPU1 <---> Codec1 > > 1:N case > CPU0 <-+-> Codec0 > \-> Codec1 > > default_map1 = [0, 1, 2, 3,...]; > default_map2 = [0, 0, 0, 0,...]; > > if (!dai_link->map) { > if (dai_link->num_cpus == dai_link->num_codecs) > dai_link->map = default_map1; /* for 1:1, N:N */ > else > dai_link->map = default_map2; /* for 1:N */ > } > > We can handle more flexible N:N connection as Richard said > > fixup N:N case > CPU0 <---> Codec2 > CPU1 <---> Codec1 > CPU2 <---> Codec0 > > .num_cpus = 3; > .num_codecs = 3; > .map = [2, 1, 0]; // From CPU point of view. > > with is new .map, we can handle existing 1:1, N:N, 1:N, and new N:M (and > M:N) > connection with same code. > This is just meddlesome from me, but I hope you can think about this. Thanks for the suggestion. I will think about it. > > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
diff --git a/include/sound/soc.h b/include/sound/soc.h index 533e553a343f..a5171b0de2fd 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -646,6 +646,11 @@ struct snd_soc_dai_link_component { const char *dai_name; }; +struct snd_soc_dai_link_codec_ch_map { + unsigned int connected_cpu_id; + unsigned int ch_mask; +}; + struct snd_soc_dai_link { /* config - must be set by machine driver */ const char *name; /* Codec name */ @@ -674,6 +679,7 @@ struct snd_soc_dai_link { struct snd_soc_dai_link_component *codecs; unsigned int num_codecs; + struct snd_soc_dai_link_codec_ch_map *codec_ch_maps; /* * You MAY specify the link's platform/PCM/DMA driver, either by * device name, or by DT/OF node, but not both. Some forms of link diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c65cc374bb3f..092a5cf20ddb 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -4477,9 +4477,31 @@ void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card) for_each_rtd_codec_dais(rtd, i, codec_dai) dapm_connect_dai_pair(card, rtd, codec_dai, asoc_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(card->dev, "%s: no codec channel mapping table provided\n", + __func__); + continue; + } + + for_each_rtd_codec_dais(rtd, i, codec_dai) { + cpu_id = rtd->dai_link->codec_ch_maps[i].connected_cpu_id; + if (cpu_id >= rtd->dai_link->num_cpus) { + dev_err(card->dev, + "%s: dai_link %s cpu_id %d too large, num_cpus is %d\n", + __func__, rtd->dai_link->name, cpu_id, + rtd->dai_link->num_cpus); + continue; + } + dapm_connect_dai_pair(card, rtd, codec_dai, + asoc_rtd_to_cpu(rtd, cpu_id)); + } } else { dev_err(card->dev, - "N cpus to M codecs link is not supported yet\n"); + "%s: codec number %d < cpu number %d is not supported\n", + __func__, rtd->dai_link->num_codecs, rtd->dai_link->num_cpus); } } } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index c13552fefbe6..88c4f3d77ade 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1034,6 +1034,10 @@ static int __soc_pcm_hw_params(struct snd_soc_pcm_runtime *rtd, } for_each_rtd_cpu_dais(rtd, i, cpu_dai) { + struct snd_pcm_hw_params cpu_params; + unsigned int ch_mask = 0; + int j; + /* * Skip CPUs which don't support the current stream * type. See soc_pcm_init_runtime_hw() for more details @@ -1041,13 +1045,32 @@ static int __soc_pcm_hw_params(struct snd_soc_pcm_runtime *rtd, if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) continue; - ret = snd_soc_dai_hw_params(cpu_dai, substream, params); + /* copy params for each cpu */ + cpu_params = *params; + + if (!rtd->dai_link->codec_ch_maps) + goto hw_params; + /* + * construct cpu channel mask by combining ch_mask of each + * codec which maps to the cpu. + */ + 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; + } + + /* fixup cpu channel number */ + if (ch_mask) + soc_pcm_codec_params_fixup(&cpu_params, ch_mask); + +hw_params: + ret = snd_soc_dai_hw_params(cpu_dai, substream, &cpu_params); if (ret < 0) goto out; /* store the parameters for each DAI */ - soc_pcm_set_dai_params(cpu_dai, params); - snd_soc_dapm_update_dai(substream, params, cpu_dai); + soc_pcm_set_dai_params(cpu_dai, &cpu_params); + snd_soc_dapm_update_dai(substream, &cpu_params, cpu_dai); } ret = snd_soc_pcm_component_hw_params(substream, params); @@ -2794,9 +2817,22 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, cpu_dai = asoc_rtd_to_cpu(rtd, 0); } else if (dai_link->num_cpus == dai_link->num_codecs) { cpu_dai = asoc_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; + } + + cpu_id = rtd->dai_link->codec_ch_maps[i].connected_cpu_id; + cpu_dai = asoc_rtd_to_cpu(rtd, cpu_id); } else { dev_err(rtd->card->dev, - "N cpus to M codecs link is not supported yet\n"); + "%s codec number %d < cpu number %d is not supported\n", + __func__, rtd->dai_link->num_codecs, + rtd->dai_link->num_cpus); return -EINVAL; }
Currently, ASoC supports dailinks with the following mappings: 1 cpu DAI to N codec DAIs N cpu DAIs to N codec DAIs But the mapping between N cpu DAIs and M codec DAIs is not supported. The reason is that we didn't have a mechanism to map cpu and codec DAIs This patch suggests a new snd_soc_dai_link_codec_ch_map struct in struct snd_soc_dai_link{} which provides codec DAI to cpu DAI mapping information used to implement N cpu DAIs to M codec DAIs support. When a dailink contains two or more cpu DAIs, we should set channel number of cpus based on its channel mask. The new struct also provides channel mask information for each codec and we can construct the cpu channel mask by combining all codec channel masks which map to the cpu. The N:M mapping is however restricted to the N <= M case due to physical restrictions on a time-multiplexed bus such as I2S/TDM, AC97, SoundWire and HDaudio. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> --- include/sound/soc.h | 6 ++++++ sound/soc/soc-dapm.c | 24 +++++++++++++++++++++++- sound/soc/soc-pcm.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 69 insertions(+), 5 deletions(-)