[PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Troy Kisky]

Allow the left and right 16 bit samples to be shifted out as 1
32 bit sample.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
--
This applies to Kevin's temp/asoc branch
---
 arch/arm/mach-davinci/include/mach/asp.h |    6 ++
 sound/soc/davinci/davinci-i2s.c          |   74 ++++++++++++++++++++++--------
 2 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-davinci/include/mach/asp.h b/arch/arm/mach-davinci/include/mach/asp.h
index 18e4ce3..a3d2aa1 100644
--- a/arch/arm/mach-davinci/include/mach/asp.h
+++ b/arch/arm/mach-davinci/include/mach/asp.h
@@ -51,6 +51,12 @@ struct snd_platform_data {
 	u32 rx_dma_offset;
 	enum dma_event_q eventq_no;	/* event queue number */
 	unsigned int codec_fmt;
+	/*
+	 * Allowing this is more efficient and eliminates left and right swaps
+	 * caused by underruns, but will swap the left and right channels
+	 * when compared to previous behavior.
+	 */
+	unsigned disable_channel_combine:1;
 
 	/* McASP specific fields */
 	int tdm_slots;
diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c
index 12a6c54..081b2d4 100644
--- a/sound/soc/davinci/davinci-i2s.c
+++ b/sound/soc/davinci/davinci-i2s.c
@@ -97,6 +97,23 @@ enum {
 	DAVINCI_MCBSP_WORD_32,
 };
 
+static const unsigned char data_type[SNDRV_PCM_FORMAT_S32_LE + 1] = {
+	[SNDRV_PCM_FORMAT_S8]		= 1,
+	[SNDRV_PCM_FORMAT_S16_LE]	= 2,
+	[SNDRV_PCM_FORMAT_S32_LE]	= 4,
+};
+
+static const unsigned char asp_word_length[SNDRV_PCM_FORMAT_S32_LE + 1] = {
+	[SNDRV_PCM_FORMAT_S8]		= DAVINCI_MCBSP_WORD_8,
+	[SNDRV_PCM_FORMAT_S16_LE]	= DAVINCI_MCBSP_WORD_16,
+	[SNDRV_PCM_FORMAT_S32_LE]	= DAVINCI_MCBSP_WORD_32,
+};
+
+static const unsigned char double_fmt[SNDRV_PCM_FORMAT_S32_LE + 1] = {
+	[SNDRV_PCM_FORMAT_S8]		= SNDRV_PCM_FORMAT_S16_LE,
+	[SNDRV_PCM_FORMAT_S16_LE]	= SNDRV_PCM_FORMAT_S32_LE,
+};
+
 static struct davinci_pcm_dma_params davinci_i2s_pcm_out = {
 	.name = "I2S PCM Stereo out",
 };
@@ -113,6 +130,27 @@ struct davinci_mcbsp_dev {
 	u32				pcr;
 	struct clk			*clk;
 	struct davinci_pcm_dma_params	*dma_params[2];
+	/*
+	 * Combining both channels into 1 element will at least double the
+	 * amount of time between servicing the dma channel, increase
+	 * effiency, and reduce the chance of overrun/underrun. But,
+	 * it will result in the left & right channels being swapped.
+	 *
+	 * If relabeling the left and right channels is not possible,
+	 * you may want to let the codec know to swap them back.
+	 *
+	 * It may allow x10 the amount of time to service dma requests,
+	 * if the codec is master and is using an unnecessarily fast bit clock
+	 * (ie. tlvaic23b), independent of the sample rate. So, having an
+	 * entire frame at once means it can be serviced at the sample rate
+	 * instead of the bit clock rate.
+	 *
+	 * In the now unlikely case that an underrun still
+	 * occurs, both the left and right samples will be repeated
+	 * so that no pops are heard, and the left and right channels
+	 * won't end up being swapped because of the underrun.
+	 */
+	unsigned disable_channel_combine:1;
 };
 
 static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
@@ -359,6 +397,8 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	int mcbsp_word_length;
 	unsigned int rcr, xcr, srgr;
 	u32 spcr;
+	snd_pcm_format_t fmt;
+	unsigned element_cnt = 1;
 
 	/* general line settings */
 	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
@@ -388,27 +428,22 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1);
 	}
 	/* Determine xfer data type */
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S8:
-		dma_params->data_type = 1;
-		mcbsp_word_length = DAVINCI_MCBSP_WORD_8;
-		break;
-	case SNDRV_PCM_FORMAT_S16_LE:
-		dma_params->data_type = 2;
-		mcbsp_word_length = DAVINCI_MCBSP_WORD_16;
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		dma_params->data_type = 4;
-		mcbsp_word_length = DAVINCI_MCBSP_WORD_32;
-		break;
-	default:
+	fmt = params_format(params);
+	if ((fmt > SNDRV_PCM_FORMAT_S32_LE) || !data_type[fmt]) {
 		printk(KERN_WARNING "davinci-i2s: unsupported PCM format\n");
 		return -EINVAL;
 	}
-
-	dma_params->acnt  = dma_params->data_type;
-	rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(1);
-	xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(1);
+	if (params_channels(params) == 2) {
+		element_cnt = 2;
+		if (double_fmt[fmt] && !dev->disable_channel_combine) {
+			element_cnt = 1;
+			fmt = double_fmt[fmt];
+		}
+	}
+	dma_params->acnt = dma_params->data_type = data_type[fmt];
+	mcbsp_word_length = asp_word_length[fmt];
+	rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(element_cnt - 1);
+	xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(element_cnt - 1);
 
 	rcr |= DAVINCI_MCBSP_RCR_RWDLEN1(mcbsp_word_length) |
 		DAVINCI_MCBSP_RCR_RWDLEN2(mcbsp_word_length);
@@ -524,7 +559,8 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 		ret = -ENOMEM;
 		goto err_release_region;
 	}
-
+	if (pdata)
+		dev->disable_channel_combine = pdata->disable_channel_combine;
 	dev->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk)) {
 		ret = -ENODEV;
-- 
1.5.6.3

[PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

Rename variable master_lch to asp_master_lch
Rename variable slave_lch to asp_link_lch[0]
Rename local variables:
	count to asp_count
	src to asp_src
	dst to asp_dst

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 sound/soc/davinci/davinci-pcm.c |   48 +++++++++++++++++++-------------------
 1 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 2f7da49..a96655c 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -51,8 +51,8 @@ static struct snd_pcm_hardware davinci_pcm_hardware = {
 struct davinci_runtime_data {
 	spinlock_t lock;
 	int period;		/* current DMA period */
-	int master_lch;		/* Master DMA channel */
-	int slave_lch;		/* linked parameter RAM reload slot */
+	int asp_master_lch;	/* Master DMA channel */
+	int asp_link_lch[2];	/* asp parameter link channel, ping/pong */
 	struct davinci_pcm_dma_params *params;	/* DMA params */
 };
 
@@ -60,7 +60,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)
 {
 	struct davinci_runtime_data *prtd = substream->runtime->private_data;
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	int lch = prtd->slave_lch;
+	int lch = prtd->asp_link_lch[0];
 	unsigned int period_size;
 	unsigned int dma_offset;
 	dma_addr_t dma_pos;
@@ -142,15 +142,15 @@ static int davinci_pcm_dma_request(struct snd_pcm_substream *substream)
 				  EVENTQ_0);
 	if (ret < 0)
 		return ret;
-	prtd->master_lch = ret;
+	prtd->asp_master_lch = ret;
 
 	/* Request parameter RAM reload slot */
-	ret = edma_alloc_slot(EDMA_CTLR(prtd->master_lch), EDMA_SLOT_ANY);
+	ret = edma_alloc_slot(EDMA_CTLR(prtd->asp_master_lch), EDMA_SLOT_ANY);
 	if (ret < 0) {
-		edma_free_channel(prtd->master_lch);
+		edma_free_channel(prtd->asp_master_lch);
 		return ret;
 	}
-	prtd->slave_lch = ret;
+	prtd->asp_link_lch[0] = ret;
 
 	/* Issue transfer completion IRQ when the channel completes a
 	 * transfer, then always reload from the same slot (by a kind
@@ -161,10 +161,10 @@ static int davinci_pcm_dma_request(struct snd_pcm_substream *substream)
 	 * the buffer and its length (ccnt) ... use it as a template
 	 * so davinci_pcm_enqueue_dma() takes less time in IRQ.
 	 */
-	edma_read_slot(prtd->slave_lch, &p_ram);
-	p_ram.opt |= TCINTEN | EDMA_TCC(EDMA_CHAN_SLOT(prtd->master_lch));
-	p_ram.link_bcntrld = EDMA_CHAN_SLOT(prtd->slave_lch) << 5;
-	edma_write_slot(prtd->slave_lch, &p_ram);
+	edma_read_slot(prtd->asp_link_lch[0], &p_ram);
+	p_ram.opt |= TCINTEN | EDMA_TCC(EDMA_CHAN_SLOT(prtd->asp_master_lch));
+	p_ram.link_bcntrld = EDMA_CHAN_SLOT(prtd->asp_link_lch[0]) << 5;
+	edma_write_slot(prtd->asp_link_lch[0], &p_ram);
 
 	return 0;
 }
@@ -180,12 +180,12 @@ static int davinci_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		edma_start(prtd->master_lch);
+		edma_start(prtd->asp_master_lch);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		edma_stop(prtd->master_lch);
+		edma_stop(prtd->asp_master_lch);
 		break;
 	default:
 		ret = -EINVAL;
@@ -206,8 +206,8 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 	davinci_pcm_enqueue_dma(substream);
 
 	/* Copy self-linked parameter RAM entry into master channel */
-	edma_read_slot(prtd->slave_lch, &temp);
-	edma_write_slot(prtd->master_lch, &temp);
+	edma_read_slot(prtd->asp_link_lch[0], &temp);
+	edma_write_slot(prtd->asp_master_lch, &temp);
 	davinci_pcm_enqueue_dma(substream);
 
 	return 0;
@@ -219,20 +219,20 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct davinci_runtime_data *prtd = runtime->private_data;
 	unsigned int offset;
-	dma_addr_t count;
-	dma_addr_t src, dst;
+	int asp_count;
+	dma_addr_t asp_src, asp_dst;
 
 	spin_lock(&prtd->lock);
 
-	edma_get_position(prtd->master_lch, &src, &dst);
+	edma_get_position(prtd->asp_master_lch, &asp_src, &asp_dst);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		count = src - runtime->dma_addr;
+		asp_count = asp_src - runtime->dma_addr;
 	else
-		count = dst - runtime->dma_addr;
+		asp_count = asp_dst - runtime->dma_addr;
 
 	spin_unlock(&prtd->lock);
 
-	offset = bytes_to_frames(runtime, count);
+	offset = bytes_to_frames(runtime, asp_count);
 	if (offset >= runtime->buffer_size)
 		offset = 0;
 
@@ -274,10 +274,10 @@ static int davinci_pcm_close(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct davinci_runtime_data *prtd = runtime->private_data;
 
-	edma_unlink(prtd->slave_lch);
+	edma_unlink(prtd->asp_link_lch[0]);
 
-	edma_free_slot(prtd->slave_lch);
-	edma_free_channel(prtd->master_lch);
+	edma_free_slot(prtd->asp_link_lch[0]);
+	edma_free_channel(prtd->asp_master_lch);
 
 	kfree(prtd);
 
-- 
1.5.6.3

[PATCH 3/3] ASoC: DaVinci: pcm, fix underrun by using sram

[Troy Kisky]

Fix underruns by using dma to copy 1st to sram
in a ping/pong buffer style and then copying from
the sram to the ASP. This also has the advantage
of tolerating very long interrupt latency on dma
completion.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 arch/arm/mach-davinci/include/mach/asp.h |    2 +
 sound/soc/davinci/davinci-i2s.c          |    5 +-
 sound/soc/davinci/davinci-pcm.c          |  513 +++++++++++++++++++++++++++---
 sound/soc/davinci/davinci-pcm.h          |    1 +
 4 files changed, 474 insertions(+), 47 deletions(-)

diff --git a/arch/arm/mach-davinci/include/mach/asp.h b/arch/arm/mach-davinci/include/mach/asp.h
index a3d2aa1..f0f3b27 100644
--- a/arch/arm/mach-davinci/include/mach/asp.h
+++ b/arch/arm/mach-davinci/include/mach/asp.h
@@ -57,6 +57,8 @@ struct snd_platform_data {
 	 * when compared to previous behavior.
 	 */
 	unsigned disable_channel_combine:1;
+	unsigned sram_size_playback;
+	unsigned sram_size_capture;
 
 	/* McASP specific fields */
 	int tdm_slots;
diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c
index 081b2d4..863313f 100644
--- a/sound/soc/davinci/davinci-i2s.c
+++ b/sound/soc/davinci/davinci-i2s.c
@@ -559,8 +559,11 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 		ret = -ENOMEM;
 		goto err_release_region;
 	}
-	if (pdata)
+	if (pdata) {
 		dev->disable_channel_combine = pdata->disable_channel_combine;
+		davinci_i2s_pcm_out.sram_size = pdata->sram_size_playback;
+		davinci_i2s_pcm_in.sram_size = pdata->sram_size_capture;
+	}
 	dev->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk)) {
 		ret = -ENODEV;
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index a96655c..2ce0a8e 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -3,6 +3,7 @@
  *
  * Author:      Vladimir Barinov, <vbarinov@embeddedalley.com>
  * Copyright:   (C) 2007 MontaVista Software, Inc., <source@mvista.com>
+ * added SRAM ping/pong (C) 2008 Troy Kisky <troy.kisky@boundarydevices.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -23,10 +24,51 @@
 
 #include <asm/dma.h>
 #include <mach/edma.h>
+#include <mach/sram.h>
 
 #include "davinci-pcm.h"
 
-static struct snd_pcm_hardware davinci_pcm_hardware = {
+#ifdef DEBUG
+static void print_buf_info(int lch, char *name)
+{
+	struct edmacc_param p;
+	if (lch < 0)
+		return;
+	edma_read_slot(lch, &p);
+	printk(KERN_DEBUG "%s: 0x%x, opt=%x, src=%x, a_b_cnt=%x dst=%x\n",
+			name, lch, p.opt, p.src, p.a_b_cnt, p.dst);
+	printk(KERN_DEBUG "    src_dst_bidx=%x link_bcntrld=%x src_dst_cidx=%x ccnt=%x\n",
+			p.src_dst_bidx, p.link_bcntrld, p.src_dst_cidx, p.ccnt);
+}
+#else
+static void print_buf_info(int lch, char *name)
+{
+}
+#endif
+
+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),
+	.formats = (SNDRV_PCM_FMTBIT_S16_LE),
+	.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 |
+		  SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000 |
+		  SNDRV_PCM_RATE_KNOT),
+	.rate_min = 8000,
+	.rate_max = 96000,
+	.channels_min = 2,
+	.channels_max = 2,
+	.buffer_bytes_max = 128 * 1024,
+	.period_bytes_min = 32,
+	.period_bytes_max = 8 * 1024,
+	.periods_min = 16,
+	.periods_max = 255,
+	.fifo_size = 0,
+};
+
+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),
@@ -48,14 +90,58 @@ static struct snd_pcm_hardware davinci_pcm_hardware = {
 	.fifo_size = 0,
 };
 
