Skip to contentSkip to conversation

drm: Introduce drm_connector_register_all() helper

PATCH Applied Alexey Brodkin 4 participants 13 messages Mar 23, 2016 – Mar 29, 2016

Subsystems (6)

v3 of 6 Applied Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v3 cover drm: Introduce drm_connector_register_all() helper

As a pair to already existing drm_connector_unplug_all()
(which we'll rename in this series to drm_connector_unregister_all()) we're adding generic implementation of what is already done in some drivers for registering all connectors.

After implementation of that new helper we're updating 2 drivers that used to use it's own implementation:
[1] atmel_hlcdc
[2] rcar_du

And one driver that uses unregister():
[1] udl

Other drivers still use load() callback and so should be first modified so their load() gets called from their probe() explicitly.

Build- and run-tested on yet to be upstreamed ARC PGU (part of AXS10x board).

Changes v2 -> v3:
* Added acks for 1, 3 and 4 patches
* Updated kerneldoc descriptins of both register_ and unregister_all()
* Updated commit messages (mostly spellos and grammar issues)

Changes v1 -> v2:
* Rename drm_connector_unplug_all() to drm_connector_unregister_all()
* Use drm_for_each_connector() instead of list_for_each_entry()
* Updated kerneldoc for drm_dev_register()

Alexey Brodkin (4):
drm: Rename drm_connector_unplug_all() to
drm_connector_unregister_all()
drm: Introduce drm_connector_register_all() helper
drm: atmel_hldc: Use generic drm_connector_register_all() helper
drm: rcar-du: Use generic drm_connector_register_all() helper

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 +-----------------
 drivers/gpu/drm/drm_crtc.c                   | 59 ++++++++++++++++++++++++----
 drivers/gpu/drm/drm_drv.c                    |  6 ++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c        | 14 +------
 drivers/gpu/drm/udl/udl_drv.c                |  2 +-
 include/drm/drm_crtc.h                       |  5 ++-
 6 files changed, 64 insertions(+), 61 deletions(-)
Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>
Cc Boris Brezillon <[email protected]>
Cc Laurent Pinchart <[email protected]>
Signature · 1 line
2.5.0
#1 lore raw

PATCH v3 1/4 drm: Rename drm_connector_unplug_all() to drm_connector_unregister_all()

Current name is a bit misleading because what that helper function really does it calls drm_connector_unregister() for all connectors.

This all has nothing to do with hotplugging so let's name things properly.

And while at it remove potentially dangerous locking around drm_connector_unregister() in rcar_du_remove() as mentioned in kerneldoc for drm_connector_unregister_all().

---

Changes v2 -> v3:
* Updated title with capital after colon
* Updated kerneldoc description of drm_connector_unregister_all() so that it will match description of register_all() to be introduced in the next change
* Added ack from Laurent

Changes v1 -> v2:
* This patch was only introduced in v2.

 drivers/gpu/drm/drm_crtc.c            | 18 +++++++++---------
 drivers/gpu/drm/rcar-du/rcar_du_drv.c |  5 +----
 drivers/gpu/drm/udl/udl_drv.c         |  2 +-
 include/drm/drm_crtc.h                |  4 ++--
 4 files changed, 13 insertions(+), 16 deletions(-)
drivers/gpu/drm/drm_crtc.c +9 -9
index 65258ac..65488a6 100644
@@ -1080,25 +1080,25 @@ void drm_connector_unregister(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_unregister);
 
-
 /**
- * drm_connector_unplug_all - unregister connector userspace interfaces
+ * drm_connector_unregister_all - unregister connector userspace interfaces
  * @dev: drm device
  *
- * This function unregisters all connector userspace interfaces in sysfs. Should
- * be call when the device is disconnected, e.g. from an usb driver's
- * ->disconnect callback.
+ * This functions unregisters all connectors from sysfs and other places so
+ * that userspace can no longer access them. Drivers should call this as the
+ * first step tearing down the device instace, or when the underlying
+ * physical device disappeared (e.g. USB unplug), right before calling
+ * drm_dev_unregister().
  */
-void drm_connector_unplug_all(struct drm_device *dev)
+void drm_connector_unregister_all(struct drm_device *dev)
 {
 	struct drm_connector *connector;
 
 	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+	drm_for_each_connector(connector, dev)
 		drm_connector_unregister(connector);
-
 }
-EXPORT_SYMBOL(drm_connector_unplug_all);
+EXPORT_SYMBOL(drm_connector_unregister_all);
 
 /**
  * drm_encoder_init - Init a preallocated encoder
drivers/gpu/drm/rcar-du/rcar_du_drv.c +1 -4
index ed6006b..644db36 100644
@@ -278,10 +278,7 @@ static int rcar_du_remove(struct platform_device *pdev)
 	struct rcar_du_device *rcdu = platform_get_drvdata(pdev);
 	struct drm_device *ddev = rcdu->ddev;
 
-	mutex_lock(&ddev->mode_config.mutex);
-	drm_connector_unplug_all(ddev);
-	mutex_unlock(&ddev->mode_config.mutex);
-
+	drm_connector_unregister_all(ddev);
 	drm_dev_unregister(ddev);
 
 	if (rcdu->fbdev)
drivers/gpu/drm/udl/udl_drv.c +1 -1
index 772ec9e..c204089 100644
@@ -94,7 +94,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
 	struct drm_device *dev = usb_get_intfdata(interface);
 
 	drm_kms_helper_poll_disable(dev);
-	drm_connector_unplug_all(dev);
+	drm_connector_unregister_all(dev);
 	udl_fbdev_unplug(dev);
 	udl_drop_usb(dev);
 	drm_unplug_dev(dev);
include/drm/drm_crtc.h +2 -2
index 8c7fb3d..42d9f4d 100644
@@ -2214,8 +2214,8 @@ void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 extern unsigned int drm_connector_index(struct drm_connector *connector);
-/* helper to unplug all connectors from sysfs for device */
-extern void drm_connector_unplug_all(struct drm_device *dev);
+/* helper to unregister all connectors from sysfs for device */
+extern void drm_connector_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
 extern void drm_bridge_remove(struct drm_bridge *bridge);
Signature · 1 line
2.5.0
Acked-by on v3 1/4
#2 lore raw

Re: [PATCH 1/4 v3] drm: Rename drm_connector_unplug_all() to drm_connector_unregister_all()

Quoted 6 lines · On Wed, 23 Mar 2016 11:42:54 +0300 Alexey Brodkin…

Current name is a bit misleading because what that helper function really does it calls drm_connector_unregister() for all connectors.

This all has nothing to do with hotplugging so let's name things properly.

Quoted 8 lines

And while at it remove potentially dangerous locking around drm_connector_unregister() in rcar_du_remove() as mentioned in kerneldoc for drm_connector_unregister_all().

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>
Cc Boris Brezillon <[email protected]>
Quoted 103 lines

---

Changes v2 -> v3:
* Updated title with capital after colon
* Updated kerneldoc description of drm_connector_unregister_all() so that it will match description of register_all() to be introduced in the next change
* Added ack from Laurent

Changes v1 -> v2:
* This patch was only introduced in v2.

 drivers/gpu/drm/drm_crtc.c            | 18 +++++++++---------
 drivers/gpu/drm/rcar-du/rcar_du_drv.c |  5 +----
 drivers/gpu/drm/udl/udl_drv.c         |  2 +-
 include/drm/drm_crtc.h                |  4 ++--
 4 files changed, 13 insertions(+), 16 deletions(-)
drivers/gpu/drm/drm_crtc.c +9 -9
index 65258ac..65488a6 100644
@@ -1080,25 +1080,25 @@ void drm_connector_unregister(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_unregister);
 
-
 /**
- * drm_connector_unplug_all - unregister connector userspace interfaces
+ * drm_connector_unregister_all - unregister connector userspace interfaces
  * @dev: drm device
  *
- * This function unregisters all connector userspace interfaces in sysfs. Should
- * be call when the device is disconnected, e.g. from an usb driver's
- * ->disconnect callback.
+ * This functions unregisters all connectors from sysfs and other places so
+ * that userspace can no longer access them. Drivers should call this as the
+ * first step tearing down the device instace, or when the underlying
+ * physical device disappeared (e.g. USB unplug), right before calling
+ * drm_dev_unregister().
  */
-void drm_connector_unplug_all(struct drm_device *dev)
+void drm_connector_unregister_all(struct drm_device *dev)
 {
 	struct drm_connector *connector;
 
 	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+	drm_for_each_connector(connector, dev)
 		drm_connector_unregister(connector);
-
 }
-EXPORT_SYMBOL(drm_connector_unplug_all);
+EXPORT_SYMBOL(drm_connector_unregister_all);
 
 /**
  * drm_encoder_init - Init a preallocated encoder
drivers/gpu/drm/rcar-du/rcar_du_drv.c +1 -4
index ed6006b..644db36 100644
@@ -278,10 +278,7 @@ static int rcar_du_remove(struct platform_device *pdev)
 	struct rcar_du_device *rcdu = platform_get_drvdata(pdev);
 	struct drm_device *ddev = rcdu->ddev;
 
-	mutex_lock(&ddev->mode_config.mutex);
-	drm_connector_unplug_all(ddev);
-	mutex_unlock(&ddev->mode_config.mutex);
-
+	drm_connector_unregister_all(ddev);
 	drm_dev_unregister(ddev);
 
 	if (rcdu->fbdev)
drivers/gpu/drm/udl/udl_drv.c +1 -1
index 772ec9e..c204089 100644
@@ -94,7 +94,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
 	struct drm_device *dev = usb_get_intfdata(interface);
 
 	drm_kms_helper_poll_disable(dev);
-	drm_connector_unplug_all(dev);
+	drm_connector_unregister_all(dev);
 	udl_fbdev_unplug(dev);
 	udl_drop_usb(dev);
 	drm_unplug_dev(dev);
include/drm/drm_crtc.h +2 -2
index 8c7fb3d..42d9f4d 100644
@@ -2214,8 +2214,8 @@ void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 extern unsigned int drm_connector_index(struct drm_connector *connector);
-/* helper to unplug all connectors from sysfs for device */
-extern void drm_connector_unplug_all(struct drm_device *dev);
+/* helper to unregister all connectors from sysfs for device */
+extern void drm_connector_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
 extern void drm_bridge_remove(struct drm_bridge *bridge);
Signature · 3 lines
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
#3 lore raw

PATCH v3 2/4 drm: Introduce drm_connector_register_all() helper

In reply to Alexey Brodkin

As a pair to already existing drm_connector_unregister_all() we're adding generic implementation of what is already done in some drivers.

Once this helper is implemented we'll be ready to switch existing driver-specific implementations with the generic one.

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>

---

Changes v2 -> v3:
* Updated title with capital after colon
* Simplified failure path with direct and unconditional invocation of unregister_all()
* Updated kerneldoc description of the drm_connector_register_all()

Changes v1 -> v2:
* Rename drm_connector_unplug_all() to drm_connector_unregister_all()
* Use drm_for_each_connector() instead of list_for_each_entry()
* Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c  |  6 +++++-
 include/drm/drm_crtc.h     |  3 ++-
 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_crtc.c +43 -0
index 65488a6..21eea11 100644
@@ -1081,6 +1081,49 @@ void drm_connector_unregister(struct drm_connector *connector)
 EXPORT_SYMBOL(drm_connector_unregister);
 
 /**
+ * drm_connector_register_all - register all connectors
+ * @dev: drm device
+ *
+ * This function registers all connectors in sysfs and other places so that
+ * userspace can start to access them. Drivers can call it after calling
+ * drm_dev_register() to complete the device registration, if they don't call
+ * drm_connector_register() on each connector individually.
+ *
+ * When a device is unplugged and should be removed from userspace access,
+ * call drm_connector_unregister_all(), which is the inverse of this
+ * function.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_connector_register_all(struct drm_device *dev)
+{
+	struct drm_connector *connector;
+	int ret;
+
+	mutex_lock(&dev->mode_config.mutex);
+
+	drm_for_each_connector(connector, dev) {
+		ret = drm_connector_register(connector);
+		if (ret) {
+			/*
+			 * We may safely call unregister_all() here within
+			 * area locked with mutex because unregister_all()
+			 * doesn't use locks inside (see a comment in that
+			 * function).
+			 */
+			drm_connector_unregister_all(dev);
+			return ret;
+		}
+	}
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_register_all);
+
+/**
  * drm_connector_unregister_all - unregister connector userspace interfaces
  * @dev: drm device
  *
drivers/gpu/drm/drm_drv.c +5 -1
index 167c8d3..2c9a2b6 100644
@@ -715,7 +715,11 @@ EXPORT_SYMBOL(drm_dev_unref);
  *
  * Register the DRM device @dev with the system, advertise device to user-space
  * and start normal device operation. @dev must be allocated via drm_dev_alloc()
- * previously.
+ * previously. Right after drm_dev_register() the driver should call
+ * drm_connector_register_all() to register all connectors in sysfs. This is
+ * a separate call for backward compatibility with drivers still using
+ * the deprecated ->load() callback, where connectors are registered from within
+ * the ->load() callback.
  *
  * Never call this twice on any device!
  *
include/drm/drm_crtc.h +2 -1
index 42d9f4d..6a34117 100644
@@ -2214,7 +2214,8 @@ void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 extern unsigned int drm_connector_index(struct drm_connector *connector);
-/* helper to unregister all connectors from sysfs for device */
+/* helpers to {un}register all connectors from sysfs for device */
+extern int drm_connector_register_all(struct drm_device *dev);
 extern void drm_connector_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
