Re: [PATCH 2/2] dw_dmac: return proper residue value

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Vinod Koul]

On Mon, Jan 21, 2013 at 11:00:44AM +0200, Andy Shevchenko 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
why do you mean by children here?
> 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;
hmmm, why not use BLOCK_TS value. That way you dont need to look at direction
and along with burst can easily calculate residue...

--
~Vinod

[PATCH 1/2] dw_dmac: fill len field of the descriptor

[Andy Shevchenko]

It will be useful to have the length of the transfer in the descriptor. The
cyclic transfer functions remained untouched.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 3ff9ef7..d1b9ba2 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -754,6 +754,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 			first = desc;
 		} else {
 			prev->lli.llp = desc->txd.phys;
+			desc->len = xfer_count << src_width;
 			list_add_tail(&desc->desc_node,
 					&first->tx_list);
 		}
@@ -852,6 +853,7 @@ slave_sg_todev_fill_desc:
 				first = desc;
 			} else {
 				prev->lli.llp = desc->txd.phys;
+				desc->len = dlen;
 				list_add_tail(&desc->desc_node,
 						&first->tx_list);
 			}
@@ -910,6 +912,7 @@ slave_sg_fromdev_fill_desc:
 				first = desc;
 			} else {
 				prev->lli.llp = desc->txd.phys;
+				desc->len = dlen;
 				list_add_tail(&desc->desc_node,
 						&first->tx_list);
 			}
-- 
1.7.10.4

[PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

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;
+}
+
+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)
+{
+	unsigned long flags;
+	size_t residue = 0;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	if (list_empty(&dwc->active_list)) {
+		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);
+
+	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));
 
 	if (dwc->paused)
 		return DMA_PAUSED;
-- 
1.7.10.4

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

On Mon, 2013-01-21 at 00:49 -0800, Vinod Koul wrote: 
> On Mon, Jan 21, 2013 at 11:00:44AM +0200, Andy Shevchenko 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
> why do you mean by children here?

The list nodes chained under (active) &desc->tx_list.

> > 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;
> hmmm, why not use BLOCK_TS value. That way you dont need to look at direction
> and along with burst can easily calculate residue...

Do you mean to read CTL hi/lo and do

desc->len - ctlhi.block_ts * ctllo.src_tr_width?

I think it could be not precise when memory-to-peripheral transfer is
going on. In that case you probably will have src_tr_width like 32 bits,
meanwhile peripheral may receive only byte stream.

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

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Vinod Koul]

On Mon, Jan 21, 2013 at 11:45:51AM +0200, Andy Shevchenko wrote:
> > > +	return 0;
> > hmmm, why not use BLOCK_TS value. That way you dont need to look at direction
> > and along with burst can easily calculate residue...
> 
> Do you mean to read CTL hi/lo and do
> 
> desc->len - ctlhi.block_ts * ctllo.src_tr_width?
> 
Yes
> I think it could be not precise when memory-to-peripheral transfer is
> going on. In that case you probably will have src_tr_width like 32 bits,
> meanwhile peripheral may receive only byte stream.
Nope that is not the case.
SAR/DAR is always incremented in src/dstn_tr_width granularity. For example if
you are using MEM to DMA, then SAR will always increment in case of x86 in 4byte
granularity as we will read bursts not singles.

Also if check the spec, it says "Once the transfer starts, the read-back value is the
total number of data items already read from the source peripheral, regardless of
what is the flow controller"

So basically you get what is read from buffer in case of MEM->PER and get what
is read from FIFO in case of PER->MEM which IMO gives you better or equal results
than your calulation.

--
~Vinod

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

On Mon, 2013-01-21 at 06:22 -0800, Vinod Koul wrote: 
> On Mon, Jan 21, 2013 at 11:45:51AM +0200, Andy Shevchenko wrote:
> > > > +	return 0;
> > > hmmm, why not use BLOCK_TS value. That way you dont need to look at direction
> > > and along with burst can easily calculate residue...
> > 
> > Do you mean to read CTL hi/lo and do
> > 
> > desc->len - ctlhi.block_ts * ctllo.src_tr_width?
> > 
> Yes
> > I think it could be not precise when memory-to-peripheral transfer is
> > going on. In that case you probably will have src_tr_width like 32 bits,
> > meanwhile peripheral may receive only byte stream.
> Nope that is not the case.
> SAR/DAR is always incremented in src/dstn_tr_width granularity. For example if
> you are using MEM to DMA, then SAR will always increment in case of x86 in 4byte
> granularity as we will read bursts not singles.
> 
> Also if check the spec, it says "Once the transfer starts, the read-back value is the
> total number of data items already read from the source peripheral, regardless of
> what is the flow controller"
> 
> So basically you get what is read from buffer in case of MEM->PER and get what
> is read from FIFO in case of PER->MEM which IMO gives you better or equal results
> than your calulation.

