dmaengine: pl330: Set residue in tx_status callback

Fill txstate.residue with the amount of bytes remaining in the current transfer if the transfer is not complete. This will be of particular use to i2s DMA transfers, providing more accurate hw_ptr values to ASoC.

I had taken the code from Dylan Reid <[email protected]> patch from the below link and modified according to the current dmaengine framework.
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/23007

Cc Dylan Reid <[email protected]>

Signed-off-by Padmavathi Venna <[email protected]>

---

This patch has been tested for audio playback on exynos5420 peach-pit.

 drivers/dma/pl330.c |   67
+++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 65 insertions(+), 2 deletions(-)

drivers/dma/pl330.c +64 -1

index b7493d2..db880ae 100644
@@ -2182,11 +2182,74 @@ static void pl330_free_chan_resources(struct
 dma_chan *chan)
 	pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
 }
 
+static inline int
+pl330_src_addr_in_desc(struct dma_pl330_desc *desc, unsigned int sar)
+{
+	return ((desc->px.src_addr <= sar) &&
+		(sar <= (desc->px.src_addr + desc->px.bytes)));
+}
+
+static inline int
+pl330_dst_addr_in_desc(struct dma_pl330_desc *desc, unsigned int dar)
+{
+	return ((desc->px.dst_addr <= dar) &&
+		(dar <= (desc->px.dst_addr + desc->px.bytes)));
+}
+
 static enum dma_status
 pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 		 struct dma_tx_state *txstate)
 {
-	return dma_cookie_status(chan, cookie, txstate);
+	dma_addr_t sar, dar;
+	struct dma_pl330_chan *pch = to_pchan(chan);
+	void __iomem *regs = pch->dmac->base;
+	struct pl330_thread *thrd = pch->thread;
+	struct dma_pl330_desc *desc;
+	unsigned int residue = 0;
+	unsigned long flags;
+	bool first = true;
+	dma_cookie_t first_c, current_c;
+	dma_cookie_t used;
+	enum dma_status ret;
+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate)
+		return ret;
+
+	used = txstate->used;
+
+	spin_lock_irqsave(&pch->lock, flags);
+	sar = readl(regs + SA(thrd->id));
+	dar = readl(regs + DA(thrd->id));
+
+	list_for_each_entry(desc, &pch->work_list, node) {
+		if (desc->status == BUSY) {
+			current_c = desc->txd.cookie;
+			if (first) {
+				first_c = desc->txd.cookie;
+				first = false;
+			}
+
+			if (first_c < current_c)
+				residue += desc->px.bytes;
+			else {
+				if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
+					residue += desc->px.bytes;
+					residue -= sar - desc->px.src_addr;
+				} else if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar)) {
+					residue += desc->px.bytes;
+					residue -= dar - desc->px.dst_addr;
+				}
+			}
+		} else if (desc->status == PREP)
+			residue += desc->px.bytes;
+
+		if (desc->txd.cookie == used)
+			break;
+	}
+	spin_unlock_irqrestore(&pch->lock, flags);
+	dma_set_residue(txstate, residue);
+	return ret;
 }
 

static void pl330_issue_pending(struct dma_chan *chan)

drivers/dma/pl330.c +1 -1

@@ -2631,7 +2694,7 @@ static int pl330_dma_device_slave_caps(struct dma_chan
 *dchan,
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = false;
 	caps->cmd_terminate = true;
-	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
 
 	return 0;

}

--
1.7.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Vinod/Lars,

Quoted 107 lines · Padmavathi Venna

Fill txstate.residue with the amount of bytes remaining in the current transfer if the transfer is not complete. This will be of particular use to i2s DMA transfers, providing more accurate hw_ptr values to ASoC.

I had taken the code from Dylan Reid <[email protected]> patch from the below link and modified according to the current dmaengine framework.
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/23007

Cc Dylan Reid <[email protected]>

Signed-off-by Padmavathi Venna <[email protected]>

---

This patch has been tested for audio playback on exynos5420 peach-pit.

  drivers/dma/pl330.c |   67
+++++++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 65 insertions(+), 2 deletions(-)

drivers/dma/pl330.c +64 -1

index b7493d2..db880ae 100644
@@ -2182,11 +2182,74 @@ static void pl330_free_chan_resources(struct
 dma_chan *chan)
  	pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
  }
 
+static inline int
+pl330_src_addr_in_desc(struct dma_pl330_desc *desc, unsigned int sar)
+{
+	return ((desc->px.src_addr <= sar) &&
+		(sar <= (desc->px.src_addr + desc->px.bytes)));
+}
+
+static inline int
+pl330_dst_addr_in_desc(struct dma_pl330_desc *desc, unsigned int dar)
+{
+	return ((desc->px.dst_addr <= dar) &&
+		(dar <= (desc->px.dst_addr + desc->px.bytes)));
+}
+
  static enum dma_status
  pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
  		 struct dma_tx_state *txstate)
  {
-	return dma_cookie_status(chan, cookie, txstate);
+	dma_addr_t sar, dar;
+	struct dma_pl330_chan *pch = to_pchan(chan);
+	void __iomem *regs = pch->dmac->base;
+	struct pl330_thread *thrd = pch->thread;
+	struct dma_pl330_desc *desc;
+	unsigned int residue = 0;
+	unsigned long flags;
+	bool first = true;
+	dma_cookie_t first_c, current_c;
+	dma_cookie_t used;
+	enum dma_status ret;
+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate)
+		return ret;
+
+	used = txstate->used;
+
+	spin_lock_irqsave(&pch->lock, flags);
+	sar = readl(regs + SA(thrd->id));
+	dar = readl(regs + DA(thrd->id));
+
+	list_for_each_entry(desc, &pch->work_list, node) {
+		if (desc->status == BUSY) {
+			current_c = desc->txd.cookie;
+			if (first) {
+				first_c = desc->txd.cookie;
+				first = false;
+			}
+
+			if (first_c < current_c)
+				residue += desc->px.bytes;
+			else {
+				if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
+					residue += desc->px.bytes;
+					residue -= sar - desc->px.src_addr;
+				} else if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar)) {
+					residue += desc->px.bytes;
+					residue -= dar - desc->px.dst_addr;
+				}
+			}
+		} else if (desc->status == PREP)
+			residue += desc->px.bytes;
+
+		if (desc->txd.cookie == used)
+			break;
+	}
+	spin_unlock_irqrestore(&pch->lock, flags);
+	dma_set_residue(txstate, residue);
+	return ret;
  }
 

static void pl330_issue_pending(struct dma_chan *chan)

drivers/dma/pl330.c +1 -1

@@ -2631,7 +2694,7 @@ static int pl330_dma_device_slave_caps(struct dma_chan
 *dchan,
  	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
  	caps->cmd_pause = false;
  	caps->cmd_terminate = true;
-	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
 

Quoted 2 lines

drivers/dma/pl330.c +0 -0

  	return 0;

  }

Any comment on this patch?

Quoted 41 lines

Quoted 41 lines

drivers/dma/pl330.c +26 -0

+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate)
+		return ret;
+
+	used = txstate->used;
+
+	spin_lock_irqsave(&pch->lock, flags);
+	sar = readl(regs + SA(thrd->id));
+	dar = readl(regs + DA(thrd->id));
+
+	list_for_each_entry(desc, &pch->work_list, node) {
+		if (desc->status == BUSY) {
+			current_c = desc->txd.cookie;
+			if (first) {
+				first_c = desc->txd.cookie;
+				first = false;
+			}
+
+			if (first_c < current_c)
+				residue += desc->px.bytes;
+			else {
+				if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
+					residue += desc->px.bytes;
+					residue -= sar - desc->px.src_addr;
+				} else if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar))

{

drivers/dma/pl330.c +13 -0

+					residue += desc->px.bytes;
+					residue -= dar - desc->px.dst_addr;
+				}
+			}
+		} else if (desc->status == PREP)
+			residue += desc->px.bytes;
+
+		if (desc->txd.cookie == used)
+			break;
+	}
+	spin_unlock_irqrestore(&pch->lock, flags);
+	dma_set_residue(txstate, residue);
+	return ret;
  }

Quoted 2 lines

Any comment on this patch?

Well it doesn't break audio, but I don't think it has the correct haviour for all cases yet.

Again, the semantics are that it should return the progress of the transfer

for which the allocation function returned the cookie that is passe to this

function. You have to consider that there might be multiple different descriptors submitted and in the work list, not just the one we want to know

the status of. The big problem with the pl330 driver is that the current structure of the driver makes it not so easy to implement the residue reporting correctly.

- Lars

Hi Lars,

[snip]

Quoted 41 lines

Quoted 41 lines

Quoted 41 lines

drivers/dma/pl330.c +26 -0

+
+   ret = dma_cookie_status(chan, cookie, txstate);
+   if (ret == DMA_COMPLETE || !txstate)
+           return ret;
+
+   used = txstate->used;
+
+   spin_lock_irqsave(&pch->lock, flags);
+   sar = readl(regs + SA(thrd->id));
+   dar = readl(regs + DA(thrd->id));
+
+   list_for_each_entry(desc, &pch->work_list, node) {
+           if (desc->status == BUSY) {
+                   current_c = desc->txd.cookie;
+                   if (first) {
+                           first_c = desc->txd.cookie;
+                           first = false;
+                   }
+
+                   if (first_c < current_c)
+                           residue += desc->px.bytes;
+                   else {
+                           if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
+                                   residue += desc->px.bytes;
+                                   residue -= sar - desc->px.src_addr;
+                           } else if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar))

{

drivers/dma/pl330.c +13 -0

+                                   residue += desc->px.bytes;
+                                   residue -= dar - desc->px.dst_addr;
+                           }
+                   }
+           } else if (desc->status == PREP)
+                   residue += desc->px.bytes;
+
+           if (desc->txd.cookie == used)
+                   break;
+   }
+   spin_unlock_irqrestore(&pch->lock, flags);
+   dma_set_residue(txstate, residue);
+   return ret;
  }

[snip]

Quoted 5 lines

Quoted 2 lines

Any comment on this patch?

Well it doesn't break audio, but I don't think it has the correct haviour for all cases yet.

OK. Any way of testing other cases like scatter-gather and memcopy. I verified memcopy in dmatest but it seems not doing anything with residue bytes.

Quoted 4 lines

Again, the semantics are that it should return the progress of the transfer

for which the allocation function returned the cookie that is passe to this

May be my understanding is wrong. For clarification..In the snd_dmaengine_pcm_pointer it is subtracting the residue bytes from the total buffer bytes not from period bytes. So how it expects
the progress of the transfer of the passed cookie which just holds period bytes?

Quoted 1 line

Quoted 2 lines

function. You have to consider that there might be multiple different descriptors submitted and in the work list, not just the one we want to know

Even though there are multiple descriptors in the work list, at a time only two descriptors are in busy state(as per the documentation in the driver) and all the descriptors cookie number is in incremental order. Not sure for other cases how it will be.

Quoted 66 lines · Padma Venkat

Hi Lars,

[snip]

Quoted 41 lines

Quoted 41 lines

Quoted 41 lines

drivers/dma/pl330.c +26 -0

+
+   ret = dma_cookie_status(chan, cookie, txstate);
+   if (ret == DMA_COMPLETE || !txstate)
+           return ret;
+
+   used = txstate->used;
+
+   spin_lock_irqsave(&pch->lock, flags);
+   sar = readl(regs + SA(thrd->id));
+   dar = readl(regs + DA(thrd->id));
+
+   list_for_each_entry(desc, &pch->work_list, node) {
+           if (desc->status == BUSY) {
+                   current_c = desc->txd.cookie;
+                   if (first) {
+                           first_c = desc->txd.cookie;
+                           first = false;
+                   }
+
+                   if (first_c < current_c)
+                           residue += desc->px.bytes;
+                   else {
+                           if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
+                                   residue += desc->px.bytes;
+                                   residue -= sar - desc->px.src_addr;
+                           } else if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar))

{

drivers/dma/pl330.c +13 -0

+                                   residue += desc->px.bytes;
+                                   residue -= dar - desc->px.dst_addr;
+                           }
+                   }
+           } else if (desc->status == PREP)
+                   residue += desc->px.bytes;
+
+           if (desc->txd.cookie == used)
+                   break;
+   }
+   spin_unlock_irqrestore(&pch->lock, flags);
+   dma_set_residue(txstate, residue);
+   return ret;
  }

[snip]

Quoted 5 lines

Quoted 2 lines

Any comment on this patch?

Well it doesn't break audio, but I don't think it has the correct haviour for all cases yet.

OK. Any way of testing other cases like scatter-gather and memcopy. I verified memcopy in dmatest but it seems not doing anything with residue bytes.

Quoted 4 lines

Again, the semantics are that it should return the progress of the transfer

for which the allocation function returned the cookie that is passe to this

May be my understanding is wrong. For clarification..In the snd_dmaengine_pcm_pointer it is subtracting the residue bytes from the total buffer bytes not from period bytes. So how it expects
the progress of the transfer of the passed cookie which just holds period bytes?

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

function. You have to consider that there might be multiple different descriptors submitted and in the work list, not just the one we want to know