Signature · 1 line
2.5.0
#4 lore raw

Hi

Quoted 56 lines · Alexey Brodkin

As a pair to already existing drm_connector_unregister_all() we're adding generic implementation of what is already done in some drivers.

Once this helper is implemented we'll be ready to switch existing driver-specific implementations with the generic one.

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>

---

Changes v2 -> v3:
* Updated title with capital after colon
* Simplified failure path with direct and unconditional invocation of unregister_all()
* Updated kerneldoc description of the drm_connector_register_all()

Changes v1 -> v2:
* Rename drm_connector_unplug_all() to drm_connector_unregister_all()
* Use drm_for_each_connector() instead of list_for_each_entry()
* Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c  |  6 +++++-
 include/drm/drm_crtc.h     |  3 ++-
 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_crtc.c +21 -0
index 65488a6..21eea11 100644
@@ -1081,6 +1081,49 @@ void drm_connector_unregister(struct drm_connector *connector)
 EXPORT_SYMBOL(drm_connector_unregister);
 
 /**
+ * drm_connector_register_all - register all connectors
+ * @dev: drm device
+ *
+ * This function registers all connectors in sysfs and other places so that
+ * userspace can start to access them. Drivers can call it after calling
+ * drm_dev_register() to complete the device registration, if they don't call
+ * drm_connector_register() on each connector individually.
+ *
+ * When a device is unplugged and should be removed from userspace access,
+ * call drm_connector_unregister_all(), which is the inverse of this
+ * function.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_connector_register_all(struct drm_device *dev)
+{
+       struct drm_connector *connector;
+       int ret;
+
+       mutex_lock(&dev->mode_config.mutex);

drivers/gpu/drm/drm_crtc.c void drm_connector_unregister(struct drm_connector *connector) · line 1114

Quoted 10 lines
drivers/gpu/drm/drm_crtc.c +10 -0
+
+       drm_for_each_connector(connector, dev) {
+               ret = drm_connector_register(connector);
+               if (ret) {
+                       /*
+                        * We may safely call unregister_all() here within
+                        * area locked with mutex because unregister_all()
+                        * doesn't use locks inside (see a comment in that
+                        * function).
+                        */