+/*
+ * How ping/pong works....
+ *
+ * Playback:
+ * ram_params - copys 2*ping_size from start of SDRAM to iram,
+ * 	links to ram_link_lch2
+ * ram_link_lch2 - copys rest of SDRAM to iram in ping_size units,
+ * 	links to ram_link_lch
+ * ram_link_lch - copys entire SDRAM to iram in ping_size uints,
+ * 	links to self
+ *
+ * asp_params - same as asp_link_lch[0]
+ * asp_link_lch[0] - copys from lower half of iram to asp port
+ * 	links to asp_link_lch[1], triggers iram copy event on completion
+ * asp_link_lch[1] - copys from upper half of iram to asp port
+ * 	links to asp_link_lch[0], triggers iram copy event on completion
+ * 	triggers interrupt only needed to let upper SOC levels update position
+ * 	in stream on completion
+ *
+ * When playback is started:
+ * 	ram_params started
+ * 	asp_params started
+ *
+ * Capture:
+ * ram_params - same as ram_link_lch,
+ * 	links to ram_link_lch
+ * ram_link_lch - same as playback
+ * 	links to self
+ *
+ * asp_params - same as playback
+ * asp_link_lch[0] - same as playback
+ * asp_link_lch[1] - same as playback
+ *
+ * When capture is started:
+ * 	asp_params started
+ */
 struct davinci_runtime_data {
 	spinlock_t lock;
 	int period;		/* current DMA period */
 	int asp_master_lch;	/* Master DMA channel */
 	int asp_link_lch[2];	/* asp parameter link channel, ping/pong */
 	struct davinci_pcm_dma_params *params;	/* DMA params */
+	int ram_master_lch;
+	int ram_link_lch;
+	int ram_link_lch2;
+	struct edmacc_param asp_params;
+	struct edmacc_param ram_params;
 };
 
+/*
+ * Not used with ping/pong
+ */
 static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)
 {
 	struct davinci_runtime_data *prtd = substream->runtime->private_data;
@@ -109,48 +195,286 @@ static void davinci_pcm_dma_irq(unsigned lch, u16 ch_status, void *data)
 	struct snd_pcm_substream *substream = data;
 	struct davinci_runtime_data *prtd = substream->runtime->private_data;
 
+	print_buf_info(prtd->ram_master_lch, "i ram_master_lch");
 	pr_debug("davinci_pcm: lch=%d, status=0x%x\n", lch, ch_status);
 
 	if (unlikely(ch_status != DMA_COMPLETE))
 		return;
 
 	if (snd_pcm_running(substream)) {
+		if (prtd->ram_master_lch < 0) {
+			/* No ping/pong must fix up link dma data*/
+			spin_lock(&prtd->lock);
+			davinci_pcm_enqueue_dma(substream);
+			spin_unlock(&prtd->lock);
+		}
 		snd_pcm_period_elapsed(substream);
+	}
+}
 
-		spin_lock(&prtd->lock);
-		davinci_pcm_enqueue_dma(substream);
-		spin_unlock(&prtd->lock);
+static int allocate_sram(struct snd_pcm_substream *substream, unsigned size,
+		struct snd_pcm_hardware *ppcm)
+{
+	struct snd_dma_buffer *buf = &substream->dma_buffer;
+	struct snd_dma_buffer *iram_dma = NULL;
+	dma_addr_t iram_phys = 0;
+	void *iram_virt = NULL;
+
+	if (buf->private_data || !size)
+		return 0;
+
+	ppcm->period_bytes_max = size;
+	iram_virt = sram_alloc(size, &iram_phys);
+	if (!iram_virt)
+		goto exit1;
+	iram_dma = kzalloc(sizeof(*iram_dma), GFP_KERNEL);
+	if (!iram_dma)
+		goto exit2;
+	iram_dma->area = iram_virt;
+	iram_dma->addr = iram_phys;
+	memset(iram_dma->area, 0, size);
+	iram_dma->bytes = size;
+	buf->private_data = iram_dma;
+	return 0;
+exit2:
+	if (iram_virt)
+		sram_free(iram_virt, size);
+exit1:
+	return -ENOMEM;
+}
+
+/*
+ * Only used with ping/pong.
+ * This is called after runtime->dma_addr, period_bytes and data_type are valid
+ */
+static int ping_pong_dma_setup(struct snd_pcm_substream *substream)
+{
+	unsigned short ram_src_cidx, ram_dst_cidx;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct davinci_runtime_data *prtd = runtime->private_data;
+	struct snd_dma_buffer *iram_dma =
+		(struct snd_dma_buffer *)substream->dma_buffer.private_data;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct davinci_pcm_dma_params *dma_data = rtd->dai->cpu_dai->dma_data;
+	unsigned int data_type = dma_data->data_type;
+	unsigned int acnt = dma_data->acnt;
+	/* divide by 2 for ping/pong */
+	unsigned int ping_size = snd_pcm_lib_period_bytes(substream) >> 1;
+	int lch = prtd->asp_link_lch[1];
+	if ((data_type == 0) || (data_type > 4)) {
+		printk(KERN_ERR "%s: data_type=%i\n", __func__, data_type);
+		return -EINVAL;
 	}
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		dma_addr_t asp_src_pong = iram_dma->addr + ping_size;
+		ram_src_cidx = ping_size;
+		ram_dst_cidx = -ping_size;
+		edma_set_src(lch, asp_src_pong, INCR, W8BIT);
+
+		lch = prtd->asp_link_lch[0];
+		edma_set_src_index(lch, data_type, 0);
+		lch = prtd->asp_link_lch[1];
+		edma_set_src_index(lch, data_type, 0);
+
+		lch = prtd->ram_link_lch;
+		edma_set_src(lch, runtime->dma_addr, INCR, W32BIT);
+	} else {
+		dma_addr_t asp_dst_pong = iram_dma->addr + ping_size;
+		ram_src_cidx = -ping_size;
+		ram_dst_cidx = ping_size;
+		edma_set_dest(lch, asp_dst_pong, INCR, W8BIT);
+
+		lch = prtd->asp_link_lch[0];
+		edma_set_dest_index(lch, data_type, 0);
+		lch = prtd->asp_link_lch[1];
+		edma_set_dest_index(lch, data_type, 0);
+
+		lch = prtd->ram_link_lch;
+		edma_set_dest(lch, runtime->dma_addr, INCR, W32BIT);
+	}
+
+	lch = prtd->asp_link_lch[0];
+	edma_set_transfer_params(lch, acnt,
+			ping_size/data_type, 1, 0, ASYNC);
+	lch = prtd->asp_link_lch[1];
+	edma_set_transfer_params(lch, acnt,
+			ping_size/data_type, 1, 0, ASYNC);
+
+
+	lch = prtd->ram_link_lch;
+	edma_set_src_index(lch, ping_size, ram_src_cidx);
+	edma_set_dest_index(lch, ping_size, ram_dst_cidx);
+	edma_set_transfer_params(lch, ping_size, 2,
+			runtime->periods, 2, ASYNC);
+
+	/* init master params */
+	edma_read_slot(prtd->asp_link_lch[0], &prtd->asp_params);
+	edma_read_slot(prtd->ram_link_lch, &prtd->ram_params);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		struct edmacc_param p_ram;
+		/* Copy entire iram buffer before playback started */
+		prtd->ram_params.a_b_cnt = (1 << 16) | (ping_size << 1);
+		/* 0 dst_bidx */
+		prtd->ram_params.src_dst_bidx = (ping_size << 1);
+		/* 0 dst_cidx */
+		prtd->ram_params.src_dst_cidx = (ping_size << 1);
+		prtd->ram_params.ccnt = 1;
+
+		/* Skip 1st period */
+		edma_read_slot(prtd->ram_link_lch, &p_ram);
+		p_ram.src += (ping_size << 1);
+		p_ram.ccnt -= 1;
+		edma_write_slot(prtd->ram_link_lch2, &p_ram);
+		/*
+		 * When 1st started, ram -> iram dma channel will fill the
+		 * entire iram.  Then, whenever a ping/pong asp buffer finishes,
+		 * 1/2 iram will be filled.
+		 */
+		prtd->ram_params.link_bcntrld =
+			EDMA_CHAN_SLOT(prtd->ram_link_lch2) << 5;
+	}
+	return 0;
+}
+
+/* 1 asp tx or rx channel using 2 parameter channels
+ * 1 ram to/from iram channel using 1 parameter channel
+ *
+ * Playback
+ * ram copy channel kicks off first,
+ * 1st ram copy of entire iram buffer completion kicks off asp channel
+ * asp tcc always kicks off ram copy of 1/2 iram buffer
+ *
+ * Record
+ * asp channel starts, tcc kicks off ram copy
+ */
+static int request_ping_pong(struct snd_pcm_substream *substream,
+		struct davinci_runtime_data *prtd,
+		struct snd_dma_buffer *iram_dma)
+{
+	dma_addr_t asp_src_ping;
+	dma_addr_t asp_dst_ping;
+	int lch;
+	struct davinci_pcm_dma_params *dma_data = prtd->params;
+
+	/* Request ram master channel */
+	lch = prtd->ram_master_lch = edma_alloc_channel(EDMA_CHANNEL_ANY,
+				  davinci_pcm_dma_irq, substream,
+				  EVENTQ_1);
+	if (lch < 0)
+		goto exit1;
+
+	/* Request ram link channel */
+	lch = prtd->ram_link_lch = edma_alloc_slot(
+			EDMA_CTLR(prtd->ram_master_lch), EDMA_SLOT_ANY);
+	if (lch < 0)
+		goto exit2;
+
+	lch = prtd->asp_link_lch[1] = edma_alloc_slot(
+			EDMA_CTLR(prtd->asp_master_lch), EDMA_SLOT_ANY);
+	if (lch < 0)
+		goto exit3;
+
+	prtd->ram_link_lch2 = -1;
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		lch = prtd->ram_link_lch2 = edma_alloc_slot(
+			EDMA_CTLR(prtd->ram_master_lch), EDMA_SLOT_ANY);
+		if (lch < 0)
+			goto exit4;
+	}
+	/* circle ping-pong buffers */
+	edma_link(prtd->asp_link_lch[0], prtd->asp_link_lch[1]);
+	edma_link(prtd->asp_link_lch[1], prtd->asp_link_lch[0]);
+	/* circle ram buffers */
+	edma_link(prtd->ram_link_lch, prtd->ram_link_lch);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		asp_src_ping = iram_dma->addr;
+		asp_dst_ping = dma_data->dma_addr;	/* fifo */
+	} else {
+		asp_src_ping = dma_data->dma_addr;	/* fifo */
+		asp_dst_ping = iram_dma->addr;
+	}
+	/* ping */
+	lch = prtd->asp_link_lch[0];
+	edma_set_src(lch, asp_src_ping, INCR, W16BIT);
+	edma_set_dest(lch, asp_dst_ping, INCR, W16BIT);
+	edma_set_src_index(lch, 0, 0);
+	edma_set_dest_index(lch, 0, 0);
+
+	edma_read_slot(lch, &prtd->asp_params);
+	prtd->asp_params.opt &= ~(TCCMODE | EDMA_TCC(0x3f) | TCINTEN);
+	prtd->asp_params.opt |= TCCHEN | EDMA_TCC(prtd->ram_master_lch & 0x3f);
+	edma_write_slot(lch, &prtd->asp_params);
+
+	/* pong */
+	lch = prtd->asp_link_lch[1];
+	edma_set_src(lch, asp_src_ping, INCR, W16BIT);
+	edma_set_dest(lch, asp_dst_ping, INCR, W16BIT);
+	edma_set_src_index(lch, 0, 0);
+	edma_set_dest_index(lch, 0, 0);
+
+	edma_read_slot(lch, &prtd->asp_params);
+	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_master_lch));
+	edma_write_slot(lch, &prtd->asp_params);
+
+	/* ram */
+	lch = prtd->ram_link_lch;
+	edma_set_src(lch, iram_dma->addr, INCR, W32BIT);
+	edma_set_dest(lch, iram_dma->addr, INCR, W32BIT);
+	pr_debug("%s: audio dma channels/slots in use for ram:%u %u %u,"
+		"for asp:%u %u %u\n", __func__,
+		prtd->ram_master_lch, prtd->ram_link_lch, prtd->ram_link_lch2,
+		prtd->asp_master_lch, prtd->asp_link_lch[0],
+		prtd->asp_link_lch[1]);
+	return 0;
+exit4:
+	edma_free_channel(prtd->asp_link_lch[1]);
+	prtd->asp_link_lch[1] = -1;
+exit3:
+	edma_free_channel(prtd->ram_link_lch);
+	prtd->ram_link_lch = -1;
+exit2:
+	edma_free_channel(prtd->ram_master_lch);
+	prtd->ram_master_lch = -1;
+exit1:
+	return lch;
 }
 
 static int davinci_pcm_dma_request(struct snd_pcm_substream *substream)
 {
+	struct snd_dma_buffer *iram_dma;
 	struct davinci_runtime_data *prtd = substream->runtime->private_data;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct davinci_pcm_dma_params *dma_data = rtd->dai->cpu_dai->dma_data;
-	struct edmacc_param p_ram;
-	int ret;
+	int lch;
 
 	if (!dma_data)
 		return -ENODEV;
 
 	prtd->params = dma_data;
 
-	/* Request master DMA channel */
-	ret = edma_alloc_channel(prtd->params->channel,
-				  davinci_pcm_dma_irq, substream,
-				  EVENTQ_0);
-	if (ret < 0)
-		return ret;
-	prtd->asp_master_lch = ret;
-
-	/* Request parameter RAM reload slot */
-	ret = edma_alloc_slot(EDMA_CTLR(prtd->asp_master_lch), EDMA_SLOT_ANY);
-	if (ret < 0) {
-		edma_free_channel(prtd->asp_master_lch);
-		return ret;
+	/* Request asp master DMA channel */
+	lch = prtd->asp_master_lch = edma_alloc_channel(dma_data->channel,
+			davinci_pcm_dma_irq, substream, EVENTQ_0);
+	if (lch < 0)
+		goto exit1;
+
+	/* Request asp link channels */
+	lch = prtd->asp_link_lch[0] = edma_alloc_slot(
+			EDMA_CTLR(prtd->asp_master_lch), EDMA_SLOT_ANY);
+	if (lch < 0)
+		goto exit2;
+
+	iram_dma = (struct snd_dma_buffer *)substream->dma_buffer.private_data;
+	if (iram_dma) {
+		if (request_ping_pong(substream, prtd, iram_dma) == 0)
+			return 0;
+		printk(KERN_WARNING "%s: dma channel allocation failed,"
+				"not using sram\n", __func__);
 	}
-	prtd->asp_link_lch[0] = ret;
 
 	/* Issue transfer completion IRQ when the channel completes a
 	 * transfer, then always reload from the same slot (by a kind
@@ -161,12 +485,17 @@ static int davinci_pcm_dma_request(struct snd_pcm_substream *substream)
 	 * the buffer and its length (ccnt) ... use it as a template
 	 * so davinci_pcm_enqueue_dma() takes less time in IRQ.
 	 */
-	edma_read_slot(prtd->asp_link_lch[0], &p_ram);
-	p_ram.opt |= TCINTEN | EDMA_TCC(EDMA_CHAN_SLOT(prtd->asp_master_lch));
-	p_ram.link_bcntrld = EDMA_CHAN_SLOT(prtd->asp_link_lch[0]) << 5;
-	edma_write_slot(prtd->asp_link_lch[0], &p_ram);
-
+	edma_read_slot(lch, &prtd->asp_params);
+	prtd->asp_params.opt |= TCINTEN |
+		EDMA_TCC(EDMA_CHAN_SLOT(prtd->asp_master_lch));
+	prtd->asp_params.link_bcntrld = EDMA_CHAN_SLOT(lch) << 5;
+	edma_write_slot(lch, &prtd->asp_params);
 	return 0;
+exit2:
+	edma_free_channel(prtd->asp_master_lch);
+	prtd->asp_master_lch = -1;
+exit1:
+	return lch;
 }
 
 static int davinci_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
@@ -180,12 +509,12 @@ static int davinci_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		edma_start(prtd->asp_master_lch);
+		edma_resume(prtd->asp_master_lch);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		edma_stop(prtd->asp_master_lch);
+		edma_pause(prtd->asp_master_lch);
 		break;
 	default:
 		ret = -EINVAL;
@@ -200,14 +529,35 @@ static int davinci_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
 {
 	struct davinci_runtime_data *prtd = substream->runtime->private_data;
-	struct edmacc_param temp;
 
+	if (prtd->ram_master_lch >= 0) {
+		int ret = ping_pong_dma_setup(substream);
+		if (ret < 0)
+			return ret;
+
+		edma_write_slot(prtd->ram_master_lch, &prtd->ram_params);
+		edma_write_slot(prtd->asp_master_lch, &prtd->asp_params);
+
+		print_buf_info(prtd->ram_master_lch, "ram_master_lch");
+		print_buf_info(prtd->ram_link_lch, "ram_link_lch");
+		print_buf_info(prtd->ram_link_lch2, "ram_link_lch2");
+		print_buf_info(prtd->asp_master_lch, "asp_master_lch");
+		print_buf_info(prtd->asp_link_lch[0], "asp_link_lch[0]");
+		print_buf_info(prtd->asp_link_lch[1], "asp_link_lch[1]");
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			/* copy 1st iram buffer */
+			edma_start(prtd->ram_master_lch);
+		}
+		edma_start(prtd->asp_master_lch);
+		return 0;
+	}
 	prtd->period = 0;
 	davinci_pcm_enqueue_dma(substream);
 
 	/* Copy self-linked parameter RAM entry into master channel */
-	edma_read_slot(prtd->asp_link_lch[0], &temp);
-	edma_write_slot(prtd->asp_master_lch, &temp);
+	edma_read_slot(prtd->asp_link_lch[0], &prtd->asp_params);
+	edma_write_slot(prtd->asp_master_lch, &prtd->asp_params);
 	davinci_pcm_enqueue_dma(substream);
 
 	return 0;
@@ -223,13 +573,46 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	dma_addr_t asp_src, asp_dst;
 
 	spin_lock(&prtd->lock);
-
-	edma_get_position(prtd->asp_master_lch, &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;
-
+	if (prtd->ram_master_lch >= 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_master_lch,
+					&ram_src, &ram_dst);
+			edma_get_position(prtd->asp_master_lch,
+					&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_master_lch,
+					&ram_src, &ram_dst);
+			ram_count = ram_dst - prtd->ram_params.dst;
+		}
+		asp_count = ram_count;
+	} else {
+		edma_get_position(prtd->asp_master_lch, &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;
+	}
 	spin_unlock(&prtd->lock);
 
 	offset = bytes_to_frames(runtime, asp_count);
@@ -241,11 +624,17 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 
 static int davinci_pcm_open(struct snd_pcm_substream *substream)
 {
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct davinci_pcm_dma_params *dma_data = rtd->dai->cpu_dai->dma_data;
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct davinci_runtime_data *prtd;
+	struct snd_pcm_hardware *ppcm;
 	int ret = 0;
 
-	snd_soc_set_runtime_hwparams(substream, &davinci_pcm_hardware);
+	ppcm = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
+			&pcm_hardware_playback : &pcm_hardware_capture;
+	allocate_sram(substream, dma_data->sram_size, ppcm);
+	snd_soc_set_runtime_hwparams(substream, ppcm);
 	/* ensure that buffer size is a multiple of period size */
 	ret = snd_pcm_hw_constraint_integer(runtime,
 						SNDRV_PCM_HW_PARAM_PERIODS);
@@ -257,6 +646,11 @@ static int davinci_pcm_open(struct snd_pcm_substream *substream)
 		return -ENOMEM;
 
 	spin_lock_init(&prtd->lock);
+	prtd->asp_master_lch = -1;
+	prtd->asp_link_lch[0] = prtd->asp_link_lch[1] = -1;
+	prtd->ram_master_lch = -1;
+	prtd->ram_link_lch = -1;
+	prtd->ram_link_lch2 = -1;
 
 	runtime->private_data = prtd;
 
@@ -274,10 +668,29 @@ static int davinci_pcm_close(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct davinci_runtime_data *prtd = runtime->private_data;
 
-	edma_unlink(prtd->asp_link_lch[0]);
-
-	edma_free_slot(prtd->asp_link_lch[0]);
-	edma_free_channel(prtd->asp_master_lch);
+	if (prtd->ram_master_lch >= 0)
+		edma_stop(prtd->ram_master_lch);
+	if (prtd->asp_master_lch >= 0)
+		edma_stop(prtd->asp_master_lch);
+	if (prtd->asp_link_lch[0] >= 0)
+		edma_unlink(prtd->asp_link_lch[0]);
+	if (prtd->asp_link_lch[1] >= 0)
+		edma_unlink(prtd->asp_link_lch[1]);
+	if (prtd->ram_link_lch >= 0)
+		edma_unlink(prtd->ram_link_lch);
+
+	if (prtd->asp_link_lch[0] >= 0)
+		edma_free_slot(prtd->asp_link_lch[0]);
+	if (prtd->asp_link_lch[1] >= 0)
+		edma_free_slot(prtd->asp_link_lch[1]);
+	if (prtd->asp_master_lch >= 0)
+		edma_free_channel(prtd->asp_master_lch);
+	if (prtd->ram_link_lch >= 0)
+		edma_free_slot(prtd->ram_link_lch);
+	if (prtd->ram_link_lch2 >= 0)
+		edma_free_slot(prtd->ram_link_lch2);
+	if (prtd->ram_master_lch >= 0)
+		edma_free_channel(prtd->ram_master_lch);
 
 	kfree(prtd);
 
@@ -319,11 +732,11 @@ static struct snd_pcm_ops davinci_pcm_ops = {
 	.mmap = 	davinci_pcm_mmap,
 };
 
-static int davinci_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream)
+static int davinci_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream,
+		size_t size)
 {
 	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
 	struct snd_dma_buffer *buf = &substream->dma_buffer;
-	size_t size = davinci_pcm_hardware.buffer_bytes_max;
 
 	buf->dev.type = SNDRV_DMA_TYPE_DEV;
 	buf->dev.dev = pcm->card->dev;
@@ -348,6 +761,7 @@ static void davinci_pcm_free(struct snd_pcm *pcm)
 	int stream;
 
 	for (stream = 0; stream < 2; stream++) {
+		struct snd_dma_buffer *iram_dma;
 		substream = pcm->streams[stream].substream;
 		if (!substream)
 			continue;
@@ -359,6 +773,11 @@ static void davinci_pcm_free(struct snd_pcm *pcm)
 		dma_free_writecombine(pcm->card->dev, buf->bytes,
 				      buf->area, buf->addr);
 		buf->area = NULL;
+		iram_dma = (struct snd_dma_buffer *)buf->private_data;
+		if (iram_dma) {
+			sram_free(iram_dma->area, iram_dma->bytes);
+			kfree(iram_dma);
+		}
 	}
 }
 
@@ -376,14 +795,16 @@ static int davinci_pcm_new(struct snd_card *card,
 
 	if (dai->playback.channels_min) {
 		ret = davinci_pcm_preallocate_dma_buffer(pcm,
-			SNDRV_PCM_STREAM_PLAYBACK);
+			SNDRV_PCM_STREAM_PLAYBACK,
+			pcm_hardware_playback.buffer_bytes_max);
 		if (ret)
 			return ret;
 	}
 
 	if (dai->capture.channels_min) {
 		ret = davinci_pcm_preallocate_dma_buffer(pcm,
-			SNDRV_PCM_STREAM_CAPTURE);
+			SNDRV_PCM_STREAM_CAPTURE,
+			pcm_hardware_capture.buffer_bytes_max);
 		if (ret)
 			return ret;
 	}
