Re: [RESEND PATCH v2 08/15] ASoC: qcom: q6asm: add support to audio stream apis
From: Srinivas Kandagatla
Date: Wed Jan 03 2018 - 11:30:47 EST
Thanks for your comments.
On 02/01/18 20:08, 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 open, write and media format commands
in the q6asm module.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
---
sound/soc/qcom/qdsp6/q6asm.c | 530 ++++++++++++++++++++++++++++++++++++++++++-
sound/soc/qcom/qdsp6/q6asm.h | 42 ++++
2 files changed, 571 insertions(+), 1 deletion(-)
diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
index 4be92441f524..dabd6509ef99 100644
--- a/sound/soc/qcom/qdsp6/q6asm.c
+++ b/sound/soc/qcom/qdsp6/q6asm.c
@@ -8,16 +8,34 @@
#include <linux/soc/qcom/apr.h>
#include <linux/device.h>
#include <linux/platform_device.h>
+#include <uapi/sound/asound.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/mm.h>
#include "q6asm.h"
#include "common.h"
+#define ASM_STREAM_CMD_CLOSE 0x00010BCD
+#define ASM_STREAM_CMD_FLUSH 0x00010BCE
+#define ASM_SESSION_CMD_PAUSE 0x00010BD3
+#define ASM_DATA_CMD_EOS 0x00010BDB
+#define DEFAULT_POPP_TOPOLOGY 0x00010BE4
+#define ASM_STREAM_CMD_FLUSH_READBUFS 0x00010C09
#define ASM_CMD_SHARED_MEM_MAP_REGIONS 0x00010D92
#define ASM_CMDRSP_SHARED_MEM_MAP_REGIONS 0x00010D93
#define ASM_CMD_SHARED_MEM_UNMAP_REGIONS 0x00010D94
-
+#define ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2 0x00010D98
+#define ASM_DATA_EVENT_WRITE_DONE_V2 0x00010D99
+#define ASM_SESSION_CMD_RUN_V2 0x00010DAA
+#define ASM_MEDIA_FMT_MULTI_CHANNEL_PCM_V2 0x00010DA5
+#define ASM_DATA_CMD_WRITE_V2 0x00010DAB
+#define ASM_SESSION_CMD_SUSPEND 0x00010DEC
+#define ASM_STREAM_CMD_OPEN_WRITE_V3 0x00010DB3
+
+#define ASM_LEGACY_STREAM_SESSION 0
+#define ASM_END_POINT_DEVICE_MATRIX 0
+#define DEFAULT_APP_TYPE 0
+#define TUN_WRITE_IO_MODE 0x0008 /* tunnel read write mode */
#define TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */
#define SYNC_IO_MODE 0x0001
#define ASYNC_IO_MODE 0x0002
Probably prettier to reorder these and make them Q6ASM_IO_MODE_xyz
Sure I will try that.
[..]
+static int32_t q6asm_callback(struct apr_device *adev,
This callback is an extracted part of q6asm_srvc_callback(), can it be
given a more descriptive name?
May be q6asm_stream_callback/q6asm_session_callback() should be better.
+ struct apr_client_data *data, int session_id)
+{
+ struct audio_client *ac;// = (struct audio_client *)priv;
+ uint32_t token;
+ uint32_t *payload;
+ uint32_t wakeup_flag = 1;
+ uint32_t client_event = 0;
+ struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
+
+ if (data == NULL)
+ return -EINVAL;
+
+ ac = q6asm_get_audio_client(q6asm, session_id);
+ if (!q6asm_is_valid_audio_client(ac))
+ return -EINVAL;
+
+ payload = data->payload;
+
+ if (data->opcode == APR_BASIC_RSP_RESULT) {
Move this into the switch.
Yep, will cleanup these instances.
+ token = data->token;
+ switch (payload[0]) {
This is again that common response struct.
yep!
[...]
+
+ return 0;
+}
+
static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
{
struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev);
@@ -415,12 +581,16 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *
struct audio_port_data *port;
uint32_t dir = 0;
uint32_t sid = 0;
+ int dest_port;
uint32_t *payload;
if (!data) {
dev_err(&adev->dev, "%s: Invalid CB\n", __func__);
return 0;
}
+ dest_port = (data->dest_port >> 8) & 0xFF;
+ if (dest_port)
+ return q6asm_callback(adev, data, dest_port);
You call dest_port "session_id" above, this seems to be a better name
for this variable.
yes
payload = data->payload;
sid = (data->token >> 8) & 0x0F;
@@ -540,6 +710,364 @@ struct audio_client *q6asm_audio_client_alloc(struct device *dev,
}
EXPORT_SYMBOL_GPL(q6asm_audio_client_alloc);
+static int __q6asm_open_write(struct audio_client *ac, uint32_t format,
+ uint16_t bits_per_sample, uint32_t stream_id,
+ bool is_gapless_mode)
+{
+ struct asm_stream_cmd_open_write_v3 open;
+ int rc;
+
+ q6asm_add_hdr(ac, &open.hdr, sizeof(open), true, stream_id);
+ ac->cmd_state = -1;
+
+ open.hdr.opcode = ASM_STREAM_CMD_OPEN_WRITE_V3;
+ open.mode_flags = 0x00;
+ open.mode_flags |= ASM_LEGACY_STREAM_SESSION;
+ if (is_gapless_mode)
This is hard coded as false.
Will clean this up.
+ open.mode_flags |= 1 << ASM_SHIFT_GAPLESS_MODE_FLAG;
+
+ /* source endpoint : matrix */
+ open.sink_endpointype = ASM_END_POINT_DEVICE_MATRIX;
+ open.bits_per_sample = bits_per_sample;
+ open.postprocopo_id = DEFAULT_POPP_TOPOLOGY;
+
+ switch (format) {
+ case FORMAT_LINEAR_PCM:
+ open.dec_fmt_id = ASM_MEDIA_FMT_MULTI_CHANNEL_PCM_V2;
+ break;
+ default:
+ dev_err(ac->dev, "Invalid format 0x%x\n", format);
+ return -EINVAL;
+ }
+ rc = apr_send_pkt(ac->adev, (uint32_t *) &open);
+ if (rc < 0)
+ return rc;
+
+ rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ);
+ if (!rc) {
+ dev_err(ac->dev, "timeout on open write\n");
+ return -ETIMEDOUT;
+ }
Almost every time you apr_send_pkt() you have this wait with timeout,
can this send/wait/return be wrapped in a helper function to reduce the
duplication?
Creating a q6asm_send_sync() and q6asm_send_async() pair with this logic
should help quite a bit.
will do that with all the apr drivers.
+
+ if (ac->cmd_state > 0)
+ return adsp_err_get_lnx_err_code(ac->cmd_state);
+
+ ac->io_mode |= TUN_WRITE_IO_MODE;
+
+ return 0;
+}
+
+/**
+ * q6asm_open_write() - Open audio client for writing
+ *
+ * @ac: audio client pointer
+ * @format: audio sample format
+ * @bits_per_sample: bits per sample
+ *
+ * Return: Will be an negative value on error or zero on success
+ */
+int q6asm_open_write(struct audio_client *ac, uint32_t format,
+ uint16_t bits_per_sample)
+{
+ return __q6asm_open_write(ac, format, bits_per_sample,
I don't see a particular reason for not inlining this, is there one
coming later in the series?
No, will clean it up.
+ ac->stream_id, false);
+}
+EXPORT_SYMBOL_GPL(q6asm_open_write);
+
+static int __q6asm_run(struct audio_client *ac, uint32_t flags,
+ uint32_t msw_ts, uint32_t lsw_ts, bool wait)
+{
+ struct asm_session_cmd_run_v2 run;
+ int rc;
+
+ q6asm_add_hdr(ac, &run.hdr, sizeof(run), true, ac->stream_id);
+ ac->cmd_state = -1;
+
+ run.hdr.opcode = ASM_SESSION_CMD_RUN_V2;
+ run.flags = flags;
+ run.time_lsw = lsw_ts;
+ run.time_msw = msw_ts;
+
+ rc = apr_send_pkt(ac->adev, (uint32_t *) &run);
+ if (rc < 0)
+ return rc;
+
+ if (wait) {
Rather than having half of the function conditional I would recommend
inlining this function in the two callers.
In particular if you can come up with a helper function for the
send/wait/handle-error case.
sure.
+ rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0),
+ 5 * HZ);
+ if (!rc) {
+ dev_err(ac->dev, "timeout on run cmd\n");
+ return -ETIMEDOUT;
+ }
+ if (ac->cmd_state > 0)
+ return adsp_err_get_lnx_err_code(ac->cmd_state);
+ }
+
+ return 0;
+}
+/**
+ * q6asm_media_format_block_multi_ch_pcm() - setup pcm configuration
+ *
+ * @ac: audio client pointer
+ * @rate: audio sample rate
+ * @channels: number of audio channels.
+ * @use_default_chmap: flag to use default ch map.
+ * @channel_map: channel map pointer
+ * @bits_per_sample: bits per sample
+ *
+ * Return: Will be an negative value on error or zero on success
+ */
+int q6asm_media_format_block_multi_ch_pcm(struct audio_client *ac,
+ uint32_t rate, uint32_t channels,
+ bool use_default_chmap,
+ char *channel_map,
This should be u8 channel_map[PCM_FORMAT_MAX_NUM_CHANNEL], possibly
char. Unless you, as I suggest below, want to be able to represent
use_default_chmap = false, by setting this to NULL.
+ uint16_t bits_per_sample)
+{
+ struct asm_multi_channel_pcm_fmt_blk_v2 fmt;
+ u8 *channel_mapping;
+ int rc = 0;
Unnecessary initialization.
yep.
+
+ q6asm_add_hdr(ac, &fmt.hdr, sizeof(fmt), true, ac->stream_id);
+ ac->cmd_state = -1;
+
+ fmt.hdr.opcode = ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2;
+ fmt.fmt_blk.fmt_blk_size = sizeof(fmt) - sizeof(fmt.hdr) -
+ sizeof(fmt.fmt_blk);
+ fmt.num_channels = channels;
+ fmt.bits_per_sample = bits_per_sample;
+ fmt.sample_rate = rate;
+ fmt.is_signed = 1;
+
+ channel_mapping = fmt.channel_mapping;
+
+ if (use_default_chmap) {
Passing NULL as channel_map would probably be a nicer way to say this,
instead of having a separate bool.
I will give it a go and see.
+ if (q6dsp_map_channels(channel_mapping, channels)) {
+ dev_err(ac->dev, " map channels failed %d\n", channels);
+ return -EINVAL;
+ }
+ } else {
+ memcpy(channel_mapping, channel_map,
+ PCM_FORMAT_MAX_NUM_CHANNEL);
+ }
+
+ rc = apr_send_pkt(ac->adev, (uint32_t *) &fmt);
+ if (rc < 0)
+ goto fail_cmd;
+
+ rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ);
+ if (!rc) {
+ dev_err(ac->dev, "timeout on format update\n");
+ return -ETIMEDOUT;
+ }
+ if (ac->cmd_state > 0)
+ return adsp_err_get_lnx_err_code(ac->cmd_state);
+
+ return 0;
+fail_cmd:
+ return rc;
+}
+EXPORT_SYMBOL_GPL(q6asm_media_format_block_multi_ch_pcm);
+
+/**
+ * q6asm_write_nolock() - non blocking write
+ *
+ * @ac: audio client pointer
+ * @len: lenght in bytes
+ * @msw_ts: timestamp msw
+ * @lsw_ts: timestamp lsw
+ * @flags: flags associated with write
+ *
+ * Return: Will be an negative value on error or zero on success
+ */
+int q6asm_write_nolock(struct audio_client *ac, uint32_t len, uint32_t msw_ts,
+ uint32_t lsw_ts, uint32_t flags)
q6asm_write_async() is probably a better name, nolock indicates some
relationship to mutual exclusions...
yep.
+{
+ struct asm_data_cmd_write_v2 write;
+ struct audio_port_data *port;
+ struct audio_buffer *ab;
+ int dsp_buf = 0;
+ int rc = 0;
+
+ if (ac->io_mode & SYNC_IO_MODE) {
Bail early if this isn't true, to save you the indentation level.
yep.
+ port = &ac->port[SNDRV_PCM_STREAM_PLAYBACK];
+ q6asm_add_hdr(ac, &write.hdr, sizeof(write), false,
+ ac->stream_id);
+
+ dsp_buf = port->dsp_buf;
+ ab = &port->buf[dsp_buf];
So we're just unconditionally telling the remote side about the next buf
in our ring buffer. Do we need to ensure that this is available/ready?
This is already synchronized at the top layer in q6asm_dai driver.
+
+ write.hdr.token = port->dsp_buf;
+ write.hdr.opcode = ASM_DATA_CMD_WRITE_V2;
+ write.buf_addr_lsw = lower_32_bits(ab->phys);
+ write.buf_addr_msw = upper_32_bits(ab->phys);
+ write.buf_size = len;
+ write.seq_id = port->dsp_buf;
+ write.timestamp_lsw = lsw_ts;
+ write.timestamp_msw = msw_ts;
+ write.mem_map_handle =
+ ac->port[SNDRV_PCM_STREAM_PLAYBACK].mem_map_handle;
+
+ if (flags == NO_TIMESTAMP)
+ write.flags = (flags & 0x800000FF);
Fill in the constant and this becomes
if flags == 0xff00:
write.flags = 0xff00 & 0x800000ff;
Or in other words:
if flags == 0xff00:
write.flags = 0;
+ else
+ write.flags = (0x80000000 | flags);
Drop the parenthesis and flip the |. It would be nice to have a define
or a comment indicating what BIT(31) is...
sure, I will make add more information here on the flag and also cleanup
as suggested.
+
+ port->dsp_buf++;
+
+ if (port->dsp_buf >= port->max_buf_cnt)
+ port->dsp_buf = 0;
+
+ rc = apr_send_pkt(ac->adev, (uint32_t *) &write);
+ if (rc < 0)
+ return rc;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(q6asm_write_nolock);
[...]
+
+static int __q6asm_cmd(struct audio_client *ac, int cmd, bool wait)
+{
+ int stream_id = ac->stream_id;
+ struct apr_hdr hdr;
+ int rc;
+
+ q6asm_add_hdr(ac, &hdr, sizeof(hdr), true, stream_id);
+ ac->cmd_state = -1;
Resetting cmd_state relates to the send, don't mix it with building the
packet.
Sure.
+ switch (cmd) {
+ case CMD_PAUSE:
+ hdr.opcode = ASM_SESSION_CMD_PAUSE;
+ break;
+ case CMD_SUSPEND:
+ hdr.opcode = ASM_SESSION_CMD_SUSPEND;
+ break;
+ case CMD_FLUSH:
+ hdr.opcode = ASM_STREAM_CMD_FLUSH;
+ break;
+ case CMD_OUT_FLUSH:
+ hdr.opcode = ASM_STREAM_CMD_FLUSH_READBUFS;
+ break;
+ case CMD_EOS:
+ hdr.opcode = ASM_DATA_CMD_EOS;
+ ac->cmd_state = 0;
+ break;
+ case CMD_CLOSE:
+ hdr.opcode = ASM_STREAM_CMD_CLOSE;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ rc = apr_send_pkt(ac->adev, (uint32_t *) &hdr);
+ if (rc < 0)
+ return rc;
+
+ if (!wait)
+ return 0;
I've asked you to split the others into _sync() vs _async() operations.
One particular concern I have is that I don't see any mutual exclusion
protecting the cmd_state and a call with !wait will overwrite the
existing value, which might be unexpected.
yes, this will be issue, we could move setting cmd_state to here.
Also I will revisit _sync() function to make sure that these are
sequenced correctly and async are not touching the cmd_state.
+
+ rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ);
+ if (!rc) {
+ dev_err(ac->dev, "timeout response for opcode[0x%x]\n",
+ hdr.opcode);
+ return -ETIMEDOUT;
+ }
+ if (ac->cmd_state > 0)
+ return adsp_err_get_lnx_err_code(ac->cmd_state);
+
+ if (cmd == CMD_FLUSH)
+ q6asm_reset_buf_state(ac);
+
+ return 0;
+}
[..]
diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h
index e1409c368600..b4896059da79 100644
--- a/sound/soc/qcom/qdsp6/q6asm.h
+++ b/sound/soc/qcom/qdsp6/q6asm.h
@@ -2,7 +2,34 @@
#ifndef __Q6_ASM_H__
#define __Q6_ASM_H__
+/* ASM client callback events */
+#define CMD_PAUSE 0x0001
These defines has rather generic names...
I can prefix them with Q6ASM to make it much more specific to Q6ASM service.
[..]
+
+#define MSM_FRONTEND_DAI_MULTIMEDIA1 0
+#define MSM_FRONTEND_DAI_MULTIMEDIA2 1
+#define MSM_FRONTEND_DAI_MULTIMEDIA3 2
+#define MSM_FRONTEND_DAI_MULTIMEDIA4 3
+#define MSM_FRONTEND_DAI_MULTIMEDIA5 4
+#define MSM_FRONTEND_DAI_MULTIMEDIA6 5
+#define MSM_FRONTEND_DAI_MULTIMEDIA7 6
+#define MSM_FRONTEND_DAI_MULTIMEDIA8 7
+
#define MAX_SESSIONS 16
+#define NO_TIMESTAMP 0xFF00
+#define FORMAT_LINEAR_PCM 0x0000
Ditto.
Regards,
Bjorn