| Message ID | 1376672707-24527-4-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/17/2013 01:05 AM, 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. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/dma/mmp_pdma.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 75 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c Hi, Daniel One thing I want to check with you and all: If we have these descriptors in the running chain: D1T1 -> D2T1 -> D3T1 => D1T2 -> D2T2 -> D3T2 | Transaction 1 | Transaction 2 | And DMA currently running to D2T1, if we provide the cookie of D3T2, what the residual value we should get? All unfinished bytes in T1+T2 or only unfinished bytes in T2 (seems not easy to implement)? > index 0be99a6..b29b581 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) */ > @@ -759,6 +759,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; From my point of view, there is one concern: If we want to use the End-of-Receive Interrupt (EORIRQEN in DCSR), when an interrupt comes, usually it doesn't mean there is no bytes left. We talked about this in this thread: http://thread.gmane.org/gmane.linux.kernel/1500713 I posted a patch some time ago: http://thread.gmane.org/gmane.linux.kernel/1510435 There were some comments suggested by Vinod. Thanks!
On Wed, Aug 21, 2013 at 2:35 PM, Xiang Wang <wangx@marvell.com> wrote: > On 08/17/2013 01:05 AM, 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. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> > One thing I want to check with you and all: > If we have these descriptors in the running chain: > D1T1 -> D2T1 -> D3T1 => D1T2 -> D2T2 -> D3T2 > | Transaction 1 | Transaction 2 | > > And DMA currently running to D2T1, if we provide the cookie of D3T2, what > the residual value we should get? All unfinished bytes in T1+T2 or only > unfinished bytes in T2 (seems not easy to implement)? You have a really good question. Different drivers do different things. In dw_dmac, for example, we return the T1 (means 'active') residue always. I don't think you have different cookies per descriptors in chain of one transaction.
Hi Xiang, On 21.08.2013 13:35, Xiang Wang wrote: > On 08/17/2013 01:05 AM, Daniel Mack wrote: > Hi, Daniel > One thing I want to check with you and all: > If we have these descriptors in the running chain: > D1T1 -> D2T1 -> D3T1 => D1T2 -> D2T2 -> D3T2 > | Transaction 1 | Transaction 2 | > > And DMA currently running to D2T1, if we provide the cookie of D3T2, > what the residual value we should get? All unfinished bytes in T1+T2 or > only unfinished bytes in T2 (seems not easy to implement)? We returned the cookie of T2 when the user submitted the transaction, and so if we're asked about the residue of cookie(T2), it should only count bytes of T2. That's also how I implemented it. >> +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; > > From my point of view, there is one concern: > If we want to use the End-of-Receive Interrupt (EORIRQEN in DCSR), when > an interrupt comes, usually it doesn't mean there is no bytes left. If an interrupt occurs, it does because any of the descriptors has EORIRQEN set. And an interrupt for one specific channel will occur for the first descriptor in the list. If there is another one which has EORIRQEN set, there will be a second one for this one, too. For non-cyclic transfers, an occured interrupt that occurs at the end of a transaction *does* mean that it is finished, and hence the above check returns 0 in that case. I believe that's correct, but only as long as we really just handle and complete the oldest transaction in the chain. Note that for cyclic transfers, things are different, because every descriptor in the transaction has the EORIRQEN set. That is necessary because users want a callback after each period. > We talked about this in this thread: > http://thread.gmane.org/gmane.linux.kernel/1500713 > > I posted a patch some time ago: > http://thread.gmane.org/gmane.linux.kernel/1510435 > There were some comments suggested by Vinod. Yes, I've read that, and I believe that all these comments are addressed in my series. I'll repost a v5 now, which includes one more patch to remove the clearing of EORIRQEN of the last descriptor when a transaction is appended. Many thanks for your review. Much appreciated :) Daniel
Hi Andy, On 21.08.2013 13:52, Andy Shevchenko wrote: > On Wed, Aug 21, 2013 at 2:35 PM, Xiang Wang <wangx@marvell.com> wrote: >> On 08/17/2013 01:05 AM, 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. >>> >>> Signed-off-by: Daniel Mack <zonque@gmail.com> > >> One thing I want to check with you and all: >> If we have these descriptors in the running chain: >> D1T1 -> D2T1 -> D3T1 => D1T2 -> D2T2 -> D3T2 >> | Transaction 1 | Transaction 2 | >> >> And DMA currently running to D2T1, if we provide the cookie of D3T2, what >> the residual value we should get? All unfinished bytes in T1+T2 or only >> unfinished bytes in T2 (seems not easy to implement)? > > You have a really good question. Different drivers do different things. I think the idea of how things should work is pretty clear, but I'd like Vinod to ack my understanding here :) The user is free to submit multiple transactions per channel, and dmaengine_submit() will submit a new cookie for each one. When the engine is asked about the residue of a specific cookie, it should of course only return the value for the related transaction. If that transaction is not yet processed, the residue value should be the complete length, regardless of the transaction that is currently processed. > In dw_dmac, for example, we return the T1 (means 'active') residue always. I'd say that's a bug. > I don't think you have different cookies per descriptors in chain of > one transaction. Currently, the pdma driver does in fact assign a cookie to each descriptor, but the external user will only get so see the cookie for the last descriptor in the transaction. I don't think that's a problem, but we have to maintain an understanding of which descriptors belong to one transaction. Thanks, Daniel
On Wed, Aug 21, 2013 at 3:03 PM, Daniel Mack <zonque@gmail.com> wrote: > On 21.08.2013 13:52, Andy Shevchenko wrote: >> On Wed, Aug 21, 2013 at 2:35 PM, Xiang Wang <wangx@marvell.com> wrote: >>> One thing I want to check with you and all: >>> If we have these descriptors in the running chain: >>> D1T1 -> D2T1 -> D3T1 => D1T2 -> D2T2 -> D3T2 >>> | Transaction 1 | Transaction 2 | >>> >>> And DMA currently running to D2T1, if we provide the cookie of D3T2, what >>> the residual value we should get? All unfinished bytes in T1+T2 or only >>> unfinished bytes in T2 (seems not easy to implement)? >> >> You have a really good question. Different drivers do different things. > > I think the idea of how things should work is pretty clear, but I'd like > Vinod to ack my understanding here :) > > The user is free to submit multiple transactions per channel, and > dmaengine_submit() will submit a new cookie for each one. > > When the engine is asked about the residue of a specific cookie, it > should of course only return the value for the related transaction. If > that transaction is not yet processed, the residue value should be the > complete length, regardless of the transaction that is currently processed. > >> In dw_dmac, for example, we return the T1 (means 'active') residue always. > > I'd say that's a bug. When I wrote and sent the previous message I have the same thought. Now I'm looking how to fix it properly (fix seems to be really simple). Though, we are using workflow when we didn't submit more than one transaction at the time (which makes quite sense in case of HSUART).
On Wed, Aug 21, 2013 at 02:03:12PM +0200, Daniel Mack wrote: > Hi Andy, > > On 21.08.2013 13:52, Andy Shevchenko wrote: > > On Wed, Aug 21, 2013 at 2:35 PM, Xiang Wang <wangx@marvell.com> wrote: > >> On 08/17/2013 01:05 AM, 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. > >>> > >>> Signed-off-by: Daniel Mack <zonque@gmail.com> > > > >> One thing I want to check with you and all: > >> If we have these descriptors in the running chain: > >> D1T1 -> D2T1 -> D3T1 => D1T2 -> D2T2 -> D3T2 > >> | Transaction 1 | Transaction 2 | > >> > >> And DMA currently running to D2T1, if we provide the cookie of D3T2, what > >> the residual value we should get? All unfinished bytes in T1+T2 or only > >> unfinished bytes in T2 (seems not easy to implement)? > > > > You have a really good question. Different drivers do different things. > > I think the idea of how things should work is pretty clear, but I'd like > Vinod to ack my understanding here :) > > The user is free to submit multiple transactions per channel, and > dmaengine_submit() will submit a new cookie for each one. > > When the engine is asked about the residue of a specific cookie, it > should of course only return the value for the related transaction. If > that transaction is not yet processed, the residue value should be the > complete length, regardless of the transaction that is currently processed. Yes that is the idea. See the argument of the dmaengine_tx_status() API is a cookie. This is the cookie you need to check and find out the status. Now there is an intresting thing about last argument dma_tx_state. Here you would know which was last cookie completed and which is used (in progress). Also descriptor is returned for a transaction when you invoke .device_prep_xxx. And each descriptor is assigned a cookie value. Now in above case you wont have D1TI unless at client level you have broken to differen prepare calls to dmaengine. Assuming that you get cookie 1, 2, 3, 4, 5, 6 assigned respectively to above. Now if you called dmaengine_tx_status() for cookie 5, when 1 has completed and 2nd in progress, the right implementation would tell you residue is size, and state->completed =1, state->used =2. If you have invoked for 2, then you would get in-flight residue. > > > In dw_dmac, for example, we return the T1 (means 'active') residue always. > > I'd say that's a bug. yes, that would be a bug BUT since you call dma_cookie_status() and maintain chan->completed_cookie by invoking dma_cookie_complete() dmaengine gives right values for completed ones. For pending ones looks like it is not right and you need to see if cookie invoked for is in progress or not. > > > I don't think you have different cookies per descriptors in chain of > > one transaction. cookie is assigned to a descriptor. > Currently, the pdma driver does in fact assign a cookie to each > descriptor, but the external user will only get so see the cookie for > the last descriptor in the transaction. I don't think that's a problem, > but we have to maintain an understanding of which descriptors belong to > one transaction. descriptor->cookie tells you the cookie value for the transaction represented by the descriptor which you check and should pass when calling dmaengine_tx_status(). The last completed descriptor is marked in chan->completed_cookie. So when somone calls dmaengine_tx_status() you already know if its in flight or not. ~Vinod
On Sun, Aug 25, 2013 at 06:01:51PM +0100, Russell King - ARM Linux wrote: > On Sun, Aug 25, 2013 at 09:41:04PM +0530, Vinod Koul wrote: > > Also descriptor is returned for a transaction when you invoke .device_prep_xxx. > > And each descriptor is assigned a cookie value. Sorry this wasnt meant to be in same timeline, just stating that descriptor is always assigned a cookie value and yes that will be done _only_ when descriptor is submitted by invoking .tx_submit(). In dma driver, one should use cookie value to check status Also, the reason for not assigning cookie in prepare is simple. If we preared three trasactions A, B and C. But sumbitted A, C and then B then whole cookie logic will fail, so it makes sense to assign cookie in an ordered fashion when trasactions are submitted not when prepared. Though i suspect in slave dma case, we would see ordered manner typically. > The descriptor is not assigned a cookie in the preparation function - it > is only assigned a cookie when it is submitted, which should always happen > shortly after preparation. The submission call returns the assigned > cookie, so there's no reason for any driver to dereference the descriptor. > > Any driver which does dereference the descriptor after submission is > potentially a bug; the DMA engine owns it, and can kfree that descriptor > any moment after submission, or even re-use it for another descriptor. I would say after callback is invoked in case when one is set. For the ones when no callback is set, yes after invoking tx_submit() is right one. Clients should remeber the cookie value returned and use it to track the status of transactions. ~Vinod
On Sun, Aug 25, 2013 at 09:41:04PM +0530, Vinod Koul wrote: > Also descriptor is returned for a transaction when you invoke .device_prep_xxx. > And each descriptor is assigned a cookie value. The descriptor is not assigned a cookie in the preparation function - it is only assigned a cookie when it is submitted, which should always happen shortly after preparation. The submission call returns the assigned cookie, so there's no reason for any driver to dereference the descriptor. Any driver which does dereference the descriptor after submission is potentially a bug; the DMA engine owns it, and can kfree that descriptor any moment after submission, or even re-use it for another descriptor.
On Sun, Aug 25, 2013 at 10:18:42PM +0530, Vinod Koul wrote: > On Sun, Aug 25, 2013 at 06:01:51PM +0100, Russell King - ARM Linux wrote: > > The descriptor is not assigned a cookie in the preparation function - it > > is only assigned a cookie when it is submitted, which should always happen > > shortly after preparation. The submission call returns the assigned > > cookie, so there's no reason for any driver to dereference the descriptor. > > > > Any driver which does dereference the descriptor after submission is > > potentially a bug; the DMA engine owns it, and can kfree that descriptor > > any moment after submission, or even re-use it for another descriptor. > > I would say after callback is invoked in case when one is set. For the > ones when no callback is set, yes after invoking tx_submit() is right one. Even when there is a callback set, there's no requirement for the DMA engine driver to keep the descriptor around after it has been submitted - it is free to copy that data into whatever internal representation it requires and then discard the descriptor if it so wishes. > Clients should remeber the cookie value returned and use it to track > the status of transactions. Yes - and the correct way is: desc = dma_prepare_....() desc->callback = my_callback; desc->callback_param = my_callback_data; cookie = dmaengine_submit(desc); /* forget that desc exists */
Hi Russell, Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > Even when there is a callback set, there's no requirement for the DMA > engine driver to keep the descriptor around after it has been submitted - > it is free to copy that data into whatever internal representation it > requires and then discard the descriptor if it so wishes. That means that a driver which wished to "resubmit" the descriptor has to "rebuild it" again, no ? I was under the impression that a descriptor had the guarantee to survive even after completion. This comes from include/linux/dmaengine.h:168, in enum dma_ctrl_flags comment : * @DMA_CTRL_ACK - if clear, the descriptor cannot be reused until the client * acknowledges receipt, i.e. has has a chance to establish any dependency * chains The sentence "cannot be reused until" meant that after it could be reused. And there are usecases for that, for example a video capture buffer. The source for DMA is always the same (camera FIFO), the destination is always the same (ie. the same scatter-gather). Why rebuild all the info to resubmit the same descriptor ? So is my understand incorrect, and in that case is there a way with dmaengine to "resubmit" a descriptor, without going through the whole preparation ? Cheers.
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c index 0be99a6..b29b581 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) */ @@ -759,6 +759,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) { @@ -768,6 +840,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(-)