On Sat, Jul 04, 2009 at 07:30:01PM -0700, Troy Kisky wrote:
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.
Overall this looks good though clearly there are some cross tree merge issues which will need to be resolved. I do have some queries and concerns below, though.
One more general question is what sort of testing you've done with user space applications - what sort of things have you tried the new DMA code with? I'd be especially interested to see this tested with PulseAudio simply because that is so good at turning up problems with DMA.
+#define DAVINCI_PCM_DEBUG 0 +#if DAVINCI_PCM_DEBUG +#define DPRINTK(format, arg...) printk(KERN_DEBUG format, ## arg)
I'd be inclined to use the standard DEBUG and VERBOSE_DEBUG defines here together with their display macros here.
+/*
- 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
- */
Indentation.
- /* Request parameter RAM reload slot */
- ret = edma_alloc_slot(EDMA_SLOT_ANY);
- if (ret < 0) {
edma_free_channel(prtd->master_lch);return ret;
- /* Request ram link channel */
- ret = prtd->ram_link_lch = edma_alloc_slot(
EDMA_CTLR(prtd->ram_master_lch), EDMA_SLOT_ANY);
The signature for edma_alloc_slot() appears to have changed - that looks awfully like the SRAM patch or whatever else introduced this change ought to be including an update for this code to keep it buildable?
+/* I2S master (codec/cpudai) should start/stop dma request,
- we shouldn't ignore them
- codec AIC33 needs fixed
- */
+#define BROKEN_MASTER 1 +#ifdef BROKEN_MASTER
Could you expand on what's going on here? The idea that if the CODEC is master it should be starting DMA sounds wrong - obviously it should know nothing about the DMA controller. If you mean that it should start and stop the clocks that causes issues in situations like TDM since there can be transfers going on independantly of the CPU which may need the clocks running. Not everything will be able to gate the clocks, too.
In any case, this looks like a separate patch that should be split out of this one.
- 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);count_asp = asp_src - prtd->asp_params.src;count_ram = ram_src - prtd->ram_params.src;mod_ram = count_ram % period_size;mod_ram -= count_asp;
Is it perhaps saner to just look at the current position as being the position of the transfer to SRAM? This does mean more variation from the point at which data is audibly playing but on the other hand as far as the CPU is concerned once the audio gets to SRAM it can't work with it any longer so it's potentially misleading to report the SRAM position. Not all hardware will be able to report the position at all so userspace ought to be able to cope.
- iram_virt = sram_alloc(iram_size, &iram_phys);
- if (!iram_virt)
goto exit2;
Should this perhaps be configured via platform data? You've currently picked 7 * 512 bytes but there appears to be no urgent reason for that particular number and presumably SRAM is a limited resource which people may want to prioritise differently. That may mean having bigger buffers for audio as well as less - things like MP3 players can get better battery life by setting up very big DMAs. On a similar vein is it worth considering making this feature entirely optional and/or falling back to non-SRAM if SRAM allocation fails?
I think we should be able to go with the approach you've got initially but I suspect that as the SRAM gets more widely used it'll become more of an issue.