diff --git a/sound/soc/davinci/davinci-pcm.h b/sound/soc/davinci/davinci-pcm.h
index 63d9625..7caba3c 100644
--- a/sound/soc/davinci/davinci-pcm.h
+++ b/sound/soc/davinci/davinci-pcm.h
@@ -21,6 +21,7 @@ struct davinci_pcm_dma_params {
 	int channel;			/* sync dma channel ID */
 	unsigned short acnt;
 	dma_addr_t dma_addr;		/* device physical address for DMA */
+	unsigned sram_size;
 	enum dma_event_q eventq_no;	/* event queue number */
 	unsigned char data_type;	/* xfer data type */
 	unsigned char convert_mono_stereo;
-- 
1.5.6.3

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Mark Brown]

On Mon, Aug 31, 2009 at 04:31:43PM -0700, Troy Kisky wrote:

> +	/*
> +	 * Allowing this is more efficient and eliminates left and right swaps
> +	 * caused by underruns, but will swap the left and right channels
> +	 * when compared to previous behavior.
> +	 */
> +	unsigned disable_channel_combine:1;

Last time you submitted this I suggested changing this to make the
default the other way round so that there's no breakage on existing
boards which aren't designed for this channel swap behaviour.  Is there
some reason not to do that?

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Troy Kisky]

Mark Brown wrote:
> On Mon, Aug 31, 2009 at 04:31:43PM -0700, Troy Kisky wrote:
> 
>> +	/*
>> +	 * Allowing this is more efficient and eliminates left and right swaps
>> +	 * caused by underruns, but will swap the left and right channels
>> +	 * when compared to previous behavior.
>> +	 */
>> +	unsigned disable_channel_combine:1;
> 
> Last time you submitted this I suggested changing this to make the
> default the other way round so that there's no breakage on existing
> boards which aren't designed for this channel swap behaviour.  Is there
> some reason not to do that?
> 
I think that it is better to make sure that they know the possible problems
disabling this may cause. Channels always swapped seems a lot better than
channels randomly swapped.

Troy

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Mark Brown]

On Tue, Sep 01, 2009 at 11:23:27AM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> > Last time you submitted this I suggested changing this to make the
> > default the other way round so that there's no breakage on existing
> > boards which aren't designed for this channel swap behaviour.  Is there
> > some reason not to do that?

> I think that it is better to make sure that they know the possible problems
> disabling this may cause. Channels always swapped seems a lot better than
> channels randomly swapped.

Of course, the other way of looking at it is that with the channel
swapping you've got guaranteed breakage - it sounds less good if you say
it that way :) How common are the channel swaps?

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Troy Kisky]

Mark Brown wrote:
> On Tue, Sep 01, 2009 at 11:23:27AM -0700, Troy Kisky wrote:
>> Mark Brown wrote:
> 
>>> Last time you submitted this I suggested changing this to make the
>>> default the other way round so that there's no breakage on existing
>>> boards which aren't designed for this channel swap behaviour.  Is there
>>> some reason not to do that?
> 
>> I think that it is better to make sure that they know the possible problems
>> disabling this may cause. Channels always swapped seems a lot better than
>> channels randomly swapped.
> 
> Of course, the other way of looking at it is that with the channel
> swapping you've got guaranteed breakage - it sounds less good if you say
> it that way :) How common are the channel swaps?
> 
I was getting a lot when playing videos. It mainly just sounded bad until I got
a stereo audio file with different frequency sine waves to understand better
what was happening. Then, you could hear the channels swap frequently, more
than once per second. If using sram, it is not an issue unless another device
is using the same TC. But sram isn't on by default either. And probably shouldn't
be since the newer chips don't have an underrun problem.


Does anyone else want to share their thoughts/ experience?

Thanks
Troy

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Mark Brown]

On Tue, Sep 01, 2009 at 12:19:26PM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> > Of course, the other way of looking at it is that with the channel
> > swapping you've got guaranteed breakage - it sounds less good if you say
> > it that way :) How common are the channel swaps?

> I was getting a lot when playing videos. It mainly just sounded bad until I got
> a stereo audio file with different frequency sine waves to understand better
> what was happening. Then, you could hear the channels swap frequently, more
> than once per second. If using sram, it is not an issue unless another device

So, very often under heavy load then?   That'd be consistent with simlar
problems on other devices.  Part of the trouble here is that things like
video can make the channel swap more noticable - if stereo is used to
track the movement of an object on screen the channel swap would stop
the effect tying in with the visuals.

> is using the same TC. But sram isn't on by default either. And probably shouldn't
> be since the newer chips don't have an underrun problem.

Hrm, that suggests that if it's enabled at all the default should depend
on the chip in use?

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Troy Kisky]