I will try this. Indeed I don't like usage of direction as well, and
your solution seems much clear in that sense.

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

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Viresh Kumar]

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.

> +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.

> +{
> +       unsigned long flags;
> +       size_t residue = 0;

ditto.

> +
> +       spin_lock_irqsave(&dwc->lock, flags);
> +       if (list_empty(&dwc->active_list)) {

can this every happen?

> +               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?

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

> +
> +       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 :(

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?

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

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

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

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Viresh Kumar]

On 22 January 2013 14:52, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2013-01-22 at 13:41 +0530, Viresh Kumar wrote:
>> > +       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.

My point was, because scan_descriptor() is just called, if the transfer
is over by now, we must have returned back, else we reach this place
and so this may never be true.

>> Over that, cyclic case can be handled before taking any locks.
>
> Does the user is responsible to follow specific workflow for cyclic API?

not sure.

>> 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.

Hey, you can start subtracting individual lengths of descriptors from
total length
in scan descriptors() and that should give you what you want.

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

On Tue, 2013-01-22 at 15:10 +0530, Viresh Kumar wrote: 
> On 22 January 2013 14:52, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, 2013-01-22 at 13:41 +0530, Viresh Kumar wrote:
> >> > +       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.
> 
> My point was, because scan_descriptor() is just called, if the transfer
> is over by now, we must have returned back, else we reach this place
> and so this may never be true.

Ah, you mean the dma_cookie_status returns DMA_SUCCESS in case
active_list is empty, don't you?

> >> Over that, cyclic case can be handled before taking any locks.
> >
> > Does the user is responsible to follow specific workflow for cyclic API?
> 
> not sure.

I have no test cases against cyclic API, so I may assume that it doesn't
require locking for that bit.

> >> 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.
> 
> Hey, you can start subtracting individual lengths of descriptors from
> total length
> in scan descriptors() and that should give you what you want.

Yesterday I spent in testing of different approaches. First of all, the
calculation of the sent amount is nicer when ctlhi/ctllo is used and it
works fine. But the approach, when we substract length of sent
descriptors, is losing data. Namely, we have no information how big is
the master descriptor (it has total length of the entire chain). So, I'm
thinking how to solve this one. Otherwise it seems simpler than mine
first idea.

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

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Viresh Kumar]

On 23 January 2013 14:42, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Ah, you mean the dma_cookie_status returns DMA_SUCCESS in case
> active_list is empty, don't you?

Bingo!!

> Yesterday I spent in testing of different approaches. First of all, the
> calculation of the sent amount is nicer when ctlhi/ctllo is used and it
> works fine. But the approach, when we substract length of sent
> descriptors, is losing data. Namely, we have no information how big is
> the master descriptor (it has total length of the entire chain). So, I'm
> thinking how to solve this one. Otherwise it seems simpler than mine
> first idea.

I was planning to give you another review comment which i thought i will
give later :)

It looks to be a bit wrong to have individual lengths in all descriptors leaving
the first one :)

This is the descriptor struct:

struct dw_desc {
	/* FIRST values the hardware uses */
	struct dw_lli			lli;

	/* THEN values for driver housekeeping */
	struct list_head		desc_node;
	struct list_head		tx_list;
	struct dma_async_tx_descriptor	txd;
	size_t				len;
};

Firstly i believe we can have a union of both list_heads as only one is used
at any time. Can you patch it down?

Second to solve your problem, you can add another field here: total_len.
I know it will consume 4 bytes per desc but i think that's the drawback we need
to accept.

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

On Wed, Jan 23, 2013 at 11:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 23 January 2013 14:42, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

>> Yesterday I spent in testing of different approaches. First of all, the
>> calculation of the sent amount is nicer when ctlhi/ctllo is used and it
>> works fine. But the approach, when we substract length of sent
>> descriptors, is losing data. Namely, we have no information how big is
>> the master descriptor (it has total length of the entire chain). So, I'm
>> thinking how to solve this one. Otherwise it seems simpler than mine
>> first idea.
>
> I was planning to give you another review comment which i thought i will
> give later :)
>
> It looks to be a bit wrong to have individual lengths in all descriptors leaving
> the first one :)
>
> This is the descriptor struct:
>
> struct dw_desc {
>         /* FIRST values the hardware uses */
>         struct dw_lli                   lli;
>
>         /* THEN values for driver housekeeping */
>         struct list_head                desc_node;
>         struct list_head                tx_list;
>         struct dma_async_tx_descriptor  txd;
>         size_t                          len;
> };
>
> Firstly i believe we can have a union of both list_heads as only one is used
> at any time. Can you patch it down?

