| Message ID | 1377086938-29145-5-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: > In order to report the channel's residue, we walk the list of running > descriptors, look for those which match the cookie, and then try to find > the descriptor which defines upper and lower boundaries that embrace the > current transport pointer. > > + /* > + * 'passed' will be latched once we found the descriptor which > + * lies inside the boundaries of the curr pointer. All > + * descriptors that occur in the list _after_ we found that > + * partially handled descriptor are still to be processed and > + * are hence added to the residual bytes counter. > + */ do you have multiple descriptors for one transaction? Should be No. The cookie is assigned to a transaction when it is submitted. so when you see descriptor cookie is less than completed one, then it already completed and should not be in pending list. > static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, > dma_cookie_t cookie, struct dma_tx_state *txstate) > { > @@ -750,6 +822,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, cookie); here you check ret value first, if descriptor is completed then you will get DMA_SUCCESS and just return that, no need to check the residue. If it is pending, then also just return size of transaction. Only when it is progress you need to calculate and check ~Vinod
On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: > @@ -750,6 +822,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, cookie); Please consider using dma_set_residue() here - it is legal to call the tx status function with a NULL txstate.
Hi Vinod, Sorry for the late response, I've been on vacations. On 25.08.2013 18:33, Vinod Koul wrote: > On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: >> In order to report the channel's residue, we walk the list of running >> descriptors, look for those which match the cookie, and then try to find >> the descriptor which defines upper and lower boundaries that embrace the >> current transport pointer. > >> >> + /* >> + * 'passed' will be latched once we found the descriptor which >> + * lies inside the boundaries of the curr pointer. All >> + * descriptors that occur in the list _after_ we found that >> + * partially handled descriptor are still to be processed and >> + * are hence added to the residual bytes counter. >> + */ > do you have multiple descriptors for one transaction? Should be No. Sure, that can be the case. One transaction could span across multiple descriptors, especially if its overall length exceeds the maximum length of one descriptor. > The cookie is assigned to a transaction when it is submitted. so when you see > descriptor cookie is less than completed one, then it already completed and > should not be in pending list. Hmm, what about an integer overrun? >> static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, >> dma_cookie_t cookie, struct dma_tx_state *txstate) >> { >> @@ -750,6 +822,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, cookie); > here you check ret value first, if descriptor is completed then you will get > DMA_SUCCESS and just return that, no need to check the residue. If it is > pending, then also just return size of transaction. Only when it is progress you > need to calculate and check Because in case of success, the residue is always 0. Alright, will change that. Thanks for the feedback! Daniel
On 25.08.2013 20:08, Russell King - ARM Linux wrote: > On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: >> @@ -750,6 +822,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, cookie); > > Please consider using dma_set_residue() here - it is legal to call the > tx status function with a NULL txstate. > Will do. Thanks for the hint. Daniel
On Tue, Sep 10, 2013 at 05:46:19PM +0200, Daniel Mack wrote: > Hi Vinod, > > Sorry for the late response, I've been on vacations. > > On 25.08.2013 18:33, Vinod Koul wrote: > > On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: > >> In order to report the channel's residue, we walk the list of running > >> descriptors, look for those which match the cookie, and then try to find > >> the descriptor which defines upper and lower boundaries that embrace the > >> current transport pointer. > > > >> > >> + /* > >> + * 'passed' will be latched once we found the descriptor which > >> + * lies inside the boundaries of the curr pointer. All > >> + * descriptors that occur in the list _after_ we found that > >> + * partially handled descriptor are still to be processed and > >> + * are hence added to the residual bytes counter. > >> + */ > > do you have multiple descriptors for one transaction? Should be No. > > Sure, that can be the case. One transaction could span across multiple > descriptors, especially if its overall length exceeds the maximum length > of one descriptor. Yes and these addiional descriptor for a transacation should be child descriptors. The child descriptors should have ->parent point to parent descriptor and ->next to next in the chain of children. > > The cookie is assigned to a transaction when it is submitted. so when you see > > descriptor cookie is less than completed one, then it already completed and > > should not be in pending list. > > Hmm, what about an integer overrun? The cookie needs to wrapped to 1. If you use virtual dma code then it would be done. Since the variable is 32bit, i dont think you will have so many eonding trasaction so i havent seen any concerns on that. ~Vinod
Hi Vinod, Very sorry for such a long delay. I only get to pick up on this topic now. Hope you can remember the context of the discussion :) On 09/13/2013 06:51 AM, Vinod Koul wrote: > On Tue, Sep 10, 2013 at 05:46:19PM +0200, Daniel Mack wrote: >> Hi Vinod, >> >> Sorry for the late response, I've been on vacations. >> >> On 25.08.2013 18:33, Vinod Koul wrote: >>> On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: >>>> In order to report the channel's residue, we walk the list of running >>>> descriptors, look for those which match the cookie, and then try to find >>>> the descriptor which defines upper and lower boundaries that embrace the >>>> current transport pointer. >>> >>>> >>>> + /* >>>> + * 'passed' will be latched once we found the descriptor which >>>> + * lies inside the boundaries of the curr pointer. All >>>> + * descriptors that occur in the list _after_ we found that >>>> + * partially handled descriptor are still to be processed and >>>> + * are hence added to the residual bytes counter. >>>> + */ >>> do you have multiple descriptors for one transaction? Should be No. >> >> Sure, that can be the case. One transaction could span across multiple >> descriptors, especially if its overall length exceeds the maximum length >> of one descriptor. > Yes and these addiional descriptor for a transacation should be child > descriptors. > The child descriptors should have ->parent point to parent descriptor and ->next > to next in the chain of children. I looked into your idea and implemented it, but I really feel having an explicit chain pointer for each descriptor is redundant information. Hence, let me summarize again how the driver currently works: * Any of the prep_* function will allocate a number of descriptors to accommodate the payload, and link them all together via the 'node' list_head, forming a transaction. * The last descriptor in a transaction has the DCMD_ENDIRQEN flag set. * When tx_submit() is called, each descriptor in the linked list will be assigned a cookie, and then entire list of the transaction is appended to the chain_pending list. At this point, the information of where a transaction ends is no longer visible in the 'node' list head. * start_pending_queue() moves the chain_pending entries to chain_running. When determining the channel's residue, we need to find the transaction currently in progress, and then count upwards until we reach the end of the transaction chain. So, while I could add a ->next pointer to the descriptors, and only use that to link up descriptors of one transaction in the prep_*() functions, that extra information doesn't actually buy us anything, as the same information is already stored in the DCMD_ENDIRQEN flag. I rebased the residue patch on top of Joe's cleanup work, and I can resubmit if you want me to. Maybe that serves as a better foundation for the ongoing discussion :) Many thanks, Daniel
On Mon, Dec 09, 2013 at 08:00:51PM +0100, Daniel Mack wrote: > Hi Vinod, > > Very sorry for such a long delay. I only get to pick up on this topic > now. Hope you can remember the context of the discussion :) > > On 09/13/2013 06:51 AM, Vinod Koul wrote: > > On Tue, Sep 10, 2013 at 05:46:19PM +0200, Daniel Mack wrote: > >> Hi Vinod, > >> > >> Sorry for the late response, I've been on vacations. > >> > >> On 25.08.2013 18:33, Vinod Koul wrote: > >>> On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote: > >>>> In order to report the channel's residue, we walk the list of running > >>>> descriptors, look for those which match the cookie, and then try to find > >>>> the descriptor which defines upper and lower boundaries that embrace the > >>>> current transport pointer. > >>> > >>>> > >>>> + /* > >>>> + * 'passed' will be latched once we found the descriptor which > >>>> + * lies inside the boundaries of the curr pointer. All > >>>> + * descriptors that occur in the list _after_ we found that > >>>> + * partially handled descriptor are still to be processed and > >>>> + * are hence added to the residual bytes counter. > >>>> + */ > >>> do you have multiple descriptors for one transaction? Should be No. > >> > >> Sure, that can be the case. One transaction could span across multiple > >> descriptors, especially if its overall length exceeds the maximum length > >> of one descriptor. > > Yes and these addiional descriptor for a transacation should be child > > descriptors. > > The child descriptors should have ->parent point to parent descriptor and ->next > > to next in the chain of children. > > I looked into your idea and implemented it, but I really feel having an > explicit chain pointer for each descriptor is redundant information. > > Hence, let me summarize again how the driver currently works: > > * Any of the prep_* function will allocate a number of descriptors to > accommodate the payload, and link them all together via the 'node' > list_head, forming a transaction. > > * The last descriptor in a transaction has the DCMD_ENDIRQEN flag set. > > * When tx_submit() is called, each descriptor in the linked list will be > assigned a cookie, and then entire list of the transaction is appended > to the chain_pending list. At this point, the information of where a > transaction ends is no longer visible in the 'node' list head. > > * start_pending_queue() moves the chain_pending entries to chain_running. > > When determining the channel's residue, we need to find the transaction > currently in progress, and then count upwards until we reach the end of > the transaction chain. ah here is the catch! You dont get reside for a channel. You get for the respetive transaction represented by the descriptor! So lets establish few rules: - Assume you have transactions A, B, C and D in a list - on sumbmit (assume serial), you get descriptor 1, 2, 3 and 4 respectively - Now residue is for a descriptor, not a series of descriptors!, so - If DMA is started and currently 1 is being transferred, then - residue on 1 will give remaining bytes of 1 - residue on 2 will give full length At client, you can sum up and decide how much is remianing for your point of interest. Yes things will be a bit different if your submit a transaction and dma driver splits to multiple descriptors and the client driver doesnt know about them. But since split is done by DMA driver, it know how to manage > > So, while I could add a ->next pointer to the descriptors, and only use > that to link up descriptors of one transaction in the prep_*() > functions, that extra information doesn't actually buy us anything, as > the same information is already stored in the DCMD_ENDIRQEN flag. > > > I rebased the residue patch on top of Joe's cleanup work, and I can > resubmit if you want me to. Maybe that serves as a better foundation for > the ongoing discussion :) Sure pls do post... -- ~Vinod
Hi, On 12/10/2013 05:11 PM, Vinod Koul wrote: > On Mon, Dec 09, 2013 at 08:00:51PM +0100, Daniel Mack wrote: >> I looked into your idea and implemented it, but I really feel having an >> explicit chain pointer for each descriptor is redundant information. >> >> Hence, let me summarize again how the driver currently works: >> >> * Any of the prep_* function will allocate a number of descriptors to >> accommodate the payload, and link them all together via the 'node' >> list_head, forming a transaction. >> >> * The last descriptor in a transaction has the DCMD_ENDIRQEN flag set. >> >> * When tx_submit() is called, each descriptor in the linked list will be >> assigned a cookie, and then entire list of the transaction is appended >> to the chain_pending list. At this point, the information of where a >> transaction ends is no longer visible in the 'node' list head. >> >> * start_pending_queue() moves the chain_pending entries to chain_running. >> >> When determining the channel's residue, we need to find the transaction >> currently in progress, and then count upwards until we reach the end of >> the transaction chain. > ah here is the catch! You dont get reside for a channel. You get for the > respetive transaction represented by the descriptor! Yes, exactly. I think part of the reason why we're not on the same page is a misleading wording on my side. Let me try and explain. > So lets establish few rules: > - Assume you have transactions A, B, C and D in a list > - on sumbmit (assume serial), you get descriptor 1, 2, 3 and 4 respectively You're referring to dma_async_tx_descriptor, while I was talking about mmp_pdma_desc_sw. IOW: what's handed out by the driver from any of its prep_* functions are dma_async_tx_descriptors, which consist of N chained mmp_pdma_desc_sw descriptors internally. Another specialty of this particular driver is that each mmp_pdma_desc_sw is assigned a dma_cookie_t, but only the last one in a chained list is handed out to the user. Following your example, the transaction-cookie matching could look like this, for example: A: 1, 2, 3, 4 B: 5 C: 6, 7, 8 D: 9, 10 So the only cookies the user 'knows' about and query the residue for, are 4, 5, 8 and 10. > - Now residue is for a descriptor, not a series of descriptors!, so > - If DMA is started and currently 1 is being transferred, then > - residue on 1 will give remaining bytes of 1 > - residue on 2 will give full length > > At client, you can sum up and decide how much is remianing for your point of > interest. Absolutely. I understand that concept :) In this driver, however, I have to deal with multiple mmp_pdma_desc_sw descriptors that are all chained up in the channel's running list. Hence, my implementation goes as follows: * Walk the list of all mmp_pdma_desc_sw until we find the end of a transaction chain; IOW: find the end of one dma_async_tx_descriptor. This end entry has the DCMD_ENDIRQEN flag set. * Check if that mmp_pdma_desc_sw corresponds with the cookie the user wants to know the residue for, and if it does, return the residue that was calculated during the iteration. * Otherwise, reset the internal state and carry on. >> I rebased the residue patch on top of Joe's cleanup work, and I can >> resubmit if you want me to. Maybe that serves as a better foundation for >> the ongoing discussion :) > Sure pls do post... Will do. Thanks, Daniel
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c index d82a4f6..efb583f 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) */ @@ -741,6 +741,78 @@ 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, + dma_cookie_t cookie) +{ + struct mmp_pdma_desc_sw *sw; + u32 curr, residue = 0; + bool passed = false; + bool cyclic = chan->cyclic_first != NULL; + + /* + * If the channel does not have a phy pointer anymore, it has already + * been completed. Therefore, its residue is 0. + */ + if (!chan->phy) + return 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, end, len; + + if (chan->dir == DMA_DEV_TO_MEM) + start = sw->desc.dtadr; + else + start = sw->desc.dsadr; + + len = sw->desc.dcmd & DCMD_LENGTH; + end = start + len; + + /* + * 'passed' will be latched once we found the descriptor which + * lies inside the boundaries of the curr pointer. All + * descriptors that occur in the list _after_ we found that + * partially handled descriptor are still to be processed and + * are hence added to the residual bytes counter. + */ + if (passed) { + residue += len; + } else if (curr >= start && curr <= end) { + residue += end - curr; + passed = true; + } + + /* + * Descriptors that have the ENDIRQEN bit set mark the end of a + * transaction chain, and the cookie assigned with it has been + * returned previously from mmp_pdma_tx_submit(). + * + * In case we have multiple transactions in the running chain, + * and the cookie does not match the one the user asked us + * about, reset the state variables and start over. + * + * This logic does not apply to cyclic transactions, where all + * descriptors have the ENDIRQEN bit set, and for which we + * can't have multiple transactions on one channel anyway. + */ + if (!cyclic && (sw->desc.dcmd & DCMD_ENDIRQEN)) { + if (sw->async_tx.cookie != cookie) { + residue = 0; + passed = false; + } else { + return residue; + } + } + } + + /* We should only get here in case of cyclic transactions */ + return residue; +} + static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, dma_cookie_t cookie, struct dma_tx_state *txstate) { @@ -750,6 +822,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, cookie); spin_unlock_irqrestore(&chan->desc_lock, flags); return ret;
In order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/dma/mmp_pdma.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 2 deletions(-)