Mark Brown wrote:
> On Tue, Sep 01, 2009 at 12:19:26PM -0700, Troy Kisky wrote:
>> Mark Brown wrote:
> 
>>> Of course, the other way of looking at it is that with the channel
>>> swapping you've got guaranteed breakage - it sounds less good if you say
>>> it that way :) How common are the channel swaps?
> 
>> I was getting a lot when playing videos. It mainly just sounded bad until I got
>> a stereo audio file with different frequency sine waves to understand better
>> what was happening. Then, you could hear the channels swap frequently, more
>> than once per second. If using sram, it is not an issue unless another device
> 
> So, very often under heavy load then?   That'd be consistent with simlar
> problems on other devices.  Part of the trouble here is that things like
> video can make the channel swap more noticable - if stereo is used to
> track the movement of an object on screen the channel swap would stop
> the effect tying in with the visuals.
> 
>> is using the same TC. But sram isn't on by default either. And probably shouldn't
>> be since the newer chips don't have an underrun problem.
> 
> Hrm, that suggests that if it's enabled at all the default should depend
> on the chip in use?
> 
That seems unnecessarily complex to me. As long as platform data can
specify what you need, you'll eventually get it right. If tracking of
an object is always wrong because of a channel swap, that is easier
to notice, and debug, and fix, then if the tracking is only occasionally
wrong. I'd much rather have a repeatable bug. And most codecs do allow
you to swap the left and right channels. So, for most people, the fix
will not be to disable channel combining.


Troy

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Mark Brown]

On Tue, Sep 01, 2009 at 01:42:02PM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> >> is using the same TC. But sram isn't on by default either. And probably shouldn't
> >> be since the newer chips don't have an underrun problem.

> > Hrm, that suggests that if it's enabled at all the default should depend
> > on the chip in use?

> That seems unnecessarily complex to me. As long as platform data can
> specify what you need, you'll eventually get it right. If tracking of
> an object is always wrong because of a channel swap, that is easier
> to notice, and debug, and fix, then if the tracking is only occasionally
> wrong. I'd much rather have a repeatable bug. And most codecs do allow
> you to swap the left and right channels. So, for most people, the fix
> will not be to disable channel combining.

My thinking was that if the newer chips don't have the underrun issue at
all then it seems like a bad move to enable the workaround for them
since they're currently fine.  There should be no intermittent problems
if the underrun issue isn't present.

Re: [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element

[Troy Kisky]

Mark Brown wrote:
> On Tue, Sep 01, 2009 at 01:42:02PM -0700, Troy Kisky wrote:
>> Mark Brown wrote:
> 
>>>> is using the same TC. But sram isn't on by default either. And probably shouldn't
>>>> be since the newer chips don't have an underrun problem.
> 
>>> Hrm, that suggests that if it's enabled at all the default should depend
>>> on the chip in use?
> 
>> That seems unnecessarily complex to me. As long as platform data can
>> specify what you need, you'll eventually get it right. If tracking of
>> an object is always wrong because of a channel swap, that is easier
>> to notice, and debug, and fix, then if the tracking is only occasionally
>> wrong. I'd much rather have a repeatable bug. And most codecs do allow
>> you to swap the left and right channels. So, for most people, the fix
>> will not be to disable channel combining.
> 
> My thinking was that if the newer chips don't have the underrun issue at
> all then it seems like a bad move to enable the workaround for them
> since they're currently fine.  There should be no intermittent problems
> if the underrun issue isn't present.
> 
True, there shouldn't be a problem. However, from a efficiency point of
view, it is still better to have the workaround. Fewer memory accesses
may free a little bandwidth for other uses.

Troy

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Mark Brown]

On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> Rename variable master_lch to asp_master_lch
> Rename variable slave_lch to asp_link_lch[0]
> Rename local variables:
> 	count to asp_count
> 	src to asp_src
> 	dst to asp_dst

These other two patches are OK but I'll wait for after the merge window
to apply them partly due to the dependency on the merged tree and also
since it's very near to the merge window opening and there's already
been a fair bit of churn and testing with the DaVinci drivers for
2.6.32.

I'll probably also apply the first patch since nobody else seems to care
one way or another, but I would urge you to look at changing the default
for the platform data to at most select the workaround only on CPUs that
have problems with channel swapping - it's going to cause confusion for
people to have it on by default.

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

Mark Brown wrote:
> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
>> Rename variable master_lch to asp_master_lch
>> Rename variable slave_lch to asp_link_lch[0]
>> Rename local variables:
>> 	count to asp_count
>> 	src to asp_src
>> 	dst to asp_dst
> 
> These other two patches are OK but I'll wait for after the merge window
> to apply them partly due to the dependency on the merged tree and also
> since it's very near to the merge window opening and there's already
> been a fair bit of churn and testing with the DaVinci drivers for
> 2.6.32.
> 
> I'll probably also apply the first patch since nobody else seems to care
> one way or another, but I would urge you to look at changing the default
> for the platform data to at most select the workaround only on CPUs that
> have problems with channel swapping - it's going to cause confusion for
> people to have it on by default.
> 
I think the ones without a problem use davinci-mcasp instead of davinci-i2s
but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s.
But if someone else knows, speak up.

Thanks
Troy

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Mark Brown]

On Wed, Sep 02, 2009 at 05:15:08PM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> > for the platform data to at most select the workaround only on CPUs that
> > have problems with channel swapping - it's going to cause confusion for
> > people to have it on by default.

> I think the ones without a problem use davinci-mcasp instead of davinci-i2s
> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s.
> But if someone else knows, speak up.

Oh, right.  That wasn't clear from what you were saying about newer
chips not being affected - if the fix was top drop the original IP block
in favour of the McASP that's OK.

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Nori, Sekhar]

On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
> Mark Brown wrote:
> > On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
[...]
> >
> > I'll probably also apply the first patch since nobody else seems to care
> > one way or another, but I would urge you to look at changing the default
> > for the platform data to at most select the workaround only on CPUs that
> > have problems with channel swapping - it's going to cause confusion for
> > people to have it on by default.
> >
> I think the ones without a problem use davinci-mcasp instead of davinci-i2s
> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s.
> But if someone else knows, speak up.
>

In my experience too, the channel swap issues got reported only with ASP
(aka McBSP) and not with McASP.

The swap was almost always at the start of playback, and supposedly because
of the errata "2.1.5 ASP: Initialization Procedure When External Device is
Frame-Sync Master" in http://focus.ti.com/lit/er/sprz241l/sprz241l.pdf

Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the
OSS drivers but I don't recall the problem morphing into an "always channel
swapped" case.

Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and
see if it leads to channels being always swapped on that hardware as well.

One feedback we have received on this solution is that it does not work for
24-bit audio. In which case, implementing the workaround described in the
errata is the only way around.

Thanks,
Sekhar

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

Nori, Sekhar wrote:
> On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
>> Mark Brown wrote:
>>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> [...]
>>> I'll probably also apply the first patch since nobody else seems to care
>>> one way or another, but I would urge you to look at changing the default
>>> for the platform data to at most select the workaround only on CPUs that
>>> have problems with channel swapping - it's going to cause confusion for
>>> people to have it on by default.
>>>
>> I think the ones without a problem use davinci-mcasp instead of davinci-i2s
>> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s.
>> But if someone else knows, speak up.
>>
> 
> In my experience too, the channel swap issues got reported only with ASP
> (aka McBSP) and not with McASP.
> 
> The swap was almost always at the start of playback, and supposedly because
> of the errata "2.1.5 ASP: Initialization Procedure When External Device is
> Frame-Sync Master" in http://focus.ti.com/lit/er/sprz241l/sprz241l.pdf


This should have been fixed when I added

davinci_i2s_prepare because it will call davinci_mcbsp_start if the codec
is master, giving plenty of time for the first dma to be serviced.

So, all that ugly code in davinci_mcbsp_start to
"/* wait for any unexpected frame sync error to occur */"
can probably be removed. But since I didn't know the
reason for it, I haven't tried. If you can give this a try
I'd like to know the results.


> 
> Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the
> OSS drivers but I don't recall the problem morphing into an "always channel
> swapped" case.
> 
> Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and
> see if it leads to channels being always swapped on that hardware as well.

Yes, I have tested with dm644x, not evm. I haven't tried to hear the channel swap,
but I have no doubt that it is.

> 
> One feedback we have received on this solution is that it does not work for
> 24-bit audio. In which case, implementing the workaround described in the
> errata is the only way around.

Yes, you cannot shift more than 32 bits at once, so 48 bits is out.
Although 24 bit format would be easy to add, currently only 8, 16, and 32
are supported by davinci-i2s.

> 
> Thanks,
> Sekhar
> 
> 

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[David Brownell]

On Monday 31 August 2009, Troy Kisky wrote:
> -       int master_lch;         /* Master DMA channel */
> -       int slave_lch;          /* linked parameter RAM reload slot */
> +       int asp_master_lch;     /* Master DMA channel */
> +       int asp_link_lch[2];    /* asp parameter link channel, ping/pong */

If you're going to rename things, may I suggest getting
rid of "lch" ... use either "channel" or "link", since
those are the two basic hardware roles in EDMA.