Okay, separately and later to avoid additional testing. Will it work for you?

>
> Second to solve your problem, you can add another field here: total_len.
> I know it will consume 4 bytes per desc but i think that's the drawback we need
> to accept.

I found already better solution I guess.
I introduced two functions to get amount of sent bytes like

u32 calc_sent(u32 ctlhi, u32 ctllo)
{
 return f1(ctlhi) * f2(ctllo);
}

u32 get_sent(dwc)
{
 return calc_sent(read(CTL_HI), read(CTL_LO));
}

And usage like

/* initial residue */
desc->len - calc_sent(desc->lli.ctlhi, desc->lli.ctllo).

What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Viresh Kumar]

On 23 January 2013 15:06, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Jan 23, 2013 at 11:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

>> Firstly i believe we can have a union of both list_heads as only one is used
>> at any time. Can you patch it down?
>
> Okay, separately and later to avoid additional testing. Will it work for you?

Yes.

>> Second to solve your problem, you can add another field here: total_len.
>> I know it will consume 4 bytes per desc but i think that's the drawback we need
>> to accept.
>
> I found already better solution I guess.

Wow!!

> I introduced two functions to get amount of sent bytes like
>
> u32 calc_sent(u32 ctlhi, u32 ctllo)
> {
>  return f1(ctlhi) * f2(ctllo);
> }
>
> u32 get_sent(dwc)
> {
>  return calc_sent(read(CTL_HI), read(CTL_LO));
> }
>
> And usage like
>
> /* initial residue */
> desc->len - calc_sent(desc->lli.ctlhi, desc->lli.ctllo).
>
> What do you think?

You plan to use it for
- the first descriptor in a list as others already have length embedded in them?
- And the descriptor currently getting used, by reading its ctlhi/lo
from registers
  instead of lli.*** ?

Looks good. but still keeping len of individual descriptors in all
descriptors leaving
the first one is not the code i would like to keep :)

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

On Wed, Jan 23, 2013 at 11:51 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 23 January 2013 15:06, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Wed, Jan 23, 2013 at 11:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

>>> Second to solve your problem, you can add another field here: total_len.
>>> I know it will consume 4 bytes per desc but i think that's the drawback we need
>>> to accept.
>>
>> I found already better solution I guess.
>
> Wow!!
>
>> I introduced two functions to get amount of sent bytes like
>>
>> u32 calc_sent(u32 ctlhi, u32 ctllo)
>> {
>>  return f1(ctlhi) * f2(ctllo);
>> }
>>
>> u32 get_sent(dwc)
>> {
>>  return calc_sent(read(CTL_HI), read(CTL_LO));
>> }
>>
>> And usage like
>>
>> /* initial residue */
>> desc->len - calc_sent(desc->lli.ctlhi, desc->lli.ctllo).
>>
>> What do you think?
>
> You plan to use it for
> - the first descriptor in a list as others already have length embedded in them?
> - And the descriptor currently getting used, by reading its ctlhi/lo
> from registers
>   instead of lli.*** ?

Roughly, yes.

> Looks good. but still keeping len of individual descriptors in all
> descriptors leaving
> the first one is not the code i would like to keep :)

What about to calc len every time from lli.* instead of keeping it in
the len field of the descriptor?
It's logically looks better, but requires more resources.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Viresh Kumar]

On 23 January 2013 15:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> What about to calc len every time from lli.* instead of keeping it in
> the len field of the descriptor?

That's even bad.. we spend time on something for no good reason. :)
I would still suggest you to add len and total_len in descriptor :)

Re: [PATCH 2/2] dw_dmac: return proper residue value

[Andy Shevchenko]

On Wed, 2013-01-23 at 15:54 +0530, Viresh Kumar wrote: 
> On 23 January 2013 15:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > What about to calc len every time from lli.* instead of keeping it in
> > the len field of the descriptor?
> 
> That's even bad.. we spend time on something for no good reason. :)
> I would still suggest you to add len and total_len in descriptor :)

Okay, let's stick to this one for now.
I will come with patch v2 soon.

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