dma: pdma: cyclic, residue, DMA_PRIVATE

Here's another spin of my pending patches for mmp_pdma. It has grown by two more patches now, and they address the issues pointed out by Chao Xie and Xiang Wang.

I'd appreciate another round of reviews.

Thanks,
Daniel

Daniel Mack (4):
dma: mmp_pdma: only complete one transaction from dma_do_tasklet()
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 | 225 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 208 insertions(+), 17 deletions(-)

1.8.3.1

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

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

1.8.3.1

Hi, Daniel
I think we would not run into the situation that there is a descriptor with ENDIRQEN set in the middle of the running chain.

in mmp_pdma.c:
mmp_pdma_tx_submit() -> append_pending_queue() ->
tail->desc.dcmd &= ~DCMD_ENDIRQEN;

So in the pending list (same for running list), only the last descriptor will have ENDIRQEN set.

Please correct me if any problems.

Regards,
Xiang

Hi Xiang,

Right, thanks for noting that.

But is that what we want? I think that clearing of the ENDIRQEN bit needs to be removed then, because if we really have multiple transactions running, then we want to get an interrupt whenever _any_ of it is completed. Also, the way I solved it now, the ENDIRQEN bit is also looked at when the residue code tries to distinguish the transactions in the list.

Hence, would you agree that we should remove that "tail->desc.dcmd &= ~DCMD_ENDIRQEN;" modification in order to make the above logic work?

Thanks,
Daniel

At least that's something pxa_camera.c needs, ie. have the dma interrupt handler called as soon as the _first_ of many transactions queued on the same channel is finished.

This is necessary to declare the video capture buffer as "done", which is signaled by a terminated DMA transaction. And of course, in video recording, another one is beginning on the channel, while the finished video buffer can be safely unmapped and handed over to userland.

Same question on my side.

My understanding is that the current bebaviour violates the dmaengine specification (Documentation/dmaengine.txt, chapter 5, "On completion of each DMA operation, the next in queue is started and a tasklet triggered. The tasklet will then call the client driver completion callback routine for notification, if set.)

Cheers.

--
Robert

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

index 9929f85..0be99a6 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;

@@ -517,6 +520,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:

@@ -592,6 +597,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:

@@ -706,8 +799,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) {

@@ -904,12 +1012,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;

1.8.3.1

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

index 0be99a6..b29b581 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) */

@@ -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;

1.8.3.1

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.

With Best Regards,
Andy Shevchenko

Hi Andy,

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.

I'd say that's a bug.

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

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

With Best Regards,
Andy Shevchenko

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.

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.

cookie is assigned to a descriptor.

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

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.

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.

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

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.

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,

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.

Robert

Hi Xiang,

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.

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.

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

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(+)

index b29b581..dfccfe1 100644
@@ -1086,6 +1086,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;

1.8.3.1