Re: [PATCH v5 15/21] ASoC: qdsp6: audioreach: add q6apm support

+static void apm_populate_container_config(

• 	struct apm_container_obj *cfg,
  

• 	struct audioreach_container *cont)
  

indentation looks rather weird, use 100 chars?

+{

• /* 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

• 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?

• 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;

+}

+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?

• 	return module;
  

• }
• return NULL;

+} +EXPORT_SYMBOL_GPL(audioreach_get_container_first_module);

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?

/* 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...

• 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];

+};

+/* 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];

[]

+} __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?

• }

+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.

• 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?

• 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?

• 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;

+}

+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;

??

• }
• 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() ?

• 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

• return 0;

+} +EXPORT_SYMBOL_GPL(q6apm_graph_close);

+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?

• 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?

• }
  

• break;
  

default case?

• }
• 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?

• .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?

+#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?