Skip to contentSkip to conversation

dma: pdma: cyclic, residue, DMA_PRIVATE

PATCH Superseded Daniel Mack 3 participants 16 messages Aug 21, 2013 – Dec 11, 2013 dmaengine

lore
Share & tools
lore
#0 lore raw

v5 cover dma: pdma: cyclic, residue, DMA_PRIVATE

This is v5 of my series to teach the pdma driver more required functions.

In contrast to v4, it has yet again grown by one more patch (2/5), which removes code that clears DCMD_ENDIRQEN at the end of the pending chain. We want to leave of the DCMD_ENDIRQEN intact in order to identify where a transaction ends.

Thanks to Xiang Wang and Andy Shevchenko for their reviews.

Daniel

Daniel Mack (5):
dma: mmp_pdma: only complete one transaction from dma_do_tasklet()
dma: mmp_pdma: don't clear DCMD_ENDIRQEN at end of pending chain
dma: mmp_pdma: add support for cyclic DMA descriptors
dma: mmp_pdma: add support for residue reporting
dma: mmp_pdma: set DMA_PRIVATE

 drivers/dma/mmp_pdma.c | 247 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 210 insertions(+), 37 deletions(-)
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v5 1/5 dma: mmp_pdma: only complete one transaction from dma_do_tasklet()

Currently, when an interrupt has occured for a channel, the tasklet worker 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 first transaction's cookie will never complete.

In fact, the interrupt we should handle will be the one related to the first descriptor in the chain with the ENDIRQEN bit set, so complete the second transaction that is in fact still running.

As a result, the driver can't currently handle multiple transactions on one chanel, and it's likely that no drivers exist that rely on this feature.

Fix this by walking the running_chain and look for the first descriptor that has the interrupt-enable bit set. Only queue descriptors up to that point for completion handling, while leaving the rest intact. Also, only make the channel idle if the list is completely empty after such a cycle.

---
 drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)
drivers/dma/mmp_pdma.c +21 -14
index 579f79a..9929f85 100644
@@ -710,25 +710,32 @@ static void dma_do_tasklet(unsigned long data)
 
 	spin_lock_irqsave(&chan->desc_lock, flags);
 
-	/* update the cookie if we have some descriptors to cleanup */
-	if (!list_empty(&chan->chain_running)) {
-		dma_cookie_t cookie;
-
-		desc = to_mmp_pdma_desc(chan->chain_running.prev);
-		cookie = desc->async_tx.cookie;
-		dma_cookie_complete(&desc->async_tx);
+	list_for_each_entry_safe(desc, _desc, &chan->chain_running, node) {
+		/*
+		 * move the descriptors to a temporary list so we can drop
+		 * the lock during the entire cleanup operation
+		 */
+		list_del(&desc->node);
+		list_add(&desc->node, &chain_cleanup);
 
-		dev_dbg(chan->dev, "completed_cookie=%d\n", cookie);
+		/*
+		 * Look for the first list entry which has the ENDIRQEN flag
+		 * set. That is the descriptor we got an interrupt for, so
+		 * complete that transaction and its cookie.
+		 */
+		if (desc->desc.dcmd & DCMD_ENDIRQEN) {
+			dma_cookie_t cookie = desc->async_tx.cookie;
+			dma_cookie_complete(&desc->async_tx);
+			dev_dbg(chan->dev, "completed_cookie=%d\n", cookie);
+			break;
+		}
 	}
 
 	/*
-	 * move the descriptors to a temporary list so we can drop the lock
-	 * during the entire cleanup operation
+	 * The hardware is idle and ready for more when the
+	 * chain_running list is empty.
 	 */
-	list_splice_tail_init(&chan->chain_running, &chain_cleanup);
-
-	/* the hardware is now idle and ready for more */
-	chan->idle = true;
+	chan->idle = list_empty(&chan->chain_running);
 
 	/* Start any pending transactions automatically */
 	start_pending_queue(chan);
Signature · 1 line
1.8.3.1
#2 lore raw

PATCH v5 2/5 dma: mmp_pdma: don't clear DCMD_ENDIRQEN at end of pending chain

In reply to Daniel Mack

In order to fully support multiple transactions per channel, we need to assure we get an interrupt for each completed transaction. That flags bit is also our only way to tell at which descriptor a transaction ends.

So, remove the manual clearing of that bit, and then inline the only remaining command that is left in append_pending_queue() for better readability.

---
 drivers/dma/mmp_pdma.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)
drivers/dma/mmp_pdma.c +2 -20
index 9929f85..31e9a71 100644
@@ -279,25 +279,6 @@ static void mmp_pdma_free_phy(struct mmp_pdma_chan *pchan)
 	spin_unlock_irqrestore(&pdev->phy_lock, flags);
 }
 
-/* desc->tx_list ==> pending list */
-static void append_pending_queue(struct mmp_pdma_chan *chan,
-					struct mmp_pdma_desc_sw *desc)
-{
-	struct mmp_pdma_desc_sw *tail =
-				to_mmp_pdma_desc(chan->chain_pending.prev);
-
-	if (list_empty(&chan->chain_pending))
-		goto out_splice;
-
-	/* one irq per queue, even appended */
-	tail->desc.ddadr = desc->async_tx.phys;
-	tail->desc.dcmd &= ~DCMD_ENDIRQEN;
-
-	/* softly link to pending list */
-out_splice:
-	list_splice_tail_init(&desc->tx_list, &chan->chain_pending);
-}
-
 /**
  * start_pending_queue - transfer any pending transactions
  * pending list ==> running list
@@ -360,7 +341,8 @@ static dma_cookie_t mmp_pdma_tx_submit(struct dma_async_tx_descriptor *tx)
 		cookie = dma_cookie_assign(&child->async_tx);
 	}
 
-	append_pending_queue(chan, desc);
+	/* softly link to pending list - desc->tx_list ==> pending list */
+	list_splice_tail_init(&desc->tx_list, &chan->chain_pending);
 
 	spin_unlock_irqrestore(&chan->desc_lock, flags);
 
Signature · 1 line
1.8.3.1
#3 lore raw

PATCH v5 3/5 dma: mmp_pdma: add support for cyclic DMA descriptors

In reply to Daniel Mack

Provide a callback to prepare cyclic DMA transfers. This is for instance needed for audio channel transport.

---
 drivers/dma/mmp_pdma.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)
