| Message ID | 20230607031242.1032060-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: add N cpus to M codecs dai link support | expand |
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
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(-)