[PATCH v4 0/4] dma: pdma: cyclic, residue, DMA_PRIVATE

[PATCH v4 0/4] dma: pdma: cyclic, residue, DMA_PRIVATE

[Daniel Mack]

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

[PATCH v4 1/4] dma: mmp_pdma: only complete one transaction from dma_do_tasklet()

[Daniel Mack]

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.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 579f79a..9929f85 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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

[PATCH v4 1/4] dma: mmp_pdma: only complete one transaction from dma_do_tasklet()

[Xiang Wang]

On 08/17/2013 01:05 AM, Daniel Mack wrote:
> 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.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>   drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++--------------
>   1 file changed, 21 insertions(+), 14 deletions(-)
>

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

[PATCH v4 1/4] dma: mmp_pdma: only complete one transaction from dma_do_tasklet()

[Daniel Mack]

Hi Xiang,

On 20.08.2013 04:27, Xiang Wang wrote:
> 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.

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

[PATCH v4 1/4] dma: mmp_pdma: only complete one transaction from dma_do_tasklet()

[Robert Jarzmik]

Daniel Mack <zonque@gmail.com> writes:

> Hi Xiang,
>
> On 20.08.2013 04:27, Xiang Wang wrote:
>> 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.
>
> 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.

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.

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

[PATCH v4 2/4] dma: mmp_pdma: add support for cyclic DMA descriptors

[Daniel Mack]

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

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 9929f85..0be99a6 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

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

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

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Xiang Wang]

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!

-- 
Regards,
Xiang

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Daniel Mack]

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

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Vinod Koul]

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

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Russell King - ARM Linux]

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.

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Vinod Koul]

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

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Russell King - ARM Linux]

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

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Robert Jarzmik]

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.

-- 
Robert

[PATCH v4 3/4] dma: mmp_pdma: add support for residue reporting

[Daniel Mack]

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

[PATCH v4 4/4] dma: mmp_pdma: set DMA_PRIVATE

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

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index b29b581..dfccfe1 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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