Re: [PATCH v5 15/21] ASoC: qdsp6: audioreach: add q6apm support
flat view
From: Pierre-Louis Bossart <hidden>
Date: 2021-09-03 16:38:17
Also in:
alsa-devel
quoted hunk ↗ jump to hunk
+static void apm_populate_container_config( + struct apm_container_obj *cfg, + struct audioreach_container *cont)
indentation looks rather weird, use 100 chars?
quoted hunk ↗ jump to hunk
+{ + + /* Container Config */ + cfg->container_cfg.container_id = cont->container_id; + cfg->container_cfg.num_prop = 4; + + /* Capability list */ + cfg->cap_data.prop_id = APM_CONTAINER_PROP_ID_CAPABILITY_LIST; + cfg->cap_data.prop_size = APM_CONTAINER_PROP_ID_CAPABILITY_SIZE; + cfg->num_capablity_id = 1;
num_capability_id
quoted hunk ↗ jump to hunk
+ cfg->capability_id = cont->capability_id; + + /* Graph Position */ + cfg->pos_data.prop_id = APM_CONTAINER_PROP_ID_GRAPH_POS; + cfg->pos_data.prop_size = sizeof(struct apm_cont_prop_id_graph_pos); + cfg->pos.graph_pos = cont->graph_pos; + + /* Stack size */ + cfg->stack_data.prop_id = APM_CONTAINER_PROP_ID_STACK_SIZE; + cfg->stack_data.prop_size = sizeof(struct + apm_cont_prop_id_stack_size);
oneline?
quoted hunk ↗ jump to hunk
+ cfg->stack.stack_size = cont->stack_size; + + /* Proc domain */ + cfg->domain_data.prop_id = APM_CONTAINER_PROP_ID_PROC_DOMAIN; + cfg->domain_data.prop_size = sizeof(struct + apm_cont_prop_id_domain);
oneline?
+ cfg->domain.proc_domain = cont->proc_domain; +}
quoted hunk ↗ jump to hunk
+struct audioreach_module *audioreach_get_container_last_module( + struct audioreach_container *container) +{ + struct audioreach_module *module; + + list_for_each_entry(module, &container->modules_list, node) { + if (module->dst_mod_inst_id == 0) + return module; + } + + return NULL; +} +EXPORT_SYMBOL_GPL(audioreach_get_container_last_module); + +static bool is_module_in_container(struct audioreach_container *container, int + module_iid) +{ + struct audioreach_module *module; + + list_for_each_entry(module, &container->modules_list, node) { + if (module->instance_id == module_iid) + return true; + } + + return false; +} + +struct audioreach_module *audioreach_get_container_first_module( + struct audioreach_container *container) +{ + struct audioreach_module *module; + + list_for_each_entry(module, &container->modules_list, node) { + if (module->src_mod_inst_id == 0 || + !is_module_in_container(container, module->src_mod_inst_id))
You may want to add a to comment to explain why you walk through the list with two nested loops?
quoted hunk ↗ jump to hunk
+ return module; + } + return NULL; +} +EXPORT_SYMBOL_GPL(audioreach_get_container_first_module);
quoted hunk ↗ jump to hunk
diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h index 8ef015aca3b2..d25ea8c48b41 100644 --- a/sound/soc/qcom/qdsp6/audioreach.h +++ b/sound/soc/qcom/qdsp6/audioreach.h@@ -5,6 +5,9 @@ #include <linux/types.h> #include <linux/soc/qcom/apr.h> #include <sound/soc.h> +struct q6apm; +struct q6apm_graph; +
spurious line? checkpatch should complain on this?
quoted hunk ↗ jump to hunk
/* Module IDs */ #define MODULE_ID_WR_SHARED_MEM_EP 0x07001000@@ -622,6 +625,20 @@ struct audioreach_module { struct snd_soc_dapm_widget *widget; }; +struct audioreach_module_config { + int direction; + u16 sample_rate;
so any rate above 64kHz cannot be represented? yay high-res audio...
quoted hunk ↗ jump to hunk
+ u16 bit_width; + u16 bits_per_sample; + + u16 data_format; + u16 num_channels; + u16 active_channels_mask; + u32 sd_line_mask; + int fmt; + u8 channel_map[PCM_MAX_NUM_CHANNEL]; +};
quoted hunk ↗ jump to hunk
+/* 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[0];
[]
quoted hunk ↗ jump to hunk
+} __packed; + +#define APM_GRAPH_MGMT_PSIZE(n) ALIGN(sizeof(struct apm_graph_mgmt_cmd) + \ + n * sizeof(uint32_t), 8) + +int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt, + uint32_t rsp_opcode) +{ + gpr_device_t *gdev = apm->gdev; + struct gpr_hdr *hdr = &pkt->hdr; + int rc; + + mutex_lock(&apm->cmd_lock); + apm->result.opcode = 0; + apm->result.status = 0; + + rc = gpr_send_pkt(apm->gdev, pkt); + if (rc < 0) + goto err; + + if (rsp_opcode) + rc = wait_event_timeout(apm->wait, + (apm->result.opcode == hdr->opcode) || + (apm->result.opcode == rsp_opcode), + 5 * HZ); + else + rc = wait_event_timeout(apm->wait, + (apm->result.opcode == hdr->opcode), + 5 * HZ); + + if (!rc) { + dev_err(&gdev->dev, "CMD timeout for [%x] opcode\n", + hdr->opcode); + rc = -ETIMEDOUT; + } else if (apm->result.status > 0) { + dev_err(&gdev->dev, "DSP returned error[%x] %x\n", hdr->opcode, + apm->result.status); + rc = -EINVAL; + } else { + dev_err(&gdev->dev, "DSP returned [%x]\n", + apm->result.status); + rc = 0;
maybe add a comment on why you squelch the error?
quoted hunk ↗ jump to hunk
+ } + +err: + mutex_unlock(&apm->cmd_lock); + return rc; +} + +static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm, + uint32_t graph_id) +{ + struct audioreach_graph *graph; + struct audioreach_graph_info *info; + unsigned long flags; + + spin_lock_irqsave(&apm->lock, flags); + graph = idr_find(&apm->graph_idr, graph_id); + spin_unlock_irqrestore(&apm->lock, flags); + + if (graph) { + kref_get(&graph->refcount); + return graph; + } + + info = idr_find(&apm->graph_info_idr, graph_id); + + if (!info) + return ERR_PTR(-ENODEV); + + graph = kzalloc(sizeof(*graph), GFP_KERNEL); + if (!graph) + return ERR_PTR(-ENOMEM); + + graph->apm = apm; + graph->info = info; + graph->id = graph_id; + + /* Assuming Linear Graphs only for now! */
Linear graphs? does this mean you don't have support for mixers/mux/demux? The cover letter says this " 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. " the comment seems contradict the cover letter, so which is it? Explaining the concept of 'nonlinear' would be good, I don't think I've ever seen this term used for a graph. The graph of a function can be linear or non-linear, but that's a different story.
quoted hunk ↗ jump to hunk
+ graph->graph = audioreach_alloc_graph_pkt(apm, &info->sg_list, graph_id); + if (IS_ERR(graph->graph)) { + kfree(graph); + return ERR_PTR(-ENOMEM); + } + + spin_lock(&apm->lock); + idr_alloc(&apm->graph_idr, graph, graph_id, + graph_id + 1, GFP_ATOMIC);
does this need to be ATOMIC?
quoted hunk ↗ jump to hunk
+ spin_unlock(&apm->lock); + + kref_init(&graph->refcount); + + q6apm_send_cmd_sync(apm, graph->graph, 0); + + return graph; +} + +static int audioreach_graph_mgmt_cmd(struct audioreach_graph *graph, + uint32_t opcode) +{ + struct gpr_pkt *pkt; + void *p; + int i = 0, rc, payload_size; + struct q6apm *apm = graph->apm; + struct audioreach_graph_info *info = graph->info; + int num_sub_graphs = info->num_sub_graphs; + struct apm_graph_mgmt_cmd *mgmt_cmd; + struct apm_module_param_data *param_data; + struct audioreach_sub_graph *sg;
reverse x-mas tree style?
quoted hunk ↗ jump to hunk
+ + payload_size = APM_GRAPH_MGMT_PSIZE(num_sub_graphs); + + p = audioreach_alloc_apm_cmd_pkt(payload_size, opcode, 0); + if (IS_ERR(p)) + return -ENOMEM; + + pkt = p; + p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE; + + mgmt_cmd = p; + mgmt_cmd->num_sub_graphs = num_sub_graphs; + + param_data = &mgmt_cmd->param_data; + param_data->module_instance_id = APM_MODULE_INSTANCE_ID; + param_data->param_id = APM_PARAM_ID_SUB_GRAPH_LIST; + param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE; + + list_for_each_entry(sg, &info->sg_list, node) { + mgmt_cmd->sub_graph_id_list[i++] = sg->sub_graph_id; + } + + rc = q6apm_send_cmd_sync(apm, pkt, 0); + + kfree(pkt); + + return rc; +} +
quoted hunk ↗ jump to hunk
+static int graph_callback(struct gpr_resp_pkt *data, void *priv, int op) +{ + struct q6apm_graph *graph = priv; + struct device *dev = graph->dev; + struct gpr_hdr *hdr = &data->hdr; + struct gpr_ibasic_rsp_result_t *result; + int ret = -EINVAL; + uint32_t client_event = 0; + struct data_cmd_rsp_wr_sh_mem_ep_data_buffer_done_v2 *done; + struct apm_cmd_rsp_shared_mem_map_regions *rsp; + phys_addr_t phys; + unsigned long flags; + int token; + + result = data->payload; + + switch (hdr->opcode) { + case DATA_CMD_RSP_WR_SH_MEM_EP_DATA_BUFFER_DONE_V2: + client_event = APM_CLIENT_EVENT_DATA_WRITE_DONE; + spin_lock_irqsave(&graph->lock, flags); + token = hdr->token & APM_WRITE_TOKEN_MASK; + + done = data->payload; + phys = graph->rx_data.buf[token].phys; + + if (lower_32_bits(phys) != done->buf_addr_lsw || + upper_32_bits(phys) != done->buf_addr_msw) { + dev_err(dev, "WR BUFF Expected Token %d addr %pa\n", token, &phys); + ret = -EINVAL; + } else { + ret = 0; + graph->result.opcode = hdr->opcode; + graph->result.status = done->status; + } + spin_unlock_irqrestore(&graph->lock, flags); + if (graph->cb) + graph->cb(client_event, hdr->token, data->payload, + graph->priv); + + break; + case APM_CMD_RSP_SHARED_MEM_MAP_REGIONS: + graph->result.opcode = hdr->opcode; + graph->result.status = 0; + rsp = data->payload; + + if (hdr->token == SNDRV_PCM_STREAM_PLAYBACK) + graph->rx_data.mem_map_handle = rsp->mem_map_handle; + else + graph->tx_data.mem_map_handle = rsp->mem_map_handle; + + wake_up(&graph->cmd_wait); + ret = 0; + break; + case DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER_V2: + done = data->payload; + spin_lock_irqsave(&graph->lock, flags); + phys = graph->tx_data.buf[hdr->token].phys; + if (upper_32_bits(phys) != done->buf_addr_msw || + lower_32_bits(phys) != done->buf_addr_lsw) { + dev_err(dev, "RD BUFF Expected addr %pa %08x-%08x\n", + &phys, + done->buf_addr_lsw, + done->buf_addr_msw); + ret = -EINVAL; + } else { + ret = 0; + } + spin_unlock_irqrestore(&graph->lock, flags); + client_event = APM_CLIENT_EVENT_DATA_READ_DONE; + wake_up(&graph->cmd_wait); + + if (graph->cb) + graph->cb(client_event, hdr->token, data->payload, + graph->priv); + break; + case DATA_CMD_WR_SH_MEM_EP_EOS_RENDERED: + break; + case GPR_BASIC_RSP_RESULT: + switch (result->opcode) { + case APM_CMD_SHARED_MEM_UNMAP_REGIONS: + graph->result.opcode = result->opcode; + graph->result.status = 0; + if (hdr->token == SNDRV_PCM_STREAM_PLAYBACK) + graph->rx_data.mem_map_handle = 0; + else + graph->tx_data.mem_map_handle = 0; + + wake_up(&graph->cmd_wait); + ret = 0; + break; + case APM_CMD_SHARED_MEM_MAP_REGIONS: + case DATA_CMD_WR_SH_MEM_EP_MEDIA_FORMAT: + case APM_CMD_SET_CFG: + graph->result.opcode = result->opcode; + graph->result.status = result->status; + if (result->status) { + dev_err(dev, + "Error (%d) Processing 0x%08x cmd\n", + result->status, result->opcode); + ret = -EINVAL; + } else { + ret = 0; + } + wake_up(&graph->cmd_wait); + if (graph->cb) + graph->cb(client_event, hdr->token, data->payload, + graph->priv); + + } + break;
default: ret = -EINVAL; break; ??
quoted hunk ↗ jump to hunk
+ } + + return ret; +} + +struct q6apm_graph *q6apm_graph_open(struct device *dev, q6apm_cb cb, + void *priv, int graph_id) +{ + struct q6apm *apm = dev_get_drvdata(dev->parent); + struct q6apm_graph *graph; + struct audioreach_graph *ar_graph; + int ret; + + dev_err(dev, "%s :graph id %d\n", __func__, graph_id);
dev_dbg() ?
quoted hunk ↗ jump to hunk
+ ar_graph = q6apm_get_audioreach_graph(apm, graph_id); + if (IS_ERR(ar_graph)) { + dev_err(dev, "No graph found with id %d\n", graph_id); + return ERR_CAST(ar_graph); + } + + graph = kzalloc(sizeof(*graph), GFP_KERNEL); + if (!graph) { + ret = -ENOMEM; + goto err; + } + + graph->apm = apm; + graph->priv = priv; + graph->cb = cb; + graph->info = ar_graph->info; + graph->ar_graph = ar_graph; + graph->id = ar_graph->id; + graph->dev = dev; + + spin_lock_init(&graph->lock); + init_waitqueue_head(&graph->cmd_wait); + mutex_init(&graph->cmd_lock); + + graph->port = gpr_alloc_port(apm->gdev, dev, graph_callback, graph); + if (!graph->port) { + kfree(graph); + ret = -ENOMEM; + goto err; + } + + dev_dbg(dev, "%s: GRAPH-DEBUG Opening graph id %d with port id 0x%08x\n", __func__, + graph_id, graph->port->id); + + return graph; +err: + kref_put(&ar_graph->refcount, q6apm_put_audioreach_graph); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(q6apm_graph_open); + +int q6apm_graph_close(struct q6apm_graph *graph) +{ + struct audioreach_graph *ar_graph = graph->ar_graph; + + gpr_free_port(graph->port); + graph->port = NULL; + kref_put(&ar_graph->refcount, q6apm_put_audioreach_graph); + kfree(graph);
is this kfree needed, I see it done in the routine below:
static void q6apm_put_audioreach_graph(struct kref *ref)
+{
+ struct audioreach_graph *graph;
+ struct q6apm *apm;
+ unsigned long flags;
+
+ graph = container_of(ref, struct audioreach_graph, refcount);
+ apm = graph->apm;
+
+ audioreach_graph_mgmt_cmd(graph, APM_CMD_GRAPH_CLOSE);
+
+ spin_lock_irqsave(&apm->lock, flags);
+ graph = idr_remove(&apm->graph_idr, graph->id);
+ spin_unlock_irqrestore(&apm->lock, flags);
+
+ kfree(graph->graph);
+ kfree(graph); <<< HERE
quoted hunk ↗ jump to hunk
+ + return 0; +} +EXPORT_SYMBOL_GPL(q6apm_graph_close); +
quoted hunk ↗ jump to hunk
+static int apm_probe(gpr_device_t *gdev) +{ + struct device *dev = &gdev->dev; + struct q6apm *apm; + + apm = devm_kzalloc(dev, sizeof(*apm), GFP_KERNEL); + if (!apm) + return -ENOMEM; + + dev_set_drvdata(dev, apm); + + mutex_init(&apm->cmd_lock); + apm->dev = dev; + apm->gdev = gdev; + init_waitqueue_head(&apm->wait); + + idr_init(&apm->graph_idr); + idr_init(&apm->graph_info_idr); + idr_init(&apm->sub_graphs_idr); + idr_init(&apm->containers_idr); + + idr_init(&apm->modules_idr); + spin_lock_init(&apm->lock); + + q6apm_get_apm_state(apm); + + devm_snd_soc_register_component(dev, &q6apm_audio_component, NULL, 0);
shouldn't this be checked for errors?
quoted hunk ↗ jump to hunk
+ + return of_platform_populate(dev->of_node, NULL, NULL, dev); +} + +static int apm_exit(gpr_device_t *gdev) +{ + return 0; +} + +struct audioreach_module *q6apm_find_module_by_mid(struct q6apm_graph *graph, + uint32_t mid) +{ + struct audioreach_graph_info *info = graph->info; + struct q6apm *apm = graph->apm; + + return __q6apm_find_module_by_mid(apm, info, mid); + +} + +static int apm_callback(struct gpr_resp_pkt *data, void *priv, int op) +{ + gpr_device_t *gdev = priv; + struct q6apm *apm = dev_get_drvdata(&gdev->dev); + struct device *dev = &gdev->dev; + struct gpr_ibasic_rsp_result_t *result; + struct gpr_hdr *hdr = &data->hdr; + int ret = -EINVAL; + + result = data->payload; + + switch (hdr->opcode) { + case APM_CMD_RSP_GET_SPF_STATE: + apm->result.opcode = hdr->opcode; + apm->result.status = 0; + /* First word of result it state */ + apm->state = result->opcode; + wake_up(&apm->wait); + break; + case GPR_BASIC_RSP_RESULT: + switch (result->opcode) { + case APM_CMD_GRAPH_START: + case APM_CMD_GRAPH_OPEN: + case APM_CMD_GRAPH_PREPARE: + case APM_CMD_GRAPH_CLOSE: + case APM_CMD_GRAPH_FLUSH: + case APM_CMD_GRAPH_STOP: + case APM_CMD_SET_CFG: + apm->result.opcode = result->opcode; + apm->result.status = result->status; + if (result->status) { + dev_err(dev, + "Error (%d) Processing 0x%08x cmd\n", + result->status, result->opcode); + ret = -EINVAL; + } else { + ret = 0; + } + wake_up(&apm->wait);
default case?
quoted hunk ↗ jump to hunk
+ + } + break;
default case?
quoted hunk ↗ jump to hunk
+ } + + return ret; +} + +static const struct of_device_id apm_device_id[] = { + { .compatible = "qcom,q6apm" }, + {}, +}; +MODULE_DEVICE_TABLE(of, apm_device_id); + +static gpr_driver_t apm_driver = { + .probe = apm_probe, + .remove = apm_exit,
this does nothing at the moment?
quoted hunk ↗ jump to hunk
+ .gpr_callback = apm_callback, + .driver = { + .name = "qcom-apm", + .of_match_table = of_match_ptr(apm_device_id), + }, +}; + +module_gpr_driver(apm_driver); +MODULE_DESCRIPTION("Audio Process Manager"); +MODULE_LICENSE("GPL v2");diff --git a/sound/soc/qcom/qdsp6/q6apm.h b/sound/soc/qcom/qdsp6/q6apm.h new file mode 100644 index 000000000000..8956a060d7cc --- /dev/null +++ b/sound/soc/qcom/qdsp6/q6apm.h@@ -0,0 +1,154 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __Q6APM_H__ +#define __Q6APM_H__ +#include <linux/types.h> +#include <linux/slab.h> +#include <linux/wait.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/spinlock.h> +#include <linux/sched.h> +#include <linux/of.h> +#include <linux/delay.h> +#include <sound/soc.h> +#include <linux/of_platform.h> +#include <linux/jiffies.h> +#include <linux/soc/qcom/apr.h> +#include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h> +#include "audioreach.h" + +#define APM_PORT_MAX 127 +#define APM_PORT_MAX_AUDIO_CHAN_CNT 8 +#define PCM_CHANNEL_NULL 0 +#define PCM_CHANNEL_FL 1 /* Front left channel. */ +#define PCM_CHANNEL_FR 2 /* Front right channel. */ +#define PCM_CHANNEL_FC 3 /* Front center channel. */ +#define PCM_CHANNEL_LS 4 /* Left surround channel. */ +#define PCM_CHANNEL_RS 5 /* Right surround channel. */ +#define PCM_CHANNEL_LFE 6 /* Low frequency effect channel. */ +#define PCM_CHANNEL_CS 7 /* Center surround channel; Rear center ch */ +#define PCM_CHANNEL_LB 8 /* Left back channel; Rear left channel. */ +#define PCM_CHANNEL_RB 9 /* Right back channel; Rear right channel. */ +#define PCM_CHANNELS 10 /* Top surround channel. */ + +#define NO_TIMESTAMP 0xFF00 +#define FORMAT_LINEAR_PCM 0x0000
APM_NO_TIMESTAMP? use prefixes...
+/* APM client callback events */ +#define CMD_EOS 0x0003
APM_CMD_EOS?
quoted hunk ↗ jump to hunk
+#define APM_CLIENT_EVENT_CMD_EOS_DONE 0x1003 +#define CMD_CLOSE 0x0004 +#define APM_CLIENT_EVENT_CMD_CLOSE_DONE 0x1004 +#define APM_CLIENT_EVENT_CMD_RUN_DONE 0x1008 +#define APM_CLIENT_EVENT_DATA_WRITE_DONE 0x1009 +#define APM_CLIENT_EVENT_DATA_READ_DONE 0x100a +#define APM_WRITE_TOKEN_MASK GENMASK(15, 0) +#define APM_WRITE_TOKEN_LEN_MASK GENMASK(31, 16) +#define APM_WRITE_TOKEN_LEN_SHIFT 16 + +#define MAX_SESSIONS 8
APM_MAX_SESSIONS?
+