| Message ID | 1376497189-21298-1-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 14, 2013 at 06:19:48PM +0200, Daniel Mack wrote: > +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) > +{ > + struct mmp_pdma_desc_sw *sw; > + u32 curr, done = 0; > + > + if (chan->dir == DMA_DEV_TO_MEM) > + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); > + else > + curr = readl(chan->phy->base + DSADR(chan->phy->idx)); > + > + list_for_each_entry(sw, &chan->chain_running, node) { > + u32 start; > + u32 len = sw->desc.dcmd & DCMD_LENGTH; > + > + if (chan->dir == DMA_DEV_TO_MEM) > + start = sw->desc.dtadr; > + else > + start = sw->desc.dsadr; > + > + if (curr >= start && curr <= (start + len)) { > + done += curr - start; > + break; > + } > + > + done += len; > + } > + > + return chan->total_len - done; Yep, that should be sufficient. Thanks for fixing that.
On Thu, Aug 15, 2013 at 12:19 AM, Daniel Mack <zonque@gmail.com> wrote: > In order to report the channel's residue, we have to memorize the first > dma buffer position when the channel is configured. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c > index 579f79a..d66340a 100644 > --- a/drivers/dma/mmp_pdma.c > +++ b/drivers/dma/mmp_pdma.c > @@ -28,8 +28,8 @@ > #define DALGN 0x00a0 > #define DINT 0x00f0 > #define DDADR 0x0200 > -#define DSADR 0x0204 > -#define DTADR 0x0208 > +#define DSADR(n) (0x0204 + ((n) << 4)) > +#define DTADR(n) (0x0208 + ((n) << 4)) > #define DCMD 0x020c > > #define DCSR_RUN (1 << 31) /* Run Bit (read / write) */ > @@ -97,6 +97,11 @@ struct mmp_pdma_chan { > struct dma_async_tx_descriptor desc; > struct mmp_pdma_phy *phy; > enum dma_transfer_direction dir; > + /* > + * We memorize the original total length so we can later determine the > + * channel's residue. > + */ > + u32 total_len; > > /* channel's basic info */ > struct tasklet_struct tasklet; > @@ -470,6 +475,8 @@ mmp_pdma_prep_memcpy(struct dma_chan *dchan, > chan->dcmd |= DCMD_BURST32; > } > > + chan->total_len = len; > + > do { > /* Allocate the link descriptor from DMA pool */ > new = mmp_pdma_alloc_descriptor(chan); > @@ -541,11 +548,14 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl, > return NULL; > > chan->byte_align = false; > + chan->total_len = 0; > > for_each_sg(sgl, sg, sg_len, i) { > addr = sg_dma_address(sg); > avail = sg_dma_len(sgl); > > + chan->total_len += avail; > + > do { > len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES); > if (addr & 0x7) > @@ -666,6 +676,36 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, > return ret; > } > > +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) > +{ > + struct mmp_pdma_desc_sw *sw; > + u32 curr, done = 0; > + > + if (chan->dir == DMA_DEV_TO_MEM) > + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); > + else > + curr = readl(chan->phy->base + DSADR(chan->phy->idx)); > + > + list_for_each_entry(sw, &chan->chain_running, node) { > + u32 start; > + u32 len = sw->desc.dcmd & DCMD_LENGTH; > + > + if (chan->dir == DMA_DEV_TO_MEM) > + start = sw->desc.dtadr; > + else > + start = sw->desc.dsadr; > + > + if (curr >= start && curr <= (start + len)) { > + done += curr - start; > + break; > + } > + > + done += len; > + } > + > + return chan->total_len - done; > +} > + It seems that you will get all the residue bytes in the channel. For DMA transfer, user may submit many trasaction. The transaction will be composed by mutiple desctiptors. So it means that chan->chain_running includes the descriptors from all ongoing transactions. When user try to get the tx_status, user will pass the cookie to identify which transaction it want to get the status. So for mmp_pdma_residue, it need care about the cookie, and find out the transaction, then calculate the residue bytes. The orignal driver has a issue, it will assign cookie for all descriptors, not the transaction. I think that need to be changed. > static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, > dma_cookie_t cookie, struct dma_tx_state *txstate) > { > @@ -675,6 +715,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, > > spin_lock_irqsave(&chan->desc_lock, flags); > ret = dma_cookie_status(dchan, cookie, txstate); > + txstate->residue = mmp_pdma_residue(chan); > spin_unlock_irqrestore(&chan->desc_lock, flags); > > return ret; > -- > 1.8.3.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Chao, On 15.08.2013 03:57, Chao Xie wrote: > On Thu, Aug 15, 2013 at 12:19 AM, Daniel Mack <zonque@gmail.com> wrote: >> +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) >> +{ >> + struct mmp_pdma_desc_sw *sw; >> + u32 curr, done = 0; >> + >> + if (chan->dir == DMA_DEV_TO_MEM) >> + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); >> + else >> + curr = readl(chan->phy->base + DSADR(chan->phy->idx)); >> + >> + list_for_each_entry(sw, &chan->chain_running, node) { >> + u32 start; >> + u32 len = sw->desc.dcmd & DCMD_LENGTH; >> + >> + if (chan->dir == DMA_DEV_TO_MEM) >> + start = sw->desc.dtadr; >> + else >> + start = sw->desc.dsadr; >> + >> + if (curr >= start && curr <= (start + len)) { >> + done += curr - start; >> + break; >> + } >> + >> + done += len; >> + } >> + >> + return chan->total_len - done; >> +} >> + > > It seems that you will get all the residue bytes in the channel. > For DMA transfer, user may submit many trasaction. > The transaction will be composed by mutiple desctiptors. So it means > that chan->chain_running includes > the descriptors from all ongoing transactions. > When user try to get the tx_status, user will pass the cookie to > identify which transaction it want to get the status. > So for mmp_pdma_residue, it need care about the cookie, and find out > the transaction, then calculate the residue bytes. Ok, right. > The orignal driver has a issue, it will assign cookie for all > descriptors, not the transaction. I think that need to be changed. Not necessarily. If all the linked descriptors are assigned the same cookie, we can as well just walk all entries in chain_running and ignore the ones that don't match the cookie we're looking for, The problem with your proposed approach is that the driver will currently dispose desc->tx_list via list_splice_tail_init() once the transaction has been submitted, so we can't access it later. But what I have in mind should work equally well. I'll post an updated patch later. Thanks for the review, Daniel
On Thu, Aug 15, 2013 at 11:44 AM, Daniel Mack <zonque@gmail.com> wrote: > Hi Chao, > > On 15.08.2013 03:57, Chao Xie wrote: >> On Thu, Aug 15, 2013 at 12:19 AM, Daniel Mack <zonque@gmail.com> wrote: > >>> +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) >>> +{ >>> + struct mmp_pdma_desc_sw *sw; >>> + u32 curr, done = 0; >>> + >>> + if (chan->dir == DMA_DEV_TO_MEM) >>> + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); >>> + else >>> + curr = readl(chan->phy->base + DSADR(chan->phy->idx)); >>> + >>> + list_for_each_entry(sw, &chan->chain_running, node) { >>> + u32 start; >>> + u32 len = sw->desc.dcmd & DCMD_LENGTH; >>> + >>> + if (chan->dir == DMA_DEV_TO_MEM) >>> + start = sw->desc.dtadr; >>> + else >>> + start = sw->desc.dsadr; >>> + >>> + if (curr >= start && curr <= (start + len)) { >>> + done += curr - start; >>> + break; >>> + } >>> + >>> + done += len; >>> + } >>> + >>> + return chan->total_len - done; >>> +} >>> + >> >> It seems that you will get all the residue bytes in the channel. >> For DMA transfer, user may submit many trasaction. >> The transaction will be composed by mutiple desctiptors. So it means >> that chan->chain_running includes >> the descriptors from all ongoing transactions. >> When user try to get the tx_status, user will pass the cookie to >> identify which transaction it want to get the status. >> So for mmp_pdma_residue, it need care about the cookie, and find out >> the transaction, then calculate the residue bytes. > > Ok, right. > >> The orignal driver has a issue, it will assign cookie for all >> descriptors, not the transaction. I think that need to be changed. > > Not necessarily. If all the linked descriptors are assigned the same > cookie, we can as well just walk all entries in chain_running and ignore > the ones that don't match the cookie we're looking for, > In the function mmp_pdma_tx_submit It will assign cookie for every descriptors by list_for_each_entry(child, &desc->tx_list, node) { cookie = dma_cookie_assign(&child->async_tx); } So not all the descriptors in same transaction will share same cookie. > The problem with your proposed approach is that the driver will > currently dispose desc->tx_list via list_splice_tail_init() once the > transaction has been submitted, so we can't access it later. But what I > have in mind should work equally well. I'll post an updated patch later. > The desc->tx_list will be linked to chain_running, so in chain_running, you can still get the mmp_pdma_desc_sw. So there are two ways 1. When user submit a transaction, only the first descriptor will be assgined the cookie, and you can check chan->completed_cookie for whether the transaction is done or not. For the residue bytes, you can go through chain_running, and find out the last descriptor for the transaction by comparing the cookie. Then you can tell the residue bytes. For all the desriptors in the chain_running, only the descriptor has non-zero cookie is the first descriptor of the transaction. 2. Assign cookie for all the desriptors in same transaction, and record the first one, and last one. Then you can calculate the residue bytes too. > > Thanks for the review, > Daniel >
On 15.08.2013 10:08, Chao Xie wrote: > In the function mmp_pdma_tx_submit > It will assign cookie for every descriptors by > list_for_each_entry(child, &desc->tx_list, node) { > cookie = dma_cookie_assign(&child->async_tx); > } > So not all the descriptors in same transaction will share same cookie. Hmm, yes. You're right. However, I see quite some more issues with the driver as it stands, especially due to assumptions that are made for multiple transactions on one channel. For example, let's think about two transactions with multiple descriptors each, and both transactions are present in the chain_running list. Each transaction has the ENDIRQEN bit set on the last descriptor only. Currently, when an interrupt is caught, the code will only look at the very last entry in the running list and complete its cookie, and then dispose the entire running chain. Hence, the other transaction's cookie will never complete. Even worse: as an interrupt will always be for the first descriptor in the chain with the ENDIRQEN bit set, we complete the second transaction that is in fact still running (and call the completion handler for it), while we don't do exactly that for the first one. Am I right with the above or do I make faulty assumptions? I'll think about how the driver can be changed in order to fix this, but it will be more invasive than I thought. Thanks for sharing your ideas, Daniel
On Fri, Aug 16, 2013 at 2:12 AM, Daniel Mack <zonque@gmail.com> wrote: > On 15.08.2013 10:08, Chao Xie wrote: >> In the function mmp_pdma_tx_submit >> It will assign cookie for every descriptors by >> list_for_each_entry(child, &desc->tx_list, node) { >> cookie = dma_cookie_assign(&child->async_tx); >> } >> So not all the descriptors in same transaction will share same cookie. > > Hmm, yes. You're right. However, I see quite some more issues with the > driver as it stands, especially due to assumptions that are made for > multiple transactions on one channel. > > For example, let's think about two transactions with multiple > descriptors each, and both transactions are present in the chain_running > list. Each transaction has the ENDIRQEN bit set on the last descriptor only. > > Currently, when an interrupt is caught, the code will only look at the > very last entry in the running list and complete its cookie, and then > dispose the entire running chain. Hence, the other transaction's cookie > will never complete. Even worse: as an interrupt will always be for the > first descriptor in the chain with the ENDIRQEN bit set, we complete the > second transaction that is in fact still running (and call the > completion handler for it), while we don't do exactly that for the first > one. > > Am I right with the above or do I make faulty assumptions? I'll think > about how the driver can be changed in order to fix this, but it will be > more invasive than I thought. > I think you are right. I include the orignal author for the driver, and maybe he can comment at it. > > Thanks for sharing your ideas, > Daniel >
On 08/15/2013 12:19 AM, Daniel Mack wrote: > In order to report the channel's residue, we have to memorize the first > dma buffer position when the channel is configured. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > > +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) > +{ > + struct mmp_pdma_desc_sw *sw; > + u32 curr, done = 0; > + > + if (chan->dir == DMA_DEV_TO_MEM) > + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); One concern for this patch: If we call dmaengine_tx_status() after dma_do_tasklet@mmp_pdma.c, chan->phy will most likely to be NULL. dma_do_tasklet() -> start_pending_queue() -> mmp_pdma_free_phy() For why do we call dmaengine_tx_status() after a DMA interrupt: briefly, if we use DMA to handle device trailing bytes, it'll report a DMA interrupt. We need to get to know how many bytes have been received in the IRQ handler. We've discussed about this in the link below: http://thread.gmane.org/gmane.linux.kernel/1500713
On 16.08.2013 09:57, Xiang Wang wrote: > On 08/15/2013 12:19 AM, Daniel Mack wrote: >> In order to report the channel's residue, we have to memorize the first >> dma buffer position when the channel is configured. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> >> --- >> drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 43 insertions(+), 2 deletions(-) >> > >> +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) >> +{ >> + struct mmp_pdma_desc_sw *sw; >> + u32 curr, done = 0; >> + >> + if (chan->dir == DMA_DEV_TO_MEM) >> + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); > One concern for this patch: > If we call dmaengine_tx_status() after dma_do_tasklet@mmp_pdma.c, > chan->phy will most likely to be NULL. > dma_do_tasklet() -> start_pending_queue() -> mmp_pdma_free_phy() Yes, I noticed this, too. I'm not really sure what the reason is to call start_pending_queue() _before_ the callbacks are done. A quick test shows that it works as well the other way around. > For why do we call dmaengine_tx_status() after a DMA interrupt: > briefly, if we use DMA to handle device trailing bytes, it'll report a > DMA interrupt. We need to get to know how many bytes have been received > in the IRQ handler. > We've discussed about this in the link below: > http://thread.gmane.org/gmane.linux.kernel/1500713 Thanks for the link, I wasn't aware of that discussion. And I actually second what Andy said: As long as the residue calculation algorithm can cope with the chan->phy==NULL condition and return 0 in that case, it's actually doing the right thing, because a completed transaction has a residue of 0 bytes. I'll send out another series later, that also aims to fix the tasklet behaviour. I'd appreciate another round of reviews :) Daniel
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c index 579f79a..d66340a 100644 --- a/drivers/dma/mmp_pdma.c +++ b/drivers/dma/mmp_pdma.c @@ -28,8 +28,8 @@ #define DALGN 0x00a0 #define DINT 0x00f0 #define DDADR 0x0200 -#define DSADR 0x0204 -#define DTADR 0x0208 +#define DSADR(n) (0x0204 + ((n) << 4)) +#define DTADR(n) (0x0208 + ((n) << 4)) #define DCMD 0x020c #define DCSR_RUN (1 << 31) /* Run Bit (read / write) */ @@ -97,6 +97,11 @@ struct mmp_pdma_chan { struct dma_async_tx_descriptor desc; struct mmp_pdma_phy *phy; enum dma_transfer_direction dir; + /* + * We memorize the original total length so we can later determine the + * channel's residue. + */ + u32 total_len; /* channel's basic info */ struct tasklet_struct tasklet; @@ -470,6 +475,8 @@ mmp_pdma_prep_memcpy(struct dma_chan *dchan, chan->dcmd |= DCMD_BURST32; } + chan->total_len = len; + do { /* Allocate the link descriptor from DMA pool */ new = mmp_pdma_alloc_descriptor(chan); @@ -541,11 +548,14 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl, return NULL; chan->byte_align = false; + chan->total_len = 0; for_each_sg(sgl, sg, sg_len, i) { addr = sg_dma_address(sg); avail = sg_dma_len(sgl); + chan->total_len += avail; + do { len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES); if (addr & 0x7) @@ -666,6 +676,36 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, return ret; } +static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan) +{ + struct mmp_pdma_desc_sw *sw; + u32 curr, done = 0; + + if (chan->dir == DMA_DEV_TO_MEM) + curr = readl(chan->phy->base + DTADR(chan->phy->idx)); + else + curr = readl(chan->phy->base + DSADR(chan->phy->idx)); + + list_for_each_entry(sw, &chan->chain_running, node) { + u32 start; + u32 len = sw->desc.dcmd & DCMD_LENGTH; + + if (chan->dir == DMA_DEV_TO_MEM) + start = sw->desc.dtadr; + else + start = sw->desc.dsadr; + + if (curr >= start && curr <= (start + len)) { + done += curr - start; + break; + } + + done += len; + } + + return chan->total_len - done; +} + static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, dma_cookie_t cookie, struct dma_tx_state *txstate) { @@ -675,6 +715,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, spin_lock_irqsave(&chan->desc_lock, flags); ret = dma_cookie_status(dchan, cookie, txstate); + txstate->residue = mmp_pdma_residue(chan); spin_unlock_irqrestore(&chan->desc_lock, flags); return ret;
In order to report the channel's residue, we have to memorize the first dma buffer position when the channel is configured. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-)