| Message ID | 1311557312-26107-5-git-send-email-boojin.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > +static void pl330_tasklet_cyclic(unsigned long data) > +{ > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > + struct dma_pl330_desc *desc, *_dt; > + unsigned long flags; > + LIST_HEAD(list); > + > + spin_lock_irqsave(&pch->lock, flags); ... > + callback = desc->txd.callback; > + if (callback) > + callback(desc->txd.callback_param); On this again - what if the callback wants to terminate the DMA activity because there's no more audio data to be sent/received from the device? > + Useless blank line. > + } > + > + spin_unlock_irqrestore(&pch->lock, flags); > +} > + > static void pl330_tasklet(unsigned long data) > { > struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > @@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum pl330_op_err err) > > spin_unlock_irqrestore(&pch->lock, flags); > > + if (pch->cyclic_task) > + tasklet_schedule(pch->cyclic_task); > + else > tasklet_schedule(&pch->task); Indentation error. > } > > @@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan *chan) > pl330_release_channel(pch->pl330_chid); > pch->pl330_chid = NULL; > > + if (pch->cyclic) { > + pch->cyclic = false; > + list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); > + if (pch->cyclic_task) { > + tasklet_kill(pch->cyclic_task); > + pch->cyclic_task = NULL; > + } > + } > + > spin_unlock_irqrestore(&pch->lock, flags); > } > > @@ -547,6 +599,63 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) > return burst_len; > } > > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( > + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, > + size_t period_len, enum dma_data_direction direction) > +{ > + struct dma_pl330_desc *desc; > + struct dma_pl330_chan *pch = to_pchan(chan); > + struct dma_pl330_peri *peri = chan->private; > + dma_addr_t dst; > + dma_addr_t src; > + > + pch = to_pchan(chan); > + if (!pch) > + return NULL; You've already done the to_pchan() thing when declaring pch. You've already dereferenced 'chan', so there's no way that pch could be NULL here. > + > + desc = pl330_get_desc(pch); > + if (!desc) { > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n", > + __func__, __LINE__); > + return NULL; > + } > + > + switch (direction) { > + case DMA_TO_DEVICE: > + desc->rqcfg.src_inc = 1; > + desc->rqcfg.dst_inc = 0; > + src = dma_addr; > + dst = peri->fifo_addr; > + break; > + case DMA_FROM_DEVICE: > + desc->rqcfg.src_inc = 0; > + desc->rqcfg.dst_inc = 1; > + src = peri->fifo_addr; > + dst = dma_addr; > + break; > + default: > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n", > + __func__, __LINE__); > + return NULL; > + } > + > + desc->rqcfg.brst_size = peri->burst_sz; > + desc->rqcfg.brst_len = 1; > + > + if (!pch->cyclic_task) { > + pch->cyclic_task = > + kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL); > + tasklet_init(pch->cyclic_task, > + pl330_tasklet_cyclic, (unsigned int)pch); Here you allocate memory for the cyclic task. Above you set this pointer to NULL. That sounds like a memory leak to me. Why are you kmallocing this memory - why can't it be part of the dma_pl330_chan structure? It's only 28 bytes.
Russell King - ARM Linux Wrote: > Sent: Monday, July 25, 2011 6:28 PM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; > Kukjin Kim; Vinod Koul; Jassi Brar; Grant Likely; Mark Brown; Dan Williams > Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > +static void pl330_tasklet_cyclic(unsigned long data) > > +{ > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > + struct dma_pl330_desc *desc, *_dt; > > + unsigned long flags; > > + LIST_HEAD(list); > > + > > + spin_lock_irqsave(&pch->lock, flags); > ... > > + callback = desc->txd.callback; > > + if (callback) > > + callback(desc->txd.callback_param); > > On this again - what if the callback wants to terminate the DMA activity > because there's no more audio data to be sent/received from the device? Do you mean what is happened if the callback() is called after channel is terminated ? Or What is happened if Callback() calls 'dma_release_channel()' to terminate DMA? > > > + > > Useless blank line. I will address your comment. > > > + } > > + > > + spin_unlock_irqrestore(&pch->lock, flags); > > +} > > + > > static void pl330_tasklet(unsigned long data) > > { > > struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > @@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum > pl330_op_err err) > > > > spin_unlock_irqrestore(&pch->lock, flags); > > > > + if (pch->cyclic_task) > > + tasklet_schedule(pch->cyclic_task); > > + else > > tasklet_schedule(&pch->task); > > Indentation error. Sorry again.. I will address your comment. > > > } > > > > @@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan > *chan) > > pl330_release_channel(pch->pl330_chid); > > pch->pl330_chid = NULL; > > > > + if (pch->cyclic) { > > + pch->cyclic = false; > > + list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); > > + if (pch->cyclic_task) { > > + tasklet_kill(pch->cyclic_task); > > + pch->cyclic_task = NULL; > > + } > > + } > > + > > spin_unlock_irqrestore(&pch->lock, flags); > > } > > > > @@ -547,6 +599,63 @@ static inline int get_burst_len(struct dma_pl330_desc > *desc, size_t len) > > return burst_len; > > } > > > > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( > > + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, > > + size_t period_len, enum dma_data_direction direction) > > +{ > > + struct dma_pl330_desc *desc; > > + struct dma_pl330_chan *pch = to_pchan(chan); > > + struct dma_pl330_peri *peri = chan->private; > > + dma_addr_t dst; > > + dma_addr_t src; > > + > > + pch = to_pchan(chan); > > + if (!pch) > > + return NULL; > > You've already done the to_pchan() thing when declaring pch. You've > already dereferenced 'chan', so there's no way that pch could be NULL > here. I will address your comment. > > > + > > + desc = pl330_get_desc(pch); > > + if (!desc) { > > + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n", > > + __func__, __LINE__); > > + return NULL; > > + } > > + > > + switch (direction) { > > + case DMA_TO_DEVICE: > > + desc->rqcfg.src_inc = 1; > > + desc->rqcfg.dst_inc = 0; > > + src = dma_addr; > > + dst = peri->fifo_addr; > > + break; > > + case DMA_FROM_DEVICE: > > + desc->rqcfg.src_inc = 0; > > + desc->rqcfg.dst_inc = 1; > > + src = peri->fifo_addr; > > + dst = dma_addr; > > + break; > > + default: > > + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n", > > + __func__, __LINE__); > > + return NULL; > > + } > > + > > + desc->rqcfg.brst_size = peri->burst_sz; > > + desc->rqcfg.brst_len = 1; > > + > > + if (!pch->cyclic_task) { > > + pch->cyclic_task = > > + kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL); > > + tasklet_init(pch->cyclic_task, > > + pl330_tasklet_cyclic, (unsigned int)pch); > > Here you allocate memory for the cyclic task. Above you set this pointer > to NULL. That sounds like a memory leak to me. Why are you kmallocing > this memory - why can't it be part of the dma_pl330_chan structure? It's > only 28 bytes. It's my mistake. I should have been free of the memory. And the reason why I use kmalloc for 'cyclic_task' is following. This memory size for 'cyclic_tasklet' is the 896 bytes ( = the number of channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And This memory size is increased according to the number of DMAC. And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. If I make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is dedicated for Mem-to-Mem operation should hold unused data. So, I think it's loss that all dma channels hold own 'cyclic_task'.
On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote: > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > > +static void pl330_tasklet_cyclic(unsigned long data) > > > +{ > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > > + struct dma_pl330_desc *desc, *_dt; > > > + unsigned long flags; > > > + LIST_HEAD(list); > > > + > > > + spin_lock_irqsave(&pch->lock, flags); > > ... > > > + callback = desc->txd.callback; > > > + if (callback) > > > + callback(desc->txd.callback_param); > > > > On this again - what if the callback wants to terminate the DMA activity > > because there's no more audio data to be sent/received from the device? > > Do you mean what is happened if the callback() is called after channel is > terminated ? > Or What is happened if Callback() calls 'dma_release_channel()' to terminate > DMA? No. I mean what if the callback wants to call dmaengine_terminate_all(). > > > + if (!pch->cyclic_task) { > > > + pch->cyclic_task = > > > + kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL); > > > + tasklet_init(pch->cyclic_task, > > > + pl330_tasklet_cyclic, (unsigned int)pch); > > > > Here you allocate memory for the cyclic task. Above you set this pointer > > to NULL. That sounds like a memory leak to me. Why are you kmallocing > > this memory - why can't it be part of the dma_pl330_chan structure? It's > > only 28 bytes. > > It's my mistake. I should have been free of the memory. > > And the reason why I use kmalloc for 'cyclic_task' is following. > This memory size for 'cyclic_tasklet' is the 896 bytes ( = the number of > channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And This > memory size is increased according to the number of DMAC. > And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. If I > make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is dedicated > for Mem-to-Mem operation should hold unused data. > So, I think it's loss that all dma channels hold own 'cyclic_task'. Could you re-use the tasklet that already exists?
On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote: > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote: > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > > > +static void pl330_tasklet_cyclic(unsigned long data) > > > > +{ > > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > > > + struct dma_pl330_desc *desc, *_dt; > > > > + unsigned long flags; > > > > + LIST_HEAD(list); > > > > + > > > > + spin_lock_irqsave(&pch->lock, flags); > > > ... > > > > + callback = desc->txd.callback; > > > > + if (callback) > > > > + callback(desc->txd.callback_param); > > > > > > On this again - what if the callback wants to terminate the DMA activity > > > because there's no more audio data to be sent/received from the device? > > > > Do you mean what is happened if the callback() is called after channel is > > terminated ? > > Or What is happened if Callback() calls 'dma_release_channel()' to terminate > > DMA? > > No. I mean what if the callback wants to call dmaengine_terminate_all(). you are supposed to drop the lock here, that way callback can call any DMA API, otherwise it will result in deadlock. This make me wonder you haven't read the documentation at all, please ensure you have read Documentation/dmaengine.txt before next posting > > > > > + if (!pch->cyclic_task) { > > > > + pch->cyclic_task = > > > > + kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL); > > > > + tasklet_init(pch->cyclic_task, > > > > + pl330_tasklet_cyclic, (unsigned int)pch); > > > > > > Here you allocate memory for the cyclic task. Above you set this pointer > > > to NULL. That sounds like a memory leak to me. Why are you kmallocing > > > this memory - why can't it be part of the dma_pl330_chan structure? It's > > > only 28 bytes. > > > > It's my mistake. I should have been free of the memory. > > > > And the reason why I use kmalloc for 'cyclic_task' is following. > > This memory size for 'cyclic_tasklet' is the 896 bytes ( = the number of > > channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And This > > memory size is increased according to the number of DMAC. > > And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. If I > > make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is dedicated > > for Mem-to-Mem operation should hold unused data. > > So, I think it's loss that all dma channels hold own 'cyclic_task'. > > Could you re-use the tasklet that already exists?
On Mon, Jul 25, 2011 at 04:18:04PM +0530, Vinod Koul wrote: > On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote: > > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote: > > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > > > > +static void pl330_tasklet_cyclic(unsigned long data) > > > > > +{ > > > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > > > > + struct dma_pl330_desc *desc, *_dt; > > > > > + unsigned long flags; > > > > > + LIST_HEAD(list); > > > > > + > > > > > + spin_lock_irqsave(&pch->lock, flags); > > > > ... > > > > > + callback = desc->txd.callback; > > > > > + if (callback) > > > > > + callback(desc->txd.callback_param); > > > > > > > > On this again - what if the callback wants to terminate the DMA activity > > > > because there's no more audio data to be sent/received from the device? > > > > > > Do you mean what is happened if the callback() is called after channel is > > > terminated ? > > > Or What is happened if Callback() calls 'dma_release_channel()' to terminate > > > DMA? > > > > No. I mean what if the callback wants to call dmaengine_terminate_all(). > you are supposed to drop the lock here, that way callback can call any > DMA API, otherwise it will result in deadlock. > This make me wonder you haven't read the documentation at all, please > ensure you have read Documentation/dmaengine.txt before next posting I know that very well thank you. Please look at my previous post in the previous round of patches, and the response from Boojin Kim to see why I used this as an *EXAMPLE* to get this fixed.
On Mon, 2011-07-25 at 11:57 +0100, Russell King - ARM Linux wrote: > On Mon, Jul 25, 2011 at 04:18:04PM +0530, Vinod Koul wrote: > > On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote: > > > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote: > > > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > > > > > +static void pl330_tasklet_cyclic(unsigned long data) > > > > > > +{ > > > > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > > > > > + struct dma_pl330_desc *desc, *_dt; > > > > > > + unsigned long flags; > > > > > > + LIST_HEAD(list); > > > > > > + > > > > > > + spin_lock_irqsave(&pch->lock, flags); > > > > > ... > > > > > > + callback = desc->txd.callback; > > > > > > + if (callback) > > > > > > + callback(desc->txd.callback_param); > > > > > > > > > > On this again - what if the callback wants to terminate the DMA activity > > > > > because there's no more audio data to be sent/received from the device? > > > > > > > > Do you mean what is happened if the callback() is called after channel is > > > > terminated ? > > > > Or What is happened if Callback() calls 'dma_release_channel()' to terminate > > > > DMA? > > > > > > No. I mean what if the callback wants to call dmaengine_terminate_all(). > > you are supposed to drop the lock here, that way callback can call any > > DMA API, otherwise it will result in deadlock. > > This make me wonder you haven't read the documentation at all, please > > ensure you have read Documentation/dmaengine.txt before next posting > > I know that very well thank you. Please look at my previous post in the > previous round of patches, and the response from Boojin Kim to see why > I used this as an *EXAMPLE* to get this fixed. Russell, Sorry that was not intended for you but for the author of patch Boojin Kim... Agree on your EXAMPLE, just wanted to ensure authors have read it as going thru this patch made it clear that they haven't.
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim <boojin.kim@samsung.com> wrote: > This patch adds DMA_CYCLIC capability that is used for audio driver. > DMA driver with DMA_CYCLIC capability reuses the dma requests that > were submitted through tx_submit(). > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > --- > drivers/dma/pl330.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 111 insertions(+), 0 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 880f010..121c75a 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -69,6 +69,11 @@ struct dma_pl330_chan { > * NULL if the channel is available to be acquired. > */ > void *pl330_chid; > + > + /* taks for cyclic capability */ > + struct tasklet_struct *cyclic_task; > + > + bool cyclic; > }; We already have a tasklet_struct member here. This 'cyclic' flag can indicate if we are doing cyclic or serial transfers. So, this cyclic_task seems unnecessary. > +static void pl330_tasklet_cyclic(unsigned long data) > +{ > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > + struct dma_pl330_desc *desc, *_dt; > + unsigned long flags; > + LIST_HEAD(list); > + > + spin_lock_irqsave(&pch->lock, flags); > + > + /* Pick up ripe tomatoes */ > + list_for_each_entry_safe(desc, _dt, &pch->work_list, node) > + if (desc->status == DONE) { > + dma_async_tx_callback callback; > + > + list_move_tail(&desc->node, &pch->work_list); > + pch->completed = desc->txd.cookie; > + > + desc->status = PREP; > + > + /* Try to submit a req imm. > + next to the last completed cookie */ > + fill_queue(pch); > + > + /* Make sure the PL330 Channel thread is active */ > + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_START); > + > + callback = desc->txd.callback; > + if (callback) > + callback(desc->txd.callback_param); > + > + } > + > + spin_unlock_irqrestore(&pch->lock, flags); > +} Please merge this with pl330_tasklet and use 'cyclic' flag to differentiate. > @@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum pl330_op_err err) > > spin_unlock_irqrestore(&pch->lock, flags); > > + if (pch->cyclic_task) > + tasklet_schedule(pch->cyclic_task); > + else > tasklet_schedule(&pch->task); A tab here please, and check for 'cyclic' flag. > @@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan *chan) > pl330_release_channel(pch->pl330_chid); > pch->pl330_chid = NULL; > > + if (pch->cyclic) { > + pch->cyclic = false; > + list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); > + if (pch->cyclic_task) { > + tasklet_kill(pch->cyclic_task); > + pch->cyclic_task = NULL; > + } > + } > + > spin_unlock_irqrestore(&pch->lock, flags); > } To be explicit, please initialize 'cyclic' flag to false in pl330_alloc_chan_resources
Russell King - ARM Linux Wrote: > Sent: Monday, July 25, 2011 7:36 PM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; 'Kukjin Kim'; 'Vinod Koul'; 'Jassi Brar'; 'Grant > Likely'; 'Mark Brown'; 'Dan Williams' > Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability > > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote: > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > > > +static void pl330_tasklet_cyclic(unsigned long data) > > > > +{ > > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > > > + struct dma_pl330_desc *desc, *_dt; > > > > + unsigned long flags; > > > > + LIST_HEAD(list); > > > > + > > > > + spin_lock_irqsave(&pch->lock, flags); > > > ... > > > > + callback = desc->txd.callback; > > > > + if (callback) > > > > + callback(desc->txd.callback_param); > > > > > > On this again - what if the callback wants to terminate the DMA > activity > > > because there's no more audio data to be sent/received from the > device? > > > > Do you mean what is happened if the callback() is called after > channel is > > terminated ? > > Or What is happened if Callback() calls 'dma_release_channel()' to > terminate > > DMA? > > No. I mean what if the callback wants to call > dmaengine_terminate_all(). I found the deadlock problem that you and Vinod post. I will fix it. > > > > > + if (!pch->cyclic_task) { > > > > + pch->cyclic_task = > > > > + kmalloc(sizeof(struct tasklet_struct), > GFP_KERNEL); > > > > + tasklet_init(pch->cyclic_task, > > > > + pl330_tasklet_cyclic, (unsigned int)pch); > > > > > > Here you allocate memory for the cyclic task. Above you set this > pointer > > > to NULL. That sounds like a memory leak to me. Why are you > kmallocing > > > this memory - why can't it be part of the dma_pl330_chan structure? > It's > > > only 28 bytes. > > > > It's my mistake. I should have been free of the memory. > > > > And the reason why I use kmalloc for 'cyclic_task' is following. > > This memory size for 'cyclic_tasklet' is the 896 bytes ( = the > number of > > channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And > This > > memory size is increased according to the number of DMAC. > > And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. > If I > > make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is > dedicated > > for Mem-to-Mem operation should hold unused data. > > So, I think it's loss that all dma channels hold own 'cyclic_task'. > > Could you re-use the tasklet that already exists? Yes, It can be re-used. But, This tasklet should be initialized on each request by using 'tasklet_init()' because it needs the own channel as its parameter.
Vinod Koul Wrote: > Sent: Monday, July 25, 2011 7:48 PM > To: Russell King - ARM Linux > Cc: vinod.koul@intel.com; Boojin Kim; 'Kukjin Kim'; 'Jassi Brar'; > 'Grant Likely'; linux-samsung-soc@vger.kernel.org; 'Mark Brown'; 'Dan > Williams'; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability > > On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote: > > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote: > > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote: > > > > > +static void pl330_tasklet_cyclic(unsigned long data) > > > > > +{ > > > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > > > > + struct dma_pl330_desc *desc, *_dt; > > > > > + unsigned long flags; > > > > > + LIST_HEAD(list); > > > > > + > > > > > + spin_lock_irqsave(&pch->lock, flags); > > > > ... > > > > > + callback = desc->txd.callback; > > > > > + if (callback) > > > > > + callback(desc->txd.callback_param); > > > > > > > > On this again - what if the callback wants to terminate the DMA > activity > > > > because there's no more audio data to be sent/received from the > device? > > > > > > Do you mean what is happened if the callback() is called after > channel is > > > terminated ? > > > Or What is happened if Callback() calls 'dma_release_channel()' to > terminate > > > DMA? > > > > No. I mean what if the callback wants to call > dmaengine_terminate_all(). > you are supposed to drop the lock here, that way callback can call any > DMA API, otherwise it will result in deadlock. > This make me wonder you haven't read the documentation at all, please > ensure you have read Documentation/dmaengine.txt before next posting I found the deadlock problem that you post. I will fix it and read the document again. > > > > > > > + if (!pch->cyclic_task) { > > > > > + pch->cyclic_task = > > > > > + kmalloc(sizeof(struct tasklet_struct), > GFP_KERNEL); > > > > > + tasklet_init(pch->cyclic_task, > > > > > + pl330_tasklet_cyclic, (unsigned int)pch); > > > > > > > > Here you allocate memory for the cyclic task. Above you set this > pointer > > > > to NULL. That sounds like a memory leak to me. Why are you > kmallocing > > > > this memory - why can't it be part of the dma_pl330_chan > structure? It's > > > > only 28 bytes. > > > > > > It's my mistake. I should have been free of the memory. > > > > > > And the reason why I use kmalloc for 'cyclic_task' is following. > > > This memory size for 'cyclic_tasklet' is the 896 bytes ( = the > number of > > > channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And > This > > > memory size is increased according to the number of DMAC. > > > And Samsung has the DMAC that is dedicated for Mem-to-Mem > operation. If I > > > make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is > dedicated > > > for Mem-to-Mem operation should hold unused data. > > > So, I think it's loss that all dma channels hold own 'cyclic_task'. > > > > Could you re-use the tasklet that already exists? > > > -- > ~Vinod Koul > Intel Corp.
Jassi Brar Wrote: > Sent: Monday, July 25, 2011 8:24 PM > To: Boojin Kim > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant > Likely; Mark Brown > Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability > > On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim <boojin.kim@samsung.com> > wrote: > > This patch adds DMA_CYCLIC capability that is used for audio driver. > > DMA driver with DMA_CYCLIC capability reuses the dma requests that > > were submitted through tx_submit(). > > > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > > --- > > drivers/dma/pl330.c | 111 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 111 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > > index 880f010..121c75a 100644 > > --- a/drivers/dma/pl330.c > > +++ b/drivers/dma/pl330.c > > @@ -69,6 +69,11 @@ struct dma_pl330_chan { > > * NULL if the channel is available to be acquired. > > */ > > void *pl330_chid; > > + > > + /* taks for cyclic capability */ > > + struct tasklet_struct *cyclic_task; > > + > > + bool cyclic; > > }; > We already have a tasklet_struct member here. > This 'cyclic' flag can indicate if we are doing cyclic or serial > transfers. So, this cyclic_task seems unnecessary. I will change cyclic operation base on your comment. Cyclic_task will be removed. > > > +static void pl330_tasklet_cyclic(unsigned long data) > > +{ > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > > + struct dma_pl330_desc *desc, *_dt; > > + unsigned long flags; > > + LIST_HEAD(list); > > + > > + spin_lock_irqsave(&pch->lock, flags); > > + > > + /* Pick up ripe tomatoes */ > > + list_for_each_entry_safe(desc, _dt, &pch->work_list, node) > > + if (desc->status == DONE) { > > + dma_async_tx_callback callback; > > + > > + list_move_tail(&desc->node, &pch->work_list); > > + pch->completed = desc->txd.cookie; > > + > > + desc->status = PREP; > > + > > + /* Try to submit a req imm. > > + next to the last completed cookie */ > > + fill_queue(pch); > > + > > + /* Make sure the PL330 Channel thread is active > */ > > + pl330_chan_ctrl(pch->pl330_chid, > PL330_OP_START); > > + > > + callback = desc->txd.callback; > > + if (callback) > > + callback(desc->txd.callback_param); > > + > > + } > > + > > + spin_unlock_irqrestore(&pch->lock, flags); > > +} > Please merge this with pl330_tasklet and use 'cyclic' flag to > differentiate. > > > @@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum > pl330_op_err err) > > > > spin_unlock_irqrestore(&pch->lock, flags); > > > > + if (pch->cyclic_task) > > + tasklet_schedule(pch->cyclic_task); > > + else > > tasklet_schedule(&pch->task); > A tab here please, and check for 'cyclic' flag. > > > > @@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct > dma_chan *chan) > > pl330_release_channel(pch->pl330_chid); > > pch->pl330_chid = NULL; > > > > + if (pch->cyclic) { > > + pch->cyclic = false; > > + list_splice_tail_init(&pch->work_list, &pch->dmac- > >desc_pool); > > + if (pch->cyclic_task) { > > + tasklet_kill(pch->cyclic_task); > > + pch->cyclic_task = NULL; > > + } > > + } > > + > > spin_unlock_irqrestore(&pch->lock, flags); > > } > To be explicit, please initialize 'cyclic' flag to false in > pl330_alloc_chan_resources I will address your comment.
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 880f010..121c75a 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -69,6 +69,11 @@ struct dma_pl330_chan { * NULL if the channel is available to be acquired. */ void *pl330_chid; + + /* taks for cyclic capability */ + struct tasklet_struct *cyclic_task; + + bool cyclic; }; struct dma_pl330_dmac { @@ -184,6 +189,41 @@ static inline void fill_queue(struct dma_pl330_chan *pch) } } +static void pl330_tasklet_cyclic(unsigned long data) +{ + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; + struct dma_pl330_desc *desc, *_dt; + unsigned long flags; + LIST_HEAD(list); + + spin_lock_irqsave(&pch->lock, flags); + + /* Pick up ripe tomatoes */ + list_for_each_entry_safe(desc, _dt, &pch->work_list, node) + if (desc->status == DONE) { + dma_async_tx_callback callback; + + list_move_tail(&desc->node, &pch->work_list); + pch->completed = desc->txd.cookie; + + desc->status = PREP; + + /* Try to submit a req imm. + next to the last completed cookie */ + fill_queue(pch); + + /* Make sure the PL330 Channel thread is active */ + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_START); + + callback = desc->txd.callback; + if (callback) + callback(desc->txd.callback_param); + + } + + spin_unlock_irqrestore(&pch->lock, flags); +} + static void pl330_tasklet(unsigned long data) { struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; @@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum pl330_op_err err) spin_unlock_irqrestore(&pch->lock, flags); + if (pch->cyclic_task) + tasklet_schedule(pch->cyclic_task); + else tasklet_schedule(&pch->task); } @@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan *chan) pl330_release_channel(pch->pl330_chid); pch->pl330_chid = NULL; + if (pch->cyclic) { + pch->cyclic = false; + list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); + if (pch->cyclic_task) { + tasklet_kill(pch->cyclic_task); + pch->cyclic_task = NULL; + } + } + spin_unlock_irqrestore(&pch->lock, flags); } @@ -547,6 +599,63 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) return burst_len; } +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic( + struct dma_chan *chan, dma_addr_t dma_addr, size_t len, + size_t period_len, enum dma_data_direction direction) +{ + struct dma_pl330_desc *desc; + struct dma_pl330_chan *pch = to_pchan(chan); + struct dma_pl330_peri *peri = chan->private; + dma_addr_t dst; + dma_addr_t src; + + pch = to_pchan(chan); + if (!pch) + return NULL; + + desc = pl330_get_desc(pch); + if (!desc) { + dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n", + __func__, __LINE__); + return NULL; + } + + switch (direction) { + case DMA_TO_DEVICE: + desc->rqcfg.src_inc = 1; + desc->rqcfg.dst_inc = 0; + src = dma_addr; + dst = peri->fifo_addr; + break; + case DMA_FROM_DEVICE: + desc->rqcfg.src_inc = 0; + desc->rqcfg.dst_inc = 1; + src = peri->fifo_addr; + dst = dma_addr; + break; + default: + dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n", + __func__, __LINE__); + return NULL; + } + + desc->rqcfg.brst_size = peri->burst_sz; + desc->rqcfg.brst_len = 1; + + if (!pch->cyclic_task) { + pch->cyclic_task = + kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL); + tasklet_init(pch->cyclic_task, + pl330_tasklet_cyclic, (unsigned int)pch); + } + + pch->cyclic = true; + + fill_px(&desc->px, dst, src, period_len); + + return &desc->txd; +} + static struct dma_async_tx_descriptor * pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src, size_t len, unsigned long flags) @@ -780,6 +889,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) case MEMTODEV: case DEVTOMEM: dma_cap_set(DMA_SLAVE, pd->cap_mask); + dma_cap_set(DMA_CYCLIC, pd->cap_mask); break; default: dev_err(&adev->dev, "DEVTODEV Not Supported\n"); @@ -805,6 +915,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) pd->device_alloc_chan_resources = pl330_alloc_chan_resources; pd->device_free_chan_resources = pl330_free_chan_resources; pd->device_prep_dma_memcpy = pl330_prep_dma_memcpy; + pd->device_prep_dma_cyclic = pl330_prep_dma_cyclic; pd->device_tx_status = pl330_tx_status; pd->device_prep_slave_sg = pl330_prep_slave_sg; pd->device_control = pl330_control;
This patch adds DMA_CYCLIC capability that is used for audio driver. DMA driver with DMA_CYCLIC capability reuses the dma requests that were submitted through tx_submit(). Signed-off-by: Boojin Kim <boojin.kim@samsung.com> --- drivers/dma/pl330.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 111 insertions(+), 0 deletions(-)