| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d3c78e43-44a9-4ef2-8e64-00f39b32172c@linaro.org>
Date: Thu, 9 May 2024 10:17:21 +0100
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
James Schulman <james.schulman@...rus.com>,
David Rhodes <david.rhodes@...rus.com>,
Richard Fitzgerald <rf@...nsource.cirrus.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>, Nuno Sá
<nuno.sa@...log.com>, Banajit Goswami <bgoswami@...cinc.com>
Cc: alsa-devel@...a-project.org, patches@...nsource.cirrus.com,
linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/4] ASoC: qcom: qdsp6: Set channel mapping instead of
fixed defaults
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@...aro.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;
> }
>
Powered by blists - more mailing lists