From patchwork Tue Sep 3 15:02:46 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Fernandes X-Patchwork-Id: 2853280 Return-Path: X-Original-To: patchwork-davinci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id DBDD39F495 for ; Tue, 3 Sep 2013 15:04:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 51444203E3 for ; Tue, 3 Sep 2013 15:04:17 +0000 (UTC) Received: from bear.ext.ti.com (bear.ext.ti.com [192.94.94.41]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 89B1720374 for ; Tue, 3 Sep 2013 15:04:15 +0000 (UTC) Received: from dlelxv90.itg.ti.com ([172.17.2.17]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id r83F2o64029466; Tue, 3 Sep 2013 10:02:51 -0500 Received: from DLEE71.ent.ti.com (dlee71.ent.ti.com [157.170.170.114]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id r83F2oYJ002054; Tue, 3 Sep 2013 10:02:50 -0500 Received: from dflp32.itg.ti.com (10.64.6.15) by DLEE71.ent.ti.com (157.170.170.114) with Microsoft SMTP Server id 14.2.342.3; Tue, 3 Sep 2013 10:02:50 -0500 Received: from linux.omap.com (dlelxs01.itg.ti.com [157.170.227.31]) by dflp32.itg.ti.com (8.14.3/8.13.8) with ESMTP id r83F2oS2007206; Tue, 3 Sep 2013 10:02:50 -0500 Received: from linux.omap.com (localhost [127.0.0.1]) by linux.omap.com (Postfix) with ESMTP id 3A7CD80627; Tue, 3 Sep 2013 10:02:50 -0500 (CDT) X-Original-To: davinci-linux-open-source@linux.davincidsp.com Delivered-To: davinci-linux-open-source@linux.davincidsp.com Received: from dlelxv90.itg.ti.com (dlelxv90.itg.ti.com [172.17.2.17]) by linux.omap.com (Postfix) with ESMTP id 6C2B980626 for ; Tue, 3 Sep 2013 10:02:48 -0500 (CDT) Received: from DLEE70.ent.ti.com (dlee70.ent.ti.com [157.170.170.113]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id r83F2mP7001999; Tue, 3 Sep 2013 10:02:48 -0500 Received: from dlep32.itg.ti.com (157.170.170.100) by DLEE70.ent.ti.com (157.170.170.113) with Microsoft SMTP Server id 14.2.342.3; Tue, 3 Sep 2013 10:02:48 -0500 Received: from [172.24.115.20] (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep32.itg.ti.com (8.14.3/8.13.8) with ESMTP id r83F2lVq031565; Tue, 3 Sep 2013 10:02:47 -0500 Message-ID: <5225FA16.5000100@ti.com> Date: Tue, 3 Sep 2013 10:02:46 -0500 From: Joel Fernandes User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130803 Thunderbird/17.0.8 MIME-Version: 1.0 To: Vinod Koul Subject: Re: [PATCH v4 2/6] dma: edma: Write out and handle MAX_NR_SG at a given time References: <1377817545-18015-1-git-send-email-joelf@ti.com> <1377817545-18015-3-git-send-email-joelf@ti.com> <20130903040815.GC15824@intel.com> In-Reply-To: <20130903040815.GC15824@intel.com> CC: Linux DaVinci Kernel List , Russell King , Linux MMC List , Koen Kooi , Linux Kernel Mailing List , Sricharan R , Franklin Cooper , Dan Williams , Linux OMAP List , Linux ARM Kernel List X-BeenThere: davinci-linux-open-source@linux.davincidsp.com X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: Errors-To: davinci-linux-open-source-bounces@linux.davincidsp.com X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 09/02/2013 11:08 PM, Vinod Koul wrote: > On Thu, Aug 29, 2013 at 06:05:41PM -0500, Joel Fernandes wrote: >> Process SG-elements in batches of MAX_NR_SG if they are greater >> than MAX_NR_SG. Due to this, at any given time only those many >> slots will be used in the given channel no matter how long the >> scatter list is. We keep track of how much has been written >> inorder to process the next batch of elements in the scatter-list >> and detect completion. >> >> For such intermediate transfer completions (one batch of MAX_NR_SG), >> make use of pause and resume functions instead of start and stop >> when such intermediate transfer is in progress or completed as we >> donot want to clear any pending events. >> >> Signed-off-by: Joel Fernandes >> --- >> drivers/dma/edma.c | 79 ++++++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 53 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c >> index e522ad5..732829b 100644 >> --- a/drivers/dma/edma.c >> +++ b/drivers/dma/edma.c >> @@ -56,6 +56,7 @@ struct edma_desc { >> struct list_head node; >> int absync; >> int pset_nr; >> + int processed; >> struct edmacc_param pset[0]; >> }; >> >> @@ -104,22 +105,34 @@ static void edma_desc_free(struct virt_dma_desc *vdesc) >> /* Dispatch a queued descriptor to the controller (caller holds lock) */ >> static void edma_execute(struct edma_chan *echan) >> { >> - struct virt_dma_desc *vdesc = vchan_next_desc(&echan->vchan); >> + struct virt_dma_desc *vdesc; >> struct edma_desc *edesc; >> - int i; >> - >> - if (!vdesc) { >> - echan->edesc = NULL; >> - return; >> + struct device *dev = echan->vchan.chan.device->dev; >> + int i, j, left, nslots; >> + >> + /* If either we processed all psets or we're still not started */ >> + if (!echan->edesc || >> + echan->edesc->pset_nr == echan->edesc->processed) { >> + /* Get next vdesc */ >> + vdesc = vchan_next_desc(&echan->vchan); >> + if (!vdesc) { >> + echan->edesc = NULL; >> + return; >> + } >> + list_del(&vdesc->node); >> + echan->edesc = to_edma_desc(&vdesc->tx); >> } >> >> - list_del(&vdesc->node); >> + edesc = echan->edesc; >> >> - echan->edesc = edesc = to_edma_desc(&vdesc->tx); >> + /* Find out how many left */ >> + left = edesc->pset_nr - edesc->processed; >> + nslots = min(MAX_NR_SG, left); >> >> /* Write descriptor PaRAM set(s) */ >> - for (i = 0; i < edesc->pset_nr; i++) { >> - edma_write_slot(echan->slot[i], &edesc->pset[i]); >> + for (i = 0; i < nslots; i++) { >> + j = i + edesc->processed; >> + edma_write_slot(echan->slot[i], &edesc->pset[j]); >> dev_dbg(echan->vchan.chan.device->dev, >> "\n pset[%d]:\n" >> " chnum\t%d\n" >> @@ -132,24 +145,31 @@ static void edma_execute(struct edma_chan *echan) >> " bidx\t%08x\n" >> " cidx\t%08x\n" >> " lkrld\t%08x\n", >> - i, echan->ch_num, echan->slot[i], >> - edesc->pset[i].opt, >> - edesc->pset[i].src, >> - edesc->pset[i].dst, >> - edesc->pset[i].a_b_cnt, >> - edesc->pset[i].ccnt, >> - edesc->pset[i].src_dst_bidx, >> - edesc->pset[i].src_dst_cidx, >> - edesc->pset[i].link_bcntrld); >> + j, echan->ch_num, echan->slot[i], >> + edesc->pset[j].opt, >> + edesc->pset[j].src, >> + edesc->pset[j].dst, >> + edesc->pset[j].a_b_cnt, >> + edesc->pset[j].ccnt, >> + edesc->pset[j].src_dst_bidx, >> + edesc->pset[j].src_dst_cidx, >> + edesc->pset[j].link_bcntrld); >> /* Link to the previous slot if not the last set */ >> - if (i != (edesc->pset_nr - 1)) >> + if (i != (nslots - 1)) >> edma_link(echan->slot[i], echan->slot[i+1]); >> /* Final pset links to the dummy pset */ >> else >> edma_link(echan->slot[i], echan->ecc->dummy_slot); >> } >> >> - edma_start(echan->ch_num); >> + edesc->processed += nslots; >> + >> + edma_resume(echan->ch_num); >> + >> + if (edesc->processed <= MAX_NR_SG) { >> + dev_dbg(dev, "first transfer starting %d\n", echan->ch_num); >> + edma_start(echan->ch_num); >> + } >> } >> >> static int edma_terminate_all(struct edma_chan *echan) >> @@ -368,19 +388,26 @@ static void edma_callback(unsigned ch_num, u16 ch_status, void *data) >> struct edma_desc *edesc; >> unsigned long flags; >> >> - /* Stop the channel */ >> - edma_stop(echan->ch_num); >> + /* Pause the channel */ >> + edma_pause(echan->ch_num); >> >> switch (ch_status) { >> case DMA_COMPLETE: >> - dev_dbg(dev, "transfer complete on channel %d\n", ch_num); >> - >> spin_lock_irqsave(&echan->vchan.lock, flags); >> >> edesc = echan->edesc; >> if (edesc) { >> + if (edesc->processed == edesc->pset_nr) { >> + dev_dbg(dev, "transfer complete." \ >> + " stopping channel %d\n", ch_num); >> + edma_stop(echan->ch_num); >> + vchan_cookie_complete(&edesc->vdesc); >> + } else { >> + dev_dbg(dev, "Intermediate transfer complete" \ >> + " on channel %d\n", ch_num); > No, these two not right... Consider someone seeing this message will try to > grep "transfer complete stopping channel" and will fail miserable to find the > offending line. And this makes it uglier... > > Also I believe checkpatch has this check, don't you run that before sending > patches? Sorry I did run checkpatch but the >80 char warnings were still there. Thanks for pointing out that user-visible strings should not be split. > > Quoting CodingStyle: > > Chapter 2: Breaking long lines and strings > > Coding style is all about readability and maintainability using commonly > available tools. > > The limit on the length of lines is 80 columns and this is a strongly > preferred limit. > > Statements longer than 80 columns will be broken into sensible chunks, unless > exceeding 80 columns significantly increases readability and does not hide > information. Descendants are always substantially shorter than the parent and > are placed substantially to the right. The same applies to function headers > with a long argument list. However, never break user-visible strings such as > printk messages, because that breaks the ability to grep for them. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sure, I removed the string split and a generate a new patch below, Thanks. ---8<--- From: Joel Fernandes Subject: [PATCH v4 2/6] dma: edma: Write out and handle MAX_NR_SG at a given time Process SG-elements in batches of MAX_NR_SG if they are greater than MAX_NR_SG. Due to this, at any given time only those many slots will be used in the given channel no matter how long the scatter list is. We keep track of how much has been written inorder to process the next batch of elements in the scatter-list and detect completion. For such intermediate transfer completions (one batch of MAX_NR_SG), make use of pause and resume functions instead of start and stop when such intermediate transfer is in progress or completed as we donot want to clear any pending events. Signed-off-by: Joel Fernandes --- drivers/dma/edma.c | 77 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 51 insertions(+), 26 deletions(-) spin_unlock_irqrestore(&echan->vchan.lock, flags); diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index e522ad5..f5232bc 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -56,6 +56,7 @@ struct edma_desc { struct list_head node; int absync; int pset_nr; + int processed; struct edmacc_param pset[0]; }; @@ -104,22 +105,34 @@ static void edma_desc_free(struct virt_dma_desc *vdesc) /* Dispatch a queued descriptor to the controller (caller holds lock) */ static void edma_execute(struct edma_chan *echan) { - struct virt_dma_desc *vdesc = vchan_next_desc(&echan->vchan); + struct virt_dma_desc *vdesc; struct edma_desc *edesc; - int i; - - if (!vdesc) { - echan->edesc = NULL; - return; + struct device *dev = echan->vchan.chan.device->dev; + int i, j, left, nslots; + + /* If either we processed all psets or we're still not started */ + if (!echan->edesc || + echan->edesc->pset_nr == echan->edesc->processed) { + /* Get next vdesc */ + vdesc = vchan_next_desc(&echan->vchan); + if (!vdesc) { + echan->edesc = NULL; + return; + } + list_del(&vdesc->node); + echan->edesc = to_edma_desc(&vdesc->tx); } - list_del(&vdesc->node); + edesc = echan->edesc; - echan->edesc = edesc = to_edma_desc(&vdesc->tx); + /* Find out how many left */ + left = edesc->pset_nr - edesc->processed; + nslots = min(MAX_NR_SG, left); /* Write descriptor PaRAM set(s) */ - for (i = 0; i < edesc->pset_nr; i++) { - edma_write_slot(echan->slot[i], &edesc->pset[i]); + for (i = 0; i < nslots; i++) { + j = i + edesc->processed; + edma_write_slot(echan->slot[i], &edesc->pset[j]); dev_dbg(echan->vchan.chan.device->dev, "\n pset[%d]:\n" " chnum\t%d\n" @@ -132,24 +145,31 @@ static void edma_execute(struct edma_chan *echan) " bidx\t%08x\n" " cidx\t%08x\n" " lkrld\t%08x\n", - i, echan->ch_num, echan->slot[i], - edesc->pset[i].opt, - edesc->pset[i].src, - edesc->pset[i].dst, - edesc->pset[i].a_b_cnt, - edesc->pset[i].ccnt, - edesc->pset[i].src_dst_bidx, - edesc->pset[i].src_dst_cidx, - edesc->pset[i].link_bcntrld); + j, echan->ch_num, echan->slot[i], + edesc->pset[j].opt, + edesc->pset[j].src, + edesc->pset[j].dst, + edesc->pset[j].a_b_cnt, + edesc->pset[j].ccnt, + edesc->pset[j].src_dst_bidx, + edesc->pset[j].src_dst_cidx, + edesc->pset[j].link_bcntrld); /* Link to the previous slot if not the last set */ - if (i != (edesc->pset_nr - 1)) + if (i != (nslots - 1)) edma_link(echan->slot[i], echan->slot[i+1]); /* Final pset links to the dummy pset */ else edma_link(echan->slot[i], echan->ecc->dummy_slot); } - edma_start(echan->ch_num); + edesc->processed += nslots; + + edma_resume(echan->ch_num); + + if (edesc->processed <= MAX_NR_SG) { + dev_dbg(dev, "first transfer starting %d\n", echan->ch_num); + edma_start(echan->ch_num); + } } static int edma_terminate_all(struct edma_chan *echan) @@ -368,19 +388,24 @@ static void edma_callback(unsigned ch_num, u16 ch_status, void *data) struct edma_desc *edesc; unsigned long flags; - /* Stop the channel */ - edma_stop(echan->ch_num); + /* Pause the channel */ + edma_pause(echan->ch_num); switch (ch_status) { case DMA_COMPLETE: - dev_dbg(dev, "transfer complete on channel %d\n", ch_num); - spin_lock_irqsave(&echan->vchan.lock, flags); edesc = echan->edesc; if (edesc) { + if (edesc->processed == edesc->pset_nr) { + dev_dbg(dev, "Transfer complete, stopping channel %d\n", ch_num); + edma_stop(echan->ch_num); + vchan_cookie_complete(&edesc->vdesc); + } else { + dev_dbg(dev, "Intermediate transfer complete on channel %d\n", ch_num); + } + edma_execute(echan); - vchan_cookie_complete(&edesc->vdesc); }