Re: [PATCH 1/4] ASoC: makes CPU/Codec channel connection map more generic

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 10/9/23 21:21, Kuninori Morimoto wrote:
> Current ASoC CPU:Codec = N:M connection is using connection mapping idea,
> but it is used for CPU < Codec case only. We want to use it for any case.
> 
> By this patch, not only N:M connection, but all existing connection
> (1:1, 1:N, N:N) will use same connection mapping.
> Because it will use default mapping, no conversion patch is needed
> to exising CPU/Codec drivers.
> 
> More over, CPU:Codec = M:N (M > N) also supported in the same time.
> 
> Link: https://lore.kernel.org/r/87fs6wuszr.wl-kuninori.morimoto.gx@renesas.com
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/soc.h              | 48 ++++++++++++++++--
>  sound/soc/intel/boards/sof_sdw.c | 14 +++---
>  sound/soc/soc-core.c             | 83 ++++++++++++++++++++++++++++++++
>  sound/soc/soc-dapm.c             | 47 +++++++-----------
>  sound/soc/soc-pcm.c              | 73 +++++++++++++++-------------
>  5 files changed, 191 insertions(+), 74 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 63b57f58cc569..13f1158df2b1e 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -655,8 +655,50 @@ struct snd_soc_dai_link_component {
>  	struct of_phandle_args *dai_args;
>  };
>  
> -struct snd_soc_dai_link_codec_ch_map {
> -	unsigned int connected_cpu_id;
> +/*
> + * [dai_link->ch_maps Image sample]
> + *
> + * CPU0 <---> Codec0
> + *
> + *	.ch_maps is from CPU
> + *
> + *	.num_cpus   = 1;
> + *	.num_codecs = 1;
> + *	.connected_node = [0];
> + *
> + * CPU0 <---> Codec_x
> + * CPU1 <---> Codec_y
> + * CPU2 <---> Codec_z
> + *
> + *	.ch_maps is from CPU
> + *
> + *	.num_cpus   = 3;
> + *	.num_codecs = 3;
> + *	.connected_node = [x, y, z];
> + *
> + * CPU0 <---> Codec_x
> + * CPU1 <-+-> Codec_y
> + * CPU2 <-/
> + *
> + *	.ch_maps is from CPU
> + *
> + *	.num_cpus   = 3;
> + *	.num_codecs = 2;
> + *	.connected_node = [x, y, y];
> + *
> + *
> + * CPU_x <---> Codec0
> + * CPU_y <-+-> Codec1
> + *	   \-> Codec2
> + *
> + *	.ch_maps is from Codec

how would we know what the convention is? Is this based on the largest
number of dais, so here num_codecs > num_cpus so we use a codec-centric
convention? That would be worth explaining in clear text

> + *
> + *	.num_cpus   = 2;
> + *	.num_codecs = 3;
> + *	.connected_node = [x, y, y];
> + */
> +struct snd_soc_dai_link_ch_map {
> +	unsigned int connected_node;

connected_node is a scalar here and an array above. maybe split this
patch between a rename and a functionality change?

>  	unsigned int ch_mask;
>  };
>  
> @@ -688,7 +730,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;
> +	struct snd_soc_dai_link_ch_map *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/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
> index 226a74a4c340f..7927b729866d9 100644
> --- a/sound/soc/intel/boards/sof_sdw.c
> +++ b/sound/soc/intel/boards/sof_sdw.c
> @@ -579,7 +579,7 @@ int sdw_hw_params(struct snd_pcm_substream *substream,
>  	int i;
>  	int j;
>  
> -	if (!rtd->dai_link->codec_ch_maps)
> +	if (!rtd->dai_link->ch_maps)
>  		return 0;
>  
>  	/* Identical data will be sent to all codecs in playback */
> @@ -607,9 +607,9 @@ int sdw_hw_params(struct snd_pcm_substream *substream,
>  	 */
>  	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)
> +			if (rtd->dai_link->ch_maps[j].connected_node != i)


>  				continue;
> -			rtd->dai_link->codec_ch_maps[j].ch_mask = ch_mask << (j * step);
> +			rtd->dai_link->ch_maps[j].ch_mask = ch_mask << (j * step);
>  		}
>  	}
>  	return 0;
> @@ -1350,7 +1350,7 @@ 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,
> +static void set_dailink_map(struct snd_soc_dai_link_ch_map *sdw_codec_ch_maps,
>  			    int codec_num, int cpu_num)
>  {
>  	int step;
> @@ -1358,7 +1358,7 @@ static void set_dailink_map(struct snd_soc_dai_link_codec_ch_map *sdw_codec_ch_m
>  
>  	step = codec_num / cpu_num;
>  	for (i = 0; i < codec_num; i++)
> -		sdw_codec_ch_maps[i].connected_cpu_id = i / step;
> +		sdw_codec_ch_maps[i].connected_node = i / step;
>  }
>  
>  static const char * const type_strings[] = {"SimpleJack", "SmartAmp", "SmartMic"};
> @@ -1453,7 +1453,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
>  		*ignore_pch_dmic = true;
>  
>  	for_each_pcm_streams(stream) {
> -		struct snd_soc_dai_link_codec_ch_map *sdw_codec_ch_maps;
> +		struct snd_soc_dai_link_ch_map *sdw_codec_ch_maps;
>  		char *name, *cpu_name;
>  		int playback, capture;
>  		static const char * const sdw_stream_name[] = {
> @@ -1530,7 +1530,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
>  		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;
> +		dai_links[*link_index].ch_maps = sdw_codec_ch_maps;
>  		ret = set_codec_init_func(card, adr_link, dai_links + (*link_index)++,
>  					  playback, group_id, adr_index, dai_index);
>  		if (ret < 0) {
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index c305e94762c39..a4bb4c29331cf 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1824,6 +1824,84 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour)
>  EXPORT_SYMBOL_GPL(snd_soc_set_dmi_name);
>  #endif /* CONFIG_DMI */
>  
> +#define MAX_DEFAULT_CONNECTION_MAP_SIZE 7

why 7?

> +static struct snd_soc_dai_link_ch_map default_connction_map1[MAX_DEFAULT_CONNECTION_MAP_SIZE] = {

typo: connection

this is repeated multiple times in comments below

> +	{ .connected_node = 0 },
> +	{ .connected_node = 1 },
> +	{ .connected_node = 2 },
> +	{ .connected_node = 3 },
> +	{ .connected_node = 4 },
> +	{ .connected_node = 5 },
> +	{ .connected_node = 6 },
> +};
> +static struct snd_soc_dai_link_ch_map default_connction_map2[MAX_DEFAULT_CONNECTION_MAP_SIZE] = {
> +	{ .connected_node = 0 },
> +	{ .connected_node = 0 },
> +	{ .connected_node = 0 },
> +	{ .connected_node = 0 },
> +	{ .connected_node = 0 },
> +	{ .connected_node = 0 },
> +	{ .connected_node = 0 },
> +};
> +
> +static int snd_soc_compensate_connection_map(struct snd_soc_card *card)
> +{
> +	struct snd_soc_dai_link *dai_link;
> +	int i, j, n, max;
> +
> +	/*
> +	 * dai_link->ch_maps indicates how CPU/Codec are connected.
> +	 * It will be a map seen from a larger number of DAI.
> +	 * see
> +	 *	soc.h :: [dai_link->ch_maps Image sample]
> +	 */
> +	for_each_card_prelinks(card, i, dai_link) {
> +
> +		/* it should have ch_maps if connection was N:M */
> +		if (dai_link->num_cpus > 1 && dai_link->num_codecs > 1 &&
> +		    dai_link->num_cpus != dai_link->num_codecs && !dai_link->ch_maps) {
> +			dev_err(card->dev, "need to have ch_maps when N:M connction (%s)",
> +				dai_link->name);
> +			return -EINVAL;
> +		}
> +
> +		/* do nothing if it has own maps */
> +		if (dai_link->ch_maps)
> +			goto sanity_check;
> +
> +		/* check default map size */
> +		if (dai_link->num_cpus   > MAX_DEFAULT_CONNECTION_MAP_SIZE ||
> +		    dai_link->num_codecs > MAX_DEFAULT_CONNECTION_MAP_SIZE) {
> +			dev_err(card->dev, "soc-core.c needs update default_connction_maps");
> +			return -EINVAL;
> +		}
> +
> +		/* Compensate missing map for ... */
> +		if (dai_link->num_cpus == dai_link->num_codecs)
> +			dai_link->ch_maps = default_connction_map1; /* for 1:1 or N:N */
> +		else
> +			dai_link->ch_maps = default_connction_map2; /* for 1:N or N:1 */
> +
> +sanity_check:
> +		if (dai_link->num_cpus >= dai_link->num_codecs) {
> +			n   = dai_link->num_cpus;
> +			max = dai_link->num_codecs;
> +		} else {
> +			n   = dai_link->num_codecs;
> +			max = dai_link->num_cpus;
> +		}
> +
> +		for (j = 0; j < n; j++)
> +			if (dai_link->ch_maps[j].connected_node >= max) {
> +				dev_err(card->dev, "strange connected_node (%d) was added to ch_maps",

maybe elaborate on what "strange" might mean so that average users can
figure this out?

> +					dai_link->ch_maps[j].connected_node);
> +				return -EINVAL;
> +			}
> +	}
> +
> +	return 0;
> +}
> +