[PATCH 0/9] ASoC: qcom: q6dsp: fixes and updates

[PATCH 0/9] ASoC: qcom: q6dsp: fixes and updates

[Srinivas Kandagatla]

This patchset has 4 fixes and some enhancements to the Elite DSP driver
support.
Fixes includes 
	- setting correct flags for expected behaviour of appl_ptr
	- fix closing of copp instances
	- fix buffer alignment.
	- fix state checks before closing asm stream
Enhancements include:
	- adding q6asm_get_hw_pointer and ack callback support

There is another set of updates comming soon, which will add support
for early memory mapping and few more modules support in audioreach.

Srinivas Kandagatla (9):
  ASoC: qcom: q6apm-dai: set flags to reflect correct operation of
    appl_ptr
  ASoC: qcom: q6adm: the the copp device only during last instance
  ASoC: qcom: qdsp6: q6asm-dai: set 10 ms period and buffer alignment.
  ASoC: qcom: q6asm-dai: perform correct state check before closing
  ASoC: qcom: q6asm: handle the responses after closing
  ASoC: qcom: q6asm-dai: schedule all available frames to avoid dsp
    under-runs
  ASoC: qcom: q6asm: add q6asm_get_hw_pointer
  ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer
  ASoC: qcom: q6asm: set runtime correctly for each stream

 sound/soc/qcom/qdsp6/q6adm.c     | 146 +++++++++++++++----------------
 sound/soc/qcom/qdsp6/q6apm-dai.c |   2 +
 sound/soc/qcom/qdsp6/q6asm-dai.c |  64 +++++++++-----
 sound/soc/qcom/qdsp6/q6asm.c     |  17 ++++
 sound/soc/qcom/qdsp6/q6asm.h     |   1 +
 5 files changed, 134 insertions(+), 96 deletions(-)

-- 
2.51.0

[PATCH 1/9] ASoC: qcom: q6apm-dai: set flags to reflect correct operation of appl_ptr

[Srinivas Kandagatla]

Driver does not expect the appl_ptr to move backward and requires
explict sync. Make sure that the userspace does not do appl_ptr rewinds
by specifying the correct flags in pcm_info.

Without this patch, the result could be a forever loop as current logic assumes
that appl_ptr can only move forward.

Fixes: 3d4a4411aa8b ("ASoC: q6apm-dai: schedule all available frames to avoid dsp under-runs")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6apm-dai.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
index 4ecaff45c518..786ab3222515 100644
--- a/sound/soc/qcom/qdsp6/q6apm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
@@ -86,6 +86,7 @@ static const struct snd_pcm_hardware q6apm_dai_hardware_capture = {
 	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				 SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED |
 				 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME |
+				 SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_SYNC_APPLPTR |
 				 SNDRV_PCM_INFO_BATCH),
 	.formats =              (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE),
 	.rates =                SNDRV_PCM_RATE_8000_48000,
@@ -105,6 +106,7 @@ static const struct snd_pcm_hardware q6apm_dai_hardware_playback = {
 	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				 SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED |
 				 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME |
+				 SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_SYNC_APPLPTR |
 				 SNDRV_PCM_INFO_BATCH),
 	.formats =              (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE),
 	.rates =                SNDRV_PCM_RATE_8000_192000,
-- 
2.51.0

[PATCH 2/9] ASoC: qcom: q6adm: the the copp device only during last instance

[Srinivas Kandagatla]

A matching Common object post processing instance is normally resused
across multiple streams. However currently we close this on DSP
eventhough there is a refcount on this copp object, this can result in
below error.

q6routing ab00000.remoteproc:glink-edge:apr:service@8:routing: Found Matching Copp 0x0
qcom-q6adm aprsvc:service:4:8: cmd = 0x10325 return error = 0x2
q6routing ab00000.remoteproc:glink-edge:apr:service@8:routing: DSP returned error[2]
q6routing ab00000.remoteproc:glink-edge:apr:service@8:routing: Found Matching Copp 0x0
qcom-q6adm aprsvc:service:4:8: cmd = 0x10325 return error = 0x2
q6routing ab00000.remoteproc:glink-edge:apr:service@8:routing: DSP returned error[2]
qcom-q6adm aprsvc:service:4:8: cmd = 0x10327 return error = 0x2
qcom-q6adm aprsvc:service:4:8: DSP returned error[2]
qcom-q6adm aprsvc:service:4:8: Failed to close copp -22
qcom-q6adm aprsvc:service:4:8: cmd = 0x10327 return error = 0x2
qcom-q6adm aprsvc:service:4:8: DSP returned error[2]
qcom-q6adm aprsvc:service:4:8: Failed to close copp -22

Fix this by addressing moving the adm_close to copp_kref destructor
callback.

Fixes: 7b20b2be51e1 ("ASoC: qdsp6: q6adm: Add q6adm driver")
Cc: <Stable@vger.kernel.org>
Reported-by: Martino Facchin <m.facchin@arduino.cc>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6adm.c | 146 +++++++++++++++++------------------
 1 file changed, 71 insertions(+), 75 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6adm.c b/sound/soc/qcom/qdsp6/q6adm.c
index 1530e98df165..75a029a696ac 100644
--- a/sound/soc/qcom/qdsp6/q6adm.c
+++ b/sound/soc/qcom/qdsp6/q6adm.c
@@ -109,11 +109,75 @@ static struct q6copp *q6adm_find_copp(struct q6adm *adm, int port_idx,
 
 }
 
