[alsa-devel] [PATCH 0/6] ASoC: davinci-pcm: some fixes and convert to BATCH
This patchset is a collection of changes for the davinci-pcm driver that were accumulated during previous forays into the davinci-mcasp and davinci-pcm drivers [1].
The first three patches are minor changes that cleanup the ping-pong DMA params setup, expand the davinci-pcm reported format and also its maximum channels.
The fourth patch in the series corrects an audible glitch that can be heard on da850evm's McASP when ping-pong buffers are enabled. The glitch occurs only on the second and susequent playbacks, not the first. Reversing the order in which the dma channels are started and moving the start to the trigger function from the prepare function fixes the glitch.
The fifth and sixth patches in the series convert the davinci-pcm driver to BATCH mode.
I noticed that the davinci-pcm pointer function is returning a value that is retrieved from within the running EDMA channel's state -- which is contrary to the warning in the comments of arch/arm/mach-davinci/dma.c:edma_get_position():
" * Returns current source and destination addresses for a particular * parameter RAM slot. Its channel should not be active when this is called. "
To that end, we begin with some refactoring to centralize the modification of the davinci_runtime_data.period member.
Followed by wholesale replacement of davinci_pcm_pointer() function so that it returns the position as reported by the last update of the period member -- along with corresponding changes to davinci_pcm_enqueue_dma() davinci_pcm_dma_irq() davinci_pcm_trigger() and reporting SNDRV_PCM_INFO_BATCH on playback and capture.
The series has been tested with and without ping-pong buffers enabled [1]; furthermore, the pointer behaviour was inspected using both XRUN_DEBUG_PERIODUPDATE and XRUN_DEBUG_HWPTRUPDATE as well as the audio checked with headphones.
Playback was tested with:
echo 24 > /proc/asound/EVM/pcm0p/xrun_debug aplay -d 1 -t wav -D "default:CARD=EVM" test.wav echo 0 > /proc/asound/EVM/pcm0p/xrun_debug
and capture was tested with (while playing test.wav on my PC into the capture input on the da850evm):
#!/bin/bash echo 24 > /proc/asound/EVM/pcm0c/xrun_debug arecord -d 2 -t wav -D "default:CARD=EVM" -f dat - |aplay -t wav echo 0 > /proc/asound/EVM/pcm0c/xrun_debug
Playback behaviour before (similar both with and without ping-pong buffers) ========================= hwptr_update: pcmC0D0p:0: pos=42/2048/32768, hwptr=42/0/42/0 hwptr_update: pcmC0D0p:0: pos=369/2048/32768, hwptr=327/42/369/0 hwptr_update: pcmC0D0p:0: pos=709/2048/32768, hwptr=340/369/709/0 hwptr_update: pcmC0D0p:0: pos=994/2048/32768, hwptr=285/709/994/0 hwptr_update: pcmC0D0p:0: pos=1367/2048/32768, hwptr=373/994/1367/0 hwptr_update: pcmC0D0p:0: pos=1729/2048/32768, hwptr=362/1367/1729/0 hwptr_update: pcmC0D0p:0: pos=2037/2048/32768, hwptr=308/1729/2037/0 period_update: pcmC0D0p:0: pos=2335/2048/32768, hwptr=298/2037/2335/0 hwptr_update: pcmC0D0p:0: pos=2762/2048/32768, hwptr=427/2335/2762/0 hwptr_update: pcmC0D0p:0: pos=3147/2048/32768, hwptr=385/2762/3147/0 hwptr_update: pcmC0D0p:0: pos=3456/2048/32768, hwptr=309/3147/3456/0 hwptr_update: pcmC0D0p:0: pos=3851/2048/32768, hwptr=395/3456/3851/0 period_update: pcmC0D0p:0: pos=4150/2048/32768, hwptr=299/3851/4150/0 [...]
Playback behaviour after (identical both with and without ping-pong buffers) ======================== hwptr_update: pcmC0D0p:0: pos=0/2048/32768, hwptr=0/0/0/0 period_update: pcmC0D0p:0: pos=2048/2048/32768, hwptr=2048/0/2048/0 hwptr_update: pcmC0D0p:0: pos=2048/2048/32768, hwptr=0/2048/2048/0 period_update: pcmC0D0p:0: pos=4096/2048/32768, hwptr=2048/2048/4096/0 [...]
[Capture behaviour was very similarly modified by this patchset]
The playback behaviour before shows inconsistent increments of the pointers in syscall and interrupt context -- this is due to the pointer function peeking into the parameters of the dma channel.
The playback behaviour afterwards shows regular increments from interrupt context. Which is to be expected since the driver now reports an incremented pointer after each dma completion.
It seems that the conversion to BATCH mode is beneficial -- other than obeying the warning comment in dma.c -- since there appear to be fewer syscalls per execution of aplay.
[1] http://article.gmane.org/gmane.linux.alsa.devel/85122
Ben Gardiner (6): ASoC: davinci-pcm: trivial: make ping-pong params setup symmetrical ASoC: davinci-pcm: expand the .formats ASoC: davinci-pcm: increase the maximum channels ASoC: davinci-pcm: fix audible glitch on 2nd ping-pong playback ASoC: davinci-pcm: extract period elapsed functions ASoC: davinci-pcm: convert to BATCH mode
sound/soc/davinci/davinci-pcm.c | 127 ++++++++++++++++++++------------------- 1 files changed, 65 insertions(+), 62 deletions(-)
The setup of the pong channel uses EDMA_CHAN_SLOT instead of & 0x3f as the setup of the ping channel does.
Make the setup of ping and pong symmetric. There is no functional change introduced by this patch.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca
--- sound/soc/davinci/davinci-pcm.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index 9d35b8c..0d04e0c 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -425,7 +425,8 @@ static int request_ping_pong(struct snd_pcm_substream *substream,
edma_read_slot(link, &prtd->asp_params); prtd->asp_params.opt &= ~(TCCMODE | EDMA_TCC(0x3f) | TCINTEN); - prtd->asp_params.opt |= TCCHEN | EDMA_TCC(prtd->ram_channel & 0x3f); + prtd->asp_params.opt |= TCCHEN | + EDMA_TCC(prtd->ram_channel & 0x3f); edma_write_slot(link, &prtd->asp_params);
/* pong */ @@ -439,7 +440,7 @@ static int request_ping_pong(struct snd_pcm_substream *substream, prtd->asp_params.opt &= ~(TCCMODE | EDMA_TCC(0x3f)); /* interrupt after every pong completion */ prtd->asp_params.opt |= TCINTEN | TCCHEN | - EDMA_TCC(EDMA_CHAN_SLOT(prtd->ram_channel)); + EDMA_TCC(prtd->ram_channel & 0x3f); edma_write_slot(link, &prtd->asp_params);
/* ram */
Based on the data_type test in ping_pong_dma_setup, davinci-pcm is capable of handling data of width up to and including 32bits.
" if ((data_type == 0) || (data_type > 4)) { printk(KERN_ERR "%s: data_type=%i\n", __func__, data_type); return -EINVAL; } "
Update the .format member of the snd_pcm_hardware instances it registers to reflect this capability.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca
--- sound/soc/davinci/davinci-pcm.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index 0d04e0c..1e47267 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -46,11 +46,27 @@ static void print_buf_info(int slot, char *name) } #endif
+#define DAVINCI_PCM_FMTBITS (\ + SNDRV_PCM_FMTBIT_S8 |\ + SNDRV_PCM_FMTBIT_U8 |\ + SNDRV_PCM_FMTBIT_S16_LE |\ + SNDRV_PCM_FMTBIT_S16_BE |\ + SNDRV_PCM_FMTBIT_U16_LE |\ + SNDRV_PCM_FMTBIT_U16_BE |\ + SNDRV_PCM_FMTBIT_S24_LE |\ + SNDRV_PCM_FMTBIT_S24_BE |\ + SNDRV_PCM_FMTBIT_U24_LE |\ + SNDRV_PCM_FMTBIT_U24_BE |\ + SNDRV_PCM_FMTBIT_S32_LE |\ + SNDRV_PCM_FMTBIT_S32_BE |\ + SNDRV_PCM_FMTBIT_U32_LE |\ + SNDRV_PCM_FMTBIT_U32_BE) + static struct snd_pcm_hardware pcm_hardware_playback = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME), - .formats = (SNDRV_PCM_FMTBIT_S16_LE), + .formats = DAVINCI_PCM_FMTBITS, .rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | @@ -72,7 +88,7 @@ static struct snd_pcm_hardware pcm_hardware_capture = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_PAUSE), - .formats = (SNDRV_PCM_FMTBIT_S16_LE), + .formats = DAVINCI_PCM_FMTBITS, .rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
Based on the registration of davinci-mcasp.1 in the davinci-evm platform setup for da830 and dm6467, davinci-pcm can handle more than the currently reported maximum channels of 2.
Increase the maximum channels to 384 to match the maximum reported by davinci-mcasp.1.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca
--- sound/soc/davinci/davinci-pcm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index 1e47267..9b5a9bf 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -75,7 +75,7 @@ static struct snd_pcm_hardware pcm_hardware_playback = { .rate_min = 8000, .rate_max = 96000, .channels_min = 2, - .channels_max = 2, + .channels_max = 384, .buffer_bytes_max = 128 * 1024, .period_bytes_min = 32, .period_bytes_max = 8 * 1024, @@ -97,7 +97,7 @@ static struct snd_pcm_hardware pcm_hardware_capture = { .rate_min = 8000, .rate_max = 96000, .channels_min = 2, - .channels_max = 2, + .channels_max = 384, .buffer_bytes_max = 128 * 1024, .period_bytes_min = 32, .period_bytes_max = 8 * 1024,
The release of the dma channels was being performed in prepare and there was a edma_resume call for the asp-channel only being executed on START, RESUME and PAUSE_RELEASE.
The mcasp on da850evm with ping-pong buffers enabled was exhibiting an audible glitch on every playback after the first. It was determined through trial and error that the following two changes fix this problem:
1) Move the edma_start calls from prepare to trigger and 2) reverse the order of starting the asp and ram channels.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca CC: Troy Kisky troy.kisky@boundarydevices.com
--- sound/soc/davinci/davinci-pcm.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index 9b5a9bf..5d9269a 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -544,6 +544,13 @@ static int davinci_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
switch (cmd) { case SNDRV_PCM_TRIGGER_START: + edma_start(prtd->asp_channel); + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && + prtd->ram_channel >= 0) { + /* copy 1st iram buffer */ + edma_start(prtd->ram_channel); + } + break; case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: edma_resume(prtd->asp_channel); @@ -582,11 +589,6 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) print_buf_info(prtd->asp_link[0], "asp_link[0]"); print_buf_info(prtd->asp_link[1], "asp_link[1]");
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - /* copy 1st iram buffer */ - edma_start(prtd->ram_channel); - } - edma_start(prtd->asp_channel); return 0; } prtd->period = 0; @@ -596,7 +598,6 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) edma_read_slot(prtd->asp_link[0], &prtd->asp_params); edma_write_slot(prtd->asp_channel, &prtd->asp_params); davinci_pcm_enqueue_dma(substream); - edma_start(prtd->asp_channel);
return 0; }
Extract functions that modify the prtd->period member in preparation for conversion to BATCH mode playback.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca
--- sound/soc/davinci/davinci-pcm.c | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index 5d9269a..fedca81 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -155,6 +155,22 @@ struct davinci_runtime_data { struct edmacc_param ram_params; };
+static void davinci_pcm_period_elapsed(struct snd_pcm_substream *substream) +{ + struct davinci_runtime_data *prtd = substream->runtime->private_data; + struct snd_pcm_runtime *runtime = substream->runtime; + + prtd->period++; + if (unlikely(prtd->period >= runtime->periods)) + prtd->period = 0; +} + +static void davinci_pcm_period_reset(struct snd_pcm_substream *substream) +{ + struct davinci_runtime_data *prtd = substream->runtime->private_data; + + prtd->period = 0; +} /* * Not used with ping/pong */ @@ -216,9 +232,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) edma_set_transfer_params(link, acnt, fifo_level, count, fifo_level, ABSYNC);
- prtd->period++; - if (unlikely(prtd->period >= runtime->periods)) - prtd->period = 0; + davinci_pcm_period_elapsed(substream); }
static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) @@ -591,7 +605,7 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream)
return 0; } - prtd->period = 0; + davinci_pcm_period_reset(substream); davinci_pcm_enqueue_dma(substream);
/* Copy self-linked parameter RAM entry into master channel */
The davinci-pcm driver's snd_pcm_ops pointer function currently calls into the edma controller driver to read the current positions of the edma channels to determine pos to return to the ALSA framework. In particular, davinci_pcm_pointer() calls edma_get_position() and the latter has a comment indicating that "Its channel should not be active when this is called" whereas the channel is surely active when snd_pcm_ops.pointer is called.
The operation of davinci-pcm in capture and playback appears to follow close the other pcm drivers who export SNDRV_PCM_INFO_BATCH except that davinci-pcm does not report it's positions from pointer() using the last transferred chunk. Instead it peeks directly into the edma controller to determine the current position as discussed above.
Convert the davinci-pcm driver to BATCH mode: count the periods elapsed in the prtd->period member and use its value to report the 'pos' to the alsa framework in the davinci_pcm_pointer function.
There is a phase offset of 2 periods between the position used by dma setup and the position reported in the pointer function. Either +2 in the dma setup or -2 in the pointer function (with wrapping, both) accounts for this offset -- I opted for the latter since it makes the first-time setup clearer.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca
--- sound/soc/davinci/davinci-pcm.c | 67 +++++++++++---------------------------- 1 files changed, 19 insertions(+), 48 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index fedca81..fa8fc61 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -65,7 +65,8 @@ static void print_buf_info(int slot, char *name) static struct snd_pcm_hardware pcm_hardware_playback = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | - SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME), + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME| + SNDRV_PCM_INFO_BATCH), .formats = DAVINCI_PCM_FMTBITS, .rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | @@ -87,7 +88,8 @@ static struct snd_pcm_hardware pcm_hardware_playback = { static struct snd_pcm_hardware pcm_hardware_capture = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | - SNDRV_PCM_INFO_PAUSE), + SNDRV_PCM_INFO_PAUSE | + SNDRV_PCM_INFO_BATCH), .formats = DAVINCI_PCM_FMTBITS, .rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | @@ -231,8 +233,6 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) else edma_set_transfer_params(link, acnt, fifo_level, count, fifo_level, ABSYNC); - - davinci_pcm_period_elapsed(substream); }
static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) @@ -247,12 +247,13 @@ static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) return;
if (snd_pcm_running(substream)) { + spin_lock(&prtd->lock); if (prtd->ram_channel < 0) { /* No ping/pong must fix up link dma data*/ - spin_lock(&prtd->lock); davinci_pcm_enqueue_dma(substream); - spin_unlock(&prtd->lock); } + davinci_pcm_period_elapsed(substream); + spin_unlock(&prtd->lock); snd_pcm_period_elapsed(substream); } } @@ -588,6 +589,7 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) { struct davinci_runtime_data *prtd = substream->runtime->private_data;
+ davinci_pcm_period_reset(substream); if (prtd->ram_channel >= 0) { int ret = ping_pong_dma_setup(substream); if (ret < 0) @@ -603,15 +605,19 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) print_buf_info(prtd->asp_link[0], "asp_link[0]"); print_buf_info(prtd->asp_link[1], "asp_link[1]");
+ davinci_pcm_period_elapsed(substream); + davinci_pcm_period_elapsed(substream); + return 0; } - davinci_pcm_period_reset(substream); davinci_pcm_enqueue_dma(substream); + davinci_pcm_period_elapsed(substream);
/* Copy self-linked parameter RAM entry into master channel */ edma_read_slot(prtd->asp_link[0], &prtd->asp_params); edma_write_slot(prtd->asp_channel, &prtd->asp_params); davinci_pcm_enqueue_dma(substream); + davinci_pcm_period_elapsed(substream);
return 0; } @@ -623,51 +629,16 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream) struct davinci_runtime_data *prtd = runtime->private_data; unsigned int offset; int asp_count; - dma_addr_t asp_src, asp_dst; + unsigned int period_size = snd_pcm_lib_period_bytes(substream);
spin_lock(&prtd->lock); - if (prtd->ram_channel >= 0) { - int ram_count; - int mod_ram; - dma_addr_t ram_src, ram_dst; - unsigned int period_size = snd_pcm_lib_period_bytes(substream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - /* reading ram before asp should be safe - * as long as the asp transfers less than a ping size - * of bytes between the 2 reads - */ - edma_get_position(prtd->ram_channel, - &ram_src, &ram_dst); - edma_get_position(prtd->asp_channel, - &asp_src, &asp_dst); - asp_count = asp_src - prtd->asp_params.src; - ram_count = ram_src - prtd->ram_params.src; - mod_ram = ram_count % period_size; - mod_ram -= asp_count; - if (mod_ram < 0) - mod_ram += period_size; - else if (mod_ram == 0) { - if (snd_pcm_running(substream)) - mod_ram += period_size; - } - ram_count -= mod_ram; - if (ram_count < 0) - ram_count += period_size * runtime->periods; - } else { - edma_get_position(prtd->ram_channel, - &ram_src, &ram_dst); - ram_count = ram_dst - prtd->ram_params.dst; - } - asp_count = ram_count; - } else { - edma_get_position(prtd->asp_channel, &asp_src, &asp_dst); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - asp_count = asp_src - runtime->dma_addr; - else - asp_count = asp_dst - runtime->dma_addr; - } + asp_count = prtd->period - 2; spin_unlock(&prtd->lock);
+ if (asp_count < 0) + asp_count += runtime->periods; + asp_count *= period_size; + offset = bytes_to_frames(runtime, asp_count); if (offset >= runtime->buffer_size) offset = 0;
On 24/05/11 19:50, Ben Gardiner wrote:
The davinci-pcm driver's snd_pcm_ops pointer function currently calls into the edma controller driver to read the current positions of the edma channels to determine pos to return to the ALSA framework. In particular, davinci_pcm_pointer() calls edma_get_position() and the latter has a comment indicating that "Its channel should not be active when this is called" whereas the channel is surely active when snd_pcm_ops.pointer is called.
The operation of davinci-pcm in capture and playback appears to follow close the other pcm drivers who export SNDRV_PCM_INFO_BATCH except that davinci-pcm does not report it's positions from pointer() using the last transferred chunk. Instead it peeks directly into the edma controller to determine the current position as discussed above.
Convert the davinci-pcm driver to BATCH mode: count the periods elapsed in the prtd->period member and use its value to report the 'pos' to the alsa framework in the davinci_pcm_pointer function.
There is a phase offset of 2 periods between the position used by dma setup and the position reported in the pointer function. Either +2 in the dma setup or -2 in the pointer function (with wrapping, both) accounts for this offset -- I opted for the latter since it makes the first-time setup clearer.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca Reviewed-by: Steven Faludi stevenfaludi@nanometrics.ca
sound/soc/davinci/davinci-pcm.c | 67 +++++++++++---------------------------- 1 files changed, 19 insertions(+), 48 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index fedca81..fa8fc61 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -65,7 +65,8 @@ static void print_buf_info(int slot, char *name) static struct snd_pcm_hardware pcm_hardware_playback = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME|SNDRV_PCM_INFO_BATCH),@@ -87,7 +88,8 @@ static struct snd_pcm_hardware pcm_hardware_playback = { static struct snd_pcm_hardware pcm_hardware_capture = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
SNDRV_PCM_INFO_PAUSE),
SNDRV_PCM_INFO_PAUSE |SNDRV_PCM_INFO_BATCH),@@ -231,8 +233,6 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) else edma_set_transfer_params(link, acnt, fifo_level, count, fifo_level, ABSYNC);
- davinci_pcm_period_elapsed(substream);
}
static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) @@ -247,12 +247,13 @@ static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) return;
if (snd_pcm_running(substream)) {
spin_lock(&prtd->lock);
spin_lock(&prtd->lock); davinci_pcm_enqueue_dma(substream);spin_unlock(&prtd->lock);
davinci_pcm_period_elapsed(substream);spin_unlock(&prtd->lock);} @@ -588,6 +589,7 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) { struct davinci_runtime_data *prtd = substream->runtime->private_data;
- davinci_pcm_period_reset(substream); if (prtd->ram_channel >= 0) { int ret = ping_pong_dma_setup(substream); if (ret < 0)
@@ -603,15 +605,19 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) print_buf_info(prtd->asp_link[0], "asp_link[0]"); print_buf_info(prtd->asp_link[1], "asp_link[1]");
davinci_pcm_period_elapsed(substream);davinci_pcm_period_elapsed(substream);
I assume these are to do with the 2 period phase offset you have so it's probably better to comment this here too.
- return 0; }
- davinci_pcm_period_reset(substream); davinci_pcm_enqueue_dma(substream);
davinci_pcm_period_elapsed(substream);
/* Copy self-linked parameter RAM entry into master channel */ edma_read_slot(prtd->asp_link[0], &prtd->asp_params); edma_write_slot(prtd->asp_channel, &prtd->asp_params); davinci_pcm_enqueue_dma(substream);
davinci_pcm_period_elapsed(substream);
return 0;
} @@ -623,51 +629,16 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream) struct davinci_runtime_data *prtd = runtime->private_data; unsigned int offset; int asp_count;
- dma_addr_t asp_src, asp_dst;
unsigned int period_size = snd_pcm_lib_period_bytes(substream);
spin_lock(&prtd->lock);
- if (prtd->ram_channel >= 0) {
int ram_count;int mod_ram;dma_addr_t ram_src, ram_dst;unsigned int period_size = snd_pcm_lib_period_bytes(substream);if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {/* reading ram before asp should be safe* as long as the asp transfers less than a ping size* of bytes between the 2 reads*/edma_get_position(prtd->ram_channel,&ram_src, &ram_dst);edma_get_position(prtd->asp_channel,&asp_src, &asp_dst);asp_count = asp_src - prtd->asp_params.src;ram_count = ram_src - prtd->ram_params.src;mod_ram = ram_count % period_size;mod_ram -= asp_count;if (mod_ram < 0)mod_ram += period_size;else if (mod_ram == 0) {if (snd_pcm_running(substream))mod_ram += period_size;}ram_count -= mod_ram;if (ram_count < 0)ram_count += period_size * runtime->periods;} else {edma_get_position(prtd->ram_channel,&ram_src, &ram_dst);ram_count = ram_dst - prtd->ram_params.dst;}asp_count = ram_count;- } else {
edma_get_position(prtd->asp_channel, &asp_src, &asp_dst);if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)asp_count = asp_src - runtime->dma_addr;elseasp_count = asp_dst - runtime->dma_addr;- }
asp_count = prtd->period - 2; spin_unlock(&prtd->lock);
if (asp_count < 0)
asp_count += runtime->periods;asp_count *= period_size;
offset = bytes_to_frames(runtime, asp_count); if (offset >= runtime->buffer_size) offset = 0;
Hi Liam,
On Wed, May 25, 2011 at 5:52 AM, Liam Girdwood lrg@ti.com wrote:
On 24/05/11 19:50, Ben Gardiner wrote:
[...] @@ -603,15 +605,19 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) print_buf_info(prtd->asp_link[0], "asp_link[0]"); print_buf_info(prtd->asp_link[1], "asp_link[1]");
- davinci_pcm_period_elapsed(substream);
- davinci_pcm_period_elapsed(substream);
I assume these are to do with the 2 period phase offset you have so it's probably better to comment this here too.
That's correct and yes I really should have commented this magic a little. I will send an incremental patch that can be folded-in. Thanks again.
Best Regards, Ben Gardiner
--- Nanometrics Inc. http://www.nanometrics.ca
In the previous commit 'ASoC: davinci-pcm: convert to BATCH mode', the phase offset of 2 was mentioned in the commit message but not well commented in the source.
Add descriptive comments of the phase offset with and without ping-pong buffers enabled.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca
--- sound/soc/davinci/davinci-pcm.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index fa8fc61..c9e0320 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -605,6 +605,18 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) print_buf_info(prtd->asp_link[0], "asp_link[0]"); print_buf_info(prtd->asp_link[1], "asp_link[1]");
+ /* + * There is a phase offset of 2 periods between the position + * used by dma setup and the position reported in the pointer + * function. + * + * The phase offset, when not using ping-pong buffers, is due to + * the two consecutive calls to davinci_pcm_enqueue_dma() below. + * + * Whereas here, with ping-pong buffers, the phase is due to + * there being an entire buffer transfer complete before the + * first dma completion event triggers davinci_pcm_dma_irq(). + */ davinci_pcm_period_elapsed(substream); davinci_pcm_period_elapsed(substream);
@@ -631,6 +643,13 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream) int asp_count; unsigned int period_size = snd_pcm_lib_period_bytes(substream);
+ /* + * There is a phase offset of 2 periods between the position used by dma + * setup and the position reported in the pointer function. Either +2 in + * the dma setup or -2 here in the pointer function (with wrapping, + * both) accounts for this offset -- choose the latter since it makes + * the first-time setup clearer. + */ spin_lock(&prtd->lock); asp_count = prtd->period - 2; spin_unlock(&prtd->lock);
On 25/05/11 14:27, Ben Gardiner wrote:
In the previous commit 'ASoC: davinci-pcm: convert to BATCH mode', the phase offset of 2 was mentioned in the commit message but not well commented in the source.
Add descriptive comments of the phase offset with and without ping-pong buffers enabled.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca
sound/soc/davinci/davinci-pcm.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index fa8fc61..c9e0320 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -605,6 +605,18 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) print_buf_info(prtd->asp_link[0], "asp_link[0]"); print_buf_info(prtd->asp_link[1], "asp_link[1]");
/** There is a phase offset of 2 periods between the position* used by dma setup and the position reported in the pointer* function.** The phase offset, when not using ping-pong buffers, is due to* the two consecutive calls to davinci_pcm_enqueue_dma() below.** Whereas here, with ping-pong buffers, the phase is due to* there being an entire buffer transfer complete before the* first dma completion event triggers davinci_pcm_dma_irq().*/@@ -631,6 +643,13 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream) int asp_count; unsigned int period_size = snd_pcm_lib_period_bytes(substream);
- /*
* There is a phase offset of 2 periods between the position used by dma* setup and the position reported in the pointer function. Either +2 in* the dma setup or -2 here in the pointer function (with wrapping,* both) accounts for this offset -- choose the latter since it makes* the first-time setup clearer.*/
Acked-by: Liam Girdwood lrg@ti.com
On Wed, May 25, 2011 at 09:27:22AM -0400, Ben Gardiner wrote:
In the previous commit 'ASoC: davinci-pcm: convert to BATCH mode', the phase offset of 2 was mentioned in the commit message but not well commented in the source.
Applied, thanks.
On 24/05/11 19:50, Ben Gardiner wrote:
This patchset is a collection of changes for the davinci-pcm driver that were accumulated during previous forays into the davinci-mcasp and davinci-pcm drivers [1].
The first three patches are minor changes that cleanup the ping-pong DMA params setup, expand the davinci-pcm reported format and also its maximum channels.
The fourth patch in the series corrects an audible glitch that can be heard on da850evm's McASP when ping-pong buffers are enabled. The glitch occurs only on the second and susequent playbacks, not the first. Reversing the order in which the dma channels are started and moving the start to the trigger function from the prepare function fixes the glitch.
The fifth and sixth patches in the series convert the davinci-pcm driver to BATCH mode.
All
Acked-by: Liam Girdwood lrg@ti.com
One minor thing for patch 6 that can be added incrementally.
On Tue, May 24, 2011 at 02:50:14PM -0400, Ben Gardiner wrote:
This patchset is a collection of changes for the davinci-pcm driver that were accumulated during previous forays into the davinci-mcasp and davinci-pcm drivers [1].
Applied all, thanks.
participants (3)
-
Ben Gardiner -
Liam Girdwood -
Mark Brown