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

[Srinivas Kandagatla]

Thanks Pierre for review,

On 15/09/2021 17:02, Pierre-Louis Bossart wrote:
> 
>> +static void audioreach_populate_graph(struct apm_graph_open_params *open,
>> +				      struct list_head *sg_list,
>> +				      int num_sub_graphs)
>> +{
>> +	struct apm_mod_conn_list_params *mc_data = open->mod_conn_list_data;
>> +	struct apm_module_list_params *ml_data = open->mod_list_data;
>> +	struct apm_prop_list_params *mp_data = open->mod_prop_data;
>> +	struct apm_container_params *c_data = open->cont_data;
>> +	struct apm_sub_graph_params *sg_data = open->sg_data;
>> +	int ncontainer = 0, nmodule = 0, nconn = 0;
>> +	struct apm_mod_prop_obj *module_prop_obj;
>> +	struct audioreach_container *container;
>> +	struct apm_module_conn_obj *conn_obj;
>> +	struct audioreach_module *module;
>> +	struct audioreach_sub_graph *sg;
>> +	struct apm_container_obj *cobj;
>> +	struct apm_mod_list_obj *mlobj;
>> +	int i = 0;
>> +
>> +	mlobj = &ml_data->mod_list_obj[0];
>> +
>> +	list_for_each_entry(sg, sg_list, node) {
>> +		struct apm_sub_graph_data *sg_cfg = &sg_data->sg_cfg[i++];
>> +
>> +		apm_populate_sub_graph_config(sg_cfg, sg);
>> +
>> +		list_for_each_entry(container, &sg->container_list, node) {
>> +			cobj = &c_data->cont_obj[ncontainer];
>> +
>> +			apm_populate_container_config(cobj, container);
>> +			apm_populate_module_list_obj(mlobj, container, sg->sub_graph_id);
>> +
>> +			list_for_each_container_module(module, container) {
>> +				uint32_t src_mod_inst_id;
>> +
>> +				src_mod_inst_id = module->src_mod_inst_id;
>> +
>> +				module_prop_obj = &mp_data->mod_prop_obj[nmodule];
>> +				apm_populate_module_prop_obj(module_prop_obj, module);
>> +
>> +				if (src_mod_inst_id /*&& dst_mod_inst_id*/) {
> 
> remove left-over code or uncomment required condition?

Yes, this can be removed.

> 
>> +					conn_obj = &mc_data->conn_obj[nconn];
>> +					apm_populate_connection_obj(conn_obj, module);
>> +					nconn++;
>> +				}
>> +
>> +				nmodule++;
>> +			}
>> +			mlobj = (void *) mlobj + APM_MOD_LIST_OBJ_PSIZE(container->num_modules);
>> +
>> +			ncontainer++;
>> +		}
>> +	}
>> +}
> 
>> +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);
>> +	}
>> +
>> +	mutex_lock(&apm->lock);
>> +	idr_alloc(&apm->graph_idr, graph, graph_id, graph_id + 1, GFP_KERNEL);
> 
> test for idr_alloc() success? You have error checks for idr_find() but
> not for the _alloc() case.

Yes, error handing is missing in this case, fixed in next version.

> 
>> +	mutex_unlock(&apm->lock);
>> +
>> +	kref_init(&graph->refcount);
>> +
>> +	q6apm_send_cmd_sync(apm, graph->graph, 0);
>> +
>> +	return graph;
>> +}
>> +

>> +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;
> 
> this would keep ret = -EINVAL and return it, that seems suspicious since
> this doesn't look as an error case?
> 
>> +	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;
> 
> That's also weird, maybe initialize to zero and return an error code in
> error cases, or fix the case APM_CMD_RSP_GET_SPF_STATE above.

this is now fixed by initializing the ret to 0 and setting it in error path.

--srini

> 
>> +			}
>> +			wake_up(&apm->wait);
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	default:
>> +		break;
> 
> so all the defaults are errors?
> 
>> +	}
>> +
>> +	return ret;
>> +}