initial displaylink kms driver + drm hotplug

This series contains some changes to the drm to allow for better hotplug support for usb devices, along with the initial displaylink KMS driver.

This relies on an fbdev patch that is in fbdev-next from Kay Sievers. (remove sysfs framebuffer device with USB .disconnect)

The initial changes just allow us to disconnect userspace/sysfs stuff properly then on close of the device we clean up things properly.

This and some other patches it relies on are in my pfdo git tree udl-unplug branch.

Dave.

From: Dave Airlie <[email protected]>

This adds an implementation for the busid callback so userspace can distinguish usb devices better.

---
 drivers/gpu/drm/drm_usb.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

index 445003f..ca77aa6 100644
@@ -85,7 +85,39 @@ static const char *drm_usb_get_name(struct drm_device *dev)
 static int drm_usb_set_busid(struct drm_device *dev,
 			       struct drm_master *master)
 {
+	struct usb_driver *udriver = dev->driver->kdriver.usb;
+	int len, ret;
+
+	master->unique_len = 40;
+	master->unique_size = master->unique_len;
+	master->unique = kmalloc(master->unique_size, GFP_KERNEL);
+	if (master->unique == NULL)
+		return -ENOMEM;
+
+	len = snprintf(master->unique, master->unique_len,
+		       "usb:%s", dev_name(&dev->usbdev->dev));
+
+	if (len >= master->unique_len) {
+		DRM_ERROR("buffer overflow");
+		ret = -EINVAL;
+		goto err;
+	} else
+		master->unique_len = len;
+
+	dev->devname = kmalloc(strlen(udriver->name) +
+			       master->unique_len + 2, GFP_KERNEL);
+
+	if (dev->devname == NULL) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	sprintf(dev->devname, "%s@%s", udriver->name,
+		master->unique);
+
 	return 0;
+err:
+	return ret;
 }
 
 static struct drm_bus drm_usb_bus = {

1.7.6

Hi Dave,

Is it really worth it to add such an interface from the old days for new drivers? I would assume that userspace has better ways to distinguish devices thanks to sysfs.

Just my 2 cents. If there are good reasons to do so I'm interested in the reasons.

Thanks
Sascha

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

From: Dave Airlie <[email protected]>

a step towards correct hot unplug for USB devices, we need to remove the userspace facing bits at the unplug time for correct udev operation.

---
 drivers/gpu/drm/drm_sysfs.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

index 62c3675..5a7bd51 100644
@@ -454,6 +454,8 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
 {
 	int i;
 
+	if (!connector->kdev.parent)
+		return;
 	DRM_DEBUG("removing \"%s\" from sysfs\n",
 		  drm_get_connector_name(connector));
 

@@ -461,6 +463,7 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
 		device_remove_file(&connector->kdev, &connector_attrs[i]);
 	sysfs_remove_bin_file(&connector->kdev.kobj, &edid_attr);
 	device_unregister(&connector->kdev);
+	connector->kdev.parent = NULL;
 }
 EXPORT_SYMBOL(drm_sysfs_connector_remove);
 

@@ -533,7 +536,9 @@ err_out:
  */
 void drm_sysfs_device_remove(struct drm_minor *minor)
 {
-	device_unregister(&minor->kdev);
+	if (minor->kdev.parent)
+		device_unregister(&minor->kdev);
+	minor->kdev.parent = NULL;
 }
 
 

1.7.6

The lack of any WARN_ON(mutex_is_locked) here and the funny comment about the sysfs vs. hotunplug deadlock in the next patch makes me a bit uneasy.

I think the only issue race we need to care about here is a hotremove vs. module unload race. Everything else should stay around for long enough (although this might not quite be true with the current code ...). Simply add a dev->hotremove_mutex and grabbing it in these two functions should fix this.

It's a bit a funky locking scheme, but imo it should keep on working even when properly fix up all our setup/teardown issues:
- setup just needs to properly order ctors, i.e. publish the userspace interfaces only after everything is properly set up - we don't need any locking to achieve that (well, currently we still rely massively on the global_mutex dutcttape).
- concurrent removal of the userspace interfaces (like here) would be protected by the new hotremove_mutex lock.
- protection against the underlying device disappearing would be handled by dev->unplugged (I have some more comments about that)
- and the actual drm_device teardown controlled by reference-counting. Currently we get away with abusing the fd open count, but I think sooner or later we want a real reference count (e.g. when exporting buffers with dma_buf we can't just reap them when the last user disappears).

Cheers, Daniel

Daniel Vetter
Mail: [email protected]
Mobile: +41 (0)79 365 57 48

From: Dave Airlie <[email protected]>

In order to get correct ordering at hot-unplug for userspace, we need to tear down all the sysfs bits at the correct time.

This adds a helper to allow drivers to remove the sysfs nodes for all connectors.

---
 drivers/gpu/drm/drm_crtc.c |   11 +++++++++++
 include/drm/drm_crtc.h     |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

index 2c27a2e..e6a2a3e 100644
@@ -523,6 +523,17 @@ void drm_connector_cleanup(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_cleanup);
 
+void drm_connector_unplug_all(struct drm_device *dev)
+{
+	struct drm_connector *connector;
+
+	/* taking the mode config mutex ends up in a clash with sysfs */
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+		drm_sysfs_connector_remove(connector);
+
+}
+EXPORT_SYMBOL(drm_connector_unplug_all);
+
 void drm_encoder_init(struct drm_device *dev,
 		      struct drm_encoder *encoder,
 		      const struct drm_encoder_funcs *funcs,

index 783322b..927e31b 100644
@@ -825,6 +825,8 @@ extern void drm_connector_init(struct drm_device *dev,
 			    int connector_type);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
+/* helper to unplug all connectors from sysfs for device */
+extern void drm_connector_unplug_all(struct drm_device *dev);
 
 extern void drm_encoder_init(struct drm_device *dev,
 			     struct drm_encoder *encoder,

1.7.6

From: Dave Airlie <[email protected]>

Two parts to this, one is simple unplug from sysfs for the device node.

The second adds an unplugged state, if we have device opens, we just set the unplugged state and return, if we have no device opens we drop the drm device.

If after a lastclose we discover we are unplugged we then drop the drm device.

---
 drivers/gpu/drm/drm_fops.c |    2 ++
 drivers/gpu/drm/drm_stub.c |   22 ++++++++++++++++++++++
 include/drm/drmP.h         |    2 ++
 3 files changed, 26 insertions(+), 0 deletions(-)

index 6263b01..154cfac 100644
@@ -579,6 +579,8 @@ int drm_release(struct inode *inode, struct file *filp)
 			retcode = -EBUSY;
 		} else
 			retcode = drm_lastclose(dev);
+		if (dev->unplugged)
+			drm_put_dev(dev);
 	}
 	mutex_unlock(&drm_global_mutex);
 

index 6d7b083..b04c92d 100644
@@ -429,6 +429,11 @@ int drm_put_minor(struct drm_minor **minor_p)
 	return 0;
 }
 
+static void drm_unplug_minor(struct drm_minor *minor)
+{
+	drm_sysfs_device_remove(minor);
+}
+
 /**
  * Called via drm_exit() at module unload time or when pci device is
  * unplugged.

@@ -492,3 +497,20 @@ void drm_put_dev(struct drm_device *dev)
 	kfree(dev);
 }
 EXPORT_SYMBOL(drm_put_dev);
+
+void drm_unplug_dev(struct drm_device *dev)
+{
+	/* for a USB device */
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		drm_unplug_minor(dev->control);
+	drm_unplug_minor(dev->primary);
+
+	dev->unplugged = true;
+
+	mutex_lock(&drm_global_mutex);
+	if (dev->open_count == 0) {
+		drm_put_dev(dev);
+	}
+	mutex_unlock(&drm_global_mutex);
+}
+EXPORT_SYMBOL(drm_unplug_dev);

index 92f0981..8c11797 100644
@@ -1170,6 +1170,7 @@ struct drm_device {
 	struct idr object_name_idr;
 	/*@} */
 	int switch_power_state;
+	bool unplugged; /* device has been unplugged or gone away */
 };
 
 #define DRM_SWITCH_POWER_ON 0

@@ -1464,6 +1465,7 @@ extern void drm_master_put(struct drm_master **master);
 
 extern void drm_put_dev(struct drm_device *dev);
 extern int drm_put_minor(struct drm_minor **minor);
+extern void drm_unplug_dev(struct drm_device *dev);
 extern unsigned int drm_debug;
 
 extern unsigned int drm_vblank_offdelay;

1.7.6

I like the unplugged state to handle whole-device removal, but I think this needs a few extensions:
- I think we should unconditionally unregister the drm dev minors. Simply checking dev->unplugged should do the trick because we can't just tear down the minor structs (we can only unregister the dev node).
- writing to dev->unplugged needs to be protected by the same mutex (i.e. drm_global_mutex) as the actual unregister step.
- I think we should add a simply drm_device_is_unplugged so that drivers can check the unplugged state in a race-free manner. Adding an smp_wmb() after writing to dev->unplugged and an smb_rmb() before reading it in that helper (plus ensuring that dev->unplugged is a separate word in drm_device) should do the trick without overhead.
- I think we wan't the drm core to return -ENODEV for any ioctls an open calls and return a SIGBUS on the fault handler. Currently you roll that yourself in udl, but I can't think of a use-case where we want to stick the interfaces around after a hotremove of the entire device.

