[PATCH v5 15/21] ASoC: qdsp6: audioreach: add q6apm support
Srinivas Kandagatla
srinivas.kandagatla at linaro.org
Mon Sep 6 18:28:57 CEST 2021
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?
>
>> +
More information about the Alsa-devel
mailing list