Thanks for your review comments.
On 02/01/18 01:50, Bjorn Andersson wrote:
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@xxxxxxxxxx wrote:
From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
This patch adds support to q6 ADM (Audio Device Manager) module in
q6dsp. ADM performs routing between audio streams and AFE ports.
It does Rate matching for streams going to devices driven by
lower case "Rate"
different clocks, it handles volume ramping, Mixing with channel
and "Mixing"
and bit-width. ADM creates and destroys dynamic COPP services
for device-related audio processing as needed.
What's a "copp"?
Common post processing.
This patch adds basic support to ADM.
Wouldn't s/to/for/ be better?
Yes!
[..]
+struct copp {
+ int afe_port;
+ int copp_idx;
+ int id;
+ int cnt;
Please rename this "refcnt" to match other kernel code.
yep.
+ int topology;
+ int mode;
[...]
+};
[...]
+static int adm_callback(struct apr_device *adev, struct apr_client_data *data)
+{
+ uint32_t *payload;
+ int port_idx, copp_idx;
+ struct copp *copp;
+ struct q6adm *adm = dev_get_drvdata(&adev->dev);
+
+ payload = data->payload;
+
+ if (data->payload_size) {
Bail if you don't have a payload and save yourself one indentation
level.
yep!
+ copp_idx = (data->token) & 0XFF;
+ port_idx = ((data->token) >> 16) & 0xFF;
+ if (port_idx < 0 || port_idx >= AFE_MAX_PORTS) {
+ dev_err(&adev->dev, "Invalid port idx %d token %d\n",
+ port_idx, data->token);
+ return 0;
+ }
+ if (copp_idx < 0 || copp_idx >= MAX_COPPS_PER_PORT) {
+ dev_err(&adev->dev, "Invalid copp idx %d token %d\n",
+ copp_idx, data->token);
+ return 0;
+ }
+
+ if (data->opcode == APR_BASIC_RSP_RESULT) {
This is a case in the following switch statement.
+ if (payload[1] != 0) {
+ dev_err(&adev->dev, "cmd = 0x%x returned error = 0x%x\n",
+ payload[0], payload[1]);
This would again benefit from a small struct...
+ }
+ switch (payload[0]) {
+ case ADM_CMD_DEVICE_OPEN_V5:
+ case ADM_CMD_DEVICE_CLOSE_V5:
+ copp = adm_find_copp(adm, port_idx, copp_idx);
+ if (IS_ERR_OR_NULL(copp))
+ return 0;
+
+ copp->stat = payload[1];
+ wake_up(&copp->wait);
+ break;
+ case ADM_CMD_MATRIX_MAP_ROUTINGS_V5:
+ adm->matrix_map_stat = payload[1];
+ wake_up(&adm->matrix_map_wait);
+ break;
+
+ default:
+ dev_err(&adev->dev, "Unknown Cmd: 0x%x\n",
+ payload[0]);
+ break;
+ }
+ return 0;
+ }
+
+ switch (data->opcode) {
+ case ADM_CMDRSP_DEVICE_OPEN_V5:{
Perhaps it would be cleaner to break these out in separate functions?
will do!
+ struct adm_cmd_rsp_device_open_v5 {
+ u32 status;
+ u16 copp_id;
+ u16 reserved;
+ } __packed * open = data->payload;
+
+ open = data->payload;
+ copp = adm_find_copp(adm, port_idx, copp_idx);
+ if (IS_ERR_OR_NULL(copp))
+ return 0;
+
+ if (open->copp_id == INVALID_COPP_ID) {
+ dev_err(&adev->dev, "Invalid coppid rxed %d\n",
+ open->copp_id);
+ copp->stat = ADSP_EBADPARAM;
+ wake_up(&copp->wait);
+ break;
+ }
+ copp->stat = payload[0];
+ copp->id = open->copp_id;
+ pr_debug("%s: coppid rxed=%d\n", __func__,
You have a dev, use dev_dbg()
I think this was a leftover from previous versions, I will fix such
occurrences.
+ open->copp_id);
+ wake_up(&copp->wait);
+
+ }
+ break;
+ default:
+ dev_err(&adev->dev, "Unknown cmd:0x%x\n",
+ data->opcode);
+ break;
+ }
+ }
+ return 0;
+}
+
+static struct copp *adm_alloc_copp(struct q6adm *adm, int port_idx)
+{
+ struct copp *c;
+ int idx;
+
+ idx = find_first_zero_bit(&adm->copp_bitmap[port_idx],
+ MAX_COPPS_PER_PORT);
+
+ if (idx > MAX_COPPS_PER_PORT)
+ return ERR_PTR(-EBUSY);
+
+ set_bit(idx, &adm->copp_bitmap[port_idx]);
+
+ c = devm_kzalloc(adm->dev, sizeof(*c), GFP_KERNEL);
+ if (!c)
Set the bit after doing the allocation and you don't need to clear it
here.
Makes sense.
+ return ERR_PTR(-ENOMEM);
+ c->copp_idx = idx;
+ c->afe_port = port_idx;
+ c->adm = adm;
+
+ init_waitqueue_head(&c->wait);
+
+ spin_lock(&adm->copps_list_lock);
+ list_add_tail(&c->node, &adm->copps_list);
+ spin_unlock(&adm->copps_list_lock);
+
+ return c;
+}
+
+static void adm_free_copp(struct q6adm *adm, struct copp *c, int port_idx)
+{
+ clear_bit(c->copp_idx, &adm->copp_bitmap[port_idx]);
+ spin_lock(&adm->copps_list_lock);
+ list_del(&c->node);
+ spin_unlock(&adm->copps_list_lock);
This function clear the copp_bitmap, so after recycling this once
c->copp_idx will be "dangling".
This seems to put the copp in a state where it is invalid and as the
copp is "reset" i don't think adm_find_matching_copp() will actually
find this again, ever...
So shouldn't you free the copp here too - rather than relying on devm
doing that at some later point in time?
Yes I agree.
+}
+/**
+ * q6adm_open() - open adm to get hold of free copp
"open adm and grab a free copp"?
yep.
+ *
+ * @dev: Pointer to adm child device.
+ * @port_id: port id
+ * @path: playback or capture path.
+ * @rate: rate at which copp is required.
+ * @channel_mode: channel mode
+ * @topology: adm topology id
+ * @perf_mode: performace mode.
+ * @bit_width: audio sample bit width
+ * @app_type: Application type.
+ * @acdb_id: ACDB id
+ *
+ * Return: Will be an negative on error or a valid copp index on success.
+ */
+int q6adm_open(struct device *dev, int port_id, int path, int rate,
+ int channel_mode, int topology, int perf_mode,
+ uint16_t bit_width, int app_type, int acdb_id)
+{
+ struct adm_cmd_device_open_v5 {
+ struct apr_hdr hdr;
+ u16 flags;
+ u16 mode_of_operation;
+ u16 endpoint_id_1;
+ u16 endpoint_id_2;
+ u32 topology_id;
+ u16 dev_num_channel;
+ u16 bit_width;
+ u32 sample_rate;
+ u8 dev_channel_mapping[8];
+ } __packed open;
+ int ret = 0;
+ int port_idx, flags;
+ int tmp_port = q6afe_get_port_id(port_id);
+ struct copp *copp;
+ struct q6adm *adm = dev_get_drvdata(dev->parent);
+
+ port_idx = port_id;
I'm not seeing the reason for having two different variables with the
same value.
not sure why it ended up like this, will fix it.
+ if (port_idx < 0) {
+ dev_err(dev, "Invalid port_id 0x%x\n", port_id);
+ return -EINVAL;
+ }
+
+ flags = ADM_LEGACY_DEVICE_SESSION;
Just put ADM_LEGACY_DEVICE_SESSION in the assignment below.
yep!
+ copp = adm_find_matching_copp(adm, port_idx, topology, perf_mode,
+ rate, bit_width, app_type);
+
+ if (!copp) {
+ copp = adm_alloc_copp(adm, port_idx);
+ if (IS_ERR_OR_NULL(copp))
+ return PTR_ERR(copp);
+
+ copp->cnt = 0;
+ copp->topology = topology;
+ copp->mode = perf_mode;
+ copp->rate = rate;
+ copp->channels = channel_mode;
+ copp->bit_width = bit_width;
+ copp->app_type = app_type;
+ }
I would suggest that you make adm_find_matching_copp() allocate the new
copp if none is found.
will have a go and see!
+
+ /* Create a COPP if port id are not enabled */
+ if (copp->cnt == 0) {
Doesn't this scheme require some locking? What about concurrent close()?
yes, it will be issue if we do concurrent closes(), will revisit locking
on this.
+
+ open.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
+ APR_HDR_LEN(APR_HDR_SIZE),
+ APR_PKT_VER);
+ open.hdr.pkt_size = sizeof(open);
+ open.hdr.src_svc = APR_SVC_ADM;
+ open.hdr.src_domain = APR_DOMAIN_APPS;
+ open.hdr.src_port = tmp_port;
+ open.hdr.dest_svc = APR_SVC_ADM;
+ open.hdr.dest_domain = APR_DOMAIN_ADSP;
+ open.hdr.dest_port = tmp_port;
+ open.hdr.token = port_idx << 16 | copp->copp_idx;
+ open.hdr.opcode = ADM_CMD_DEVICE_OPEN_V5;
+ open.flags = flags;
+ open.mode_of_operation = path;
+ open.endpoint_id_1 = tmp_port;
+ open.topology_id = topology;
+ open.dev_num_channel = channel_mode & 0x00FF;
+ open.bit_width = bit_width;
+ open.sample_rate = rate;
This looks like a q6adm_device_open() helper function.
yep!
+
+ ret = q6dsp_map_channels(&open.dev_channel_mapping[0],
+ channel_mode);
+
+ if (ret)
+ return ret;
+
+ copp->stat = -1;
+ ret = apr_send_pkt(adm->apr, (uint32_t *)&open);
+ if (ret < 0) {
+ dev_err(dev, "port_id: 0x%x for[0x%x] failed %d\n",
+ tmp_port, port_id, ret);
+ return -EINVAL;
+ }
+ /* Wait for the callback with copp id */
+ ret =
+ wait_event_timeout(copp->wait, copp->stat >= 0,
+ msecs_to_jiffies(TIMEOUT_MS));
+ if (!ret) {
+ dev_err(dev, "ADM timedout port_id: 0x%x for [0x%x]\n",
+ tmp_port, port_id);
+ return -EINVAL;
+ } else if (copp->stat > 0) {
+ dev_err(dev, "DSP returned error[%s]\n",
+ adsp_err_get_err_str(copp->stat));
+ return adsp_err_get_lnx_err_code(copp->stat);
+ }
+ }
+ copp->cnt++;
+ return copp->copp_idx;
+}
+EXPORT_SYMBOL_GPL(q6adm_open);
+/**
+ * q6adm_matrix_map() - Map asm streams and afe ports using payload
+ *
+ * @dev: Pointer to adm child device.
+ * @path: playback or capture path.
+ * @payload_map: map between session id and afe ports.
+ * @perf_mode: Performace mode.
+ *
+ * Return: Will be an negative on error or a zero on success.
+ */
+int q6adm_matrix_map(struct device *dev, int path,
+ struct route_payload payload_map, int perf_mode)
+{
+ struct adm_cmd_matrix_map_routings_v5 {
+ struct apr_hdr hdr;
+ u32 matrix_id;
+ u32 num_sessions;
+ } __packed * route;
+
+ struct adm_session_map_node_v5 {
+ u16 session_id;
+ u16 num_copps;
+ } __packed * node;
+ struct q6adm *adm = dev_get_drvdata(dev->parent);
+ uint16_t *copps_list;
+ int cmd_size = 0;
+ int ret = 0, i = 0;
+ void *payload = NULL;
+ void *matrix_map = NULL;
+ int port_idx, copp_idx;
+ struct copp *copp;
+
+ /* Assumes port_ids have already been validated during adm_open */
+ cmd_size = (sizeof(*route) +
+ sizeof(*node) +
+ (sizeof(uint32_t) * payload_map.num_copps));
+ matrix_map = kzalloc(cmd_size, GFP_KERNEL);
+ if (!matrix_map)
+ return -ENOMEM;
+
+ route = (struct adm_cmd_matrix_map_routings_v5 *)matrix_map;
+ route->hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
+ APR_HDR_LEN(APR_HDR_SIZE),
+ APR_PKT_VER);
+ route->hdr.pkt_size = cmd_size;
+ route->hdr.src_svc = 0;
+ route->hdr.src_domain = APR_DOMAIN_APPS;
+ route->hdr.src_port = 0; /* Ignored */
Omit the ignored members instead.
yep!
+ route->hdr.dest_svc = APR_SVC_ADM;
+ route->hdr.dest_domain = APR_DOMAIN_ADSP;
+ route->hdr.dest_port = 0; /* Ignored */
+ route->hdr.token = 0;
+ route->hdr.opcode = ADM_CMD_MATRIX_MAP_ROUTINGS_V5;
+ route->num_sessions = 1;
+
+ switch (path) {
+ case ADM_PATH_PLAYBACK:
+ route->matrix_id = ADM_MATRIX_ID_AUDIO_RX;
+ break;
+ default:
+ dev_err(dev, "Wrong path set[%d]\n", path);
+
+ break;
+ }
+
+ payload = ((u8 *) matrix_map + sizeof(*route));
matrix_map is a void *, so no need to cast it to u8 * to calculate the
payload address.
yep.
+ node = (struct adm_session_map_node_v5 *)payload;
payload is void *, so no need to typecast here. And for that matter, I'm
not sure about the benefits of having this intermediate "payload"
variable, just assign node directly.
+
+ node->session_id = payload_map.session_id;
+ node->num_copps = payload_map.num_copps;
+ payload = (u8 *) node + sizeof(*node);
+ copps_list = (uint16_t *) payload;
As with node above, drop the temporary variable and drop the type casts.
+
+ for (i = 0; i < payload_map.num_copps; i++) {
+ port_idx = payload_map.port_id[i];
+ if (port_idx < 0) {
+ dev_err(dev, "Invalid port_id 0x%x\n",
+ payload_map.port_id[i]);
Leaking matrix_map.
yep!
+ return -EINVAL;
+ }
+ copp_idx = payload_map.copp_idx[i];
+
+ copp = adm_find_copp(adm, port_idx, copp_idx);
+ if (IS_ERR_OR_NULL(copp))
Dito.
+ return -EINVAL;
+
+ copps_list[i] = copp->id;
+ }
+
+ adm->matrix_map_stat = -1;
+
+ ret = apr_send_pkt(adm->apr, (uint32_t *) matrix_map);
+ if (ret < 0) {
+ dev_err(dev, "routing for syream %d failed ret %d\n",
+ payload_map.session_id, ret);
+ ret = -EINVAL;
+ goto fail_cmd;
+ }
+ ret = wait_event_timeout(adm->matrix_map_wait,
+ adm->matrix_map_stat >= 0,
+ msecs_to_jiffies(TIMEOUT_MS));
+ if (!ret) {
+ dev_err(dev, "routing for syream %d failed\n",
+ payload_map.session_id);
+ ret = -EINVAL;
+ goto fail_cmd;
+ } else if (adm->matrix_map_stat > 0) {
+ dev_err(dev, "DSP returned error[%s]\n",
+ adsp_err_get_err_str(adm->matrix_map_stat));
+ ret = adsp_err_get_lnx_err_code(adm->matrix_map_stat);
+ goto fail_cmd;
+ }
+
+fail_cmd:
+ kfree(matrix_map);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(q6adm_matrix_map);
+
+static void adm_reset_copp(struct copp *c)
As far as I can see this will decommission the copp, so I don't think
there is a point in updating any of this and then keep it around?
yes, if we free it as suggested above we can get rid of this totally.
+{
+ c->id = RESET_COPP_ID;
+ c->cnt = 0;
+ c->topology = 0;
+ c->mode = 0;
+ c->stat = -1;
+ c->rate = 0;
+ c->channels = 0;
+ c->bit_width = 0;
+ c->app_type = 0;
+}
+/**
+ * q6adm_close() - Close adm copp
+ *
+ * @dev: Pointer to adm child device.
+ * @port_id: afe port id.
+ * @perf_mode: perf_mode mode
+ * @copp_idx: copp index to close
+ *
+ * Return: Will be an negative on error or a zero on success.
+ */
+int q6adm_close(struct device *dev, int port_id, int perf_mode, int copp_idx)
+{
+ struct apr_hdr close;
+ struct copp *copp;
+
+ int ret = 0, port_idx;
+ int copp_id = RESET_COPP_ID;
+ struct q6adm *adm = dev_get_drvdata(dev->parent);
+
+ port_idx = port_id;
+ if (port_idx < 0) {
+ dev_err(dev, "Invalid port_id 0x%x\n", port_id);
+ return -EINVAL;
+ }
+
+ if ((copp_idx < 0) || (copp_idx >= MAX_COPPS_PER_PORT)) {
+ dev_err(dev, "Invalid copp idx: %d\n", copp_idx);
+ return -EINVAL;
+ }
+
+ copp = adm_find_copp(adm, port_id, copp_idx);
+ if (IS_ERR_OR_NULL(copp))
+ return -EINVAL;
+
+ copp->cnt--;
+ if (!copp->cnt) {
Again, this needs some locking.
Yes, this needs some protection, i will revisit this part.
+ copp_id = copp->id;
+
+ close.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
+ APR_HDR_LEN(APR_HDR_SIZE),
+ APR_PKT_VER);
+ close.pkt_size = sizeof(close);
+ close.src_svc = APR_SVC_ADM;
+ close.src_domain = APR_DOMAIN_APPS;
+ close.src_port = port_id;
+ close.dest_svc = APR_SVC_ADM;
+ close.dest_domain = APR_DOMAIN_ADSP;
+ close.dest_port = copp_id;
+ close.token = port_idx << 16 | copp_idx;
+ close.opcode = ADM_CMD_DEVICE_CLOSE_V5;
+
Split this out into a separate helper function.
yep!
+ ret = apr_send_pkt(adm->apr, (uint32_t *) &close);
+ if (ret < 0) {
+ dev_err(dev, "ADM close failed %d\n", ret);
+ return -EINVAL;
+ }
+
+ ret = wait_event_timeout(copp->wait, copp->stat >= 0,
+ msecs_to_jiffies(TIMEOUT_MS));
+ if (!ret) {
+ dev_err(dev, "ADM cmd Route timedout for port 0x%x\n",
+ port_id);
+ return -EINVAL;
+ } else if (copp->stat > 0) {
+ dev_err(dev, "DSP returned error[%s]\n",
+ adsp_err_get_err_str(copp->stat));
+ return adsp_err_get_lnx_err_code(copp->stat);
+ }
+
+ adm_reset_copp(copp);
+ adm_free_copp(adm, copp, port_id);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(q6adm_close);
[..]
+static struct apr_driver qcom_q6adm_driver = {
+ .probe = q6adm_probe,
+ .remove = q6adm_exit,
+ .callback = adm_callback,
+ .id_table = adm_id,
+ .driver = {
+ .name = "qcom-q6adm",
+ },
Indentation.
yep.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