Ugh? unregister_all() says:

/* FIXME: taking the mode config mutex ends up in a clash with sysfs */

This strongly contradicts your comment. Anyway, regardless how you want to fix it: You better unlock the mode-config mutex before returning below.

Thanks
David

Quoted 47 lines
drivers/gpu/drm/drm_crtc.c +12 -0
+                       drm_connector_unregister_all(dev);
+                       return ret;
+               }
+       }
+
+       mutex_unlock(&dev->mode_config.mutex);
+
+       return 0;
+}
+EXPORT_SYMBOL(drm_connector_register_all);
+
+/**
  * drm_connector_unregister_all - unregister connector userspace interfaces
  * @dev: drm device
  *
drivers/gpu/drm/drm_drv.c +5 -1
index 167c8d3..2c9a2b6 100644
@@ -715,7 +715,11 @@ EXPORT_SYMBOL(drm_dev_unref);
  *
  * Register the DRM device @dev with the system, advertise device to user-space
  * and start normal device operation. @dev must be allocated via drm_dev_alloc()
- * previously.
+ * previously. Right after drm_dev_register() the driver should call
+ * drm_connector_register_all() to register all connectors in sysfs. This is
+ * a separate call for backward compatibility with drivers still using
+ * the deprecated ->load() callback, where connectors are registered from within
+ * the ->load() callback.
  *
  * Never call this twice on any device!
  *
include/drm/drm_crtc.h +2 -2
index 42d9f4d..6a34117 100644
@@ -2214,7 +2214,8 @@ void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 extern unsigned int drm_connector_index(struct drm_connector *connector);
-/* helper to unregister all connectors from sysfs for device */
+/* helpers to {un}register all connectors from sysfs for device */
+extern int drm_connector_register_all(struct drm_device *dev);
 extern void drm_connector_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
