Thanks Pierre for review,
On 14/07/2021 17:40, Pierre-Louis Bossart wrote:
/* SubGraph Config */
@@ -32,7 +33,7 @@ struct apm_sub_graph_params {
/* container config */
struct apm_container_obj {
struct apm_container_cfg container_cfg;
- /* Capablity ID list */
+ /* Capability ID list */
squash in wrong patch, this should have been included in the previous patch.
My bad.. will fix such instances in next spin.
struct apm_prop_data cap_data;
uint32_t num_capablity_id;
...
+static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm,
+ uint32_t graph_id)
+{
+ struct audioreach_graph *graph = NULL;
useless init
+ 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! */
+ graph->graph = audioreach_alloc_graph_pkt(apm, &info->sg_list, graph_id);
+ if (IS_ERR(graph->graph))
+ return ERR_PTR(-ENOMEM);
+
+ spin_lock(&apm->lock);
+ idr_alloc(&apm->graph_idr, graph, graph_id,
+ graph_id + 1, GFP_ATOMIC);
ATOMIC?
we are under spinlock here, so we need this.
+ 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;
+
+ payload_size = APM_GRAPH_MGMT_PSIZE(num_sub_graphs);
+
+ p = audioreach_alloc_apm_cmd_pkt(payload_size, opcode, 0);
+ if (IS_ERR(p))
+ return -ENOMEM;
+
+ pkt = p;
+ p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
+
+ mgmt_cmd = p;
+ mgmt_cmd->num_sub_graphs = num_sub_graphs;
+
+ param_data = &mgmt_cmd->param_data;
+ param_data->module_instance_id = APM_MODULE_INSTANCE_ID;
+ param_data->param_id = APM_PARAM_ID_SUB_GRAPH_LIST;
+ param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE;
+
+ list_for_each_entry(sg, &info->sg_list, node) {
+ mgmt_cmd->sub_graph_id_list[i++] = sg->sub_graph_id;
+ }
+
+ rc = q6apm_send_cmd_sync(apm, pkt, 0);
+
+ kfree(pkt);
+
+ return rc;
+}
+
+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);
earlier in the _get routine, you had a kref_get(&graph->refcount)
This is a release callback for kref. Probably I should rename this to
q6apm_release_audioreach_graph().
is it intentional that there's kref_put?
kref_put is called in q6apm_graph_close()
q6apm_graph_open() calls q6apm_get_audioreach_graph() which will do
kref_get.
q6apm_graph_close() will call kref_put().
when refcount is zero q6apm_put_audioreach_graph() callback will be invoked.
--srini
adding a comment on the refcounts might be useful...
