[alsa-devel] [RFC] [PATCH 3/3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter
Enhance period index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting interrupts
This patch applies on top of patch 2 from this series: [RFC][PATCH 2/3] ASoC: OMAP: Make use of DMA channel self linking on OMAP1510
Created against linux-2.6.31-rc5.
Tested on Amstrad Delta.
Signed-off-by: Janusz Krzysztofik jkrzyszt@tis.icnet.pl
--- --- linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c.orig 2009-08-10 08:31:08.000000000 +0200 +++ linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c 2009-08-10 08:55:09.000000000 +0200 @@ -68,13 +68,28 @@ static void omap_pcm_dma_irq(int ch, u16 * that can be used by omap_pcm_pointer() instead. */ spin_lock_irqsave(&prtd->lock, flags); - if (prtd->period_index >= 0) { - if (++prtd->period_index == runtime->periods) { + if (stat & OMAP_DMA_LAST_IRQ) { + /* last period of a buffer has been started */ + if (prtd->period_index == (runtime->periods - 1)) { + /* we are in sync, do nothing */ + spin_unlock_irqrestore(&prtd->lock, flags); + return; + } else if (prtd->period_index >= 0) { + /* possible IRQ loss, update the pointer */ + prtd->period_index = runtime->periods - 1; + } + } else if (prtd->period_index >= 0) { + /* playback in progress - increment the counter, + * check for end of buffer */ + if ((++prtd->period_index >= runtime->periods) || + (stat & OMAP_DMA_BLOCK_IRQ)) { + /* end of buffer reached, loop back */ prtd->period_index = 0; } } spin_unlock_irqrestore(&prtd->lock, flags); - } + } else if (stat & OMAP_DMA_LAST_IRQ) + return;
snd_pcm_period_elapsed(substream); } @@ -175,7 +190,8 @@ static int omap_pcm_prepare(struct snd_p dma_params.frame_count = runtime->periods; omap_set_dma_params(prtd->dma_ch, &dma_params);
- omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ); + omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ | + (cpu_is_omap1510() ? OMAP_DMA_LAST_IRQ : 0));
return 0; }
Monday 10 August 2009 10:50:06 Janusz Krzysztofik wrote:
Enhance period index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting interrupts
Sorry, this one is still buggy an can break other OMAP models. Please, do not apply, but wait for next version. However, comments are welcome.
The bug hits when multiple interrupt flags are set in parallel.
Thanks, Janusz
Monday 10 August 2009 11:43:25 Janusz Krzysztofik wrote:
Monday 10 August 2009 10:50:06 Janusz Krzysztofik wrote:
Enhance period index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting interrupts
Sorry, this one is still buggy
To be clear: I meant only patch 3/3 was buggy, others seemed OK.
an can break other OMAP models.
Overstatement.
The bug hits when multiple interrupt flags are set in parallel.
Under heavy debugging, for the first time I have noticed several consecutive DMA interrupt requests with all three flags set alltogether: FRAME, LAST and BLOCK. It looks hopeless, doesn't it? Keeping the period index at 0 will not report any progress, other choices (increment, set max) will lead to buffer loss, right?
Anyway, that observation seems to prove the requirement for a more accurate period counter. Improving it should help under heavy load. I think it is worth of trying, even if there may be other corner cases still not addressed.
v2 of the patch will follow next.
Janusz
Enhance period index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting up interrupts.
Corrections since v1: 1. Fix buggy DMA interrupt handling in case of multiple status flags set in parallel. 2. Enable OMAP_DMA_LAST_IRQ only for playback on OMAP1510.
This patch applies on top of patch 2 from this series: [RFC][PATCH 2/3] ASoC: OMAP: Make use of DMA channel self linking on OMAP1510
Created against linux-2.6.31-rc5.
Tested on Amstrad Delta.
Signed-off-by: Janusz Krzysztofik jkrzyszt@tis.icnet.pl
--- --- linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c.orig 2009-08-10 22:41:25.000000000 +0200 +++ linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c 2009-08-10 23:15:46.000000000 +0200 @@ -68,13 +68,27 @@ static void omap_pcm_dma_irq(int ch, u16 * that can be used by omap_pcm_pointer() instead. */ spin_lock_irqsave(&prtd->lock, flags); + if ((stat == OMAP_DMA_LAST_IRQ) && + (prtd->period_index == runtime->periods - 1)) { + /* we are in sync, do nothing */ + spin_unlock_irqrestore(&prtd->lock, flags); + return; + } if (prtd->period_index >= 0) { - if (++prtd->period_index == runtime->periods) { + if (stat & OMAP_DMA_BLOCK_IRQ) { + /* end of buffer reached, loop back */ + prtd->period_index = 0; + } else if (stat & OMAP_DMA_LAST_IRQ) { + /* update the counter for the last period */ + prtd->period_index = runtime->periods - 1; + } else if (++prtd->period_index >= runtime->periods) { + /* end of buffer missed? loop back */ prtd->period_index = 0; } } spin_unlock_irqrestore(&prtd->lock, flags); - } + } else if (stat == OMAP_DMA_LAST_IRQ) + return;
snd_pcm_period_elapsed(substream); } @@ -175,7 +189,12 @@ static int omap_pcm_prepare(struct snd_p dma_params.frame_count = runtime->periods; omap_set_dma_params(prtd->dma_ch, &dma_params);
- omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ); + if ((cpu_is_omap1510()) && + (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)) + omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ | + OMAP_DMA_LAST_IRQ); + else + omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ);
return 0; }
On Tue, 11 Aug 2009 01:07:10 +0200 Janusz Krzysztofik jkrzyszt@tis.icnet.pl wrote:
Enhance period index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting up interrupts.
Corrections since v1:
- Fix buggy DMA interrupt handling in case of multiple status flags set in parallel.
- Enable OMAP_DMA_LAST_IRQ only for playback on OMAP1510.
This patch applies on top of patch 2 from this series: [RFC][PATCH 2/3] ASoC: OMAP: Make use of DMA channel self linking on OMAP1510
Created against linux-2.6.31-rc5.
Tested on Amstrad Delta.
Signed-off-by: Janusz Krzysztofik jkrzyszt@tis.icnet.pl
--- linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c.orig 2009-08-10 22:41:25.000000000 +0200 +++ linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c 2009-08-10 23:15:46.000000000 +0200 @@ -68,13 +68,27 @@ static void omap_pcm_dma_irq(int ch, u16 * that can be used by omap_pcm_pointer() instead. */ spin_lock_irqsave(&prtd->lock, flags);
if ((stat == OMAP_DMA_LAST_IRQ) &&(prtd->period_index == runtime->periods - 1)) {/* we are in sync, do nothing */spin_unlock_irqrestore(&prtd->lock, flags);return;}
if (++prtd->period_index == runtime->periods) {
if (stat & OMAP_DMA_BLOCK_IRQ) {/* end of buffer reached, loop back */prtd->period_index = 0;} else if (stat & OMAP_DMA_LAST_IRQ) {/* update the counter for the last period */prtd->period_index = runtime->periods - 1;} else if (++prtd->period_index >= runtime->periods) {/* end of buffer missed? loop back */ prtd->period_index = 0; }
- }
- } else if (stat == OMAP_DMA_LAST_IRQ)
return;
Is this test needed? This interrupt is set only for playback on omap1510 so this looks null-op.
snd_pcm_period_elapsed(substream); } @@ -175,7 +189,12 @@ static int omap_pcm_prepare(struct snd_p dma_params.frame_count = runtime->periods; omap_set_dma_params(prtd->dma_ch, &dma_params);
- omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ);
- if ((cpu_is_omap1510()) &&
(substream->stream == SNDRV_PCM_STREAM_PLAYBACK))omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ |OMAP_DMA_LAST_IRQ);
Indent OMAP_DMA_LAST_IRQ with tab(s) and spaces to the same column than OMAP_DMA_FRAME_IRQ. Looks nicer then. Should the OMAP_DMA_BLOCK_IRQ to be set since it is handled in omap_pcm_dma_irq?
Hi Jarkko,
Tuesday 11 August 2009 08:42:48 Jarkko Nikula wrote:
On Tue, 11 Aug 2009 01:07:10 +0200 Janusz Krzysztofik jkrzyszt@tis.icnet.pl wrote:
Enhance period index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting up interrupts.
Corrections since v1:
- Fix buggy DMA interrupt handling in case of multiple status flags set
in parallel. 2. Enable OMAP_DMA_LAST_IRQ only for playback on OMAP1510.
- }
- } else if (stat == OMAP_DMA_LAST_IRQ)
return;Is this test needed? This interrupt is set only for playback on omap1510 so this looks null-op.
You're right, I have put it here before limiting the flag request to playback on OMAP1510 only. So it can be omitted, but only if we assume we are always called exactly as requested. It is not needed unless the flag is ever requested (by mistake?) by other arch/arm/plat-omap/dma.c bits and passed on by omap1_dma_handle_ch() that passes all flags without checking if requested. But then, shouldn't we start from always checking for OMAP_DMA_FRAME_IRQ set?
- omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ);
- if ((cpu_is_omap1510()) &&
(substream->stream == SNDRV_PCM_STREAM_PLAYBACK))omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ |OMAP_DMA_LAST_IRQ);Indent OMAP_DMA_LAST_IRQ with tab(s) and spaces to the same column than OMAP_DMA_FRAME_IRQ. Looks nicer then.
OK, will fix it.
Should the OMAP_DMA_BLOCK_IRQ to be set since it is handled in omap_pcm_dma_irq?
This one is already requested from inside omap_request_dma() and used inside omap1_dma_handle_ch() in addition to passing it to us.
Cheers, Janusz
Enhance period_index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting up interrupts.
Changes since v2:
- } + } else if (stat == OMAP_DMA_LAST_IRQ) + return;
Is this test needed? This interrupt is set only for playback on omap1510 so this looks null-op.
You're right, I have put it here before limiting the flag request to playback on OMAP1510 only. So it can be omitted...
+ omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ | + OMAP_DMA_LAST_IRQ);
Indent OMAP_DMA_LAST_IRQ with tab(s) and spaces to the same column than OMAP_DMA_FRAME_IRQ. Looks nicer then.
OK, will fix it.
Should the OMAP_DMA_BLOCK_IRQ to be set since it is handled in omap_pcm_dma_irq?
This one is already requested from inside omap_request_dma() and used inside omap1_dma_handle_ch() in addition to passing it to us.
But for less confusion, it'll be better if requested from here too.
Changes since v1: 1. Fix buggy DMA interrupt handling in case of multiple status flags set in parallel. 2. Enable OMAP_DMA_LAST_IRQ only for playback on OMAP1510.
This patch applies on top of patch 2 from this series: [RFC][PATCH 2/3] ASoC: OMAP: Make use of DMA channel self linking on OMAP1510
Created against linux-2.6.31-rc5.
Tested on Amstrad Delta.
Signed-off-by: Janusz Krzysztofik jkrzyszt@tis.icnet.pl
--- --- linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c.orig 2009-08-11 21:13:22.000000000 +0200 +++ linux-2.6.31-rc5/sound/soc/omap/omap-pcm.c 2009-08-11 21:34:34.000000000 +0200 @@ -68,8 +68,21 @@ static void omap_pcm_dma_irq(int ch, u16 * that can be used by omap_pcm_pointer() instead. */ spin_lock_irqsave(&prtd->lock, flags); + if ((stat == OMAP_DMA_LAST_IRQ) && + (prtd->period_index == runtime->periods - 1)) { + /* we are in sync, do nothing */ + spin_unlock_irqrestore(&prtd->lock, flags); + return; + } if (prtd->period_index >= 0) { - if (++prtd->period_index == runtime->periods) { + if (stat & OMAP_DMA_BLOCK_IRQ) { + /* end of buffer reached, loop back */ + prtd->period_index = 0; + } else if (stat & OMAP_DMA_LAST_IRQ) { + /* update the counter for the last period */ + prtd->period_index = runtime->periods - 1; + } else if (++prtd->period_index >= runtime->periods) { + /* end of buffer missed? loop back */ prtd->period_index = 0; } } @@ -175,7 +188,12 @@ static int omap_pcm_prepare(struct snd_p dma_params.frame_count = runtime->periods; omap_set_dma_params(prtd->dma_ch, &dma_params);
- omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ); + if ((cpu_is_omap1510()) && + (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)) + omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ | + OMAP_DMA_LAST_IRQ | OMAP_DMA_BLOCK_IRQ); + else + omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ);
return 0; }
On Tue, 11 Aug 2009 21:44:29 +0200 Janusz Krzysztofik jkrzyszt@tis.icnet.pl wrote:
Enhance period_index accuracy, particularly just before buffer rewind, by making use of DMA interrupt status flags in addition to simply counting up interrupts.
Changes since v2:
- } + } else if (stat == OMAP_DMA_LAST_IRQ) + return;
Is this test needed? This interrupt is set only for playback on omap1510 so this looks null-op.
You're right, I have put it here before limiting the flag request to playback on OMAP1510 only. So it can be omitted...
+ omap_enable_dma_irq(prtd->dma_ch, OMAP_DMA_FRAME_IRQ | + OMAP_DMA_LAST_IRQ);
Indent OMAP_DMA_LAST_IRQ with tab(s) and spaces to the same column than OMAP_DMA_FRAME_IRQ. Looks nicer then.
OK, will fix it.
Should the OMAP_DMA_BLOCK_IRQ to be set since it is handled in omap_pcm_dma_irq?
This one is already requested from inside omap_request_dma() and used inside omap1_dma_handle_ch() in addition to passing it to us.
But for less confusion, it'll be better if requested from here too.
I'm fine with this 3rd version. Probably Mark would like to have git format-patch formatted version for avoiding manual commit log editing.
Acked-by: Jarkko Nikula jhnikula@gmail.com
On Mon, Aug 17, 2009 at 12:26:05PM +0300, Jarkko Nikula wrote:
I'm fine with this 3rd version. Probably Mark would like to have git format-patch formatted version for avoiding manual commit log editing.
Acked-by: Jarkko Nikula jhnikula@gmail.com
Applied both 2-3, thanks.
Mark Brown wrote:
On Mon, Aug 17, 2009 at 12:26:05PM +0300, Jarkko Nikula wrote:
I'm fine with this 3rd version. Probably Mark would like to have git format-patch formatted version for avoiding manual commit log editing.
Acked-by: Jarkko Nikula jhnikula@gmail.com
Applied both 2-3, thanks.
Jarkko, Mark,
Thanks.
And now, how is the patch 1/3 supposed to get into the mainline? Whom should I ping from time to time to get it integrated? If not then, as you know for sure, applying patch 2/3 whithout 1/3 will result in ASoC support for OMAP1510 broken.
Thanks, Janusz
On Tue, Aug 18, 2009 at 03:42:05PM +0200, Janusz Krzysztofik wrote:
And now, how is the patch 1/3 supposed to get into the mainline? Whom should I ping from time to time to get it integrated? If not then, as you know for sure, applying patch 2/3 whithout 1/3 will result in ASoC support for OMAP1510 broken.
Normally the OMAP tree, though that's possibly a bit tricky due to the ARM tree closing early. If Tony's OK with merging via the ALSA tree we could also apply the ARM patch there?
On Tue, 18 Aug 2009 14:45:41 +0100 Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Aug 18, 2009 at 03:42:05PM +0200, Janusz Krzysztofik wrote:
And now, how is the patch 1/3 supposed to get into the mainline? Whom should I ping from time to time to get it integrated? If not then, as you know for sure, applying patch 2/3 whithout 1/3 will result in ASoC support for OMAP1510 broken.
Normally the OMAP tree, though that's possibly a bit tricky due to the ARM tree closing early. If Tony's OK with merging via the ALSA tree we could also apply the ARM patch there?
I would say that it's better to keep in Tony's hand since otherwise there can be risk for DMA code being out-of-sync as there are quite frequent other PM, OMAP4, errata, etc. patches coming into it.
Of course it means that OMAP1510 audio is broken until the patch 1/3 is merged but probably patch can be merged as a fix if merge window is missed.
* Jarkko Nikula jhnikula@gmail.com [090818 19:55]:
On Tue, 18 Aug 2009 14:45:41 +0100 Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Aug 18, 2009 at 03:42:05PM +0200, Janusz Krzysztofik wrote:
And now, how is the patch 1/3 supposed to get into the mainline? Whom should I ping from time to time to get it integrated? If not then, as you know for sure, applying patch 2/3 whithout 1/3 will result in ASoC support for OMAP1510 broken.
Normally the OMAP tree, though that's possibly a bit tricky due to the ARM tree closing early. If Tony's OK with merging via the ALSA tree we could also apply the ARM patch there?
I would say that it's better to keep in Tony's hand since otherwise there can be risk for DMA code being out-of-sync as there are quite frequent other PM, OMAP4, errata, etc. patches coming into it.
Of course it means that OMAP1510 audio is broken until the patch 1/3 is merged but probably patch can be merged as a fix if merge window is missed.
Already replied in another part of the thread, but just for the archives:
Since we don't have other dma or mcbsp patches going in right now, let's merge these all via the alsa list so audio keeps working.
Regards,
Tony
participants (4)
-
Janusz Krzysztofik -
Jarkko Nikula -
Mark Brown -
Tony Lindgren