--
Quoted 2 lines

2.5.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#5 lore raw

Hi David,

Quoted 73 lines · David Herrmann

Hi

Quoted 59 lines · Alexey Brodkin

As a pair to already existing drm_connector_unregister_all() we're adding generic implementation of what is already done in some drivers.

Once this helper is implemented we'll be ready to switch existing driver-specific implementations with the generic one.

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>

---

Changes v2 -> v3:
 * Updated title with capital after colon
 * Simplified failure path with direct and unconditional invocation of unregister_all()
 * Updated kerneldoc description of the drm_connector_register_all()

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 43 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- drivers/gpu/drm/drm_drv.c  |  6 +-+-+-+-+--
 include/drm/drm_crtc.h     |  3 +-+--

 3 files changed, 50 insertions(+-), 2 deletions(-)
drivers/gpu/drm/drm_crtc.c +1 -1
index 65488a6..21eea11 100644
--- a/drivers/gpu/drm/drm_crtc.c
+-+-+- b/drivers/gpu/drm/drm_crtc.c

@@ -1081,6 +-1081,49 @@ void drm_connector_unregister(struct drm_connector *connector) EXPORT_SYMBOL(drm_connector_unregister);

 /**

+- * drm_connector_register_all - register all connectors
+- * @dev: drm device
+- *
+- * This function registers all connectors in sysfs and other places so that
+- * userspace can start to access them. Drivers can call it after calling
+- * drm_dev_register() to complete the device registration, if they don't call
+- * drm_connector_register() on each connector individually.
+- *
+- * When a device is unplugged and should be removed from userspace access,
+- * call drm_connector_unregister_all(), which is the inverse of this
+- * function.
+- *
+- * Returns:
+- * Zero on success, error code on failure.
+- */
+-int drm_connector_register_all(struct drm_device *dev)
+-{
+-       struct drm_connector *connector;
+-       int ret;
+-
+-       mutex_lock(&dev->mode_config.mutex);
+-
+-       drm_for_each_connector(connector, dev) {
Quoted 8 lines
+-               ret = drm_connector_register(connector);
+-               if (ret) {
+-                       /*
+-                        * We may safely call unregister_all() here within
+-                        * area locked with mutex because unregister_all()
+-                        * doesn't use locks inside (see a comment in that
+-                        * function).
+-                        */

Ugh? unregister_all() says:

Quoted 5 lines

/* FIXME: taking the mode config mutex ends up in a clash with sysfs */

This strongly contradicts your comment. Anyway, regardless how you want to fix it: You better unlock the mode-config mutex before returning below.

So good catch.
But what I really meant since we didn't get any further after registering all "good" connections (see we're not releasing mutex still) the will be no clashes with sysfs.

Still I;d like Daniel to comment on that separately.

-Alexey

#6 lore raw
Quoted 82 lines · Alexey Brodkin

Hi David,

Quoted 73 lines · David Herrmann

Hi

Quoted 57 lines · Alexey Brodkin

As a pair to already existing drm_connector_unregister_all() we're adding generic implementation of what is already done in some drivers.

Once this helper is implemented we'll be ready to switch existing driver-specific implementations with the generic one.

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>

---

Changes v2 -> v3:
 * Updated title with capital after colon
 * Simplified failure path with direct and unconditional invocation of unregister_all()
 * Updated kerneldoc description of the drm_connector_register_all()

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c  |  6 +++++-
 include/drm/drm_crtc.h     |  3 ++-

 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_crtc.c +21 -0
index 65488a6..21eea11 100644
@@ -1081,6 +1081,49 @@ void drm_connector_unregister(struct drm_connector *connector)
  EXPORT_SYMBOL(drm_connector_unregister);
 
  /**
+ * drm_connector_register_all - register all connectors
+ * @dev: drm device
+ *
+ * This function registers all connectors in sysfs and other places so that
+ * userspace can start to access them. Drivers can call it after calling
+ * drm_dev_register() to complete the device registration, if they don't call
+ * drm_connector_register() on each connector individually.
+ *
+ * When a device is unplugged and should be removed from userspace access,
+ * call drm_connector_unregister_all(), which is the inverse of this
+ * function.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_connector_register_all(struct drm_device *dev)
+{
+       struct drm_connector *connector;
+       int ret;
+
+       mutex_lock(&dev->mode_config.mutex);

drivers/gpu/drm/drm_crtc.c void drm_connector_unregister(struct drm_connector *connector) · line 1114

Quoted 10 lines
drivers/gpu/drm/drm_crtc.c +10 -0
+
+       drm_for_each_connector(connector, dev) {
+               ret = drm_connector_register(connector);
+               if (ret) {
+                       /*
+                        * We may safely call unregister_all() here within
+                        * area locked with mutex because unregister_all()
+                        * doesn't use locks inside (see a comment in that
+                        * function).
+                        */

Ugh? unregister_all() says:

Quoted 5 lines

/* FIXME: taking the mode config mutex ends up in a clash with sysfs */

This strongly contradicts your comment. Anyway, regardless how you want to fix it: You better unlock the mode-config mutex before returning below.

Quoted 6 lines

So good catch.
But what I really meant since we didn't get any further after registering all "good" connections (see we're not releasing mutex still) the will be no clashes with sysfs.

Still I;d like Daniel to comment on that separately.

I think doing the unregister_all outside of the loop&locked section is better for future-proofing. My long-term plan for connector lifetimes and the connector list is:
- refcounting for connectors (Dave has wip patches already).
- separate lock for the connector list (and only that).

Doing it entirely separate would make things easier and more robust.

I merged patch 1 meanwhile to drm-misc.

Thanks, Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#7 lore raw

Hi Daniel,

Quoted 90 lines · Daniel Vetter
Quoted 89 lines · Alexey Brodkin

Hi David,

Quoted 75 lines · David Herrmann

Hi

Quoted 60 lines · Alexey Brodkin

As a pair to already existing drm_connector_unregister_all() we're adding generic implementation of what is already done in some drivers.

Once this helper is implemented we'll be ready to switch existing driver-specific implementations with the generic one.

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>

---

Changes v2 -> v3:
 * Updated title with capital after colon
 * Simplified failure path with direct and unconditional invocation of unregister_all()
 * Updated kerneldoc description of the drm_connector_register_all()

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 43 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- drivers/gpu/drm/drm_drv.c  |  6 +-+-+-+-+--
 include/drm/drm_crtc.h     |  3 +-+--

 3 files changed, 50 insertions(+-), 2 deletions(-)
drivers/gpu/drm/drm_crtc.c +1 -1
index 65488a6..21eea11 100644
--- a/drivers/gpu/drm/drm_crtc.c
+-+-+- b/drivers/gpu/drm/drm_crtc.c

@@ -1081,6 +-1081,49 @@ void drm_connector_unregister(struct drm_connector *connector) EXPORT_SYMBOL(drm_connector_unregister);

 /**

+- * drm_connector_register_all - register all connectors
+- * @dev: drm device
+- *
+- * This function registers all connectors in sysfs and other places so that
+- * userspace can start to access them. Drivers can call it after calling
+- * drm_dev_register() to complete the device registration, if they don't call
+- * drm_connector_register() on each connector individually.
+- *
+- * When a device is unplugged and should be removed from userspace access,
+- * call drm_connector_unregister_all(), which is the inverse of this
+- * function.
+- *
+- * Returns:
+- * Zero on success, error code on failure.
+- */
+-int drm_connector_register_all(struct drm_device *dev)
+-{
+-       struct drm_connector *connector;
+-       int ret;
+-
+-       mutex_lock(&dev->mode_config.mutex);
+-
+-       drm_for_each_connector(connector, dev) {
Quoted 8 lines
+-               ret = drm_connector_register(connector);
+-               if (ret) {
+-                       /*
+-                        * We may safely call unregister_all() here within
+-                        * area locked with mutex because unregister_all()
+-                        * doesn't use locks inside (see a comment in that
+-                        * function).
+-                        */

Ugh? unregister_all() says:

Quoted 5 lines

/* FIXME: taking the mode config mutex ends up in a clash with sysfs */

This strongly contradicts your comment. Anyway, regardless how you want to fix it: You better unlock the mode-config mutex before returning below.

So good catch.
But what I really meant since we didn't get any further after registering all "good" connections (see we're not releasing mutex still) the will be no clashes with sysfs.

Quoted 8 lines
Quoted 1 line

Still I;d like Daniel to comment on that separately.

I think doing the unregister_all outside of the loop&locked section is better for future-proofing. My long-term plan for connector lifetimes and the connector list is:
- refcounting for connectors (Dave has wip patches already).
- separate lock for the connector list (and only that).

Doing it entirely separate would make things easier and more robust.

Ok makes sense.

Quoted 1 line

I merged patch 1 meanwhile to drm-misc.

So may I re-send only patches 2-4 then (using "drm-misc" as a base)?

-Alexey

#8 lore raw
Quoted 100 lines · Alexey Brodkin

Hi Daniel,

Quoted 97 lines · Daniel Vetter
Quoted 84 lines · Alexey Brodkin

Hi David,

Quoted 75 lines · David Herrmann

Hi

Quoted 58 lines · Alexey Brodkin

As a pair to already existing drm_connector_unregister_all() we're adding generic implementation of what is already done in some drivers.

Once this helper is implemented we'll be ready to switch existing driver-specific implementations with the generic one.

Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>

---

Changes v2 -> v3:
 * Updated title with capital after colon
 * Simplified failure path with direct and unconditional invocation of unregister_all()
 * Updated kerneldoc description of the drm_connector_register_all()

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c  |  6 +++++-
 include/drm/drm_crtc.h     |  3 ++-

 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_crtc.c +21 -0
index 65488a6..21eea11 100644
@@ -1081,6 +1081,49 @@ void drm_connector_unregister(struct drm_connector *connector)
  EXPORT_SYMBOL(drm_connector_unregister);
 
  /**
+ * drm_connector_register_all - register all connectors
+ * @dev: drm device
+ *
+ * This function registers all connectors in sysfs and other places so that
+ * userspace can start to access them. Drivers can call it after calling
+ * drm_dev_register() to complete the device registration, if they don't call
+ * drm_connector_register() on each connector individually.
+ *
+ * When a device is unplugged and should be removed from userspace access,
+ * call drm_connector_unregister_all(), which is the inverse of this
+ * function.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_connector_register_all(struct drm_device *dev)
+{
+       struct drm_connector *connector;
+       int ret;
+
+       mutex_lock(&dev->mode_config.mutex);

drivers/gpu/drm/drm_crtc.c void drm_connector_unregister(struct drm_connector *connector) · line 1114

Quoted 10 lines
drivers/gpu/drm/drm_crtc.c +10 -0
+
+       drm_for_each_connector(connector, dev) {
+               ret = drm_connector_register(connector);
+               if (ret) {
+                       /*
+                        * We may safely call unregister_all() here within
+                        * area locked with mutex because unregister_all()
+                        * doesn't use locks inside (see a comment in that
+                        * function).
+                        */

Ugh? unregister_all() says:

Quoted 5 lines

/* FIXME: taking the mode config mutex ends up in a clash with sysfs */

This strongly contradicts your comment. Anyway, regardless how you want to fix it: You better unlock the mode-config mutex before returning below.

Quoted 6 lines

So good catch.
But what I really meant since we didn't get any further after registering all "good" connections (see we're not releasing mutex still) the will be no clashes with sysfs.

Still I;d like Daniel to comment on that separately.

I think doing the unregister_all outside of the loop&locked section is better for future-proofing. My long-term plan for connector lifetimes and the connector list is:
- refcounting for connectors (Dave has wip patches already).
- separate lock for the connector list (and only that).

Quoted 7 lines
Quoted 1 line

Doing it entirely separate would make things easier and more robust.

Ok makes sense.

Quoted 1 line

I merged patch 1 meanwhile to drm-misc.

So may I re-send only patches 2-4 then (using "drm-misc" as a base)?

Sure. drm-misc is also in linux-next, in case you need other bits to be able to test all your patches.
-Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#9 lore raw

PATCH v3 3/4 drm: atmel_hldc: Use generic drm_connector_register_all() helper

In reply to Alexey Brodkin

This driver used to have its own implementation of connector_register_all() which actually was taken as a prototype of drm_connector_register_all().

Now when drm_connector_register_all() exists reusing it here.

And while at it replace atmel_hlcdc_dc_connector_unplug_all() with generic drm_connector_unregister_all().

---

Changes v2 -> v3:
* Updated title with capital after colon
* Added ack from Boris

No changes v1 -> v2.

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 ++--------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +2 -37
index 3d8d164..1c537e4 100644
@@ -584,41 +584,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
 	destroy_workqueue(dc->wq);
 }
 
-static int atmel_hlcdc_dc_connector_plug_all(struct drm_device *dev)
-{
-	struct drm_connector *connector, *failed;
-	int ret;
-
-	mutex_lock(&dev->mode_config.mutex);
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		ret = drm_connector_register(connector);
-		if (ret) {
-			failed = connector;
-			goto err;
-		}
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-	return 0;
-
-err:
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (failed == connector)
-			break;
-
-		drm_connector_unregister(connector);
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-
-	return ret;
-}
-
-static void atmel_hlcdc_dc_connector_unplug_all(struct drm_device *dev)
-{
-	mutex_lock(&dev->mode_config.mutex);
-	drm_connector_unplug_all(dev);
-	mutex_unlock(&dev->mode_config.mutex);
-}
-
 static void atmel_hlcdc_dc_lastclose(struct drm_device *dev)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
@@ -736,7 +701,7 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unload;
 
-	ret = atmel_hlcdc_dc_connector_plug_all(ddev);
+	ret = drm_connector_register_all(ddev);
 	if (ret)
 		goto err_unregister;
 
@@ -758,7 +723,7 @@ static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
 {
 	struct drm_device *ddev = platform_get_drvdata(pdev);
 
-	atmel_hlcdc_dc_connector_unplug_all(ddev);
+	drm_connector_unregister_all(ddev);
 	drm_dev_unregister(ddev);
 	atmel_hlcdc_dc_unload(ddev);
 	drm_dev_unref(ddev);
Signature · 1 line
2.5.0
#10 lore raw

PATCH v3 4/4 drm: rcar-du: Use generic drm_connector_register_all() helper

In reply to Alexey Brodkin

Now that a generic drm_connector_register_all() helper exists we may safely substitute it for the driver-specific implementation of connectors plugging in sysfs.

---

Changes v2 -> v3:
* Updated title with capital after colon
* Updated commit message with fixes of spellos and grammar issues
* Added ack from Laurent

No changes v1 -> v2.

 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/gpu/drm/rcar-du/rcar_du_drv.c +1 -8
index 644db36..0f251dc 100644
@@ -361,14 +361,7 @@ static int rcar_du_probe(struct platform_device *pdev)
 	if (ret)
 		goto error;
 
-	mutex_lock(&ddev->mode_config.mutex);
-	drm_for_each_connector(connector, ddev) {
-		ret = drm_connector_register(connector);
-		if (ret < 0)
-			break;
-	}
-	mutex_unlock(&ddev->mode_config.mutex);
-
+	ret = drm_connector_register_all(ddev);
 	if (ret < 0)
 		goto error;
 
Signature · 1 line
2.5.0
#11 lore raw
In reply to Alexey Brodkin
Quoted 28 lines · Alexey Brodkin

As a pair to already existing drm_connector_unplug_all()
(which we'll rename in this series to drm_connector_unregister_all()) we're adding generic implementation of what is already done in some drivers for registering all connectors.

After implementation of that new helper we're updating 2 drivers that used to use it's own implementation:
[1] atmel_hlcdc
[2] rcar_du

And one driver that uses unregister():
[1] udl

Other drivers still use load() callback and so should be first modified so their load() gets called from their probe() explicitly.

Build- and run-tested on yet to be upstreamed ARC PGU (part of AXS10x board).

Changes v2 -> v3:
* Added acks for 1, 3 and 4 patches
* Updated kerneldoc descriptins of both register_ and unregister_all()
* Updated commit messages (mostly spellos and grammar issues)

Changes v1 -> v2:
* Rename drm_connector_unplug_all() to drm_connector_unregister_all()
* Use drm_for_each_connector() instead of list_for_each_entry()
* Updated kerneldoc for drm_dev_register()

Quoted 6 lines

Alexey Brodkin (4):
drm: Rename drm_connector_unplug_all() to
drm_connector_unregister_all()
drm: Introduce drm_connector_register_all() helper
drm: atmel_hldc: Use generic drm_connector_register_all() helper
drm: rcar-du: Use generic drm_connector_register_all() helper

lgtm overall, but merge window is happening so don't want to throw 4.7 patches into drm-misc. So will let these soak for a while more, please ping me after -rc1 is out that I don't forget them.

Thanks, Daniel

Quoted 17 lines
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 +-----------------
 drivers/gpu/drm/drm_crtc.c                   | 59 ++++++++++++++++++++++++----
 drivers/gpu/drm/drm_drv.c                    |  6 ++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c        | 14 +------
 drivers/gpu/drm/udl/udl_drv.c                |  2 +-
 include/drm/drm_crtc.h                       |  5 ++-
 6 files changed, 64 insertions(+), 61 deletions(-)
Cc Daniel Vetter <[email protected]>
Cc David Airlie <[email protected]>
Cc Boris Brezillon <[email protected]>
Cc Laurent Pinchart <[email protected]>
Signature · 1 line
2.5.0
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
#12 lore raw

Hi Daniel,

Quoted 36 lines · Daniel Vetter
Quoted 29 lines · Alexey Brodkin

As a pair to already existing drm_connector_unplug_all()
(which we'll rename in this series to drm_connector_unregister_all()) we're adding generic implementation of what is already done in some drivers for registering all connectors.

After implementation of that new helper we're updating 2 drivers that used to use it's own implementation:
 [1] atmel_hlcdc
 [2] rcar_du

And one driver that uses unregister():
 [1] udl

Other drivers still use load() callback and so should be first modified so their load() gets called from their probe() explicitly.

Build- and run-tested on yet to be upstreamed ARC PGU (part of AXS10x board).

Changes v2 -> v3:
 * Added acks for 1, 3 and 4 patches
 * Updated kerneldoc descriptins of both register_ and unregister_all()
 * Updated commit messages (mostly spellos and grammar issues)

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

Quoted 6 lines

Alexey Brodkin (4):
  drm: Rename drm_connector_unplug_all() to drm_connector_unregister_all()
  drm: Introduce drm_connector_register_all() helper
  drm: atmel_hldc: Use generic drm_connector_register_all() helper
  drm: rcar-du: Use generic drm_connector_register_all() helper

Quoted 3 lines

lgtm overall, but merge window is happening so don't want to throw 4.7 patches into drm-misc. So will let these soak for a while more, please ping me after -rc1 is out that I don't forget them.

As you asked I'm pinging you with request to apply that series.

Also would be very nice if you take a look at that comment from David Herrmann before application of that series:
http://article.gmane.org/gmane.comp.video.dri.devel/149708/match=re+-patch+-2+-4+-v3+-drm+-introduce+-drm_connector_register_al
l+-helper

-Alexey