Re: [PATCH] ASoC: qcom: x1e80100: Allow userspace WSA RX channel map override

[Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>]

On 6/11/26 9:58 AM, Srinivas Kandagatla wrote:
> Thanks Abel, for the patch.
> 
> On 6/10/26 12:16 PM, Abel Vesa wrote:
>> The WSA RX slot mapping is currently computed entirely from the channel
>> count in .prepare() and pushed unconditionally via
>> snd_soc_dai_set_channel_map(). Userspace has no way to express a different
>> channel map, so any map defined in UCM is ignored.
>>
>> Add a writable "Playback Channel Map" control on the WSA RX PCM via
>> snd_pcm_add_chmap_ctls() and make it writable. The map selected by
>> userspace (SNDRV_CHMAP_*) is stored per AFE port and, at .prepare()
>> time, translated into the q6 PCM_CHANNEL_* slots and programmed into the
>> DSP.
>>
>> When userspace has not provided a full map, the previous default
>> mapping is used, so existing behaviour is preserved.
>>

Also if possible can you move some common code to common.c so that other
machine drivers can find useful.

--srini

>> Assisted-by: Codex:GPT-5.5
>> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
>> ---
>>  sound/soc/qcom/x1e80100.c | 152 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 150 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/qcom/x1e80100.c b/sound/soc/qcom/x1e80100.c
>> index c81df41ace88..63cd00620ade 100644
>> --- a/sound/soc/qcom/x1e80100.c
>> +++ b/sound/soc/qcom/x1e80100.c
>> @@ -15,14 +15,105 @@
>>  #include "qdsp6/q6dsp-common.h"
>>  #include "sdw.h"
>>  
>> +#define X1E80100_WSA_MAX_CHANNELS	4
> 
> this is not the case anymore, we have 6 channels on Glymur.
> 
>> +
>>  struct x1e80100_snd_data {
>>  	bool stream_prepared[AFE_PORT_MAX];
>>  	struct snd_soc_card *card;
>>  	struct snd_soc_jack jack;
>>  	struct snd_soc_jack dp_jack[8];
>>  	bool jack_setup;
>> +	unsigned int user_chmap[AFE_PORT_MAX][4];
>> +	bool chmap_added[AFE_PORT_MAX];
>>  };
>>  
>> +static const struct snd_pcm_chmap_elem x1e80100_wsa_chmaps[] = {
> 
> can you not use snd_pcm_std_chmaps instead ?
> 
>> +	{ .channels = 1, .map = { SNDRV_CHMAP_FC } },
>> +	{ .channels = 2, .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_FR } },
>> +	{ .channels = 3, .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_FR,
>> +				  SNDRV_CHMAP_FC } },
>> +	{ .channels = 4, .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_FR,
>> +				  SNDRV_CHMAP_RL, SNDRV_CHMAP_RR } },
>> +	{ }
>> +};
>> +
>> +static int x1e80100_chmap_to_q6(unsigned int pos)
>> +{> +	switch (pos) {
>> +	case SNDRV_CHMAP_FL:	return PCM_CHANNEL_FL;
>> +	case SNDRV_CHMAP_FR:	return PCM_CHANNEL_FR;
>> +	case SNDRV_CHMAP_MONO:
>> +	case SNDRV_CHMAP_FC:	return PCM_CHANNEL_FC;
>> +	case SNDRV_CHMAP_RL:	return PCM_CHANNEL_LB;
>> +	case SNDRV_CHMAP_RR:	return PCM_CHANNEL_RB;
>> +	case SNDRV_CHMAP_SL:	return PCM_CHANNEL_LS;
>> +	case SNDRV_CHMAP_SR:	return PCM_CHANNEL_RS;
>> +	case SNDRV_CHMAP_LFE:	return PCM_CHANNEL_LFE;
>> +	case SNDRV_CHMAP_RC:	return PCM_CHANNEL_CS;
>> +	default:		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int x1e80100_chmap_ctl_get(struct snd_kcontrol *kcontrol,
>> +				  struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
>> +	struct snd_soc_pcm_runtime *rtd = info->pcm->private_data;
>> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
>> +	struct x1e80100_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
>> +	unsigned int *map = data->user_chmap[cpu_dai->id];
>> +	int i;
>> +
>> +	for (i = 0; i < info->max_channels; i++)
>> +		ucontrol->value.integer.value[i] = map[i];
>> +
>> +	return 0;
>> +}
>> +
>> +static int x1e80100_chmap_ctl_put(struct snd_kcontrol *kcontrol,
>> +				  struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
>> +	struct snd_soc_pcm_runtime *rtd = info->pcm->private_data;
>> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
>> +	struct x1e80100_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
>> +	unsigned int *map = data->user_chmap[cpu_dai->id];
>> +	int i;
>> +
>> +	for (i = 0; i < info->max_channels; i++) {
>> +		unsigned int pos = ucontrol->value.integer.value[i];
>> +
>> +		/* Validate every requested non-unset position up front. */
>> +		if (pos && x1e80100_chmap_to_q6(pos) < 0)
>> +			return -EINVAL;
>> +
>> +		map[i] = pos;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int x1e80100_add_wsa_chmap_ctls(struct snd_soc_pcm_runtime *rtd)
>> +{
>> +	struct snd_pcm_chmap *info;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	ret = snd_pcm_add_chmap_ctls(rtd->pcm, SNDRV_PCM_STREAM_PLAYBACK,
>> +				     x1e80100_wsa_chmaps,
>> +				     X1E80100_WSA_MAX_CHANNELS, 0, &info);
> 
> This will give mixer control as "Playback Channel Map", can we prefix
> this with WSA, as this can be cryptic for user side to interpret
> Playback Channel Map as a WSA Channel map and also if we add more
> channel maps for example from hdmi codec we will endup with something
> like on my t14s:
> numid=136,iface=PCM,name='Playback Channel Map',device=16
> numid=141,iface=PCM,name='Playback Channel Map',device=17
> numid=146,iface=PCM,name='Playback Channel Map',device=18
> numid=152,iface=PCM,name='Playback Channel Map',device=19
> 
> This is really confusing, with below patch
> you should get:
> numid=136,iface=PCM,name='Playback Channel Map',device=16
> numid=141,iface=PCM,name='Playback Channel Map',device=17
> numid=146,iface=PCM,name='Playback Channel Map',device=18
> numid=152,iface=PCM,name='WSA Playback Channel Map',device=21
> 
> Clearly giving users more clear picture.
> Please use this patch and let me know I have tested this on t14s along
> with your patch and making some modifications to it.
> 
> 
> -------------------------------------->cut<----------------------------
> Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> Date:   Thu Jun 11 08:12:17 2026 +0100
> 
>     ALSA: pcm: Add snd_pcm_add_named_chmap_ctls() for custom control names
> 
>     Add snd_pcm_add_named_chmap_ctls() to allowing allowing drivers to
>     register channel map controls with names other than the default
>     "Playback/Capture Channel Map".
> 
>     Signed-off-by: Srinivas Kandagatla
> <srinivas.kandagatla@oss.qualcomm.com>
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 76fc33dce537..2a51f7c02dc6 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -1501,6 +1501,12 @@ extern const struct snd_pcm_chmap_elem
> snd_pcm_alt_chmaps[];
>  #define SND_PCM_CHMAP_MASK_246 (SND_PCM_CHMAP_MASK_24 | (1U << 6))
>  #define SND_PCM_CHMAP_MASK_2468        (SND_PCM_CHMAP_MASK_246 | (1U << 8))
> 
> +int snd_pcm_add_named_chmap_ctls(struct snd_pcm *pcm, int stream,
> +                                const char *name,
> +                                const struct snd_pcm_chmap_elem *chmap,
> +                                int max_channels,
> +                                unsigned long private_value,
> +                                struct snd_pcm_chmap **info_ret);
>  int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm, int stream,
>                            const struct snd_pcm_chmap_elem *chmap,
>                            int max_channels,
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index fe597f7d522d..7dae270eaedc 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2572,9 +2572,10 @@ static void pcm_chmap_ctl_private_free(struct
> snd_kcontrol *kcontrol)
>  }
> 
>  /**
> - * snd_pcm_add_chmap_ctls - create channel-mapping control elements
> + * snd_pcm_add_named_chmap_ctls - create channel-mapping control
> elements with a custom name
>   * @pcm: the assigned PCM instance
>   * @stream: stream direction
> + * @name: the control element name
>   * @chmap: channel map elements (for query)
>   * @max_channels: the max number of channels for the stream
>   * @private_value: the value passed to each kcontrol's private_value field
> @@ -2583,11 +2584,12 @@ static void pcm_chmap_ctl_private_free(struct
> snd_kcontrol *kcontrol)
>   * Create channel-mapping control elements assigned to the given PCM
> stream(s).
>   * Return: Zero if successful, or a negative error value.
>   */
> -int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm, int stream,
> -                          const struct snd_pcm_chmap_elem *chmap,
> -                          int max_channels,
> -                          unsigned long private_value,
> -                          struct snd_pcm_chmap **info_ret)
> +int snd_pcm_add_named_chmap_ctls(struct snd_pcm *pcm, int stream,
> +                       const char *name,
> +                       const struct snd_pcm_chmap_elem *chmap,
> +                       int max_channels,
> +                       unsigned long private_value,
> +                       struct snd_pcm_chmap **info_ret)
>  {
>         struct snd_pcm_chmap *info;
>         struct snd_kcontrol_new knew = {
> @@ -2611,10 +2613,7 @@ int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm,
> int stream,
>         info->stream = stream;
>         info->chmap = chmap;
>         info->max_channels = max_channels;
> -       if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> -               knew.name = "Playback Channel Map";
> -       else
> -               knew.name = "Capture Channel Map";
> +       knew.name = name;
>         knew.device = pcm->device;
>         knew.count = pcm->streams[stream].substream_count;
>         knew.private_value = private_value;
> @@ -2632,4 +2631,29 @@ int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm,
> int stream,
>                 *info_ret = info;
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(snd_pcm_add_named_chmap_ctls);
> +
> +/**
> + * snd_pcm_add_chmap_ctls - create channel-mapping control elements
> + * @pcm: the assigned PCM instance
> + * @stream: stream direction
> + * @chmap: channel map elements (for query)
> + * @max_channels: the max number of channels for the stream
> + * @private_value: the value passed to each kcontrol's private_value field
> + * @info_ret: store struct snd_pcm_chmap instance if non-NULL
> + *
> + * Create channel-mapping control elements assigned to the given PCM
> stream(s).
> + * Return: Zero if successful, or a negative error value.
> + */
> +int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm, int stream,
> +                          const struct snd_pcm_chmap_elem *chmap,
> +                          int max_channels,
> +                          unsigned long private_value,
> +                          struct snd_pcm_chmap **info_ret)
> +{
> +       return snd_pcm_add_named_chmap_ctls(pcm, stream,
> +                       stream == SNDRV_PCM_STREAM_PLAYBACK ? "Playback
> Channel Map" :
> +                                  "Capture Channel Map", chmap,
> max_channels,
> +                                  private_value, info_ret);
> +}
>  EXPORT_SYMBOL_GPL(snd_pcm_add_chmap_ctls);
> 
> -------------------------------------->cut<----------------------------
> 
> changes to your patch:
> 
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Make the query-only chmap control writable from userspace. */
>> +	for (i = 0; i < info->kctl->count; i++)
>> +		info->kctl->vd[i].access |= SNDRV_CTL_ELEM_ACCESS_WRITE;
>> +	info->kctl->get = x1e80100_chmap_ctl_get;
>> +	info->kctl->put = x1e80100_chmap_ctl_put;
>> +
>> +	return 0;
>> +}
>> +
>>  static int x1e80100_snd_init(struct snd_soc_pcm_runtime *rtd)
>>  {
>>  	struct x1e80100_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
>> @@ -34,6 +125,9 @@ static int x1e80100_snd_init(struct snd_soc_pcm_runtime *rtd)
>>  	switch (cpu_dai->id) {
>>  	case WSA_CODEC_DMA_RX_0:
>>  	case WSA_CODEC_DMA_RX_1:
>> +		if (!rtd->pcm)
>> +			return 0;
>> +
>>  		/*
>>  		 * Set limit of -3 dB on Digital Volume and 0 dB on PA Volume
>>  		 * to reduce the risk of speaker damage until we have active
>> @@ -92,6 +186,7 @@ static int x1e80100_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
>>  	return 0;
>>  }
>>  
>> +/* Default WSA RX slot mapping when userspace has not provided a channel map. */
>>  static int x1e80100_snd_hw_map_channels(unsigned int *ch_map, int num)
>>  {
>>  	switch (num) {
>> @@ -120,6 +215,58 @@ static int x1e80100_snd_hw_map_channels(unsigned int *ch_map, int num)
>>  	return 0;
>>  }
>>  
>> +static int x1e80100_snd_build_rx_slot(struct x1e80100_snd_data *data,
>> +				      unsigned int dai_id, unsigned int channels,
>> +				      unsigned int *rx_slot)
>> +{
>> +	unsigned int *user = data->user_chmap[dai_id];
>> +	unsigned int i, set = 0;
>> +
>> +	for (i = 0; i < channels && i < ARRAY_SIZE(data->user_chmap[0]); i++)
>> +		if (user[i])
>> +			set++;
>> +
>> +	if (set != channels)
>> +		return x1e80100_snd_hw_map_channels(rx_slot, channels);
>> +
>> +	for (i = 0; i < channels; i++) {
>> +		int q6 = x1e80100_chmap_to_q6(user[i]);
>> +
>> +		if (q6 < 0)
>> +			return q6;
>> +		rx_slot[i] = q6;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int x1e80100_snd_startup(struct snd_pcm_substream *substream)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
>> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
>> +	struct x1e80100_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
>> +	int ret;
>> +
>> +	ret = qcom_snd_sdw_startup(substream);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (cpu_dai->id) {
>> +	case WSA_CODEC_DMA_RX_0:
>> +	case WSA_CODEC_DMA_RX_1:
>> +		if (!data->chmap_added[cpu_dai->id]) {
>> +			ret = x1e80100_add_wsa_chmap_ctls(rtd);
> 
> This is will lockup, because startup is called under a dapm_mutex and
> function calls to snd_ctl_add(), which acquires card->controls_rwsem
> (write).
> 
> This creates an AB-BA deadlock with DAPM:
> 
>   Thread A -- PCM open:
>     pcm->open_mutex -> dpcm_mutex -> controls_rwsem (write, via snd_ctl_add)
> 
>   Thread B -- DAPM mixer write:
>     controls_rwsem (write, via snd_ctl_elem_write) -> dpcm_mutex (via
> dapm_power_widgets)
> 
> 
> So better move this to to late_probe:
> 
> Here is what i changed in your patch:
> 
> -------------------->cut<----------------------------
> 
> diff --git a/sound/soc/qcom/x1e80100.c b/sound/soc/qcom/x1e80100.c
> index 63cd00620ade..8c066f1fd2b7 100644
> --- a/sound/soc/qcom/x1e80100.c
> +++ b/sound/soc/qcom/x1e80100.c
> @@ -24,7 +24,6 @@ struct x1e80100_snd_data {
>         struct snd_soc_jack dp_jack[8];
>         bool jack_setup;
>         unsigned int user_chmap[AFE_PORT_MAX][4];
> -       bool chmap_added[AFE_PORT_MAX];
>  };
> 
>  static const struct snd_pcm_chmap_elem x1e80100_wsa_chmaps[] = {
> @@ -93,14 +92,14 @@ static int x1e80100_chmap_ctl_put(struct
> snd_kcontrol *kcontrol,
>         return 0;
>  }
> 
> -static int x1e80100_add_wsa_chmap_ctls(struct snd_soc_pcm_runtime *rtd)
> +static int x1e80100_add_wsa_chmap_ctls(struct snd_soc_pcm_runtime *rtd,
> const char *name)
>  {
>         struct snd_pcm_chmap *info;
>         unsigned int i;
>         int ret;
> 
> -       ret = snd_pcm_add_chmap_ctls(rtd->pcm, SNDRV_PCM_STREAM_PLAYBACK,
> -                                    x1e80100_wsa_chmaps,
> +       ret = snd_pcm_add_named_chmap_ctls(rtd->pcm,
> SNDRV_PCM_STREAM_PLAYBACK,
> +                                    name, x1e80100_wsa_chmaps,
>                                      X1E80100_WSA_MAX_CHANNELS, 0, &info);
>         if (ret < 0)
>                 return ret;
> @@ -242,29 +241,7 @@ static int x1e80100_snd_build_rx_slot(struct
> x1e80100_snd_data *data,
> 
>  static int x1e80100_snd_startup(struct snd_pcm_substream *substream)
>  {
> -       struct snd_soc_pcm_runtime *rtd =
> snd_soc_substream_to_rtd(substream);
> -       struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> -       struct x1e80100_snd_data *data =
> snd_soc_card_get_drvdata(rtd->card);
> -       int ret;
> -
> -       ret = qcom_snd_sdw_startup(substream);
> -       if (ret)
> -               return ret;
> -
> -       switch (cpu_dai->id) {
> -       case WSA_CODEC_DMA_RX_0:
> -       case WSA_CODEC_DMA_RX_1:
> -               if (!data->chmap_added[cpu_dai->id]) {
> -                       ret = x1e80100_add_wsa_chmap_ctls(rtd);
> -                       if (ret)
> -                               return ret;
> -
> -                       data->chmap_added[cpu_dai->id] = true;
> -               }
> -               break;
> -       }
> -
> -       return 0;
> +       return qcom_snd_sdw_startup(substream);
>  }
> 
>  static int x1e80100_snd_prepare(struct snd_pcm_substream *substream)
> @@ -312,6 +289,29 @@ static const struct snd_soc_ops x1e80100_be_ops = {
>         .prepare = x1e80100_snd_prepare,
>  };
> 
> +static int x1e80100_late_probe(struct snd_soc_card *card)
> +{
> +       struct snd_soc_pcm_runtime *rtd;
> +       int ret;
> +
> +       for_each_card_rtds(card, rtd) {
> +               struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> +
> +               switch (cpu_dai->id) {
> +               case WSA_CODEC_DMA_RX_0:
> +               case WSA_CODEC_DMA_RX_1:
> +                       if (!rtd->pcm)
> +                               break;
> +                       ret = x1e80100_add_wsa_chmap_ctls(rtd, "WSA
> Playback Channel Map");
> +                       if (ret)
> +                               return ret;
> +                       break;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static void x1e80100_add_be_ops(struct snd_soc_card *card)
>  {
>         struct snd_soc_dai_link *link;
> @@ -351,6 +351,7 @@ static int x1e80100_platform_probe(struct
> platform_device *pdev)
>                 return ret;
> 
>         card->driver_name = of_device_get_match_data(dev);
> +       card->late_probe = x1e80100_late_probe;
>         x1e80100_add_be_ops(card);
> 
>         return devm_snd_soc_register_card(dev, card);
> 
> -------------------->cut<----------------------------	
> 
> 
>> +			if (ret)
>> +				return ret;
>> +
>> +			data->chmap_added[cpu_dai->id] = true;
>> +		}
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int x1e80100_snd_prepare(struct snd_pcm_substream *substream)
>>  {
>>  	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
>> @@ -132,7 +279,8 @@ static int x1e80100_snd_prepare(struct snd_pcm_substream *substream)
>>  	switch (cpu_dai->id) {
>>  	case WSA_CODEC_DMA_RX_0:
>>  	case WSA_CODEC_DMA_RX_1:
>> -		ret = x1e80100_snd_hw_map_channels(rx_slot, channels);
>> +		ret = x1e80100_snd_build_rx_slot(data, cpu_dai->id, channels,
>> +						 rx_slot);
>>  		if (ret)
>>  			return ret;
>>  
>> @@ -158,7 +306,7 @@ static int x1e80100_snd_hw_free(struct snd_pcm_substream *substream)
>>  }
>>  
>>  static const struct snd_soc_ops x1e80100_be_ops = {
>> -	.startup = qcom_snd_sdw_startup,
>> +	.startup = x1e80100_snd_startup,
>>  	.shutdown = qcom_snd_sdw_shutdown,
>>  	.hw_free = x1e80100_snd_hw_free,
>>  	.prepare = x1e80100_snd_prepare,
>>
>> ---
>> base-commit: a87737435cfa134f9cdcc696ba3080759d04cf72
>> change-id: 20260609-sound-qcom-x1e80100-allow-ucm-channel-map-2302f2d552d5
>>
>> Best regards,
>> --  
>> Abel Vesa <abel.vesa@oss.qualcomm.com>
>>
>