Even though there are multiple descriptors in the work list, at a time only two descriptors are in busy state(as per the documentation in the driver) and all the descriptors cookie number is in incremental order. Not sure for other cases how it will be.

Yes.

Tracing the history ... I think we could have done without

04abf5daf7d dma: pl330: Differentiate between submitted and issued descriptors

The pl330 dmaengine driver currently does not differentiate
between submitted
and issued descriptors. It won't start transferring a newly submitted descriptor until issue_pending() is called, but only if it is idle. If it is active and a new descriptor is submitted before it goes idle it will happily start the newly submitted descriptor once all earlier submitted
descriptors have
been completed. This is not a 100% correct with regards to the dmaengine interface semantics. A descriptor is not supposed to be started
until the next
issue_pending() call after the descriptor has been submitted.

because the reasoning above seems incorrect considering the following documentation...

Documentation/crypto/async-tx-api.txt says
" .... Once a driver-specific threshold is met the driver
automatically issues pending operations. An application can force this event by calling async_tx_issue_pending_all(). ...."

And

include/linux/dmaengine.h says
dma_async_tx_descriptor.tx_submit(): set the prepared descriptor(s)
to be executed by the engine

so theoretically a driver, not starting transfer until issue_pending(), is "broken".
At best the patch@04abf5daf7d makes the driver slightly more complicated and the reason behind confusion such as in this thread.

-jassi

because the reasoning above seems incorrect considering the following documentation...

Documentation/crypto/async-tx-api.txt says

" .... Once a driver-specific threshold is met the driver
automatically issues pending operations. An application can force this event by calling async_tx_issue_pending_all(). ...."

include/linux/dmaengine.h says
dma_async_tx_descriptor.tx_submit(): set the prepared descriptor(s)
to be executed by the engine

so theoretically a driver, not starting transfer until issue_pending(), is "broken".

At best the patch at 04abf5daf7d makes the driver slightly more complicated and the reason behind confusion such as in this thread.

Quoted 4 lines · Jassi Brar

because the reasoning above seems incorrect considering the following documentation...

Documentation/crypto/async-tx-api.txt says

                       ^^^^^^^^^^^^

async-tx-api is not the DMA slave API.

Quoted 3 lines

" .... Once a driver-specific threshold is met the driver
automatically issues pending operations. An application can force this event by calling async_tx_issue_pending_all(). ...."

                   ^^^^^^^^^^^^^^^^^^^^^^^^^^

DMA slave users do not call this function. This documentation is not relevant for DMA slave.

Quoted 3 lines

include/linux/dmaengine.h says
dma_async_tx_descriptor.tx_submit(): set the prepared descriptor(s)
to be executed by the engine

It doesn't say when.

Quoted 2 lines

so theoretically a driver, not starting transfer until issue_pending(), is "broken".

It isn't. DMA slave engine implementations have been needing the issue_pending() call since their dawn, so it's something that they've always needed.

Quoted 2 lines

At best the patch at 04abf5daf7d makes the driver slightly more complicated and the reason behind confusion such as in this thread.

That may be, and yes, it _might_ be worth discussing whether this should be relaxed or not, but that should be done as a proposal, not trying to hide it as a bug fix. It isn't.

It does, though, create an "awkward situation" when a channel is active while new requests are submitted - why would the channel want to stop after current transfer and await fresh issue_pending()? It's not that the request can be modified after submission.

And it isn't that tx_submit() is meant for 'sleepable' preparation for the transfer and we need another call to be issued from atomic context. From what I see most drivers don't need to sleep in tx_submit(). In fact, from a quick look most clients seem to suffer from the ritual i.e, there's nothing between tx_submit() and issue_pending() calls. And when there is indeed some code, it seems that can be moved just before tx_submit().

Last and apparently the least of all, we can never enforce the same behavior of the api on Async dma and have uniform behavior over the api.

So, if all tx_submit() does is put the request in controller driver's internal queue and the client can't touch the request after tx_submit(), why not merge the tx_submit() and issue_pending()?

I actually like the split model, you can also prepare txn ahead of time and submit them when needed.

Quoted 2 lines · Vinod Koul

I actually like the split model, you can also prepare txn ahead of time and submit them when needed.

Actually, you can't - that's not permitted. I have email(s) from Dan explicitly stating that it is permitted for a driver to take a spinlock in their prepare callback, and release it when the descriptor is submitted. Several DMA engine drivers (particularly those in for async_tx) do exactly that.

