| Message ID | 20210927135559.738-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: qcom: Add AudioReach support | expand |
> +struct apm_sub_graph_params { > + struct apm_module_param_data param_data; > + uint32_t num_sub_graphs; > + struct apm_sub_graph_data sg_cfg[]; > +} __packed; The style you use is num_foobar and later foobar[] > +/* container config */ > +struct apm_container_obj { > + struct apm_container_cfg container_cfg; > + /* Capability ID list */ > + struct apm_prop_data cap_data; > + uint32_t num_capability_id; > + uint32_t capability_id; but here you have both a num_capability_id and capability_id It's not very clear what they mean, or if there is a dependency? > + /* Container graph Position */ > + struct apm_prop_data pos_data; > + struct apm_cont_prop_id_graph_pos pos; > + > + /* Container Stack size */ > + struct apm_prop_data stack_data; > + struct apm_cont_prop_id_stack_size stack; > + > + /* Container proc domain id */ > + struct apm_prop_data domain_data; > + struct apm_cont_prop_id_domain domain; > +} __packed; > +/* Module IDs */ > +#define MODULE_ID_WR_SHARED_MEM_EP 0x07001000 > +#define MODULE_ID_RD_SHARED_MEM_EP 0x07001001 > +#define MODULE_ID_GAIN 0x07001002 > +#define MODULE_ID_PCM_CNV 0x07001003 > +#define MODULE_ID_PCM_ENC 0x07001004 > +#define MODULE_ID_PCM_DEC 0x07001005 > +#define MODULE_ID_CODEC_DMA_SINK 0x07001023 > +#define MODULE_ID_CODEC_DMA_SOURCE 0x07001024 > +#define MODULE_ID_I2S_SINK 0x0700100A > +#define MODULE_ID_I2S_SOURCE 0x0700100b > +#define MODULE_ID_DATA_LOGGING 0x0700101A > + > +#define APM_CMD_GET_SPF_STATE 0x01001021 > +#define APM_CMD_RSP_GET_SPF_STATE 0x02001007 > + > +#define APM_MODULE_INSTANCE_ID 0x00000001 > +#define PRM_MODULE_INSTANCE_ID 0x00000002 > +#define AMDB_MODULE_INSTANCE_ID 0x00000003 > +#define VCPM_MODULE_INSTANCE_ID 0x00000004 > +#define AR_MODULE_INSTANCE_ID_START 0x00006000 > +#define AR_MODULE_INSTANCE_ID_END 0x00007000 > +#define AR_MODULE_DYNAMIC_INSTANCE_ID_START 0x00007000 > +#define AR_MODULE_DYNAMIC_INSTANCE_ID_END 0x00008000 > +#define AR_CONT_INSTANCE_ID_START 0x00005000 > +#define AR_CONT_INSTANCE_ID_END 0x00006000 > +#define AR_SG_INSTANCE_ID_START 0x00004000 > + > +#define APM_CMD_GRAPH_OPEN 0x01001000 > +#define APM_CMD_GRAPH_PREPARE 0x01001001 > +#define APM_CMD_GRAPH_START 0x01001002 > +#define APM_CMD_GRAPH_STOP 0x01001003 > +#define APM_CMD_GRAPH_CLOSE 0x01001004 > +#define APM_CMD_GRAPH_FLUSH 0x01001005 > +#define APM_CMD_SET_CFG 0x01001006 > +#define APM_CMD_GET_CFG 0x01001007 > +#define APM_CMD_SHARED_MEM_MAP_REGIONS 0x0100100c > +#define APM_CMD_SHARED_MEM_UNMAP_REGIONS 0x0100100d > +#define APM_CMD_RSP_SHARED_MEM_MAP_REGIONS 0x02001001 > +#define APM_CMD_RSP_GET_CFG 0x02001000 > +#define APM_CMD_CLOSE_ALL 0x01001013 > +#define APM_CMD_REGISTER_SHARED_CFG 0x0100100A > +/* APM module */ > +#define APM_PARAM_ID_SUB_GRAPH_LIST 0x08001005 > + > +#define APM_PARAM_ID_MODULE_LIST 0x08001002 > +#define APM_PARAM_ID_MODULE_PROP 0x08001003 It seems like those definition follow a pattern, e.g. bits 28..32 a type and bits 0..15 a token? >
> +static int audioreach_shmem_set_media_format(struct q6apm_graph *graph, > + struct audioreach_module *module, > + struct audioreach_module_config *mcfg) > +{ > + uint32_t num_channels = mcfg->num_channels; > + struct apm_module_param_data *param_data; > + struct payload_media_fmt_pcm *cfg; > + struct media_format *header; > + int rc, payload_size; > + struct gpr_pkt *pkt; > + void *p; > + > + if (num_channels > 2) { > + dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels); > + return -EINVAL; > + } so here mcfg->num_channels > 2 is flagged as an error, but ... > + > + payload_size = APM_SHMEM_FMT_CFG_PSIZE(num_channels) + APM_MODULE_PARAM_DATA_SIZE; > + > + pkt = audioreach_alloc_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0, > + graph->port->id, module->instance_id); > + if (IS_ERR(pkt)) > + return PTR_ERR(pkt); > + > + p = (void *)pkt + GPR_HDR_SIZE + APM_CMD_HDR_SIZE; > + > + param_data = p; > + param_data->module_instance_id = module->instance_id; > + param_data->error_code = 0; > + param_data->param_id = PARAM_ID_MEDIA_FORMAT; > + param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE; > + p = p + APM_MODULE_PARAM_DATA_SIZE; > + > + header = p; > + header->data_format = DATA_FORMAT_FIXED_POINT; > + header->fmt_id = MEDIA_FMT_ID_PCM; > + header->payload_size = payload_size - sizeof(*header); > + > + p = p + sizeof(*header); > + cfg = p; > + cfg->sample_rate = mcfg->sample_rate; > + cfg->bit_width = mcfg->bit_width; > + cfg->alignment = PCM_LSB_ALIGNED; > + cfg->bits_per_sample = mcfg->bit_width; > + cfg->q_factor = mcfg->bit_width - 1; > + cfg->endianness = PCM_LITTLE_ENDIAN; > + cfg->num_channels = mcfg->num_channels; > + > + if (mcfg->num_channels == 1) { > + cfg->channel_mapping[0] = PCM_CHANNEL_L; > + } else if (num_channels == 2) { > + cfg->channel_mapping[0] = PCM_CHANNEL_L; > + cfg->channel_mapping[1] = PCM_CHANNEL_R; > + } else { > + dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels); > + rc = -EINVAL; > + goto err; ... this is again the case where mcfg->num_channels > 2 so this block is never executed. > + } > + > + rc = audioreach_graph_send_cmd_sync(graph, pkt, 0); > +err: > + kfree(pkt); > + > + return rc; > +} > +
> +static int audioreach_control_load_mix(struct snd_soc_component *scomp, > + struct snd_ar_control *scontrol, > + struct snd_kcontrol_new *kc, > + struct snd_soc_tplg_ctl_hdr *hdr) > +{ > + struct snd_soc_tplg_vendor_value_elem *c_elem; > + struct snd_soc_tplg_vendor_array *c_array; > + struct snd_soc_tplg_mixer_control *mc; > + int tkn_count = 0; > + > + mc = container_of(hdr, struct snd_soc_tplg_mixer_control, hdr); > + c_array = (struct snd_soc_tplg_vendor_array *)mc->priv.data; > + > + c_elem = c_array->value; > + > + while (tkn_count <= (le32_to_cpu(c_array->num_elems) - 1)) { > + switch (le32_to_cpu(c_elem->token)) { > + case AR_TKN_U32_SUB_GRAPH_INSTANCE_ID: > + scontrol->sgid = le32_to_cpu(c_elem->value); > + break; > + default: > + /* Ignore other tokens */ > + break; indentation still off > + > + } > + c_elem++; > + tkn_count++; > + } > + > + return 0; > +}
> +static int q6apm_dai_prepare(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct q6apm_dai_rtd *prtd = runtime->private_data; > + struct audioreach_module_config cfg; > + struct device *dev = component->dev; > + struct q6apm_dai_data *pdata; > + int ret; > + > + pdata = snd_soc_component_get_drvdata(component); > + if (!pdata) > + return -EINVAL; > + > + if (!prtd || !prtd->graph) { > + dev_err(dev, "%s: private data null or audio client freed\n", __func__); > + return -EINVAL; > + } > + > + cfg.direction = substream->stream; > + cfg.sample_rate = runtime->rate; > + cfg.num_channels = runtime->channels; > + cfg.bit_width = prtd->bits_per_sample; > + > + prtd->pcm_count = snd_pcm_lib_period_bytes(substream); > + prtd->pos = 0; > + /* rate and channels are sent to audio driver */ > + ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg); > + if (ret < 0) { > + dev_err(dev, "%s: q6apm_open_write failed\n", __func__); > + return ret; > + } > + > + ret = q6apm_graph_media_format_pcm(prtd->graph, &cfg); > + if (ret < 0) > + dev_err(dev, "%s: CMD Format block failed\n", __func__); > + > + ret = q6apm_map_memory_regions(prtd->graph, substream->stream, prtd->phys, > + (prtd->pcm_size / prtd->periods), prtd->periods); > + > + if (ret < 0) { > + dev_err(dev, "Audio Start: Buffer Allocation failed rc = %d\n", ret); > + return -ENOMEM; > + } > + > + ret = q6apm_graph_prepare(prtd->graph); > + if (ret) { > + dev_err(dev, "Failed to prepare Graph %d\n", ret); > + return ret; > + } > + > + ret = q6apm_graph_start(prtd->graph); > + if (ret) { > + dev_err(dev, "Failed to Start Graph %d\n", ret); > + return ret; > + } > + > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { > + int i; > + /* Queue the buffers for Capture ONLY after graph is started */ > + for (i = 0; i < runtime->periods; i++) > + q6apm_read(prtd->graph); > + > + } > + > + prtd->state = Q6APM_STREAM_RUNNING; you should probably explain why a stream moves to the 'RUNNING' state in a .prepare() callback, instead of TRIGGER_START? > + > + return 0; > +} > + > +static int q6apm_dai_trigger(struct snd_soc_component *component, > + struct snd_pcm_substream *substream, int cmd) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct q6apm_dai_rtd *prtd = runtime->private_data; > + int ret = 0; > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + /* start writing buffers for playback only as we already queued capture buffers */ > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > + ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0); > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + /* TODO support be handled via SoftPause Module */ > + prtd->state = Q6APM_STREAM_STOPPED; > + break; > + case SNDRV_PCM_TRIGGER_SUSPEND: > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +}
> Srinivas Kandagatla (22): > soc: dt-bindings: qcom: apr: convert to yaml > soc: dt-bindings: qcom: apr: deprecate qcom,apr-domain property > soc: qcom: apr: make code more reuseable > soc: dt-bindings: qcom: add gpr bindings > soc: qcom: apr: Add GPR support > ASoC: dt-bindings: move LPASS dai related bindings out of q6afe > ASoC: dt-bindings: move LPASS clocks related bindings out of q6afe > ASoC: dt-bindings: rename q6afe.h to q6dsp-lpass-ports.h > ASoC: qdsp6: q6afe-dai: move lpass audio ports to common file > ASoC: qdsp6: q6afe-clocks: move audio-clocks to common file > ASoC: dt-bindings: q6dsp: add q6apm-lpass-dai compatible > ASoC: dt-bindings: lpass-clocks: add q6prm clocks compatible > ASoC: dt-bindings: add q6apm digital audio stream bindings > ASoC: qdsp6: audioreach: add basic pkt alloc support > ASoC: qdsp6: audioreach: add q6apm support > ASoC: qdsp6: audioreach: add module configuration command helpers > ASoC: qdsp6: audioreach: add Kconfig and Makefile > ASoC: qdsp6: audioreach: add topology support > ASoC: qdsp6: audioreach: add q6apm-dai support > ASoC: qdsp6: audioreach: add q6apm lpass dai support > ASoC: qdsp6: audioreach: add q6prm support > ASoC: qdsp6: audioreach: add support for q6prm-clocks I added a couple of minor comments, at this point other reviewers should start looking at this patchset. For patches 5, 9, 10, 14..22 (everything except dt-bindings which I didn't even look at): Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
On Mon 27 Sep 08:55 CDT 2021, Srinivas Kandagatla wrote: [..] > Srinivas Kandagatla (22): > soc: dt-bindings: qcom: apr: convert to yaml > soc: dt-bindings: qcom: apr: deprecate qcom,apr-domain property > soc: qcom: apr: make code more reuseable > soc: dt-bindings: qcom: add gpr bindings > soc: qcom: apr: Add GPR support These patches has been merged into the Qualcomm drivers-for-5.16 tree and an immutable tag is available at: https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git tags/20210927135559.738-6-srinivas.kandagatla@linaro.org Regards, Bjorn
On Mon, 27 Sep 2021 14:55:37 +0100, Srinivas Kandagatla wrote: > Many thanks for reviewing v7 This version addresses all the comments > raised as part of v8 review. > > This patchset adds ASoC driver support to configure signal processing > framework ("AudioReach") which is integral part of Qualcomm next > generation audio SDK and will be deployed on upcoming Qualcomm chipsets. > It makes use of ASoC Topology to load graphs on to the DSP which is then > managed by APM (Audio Processing Manager) service to prepare/start/stop. > > [...] Applied, thanks! [01/22] soc: dt-bindings: qcom: apr: convert to yaml commit: 985f62a9a13175217978a797cd8f1f26216b2c87 [02/22] soc: dt-bindings: qcom: apr: deprecate qcom,apr-domain property commit: 1ff63d5465d0b0bf4e69562096b2d3ec9ff1a116 [03/22] soc: qcom: apr: make code more reuseable commit: 99139b80c1b3d73026ed8be2de42c52e2976ab64 [04/22] soc: dt-bindings: qcom: add gpr bindings commit: 974c6faf7667e551d202712470ca210c14ca249d [05/22] soc: qcom: apr: Add GPR support commit: ec1471a898cca38af6b8956a83ebc1297214546f Best regards,
On 9/27/2021 3:55 PM, Srinivas Kandagatla wrote: > Add basic helper functions for AudioReach. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/qdsp6/audioreach.c | 265 ++++++++++++ > sound/soc/qcom/qdsp6/audioreach.h | 668 ++++++++++++++++++++++++++++++ > 2 files changed, 933 insertions(+) > create mode 100644 sound/soc/qcom/qdsp6/audioreach.c > create mode 100644 sound/soc/qcom/qdsp6/audioreach.h > > diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c > new file mode 100644 > index 000000000000..34ec4c0d0175 > --- /dev/null > +++ b/sound/soc/qcom/qdsp6/audioreach.c > @@ -0,0 +1,265 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2020, Linaro Limited > + > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/soc/qcom/apr.h> > +#include <dt-bindings/soc/qcom,gpr.h> > +#include "audioreach.h" > + > +/* SubGraph Config */ > +struct apm_sub_graph_data { > + struct apm_sub_graph_cfg sub_graph_cfg; > + struct apm_prop_data perf_data; > + struct apm_sg_prop_id_perf_mode perf; > + struct apm_prop_data dir_data; > + struct apm_sg_prop_id_direction dir; > + struct apm_prop_data sid_data; > + struct apm_sg_prop_id_scenario_id sid; > + > +} __packed; > + > +#define APM_SUB_GRAPH_CFG_NPROP 3 > + > +struct apm_sub_graph_params { > + struct apm_module_param_data param_data; > + uint32_t num_sub_graphs; > + struct apm_sub_graph_data sg_cfg[]; > +} __packed; > + > +#define APM_SUB_GRAPH_PSIZE(n) ALIGN(sizeof(struct apm_sub_graph_params) + \ > + n * sizeof(struct apm_sub_graph_data), 8) This looks, like you could use struct_size helper, something like: #define APM_SUB_GRAPH_PSIZE(n) ALIGN(struct_size(apm_sub_graph_params, apm_sub_graph_data, n), 8) > +/* container config */ > +struct apm_container_obj { > + struct apm_container_cfg container_cfg; > + /* Capability ID list */ > + struct apm_prop_data cap_data; > + uint32_t num_capability_id; > + uint32_t capability_id; > + > + /* Container graph Position */ > + struct apm_prop_data pos_data; > + struct apm_cont_prop_id_graph_pos pos; > + > + /* Container Stack size */ > + struct apm_prop_data stack_data; > + struct apm_cont_prop_id_stack_size stack; > + > + /* Container proc domain id */ > + struct apm_prop_data domain_data; > + struct apm_cont_prop_id_domain domain; > +} __packed; > + > +struct apm_container_params { > + struct apm_module_param_data param_data; > + uint32_t num_containers; > + struct apm_container_obj cont_obj[]; > +} __packed; > + > +#define APM_CONTAINER_PSIZE(n) ALIGN(sizeof(struct apm_container_params) + \ > + n * sizeof(struct apm_container_obj), 8) > + Same here, use struct_size, and same applies for other defines in this file, I won't add comment to all of them ;) > +/* Module List config */ > +struct apm_mod_list_obj { > + /* Modules list cfg */ > + uint32_t sub_graph_id; > + uint32_t container_id; > + uint32_t num_modules; > + struct apm_module_obj mod_cfg[]; > +} __packed; > + > +struct apm_module_list_params { > + struct apm_module_param_data param_data; > + uint32_t num_modules_list; > + /* Module list config array */ > + struct apm_mod_list_obj mod_list_obj[]; > +} __packed; > + (...) > diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h > new file mode 100644 > index 000000000000..556443155416 > --- /dev/null > +++ b/sound/soc/qcom/qdsp6/audioreach.h > @@ -0,0 +1,668 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef __AUDIOREACH_H__ > +#define __AUDIOREACH_H__ > +#include <linux/types.h> > +#include <linux/soc/qcom/apr.h> > +#include <sound/soc.h> > + > +/* Module IDs */ > +#define MODULE_ID_WR_SHARED_MEM_EP 0x07001000 > +#define MODULE_ID_RD_SHARED_MEM_EP 0x07001001 > +#define MODULE_ID_GAIN 0x07001002 > +#define MODULE_ID_PCM_CNV 0x07001003 > +#define MODULE_ID_PCM_ENC 0x07001004 > +#define MODULE_ID_PCM_DEC 0x07001005 > +#define MODULE_ID_CODEC_DMA_SINK 0x07001023 > +#define MODULE_ID_CODEC_DMA_SOURCE 0x07001024 > +#define MODULE_ID_I2S_SINK 0x0700100A > +#define MODULE_ID_I2S_SOURCE 0x0700100b Small 'b' in hex value, use 'B' for consistency > +#define MODULE_ID_DATA_LOGGING 0x0700101A > + > +#define APM_CMD_GET_SPF_STATE 0x01001021 > +#define APM_CMD_RSP_GET_SPF_STATE 0x02001007 > + > +#define APM_MODULE_INSTANCE_ID 0x00000001 > +#define PRM_MODULE_INSTANCE_ID 0x00000002 > +#define AMDB_MODULE_INSTANCE_ID 0x00000003 > +#define VCPM_MODULE_INSTANCE_ID 0x00000004 > +#define AR_MODULE_INSTANCE_ID_START 0x00006000 > +#define AR_MODULE_INSTANCE_ID_END 0x00007000 > +#define AR_MODULE_DYNAMIC_INSTANCE_ID_START 0x00007000 > +#define AR_MODULE_DYNAMIC_INSTANCE_ID_END 0x00008000 > +#define AR_CONT_INSTANCE_ID_START 0x00005000 > +#define AR_CONT_INSTANCE_ID_END 0x00006000 > +#define AR_SG_INSTANCE_ID_START 0x00004000 > + > +#define APM_CMD_GRAPH_OPEN 0x01001000 > +#define APM_CMD_GRAPH_PREPARE 0x01001001 > +#define APM_CMD_GRAPH_START 0x01001002 > +#define APM_CMD_GRAPH_STOP 0x01001003 > +#define APM_CMD_GRAPH_CLOSE 0x01001004 > +#define APM_CMD_GRAPH_FLUSH 0x01001005 > +#define APM_CMD_SET_CFG 0x01001006 > +#define APM_CMD_GET_CFG 0x01001007 > +#define APM_CMD_SHARED_MEM_MAP_REGIONS 0x0100100c Small 'c' here, use 'C' > +#define APM_CMD_SHARED_MEM_UNMAP_REGIONS 0x0100100d and 'D' insted of 'd' here. > +#define APM_CMD_RSP_SHARED_MEM_MAP_REGIONS 0x02001001 > +#define APM_CMD_RSP_GET_CFG 0x02001000 > +#define APM_CMD_CLOSE_ALL 0x01001013 > +#define APM_CMD_REGISTER_SHARED_CFG 0x0100100A > + > +#define APM_MEMORY_MAP_SHMEM8_4K_POOL 3 > + > +struct apm_cmd_shared_mem_map_regions { > + uint16_t mem_pool_id; > + uint16_t num_regions; > + uint32_t property_flag; > +} __packed; > + > +struct apm_shared_map_region_payload { > + uint32_t shm_addr_lsw; > + uint32_t shm_addr_msw; > + uint32_t mem_size_bytes; > +} __packed; > + > +struct apm_cmd_shared_mem_unmap_regions { > + uint32_t mem_map_handle; > +} __packed; > + > +struct apm_cmd_rsp_shared_mem_map_regions { > + uint32_t mem_map_handle; > +} __packed; > + (...) > +#define DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER 0x04001003 > + > +struct data_cmd_rd_sh_mem_ep_data_buffer { > + uint32_t buf_addr_lsw; > + uint32_t buf_addr_msw; > + uint32_t mem_map_handle; > + uint32_t buf_size; > +}; > + > +#define DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER 0x05001002 > + > +struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done { > + uint32_t status; > + uint32_t buf_addr_lsw; > + uint32_t buf_addr_msw; > + uint32_t mem_map_handle; > + uint32_t data_size; > + uint32_t offset; > + uint32_t timestamp_lsw; > + uint32_t timestamp_msw; > + uint32_t flags; > + uint32_t num_frames; > +}; Rest of structs is marked with __packed and it seems like it describes communication protocol, so it seems like you missed it here. > + > +#define DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER_V2 0x0400100B > + > +struct data_cmd_rd_sh_mem_ep_data_buffer_v2 { > + uint32_t buf_addr_lsw; > + uint32_t buf_addr_msw; > + uint32_t mem_map_handle; > + uint32_t buf_size; > + uint32_t md_buf_addr_lsw; > + uint32_t md_buf_addr_msw; > + uint32_t md_mem_map_handle; > + uint32_t md_buf_size; > +}; Same comment as above. > + > +#define DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER_V2 0x05001005 > + > +struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done_v2 { > + uint32_t status; > + uint32_t buf_addr_lsw; > + uint32_t buf_addr_msw; > + uint32_t mem_map_handle; > + uint32_t data_size; > + uint32_t offset; > + uint32_t timestamp_lsw; > + uint32_t timestamp_msw; > + uint32_t flags; > + uint32_t num_frames; > + uint32_t md_status; > + uint32_t md_buf_addr_lsw; > + uint32_t md_buf_addr_msw; > + uint32_t md_mem_map_handle; > + uint32_t md_size; > +} __packed; > + > +#define PARAM_ID_RD_SH_MEM_CFG 0x08001007 > + > +struct param_id_rd_sh_mem_cfg { > + uint32_t num_frames_per_buffer; > + uint32_t metadata_control_flags; > + > +} __packed; > + (...)
On 9/27/2021 3:55 PM, Srinivas Kandagatla wrote: > Add support to q6apm (Audio Process Manager) component which is > core Audioreach service running in the DSP. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- (...) > +++ b/sound/soc/qcom/qdsp6/q6apm.c > @@ -0,0 +1,597 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2020, Linaro Limited > + > +#include <dt-bindings/soc/qcom,gpr.h> > +#include <linux/delay.h> > +#include <linux/jiffies.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > +#include <linux/soc/qcom/apr.h> > +#include <linux/wait.h> > +#include <sound/soc.h> > +#include <sound/soc-dapm.h> > +#include <sound/pcm.h> > +#include "audioreach.h" > +#include "q6apm.h" > + > +/* Graph Management */ > +struct apm_graph_mgmt_cmd { > + struct apm_module_param_data param_data; > + uint32_t num_sub_graphs; > + uint32_t sub_graph_id_list[]; > +} __packed; > + > +#define APM_GRAPH_MGMT_PSIZE(n) ALIGN(sizeof(struct apm_graph_mgmt_cmd) + \ > + n * sizeof(uint32_t), 8) Possible struct_size again > + > +int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt, uint32_t rsp_opcode) There seems to be 'tab' in argument list? > +{ > + gpr_device_t *gdev = apm->gdev; > + > + return audioreach_send_cmd_sync(&gdev->dev, gdev, &apm->result, &apm->lock, > + NULL, &apm->wait, pkt, rsp_opcode); > +} > + (...)
Hi Bjorn, On 28/09/2021 04:20, Bjorn Andersson wrote: > On Mon, 27 Sep 2021 14:55:37 +0100, Srinivas Kandagatla wrote: >> Many thanks for reviewing v7 This version addresses all the comments >> raised as part of v8 review. >> >> This patchset adds ASoC driver support to configure signal processing >> framework ("AudioReach") which is integral part of Qualcomm next >> generation audio SDK and will be deployed on upcoming Qualcomm chipsets. >> It makes use of ASoC Topology to load graphs on to the DSP which is then >> managed by APM (Audio Processing Manager) service to prepare/start/stop. >> >> [...] > > Applied, thanks! > > [01/22] soc: dt-bindings: qcom: apr: convert to yaml > commit: 985f62a9a13175217978a797cd8f1f26216b2c87 > [02/22] soc: dt-bindings: qcom: apr: deprecate qcom,apr-domain property > commit: 1ff63d5465d0b0bf4e69562096b2d3ec9ff1a116 > [03/22] soc: qcom: apr: make code more reuseable > commit: 99139b80c1b3d73026ed8be2de42c52e2976ab64 > [04/22] soc: dt-bindings: qcom: add gpr bindings > commit: 974c6faf7667e551d202712470ca210c14ca249d > [05/22] soc: qcom: apr: Add GPR support > commit: ec1471a898cca38af6b8956a83ebc1297214546f > Just in case you missed, rest of the series depend on some of the GRP apis in these patches. Am not sure how Mark would prefer to take the rest of series. --srini > Best regards, >
Thanks Amadeusz for reviewing, On 28/09/2021 09:00, Amadeusz Sławiński wrote: > On 9/27/2021 3:55 PM, Srinivas Kandagatla wrote: >> Add basic helper functions for AudioReach. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> sound/soc/qcom/qdsp6/audioreach.c | 265 ++++++++++++ >> sound/soc/qcom/qdsp6/audioreach.h | 668 ++++++++++++++++++++++++++++++ >> 2 files changed, 933 insertions(+) >> create mode 100644 sound/soc/qcom/qdsp6/audioreach.c >> create mode 100644 sound/soc/qcom/qdsp6/audioreach.h >> >> diff --git a/sound/soc/qcom/qdsp6/audioreach.c >> b/sound/soc/qcom/qdsp6/audioreach.c >> new file mode 100644 >> index 000000000000..34ec4c0d0175 >> --- /dev/null >> +++ b/sound/soc/qcom/qdsp6/audioreach.c >> @@ -0,0 +1,265 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2020, Linaro Limited >> + >> +#include <linux/kernel.h> >> +#include <linux/slab.h> >> +#include <linux/soc/qcom/apr.h> >> +#include <dt-bindings/soc/qcom,gpr.h> >> +#include "audioreach.h" >> + >> +/* SubGraph Config */ >> +struct apm_sub_graph_data { >> + struct apm_sub_graph_cfg sub_graph_cfg; >> + struct apm_prop_data perf_data; >> + struct apm_sg_prop_id_perf_mode perf; >> + struct apm_prop_data dir_data; >> + struct apm_sg_prop_id_direction dir; >> + struct apm_prop_data sid_data; >> + struct apm_sg_prop_id_scenario_id sid; >> + >> +} __packed; >> + >> +#define APM_SUB_GRAPH_CFG_NPROP 3 >> + >> +struct apm_sub_graph_params { >> + struct apm_module_param_data param_data; >> + uint32_t num_sub_graphs; >> + struct apm_sub_graph_data sg_cfg[]; >> +} __packed; >> + >> +#define APM_SUB_GRAPH_PSIZE(n) ALIGN(sizeof(struct >> apm_sub_graph_params) + \ >> + n * sizeof(struct apm_sub_graph_data), 8) > > This looks, like you could use struct_size helper, something like: > #define APM_SUB_GRAPH_PSIZE(n) ALIGN(struct_size(apm_sub_graph_params, > apm_sub_graph_data, n), 8) > struct_size() macro first argument is structure pointer, so above will not work as it is. I agree that we could use struct_size() in some cases, but in this particular case we will endup with more local pointer variables that are just declared to keep struct_size() macro happy. IMO, Which will not make code any better than what it is now. I will revist these and see if it makes sense to use it in other places. >> +/* container config */ >> +struct apm_container_obj { >> + struct apm_container_cfg container_cfg; >> + /* Capability ID list */ >> + struct apm_prop_data cap_data; >> + uint32_t num_capability_id; >> + uint32_t capability_id; >> + >> + /* Container graph Position */ >> + struct apm_prop_data pos_data; >> + struct apm_cont_prop_id_graph_pos pos; >> + >> + /* Container Stack size */ >> + struct apm_prop_data stack_data; >> + struct apm_cont_prop_id_stack_size stack; >> + >> + /* Container proc domain id */ >> + struct apm_prop_data domain_data; >> + struct apm_cont_prop_id_domain domain; >> +} __packed; >> + >> +struct apm_container_params { >> + struct apm_module_param_data param_data; >> + uint32_t num_containers; >> + struct apm_container_obj cont_obj[]; >> +} __packed; >> + >> +#define APM_CONTAINER_PSIZE(n) ALIGN(sizeof(struct >> apm_container_params) + \ >> + n * sizeof(struct apm_container_obj), 8) >> + > > Same here, use struct_size, and same applies for other defines in this > file, I won't add comment to all of them ;) > >> +/* Module List config */ >> +struct apm_mod_list_obj { >> + /* Modules list cfg */ >> + uint32_t sub_graph_id; >> + uint32_t container_id; >> + uint32_t num_modules; >> + struct apm_module_obj mod_cfg[]; >> +} __packed; >> + >> +struct apm_module_list_params { >> + struct apm_module_param_data param_data; >> + uint32_t num_modules_list; >> + /* Module list config array */ >> + struct apm_mod_list_obj mod_list_obj[]; >> +} __packed; >> + > > (...) > >> diff --git a/sound/soc/qcom/qdsp6/audioreach.h >> b/sound/soc/qcom/qdsp6/audioreach.h >> new file mode 100644 >> index 000000000000..556443155416 >> --- /dev/null >> +++ b/sound/soc/qcom/qdsp6/audioreach.h >> @@ -0,0 +1,668 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef __AUDIOREACH_H__ >> +#define __AUDIOREACH_H__ >> +#include <linux/types.h> >> +#include <linux/soc/qcom/apr.h> >> +#include <sound/soc.h> >> + >> +/* Module IDs */ >> +#define MODULE_ID_WR_SHARED_MEM_EP 0x07001000 >> +#define MODULE_ID_RD_SHARED_MEM_EP 0x07001001 >> +#define MODULE_ID_GAIN 0x07001002 >> +#define MODULE_ID_PCM_CNV 0x07001003 >> +#define MODULE_ID_PCM_ENC 0x07001004 >> +#define MODULE_ID_PCM_DEC 0x07001005 >> +#define MODULE_ID_CODEC_DMA_SINK 0x07001023 >> +#define MODULE_ID_CODEC_DMA_SOURCE 0x07001024 >> +#define MODULE_ID_I2S_SINK 0x0700100A >> +#define MODULE_ID_I2S_SOURCE 0x0700100b > > Small 'b' in hex value, use 'B' for consistency I agree, will try find such instances and fix them before I send out next version. > >> +#define MODULE_ID_DATA_LOGGING 0x0700101A >> + >> +#define APM_CMD_GET_SPF_STATE 0x01001021 >> +#define APM_CMD_RSP_GET_SPF_STATE 0x02001007 >> + >> +#define APM_MODULE_INSTANCE_ID 0x00000001 >> +#define PRM_MODULE_INSTANCE_ID 0x00000002 >> +#define AMDB_MODULE_INSTANCE_ID 0x00000003 >> +#define VCPM_MODULE_INSTANCE_ID 0x00000004 >> +#define AR_MODULE_INSTANCE_ID_START 0x00006000 >> +#define AR_MODULE_INSTANCE_ID_END 0x00007000 >> +#define AR_MODULE_DYNAMIC_INSTANCE_ID_START 0x00007000 >> +#define AR_MODULE_DYNAMIC_INSTANCE_ID_END 0x00008000 >> +#define AR_CONT_INSTANCE_ID_START 0x00005000 >> +#define AR_CONT_INSTANCE_ID_END 0x00006000 >> +#define AR_SG_INSTANCE_ID_START 0x00004000 >> + >> +#define APM_CMD_GRAPH_OPEN 0x01001000 >> +#define APM_CMD_GRAPH_PREPARE 0x01001001 >> +#define APM_CMD_GRAPH_START 0x01001002 >> +#define APM_CMD_GRAPH_STOP 0x01001003 >> +#define APM_CMD_GRAPH_CLOSE 0x01001004 >> +#define APM_CMD_GRAPH_FLUSH 0x01001005 >> +#define APM_CMD_SET_CFG 0x01001006 >> +#define APM_CMD_GET_CFG 0x01001007 >> +#define APM_CMD_SHARED_MEM_MAP_REGIONS 0x0100100c > > Small 'c' here, use 'C' > >> +#define APM_CMD_SHARED_MEM_UNMAP_REGIONS 0x0100100d > > and 'D' insted of 'd' here. > >> +#define APM_CMD_RSP_SHARED_MEM_MAP_REGIONS 0x02001001 >> +#define APM_CMD_RSP_GET_CFG 0x02001000 >> +#define APM_CMD_CLOSE_ALL 0x01001013 >> +#define APM_CMD_REGISTER_SHARED_CFG 0x0100100A >> + >> +#define APM_MEMORY_MAP_SHMEM8_4K_POOL 3 >> + >> +struct apm_cmd_shared_mem_map_regions { >> + uint16_t mem_pool_id; >> + uint16_t num_regions; >> + uint32_t property_flag; >> +} __packed; >> + >> +struct apm_shared_map_region_payload { >> + uint32_t shm_addr_lsw; >> + uint32_t shm_addr_msw; >> + uint32_t mem_size_bytes; >> +} __packed; >> + >> +struct apm_cmd_shared_mem_unmap_regions { >> + uint32_t mem_map_handle; >> +} __packed; >> + >> +struct apm_cmd_rsp_shared_mem_map_regions { >> + uint32_t mem_map_handle; >> +} __packed; >> + > > (...) > > >> +#define DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER 0x04001003 >> + >> +struct data_cmd_rd_sh_mem_ep_data_buffer { >> + uint32_t buf_addr_lsw; >> + uint32_t buf_addr_msw; >> + uint32_t mem_map_handle; >> + uint32_t buf_size; >> +}; >> + >> +#define DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER 0x05001002 >> + >> +struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done { >> + uint32_t status; >> + uint32_t buf_addr_lsw; >> + uint32_t buf_addr_msw; >> + uint32_t mem_map_handle; >> + uint32_t data_size; >> + uint32_t offset; >> + uint32_t timestamp_lsw; >> + uint32_t timestamp_msw; >> + uint32_t flags; >> + uint32_t num_frames; >> +}; > > Rest of structs is marked with __packed and it seems like it describes > communication protocol, so it seems like you missed it here. > These are mostly u32 it did work without packed, but I agree with you on missing __packed. Will fix such instances. --srini >> + >> +#define DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER_V2 0x0400100B >> + >> +struct data_cmd_rd_sh_mem_ep_data_buffer_v2 { >> + uint32_t buf_addr_lsw; >> + uint32_t buf_addr_msw; >> + uint32_t mem_map_handle; >> + uint32_t buf_size; >> + uint32_t md_buf_addr_lsw; >> + uint32_t md_buf_addr_msw; >> + uint32_t md_mem_map_handle; >> + uint32_t md_buf_size; >> +}; > > Same comment as above. > >> + >> +#define DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER_V2 0x05001005 >> + >> +struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done_v2 { >> + uint32_t status; >> + uint32_t buf_addr_lsw; >> + uint32_t buf_addr_msw; >> + uint32_t mem_map_handle; >> + uint32_t data_size; >> + uint32_t offset; >> + uint32_t timestamp_lsw; >> + uint32_t timestamp_msw; >> + uint32_t flags; >> + uint32_t num_frames; >> + uint32_t md_status; >> + uint32_t md_buf_addr_lsw; >> + uint32_t md_buf_addr_msw; >> + uint32_t md_mem_map_handle; >> + uint32_t md_size; >> +} __packed; >> + >> +#define PARAM_ID_RD_SH_MEM_CFG 0x08001007 >> + >> +struct param_id_rd_sh_mem_cfg { >> + uint32_t num_frames_per_buffer; >> + uint32_t metadata_control_flags; >> + >> +} __packed; >> + > > (...)
On 28/09/2021 09:23, Amadeusz Sławiński wrote: > On 9/27/2021 3:55 PM, Srinivas Kandagatla wrote: >> Add support to q6apm (Audio Process Manager) component which is >> core Audioreach service running in the DSP. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- > > (...) > >> +++ b/sound/soc/qcom/qdsp6/q6apm.c >> @@ -0,0 +1,597 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2020, Linaro Limited >> + >> +#include <dt-bindings/soc/qcom,gpr.h> >> +#include <linux/delay.h> >> +#include <linux/jiffies.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/sched.h> >> +#include <linux/slab.h> >> +#include <linux/soc/qcom/apr.h> >> +#include <linux/wait.h> >> +#include <sound/soc.h> >> +#include <sound/soc-dapm.h> >> +#include <sound/pcm.h> >> +#include "audioreach.h" >> +#include "q6apm.h" >> + >> +/* Graph Management */ >> +struct apm_graph_mgmt_cmd { >> + struct apm_module_param_data param_data; >> + uint32_t num_sub_graphs; >> + uint32_t sub_graph_id_list[]; >> +} __packed; >> + >> +#define APM_GRAPH_MGMT_PSIZE(n) ALIGN(sizeof(struct >> apm_graph_mgmt_cmd) + \ >> + n * sizeof(uint32_t), 8) > > Possible struct_size again Yes, we could use struct_size here. > >> + >> +int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt, >> uint32_t rsp_opcode) > > There seems to be 'tab' in argument list? That's true, in vi I could not spot this by just looking. > >> +{ >> + gpr_device_t *gdev = apm->gdev; >> + >> + return audioreach_send_cmd_sync(&gdev->dev, gdev, &apm->result, >> &apm->lock, >> + NULL, &apm->wait, pkt, rsp_opcode); >> +} >> + > > (...)
On 27/09/2021 17:25, Pierre-Louis Bossart wrote: > >> +static int q6apm_dai_prepare(struct snd_soc_component *component, >> + struct snd_pcm_substream *substream) >> +{ >> + struct snd_pcm_runtime *runtime = substream->runtime; >> + struct q6apm_dai_rtd *prtd = runtime->private_data; >> + struct audioreach_module_config cfg; >> + struct device *dev = component->dev; >> + struct q6apm_dai_data *pdata; >> + int ret; >> + >> + pdata = snd_soc_component_get_drvdata(component); >> + if (!pdata) >> + return -EINVAL; >> + >> + if (!prtd || !prtd->graph) { >> + dev_err(dev, "%s: private data null or audio client freed\n", __func__); >> + return -EINVAL; >> + } >> + >> + cfg.direction = substream->stream; >> + cfg.sample_rate = runtime->rate; >> + cfg.num_channels = runtime->channels; >> + cfg.bit_width = prtd->bits_per_sample; >> + >> + prtd->pcm_count = snd_pcm_lib_period_bytes(substream); >> + prtd->pos = 0; >> + /* rate and channels are sent to audio driver */ >> + ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg); >> + if (ret < 0) { >> + dev_err(dev, "%s: q6apm_open_write failed\n", __func__); >> + return ret; >> + } >> + >> + ret = q6apm_graph_media_format_pcm(prtd->graph, &cfg); >> + if (ret < 0) >> + dev_err(dev, "%s: CMD Format block failed\n", __func__); >> + >> + ret = q6apm_map_memory_regions(prtd->graph, substream->stream, prtd->phys, >> + (prtd->pcm_size / prtd->periods), prtd->periods); >> + >> + if (ret < 0) { >> + dev_err(dev, "Audio Start: Buffer Allocation failed rc = %d\n", ret); >> + return -ENOMEM; >> + } >> + >> + ret = q6apm_graph_prepare(prtd->graph); >> + if (ret) { >> + dev_err(dev, "Failed to prepare Graph %d\n", ret); >> + return ret; >> + } >> + >> + ret = q6apm_graph_start(prtd->graph); >> + if (ret) { >> + dev_err(dev, "Failed to Start Graph %d\n", ret); >> + return ret; >> + } >> + >> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { >> + int i; >> + /* Queue the buffers for Capture ONLY after graph is started */ >> + for (i = 0; i < runtime->periods; i++) >> + q6apm_read(prtd->graph); >> + >> + } >> + >> + prtd->state = Q6APM_STREAM_RUNNING; > > you should probably explain why a stream moves to the 'RUNNING' state in > a .prepare() callback, instead of TRIGGER_START? Sure, will add a comment, --srini > >> + >> + return 0; >> +} >> + >> +static int q6apm_dai_trigger(struct snd_soc_component *component, >> + struct snd_pcm_substream *substream, int cmd) >> +{ >> + struct snd_pcm_runtime *runtime = substream->runtime; >> + struct q6apm_dai_rtd *prtd = runtime->private_data; >> + int ret = 0; >> + >> + switch (cmd) { >> + case SNDRV_PCM_TRIGGER_START: >> + case SNDRV_PCM_TRIGGER_RESUME: >> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: >> + /* start writing buffers for playback only as we already queued capture buffers */ >> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) >> + ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0); >> + break; >> + case SNDRV_PCM_TRIGGER_STOP: >> + /* TODO support be handled via SoftPause Module */ >> + prtd->state = Q6APM_STREAM_STOPPED; >> + break; >> + case SNDRV_PCM_TRIGGER_SUSPEND: >> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: >> + break; >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + return ret; >> +} >
On 27/09/2021 17:21, Pierre-Louis Bossart wrote: > >> +static int audioreach_control_load_mix(struct snd_soc_component *scomp, >> + struct snd_ar_control *scontrol, >> + struct snd_kcontrol_new *kc, >> + struct snd_soc_tplg_ctl_hdr *hdr) >> +{ >> + struct snd_soc_tplg_vendor_value_elem *c_elem; >> + struct snd_soc_tplg_vendor_array *c_array; >> + struct snd_soc_tplg_mixer_control *mc; >> + int tkn_count = 0; >> + >> + mc = container_of(hdr, struct snd_soc_tplg_mixer_control, hdr); >> + c_array = (struct snd_soc_tplg_vendor_array *)mc->priv.data; >> + >> + c_elem = c_array->value; >> + >> + while (tkn_count <= (le32_to_cpu(c_array->num_elems) - 1)) { >> + switch (le32_to_cpu(c_elem->token)) { >> + case AR_TKN_U32_SUB_GRAPH_INSTANCE_ID: >> + scontrol->sgid = le32_to_cpu(c_elem->value); >> + break; >> + default: >> + /* Ignore other tokens */ >> + break; > > indentation still off > its fixed now --srini >> + >> + } >> + c_elem++; >> + tkn_count++; >> + } >> + >> + return 0; >> +}
On 27/09/2021 17:08, Pierre-Louis Bossart wrote: > > >> +struct apm_sub_graph_params { >> + struct apm_module_param_data param_data; >> + uint32_t num_sub_graphs; >> + struct apm_sub_graph_data sg_cfg[]; >> +} __packed; > > The style you use is num_foobar and later foobar[] > >> +/* container config */ >> +struct apm_container_obj { >> + struct apm_container_cfg container_cfg; >> + /* Capability ID list */ >> + struct apm_prop_data cap_data; >> + uint32_t num_capability_id; >> + uint32_t capability_id; > > but here you have both a num_capability_id and capability_id > > It's not very clear what they mean, or if there is a dependency? DSP supports multiple capabilities for a container, however for now this version of patches only support 1 capability for a container. I will add multiple capabilities once am in a position to test it. > >> + /* Container graph Position */ >> + struct apm_prop_data pos_data; >> + struct apm_cont_prop_id_graph_pos pos; >> + >> + /* Container Stack size */ >> + struct apm_prop_data stack_data; >> + struct apm_cont_prop_id_stack_size stack; >> + >> + /* Container proc domain id */ >> + struct apm_prop_data domain_data; >> + struct apm_cont_prop_id_domain domain; >> +} __packed; > >> +/* Module IDs */ >> +#define MODULE_ID_WR_SHARED_MEM_EP 0x07001000 >> +#define MODULE_ID_RD_SHARED_MEM_EP 0x07001001 >> +#define MODULE_ID_GAIN 0x07001002 >> +#define MODULE_ID_PCM_CNV 0x07001003 >> +#define MODULE_ID_PCM_ENC 0x07001004 >> +#define MODULE_ID_PCM_DEC 0x07001005 >> +#define MODULE_ID_CODEC_DMA_SINK 0x07001023 >> +#define MODULE_ID_CODEC_DMA_SOURCE 0x07001024 >> +#define MODULE_ID_I2S_SINK 0x0700100A >> +#define MODULE_ID_I2S_SOURCE 0x0700100b >> +#define MODULE_ID_DATA_LOGGING 0x0700101A >> + >> +#define APM_CMD_GET_SPF_STATE 0x01001021 >> +#define APM_CMD_RSP_GET_SPF_STATE 0x02001007 >> + >> +#define APM_MODULE_INSTANCE_ID 0x00000001 >> +#define PRM_MODULE_INSTANCE_ID 0x00000002 >> +#define AMDB_MODULE_INSTANCE_ID 0x00000003 >> +#define VCPM_MODULE_INSTANCE_ID 0x00000004 >> +#define AR_MODULE_INSTANCE_ID_START 0x00006000 >> +#define AR_MODULE_INSTANCE_ID_END 0x00007000 >> +#define AR_MODULE_DYNAMIC_INSTANCE_ID_START 0x00007000 >> +#define AR_MODULE_DYNAMIC_INSTANCE_ID_END 0x00008000 >> +#define AR_CONT_INSTANCE_ID_START 0x00005000 >> +#define AR_CONT_INSTANCE_ID_END 0x00006000 >> +#define AR_SG_INSTANCE_ID_START 0x00004000 >> + >> +#define APM_CMD_GRAPH_OPEN 0x01001000 >> +#define APM_CMD_GRAPH_PREPARE 0x01001001 >> +#define APM_CMD_GRAPH_START 0x01001002 >> +#define APM_CMD_GRAPH_STOP 0x01001003 >> +#define APM_CMD_GRAPH_CLOSE 0x01001004 >> +#define APM_CMD_GRAPH_FLUSH 0x01001005 >> +#define APM_CMD_SET_CFG 0x01001006 >> +#define APM_CMD_GET_CFG 0x01001007 >> +#define APM_CMD_SHARED_MEM_MAP_REGIONS 0x0100100c >> +#define APM_CMD_SHARED_MEM_UNMAP_REGIONS 0x0100100d >> +#define APM_CMD_RSP_SHARED_MEM_MAP_REGIONS 0x02001001 >> +#define APM_CMD_RSP_GET_CFG 0x02001000 >> +#define APM_CMD_CLOSE_ALL 0x01001013 >> +#define APM_CMD_REGISTER_SHARED_CFG 0x0100100A > >> +/* APM module */ >> +#define APM_PARAM_ID_SUB_GRAPH_LIST 0x08001005 >> + >> +#define APM_PARAM_ID_MODULE_LIST 0x08001002 > >> +#define APM_PARAM_ID_MODULE_PROP 0x08001003 > > It seems like those definition follow a pattern, e.g. bits 28..32 a type > and bits 0..15 a token? Yes, it does have a pattern, each Opcode is divided in to 3 fields. GUID OWNER 31:28 GUID Type 27:24 MAIN GUID 23:0 --srini > > >>
On 27/09/2021 17:16, Pierre-Louis Bossart wrote: > >> +static int audioreach_shmem_set_media_format(struct q6apm_graph *graph, >> + struct audioreach_module *module, >> + struct audioreach_module_config *mcfg) >> +{ >> + uint32_t num_channels = mcfg->num_channels; >> + struct apm_module_param_data *param_data; >> + struct payload_media_fmt_pcm *cfg; >> + struct media_format *header; >> + int rc, payload_size; >> + struct gpr_pkt *pkt; >> + void *p; >> + >> + if (num_channels > 2) { >> + dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels); >> + return -EINVAL; >> + } > > so here mcfg->num_channels > 2 is flagged as an error, but ... > >> + >> + payload_size = APM_SHMEM_FMT_CFG_PSIZE(num_channels) + APM_MODULE_PARAM_DATA_SIZE; >> + >> + pkt = audioreach_alloc_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0, >> + graph->port->id, module->instance_id); >> + if (IS_ERR(pkt)) >> + return PTR_ERR(pkt); >> + >> + p = (void *)pkt + GPR_HDR_SIZE + APM_CMD_HDR_SIZE; >> + >> + param_data = p; >> + param_data->module_instance_id = module->instance_id; >> + param_data->error_code = 0; >> + param_data->param_id = PARAM_ID_MEDIA_FORMAT; >> + param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE; >> + p = p + APM_MODULE_PARAM_DATA_SIZE; >> + >> + header = p; >> + header->data_format = DATA_FORMAT_FIXED_POINT; >> + header->fmt_id = MEDIA_FMT_ID_PCM; >> + header->payload_size = payload_size - sizeof(*header); >> + >> + p = p + sizeof(*header); >> + cfg = p; >> + cfg->sample_rate = mcfg->sample_rate; >> + cfg->bit_width = mcfg->bit_width; >> + cfg->alignment = PCM_LSB_ALIGNED; >> + cfg->bits_per_sample = mcfg->bit_width; >> + cfg->q_factor = mcfg->bit_width - 1; >> + cfg->endianness = PCM_LITTLE_ENDIAN; >> + cfg->num_channels = mcfg->num_channels; >> + >> + if (mcfg->num_channels == 1) { >> + cfg->channel_mapping[0] = PCM_CHANNEL_L; >> + } else if (num_channels == 2) { >> + cfg->channel_mapping[0] = PCM_CHANNEL_L; >> + cfg->channel_mapping[1] = PCM_CHANNEL_R; >> + } else { >> + dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels); >> + rc = -EINVAL; >> + goto err; > > ... this is again the case where mcfg->num_channels > 2 so this block is > never executed. I agree this else is dead code, will remove this. --srini > >> + } >> + >> + rc = audioreach_graph_send_cmd_sync(graph, pkt, 0); >> +err: >> + kfree(pkt); >> + >> + return rc; >> +} >> +
Many thanks for reviewing v7 This version addresses all the comments raised as part of v8 review. This patchset adds ASoC driver support to configure signal processing framework ("AudioReach") which is integral part of Qualcomm next generation audio SDK and will be deployed on upcoming Qualcomm chipsets. It makes use of ASoC Topology to load graphs on to the DSP which is then managed by APM (Audio Processing Manager) service to prepare/start/stop. Here is simplified high-level block diagram of AudioReach: ___________________________________________________________ | CPU (Application Processor) | | +---------+ +---------+ +----------+ | | | q6apm | | q6apm | | q6apm | | | | dais | <------> | | <-----> |lpass-dais| | | +---------+ +---------+ +----------+ | | ^ ^ | | | | +---------+ | | +---------+ v +---------->|topology | | | | q6prm | +---------+ | | | | | |<-------->| GPR | +---------+ | | +---------+ +---------+ | | ^ ^ | | | | | | +----------+ | | | | q6prm | | | | |lpass-clks| | | | +----------+ | | |____________________________|______________________________| | | RPMSG (IPC over GLINK) ____________________________|______________________________ | | | | +-----------------------+ | | | | | | v v q6 (Audio DSP) | |+-----+ +----------------------------------+ | || PRM | | APM (Audio Processing Manager) | | |+-----+ | . Graph Management | | | | . Command Handing | | | | . Event Management | | | | ... | | | +----------------------------------+ | | ^ | |____________________________|______________________________| | | LPASS AIF ____________________________|______________________________ | | Audio I/O | | v | | +--------------------------------------------------+ | | | Audio devices | | | | CODEC | HDMI-TX | PCM | SLIMBUS | I2S |MI2S |...| | | | | | | +--------------------------------------------------+ | |___________________________________________________________| AudioReach has constructs of sub-graph, container and modules. Each sub-graph can have N containers and each Container can have N Modules and connections between them can be linear or non-linear. An audio function can be realized with one or many connected sub-graphs. There are also control/event paths between modules that can be wired up while building graph to achieve various control mechanism between modules. These concepts of Sub-Graph, Containers and Modules are represented in ASoC topology. Here is simple I2S graph with a Write Shared Memory and a Volume control module within a single Subgraph (1) with one Container (1) and 5 modules. ____________________________________________________________ | Sub-Graph [1] | | _______________________________________________________ | | | Container [1] | | | | [WR_SH] -> [PCM DEC] -> [PCM CONV] -> [VOL]-> [I2S-EP]| | | |_______________________________________________________| | |____________________________________________________________| For now this graph is split into two subgraphs to achieve dpcm like below: ________________________________________________ _________________ | Sub-Graph [1] | | Sub-Graph [2] | | ____________________________________________ | | _____________ | | | Container [1] | | | |Container [2]| | | | [WR_SH] -> [PCM DEC] -> [PCM CONV] -> [VOL]| | | | [I2S-EP] | | | |____________________________________________| | | |_____________| | |________________________________________________| |_________________| _________________ | Sub-Graph [3] | | _____________ | | |Container [3]| | | | [DMA-EP] | | | |_____________| | |_________________| This patchset adds very minimal support for AudioReach which includes supporting sub-graphs containing CODEC DMA ports and simple PCM Decoder/Encoder and Logger Modules. Additional capabilities will be built over time to expose features offered by AudioReach. This patchset is Tested on SM8250 SoC based Qualcomm Robotics Platform RB5 and SM9250 MTP with WSA881X Smart Speaker Amplifiers, DMICs connected via VA Macro and WCD938x Codec connected via TX and RX Macro and HDMI audio via I2S. First 10 Patches are mostly reorganization existing Old QDSP Audio Framework code and bindings so that we could reuse them on AudioReach. ASoC topology graphs for DragonBoard RB5 and SM8250 MTP are available at https://git.linaro.org/people/srinivas.kandagatla/audioreach-topology.git/ and Qualcomm AudioReach DSP headers are available at: https://source.codeaurora.org/quic/la/platform/vendor/opensource/arspf-headers Note: There is one false positive warning in this patchset: audioreach.c:80:45: warning: array of flexible structures Thanks, srini Changes since v7: - removed returning errors in dsp command callbacks as it has no effect - removed uncessary err prints - removed default topology file as its impossible to have one for all boards. - removed references of Elite in commit log - return correct error codes returned from functions. Srinivas Kandagatla (22): soc: dt-bindings: qcom: apr: convert to yaml soc: dt-bindings: qcom: apr: deprecate qcom,apr-domain property soc: qcom: apr: make code more reuseable soc: dt-bindings: qcom: add gpr bindings soc: qcom: apr: Add GPR support ASoC: dt-bindings: move LPASS dai related bindings out of q6afe ASoC: dt-bindings: move LPASS clocks related bindings out of q6afe ASoC: dt-bindings: rename q6afe.h to q6dsp-lpass-ports.h ASoC: qdsp6: q6afe-dai: move lpass audio ports to common file ASoC: qdsp6: q6afe-clocks: move audio-clocks to common file ASoC: dt-bindings: q6dsp: add q6apm-lpass-dai compatible ASoC: dt-bindings: lpass-clocks: add q6prm clocks compatible ASoC: dt-bindings: add q6apm digital audio stream bindings ASoC: qdsp6: audioreach: add basic pkt alloc support ASoC: qdsp6: audioreach: add q6apm support ASoC: qdsp6: audioreach: add module configuration command helpers ASoC: qdsp6: audioreach: add Kconfig and Makefile ASoC: qdsp6: audioreach: add topology support ASoC: qdsp6: audioreach: add q6apm-dai support ASoC: qdsp6: audioreach: add q6apm lpass dai support ASoC: qdsp6: audioreach: add q6prm support ASoC: qdsp6: audioreach: add support for q6prm-clocks .../devicetree/bindings/soc/qcom/qcom,apr.txt | 134 -- .../bindings/soc/qcom/qcom,apr.yaml | 177 +++ .../devicetree/bindings/sound/qcom,q6afe.txt | 181 --- .../bindings/sound/qcom,q6apm-dai.yaml | 53 + .../sound/qcom,q6dsp-lpass-clocks.yaml | 77 ++ .../sound/qcom,q6dsp-lpass-ports.yaml | 205 +++ drivers/soc/qcom/Kconfig | 2 +- drivers/soc/qcom/apr.c | 287 ++++- include/dt-bindings/soc/qcom,gpr.h | 19 + include/dt-bindings/sound/qcom,q6afe.h | 203 +-- .../sound/qcom,q6dsp-lpass-ports.h | 208 +++ include/linux/soc/qcom/apr.h | 70 +- include/uapi/sound/snd_ar_tokens.h | 208 +++ sound/soc/qcom/Kconfig | 22 + sound/soc/qcom/qdsp6/Makefile | 11 +- sound/soc/qcom/qdsp6/audioreach.c | 1135 +++++++++++++++++ sound/soc/qcom/qdsp6/audioreach.h | 726 +++++++++++ sound/soc/qcom/qdsp6/q6afe-clocks.c | 187 +-- sound/soc/qcom/qdsp6/q6afe-dai.c | 687 +--------- sound/soc/qcom/qdsp6/q6apm-dai.c | 415 ++++++ sound/soc/qcom/qdsp6/q6apm-lpass-dais.c | 260 ++++ sound/soc/qcom/qdsp6/q6apm.c | 823 ++++++++++++ sound/soc/qcom/qdsp6/q6apm.h | 152 +++ sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c | 186 +++ sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h | 30 + sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c | 627 +++++++++ sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h | 22 + sound/soc/qcom/qdsp6/q6prm-clocks.c | 85 ++ sound/soc/qcom/qdsp6/q6prm.c | 202 +++ sound/soc/qcom/qdsp6/q6prm.h | 78 ++ sound/soc/qcom/qdsp6/topology.c | 1113 ++++++++++++++++ 31 files changed, 7170 insertions(+), 1415 deletions(-) delete mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,apr.yaml create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6apm-dai.yaml create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6dsp-lpass-clocks.yaml create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6dsp-lpass-ports.yaml create mode 100644 include/dt-bindings/soc/qcom,gpr.h create mode 100644 include/dt-bindings/sound/qcom,q6dsp-lpass-ports.h create mode 100644 include/uapi/sound/snd_ar_tokens.h create mode 100644 sound/soc/qcom/qdsp6/audioreach.c create mode 100644 sound/soc/qcom/qdsp6/audioreach.h create mode 100644 sound/soc/qcom/qdsp6/q6apm-dai.c create mode 100644 sound/soc/qcom/qdsp6/q6apm-lpass-dais.c create mode 100644 sound/soc/qcom/qdsp6/q6apm.c create mode 100644 sound/soc/qcom/qdsp6/q6apm.h create mode 100644 sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c create mode 100644 sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h create mode 100644 sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c create mode 100644 sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h create mode 100644 sound/soc/qcom/qdsp6/q6prm-clocks.c create mode 100644 sound/soc/qcom/qdsp6/q6prm.c create mode 100644 sound/soc/qcom/qdsp6/q6prm.h create mode 100644 sound/soc/qcom/qdsp6/topology.c