[RFC] [PATCH 3/3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[RFC] [PATCH 3/3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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;
 }

Re: [RFC] [PATCH 3/3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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

Re: [RFC] [PATCH 3/3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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

[RFC][PATCH 3/3 v2] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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;
 }

Re: [RFC][PATCH 3/3 v2] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Jarkko Nikula]

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:
> 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;
>  
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?


-- 
Jarkko

Re: [RFC][PATCH 3/3 v2] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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:
> > 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.
> >
> > -	}
> > +	} 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

[PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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;
 }

Re: [PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Jarkko Nikula]

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>

Re: [PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Mark Brown]

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.

Re: [PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Janusz Krzysztofik]

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

Re: [PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Mark Brown]

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?

Re: [PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Jarkko Nikula]

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

Re: [PATCH 3/3 v3] ASoC: OMAP: Enhance OMAP1510 DMA progress software counter

[Tony Lindgren]

* 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