Re: [PATCH v1] dmaengine: tegra-apb: Support per-burst residue granularity

[Ben Dooks]

On 19/06/2019 11:13, Jon Hunter wrote:
> On 19/06/2019 11:08, Ben Dooks wrote:
> > On 19/06/2019 11:04, Jon Hunter wrote:
> > > On 19/06/2019 00:27, Dmitry Osipenko wrote:
> > > > 19.06.2019 1:22, Ben Dooks ÐÐÑÐÑ:
> > > > > On 13/06/2019 22:08, Dmitry Osipenko wrote:
> > > > > > Tegra's APB DMA engine updates words counter after each transferred
> > > > > > burst
> > > > > > of data, hence it can report transfer's residual with more fidelity
> > > > > > which
> > > > > > may be required in cases like audio playback. In particular this fixes
> > > > > > audio stuttering during playback in a chromiuim web browser. The
> > > > > > patch is
> > > > > > based on the original work that was made by Ben Dooks [1]. It was
> > > > > > tested
> > > > > > on Tegra20 and Tegra30 devices.
> > > > > >
> > > > > > [1]
> > > > > > https://lore.kernel.org/lkml/20190424162348.23692-1-ben.dooks@xxxxxxxxxxxxxxx/
> > > > > >
> > > > > >
> > > > > > Inspired-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> > > > > > ---
> > > > > >  ÂÂ drivers/dma/tegra20-apb-dma.c | 35
> > > > > > ++++++++++++++++++++++++++++-------
> > > > > >  ÂÂ 1 file changed, 28 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/dma/tegra20-apb-dma.c
> > > > > > b/drivers/dma/tegra20-apb-dma.c
> > > > > > index 79e9593815f1..c5af8f703548 100644
> > > > > > --- a/drivers/dma/tegra20-apb-dma.c
> > > > > > +++ b/drivers/dma/tegra20-apb-dma.c
> > > > > > @@ -797,12 +797,36 @@ static int tegra_dma_terminate_all(struct
> > > > > > dma_chan *dc)
> > > > > >  ÂÂÂÂÂÂ return 0;
> > > > > >  ÂÂ }
> > > > > >  ÂÂ +static unsigned int tegra_dma_update_residual(struct
> > > > > > tegra_dma_channel *tdc,
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct tegra_dma_sg_req *sg_req,
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct tegra_dma_desc *dma_desc,
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int residual)
> > > > > > +{
> > > > > > +ÂÂÂ unsigned long status, wcount = 0;
> > > > > > +
> > > > > > +ÂÂÂ if (!list_is_first(&sg_req->node, &tdc->pending_sg_req))
> > > > > > +ÂÂÂÂÂÂÂ return residual;
> > > > > > +
> > > > > > +ÂÂÂ if (tdc->tdma->chip_data->support_separate_wcount_reg)
> > > > > > +ÂÂÂÂÂÂÂ wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
> > > > > > +
> > > > > > +ÂÂÂ status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> > > > > > +
> > > > > > +ÂÂÂ if (!tdc->tdma->chip_data->support_separate_wcount_reg)
> > > > > > +ÂÂÂÂÂÂÂ wcount = status;
> > > > > > +
> > > > > > +ÂÂÂ if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
> > > > > > +ÂÂÂÂÂÂÂ return residual - sg_req->req_len;
> > > > > > +
> > > > > > +ÂÂÂ return residual - get_current_xferred_count(tdc, sg_req, wcount);
> > > > > > +}
> > > > >
> > > > > I am unfortunately nowhere near my notes, so can't completely
> > > > > review this. I think the complexity of my patch series is due
> > > > > to an issue with the count being updated before the EOC IRQ
> > > > > is actually flagged (and most definetly before it gets to the
> > > > > CPU IRQ handler).
> > > > >
> > > > > The test system I was using, which i've not really got any
> > > > > access to at the moment would show these internal inconsistent
> > > > > states every few hours, however it was moving 48kHz 8ch 16bit
> > > > > TDM data.
> > > > >
> > > > > Thanks for looking into this, I am not sure if I am going to
> > > > > get any time to look into this within the next couple of
> > > > > months.
> > > >
> > > > I'll try to add some debug checks to try to catch the case where
> > > > count is updated before EOC
> > > > is set. Thank you very much for the clarification of the problem. So
> > > > far I haven't spotted
> > > > anything going wrong.
> > > >
> > > > Jon / Laxman, are you aware about the possibility to get such
> > > > inconsistency of words count
> > > > vs EOC? Assuming the cyclic transfer mode.
> > >
> > > I can't say that I am. However, for the case of cyclic transfer, given
> > > that the next transfer is always programmed into the registers before
> > > the last one completes, I could see that by the time the interrupt is
> > > serviced that the DMA has moved on to the next transfer (which I assume
> > > would reset the count).
> > >
> > > Interestingly, our downstream kernel implemented a change to avoid the
> > > count appearing to move backwards. I am curious if this also works,
> > > which would be a lot simpler that what Ben has implemented and may
> > > mitigate that race condition that Ben is describing.
> >
> > That might be the same thing we saw. IIRC it looks like the DMA has
> > moved on, but the count gets re-set before the EOC? I can't see that
> > git site so can't comment.
>
> That's odd, that should be a public site AFAIK. Does the following not
> work ...
>
> https://nv-tegra.nvidia.com/gitweb/
>
> Jon

Getting ssl errors which i think is causing a firewall issue here.

I can try again when back at the hotel instead of client site.



--
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html