Re: [PATCH 1/3] dmaengine: pl330: Set residue in tx_status callback.

[Alban Browaeys <prahal@yahoo.com>]

Hi Dylan,

Le mardi 01 octobre 2013 à 21:33 -0700, Dylan Reid a écrit :
> On Thu, Sep 12, 2013 at 4:40 AM, Chanho Park <chanho61.park@samsung.com> wrote:
> > Hi Padmavathi,
> >
> >> -----Original Message-----
> >> From: linux-arm-kernel [mailto:linux-arm-kernel-
> >> bounces@lists.infradead.org] On Behalf Of Padmavathi Venna
> >> Sent: Wednesday, September 11, 2013 3:08 PM
> >> To: linux-samsung-soc@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org; padma.v@samsung.com; padma.kvr@gmail.com
> >> Cc: kgene.kim@samsung.com; arnd@arndb.de; sbkim73@samsung.com;
> >> vinod.koul@intel.com; broonie@kernel.org; dgreid@chromium.org;
> >> olofj@chromium.org
> >> Subject: [PATCH 1/3] dmaengine: pl330: Set residue in tx_status callback.
> >>
> >> From: Dylan Reid <dgreid@chromium.org>
> >>
> >> Fill txstate.residue with the amount of bytes remaining in the current
> >> transfer if the transfer is not complete.  This will be of particular use
> >> to i2s DMA transfers, providing more accurate hw_ptr values to ASoC.
> >>
> >> Signed-off-by: Dylan Reid <dgreid@chromium.org>
> >> Reviewed-by: Olof Johansson <olofj@chromium.org>
> >> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> >> ---
> >>  drivers/dma/pl330.c |   55
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 files changed, 54 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index
> >> 593827b..7ab9136 100644
> >> --- a/drivers/dma/pl330.c
> >> +++ b/drivers/dma/pl330.c
> >> @@ -2476,11 +2476,64 @@ static void pl330_free_chan_resources(struct
> >> dma_chan *chan)
> >>       spin_unlock_irqrestore(&pch->lock, flags);  }
> >>
> >> +static inline int
> >> +pl330_src_addr_in_desc(struct dma_pl330_desc *desc, unsigned int sar) {
> >> +     return ((desc->px.src_addr <= sar) &&
> >> +             (sar <= (desc->px.src_addr + desc->px.bytes))); }
> >> +
> >> +static inline int
> >> +pl330_dst_addr_in_desc(struct dma_pl330_desc *desc, unsigned int dar) {
> >> +     return ((desc->px.dst_addr <= dar) &&
> >> +             (dar <= (desc->px.dst_addr + desc->px.bytes))); }
> >> +
> >> +static unsigned int pl330_tx_residue(struct dma_chan *chan) {
> >> +     struct dma_pl330_chan *pch = to_pchan(chan);
> >> +     void __iomem *regs = pch->dmac->pif.base;
> >> +     struct pl330_thread *thrd = pch->pl330_chid;
> >> +     struct dma_pl330_desc *desc;
> >> +     unsigned int sar, dar;
> >> +     unsigned int residue = 0;
> >> +     unsigned long flags;
> >> +
> >> +     sar = readl(regs + SA(thrd->id));
> >> +     dar = readl(regs + DA(thrd->id));
> >> +
> >> +     spin_lock_irqsave(&pch->lock, flags);
> >> +
> >> +     /* Find the desc related to the current buffer. */
> >> +     list_for_each_entry(desc, &pch->work_list, node) {
> >> +             if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc,
> >> sar)) {
> >> +                     residue = desc->px.bytes - (sar -
> > desc->px.src_addr);
> >> +                     goto found_unlock;
> >> +             }
> >> +             if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc,
> >> dar)) {
> >> +                     residue = desc->px.bytes - (dar -
> > desc->px.dst_addr);
> >> +                     goto found_unlock;
> >> +             }
> >> +     }
> >> +
> >> +found_unlock:
> >> +     spin_unlock_irqrestore(&pch->lock, flags);
> >> +
> >> +     return residue;
> >> +}
> >> +
> >>  static enum dma_status
> >>  pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> >>                struct dma_tx_state *txstate)
> >>  {
> >> -     return dma_cookie_status(chan, cookie, txstate);
> >> +     enum dma_status ret;
> >> +
> >> +     ret = dma_cookie_status(chan, cookie, txstate);
> >> +     if (ret != DMA_SUCCESS) /* Not complete, check amount left. */
> >> +             dma_set_residue(txstate, pl330_tx_residue(chan));
> >> +
> >> +     return ret;
> >
> > Why didn't you use a cookie value to track the request?
> > The cookie is assigned when each transfer is submitted.
> > If you save the value in the desc, we can find the request easily.
> 
> If there are several cyclic desc in the work list, is there a better
> way to find the "current" one?  The chan struct tracks the last
> completed and last submitted cookies, but these will be the first and
> last respectively as long as the cyclic transfer is active.  Is there
> an "active" cookie stored somewhere that I missed?
> 
> Looking for the first buffer with status == BUSY is an improvement
> I'll make.  Any way to avoid looking through the list?
> 
> Thanks,
> 
> Dylan
> 
> >
> > Thanks,
> >
> > Best  Regards,
> > Chanho Park
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> !DSPAM:524ba462143697141112574!
> 


I phased out following hack to get asoc working (using a driver not yet
submitted but derived from hardkernel tree driver hkdk-max98090) with
dmaengine pcm. Highly likely it will only work for cyclic; it is aimed at
raising awareness of the current asoc dmaengine + pl330 breakage. I had to
include the transfer up to "used" to compute the residue to the end of
the buffer :

From 1199129e9a067e32f5aa4e9bc63f9527590b4c92 Mon Sep 17 00:00:00 2001
From: Dylan Reid <dgreid@chromium.org>
Date: Wed, 11 Sep 2013 11:38:03 +0530
Subject: [PATCH] dmaengine: pl330: Set residue in tx_status callback.

Fill txstate.residue with the amount of bytes remaining in the
transfers. This is required by alsa core dmaengine pcm pointer.

Based on patch from Dylan Reid <dgreid@chromium.org> but compute the
the residue in all transfers instead of the current one.

Signed-off-by: Alban Browaeys <prahal@yahoo.com>
---
 drivers/dma/pl330.c | 91
++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index ada7650..58df9ec 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2447,11 +2447,100 @@ static void pl330_free_chan_resources(struct
dma_chan *chan)
 	spin_unlock_irqrestore(&pch->lock, flags);
 }
 
