[PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Eric Anholt]

The tx_status hook is supposed to be safe to call from interrupt
context, but it wouldn't ever return completion for the last transfer,
meaning you couldn't poll for DMA completion with interrupts masked.

This fixes IRQ handling for bcm2835's DSI1, which requires using the
DMA engine to write its registers due to a bug in the AXI bridge.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 6149b27c33ad..320461c578e3 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
 	struct virt_dma_desc *vd;
 	enum dma_status ret;
 	unsigned long flags;
+	u32 residue;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
-	if (ret == DMA_COMPLETE || !txstate)
+	if (ret == DMA_COMPLETE)
 		return ret;
 
 	spin_lock_irqsave(&c->vc.lock, flags);
 	vd = vchan_find_desc(&c->vc, cookie);
 	if (vd) {
-		txstate->residue =
-			bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx));
+		residue = bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx));
 	} else if (c->desc && c->desc->vd.tx.cookie == cookie) {
 		struct bcm2835_desc *d = c->desc;
 		dma_addr_t pos;
@@ -591,11 +591,25 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
 		else
 			pos = 0;
 
-		txstate->residue = bcm2835_dma_desc_size_pos(d, pos);
+		residue = bcm2835_dma_desc_size_pos(d, pos);
+
+		/*
+		 * If our non-cyclic transfer is done, then report
+		 * complete and trigger the next tx now.  This lets
+		 * the dmaengine API be used synchronously from an IRQ
+		 * handler.
+		 */
+		if (!d->cyclic && residue == 0) {
+			vchan_cookie_complete(&c->desc->vd);
+			bcm2835_dma_start_desc(c);
+			ret = dma_cookie_status(chan, cookie, txstate);
+		}
 	} else {
-		txstate->residue = 0;
+		residue = 0;
 	}
 
+	dma_set_residue(txstate, residue);
+
 	spin_unlock_irqrestore(&c->vc.lock, flags);
 
 	return ret;
-- 
2.8.0.rc3

Re: [PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Vinod Koul]

On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote:
> The tx_status hook is supposed to be safe to call from interrupt
> context, but it wouldn't ever return completion for the last transfer,
> meaning you couldn't poll for DMA completion with interrupts masked.

and why is that?

> This fixes IRQ handling for bcm2835's DSI1, which requires using the
> DMA engine to write its registers due to a bug in the AXI bridge.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 6149b27c33ad..320461c578e3 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
>  	struct virt_dma_desc *vd;
>  	enum dma_status ret;
>  	unsigned long flags;
> +	u32 residue;
>  
>  	ret = dma_cookie_status(chan, cookie, txstate);
> -	if (ret == DMA_COMPLETE || !txstate)
> +	if (ret == DMA_COMPLETE)

Why do you change this? txstate can be NULL, so no point calculating reside
for those cases

>  		return ret;
>  
>  	spin_lock_irqsave(&c->vc.lock, flags);
>  	vd = vchan_find_desc(&c->vc, cookie);
>  	if (vd) {
> -		txstate->residue =
> -			bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx));
> +		residue = bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx));
>  	} else if (c->desc && c->desc->vd.tx.cookie == cookie) {
>  		struct bcm2835_desc *d = c->desc;
>  		dma_addr_t pos;
> @@ -591,11 +591,25 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
>  		else
>  			pos = 0;
>  
> -		txstate->residue = bcm2835_dma_desc_size_pos(d, pos);
> +		residue = bcm2835_dma_desc_size_pos(d, pos);
> +
> +		/*
> +		 * If our non-cyclic transfer is done, then report
> +		 * complete and trigger the next tx now.  This lets
> +		 * the dmaengine API be used synchronously from an IRQ
> +		 * handler.
> +		 */
> +		if (!d->cyclic && residue == 0) {
> +			vchan_cookie_complete(&c->desc->vd);
> +			bcm2835_dma_start_desc(c);
> +			ret = dma_cookie_status(chan, cookie, txstate);
> +		}
>  	} else {
> -		txstate->residue = 0;
> +		residue = 0;
>  	}
>  
> +	dma_set_residue(txstate, residue);
> +
>  	spin_unlock_irqrestore(&c->vc.lock, flags);
>  
>  	return ret;
> -- 
> 2.8.0.rc3
> 

-- 
~Vinod