drivers/dma/mmp_pdma.c +111 -1
index 31e9a71..d82a4f6 100644
@@ -98,6 +98,9 @@ struct mmp_pdma_chan {
 	struct mmp_pdma_phy *phy;
 	enum dma_transfer_direction dir;
 
+	struct mmp_pdma_desc_sw *cyclic_first;	/* first desc_sw if channel
+						 * is in cyclic mode */
+
 	/* channel's basic info */
 	struct tasklet_struct tasklet;
 	u32 dcmd;
@@ -499,6 +502,8 @@ mmp_pdma_prep_memcpy(struct dma_chan *dchan,
 	new->desc.ddadr = DDADR_STOP;
 	new->desc.dcmd |= DCMD_ENDIRQEN;
 
+	chan->cyclic_first = NULL;
+
 	return &first->async_tx;
 
 fail:
@@ -574,6 +579,94 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
 	new->desc.ddadr = DDADR_STOP;
 	new->desc.dcmd |= DCMD_ENDIRQEN;
 
+	chan->dir = dir;
+	chan->cyclic_first = NULL;
+
+	return &first->async_tx;
+
+fail:
+	if (first)
+		mmp_pdma_free_desc_list(chan, &first->tx_list);
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *mmp_pdma_prep_dma_cyclic(
+	struct dma_chan *dchan, dma_addr_t buf_addr, size_t len,
+	size_t period_len, enum dma_transfer_direction direction,
+	unsigned long flags, void *context)
+{
+	struct mmp_pdma_chan *chan;
+	struct mmp_pdma_desc_sw *first = NULL, *prev = NULL, *new;
+	dma_addr_t dma_src, dma_dst;
+
+	if (!dchan || !len || !period_len)
+		return NULL;
+
+	/* the buffer length must be a multiple of period_len */
+	if (len % period_len != 0)
+		return NULL;
+
+	if (period_len > PDMA_MAX_DESC_BYTES)
+		return NULL;
+
+	chan = to_mmp_pdma_chan(dchan);
+
+	switch (direction) {
+	case DMA_MEM_TO_DEV:
+		dma_src = buf_addr;
+		dma_dst = chan->dev_addr;
+		break;
+	case DMA_DEV_TO_MEM:
+		dma_dst = buf_addr;
+		dma_src = chan->dev_addr;
+		break;
+	default:
+		dev_err(chan->dev, "Unsupported direction for cyclic DMA\n");
+		return NULL;
+	}
+
+	chan->dir = direction;
+
+	do {
+		/* Allocate the link descriptor from DMA pool */
+		new = mmp_pdma_alloc_descriptor(chan);
+		if (!new) {
+			dev_err(chan->dev, "no memory for desc\n");
+			goto fail;
+		}
+
+		new->desc.dcmd = chan->dcmd | DCMD_ENDIRQEN |
+					(DCMD_LENGTH & period_len);
+		new->desc.dsadr = dma_src;
+		new->desc.dtadr = dma_dst;
+
+		if (!first)
+			first = new;
+		else
+			prev->desc.ddadr = new->async_tx.phys;
+
+		new->async_tx.cookie = 0;
+		async_tx_ack(&new->async_tx);
+
+		prev = new;
+		len -= period_len;
+
+		if (chan->dir == DMA_MEM_TO_DEV)
+			dma_src += period_len;
+		else
+			dma_dst += period_len;
+
+		/* Insert the link descriptor to the LD ring */
+		list_add_tail(&new->node, &first->tx_list);
+	} while (len);
+
+	first->async_tx.flags = flags; /* client is in control of this ack */
+	first->async_tx.cookie = -EBUSY;
+
+	/* make the cyclic link */
+	new->desc.ddadr = first->async_tx.phys;
+	chan->cyclic_first = first;
+
 	return &first->async_tx;
 
 fail:
@@ -688,8 +781,23 @@ static void dma_do_tasklet(unsigned long data)
 	LIST_HEAD(chain_cleanup);
 	unsigned long flags;
 
-	/* submit pending list; callback for each desc; free desc */
+	if (chan->cyclic_first) {
+		dma_async_tx_callback cb = NULL;
+		void *cb_data = NULL;
 
+		spin_lock_irqsave(&chan->desc_lock, flags);
+		desc = chan->cyclic_first;
+		cb = desc->async_tx.callback;
+		cb_data = desc->async_tx.callback_param;
+		spin_unlock_irqrestore(&chan->desc_lock, flags);
+
+		if (cb)
+			cb(cb_data);
+
+		return;
+	}
+
+	/* submit pending list; callback for each desc; free desc */
 	spin_lock_irqsave(&chan->desc_lock, flags);
 
 	list_for_each_entry_safe(desc, _desc, &chan->chain_running, node) {
@@ -886,12 +994,14 @@ static int mmp_pdma_probe(struct platform_device *op)
 
 	dma_cap_set(DMA_SLAVE, pdev->device.cap_mask);
 	dma_cap_set(DMA_MEMCPY, pdev->device.cap_mask);
+	dma_cap_set(DMA_CYCLIC, pdev->device.cap_mask);
 	pdev->device.dev = &op->dev;
 	pdev->device.device_alloc_chan_resources = mmp_pdma_alloc_chan_resources;
 	pdev->device.device_free_chan_resources = mmp_pdma_free_chan_resources;
 	pdev->device.device_tx_status = mmp_pdma_tx_status;
 	pdev->device.device_prep_dma_memcpy = mmp_pdma_prep_memcpy;
 	pdev->device.device_prep_slave_sg = mmp_pdma_prep_slave_sg;
+	pdev->device.device_prep_dma_cyclic = mmp_pdma_prep_dma_cyclic;
 	pdev->device.device_issue_pending = mmp_pdma_issue_pending;
 	pdev->device.device_control = mmp_pdma_control;
 	pdev->device.copy_align = PDMA_ALIGNMENT;
Signature · 1 line
1.8.3.1
#4 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

In reply to Daniel Mack

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.

---
 drivers/dma/mmp_pdma.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)
drivers/dma/mmp_pdma.c +75 -2
index d82a4f6..efb583f 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) */
@@ -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;
Signature · 1 line
1.8.3.1
#5 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

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

Quoted 13 lines · Daniel Mack

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.

drivers/dma/mmp_pdma.c +7 -0
+		/*
+		 * '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.

drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825

Quoted 8 lines

static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,

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

#6 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

Hi Vinod,

Sorry for the late response, I've been on vacations.

Quoted 7 lines · Vinod Koul
Quoted 6 lines · Daniel Mack

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.

Quoted 8 lines

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

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

Quoted 3 lines

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?

Quoted 5 lines
Quoted 5 lines

static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,

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

drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825

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

#7 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

Quoted 21 lines · Daniel Mack

Hi Vinod,

Sorry for the late response, I've been on vacations.

Quoted 7 lines · Vinod Koul
Quoted 6 lines · Daniel Mack

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.

Quoted 8 lines

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

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

Quoted 3 lines

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.

Quoted 5 lines
Quoted 3 lines

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

#8 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

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 :)

Quoted 22 lines · Vinod Koul
Quoted 21 lines · Daniel Mack

Hi Vinod,

Sorry for the late response, I've been on vacations.

Quoted 15 lines · Vinod Koul

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

Quoted 13 lines · Daniel Mack

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.

drivers/dma/mmp_pdma.c +7 -0
+		/*
+		 * '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.

Quoted 7 lines
Quoted 3 lines

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

#9 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

Quoted 52 lines · Daniel Mack

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 :)

Quoted 22 lines · Vinod Koul
Quoted 21 lines · Daniel Mack

Hi Vinod,

Sorry for the late response, I've been on vacations.

Quoted 15 lines · Vinod Koul

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

Quoted 13 lines · Daniel Mack

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.

drivers/dma/mmp_pdma.c +7 -0
+		/*
+		 * '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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 5 lines

* 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

Quoted 10 lines

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

#10 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

Hi,

Quoted 18 lines · Vinod Koul
Quoted 16 lines · Daniel Mack

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.

Quoted 7 lines
Quoted 5 lines

* 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.

Quoted 3 lines

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.

Quoted 7 lines

- 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.

Quoted 4 lines
Quoted 3 lines

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

#11 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

In reply to Daniel Mack

drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825

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

#12 lore raw

PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting

Quoted 10 lines · Russell King - ARM Linux

drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825

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

#13 lore raw

PATCH v5 5/5 dma: mmp_pdma: set DMA_PRIVATE

In reply to Daniel Mack

As the driver now has its own xlate function and makes use of the dma_get_slave_channel(), we need to manually set the DMA_PRIVATE flags.

Drivers which rely on of_dma_simple_xlate() do implicitly the same by going through __dma_request_channel().

---
 drivers/dma/mmp_pdma.c | 1 +
 1 file changed, 1 insertion(+)
drivers/dma/mmp_pdma.c +1 -0
index efb583f..3efde2d 100644
@@ -1068,6 +1068,7 @@ static int mmp_pdma_probe(struct platform_device *op)
 	dma_cap_set(DMA_SLAVE, pdev->device.cap_mask);
 	dma_cap_set(DMA_MEMCPY, pdev->device.cap_mask);
 	dma_cap_set(DMA_CYCLIC, pdev->device.cap_mask);
+	dma_cap_set(DMA_PRIVATE, pdev->device.cap_mask);
 	pdev->device.dev = &op->dev;
 	pdev->device.device_alloc_chan_resources = mmp_pdma_alloc_chan_resources;
 	pdev->device.device_free_chan_resources = mmp_pdma_free_chan_resources;
Signature · 1 line
1.8.3.1
Applied
#14 lore raw

v5 cover dma: pdma: cyclic, residue, DMA_PRIVATE

In reply to Daniel Mack
Quoted 6 lines · Daniel Mack

This is v5 of my series to teach the pdma driver more required functions.

In contrast to v4, it has yet again grown by one more patch (2/5), which removes code that clears DCMD_ENDIRQEN at the end of the pending chain. We want to leave of the DCMD_ENDIRQEN intact in order to identify where a transaction ends.

Applied, 1-3 & 5. The 4th one still needs some work

~Vinod

#15 lore raw

v5 cover dma: pdma: cyclic, residue, DMA_PRIVATE

Quoted 10 lines · Vinod Koul
Quoted 1 line · Daniel Mack

This is v5 of my series to teach the pdma driver more required

functions.

Quoted 3 lines

In contrast to v4, it has yet again grown by one more patch (2/5), which removes code that clears DCMD_ENDIRQEN at the end of the

pending

Quoted 2 lines

chain. We want to leave of the DCMD_ENDIRQEN intact in order to identify where a transaction ends.

Applied, 1-3 & 5. The 4th one still needs some work

Thanks. I'm currently travelling and AFK. Will get back to this issue once I'm back, but that will most certainly be after the merge window has opened.

Daniel