| Message ID | 20240509-asoc-x1e80100-4-channel-mapping-v3-3-6f874552d7b2@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: qcom: x1e80100: Correct channel mapping | expand |
Thanks Krzysztof for the patch. On 09/05/2024 07:51, Krzysztof Kozlowski wrote: > When constructing packets to DSP, the Audioreach code uses 'struct > audioreach_module_config' to configure parameters like number of > channels, bitrate, sample rate etc, but uses defaults for the channel > mapping. > > Rework this code to copy the channel mapping from 'struct > audioreach_module_config', instead of using the default. This requires > all callers to fill that structure: add missing initialization of > channel mapping. Adding this new function call is logically fine but its going to introducing some sequencing issues. set_channel_map might be overwritten by this if not done correctly. One such instance is in this patch.. > > Entire patch makes code more logical and easier to follow: > 1. q6apm-dai and q6apm-lpass-dais code which allocates 'struct > audioreach_module_config' initializes it fully, so fills both > the number of channels and the channel mapping. > 2. Audioreach code, which uses 'struct audioreach_module_config' when > constructing packets, copies entire contents of passed config, not > only pieces of it. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Changes in v2: > 1. Extend commit msg, rationale. > 2. Rework to new approach, but most of the code stays. > 3. Export audioreach_set_channel_mapping() (needed by Q6APM DAIS and > LPASS DAIS) > 4. Correct channel mapping also in audioreach_mfc_set_media_format(), > because MFC DAI is now part of backend. > 5. Do not adjust dynamic DAIs (drop audioreach_dai_load()). > --- > sound/soc/qcom/qdsp6/audioreach.c | 30 +++++++----------------------- > sound/soc/qcom/qdsp6/audioreach.h | 1 + > sound/soc/qcom/qdsp6/q6apm-dai.c | 2 ++ > sound/soc/qcom/qdsp6/q6apm-lpass-dais.c | 5 ++++- > 4 files changed, 14 insertions(+), 24 deletions(-) > > diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c > index 5291deac0a0b..750b8ba64211 100644 > --- a/sound/soc/qcom/qdsp6/audioreach.c > +++ b/sound/soc/qcom/qdsp6/audioreach.c > @@ -267,7 +267,7 @@ void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode, uint32_t token > } > EXPORT_SYMBOL_GPL(audioreach_alloc_apm_cmd_pkt); > > -static void audioreach_set_channel_mapping(u8 *ch_map, int num_channels) > +void audioreach_set_channel_mapping(u8 *ch_map, int num_channels) > { > if (num_channels == 1) { > ch_map[0] = PCM_CHANNEL_FL; > @@ -281,6 +281,7 @@ static void audioreach_set_channel_mapping(u8 *ch_map, int num_channels) > ch_map[3] = PCM_CHANNEL_RS; > } > } > +EXPORT_SYMBOL_GPL(audioreach_set_channel_mapping); > > static void apm_populate_container_config(struct apm_container_obj *cfg, > struct audioreach_container *cont) > @@ -819,7 +820,7 @@ static int audioreach_mfc_set_media_format(struct q6apm_graph *graph, > uint32_t num_channels = cfg->num_channels; > int payload_size; > struct gpr_pkt *pkt; > - int rc; > + int rc, i; > void *p; > > payload_size = APM_MFC_CFG_PSIZE(media_format, num_channels) + > @@ -842,18 +843,8 @@ static int audioreach_mfc_set_media_format(struct q6apm_graph *graph, > media_format->sample_rate = cfg->sample_rate; > media_format->bit_width = cfg->bit_width; > media_format->num_channels = cfg->num_channels; > - > - if (num_channels == 1) { > - media_format->channel_mapping[0] = PCM_CHANNEL_FL; > - } else if (num_channels == 2) { > - media_format->channel_mapping[0] = PCM_CHANNEL_FL; > - media_format->channel_mapping[1] = PCM_CHANNEL_FR; > - } else if (num_channels == 4) { > - media_format->channel_mapping[0] = PCM_CHANNEL_FL; > - media_format->channel_mapping[1] = PCM_CHANNEL_FR; > - media_format->channel_mapping[2] = PCM_CHANNEL_LS; > - media_format->channel_mapping[3] = PCM_CHANNEL_RS; > - } > + for (i = 0; i < num_channels; i++) > + media_format->channel_mapping[i] = cfg->channel_map[i]; > > rc = q6apm_send_cmd_sync(graph->apm, pkt, 0); > > @@ -883,9 +874,6 @@ static int audioreach_set_compr_media_format(struct media_format *media_fmt_hdr, > mp3_cfg->q_factor = mcfg->bit_width - 1; > mp3_cfg->endianness = PCM_LITTLE_ENDIAN; > mp3_cfg->num_channels = mcfg->num_channels; > - > - audioreach_set_channel_mapping(mp3_cfg->channel_mapping, > - mcfg->num_channels); > break; > case SND_AUDIOCODEC_AAC: > media_fmt_hdr->data_format = DATA_FORMAT_RAW_COMPRESSED; > @@ -1104,9 +1092,7 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph, > media_cfg->num_channels = mcfg->num_channels; > media_cfg->q_factor = mcfg->bit_width - 1; > media_cfg->bits_per_sample = mcfg->bit_width; > - > - audioreach_set_channel_mapping(media_cfg->channel_mapping, > - num_channels); > + memcpy(media_cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels); > > rc = q6apm_send_cmd_sync(graph->apm, pkt, 0); > > @@ -1163,9 +1149,7 @@ static int audioreach_shmem_set_media_format(struct q6apm_graph *graph, > cfg->q_factor = mcfg->bit_width - 1; > cfg->endianness = PCM_LITTLE_ENDIAN; > cfg->num_channels = mcfg->num_channels; > - > - audioreach_set_channel_mapping(cfg->channel_mapping, > - num_channels); > + memcpy(cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels); > } else { > rc = audioreach_set_compr_media_format(header, p, mcfg); > if (rc) { > diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h > index eb9306280988..208b74e50445 100644 > --- a/sound/soc/qcom/qdsp6/audioreach.h > +++ b/sound/soc/qcom/qdsp6/audioreach.h > @@ -766,6 +766,7 @@ struct audioreach_module_config { > /* Packet Allocation routines */ > void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode, uint32_t > token); > +void audioreach_set_channel_mapping(u8 *ch_map, int num_channels); > void *audioreach_alloc_cmd_pkt(int payload_size, uint32_t opcode, > uint32_t token, uint32_t src_port, > uint32_t dest_port); > diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c > index 00bbd291be5c..8ab55869e8a2 100644 > --- a/sound/soc/qcom/qdsp6/q6apm-dai.c > +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c > @@ -243,6 +243,7 @@ static int q6apm_dai_prepare(struct snd_soc_component *component, > cfg.num_channels = runtime->channels; > cfg.bit_width = prtd->bits_per_sample; > cfg.fmt = SND_AUDIOCODEC_PCM; > + audioreach_set_channel_mapping(cfg.channel_map, runtime->channels); > Prepare can be called multiple times.. so we have channels overwritten here. --srini > if (prtd->state) { > /* clear the previous setup if any */ > @@ -669,6 +670,7 @@ static int q6apm_dai_compr_set_params(struct snd_soc_component *component, > cfg.num_channels = 2; > cfg.bit_width = prtd->bits_per_sample; > cfg.fmt = codec->id; > + audioreach_set_channel_mapping(cfg.channel_map, cfg.num_channels); > memcpy(&cfg.codec, codec, sizeof(*codec)); > > ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg); > diff --git a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c > index a4ad1d0e6abd..8340e4fb78f4 100644 > --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c > +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c > @@ -106,6 +106,7 @@ static int q6hdmi_hw_params(struct snd_pcm_substream *substream, > cfg->bit_width = params_width(params); > cfg->sample_rate = params_rate(params); > cfg->num_channels = channels; > + audioreach_set_channel_mapping(cfg->channel_map, channels); > > switch (dai->id) { > case DISPLAY_PORT_RX_0: > @@ -130,10 +131,12 @@ static int q6dma_hw_params(struct snd_pcm_substream *substream, > { > struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev); > struct audioreach_module_config *cfg = &dai_data->module_config[dai->id]; > + int channels = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS)->max; > > cfg->bit_width = params_width(params); > cfg->sample_rate = params_rate(params); > - cfg->num_channels = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS)->max; > + cfg->num_channels = channels; > + audioreach_set_channel_mapping(cfg->channel_map, channels); > > return 0; > } >
On 09/05/2024 11:17, Srinivas Kandagatla wrote: >> diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h >> index eb9306280988..208b74e50445 100644 >> --- a/sound/soc/qcom/qdsp6/audioreach.h >> +++ b/sound/soc/qcom/qdsp6/audioreach.h >> @@ -766,6 +766,7 @@ struct audioreach_module_config { >> /* Packet Allocation routines */ >> void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode, uint32_t >> token); >> +void audioreach_set_channel_mapping(u8 *ch_map, int num_channels); >> void *audioreach_alloc_cmd_pkt(int payload_size, uint32_t opcode, >> uint32_t token, uint32_t src_port, >> uint32_t dest_port); >> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c >> index 00bbd291be5c..8ab55869e8a2 100644 >> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c >> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c >> @@ -243,6 +243,7 @@ static int q6apm_dai_prepare(struct snd_soc_component *component, >> cfg.num_channels = runtime->channels; >> cfg.bit_width = prtd->bits_per_sample; >> cfg.fmt = SND_AUDIOCODEC_PCM; >> + audioreach_set_channel_mapping(cfg.channel_map, runtime->channels); >> > > Prepare can be called multiple times.. so we have channels overwritten here. Which is expected - just like we overwrite number of channels. Best regards, Krzysztof
On 09/05/2024 12:43, Krzysztof Kozlowski wrote: > On 09/05/2024 11:17, Srinivas Kandagatla wrote: >>> diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h >>> index eb9306280988..208b74e50445 100644 >>> --- a/sound/soc/qcom/qdsp6/audioreach.h >>> +++ b/sound/soc/qcom/qdsp6/audioreach.h >>> @@ -766,6 +766,7 @@ struct audioreach_module_config { >>> /* Packet Allocation routines */ >>> void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode, uint32_t >>> token); >>> +void audioreach_set_channel_mapping(u8 *ch_map, int num_channels); >>> void *audioreach_alloc_cmd_pkt(int payload_size, uint32_t opcode, >>> uint32_t token, uint32_t src_port, >>> uint32_t dest_port); >>> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c >>> index 00bbd291be5c..8ab55869e8a2 100644 >>> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c >>> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c >>> @@ -243,6 +243,7 @@ static int q6apm_dai_prepare(struct snd_soc_component *component, >>> cfg.num_channels = runtime->channels; >>> cfg.bit_width = prtd->bits_per_sample; >>> cfg.fmt = SND_AUDIOCODEC_PCM; >>> + audioreach_set_channel_mapping(cfg.channel_map, runtime->channels); >>> >> >> Prepare can be called multiple times.. so we have channels overwritten here. > > Which is expected - just like we overwrite number of channels. This will work in q6apm-dai.c case as there is no set_channel_map callback. lgtm. Can you rename audioreach_set_channel_mapping to audioreach_set_default_channel_mapping which makes it more obvious that we are setting a default channel mappings. --srini > > Best regards, > Krzysztof >
diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c index 5291deac0a0b..750b8ba64211 100644 --- a/sound/soc/qcom/qdsp6/audioreach.c +++ b/sound/soc/qcom/qdsp6/audioreach.c @@ -267,7 +267,7 @@ void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode, uint32_t token } EXPORT_SYMBOL_GPL(audioreach_alloc_apm_cmd_pkt); -static void audioreach_set_channel_mapping(u8 *ch_map, int num_channels) +void audioreach_set_channel_mapping(u8 *ch_map, int num_channels) { if (num_channels == 1) { ch_map[0] = PCM_CHANNEL_FL; @@ -281,6 +281,7 @@ static void audioreach_set_channel_mapping(u8 *ch_map, int num_channels) ch_map[3] = PCM_CHANNEL_RS; } } +EXPORT_SYMBOL_GPL(audioreach_set_channel_mapping); static void apm_populate_container_config(struct apm_container_obj *cfg, struct audioreach_container *cont) @@ -819,7 +820,7 @@ static int audioreach_mfc_set_media_format(struct q6apm_graph *graph, uint32_t num_channels = cfg->num_channels; int payload_size; struct gpr_pkt *pkt; - int rc; + int rc, i; void *p; payload_size = APM_MFC_CFG_PSIZE(media_format, num_channels) + @@ -842,18 +843,8 @@ static int audioreach_mfc_set_media_format(struct q6apm_graph *graph, media_format->sample_rate = cfg->sample_rate; media_format->bit_width = cfg->bit_width; media_format->num_channels = cfg->num_channels; - - if (num_channels == 1) { - media_format->channel_mapping[0] = PCM_CHANNEL_FL; - } else if (num_channels == 2) { - media_format->channel_mapping[0] = PCM_CHANNEL_FL; - media_format->channel_mapping[1] = PCM_CHANNEL_FR; - } else if (num_channels == 4) { - media_format->channel_mapping[0] = PCM_CHANNEL_FL; - media_format->channel_mapping[1] = PCM_CHANNEL_FR; - media_format->channel_mapping[2] = PCM_CHANNEL_LS; - media_format->channel_mapping[3] = PCM_CHANNEL_RS; - } + for (i = 0; i < num_channels; i++) + media_format->channel_mapping[i] = cfg->channel_map[i]; rc = q6apm_send_cmd_sync(graph->apm, pkt, 0); @@ -883,9 +874,6 @@ static int audioreach_set_compr_media_format(struct media_format *media_fmt_hdr, mp3_cfg->q_factor = mcfg->bit_width - 1; mp3_cfg->endianness = PCM_LITTLE_ENDIAN; mp3_cfg->num_channels = mcfg->num_channels; - - audioreach_set_channel_mapping(mp3_cfg->channel_mapping, - mcfg->num_channels); break; case SND_AUDIOCODEC_AAC: media_fmt_hdr->data_format = DATA_FORMAT_RAW_COMPRESSED; @@ -1104,9 +1092,7 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph, media_cfg->num_channels = mcfg->num_channels; media_cfg->q_factor = mcfg->bit_width - 1; media_cfg->bits_per_sample = mcfg->bit_width; - - audioreach_set_channel_mapping(media_cfg->channel_mapping, - num_channels); + memcpy(media_cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels); rc = q6apm_send_cmd_sync(graph->apm, pkt, 0); @@ -1163,9 +1149,7 @@ static int audioreach_shmem_set_media_format(struct q6apm_graph *graph, cfg->q_factor = mcfg->bit_width - 1; cfg->endianness = PCM_LITTLE_ENDIAN; cfg->num_channels = mcfg->num_channels; - - audioreach_set_channel_mapping(cfg->channel_mapping, - num_channels); + memcpy(cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels); } else { rc = audioreach_set_compr_media_format(header, p, mcfg); if (rc) { diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h index eb9306280988..208b74e50445 100644 --- a/sound/soc/qcom/qdsp6/audioreach.h +++ b/sound/soc/qcom/qdsp6/audioreach.h @@ -766,6 +766,7 @@ struct audioreach_module_config { /* Packet Allocation routines */ void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode, uint32_t token); +void audioreach_set_channel_mapping(u8 *ch_map, int num_channels); void *audioreach_alloc_cmd_pkt(int payload_size, uint32_t opcode, uint32_t token, uint32_t src_port, uint32_t dest_port); diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c index 00bbd291be5c..8ab55869e8a2 100644 --- a/sound/soc/qcom/qdsp6/q6apm-dai.c +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c @@ -243,6 +243,7 @@ static int q6apm_dai_prepare(struct snd_soc_component *component, cfg.num_channels = runtime->channels; cfg.bit_width = prtd->bits_per_sample; cfg.fmt = SND_AUDIOCODEC_PCM; + audioreach_set_channel_mapping(cfg.channel_map, runtime->channels); if (prtd->state) { /* clear the previous setup if any */ @@ -669,6 +670,7 @@ static int q6apm_dai_compr_set_params(struct snd_soc_component *component, cfg.num_channels = 2; cfg.bit_width = prtd->bits_per_sample; cfg.fmt = codec->id; + audioreach_set_channel_mapping(cfg.channel_map, cfg.num_channels); memcpy(&cfg.codec, codec, sizeof(*codec)); ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg); diff --git a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c index a4ad1d0e6abd..8340e4fb78f4 100644 --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c @@ -106,6 +106,7 @@ static int q6hdmi_hw_params(struct snd_pcm_substream *substream, cfg->bit_width = params_width(params); cfg->sample_rate = params_rate(params); cfg->num_channels = channels; + audioreach_set_channel_mapping(cfg->channel_map, channels); switch (dai->id) { case DISPLAY_PORT_RX_0: @@ -130,10 +131,12 @@ static int q6dma_hw_params(struct snd_pcm_substream *substream, { struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev); struct audioreach_module_config *cfg = &dai_data->module_config[dai->id]; + int channels = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS)->max; cfg->bit_width = params_width(params); cfg->sample_rate = params_rate(params); - cfg->num_channels = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS)->max; + cfg->num_channels = channels; + audioreach_set_channel_mapping(cfg->channel_map, channels); return 0; }
When constructing packets to DSP, the Audioreach code uses 'struct audioreach_module_config' to configure parameters like number of channels, bitrate, sample rate etc, but uses defaults for the channel mapping. Rework this code to copy the channel mapping from 'struct audioreach_module_config', instead of using the default. This requires all callers to fill that structure: add missing initialization of channel mapping. Entire patch makes code more logical and easier to follow: 1. q6apm-dai and q6apm-lpass-dais code which allocates 'struct audioreach_module_config' initializes it fully, so fills both the number of channels and the channel mapping. 2. Audioreach code, which uses 'struct audioreach_module_config' when constructing packets, copies entire contents of passed config, not only pieces of it. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Changes in v2: 1. Extend commit msg, rationale. 2. Rework to new approach, but most of the code stays. 3. Export audioreach_set_channel_mapping() (needed by Q6APM DAIS and LPASS DAIS) 4. Correct channel mapping also in audioreach_mfc_set_media_format(), because MFC DAI is now part of backend. 5. Do not adjust dynamic DAIs (drop audioreach_dai_load()). --- sound/soc/qcom/qdsp6/audioreach.c | 30 +++++++----------------------- sound/soc/qcom/qdsp6/audioreach.h | 1 + sound/soc/qcom/qdsp6/q6apm-dai.c | 2 ++ sound/soc/qcom/qdsp6/q6apm-lpass-dais.c | 5 ++++- 4 files changed, 14 insertions(+), 24 deletions(-)