Re: [PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

Vinod Koul <vinod.koul@intel.com> writes:

> On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote:
>> The tx_status hook is supposed to be safe to call from interrupt
>> context, but it wouldn't ever return completion for the last transfer,
>> meaning you couldn't poll for DMA completion with interrupts masked.
>
> and why is that?

Maybe this was poorly worded.  How about:

The tx_status hook is supposed to be safe to call from interrupt
context.  However, the current transfer currently only gets marked
complete by the IRQ handler, so if interrupts were masked then polling
for completion would never finish.

>> This fixes IRQ handling for bcm2835's DSI1, which requires using the
>> DMA engine to write its registers due to a bug in the AXI bridge.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>  drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
>> index 6149b27c33ad..320461c578e3 100644
>> --- a/drivers/dma/bcm2835-dma.c
>> +++ b/drivers/dma/bcm2835-dma.c
>> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
>>  	struct virt_dma_desc *vd;
>>  	enum dma_status ret;
>>  	unsigned long flags;
>> +	u32 residue;
>>  
>>  	ret = dma_cookie_status(chan, cookie, txstate);
>> -	if (ret == DMA_COMPLETE || !txstate)
>> +	if (ret == DMA_COMPLETE)
>
> Why do you change this? txstate can be NULL, so no point calculating reside
> for those cases

The point was to go into the "Calculate where we're at in our current
DMA (if the current DMA is the one we're asking about status for)" path,
so that we could note when the DMA is complete even when there's no
txstate passed in.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Vinod Koul]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Mon, Jun 06, 2016 at 10:33:18AM -0700, Eric Anholt wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote:
> >> The tx_status hook is supposed to be safe to call from interrupt
> >> context, but it wouldn't ever return completion for the last transfer,
> >> meaning you couldn't poll for DMA completion with interrupts masked.
> >
> > and why is that?
> 
> Maybe this was poorly worded.  How about:
> 
> The tx_status hook is supposed to be safe to call from interrupt
> context.  However, the current transfer currently only gets marked
> complete by the IRQ handler, so if interrupts were masked then polling
> for completion would never finish.

Sound better :)

> 
> >> This fixes IRQ handling for bcm2835's DSI1, which requires using the
> >> DMA engine to write its registers due to a bug in the AXI bridge.
> >> 
> >> Signed-off-by: Eric Anholt <eric@anholt.net>
> >> ---
> >>  drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++-----
> >>  1 file changed, 19 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> >> index 6149b27c33ad..320461c578e3 100644
> >> --- a/drivers/dma/bcm2835-dma.c
> >> +++ b/drivers/dma/bcm2835-dma.c
> >> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
> >>  	struct virt_dma_desc *vd;
> >>  	enum dma_status ret;
> >>  	unsigned long flags;
> >> +	u32 residue;
> >>  
> >>  	ret = dma_cookie_status(chan, cookie, txstate);
> >> -	if (ret == DMA_COMPLETE || !txstate)
> >> +	if (ret == DMA_COMPLETE)
> >
> > Why do you change this? txstate can be NULL, so no point calculating reside
> > for those cases
> 
> The point was to go into the "Calculate where we're at in our current
> DMA (if the current DMA is the one we're asking about status for)" path,
> so that we could note when the DMA is complete even when there's no
> txstate passed in.

Can you explain what you mean by current DMA!

The claulation is always done for 'descriptor' represnted by the cookie. So
it doesnt not matter...!

Thanks
-- 
~Vinod

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

Vinod Koul <vinod.koul@intel.com> writes:

> On Mon, Jun 06, 2016 at 10:33:18AM -0700, Eric Anholt wrote:
>> Vinod Koul <vinod.koul@intel.com> writes:
>> 
>> > On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote:
>> >> The tx_status hook is supposed to be safe to call from interrupt
>> >> context, but it wouldn't ever return completion for the last transfer,
>> >> meaning you couldn't poll for DMA completion with interrupts masked.
>> >
>> > and why is that?
>> 
>> Maybe this was poorly worded.  How about:
>> 
>> The tx_status hook is supposed to be safe to call from interrupt
>> context.  However, the current transfer currently only gets marked
>> complete by the IRQ handler, so if interrupts were masked then polling
>> for completion would never finish.
>
> Sound better :)
>
>> 
>> >> This fixes IRQ handling for bcm2835's DSI1, which requires using the
>> >> DMA engine to write its registers due to a bug in the AXI bridge.
>> >> 
>> >> Signed-off-by: Eric Anholt <eric@anholt.net>
>> >> ---
>> >>  drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++-----
>> >>  1 file changed, 19 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
>> >> index 6149b27c33ad..320461c578e3 100644
>> >> --- a/drivers/dma/bcm2835-dma.c
>> >> +++ b/drivers/dma/bcm2835-dma.c
>> >> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,
>> >>  	struct virt_dma_desc *vd;
>> >>  	enum dma_status ret;
>> >>  	unsigned long flags;
>> >> +	u32 residue;
>> >>  
>> >>  	ret = dma_cookie_status(chan, cookie, txstate);
>> >> -	if (ret == DMA_COMPLETE || !txstate)
>> >> +	if (ret == DMA_COMPLETE)
>> >
>> > Why do you change this? txstate can be NULL, so no point calculating reside
>> > for those cases
>> 
>> The point was to go into the "Calculate where we're at in our current
>> DMA (if the current DMA is the one we're asking about status for)" path,
>> so that we could note when the DMA is complete even when there's no
>> txstate passed in.
>
> Can you explain what you mean by current DMA!
>
> The claulation is always done for 'descriptor' represnted by the cookie. So
> it doesnt not matter...!

By current I mean the current descriptor that has been submitted to the
hardware, in bcm2835_chan->desc.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Vinod Koul]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Mon, Jun 06, 2016 at 11:10:38PM -0700, Eric Anholt wrote:
> >> >> -	if (ret == DMA_COMPLETE || !txstate)
> >> >> +	if (ret == DMA_COMPLETE)
> >> >
> >> > Why do you change this? txstate can be NULL, so no point calculating reside
> >> > for those cases
> >> 
> >> The point was to go into the "Calculate where we're at in our current
> >> DMA (if the current DMA is the one we're asking about status for)" path,
> >> so that we could note when the DMA is complete even when there's no
> >> txstate passed in.
> >
> > Can you explain what you mean by current DMA!
> >
> > The claulation is always done for 'descriptor' represnted by the cookie. So
> > it doesnt not matter...!
> 
> By current I mean the current descriptor that has been submitted to the
> hardware, in bcm2835_chan->desc.

As I said, you calculate for the descriptor respresnted by cookie and
not the one getting processed!

-- 
~Vinod

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] dmaengine: bcm2835: Fix polling for completion of DMA with interrupts masked.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