The original "lch" ("logical channel") terminology was
an IMO misguded attempt to hide the distinction between
channels and links.  But links can not be used when a
channel is required, so that attempt was doomed to fail.
(Channels could double as links, but as a rule that's
not done since they're relatively scarce.)

- Dave

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

David Brownell wrote:
> On Monday 31 August 2009, Troy Kisky wrote:
>> -       int master_lch;         /* Master DMA channel */
>> -       int slave_lch;          /* linked parameter RAM reload slot */
>> +       int asp_master_lch;     /* Master DMA channel */
>> +       int asp_link_lch[2];    /* asp parameter link channel, ping/pong */
> 
> If you're going to rename things, may I suggest getting
> rid of "lch" ... use either "channel" or "link", since
> those are the two basic hardware roles in EDMA.
> 
> The original "lch" ("logical channel") terminology was
> an IMO misguded attempt to hide the distinction between
> channels and links.  But links can not be used when a
> channel is required, so that attempt was doomed to fail.
> (Channels could double as links, but as a rule that's
> not done since they're relatively scarce.)
> 
> - Dave
> 
> 
> 
I agree, but can that be a separate patch...

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[David Brownell]

On Thursday 03 September 2009, Troy Kisky wrote:
> David Brownell wrote:
> > On Monday 31 August 2009, Troy Kisky wrote:
> >> -       int master_lch;         /* Master DMA channel */
> >> -       int slave_lch;          /* linked parameter RAM reload slot */
> >> +       int asp_master_lch;     /* Master DMA channel */
> >> +       int asp_link_lch[2];    /* asp parameter link channel, ping/pong */
> > 
> > If you're going to rename things, may I suggest getting
> > rid of "lch" ... use either "channel" or "link", since
> > those are the two basic hardware roles in EDMA.
>
> I agree, but can that be a separate patch...

However you like; I'd just suggest that *one* rename-things
patch is normally preferred.  If this one's already in the
merge queue that might be awkward.

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

David Brownell wrote:
> On Thursday 03 September 2009, Troy Kisky wrote:
>> David Brownell wrote:
>>> On Monday 31 August 2009, Troy Kisky wrote:
>>>> -       int master_lch;         /* Master DMA channel */
>>>> -       int slave_lch;          /* linked parameter RAM reload slot */
>>>> +       int asp_master_lch;     /* Master DMA channel */
>>>> +       int asp_link_lch[2];    /* asp parameter link channel, ping/pong */
>>> If you're going to rename things, may I suggest getting
>>> rid of "lch" ... use either "channel" or "link", since
>>> those are the two basic hardware roles in EDMA.
>> I agree, but can that be a separate patch...
> 
> However you like; I'd just suggest that *one* rename-things
> patch is normally preferred.  If this one's already in the
> merge queue that might be awkward.
> 
Kevin,

Is the window too close, or is there time to change this?

Troy

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Nori, Sekhar]

On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote:
> Nori, Sekhar wrote:
> > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
> >> Mark Brown wrote:
> >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> > [...]
> >>> I'll probably also apply the first patch since nobody else seems to care
> >>> one way or another, but I would urge you to look at changing the default
> >>> for the platform data to at most select the workaround only on CPUs that
> >>> have problems with channel swapping - it's going to cause confusion for
> >>> people to have it on by default.
> >>>
> >> I think the ones without a problem use davinci-mcasp instead of davinci-i2s
> >> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s.
> >> But if someone else knows, speak up.
> >>

[...]

>
> >
> > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the
> > OSS drivers but I don't recall the problem morphing into an "always channel
> > swapped" case.
> >
> > Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and
> > see if it leads to channels being always swapped on that hardware as well.
>
> Yes, I have tested with dm644x, not evm. I haven't tried to hear the channel swap,
> but I have no doubt that it is.

I finally got around to testing your patch 1/3 on DM6446 EVM.

Without your patch, channel swap is quite easy to reproduce using audio
loopback:

arecord -fcd | aplay -fcd

The audio source is a PC which speaker balance set to an extreme.
By starting and stopping this command repeatedly, you can see the audio
moving from one channel to the other.

Applying your patch fixes this issue.

Also, I did not notice any permanent channel swap. Used aplay to play data
which was first left-only and then right-only. Plays the same way on a Linux PC.

I will test on couple of other platform using davinci-i2s (DM355 etc) before
acking the patch.

However, with or without your patch, I noticed a segmentation fault for the
first time the 'arecord | aplay' loop is created. There is no fault the second
time around. I haven't spent time debugging this yet.

root@arago:~# arecord -fcd | aplay -fcd
Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
Division by zero in kernel.
Backtrace:
[<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>] (dump_stack+0x18/0x1c)
 r7:86a60000 r6:c77cdae0 r5:00000040 r4:00000000
[<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20)
[<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10)
[<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baea0>] (davinci_pcm_
prepare+0x2c/0x58)
[<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>] (soc_pcm_prepare+0
x84/0x184)
 r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000
[<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>] (snd_pcm_do_prepare+0
x1c/0x34)
[<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>] (snd_pcm_action_sin
gle+0x40/0x7c)
 r5:c70d3900 r4:c030cbc8
[<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>] (snd_pcm_action_
nonatomic+0x58/0x70)
 r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900
[<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>] (snd_pcm_comm
on_ioctl1+0x814/0x1308)
 r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80
[<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>] (snd_pcm_captu
re_ioctl1+0x44c/0x470)
[<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] (snd_pcm_captu
re_ioctl+0x38/0x3c)
 r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80
[<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] (vfs_ioctl+0x34/
0x94)
[<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] (do_vfs_ioctl+0x56c/0x5c8)
 r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8
[<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] (sys_ioctl+0x40/0x64)
[<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] (ret_fast_syscall+0x0/0x2c)
 r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f
Division by zero in kernel.
Backtrace:
[<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>] (dump_stack+0x18/0x1c)
 r7:86a62000 r6:c77cdae0 r5:00000040 r4:00000000
[<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20)
[<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10)
[<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baec0>] (davinci_pcm_
prepare+0x4c/0x58)
[<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>] (soc_pcm_prepare+0
x84/0x184)
 r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000
[<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>] (snd_pcm_do_prepare+0
x1c/0x34)
[<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>] (snd_pcm_action_sin
gle+0x40/0x7c)
 r5:c70d3900 r4:c030cbc8
[<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>] (snd_pcm_action_
nonatomic+0x58/0x70)
 r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900
[<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>] (snd_pcm_comm
on_ioctl1+0x814/0x1308)
 r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80
[<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>] (snd_pcm_captu
re_ioctl1+0x44c/0x470)
[<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] (snd_pcm_captu
re_ioctl+0x38/0x3c)
 r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80
[<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] (vfs_ioctl+0x34/
0x94)
[<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] (do_vfs_ioctl+0x56c/0x5c8)
 r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8
[<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] (sys_ioctl+0x40/0x64)
[<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] (ret_fast_syscall+0x0/0x2c)
 r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f
Division by zero in kernel.
Playing WAVE 'stBacktrace: din' : Signed 16
 bit Little Endi[<c002b784>] an, Rate 44100 H(dump_backtrace+0x0/0x114) z, Stere
o
from [<c0248cf0>] (dump_stack+0x18/0x1c)
 r7:86a64000 r6:c77cdae0 r5:00000040 r4:00000000
[<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20)
[<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10)
[<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01bb0bc>] (davinci_pcm_
dma_irq+0x58/0x80)
[<c01bb064>] (davinci_pcm_dma_irq+0x0/0x80) from [<c0031904>] (dma_irq_handler+0
xec/0x12c)
 r5:00000003 r4:00000004
[<c0031818>] (dma_irq_handler+0x0/0x12c) from [<c0068ee8>] (handle_IRQ_event+0x4
4/0x114)
[<c0068ea4>] (handle_IRQ_event+0x0/0x114) from [<c006ab74>] (handle_edge_irq+0x1
48/0x1b4)
 r7:c7007440 r6:00000010 r5:c02ff060 r4:c6baa000
[<c006aa2c>] (handle_edge_irq+0x0/0x1b4) from [<c0027070>] (_text+0x70/0x8c)
 r7:00000002 r6:00000800 r5:00000000 r4:00000010
[<c0027000>] (_text+0x0/0x8c) from [<c0027aac>] (__irq_svc+0x4c/0x90)
Exception stack(0xc6babdf0 to 0xc6babe38)
bde0:                                     00000001 00000000 c6baa000 40000013
be00: 00000800 c76f2800 00000800 c70d3900 00000000 00000800 00000000 c6babe8c
be20: c6baa000 c6babe38 c01b16f4 c01b1714 40000013 ffffffff
 r5:fec48000 r4:ffffffff
[<c01b1548>] (snd_pcm_lib_read1+0x0/0x30c) from [<c01b1934>] (snd_pcm_lib_read+0
x60/0x6c)
[<c01b18d4>] (snd_pcm_lib_read+0x0/0x6c) from [<c01aee8c>] (snd_pcm_capture_ioct
l1+0xcc/0x470)
 r6:bec98ab4 r5:bec98ab4 r4:00000000
[<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] (snd_pcm_captu
re_ioctl+0x38/0x3c)
 r7:c77b6c80 r6:800c4151 r5:bec98ab4 r4:c77b6c80
[<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] (vfs_ioctl+0x34/
0x94)
[<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] (do_vfs_ioctl+0x56c/0x5c8)
 r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8
[<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] (sys_ioctl+0x40/0x64)
[<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] (ret_fast_syscall+0x0/0x2c)
 r7:00000036 r6:00000000 r5:0002a4b8 r4:0002a468
arecord: pcm_read:1529: read error: Input/output error
root@arago:~#

Thanks,
Sekhar

>
> >
> > One feedback we have received on this solution is that it does not work for
> > 24-bit audio. In which case, implementing the workaround described in the
> > errata is the only way around.
>
> Yes, you cannot shift more than 32 bits at once, so 48 bits is out.
> Although 24 bit format would be easy to add, currently only 8, 16, and 32
> are supported by davinci-i2s.
>
> >
> > Thanks,
> > Sekhar
> >
> >
>
>
>

Re: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Caglar Akyuz]

On Wednesday 09 September 2009 15:08:42 Nori, Sekhar wrote:
> On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote:
> > Nori, Sekhar wrote:
> > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
> > >> Mark Brown wrote:
> > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> > >
> > > [...]
> > >
> > >>> I'll probably also apply the first patch since nobody else seems to
> > >>> care one way or another, but I would urge you to look at changing the
> > >>> default for the platform data to at most select the workaround only
> > >>> on CPUs that have problems with channel swapping - it's going to
> > >>> cause confusion for people to have it on by default.
> > >>
> > >> I think the ones without a problem use davinci-mcasp instead of
> > >> davinci-i2s but share davinci-pcm. So, I don't know of any machines to
> > >> exclude in davinci-i2s. But if someone else knows, speak up.
>
> [...]
>
> > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue
> > > on the OSS drivers but I don't recall the problem morphing into an
> > > "always channel swapped" case.
> > >
> > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do
> > > that and see if it leads to channels being always swapped on that
> > > hardware as well.
> >
> > Yes, I have tested with dm644x, not evm. I haven't tried to hear the
> > channel swap, but I have no doubt that it is.
>
> I finally got around to testing your patch 1/3 on DM6446 EVM.
>
> Without your patch, channel swap is quite easy to reproduce using audio
> loopback:
>
> arecord -fcd | aplay -fcd
>
> The audio source is a PC which speaker balance set to an extreme.
> By starting and stopping this command repeatedly, you can see the audio
> moving from one channel to the other.
>
> Applying your patch fixes this issue.
>
> Also, I did not notice any permanent channel swap. Used aplay to play data
> which was first left-only and then right-only. Plays the same way on a
> Linux PC.
>
> I will test on couple of other platform using davinci-i2s (DM355 etc)
> before acking the patch.
>
> However, with or without your patch, I noticed a segmentation fault for the
> first time the 'arecord | aplay' loop is created. There is no fault the
> second time around. I haven't spent time debugging this yet.
>

This is due to these lines in davinci_pcm_enqueue_dma:

	data_type = prtd->params->data_type;	
	count = period_size / data_type;

First time running data_type is set to 0. This is a long time issue, I 
remember modifying my kernels to check for zero before this division..

Regards,
Caglar

