dmaengine: qcom-bam: Process multiple pending descriptors

PATCH Superseded Sricharan R 2 participants 3 messages Aug 3, 2017 – Aug 9, 2017

Subsystems (2)

Arm Qualcomm mailing list dmaengine

lore

Share & tools

lore

Sricharan R

author

2017-08-03 13:21 UTC #0 lore raw

PATCH v2 1/1 dmaengine: qcom-bam: Process multiple pending descriptors

The bam dmaengine has a circular FIFO to which we add hw descriptors that describes the transaction. The FIFO has space for about 4096 hw descriptors.

Currently we add one descriptor and wait for it to complete with interrupt and then add the next pending descriptor. In this way, the FIFO is underutilized since only one descriptor is processed at a time, although there is space in FIFO for the BAM to process more.

Instead keep adding descriptors to FIFO till its full, that allows BAM to continue to work on the next descriptor immediately after signalling completion interrupt for the previous descriptor.

Also when the client has not set the DMA_PREP_INTERRUPT for a descriptor, then do not configure BAM to trigger a interrupt upon completion of that descriptor. This way we get a interrupt only for the descriptor for which DMA_PREP_INTERRUPT was requested and there signal completion of all the previous completed descriptors. So we still do callbacks for all requested descriptors, but just that the number of interrupts are reduced.

CURRENT:

            ------      -------   ---------------
            |DES 0|     |DESC 1|  |DESC 2 + INT |
            ------      -------   ---------------
               |           |            |
               |           |            |
INTERRUPT:   (INT)       (INT)	      (INT)
CALLBACK:     (CB)        (CB)         (CB)

MTD_SPEEDTEST READ PAGE: 3560 KiB/s MTD_SPEEDTEST WRITE PAGE: 2664 KiB/s IOZONE READ: 2456 KB/s IOZONE WRITE: 1230 KB/s

bam dma interrupts (after tests): 96508

CHANGE:

        ------  -------    -------------
        |DES 0| |DESC 1   |DESC 2 + INT |
        ------  -------   --------------
				|
				|
          		      (INT)
			      (CB for 0, 1, 2)

		MTD_SPEEDTEST READ PAGE: 3860 KiB/s
		MTD_SPEEDTEST WRITE PAGE: 2837 KiB/s
		IOZONE READ: 2677 KB/s
		IOZONE WRITE: 1308 KB/s

	bam dma interrupts (after tests): 58806

Signed-off-by Sricharan R <[email protected]>

---
 drivers/dma/qcom/bam_dma.c | 202 +++++++++++++++++++++++++++++----------------
 1 file changed, 129 insertions(+), 73 deletions(-)

drivers/dma/qcom/bam_dma.c +129 ~~-73~~

index 6d89fb6..9e92d7c 100644
@@ -46,6 +46,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_dma.h>
+#include <linux/circ_buf.h>
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/pm_runtime.h>

@@ -78,6 +79,8 @@ struct bam_async_desc {
 
 	struct bam_desc_hw *curr_desc;
 
+	/* list node for the desc in the bam_chan list of descriptors */
+	struct list_head desc_node;
 	enum dma_transfer_direction dir;
 	size_t length;
 	struct bam_desc_hw desc[0];

@@ -356,7 +359,7 @@ struct bam_chan {
 	/* configuration from device tree */
 	u32 id;
 
-	struct bam_async_desc *curr_txd;	/* current running dma */
+	bool is_busy; /* Set to true if FIFO is full */
 
 	/* runtime configuration */
 	struct dma_slave_config slave;

@@ -372,6 +375,8 @@ struct bam_chan {
 	unsigned int initialized;	/* is the channel hw initialized? */
 	unsigned int paused;		/* is the channel paused? */
 	unsigned int reconfigure;	/* new slave config? */
+	/* list of descriptors currently processed */
+	struct list_head desc_list;
 
 	struct list_head node;
 };

@@ -539,7 +544,7 @@ static void bam_free_chan(struct dma_chan *chan)
 
 	vchan_free_chan_resources(to_virt_chan(chan));
 
-	if (bchan->curr_txd) {
+	if (!list_empty(&bchan->desc_list)) {
 		dev_err(bchan->bdev->dev, "Cannot free busy channel\n");
 		goto err;
 	}

@@ -632,8 +637,6 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 
 	if (flags & DMA_PREP_INTERRUPT)
 		async_desc->flags |= DESC_FLAG_EOT;
-	else
-		async_desc->flags |= DESC_FLAG_INT;
 
 	async_desc->num_desc = num_alloc;
 	async_desc->curr_desc = async_desc->desc;

@@ -684,15 +687,14 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 static int bam_dma_terminate_all(struct dma_chan *chan)
 {
 	struct bam_chan *bchan = to_bam_chan(chan);
+	struct bam_async_desc *async_desc;
 	unsigned long flag;
 	LIST_HEAD(head);
 
 	/* remove all transactions, including active transaction */
 	spin_lock_irqsave(&bchan->vc.lock, flag);
-	if (bchan->curr_txd) {
-		list_add(&bchan->curr_txd->vd.node, &bchan->vc.desc_issued);
-		bchan->curr_txd = NULL;
-	}
+	list_for_each_entry(async_desc, &bchan->desc_list, desc_node)
+		list_add(&async_desc->vd.node, &bchan->vc.desc_issued);
 
 	vchan_get_all_descriptors(&bchan->vc, &head);
 	spin_unlock_irqrestore(&bchan->vc.lock, flag);

@@ -763,9 +765,9 @@ static int bam_resume(struct dma_chan *chan)
  */
 static u32 process_channel_irqs(struct bam_device *bdev)
 {
-	u32 i, srcs, pipe_stts;
+	u32 i, srcs, pipe_stts, offset, avail;
 	unsigned long flags;
-	struct bam_async_desc *async_desc;
+	struct bam_async_desc *async_desc, *tmp;
 
 	srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));

@@ -781,31 +783,51 @@ static u32 process_channel_irqs(struct bam_device *bdev)
 
 		/* clear pipe irq */
 		pipe_stts = readl_relaxed(bam_addr(bdev, i, BAM_P_IRQ_STTS));
-
 		writel_relaxed(pipe_stts, bam_addr(bdev, i, BAM_P_IRQ_CLR));
 
 		spin_lock_irqsave(&bchan->vc.lock, flags);
-		async_desc = bchan->curr_txd;
-
-		if (async_desc) {
-			async_desc->num_desc -= async_desc->xfer_len;
-			async_desc->curr_desc += async_desc->xfer_len;
-			bchan->curr_txd = NULL;
-
-			/* manage FIFO */
-			bchan->head += async_desc->xfer_len;
-			bchan->head %= MAX_DESCRIPTORS;
-
-			/*
-			 * if complete, process cookie.  Otherwise
-			 * push back to front of desc_issued so that
-			 * it gets restarted by the tasklet
-			 */
-			if (!async_desc->num_desc)
-				vchan_cookie_complete(&async_desc->vd);
-			else
-				list_add(&async_desc->vd.node,
-					&bchan->vc.desc_issued);
+
+		offset = readl_relaxed(bam_addr(bdev, i, BAM_P_SW_OFSTS)) &
+				       P_SW_OFSTS_MASK;
+		offset /= sizeof(struct bam_desc_hw);
+
+		/* Number of bytes available to read */
+		avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS + 1);
+
+		list_for_each_entry_safe(async_desc, tmp,
+					 &bchan->desc_list, desc_node) {
+			if (async_desc) {
+				/* Not enough data to read */
+				if (avail < async_desc->xfer_len)
+					break;
+
+				/* manage FIFO */
+				bchan->head += async_desc->xfer_len;
+				bchan->head %= MAX_DESCRIPTORS;
+
+				async_desc->num_desc -= async_desc->xfer_len;
+				async_desc->curr_desc += async_desc->xfer_len;
+				avail -= async_desc->xfer_len;
+
+				/*
+				 * if complete, process cookie. Otherwise
+				 * push back to front of desc_issued so that
+				 * it gets restarted by the tasklet
+				 */
+				if (!async_desc->num_desc) {
+					vchan_cookie_complete(&async_desc->vd);
+				} else {
+					list_add(&async_desc->vd.node,
+						 &bchan->vc.desc_issued);
+				}
+				list_del(&async_desc->desc_node);
+
+				/*
+				 * one desc has completed, so there is space
+				 * in FIFO
+				 */
+				bchan->is_busy = false;
+			}
 		}
 
 		spin_unlock_irqrestore(&bchan->vc.lock, flags);

@@ -867,6 +889,7 @@ static enum dma_status bam_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 		struct dma_tx_state *txstate)
 {
 	struct bam_chan *bchan = to_bam_chan(chan);
+	struct bam_async_desc *async_desc;
 	struct virt_dma_desc *vd;
 	int ret;
 	size_t residue = 0;

@@ -882,11 +905,17 @@ static enum dma_status bam_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 
 	spin_lock_irqsave(&bchan->vc.lock, flags);
 	vd = vchan_find_desc(&bchan->vc, cookie);
-	if (vd)
+	if (vd) {
 		residue = container_of(vd, struct bam_async_desc, vd)->length;
-	else if (bchan->curr_txd && bchan->curr_txd->vd.tx.cookie == cookie)
-		for (i = 0; i < bchan->curr_txd->num_desc; i++)
-			residue += bchan->curr_txd->curr_desc[i].size;
+	} else {
+		list_for_each_entry(async_desc, &bchan->desc_list, desc_node) {
+			if (async_desc->vd.tx.cookie != cookie)
+				continue;
+
+			for (i = 0; i < async_desc->num_desc; i++)
+				residue += async_desc->curr_desc[i].size;
+		}
+	}
 
 	spin_unlock_irqrestore(&bchan->vc.lock, flags);

@@ -927,63 +956,89 @@ static void bam_start_dma(struct bam_chan *bchan)
 {
 	struct virt_dma_desc *vd = vchan_next_desc(&bchan->vc);
 	struct bam_device *bdev = bchan->bdev;
-	struct bam_async_desc *async_desc;
+	struct bam_async_desc *async_desc = NULL;
 	struct bam_desc_hw *desc;
 	struct bam_desc_hw *fifo = PTR_ALIGN(bchan->fifo_virt,
 					sizeof(struct bam_desc_hw));
 	int ret;
+	unsigned int avail;
+	struct dmaengine_desc_callback cb;
 
 	lockdep_assert_held(&bchan->vc.lock);
 
 	if (!vd)
 		return;
 
-	list_del(&vd->node);
-
-	async_desc = container_of(vd, struct bam_async_desc, vd);
-	bchan->curr_txd = async_desc;
-
 	ret = pm_runtime_get_sync(bdev->dev);
 	if (ret < 0)
 		return;
 
-	/* on first use, initialize the channel hardware */
-	if (!bchan->initialized)
-		bam_chan_init_hw(bchan, async_desc->dir);
+	while (vd && !bchan->is_busy) {
+		list_del(&vd->node);
 
-	/* apply new slave config changes, if necessary */
-	if (bchan->reconfigure)
-		bam_apply_new_config(bchan, async_desc->dir);
+		async_desc = container_of(vd, struct bam_async_desc, vd);
 
-	desc = bchan->curr_txd->curr_desc;
+		/* on first use, initialize the channel hardware */
+		if (!bchan->initialized)
+			bam_chan_init_hw(bchan, async_desc->dir);
 
-	if (async_desc->num_desc > MAX_DESCRIPTORS)
-		async_desc->xfer_len = MAX_DESCRIPTORS;
-	else
-		async_desc->xfer_len = async_desc->num_desc;
+		/* apply new slave config changes, if necessary */
+		if (bchan->reconfigure)
+			bam_apply_new_config(bchan, async_desc->dir);
 
-	/* set any special flags on the last descriptor */
-	if (async_desc->num_desc == async_desc->xfer_len)
-		desc[async_desc->xfer_len - 1].flags |=
-					cpu_to_le16(async_desc->flags);
-	else
-		desc[async_desc->xfer_len - 1].flags |=
-					cpu_to_le16(DESC_FLAG_INT);
+		desc = async_desc->curr_desc;
+		avail = CIRC_SPACE(bchan->tail, bchan->head,
+				   MAX_DESCRIPTORS + 1);
+
+		if (async_desc->num_desc > avail)
+			async_desc->xfer_len = avail;
+		else
+			async_desc->xfer_len = async_desc->num_desc;
+
+		/* set any special flags on the last descriptor */
+		if (async_desc->num_desc == async_desc->xfer_len)
+			desc[async_desc->xfer_len - 1].flags |=
+						cpu_to_le16(async_desc->flags);
+
+		vd = vchan_next_desc(&bchan->vc);
+
+		/* FIFO is FULL */
+		bchan->is_busy = (avail <= async_desc->xfer_len) ? true : false;
 
-	if (bchan->tail + async_desc->xfer_len > MAX_DESCRIPTORS) {
-		u32 partial = MAX_DESCRIPTORS - bchan->tail;
+		dmaengine_desc_get_callback(&async_desc->vd.tx, &cb);
 
-		memcpy(&fifo[bchan->tail], desc,
-				partial * sizeof(struct bam_desc_hw));
-		memcpy(fifo, &desc[partial], (async_desc->xfer_len - partial) *
+		/*
+		 * An interrupt is generated at this desc, if
+		 *  - FIFO is FULL.
+		 *  - No more descriptors to add.
+		 *  - If a callback completion was requested for this DESC,
+		 *     In this case, BAM will deliver the completion callback
+		 *     for this desc and continue processing the next desc.
+		 */
+		if ((bchan->is_busy || !vd ||
+		     dmaengine_desc_callback_valid(&cb)) &&
+		    !(async_desc->flags & DESC_FLAG_EOT))
+			desc[async_desc->xfer_len - 1].flags |=
+				cpu_to_le16(DESC_FLAG_INT);
+
+		if (bchan->tail + async_desc->xfer_len > MAX_DESCRIPTORS) {
+			u32 partial = MAX_DESCRIPTORS - bchan->tail;
+
+			memcpy(&fifo[bchan->tail], desc,
+			       partial * sizeof(struct bam_desc_hw));
+			memcpy(fifo, &desc[partial],
+			       (async_desc->xfer_len - partial) *
 				sizeof(struct bam_desc_hw));
-	} else {
-		memcpy(&fifo[bchan->tail], desc,
-			async_desc->xfer_len * sizeof(struct bam_desc_hw));
-	}
+		} else {
+			memcpy(&fifo[bchan->tail], desc,
+			       async_desc->xfer_len *
+			       sizeof(struct bam_desc_hw));
+		}
 
-	bchan->tail += async_desc->xfer_len;
-	bchan->tail %= MAX_DESCRIPTORS;
+		bchan->tail += async_desc->xfer_len;
+		bchan->tail %= MAX_DESCRIPTORS;
+		list_add_tail(&async_desc->desc_node, &bchan->desc_list);
+	}
 
 	/* ensure descriptor writes and dma start not reordered */
 	wmb();

@@ -1012,7 +1067,7 @@ static void dma_tasklet(unsigned long data)
 		bchan = &bdev->channels[i];
 		spin_lock_irqsave(&bchan->vc.lock, flags);
 
-		if (!list_empty(&bchan->vc.desc_issued) && !bchan->curr_txd)
+		if (!list_empty(&bchan->vc.desc_issued) && !bchan->is_busy)
 			bam_start_dma(bchan);
 		spin_unlock_irqrestore(&bchan->vc.lock, flags);
 	}

@@ -1033,7 +1088,7 @@ static void bam_issue_pending(struct dma_chan *chan)
 	spin_lock_irqsave(&bchan->vc.lock, flags);
 
 	/* if work pending and idle, start a transaction */
-	if (vchan_issue_pending(&bchan->vc) && !bchan->curr_txd)
+	if (vchan_issue_pending(&bchan->vc) && !bchan->is_busy)
 		bam_start_dma(bchan);
 
 	spin_unlock_irqrestore(&bchan->vc.lock, flags);

@@ -1133,6 +1188,7 @@ static void bam_channel_init(struct bam_device *bdev, struct bam_chan *bchan,
 
 	vchan_init(&bchan->vc, &bdev->common);
 	bchan->vc.desc_free = bam_dma_free_desc;
+	INIT_LIST_HEAD(&bchan->desc_list);
 }
 
 static const struct of_device_id bam_of_match[] = {

Signature · 1 line

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Abhishek Sahu

Tested-by on v2 1/1

2017-08-09 14:18 UTC #1 lore raw

Quoted 57 lines · Sricharan R

The bam dmaengine has a circular FIFO to which we add hw descriptors that describes the transaction. The FIFO has space for about 4096 hw descriptors.
Currently we add one descriptor and wait for it to complete with interrupt and then add the next pending descriptor. In this way, the FIFO is underutilized since only one descriptor is processed at a time, although there is space in FIFO for the BAM to process more.
Instead keep adding descriptors to FIFO till its full, that allows BAM to continue to work on the next descriptor immediately after signalling completion interrupt for the previous descriptor.
Also when the client has not set the DMA_PREP_INTERRUPT for a descriptor, then do not configure BAM to trigger a interrupt upon completion of that descriptor. This way we get a interrupt only for the descriptor for which DMA_PREP_INTERRUPT was requested and there signal completion of all the previous completed descriptors. So we still do callbacks for all requested descriptors, but just that the number of interrupts are reduced.
CURRENT:
            ------      -------   ---------------
            |DES 0|     |DESC 1|  |DESC 2 + INT |
            ------      -------   ---------------
               |           |            |
               |           |            |
INTERRUPT:   (INT)       (INT)	      (INT)
CALLBACK:     (CB)        (CB)         (CB)
MTD_SPEEDTEST READ PAGE: 3560 KiB/s MTD_SPEEDTEST WRITE PAGE: 2664 KiB/s IOZONE READ: 2456 KB/s IOZONE WRITE: 1230 KB/s
bam dma interrupts (after tests): 96508
CHANGE:
        ------  -------    -------------
        |DES 0| |DESC 1   |DESC 2 + INT |
        ------  -------   --------------
				|
				|
          		      (INT)
			      (CB for 0, 1, 2)

		MTD_SPEEDTEST READ PAGE: 3860 KiB/s
		MTD_SPEEDTEST WRITE PAGE: 2837 KiB/s
		IOZONE READ: 2677 KB/s
		IOZONE WRITE: 1308 KB/s

	bam dma interrupts (after tests): 58806

Quoted 1 line

Signed-off-by Sricharan R <[email protected]>

Thanks Sricharan for your patch to do the descriptor clubbing in BAM DMA driver.

Verified this patch with my NAND QPIC patches
https://www.spinics.net/lists/kernel/msg2573736.html

I run the MTD test overnight and no failure observed. Also, achieved significant improvement in NAND speed. Following are the numbers for IPQ4019 DK04 board.

Test Speed in KiB/s
Before After

 eraseblock write speed  4716    5483
 eraseblock read speed   6855    8294
 page write speed        4678    5436
 page read speed         6784    8217

Tested-by Abhishek Sahu <[email protected]>

Also, I reviewed the patch and following are
minor comments.

Quoted 72 lines

---
 drivers/dma/qcom/bam_dma.c | 202
+++++++++++++++++++++++++++++----------------
 1 file changed, 129 insertions(+), 73 deletions(-)

drivers/dma/qcom/bam_dma.c +6 -1

index 6d89fb6..9e92d7c 100644
@@ -46,6 +46,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_dma.h>
+#include <linux/circ_buf.h>
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/pm_runtime.h>

@@ -78,6 +79,8 @@ struct bam_async_desc {
 
 	struct bam_desc_hw *curr_desc;
 
+	/* list node for the desc in the bam_chan list of descriptors */
+	struct list_head desc_node;
 	enum dma_transfer_direction dir;
 	size_t length;
 	struct bam_desc_hw desc[0];

@@ -356,7 +359,7 @@ struct bam_chan {
 	/* configuration from device tree */
 	u32 id;
 
-	struct bam_async_desc *curr_txd;	/* current running dma */
+	bool is_busy; /* Set to true if FIFO is full */
 
 	/* runtime configuration */
 	struct dma_slave_config slave;

@@ -372,6 +375,8 @@ struct bam_chan {
 	unsigned int initialized;	/* is the channel hw initialized?
 */
 	unsigned int paused;		/* is the channel paused? */
 	unsigned int reconfigure;	/* new slave config? */
+	/* list of descriptors currently processed */
+	struct list_head desc_list;
 
 	struct list_head node;

};

drivers/dma/qcom/bam_dma.c +1 -3

@@ -539,7 +544,7 @@ static void bam_free_chan(struct dma_chan *chan)
 
 	vchan_free_chan_resources(to_virt_chan(chan));
 
-	if (bchan->curr_txd) {
+	if (!list_empty(&bchan->desc_list)) {
 		dev_err(bchan->bdev->dev, "Cannot free busy channel\n");
 		goto err;
 	}

@@ -632,8 +637,6 @@ static struct dma_async_tx_descriptor
 *bam_prep_slave_sg(struct dma_chan *chan,
 
 	if (flags & DMA_PREP_INTERRUPT)
 		async_desc->flags |= DESC_FLAG_EOT;
-	else
-		async_desc->flags |= DESC_FLAG_INT;
 
 	async_desc->num_desc = num_alloc;

drivers/dma/qcom/bam_dma.c +1 -0

 	async_desc->curr_desc = async_desc->desc;

@@ -684,15 +687,14 @@ static struct dma_async_tx_descriptor
 *bam_prep_slave_sg(struct dma_chan *chan,
 static int bam_dma_terminate_all(struct dma_chan *chan)
 {
 	struct bam_chan *bchan = to_bam_chan(chan);
+	struct bam_async_desc *async_desc;
 	unsigned long flag;
 	LIST_HEAD(head);

drivers/dma/qcom/bam_dma.c static struct dma_async_tx_descriptor · line 699

Quoted 10 lines

drivers/dma/qcom/bam_dma.c +2 -4

 
 	/* remove all transactions, including active transaction */
 	spin_lock_irqsave(&bchan->vc.lock, flag);
-	if (bchan->curr_txd) {
-		list_add(&bchan->curr_txd->vd.node,
 &bchan->vc.desc_issued);
-		bchan->curr_txd = NULL;
-	}
+	list_for_each_entry(async_desc, &bchan->desc_list, desc_node)
+		list_add(&async_desc->vd.node, &bchan->vc.desc_issued);

should we free the list also since we are adding these descriptor back to issued and vchan_dma_desc_free_list will free all theses descriptors

When the IRQ will be triggered then it will traverse this list and fetch the async descriptor for which already we freed the memory.

Quoted 56 lines

drivers/dma/qcom/bam_dma.c +0 -0

 	vchan_get_all_descriptors(&bchan->vc, &head);

drivers/dma/qcom/bam_dma.c +4 ~~-24~~

 	spin_unlock_irqrestore(&bchan->vc.lock, flag);

@@ -763,9 +765,9 @@ static int bam_resume(struct dma_chan *chan)
  */
 static u32 process_channel_irqs(struct bam_device *bdev)
 {
-	u32 i, srcs, pipe_stts;
+	u32 i, srcs, pipe_stts, offset, avail;
 	unsigned long flags;
-	struct bam_async_desc *async_desc;
+	struct bam_async_desc *async_desc, *tmp;
 
 	srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));

@@ -781,31 +783,51 @@ static u32 process_channel_irqs(struct bam_device
 *bdev)
 
 		/* clear pipe irq */
 		pipe_stts = readl_relaxed(bam_addr(bdev, i,
 BAM_P_IRQ_STTS));
-
 		writel_relaxed(pipe_stts, bam_addr(bdev, i,
 BAM_P_IRQ_CLR));
 
 		spin_lock_irqsave(&bchan->vc.lock, flags);
-		async_desc = bchan->curr_txd;
-
-		if (async_desc) {
-			async_desc->num_desc -= async_desc->xfer_len;
-			async_desc->curr_desc += async_desc->xfer_len;
-			bchan->curr_txd = NULL;
-
-			/* manage FIFO */
-			bchan->head += async_desc->xfer_len;
-			bchan->head %= MAX_DESCRIPTORS;
-
-			/*
-			 * if complete, process cookie.  Otherwise
-			 * push back to front of desc_issued so that
-			 * it gets restarted by the tasklet
-			 */
-			if (!async_desc->num_desc)
-				vchan_cookie_complete(&async_desc->vd);
-			else
-				list_add(&async_desc->vd.node,
-					&bchan->vc.desc_issued);
+
+		offset = readl_relaxed(bam_addr(bdev, i, BAM_P_SW_OFSTS))

drivers/dma/qcom/bam_dma.c +5 -0

+				       P_SW_OFSTS_MASK;
+		offset /= sizeof(struct bam_desc_hw);
+
+		/* Number of bytes available to read */
+		avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS +

1);

drivers/dma/qcom/bam_dma.c static u32 process_channel_irqs(struct bam_device *bdev) · line 799

Quoted 4 lines

drivers/dma/qcom/bam_dma.c +4 -0

+
+		list_for_each_entry_safe(async_desc, tmp,
+					 &bchan->desc_list, desc_node) {
+			if (async_desc) {

Do we need this check since async_desc will be always not NULL.

Quoted 221 lines

drivers/dma/qcom/bam_dma.c +9 -0

+				/* Not enough data to read */
+				if (avail < async_desc->xfer_len)
+					break;
+
+				/* manage FIFO */
+				bchan->head += async_desc->xfer_len;
+				bchan->head %= MAX_DESCRIPTORS;
+
+				async_desc->num_desc -=

async_desc->xfer_len;

drivers/dma/qcom/bam_dma.c +1 -0

+				async_desc->curr_desc +=

async_desc->xfer_len;

drivers/dma/qcom/bam_dma.c +5 -0

+				avail -= async_desc->xfer_len;
+
+				/*
+				 * if complete, process cookie. Otherwise
+				 * push back to front of desc_issued so

that

drivers/dma/qcom/bam_dma.c +4 -0

+				 * it gets restarted by the tasklet
+				 */
+				if (!async_desc->num_desc) {
+

vchan_cookie_complete(&async_desc->vd);

drivers/dma/qcom/bam_dma.c +8 -0

+				} else {
+					list_add(&async_desc->vd.node,
+						 &bchan->vc.desc_issued);
+				}
+				list_del(&async_desc->desc_node);
+
+				/*
+				 * one desc has completed, so there is

space

drivers/dma/qcom/bam_dma.c +4 -0

+				 * in FIFO
+				 */
+				bchan->is_busy = false;
+			}
 		}
 
 		spin_unlock_irqrestore(&bchan->vc.lock, flags);

@@ -867,6 +889,7 @@ static enum dma_status bam_tx_status(struct

dma_chan
*chan, dma_cookie_t cookie,

drivers/dma/qcom/bam_dma.c +1 -0

 		struct dma_tx_state *txstate)
 {
 	struct bam_chan *bchan = to_bam_chan(chan);
+	struct bam_async_desc *async_desc;
 	struct virt_dma_desc *vd;
 	int ret;
 	size_t residue = 0;

@@ -882,11 +905,17 @@ static enum dma_status bam_tx_status(struct

dma_chan
*chan, dma_cookie_t cookie,

drivers/dma/qcom/bam_dma.c +1 -1

 	spin_lock_irqsave(&bchan->vc.lock, flags);
 	vd = vchan_find_desc(&bchan->vc, cookie);
-	if (vd)
+	if (vd) {
 		residue = container_of(vd, struct bam_async_desc,

vd)->length;

drivers/dma/qcom/bam_dma.c +0 -1

-	else if (bchan->curr_txd && bchan->curr_txd->vd.tx.cookie ==

cookie)

drivers/dma/qcom/bam_dma.c +2 -2

-		for (i = 0; i < bchan->curr_txd->num_desc; i++)
-			residue += bchan->curr_txd->curr_desc[i].size;
+	} else {
+		list_for_each_entry(async_desc, &bchan->desc_list,

desc_node) {

drivers/dma/qcom/bam_dma.c +38 ~~-29~~

+			if (async_desc->vd.tx.cookie != cookie)
+				continue;
+
+			for (i = 0; i < async_desc->num_desc; i++)
+				residue += async_desc->curr_desc[i].size;
+		}
+	}
 
 	spin_unlock_irqrestore(&bchan->vc.lock, flags);

@@ -927,63 +956,89 @@ static void bam_start_dma(struct bam_chan *bchan)
 {
 	struct virt_dma_desc *vd = vchan_next_desc(&bchan->vc);
 	struct bam_device *bdev = bchan->bdev;
-	struct bam_async_desc *async_desc;
+	struct bam_async_desc *async_desc = NULL;
 	struct bam_desc_hw *desc;
 	struct bam_desc_hw *fifo = PTR_ALIGN(bchan->fifo_virt,
 					sizeof(struct bam_desc_hw));
 	int ret;
+	unsigned int avail;
+	struct dmaengine_desc_callback cb;
 
 	lockdep_assert_held(&bchan->vc.lock);
 
 	if (!vd)
 		return;
 
-	list_del(&vd->node);
-
-	async_desc = container_of(vd, struct bam_async_desc, vd);
-	bchan->curr_txd = async_desc;
-
 	ret = pm_runtime_get_sync(bdev->dev);
 	if (ret < 0)
 		return;
 
-	/* on first use, initialize the channel hardware */
-	if (!bchan->initialized)
-		bam_chan_init_hw(bchan, async_desc->dir);
+	while (vd && !bchan->is_busy) {
+		list_del(&vd->node);
 
-	/* apply new slave config changes, if necessary */
-	if (bchan->reconfigure)
-		bam_apply_new_config(bchan, async_desc->dir);
+		async_desc = container_of(vd, struct bam_async_desc, vd);
 
-	desc = bchan->curr_txd->curr_desc;
+		/* on first use, initialize the channel hardware */
+		if (!bchan->initialized)
+			bam_chan_init_hw(bchan, async_desc->dir);
 
-	if (async_desc->num_desc > MAX_DESCRIPTORS)
-		async_desc->xfer_len = MAX_DESCRIPTORS;
-	else
-		async_desc->xfer_len = async_desc->num_desc;
+		/* apply new slave config changes, if necessary */
+		if (bchan->reconfigure)
+			bam_apply_new_config(bchan, async_desc->dir);
 
-	/* set any special flags on the last descriptor */
-	if (async_desc->num_desc == async_desc->xfer_len)
-		desc[async_desc->xfer_len - 1].flags |=
-					cpu_to_le16(async_desc->flags);
-	else
-		desc[async_desc->xfer_len - 1].flags |=
-					cpu_to_le16(DESC_FLAG_INT);
+		desc = async_desc->curr_desc;
+		avail = CIRC_SPACE(bchan->tail, bchan->head,
+				   MAX_DESCRIPTORS + 1);
+
+		if (async_desc->num_desc > avail)
+			async_desc->xfer_len = avail;
+		else
+			async_desc->xfer_len = async_desc->num_desc;
+
+		/* set any special flags on the last descriptor */
+		if (async_desc->num_desc == async_desc->xfer_len)
+			desc[async_desc->xfer_len - 1].flags |=
+
 cpu_to_le16(async_desc->flags);
+
+		vd = vchan_next_desc(&bchan->vc);
+
+		/* FIFO is FULL */
+		bchan->is_busy = (avail <= async_desc->xfer_len) ? true :
 false;
 
-	if (bchan->tail + async_desc->xfer_len > MAX_DESCRIPTORS) {
-		u32 partial = MAX_DESCRIPTORS - bchan->tail;
+		dmaengine_desc_get_callback(&async_desc->vd.tx, &cb);
 
-		memcpy(&fifo[bchan->tail], desc,
-				partial * sizeof(struct bam_desc_hw));
-		memcpy(fifo, &desc[partial], (async_desc->xfer_len -

partial) *

drivers/dma/qcom/bam_dma.c +5 -0

+		/*
+		 * An interrupt is generated at this desc, if
+		 *  - FIFO is FULL.
+		 *  - No more descriptors to add.
+		 *  - If a callback completion was requested for this

DESC,

drivers/dma/qcom/bam_dma.c +1 -0

+		 *     In this case, BAM will deliver the completion

callback

drivers/dma/qcom/bam_dma.c +1 -0

+		 *     for this desc and continue processing the next

desc.

drivers/dma/qcom/bam_dma.c +8 -0

+		 */
+		if ((bchan->is_busy || !vd ||
+		     dmaengine_desc_callback_valid(&cb)) &&
+		    !(async_desc->flags & DESC_FLAG_EOT))
+			desc[async_desc->xfer_len - 1].flags |=
+				cpu_to_le16(DESC_FLAG_INT);
+
+		if (bchan->tail + async_desc->xfer_len > MAX_DESCRIPTORS)

{

drivers/dma/qcom/bam_dma.c +6 -3

+			u32 partial = MAX_DESCRIPTORS - bchan->tail;
+
+			memcpy(&fifo[bchan->tail], desc,
+			       partial * sizeof(struct bam_desc_hw));
+			memcpy(fifo, &desc[partial],
+			       (async_desc->xfer_len - partial) *
 				sizeof(struct bam_desc_hw));
-	} else {
-		memcpy(&fifo[bchan->tail], desc,
-			async_desc->xfer_len * sizeof(struct

bam_desc_hw));

drivers/dma/qcom/bam_dma.c +10 -4

-	}
+		} else {
+			memcpy(&fifo[bchan->tail], desc,
+			       async_desc->xfer_len *
+			       sizeof(struct bam_desc_hw));
+		}
 
-	bchan->tail += async_desc->xfer_len;
-	bchan->tail %= MAX_DESCRIPTORS;
+		bchan->tail += async_desc->xfer_len;
+		bchan->tail %= MAX_DESCRIPTORS;
+		list_add_tail(&async_desc->desc_node, &bchan->desc_list);
+	}
 
 	/* ensure descriptor writes and dma start not reordered */
 	wmb();

@@ -1012,7 +1067,7 @@ static void dma_tasklet(unsigned long data)
 		bchan = &bdev->channels[i];
 		spin_lock_irqsave(&bchan->vc.lock, flags);
 
-		if (!list_empty(&bchan->vc.desc_issued) &&
 !bchan->curr_txd)
+		if (!list_empty(&bchan->vc.desc_issued) &&
 !bchan->is_busy)
 			bam_start_dma(bchan);

drivers/dma/qcom/bam_dma.c +0 -0

 		spin_unlock_irqrestore(&bchan->vc.lock, flags);
 	}

drivers/dma/qcom/bam_dma.c static void bam_issue_pending(struct dma_chan · line 1093

Quoted 7 lines

drivers/dma/qcom/bam_dma.c +1 -1

@@ -1033,7 +1088,7 @@ static void bam_issue_pending(struct dma_chan 
 *chan)
 	spin_lock_irqsave(&bchan->vc.lock, flags);
 
 	/* if work pending and idle, start a transaction */
-	if (vchan_issue_pending(&bchan->vc) && !bchan->curr_txd)
+	if (vchan_issue_pending(&bchan->vc) && !bchan->is_busy)
 		bam_start_dma(bchan);

can we get rid of these bchan->is_busy since it is being used whether we have space in actual hardware FIFO and same we can check with CIRC_SPACE(bchan->tail, bchan->head, MAX_DESCRIPTORS + 1)

Thanks,
Abhishek

Quoted 11 lines

drivers/dma/qcom/bam_dma.c +1 -0

 	spin_unlock_irqrestore(&bchan->vc.lock, flags);

@@ -1133,6 +1188,7 @@ static void bam_channel_init(struct bam_device
 *bdev, struct bam_chan *bchan,
 
 	vchan_init(&bchan->vc, &bdev->common);
 	bchan->vc.desc_free = bam_dma_free_desc;
+	INIT_LIST_HEAD(&bchan->desc_list);
 }

static const struct of_device_id bam_of_match[] = {

Sricharan R

author

2017-08-09 14:24 UTC #2 lore raw

Hi Abhishek,

Quoted 74 lines · Abhishek Sahu

Quoted 57 lines · Sricharan R
The bam dmaengine has a circular FIFO to which we add hw descriptors that describes the transaction. The FIFO has space for about 4096 hw descriptors.
Currently we add one descriptor and wait for it to complete with interrupt and then add the next pending descriptor. In this way, the FIFO is underutilized since only one descriptor is processed at a time, although there is space in FIFO for the BAM to process more.
Instead keep adding descriptors to FIFO till its full, that allows BAM to continue to work on the next descriptor immediately after signalling completion interrupt for the previous descriptor.
Also when the client has not set the DMA_PREP_INTERRUPT for a descriptor, then do not configure BAM to trigger a interrupt upon completion of that descriptor. This way we get a interrupt only for the descriptor for which DMA_PREP_INTERRUPT was requested and there signal completion of all the previous completed descriptors. So we still do callbacks for all requested descriptors, but just that the number of interrupts are reduced.
CURRENT:
            ------      -------   ---------------
            |DES 0|     |DESC 1|  |DESC 2 + INT |
            ------      -------   ---------------
               |           |            |
               |           |            |
INTERRUPT:   (INT)       (INT)          (INT)
CALLBACK:     (CB)        (CB)         (CB)
MTD_SPEEDTEST READ PAGE: 3560 KiB/s
MTD_SPEEDTEST WRITE PAGE: 2664 KiB/s
IOZONE READ: 2456 KB/s
IOZONE WRITE: 1230 KB/s
bam dma interrupts (after tests): 96508
CHANGE:
        ------  -------    -------------
        |DES 0| |DESC 1   |DESC 2 + INT |
        ------  -------   --------------
                |
                |
                        (INT)
                  (CB for 0, 1, 2)

        MTD_SPEEDTEST READ PAGE: 3860 KiB/s
        MTD_SPEEDTEST WRITE PAGE: 2837 KiB/s
        IOZONE READ: 2677 KB/s
        IOZONE WRITE: 1308 KB/s

    bam dma interrupts (after tests): 58806
Quoted 1 line
Signed-off-by Sricharan R <[email protected]>
Thanks Sricharan for your patch to do the descriptor clubbing in BAM DMA driver.
Verified this patch with my NAND QPIC patches
https://www.spinics.net/lists/kernel/msg2573736.html
I run the MTD test overnight and no failure observed. Also, achieved significant improvement in NAND speed. Following are the numbers for IPQ4019 DK04 board.
Test Speed in KiB/s
Before After

Quoted 7 lines

 eraseblock write speed  4716    5483
 eraseblock read speed   6855    8294
 page write speed        4678    5436
 page read speed         6784    8217

Tested-by Abhishek Sahu <[email protected]>

Thanks for the testing.

Quoted 2 lines

Also, I reviewed the patch and following are
minor comments.

ok.

Quoted 6 lines

drivers/dma/qcom/bam_dma.c static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan, · line 697
Quoted 5 lines
&bchan->vc.desc_issued);
drivers/dma/qcom/bam_dma.c +2 -2
-        bchan->curr_txd = NULL;
-    }
+    list_for_each_entry(async_desc, &bchan->desc_list, desc_node)
+        list_add(&async_desc->vd.node, &bchan->vc.desc_issued);

Quoted 7 lines

should we free the list also since we are adding these descriptor back to issued and vchan_dma_desc_free_list will free all theses descriptors
When the IRQ will be triggered then it will traverse this list and fetch the async descriptor for which already we freed the memory.

ha ok, should add list_del here.

<snip..>

Quoted 7 lines

drivers/dma/qcom/bam_dma.c static u32 process_channel_irqs(struct bam_device *bdev) · line 799
Quoted 5 lines
1);
drivers/dma/qcom/bam_dma.c +4 -0
+
+        list_for_each_entry_safe(async_desc, tmp,
+                     &bchan->desc_list, desc_node) {
+            if (async_desc) {
Do we need this check since async_desc will be always not NULL.

not needed, will remove.

<snip..>

Quoted 9 lines

drivers/dma/qcom/bam_dma.c static void bam_issue_pending(struct dma_chan *chan) · line 1092
Quoted 4 lines
     /* if work pending and idle, start a transaction */
drivers/dma/qcom/bam_dma.c +1 -1
-    if (vchan_issue_pending(&bchan->vc) && !bchan->curr_txd)
+    if (vchan_issue_pending(&bchan->vc) && !bchan->is_busy)
         bam_start_dma(bchan);
can we get rid of these bchan->is_busy since it is being used whether we have space in actual hardware FIFO and same we can check with CIRC_SPACE(bchan->tail, bchan->head, MAX_DESCRIPTORS + 1)

yeah, this variable can be removed and simplified. Will do in V3.

Regards,
Sricharan

Signature · 5 lines

"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus