| Message ID | 1335510185-7906-2-git-send-email-richard.zhao@freescale.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 27, 2012 at 03:02:55PM +0800, Richard Zhao wrote: > device_prep_dma_cyclic may be call in audio trigger function which is > atomic context, so we make it atomic too. > > - change channel0 lock to spinlock. > - Use polling to wait for channel0 finish running. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > --- > drivers/dma/imx-sdma.c | 57 +++++++++++++++++++++++++++-------------------- > 1 files changed, 33 insertions(+), 24 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index fddccae..fc49ffa 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -24,7 +24,7 @@ > #include <linux/mm.h> > #include <linux/interrupt.h> > #include <linux/clk.h> > -#include <linux/wait.h> > +#include <linux/delay.h> > #include <linux/sched.h> > #include <linux/semaphore.h> > #include <linux/spinlock.h> > @@ -324,7 +324,7 @@ struct sdma_engine { > struct dma_device dma_device; > struct clk *clk_ipg; > struct clk *clk_ahb; > - struct mutex channel_0_lock; > + spinlock_t channel_0_lock; > struct sdma_script_start_addrs *script_addrs; > }; > > @@ -402,19 +402,31 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel) > } > > /* > - * sdma_run_channel - run a channel and wait till it's done > + * sdma_run_channel0 - run a channel and wait till it's done > */ > -static int sdma_run_channel(struct sdma_channel *sdmac) > +static int sdma_run_channel0(struct sdma_channel *sdmac) Renaming this to sdma_run_channel0 is fine, but then the argument should be changed to struct sdma_engine. It makes no sense to say in the function name that this function is channel 0 only and at the same time allow to pass in an arbitrary other channel. Sascha
On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote: > device_prep_dma_cyclic may be call in audio trigger function which is > atomic context, so we make it atomic too. No this is wrong behavior. You should not call dma prepare functions in any of the sound trigger calls. It would make sense to move this in sound prepare callback.
On Fri, Apr 27, 2012 at 01:51:40PM +0530, Vinod Koul wrote: > On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote: > > device_prep_dma_cyclic may be call in audio trigger function which is > > atomic context, so we make it atomic too. > No this is wrong behavior. You should not call dma prepare functions in > any of the sound trigger calls. It would make sense to move this in > sound prepare callback. Then, could you please doc it somewhere? I think I'm not the only one confused. Thanks Richard > > -- > ~Vinod > >
On Fri, Apr 27, 2012 at 09:55:44AM +0200, Sascha Hauer wrote: > On Fri, Apr 27, 2012 at 03:02:55PM +0800, Richard Zhao wrote: > > device_prep_dma_cyclic may be call in audio trigger function which is > > atomic context, so we make it atomic too. > > > > - change channel0 lock to spinlock. > > - Use polling to wait for channel0 finish running. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > > --- > > drivers/dma/imx-sdma.c | 57 +++++++++++++++++++++++++++-------------------- > > 1 files changed, 33 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > > index fddccae..fc49ffa 100644 > > --- a/drivers/dma/imx-sdma.c > > +++ b/drivers/dma/imx-sdma.c > > @@ -24,7 +24,7 @@ > > #include <linux/mm.h> > > #include <linux/interrupt.h> > > #include <linux/clk.h> > > -#include <linux/wait.h> > > +#include <linux/delay.h> > > #include <linux/sched.h> > > #include <linux/semaphore.h> > > #include <linux/spinlock.h> > > @@ -324,7 +324,7 @@ struct sdma_engine { > > struct dma_device dma_device; > > struct clk *clk_ipg; > > struct clk *clk_ahb; > > - struct mutex channel_0_lock; > > + spinlock_t channel_0_lock; > > struct sdma_script_start_addrs *script_addrs; > > }; > > > > @@ -402,19 +402,31 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel) > > } > > > > /* > > - * sdma_run_channel - run a channel and wait till it's done > > + * sdma_run_channel0 - run a channel and wait till it's done > > */ > > -static int sdma_run_channel(struct sdma_channel *sdmac) > > +static int sdma_run_channel0(struct sdma_channel *sdmac) > > Renaming this to sdma_run_channel0 is fine, but then the argument should > be changed to struct sdma_engine. It makes no sense to say in the > function name that this function is channel 0 only and at the same time > allow to pass in an arbitrary other channel. Correct. Thanks. Richard > > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
Hi, Richard Zhao writes: > device_prep_dma_cyclic may be call in audio trigger function which is > atomic context, so we make it atomic too. > > - change channel0 lock to spinlock. > - Use polling to wait for channel0 finish running. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > Actually I didn't sign off the patch that I posted, because I wanted to wait for more comments first. Lothar Waßmann
On Fri, 2012-04-27 at 16:41 +0800, Richard Zhao wrote: > On Fri, Apr 27, 2012 at 01:51:40PM +0530, Vinod Koul wrote: > > On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote: > > > device_prep_dma_cyclic may be call in audio trigger function which is > > > atomic context, so we make it atomic too. > > No this is wrong behavior. You should not call dma prepare functions in > > any of the sound trigger calls. It would make sense to move this in > > sound prepare callback. > Then, could you please doc it somewhere? I think I'm not the only one > confused. See the soc-dmaengine.c for correct behavior! I can document only dmaengine behavior (which is already there) and not how each subsystem should use this. People is subsystems need to see how to use the dmaengine APIs sanely
On Fri, Apr 27, 2012 at 03:52:10PM +0530, Vinod Koul wrote: > On Fri, 2012-04-27 at 16:41 +0800, Richard Zhao wrote: > > Then, could you please doc it somewhere? I think I'm not the only one > > confused. > See the soc-dmaengine.c for correct behavior! Better yet, convert your code to use it if it's not doing so already!
On Fri, Apr 27, 2012 at 03:52:10PM +0530, Vinod Koul wrote: > On Fri, 2012-04-27 at 16:41 +0800, Richard Zhao wrote: > > On Fri, Apr 27, 2012 at 01:51:40PM +0530, Vinod Koul wrote: > > > On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote: > > > > device_prep_dma_cyclic may be call in audio trigger function which is > > > > atomic context, so we make it atomic too. > > > No this is wrong behavior. You should not call dma prepare functions in > > > any of the sound trigger calls. It would make sense to move this in > > > sound prepare callback. > > Then, could you please doc it somewhere? I think I'm not the only one > > confused. > See the soc-dmaengine.c for correct behavior! Do you mean sound/soc/soc-dmaengine-pcm.c ? It's where it calls dma prepare in trigger function. snd_dmaengine_pcm_trigger --> dmaengine_pcm_prepare_and_submit --> dmaengine_prep_dma_cyclic > I can document only dmaengine behavior (which is already there) Sure, I mean, can you doc in include/linux/dmaengine.h that dmaengine_prep_xxx may sleep? > and not > how each subsystem should use this. People is subsystems need to see how > to use the dmaengine APIs sanely Thanks Richard > > > -- > ~Vinod > >
On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote: > Sure, I mean, can you doc in include/linux/dmaengine.h that > dmaengine_prep_xxx may sleep? Incorrect. They may _not_ sleep.
On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote: > On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote: >> Sure, I mean, can you doc in include/linux/dmaengine.h that >> dmaengine_prep_xxx may sleep? > Incorrect. They may _not_ sleep. But I have seen that we are using the kzalloc in the dmaengine_prep_xxx and kzalloc is sleeping call.
On Fri, Apr 27, 2012 at 06:31:13PM +0530, Laxman Dewangan wrote: > On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote: >> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote: >>> Sure, I mean, can you doc in include/linux/dmaengine.h that >>> dmaengine_prep_xxx may sleep? >> Incorrect. They may _not_ sleep. > > But I have seen that we are using the kzalloc in the dmaengine_prep_xxx > and kzalloc is sleeping call. Not with GFP_ATOMIC it isn't.
On Friday 27 April 2012 06:40 PM, Russell King - ARM Linux wrote: > On Fri, Apr 27, 2012 at 06:31:13PM +0530, Laxman Dewangan wrote: >> On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote: >>> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote: >>>> Sure, I mean, can you doc in include/linux/dmaengine.h that >>>> dmaengine_prep_xxx may sleep? >>> Incorrect. They may _not_ sleep. >> But I have seen that we are using the kzalloc in the dmaengine_prep_xxx >> and kzalloc is sleeping call. > Not with GFP_ATOMIC it isn't. Yes, with GFP_ATOMIC , it will not be a sleeping calls. I will fix my dma driver accordingly.
On Fri, Apr 27, 2012 at 02:10:10PM +0100, Russell King - ARM Linux wrote: > On Fri, Apr 27, 2012 at 06:31:13PM +0530, Laxman Dewangan wrote: > > On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote: > >> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote: > >>> Sure, I mean, can you doc in include/linux/dmaengine.h that > >>> dmaengine_prep_xxx may sleep? > >> Incorrect. They may _not_ sleep. Vinod, do you agree? Any way, better doc it. Thanks Richard > > > > But I have seen that we are using the kzalloc in the dmaengine_prep_xxx > > and kzalloc is sleeping call. > > Not with GFP_ATOMIC it isn't. >
On Fri, Apr 27, 2012 at 11:18:31AM +0200, Lothar Waßmann wrote: > Hi, > > Richard Zhao writes: > > device_prep_dma_cyclic may be call in audio trigger function which is > > atomic context, so we make it atomic too. > > > > - change channel0 lock to spinlock. > > - Use polling to wait for channel0 finish running. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > Actually I didn't sign off the patch that I posted, because I wanted > to wait for more comments first. I send it out with slight modifications because the series highly depend on it. Will you take it over or let me put it in next version? Both are ok to me. Thanks Richard > > > Lothar Waßmann > -- > ___________________________________________________________ > > Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen > Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 > Geschäftsführer: Matthias Kaussen > Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 > > www.karo-electronics.de | info@karo-electronics.de > ___________________________________________________________ >
Hi, Richard Zhao writes: > On Fri, Apr 27, 2012 at 11:18:31AM +0200, Lothar Waßmann wrote: > > Hi, > > > > Richard Zhao writes: > > > device_prep_dma_cyclic may be call in audio trigger function which is > > > atomic context, so we make it atomic too. > > > > > > - change channel0 lock to spinlock. > > > - Use polling to wait for channel0 finish running. > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > Actually I didn't sign off the patch that I posted, because I wanted > > to wait for more comments first. > I send it out with slight modifications because the series highly > depend on it. Will you take it over or let me put it in next version? > Both are ok to me. > I think you should keep it as part of your sound patches and I will test your final version on our hardware. Lothar Waßmann
On Fri, Apr 27, 2012 at 11:13 AM, Lothar Waßmann <LW@karo-electronics.de> wrote: > Hi, > > Richard Zhao writes: >> On Fri, Apr 27, 2012 at 11:18:31AM +0200, Lothar Waßmann wrote: >> > Hi, >> > >> > Richard Zhao writes: >> > > device_prep_dma_cyclic may be call in audio trigger function which is >> > > atomic context, so we make it atomic too. >> > > >> > > - change channel0 lock to spinlock. >> > > - Use polling to wait for channel0 finish running. >> > > >> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> >> > > >> > Actually I didn't sign off the patch that I posted, because I wanted >> > to wait for more comments first. >> I send it out with slight modifications because the series highly >> depend on it. Will you take it over or let me put it in next version? >> Both are ok to me. >> > I think you should keep it as part of your sound patches and I will > test your final version on our hardware. > I hope we can get a conclusion that the prep_slave_sg() can be called in atomic context or not. My patch "add DMA support to UART" heavily depends on it. Huang Shijie > > Lothar Waßmann > -- > ___________________________________________________________ > > Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen > Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 > Geschäftsführer: Matthias Kaussen > Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 > > www.karo-electronics.de | info@karo-electronics.de > ___________________________________________________________ > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index fddccae..fc49ffa 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -24,7 +24,7 @@ #include <linux/mm.h> #include <linux/interrupt.h> #include <linux/clk.h> -#include <linux/wait.h> +#include <linux/delay.h> #include <linux/sched.h> #include <linux/semaphore.h> #include <linux/spinlock.h> @@ -324,7 +324,7 @@ struct sdma_engine { struct dma_device dma_device; struct clk *clk_ipg; struct clk *clk_ahb; - struct mutex channel_0_lock; + spinlock_t channel_0_lock; struct sdma_script_start_addrs *script_addrs; }; @@ -402,19 +402,31 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel) } /* - * sdma_run_channel - run a channel and wait till it's done + * sdma_run_channel0 - run a channel and wait till it's done */ -static int sdma_run_channel(struct sdma_channel *sdmac) +static int sdma_run_channel0(struct sdma_channel *sdmac) { struct sdma_engine *sdma = sdmac->sdma; int channel = sdmac->channel; int ret; + unsigned long timeout = 500; - init_completion(&sdmac->done); - + if (channel) + return -EINVAL; sdma_enable_channel(sdma, channel); - ret = wait_for_completion_timeout(&sdmac->done, HZ); + while (!(ret = readl_relaxed(sdma->regs + SDMA_H_INTR) & 1)) { + if (timeout-- <= 0) + break; + udelay(1); + } + + if (ret) { + /* Clear the interrupt status */ + writel_relaxed(ret, sdma->regs + SDMA_H_INTR); + } else { + dev_err(sdma->dev, "Timeout waiting for CH0 ready\n"); + } return ret ? 0 : -ETIMEDOUT; } @@ -426,17 +438,17 @@ static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size, void *buf_virt; dma_addr_t buf_phys; int ret; - - mutex_lock(&sdma->channel_0_lock); + unsigned long flags; buf_virt = dma_alloc_coherent(NULL, size, &buf_phys, GFP_KERNEL); if (!buf_virt) { - ret = -ENOMEM; - goto err_out; + return -ENOMEM; } + spin_lock_irqsave(&sdma->channel_0_lock, flags); + bd0->mode.command = C0_SETPM; bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD; bd0->mode.count = size / 2; @@ -445,12 +457,11 @@ static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size, memcpy(buf_virt, buf, size); - ret = sdma_run_channel(&sdma->channel[0]); + ret = sdma_run_channel0(&sdma->channel[0]); - dma_free_coherent(NULL, size, buf_virt, buf_phys); + spin_unlock_irqrestore(&sdma->channel_0_lock, flags); -err_out: - mutex_unlock(&sdma->channel_0_lock); + dma_free_coherent(NULL, size, buf_virt, buf_phys); return ret; } @@ -539,10 +550,6 @@ static void mxc_sdma_handle_channel(struct sdma_channel *sdmac) { complete(&sdmac->done); - /* not interested in channel 0 interrupts */ - if (sdmac->channel == 0) - return; - if (sdmac->flags & IMX_DMA_SG_LOOP) sdma_handle_channel_loop(sdmac); else @@ -555,6 +562,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id) unsigned long stat; stat = readl_relaxed(sdma->regs + SDMA_H_INTR); + /* not interested in channel 0 interrupts */ + stat &= ~1; writel_relaxed(stat, sdma->regs + SDMA_H_INTR); while (stat) { @@ -660,6 +669,7 @@ static int sdma_load_context(struct sdma_channel *sdmac) struct sdma_context_data *context = sdma->context; struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd; int ret; + unsigned long flags; if (sdmac->direction == DMA_DEV_TO_MEM) { load_address = sdmac->pc_from_device; @@ -677,7 +687,7 @@ static int sdma_load_context(struct sdma_channel *sdmac) dev_dbg(sdma->dev, "event_mask0 = 0x%08x\n", (u32)sdmac->event_mask[0]); dev_dbg(sdma->dev, "event_mask1 = 0x%08x\n", (u32)sdmac->event_mask[1]); - mutex_lock(&sdma->channel_0_lock); + spin_lock_irqsave(&sdma->channel_0_lock, flags); memset(context, 0, sizeof(*context)); context->channel_state.pc = load_address; @@ -696,10 +706,9 @@ static int sdma_load_context(struct sdma_channel *sdmac) bd0->mode.count = sizeof(*context) / 4; bd0->buffer_addr = sdma->context_phys; bd0->ext_buffer_addr = 2048 + (sizeof(*context) / 4) * channel; + ret = sdma_run_channel0(&sdma->channel[0]); - ret = sdma_run_channel(&sdma->channel[0]); - - mutex_unlock(&sdma->channel_0_lock); + spin_unlock_irqrestore(&sdma->channel_0_lock, flags); return ret; } @@ -1305,7 +1314,7 @@ static int __init sdma_probe(struct platform_device *pdev) if (!sdma) return -ENOMEM; - mutex_init(&sdma->channel_0_lock); + spin_lock_init(&sdma->channel_0_lock); sdma->dev = &pdev->dev;