> root@arago:~# arecord -fcd | aplay -fcd
> Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
> Division by zero in kernel.
> Backtrace:
> [<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>]
> (dump_stack+0x18/0x1c) r7:86a60000 r6:c77cdae0 r5:00000040 r4:00000000
> [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20)
> [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10)
> [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baea0>]
> (davinci_pcm_ prepare+0x2c/0x58)
> [<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>]
> (soc_pcm_prepare+0 x84/0x184)
>  r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000
> [<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>]
> (snd_pcm_do_prepare+0 x1c/0x34)
> [<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>]
> (snd_pcm_action_sin gle+0x40/0x7c)
>  r5:c70d3900 r4:c030cbc8
> [<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>]
> (snd_pcm_action_ nonatomic+0x58/0x70)
>  r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900
> [<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>]
> (snd_pcm_comm on_ioctl1+0x814/0x1308)
>  r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80
> [<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>]
> (snd_pcm_captu re_ioctl1+0x44c/0x470)
> [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>]
> (snd_pcm_captu re_ioctl+0x38/0x3c)
>  r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80
> [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>]
> (vfs_ioctl+0x34/ 0x94)
> [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>]
> (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8
> [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>]
> (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>]
> (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f
> Division by zero in kernel.
> Backtrace:
> [<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>]
> (dump_stack+0x18/0x1c) r7:86a62000 r6:c77cdae0 r5:00000040 r4:00000000
> [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20)
> [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10)
> [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baec0>]
> (davinci_pcm_ prepare+0x4c/0x58)
> [<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>]
> (soc_pcm_prepare+0 x84/0x184)
>  r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000
> [<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>]
> (snd_pcm_do_prepare+0 x1c/0x34)
> [<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>]
> (snd_pcm_action_sin gle+0x40/0x7c)
>  r5:c70d3900 r4:c030cbc8
> [<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>]
> (snd_pcm_action_ nonatomic+0x58/0x70)
>  r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900
> [<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>]
> (snd_pcm_comm on_ioctl1+0x814/0x1308)
>  r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80
> [<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>]
> (snd_pcm_captu re_ioctl1+0x44c/0x470)
> [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>]
> (snd_pcm_captu re_ioctl+0x38/0x3c)
>  r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80
> [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>]
> (vfs_ioctl+0x34/ 0x94)
> [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>]
> (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8
> [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>]
> (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>]
> (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f
> Division by zero in kernel.
> Playing WAVE 'stBacktrace: din' : Signed 16
>  bit Little Endi[<c002b784>] an, Rate 44100 H(dump_backtrace+0x0/0x114) z,
> Stere o
> from [<c0248cf0>] (dump_stack+0x18/0x1c)
>  r7:86a64000 r6:c77cdae0 r5:00000040 r4:00000000
> [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20)
> [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10)
> [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01bb0bc>]
> (davinci_pcm_ dma_irq+0x58/0x80)
> [<c01bb064>] (davinci_pcm_dma_irq+0x0/0x80) from [<c0031904>]
> (dma_irq_handler+0 xec/0x12c)
>  r5:00000003 r4:00000004
> [<c0031818>] (dma_irq_handler+0x0/0x12c) from [<c0068ee8>]
> (handle_IRQ_event+0x4 4/0x114)
> [<c0068ea4>] (handle_IRQ_event+0x0/0x114) from [<c006ab74>]
> (handle_edge_irq+0x1 48/0x1b4)
>  r7:c7007440 r6:00000010 r5:c02ff060 r4:c6baa000
> [<c006aa2c>] (handle_edge_irq+0x0/0x1b4) from [<c0027070>]
> (_text+0x70/0x8c) r7:00000002 r6:00000800 r5:00000000 r4:00000010
> [<c0027000>] (_text+0x0/0x8c) from [<c0027aac>] (__irq_svc+0x4c/0x90)
> Exception stack(0xc6babdf0 to 0xc6babe38)
> bde0:                                     00000001 00000000 c6baa000
> 40000013 be00: 00000800 c76f2800 00000800 c70d3900 00000000 00000800
> 00000000 c6babe8c be20: c6baa000 c6babe38 c01b16f4 c01b1714 40000013
> ffffffff
>  r5:fec48000 r4:ffffffff
> [<c01b1548>] (snd_pcm_lib_read1+0x0/0x30c) from [<c01b1934>]
> (snd_pcm_lib_read+0 x60/0x6c)
> [<c01b18d4>] (snd_pcm_lib_read+0x0/0x6c) from [<c01aee8c>]
> (snd_pcm_capture_ioct l1+0xcc/0x470)
>  r6:bec98ab4 r5:bec98ab4 r4:00000000
> [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>]
> (snd_pcm_captu re_ioctl+0x38/0x3c)
>  r7:c77b6c80 r6:800c4151 r5:bec98ab4 r4:c77b6c80
> [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>]
> (vfs_ioctl+0x34/ 0x94)
> [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>]
> (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8
> [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>]
> (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>]
> (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:00000000 r5:0002a4b8 r4:0002a468
> arecord: pcm_read:1529: read error: Input/output error
> root@arago:~#
>
> Thanks,
> Sekhar
>
> > > One feedback we have received on this solution is that it does not work
> > > for 24-bit audio. In which case, implementing the workaround described
> > > in the errata is the only way around.
> >
> > Yes, you cannot shift more than 32 bits at once, so 48 bits is out.
> > Although 24 bit format would be easy to add, currently only 8, 16, and 32
> > are supported by davinci-i2s.
> >
> > > Thanks,
> > > Sekhar
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Nori, Sekhar]

On Wed, Sep 09, 2009 at 18:03:58, Caglar Akyuz wrote:
> On Wednesday 09 September 2009 15:08:42 Nori, Sekhar wrote:

[...]

> >
> > However, with or without your patch, I noticed a segmentation fault for the
> > first time the 'arecord | aplay' loop is created. There is no fault the
> > second time around. I haven't spent time debugging this yet.
> >
>
> This is due to these lines in davinci_pcm_enqueue_dma:
>
>       data_type = prtd->params->data_type;
>       count = period_size / data_type;
>
> First time running data_type is set to 0. This is a long time issue, I
> remember modifying my kernels to check for zero before this division..

Ah, thanks! Planning to submit a fix for this? We can work on the patch
if you are busy otherwise.

Regards,
Sekhar

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Narnakaje, Snehaprabha]

> -----Original Message-----
> From: davinci-linux-open-source-bounces-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> [mailto:davinci-linux-open-source-bounces-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org] On Behalf
> Of Nori, Sekhar
> Sent: Wednesday, September 09, 2009 9:06 AM
> To: caglarakyuz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org; Mark Brown
> Subject: RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables
> in prep for ping/pong
> 
> On Wed, Sep 09, 2009 at 18:03:58, Caglar Akyuz wrote:
> > On Wednesday 09 September 2009 15:08:42 Nori, Sekhar wrote:
> 
> [...]
> 
> > >
> > > However, with or without your patch, I noticed a segmentation fault
> for the
> > > first time the 'arecord | aplay' loop is created. There is no fault
> the
> > > second time around. I haven't spent time debugging this yet.
> > >
> >
> > This is due to these lines in davinci_pcm_enqueue_dma:
> >
> >       data_type = prtd->params->data_type;
> >       count = period_size / data_type;
> >
> > First time running data_type is set to 0. This is a long time issue, I
> > remember modifying my kernels to check for zero before this division..
> 
> Ah, thanks! Planning to submit a fix for this? We can work on the patch
> if you are busy otherwise.

There was a patch from Arun Mani on Arago linux-davinci staging tree -

http://arago-project.org/git/projects/?p=linux-davinci.git;a=commitdiff;h=0025e7d8024e9b03bada7a1d65f1aa7b7dfec061

Thanks
Sneha

> 
> Regards,
> Sekhar
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Mark Brown]

On Wed, Sep 09, 2009 at 08:22:49AM -0500, Narnakaje, Snehaprabha wrote:

> There was a patch from Arun Mani on Arago linux-davinci staging tree -
> 
> http://arago-project.org/git/projects/?p=linux-davinci.git;a=commitdiff;h=0025e7d8024e9b03bada7a1d65f1aa7b7dfec061

Sounds like something that should be submitted to mainline for 2.6.32.

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Nori, Sekhar]

On Wed, Sep 09, 2009 at 17:38:42, Nori, Sekhar wrote:
> On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote:
> > Nori, Sekhar wrote:
> > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
> > >> Mark Brown wrote:
> > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> > > [...]
> > >>> I'll probably also apply the first patch since nobody else seems to care
> > >>> one way or another, but I would urge you to look at changing the default
> > >>> for the platform data to at most select the workaround only on CPUs that
> > >>> have problems with channel swapping - it's going to cause confusion for
> > >>> people to have it on by default.
> > >>>
> > >> I think the ones without a problem use davinci-mcasp instead of davinci-i2s
> > >> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s.
> > >> But if someone else knows, speak up.
> > >>
>
> [...]
>
> >
> > >
> > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the
> > > OSS drivers but I don't recall the problem morphing into an "always channel
> > > swapped" case.
> > >
> > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and
> > > see if it leads to channels being always swapped on that hardware as well.
> >
> > Yes, I have tested with dm644x, not evm. I haven't tried to hear the channel swap,
> > but I have no doubt that it is.
>
> I finally got around to testing your patch 1/3 on DM6446 EVM.
>
> Without your patch, channel swap is quite easy to reproduce using audio
> loopback:
>
> arecord -fcd | aplay -fcd
>
> The audio source is a PC which speaker balance set to an extreme.
> By starting and stopping this command repeatedly, you can see the audio
> moving from one channel to the other.
>
> Applying your patch fixes this issue.
>
> Also, I did not notice any permanent channel swap. Used aplay to play data
> which was first left-only and then right-only. Plays the same way on a Linux PC.
>
> I will test on couple of other platform using davinci-i2s (DM355 etc) before
> acking the patch.

Even on the DM355 EVM this patch fixes the random channel swap on audio
loopback 'arecord -fcd | aplay -fcd'.

However, on playback, the channels do seem to be permanently swapped. This cannot
surely be blamed on this patch because, without it, the channels get randomly
swapped.

Since this was not observed on DM6446 EVM, I have to see if the DM355 EVM
hardware swaps the channels. I briefly compared the schematics of the two EVMs,
but nothing seems to be wrong there.

Troy, do you have any theory yet on why your patch should permanently swap
channels?

Anyway, the patch surely improves the situation on the EVMs, so, for patch 1/3
of this series:

Tested-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
[tested on DM6446 and DM355 EVMs]

Thanks,
Sekhar

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Mani, Arun]

Hi Sekhar,
DM355 uses MCBSP 1 instead of 0 as in DM644x. Do you think this will affect the channel swapping? Another thing is I was able to run aplay and arecord without Troy's patches, but seems like gstreamer still fails with his patches.

Thanks,
Arun

> -----Original Message-----
> From: davinci-linux-open-source-bounces+avm=ti.com-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> [mailto:davinci-linux-open-source-bounces+avm=ti.com-VycZQUHpC/PFrsHnngEfi/UYCXxBIdiY@public.gmane.orgom]
> On Behalf Of Nori, Sekhar
> Sent: Tuesday, September 29, 2009 6:46 AM
> To: Nori, Sekhar; Troy Kisky
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org; Mark Brown; alsa-
> devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org
> Subject: RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables
> in prep for ping/pong
> 
> On Wed, Sep 09, 2009 at 17:38:42, Nori, Sekhar wrote:
> > On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote:
> > > Nori, Sekhar wrote:
> > > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
> > > >> Mark Brown wrote:
> > > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> > > > [...]
> > > >>> I'll probably also apply the first patch since nobody else seems
> to care
> > > >>> one way or another, but I would urge you to look at changing the
> default
> > > >>> for the platform data to at most select the workaround only on
> CPUs that
> > > >>> have problems with channel swapping - it's going to cause
> confusion for
> > > >>> people to have it on by default.
> > > >>>
> > > >> I think the ones without a problem use davinci-mcasp instead of
> davinci-i2s
> > > >> but share davinci-pcm. So, I don't know of any machines to exclude
> in davinci-i2s.
> > > >> But if someone else knows, speak up.
> > > >>
> >
> > [...]
> >
> > >
> > > >
> > > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that
> issue on the
> > > > OSS drivers but I don't recall the problem morphing into an "always
> channel
> > > > swapped" case.
> > > >
> > > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do
> that and
> > > > see if it leads to channels being always swapped on that hardware as
> well.
> > >
> > > Yes, I have tested with dm644x, not evm. I haven't tried to hear the
> channel swap,
> > > but I have no doubt that it is.
> >
> > I finally got around to testing your patch 1/3 on DM6446 EVM.
> >
> > Without your patch, channel swap is quite easy to reproduce using audio
> > loopback:
> >
> > arecord -fcd | aplay -fcd
> >
> > The audio source is a PC which speaker balance set to an extreme.
> > By starting and stopping this command repeatedly, you can see the audio
> > moving from one channel to the other.
> >
> > Applying your patch fixes this issue.
> >
> > Also, I did not notice any permanent channel swap. Used aplay to play
> data
> > which was first left-only and then right-only. Plays the same way on a
> Linux PC.
> >
> > I will test on couple of other platform using davinci-i2s (DM355 etc)
> before
> > acking the patch.
> 
> Even on the DM355 EVM this patch fixes the random channel swap on audio
> loopback 'arecord -fcd | aplay -fcd'.
> 
> However, on playback, the channels do seem to be permanently swapped. This
> cannot
> surely be blamed on this patch because, without it, the channels get
> randomly
> swapped.
> 
> Since this was not observed on DM6446 EVM, I have to see if the DM355 EVM
> hardware swaps the channels. I briefly compared the schematics of the two
> EVMs,
> but nothing seems to be wrong there.
> 
> Troy, do you have any theory yet on why your patch should permanently swap
> channels?
> 
> Anyway, the patch surely improves the situation on the EVMs, so, for patch
> 1/3
> of this series:
> 
> Tested-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> [tested on DM6446 and DM355 EVMs]
> 
> Thanks,
> Sekhar
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

> 
> Troy, do you have any theory yet on why your patch should permanently swap
> channels?
> 
Memory in 16 bit samples
L1,R1,L2,R2

Shifting out in 16 bit samples
L1,R1,L2,R2


Memory in 32 bit samples
R1L1,R2L2,R3L3

Shifting out in 32 bit samples
R1L1,R2L2

Codec sees 16 bit samples
R1,L1,R2,L2

Re: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Steve Chen]