Cheers, Daniel

Daniel Vetter
Mail: [email protected]
Mobile: +41 (0)79 365 57 48

This is an initial drm/kms driver for the displaylink devices.

Supports fb_defio,
supports KMS dumb interface
supports 24bpp via conversion to 16bpp, hw can do this better. supports hot unplug using new drm core features.

On an unplug, it disables connector polling, unplugs connectors from sysfs, unplugs fbdev layer (using Kay's API), drops all the USB device URBs, and call the drm core to unplug the device.

This driver is based in large parts on udlfb.c so I've licensed it under GPLv2.

---
 drivers/gpu/drm/Kconfig             |    1 +
 drivers/gpu/drm/Makefile            |    1 +
 drivers/gpu/drm/udl/Kconfig         |   10 +
 drivers/gpu/drm/udl/Makefile        |    6 +
 drivers/gpu/drm/udl/udl_connector.c |  141 ++++++++
 drivers/gpu/drm/udl/udl_drv.c       |   99 ++++++
 drivers/gpu/drm/udl/udl_drv.h       |  141 ++++++++
 drivers/gpu/drm/udl/udl_encoder.c   |   80 +++++
 drivers/gpu/drm/udl/udl_fb.c        |  619 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/udl/udl_gem.c       |  227 +++++++++++++
 drivers/gpu/drm/udl/udl_main.c      |  338 +++++++++++++++++++
 drivers/gpu/drm/udl/udl_modeset.c   |  414 +++++++++++++++++++++++
 drivers/gpu/drm/udl/udl_transfer.c  |  253 ++++++++++++++
 13 files changed, 2330 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpu/drm/udl/Kconfig
 create mode 100644 drivers/gpu/drm/udl/Makefile
 create mode 100644 drivers/gpu/drm/udl/udl_connector.c
 create mode 100644 drivers/gpu/drm/udl/udl_drv.c
 create mode 100644 drivers/gpu/drm/udl/udl_drv.h
 create mode 100644 drivers/gpu/drm/udl/udl_encoder.c
 create mode 100644 drivers/gpu/drm/udl/udl_fb.c
 create mode 100644 drivers/gpu/drm/udl/udl_gem.c
 create mode 100644 drivers/gpu/drm/udl/udl_main.c
 create mode 100644 drivers/gpu/drm/udl/udl_modeset.c
 create mode 100644 drivers/gpu/drm/udl/udl_transfer.c

index 2418429..ed2f690 100644
@@ -165,3 +165,4 @@ source "drivers/gpu/drm/vmwgfx/Kconfig"
 
 source "drivers/gpu/drm/gma500/Kconfig"
 
+source "drivers/gpu/drm/udl/Kconfig"

index 0cde1b8..7f20297 100644
@@ -37,4 +37,5 @@ obj-$(CONFIG_DRM_VIA)	+=via/
 obj-$(CONFIG_DRM_NOUVEAU) +=nouveau/
 obj-$(CONFIG_DRM_EXYNOS) +=exynos/
 obj-$(CONFIG_DRM_GMA500) += gma500/
+obj-$(CONFIG_DRM_UDL) += udl/
 obj-y			+= i2c/

new file mode 100644
index 0000000..b4642c1
@@ -0,0 +1,10 @@
+config DRM_UDL
+	tristate "DisplayLink"
+	depends on DRM && USB
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	select DRM_KMS_HELPER
+	help
+	  This is a KMS driver for the USB displaylink video adapters.
+          Say M/Y to add support for these devices via drm/kms interfaces.

new file mode 100644
index 0000000..05c7481
@@ -0,0 +1,6 @@
+
+ccflags-y := -Iinclude/drm
+
+udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_encoder.o udl_main.o udl_fb.o udl_transfer.o udl_gem.o
+
+obj-$(CONFIG_DRM_UDL) := udl.o

1.7.6