+static int q6adm_apr_send_copp_pkt(struct q6adm *adm, struct q6copp *copp,
+				   struct apr_pkt *pkt, uint32_t rsp_opcode)
+{
+	struct device *dev = adm->dev;
+	uint32_t opcode = pkt->hdr.opcode;
+	int ret;
+
+	mutex_lock(&adm->lock);
+	copp->result.opcode = 0;
+	copp->result.status = 0;
+	ret = apr_send_pkt(adm->apr, pkt);
+	if (ret < 0) {
+		dev_err(dev, "Failed to send APR packet\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* Wait for the callback with copp id */
+	if (rsp_opcode)
+		ret = wait_event_timeout(copp->wait,
+					 (copp->result.opcode == opcode) ||
+					 (copp->result.opcode == rsp_opcode),
+					 msecs_to_jiffies(TIMEOUT_MS));
+	else
+		ret = wait_event_timeout(copp->wait,
+					 (copp->result.opcode == opcode),
+					 msecs_to_jiffies(TIMEOUT_MS));
+
+	if (!ret) {
+		dev_err(dev, "ADM copp cmd timedout\n");
+		ret = -ETIMEDOUT;
+	} else if (copp->result.status > 0) {
+		dev_err(dev, "DSP returned error[%d]\n",
+			copp->result.status);
+		ret = -EINVAL;
+	}
+
+err:
+	mutex_unlock(&adm->lock);
+	return ret;
+}
+
+static int q6adm_device_close(struct q6adm *adm, struct q6copp *copp,
+			      int port_id, int copp_idx)
+{
+	struct apr_pkt close;
+
+	close.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
+					APR_HDR_LEN(APR_HDR_SIZE),
+					APR_PKT_VER);
+	close.hdr.pkt_size = sizeof(close);
+	close.hdr.src_port = port_id;
+	close.hdr.dest_port = copp->id;
+	close.hdr.token = port_id << 16 | copp_idx;
+	close.hdr.opcode = ADM_CMD_DEVICE_CLOSE_V5;
+
+	return q6adm_apr_send_copp_pkt(adm, copp, &close, 0);
+}
+
 static void q6adm_free_copp(struct kref *ref)
 {
 	struct q6copp *c = container_of(ref, struct q6copp, refcount);
 	struct q6adm *adm = c->adm;
 	unsigned long flags;
+	int ret;
+
+	ret = q6adm_device_close(adm, c, c->afe_port, c->copp_idx);
+	if (ret < 0)
+		dev_err(adm->dev, "Failed to close copp %d\n", ret);
 
 	spin_lock_irqsave(&adm->copps_list_lock, flags);
 	clear_bit(c->copp_idx, &adm->copp_bitmap[c->afe_port]);
@@ -155,13 +219,13 @@ static int q6adm_callback(struct apr_device *adev, struct apr_resp_pkt *data)
 		switch (result->opcode) {
 		case ADM_CMD_DEVICE_OPEN_V5:
 		case ADM_CMD_DEVICE_CLOSE_V5:
-			copp = q6adm_find_copp(adm, port_idx, copp_idx);
-			if (!copp)
-				return 0;
-
-			copp->result = *result;
-			wake_up(&copp->wait);
-			kref_put(&copp->refcount, q6adm_free_copp);
+			list_for_each_entry(copp, &adm->copps_list, node) {
+				if ((port_idx == copp->afe_port) && (copp_idx == copp->copp_idx)) {
+					copp->result = *result;
+					wake_up(&copp->wait);
+					break;
+				}
+			}
 			break;
 		case ADM_CMD_MATRIX_MAP_ROUTINGS_V5:
 			adm->result = *result;
@@ -234,65 +298,6 @@ static struct q6copp *q6adm_alloc_copp(struct q6adm *adm, int port_idx)
 	return c;
 }
 
-static int q6adm_apr_send_copp_pkt(struct q6adm *adm, struct q6copp *copp,
-				   struct apr_pkt *pkt, uint32_t rsp_opcode)
-{
-	struct device *dev = adm->dev;
-	uint32_t opcode = pkt->hdr.opcode;
-	int ret;
-
-	mutex_lock(&adm->lock);
-	copp->result.opcode = 0;
-	copp->result.status = 0;
-	ret = apr_send_pkt(adm->apr, pkt);
-	if (ret < 0) {
-		dev_err(dev, "Failed to send APR packet\n");
-		ret = -EINVAL;
-		goto err;
-	}
-
-	/* Wait for the callback with copp id */
-	if (rsp_opcode)
-		ret = wait_event_timeout(copp->wait,
-					 (copp->result.opcode == opcode) ||
-					 (copp->result.opcode == rsp_opcode),
-					 msecs_to_jiffies(TIMEOUT_MS));
-	else
-		ret = wait_event_timeout(copp->wait,
-					 (copp->result.opcode == opcode),
-					 msecs_to_jiffies(TIMEOUT_MS));
-
-	if (!ret) {
-		dev_err(dev, "ADM copp cmd timedout\n");
-		ret = -ETIMEDOUT;
-	} else if (copp->result.status > 0) {
-		dev_err(dev, "DSP returned error[%d]\n",
-			copp->result.status);
-		ret = -EINVAL;
-	}
-
-err:
-	mutex_unlock(&adm->lock);
-	return ret;
-}
-
-static int q6adm_device_close(struct q6adm *adm, struct q6copp *copp,
-			      int port_id, int copp_idx)
-{
-	struct apr_pkt close;
-
-	close.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
-					APR_HDR_LEN(APR_HDR_SIZE),
-					APR_PKT_VER);
-	close.hdr.pkt_size = sizeof(close);
-	close.hdr.src_port = port_id;
-	close.hdr.dest_port = copp->id;
-	close.hdr.token = port_id << 16 | copp_idx;
-	close.hdr.opcode = ADM_CMD_DEVICE_CLOSE_V5;
-
-	return q6adm_apr_send_copp_pkt(adm, copp, &close, 0);
-}
-
 static struct q6copp *q6adm_find_matching_copp(struct q6adm *adm,
 					       int port_id, int topology,
 					       int mode, int rate,
@@ -567,15 +572,6 @@ EXPORT_SYMBOL_GPL(q6adm_matrix_map);
  */
 int q6adm_close(struct device *dev, struct q6copp *copp)
 {
-	struct q6adm *adm = dev_get_drvdata(dev->parent);
-	int ret = 0;
-
-	ret = q6adm_device_close(adm, copp, copp->afe_port, copp->copp_idx);
-	if (ret < 0) {
-		dev_err(adm->dev, "Failed to close copp %d\n", ret);
-		return ret;
-	}
-
 	kref_put(&copp->refcount, q6adm_free_copp);
 
 	return 0;
-- 
2.51.0

[PATCH 3/9] ASoC: qcom: qdsp6: q6asm-dai: set 10 ms period and buffer alignment.

[Srinivas Kandagatla]

DSP expects the periods to be aligned to fragment sizes, currently
setting up to hw constriants on periods bytes is not going to work
correctly as we can endup with periods sizes aligned to 32 bytes however
not aligned to fragment size.

Update the constriants to use fragment size, and also set at step of
10ms for period size to accommodate DSP requirements of 10ms latency.

Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index b616ce316d2f..e8129510a734 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -403,13 +403,13 @@ static int q6asm_dai_open(struct snd_soc_component *component,
 	}
 
 	ret = snd_pcm_hw_constraint_step(runtime, 0,
-		SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32);
+		SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 480);
 	if (ret < 0) {
 		dev_err(dev, "constraint for period bytes step ret = %d\n",
 								ret);
 	}
 	ret = snd_pcm_hw_constraint_step(runtime, 0,
-		SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32);
+		SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 480);
 	if (ret < 0) {
 		dev_err(dev, "constraint for buffer bytes step ret = %d\n",
 								ret);
-- 
2.51.0

[PATCH 4/9] ASoC: qcom: q6asm-dai: perform correct state check before closing

[Srinivas Kandagatla]

Do not stop a q6asm stream if its not started, this can result in
unnecessary dsp command which will timeout anyway something like below:

q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: CMD 10bcd timeout

Fix this by correctly checking the state.

Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index e8129510a734..0eae8c6e42b8 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -233,13 +233,14 @@ static int q6asm_dai_prepare(struct snd_soc_component *component,
 	prtd->pcm_count = snd_pcm_lib_period_bytes(substream);
 	prtd->pcm_irq_pos = 0;
 	/* rate and channels are sent to audio driver */
-	if (prtd->state) {
+	if (prtd->state == Q6ASM_STREAM_RUNNING) {
 		/* clear the previous setup if any  */
 		q6asm_cmd(prtd->audio_client, prtd->stream_id, CMD_CLOSE);
 		q6asm_unmap_memory_regions(substream->stream,
 					   prtd->audio_client);
 		q6routing_stream_close(soc_prtd->dai_link->id,
 					 substream->stream);
+		prtd->state = Q6ASM_STREAM_STOPPED;
 	}
 
 	ret = q6asm_map_memory_regions(substream->stream, prtd->audio_client,
-- 
2.51.0

[PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Srinivas Kandagatla]

For record path, multiple read requests are queued to dsp in advance.
However when data stream is closed, the pending read requests are rejected
by the dsp and a response is sent to the driver, this case is not handled
in the driver resulting in errors like below

q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp

This is same for both write and eos.

Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
ASM_DATA_CMD_WRITE_V2 as expected response.

Reported-by: Martino Facchin <m.facchin@arduino.cc>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
index 06a802f9dba5..371389c8fa7a 100644
--- a/sound/soc/qcom/qdsp6/q6asm.c
+++ b/sound/soc/qcom/qdsp6/q6asm.c
@@ -638,6 +638,7 @@ static int32_t q6asm_stream_callback(struct apr_device *adev,
 			client_event = ASM_CLIENT_EVENT_CMD_OUT_FLUSH_DONE;
 			break;
 		case ASM_STREAM_CMD_OPEN_WRITE_V3:
+		case ASM_DATA_CMD_WRITE_V2:
 		case ASM_STREAM_CMD_OPEN_READ_V3:
 		case ASM_STREAM_CMD_OPEN_READWRITE_V2:
 		case ASM_STREAM_CMD_SET_ENCDEC_PARAM:
@@ -654,6 +655,10 @@ static int32_t q6asm_stream_callback(struct apr_device *adev,
 				goto done;
 			}
 			break;
+		case ASM_DATA_CMD_EOS:
+		case ASM_DATA_CMD_READ_V2:
+			/* response as result of close stream */
+			break;
 		default:
 			dev_err(ac->dev, "command[0x%x] not expecting rsp\n",
 				result->opcode);
-- 
2.51.0

[PATCH 6/9] ASoC: qcom: q6asm-dai: schedule all available frames to avoid dsp under-runs

[Srinivas Kandagatla]

With the existing code, we are only setting up one period at a time, in a
ping-pong buffer style. This triggers lot of underruns in the dsp
leading to jitter noise during audio playback.

Fix this by scheduling all available periods, this will ensure that the dsp
has enough buffer feed and ultimatley fixing the underruns and audio distortion.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 34 +++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index 0eae8c6e42b8..db2ea8973ac9 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -64,6 +64,7 @@ struct q6asm_dai_rtd {
 	uint64_t bytes_received;
 	uint64_t copied_total;
 	uint16_t bits_per_sample;
+	snd_pcm_uframes_t queue_ptr;
 	uint16_t source; /* Encoding source bit mask */
 	struct audio_client *audio_client;
 	uint32_t next_track_stream_id;
@@ -85,6 +86,7 @@ struct q6asm_dai_data {
 static const struct snd_pcm_hardware q6asm_dai_hardware_capture = {
 	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BATCH |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
+				SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_SYNC_APPLPTR |
 				SNDRV_PCM_INFO_MMAP_VALID |
 				SNDRV_PCM_INFO_INTERLEAVED |
 				SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
@@ -108,6 +110,7 @@ static const struct snd_pcm_hardware q6asm_dai_hardware_playback = {
 	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BATCH |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				SNDRV_PCM_INFO_MMAP_VALID |
+				SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_SYNC_APPLPTR |
 				SNDRV_PCM_INFO_INTERLEAVED |
 				SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
 	.formats =              (SNDRV_PCM_FMTBIT_S16_LE |
@@ -182,9 +185,6 @@ static void event_handler(uint32_t opcode, uint32_t token,
 
 	switch (opcode) {
 	case ASM_CLIENT_EVENT_CMD_RUN_DONE:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			q6asm_write_async(prtd->audio_client, prtd->stream_id,
-				   prtd->pcm_count, 0, 0, 0);
 		break;
 	case ASM_CLIENT_EVENT_CMD_EOS_DONE:
 		prtd->state = Q6ASM_STREAM_STOPPED;
@@ -192,10 +192,6 @@ static void event_handler(uint32_t opcode, uint32_t token,
 	case ASM_CLIENT_EVENT_DATA_WRITE_DONE: {
 		prtd->pcm_irq_pos += prtd->pcm_count;
 		snd_pcm_period_elapsed(substream);
-		if (prtd->state == Q6ASM_STREAM_RUNNING)
-			q6asm_write_async(prtd->audio_client, prtd->stream_id,
-					   prtd->pcm_count, 0, 0, 0);
-
 		break;
 		}
 	case ASM_CLIENT_EVENT_DATA_READ_DONE:
@@ -311,6 +307,29 @@ static int q6asm_dai_prepare(struct snd_soc_component *component,
 	return ret;
 }
 
+static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct q6asm_dai_rtd *prtd = runtime->private_data;
+	int i, ret = 0, avail_periods;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
+		avail_periods = (runtime->control->appl_ptr - prtd->queue_ptr)/runtime->period_size;
+		for (i = 0; i < avail_periods; i++) {
+			ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
+					   prtd->pcm_count, 0, 0, 0);
+
+			if (ret < 0) {
+				dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
+				return ret;
+			}
+			prtd->queue_ptr += runtime->period_size;
+		}
+	}
+
+	return ret;
+}
+
 static int q6asm_dai_trigger(struct snd_soc_component *component,
 			     struct snd_pcm_substream *substream, int cmd)
 {
@@ -1215,6 +1234,7 @@ static const struct snd_soc_component_driver q6asm_fe_dai_component = {
 	.close			= q6asm_dai_close,
 	.prepare		= q6asm_dai_prepare,
 	.trigger		= q6asm_dai_trigger,
+	.ack			= q6asm_dai_ack,
 	.pointer		= q6asm_dai_pointer,
 	.pcm_construct		= q6asm_dai_pcm_new,
 	.compress_ops		= &q6asm_dai_compress_ops,
-- 
2.51.0

[PATCH 7/9] ASoC: qcom: q6asm: add q6asm_get_hw_pointer

[Srinivas Kandagatla]

Currently we are performing an extra layer of calculation on the hw_ptr,
which is always prone to errors.
Move this to common dsp layer for better accuracy.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm.c | 12 ++++++++++++
 sound/soc/qcom/qdsp6/q6asm.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
index 371389c8fa7a..643ca944b1b5 100644
--- a/sound/soc/qcom/qdsp6/q6asm.c
+++ b/sound/soc/qcom/qdsp6/q6asm.c
@@ -6,6 +6,7 @@
 #include <linux/mutex.h>
 #include <linux/wait.h>
 #include <linux/module.h>
+#include <linux/atomic.h>
 #include <linux/soc/qcom/apr.h>
 #include <linux/device.h>
 #include <linux/of_platform.h>
@@ -248,6 +249,7 @@ struct audio_port_data {
 	uint32_t num_periods;
 	uint32_t dsp_buf;
 	uint32_t mem_map_handle;
+	atomic_t hw_ptr;
 };
 
 struct q6asm {
@@ -598,6 +600,14 @@ static struct audio_client *q6asm_get_audio_client(struct q6asm *a,
 	return ac;
 }
 
+int q6asm_get_hw_pointer(struct audio_client *ac, unsigned int dir)
+{
+	struct audio_port_data *data = &ac->port[dir];
+
+	return (int)atomic_read(&data->hw_ptr);
+}
+EXPORT_SYMBOL_GPL(q6asm_get_hw_pointer);
+
 static int32_t q6asm_stream_callback(struct apr_device *adev,
 				     struct apr_resp_pkt *data,
 				     int session_id)
@@ -703,6 +713,7 @@ static int32_t q6asm_stream_callback(struct apr_device *adev,
 				goto done;
 			}
 			spin_unlock_irqrestore(&ac->lock, flags);
+			atomic_set(&port->hw_ptr, token + 1);
 		}
 		break;
 	case ASM_DATA_EVENT_READ_DONE_V2:
@@ -721,6 +732,7 @@ static int32_t q6asm_stream_callback(struct apr_device *adev,
 			}
 
 			phys = port->buf[hdr->token].phys;
+			atomic_set(&port->hw_ptr, hdr->token + 1);
 
 			if (upper_32_bits(phys) != done->buf_addr_msw ||
 			    lower_32_bits(phys) != done->buf_addr_lsw) {
diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h
index 519e1b3a3f7c..6fafda5bd849 100644
--- a/sound/soc/qcom/qdsp6/q6asm.h
+++ b/sound/soc/qcom/qdsp6/q6asm.h
@@ -148,4 +148,5 @@ int q6asm_map_memory_regions(unsigned int dir,
 			     phys_addr_t phys,
 			     size_t period_sz, unsigned int periods);
 int q6asm_unmap_memory_regions(unsigned int dir, struct audio_client *ac);
+int q6asm_get_hw_pointer(struct audio_client *ac, unsigned int dir);
 #endif /* __Q6_ASM_H__ */
-- 
2.51.0

[PATCH 8/9] ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer

[Srinivas Kandagatla]

make use of q6asm_get_hw_pointer to get the current hardware read/write
pointer and remove any additional code that does calculate this manually.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index db2ea8973ac9..c2a33d6a0827 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -58,7 +58,6 @@ struct q6asm_dai_rtd {
 	phys_addr_t phys;
 	unsigned int pcm_size;
 	unsigned int pcm_count;
-	unsigned int pcm_irq_pos;       /* IRQ position */
 	unsigned int periods;
 	uint64_t bytes_sent;
 	uint64_t bytes_received;
@@ -190,12 +189,10 @@ static void event_handler(uint32_t opcode, uint32_t token,
 		prtd->state = Q6ASM_STREAM_STOPPED;
 		break;
 	case ASM_CLIENT_EVENT_DATA_WRITE_DONE: {
-		prtd->pcm_irq_pos += prtd->pcm_count;
 		snd_pcm_period_elapsed(substream);
 		break;
 		}
 	case ASM_CLIENT_EVENT_DATA_READ_DONE:
-		prtd->pcm_irq_pos += prtd->pcm_count;
 		snd_pcm_period_elapsed(substream);
 		if (prtd->state == Q6ASM_STREAM_RUNNING)
 			q6asm_read(prtd->audio_client, prtd->stream_id);
@@ -227,7 +224,6 @@ static int q6asm_dai_prepare(struct snd_soc_component *component,
 	}
 
 	prtd->pcm_count = snd_pcm_lib_period_bytes(substream);
-	prtd->pcm_irq_pos = 0;
 	/* rate and channels are sent to audio driver */
 	if (prtd->state == Q6ASM_STREAM_RUNNING) {
 		/* clear the previous setup if any  */
@@ -479,11 +475,13 @@ static snd_pcm_uframes_t q6asm_dai_pointer(struct snd_soc_component *component,
 
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct q6asm_dai_rtd *prtd = runtime->private_data;
+	snd_pcm_uframes_t ptr;
 
-	if (prtd->pcm_irq_pos >= prtd->pcm_size)
-		prtd->pcm_irq_pos = 0;
+	ptr = q6asm_get_hw_pointer(prtd->audio_client, substream->stream) * runtime->period_size;
+	if (ptr)
+		return ptr - 1;
 
-	return bytes_to_frames(runtime, (prtd->pcm_irq_pos));
+	return 0;
 }
 
 static int q6asm_dai_hw_params(struct snd_soc_component *component,
-- 
2.51.0

[PATCH 9/9] ASoC: qcom: q6asm: set runtime correctly for each stream

[Srinivas Kandagatla]

Both capture and playback can have different runtimes, so set them
accordingly.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index c2a33d6a0827..97256313c01a 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -433,10 +433,13 @@ static int q6asm_dai_open(struct snd_soc_component *component,
 
 	runtime->private_data = prtd;
 
-	snd_soc_set_runtime_hwparams(substream, &q6asm_dai_hardware_playback);
-
-	runtime->dma_bytes = q6asm_dai_hardware_playback.buffer_bytes_max;
-
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		snd_soc_set_runtime_hwparams(substream, &q6asm_dai_hardware_playback);
+		runtime->dma_bytes = q6asm_dai_hardware_playback.buffer_bytes_max;
+	} else {
+		snd_soc_set_runtime_hwparams(substream, &q6asm_dai_hardware_capture);
+		runtime->dma_bytes = q6asm_dai_hardware_capture.buffer_bytes_max;
+	}
 
 	if (pdata->sid < 0)
 		prtd->phys = substream->dma_buffer.addr;
-- 
2.51.0

Re: [PATCH 0/9] ASoC: qcom: q6dsp: fixes and updates

[Alexey Klimov]

On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
> This patchset has 4 fixes and some enhancements to the Elite DSP driver
> support.
> Fixes includes 
> 	- setting correct flags for expected behaviour of appl_ptr
> 	- fix closing of copp instances
> 	- fix buffer alignment.
> 	- fix state checks before closing asm stream
> Enhancements include:
> 	- adding q6asm_get_hw_pointer and ack callback support
>
> There is another set of updates comming soon, which will add support
> for early memory mapping and few more modules support in audioreach.
>
> Srinivas Kandagatla (9):
>   ASoC: qcom: q6apm-dai: set flags to reflect correct operation of
>     appl_ptr
>   ASoC: qcom: q6adm: the the copp device only during last instance
>   ASoC: qcom: qdsp6: q6asm-dai: set 10 ms period and buffer alignment.
>   ASoC: qcom: q6asm-dai: perform correct state check before closing
>   ASoC: qcom: q6asm: handle the responses after closing
>   ASoC: qcom: q6asm-dai: schedule all available frames to avoid dsp
>     under-runs
>   ASoC: qcom: q6asm: add q6asm_get_hw_pointer
>   ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer
>   ASoC: qcom: q6asm: set runtime correctly for each stream

I am currently testing this series, so far looks good but
I'd like to test it on one more different board.
I'll also have few minor comments later.

Thanks,
Alexey

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Alexey Klimov]

On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
> For record path, multiple read requests are queued to dsp in advance.
> However when data stream is closed, the pending read requests are rejected
> by the dsp and a response is sent to the driver, this case is not handled
> in the driver resulting in errors like below
>
> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>
> This is same for both write and eos.
>
> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
> ASM_DATA_CMD_WRITE_V2 as expected response.
>
> Reported-by: Martino Facchin <m.facchin@arduino.cc>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

I was also reporting this error months ago.
Thanks for fixing this.

FWIW there is also DSP timeout error on Audioreach-based platforms.

Thanks,
Alexey Klimov

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Srinivas Kandagatla]

On 10/20/25 3:35 PM, Alexey Klimov wrote:
> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>> For record path, multiple read requests are queued to dsp in advance.
>> However when data stream is closed, the pending read requests are rejected
>> by the dsp and a response is sent to the driver, this case is not handled
>> in the driver resulting in errors like below
>>
>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>
>> This is same for both write and eos.
>>
>> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
>> ASM_DATA_CMD_WRITE_V2 as expected response.
>>
>> Reported-by: Martino Facchin <m.facchin@arduino.cc>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> 
> I was also reporting this error months ago.
> Thanks for fixing this.
> 
> FWIW there is also DSP timeout error on Audioreach-based platforms.
there should be only one for SPF get state command.

qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode

This is expected as we are checking for DSP readiness and it will
timeout if the dsp is not ready.

--srini



> 
> Thanks,
> Alexey Klimov

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Konrad Dybcio]

On 10/20/25 4:37 PM, Srinivas Kandagatla wrote:
> On 10/20/25 3:35 PM, Alexey Klimov wrote:
>> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>>> For record path, multiple read requests are queued to dsp in advance.
>>> However when data stream is closed, the pending read requests are rejected
>>> by the dsp and a response is sent to the driver, this case is not handled
>>> in the driver resulting in errors like below
>>>
>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>
>>> This is same for both write and eos.
>>>
>>> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
>>> ASM_DATA_CMD_WRITE_V2 as expected response.
>>>
>>> Reported-by: Martino Facchin <m.facchin@arduino.cc>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>
>> I was also reporting this error months ago.
>> Thanks for fixing this.
>>
>> FWIW there is also DSP timeout error on Audioreach-based platforms.
> there should be only one for SPF get state command.
> 
> qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
> 
> This is expected as we are checking for DSP readiness and it will
> timeout if the dsp is not ready.

Can we make the UX better by adding "bool timeout_expected" in the
path that we "want" it to fail?

(I have no clue how this is tied together, this is a high-level idea)

Konrad

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Srinivas Kandagatla]

On 10/20/25 3:39 PM, Konrad Dybcio wrote:
> On 10/20/25 4:37 PM, Srinivas Kandagatla wrote:
>> On 10/20/25 3:35 PM, Alexey Klimov wrote:
>>> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>>>> For record path, multiple read requests are queued to dsp in advance.
>>>> However when data stream is closed, the pending read requests are rejected
>>>> by the dsp and a response is sent to the driver, this case is not handled
>>>> in the driver resulting in errors like below
>>>>
>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>
>>>> This is same for both write and eos.
>>>>
>>>> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
>>>> ASM_DATA_CMD_WRITE_V2 as expected response.
>>>>
>>>> Reported-by: Martino Facchin <m.facchin@arduino.cc>
>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>>
>>> I was also reporting this error months ago.
>>> Thanks for fixing this.
>>>
>>> FWIW there is also DSP timeout error on Audioreach-based platforms.
>> there should be only one for SPF get state command.
>>
>> qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
>>
>> This is expected as we are checking for DSP readiness and it will
>> timeout if the dsp is not ready.
> 
> Can we make the UX better by adding "bool timeout_expected" in the
> path that we "want" it to fail?
its not that we want it to fail, its just that we might fail, if we send
the command too early.


--srini>
> (I have no clue how this is tied together, this is a high-level idea)
> 
> Konrad

Re: [PATCH 7/9] ASoC: qcom: q6asm: add q6asm_get_hw_pointer

[Alexey Klimov]

On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
> Currently we are performing an extra layer of calculation on the hw_ptr,
> which is always prone to errors.
> Move this to common dsp layer for better accuracy.

The subject says that the change adds q6asm_get_hw_ptr but here it says
that calculation of hw_ptr is moved. Where is it moved out of or from?

Currently the commit message is confusing.

It seems to be potential confusing split with commit.
("ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer") where calculation
of hw_ptr was implemented in q6asm-dai.c.

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
>  sound/soc/qcom/qdsp6/q6asm.c | 12 ++++++++++++
>  sound/soc/qcom/qdsp6/q6asm.h |  1 +
>  2 files changed, 13 insertions(+)
>
> diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
> index 371389c8fa7a..643ca944b1b5 100644
> --- a/sound/soc/qcom/qdsp6/q6asm.c
> +++ b/sound/soc/qcom/qdsp6/q6asm.c
> @@ -6,6 +6,7 @@
>  #include <linux/mutex.h>
>  #include <linux/wait.h>
>  #include <linux/module.h>
> +#include <linux/atomic.h>

Ideally this should be sorted but it seems it was not initially.

>  #include <linux/soc/qcom/apr.h>
>  #include <linux/device.h>
>  #include <linux/of_platform.h>
> @@ -248,6 +249,7 @@ struct audio_port_data {
>  	uint32_t num_periods;
>  	uint32_t dsp_buf;
>  	uint32_t mem_map_handle;
> +	atomic_t hw_ptr;
>  };

Thanks,
Alexey

Re: [PATCH 2/9] ASoC: qcom: q6adm: the the copp device only during last instance

[Alexey Klimov]

On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
> A matching Common object post processing instance is normally resused
> across multiple streams. However currently we close this on DSP
> eventhough there is a refcount on this copp object, this can result in

even though?

> below error.

[..]

Best regards,
Alexey

Re: [PATCH 6/9] ASoC: qcom: q6asm-dai: schedule all available frames to avoid dsp under-runs

[Alexey Klimov]

On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
> With the existing code, we are only setting up one period at a time, in a
> ping-pong buffer style. This triggers lot of underruns in the dsp
> leading to jitter noise during audio playback.
>
> Fix this by scheduling all available periods, this will ensure that the dsp
> has enough buffer feed and ultimatley fixing the underruns and audio distortion.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
>  sound/soc/qcom/qdsp6/q6asm-dai.c | 34 +++++++++++++++++++++++++-------

[..]

> +static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct q6asm_dai_rtd *prtd = runtime->private_data;
> +	int i, ret = 0, avail_periods;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
> +		avail_periods = (runtime->control->appl_ptr - prtd->queue_ptr)/runtime->period_size;
> +		for (i = 0; i < avail_periods; i++) {
> +			ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
> +					   prtd->pcm_count, 0, 0, 0);
> +
> +			if (ret < 0) {
> +				dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
> +				return ret;
> +			}
> +			prtd->queue_ptr += runtime->period_size;
> +		}
> +	}
> +
> +	return ret;
> +}

So when I compiled this on a common arm64 desktop machine with defconfig,
nothing fancy, with gcc, this loop and function really does compile with
udiv instruction.

avail_periods = (runtime->control->appl_ptr - prtd->queue_ptr)/runtime->period_size;
     350:       f9001bf7        str     x23, [sp, #48]
     354:       f94086a0        ldr     x0, [x21, #264]
     358:       f9408262        ldr     x2, [x19, #256]
     35c:       f9400000        ldr     x0, [x0]
     360:       f9403ea1        ldr     x1, [x21, #120]
     364:       cb020000        sub     x0, x0, x2
     368:       9ac10800        udiv    x0, x0, x1  <--- here
     36c:       2a0003f7        mov     w23, w0

What do you think about rewriting this loop without division and
without using additional iterator? I don't think this is a hot path
but anyway.

The first iteration that I came up with is (1):


diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index 97256313c01a..d01f805947b8 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -310,6 +310,23 @@ static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_sub
 	int ret = 0;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
+		if (prtd->queue_ptr < runtime->control->appl_ptr) {
+
+			do {
+				ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
+							prtd->pcm_count, 0, 0, 0);
+
+				if (ret < 0) {
+						dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
+						return ret;
+				}
+
+				prtd->queue_ptr += runtime->period_size;
+
+			} while (prtd->queue_ptr < runtime->control->appl_ptr);
+
+		}

No division and no calculation of iterator and avail_periods.

Rewriting it further (2):

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index 97256313c01a..317f06d07a09 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -307,9 +307,26 @@ static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_sub
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct q6asm_dai_rtd *prtd = runtime->private_data;
-	int i, ret = 0, avail_periods;
+	int ret = 0, diff;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
+		diff = (runtime->control->appl_ptr - prtd->queue_ptr);
+		
+		while (diff >= runtime->period_size) {
+			ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
+						prtd->pcm_count, 0, 0, 0);
+			
+			if (ret < 0) {
+				dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
+				return ret;
+			}
+
+			prtd->queue_ptr += runtime->period_size;
+			diff -= runtime->period_size;
+		};
+
+

Surprisingly, in (1) the size of resulting object file is smaller than in (2):

With original patch:	110008 Oct 20 15:26 sound/soc/qcom/qdsp6/q6asm-dai.o
with (1): 		109776 Oct 20 16:53 sound/soc/qcom/qdsp6/q6asm-dai.o
with (2):		109896 Oct 20 16:52 sound/soc/qcom/qdsp6/q6asm-dai.o

I think the readability won't be damaged as a result of the rewriting
and the code seems to be smaller.
Obviusly, this needs to be verified for some corner cases and etc.
I'd go with (1) but it is up to you. I tested (1) and it seems to work.

Best regards,
Alexey

Re: [PATCH 0/9] ASoC: qcom: q6dsp: fixes and updates

[Alexey Klimov]

On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
> This patchset has 4 fixes and some enhancements to the Elite DSP driver
> support.
> Fixes includes 
> 	- setting correct flags for expected behaviour of appl_ptr
> 	- fix closing of copp instances
> 	- fix buffer alignment.
> 	- fix state checks before closing asm stream
> Enhancements include:
> 	- adding q6asm_get_hw_pointer and ack callback support
>
> There is another set of updates comming soon, which will add support
> for early memory mapping and few more modules support in audioreach.
>
> Srinivas Kandagatla (9):
>   ASoC: qcom: q6apm-dai: set flags to reflect correct operation of
>     appl_ptr
>   ASoC: qcom: q6adm: the the copp device only during last instance
>   ASoC: qcom: qdsp6: q6asm-dai: set 10 ms period and buffer alignment.
>   ASoC: qcom: q6asm-dai: perform correct state check before closing
>   ASoC: qcom: q6asm: handle the responses after closing
>   ASoC: qcom: q6asm-dai: schedule all available frames to avoid dsp
>     under-runs
>   ASoC: qcom: q6asm: add q6asm_get_hw_pointer
>   ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer
>   ASoC: qcom: q6asm: set runtime correctly for each stream

I tested this series on RB5 and RB3 boards and it seems to work.

It becomes a real pain to verify things on RB3. There is a regression
related to slimbus regmap and linux-next is broken on RB3 starting
with next-20251017.
The tag next-20251016 works just fine (after applying slimbus regmap
fix).

For the whole series:

Tested-by: Alexey Klimov <alexey.klimov@linaro.org> # RB5, RB3

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Konrad Dybcio]

On 10/20/25 4:42 PM, Srinivas Kandagatla wrote:
> On 10/20/25 3:39 PM, Konrad Dybcio wrote:
>> On 10/20/25 4:37 PM, Srinivas Kandagatla wrote:
>>> On 10/20/25 3:35 PM, Alexey Klimov wrote:
>>>> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>>>>> For record path, multiple read requests are queued to dsp in advance.
>>>>> However when data stream is closed, the pending read requests are rejected
>>>>> by the dsp and a response is sent to the driver, this case is not handled
>>>>> in the driver resulting in errors like below
>>>>>
>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>
>>>>> This is same for both write and eos.
>>>>>
>>>>> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
>>>>> ASM_DATA_CMD_WRITE_V2 as expected response.
>>>>>
>>>>> Reported-by: Martino Facchin <m.facchin@arduino.cc>
>>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>>>
>>>> I was also reporting this error months ago.
>>>> Thanks for fixing this.
>>>>
>>>> FWIW there is also DSP timeout error on Audioreach-based platforms.
>>> there should be only one for SPF get state command.
>>>
>>> qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
>>>
>>> This is expected as we are checking for DSP readiness and it will
>>> timeout if the dsp is not ready.
>>
>> Can we make the UX better by adding "bool timeout_expected" in the
>> path that we "want" it to fail?
> its not that we want it to fail, its just that we might fail, if we send
> the command too early.

I came up with this to potentially work around it, but it seems a little
heavy-handed to avoid a single line in dmesg..


diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
index 2365424a9b42..a39e717cb4e0 100644
--- a/sound/soc/qcom/qdsp6/audioreach.c
+++ b/sound/soc/qcom/qdsp6/audioreach.c
@@ -552,7 +552,7 @@ EXPORT_SYMBOL_GPL(audioreach_alloc_graph_pkt);
 int audioreach_send_cmd_sync(struct device *dev, gpr_device_t *gdev,
                             struct gpr_ibasic_rsp_result_t *result, struct mutex *cmd_lock,
                             gpr_port_t *port, wait_queue_head_t *cmd_wait,
-                            struct gpr_pkt *pkt, uint32_t rsp_opcode)
+                            struct gpr_pkt *pkt, uint32_t rsp_opcode, bool silent_timeout)
 {
 
        struct gpr_hdr *hdr = &pkt->hdr;
@@ -579,7 +579,10 @@ int audioreach_send_cmd_sync(struct device *dev, gpr_device_t *gdev,
                rc = wait_event_timeout(*cmd_wait, (result->opcode == hdr->opcode), 5 * HZ);
 
        if (!rc) {
-               dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
+               if (silent_timeout)
+                       dev_dbg(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
+               else
+                       dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
                rc = -ETIMEDOUT;
        } else if (result->status > 0) {
                dev_err(dev, "DSP returned error[%x] %x\n", hdr->opcode, result->status);
@@ -600,7 +603,7 @@ int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph, struct gpr_pkt *pk
 {
 
        return audioreach_send_cmd_sync(graph->dev, NULL,  &graph->result, &graph->lock,
-                                       graph->port, &graph->cmd_wait, pkt, rsp_opcode);
+                                       graph->port, &graph->cmd_wait, pkt, rsp_opcode, false);
 }
 EXPORT_SYMBOL_GPL(audioreach_graph_send_cmd_sync);
 
diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h
index d1b60b36468a..55ce1e367cda 100644
--- a/sound/soc/qcom/qdsp6/audioreach.h
+++ b/sound/soc/qcom/qdsp6/audioreach.h
@@ -805,7 +805,7 @@ int audioreach_map_memory_regions(struct q6apm_graph *graph,
                                  bool is_contiguous);
 int audioreach_send_cmd_sync(struct device *dev, gpr_device_t *gdev, struct gpr_ibasic_rsp_result_t *result,
                             struct mutex *cmd_lock, gpr_port_t *port, wait_queue_head_t *cmd_wait,
-                            struct gpr_pkt *pkt, uint32_t rsp_opcode);
+                            struct gpr_pkt *pkt, uint32_t rsp_opcode, bool silent_timeout);
 int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph, struct gpr_pkt *pkt,
                                   uint32_t rsp_opcode);
 int audioreach_set_media_format(struct q6apm_graph *graph,
diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
index 0e667a7eb546..f96c0ae20eb2 100644
--- a/sound/soc/qcom/qdsp6/q6apm.c
+++ b/sound/soc/qcom/qdsp6/q6apm.c
@@ -34,7 +34,16 @@ int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt, uint32_t rsp_opc
        gpr_device_t *gdev = apm->gdev;
 
        return audioreach_send_cmd_sync(&gdev->dev, gdev, &apm->result, &apm->lock,
-                                       NULL, &apm->wait, pkt, rsp_opcode);
+                                       NULL, &apm->wait, pkt, rsp_opcode, false);
+}
+
+static int q6apm_send_cmd_sync_may_timeout(struct q6apm *apm, struct gpr_pkt *pkt,
+                                          uint32_t rsp_opcode)
+{
+       gpr_device_t *gdev = apm->gdev;
+
+       return audioreach_send_cmd_sync(&gdev->dev, gdev, &apm->result, &apm->lock,
+                                       NULL, &apm->wait, pkt, rsp_opcode, true);
 }
 
 static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm, uint32_t graph_id)
@@ -154,7 +163,7 @@ static int q6apm_get_apm_state(struct q6apm *apm)
        if (IS_ERR(pkt))
                return PTR_ERR(pkt);
 
-       q6apm_send_cmd_sync(apm, pkt, APM_CMD_RSP_GET_SPF_STATE);
+       q6apm_send_cmd_sync_may_timeout(apm, pkt, APM_CMD_RSP_GET_SPF_STATE);
 
        kfree(pkt);
 
diff --git a/sound/soc/qcom/qdsp6/q6prm.c b/sound/soc/qcom/qdsp6/q6prm.c
index 81554d202658..7b71d6dfc993 100644
--- a/sound/soc/qcom/qdsp6/q6prm.c
+++ b/sound/soc/qcom/qdsp6/q6prm.c
@@ -52,7 +52,7 @@ struct prm_cmd_release_rsc {
 static int q6prm_send_cmd_sync(struct q6prm *prm, struct gpr_pkt *pkt, uint32_t rsp_opcode)
 {
        return audioreach_send_cmd_sync(prm->dev, prm->gdev, &prm->result, &prm->lock,
-                                       NULL, &prm->wait, pkt, rsp_opcode);
+                                       NULL, &prm->wait, pkt, rsp_opcode, false);
 }
 
 static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id, bool enable)

Konrad

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Srinivas Kandagatla]

On 10/21/25 10:12 AM, Konrad Dybcio wrote:
> On 10/20/25 4:42 PM, Srinivas Kandagatla wrote:
>> On 10/20/25 3:39 PM, Konrad Dybcio wrote:
>>> On 10/20/25 4:37 PM, Srinivas Kandagatla wrote:
>>>> On 10/20/25 3:35 PM, Alexey Klimov wrote:
>>>>> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>>>>>> For record path, multiple read requests are queued to dsp in advance.
>>>>>> However when data stream is closed, the pending read requests are rejected
>>>>>> by the dsp and a response is sent to the driver, this case is not handled
>>>>>> in the driver resulting in errors like below
>>>>>>
>>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>>
>>>>>> This is same for both write and eos.
>>>>>>
>>>>>> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
>>>>>> ASM_DATA_CMD_WRITE_V2 as expected response.
>>>>>>
>>>>>> Reported-by: Martino Facchin <m.facchin@arduino.cc>
>>>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>>>>
>>>>> I was also reporting this error months ago.
>>>>> Thanks for fixing this.
>>>>>
>>>>> FWIW there is also DSP timeout error on Audioreach-based platforms.
>>>> there should be only one for SPF get state command.
>>>>
>>>> qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
>>>>
>>>> This is expected as we are checking for DSP readiness and it will
>>>> timeout if the dsp is not ready.
>>>
>>> Can we make the UX better by adding "bool timeout_expected" in the
>>> path that we "want" it to fail?
>> its not that we want it to fail, its just that we might fail, if we send
>> the command too early.
> 
> I came up with this to potentially work around it, but it seems a little
> heavy-handed to avoid a single line in dmesg..
> 
Yes it is. simple thing to do is wait for few ms before requesting for
SPF ready status, that should do that trick.

--srini

> 
> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
> index 2365424a9b42..a39e717cb4e0 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.c
> +++ b/sound/soc/qcom/qdsp6/audioreach.c
> @@ -552,7 +552,7 @@ EXPORT_SYMBOL_GPL(audioreach_alloc_graph_pkt);
>  int audioreach_send_cmd_sync(struct device *dev, gpr_device_t *gdev,
>                              struct gpr_ibasic_rsp_result_t *result, struct mutex *cmd_lock,
>                              gpr_port_t *port, wait_queue_head_t *cmd_wait,
> -                            struct gpr_pkt *pkt, uint32_t rsp_opcode)
> +                            struct gpr_pkt *pkt, uint32_t rsp_opcode, bool silent_timeout)
>  {
>  
>         struct gpr_hdr *hdr = &pkt->hdr;
> @@ -579,7 +579,10 @@ int audioreach_send_cmd_sync(struct device *dev, gpr_device_t *gdev,
>                 rc = wait_event_timeout(*cmd_wait, (result->opcode == hdr->opcode), 5 * HZ);
>  
>         if (!rc) {
> -               dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
> +               if (silent_timeout)
> +                       dev_dbg(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
> +               else
> +                       dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
>                 rc = -ETIMEDOUT;
>         } else if (result->status > 0) {
>                 dev_err(dev, "DSP returned error[%x] %x\n", hdr->opcode, result->status);
> @@ -600,7 +603,7 @@ int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph, struct gpr_pkt *pk
>  {
>  
>         return audioreach_send_cmd_sync(graph->dev, NULL,  &graph->result, &graph->lock,
> -                                       graph->port, &graph->cmd_wait, pkt, rsp_opcode);
> +                                       graph->port, &graph->cmd_wait, pkt, rsp_opcode, false);
>  }
>  EXPORT_SYMBOL_GPL(audioreach_graph_send_cmd_sync);
>  
> diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h
> index d1b60b36468a..55ce1e367cda 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.h
> +++ b/sound/soc/qcom/qdsp6/audioreach.h
> @@ -805,7 +805,7 @@ int audioreach_map_memory_regions(struct q6apm_graph *graph,
>                                   bool is_contiguous);
>  int audioreach_send_cmd_sync(struct device *dev, gpr_device_t *gdev, struct gpr_ibasic_rsp_result_t *result,
>                              struct mutex *cmd_lock, gpr_port_t *port, wait_queue_head_t *cmd_wait,
> -                            struct gpr_pkt *pkt, uint32_t rsp_opcode);
> +                            struct gpr_pkt *pkt, uint32_t rsp_opcode, bool silent_timeout);
>  int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph, struct gpr_pkt *pkt,
>                                    uint32_t rsp_opcode);
>  int audioreach_set_media_format(struct q6apm_graph *graph,
> diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
> index 0e667a7eb546..f96c0ae20eb2 100644
> --- a/sound/soc/qcom/qdsp6/q6apm.c
> +++ b/sound/soc/qcom/qdsp6/q6apm.c
> @@ -34,7 +34,16 @@ int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt, uint32_t rsp_opc
>         gpr_device_t *gdev = apm->gdev;
>  
>         return audioreach_send_cmd_sync(&gdev->dev, gdev, &apm->result, &apm->lock,
> -                                       NULL, &apm->wait, pkt, rsp_opcode);
> +                                       NULL, &apm->wait, pkt, rsp_opcode, false);
> +}
> +
> +static int q6apm_send_cmd_sync_may_timeout(struct q6apm *apm, struct gpr_pkt *pkt,
> +                                          uint32_t rsp_opcode)
> +{
> +       gpr_device_t *gdev = apm->gdev;
> +
> +       return audioreach_send_cmd_sync(&gdev->dev, gdev, &apm->result, &apm->lock,
> +                                       NULL, &apm->wait, pkt, rsp_opcode, true);
>  }
>  
>  static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm, uint32_t graph_id)
> @@ -154,7 +163,7 @@ static int q6apm_get_apm_state(struct q6apm *apm)
>         if (IS_ERR(pkt))
>                 return PTR_ERR(pkt);
>  
> -       q6apm_send_cmd_sync(apm, pkt, APM_CMD_RSP_GET_SPF_STATE);
> +       q6apm_send_cmd_sync_may_timeout(apm, pkt, APM_CMD_RSP_GET_SPF_STATE);
>  
>         kfree(pkt);
>  
> diff --git a/sound/soc/qcom/qdsp6/q6prm.c b/sound/soc/qcom/qdsp6/q6prm.c
> index 81554d202658..7b71d6dfc993 100644
> --- a/sound/soc/qcom/qdsp6/q6prm.c
> +++ b/sound/soc/qcom/qdsp6/q6prm.c
> @@ -52,7 +52,7 @@ struct prm_cmd_release_rsc {
>  static int q6prm_send_cmd_sync(struct q6prm *prm, struct gpr_pkt *pkt, uint32_t rsp_opcode)
>  {
>         return audioreach_send_cmd_sync(prm->dev, prm->gdev, &prm->result, &prm->lock,
> -                                       NULL, &prm->wait, pkt, rsp_opcode);
> +                                       NULL, &prm->wait, pkt, rsp_opcode, false);
>  }
>  
>  static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id, bool enable)
> 
> Konrad
-

Re: [PATCH 5/9] ASoC: qcom: q6asm: handle the responses after closing

[Konrad Dybcio]

On 10/21/25 11:21 AM, Srinivas Kandagatla wrote:
> 
> 
> On 10/21/25 10:12 AM, Konrad Dybcio wrote:
>> On 10/20/25 4:42 PM, Srinivas Kandagatla wrote:
>>> On 10/20/25 3:39 PM, Konrad Dybcio wrote:
>>>> On 10/20/25 4:37 PM, Srinivas Kandagatla wrote:
>>>>> On 10/20/25 3:35 PM, Alexey Klimov wrote:
>>>>>> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>>>>>>> For record path, multiple read requests are queued to dsp in advance.
>>>>>>> However when data stream is closed, the pending read requests are rejected
>>>>>>> by the dsp and a response is sent to the driver, this case is not handled
>>>>>>> in the driver resulting in errors like below
>>>>>>>
>>>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>>> q6asm-dai ab00000.remoteproc:glink-edge:apr:service@7:dais: command[0x10dac] not expecting rsp
>>>>>>>
>>>>>>> This is same for both write and eos.
>>>>>>>
>>>>>>> Fix this by allowing ASM_DATA_CMD_READ_V2, ASM_DATA_CMD_EOS and
>>>>>>> ASM_DATA_CMD_WRITE_V2 as expected response.
>>>>>>>
>>>>>>> Reported-by: Martino Facchin <m.facchin@arduino.cc>
>>>>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>>>>>
>>>>>> I was also reporting this error months ago.
>>>>>> Thanks for fixing this.
>>>>>>
>>>>>> FWIW there is also DSP timeout error on Audioreach-based platforms.
>>>>> there should be only one for SPF get state command.
>>>>>
>>>>> qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
>>>>>
>>>>> This is expected as we are checking for DSP readiness and it will
>>>>> timeout if the dsp is not ready.
>>>>
>>>> Can we make the UX better by adding "bool timeout_expected" in the
>>>> path that we "want" it to fail?
>>> its not that we want it to fail, its just that we might fail, if we send
>>> the command too early.
>>
>> I came up with this to potentially work around it, but it seems a little
>> heavy-handed to avoid a single line in dmesg..
>>
> Yes it is. simple thing to do is wait for few ms before requesting for
> SPF ready status, that should do that trick.

Sounds good!

Konrad

Re: [PATCH 6/9] ASoC: qcom: q6asm-dai: schedule all available frames to avoid dsp under-runs

[Srinivas Kandagatla]

On 10/20/25 5:06 PM, Alexey Klimov wrote:
> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>> With the existing code, we are only setting up one period at a time, in a
>> ping-pong buffer style. This triggers lot of underruns in the dsp
>> leading to jitter noise during audio playback.
>>
>> Fix this by scheduling all available periods, this will ensure that the dsp
>> has enough buffer feed and ultimatley fixing the underruns and audio distortion.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> ---
>>  sound/soc/qcom/qdsp6/q6asm-dai.c | 34 +++++++++++++++++++++++++-------
> 
> [..]
> 
>> +static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_substream *substream)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct q6asm_dai_rtd *prtd = runtime->private_data;
>> +	int i, ret = 0, avail_periods;
>> +
>> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
>> +		avail_periods = (runtime->control->appl_ptr - prtd->queue_ptr)/runtime->period_size;
>> +		for (i = 0; i < avail_periods; i++) {
>> +			ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
>> +					   prtd->pcm_count, 0, 0, 0);
>> +
>> +			if (ret < 0) {
>> +				dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
>> +				return ret;
>> +			}
>> +			prtd->queue_ptr += runtime->period_size;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}

We have exactly same logic in q6apm-dai.c I will keep this as it is for
this patch series, we can always improve this over time. Patches welcome.>
> So when I compiled this on a common arm64 desktop machine with defconfig,
> nothing fancy, with gcc, this loop and function really does compile with
> udiv instruction.
> 
> avail_periods = (runtime->control->appl_ptr - prtd->queue_ptr)/runtime->period_size;
>      350:       f9001bf7        str     x23, [sp, #48]
>      354:       f94086a0        ldr     x0, [x21, #264]
>      358:       f9408262        ldr     x2, [x19, #256]
>      35c:       f9400000        ldr     x0, [x0]
>      360:       f9403ea1        ldr     x1, [x21, #120]
>      364:       cb020000        sub     x0, x0, x2
>      368:       9ac10800        udiv    x0, x0, x1  <--- here
>      36c:       2a0003f7        mov     w23, w0
> 
> What do you think about rewriting this loop without division and
> without using additional iterator? I don't think this is a hot path
> but anyway.
> 
> The first iteration that I came up with is (1):
> 
> 
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index 97256313c01a..d01f805947b8 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -310,6 +310,23 @@ static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_sub
>  	int ret = 0;
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
> +		if (prtd->queue_ptr < runtime->control->appl_ptr) {
This is not going to work, as we need alteast 1 period of data.> +
> +			do {
> +				ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
> +							prtd->pcm_count, 0, 0, 0);
> +
> +				if (ret < 0) {
> +						dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
> +						return ret;
> +				}
> +
> +				prtd->queue_ptr += runtime->period_size;
This queue_ptr can go over appl_ptr.. can impact sound quality as we
will be queuing something that is not in the data yet.

--srini> +
> +			} while (prtd->queue_ptr < runtime->control->appl_ptr);
> +
> +		}
> 
> No division and no calculation of iterator and avail_periods.
> 
> Rewriting it further (2):
> 
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index 97256313c01a..317f06d07a09 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -307,9 +307,26 @@ static int q6asm_dai_ack(struct snd_soc_component *component, struct snd_pcm_sub
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct q6asm_dai_rtd *prtd = runtime->private_data;
> -	int i, ret = 0, avail_periods;
> +	int ret = 0, diff;
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && prtd->state == Q6ASM_STREAM_RUNNING) {
> +		diff = (runtime->control->appl_ptr - prtd->queue_ptr);
> +		
> +		while (diff >= runtime->period_size) {
> +			ret = q6asm_write_async(prtd->audio_client, prtd->stream_id,
> +						prtd->pcm_count, 0, 0, 0);
> +			
> +			if (ret < 0) {
> +				dev_err(component->dev, "Error queuing playback buffer %d\n", ret);
> +				return ret;
> +			}
> +
> +			prtd->queue_ptr += runtime->period_size;
> +			diff -= runtime->period_size;
> +		};
> +
> +
> 
> Surprisingly, in (1) the size of resulting object file is smaller than in (2):
> 
> With original patch:	110008 Oct 20 15:26 sound/soc/qcom/qdsp6/q6asm-dai.o
> with (1): 		109776 Oct 20 16:53 sound/soc/qcom/qdsp6/q6asm-dai.o
> with (2):		109896 Oct 20 16:52 sound/soc/qcom/qdsp6/q6asm-dai.o
> 
> I think the readability won't be damaged as a result of the rewriting
> and the code seems to be smaller.
> Obviusly, this needs to be verified for some corner cases and etc.
> I'd go with (1) but it is up to you. I tested (1) and it seems to work.
> 
> Best regards,
> Alexey
> 
> 
> 
> 
> 

Re: [PATCH 7/9] ASoC: qcom: q6asm: add q6asm_get_hw_pointer

[Srinivas Kandagatla]

On 10/20/25 4:04 PM, Alexey Klimov wrote:
> On Wed Oct 15, 2025 at 2:17 PM BST, Srinivas Kandagatla wrote:
>> Currently we are performing an extra layer of calculation on the hw_ptr,
>> which is always prone to errors.
>> Move this to common dsp layer for better accuracy.
> 
> The subject says that the change adds q6asm_get_hw_ptr but here it says
> that calculation of hw_ptr is moved. Where is it moved out of or from?
> 
> Currently the commit message is confusing.

Sure, Will rephrase this.

Currently q6asm-dai.c implement tracking the dsp hardware pointer based
on callbacks from q6asm, this is really an overhead, prone to errors and
redundant.
We already have buffers and tokens which can be used to get hw_ptr
location, use this instead.

--srini>
> It seems to be potential confusing split with commit.
> ("ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer") where calculation
> of hw_ptr was implemented in q6asm-dai.c.
> 
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> ---
>>  sound/soc/qcom/qdsp6/q6asm.c | 12 ++++++++++++
>>  sound/soc/qcom/qdsp6/q6asm.h |  1 +
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
>> index 371389c8fa7a..643ca944b1b5 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm.c
>> +++ b/sound/soc/qcom/qdsp6/q6asm.c
>> @@ -6,6 +6,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/wait.h>
>>  #include <linux/module.h>
>> +#include <linux/atomic.h>
> 
> Ideally this should be sorted but it seems it was not initially.
> 
>>  #include <linux/soc/qcom/apr.h>
>>  #include <linux/device.h>
>>  #include <linux/of_platform.h>
>> @@ -248,6 +249,7 @@ struct audio_port_data {
>>  	uint32_t num_periods;
>>  	uint32_t dsp_buf;
>>  	uint32_t mem_map_handle;
>> +	atomic_t hw_ptr;
>>  };
> 
> Thanks,
> Alexey