On Tue, 2009-09-29 at 15:09 -0700, Troy Kisky wrote:
> > 
> > Troy, do you have any theory yet on why your patch should permanently swap
> > channels?
> > 
> Memory in 16 bit samples
> L1,R1,L2,R2
> 
> Shifting out in 16 bit samples
> L1,R1,L2,R2
> 
> 
> Memory in 32 bit samples
> R1L1,R2L2,R3L3
> 
> Shifting out in 32 bit samples
> R1L1,R2L2
> 
> Codec sees 16 bit samples
> R1,L1,R2,L2
> 

If that is the case, wouldn't we see the channel swap on DM644x as well?
>From Sekhar's earlier e-mail, the channel swap appears specific to DM355
EVM.

Steve

Re: [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Troy Kisky]

Steve Chen wrote:
> On Tue, 2009-09-29 at 15:09 -0700, Troy Kisky wrote:
>>> Troy, do you have any theory yet on why your patch should permanently swap
>>> channels?
>>>
>> Memory in 16 bit samples
>> L1,R1,L2,R2
>>
>> Shifting out in 16 bit samples
>> L1,R1,L2,R2
>>
>>
>> Memory in 32 bit samples
>> R1L1,R2L2,R3L3
>>
>> Shifting out in 32 bit samples
>> R1L1,R2L2
>>
>> Codec sees 16 bit samples
>> R1,L1,R2,L2
>>
> 
> If that is the case, wouldn't we see the channel swap on DM644x as well?
>>From Sekhar's earlier e-mail, the channel swap appears specific to DM355
> EVM.
> 
> Steve
> 
> 
Yes, unless an underrun causes the 1st 16 bit sample to be delayed when transmitting
out 16 bit samples at a time. That is why the 16 bit shift leads to random swaps
and 32 bit shifts leads to always swapped. When an underrun happens with 32 bit shifts
the last sample (both left AND right) is merely shifted again. When an underrun happens
with 16 bit shifts only the immediately preceding left OR right channel sample is repeated
leading to a random swap.


Troy

Re: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Steve Chen]

On Tue, 2009-09-29 at 15:31 -0700, Troy Kisky wrote:
> Steve Chen wrote:
> > On Tue, 2009-09-29 at 15:09 -0700, Troy Kisky wrote:
> >>> Troy, do you have any theory yet on why your patch should permanently swap
> >>> channels?
> >>>
> >> Memory in 16 bit samples
> >> L1,R1,L2,R2
> >>
> >> Shifting out in 16 bit samples
> >> L1,R1,L2,R2
> >>
> >>
> >> Memory in 32 bit samples
> >> R1L1,R2L2,R3L3
> >>
> >> Shifting out in 32 bit samples
> >> R1L1,R2L2
> >>
> >> Codec sees 16 bit samples
> >> R1,L1,R2,L2
> >>
> > 
> > If that is the case, wouldn't we see the channel swap on DM644x as well?
> >>From Sekhar's earlier e-mail, the channel swap appears specific to DM355
> > EVM.
> > 
> > Steve
> > 
> > 
> Yes, unless an underrun causes the 1st 16 bit sample to be delayed when transmitting
> out 16 bit samples at a time. That is why the 16 bit shift leads to random swaps
> and 32 bit shifts leads to always swapped. When an underrun happens with 32 bit shifts
> the last sample (both left AND right) is merely shifted again. When an underrun happens
> with 16 bit shifts only the immediately preceding left OR right channel sample is repeated
> leading to a random swap.
> 

I see, so "random" channel swap only happens for 16 bit shift and only
after an underrun.  So if underrun does not happen, we should not see
any channel swap right?

As for the 32 bit shift, if the channels are always swapped, can we not
configure the DMA engine to automatically swap the upper and lower 16
bits to get around the issue?  Am I missing something?

Steve

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Nori, Sekhar]

Hi Arun,

On Tue, Sep 29, 2009 at 20:51:49, Mani, Arun wrote:
> Hi Sekhar,
> DM355 uses MCBSP 1 instead of 0 as in DM644x. Do you think this will affect the channel
> swapping?

I don't think this would have a bearing.

> Another thing is I was able to run aplay and arecord without Troy's patches, but seems
> like gstreamer still fails with his patches.

I haven't tried gstreamer, but that's surprising. Can you post
the error logs? Maybe someone familiar with gstreamer could comment.

Thanks,
Sekhar

>
> Thanks,
> Arun
>
> > -----Original Message-----
> > From: davinci-linux-open-source-bounces+avm=ti.com-VycZQUHpC/PFrsHnngEfi/UYCXxBIdiY@public.gmane.orgom
> > [mailto:davinci-linux-open-source-bounces+avm=ti.com-VycZQUHpC/PFrsHnngEfiw@public.gmane.org.com]
> > On Behalf Of Nori, Sekhar
> > Sent: Tuesday, September 29, 2009 6:46 AM
> > To: Nori, Sekhar; Troy Kisky
> > Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org; Mark Brown; alsa-
> > devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org
> > Subject: RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables
> > in prep for ping/pong
> >
> > On Wed, Sep 09, 2009 at 17:38:42, Nori, Sekhar wrote:
> > > On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote:
> > > > Nori, Sekhar wrote:
> > > > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote:
> > > > >> Mark Brown wrote:
> > > > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote:
> > > > > [...]
> > > > >>> I'll probably also apply the first patch since nobody else seems
> > to care
> > > > >>> one way or another, but I would urge you to look at changing the
> > default
> > > > >>> for the platform data to at most select the workaround only on
> > CPUs that
> > > > >>> have problems with channel swapping - it's going to cause
> > confusion for
> > > > >>> people to have it on by default.
> > > > >>>
> > > > >> I think the ones without a problem use davinci-mcasp instead of
> > davinci-i2s
> > > > >> but share davinci-pcm. So, I don't know of any machines to exclude
> > in davinci-i2s.
> > > > >> But if someone else knows, speak up.
> > > > >>
> > >
> > > [...]
> > >
> > > >
> > > > >
> > > > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that
> > issue on the
> > > > > OSS drivers but I don't recall the problem morphing into an "always
> > channel
> > > > > swapped" case.
> > > > >
> > > > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do
> > that and
> > > > > see if it leads to channels being always swapped on that hardware as
> > well.
> > > >
> > > > Yes, I have tested with dm644x, not evm. I haven't tried to hear the
> > channel swap,
> > > > but I have no doubt that it is.
> > >
> > > I finally got around to testing your patch 1/3 on DM6446 EVM.
> > >
> > > Without your patch, channel swap is quite easy to reproduce using audio
> > > loopback:
> > >
> > > arecord -fcd | aplay -fcd
> > >
> > > The audio source is a PC which speaker balance set to an extreme.
> > > By starting and stopping this command repeatedly, you can see the audio
> > > moving from one channel to the other.
> > >
> > > Applying your patch fixes this issue.
> > >
> > > Also, I did not notice any permanent channel swap. Used aplay to play
> > data
> > > which was first left-only and then right-only. Plays the same way on a
> > Linux PC.
> > >
> > > I will test on couple of other platform using davinci-i2s (DM355 etc)
> > before
> > > acking the patch.
> >
> > Even on the DM355 EVM this patch fixes the random channel swap on audio
> > loopback 'arecord -fcd | aplay -fcd'.
> >
> > However, on playback, the channels do seem to be permanently swapped. This
> > cannot
> > surely be blamed on this patch because, without it, the channels get
> > randomly
> > swapped.
> >
> > Since this was not observed on DM6446 EVM, I have to see if the DM355 EVM
> > hardware swaps the channels. I briefly compared the schematics of the two
> > EVMs,
> > but nothing seems to be wrong there.
> >
> > Troy, do you have any theory yet on why your patch should permanently swap
> > channels?
> >
> > Anyway, the patch surely improves the situation on the EVMs, so, for patch
> > 1/3
> > of this series:
> >
> > Tested-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> > [tested on DM6446 and DM355 EVMs]
> >
> > Thanks,
> > Sekhar
> > _______________________________________________
> > Davinci-linux-open-source mailing list
> > Davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>

RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables in prep for ping/pong

[Steve Chen]

On Thu, 2009-10-01 at 23:43 +0530, Nori, Sekhar wrote:
> Hi Arun,
> 
> On Tue, Sep 29, 2009 at 20:51:49, Mani, Arun wrote:
> > Hi Sekhar,
> > DM355 uses MCBSP 1 instead of 0 as in DM644x. Do you think this will affect the channel
> > swapping?
> 
> I don't think this would have a bearing.
> 
> > Another thing is I was able to run aplay and arecord without Troy's patches, but seems
> > like gstreamer still fails with his patches.
> 
> I haven't tried gstreamer, but that's surprising. Can you post
> the error logs? Maybe someone familiar with gstreamer could comment.
> 
Arun,

Do you observe any errors when playing sound file through ALSA's  OSS
emulation layer (ie. using waeplay or similar applications)?  Based on
the options, gstreamer may (or may not) use the OSS emulation.  May want
to try it out to see if that makes a difference.

Regards,

Steve