Skip to contentSkip to conversation

I/OAT: Add support for version 2 of ioatdma device

PATCH In review Shannon Nelson 2 participants 3 messages Oct 18, 2007 – Oct 24, 2007

Subsystems (3)
PCI dmaengine Intel i/oat DMA driver
lore
Share & tools
lore

Shannon Nelson

author
#0 lore raw

PATCH 1/1 I/OAT: Add support for version 2 of ioatdma device

Add support for version 2 of the ioatdma device. This device handles the descriptor chain and DCA services slightly differently:
- Instead of moving the dma descriptors between a busy and an idle chain, this new version uses a single circular chain so that we don't have rewrite the next_descriptor pointers as we add new requests, and the device doesn't need to re-read the last descriptor.
- The new device has the DCA tags defined internally instead of needing them defined statically.

Signed-off-by Shannon Nelson <[email protected]>

---

 drivers/dma/ioat.c              |   11 +
 drivers/dma/ioat_dca.c          |  164 +++++++++++
 drivers/dma/ioat_dma.c          |  578 ++++++++++++++++++++++++++++++++-------
 drivers/dma/ioatdma.h           |   32 +-
 drivers/dma/ioatdma_hw.h        |   33 ++
 drivers/dma/ioatdma_registers.h |  106 ++++++-
 include/linux/pci_ids.h         |    1 
 7 files changed, 780 insertions(+), 145 deletions(-)
drivers/dma/ioat.c +11 -0
index f204c39..16e0fd8 100644
@@ -39,10 +39,14 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Intel Corporation");
 
 static struct pci_device_id ioat_pci_tbl[] = {
+	/* I/OAT v1 platforms */
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IOAT) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IOAT_CNB)  },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IOAT_SCNB) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_UNISYS, PCI_DEVICE_ID_UNISYS_DMA_DIRECTOR) },
+
+	/* I/OAT v2 platforms */
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IOAT_SNB) },
 	{ 0, }
 };
 

@@ -74,10 +78,17 @@ static int ioat_setup_functionality(struct pci_dev *pdev, void __iomem *iobase)
 		if (device->dma && ioat_dca_enabled)
 			device->dca = ioat_dca_init(pdev, iobase);
 		break;
+	case IOAT_VER_2_0:
+		device->dma = ioat_dma_probe(pdev, iobase);
+		if (device->dma && ioat_dca_enabled)
+			device->dca = ioat2_dca_init(pdev, iobase);
+		break;
 	default:
 		err = -ENODEV;
 		break;
 	}
+	if (!device->dma)
+		err = -ENODEV;
 	return err;
 }
drivers/dma/ioat_dca.c +164 -0

167 lines not on this page · raw message

drivers/dma/ioat_dma.c +476 -102

804 lines not on this page · raw message

drivers/dma/ioatdma.h +12 -20
index 5f9881e..b668234 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright(c) 2004 - 2006 Intel Corporation. All rights reserved.
+ * Copyright(c) 2004 - 2007 Intel Corporation. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the Free

@@ -28,7 +28,7 @@
 #include <linux/cache.h>
 #include <linux/pci_ids.h>
 
-#define IOAT_DMA_VERSION "1.26"
+#define IOAT_DMA_VERSION  "2.04"
 
 enum ioat_interrupt {
 	none = 0,

@@ -39,6 +39,8 @@ enum ioat_interrupt {
 };
 
 #define IOAT_LOW_COMPLETION_MASK	0xffffffc0
+#define IOAT_DMA_DCA_ANY_CPU		~0
+
 
 /**
  * struct ioatdma_device - internal representation of a IOAT device

@@ -47,6 +49,9 @@ enum ioat_interrupt {
  * @dma_pool: for allocating DMA descriptors
  * @common: embedded struct dma_device
  * @version: version of ioatdma device
+ * @irq_mode: which style irq to use
+ * @msix_entries: irq handlers
+ * @idx: per channel data
  */
 
 struct ioatdma_device {

@@ -63,23 +68,7 @@ struct ioatdma_device {
 
 /**
  * struct ioat_dma_chan - internal representation of a DMA channel
- * @device:
- * @reg_base:
- * @sw_in_use:
- * @completion:
- * @completion_low:
- * @completion_high:
- * @completed_cookie: last cookie seen completed on cleanup
- * @cookie: value of last cookie given to client
- * @last_completion:
- * @xfercap:
- * @desc_lock:
- * @free_desc:
- * @used_desc:
- * @resource:
- * @device_node:
  */
-
 struct ioat_dma_chan {
 
 	void __iomem *reg_base;

@@ -95,6 +84,8 @@ struct ioat_dma_chan {
 	struct list_head used_desc;
 
 	int pending;
+	int dmacount;
+	int desccount;
 
 	struct ioatdma_device *device;
 	struct dma_chan common;

@@ -134,12 +125,13 @@ struct ioat_desc_sw {
 struct ioatdma_device *ioat_dma_probe(struct pci_dev *pdev,
 				      void __iomem *iobase);
 void ioat_dma_remove(struct ioatdma_device *device);
-struct dca_provider *ioat_dca_init(struct pci_dev *pdev,
-				   void __iomem *iobase);
+struct dca_provider *ioat_dca_init(struct pci_dev *pdev, void __iomem *iobase);
+struct dca_provider *ioat2_dca_init(struct pci_dev *pdev, void __iomem *iobase);
 #else
 #define ioat_dma_probe(pdev, iobase)    NULL
 #define ioat_dma_remove(device)         do { } while (0)
 #define ioat_dca_init(pdev, iobase)	NULL
+#define ioat2_dca_init(pdev, iobase)	NULL
 #endif
 
 #endif /* IOATDMA_H */
drivers/dma/ioatdma_hw.h +25 -8
index 9e7434e..dd470fa 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright(c) 2004 - 2006 Intel Corporation. All rights reserved.
+ * Copyright(c) 2004 - 2007 Intel Corporation. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the Free

@@ -22,12 +22,19 @@
 #define _IOAT_HW_H_
 
 /* PCI Configuration Space Values */
-#define IOAT_PCI_VID			0x8086
-#define IOAT_PCI_DID			0x1A38
-#define IOAT_PCI_RID			0x00
-#define IOAT_PCI_SVID			0x8086
-#define IOAT_PCI_SID			0x8086
-#define IOAT_VER_1_2			0x12	/* Version 1.2 */
+#define IOAT_PCI_VID            0x8086
+
+/* CB device ID's */
+#define IOAT_PCI_DID_5000       0x1A38
+#define IOAT_PCI_DID_CNB        0x360B
+#define IOAT_PCI_DID_SCNB       0x65FF
+#define IOAT_PCI_DID_SNB        0x402F
+
+#define IOAT_PCI_RID            0x00
+#define IOAT_PCI_SVID           0x8086
+#define IOAT_PCI_SID            0x8086
+#define IOAT_VER_1_2            0x12    /* Version 1.2 */
+#define IOAT_VER_2_0            0x20    /* Version 2.0 */
 
 struct ioat_dma_descriptor {
 	uint32_t	size;

@@ -47,6 +54,16 @@ struct ioat_dma_descriptor {
 #define IOAT_DMA_DESCRIPTOR_CTL_CP_STS	0x00000008
 #define IOAT_DMA_DESCRIPTOR_CTL_FRAME	0x00000010
 #define IOAT_DMA_DESCRIPTOR_NUL		0x00000020
-#define IOAT_DMA_DESCRIPTOR_OPCODE	0xFF000000
+#define IOAT_DMA_DESCRIPTOR_CTL_SP_BRK	0x00000040
+#define IOAT_DMA_DESCRIPTOR_CTL_DP_BRK	0x00000080
+#define IOAT_DMA_DESCRIPTOR_CTL_BNDL	0x00000100
+#define IOAT_DMA_DESCRIPTOR_CTL_DCA	0x00000200
+#define IOAT_DMA_DESCRIPTOR_CTL_BUFHINT	0x00000400
+
+#define IOAT_DMA_DESCRIPTOR_CTL_OPCODE_CONTEXT	0xFF000000
+#define IOAT_DMA_DESCRIPTOR_CTL_OPCODE_DMA	0x00000000
+
+#define IOAT_DMA_DESCRIPTOR_CTL_CONTEXT_DCA	0x00000001
+#define IOAT_DMA_DESCRIPTOR_CTL_OPCODE_MASK	0xFF000000
 
 #endif
drivers/dma/ioatdma_registers.h +91 -15

148 lines not on this page · raw message

include/linux/pci_ids.h +1 -0
index df948b4..61a3023 100644
@@ -2327,6 +2327,7 @@
 #define PCI_DEVICE_ID_INTEL_MCH_PC1	0x359a
 #define PCI_DEVICE_ID_INTEL_E7525_MCH	0x359e
 #define PCI_DEVICE_ID_INTEL_IOAT_CNB	0x360b
+#define PCI_DEVICE_ID_INTEL_IOAT_SNB	0x402f
 #define PCI_DEVICE_ID_INTEL_IOAT_SCNB	0x65ff
 #define PCI_DEVICE_ID_INTEL_TOLAPAI_0	0x5031
 #define PCI_DEVICE_ID_INTEL_TOLAPAI_1	0x5032
#1 lore raw
Quoted 11 lines · On Fri, 19 Oct 2007 00:20:25 -0700 Shannon Nelson <[email protected]>…

Add support for version 2 of the ioatdma device. This device handles the descriptor chain and DCA services slightly differently:
- Instead of moving the dma descriptors between a busy and an idle chain, this new version uses a single circular chain so that we don't have rewrite the next_descriptor pointers as we add new requests, and the device doesn't need to re-read the last descriptor.
- The new device has the DCA tags defined internally instead of needing them defined statically.

...

drivers/dma/ioat_dma.c static void ioat_set_dest(dma_addr_t addr, · line 179

Quoted 4 lines
drivers/dma/ioat_dma.c +4 -0
+static inline void __ioat1_dma_memcpy_issue_pending(
+					       struct ioat_dma_chan *ioat_chan);
+static inline void __ioat2_dma_memcpy_issue_pending(
+					       struct ioat_dma_chan *ioat_chan);

It's somewhat unusual to forward-declare an inline like this. My version of gcc does do the right thing with it, but for the sake of conventionality, readability and cleanliness, please consider just moving the implementations higher up the file sometime, thanks.

Quoted 14 lines
drivers/dma/ioat_dma.c +14 -0
+static dma_cookie_t ioat2_tx_submit(struct dma_async_tx_descriptor *tx)
+{
+	struct ioat_dma_chan *ioat_chan = to_ioat_chan(tx->chan);
+	struct ioat_desc_sw *first = tx_to_ioat_desc(tx);
+	struct ioat_desc_sw *new;
+	struct ioat_dma_descriptor *hw;
+	dma_cookie_t cookie;
+	u32 copy;
+	size_t len;
+	dma_addr_t src, dst;
+	int orig_ack;
+	unsigned int desc_count = 0;
+
+	/* src and dest and len are stored in the initial descriptor */

drivers/dma/ioat_dma.c static dma_cookie_t ioat_tx_submit(struct dma_async_tx_descriptor *tx) · line 297

Quoted 10 lines
drivers/dma/ioat_dma.c +10 -0
+	len = first->len;
+	src = first->src;
+	dst = first->dst;
+	orig_ack = first->async_tx.ack;
+	new = first;
+
+	/* ioat_chan->desc_lock is still in force in version 2 path */
+
+	do {
+		copy = min((u32) len, ioat_chan->xfercap);

min_t is the preferred tool for this.

Quoted 60 lines
drivers/dma/ioat_dma.c +56 -0
+		new->async_tx.ack = 1;
+
+		hw = new->hw;
+		hw->size = copy;
+		hw->ctl = 0;
+		hw->src_addr = src;
+		hw->dst_addr = dst;
+
+		len -= copy;
+		dst += copy;
+		src += copy;
+		desc_count++;
+	} while (len && (new = ioat2_dma_get_next_descriptor(ioat_chan)));
+
+	hw->ctl = IOAT_DMA_DESCRIPTOR_CTL_CP_STS;
+	if (new->async_tx.callback) {
+		hw->ctl |= IOAT_DMA_DESCRIPTOR_CTL_INT_GN;
+		if (first != new) {
+			/* move callback into to last desc */
+			new->async_tx.callback = first->async_tx.callback;
+			new->async_tx.callback_param
+					= first->async_tx.callback_param;
+			first->async_tx.callback = NULL;
+			first->async_tx.callback_param = NULL;
+		}
+	}
+
+	new->tx_cnt = desc_count;
+	new->async_tx.ack = orig_ack; /* client is in control of this ack */
+
+	/* store the original values for use in later cleanup */
+	if (new != first) {
+		new->src = first->src;
+		new->dst = first->dst;
+		new->len = first->len;
+	}
+
+	/* cookie incr and addition to used_list must be atomic */
+	cookie = ioat_chan->common.cookie;
+	cookie++;
+	if (cookie < 0)
+		cookie = 1;
+	ioat_chan->common.cookie = new->async_tx.cookie = cookie;
+
+	ioat_chan->dmacount += desc_count;
+	ioat_chan->pending += desc_count;
+	if (ioat_chan->pending >= ioat_pending_level)
+		__ioat2_dma_memcpy_issue_pending(ioat_chan);
+	spin_unlock_bh(&ioat_chan->desc_lock);
 
 	return cookie;
 }
 
+{
+	struct ioat_desc_sw *new = NULL;
+
+	/*
+	 * used.prev points to where to start processing
+	 * used.next points to next free descriptor
+	 * if used.prev == NULL, there are none waiting to be processed

drivers/dma/ioat_dma.c static dma_cookie_t ioat_tx_submit(struct dma_async_tx_descriptor *tx)

Quoted 10 lines
drivers/dma/ioat_dma.c +10 -0
+	 * if used.next == used.prev.prev, there is only one free descriptor,
+	 *      and we need to use it to as a noop descriptor before
+	 *      linking in a new set of descriptors, since the device
+	 *      has probably already read the pointer to it
+	 */
+	if (ioat_chan->used_desc.prev &&
+	    ioat_chan->used_desc.next == ioat_chan->used_desc.prev->prev) {
+
+		struct ioat_desc_sw *desc = NULL;
+		struct ioat_desc_sw *noop_desc = NULL;

The noop_desc initialisation is unneeded and so too is the initialisation of `desc', I suspect.

The compiler should avoid any additional code generation, but unneeded initialisations like this can cause useful warnings to be suppressed and they're a bit misleading to the reader of the code.

Quoted 7 lines
drivers/dma/ioat_dma.c +7 -0
+		int i;
+
+		/* set up the noop descriptor */
+		noop_desc = to_ioat_desc(ioat_chan->used_desc.next);
+		noop_desc->hw->size = 0;
+		noop_desc->hw->ctl = IOAT_DMA_DESCRIPTOR_NUL;
+		noop_desc->hw->src_addr = 0;

drivers/dma/ioat_dma.c ioat_dma_get_next_descriptor(struct ioat_dma_chan *ioat_chan) · line 630

Quoted 10 lines
drivers/dma/ioat_dma.c +10 -0
+		noop_desc->hw->dst_addr = 0;
+
+		ioat_chan->used_desc.next = ioat_chan->used_desc.next->next;
+		ioat_chan->pending++;
+		ioat_chan->dmacount++;
+
+		/* get a few more descriptors */
+		for (i = 16; i; i--) {
+			desc = ioat_dma_alloc_descriptor(ioat_chan, GFP_ATOMIC);
+			BUG_ON(!desc);

You can't do that!

GFP_ATOMIC allocations can and do fail under regular (albeit heavy) use. Code _must_ be able to recover from an allocation failure. Taking out the whole machine is not acceptable.

An exception to this is when the GFP_ATOMIC allocation is happening at initial boot time (ie: in __init in code which cannot be loaded as a module).

Quoted 28 lines
drivers/dma/ioat_dma.c +27 -0
+			list_add_tail(&desc->node, ioat_chan->used_desc.next);
+
+			desc->hw->next
+				= to_ioat_desc(desc->node.next)->async_tx.phys;
+			to_ioat_desc(desc->node.prev)->hw->next
+				= desc->async_tx.phys;
+			ioat_chan->desccount++;
+		}
+
+		ioat_chan->used_desc.next = noop_desc->node.next;
+	}
+	new = to_ioat_desc(ioat_chan->used_desc.next);
+	prefetch(new);
+	ioat_chan->used_desc.next = new->node.next;
+
+	if (ioat_chan->used_desc.prev == NULL)
+		ioat_chan->used_desc.prev = &new->node;
+
+	prefetch(new->hw);
+	return new;
+}
+
 
+static struct dma_async_tx_descriptor *ioat2_dma_prep_memcpy(
+						struct dma_chan *chan,
+						size_t len,
+						int int_en)
+{

drivers/dma/ioat_dma.c ioat_dma_get_next_descriptor(struct ioat_dma_chan *ioat_chan)

Quoted 10 lines
drivers/dma/ioat_dma.c +10 -0
+	struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
+	struct ioat_desc_sw *new;
+
+	spin_lock_bh(&ioat_chan->desc_lock);
+	new = ioat2_dma_get_next_descriptor(ioat_chan);
+	new->len = len;
+
+	/* leave ioat_chan->desc_lock set in version 2 path */
+	return new ? &new->async_tx : NULL;
+}

I assume that there's a spin_unlock_bh() somewhere..

drivers/dma/ioat_dma.c static struct dma_async_tx_descriptor *ioat_dma_prep_memcpy( · line 743

Quoted 10 lines
drivers/dma/ioat_dma.c +8 -0
+static void ioat2_dma_memcpy_issue_pending(struct dma_chan *chan)
+{
+	struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
+
+	if (ioat_chan->pending != 0) {
+		spin_lock_bh(&ioat_chan->desc_lock);
+		__ioat2_dma_memcpy_issue_pending(ioat_chan);
+		spin_unlock_bh(&ioat_chan->desc_lock);
 	}
 }

Is it non-racy to test ->pending outside the lock?

drivers/dma/ioatdma.h enum ioat_interrupt { · line 43

Quoted 4 lines

#define IOAT_LOW_COMPLETION_MASK 0xffffffc0

drivers/dma/ioatdma.h +2 -0
+#define IOAT_DMA_DCA_ANY_CPU		~0
+

The code is refreshingly free from usable comments. A reader might wonder what things like IOAT_DMA_DCA_ANY_CPU actually represent. I can't work it out from the bare C implementation.

drivers/dma/ioatdma.h struct ioat_desc_sw { · line 135

Quoted 11 lines

void ioat_dma_remove(struct ioatdma_device *device);

drivers/dma/ioatdma.h +3 -2
-struct dca_provider *ioat_dca_init(struct pci_dev *pdev,
-				   void __iomem *iobase);
+struct dca_provider *ioat_dca_init(struct pci_dev *pdev, void __iomem *iobase);
+struct dca_provider *ioat2_dca_init(struct pci_dev *pdev, void __iomem *iobase);
 #else
 #define ioat_dma_probe(pdev, iobase)    NULL
 #define ioat_dma_remove(device)         do { } while (0)
 #define ioat_dca_init(pdev, iobase)	NULL
+#define ioat2_dca_init(pdev, iobase)	NULL
 #endif

grumble. All these could be implemented as inlines. Please only use macros when the code cannot be implemented in C.

Reasons:

- we get typechecking even when CONFIG_YOUR_STUFF=n

- can still take the address of the functions (dubious)

- looks nicer

author
#2 lore raw
Quoted 7 lines

From: Andrew Morton [mailto:[email protected]]

Quoted 3 lines · On Fri, 19 Oct 2007 00:20:25 -0700 Shannon Nelson <[email protected]>…

Add support for version 2 of the ioatdma device. This device handles the descriptor chain and DCA services slightly differently:
- Instead of moving the dma descriptors between a busy and

Quoted 10 lines

an idle chain,

Quoted 1 line

this new version uses a single circular chain so that we

don't have

Quoted 1 line

rewrite the next_descriptor pointers as we add new

requests, and the

Quoted 2 lines

device doesn't need to re-read the last descriptor.
- The new device has the DCA tags defined internally

instead of needing

Quoted 2 lines

them defined statically.

Andrew,

Thank you for your time and the review comments. I'll attend to the details and repost.

sln