* Re: [PATCH 0/8] 8xx: Misc fixes for buggy insn
From: Joakim Tjernlund @ 2009-11-06 9:29 UTC (permalink / raw)
Cc: Scott Wood, linuxppc-dev@ozlabs.org, Rex Feany
In-Reply-To: <OF2C4359C9.0EB89938-ONC1257666.00299975-C1257666.002C206F@transmode.se>
>
> Scott Wood <scottwood@freescale.com> wrote on 06/11/2009 01:33:05:
> >
> > On Wed, Nov 04, 2009 at 02:38:32PM +0100, Joakim Tjernlund wrote:
> > > Here is the latest(last?) round of this series. I
> > > hope I got everything right now.
> > > Scott and Rex, please test and send ACK/NACK.
> > >
> > > Jocke
> > >
> > > Joakim Tjernlund (8):
> > > 8xx: invalidate non present TLBs
> >
> > This works, and is an important fix -- it should be applied even if the rest
> > of the patchset isn't ready.
>
> True.
>
> >
> > > 8xx: Update TLB asm so it behaves as linux mm expects.
>
> I think this is ready too.
>
> > > 8xx: Tag DAR with 0x00f0 to catch buggy instructions.
> >
> > Up through this point works.
>
> hmm, here tagging of DAR is in place, do you ever hit the
> page fault handler with address == 0x00f0? If you do,
> the kernel somehow manges to fix it instead of erroring out.
>
> I do notice one thing, I forgot to add the CPU6 errata to
> the DAR tagging. Are you using the CPU6 errata?
DAR isn't affected by CPU6 so this should not be a problem.
>
> >
> > > 8xx: Fixup DAR from buggy dcbX instructions.
> >
> > With this, the kernel hangs after "Mount-cache hash table entries: 512".
>
> Somewhat surprising result. I didn't expect you would even hit this
> condition now as we haven't enabled the use of dcbX insn yet.
> The only thing I can think of is the you hit the 0x00f0 due to other
> dcbX insn use and the kernel managed to fixup this in the page fault handler
> by pure luck before.
>
> The only thing I can thing of being wrong here is your suggested fix:
> + lis r11, (swapper_pg_dir-PAGE_OFFSET)@h
> + ori r11, r11, (swapper_pg_dir-PAGE_OFFSET)@l
> + rlwimi r11, r10, 22, 0xffc
>
> What if you change that back to what worked for you before:
> lis r11, swapper_pg_dir@h
> ori r11, r11, swapper_pg_dir@l
> rlwinm r11, r11, 0, 0x3ffff000
> rlwimi r11, r10, 22, 0xffc
> or possibly
> lis r11, swapper_pg_dir@h
> ori r11, r11, swapper_pg_dir@l
> subis r11, r11, PAGE_OFFSET@h
> rlwimi r11, r10, 22, 0xffc
>
> hmm, some missed CPU6 errata calls in DARFixup too.
Same here, not a problem
I did notice a bug that has been there a long time so
I don't think it is the problem:
+ add r10, r10, r25 ;b 151f
+ add r10, r10, r25 ;b 151f
should be r26 instead:
+ add r10, r10, r25 ;b 151f
+ add r10, r10, r26 ;b 151f
Jocke
^ permalink raw reply
* RE: DMA to User-Space
From: Jonathan Haws @ 2009-11-06 16:34 UTC (permalink / raw)
To: Chris Friesen; +Cc: Bo.Liu@windriver.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4AF3BCC4.6070702@nortel.com>
> > One more question about this approach: does the mmap() call
> prevent
> > the kernel from using this memory for other purposes? Will the
> > kernel be able to "move" this memory elsewhere? I guess what I am
> > asking is if this memory is locked for all other purposes?
>=20
> You've allocated the memory in the kernel and mapped it to
> userspace.
> If the kernel uses that memory for anything else it will be visible
> to
> userspace.
So, does that mean that the kernel could use that memory for something else=
? I was under the impression that locking the memory prevented the kernel =
from swapping it out, moving it, *and* using it for other purposes.
This memory that I am mapping is reserved for DMA buffers. If anything els=
e uses it, then that will corrupt my buffer and make it worthless. So basi=
cally I want user space to tell the kernel that that portion of memory is o=
ut of bounds. Is that possible?
Thanks,
Jonathan
^ permalink raw reply
* Re: DMA to User-Space
From: Alan Nishioka @ 2009-11-06 17:07 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <BB99A6BA28709744BF22A68E6D7EB51F0331031240@midas.usurf.usu.edu>
you can also reserve some physical memory at the top of memory and dma
into that.
of course, this means you have to know how much memory you need before
booting the kernel.
^ permalink raw reply
* RE: DMA to User-Space
From: Jonathan Haws @ 2009-11-06 18:47 UTC (permalink / raw)
To: Alan Nishioka, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4AF457D1.2040607@nishioka.com>
> you can also reserve some physical memory at the top of memory and
> dma
> into that.
> of course, this means you have to know how much memory you need
> before
> booting the kernel.
I had thought about that, however can I mmap into memory that area if I hav=
e set mem=3D250M in the boot arguments? I would plan to DMA to above 250M =
(up to 256M). I know that would keep the kernel out of it, but will /dev/m=
em be able to "see" that memory if I had told the kernel it was not there?
Thanks for the help!
Jonathan
^ permalink raw reply
* Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs
From: Jon Loeliger @ 2009-11-06 20:38 UTC (permalink / raw)
To: avorontsov
Cc: Kumar Gopalpet-B05799, netdev, linuxppc-dev,
Fleming Andy-AFLEMING, Jason Wessel, Stephen Hemminger,
David Miller, Lennert Buytenhek
In-Reply-To: <20091105175316.GA27099@oksana.dev.rtsoft.ru>
>
> Here is the patch on top of the Linus' git tree, if you haven't
> already 'back-ported' the previous patch.
This back-ported patch has been running in my (2.6.31) kernel
for a couple days now without showing any sign of problem.
Maybe throw a
Tested-by: Jon Loeliger <jdl@jdl.com>
at it?
jdl
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index 5bf31f1..5dca99c 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -1274,7 +1274,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct
> net_device *dev)
> u32 lstatus;
> int i;
> u32 bufaddr;
> - unsigned long flags;
> unsigned int nr_frags, length;
>
> base = priv->tx_bd_base;
> @@ -1298,14 +1297,11 @@ static int gfar_start_xmit(struct sk_buff *skb, struc
> t net_device *dev)
> /* total number of fragments in the SKB */
> nr_frags = skb_shinfo(skb)->nr_frags;
>
> - spin_lock_irqsave(&priv->txlock, flags);
> -
> /* check if there is space to queue this packet */
> if ((nr_frags+1) > priv->num_txbdfree) {
> /* no space, stop the queue */
> netif_stop_queue(dev);
> dev->stats.tx_fifo_errors++;
> - spin_unlock_irqrestore(&priv->txlock, flags);
> return NETDEV_TX_BUSY;
> }
>
> @@ -1403,9 +1399,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct
> net_device *dev)
> /* Tell the DMA to go go go */
> gfar_write(&priv->regs->tstat, TSTAT_CLEAR_THALT);
>
> - /* Unlock priv */
> - spin_unlock_irqrestore(&priv->txlock, flags);
> -
> return NETDEV_TX_OK;
> }
>
> @@ -1915,17 +1908,14 @@ static int gfar_poll(struct napi_struct *napi, int bu
> dget)
> struct net_device *dev = priv->ndev;
> int tx_cleaned = 0;
> int rx_cleaned = 0;
> - unsigned long flags;
>
> /* Clear IEVENT, so interrupts aren't called again
> * because of the packets that have already arrived */
> gfar_write(&priv->regs->ievent, IEVENT_RTX_MASK);
>
> - /* If we fail to get the lock, don't bother with the TX BDs */
> - if (spin_trylock_irqsave(&priv->txlock, flags)) {
> - tx_cleaned = gfar_clean_tx_ring(dev);
> - spin_unlock_irqrestore(&priv->txlock, flags);
> - }
> + netif_tx_lock_bh(priv->ndev);
> + tx_cleaned = gfar_clean_tx_ring(dev);
> + netif_tx_unlock_bh(priv->ndev);
>
> rx_cleaned = gfar_clean_rx_ring(dev, budget);
>
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH v3] powerpc/ppc64: Use preempt_schedule_irq instead of preempt_schedule
From: Valentine @ 2009-11-06 22:38 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: olof, linuxppc-dev, paulus
In-Reply-To: <1256777373.26770.14.camel@pasglop>
Benjamin Herrenschmidt wrote:
>> Yes, the MSR_EE is cleared before we jump to do_work. I'm OK with
>> clearing the hardirqenable flag. I just assumed that the hardirq flag
>> was supposed to reflect the MSR_EE state, so it looked a bit odd
>> clearing the MSR_EE at one place and then reflecting the change at another.
>
> Yeah well, it is supposed to reflect EE in the "general case", it's just
> that in the exception entry/exit, we take shortcuts when turning EE off
> for short amount of times without reflecting it in the PACA. This is
> why, in this case, since we are going back to C code, I want to have it
> "fixed up" to reflect reality.
>
Ben, this one works fine. Are you going to pick it?
Thanks,
Val.
^ permalink raw reply
* [patch 09/16] powerpc: Replace old style lock initializer
From: Thomas Gleixner @ 2009-11-06 22:41 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra, Ingo Molnar, linuxppc-dev
In-Reply-To: <20091106223547.784916750@linutronix.de>
SPIN_LOCK_UNLOCKED is deprecated. Init the lock array at runtime
instead.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@ozlabs.org
---
arch/powerpc/platforms/iseries/htab.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
Index: linux-2.6/arch/powerpc/platforms/iseries/htab.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/iseries/htab.c
+++ linux-2.6/arch/powerpc/platforms/iseries/htab.c
@@ -19,8 +19,7 @@
#include "call_hpt.h"
-static spinlock_t iSeries_hlocks[64] __cacheline_aligned_in_smp =
- { [0 ... 63] = SPIN_LOCK_UNLOCKED};
+static spinlock_t iSeries_hlocks[64] __cacheline_aligned_in_smp;
/*
* Very primitive algorithm for picking up a lock
@@ -245,6 +244,11 @@ static void iSeries_hpte_invalidate(unsi
void __init hpte_init_iSeries(void)
{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(iSeries_hlocks); i++)
+ spin_lock_init(&iSeries_hlocks[i]);
+
ppc_md.hpte_invalidate = iSeries_hpte_invalidate;
ppc_md.hpte_updatepp = iSeries_hpte_updatepp;
ppc_md.hpte_updateboltedpp = iSeries_hpte_updateboltedpp;
^ permalink raw reply
* Re: [PATCH v3] powerpc/ppc64: Use preempt_schedule_irq instead of preempt_schedule
From: Benjamin Herrenschmidt @ 2009-11-06 22:49 UTC (permalink / raw)
To: Valentine; +Cc: olof, linuxppc-dev, paulus
In-Reply-To: <4AF4A554.9010000@ru.mvista.com>
On Sat, 2009-11-07 at 01:38 +0300, Valentine wrote:
> Benjamin Herrenschmidt wrote:
> >> Yes, the MSR_EE is cleared before we jump to do_work. I'm OK with
> >> clearing the hardirqenable flag. I just assumed that the hardirq flag
> >> was supposed to reflect the MSR_EE state, so it looked a bit odd
> >> clearing the MSR_EE at one place and then reflecting the change at another.
> >
> > Yeah well, it is supposed to reflect EE in the "general case", it's just
> > that in the exception entry/exit, we take shortcuts when turning EE off
> > for short amount of times without reflecting it in the PACA. This is
> > why, in this case, since we are going back to C code, I want to have it
> > "fixed up" to reflect reality.
> >
>
> Ben, this one works fine. Are you going to pick it?
Already upstream:
Gitweb:
http://git.kernel.org/linus/4f917ba3d5ee9c98d60fa357e799942df8412de3
Commit: 4f917ba3d5ee9c98d60fa357e799942df8412de3
Parent: 01deab98e3ad8ff27243a8d5f8dd746c7110ae4f
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
AuthorDate: Mon Oct 26 19:41:17 2009 +0000
Committer: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CommitDate: Tue Oct 27 16:42:43 2009 +1100
powerpc/ppc64: Use preempt_schedule_irq instead of preempt_schedule
Based on an original patch by Valentine Barshak <vbarshak@ru.mvista.com>
Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
.../...
Now, it might be a good idea to do a -stable variant of it for 2.6.31
and back, but that will have to be a separate patch due to the new
Book3E churn in .32
Cheers,
Ben.
^ permalink raw reply
* Re: [patch 09/16] powerpc: Replace old style lock initializer
From: Benjamin Herrenschmidt @ 2009-11-06 22:55 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Peter Zijlstra, linuxppc-dev, Ingo Molnar, LKML
In-Reply-To: <20091106223806.762624109@linutronix.de>
On Fri, 2009-11-06 at 22:41 +0000, Thomas Gleixner wrote:
> plain text document attachment
> (power-replace-old-style-lock-init.patch)
> SPIN_LOCK_UNLOCKED is deprecated. Init the lock array at runtime
> instead.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@ozlabs.org
> ---
Looks reasonable. But iseries can be a bitch, so we do need to test it
on monday.
Cheers,
Ben.
> arch/powerpc/platforms/iseries/htab.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/arch/powerpc/platforms/iseries/htab.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/platforms/iseries/htab.c
> +++ linux-2.6/arch/powerpc/platforms/iseries/htab.c
> @@ -19,8 +19,7 @@
>
> #include "call_hpt.h"
>
> -static spinlock_t iSeries_hlocks[64] __cacheline_aligned_in_smp =
> - { [0 ... 63] = SPIN_LOCK_UNLOCKED};
> +static spinlock_t iSeries_hlocks[64] __cacheline_aligned_in_smp;
>
> /*
> * Very primitive algorithm for picking up a lock
> @@ -245,6 +244,11 @@ static void iSeries_hpte_invalidate(unsi
>
> void __init hpte_init_iSeries(void)
> {
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(iSeries_hlocks); i++)
> + spin_lock_init(&iSeries_hlocks[i]);
> +
> ppc_md.hpte_invalidate = iSeries_hpte_invalidate;
> ppc_md.hpte_updatepp = iSeries_hpte_updatepp;
> ppc_md.hpte_updateboltedpp = iSeries_hpte_updateboltedpp;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: help with unhandled IRQ error with mpt2sas driver and powerpc 460EX
From: Ayman El-Khashab @ 2009-11-07 2:47 UTC (permalink / raw)
Cc: linuxppc-dev
In-Reply-To: <1256689358.11607.139.camel@pasglop>
On 10/27/2009 7:22 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2009-10-27 at 12:27 -0500, Ayman El-Khashab wrote:
>
>> The first problem I noticed is that the physical address is read into a
>> 32 bit variable, but the 460ex has a 36 bit bus so the ioremap would
>> always fail. I've change the defn of chip_phys in mpt2sas_base.h to u64
>> and that cleared up that issue.
>>
>
> That looks indeed like a common driver bug. Please make sure you submit
> the fix upstream. The "right" type to use is resource_size_t in fact.
>
>
Thank you Benjamin for your help. The info was passed onto the
maintainer and they were able to use it
to improve their driver. In the background, the mpt2sas was brought up
on the canyonlands/460ex. The
biggest thing turned out to be an #ifdef in the code that referred to
BITS_32 to determine whether to prog
the high and low of the DMA addresses. Once that was adjusted, it did
function and detect the sas drive.
^ permalink raw reply
* [PATCH 0/6] Fixups to MPC5200 ASoC drivers
From: Grant Likely @ 2009-11-07 8:33 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
Hi everyone,
Jon, as we talked about earlier today, I've spent some time working
on the MPC5200 AC97 driver to try and get rid of some of the nagging
bugs. The two big changes that I ended up making were to remove the
tracking of appl_ptr (and all the race conditions that went with it),
and to fix the handling of AC97 slot enables/disables so that a
stream can be stopped and started again without going through the
hw_params() step.
I know that you had the appl_ptr tracking in to try and solve the
problem of audible noise at the end of playback. After discussing
the data flow model on #alsa-soc this morning, I learned that
the driver is really supposed to be free-running, and the ALSA layer
is supposed to be able to keep up. It is also responsible to ensure
that buffer is filled with silence at the end of playback to eliminate
noise before the trigger can properly turn everything off. Only a
couple of other drivers use appl_ptr, so I'm pretty sure this driver
shouldn't be doing it either.
Instead, from a recommendation this morning, I added a 'fudge' factor
to the value reported by the .pointer() callback of 1/4 the period
size. At first I thought this helped, but after more testing I find
that the driver seems to work correctly with aplay both with and
without the fudge factor applied.
However, I was able to reproduce the noise problem when using aplay
if I have DEBUG #defined at the top of the mpc5200_dma.c file with
debug console logs being spit out the serial port. In that situation,
the STOP trigger calls printk(), and a stale sample can be heard at
the end of playback. However, I believe this is a bug with the serial
console driver (in that it disables IRQs for a long time) causing
unbounded latencies, so the trigger doesn't get processed fast enough.
It doesn't help that aplay doesn't flush the buffers with silence
before triggering STOP. Other programs (like gstreamer) do seem to
flush out stale data before stopping the stream. I'm sure someone
will correct me if I'm making some bad assumptions here.
So, please test. Let me know if these work or not. I've don't know
if the last patch (Add fudge factor...) is needed or not.
Cheers,
g.
---
Grant Likely (6):
ASoC/mpc5200: Add fudge factor to value reported by .pointer()
ASoC/mpc5200: fix enable/disable of AC97 slots
ASoC/mpc5200: add to_psc_dma_stream() helper
ASoC/mpc5200: Improve printk debug output for trigger
ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
ASoC/mpc5200: Track DMA position by period number instead of bytes
sound/soc/fsl/mpc5200_dma.c | 98 ++++++++++----------------------------
sound/soc/fsl/mpc5200_dma.h | 24 ++++++---
sound/soc/fsl/mpc5200_psc_ac97.c | 39 ++++++++-------
3 files changed, 63 insertions(+), 98 deletions(-)
--
Signature
^ permalink raw reply
* [PATCH 1/6] ASoC/mpc5200: Track DMA position by period number instead of bytes
From: Grant Likely @ 2009-11-07 8:33 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
All DMA blocks are lined up to period boundaries, but the DMA
handling code tracks bytes instead. This patch reworks the code
to track the period index into the DMA buffer instead of the
physical address pointer. Doing so makes the code simpler and
easier to understand.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
sound/soc/fsl/mpc5200_dma.c | 28 +++++++++-------------------
sound/soc/fsl/mpc5200_dma.h | 8 ++------
2 files changed, 11 insertions(+), 25 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index 6096d22..986d3c8 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -58,13 +58,11 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
/* Prepare and enqueue the next buffer descriptor */
bd = bcom_prepare_next_buffer(s->bcom_task);
bd->status = s->period_bytes;
- bd->data[0] = s->period_next_pt;
+ bd->data[0] = s->runtime->dma_addr + (s->period_next * s->period_bytes);
bcom_submit_next_buffer(s->bcom_task, NULL);
/* Update for next period */
- s->period_next_pt += s->period_bytes;
- if (s->period_next_pt >= s->period_end)
- s->period_next_pt = s->period_start;
+ s->period_next = (s->period_next + 1) % s->runtime->periods;
}
static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
@@ -79,7 +77,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
if (bcom_queue_full(s->bcom_task))
return;
- s->appl_ptr += s->period_size;
+ s->appl_ptr += s->runtime->period_size;
psc_dma_bcom_enqueue_next_buffer(s);
}
@@ -91,7 +89,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
if (bcom_queue_full(s->bcom_task))
return;
- s->appl_ptr += s->period_size;
+ s->appl_ptr += s->runtime->period_size;
psc_dma_bcom_enqueue_next_buffer(s);
}
@@ -108,9 +106,7 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
while (bcom_buffer_done(s->bcom_task)) {
bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
- s->period_current_pt += s->period_bytes;
- if (s->period_current_pt >= s->period_end)
- s->period_current_pt = s->period_start;
+ s->period_current = (s->period_current+1) % s->runtime->periods;
}
psc_dma_bcom_enqueue_tx(s);
spin_unlock(&s->psc_dma->lock);
@@ -133,9 +129,7 @@ static irqreturn_t psc_dma_bcom_irq_rx(int irq, void *_psc_dma_stream)
while (bcom_buffer_done(s->bcom_task)) {
bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
- s->period_current_pt += s->period_bytes;
- if (s->period_current_pt >= s->period_end)
- s->period_current_pt = s->period_start;
+ s->period_current = (s->period_current+1) % s->runtime->periods;
psc_dma_bcom_enqueue_next_buffer(s);
}
@@ -185,12 +179,8 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_START:
s->period_bytes = frames_to_bytes(runtime,
runtime->period_size);
- s->period_start = virt_to_phys(runtime->dma_area);
- s->period_end = s->period_start +
- (s->period_bytes * runtime->periods);
- s->period_next_pt = s->period_start;
- s->period_current_pt = s->period_start;
- s->period_size = runtime->period_size;
+ s->period_next = 0;
+ s->period_current = 0;
s->active = 1;
/* track appl_ptr so that we have a better chance of detecting
@@ -343,7 +333,7 @@ psc_dma_pointer(struct snd_pcm_substream *substream)
else
s = &psc_dma->playback;
- count = s->period_current_pt - s->period_start;
+ count = s->period_current * s->period_bytes;
return bytes_to_frames(substream->runtime, count);
}
diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index 8d396bb..529f7a0 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -13,7 +13,6 @@
* @psc_dma: pointer back to parent psc_dma data structure
* @bcom_task: bestcomm task structure
* @irq: irq number for bestcomm task
- * @period_start: physical address of start of DMA region
* @period_end: physical address of end of DMA region
* @period_next_pt: physical address of next DMA buffer to enqueue
* @period_bytes: size of DMA period in bytes
@@ -27,12 +26,9 @@ struct psc_dma_stream {
struct bcom_task *bcom_task;
int irq;
struct snd_pcm_substream *stream;
- dma_addr_t period_start;
- dma_addr_t period_end;
- dma_addr_t period_next_pt;
- dma_addr_t period_current_pt;
+ int period_next;
+ int period_current;
int period_bytes;
- int period_size;
};
/**
^ permalink raw reply related
* [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
From: Grant Likely @ 2009-11-07 8:34 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
Sound drivers PCM DMA is supposed to free-run until told to stop
by the trigger callback. The current code tries to track appl_ptr,
to avoid stale buffer data getting played out at the end of the
data stream. Unfortunately it also results in race conditions
which can cause the audio to stall.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
sound/soc/fsl/mpc5200_dma.c | 52 +++++++------------------------------------
sound/soc/fsl/mpc5200_dma.h | 2 --
2 files changed, 8 insertions(+), 46 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index 986d3c8..4e47586 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -65,36 +65,6 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
s->period_next = (s->period_next + 1) % s->runtime->periods;
}
-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->runtime->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))
- return;
-
- s->appl_ptr += s->runtime->period_size;
-
- psc_dma_bcom_enqueue_next_buffer(s);
- }
-}
-
/* Bestcomm DMA irq handler */
static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
{
@@ -107,8 +77,9 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
s->period_current = (s->period_current+1) % s->runtime->periods;
+
+ psc_dma_bcom_enqueue_next_buffer(s);
}
- psc_dma_bcom_enqueue_tx(s);
spin_unlock(&s->psc_dma->lock);
/* If the stream is active, then also inform the PCM middle layer
@@ -182,28 +153,21 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
s->period_next = 0;
s->period_current = 0;
s->active = 1;
-
- /* track appl_ptr so that we have a better chance of detecting
- * end of stream and not over running it.
- */
s->runtime = runtime;
- s->appl_ptr = s->runtime->control->appl_ptr -
- (runtime->period_size * runtime->periods);
/* Fill up the bestcomm bd queue and enable DMA.
* This will begin filling the PSC's fifo.
*/
spin_lock_irqsave(&psc_dma->lock, flags);
- if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) {
+ if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
bcom_gen_bd_rx_reset(s->bcom_task);
- for (i = 0; i < runtime->periods; i++)
- if (!bcom_queue_full(s->bcom_task))
- psc_dma_bcom_enqueue_next_buffer(s);
- } else {
+ else
bcom_gen_bd_tx_reset(s->bcom_task);
- psc_dma_bcom_enqueue_tx(s);
- }
+
+ for (i = 0; i < runtime->periods; i++)
+ if (!bcom_queue_full(s->bcom_task))
+ psc_dma_bcom_enqueue_next_buffer(s);
bcom_enable(s->bcom_task);
spin_unlock_irqrestore(&psc_dma->lock, flags);
diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index 529f7a0..d9c741b 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -19,8 +19,6 @@
*/
struct psc_dma_stream {
struct snd_pcm_runtime *runtime;
- snd_pcm_uframes_t appl_ptr;
-
int active;
struct psc_dma *psc_dma;
struct bcom_task *bcom_task;
^ permalink raw reply related
* [PATCH 3/6] ASoC/mpc5200: Improve printk debug output for trigger
From: Grant Likely @ 2009-11-07 8:34 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
sound/soc/fsl/mpc5200_dma.c | 15 ++++++++++-----
sound/soc/fsl/mpc5200_dma.h | 1 +
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index 4e47586..658e3fa 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -77,6 +77,7 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
s->period_current = (s->period_current+1) % s->runtime->periods;
+ s->period_count++;
psc_dma_bcom_enqueue_next_buffer(s);
}
@@ -101,6 +102,7 @@ static irqreturn_t psc_dma_bcom_irq_rx(int irq, void *_psc_dma_stream)
bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
s->period_current = (s->period_current+1) % s->runtime->periods;
+ s->period_count++;
psc_dma_bcom_enqueue_next_buffer(s);
}
@@ -142,17 +144,17 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
else
s = &psc_dma->playback;
- dev_dbg(psc_dma->dev, "psc_dma_trigger(substream=%p, cmd=%i)"
- " stream_id=%i\n",
- substream, cmd, substream->pstr->stream);
-
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
+ dev_dbg(psc_dma->dev, "START: stream=%i fbits=%u ps=%u #p=%u\n",
+ substream->pstr->stream, runtime->frame_bits,
+ (int)runtime->period_size, runtime->periods);
s->period_bytes = frames_to_bytes(runtime,
runtime->period_size);
s->period_next = 0;
s->period_current = 0;
s->active = 1;
+ s->period_count = 0;
s->runtime = runtime;
/* Fill up the bestcomm bd queue and enable DMA.
@@ -177,6 +179,8 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
break;
case SNDRV_PCM_TRIGGER_STOP:
+ dev_dbg(psc_dma->dev, "STOP: stream=%i periods_count=%i\n",
+ substream->pstr->stream, s->period_count);
s->active = 0;
spin_lock_irqsave(&psc_dma->lock, flags);
@@ -190,7 +194,8 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
break;
default:
- dev_dbg(psc_dma->dev, "invalid command\n");
+ dev_dbg(psc_dma->dev, "unhandled trigger: stream=%i cmd=%i\n",
+ substream->pstr->stream, cmd);
return -EINVAL;
}
diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index d9c741b..c6f29e4 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -27,6 +27,7 @@ struct psc_dma_stream {
int period_next;
int period_current;
int period_bytes;
+ int period_count;
};
/**
^ permalink raw reply related
* [PATCH 4/6] ASoC/mpc5200: add to_psc_dma_stream() helper
From: Grant Likely @ 2009-11-07 8:34 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
Move the resolving of the psc_dma_stream pointer to a helper function
to reduce duplicate code
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
sound/soc/fsl/mpc5200_dma.c | 7 +------
sound/soc/fsl/mpc5200_dma.h | 9 +++++++++
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index 658e3fa..9c88e15 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -133,17 +133,12 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data;
struct snd_pcm_runtime *runtime = substream->runtime;
- struct psc_dma_stream *s;
+ struct psc_dma_stream *s = to_psc_dma_stream(substream, psc_dma);
struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs;
u16 imr;
unsigned long flags;
int i;
- if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
- s = &psc_dma->capture;
- else
- s = &psc_dma->playback;
-
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
dev_dbg(psc_dma->dev, "START: stream=%i fbits=%u ps=%u #p=%u\n",
diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index c6f29e4..956d6a5 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -68,6 +68,15 @@ struct psc_dma {
} stats;
};
+/* Utility for retrieving psc_dma_stream structure from a substream */
+inline struct psc_dma_stream *
+to_psc_dma_stream(struct snd_pcm_substream *substream, struct psc_dma *psc_dma)
+{
+ if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
+ return &psc_dma->capture;
+ return &psc_dma->playback;
+}
+
int mpc5200_audio_dma_create(struct of_device *op);
int mpc5200_audio_dma_destroy(struct of_device *op);
^ permalink raw reply related
* [PATCH 5/6] ASoC/mpc5200: fix enable/disable of AC97 slots
From: Grant Likely @ 2009-11-07 8:34 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
The MPC5200 AC97 driver is disabling the slots when a stop
trigger is received, but not reenabling them if the stream
is started again without processing the hw_params again.
This patch fixes the problem by caching the slot enable bit
settings calculated at hw_params time so that they can be
reapplied every time the start trigger is received.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
sound/soc/fsl/mpc5200_dma.h | 4 ++++
sound/soc/fsl/mpc5200_psc_ac97.c | 39 ++++++++++++++++++++------------------
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index 956d6a5..22208b3 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -16,6 +16,7 @@
* @period_end: physical address of end of DMA region
* @period_next_pt: physical address of next DMA buffer to enqueue
* @period_bytes: size of DMA period in bytes
+ * @ac97_slot_bits: Enable bits for turning on the correct AC97 slot
*/
struct psc_dma_stream {
struct snd_pcm_runtime *runtime;
@@ -28,6 +29,9 @@ struct psc_dma_stream {
int period_current;
int period_bytes;
int period_count;
+
+ /* AC97 state */
+ u32 ac97_slot_bits;
};
/**
diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
index c4ae3e0..3dbc7f7 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -130,6 +130,7 @@ static int psc_ac97_hw_analog_params(struct snd_pcm_substream *substream,
struct snd_soc_dai *cpu_dai)
{
struct psc_dma *psc_dma = cpu_dai->private_data;
+ struct psc_dma_stream *s = to_psc_dma_stream(substream, psc_dma);
dev_dbg(psc_dma->dev, "%s(substream=%p) p_size=%i p_bytes=%i"
" periods=%i buffer_size=%i buffer_bytes=%i channels=%i"
@@ -140,20 +141,10 @@ static int psc_ac97_hw_analog_params(struct snd_pcm_substream *substream,
params_channels(params), params_rate(params),
params_format(params));
-
- if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) {
- if (params_channels(params) == 1)
- psc_dma->slots |= 0x00000100;
- else
- psc_dma->slots |= 0x00000300;
- } else {
- if (params_channels(params) == 1)
- psc_dma->slots |= 0x01000000;
- else
- psc_dma->slots |= 0x03000000;
- }
- out_be32(&psc_dma->psc_regs->ac97_slots, psc_dma->slots);
-
+ /* Determine the set of enable bits to turn on */
+ s->ac97_slot_bits = (params_channels(params) == 1) ? 0x100 : 0x300;
+ if (substream->pstr->stream != SNDRV_PCM_STREAM_CAPTURE)
+ s->ac97_slot_bits <<= 16;
return 0;
}
@@ -163,6 +154,8 @@ static int psc_ac97_hw_digital_params(struct snd_pcm_substream *substream,
{
struct psc_dma *psc_dma = cpu_dai->private_data;
+ dev_dbg(psc_dma->dev, "%s(substream=%p)\n", __func__, substream);
+
if (params_channels(params) == 1)
out_be32(&psc_dma->psc_regs->ac97_slots, 0x01000000);
else
@@ -176,14 +169,24 @@ static int psc_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data;
+ struct psc_dma_stream *s = to_psc_dma_stream(substream, psc_dma);
switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ dev_dbg(psc_dma->dev, "AC97 START: stream=%i\n",
+ substream->pstr->stream);
+
+ /* Set the slot enable bits */
+ psc_dma->slots |= s->ac97_slot_bits;
+ out_be32(&psc_dma->psc_regs->ac97_slots, psc_dma->slots);
+ break;
+
case SNDRV_PCM_TRIGGER_STOP:
- if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
- psc_dma->slots &= 0xFFFF0000;
- else
- psc_dma->slots &= 0x0000FFFF;
+ dev_dbg(psc_dma->dev, "AC97 STOP: stream=%i\n",
+ substream->pstr->stream);
+ /* Clear the slot enable bits */
+ psc_dma->slots &= ~(s->ac97_slot_bits);
out_be32(&psc_dma->psc_regs->ac97_slots, psc_dma->slots);
break;
}
^ permalink raw reply related
* [PATCH 6/6] ASoC/mpc5200: Add fudge factor to value reported by .pointer()
From: Grant Likely @ 2009-11-07 8:34 UTC (permalink / raw)
To: linuxppc-dev, alsa-devel, broonie; +Cc: lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
ALSA playback seems to be more reliable if the .pointer() hook reports
a value slightly into the period, rather than right on the period
boundary. This patch adds a fudge factor of 1/4 the period size
to better estimate the actual position of the DMA engine in the
audio buffer.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
sound/soc/fsl/mpc5200_dma.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index 9c88e15..ee5a606 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -297,7 +297,7 @@ psc_dma_pointer(struct snd_pcm_substream *substream)
else
s = &psc_dma->playback;
- count = s->period_current * s->period_bytes;
+ count = (s->period_current * s->period_bytes) + (s->period_bytes >> 2);
return bytes_to_frames(substream->runtime, count);
}
^ permalink raw reply related
* Re: [alsa-devel] [PATCH 1/6] ASoC/mpc5200: Track DMA position by period number instead of bytes
From: Liam Girdwood @ 2009-11-07 10:35 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, broonie, linuxppc-dev
In-Reply-To: <20091107083345.18908.96473.stgit@angua>
On Sat, 2009-11-07 at 01:33 -0700, Grant Likely wrote:
> All DMA blocks are lined up to period boundaries, but the DMA
> handling code tracks bytes instead. This patch reworks the code
> to track the period index into the DMA buffer instead of the
> physical address pointer. Doing so makes the code simpler and
> easier to understand.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Very minor coding style thing below otherwise all get my Ack.
Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
> ---
>
> sound/soc/fsl/mpc5200_dma.c | 28 +++++++++-------------------
> sound/soc/fsl/mpc5200_dma.h | 8 ++------
> 2 files changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index 6096d22..986d3c8 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -58,13 +58,11 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
> /* Prepare and enqueue the next buffer descriptor */
> bd = bcom_prepare_next_buffer(s->bcom_task);
> bd->status = s->period_bytes;
> - bd->data[0] = s->period_next_pt;
> + bd->data[0] = s->runtime->dma_addr + (s->period_next * s->period_bytes);
> bcom_submit_next_buffer(s->bcom_task, NULL);
>
> /* Update for next period */
> - s->period_next_pt += s->period_bytes;
> - if (s->period_next_pt >= s->period_end)
> - s->period_next_pt = s->period_start;
> + s->period_next = (s->period_next + 1) % s->runtime->periods;
> }
>
> static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> @@ -79,7 +77,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> if (bcom_queue_full(s->bcom_task))
> return;
>
> - s->appl_ptr += s->period_size;
> + s->appl_ptr += s->runtime->period_size;
>
> psc_dma_bcom_enqueue_next_buffer(s);
> }
> @@ -91,7 +89,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> if (bcom_queue_full(s->bcom_task))
> return;
>
> - s->appl_ptr += s->period_size;
> + s->appl_ptr += s->runtime->period_size;
>
> psc_dma_bcom_enqueue_next_buffer(s);
> }
> @@ -108,9 +106,7 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
> while (bcom_buffer_done(s->bcom_task)) {
> bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
>
> - s->period_current_pt += s->period_bytes;
> - if (s->period_current_pt >= s->period_end)
> - s->period_current_pt = s->period_start;
> + s->period_current = (s->period_current+1) % s->runtime->periods;
I prefer a space around operators.
s->period_current = (s->period_current + 1) % s->runtime->periods;
Liam
^ permalink raw reply
* Re: [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
From: Jon Smirl @ 2009-11-07 12:51 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, broonie, linuxppc-dev, lrg
In-Reply-To: <20091107083358.18908.10635.stgit@angua>
On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely <grant.likely@secretlab.ca> wr=
ote:
> Sound drivers PCM DMA is supposed to free-run until told to stop
> by the trigger callback. =A0The current code tries to track appl_ptr,
> to avoid stale buffer data getting played out at the end of the
> data stream. =A0Unfortunately it also results in race conditions
> which can cause the audio to stall.
I leave in an hour and I will be off net for a week so I can't look at thes=
e.
The problem at end of stream works like this:
last buffer containing music plays
this buffer has been padded with zero to make a full sample
interrupt occurs at end of buffer
--- at this point the next chained buffer starts playing, it is full of ju=
nk
--- this chaining happens in hardware
Alsa processes the callback and sends stop stream
--- oops, too late, buffer full of noise has already played several samples
--- these samples of noise are clearly audible.
--- they are usually a fragment from earlier in the song.
Using aplay with short clips like the action sounds for pidgin, etc
makes these noise bursts obviously visible.
To fix this you need a mechanism to determine where the valid data in
the buffering system ends and noise starts. Once you know the extent
of the valid data we can keep the DMA channel programmed to not play
invalid data. You can play off the end of valid data two ways; under
run when ALSA doesn't supply data fast enough and end of buffer.
ALSA does not provide information on where valid data ends in easily
consumable form so I've been trying to reconstruct it from appl_ptr.
A much cleaner solution would be for ALSA to provide a field that
indicates the last valid address in the ring buffer system. Then in
the driver's buffer complete callback I could get that value and
reprogram the DMA engine not to run off the end of valid data. As each
buffer completes I would reread the value and update the DMA stop
address. You also need the last valid address field when DMA is first
started.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> =A0sound/soc/fsl/mpc5200_dma.c | =A0 52 +++++++--------------------------=
----------
> =A0sound/soc/fsl/mpc5200_dma.h | =A0 =A02 --
> =A02 files changed, 8 insertions(+), 46 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index 986d3c8..4e47586 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -65,36 +65,6 @@ static void psc_dma_bcom_enqueue_next_buffer(struct ps=
c_dma_stream *s)
> =A0 =A0 =A0 =A0s->period_next =3D (s->period_next + 1) % s->runtime->peri=
ods;
> =A0}
>
> -static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> -{
> - =A0 =A0 =A0 if (s->appl_ptr > s->runtime->control->appl_ptr) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* In this case s->runtime->control->appl=
_ptr has wrapped around.
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* Play the data to the end of the bounda=
ry, then wrap our own
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* appl_ptr back around.
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 while (s->appl_ptr < s->runtime->boundary) =
{
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bcom_queue_full(s->bcom=
_task))
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr +=3D s->runtime=
->period_size;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_next_b=
uffer(s);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr -=3D s->runtime->boundary;
> - =A0 =A0 =A0 }
> -
> - =A0 =A0 =A0 while (s->appl_ptr < s->runtime->control->appl_ptr) {
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bcom_queue_full(s->bcom_task))
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr +=3D s->runtime->period_size;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_next_buffer(s);
> - =A0 =A0 =A0 }
> -}
> -
> =A0/* Bestcomm DMA irq handler */
> =A0static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
> =A0{
> @@ -107,8 +77,9 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *=
_psc_dma_stream)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_retrieve_buffer(s->bcom_task, NULL, N=
ULL);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->period_current =3D (s->period_current+1=
) % s->runtime->periods;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_next_buffer(s);
> =A0 =A0 =A0 =A0}
> - =A0 =A0 =A0 psc_dma_bcom_enqueue_tx(s);
> =A0 =A0 =A0 =A0spin_unlock(&s->psc_dma->lock);
>
> =A0 =A0 =A0 =A0/* If the stream is active, then also inform the PCM middl=
e layer
> @@ -182,28 +153,21 @@ static int psc_dma_trigger(struct snd_pcm_substream=
*substream, int cmd)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->period_next =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->period_current =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->active =3D 1;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* track appl_ptr so that we have a better =
chance of detecting
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* end of stream and not over running it.
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->runtime =3D runtime;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr =3D s->runtime->control->appl_p=
tr -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (runtime->p=
eriod_size * runtime->periods);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Fill up the bestcomm bd queue and enabl=
e DMA.
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * This will begin filling the PSC's fifo.
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spin_lock_irqsave(&psc_dma->lock, flags);
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (substream->pstr->stream =3D=3D SNDRV_PC=
M_STREAM_CAPTURE) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (substream->pstr->stream =3D=3D SNDRV_PC=
M_STREAM_CAPTURE)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_gen_bd_rx_reset(s->bc=
om_task);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; i < runtime->=
periods; i++)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!bcom_q=
ueue_full(s->bcom_task))
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 psc_dma_bcom_enqueue_next_buffer(s);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_gen_bd_tx_reset(s->bc=
om_task);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_tx(s);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; i < runtime->periods; i++)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!bcom_queue_full(s->bco=
m_task))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bco=
m_enqueue_next_buffer(s);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_enable(s->bcom_task);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spin_unlock_irqrestore(&psc_dma->lock, fla=
gs);
> diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
> index 529f7a0..d9c741b 100644
> --- a/sound/soc/fsl/mpc5200_dma.h
> +++ b/sound/soc/fsl/mpc5200_dma.h
> @@ -19,8 +19,6 @@
> =A0*/
> =A0struct psc_dma_stream {
> =A0 =A0 =A0 =A0struct snd_pcm_runtime *runtime;
> - =A0 =A0 =A0 snd_pcm_uframes_t appl_ptr;
> -
> =A0 =A0 =A0 =A0int active;
> =A0 =A0 =A0 =A0struct psc_dma *psc_dma;
> =A0 =A0 =A0 =A0struct bcom_task *bcom_task;
>
>
--=20
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [alsa-devel] [PATCH 4/6] ASoC/mpc5200: add to_psc_dma_stream() helper
From: Mark Brown @ 2009-11-07 12:33 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, linuxppc-dev, lrg
In-Reply-To: <20091107083423.18908.21177.stgit@angua>
On Sat, Nov 07, 2009 at 01:34:31AM -0700, Grant Likely wrote:
> +/* Utility for retrieving psc_dma_stream structure from a substream */
> +inline struct psc_dma_stream *
> +to_psc_dma_stream(struct snd_pcm_substream *substream, struct psc_dma *psc_dma)
> +{
> + if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> + return &psc_dma->capture;
> + return &psc_dma->playback;
> +}
Traditionally this'd be an if () else but it makes no difference either
way to the generated code.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 0/6] Fixups to MPC5200 ASoC drivers
From: Mark Brown @ 2009-11-07 12:57 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, linuxppc-dev, lrg
In-Reply-To: <20091107081631.18908.82921.stgit@angua>
On Sat, Nov 07, 2009 at 01:33:40AM -0700, Grant Likely wrote:
> However, I was able to reproduce the noise problem when using aplay
> if I have DEBUG #defined at the top of the mpc5200_dma.c file with
> debug console logs being spit out the serial port. In that situation,
> the STOP trigger calls printk(), and a stale sample can be heard at
> the end of playback. However, I believe this is a bug with the serial
> console driver (in that it disables IRQs for a long time) causing
> unbounded latencies, so the trigger doesn't get processed fast enough.
Yes, that will always be a problem with free running DMA - if it's not
shut down quickly enough then it'll just keep processing data. Serial
ports don't tend to play well with instrumenting it, sadly.
> So, please test. Let me know if these work or not. I've don't know
> if the last patch (Add fudge factor...) is needed or not.
I've applied patches 1-5 since they seem fairly clear code cleanups and
fixes. If they've introduced any problems we can fix them incrementally.
I'll wait to see what the outcome of testing is for patch 6.
As I mentioned on IRC testing with PulseAudio would be good for this -
it makes more demands on the quality of the DMA implementation than most
applications.
^ permalink raw reply
* Re: [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
From: Jon Smirl @ 2009-11-07 13:04 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, broonie, linuxppc-dev, lrg
In-Reply-To: <9e4733910911070451y28073dbeke6ced6246f5a5c24@mail.gmail.com>
On Sat, Nov 7, 2009 at 7:51 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
> On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely <grant.likely@secretlab.ca> =
wrote:
>> Sound drivers PCM DMA is supposed to free-run until told to stop
>> by the trigger callback. =A0The current code tries to track appl_ptr,
>> to avoid stale buffer data getting played out at the end of the
>> data stream. =A0Unfortunately it also results in race conditions
>> which can cause the audio to stall.
>
> I leave in an hour and I will be off net for a week so I can't look at th=
ese.
There is a surefire way to fix this but I have resisted doing it
because it is fixing a symptom not a cause.
Simply have the driver zero out the buffer in the completion interrupt
before handing it back to ALSA. Then if ALSA lets us play invalid data
the invalid data will be silence. I implemented this and it works
every time.
Downside is a big memset() in an IRQ handler.
> The problem at end of stream works like this:
>
> last buffer containing music plays
> this buffer has been padded with zero to make a full sample
> interrupt occurs at end of buffer
> =A0--- at this point the next chained buffer starts playing, it is full o=
f junk
> =A0--- this chaining happens in hardware
> Alsa processes the callback and sends stop stream
> --- oops, too late, buffer full of noise has already played several sampl=
es
> --- these samples of noise are clearly audible.
> --- they are usually a fragment from earlier in the song.
>
> Using aplay with short clips like the action sounds for pidgin, etc
> makes these noise bursts obviously visible.
>
> To fix this you need a mechanism to determine where the valid data in
> the buffering system ends and noise starts. Once you know the extent
> of the valid data we can keep the DMA channel programmed to not play
> invalid data. You can play off the end of valid data two ways; under
> run when ALSA doesn't supply data fast enough and end of buffer.
>
> ALSA does not provide information on where valid data ends in easily
> consumable form so I've been trying to reconstruct it from appl_ptr.
> A much cleaner solution would be for ALSA to provide a field that
> indicates the last valid address in the ring buffer system. Then in
> the driver's buffer complete callback I could get that value and
> reprogram the DMA engine not to run off the end of valid data. As each
> buffer completes I would reread the value and update the DMA stop
> address. You also need the last valid address field when DMA is first
> started.
>
>
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>>
>> =A0sound/soc/fsl/mpc5200_dma.c | =A0 52 +++++++-------------------------=
-----------
>> =A0sound/soc/fsl/mpc5200_dma.h | =A0 =A02 --
>> =A02 files changed, 8 insertions(+), 46 deletions(-)
>>
>> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
>> index 986d3c8..4e47586 100644
>> --- a/sound/soc/fsl/mpc5200_dma.c
>> +++ b/sound/soc/fsl/mpc5200_dma.c
>> @@ -65,36 +65,6 @@ static void psc_dma_bcom_enqueue_next_buffer(struct p=
sc_dma_stream *s)
>> =A0 =A0 =A0 =A0s->period_next =3D (s->period_next + 1) % s->runtime->per=
iods;
>> =A0}
>>
>> -static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>> -{
>> - =A0 =A0 =A0 if (s->appl_ptr > s->runtime->control->appl_ptr) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* In this case s->runtime->control->app=
l_ptr has wrapped around.
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* Play the data to the end of the bound=
ary, then wrap our own
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* appl_ptr back around.
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 while (s->appl_ptr < s->runtime->boundary)=
{
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bcom_queue_full(s->bco=
m_task))
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr +=3D s->runtim=
e->period_size;
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_next_=
buffer(s);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr -=3D s->runtime->boundary;
>> - =A0 =A0 =A0 }
>> -
>> - =A0 =A0 =A0 while (s->appl_ptr < s->runtime->control->appl_ptr) {
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bcom_queue_full(s->bcom_task))
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr +=3D s->runtime->period_size;
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_next_buffer(s);
>> - =A0 =A0 =A0 }
>> -}
>> -
>> =A0/* Bestcomm DMA irq handler */
>> =A0static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream=
)
>> =A0{
>> @@ -107,8 +77,9 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void =
*_psc_dma_stream)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_retrieve_buffer(s->bcom_task, NULL, =
NULL);
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->period_current =3D (s->period_current+=
1) % s->runtime->periods;
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_next_buffer(s);
>> =A0 =A0 =A0 =A0}
>> - =A0 =A0 =A0 psc_dma_bcom_enqueue_tx(s);
>> =A0 =A0 =A0 =A0spin_unlock(&s->psc_dma->lock);
>>
>> =A0 =A0 =A0 =A0/* If the stream is active, then also inform the PCM midd=
le layer
>> @@ -182,28 +153,21 @@ static int psc_dma_trigger(struct snd_pcm_substrea=
m *substream, int cmd)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->period_next =3D 0;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->period_current =3D 0;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->active =3D 1;
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* track appl_ptr so that we have a better=
chance of detecting
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* end of stream and not over running it=
.
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0s->runtime =3D runtime;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->appl_ptr =3D s->runtime->control->appl_=
ptr -
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (runtime->=
period_size * runtime->periods);
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Fill up the bestcomm bd queue and enab=
le DMA.
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * This will begin filling the PSC's fifo=
.
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spin_lock_irqsave(&psc_dma->lock, flags);
>>
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (substream->pstr->stream =3D=3D SNDRV_P=
CM_STREAM_CAPTURE) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (substream->pstr->stream =3D=3D SNDRV_P=
CM_STREAM_CAPTURE)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_gen_bd_rx_reset(s->b=
com_task);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; i < runtime-=
>periods; i++)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!bcom_=
queue_full(s->bcom_task))
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 psc_dma_bcom_enqueue_next_buffer(s);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_gen_bd_tx_reset(s->b=
com_task);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bcom_enqueue_tx(s)=
;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; i < runtime->periods; i++)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!bcom_queue_full(s->bc=
om_task))
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_dma_bc=
om_enqueue_next_buffer(s);
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_enable(s->bcom_task);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spin_unlock_irqrestore(&psc_dma->lock, fl=
ags);
>> diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
>> index 529f7a0..d9c741b 100644
>> --- a/sound/soc/fsl/mpc5200_dma.h
>> +++ b/sound/soc/fsl/mpc5200_dma.h
>> @@ -19,8 +19,6 @@
>> =A0*/
>> =A0struct psc_dma_stream {
>> =A0 =A0 =A0 =A0struct snd_pcm_runtime *runtime;
>> - =A0 =A0 =A0 snd_pcm_uframes_t appl_ptr;
>> -
>> =A0 =A0 =A0 =A0int active;
>> =A0 =A0 =A0 =A0struct psc_dma *psc_dma;
>> =A0 =A0 =A0 =A0struct bcom_task *bcom_task;
>>
>>
>
>
>
> --
> Jon Smirl
> jonsmirl@gmail.com
>
--=20
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/6] ASoC/mpc5200: Track DMA position by period number instead of bytes
From: Grant Likely @ 2009-11-07 16:50 UTC (permalink / raw)
To: Liam Girdwood; +Cc: alsa-devel, broonie, linuxppc-dev
In-Reply-To: <1257590153.3960.3.camel@odin>
On Sat, Nov 7, 2009 at 3:35 AM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> On Sat, 2009-11-07 at 01:33 -0700, Grant Likely wrote:
>> All DMA blocks are lined up to period boundaries, but the DMA
>> handling code tracks bytes instead. =A0This patch reworks the code
>> to track the period index into the DMA buffer instead of the
>> physical address pointer. =A0Doing so makes the code simpler and
>> easier to understand.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> Very minor coding style thing below otherwise all get my Ack.
>
> Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
Thanks Liam.
>> - =A0 =A0 =A0 =A0 =A0 =A0 s->period_current_pt +=3D s->period_bytes;
>> - =A0 =A0 =A0 =A0 =A0 =A0 if (s->period_current_pt >=3D s->period_end)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->period_current_pt =3D s->pe=
riod_start;
>> + =A0 =A0 =A0 =A0 =A0 =A0 s->period_current =3D (s->period_current+1) % =
s->runtime->periods;
>
> I prefer a space around operators.
>
> s->period_current =3D (s->period_current + 1) % s->runtime->periods;
So do I, but this kept the line length down below 80 chars. Avoiding
the line spillage this way looks nicer than the alternatives.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 0/6] Fixups to MPC5200 ASoC drivers
From: Grant Likely @ 2009-11-07 16:52 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, linuxppc-dev, lrg
In-Reply-To: <20091107125713.GF3228@sirena.org.uk>
On Sat, Nov 7, 2009 at 5:57 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Nov 07, 2009 at 01:33:40AM -0700, Grant Likely wrote:
>> So, please test. =A0Let me know if these work or not. =A0I've don't know
>> if the last patch (Add fudge factor...) is needed or not.
>
> I've applied patches 1-5 since they seem fairly clear code cleanups and
> fixes. =A0If they've introduced any problems we can fix them incrementall=
y.
> I'll wait to see what the outcome of testing is for patch 6.
Cool, thanks!
> As I mentioned on IRC testing with PulseAudio would be good for this -
> it makes more demands on the quality of the DMA implementation than most
> applications.
I had trouble getting pulseaudio to work on my headless system.
Couldn't get clients to connect. I'll hack on it again next week.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 6/6] ASoC/mpc5200: Add fudge factor to value reported by .pointer()
From: Mark Brown @ 2009-11-07 18:11 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, linuxppc-dev, lrg
In-Reply-To: <20091107083448.18908.80366.stgit@angua>
On Sat, Nov 07, 2009 at 01:34:55AM -0700, Grant Likely wrote:
> ALSA playback seems to be more reliable if the .pointer() hook reports
> a value slightly into the period, rather than right on the period
> boundary. This patch adds a fudge factor of 1/4 the period size
> to better estimate the actual position of the DMA engine in the
> audio buffer.
It occurs to me that in terms of dealing with what's going on here this
probably is achieving exactly the same effect as Jon's code in that it
tells ALSA that things are a bit ahead of where the buffer started.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox