drm: Introduce drm_connector_register_all() helper
Subsystems (6)
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(-)
Signature · 1 line
2.5.0
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
@@ -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
@@ -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
@@ -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
@@ -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
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()inrcar_du_remove()as mentioned in kerneldoc fordrm_connector_unregister_all().Signed-off-by Alexey Brodkin <[email protected]>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 ofdrm_connector_unregister_all()so that it will match description ofregister_all()to be introduced in the next change
* Added ack from LaurentChanges 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@@ -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@@ -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@@ -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@@ -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
PATCH v3 2/4 drm: Introduce drm_connector_register_all() helper
In reply to Alexey BrodkinAs 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.
---
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
@@ -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
@@ -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
@@ -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
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.
Signed-off-by Alexey Brodkin <[email protected]>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 ofunregister_all()
* Updated kerneldoc description of thedrm_connector_register_all()Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_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@@ -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@@ -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@@ -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
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.
Signed-off-by Alexey Brodkin <[email protected]>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 ofunregister_all()
* Updated kerneldoc description of thedrm_connector_register_all()Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_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--- 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
Daniel Vetter
AppliedQuoted 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.
Signed-off-by Alexey Brodkin <[email protected]>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 ofunregister_all()
* Updated kerneldoc description of thedrm_connector_register_all()Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_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@@ -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
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.
Signed-off-by Alexey Brodkin <[email protected]>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 ofunregister_all()
* Updated kerneldoc description of thedrm_connector_register_all()Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_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--- 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_alloutside 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
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.
Signed-off-by Alexey Brodkin <[email protected]>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 ofunregister_all()
* Updated kerneldoc description of thedrm_connector_register_all()Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_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@@ -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_alloutside 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
PATCH v3 3/4 drm: atmel_hldc: Use generic drm_connector_register_all() helper
In reply to Alexey BrodkinThis 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
@@ -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
PATCH v3 4/4 drm: rcar-du: Use generic drm_connector_register_all() helper
In reply to Alexey BrodkinNow 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
@@ -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
Quoted 28 lines · Alexey Brodkin
As a pair to already existing
drm_connector_unplug_all()
(which we'll rename in this series todrm_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_duAnd one driver that uses
unregister():
[1] udlOther drivers still use
load()callback and so should be first modified so theirload()gets called from theirprobe()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 bothregister_andunregister_all()
* Updated commit messages (mostly spellos and grammar issues)Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_dev_register()
Quoted 6 lines
Alexey Brodkin (4):
drm: Renamedrm_connector_unplug_all()to
drm_connector_unregister_all()
drm: Introducedrm_connector_register_all()helper
drm:atmel_hldc: Use genericdrm_connector_register_all()helper
drm: rcar-du: Use genericdrm_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
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 todrm_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_duAnd one driver that uses
unregister():
[1] udlOther drivers still use
load()callback and so should be first modified so theirload()gets called from theirprobe()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 bothregister_andunregister_all()
* Updated commit messages (mostly spellos and grammar issues)Changes v1 -> v2:
* Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
* Usedrm_for_each_connector()instead oflist_for_each_entry()
* Updated kerneldoc fordrm_dev_register()Quoted 6 lines
Alexey Brodkin (4):
drm: Renamedrm_connector_unplug_all()todrm_connector_unregister_all()
drm: Introducedrm_connector_register_all()helper
drm:atmel_hldc: Use genericdrm_connector_register_all()helper
drm: rcar-du: Use genericdrm_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