* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-11-26 9:44 ` Padmavathi Venna
0 siblings, 0 replies; 34+ messages in thread
From: Padmavathi Venna @ 2014-11-26 9:44 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc, dmaengine
Cc: vinod.koul, broonie, lars, Dylan Reid
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 <dgreid@chromium.org> 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 <dgreid@chromium.org>
Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
This patch has been tested for audio playback on exynos5420 peach-pit.
drivers/dma/pl330.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b7493d2..db880ae 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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)
@@ -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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-11-26 9:44 ` Padmavathi Venna
0 siblings, 0 replies; 34+ messages in thread
From: Padmavathi Venna @ 2014-11-26 9:44 UTC (permalink / raw)
To: linux-arm-kernel
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 <dgreid@chromium.org> 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 <dgreid@chromium.org>
Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
This patch has been tested for audio playback on exynos5420 peach-pit.
drivers/dma/pl330.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b7493d2..db880ae 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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)
@@ -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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-11-26 9:44 ` Padmavathi Venna
@ 2014-12-02 5:38 ` Padma Venkat
-1 siblings, 0 replies; 34+ messages in thread
From: Padma Venkat @ 2014-12-02 5:38 UTC (permalink / raw)
To: Padmavathi Venna
Cc: linux-arm-kernel, linux-samsung-soc, dmaengine, vinod.koul,
broonie, lars, Dylan Reid
Hi Vinod/Lars,
On 11/26/14, Padmavathi Venna <padma.v@samsung.com> wrote:
> 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 <dgreid@chromium.org> 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 <dgreid@chromium.org>
> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> ---
>
> This patch has been tested for audio playback on exynos5420 peach-pit.
>
> drivers/dma/pl330.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index b7493d2..db880ae 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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)
> @@ -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;
> }
Any comment on this patch?
Thanks
Padma
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-02 5:38 ` Padma Venkat
0 siblings, 0 replies; 34+ messages in thread
From: Padma Venkat @ 2014-12-02 5:38 UTC (permalink / raw)
To: linux-arm-kernel
Hi Vinod/Lars,
On 11/26/14, Padmavathi Venna <padma.v@samsung.com> wrote:
> 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 <dgreid@chromium.org> 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 <dgreid@chromium.org>
> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> ---
>
> This patch has been tested for audio playback on exynos5420 peach-pit.
>
> drivers/dma/pl330.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index b7493d2..db880ae 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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)
> @@ -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;
> }
Any comment on this patch?
Thanks
Padma
> --
> 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
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-02 5:38 ` Padma Venkat
@ 2014-12-02 17:25 ` Lars-Peter Clausen
-1 siblings, 0 replies; 34+ messages in thread
From: Lars-Peter Clausen @ 2014-12-02 17:25 UTC (permalink / raw)
To: Padma Venkat, Padmavathi Venna
Cc: linux-samsung-soc, vinod.koul, broonie, dmaengine, Dylan Reid,
linux-arm-kernel
On 12/02/2014 06:38 AM, Padma Venkat wrote:
> Hi Vinod/Lars,
>
> On 11/26/14, Padmavathi Venna <padma.v@samsung.com> wrote:
>> 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 <dgreid@chromium.org> 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 <dgreid@chromium.org>
>> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
>> ---
>>
>> This patch has been tested for audio playback on exynos5420 peach-pit.
>>
>> drivers/dma/pl330.c | 67
>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index b7493d2..db880ae 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -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)
>> @@ -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;
>> }
>
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-02 17:25 ` Lars-Peter Clausen
0 siblings, 0 replies; 34+ messages in thread
From: Lars-Peter Clausen @ 2014-12-02 17:25 UTC (permalink / raw)
To: linux-arm-kernel
On 12/02/2014 06:38 AM, Padma Venkat wrote:
> Hi Vinod/Lars,
>
> On 11/26/14, Padmavathi Venna <padma.v@samsung.com> wrote:
>> 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 <dgreid@chromium.org> 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 <dgreid@chromium.org>
>> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
>> ---
>>
>> This patch has been tested for audio playback on exynos5420 peach-pit.
>>
>> drivers/dma/pl330.c | 67
>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index b7493d2..db880ae 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -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)
>> @@ -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;
>> }
>
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-02 17:25 ` Lars-Peter Clausen
@ 2014-12-03 4:47 ` Padma Venkat
-1 siblings, 0 replies; 34+ messages in thread
From: Padma Venkat @ 2014-12-03 4:47 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Padmavathi Venna, linux-arm-kernel, linux-samsung-soc, dmaengine,
vinod.koul, broonie, Dylan Reid
Hi Lars,
[snip]
>>> +
>>> + 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;
>>> }
[snip]
>>
>> 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.
>
> 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?
>
> 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.
>
> 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
Thanks
Padma
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-03 4:47 ` Padma Venkat
0 siblings, 0 replies; 34+ messages in thread
From: Padma Venkat @ 2014-12-03 4:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Lars,
[snip]
>>> +
>>> + 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;
>>> }
[snip]
>>
>> 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.
>
> 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?
>
> 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.
>
> 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
Thanks
Padma
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-03 4:47 ` Padma Venkat
@ 2014-12-03 7:51 ` Jassi Brar
-1 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-03 7:51 UTC (permalink / raw)
To: Padma Venkat
Cc: Lars-Peter Clausen, linux-samsung-soc, Padmavathi Venna, Koul,
Vinod, Mark Brown, dmaengine, Dylan Reid, linux-arm-kernel
On 3 December 2014 at 10:17, Padma Venkat <padma.kvr@gmail.com> wrote:
> Hi Lars,
>
> [snip]
>>>> +
>>>> + 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;
>>>> }
> [snip]
>>>
>>> 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.
>
>>
>> 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?
>
>>
>> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-03 7:51 ` Jassi Brar
0 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-03 7:51 UTC (permalink / raw)
To: linux-arm-kernel
On 3 December 2014 at 10:17, Padma Venkat <padma.kvr@gmail.com> wrote:
> Hi Lars,
>
> [snip]
>>>> +
>>>> + 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;
>>>> }
> [snip]
>>>
>>> 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.
>
>>
>> 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?
>
>>
>> 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 at 04abf5daf7d makes the driver slightly more
complicated and the reason behind confusion such as in this thread.
-jassi
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-03 4:47 ` Padma Venkat
@ 2014-12-04 20:15 ` Lars-Peter Clausen
-1 siblings, 0 replies; 34+ messages in thread
From: Lars-Peter Clausen @ 2014-12-04 20:15 UTC (permalink / raw)
To: Padma Venkat
Cc: Padmavathi Venna, linux-arm-kernel, linux-samsung-soc, dmaengine,
vinod.koul, broonie, Dylan Reid
On 12/03/2014 05:47 AM, Padma Venkat wrote:
> Hi Lars,
>
> [snip]
>>>> +
>>>> + 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;
>>>> }
> [snip]
>>>
>>> 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.
E.g. I think your current patch fails if more than one transfer has been
submitted. In that case you'll count the bytes for all of them rather than
just the one requested.
>
>>
>> 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?
The issue that makes implementing this correctly for the pl330 driver
complicated is that the driver allocates one cookie per segment/period, but
the external API works with one cookie per transfer. All those cookies that
are assigned to the segments/periods that are not the first in a transfer
are not externally visible.
dmaengine_prep_slave_sg() and friends create a transfer and return
descriptor for this transfer with a single cookie. If that cookie is passed
to dmaengine_tx_status() the function is supposed to report the progress on
that one transfer that was previously allocated and returned that cookie
number. residue is the total number of bytes that are still left to to be
processed for that transfer.
I've started reworking the pl330 driver to only have a single cookie per
transfer, instead of one per period/segment. This makes implementing the
residue reporting a lot easier. I never finished that work though, but I can
try to see if I can revive it next week.
- Lars
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-04 20:15 ` Lars-Peter Clausen
0 siblings, 0 replies; 34+ messages in thread
From: Lars-Peter Clausen @ 2014-12-04 20:15 UTC (permalink / raw)
To: linux-arm-kernel
On 12/03/2014 05:47 AM, Padma Venkat wrote:
> Hi Lars,
>
> [snip]
>>>> +
>>>> + 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;
>>>> }
> [snip]
>>>
>>> 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.
E.g. I think your current patch fails if more than one transfer has been
submitted. In that case you'll count the bytes for all of them rather than
just the one requested.
>
>>
>> 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?
The issue that makes implementing this correctly for the pl330 driver
complicated is that the driver allocates one cookie per segment/period, but
the external API works with one cookie per transfer. All those cookies that
are assigned to the segments/periods that are not the first in a transfer
are not externally visible.
dmaengine_prep_slave_sg() and friends create a transfer and return
descriptor for this transfer with a single cookie. If that cookie is passed
to dmaengine_tx_status() the function is supposed to report the progress on
that one transfer that was previously allocated and returned that cookie
number. residue is the total number of bytes that are still left to to be
processed for that transfer.
I've started reworking the pl330 driver to only have a single cookie per
transfer, instead of one per period/segment. This makes implementing the
residue reporting a lot easier. I never finished that work though, but I can
try to see if I can revive it next week.
- Lars
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-02 17:25 ` Lars-Peter Clausen
@ 2014-12-05 15:10 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-05 15:10 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Padma Venkat, Padmavathi Venna, linux-arm-kernel,
linux-samsung-soc, dmaengine, broonie, Dylan Reid
On Tue, Dec 02, 2014 at 06:25:49PM +0100, Lars-Peter Clausen wrote:
> 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.
well you can use the completed_cookie as hint. This was last trasaction
which was issues, so calculate remianing bytes for this and subsequent ones
resiude would only be size of transaction
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-05 15:10 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-05 15:10 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 02, 2014 at 06:25:49PM +0100, Lars-Peter Clausen wrote:
> 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.
well you can use the completed_cookie as hint. This was last trasaction
which was issues, so calculate remianing bytes for this and subsequent ones
resiude would only be size of transaction
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-03 7:51 ` Jassi Brar
@ 2014-12-05 15:15 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-05 15:15 UTC (permalink / raw)
To: Jassi Brar
Cc: Padma Venkat, Lars-Peter Clausen, linux-samsung-soc,
Padmavathi Venna, Mark Brown, dmaengine, Dylan Reid,
linux-arm-kernel
On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
> On 3 December 2014 at 10:17, Padma Venkat <padma.kvr@gmail.com> wrote:
> > Hi Lars,
> >
> > [snip]
> >>>> +
> >>>> + 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;
> >>>> }
> > [snip]
> >>>
> >>> 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.
> >
> >>
> >> 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?
> >
> >>
> >> 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
"to be" here can lead to different conculsion. I will reword this :)
@tx_submit: accept the descriptor and assign ordered cookie and mark the
descriptor pending. To be pushed on .issue_pending() call
--
~Vinod
>
> 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
--
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-05 15:15 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-05 15:15 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
> On 3 December 2014 at 10:17, Padma Venkat <padma.kvr@gmail.com> wrote:
> > Hi Lars,
> >
> > [snip]
> >>>> +
> >>>> + 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;
> >>>> }
> > [snip]
> >>>
> >>> 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.
> >
> >>
> >> 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?
> >
> >>
> >> 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
"to be" here can lead to different conculsion. I will reword this :)
@tx_submit: accept the descriptor and assign ordered cookie and mark the
descriptor pending. To be pushed on .issue_pending() call
--
~Vinod
>
> 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.
>
> -jassi
--
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-03 7:51 ` Jassi Brar
@ 2014-12-05 15:18 ` Russell King - ARM Linux
-1 siblings, 0 replies; 34+ messages in thread
From: Russell King - ARM Linux @ 2014-12-05 15:18 UTC (permalink / raw)
To: Jassi Brar
Cc: Padma Venkat, linux-samsung-soc, Lars-Peter Clausen,
Padmavathi Venna, Koul, Vinod, Mark Brown, dmaengine, Dylan Reid,
linux-arm-kernel
On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
> 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.
> " .... 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.
> 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.
> 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.
> At best the patch@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.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-05 15:18 ` Russell King - ARM Linux
0 siblings, 0 replies; 34+ messages in thread
From: Russell King - ARM Linux @ 2014-12-05 15:18 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
> 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.
> " .... 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.
> 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.
> 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.
> 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.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-05 15:18 ` Russell King - ARM Linux
@ 2014-12-06 7:01 ` Jassi Brar
-1 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-06 7:01 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Padma Venkat, linux-samsung-soc, Lars-Peter Clausen,
Padmavathi Venna, Koul, Vinod, Mark Brown, dmaengine, Dylan Reid,
linux-arm-kernel
On 5 December 2014 at 20:48, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
>> 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.
>
>> " .... 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.
>
The APIs (device_issue_pending, tx_submit etc) are same for Slave and Async.
async_tx_issue_pending_all() for Async and dma_async_issue_pending()
for Slave both boil down to device_issue_pending()
>> 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.
>
:) OK
>> 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.
>
>> At best the patch@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()?
Thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-06 7:01 ` Jassi Brar
0 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-06 7:01 UTC (permalink / raw)
To: linux-arm-kernel
On 5 December 2014 at 20:48, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
>> 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.
>
>> " .... 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.
>
The APIs (device_issue_pending, tx_submit etc) are same for Slave and Async.
async_tx_issue_pending_all() for Async and dma_async_issue_pending()
for Slave both boil down to device_issue_pending()
>> 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.
>
:) OK
>> 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.
>
>> 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()?
Thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-06 7:01 ` Jassi Brar
@ 2014-12-08 13:07 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-08 13:07 UTC (permalink / raw)
To: Jassi Brar
Cc: Russell King - ARM Linux, Padma Venkat, linux-samsung-soc,
Lars-Peter Clausen, Padmavathi Venna, Mark Brown, dmaengine,
Dylan Reid, linux-arm-kernel
On Sat, Dec 06, 2014 at 12:31:01PM +0530, Jassi Brar wrote:
> 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()?
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
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-08 13:07 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-08 13:07 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Dec 06, 2014 at 12:31:01PM +0530, Jassi Brar wrote:
> 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()?
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
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-08 13:07 ` Vinod Koul
@ 2014-12-08 14:23 ` Russell King - ARM Linux
-1 siblings, 0 replies; 34+ messages in thread
From: Russell King - ARM Linux @ 2014-12-08 14:23 UTC (permalink / raw)
To: Vinod Koul
Cc: Jassi Brar, Padma Venkat, linux-samsung-soc, Lars-Peter Clausen,
Padmavathi Venna, Mark Brown, dmaengine, Dylan Reid,
linux-arm-kernel
On Mon, Dec 08, 2014 at 06:37:27PM +0530, Vinod Koul wrote:
> 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.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-08 14:23 ` Russell King - ARM Linux
0 siblings, 0 replies; 34+ messages in thread
From: Russell King - ARM Linux @ 2014-12-08 14:23 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 08, 2014 at 06:37:27PM +0530, Vinod Koul wrote:
> 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.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-08 14:23 ` Russell King - ARM Linux
@ 2014-12-09 6:10 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-09 6:10 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jassi Brar, Padma Venkat, linux-samsung-soc, Lars-Peter Clausen,
Padmavathi Venna, Mark Brown, dmaengine, Dylan Reid,
linux-arm-kernel
On Mon, Dec 08, 2014 at 02:23:21PM +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 08, 2014 at 06:37:27PM +0530, Vinod Koul wrote:
> > 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.
Yes thats right.
Do you mind pointing to thread Dan replied, I would like to add these bits
and anything else missing to Documentation
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-09 6:10 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-09 6:10 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 08, 2014 at 02:23:21PM +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 08, 2014 at 06:37:27PM +0530, Vinod Koul wrote:
> > 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.
Yes thats right.
Do you mind pointing to thread Dan replied, I would like to add these bits
and anything else missing to Documentation
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-08 13:07 ` Vinod Koul
@ 2014-12-09 15:18 ` Jassi Brar
-1 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-09 15:18 UTC (permalink / raw)
To: Vinod Koul
Cc: Russell King - ARM Linux, Padma Venkat, linux-samsung-soc,
Lars-Peter Clausen, Padmavathi Venna, Mark Brown, dmaengine,
Dylan Reid, linux-arm-kernel
On 8 December 2014 at 18:37, Vinod Koul <vinod.koul@intel.com> wrote:
> On Sat, Dec 06, 2014 at 12:31:01PM +0530, Jassi Brar wrote:
>> 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()?
> You are thinking from an API point and not what can be done with this API.
>
"awkward situation" and "ritual suffering" above, are two practical
issues that we see.
> 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...
>
So we at least agree that the "feature" is unused so far.
And I doubt if it would ever make sense to batch transfers in slave
dma (unlike offloading on async-dma) because the user has to also
setup the master for each queued request, mostly if not always.
> 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().
-Jassi
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-09 15:18 ` Jassi Brar
0 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-09 15:18 UTC (permalink / raw)
To: linux-arm-kernel
On 8 December 2014 at 18:37, Vinod Koul <vinod.koul@intel.com> wrote:
> On Sat, Dec 06, 2014 at 12:31:01PM +0530, Jassi Brar wrote:
>> 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()?
> You are thinking from an API point and not what can be done with this API.
>
"awkward situation" and "ritual suffering" above, are two practical
issues that we see.
> 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...
>
So we at least agree that the "feature" is unused so far.
And I doubt if it would ever make sense to batch transfers in slave
dma (unlike offloading on async-dma) because the user has to also
setup the master for each queued request, mostly if not always.
> 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().
-Jassi
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-09 15:18 ` Jassi Brar
@ 2014-12-11 4:47 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-11 4:47 UTC (permalink / raw)
To: Jassi Brar
Cc: Russell King - ARM Linux, Padma Venkat, linux-samsung-soc,
Lars-Peter Clausen, Padmavathi Venna, Mark Brown, dmaengine,
Dylan Reid, linux-arm-kernel
On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote:
> 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 ?
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-11 4:47 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2014-12-11 4:47 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote:
> 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 ?
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-11 4:47 ` Vinod Koul
@ 2014-12-11 6:12 ` Jassi Brar
-1 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-11 6:12 UTC (permalink / raw)
To: Vinod Koul
Cc: Russell King - ARM Linux, Padma Venkat, linux-samsung-soc,
Lars-Peter Clausen, Padmavathi Venna, Mark Brown, dmaengine,
Dylan Reid, linux-arm-kernel
On 11 December 2014 at 10:17, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote:
>> 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().
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2014-12-11 6:12 ` Jassi Brar
0 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2014-12-11 6:12 UTC (permalink / raw)
To: linux-arm-kernel
On 11 December 2014 at 10:17, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote:
>> 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().
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
2014-12-11 6:12 ` Jassi Brar
@ 2015-03-12 8:47 ` Jassi Brar
-1 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2015-03-12 8:47 UTC (permalink / raw)
To: Vinod Koul
Cc: Russell King - ARM Linux, Padma Venkat, linux-samsung-soc,
Lars-Peter Clausen, Padmavathi Venna, Mark Brown, dmaengine,
Dylan Reid, linux-arm-kernel
Hi Vinod, Hi Russell,
On 11 December 2014 at 11:42, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 11 December 2014 at 10:17, Vinod Koul <vinod.koul@intel.com> wrote:
>> On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote:
>>> 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().
Perhaps the idea dates back to async-only days, when client drivers
would prepare and queue descriptors in the controller driver rather
than having to manage the dependency queues themselves (?).
Today ~95% clients are slave and I am yet to find one that really
can't work with submit and issue_pending tied together. Not to forget
the 100% of the controller drivers have to manage 'submitted' and
'active' queues -- only to have arguably negative side-effects.
If we agree that clubbing submit and issue_pending is the right thing
to do, I can start converting the ~90 client drivers. Please let me
know either way.
Cheers!
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] dmaengine: pl330: Set residue in tx_status callback
@ 2015-03-12 8:47 ` Jassi Brar
0 siblings, 0 replies; 34+ messages in thread
From: Jassi Brar @ 2015-03-12 8:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Vinod, Hi Russell,
On 11 December 2014 at 11:42, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 11 December 2014 at 10:17, Vinod Koul <vinod.koul@intel.com> wrote:
>> On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote:
>>> 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().
Perhaps the idea dates back to async-only days, when client drivers
would prepare and queue descriptors in the controller driver rather
than having to manage the dependency queues themselves (?).
Today ~95% clients are slave and I am yet to find one that really
can't work with submit and issue_pending tied together. Not to forget
the 100% of the controller drivers have to manage 'submitted' and
'active' queues -- only to have arguably negative side-effects.
If we agree that clubbing submit and issue_pending is the right thing
to do, I can start converting the ~90 client drivers. Please let me
know either way.
Cheers!
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-03-12 8:47 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-26 9:44 [PATCH] dmaengine: pl330: Set residue in tx_status callback Padmavathi Venna
2014-11-26 9:44 ` Padmavathi Venna
2014-12-02 5:38 ` Padma Venkat
2014-12-02 5:38 ` Padma Venkat
2014-12-02 17:25 ` Lars-Peter Clausen
2014-12-02 17:25 ` Lars-Peter Clausen
2014-12-03 4:47 ` Padma Venkat
2014-12-03 4:47 ` Padma Venkat
2014-12-03 7:51 ` Jassi Brar
2014-12-03 7:51 ` Jassi Brar
2014-12-05 15:15 ` Vinod Koul
2014-12-05 15:15 ` Vinod Koul
2014-12-05 15:18 ` Russell King - ARM Linux
2014-12-05 15:18 ` Russell King - ARM Linux
2014-12-06 7:01 ` Jassi Brar
2014-12-06 7:01 ` Jassi Brar
2014-12-08 13:07 ` Vinod Koul
2014-12-08 13:07 ` Vinod Koul
2014-12-08 14:23 ` Russell King - ARM Linux
2014-12-08 14:23 ` Russell King - ARM Linux
2014-12-09 6:10 ` Vinod Koul
2014-12-09 6:10 ` Vinod Koul
2014-12-09 15:18 ` Jassi Brar
2014-12-09 15:18 ` Jassi Brar
2014-12-11 4:47 ` Vinod Koul
2014-12-11 4:47 ` Vinod Koul
2014-12-11 6:12 ` Jassi Brar
2014-12-11 6:12 ` Jassi Brar
2015-03-12 8:47 ` Jassi Brar
2015-03-12 8:47 ` Jassi Brar
2014-12-04 20:15 ` Lars-Peter Clausen
2014-12-04 20:15 ` Lars-Peter Clausen
2014-12-05 15:10 ` Vinod Koul
2014-12-05 15:10 ` Vinod Koul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.