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 daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org
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().
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: linux-renesas-soc@vger.kernel.org Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 65258ac..65488a6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -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 diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index ed6006b..644db36 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -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) diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 772ec9e..c204089 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -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); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8c7fb3d..42d9f4d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -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);
On Wed, 23 Mar 2016 11:42:54 +0300 Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
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().
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Cc: Boris Brezillon boris.brezillon@free-electrons.com
Acked-by: Boris Brezillon boris.brezillon@free-electrons.com
Cc: linux-renesas-soc@vger.kernel.org Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 65258ac..65488a6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -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
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index ed6006b..644db36 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -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)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 772ec9e..c204089 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -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);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8c7fb3d..42d9f4d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -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);
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 abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie ---
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 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) { + 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 * diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..2c9a2b6 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -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! * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 42d9f4d..6a34117 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -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);
Hi
On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
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 abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 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) {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
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
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 167c8d3..2c9a2b6 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -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!
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 42d9f4d..6a34117 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -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);
2.5.0
Hi David,
On Wed, 2016-03-23 at 12:13 +0100, David Herrmann wrote:
Hi
On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
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 abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 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) { + 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.
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
On Wed, Mar 23, 2016 at 01:41:05PM +0000, Alexey Brodkin wrote:
Hi David,
On Wed, 2016-03-23 at 12:13 +0100, David Herrmann wrote:
Hi
On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
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 abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 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) { + 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.
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
Hi Daniel,
On Tue, 2016-03-29 at 10:19 +0200, Daniel Vetter wrote:
On Wed, Mar 23, 2016 at 01:41:05PM +0000, Alexey Brodkin wrote:
Hi David,
On Wed, 2016-03-23 at 12:13 +0100, David Herrmann wrote:
Hi
On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
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 abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 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) { + 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.
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.
Ok makes sense.
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
On Tue, Mar 29, 2016 at 09:12:38AM +0000, Alexey Brodkin wrote:
Hi Daniel,
On Tue, 2016-03-29 at 10:19 +0200, Daniel Vetter wrote:
On Wed, Mar 23, 2016 at 01:41:05PM +0000, Alexey Brodkin wrote:
Hi David,
On Wed, 2016-03-23 at 12:13 +0100, David Herrmann wrote:
Hi
On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
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 abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
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(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 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) { + 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.
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.
Ok makes sense.
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
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().
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Acked-by: Boris Brezillon boris.brezillon@free-electrons.com ---
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(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 3d8d164..1c537e4 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -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);
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.
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Cc: linux-renesas-soc@vger.kernel.org Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
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(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 644db36..0f251dc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -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;
On Wed, Mar 23, 2016 at 11:42:53AM +0300, Alexey Brodkin wrote:
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
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
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 daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org -- 2.5.0
Hi Daniel,
On Wed, 2016-03-23 at 11:37 +0100, Daniel Vetter wrote:
On Wed, Mar 23, 2016 at 11:42:53AM +0300, Alexey Brodkin wrote:
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
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+... l+helper
-Alexey
dri-devel@lists.freedesktop.org
-
Alexey Brodkin -
Boris Brezillon -
Daniel Vetter -
David Herrmann