[PATCH 0/5] ASoC: Fix missing channel fixup for codec end of ch_map

[PATCH 0/5] ASoC: Fix missing channel fixup for codec end of ch_map

[Richard Fitzgerald]

Struct snd_soc_dai_link_ch_map had a single mask member to set the
CPU channel masks. But no fixup was done to the codec end of the link.

For example if a 4-channel CPU capture DAI was made from two codecs both
supplying 2 channels, the hw_params() of the codec would be passed a
channel count of 4.

On SoundWire this could cause multiple codecs to send data in the same
bits of a frame because the unused channels were not disabled.

The changes in this series are:
- Separate channel masks for CPU and codec in struct
  snd_soc_dai_link_ch_map .

- Apply the codec channel mask as a channel count fixup if the machine
  drive has not set a TDM mask.

- Set the codec channel mask in the SoundWire machine driver.

- Remove the workaround from the cs_amp machine driver.

Richard Fitzgerald (5):
  ASoC: Rename snd_soc_dai_link_ch_map.ch_mask to cpu_ch_mask
  ASoC: Add codec_ch_mask to snd_soc_dai_link_ch_map
  ASoC: soc-pcm: Apply snd_soc_dai_link_ch_map.codec_ch_mask to codec
    params
  ASoC: sdw_utils: Set snd_soc_dai_link_ch_map.codec_ch_mask for capture
  ASoC: sdw_utils: cs_amp: Delete bogus and incorrect capture channel
    fixup

 include/sound/soc.h                  |  3 +-
 include/sound/soc_sdw_utils.h        |  2 --
 sound/soc/sdw_utils/soc_sdw_cs_amp.c | 46 ----------------------------
 sound/soc/sdw_utils/soc_sdw_utils.c  | 24 +++++++++------
 sound/soc/soc-pcm.c                  | 18 ++++++++---
 5 files changed, 29 insertions(+), 64 deletions(-)

-- 
2.47.3

[PATCH 1/5] ASoC: Rename snd_soc_dai_link_ch_map.ch_mask to cpu_ch_mask

[Richard Fitzgerald]

Rename the ch_mask member of snd_soc_dai_link_ch_map to cpu_ch_mask,
as that is what it is used for.

The CPU and codec channel masks are not necessarily the same, and are
quite likely different. SoundWire and I2S/TDM both support assigning
different sample slots to each codec, so for example channel 0 on each
codec could map to different channels at the CPU. So it's quite normal
that the channel mask at the CPU end is different for each codec, but
the codec channel masks are the same for each codec.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/soc.h                 | 2 +-
 sound/soc/sdw_utils/soc_sdw_utils.c | 2 +-
 sound/soc/soc-pcm.c                 | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index f46b2bc2a022..94c9b75e27e3 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -699,7 +699,7 @@ struct snd_soc_dai_link_component {
 struct snd_soc_dai_link_ch_map {
 	unsigned int cpu;
 	unsigned int codec;
-	unsigned int ch_mask;
+	unsigned int cpu_ch_mask;
 };
 
 struct snd_soc_dai_link {
diff --git a/sound/soc/sdw_utils/soc_sdw_utils.c b/sound/soc/sdw_utils/soc_sdw_utils.c
index 88031238c04e..cab1578b893d 100644
--- a/sound/soc/sdw_utils/soc_sdw_utils.c
+++ b/sound/soc/sdw_utils/soc_sdw_utils.c
@@ -1762,7 +1762,7 @@ int asoc_sdw_hw_params(struct snd_pcm_substream *substream,
 	 * ASoC will set the corresponding channel numbers for each cpu dai.
 	 */
 	for_each_link_ch_maps(rtd->dai_link, i, ch_maps)
-		ch_maps->ch_mask = ch_mask << (i * step);
+		ch_maps->cpu_ch_mask = ch_mask << (i * step);
 
 	return 0;
 }
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 0e49290a8c90..cb64ced21149 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1264,7 +1264,7 @@ static int __soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		 */
 		for_each_rtd_ch_maps(rtd, j, ch_maps)
 			if (ch_maps->cpu == i)
-				ch_mask |= ch_maps->ch_mask;
+				ch_mask |= ch_maps->cpu_ch_mask;
 
 		/* fixup cpu channel number */
 		if (ch_mask)
-- 
2.47.3

[PATCH 2/5] ASoC: Add codec_ch_mask to snd_soc_dai_link_ch_map

[Richard Fitzgerald]

Add a codec_ch_mask member to snd_soc_dai_link_ch_map.

The CPU and codec channel masks are not necessarily the same, and are
quite likely different. SoundWire and I2S/TDM both support assigning
different sample slots to each codec, so for example channel 0 on each
codec could map to different channels at the CPU.

It is also possible for one TX channel to map to multiple RX channels.
So it isn't _always_ safe to assume that the total number of set bits
in the CPU ch_mask is the same as the total number of enabled channels
on the codec.

For example consider this mapping on a capture stream:

CPU0 CODEC0 cpu_ch_mask = 0x03
CPU1 CODEC0 cpu_ch_mask = 0x03

This could be either four TX channels on the codec split across two
receiving CPUs, or two TX channels on the codec duplicated to two CPUs.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/soc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 94c9b75e27e3..5afc34b147b5 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -700,6 +700,7 @@ struct snd_soc_dai_link_ch_map {
 	unsigned int cpu;
 	unsigned int codec;
 	unsigned int cpu_ch_mask;
+	unsigned int codec_ch_mask;
 };
 
 struct snd_soc_dai_link {
-- 
2.47.3

[PATCH 3/5] ASoC: soc-pcm: Apply snd_soc_dai_link_ch_map.codec_ch_mask to codec params

[Richard Fitzgerald]

In __soc_pcm_hw_params() if there is a snd_soc_dai_link_ch_map with
non-zero codec_ch_mask, use that channel mask to restrict which channels
are enabled on the codec. But only if there isn't a TDM mask.

It is possible that a snd_soc_dai_link_ch_map could include the same codec
multiple times on different CPUs so the for_each_rtd_ch_maps() loop
accumulates the channel masks for all entries of that codec.

If a TDM mask was also set, it takes priority and is used instead of any
possible snd_soc_dai_link_ch_map entries. (They cannot be ANDed together
because the bit positions are indicating different things: TDM is a bit
for each TDM slot, codec_ch_mask is a bit for each codec channel.)

This fixes a problem of incorrect TX channels enabled on the codec when
multiple codecs are aggregated on a single capture link. For example:

- Two CPUs with six 4-channel codecs.
- The machine driver chooses to assign one channel from each codec to
  one channel on the CPU
- But the codec hw_params() would be passed a channel count of 6, which
  (a) is more channels than the codec has and (b) allows enabling channels
  that should not be driving the audio bus.

Fixes: ac950278b087 ("ASoC: add N cpus to M codecs dai link support")
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/soc-pcm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index cb64ced21149..3137c091bdb8 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1206,7 +1206,9 @@ static int __soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		goto out;
 
 	for_each_rtd_codec_dais(rtd, i, codec_dai) {
-		unsigned int tdm_mask = snd_soc_dai_tdm_mask_get(codec_dai, substream->stream);
+		unsigned int ch_mask = snd_soc_dai_tdm_mask_get(codec_dai, substream->stream);
+		struct snd_soc_dai_link_ch_map *ch_maps;
+		int j;
 
 		/*
 		 * Skip CODECs which don't support the current stream type,
@@ -1228,9 +1230,15 @@ static int __soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		/* copy params for each codec */
 		tmp_params = *params;
 
-		/* fixup params based on TDM slot masks */
-		if (tdm_mask)
-			soc_pcm_codec_params_fixup(&tmp_params, tdm_mask);
+		/* fixup params based on TDM or ch_map masks */
+		if (!ch_mask) {
+			for_each_rtd_ch_maps(rtd, j, ch_maps)
+				if (ch_maps->codec == i)
+					ch_mask |= ch_maps->codec_ch_mask;
+		}
+
+		if (ch_mask)
+			soc_pcm_codec_params_fixup(&tmp_params, ch_mask);
 
 		ret = snd_soc_dai_hw_params(codec_dai, substream,
 					    &tmp_params);
-- 
2.47.3

[PATCH 4/5] ASoC: sdw_utils: Set snd_soc_dai_link_ch_map.codec_ch_mask for capture

[Richard Fitzgerald]

In asoc_sdw_hw_params() set the codec_ch_mask member of struct
snd_soc_dai_link_ch_map for capture streams. ASoC will then pass the
correct number of channels to each codec hw_params(). This prevents
trying to enable more channels on the codec DP than have been allocated
bitslots in the SoundWire frame, which would cause bus clash errors.

In theory codec_ch_mask could also be set for playback streams, but for
those the CPU is the only sender so there is no risk of bus clash.
For playback streams codec_ch_mask is set to 0 to preserve the existing
behavior and avoid introducing bugs.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/sdw_utils/soc_sdw_utils.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sdw_utils/soc_sdw_utils.c b/sound/soc/sdw_utils/soc_sdw_utils.c
index cab1578b893d..778ba6531434 100644
--- a/sound/soc/sdw_utils/soc_sdw_utils.c
+++ b/sound/soc/sdw_utils/soc_sdw_utils.c
@@ -1731,7 +1731,7 @@ int asoc_sdw_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
 	struct snd_soc_dai_link_ch_map *ch_maps;
 	int ch = params_channels(params);
-	unsigned int ch_mask;
+	unsigned int cpu_ch_mask, codec_ch_mask;
 	int num_codecs;
 	int step;
 	int i;
@@ -1741,8 +1741,9 @@ int asoc_sdw_hw_params(struct snd_pcm_substream *substream,
 
 	/* Identical data will be sent to all codecs in playback */
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		ch_mask = GENMASK(ch - 1, 0);
+		cpu_ch_mask = GENMASK(ch - 1, 0);
 		step = 0;
+		codec_ch_mask = 0;
 	} else {
 		num_codecs = rtd->dai_link->num_codecs;
 
@@ -1752,17 +1753,24 @@ int asoc_sdw_hw_params(struct snd_pcm_substream *substream,
 			return -EINVAL;
 		}
 
-		ch_mask = GENMASK(ch / num_codecs - 1, 0);
-		step = hweight_long(ch_mask);
+		cpu_ch_mask = GENMASK(ch / num_codecs - 1, 0);
+		step = hweight_long(cpu_ch_mask);
+		codec_ch_mask = cpu_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.
+	 *
+	 * sdw_stream_add_slave() assigns different payload offsets to each
+	 * codec in a capture stream, so that the same channels on each
+	 * codec map to different channels on the CPU.
 	 */
-	for_each_link_ch_maps(rtd->dai_link, i, ch_maps)
-		ch_maps->cpu_ch_mask = ch_mask << (i * step);
+	for_each_link_ch_maps(rtd->dai_link, i, ch_maps) {
+		ch_maps->cpu_ch_mask = cpu_ch_mask << (i * step);
+		ch_maps->codec_ch_mask = codec_ch_mask;
+	}
 
 	return 0;
 }
-- 
2.47.3

[PATCH 5/5] ASoC: sdw_utils: cs_amp: Delete bogus and incorrect capture channel fixup

[Richard Fitzgerald]

Delete the asoc_sdw_cs_spk_feedback_rtd_init(). This is not needed now
that the ASoC bug it was working around has been fixed. And it was broken
anyway because it didn't match the way the core SoundWire code mapped
codec channels to frame bitslots.

This code was added to avoid a problem where multiple codec DP outputs
were mapped to the same SoundWire frame bit slot. This would allow a
user to break the SoundWire bus just by enabling mixer outputs using
ALSA controls.

As no production system has used the capture stream, this workaround
was of little consequence and the problem of conflicting DP mappings
was not investigated.

The ASoC bug that enabled too many channels on each codec has now been
fixed. So this workaround can be completely deleted.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/soc_sdw_utils.h        |  2 --
 sound/soc/sdw_utils/soc_sdw_cs_amp.c | 46 ----------------------------
 sound/soc/sdw_utils/soc_sdw_utils.c  |  4 ---
 3 files changed, 52 deletions(-)

diff --git a/include/sound/soc_sdw_utils.h b/include/sound/soc_sdw_utils.h
index 3c1d177a2f2d..418466438ea1 100644
--- a/include/sound/soc_sdw_utils.h
+++ b/include/sound/soc_sdw_utils.h
@@ -250,8 +250,6 @@ int asoc_sdw_cs_amp_init(struct snd_soc_card *card,
 			 struct snd_soc_dai_link *dai_links,
 			 struct asoc_sdw_codec_info *info,
 			 bool playback);
-int asoc_sdw_cs_spk_feedback_rtd_init(struct snd_soc_pcm_runtime *rtd,
-				      struct snd_soc_dai *dai);
 int asoc_sdw_cs35l56_volume_limit(struct snd_soc_card *card, const char *name_prefix);
 
 /* MAXIM codec support */
diff --git a/sound/soc/sdw_utils/soc_sdw_cs_amp.c b/sound/soc/sdw_utils/soc_sdw_cs_amp.c
index 325ab7230481..6e21ef8f87e2 100644
--- a/sound/soc/sdw_utils/soc_sdw_cs_amp.c
+++ b/sound/soc/sdw_utils/soc_sdw_cs_amp.c
@@ -14,7 +14,6 @@
 #include <sound/soc-dai.h>
 #include <sound/soc_sdw_utils.h>
 
-#define CS_AMP_CHANNELS_PER_AMP	4
 #define CS35L56_SPK_VOLUME_0DB 400 /* 0dB Max */
 
 int asoc_sdw_cs35l56_volume_limit(struct snd_soc_card *card, const char *name_prefix)
@@ -64,51 +63,6 @@ int asoc_sdw_cs_spk_rtd_init(struct snd_soc_pcm_runtime *rtd, struct snd_soc_dai
 }
 EXPORT_SYMBOL_NS(asoc_sdw_cs_spk_rtd_init, "SND_SOC_SDW_UTILS");
 
-int asoc_sdw_cs_spk_feedback_rtd_init(struct snd_soc_pcm_runtime *rtd, struct snd_soc_dai *dai)
-{
-	const struct snd_soc_dai_link *dai_link = rtd->dai_link;
-	const struct snd_soc_dai_link_ch_map *ch_map;
-	const struct snd_soc_dai_link_component *codec_dlc;
-	struct snd_soc_dai *codec_dai;
-	u8 ch_slot[8] = {};
-	unsigned int amps_per_bus, ch_per_amp, mask;
-	int i, ret;
-
-	WARN_ON(dai_link->num_cpus > ARRAY_SIZE(ch_slot));
-
-	/*
-	 * CS35L56 has 4 TX channels. When the capture is aggregated the
-	 * same bus slots will be allocated to all the amps on a bus. Only
-	 * one amp on that bus can be transmitting in each slot so divide
-	 * the available 4 slots between all the amps on a bus.
-	 */
-	amps_per_bus = dai_link->num_codecs / dai_link->num_cpus;
-	if ((amps_per_bus == 0) || (amps_per_bus > CS_AMP_CHANNELS_PER_AMP)) {
-		dev_err(rtd->card->dev, "Illegal num_codecs:%u / num_cpus:%u\n",
-			dai_link->num_codecs, dai_link->num_cpus);
-		return -EINVAL;
-	}
-
-	ch_per_amp = CS_AMP_CHANNELS_PER_AMP / amps_per_bus;
-
-	for_each_rtd_ch_maps(rtd, i, ch_map) {
-		codec_dlc = snd_soc_link_to_codec(rtd->dai_link, i);
-		codec_dai = snd_soc_find_dai(codec_dlc);
-		mask = GENMASK(ch_per_amp - 1, 0) << ch_slot[ch_map->cpu];
-
-		ret = snd_soc_dai_set_tdm_slot(codec_dai, 0, mask, 4, 32);
-		if (ret < 0) {
-			dev_err(rtd->card->dev, "Failed to set TDM slot:%d\n", ret);
-			return ret;
-		}
-
-		ch_slot[ch_map->cpu] += ch_per_amp;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_NS(asoc_sdw_cs_spk_feedback_rtd_init, "SND_SOC_SDW_UTILS");
-
 int asoc_sdw_cs_amp_init(struct snd_soc_card *card,
 			 struct snd_soc_dai_link *dai_links,
 			 struct asoc_sdw_codec_info *info,
diff --git a/sound/soc/sdw_utils/soc_sdw_utils.c b/sound/soc/sdw_utils/soc_sdw_utils.c
index 778ba6531434..0d7182bebcdc 100644
--- a/sound/soc/sdw_utils/soc_sdw_utils.c
+++ b/sound/soc/sdw_utils/soc_sdw_utils.c
@@ -820,7 +820,6 @@ struct asoc_sdw_codec_info codec_info_list[] = {
 				.dai_name = "cs35l56-sdw1c",
 				.dai_type = SOC_SDW_DAI_TYPE_AMP,
 				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_AMP_IN_DAI_ID},
-				.rtd_init = asoc_sdw_cs_spk_feedback_rtd_init,
 			},
 		},
 		.dai_num = 2,
@@ -849,7 +848,6 @@ struct asoc_sdw_codec_info codec_info_list[] = {
 				.dai_name = "cs35l56-sdw1c",
 				.dai_type = SOC_SDW_DAI_TYPE_AMP,
 				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_AMP_IN_DAI_ID},
-				.rtd_init = asoc_sdw_cs_spk_feedback_rtd_init,
 			},
 		},
 		.dai_num = 2,
@@ -878,7 +876,6 @@ struct asoc_sdw_codec_info codec_info_list[] = {
 				.dai_name = "cs35l56-sdw1c",
 				.dai_type = SOC_SDW_DAI_TYPE_AMP,
 				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_AMP_IN_DAI_ID},
-				.rtd_init = asoc_sdw_cs_spk_feedback_rtd_init,
 			},
 		},
 		.dai_num = 2,
@@ -907,7 +904,6 @@ struct asoc_sdw_codec_info codec_info_list[] = {
 				.dai_name = "cs35l56-sdw1c",
 				.dai_type = SOC_SDW_DAI_TYPE_AMP,
 				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_AMP_IN_DAI_ID},
-				.rtd_init = asoc_sdw_cs_spk_feedback_rtd_init,
 			},
 		},
 		.dai_num = 2,
-- 
2.47.3