lkml.org 
[lkml]   [2013]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] dw_dmac: return proper residue value
From
Date
On Tue, 2013-01-22 at 13:41 +0530, Viresh Kumar wrote: 
> On Mon, Jan 21, 2013 at 2:30 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Currently the driver returns full length of the active descriptor which is
> > wrong. We have to go throught the active descriptor and sum up the length of
> > unsent children in the chain along with the actual data in the DMA channel
> > registers.
> >
> > The cyclic case is not handled by this patch due to len field in the descriptor
> > structure is left untouched by the original code.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > drivers/dma/dw_dmac.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 92 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > index d1b9ba2..4325c68 100644
> > --- a/drivers/dma/dw_dmac.c
> > +++ b/drivers/dma/dw_dmac.c
> > @@ -941,6 +941,97 @@ err_desc_get:
> > return NULL;
> > }
> >
> > +/* --------------------- Calculate residue ---------------------------- */
> > +
> > +static inline size_t dwc_get_rest(struct dw_dma_chan *dwc, struct dw_desc *desc)
> > +{
> > + enum dma_transfer_direction direction = dwc->direction;
> > +
> > + if (direction == DMA_MEM_TO_DEV || direction == DMA_MEM_TO_MEM)
> > + return desc->len - (channel_readl(dwc, SAR) - desc->lli.sar);
> > + else if (direction == DMA_DEV_TO_MEM)
> > + return desc->len - (channel_readl(dwc, DAR) - desc->lli.dar);
> > +
> > + return 0;
> > +}
>
> I agree with Vinod on this routine.

I'm checking this now. But on first glance his proposal is quite better.

> > +static inline size_t dwc_get_residue_in_llp(struct dw_dma_chan *dwc,
> > + struct list_head *active,
> > + struct list_head *head)
> > +{
> > + size_t residue = dwc_get_rest(dwc, to_dw_desc(active));
> > +
> > + do {
> > + active = active->next;
> > + if (active == head)
> > + break;
> > + residue += to_dw_desc(active)->len;
> > + } while (true);
> > +
> > + return residue;
> > +}
> > +
> > +static size_t dwc_calc_residue_llp_hw(struct dw_dma_chan *dwc)
> > +{
> > + struct dw_desc *desc = dwc_first_active(dwc);
> > + struct list_head *head = &desc->tx_list, *active;
> > + dma_addr_t llp = channel_readl(dwc, LLP);
> > +
> > + /* The transfer didn't start yet */
> > + if (unlikely(desc->txd.phys == llp))
> > + return desc->len;
> > +
> > + /* First descriptor is active */
> > + if (desc->lli.llp == llp)
> > + return dwc_get_rest(dwc, desc);
> > +
> > + /* Somewhere in the middle */
> > + list_for_each(active, head)
> > + if (to_dw_desc(active)->lli.llp == llp)
> > + return dwc_get_residue_in_llp(dwc, active, head);
> > +
> > + return 0;
> > +}
> > +
> > +static size_t dwc_calc_residue_llp_sw(struct dw_dma_chan *dwc)
> > +{
> > + struct dw_desc *desc = dwc_first_active(dwc);
> > + struct list_head *head = dwc->tx_list, *active;
> > +
> > + active = dwc->tx_node_active;
> > +
> > + /* First node is active */
> > + if (active == head->next)
> > + return dwc_get_rest(dwc, desc);
> > +
> > + /* Last node is active */
> > + if (active == head)
> > + return dwc_get_rest(dwc, to_dw_desc(active->prev));
> > +
> > + /* Somewhere in the middle */
> > + return dwc_get_residue_in_llp(dwc, active, head);
> > +}
> > +
> > +static size_t dwc_get_residue(struct dw_dma_chan *dwc)
>
> return u32, that's the type of residue field.

Hmm... I didn't remember where I got size_t for that, but you are right.
I'll fix that across the code.

>
> > +{
> > + unsigned long flags;
> > + size_t residue = 0;
>
> ditto.
>
> > +
> > + spin_lock_irqsave(&dwc->lock, flags);
> > + if (list_empty(&dwc->active_list)) {
>
> can this every happen?

In the same way as in dwc_scan_descriptors().
For example when transfer is done.

> > + spin_unlock_irqrestore(&dwc->lock, flags);
> > + return 0;
> > + }
> > +
> > + if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags))
> > + residue = dwc_calc_residue_llp_sw(dwc);
> > + else if (!test_bit(DW_DMA_IS_CYCLIC, &dwc->flags))
> > + residue = dwc_calc_residue_llp_hw(dwc);
>
> Hmm. you return zero for cyclic transfers, shouldn't that be the total
> length instead?

Total length is zero as I mentioned in commit message. The cyclic API
should be fixed separately if someone wants to have such functionality
of it.

> Over that, cyclic case can be handled before taking any locks.

Does the user is responsible to follow specific workflow for cyclic API?
However, it isn't related to current topic. I will move it out of
spinlock.

> > +
> > + spin_unlock_irqrestore(&dwc->lock, flags);
> > + return residue;
> > +}
> > +
> > /*
> > * Fix sconfig's burst size according to dw_dmac. We need to convert them as:
> > * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3.
> > @@ -1062,7 +1153,7 @@ dwc_tx_status(struct dma_chan *chan,
> > }
> >
> > if (ret != DMA_SUCCESS)
> > - dma_set_residue(txstate, dwc_first_active(dwc)->len);
> > + dma_set_residue(txstate, dwc_get_residue(dwc));
>
> Hmm.. After thinking a bit more on this, i am not sure if what u have done is
> the best way of doing it :(

At least it works for us :-)

>
> Just before above line got executed, we called dwc_scan_descriptors(), which
> also scans through the llis to see where we are.. What about updating
> first->residue
> there, so that we don't traverse it twice?

The dwc_scan_descriptors traverses the chain and returns immediately if
it found a match. But to calculate residue we have to traverse from that
point to the end of the chain along with current DMA register check.

> >
> > if (dwc->paused)
> > return DMA_PAUSED;


--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy


\
 
 \ /
  Last update: 2013-01-22 11:02    [W:0.116 / U:1.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site