Vinod Koul <vinod.koul@intel.com> writes:

> On Mon, Jun 06, 2016 at 11:10:38PM -0700, Eric Anholt wrote:
>> >> >> -	if (ret == DMA_COMPLETE || !txstate)
>> >> >> +	if (ret == DMA_COMPLETE)
>> >> >
>> >> > Why do you change this? txstate can be NULL, so no point calculating reside
>> >> > for those cases
>> >> 
>> >> The point was to go into the "Calculate where we're at in our current
>> >> DMA (if the current DMA is the one we're asking about status for)" path,
>> >> so that we could note when the DMA is complete even when there's no
>> >> txstate passed in.
>> >
>> > Can you explain what you mean by current DMA!
>> >
>> > The claulation is always done for 'descriptor' represnted by the cookie. So
>> > it doesnt not matter...!
>> 
>> By current I mean the current descriptor that has been submitted to the
>> hardware, in bcm2835_chan->desc.
>
> As I said, you calculate for the descriptor respresnted by cookie and
> not the one getting processed!

I believe I'm calculating the state for the descriptor being processed
only in the case where the cookie being asked about is for the
descriptor being processed.  I'm confused what your objection is, so I'm
going to annotate what I think is going on in the function so maybe you
can point to where I've got something wrong.

	spin_lock_irqsave(&c->vc.lock, flags);
	vd = vchan_find_desc(&c->vc, cookie);
	if (vd) {
		txstate->residue =
			bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx));

I believe this is the case for "the descriptor for the cookie being
asked about hasn't been pulled off the list and submitted to the
hardware yet"

	} else if (c->desc && c->desc->vd.tx.cookie == cookie) {
		struct bcm2835_desc *d = c->desc;
		dma_addr_t pos;

		if (d->dir == DMA_MEM_TO_DEV)
			pos = readl(c->chan_base + BCM2835_DMA_SOURCE_AD);
		else if (d->dir == DMA_DEV_TO_MEM)
			pos = readl(c->chan_base + BCM2835_DMA_DEST_AD);
		else
			pos = 0;

		txstate->residue = bcm2835_dma_desc_size_pos(d, pos);

Here the descriptor for the cookie is currently being processed by the
hardware.  It might also be done but hasn't had its done interrupt
handled yet, so this is the case where I want to effectively do the done
IRQ's work of updating the status.

	} else {
		txstate->residue = 0;

Here, it's not in the hardware and it's not queued to be submitted to
the hardware, so it must be done.

	}

What am I missing?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]