[PATCH] ASoC: MPC5200: Support for buffer wrap around

[PATCH] ASoC: MPC5200: Support for buffer wrap around

[John Bonesio]

Here's another appl_ptr related patch to the alsa soc mpc5200 ac97 driver. This 
time the patch is to fix a problem with not correctly handling appl_ptr
wrapping back around to the beginning of the buffer.

I haven't yet seen a symptom related to this. This came up when I was chatting
with Jon Smirl about another bug I was examining. Right now there is another
problem that is preventing me from playing really long audio streams. 

Let me know if this is the wrong approach.

- John

The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
s->runtime->control->appl_ptr can wrap around to the beginning of the
buffer. This change fixes this problem.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---

 sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index cfe0ea4..2551c58 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
 
 static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
 {
+	if (s->appl_ptr > s->runtime->control->appl_ptr) {
+		/*
+		 * In this case s->runtime->control->appl_ptr has wrapped around.
+		 * Play the data to the end of the boundary, then wrap our own
+		 * appl_ptr back around.
+		 */
+		while (s->appl_ptr < s->runtime->boundary) {
+			if (bcom_queue_full(s->bcom_task))
+				return;
+
+			s->appl_ptr += s->period_size;
+
+			psc_dma_bcom_enqueue_next_buffer(s);
+		}
+		s->appl_ptr -= s->runtime->boundary;
+	}
+
 	while (s->appl_ptr < s->runtime->control->appl_ptr) {
 
 		if (bcom_queue_full(s->bcom_task))

[PATCH] ASoC: MPC5200: Support for buffer wrap around

[John Bonesio]

We've encountered strange behavior in the alsamixer settings using the wm9712
codec. If we unmute the headphone output and then unmute the PCM output, the
headphone output gets reset to mute in the hardware register. At this point
the hardware register does not match the value in the register cache.

I've spent some time debugging this, and the headphone setting is set outside
of any code path that would call the ac97_write() routine. As best as I can
tell, there is something strange going on in hardware.

I've provided this patch that works around the problem.

Have any of you seen this before? Is this patch the right approach?

- John

The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
s->runtime->control->appl_ptr can wrap around to the beginning of the
buffer. This change fixes this problem.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---

 sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index cfe0ea4..2551c58 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
 
 static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
 {
+	if (s->appl_ptr > s->runtime->control->appl_ptr) {
+		/*
+		 * In this case s->runtime->control->appl_ptr has wrapped around.
+		 * Play the data to the end of the boundary, then wrap our own
+		 * appl_ptr back around.
+		 */
+		while (s->appl_ptr < s->runtime->boundary) {
+			if (bcom_queue_full(s->bcom_task))
+				return;
+
+			s->appl_ptr += s->period_size;
+
+			psc_dma_bcom_enqueue_next_buffer(s);
+		}
+		s->appl_ptr -= s->runtime->boundary;
+	}
+
 	while (s->appl_ptr < s->runtime->control->appl_ptr) {
 
 		if (bcom_queue_full(s->bcom_task))

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Grant Likely]

On Friday, July 31, 2009, John Bonesio <bones@secretlab.ca> wrote:
> We've encountered strange behavior in the alsamixer settings using the wm9712
> codec. If we unmute the headphone output and then unmute the PCM output, the
> headphone output gets reset to mute in the hardware register. At this point
> the hardware register does not match the value in the register cache.
>
> I've spent some time debugging this, and the headphone setting is set outside
> of any code path that would call the ac97_write() routine. As best as I can
> tell, there is something strange going on in hardware.
>
> I've provided this patch that works around the problem.
>
> Have any of you seen this before? Is this patch the right approach?
>
> - John
>
> The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> s->runtime->control->appl_ptr can wrap around to the beginning of the
> buffer. This change fixes this problem.
>
> Signed-off-by: John Bonesio <bones@secretlab.ca>

Hey John,

It would appear that you've attached the wrong patch.

g.

> ---
>
>  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index cfe0ea4..2551c58 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>
>  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>  {
> +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
> +               /*
> +                * In this case s->runtime->control->appl_ptr has wrapped around.
> +                * Play the data to the end of the boundary, then wrap our own
> +                * appl_ptr back around.
> +                */
> +               while (s->appl_ptr < s->runtime->boundary) {
> +                       if (bcom_queue_full(s->bcom_task))
> +                               return;
> +
> +                       s->appl_ptr += s->period_size;
> +
> +                       psc_dma_bcom_enqueue_next_buffer(s);
> +               }
> +               s->appl_ptr -= s->runtime->boundary;
> +       }
> +
>         while (s->appl_ptr < s->runtime->control->appl_ptr) {
>
>                 if (bcom_queue_full(s->bcom_task))
>
>

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Mark Brown]

On Fri, Jul 31, 2009 at 04:08:55PM -0700, John Bonesio wrote:
> We've encountered strange behavior in the alsamixer settings using the wm9712
> codec. If we unmute the headphone output and then unmute the PCM output, the
> headphone output gets reset to mute in the hardware register. At this point
> the hardware register does not match the value in the register cache.

Which specific controls are you looking at here?

> I've spent some time debugging this, and the headphone setting is set outside
> of any code path that would call the ac97_write() routine. As best as I can
> tell, there is something strange going on in hardware.

> I've provided this patch that works around the problem.

Like Grant says I suspect this isn't the patch you meant...

> Have any of you seen this before? Is this patch the right approach?

I've never heard of that before and the WM9712 is very widely deployed
so if it were a hardware issue in the CODEC I'd imagine it would have
come up.  On the other hand, it's not beyond the bounds of possibility.
If you could post the patch showing exactly what you're doing I could
probably say more.

If you could get a scope on the bus that might be interesting...

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Jon Smirl]

On Fri, Jul 31, 2009 at 7:08 PM, John Bonesio<bones@secretlab.ca> wrote:
> We've encountered strange behavior in the alsamixer settings using the wm9712
> codec. If we unmute the headphone output and then unmute the PCM output, the
> headphone output gets reset to mute in the hardware register. At this point
> the hardware register does not match the value in the register cache.
>
> I've spent some time debugging this, and the headphone setting is set outside
> of any code path that would call the ac97_write() routine. As best as I can
> tell, there is something strange going on in hardware.

This could be something wrong in the mpc5200 code that writes the AC97
registers too. Maybe a register write command is getting generated
when it wasn't supposed to.

You can set the mpc5200 to generate an interrupt everything it
finishes writing a AC97 register. If you get more interrupts than you
expected the problem is in the driver.

> I've provided this patch that works around the problem.
>
> Have any of you seen this before? Is this patch the right approach?
>
> - John
>
> The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> s->runtime->control->appl_ptr can wrap around to the beginning of the
> buffer. This change fixes this problem.
>
> Signed-off-by: John Bonesio <bones@secretlab.ca>
> ---
>
>  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index cfe0ea4..2551c58 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>
>  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>  {
> +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
> +               /*
> +                * In this case s->runtime->control->appl_ptr has wrapped around.
> +                * Play the data to the end of the boundary, then wrap our own
> +                * appl_ptr back around.
> +                */
> +               while (s->appl_ptr < s->runtime->boundary) {
> +                       if (bcom_queue_full(s->bcom_task))
> +                               return;
> +
> +                       s->appl_ptr += s->period_size;
> +
> +                       psc_dma_bcom_enqueue_next_buffer(s);
> +               }
> +               s->appl_ptr -= s->runtime->boundary;
> +       }
> +
>        while (s->appl_ptr < s->runtime->control->appl_ptr) {
>
>                if (bcom_queue_full(s->bcom_task))
>
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[John Bonesio]

[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]

Hi All,

Woops! Yep, this is the wrong patch. I've included the correct one. I've
also attached the patch in case email messes with its formatting. Sorry
about the confusion.

Jon,

I put in a debug hook in the out_be32() routine. It would printk output
anytime the AC97_HEADPHONE register was written to. This printk text
would come out as expected when the Headphone mixer setting was
muted/unmuted.

When I umuted the PCM mixer setting, this printk text did not appear,
yet when I read the mixer register setting for the headphone, it was set
back to mute.

If there is code in software doing this, it's very subtle.

- John

----- correct patch -----

>From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
From: John Bonesio <bones@secretlab.ca>
Date: Fri, 31 Jul 2009 16:01:33 -0700
Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute  problem

When setting the PCM mixer (unuting), the main Headphone mixer setting gets
set back to mute. At this time it appears this is occuring in hardware.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---
 sound/soc/codecs/wm9712.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index b57c817..6f8d164 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec,
 	unsigned int reg);
 static int ac97_write(struct snd_soc_codec *codec,
 	unsigned int reg, unsigned int val);
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
 
 /*
  * WM9712 register cache
@@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w,
 	else
 		ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
 
-	if (l & 0x2 || r & 0x2)
+	if (l & 0x2 || r & 0x2) {
 		ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
-	else
+		/*
+		 * Workaround an apparent bug where the headphone mute setting
+		 * is modified when the PCM mute setting is enabled.
+		 */
+		ac97_flush(w->codec, AC97_HEADPHONE);
+	} else
 		ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
 
 	if (l & 0x4 || r & 0x4)
@@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
 	return 0;
 }
 
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
+{
+	unsigned int val;
+	u16 *cache = codec->reg_cache;
+
+	if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
+		val = cache[reg >> 1];
+		soc_ac97_ops.write(codec->ac97, reg, val);
+	}
+}
+
 static int ac97_prepare(struct snd_pcm_substream *substream,
 			struct snd_soc_dai *dai)
 {
-- 
1.6.0.4


-------------------------



On Sat, 2009-08-01 at 09:47 -0400, Jon Smirl wrote:
> On Fri, Jul 31, 2009 at 7:08 PM, John Bonesio<bones@secretlab.ca> wrote:
> > We've encountered strange behavior in the alsamixer settings using the wm9712
> > codec. If we unmute the headphone output and then unmute the PCM output, the
> > headphone output gets reset to mute in the hardware register. At this point
> > the hardware register does not match the value in the register cache.
> >
> > I've spent some time debugging this, and the headphone setting is set outside
> > of any code path that would call the ac97_write() routine. As best as I can
> > tell, there is something strange going on in hardware.
> 
> This could be something wrong in the mpc5200 code that writes the AC97
> registers too. Maybe a register write command is getting generated
> when it wasn't supposed to.
> 
> You can set the mpc5200 to generate an interrupt everything it
> finishes writing a AC97 register. If you get more interrupts than you
> expected the problem is in the driver.
> 
> > I've provided this patch that works around the problem.
> >
> > Have any of you seen this before? Is this patch the right approach?
> >
> > - John
> >
> > The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> > s->runtime->control->appl_ptr can wrap around to the beginning of the
> > buffer. This change fixes this problem.
> >
> > Signed-off-by: John Bonesio <bones@secretlab.ca>
> > ---
> >
> >  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
> >  1 files changed, 17 insertions(+), 0 deletions(-)
> >
> > diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> > index cfe0ea4..2551c58 100644
> > --- a/sound/soc/fsl/mpc5200_dma.c
> > +++ b/sound/soc/fsl/mpc5200_dma.c
> > @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
> >
> >  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> >  {
> > +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
> > +               /*
> > +                * In this case s->runtime->control->appl_ptr has wrapped around.
> > +                * Play the data to the end of the boundary, then wrap our own
> > +                * appl_ptr back around.
> > +                */
> > +               while (s->appl_ptr < s->runtime->boundary) {
> > +                       if (bcom_queue_full(s->bcom_task))
> > +                               return;
> > +
> > +                       s->appl_ptr += s->period_size;
> > +
> > +                       psc_dma_bcom_enqueue_next_buffer(s);
> > +               }
> > +               s->appl_ptr -= s->runtime->boundary;
> > +       }
> > +
> >        while (s->appl_ptr < s->runtime->control->appl_ptr) {
> >
> >                if (bcom_queue_full(s->bcom_task))
> >
> >
> 
> 
> 

[-- Attachment #2: 0001-ASoC-WM9712-Codec-Workaround-an-unmute-problem.patch --]
[-- Type: text/x-patch, Size: 1958 bytes --]

>From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
From: John Bonesio <bones@secretlab.ca>
Date: Fri, 31 Jul 2009 16:01:33 -0700
Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute  problem

When setting the PCM mixer (unuting), the main Headphone mixer setting gets
set back to mute. At this time it appears this is occuring in hardware.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---
 sound/soc/codecs/wm9712.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index b57c817..6f8d164 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec,
 	unsigned int reg);
 static int ac97_write(struct snd_soc_codec *codec,
 	unsigned int reg, unsigned int val);
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
 
 /*
  * WM9712 register cache
@@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w,
 	else
 		ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
 
-	if (l & 0x2 || r & 0x2)
+	if (l & 0x2 || r & 0x2) {
 		ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
-	else
+		/*
+		 * Workaround an apparent bug where the headphone mute setting
+		 * is modified when the PCM mute setting is enabled.
+		 */
+		ac97_flush(w->codec, AC97_HEADPHONE);
+	} else
 		ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
 
 	if (l & 0x4 || r & 0x4)
@@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
 	return 0;
 }
 
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
+{
+	unsigned int val;
+	u16 *cache = codec->reg_cache;
+
+	if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
+		val = cache[reg >> 1];
+		soc_ac97_ops.write(codec->ac97, reg, val);
+	}
+}
+
 static int ac97_prepare(struct snd_pcm_substream *substream,
 			struct snd_soc_dai *dai)
 {
-- 
1.6.0.4


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Jon Smirl]

On Sat, Aug 1, 2009 at 11:04 AM, John Bonesio<bones@secretlab.ca> wrote:
> Hi All,
>
> Woops! Yep, this is the wrong patch. I've included the correct one. I've
> also attached the patch in case email messes with its formatting. Sorry
> about the confusion.
>
> Jon,
>
> I put in a debug hook in the out_be32() routine. It would printk output
> anytime the AC97_HEADPHONE register was written to. This printk text
> would come out as expected when the Headphone mixer setting was
> muted/unmuted.

I was thinking a stale register write may have gotten into the mpc5200
AC97 hardware somehow and not been sent immediately to the codec.
Something you do later causes it to be sent.  I don't trust the
mpc5200 AC97 register write hardware 100%, it was an after thought
added in the mpc5200b.

Some logic analyzers have AC97 protocol decode capability. That would
isolate easily isolate the problem.

>
> When I umuted the PCM mixer setting, this printk text did not appear,
> yet when I read the mixer register setting for the headphone, it was set
> back to mute.
>
> If there is code in software doing this, it's very subtle.
>
> - John
>
> ----- correct patch -----
>
> >From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
> From: John Bonesio <bones@secretlab.ca>
> Date: Fri, 31 Jul 2009 16:01:33 -0700
> Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute  problem
>
> When setting the PCM mixer (unuting), the main Headphone mixer setting gets
> set back to mute. At this time it appears this is occuring in hardware.
>
> Signed-off-by: John Bonesio <bones@secretlab.ca>
> ---
>  sound/soc/codecs/wm9712.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
> index b57c817..6f8d164 100644
> --- a/sound/soc/codecs/wm9712.c
> +++ b/sound/soc/codecs/wm9712.c
> @@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec,
>        unsigned int reg);
>  static int ac97_write(struct snd_soc_codec *codec,
>        unsigned int reg, unsigned int val);
> +static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
>
>  /*
>  * WM9712 register cache
> @@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w,
>        else
>                ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
>
> -       if (l & 0x2 || r & 0x2)
> +       if (l & 0x2 || r & 0x2) {
>                ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
> -       else
> +               /*
> +                * Workaround an apparent bug where the headphone mute setting
> +                * is modified when the PCM mute setting is enabled.
> +                */
> +               ac97_flush(w->codec, AC97_HEADPHONE);
> +       } else
>                ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
>
>        if (l & 0x4 || r & 0x4)
> @@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
>        return 0;
>  }
>
> +static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
> +{
> +       unsigned int val;
> +       u16 *cache = codec->reg_cache;
> +
> +       if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
> +               val = cache[reg >> 1];
> +               soc_ac97_ops.write(codec->ac97, reg, val);
> +       }
> +}
> +
>  static int ac97_prepare(struct snd_pcm_substream *substream,
>                        struct snd_soc_dai *dai)
>  {
> --
> 1.6.0.4
>
>
> -------------------------
>
>
>
> On Sat, 2009-08-01 at 09:47 -0400, Jon Smirl wrote:
>> On Fri, Jul 31, 2009 at 7:08 PM, John Bonesio<bones@secretlab.ca> wrote:
>> > We've encountered strange behavior in the alsamixer settings using the wm9712
>> > codec. If we unmute the headphone output and then unmute the PCM output, the
>> > headphone output gets reset to mute in the hardware register. At this point
>> > the hardware register does not match the value in the register cache.
>> >
>> > I've spent some time debugging this, and the headphone setting is set outside
>> > of any code path that would call the ac97_write() routine. As best as I can
>> > tell, there is something strange going on in hardware.
>>
>> This could be something wrong in the mpc5200 code that writes the AC97
>> registers too. Maybe a register write command is getting generated
>> when it wasn't supposed to.
>>
>> You can set the mpc5200 to generate an interrupt everything it
>> finishes writing a AC97 register. If you get more interrupts than you
>> expected the problem is in the driver.
>>
>> > I've provided this patch that works around the problem.
>> >
>> > Have any of you seen this before? Is this patch the right approach?
>> >
>> > - John
>> >
>> > The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
>> > s->runtime->control->appl_ptr can wrap around to the beginning of the
>> > buffer. This change fixes this problem.
>> >
>> > Signed-off-by: John Bonesio <bones@secretlab.ca>
>> > ---
>> >
>> >  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>> >  1 files changed, 17 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
>> > index cfe0ea4..2551c58 100644
>> > --- a/sound/soc/fsl/mpc5200_dma.c
>> > +++ b/sound/soc/fsl/mpc5200_dma.c
>> > @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>> >
>> >  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>> >  {
>> > +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
>> > +               /*
>> > +                * In this case s->runtime->control->appl_ptr has wrapped around.
>> > +                * Play the data to the end of the boundary, then wrap our own
>> > +                * appl_ptr back around.
>> > +                */
>> > +               while (s->appl_ptr < s->runtime->boundary) {
>> > +                       if (bcom_queue_full(s->bcom_task))
>> > +                               return;
>> > +
>> > +                       s->appl_ptr += s->period_size;
>> > +
>> > +                       psc_dma_bcom_enqueue_next_buffer(s);
>> > +               }
>> > +               s->appl_ptr -= s->runtime->boundary;
>> > +       }
>> > +
>> >        while (s->appl_ptr < s->runtime->control->appl_ptr) {
>> >
>> >                if (bcom_queue_full(s->bcom_task))
>> >
>> >
>>
>>
>>
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Mark Brown]

On Sat, Aug 01, 2009 at 08:04:56AM -0700, John Bonesio wrote:

> When I umuted the PCM mixer setting, this printk text did not appear,
> yet when I read the mixer register setting for the headphone, it was set
> back to mute.

Like I said before, exactly which control are you adjusting here?

> If there is code in software doing this, it's very subtle.

My money would be on the AC97 controller having problems; the quality of
SoC AC97 controllers is variable.  It certainly doesn't sound like a
WM9712 issue; as I say I'd be very surprised if such an issue hadn't
come up before given how widely deployed the part is.

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[John Bonesio]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

Hi Mark,

This is another patch for ALSA on the MPC5200 that was sent to the ALSA
devel list. I'm not sure if you noticed it.

Since I've submitted the patch to the list, we've been able to test it,
and audio continues to play right through appl_ptr wrapping back around
to the beginning of the buffer.

I've attached the patch, for your convenience.

- John

On Wed, 2009-07-29 at 08:38 -0700, John Bonesio wrote:
> Here's another appl_ptr related patch to the alsa soc mpc5200 ac97 driver. This 
> time the patch is to fix a problem with not correctly handling appl_ptr
> wrapping back around to the beginning of the buffer.
> 
> I haven't yet seen a symptom related to this. This came up when I was chatting
> with Jon Smirl about another bug I was examining. Right now there is another
> problem that is preventing me from playing really long audio streams. 
> 
> Let me know if this is the wrong approach.
> 
> - John
> 
> The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> s->runtime->control->appl_ptr can wrap around to the beginning of the
> buffer. This change fixes this problem.
> 
> Signed-off-by: John Bonesio <bones@secretlab.ca>
> ---
> 
>  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index cfe0ea4..2551c58 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>  
>  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>  {
> +	if (s->appl_ptr > s->runtime->control->appl_ptr) {
> +		/*
> +		 * In this case s->runtime->control->appl_ptr has wrapped around.
> +		 * Play the data to the end of the boundary, then wrap our own
> +		 * appl_ptr back around.
> +		 */
> +		while (s->appl_ptr < s->runtime->boundary) {
> +			if (bcom_queue_full(s->bcom_task))
> +				return;
> +
> +			s->appl_ptr += s->period_size;
> +
> +			psc_dma_bcom_enqueue_next_buffer(s);
> +		}
> +		s->appl_ptr -= s->runtime->boundary;
> +	}
> +
>  	while (s->appl_ptr < s->runtime->control->appl_ptr) {
>  
>  		if (bcom_queue_full(s->bcom_task))
> 

[-- Attachment #2: 0001-ASoC-MPC5200-Support-for-buffer-wrap-around.patch --]
[-- Type: text/x-patch, Size: 1451 bytes --]

>From 4df451365d98bd33b27fc61896fad32bcbc5692b Mon Sep 17 00:00:00 2001
From: John Bonesio <bones@secretlab.ca>
Date: Tue, 28 Jul 2009 15:51:07 -0700
Subject: [PATCH] ASoC: MPC5200: Support for buffer wrap around

The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
s->runtime->control->appl_ptr can wrap around to the beginning of the
buffer. This change fixes this problem.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---
 sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index cfe0ea4..2551c58 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
 
 static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
 {
+	if (s->appl_ptr > s->runtime->control->appl_ptr) {
+		/*
+		 * In this case s->runtime->control->appl_ptr has wrapped around.
+		 * Play the data to the end of the boundary, then wrap our own
+		 * appl_ptr back around.
+		 */
+		while (s->appl_ptr < s->runtime->boundary) {
+			if (bcom_queue_full(s->bcom_task))
+				return;
+
+			s->appl_ptr += s->period_size;
+
+			psc_dma_bcom_enqueue_next_buffer(s);
+		}
+		s->appl_ptr -= s->runtime->boundary;
+	}
+
 	while (s->appl_ptr < s->runtime->control->appl_ptr) {
 
 		if (bcom_queue_full(s->bcom_task))
-- 
1.6.0.4


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Mark Brown]

On Wed, Aug 05, 2009 at 09:44:32AM -0700, John Bonesio wrote:

> This is another patch for ALSA on the MPC5200 that was sent to the ALSA
> devel list. I'm not sure if you noticed it.

> Since I've submitted the patch to the list, we've been able to test it,
> and audio continues to play right through appl_ptr wrapping back around
> to the beginning of the buffer.

> I've attached the patch, for your convenience.

I've been waiting for one of the driver maintainers (Grant or Jon) to
comment on it.

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Grant Likely]

On Wed, Aug 5, 2009 at 11:05 AM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Aug 05, 2009 at 09:44:32AM -0700, John Bonesio wrote:
>
>> This is another patch for ALSA on the MPC5200 that was sent to the ALSA
>> devel list. I'm not sure if you noticed it.
>
>> Since I've submitted the patch to the list, we've been able to test it,
>> and audio continues to play right through appl_ptr wrapping back around
>> to the beginning of the buffer.
>
>> I've attached the patch, for your convenience.
>
> I've been waiting for one of the driver maintainers (Grant or Jon) to
> comment on it.

Heh.... helps if I post my approval publicly instead of just talking
to John over IM.

The patch looks correct and I know that it works because John got me
to help with testing.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

An Ack from Jon would be nice too.

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Jon Smirl]

On Wed, Aug 5, 2009 at 2:05 PM, Grant Likely<grant.likely@secretlab.ca> wrote:
> The patch looks correct and I know that it works because John got me
> to help with testing.
>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
> An Ack from Jon would be nice too.

It looks ok to me. I have my hardware torn up so I can't run anything.
I am rebuilding it to work on I2S.

I wish the upper layers would deal with appl_ptr instead of making the
drivers do it.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Mark Brown]

On Wed, Aug 05, 2009 at 02:34:38PM -0400, Jon Smirl wrote:
> On Wed, Aug 5, 2009 at 2:05 PM, Grant Likely<grant.likely@secretlab.ca> wrote:

> > Acked-by: Grant Likely <grant.likely@secretlab.ca>

> It looks ok to me. I have my hardware torn up so I can't run anything.
> I am rebuilding it to work on I2S.

Applied, thanks.

> I wish the upper layers would deal with appl_ptr instead of making the
> drivers do it.

wv :)