From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
bjorn.andersson@linaro.org, broonie@kernel.org, robh@kernel.org
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
bgoswami@codeaurora.org, tiwai@suse.de, plai@codeaurora.org,
lgirdwood@gmail.com
Subject: Re: [PATCH v7 15/22] ASoC: qdsp6: audioreach: add q6apm support
Date: Wed, 22 Sep 2021 11:44:45 +0100 [thread overview]
Message-ID: <8dd1aff9-b837-a563-7466-62a45bb4ec74@linaro.org> (raw)
In-Reply-To: <fdb5cd4d-4b8b-3dea-872a-f225a5c90d1f@linux.intel.com>
On 21/09/2021 18:05, Pierre-Louis Bossart wrote:
>
>> +static void apm_populate_connection_obj(struct apm_module_conn_obj *obj,
>> + struct audioreach_module *module)
>> +{
>> + obj->src_mod_inst_id = module->src_mod_inst_id;
>> + obj->src_mod_op_port_id = module->src_mod_op_port_id;
>> + obj->dst_mod_inst_id = module->instance_id;
>> + obj->dst_mod_ip_port_id = module->in_port;
>
> alignment seems off?
Its fixed in next version.
>
>
>> +void *audioreach_alloc_graph_pkt(struct q6apm *apm, struct list_head *sg_list, int graph_id)
>> +{
>> + void *p;
>
> move this as last declaration to have a nice reverse christmas tree style?
Done.
>
>> + int payload_size, sg_sz, cont_sz, ml_sz, mp_sz, mc_sz;
>> + struct apm_module_param_data *param_data;
>> + struct audioreach_container *container;
>> + struct apm_graph_open_params params;
>> + struct audioreach_sub_graph *sgs;
>> + struct audioreach_module *module;
>> + int num_modules_per_list;
>> + int num_connections = 0;
>> + int num_containers = 0;
>> + int num_sub_graphs = 0;
>> + int num_modules = 0;
>> + int num_modules_list;
>> + struct gpr_pkt *pkt;
>
> [...]
>
>> +static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm, uint32_t graph_id)
>> +{
>> + struct audioreach_graph_info *info;
>> + struct audioreach_graph *graph;
>> +
>> + mutex_lock(&apm->lock);
>> + graph = idr_find(&apm->graph_idr, graph_id);
>> + mutex_unlock(&apm->lock);
>> +
>> + 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;
>> +
>> + graph->graph = audioreach_alloc_graph_pkt(apm, &info->sg_list, graph_id);
>> + if (IS_ERR(graph->graph)) {
>> + kfree(graph);
>> + return ERR_PTR(-ENOMEM);
>
> why not return graph->graph (store and return the value before freeing
> graph)?
Updated something like this now:
if (IS_ERR(graph->graph)) {
void *err = graph->graph;
kfree(graph);
return ERR_CAST(err);
}
>
>> + }
>> +
>> + mutex_lock(&apm->lock);
>> + if (idr_alloc(&apm->graph_idr, graph, graph_id, graph_id + 1, GFP_KERNEL) < 0) {
>> + dev_err(apm->dev, "Unable to allocate graph id (%d)\n", graph_id);
>> + kfree(graph);
>> + mutex_unlock(&apm->lock);
>> + return ERR_PTR(-ENOMEM);
>> + }
>> + mutex_unlock(&apm->lock);
>> +
>> + kref_init(&graph->refcount);
>> +
>> + q6apm_send_cmd_sync(apm, graph->graph, 0);
>> +
>> + return graph;
>> +}
>
>> +static int graph_callback(struct gpr_resp_pkt *data, void *priv, int op)
>> +{
>> + struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done_v2 *rd_done;
>> + struct data_cmd_rsp_wr_sh_mem_ep_data_buffer_done_v2 *done;
>> + struct apm_cmd_rsp_shared_mem_map_regions *rsp;
>> + struct gpr_ibasic_rsp_result_t *result;
>> + struct q6apm_graph *graph = priv;
>> + struct gpr_hdr *hdr = &data->hdr;
>> + struct device *dev = graph->dev;
>> + uint32_t client_event;
>> + int ret = -EINVAL;
>> + phys_addr_t phys;
>> + 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;
>> + mutex_lock(&graph->lock);
>> + 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 Unexpected addr %08x-%08x\n",
>> + done->buf_addr_lsw, done->buf_addr_msw);
>> + ret = -EINVAL;
>
> since you don't return here you might invoke graph->cb() below, is this
> desired?
>
No, it does not make sense to do the cb() for un-expected/error response.
TBH, rethinking on this..
returning error from these callbacks is totally useless as the apr-bus
will not do anything about this. Only reporting it as dev_err and caller
receiving a command timeout on error is the right thing to do.
This callback will now only return 0 irrespective of error response or
not. CMD will Timeout for the caller in error cases.
--srini
>> + } else {
>> + ret = 0;
>> + graph->result.opcode = hdr->opcode;
>> + graph->result.status = done->status;
>> + }
>> + mutex_unlock(&graph->lock);
>> + 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:
>> + client_event = APM_CLIENT_EVENT_DATA_READ_DONE;
>> + mutex_lock(&graph->lock);
>> + rd_done = data->payload;
>> + phys = graph->tx_data.buf[hdr->token].phys;
>> + if (upper_32_bits(phys) != rd_done->buf_addr_msw ||
>> + lower_32_bits(phys) != rd_done->buf_addr_lsw) {
>> + dev_err(dev, "RD BUFF Unexpected addr %08x-%08x\n",
>> + rd_done->buf_addr_lsw, rd_done->buf_addr_msw);
>> + ret = -EINVAL;
>
> same here, you will call wake_up and conditionally the callback.
>
>> + } else {
>> + ret = 0;
>> + }
>> + mutex_unlock(&graph->lock);
>> + 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;
>
> and here as well.
>
>> + } else {
>> + ret = 0;
>> + }
>> + wake_up(&graph->cmd_wait);
>> + break;
>> + default:
>> + break;
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
>> +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 = 0;
>> +
>> + 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;
>
> same pattern, you will call wake_up even on an error?
>
>> + }
>> + wake_up(&apm->wait);
>> + break;
>> + default:
>> + break;
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
next prev parent reply other threads:[~2021-09-22 10:46 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-21 13:36 [PATCH v7 00/22] ASoC: qcom: Add AudioReach support Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 01/22] soc: dt-bindings: qcom: apr: convert to yaml Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 02/22] soc: dt-bindings: qcom: apr: deprecate qcom, apr-domain property Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 03/22] soc: qcom: apr: make code more reuseable Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 04/22] soc: dt-bindings: qcom: add gpr bindings Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 05/22] soc: qcom: apr: Add GPR support Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 06/22] ASoC: dt-bindings: move LPASS dai related bindings out of q6afe Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 07/22] ASoC: dt-bindings: move LPASS clocks " Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 08/22] ASoC: dt-bindings: rename q6afe.h to q6dsp-lpass-ports.h Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 09/22] ASoC: qdsp6: q6afe-dai: move lpass audio ports to common file Srinivas Kandagatla
2021-09-21 16:51 ` Pierre-Louis Bossart
2021-09-22 10:44 ` Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 10/22] ASoC: qdsp6: q6afe-clocks: move audio-clocks " Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 11/22] ASoC: dt-bindings: q6dsp: add q6apm-lpass-dai compatible Srinivas Kandagatla
2021-09-21 13:36 ` [PATCH v7 12/22] ASoC: dt-bindings: lpass-clocks: add q6prm clocks compatible Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 13/22] ASoC: dt-bindings: add q6apm digital audio stream bindings Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 14/22] ASoC: qdsp6: audioreach: add basic pkt alloc support Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 15/22] ASoC: qdsp6: audioreach: add q6apm support Srinivas Kandagatla
2021-09-21 17:05 ` Pierre-Louis Bossart
2021-09-22 10:44 ` Srinivas Kandagatla [this message]
2021-09-21 13:37 ` [PATCH v7 16/22] ASoC: qdsp6: audioreach: add module configuration command helpers Srinivas Kandagatla
2021-09-21 17:09 ` Pierre-Louis Bossart
2021-09-22 10:44 ` Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 17/22] ASoC: qdsp6: audioreach: add Kconfig and Makefile Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 18/22] ASoC: qdsp6: audioreach: add topology support Srinivas Kandagatla
2021-09-21 18:44 ` Pierre-Louis Bossart
2021-09-22 10:44 ` Srinivas Kandagatla
2021-09-22 15:40 ` Pierre-Louis Bossart
2021-09-21 13:37 ` [PATCH v7 19/22] ASoC: qdsp6: audioreach: add q6apm-dai support Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 20/22] ASoC: qdsp6: audioreach: add q6apm lpass dai support Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 21/22] ASoC: qdsp6: audioreach: add q6prm support Srinivas Kandagatla
2021-09-21 13:37 ` [PATCH v7 22/22] ASoC: qdsp6: audioreach: add support for q6prm-clocks Srinivas Kandagatla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8dd1aff9-b837-a563-7466-62a45bb4ec74@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=alsa-devel@alsa-project.org \
--cc=bgoswami@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=plai@codeaurora.org \
--cc=robh@kernel.org \
--cc=tiwai@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).