The reason that submit is separate from prepare is to allow DMA engine users to set a callback - if it weren't for that, there wouldn't be a submit step, prepare would have done everything.

Quoted 13 lines · Jassi Brar

It does, though, create an "awkward situation" when a channel is active while new requests are submitted - why would the channel want to stop after current transfer and await fresh issue_pending()? It's not that the request can be modified after submission.

And it isn't that tx_submit() is meant for 'sleepable' preparation for the transfer and we need another call to be issued from atomic context. From what I see most drivers don't need to sleep in tx_submit(). In fact, from a quick look most clients seem to suffer from the ritual i.e, there's nothing between tx_submit() and issue_pending() calls. And when there is indeed some code, it seems that can be moved just before tx_submit().

Quoted 7 lines

Last and apparently the least of all, we can never enforce the same behavior of the api on Async dma and have uniform behavior over the api.

So, if all tx_submit() does is put the request in controller driver's internal queue and the client can't touch the request after tx_submit(), why not merge the tx_submit() and issue_pending()?

You are thinking from an API point and not what can be done with this API.

Today this API allows you to collate all pending txn and start while dmaengine driver can collate and submit as a batch to hardware and not waste time in getting irq and then setting next one. Sadly none of the drivers use this feature...

I actually like the split model, you can also prepare txn ahead of time and submit them when needed. If you don't like this, then please do as most implementation do today, call prepare and submit in series. Flexiblity in API is a better option IMO

As Russell pointed out, that ain't the case either. So we are yet to figure out benefits of having explicit issue_pending() after tx_submit().

Quoted 3 lines · Jassi Brar

As Russell pointed out, that ain't the case either. So we are yet to figure out benefits of having explicit issue_pending() after tx_submit().

callback ?

Quoted 6 lines · Vinod Koul

Quoted 3 lines · Jassi Brar

As Russell pointed out, that ain't the case either. So we are yet to figure out benefits of having explicit issue_pending() after tx_submit().

callback ?

The callback is set after prep() and before tx_submit(), but here we talk after tx_submit().

Hi Lars,

[snip]

Quoted 41 lines

Quoted 41 lines

Quoted 41 lines

drivers/dma/pl330.c +26 -0

+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate)
+		return ret;
+
+	used = txstate->used;
+
+	spin_lock_irqsave(&pch->lock, flags);
+	sar = readl(regs + SA(thrd->id));
+	dar = readl(regs + DA(thrd->id));
+
+	list_for_each_entry(desc, &pch->work_list, node) {
+		if (desc->status == BUSY) {
+			current_c = desc->txd.cookie;
+			if (first) {
+				first_c = desc->txd.cookie;
+				first = false;
+			}
+
+			if (first_c < current_c)
+				residue += desc->px.bytes;
+			else {
+				if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
+					residue += desc->px.bytes;
+					residue -= sar - desc->px.src_addr;
+				} else if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar))

{

drivers/dma/pl330.c +13 -0

+					residue += desc->px.bytes;
+					residue -= dar - desc->px.dst_addr;
+				}
+			}
+		} else if (desc->status == PREP)
+			residue += desc->px.bytes;
+
+		if (desc->txd.cookie == used)
+			break;
+	}
+	spin_unlock_irqrestore(&pch->lock, flags);
+	dma_set_residue(txstate, residue);
+	return ret;
   }

[snip]

Quoted 1 line

Quoted 1 line

Quoted 4 lines

Quoted 1 line

Any comment on this patch?

Well it doesn't break audio, but I don't think it has the correct haviour for all cases yet.

OK. Any way of testing other cases like scatter-gather and memcopy. I verified memcopy in dmatest but it seems not doing anything with residue bytes.

Quoted 5 lines

Again, the semantics are that it should return the progress of the transfer

for which the allocation function returned the cookie that is passe to this

May be my understanding is wrong. For clarification..In the snd_dmaengine_pcm_pointer it is subtracting the residue bytes from the total buffer bytes not from period bytes. So how it expects
the progress of the transfer of the passed cookie which just holds period bytes?

On 12/02/2014 06:38 AM, Padma Venkat wrote:

Well it doesn't break audio, but I don't think it has the correct haviour for all cases yet.

Again, the semantics are that it should return the progress of the transfer for which the allocation function returned the cookie that is passe to this function. You have to consider that there might be multiple different descriptors submitted and in the work list, not just the one we want to know the status of. The big problem with the pl330 driver is that the current structure of the driver makes it not so easy to implement the residue reporting correctly.