Re: [PATCH 2/2 v2] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, Nov 16, 2017 at 5:34 AM, Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>

Thanks for your patch!

> SYS/RT/Audio DMAC includes independent data buffers for reading
> and writing. Therefore, the read transfer counter and write transfer
> counter have different values.
> TCR indicates read counter, and TCRB indicates write counter.
> The relationship is like below.
>
>                 TCR       TCRB
>         [SOURCE] -> [DMAC] -> [SINK]
>
> In the MEM_TO_DEV direction, what really matters is how much data has
> been written to the device. If the DMA is interrupted between read and
> write, then, the data doesn't end up in the destination, so shouldn't
> be counted. TCRB is thus the register we should use in this cases.
>
> In the DEV_TO_MEM direction, the situation is more complex. Both the
> read and write side are important. What matters from a data consumer
> point of view is how much data has been written to memory.
> On the other hand, if the transfer is interrupted between read and
> write, we'll end up losing data. It can also be important to report.
>
> In the MEM_TO_MEM direction, what matters is of course how much data
> has been written to memory from data consumer point of view.
> Here, because read and write have independent data buffers, it will
> take a while for TCR and TCRB to become equal. Thus we should check
> TCRB in this case, too.
>
> Thus, all cases we should check TCRB instead of TCR.
>
> Without this patch, Sound Capture has noise after PluseAudio support

PulseAudio

> (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate")), because
> the recorder will use wrong residue counter which indicates transferred
> from sound device, but in reality the data was not yet put to memory
> and recorder will record it.
>
> However, because DMAC is buffering data until it can be transferable
> size, TCRB might not be updated.
> For example, if consumer doesn't know how much data can be receaved,

received

> it requests enough size to DMAC. But in reality, it might receave very

receive

> few data. In such case, DMAC just buffered it untile transferable size,

until

> and no TCRB updated.
>
> In such case, this buffered data will be transferred if CHCR::DE bit was
> cleared, and this is happen if rcar_dmac_chan_halt(). In other word, it
> happen when consumer called dmaengine_terminate_all().
>
> Because of this behavior, it need to flush buffered data when it returns
> "residue" (= dmaengine_tx_status()).
> Otherwise, consumer might calculate wrong things if it called
> dmaengine_tx_status() and dmaengine_terminate_all() consecutively.
>
> Tested-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@xxxxxxxxxxx>
> Tested-by: Ryo Kodama <ryo.kodama.vz@xxxxxxxxxxx>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>

My serial console is happy again (on Koelsch and Salvator-XS), so:
Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -761,6 +761,23 @@ static void rcar_dmac_chcr_de_barrier(struct rcar_dmac_chan *chan)
>         dev_err(chan->chan.device->dev, "CHCR DE check error\n");
>  }
>
> +static void rcar_dmac_sync_tcr(struct rcar_dmac_chan *chan)
> +{
> +       u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> +
> +       if (!(chcr & RCAR_DMACHCR_DE))
> +               return;
> +
> +       /* set DE=0 and flush remaining data */
> +       rcar_dmac_chan_write(chan, RCAR_DMACHCR, (chcr & ~RCAR_DMACHCR_DE));
> +
> +       /* make sure all remaining data was fulshed */

flushed

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds