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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



thanks Pierre for taking time to review the patches.

On 03/09/2021 15:54, Pierre-Louis Bossart wrote:

+static void apm_populate_container_config(
+			struct apm_container_obj *cfg,
+			struct audioreach_container *cont)

indentation looks rather weird, use 100 chars?

Fixed simillar indendataion across all the patches.

+{
+

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

Yes, I have add a comment now.


+			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?
Its fixed now.


  /* 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...
True, this is just an intermediate data-structure, I have have now changed this to u32.

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

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

DSP finished command successfully at this point. DSP returns error info in result.status. I have added a comment here for clarity and removed this dev_err which I have used in past for debug.


+	}
+
+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! */

THis is very old comment forgot to clean it up, its removed 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?

We are inside spinlock.


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

I think I tried to do this but some how I missed it in few places, will fix all such instances.

+
+

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


Yes, it needs
default:
	break;
at two places, ret is already initialized to -EINVAL.

??

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

This looks like lef-over from debug. Will remove this.


+	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:
Yes, its redundant, we can remove this.



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?

I have fixed this now.


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

its done in next version.


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

These are now prefixed.

--srini

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

+



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux