[PATCH 0/6] ASoC: davinci-pcm: some fixes and convert to BATCH

[PATCH 0/6] ASoC: davinci-pcm: some fixes and convert to BATCH

[Ben Gardiner]

This patchset is a collection of changes for the davinci-pcm driver that were
accumulated during previous forays into the davinci-mcasp and davinci-pcm
drivers [1].

The first three patches are minor changes that cleanup the ping-pong DMA params
setup, expand the davinci-pcm reported format and also its maximum channels.

The fourth patch in the series corrects an audible glitch that can be heard on
da850evm's McASP when ping-pong buffers are enabled. The glitch occurs only
on the second and susequent playbacks, not the first. Reversing the order in
which the dma channels are started and moving the start to the trigger function
from the prepare function fixes the glitch.

The fifth and sixth patches in the series convert the davinci-pcm driver to
BATCH mode. 

I noticed that the davinci-pcm pointer function is returning a value that is 
retrieved from within the running EDMA channel's state -- which is contrary to
the warning in the comments of arch/arm/mach-davinci/dma.c:edma_get_position():

"
 * Returns current source and destination addresses for a particular
 * parameter RAM slot.  Its channel should not be active when this is called.
"

To that end, we begin with some refactoring to centralize the modification of
the davinci_runtime_data.period member.

Followed by wholesale replacement of davinci_pcm_pointer() function so that it
returns the position as reported by the last update of the period member --
along with corresponding changes to davinci_pcm_enqueue_dma()
davinci_pcm_dma_irq() davinci_pcm_trigger() and reporting SNDRV_PCM_INFO_BATCH
on playback and capture.

The series has been tested with and without ping-pong buffers enabled [1];
furthermore, the pointer behaviour was inspected using both
XRUN_DEBUG_PERIODUPDATE and XRUN_DEBUG_HWPTRUPDATE as well as the audio checked
with headphones.

Playback was tested with:

echo 24 > /proc/asound/EVM/pcm0p/xrun_debug
aplay -d 1 -t wav -D "default:CARD=EVM" test.wav
echo 0 > /proc/asound/EVM/pcm0p/xrun_debug

and capture was tested with (while playing test.wav on my PC into the capture
input on the da850evm): 

#!/bin/bash
echo 24 > /proc/asound/EVM/pcm0c/xrun_debug
arecord -d 2 -t wav -D "default:CARD=EVM" -f dat - |aplay -t wav
echo 0 > /proc/asound/EVM/pcm0c/xrun_debug

Playback behaviour before (similar both with and without ping-pong buffers)
=========================
hwptr_update: pcmC0D0p:0: pos=42/2048/32768, hwptr=42/0/42/0
hwptr_update: pcmC0D0p:0: pos=369/2048/32768, hwptr=327/42/369/0
hwptr_update: pcmC0D0p:0: pos=709/2048/32768, hwptr=340/369/709/0
hwptr_update: pcmC0D0p:0: pos=994/2048/32768, hwptr=285/709/994/0
hwptr_update: pcmC0D0p:0: pos=1367/2048/32768, hwptr=373/994/1367/0
hwptr_update: pcmC0D0p:0: pos=1729/2048/32768, hwptr=362/1367/1729/0
hwptr_update: pcmC0D0p:0: pos=2037/2048/32768, hwptr=308/1729/2037/0
period_update: pcmC0D0p:0: pos=2335/2048/32768, hwptr=298/2037/2335/0
hwptr_update: pcmC0D0p:0: pos=2762/2048/32768, hwptr=427/2335/2762/0
hwptr_update: pcmC0D0p:0: pos=3147/2048/32768, hwptr=385/2762/3147/0
hwptr_update: pcmC0D0p:0: pos=3456/2048/32768, hwptr=309/3147/3456/0
hwptr_update: pcmC0D0p:0: pos=3851/2048/32768, hwptr=395/3456/3851/0
period_update: pcmC0D0p:0: pos=4150/2048/32768, hwptr=299/3851/4150/0
[...]

Playback behaviour after (identical both with and without ping-pong buffers)
========================
hwptr_update: pcmC0D0p:0: pos=0/2048/32768, hwptr=0/0/0/0
period_update: pcmC0D0p:0: pos=2048/2048/32768, hwptr=2048/0/2048/0
hwptr_update: pcmC0D0p:0: pos=2048/2048/32768, hwptr=0/2048/2048/0
period_update: pcmC0D0p:0: pos=4096/2048/32768, hwptr=2048/2048/4096/0
[...]

[Capture behaviour was very similarly modified by this patchset]

The playback behaviour before shows inconsistent increments of the pointers
in syscall and interrupt context -- this is due to the pointer function
peeking into the parameters of the dma channel.

The playback behaviour afterwards shows regular increments from interrupt
context. Which is to be expected since the driver now reports an incremented
pointer after each dma completion. 

It seems that the conversion to BATCH mode is beneficial -- other than
obeying the warning comment in dma.c -- since there appear to be fewer
syscalls per execution of aplay.

[1] http://article.gmane.org/gmane.linux.alsa.devel/85122

Ben Gardiner (6):
  ASoC: davinci-pcm: trivial: make ping-pong params setup symmetrical
  ASoC: davinci-pcm: expand the .formats
  ASoC: davinci-pcm: increase the maximum channels
  ASoC: davinci-pcm: fix audible glitch on 2nd ping-pong playback
  ASoC: davinci-pcm: extract period elapsed functions
  ASoC: davinci-pcm: convert to BATCH mode

 sound/soc/davinci/davinci-pcm.c |  127 ++++++++++++++++++++-------------------
 1 files changed, 65 insertions(+), 62 deletions(-)

-- 
1.7.4.1

[PATCH 1/6] ASoC: davinci-pcm: trivial: make ping-pong params setup symmetrical

[Ben Gardiner]

The setup of the pong channel uses EDMA_CHAN_SLOT instead of & 0x3f as the
setup of the ping channel does.

Make the setup of ping and pong symmetric. There is no functional change
introduced by this patch.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>

---
 sound/soc/davinci/davinci-pcm.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 9d35b8c..0d04e0c 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -425,7 +425,8 @@ static int request_ping_pong(struct snd_pcm_substream *substream,
 
 	edma_read_slot(link, &prtd->asp_params);
 	prtd->asp_params.opt &= ~(TCCMODE | EDMA_TCC(0x3f) | TCINTEN);
-	prtd->asp_params.opt |= TCCHEN | EDMA_TCC(prtd->ram_channel & 0x3f);
+	prtd->asp_params.opt |= TCCHEN |
+		EDMA_TCC(prtd->ram_channel & 0x3f);
 	edma_write_slot(link, &prtd->asp_params);
 
 	/* pong */
@@ -439,7 +440,7 @@ static int request_ping_pong(struct snd_pcm_substream *substream,
 	prtd->asp_params.opt &= ~(TCCMODE | EDMA_TCC(0x3f));
 	/* interrupt after every pong completion */
 	prtd->asp_params.opt |= TCINTEN | TCCHEN |
-		EDMA_TCC(EDMA_CHAN_SLOT(prtd->ram_channel));
+		EDMA_TCC(prtd->ram_channel & 0x3f);
 	edma_write_slot(link, &prtd->asp_params);
 
 	/* ram */
-- 
1.7.4.1

[PATCH 2/6] ASoC: davinci-pcm: expand the .formats

[Ben Gardiner]

Based on the data_type test in ping_pong_dma_setup, davinci-pcm is capable of
handling data of width up to and including 32bits.

"
	if ((data_type == 0) || (data_type > 4)) {
		printk(KERN_ERR "%s: data_type=%i\n", __func__, data_type);
		return -EINVAL;
	}
"

Update the .format member of the snd_pcm_hardware instances it registers to
reflect this capability.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>

---
 sound/soc/davinci/davinci-pcm.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 0d04e0c..1e47267 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -46,11 +46,27 @@ static void print_buf_info(int slot, char *name)
 }
 #endif
 
+#define DAVINCI_PCM_FMTBITS	(\
+				SNDRV_PCM_FMTBIT_S8	|\
+				SNDRV_PCM_FMTBIT_U8	|\
+				SNDRV_PCM_FMTBIT_S16_LE	|\
+				SNDRV_PCM_FMTBIT_S16_BE	|\
+				SNDRV_PCM_FMTBIT_U16_LE	|\
+				SNDRV_PCM_FMTBIT_U16_BE	|\
+				SNDRV_PCM_FMTBIT_S24_LE	|\
+				SNDRV_PCM_FMTBIT_S24_BE	|\
+				SNDRV_PCM_FMTBIT_U24_LE	|\
+				SNDRV_PCM_FMTBIT_U24_BE	|\
+				SNDRV_PCM_FMTBIT_S32_LE	|\
+				SNDRV_PCM_FMTBIT_S32_BE	|\
+				SNDRV_PCM_FMTBIT_U32_LE	|\
+				SNDRV_PCM_FMTBIT_U32_BE)
+
 static struct snd_pcm_hardware pcm_hardware_playback = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
 		 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
-	.formats = (SNDRV_PCM_FMTBIT_S16_LE),
+	.formats = DAVINCI_PCM_FMTBITS,
 	.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
 		  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
 		  SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
@@ -72,7 +88,7 @@ static struct snd_pcm_hardware pcm_hardware_capture = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
 		 SNDRV_PCM_INFO_PAUSE),
-	.formats = (SNDRV_PCM_FMTBIT_S16_LE),
+	.formats = DAVINCI_PCM_FMTBITS,
 	.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
 		  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
 		  SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
-- 
1.7.4.1

[PATCH 3/6] ASoC: davinci-pcm: increase the maximum channels

[Ben Gardiner]

Based on the registration of davinci-mcasp.1 in the davinci-evm platform
setup for da830 and dm6467, davinci-pcm can handle more than the currently
reported maximum channels of 2.

Increase the maximum channels to 384 to match the maximum reported by
davinci-mcasp.1.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>

---
 sound/soc/davinci/davinci-pcm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 1e47267..9b5a9bf 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -75,7 +75,7 @@ static struct snd_pcm_hardware pcm_hardware_playback = {
 	.rate_min = 8000,
 	.rate_max = 96000,
 	.channels_min = 2,
-	.channels_max = 2,
+	.channels_max = 384,
 	.buffer_bytes_max = 128 * 1024,
 	.period_bytes_min = 32,
 	.period_bytes_max = 8 * 1024,
@@ -97,7 +97,7 @@ static struct snd_pcm_hardware pcm_hardware_capture = {
 	.rate_min = 8000,
 	.rate_max = 96000,
 	.channels_min = 2,
-	.channels_max = 2,
+	.channels_max = 384,
 	.buffer_bytes_max = 128 * 1024,
 	.period_bytes_min = 32,
 	.period_bytes_max = 8 * 1024,
-- 
1.7.4.1

[PATCH 4/6] ASoC: davinci-pcm: fix audible glitch on 2nd ping-pong playback

[Ben Gardiner]

The release of the dma channels was being performed in prepare and there was a
edma_resume call for the asp-channel only being executed on START, RESUME and
PAUSE_RELEASE.

The mcasp on da850evm with ping-pong buffers enabled was exhibiting an audible
glitch on every playback after the first. It was determined through trial and
error that the following two changes fix this problem:

1) Move the edma_start calls from prepare to trigger and 2) reverse the order
of starting the asp and ram channels.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>
CC: Troy Kisky <troy.kisky@boundarydevices.com>

---
 sound/soc/davinci/davinci-pcm.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 9b5a9bf..5d9269a 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -544,6 +544,13 @@ static int davinci_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		edma_start(prtd->asp_channel);
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
+		    prtd->ram_channel >= 0) {
+			/* copy 1st iram buffer */
+			edma_start(prtd->ram_channel);
+		}
+		break;
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		edma_resume(prtd->asp_channel);
@@ -582,11 +589,6 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 		print_buf_info(prtd->asp_link[0], "asp_link[0]");
 		print_buf_info(prtd->asp_link[1], "asp_link[1]");
 
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-			/* copy 1st iram buffer */
-			edma_start(prtd->ram_channel);
-		}
-		edma_start(prtd->asp_channel);
 		return 0;
 	}
 	prtd->period = 0;
@@ -596,7 +598,6 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 	edma_read_slot(prtd->asp_link[0], &prtd->asp_params);
 	edma_write_slot(prtd->asp_channel, &prtd->asp_params);
 	davinci_pcm_enqueue_dma(substream);
-	edma_start(prtd->asp_channel);
 
 	return 0;
 }
-- 
1.7.4.1

[PATCH 5/6] ASoC: davinci-pcm: extract period elapsed functions

[Ben Gardiner]

Extract functions that modify the prtd->period member in preparation for
conversion to BATCH mode playback.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>

---
 sound/soc/davinci/davinci-pcm.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 5d9269a..fedca81 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -155,6 +155,22 @@ struct davinci_runtime_data {
 	struct edmacc_param ram_params;
 };
 
+static void davinci_pcm_period_elapsed(struct snd_pcm_substream *substream)
+{
+	struct davinci_runtime_data *prtd = substream->runtime->private_data;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	prtd->period++;
+	if (unlikely(prtd->period >= runtime->periods))
+		prtd->period = 0;
+}
+
+static void davinci_pcm_period_reset(struct snd_pcm_substream *substream)
+{
+	struct davinci_runtime_data *prtd = substream->runtime->private_data;
+
+	prtd->period = 0;
+}
 /*
  * Not used with ping/pong
  */
@@ -216,9 +232,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)
 		edma_set_transfer_params(link, acnt, fifo_level, count,
 							fifo_level, ABSYNC);
 
-	prtd->period++;
-	if (unlikely(prtd->period >= runtime->periods))
-		prtd->period = 0;
+	davinci_pcm_period_elapsed(substream);
 }
 
 static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
@@ -591,7 +605,7 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 
 		return 0;
 	}
-	prtd->period = 0;
+	davinci_pcm_period_reset(substream);
 	davinci_pcm_enqueue_dma(substream);
 
 	/* Copy self-linked parameter RAM entry into master channel */
-- 
1.7.4.1

[PATCH 6/6] ASoC: davinci-pcm: convert to BATCH mode

[Ben Gardiner]

The davinci-pcm driver's snd_pcm_ops pointer function currently calls into
the edma controller driver to read the current positions of the edma channels
to determine pos to return to the ALSA framework. In particular,
davinci_pcm_pointer() calls edma_get_position() and the latter has a comment
indicating that "Its channel should not be active when this is called" whereas
the channel is surely active when snd_pcm_ops.pointer is called.

The operation of davinci-pcm in capture and playback appears to follow close
the other pcm drivers who export SNDRV_PCM_INFO_BATCH except that davinci-pcm
does not report it's positions from pointer() using the last transferred
chunk. Instead it peeks directly into the edma controller to determine the
current position as discussed above.

Convert the davinci-pcm driver to BATCH mode: count the periods elapsed in the
prtd->period member and use its value to report the 'pos' to the alsa
framework in the davinci_pcm_pointer function.

There is a phase offset of 2 periods between the position used by dma setup
and the position reported in the pointer function. Either +2 in the dma
setup or -2 in the pointer function (with wrapping, both) accounts for this
offset -- I opted for the latter since it makes the first-time setup clearer.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>

---
 sound/soc/davinci/davinci-pcm.c |   67 +++++++++++----------------------------
 1 files changed, 19 insertions(+), 48 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index fedca81..fa8fc61 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -65,7 +65,8 @@ static void print_buf_info(int slot, char *name)
 static struct snd_pcm_hardware pcm_hardware_playback = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
-		 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
+		 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME|
+		 SNDRV_PCM_INFO_BATCH),
 	.formats = DAVINCI_PCM_FMTBITS,
 	.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
 		  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
@@ -87,7 +88,8 @@ static struct snd_pcm_hardware pcm_hardware_playback = {
 static struct snd_pcm_hardware pcm_hardware_capture = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
-		 SNDRV_PCM_INFO_PAUSE),
+		 SNDRV_PCM_INFO_PAUSE |
+		 SNDRV_PCM_INFO_BATCH),
 	.formats = DAVINCI_PCM_FMTBITS,
 	.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
 		  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
@@ -231,8 +233,6 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)
 	else
 		edma_set_transfer_params(link, acnt, fifo_level, count,
 							fifo_level, ABSYNC);
-
-	davinci_pcm_period_elapsed(substream);
 }
 
 static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
@@ -247,12 +247,13 @@ static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
 		return;
 
 	if (snd_pcm_running(substream)) {
+		spin_lock(&prtd->lock);
 		if (prtd->ram_channel < 0) {
 			/* No ping/pong must fix up link dma data*/
-			spin_lock(&prtd->lock);
 			davinci_pcm_enqueue_dma(substream);
-			spin_unlock(&prtd->lock);
 		}
+		davinci_pcm_period_elapsed(substream);
+		spin_unlock(&prtd->lock);
 		snd_pcm_period_elapsed(substream);
 	}
 }
@@ -588,6 +589,7 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 {
 	struct davinci_runtime_data *prtd = substream->runtime->private_data;
 
+	davinci_pcm_period_reset(substream);
 	if (prtd->ram_channel >= 0) {
 		int ret = ping_pong_dma_setup(substream);
 		if (ret < 0)
@@ -603,15 +605,19 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 		print_buf_info(prtd->asp_link[0], "asp_link[0]");
 		print_buf_info(prtd->asp_link[1], "asp_link[1]");
 
+		davinci_pcm_period_elapsed(substream);
+		davinci_pcm_period_elapsed(substream);
+
 		return 0;
 	}
-	davinci_pcm_period_reset(substream);
 	davinci_pcm_enqueue_dma(substream);
+	davinci_pcm_period_elapsed(substream);
 
 	/* Copy self-linked parameter RAM entry into master channel */
 	edma_read_slot(prtd->asp_link[0], &prtd->asp_params);
 	edma_write_slot(prtd->asp_channel, &prtd->asp_params);
 	davinci_pcm_enqueue_dma(substream);
+	davinci_pcm_period_elapsed(substream);
 
 	return 0;
 }
@@ -623,51 +629,16 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	struct davinci_runtime_data *prtd = runtime->private_data;
 	unsigned int offset;
 	int asp_count;
-	dma_addr_t asp_src, asp_dst;
+	unsigned int period_size = snd_pcm_lib_period_bytes(substream);
 
 	spin_lock(&prtd->lock);
-	if (prtd->ram_channel >= 0) {
-		int ram_count;
-		int mod_ram;
-		dma_addr_t ram_src, ram_dst;
-		unsigned int period_size = snd_pcm_lib_period_bytes(substream);
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-			/* reading ram before asp should be safe
-			 * as long as the asp transfers less than a ping size
-			 * of bytes between the 2 reads
-			 */
-			edma_get_position(prtd->ram_channel,
-					&ram_src, &ram_dst);
-			edma_get_position(prtd->asp_channel,
-					&asp_src, &asp_dst);
-			asp_count = asp_src - prtd->asp_params.src;
-			ram_count = ram_src - prtd->ram_params.src;
-			mod_ram = ram_count % period_size;
-			mod_ram -= asp_count;
-			if (mod_ram < 0)
-				mod_ram += period_size;
-			else if (mod_ram == 0) {
-				if (snd_pcm_running(substream))
-					mod_ram += period_size;
-			}
-			ram_count -= mod_ram;
-			if (ram_count < 0)
-				ram_count += period_size * runtime->periods;
-		} else {
-			edma_get_position(prtd->ram_channel,
-					&ram_src, &ram_dst);
-			ram_count = ram_dst - prtd->ram_params.dst;
-		}
-		asp_count = ram_count;
-	} else {
-		edma_get_position(prtd->asp_channel, &asp_src, &asp_dst);
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			asp_count = asp_src - runtime->dma_addr;
-		else
-			asp_count = asp_dst - runtime->dma_addr;
-	}
+	asp_count = prtd->period - 2;
 	spin_unlock(&prtd->lock);
 
+	if (asp_count < 0)
+		asp_count += runtime->periods;
+	asp_count *= period_size;
+
 	offset = bytes_to_frames(runtime, asp_count);
 	if (offset >= runtime->buffer_size)
 		offset = 0;
-- 
1.7.4.1

Re: [PATCH 6/6] ASoC: davinci-pcm: convert to BATCH mode

[Liam Girdwood]

On 24/05/11 19:50, Ben Gardiner wrote:
> The davinci-pcm driver's snd_pcm_ops pointer function currently calls into
> the edma controller driver to read the current positions of the edma channels
> to determine pos to return to the ALSA framework. In particular,
> davinci_pcm_pointer() calls edma_get_position() and the latter has a comment
> indicating that "Its channel should not be active when this is called" whereas
> the channel is surely active when snd_pcm_ops.pointer is called.
> 
> The operation of davinci-pcm in capture and playback appears to follow close
> the other pcm drivers who export SNDRV_PCM_INFO_BATCH except that davinci-pcm
> does not report it's positions from pointer() using the last transferred
> chunk. Instead it peeks directly into the edma controller to determine the
> current position as discussed above.
> 
> Convert the davinci-pcm driver to BATCH mode: count the periods elapsed in the
> prtd->period member and use its value to report the 'pos' to the alsa
> framework in the davinci_pcm_pointer function.
> 
> There is a phase offset of 2 periods between the position used by dma setup
> and the position reported in the pointer function. Either +2 in the dma
> setup or -2 in the pointer function (with wrapping, both) accounts for this
> offset -- I opted for the latter since it makes the first-time setup clearer.
> 
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
> Reviewed-by: Steven Faludi <stevenfaludi@nanometrics.ca>
> 
> ---
>  sound/soc/davinci/davinci-pcm.c |   67 +++++++++++----------------------------
>  1 files changed, 19 insertions(+), 48 deletions(-)
> 
> diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
> index fedca81..fa8fc61 100644
> --- a/sound/soc/davinci/davinci-pcm.c
> +++ b/sound/soc/davinci/davinci-pcm.c
> @@ -65,7 +65,8 @@ static void print_buf_info(int slot, char *name)
>  static struct snd_pcm_hardware pcm_hardware_playback = {
>  	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
>  		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> -		 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
> +		 SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME|
> +		 SNDRV_PCM_INFO_BATCH),
>  	.formats = DAVINCI_PCM_FMTBITS,
>  	.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
>  		  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
> @@ -87,7 +88,8 @@ static struct snd_pcm_hardware pcm_hardware_playback = {
>  static struct snd_pcm_hardware pcm_hardware_capture = {
>  	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
>  		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> -		 SNDRV_PCM_INFO_PAUSE),
> +		 SNDRV_PCM_INFO_PAUSE |
> +		 SNDRV_PCM_INFO_BATCH),
>  	.formats = DAVINCI_PCM_FMTBITS,
>  	.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
>  		  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 |
> @@ -231,8 +233,6 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)
>  	else
>  		edma_set_transfer_params(link, acnt, fifo_level, count,
>  							fifo_level, ABSYNC);
> -
> -	davinci_pcm_period_elapsed(substream);
>  }
>  
>  static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
> @@ -247,12 +247,13 @@ static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
>  		return;
>  
>  	if (snd_pcm_running(substream)) {
> +		spin_lock(&prtd->lock);
>  		if (prtd->ram_channel < 0) {
>  			/* No ping/pong must fix up link dma data*/
> -			spin_lock(&prtd->lock);
>  			davinci_pcm_enqueue_dma(substream);
> -			spin_unlock(&prtd->lock);
>  		}
> +		davinci_pcm_period_elapsed(substream);
> +		spin_unlock(&prtd->lock);
>  		snd_pcm_period_elapsed(substream);
>  	}
>  }
> @@ -588,6 +589,7 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
>  {
>  	struct davinci_runtime_data *prtd = substream->runtime->private_data;
>  
> +	davinci_pcm_period_reset(substream);
>  	if (prtd->ram_channel >= 0) {
>  		int ret = ping_pong_dma_setup(substream);
>  		if (ret < 0)
> @@ -603,15 +605,19 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
>  		print_buf_info(prtd->asp_link[0], "asp_link[0]");
>  		print_buf_info(prtd->asp_link[1], "asp_link[1]");
>  
> +		davinci_pcm_period_elapsed(substream);
> +		davinci_pcm_period_elapsed(substream);

I assume these are to do with the 2 period phase offset you have so it's probably better to comment this here too. 

> +
>  		return 0;
>  	}
> -	davinci_pcm_period_reset(substream);
>  	davinci_pcm_enqueue_dma(substream);
> +	davinci_pcm_period_elapsed(substream);
>  
>  	/* Copy self-linked parameter RAM entry into master channel */
>  	edma_read_slot(prtd->asp_link[0], &prtd->asp_params);
>  	edma_write_slot(prtd->asp_channel, &prtd->asp_params);
>  	davinci_pcm_enqueue_dma(substream);
> +	davinci_pcm_period_elapsed(substream);
>  
>  	return 0;
>  }
> @@ -623,51 +629,16 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
>  	struct davinci_runtime_data *prtd = runtime->private_data;
>  	unsigned int offset;
>  	int asp_count;
> -	dma_addr_t asp_src, asp_dst;
> +	unsigned int period_size = snd_pcm_lib_period_bytes(substream);
>  
>  	spin_lock(&prtd->lock);
> -	if (prtd->ram_channel >= 0) {
> -		int ram_count;
> -		int mod_ram;
> -		dma_addr_t ram_src, ram_dst;
> -		unsigned int period_size = snd_pcm_lib_period_bytes(substream);
> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -			/* reading ram before asp should be safe
> -			 * as long as the asp transfers less than a ping size
> -			 * of bytes between the 2 reads
> -			 */
> -			edma_get_position(prtd->ram_channel,
> -					&ram_src, &ram_dst);
> -			edma_get_position(prtd->asp_channel,
> -					&asp_src, &asp_dst);
> -			asp_count = asp_src - prtd->asp_params.src;
> -			ram_count = ram_src - prtd->ram_params.src;
> -			mod_ram = ram_count % period_size;
> -			mod_ram -= asp_count;
> -			if (mod_ram < 0)
> -				mod_ram += period_size;
> -			else if (mod_ram == 0) {
> -				if (snd_pcm_running(substream))
> -					mod_ram += period_size;
> -			}
> -			ram_count -= mod_ram;
> -			if (ram_count < 0)
> -				ram_count += period_size * runtime->periods;
> -		} else {
> -			edma_get_position(prtd->ram_channel,
> -					&ram_src, &ram_dst);
> -			ram_count = ram_dst - prtd->ram_params.dst;
> -		}
> -		asp_count = ram_count;
> -	} else {
> -		edma_get_position(prtd->asp_channel, &asp_src, &asp_dst);
> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> -			asp_count = asp_src - runtime->dma_addr;
> -		else
> -			asp_count = asp_dst - runtime->dma_addr;
> -	}
> +	asp_count = prtd->period - 2;
>  	spin_unlock(&prtd->lock);
>  
> +	if (asp_count < 0)
> +		asp_count += runtime->periods;
> +	asp_count *= period_size;
> +
>  	offset = bytes_to_frames(runtime, asp_count);
>  	if (offset >= runtime->buffer_size)
>  		offset = 0;

Re: [PATCH 0/6] ASoC: davinci-pcm: some fixes and convert to BATCH

[Liam Girdwood]

On 24/05/11 19:50, Ben Gardiner wrote:
> This patchset is a collection of changes for the davinci-pcm driver that were
> accumulated during previous forays into the davinci-mcasp and davinci-pcm
> drivers [1].
> 
> The first three patches are minor changes that cleanup the ping-pong DMA params
> setup, expand the davinci-pcm reported format and also its maximum channels.
> 
> The fourth patch in the series corrects an audible glitch that can be heard on
> da850evm's McASP when ping-pong buffers are enabled. The glitch occurs only
> on the second and susequent playbacks, not the first. Reversing the order in
> which the dma channels are started and moving the start to the trigger function
> from the prepare function fixes the glitch.
> 
> The fifth and sixth patches in the series convert the davinci-pcm driver to
> BATCH mode. 
> 

All

Acked-by: Liam Girdwood <lrg@ti.com>

One minor thing for patch 6 that can be added incrementally.

Re: [PATCH 0/6] ASoC: davinci-pcm: some fixes and convert to BATCH

[Mark Brown]

On Tue, May 24, 2011 at 02:50:14PM -0400, Ben Gardiner wrote:
> This patchset is a collection of changes for the davinci-pcm driver that were
> accumulated during previous forays into the davinci-mcasp and davinci-pcm
> drivers [1].

Applied all, thanks.

Re: [PATCH 6/6] ASoC: davinci-pcm: convert to BATCH mode

[Ben Gardiner]

Hi Liam,

On Wed, May 25, 2011 at 5:52 AM, Liam Girdwood <lrg@ti.com> wrote:
> On 24/05/11 19:50, Ben Gardiner wrote:
>> [...]
>> @@ -603,15 +605,19 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
>>               print_buf_info(prtd->asp_link[0], "asp_link[0]");
>>               print_buf_info(prtd->asp_link[1], "asp_link[1]");
>>
>> +             davinci_pcm_period_elapsed(substream);
>> +             davinci_pcm_period_elapsed(substream);
>
> I assume these are to do with the 2 period phase offset you have so it's probably better to comment this here too.

That's correct and yes I really should have commented this magic a
little. I will send an incremental patch that can be folded-in. Thanks
again.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

[PATCH] ASoC: davinci-pcm: comments for the conversion to BATCH mode

[Ben Gardiner]

In the previous commit 'ASoC: davinci-pcm: convert to BATCH mode', the phase
offset of 2 was mentioned in the commit message but not well commented in the
source.

Add descriptive comments of the phase offset with and without ping-pong
buffers enabled.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>

---
 sound/soc/davinci/davinci-pcm.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index fa8fc61..c9e0320 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -605,6 +605,18 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 		print_buf_info(prtd->asp_link[0], "asp_link[0]");
 		print_buf_info(prtd->asp_link[1], "asp_link[1]");
 
+		/*
+		 * There is a phase offset of 2 periods between the position
+		 * used by dma setup and the position reported in the pointer
+		 * function.
+		 *
+		 * The phase offset, when not using ping-pong buffers, is due to
+		 * the two consecutive calls to davinci_pcm_enqueue_dma() below.
+		 *
+		 * Whereas here, with ping-pong buffers, the phase is due to
+		 * there being an entire buffer transfer complete before the
+		 * first dma completion event triggers davinci_pcm_dma_irq().
+		 */
 		davinci_pcm_period_elapsed(substream);
 		davinci_pcm_period_elapsed(substream);
 
@@ -631,6 +643,13 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	int asp_count;
 	unsigned int period_size = snd_pcm_lib_period_bytes(substream);
 
+	/*
+	 * There is a phase offset of 2 periods between the position used by dma
+	 * setup and the position reported in the pointer function. Either +2 in
+	 * the dma setup or -2 here in the pointer function (with wrapping,
+	 * both) accounts for this offset -- choose the latter since it makes
+	 * the first-time setup clearer.
+	 */
 	spin_lock(&prtd->lock);
 	asp_count = prtd->period - 2;
 	spin_unlock(&prtd->lock);
-- 
1.7.4.1

Re: [PATCH] ASoC: davinci-pcm: comments for the conversion to BATCH mode

[Liam Girdwood]

On 25/05/11 14:27, Ben Gardiner wrote:
> In the previous commit 'ASoC: davinci-pcm: convert to BATCH mode', the phase
> offset of 2 was mentioned in the commit message but not well commented in the
> source.
> 
> Add descriptive comments of the phase offset with and without ping-pong
> buffers enabled.
> 
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
> 
> ---
>  sound/soc/davinci/davinci-pcm.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
> index fa8fc61..c9e0320 100644
> --- a/sound/soc/davinci/davinci-pcm.c
> +++ b/sound/soc/davinci/davinci-pcm.c
> @@ -605,6 +605,18 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
>  		print_buf_info(prtd->asp_link[0], "asp_link[0]");
>  		print_buf_info(prtd->asp_link[1], "asp_link[1]");
>  
> +		/*
> +		 * There is a phase offset of 2 periods between the position
> +		 * used by dma setup and the position reported in the pointer
> +		 * function.
> +		 *
> +		 * The phase offset, when not using ping-pong buffers, is due to
> +		 * the two consecutive calls to davinci_pcm_enqueue_dma() below.
> +		 *
> +		 * Whereas here, with ping-pong buffers, the phase is due to
> +		 * there being an entire buffer transfer complete before the
> +		 * first dma completion event triggers davinci_pcm_dma_irq().
> +		 */
>  		davinci_pcm_period_elapsed(substream);
>  		davinci_pcm_period_elapsed(substream);
>  
> @@ -631,6 +643,13 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
>  	int asp_count;
>  	unsigned int period_size = snd_pcm_lib_period_bytes(substream);
>  
> +	/*
> +	 * There is a phase offset of 2 periods between the position used by dma
> +	 * setup and the position reported in the pointer function. Either +2 in
> +	 * the dma setup or -2 here in the pointer function (with wrapping,
> +	 * both) accounts for this offset -- choose the latter since it makes
> +	 * the first-time setup clearer.
> +	 */
>  	spin_lock(&prtd->lock);
>  	asp_count = prtd->period - 2;
>  	spin_unlock(&prtd->lock);

Acked-by: Liam Girdwood <lrg@ti.com>

Re: [PATCH] ASoC: davinci-pcm: comments for the conversion to BATCH mode

[Mark Brown]

On Wed, May 25, 2011 at 09:27:22AM -0400, Ben Gardiner wrote:
> In the previous commit 'ASoC: davinci-pcm: convert to BATCH mode', the phase
> offset of 2 was mentioned in the commit message but not well commented in the
> source.

Applied, thanks.