Hi Dirk,
(revised Vinod's email address)
> From: Dirk Behme, Sent: Tuesday, January 15, 2019 9:44 PM
>
> On 30.06.2016 17:15, Niklas Söderlund wrote:
> > From: Muhammad Hamza Farooq <mfarooq@xxxxxxxxxxx>
> >
> > The hardware might have complete the transfer but the interrupt handler
> > might not have had a chance to run. If rcar_dmac_chan_get_residue()
> > which reads HW registers finds that there is no residue return
> > DMA_COMPLETE.
> >
> > Signed-off-by: Muhammad Hamza Farooq <mfarooq@xxxxxxxxxxx>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > [Niklas: add explanation in commit message]
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/dma/sh/rcar-dmac.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> > index dfb1792..791a064 100644
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
> > residue = rcar_dmac_chan_get_residue(rchan, cookie);
> > spin_unlock_irqrestore(&rchan->lock, flags);
> >
> > + /* if there's no residue, the cookie is complete */
> > + if (!residue)
> > + return DMA_COMPLETE;
> > +
> > dma_set_residue(txstate, residue);
> >
> > return status;
>
>
> We have some doubts about this change (mainline commit [1]). Not being a
> DMA expert, let me try to explain:
>
> We are configuring a cyclic, never ending DMA
> (dmaengine_prep_dma_cyclic()). For this cyclic DMA we continuously poll
> the residue (txstate->residue) via rcar_dmac_tx_status(). Having a
> cyclic, never ending DMA we think that residue == 0 is a valid value.
> However, with above change, a residue value of 0 is 'dropped' and never
> written via dma_set_residue() to txstate. So in case we continuously
> poll, this value is lost, resulting in wrong behavior of the caller.
According to the Documentation/driver-api/dmaengine/provider.rst [1],
device_tx_status should report the bytes left. So, we should fix
the current implementation as you said.
---
[1]
``device_tx_status``
- Should report the bytes left to go over on the given channel
<snip>
- In the case of a cyclic transfer, it should only take into
account the current period.
---
> In our case with cyclic, never ending DMA, changing this to
>
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1388,7 +1388,7 @@ static enum dma_status rcar_dmac_tx_status(struct
> dma_chan *chan,
>
> /* if there's no residue, the cookie is complete */
> if (!residue)
> - return DMA_COMPLETE;
> + status = DMA_COMPLETE;
>
> dma_set_residue(txstate, residue);
>
> seems to help.
So, at least, we should apply this code above.
> If we ignore the still wrong DMA_COMPLETE status (which
> in case of cyclic DMA doesn't make any sense?) the caller get the
> correct txstate->residue values (not dropping the 0).
>
> So maybe we need anything like
>
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1387,7 +1387,7 @@ static enum dma_status rcar_dmac_tx_status(struct
> dma_chan *chan,
> spin_unlock_irqrestore(&rchan->lock, flags);
>
> /* if there's no residue, the cookie is complete */
> - if (!residue)
> + if (!residue && !chan->desc.running->cyclic)
> return DMA_COMPLETE;
>
> dma_set_residue(txstate, residue);
>
> ?
I could not find whether a cyclic transfer should not return DMA_COMPLETE on
Documentations/. I only found the following in the include/linux/dmaengine.h:
/**
* enum dma_status - DMA transaction status
* @DMA_COMPLETE: transaction completed
Best regards,
Yoshihiro Shimoda
> Opinions?
>
> Best regards
>
> Dirk
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/sh/rcar-dmac.c?h=v5.0-rc2&id=3
> 544d2878817bd139dda238cdd86a15e1c03d037
