Skip to contentSkip to conversation

dma: mmp_pdma: add support for residue reporting

PATCH Superseded Daniel Mack 4 participants 9 messages Aug 14, 2013 – Aug 16, 2013 dmaengine

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

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 <[email protected]>
---
 drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)
drivers/dma/mmp_pdma.c +43 -2
index 579f79a..d66340a 100644
@@ -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;
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

Quoted 18 lines · Daniel Mack
drivers/dma/mmp_pdma.c +18 -0
+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;

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 706

Quoted 10 lines
drivers/dma/mmp_pdma.c +10 -0
+
+		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.

#2 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

In reply to Daniel Mack
Quoted 83 lines · Daniel Mack

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 <[email protected]>
---
 drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)
drivers/dma/mmp_pdma.c +32 -2
index 579f79a..d66340a 100644
@@ -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)) {

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 708

Quoted 10 lines
drivers/dma/mmp_pdma.c +10 -0
+                       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.

Quoted 19 lines
drivers/dma/mmp_pdma.c +1 -1
 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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

author
#3 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

Hi Chao,

Quoted 34 lines · Chao Xie
Quoted 20 lines · Daniel Mack
drivers/dma/mmp_pdma.c +20 -0
+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)) {

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 708

Quoted 10 lines
drivers/dma/mmp_pdma.c +10 -0
+                       done += curr - start;
+                       break;
+               }
+
+               done += len;
+       }
+
+       return chan->total_len - done;
+}
+

It seems that you will get all the residue bytes in the channel.

Quoted 8 lines

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.

Quoted 2 lines

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

#4 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

Quoted 46 lines · Daniel Mack

Hi Chao,

Quoted 34 lines · Chao Xie
Quoted 20 lines · Daniel Mack
drivers/dma/mmp_pdma.c +20 -0
+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)) {

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 708

Quoted 10 lines
drivers/dma/mmp_pdma.c +10 -0
+                       done += curr - start;
+                       break;
+               }
+
+               done += len;
+       }
+
+       return chan->total_len - done;
+}
+

It seems that you will get all the residue bytes in the channel.

Quoted 8 lines

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.

Quoted 9 lines

Ok, right.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

Thanks for the review,
Daniel

author
#5 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

Quoted 6 lines · Chao Xie

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

#6 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

Quoted 25 lines · Daniel Mack
Quoted 6 lines · Chao Xie

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.

Quoted 4 lines

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.

Quoted 4 lines

Thanks for sharing your ideas,
Daniel

#7 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

In reply to Daniel Mack
Quoted 9 lines · Daniel Mack

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 <[email protected]>
---
  drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 43 insertions(+), 2 deletions(-)

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 685

Quoted 7 lines
drivers/dma/mmp_pdma.c +7 -0
+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 at 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

Signature · 2 lines
Regards,
Xiang
author
#8 lore raw

PATCH v3 1/2 dma: mmp_pdma: add support for residue reporting

Quoted 10 lines · Xiang Wang
Quoted 8 lines · Daniel Mack

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 <[email protected]>
---
  drivers/dma/mmp_pdma.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 43 insertions(+), 2 deletions(-)
Quoted 11 lines

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 685

Quoted 7 lines
drivers/dma/mmp_pdma.c +7 -0
+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 at 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.

Quoted 6 lines

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