Re: [PATCH v7 15/22] ASoC: qdsp6: audioreach: add q6apm support

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;

+}