+static inline int
+pl330_src_addr_in_desc(struct dma_pl330_desc *desc, unsigned int sar)
+{
+	return (desc->px.src_addr <= sar) &&
+		(sar <= (desc->px.src_addr + desc->px.bytes));
+}
+
+static inline int
+pl330_dst_addr_in_desc(struct dma_pl330_desc *desc, unsigned int dar)
+{
+	return (desc->px.dst_addr <= dar) &&
+		(dar <= (desc->px.dst_addr + desc->px.bytes));
+}
+
+
 static enum dma_status
 pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 		 struct dma_tx_state *txstate)
 {
-	return dma_cookie_status(chan, cookie, txstate);
+	struct dma_pl330_chan *pch = to_pchan(chan);
+	void __iomem *regs = pch->dmac->pif.base;
+	struct pl330_thread *thrd = pch->pl330_chid;
+	struct dma_pl330_desc *desc;
+	unsigned int sar, dar;
+	unsigned int residue = 0;
+	unsigned long flags;
+	bool first = true;
+	bool running = false;
+	dma_cookie_t last;
+	dma_cookie_t used;
+	enum dma_status ret;
+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE)
+		return ret;
+
+	/*
+	 * There's no point calculating the residue if there's
+	 * no txstate to store the value.
+	 */
+	if (!txstate)
+		return ret;
+
+	spin_lock_irqsave(&pch->lock, flags);
+	ret = dma_cookie_status(chan, cookie, txstate);
+	last = txstate->last;
+	used = txstate->used;
+	sar = readl(regs + SA(thrd->id));
+	dar = readl(regs + DA(thrd->id));
+
+	if (ret == DMA_COMPLETE) {
+		spin_unlock_irqrestore(&pch->lock, flags);
+		return ret;
+	}
+
+	list_for_each_entry(desc, &pch->work_list, node) {
+		if (desc->status == BUSY) {
+			if (first) {
+				first = false;
+				running = true;
+			}
+
+			if (!running) {
+				residue += desc->px.bytes;
+				continue;
+			}
+
+			if (desc->rqcfg.src_inc
+			    && pl330_src_addr_in_desc(desc, sar)) {
+				residue += desc->px.bytes;
+				residue -= sar - desc->px.src_addr;
+			} else if (desc->rqcfg.dst_inc
+				   && pl330_dst_addr_in_desc(desc, dar)) {
+				residue += desc->px.bytes;
+				residue -= dar - desc->px.dst_addr;
+			} else
+				WARN_ON(1);
+
+			running = false;
+		} else if (desc->status == PREP)
+			residue += desc->px.bytes;
+
+		if (desc->txd.cookie == used)
+			break;
+	}
+	spin_unlock_irqrestore(&pch->lock, flags);
+
+	/*
+	 * This cookie not complete yet
+	 * Get number of bytes left in the active transactions and queue
+	 */
+	dma_set_residue(txstate, residue);
+
+	return ret;
 }
 
 static void pl330_issue_pending(struct dma_chan *chan)
-- 
1.8.5



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel