[PATCH 3/10] udlfb: pre-allocated urb list helpers

[PATCH 3/10] udlfb: pre-allocated urb list helpers

[Bernie Thompson]

Add functions to pre-allocate and free usb bulk urbs for core render path.

Udlfb currently allocates a single urb, guarded by a mutex, that is a key
bottleneck. Because udlfb sends so much data, preallocation is most efficient.

Functions will be used by new rendering functions in later patches.

Signed-off-by: Bernie Thompson <bernie@plugable.com>
---
 drivers/staging/udlfb/udlfb.c |  192 ++++++++++++++++++++++++++++++++++++++++++
 drivers/staging/udlfb/udlfb.h |   35 +++++++
 2 files changed, 227 insertions(+)

Index: linux-next/drivers/staging/udlfb/udlfb.c
=================================--- linux-next.orig/drivers/staging/udlfb/udlfb.c	2010-02-14 20:26:49.000000000 -0800
+++ linux-next/drivers/staging/udlfb/udlfb.c	2010-02-14 20:26:52.000000000 -0800
@@ -51,6 +51,13 @@ static struct usb_device_id id_table[]  };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* dlfb keeps a list of urbs for efficient bulk transfers */
+static void dlfb_urb_completion(struct urb *urb);
+static struct urb *dlfb_get_urb(struct dlfb_data *dev);
+static int dlfb_submit_urb(struct dlfb_data *dev, struct urb * urb, size_t len);
+static int dlfb_alloc_urb_list(struct dlfb_data *dev, int count, size_t size);
+static void dlfb_free_urb_list(struct dlfb_data *dev);
+
 /*
  * Inserts a specific DisplayLink controller command into the provided
  * buffer.
@@ -901,6 +908,21 @@ static int dlfb_release(struct fb_info *
 	return 0;
 }
 
+/*
+ * Called when all client interfaces to start transactions have been disabled,
+ * and all references to our device instance (dlfb_data) are released.
+ * Every transaction must have a reference, so we know are fully spun down
+ */
+static void dlfb_delete(struct kref *kref)
+{
+	struct dlfb_data *dev = container_of(kref, struct dlfb_data, kref);
+
+	if (dev->backing_buffer)
+		vfree(dev->backing_buffer);
+
+	kfree(dev);
+}
+
 static int dlfb_blank(int blank_mode, struct fb_info *info)
 {
 	struct dlfb_data *dev_info = info->par;
@@ -974,6 +996,7 @@ static int dlfb_probe(struct usb_interfa
 
 	mutex_init(&dev->bulk_mutex);
 	dev->udev = usbdev;
+	dev->gdev = &usbdev->dev; /* our generic struct device * */
 	dev->interface = interface;
 	usb_set_intfdata(interface, dev);
 
@@ -1168,6 +1191,175 @@ static void __exit dlfb_exit(void)
 module_init(dlfb_init);
 module_exit(dlfb_exit);
 
+static void dlfb_urb_completion(struct urb *urb)
+{
+	struct urb_node *unode = urb->context;
+	struct dlfb_data *dev = unode->dev;
+	unsigned long flags;
+
+	/* sync/async unlink faults aren't errors */
+	if (urb->status) {
+		if (!(urb->status = -ENOENT ||
+		    urb->status = -ECONNRESET ||
+		    urb->status = -ESHUTDOWN)) {
+			dl_err("%s - nonzero write bulk status received: %d\n",
+				__func__, urb->status);
+			atomic_set(&dev->lost_pixels, 1);
+		}
+	}
+
+	urb->transfer_buffer_length = dev->urbs.size; /* reset to actual */
+
+	spin_lock_irqsave(&dev->urbs.lock, flags);
+	list_add_tail(&unode->entry, &dev->urbs.list);
+	dev->urbs.available++;
+	spin_unlock_irqrestore(&dev->urbs.lock, flags);
+
+	up(&dev->urbs.limit_sem);
+}
+
+static void dlfb_free_urb_list(struct dlfb_data *dev)
+{
+	int count = dev->urbs.count;
+	struct list_head *node;
+	struct urb_node *unode;
+	struct urb *urb;
+	int ret;
+	unsigned long flags;
+
+	dl_notice("Waiting for completes and freeing all render urbs\n");
+
+	/* keep waiting and freeing, until we've got 'em all */
+	while (count--) {
+		/* Timeout means a memory leak and/or fault */
+		ret = down_timeout(&dev->urbs.limit_sem, FREE_URB_TIMEOUT);
+		if (ret) {
+			BUG_ON(ret);
+			break;
+		}
+		spin_lock_irqsave(&dev->urbs.lock, flags);
+
+		node = dev->urbs.list.next; /* have reserved one with sem */
+		list_del_init(node);
+
+		spin_unlock_irqrestore(&dev->urbs.lock, flags);
+
+		unode = list_entry(node, struct urb_node, entry);
+		urb = unode->urb;
+
+		/* Free each separately allocated piece */
+		usb_buffer_free(urb->dev, dev->urbs.size,
+			urb->transfer_buffer, urb->transfer_dma);
+		usb_free_urb(urb);
+		kfree(node);
+	}
+
+	kref_put(&dev->kref, dlfb_delete);
+
+}
+
+static int dlfb_alloc_urb_list(struct dlfb_data *dev, int count, size_t size)
+{
+	int i = 0;
+	struct urb *urb;
+	struct urb_node *unode;
+	char *buf;
+
+	spin_lock_init(&dev->urbs.lock);
+
+	dev->urbs.size = size;
+	INIT_LIST_HEAD(&dev->urbs.list);
+
+	while (i < count) {
+		unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
+		if (!unode)
+			break;
+		unode->dev = dev;
+
+		urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!urb) {
+			kfree(unode);
+			break;
+		}
+		unode->urb = urb;
+
+		buf = usb_buffer_alloc(dev->udev, MAX_TRANSFER, GFP_KERNEL,
+					&urb->transfer_dma);
+		if (!buf) {
+			kfree(unode);
+			usb_free_urb(urb);
+			break;
+		}
+
+		/* urb->transfer_buffer_length set to actual before submit */
+		usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 1),
+			buf, size, dlfb_urb_completion, unode);
+		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+
+		list_add_tail(&unode->entry, &dev->urbs.list);
+
+		i++;
+	}
+
+	sema_init(&dev->urbs.limit_sem, i);
+	dev->urbs.count = i;
+	dev->urbs.available = i;
+
+	kref_get(&dev->kref); /* released in free_render_urbs() */
+
+	dl_notice("allocated %d %d byte urbs \n", i, (int) size);
+
+	return i;
+}
+
+static struct urb *dlfb_get_urb(struct dlfb_data *dev)
+{
+	int ret = 0;
+	struct list_head *entry;
+	struct urb_node *unode;
+	struct urb *urb = NULL;
+	unsigned long flags;
+
+	/* Wait for an in-flight buffer to complete and get re-queued */
+	ret = down_timeout(&dev->urbs.limit_sem, GET_URB_TIMEOUT);
+	if (ret) {
+		atomic_set(&dev->lost_pixels, 1);
+		dl_err("wait for urb interrupted: %x\n", ret);
+		goto error;
+	}
+
+	spin_lock_irqsave(&dev->urbs.lock, flags);
+
+	BUG_ON(list_empty(&dev->urbs.list)); /* reserved one with limit_sem */
+	entry = dev->urbs.list.next;
+	list_del_init(entry);
+	dev->urbs.available--;
+
+	spin_unlock_irqrestore(&dev->urbs.lock, flags);
+
+	unode = list_entry(entry, struct urb_node, entry);
+	urb = unode->urb;
+
+error:
+	return urb;
+}
+
+static int dlfb_submit_urb(struct dlfb_data *dev, struct urb *urb, size_t len)
+{
+	int ret;
+
+	BUG_ON(len > dev->urbs.size);
+
+	urb->transfer_buffer_length = len; /* set to actual payload len */
+	ret = usb_submit_urb(urb, GFP_KERNEL);
+	if (ret) {
+		dlfb_urb_completion(urb); /* because no one else will */
+		atomic_set(&dev->lost_pixels, 1);
+		dl_err("usb_submit_urb error %x\n", ret);
+	}
+	return ret;
+}
+
 MODULE_AUTHOR("Roberto De Ioris <roberto@unbit.it>, "
 	      "Jaya Kumar <jayakumar.lkml@gmail.com>");
 MODULE_DESCRIPTION(DRIVER_VERSION);
Index: linux-next/drivers/staging/udlfb/udlfb.h
=================================--- linux-next.orig/drivers/staging/udlfb/udlfb.h	2010-02-14 20:26:49.000000000 -0800
+++ linux-next/drivers/staging/udlfb/udlfb.h	2010-02-14 20:26:52.000000000 -0800
@@ -5,16 +5,35 @@
 #define BUF_HIGH_WATER_MARK	1024
 #define BUF_SIZE		(64*1024)
 
+struct urb_node {
+	struct list_head entry;
+	struct dlfb_data *dev;
+	struct urb *urb;
+};
+
+struct urb_list {
+	struct list_head list;
+	spinlock_t lock;
+	struct semaphore limit_sem;
+	int available;
+	int count;
+	size_t size;
+};
+
 struct dlfb_data {
 	struct usb_device *udev;
+	struct device *gdev; /* &udev->dev */
 	struct usb_interface *interface;
 	struct urb *tx_urb, *ctrl_urb;
 	struct usb_ctrlrequest dr;
 	struct fb_info *info;
+	struct urb_list urbs;
+	struct kref kref;
 	char *buf;
 	char *bufend;
 	char *backing_buffer;
 	struct mutex bulk_mutex;
+	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
 	char edid[128];
 	int screen_size;
 	int line_length;
@@ -29,6 +48,14 @@ struct dlfb_data {
 #define NR_USB_REQUEST_I2C_SUB_IO 0x02
 #define NR_USB_REQUEST_CHANNEL 0x12
 
+/* -BULK_SIZE as per usb-skeleton. Can we get full page and avoid overhead? */
+#define BULK_SIZE 512
+#define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
+#define WRITES_IN_FLIGHT (4)
+
+#define GET_URB_TIMEOUT	HZ
+#define FREE_URB_TIMEOUT (HZ*2)
+
 static void dlfb_bulk_callback(struct urb *urb)
 {
 	struct dlfb_data *dev_info = urb->context;
@@ -72,4 +99,12 @@ static int dlfb_bulk_msg(struct dlfb_dat
 
 #define dlfb_set_register insert_command
 
+#define dl_err(format, arg...) \
+	dev_err(dev->gdev, "dlfb: " format, ## arg)
+#define dl_warn(format, arg...) \
+	dev_warn(dev->gdev, "dlfb: " format, ## arg)
+#define dl_notice(format, arg...) \
+	dev_notice(dev->gdev, "dlfb: " format, ## arg)
+#define dl_info(format, arg...) \
+	dev_info(dev->gdev, "dlfb: " format, ## arg)
 #endif

Re: [PATCH 3/10] udlfb: pre-allocated urb list helpers

[Greg KH]

On Mon, Feb 15, 2010 at 06:45:55AM -0800, Bernie Thompson wrote:
> Add functions to pre-allocate and free usb bulk urbs for core render path.
> 
> Udlfb currently allocates a single urb, guarded by a mutex, that is a key
> bottleneck. Because udlfb sends so much data, preallocation is most efficient.

I'm not going to reject this patch, but are you sure about this being
needed?  The code path for creating a new urb is very tiny, just a
memory allocation.  Is that really noticable in any benchmarks or cpu
usage that you have found?

curious,

greg k-h

Re: [PATCH 3/10] udlfb: pre-allocated urb list helpers

[Bernie Thompson]

Hi Greg,

On Thu, Feb 18, 2010 at 7:54 AM, Greg KH <greg@kroah.com> wrote:
> I'm not going to reject this patch, but are you sure about this being
> needed?  The code path for creating a new urb is very tiny, just a
> memory allocation.  Is that really noticable in any benchmarks or cpu
> usage that you have found?

You're definitely right, from a performance standpoint, allocating a
fresh urb/buffer each
transfer itself wouldn't be a problem. The big perf win here, over the
older udlfb code, is
the asynchronous dispatch and being able to have several urbs in flight
at once, not the pre-allocation itself.

I actually implemented it first with alloc/free for each transfer
(http://git.plugable.com/gitphp/index.php?p=udlfb&a=commit&h
4fa3b22580fd1532d06292e9061b9b2057f6a6),
but freeing the associated buffer during completion generated WARN_ONs
during each transfer.

Google background on the problem I hit (lots of others hitting, too):
http://www.google.com/search?q=Linux+WARN_ON+dma_free_coherent

I thought about working around by queuing up a deferred op to free
buffers outside of interrupt context, but that raised overhead
concerns and leak concerns.

So that led to the current udlfb implementation, which is a pretty
common and efficient pattern, in the drivers I've known.  And it's
well tested at this point.

usb-skel needs to also deal with this same WARN_ON issue, as others
are hitting this same problem -- something like the udlfb
implementation (using internal list anchor, rather than externally
allocated one) may be a good way to go.

Best wishes,
Bernie

Re: [PATCH 3/10] udlfb: pre-allocated urb list helpers

[Greg KH]

On Thu, Feb 18, 2010 at 02:32:35PM -0800, Bernie Thompson wrote:
> Hi Greg,
> 
> On Thu, Feb 18, 2010 at 7:54 AM, Greg KH <greg@kroah.com> wrote:
> > I'm not going to reject this patch, but are you sure about this being
> > needed?  The code path for creating a new urb is very tiny, just a
> > memory allocation.  Is that really noticable in any benchmarks or cpu
> > usage that you have found?
> 
> You're definitely right, from a performance standpoint, allocating a
> fresh urb/buffer each
> transfer itself wouldn't be a problem. The big perf win here, over the
> older udlfb code, is
> the asynchronous dispatch and being able to have several urbs in flight
> at once, not the pre-allocation itself.
> 
> I actually implemented it first with alloc/free for each transfer
> (http://git.plugable.com/gitphp/index.php?p=udlfb&a=commit&h
4fa3b22580fd1532d06292e9061b9b2057f6a6),
> but freeing the associated buffer during completion generated WARN_ONs
> during each transfer.
> 
> Google background on the problem I hit (lots of others hitting, too):
> http://www.google.com/search?q=Linux+WARN_ON+dma_free_coherent

Ick, that's not good.

> I thought about working around by queuing up a deferred op to free
> buffers outside of interrupt context, but that raised overhead
> concerns and leak concerns.

You can just have the urb framework purge the memory when it is finished
automatically, right?  What's wrong with that, it should work for what
you need.

> So that led to the current udlfb implementation, which is a pretty
> common and efficient pattern, in the drivers I've known.  And it's
> well tested at this point.

Yeah, because it's a common pattern, it either:
	- needs to be in the usb core so people don't keep duplicating
	  it everywhere
	- removed because there is a better and simpler way.

Right now I'm thinking the latter, as the urb can be dynamically handled
including the buffer attached to it.  We also have urb "anchors" to
handle disconnecting devices for dynamic urbs, so this should all be
handled.

But we can discuss this on the linux-usb list, not here :)

thanks,

greg k-h

Re: [PATCH 3/10] udlfb: pre-allocated urb list helpers

[Bernie Thompson]

Hi Greg,

On Thu, Feb 18, 2010 at 4:36 PM, Greg KH <gregkh@suse.de> wrote:
> You can just have the urb framework purge the memory when it is finished
> automatically, right?  What's wrong with that, it should work for what
> you need.

I don't know. Is there a similar driver that's a good example to follow?

Thanks,
Bernie

Re: [PATCH 3/10] udlfb: pre-allocated urb list helpers

[Greg KH]

On Thu, Feb 18, 2010 at 06:22:11PM -0800, Bernie Thompson wrote:
> Hi Greg,
> 
> On Thu, Feb 18, 2010 at 4:36 PM, Greg KH <gregkh@suse.de> wrote:
> > You can just have the urb framework purge the memory when it is finished
> > automatically, right?  What's wrong with that, it should work for what
> > you need.
> 
> I don't know. Is there a similar driver that's a good example to follow?

Hm, I really don't know, I haven't looked in a while, sorry :(