[PATCH 0/2] ASoC: add N cpus to M codecs dai link support
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 series 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.
And add the codec_ch_maps to SOF SoundWire machine driver.
Bard Liao (2): ASoC: add N cpus to M codecs dai link support ASoC: Intel: sof_sdw: add dai_link_codec_ch_map
include/sound/soc.h | 6 +++ sound/soc/intel/boards/sof_sdw.c | 69 +++++++++++++++++++++++++ sound/soc/intel/boards/sof_sdw_common.h | 2 + sound/soc/intel/boards/sof_sdw_maxim.c | 1 + sound/soc/soc-dapm.c | 24 ++++++++- sound/soc/soc-pcm.c | 44 ++++++++++++++-- 6 files changed, 141 insertions(+), 5 deletions(-)
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(-)
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; }
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
The captured data will be combined from each cpu DAI if the dai link has more than one cpu DAIs. We can set channel number indirectly by adding sdw_codec_ch_maps.
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 --- sound/soc/intel/boards/sof_sdw.c | 69 +++++++++++++++++++++++++ sound/soc/intel/boards/sof_sdw_common.h | 2 + sound/soc/intel/boards/sof_sdw_maxim.c | 1 + 3 files changed, 72 insertions(+)
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c index d942696b36cd..f2f56150e1da 100644 --- a/sound/soc/intel/boards/sof_sdw.c +++ b/sound/soc/intel/boards/sof_sdw.c @@ -560,6 +560,55 @@ int sdw_trigger(struct snd_pcm_substream *substream, int cmd) return ret; }
+int sdw_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + int ch = params_channels(params); + struct snd_soc_dai *codec_dai; + struct snd_soc_dai *cpu_dai; + unsigned int ch_mask; + int num_codecs; + int step; + int i; + int j; + + if (!rtd->dai_link->codec_ch_maps) + return 0; + + /* Identical data will be sent to all codecs in playback */ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + ch_mask = GENMASK(ch - 1, 0); + step = 0; + } else { + num_codecs = rtd->dai_link->num_codecs; + + if (ch < num_codecs || ch % num_codecs != 0) { + dev_err(rtd->dev, "Channels number %d is invalid when codec number = %d\n", + ch, num_codecs); + return -EINVAL; + } + + ch_mask = GENMASK(ch / num_codecs - 1, 0); + step = hweight_long(ch_mask); + + } + + /* + * The captured data will be combined from each cpu DAI if the dai + * link has more than one codec DAIs. Set codec channel mask and + * ASoC will set the corresponding channel numbers for each cpu dai. + */ + for_each_rtd_cpu_dais(rtd, i, cpu_dai) { + for_each_rtd_codec_dais(rtd, j, codec_dai) { + if (rtd->dai_link->codec_ch_maps[j].connected_cpu_id != i) + continue; + rtd->dai_link->codec_ch_maps[j].ch_mask = ch_mask << (j * step); + } + } + return 0; +} + int sdw_hw_free(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); @@ -588,6 +637,7 @@ static const struct snd_soc_ops sdw_ops = { .startup = sdw_startup, .prepare = sdw_prepare, .trigger = sdw_trigger, + .hw_params = sdw_hw_params, .hw_free = sdw_hw_free, .shutdown = sdw_shutdown, }; @@ -1281,6 +1331,17 @@ static int get_slave_info(const struct snd_soc_acpi_link_adr *adr_link, return 0; }
+static void set_dailink_map(struct snd_soc_dai_link_codec_ch_map *sdw_codec_ch_maps, + int codec_num, int cpu_num) +{ + int step; + int i; + + step = codec_num / cpu_num; + for (i = 0; i < codec_num; i++) + sdw_codec_ch_maps[i].connected_cpu_id = i / step; +} + static const char * const type_strings[] = {"SimpleJack", "SmartAmp", "SmartMic"};
static int create_sdw_dailink(struct snd_soc_card *card, @@ -1357,6 +1418,7 @@ static int create_sdw_dailink(struct snd_soc_card *card,
cpu_dai_index = *cpu_id; for_each_pcm_streams(stream) { + struct snd_soc_dai_link_codec_ch_map *sdw_codec_ch_maps; char *name, *cpu_name; int playback, capture; static const char * const sdw_stream_name[] = { @@ -1375,6 +1437,11 @@ static int create_sdw_dailink(struct snd_soc_card *card, return -EINVAL; }
+ sdw_codec_ch_maps = devm_kcalloc(dev, codec_num, + sizeof(*sdw_codec_ch_maps), GFP_KERNEL); + if (!sdw_codec_ch_maps) + return -ENOMEM; + /* create stream name according to first link id */ if (append_dai_type) { name = devm_kasprintf(dev, GFP_KERNEL, @@ -1435,6 +1502,8 @@ static int create_sdw_dailink(struct snd_soc_card *card, */ dai_links[*link_index].nonatomic = true;
+ set_dailink_map(sdw_codec_ch_maps, codec_num, cpu_dai_num); + dai_links[*link_index].codec_ch_maps = sdw_codec_ch_maps; ret = set_codec_init_func(card, link, dai_links + (*link_index)++, playback, group_id, adr_index, dai_index); if (ret < 0) { diff --git a/sound/soc/intel/boards/sof_sdw_common.h b/sound/soc/intel/boards/sof_sdw_common.h index 64cfa5d1aceb..37402170d5f9 100644 --- a/sound/soc/intel/boards/sof_sdw_common.h +++ b/sound/soc/intel/boards/sof_sdw_common.h @@ -103,6 +103,8 @@ extern unsigned long sof_sdw_quirk; int sdw_startup(struct snd_pcm_substream *substream); int sdw_prepare(struct snd_pcm_substream *substream); int sdw_trigger(struct snd_pcm_substream *substream, int cmd); +int sdw_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params); int sdw_hw_free(struct snd_pcm_substream *substream); void sdw_shutdown(struct snd_pcm_substream *substream);
diff --git a/sound/soc/intel/boards/sof_sdw_maxim.c b/sound/soc/intel/boards/sof_sdw_maxim.c index 8d40a83ad98e..414c4d8dac77 100644 --- a/sound/soc/intel/boards/sof_sdw_maxim.c +++ b/sound/soc/intel/boards/sof_sdw_maxim.c @@ -123,6 +123,7 @@ static const struct snd_soc_ops max_98373_sdw_ops = { .startup = sdw_startup, .prepare = mx8373_sdw_prepare, .trigger = sdw_trigger, + .hw_params = sdw_hw_params, .hw_free = mx8373_sdw_hw_free, .shutdown = sdw_shutdown, };
On 07/06/2023 04:12, Bard Liao wrote:
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 series 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.
And add the codec_ch_maps to SOF SoundWire machine driver.
I think there is a much simpler way to handle this. The existing ASoC code is trying to map CPU to CODEC to match the physical connection. But the physical connection is not important. The dailink represents the _logical_ link.
You are declaring that all the CPU and CODEC in the dailink behave as a single logical link. So you can just connect all CPUs to all CODECS.
That also fixes a problem with the existing N CPU to N CODEC mapping. It assumes that means CPU0 is connected to CODEC0, CPU1 is connected to CODEC1, ... But that isn't necessarily true. You could have an N:N mapping where multiple CPUs have been combined to create a multi-channel stream that is sent to all CODECs. But the existing N:N code won't hook that up correctly.
I suggest that the simple fix is this:
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 5d9a671e50f1..f4f955a991d5 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -4423,8 +4423,8 @@ static void soc_dapm_dai_stream_event(struct snd_soc_dai *dai, int stream, void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card) { struct snd_soc_pcm_runtime *rtd; - struct snd_soc_dai *codec_dai; - int i; + struct snd_soc_dai *codec_dai, *cpu_dai; + int i, j;
/* for each BE DAI link... */ for_each_card_rtds(card, rtd) { @@ -4435,17 +4435,9 @@ void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card) if (rtd->dai_link->dynamic) continue;
- if (rtd->dai_link->num_cpus == 1) { - for_each_rtd_codec_dais(rtd, i, codec_dai) - dapm_connect_dai_pair(card, rtd, codec_dai, - asoc_rtd_to_cpu(rtd, 0)); - } else if (rtd->dai_link->num_codecs == rtd->dai_link->num_cpus) { - for_each_rtd_codec_dais(rtd, i, codec_dai) - dapm_connect_dai_pair(card, rtd, codec_dai, - asoc_rtd_to_cpu(rtd, i)); - } else { - dev_err(card->dev, - "N cpus to M codecs link is not supported yet\n"); + for_each_rtd_codec_dais(rtd, i, codec_dai) { + for_each_rtd_cpu_dais(rtd, j, cpu_dai) + dapm_connect_dai_pair(card, rtd, codec_dai, cpu_dai); } } } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7958c9defd49..43b1166eb333 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2747,7 +2747,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, int *playback, int *capture) { struct snd_soc_dai *cpu_dai; - int i; + int i, j;
if (rtd->dai_link->dynamic && rtd->dai_link->num_cpus > 1) { dev_err(rtd->dev, @@ -2801,22 +2801,14 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
for_each_rtd_codec_dais(rtd, i, codec_dai) { - if (rtd->dai_link->num_cpus == 1) { - cpu_dai = asoc_rtd_to_cpu(rtd, 0); - } else if (rtd->dai_link->num_cpus == rtd->dai_link->num_codecs) { - cpu_dai = asoc_rtd_to_cpu(rtd, i); - } else { - dev_err(rtd->card->dev, - "N cpus to M codecs link is not supported yet\n"); - return -EINVAL; + for_each_rtd_cpu_dais(rtd, j, cpu_dai) { + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && + snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) + *playback = 1; + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && + snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) + *capture = 1; } - - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) - *playback = 1; - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) - *capture = 1; } }
On 6/7/23 04:29, Richard Fitzgerald wrote:
On 07/06/2023 04:12, Bard Liao wrote:
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 series 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.
And add the codec_ch_maps to SOF SoundWire machine driver.
I think there is a much simpler way to handle this. The existing ASoC code is trying to map CPU to CODEC to match the physical connection. But the physical connection is not important. The dailink represents the _logical_ link.
Humm, that's not really true. Each SoundWire link and the CPU DAI they expose are handled by different auxiliary devices with their own pm_runtime handling.
You are declaring that all the CPU and CODEC in the dailink behave as a single logical link. So you can just connect all CPUs to all CODECS.
That also fixes a problem with the existing N CPU to N CODEC mapping. It assumes that means CPU0 is connected to CODEC0, CPU1 is connected to CODEC1, ... But that isn't necessarily true. You could have an N:N mapping where multiple CPUs have been combined to create a multi-channel stream that is sent to all CODECs.
This is questionable when the CPUs are connected to different links. SoundWire is not a giant switch matrix, there's a clear parent-child dependency and limited scope.
Example topology for a 2 link/4 amplifier solution.
link1 CPU DAI1 - Codec1 DAI1 - Codec2 DAI1 link2 CPU DAI1 - Codec1 DAI1 - Codec2 DAI1
There is no physical or logical connection between link2 CPU DAI1 and the two Codec1 DAI1 and Codec2 DAI1.
I don't think it's wise to make DAPM connections between devices that are not on the same link. Each link is clock- and powered-managed separately, I only see trouble ahead with such virtual connections.
But the existing N:N code won't hook
that up correctly.
I suggest that the simple fix is this:
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 5d9a671e50f1..f4f955a991d5 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -4423,8 +4423,8 @@ static void soc_dapm_dai_stream_event(struct snd_soc_dai *dai, int stream, void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card) { struct snd_soc_pcm_runtime *rtd; - struct snd_soc_dai *codec_dai; - int i; + struct snd_soc_dai *codec_dai, *cpu_dai; + int i, j;
/* for each BE DAI link... */ for_each_card_rtds(card, rtd) { @@ -4435,17 +4435,9 @@ void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card) if (rtd->dai_link->dynamic) continue;
- if (rtd->dai_link->num_cpus == 1) { - for_each_rtd_codec_dais(rtd, i, codec_dai) - dapm_connect_dai_pair(card, rtd, codec_dai, - asoc_rtd_to_cpu(rtd, 0)); - } else if (rtd->dai_link->num_codecs == rtd->dai_link->num_cpus) { - for_each_rtd_codec_dais(rtd, i, codec_dai) - dapm_connect_dai_pair(card, rtd, codec_dai, - asoc_rtd_to_cpu(rtd, i)); - } else { - dev_err(card->dev, - "N cpus to M codecs link is not supported yet\n"); + for_each_rtd_codec_dais(rtd, i, codec_dai) { + for_each_rtd_cpu_dais(rtd, j, cpu_dai) + dapm_connect_dai_pair(card, rtd, codec_dai, cpu_dai); } } } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7958c9defd49..43b1166eb333 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2747,7 +2747,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, int *playback, int *capture) { struct snd_soc_dai *cpu_dai; - int i; + int i, j;
if (rtd->dai_link->dynamic && rtd->dai_link->num_cpus > 1) { dev_err(rtd->dev, @@ -2801,22 +2801,14 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
for_each_rtd_codec_dais(rtd, i, codec_dai) { - if (rtd->dai_link->num_cpus == 1) { - cpu_dai = asoc_rtd_to_cpu(rtd, 0); - } else if (rtd->dai_link->num_cpus == rtd->dai_link->num_codecs) { - cpu_dai = asoc_rtd_to_cpu(rtd, i); - } else { - dev_err(rtd->card->dev, - "N cpus to M codecs link is not supported yet\n"); - return -EINVAL; + for_each_rtd_cpu_dais(rtd, j, cpu_dai) { + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && + snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) + *playback = 1; + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && + snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) + *capture = 1; }
- if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) - *playback = 1; - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) - *capture = 1; } }
On Wed, Jun 07, 2023 at 10:10:24AM -0500, Pierre-Louis Bossart wrote:
On 6/7/23 04:29, Richard Fitzgerald wrote:
On 07/06/2023 04:12, Bard Liao wrote:
You are declaring that all the CPU and CODEC in the dailink behave as a single logical link. So you can just connect all CPUs to all CODECS.
That also fixes a problem with the existing N CPU to N CODEC mapping. It assumes that means CPU0 is connected to CODEC0, CPU1 is connected to CODEC1, ... But that isn't necessarily true. You could have an N:N mapping where multiple CPUs have been combined to create a multi-channel stream that is sent to all CODECs.
This is questionable when the CPUs are connected to different links. SoundWire is not a giant switch matrix, there's a clear parent-child dependency and limited scope.
Example topology for a 2 link/4 amplifier solution.
Or a system with two distinct I2S DAIs (TDM is another thing).
On Wed, Jun 07, 2023 at 05:22:45PM +0100, Mark Brown wrote:
On Wed, Jun 07, 2023 at 10:10:24AM -0500, Pierre-Louis Bossart wrote:
On 6/7/23 04:29, Richard Fitzgerald wrote:
On 07/06/2023 04:12, Bard Liao wrote:
You are declaring that all the CPU and CODEC in the dailink behave as a single logical link. So you can just connect all CPUs to all CODECS.
That also fixes a problem with the existing N CPU to N CODEC mapping. It assumes that means CPU0 is connected to CODEC0, CPU1 is connected to CODEC1, ... But that isn't necessarily true. You could have an N:N mapping where multiple CPUs have been combined to create a multi-channel stream that is sent to all CODECs.
This is questionable when the CPUs are connected to different links. SoundWire is not a giant switch matrix, there's a clear parent-child dependency and limited scope.
Example topology for a 2 link/4 amplifier solution.
Or a system with two distinct I2S DAIs (TDM is another thing).
I guess the bit that slightly phases me here is, historically a DAI link has been the thing that specifies what is connected to what. What kinda happened when we added multi-cpu is we bent that assumption, at least for the N -> N case, and now even more so for the N -> M case, where only a subset of the DAI link is actually connected.
If your system looks like:
CPU A -> CODEC A CPU B -> CODEC B
What makes this a single DAI link, rather than 2 DAI links? And does the information within the DAI link about what is connected to what not just start looking like DAI links?
Thanks, Charles
On 6/7/23 12:05, Charles Keepax wrote:
On Wed, Jun 07, 2023 at 05:22:45PM +0100, Mark Brown wrote:
On Wed, Jun 07, 2023 at 10:10:24AM -0500, Pierre-Louis Bossart wrote:
On 6/7/23 04:29, Richard Fitzgerald wrote:
On 07/06/2023 04:12, Bard Liao wrote:
You are declaring that all the CPU and CODEC in the dailink behave as a single logical link. So you can just connect all CPUs to all CODECS.
That also fixes a problem with the existing N CPU to N CODEC mapping. It assumes that means CPU0 is connected to CODEC0, CPU1 is connected to CODEC1, ... But that isn't necessarily true. You could have an N:N mapping where multiple CPUs have been combined to create a multi-channel stream that is sent to all CODECs.
This is questionable when the CPUs are connected to different links. SoundWire is not a giant switch matrix, there's a clear parent-child dependency and limited scope.
Example topology for a 2 link/4 amplifier solution.
Or a system with two distinct I2S DAIs (TDM is another thing).
I guess the bit that slightly phases me here is, historically a DAI link has been the thing that specifies what is connected to what. What kinda happened when we added multi-cpu is we bent that assumption, at least for the N -> N case, and now even more so for the N -> M case, where only a subset of the DAI link is actually connected.
If your system looks like:
CPU A -> CODEC A CPU B -> CODEC B
What makes this a single DAI link, rather than 2 DAI links? And does the information within the DAI link about what is connected to what not just start looking like DAI links?
Synchronized starts/operation is the big difference IMHO. There's one TRIGGER_START, not two.
On Wed, Jun 07, 2023 at 05:05:20PM +0000, Charles Keepax wrote:
On Wed, Jun 07, 2023 at 05:22:45PM +0100, Mark Brown wrote:
This is questionable when the CPUs are connected to different links. SoundWire is not a giant switch matrix, there's a clear parent-child dependency and limited scope.
Example topology for a 2 link/4 amplifier solution.
Or a system with two distinct I2S DAIs (TDM is another thing).
I guess the bit that slightly phases me here is, historically a DAI link has been the thing that specifies what is connected to what. What kinda happened when we added multi-cpu is we bent that assumption, at least for the N -> N case, and now even more so for the N -> M case, where only a subset of the DAI link is actually connected.
If your system looks like:
CPU A -> CODEC A CPU B -> CODEC B
What makes this a single DAI link, rather than 2 DAI links? And does the information within the DAI link about what is connected to what not just start looking like DAI links?
Ah, indeed. My expectation would be that for things on the same physical set of wires we'd at some point be able to get to a point where the the SoundWire routing would be exposed to userspace for control, probably at the point where we get digital routing working (whenever in the far far future that might be, it's only been a bit more than a decade thus far). I have to say Pierre's example looked like two separate buses.
On 6/7/23 12:18, Mark Brown wrote:
On Wed, Jun 07, 2023 at 05:05:20PM +0000, Charles Keepax wrote:
On Wed, Jun 07, 2023 at 05:22:45PM +0100, Mark Brown wrote:
This is questionable when the CPUs are connected to different links. SoundWire is not a giant switch matrix, there's a clear parent-child dependency and limited scope.
Example topology for a 2 link/4 amplifier solution.
Or a system with two distinct I2S DAIs (TDM is another thing).
I guess the bit that slightly phases me here is, historically a DAI link has been the thing that specifies what is connected to what. What kinda happened when we added multi-cpu is we bent that assumption, at least for the N -> N case, and now even more so for the N -> M case, where only a subset of the DAI link is actually connected.
If your system looks like:
CPU A -> CODEC A CPU B -> CODEC B
What makes this a single DAI link, rather than 2 DAI links? And does the information within the DAI link about what is connected to what not just start looking like DAI links?
Ah, indeed. My expectation would be that for things on the same physical set of wires we'd at some point be able to get to a point where the the SoundWire routing would be exposed to userspace for control, probably at the point where we get digital routing working (whenever in the far far future that might be, it's only been a bit more than a decade thus far). I have to say Pierre's example looked like two separate buses.
They are separate buses indeed at the hardware level, and even on the Linux side we have one manager device per link.
The key point is that all buses are synchronized with a common hardware signal, typically 4kHz, on the SOC/PCH side.
The dailink .trigger is translated as a multi-link bank switch which will start transmission exactly at the same time on all links.
That's the big difference with using multiple dailinks, if we had one dailink per physical pair of (clock, data) wires, we would not be able to synchronize amplifiers, leading to random phase offsets between amplifiers. Not so good.
On Wed, Jun 07, 2023 at 01:13:49PM -0500, Pierre-Louis Bossart wrote:
On 6/7/23 12:18, Mark Brown wrote:
On Wed, Jun 07, 2023 at 05:05:20PM +0000, Charles Keepax wrote:
On Wed, Jun 07, 2023 at 05:22:45PM +0100, Mark Brown wrote: CPU A -> CODEC A CPU B -> CODEC B
What makes this a single DAI link, rather than 2 DAI links? And does the information within the DAI link about what is connected to what not just start looking like DAI links?
Ah, indeed. My expectation would be that for things on the same physical set of wires we'd at some point be able to get to a point where the the SoundWire routing would be exposed to userspace for control, probably at the point where we get digital routing working (whenever in the far far future that might be, it's only been a bit more than a decade thus far). I have to say Pierre's example looked like two separate buses.
They are separate buses indeed at the hardware level, and even on the Linux side we have one manager device per link.
The key point is that all buses are synchronized with a common hardware signal, typically 4kHz, on the SOC/PCH side.
The dailink .trigger is translated as a multi-link bank switch which will start transmission exactly at the same time on all links.
That's the big difference with using multiple dailinks, if we had one dailink per physical pair of (clock, data) wires, we would not be able to synchronize amplifiers, leading to random phase offsets between amplifiers. Not so good.
Indeed, not so good. I had a chat with Richard and we were wonder if this is really one of those we have started down a path and it's a bit late to change course now situations. I don't think either of us have a great objection to the within the DAI link hook up table really, just hard to get my head around what a DAI link means in that case. I guess if I just think of DAI links as being more a logical grouping, that is being treated as a single stream, rather than representing physical links?
To provide the other side, in my head, where most things are C2C links rather than DPCM, the situation really looks something like this:
+----------+ +---------+ + SDW A + <-> + CODEC A + +-----+ + + +---------+ + CPU + <-> + DSP + +-----+ + + +---------+ + SDW B + <-> + CODEC B + +----------+ +---------+
And the responsibility for starting the bank switch would lie with the DSP driver. It gets a single trigger from its DAI to the CPU, which provides the sync point. But this seems fairly removed from how things are presently implemented and it perhaps feels like the effort to get there isn't worth it, especially since me and Richard are unlikely to have the time in the near term to do a lot of it.
Thanks, Charles
On Wed, 07 Jun 2023 11:12:40 +0800, Bard Liao wrote:
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 series 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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: add N cpus to M codecs dai link support commit: e8181a895b05b264883288c4043ff83f893b56b0 [2/2] ASoC: Intel: sof_sdw: add dai_link_codec_ch_map commit: 0281b02e1913a9443ce891dcc13613829e4dc3c5
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (7)
-
Bard Liao -
Charles Keepax -
Kuninori Morimoto -
Liao, Bard -
Mark Brown -
Pierre-Louis Bossart -
Richard Fitzgerald