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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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


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

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

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

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