[alsa-devel] [PATCH 1/3] ASoC: DaVinci: i2s, reduce underruns by combining into 1 element
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;
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);
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;
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.
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
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.
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
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
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
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@ti.com [tested on DM6446 and DM355 EVMs]
Thanks, Sekhar
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@linux.davincidsp.com [mailto:davinci-linux-open-source-bounces+avm=ti.com@linux.davincidsp.com] On Behalf Of Nori, Sekhar Sent: Tuesday, September 29, 2009 6:46 AM To: Nori, Sekhar; Troy Kisky Cc: davinci-linux-open-source@linux.davincidsp.com; Mark Brown; alsa- devel@alsa-project.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@ti.com [tested on DM6446 and DM355 EVMs]
Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
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@linux.davincidsp.com [mailto:davinci-linux-open-source-bounces+avm=ti.com@linux.davincidsp.com] On Behalf Of Nori, Sekhar Sent: Tuesday, September 29, 2009 6:46 AM To: Nori, Sekhar; Troy Kisky Cc: davinci-linux-open-source@linux.davincidsp.com; Mark Brown; alsa- devel@alsa-project.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@ti.com [tested on DM6446 and DM355 EVMs]
Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
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
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
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
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
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
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
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...
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.
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
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?
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
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?
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
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?
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
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.
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
participants (6)
-
David Brownell -
Mani, Arun -
Mark Brown -
Nori, Sekhar -
Steve Chen -
Troy Kisky