Re: [RESEND PATCH v2 08/15] ASoC: qcom: q6asm: add support to audio stream apis

On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote:

Probably prettier to reorder these and make them Q6ASM_IO_MODE_xyz

[..]

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

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

+		token = data->token;
+		switch (payload[0]) {

This is again that common response struct.

You call dest_port "session_id" above, this seems to be a better name
for this variable.

This is hard coded as false.

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

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

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

+		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_run() - start the audio client
+ *
+ * @ac: audio client pointer
+ * @flags: flags associated with write
+ * @msw_ts: timestamp msw
+ * @lsw_ts: timestamp lsw
+ *
+ * Return: Will be an negative value on error or zero on success
+ */
+int q6asm_run(struct audio_client *ac, uint32_t flags,
+	      uint32_t msw_ts, uint32_t lsw_ts)
+{
+	return __q6asm_run(ac, flags, msw_ts, lsw_ts, true);
+}
+EXPORT_SYMBOL_GPL(q6asm_run);
+
+/**
+ * q6asm_run_nowait() - start the audio client withou blocking
+ *
+ * @ac: audio client pointer
+ * @flags: flags associated with write
+ * @msw_ts: timestamp msw
+ * @lsw_ts: timestamp lsw
+ *
+ * Return: Will be an negative value on error or zero on success
+ */
+int q6asm_run_nowait(struct audio_client *ac, uint32_t flags,
+	      uint32_t msw_ts, uint32_t lsw_ts)
+{
+	return __q6asm_run(ac, flags, msw_ts, lsw_ts, false);
+}
+EXPORT_SYMBOL_GPL(q6asm_run_nowait);
+
+/**
+ * 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.

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

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

+{
+	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.

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

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

+
+		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 void q6asm_reset_buf_state(struct audio_client *ac)
+{
+	int cnt = 0;
+	int loopcnt = 0;
+	int used;
+	struct audio_port_data *port = NULL;
+
+	if (ac->io_mode & SYNC_IO_MODE) {
+		used = (ac->io_mode & TUN_WRITE_IO_MODE ? 1 : 0);
+		mutex_lock(&ac->cmd_lock);
+		for (loopcnt = 0; loopcnt <= SNDRV_PCM_STREAM_CAPTURE;
+		     loopcnt++) {
+			port = &ac->port[loopcnt];
+			cnt = port->max_buf_cnt - 1;
+			port->dsp_buf = 0;
+			while (cnt >= 0) {
+				if (!port->buf)
+					continue;
+				port->buf[cnt].used = used;
+				cnt--;
+			}
+		}
+		mutex_unlock(&ac->cmd_lock);
+	}
+}
+
+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.

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

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

[..]

These defines has rather generic names...

[..]

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