[PATCH 0/3] drm: introduce drm_connector_plug_all() helper

[PATCH 0/3] drm: introduce drm_connector_plug_all() helper

[Alexey Brodkin]

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

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

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).

Alexey Brodkin (3):
  drm: introduce drm_connector_plug_all() helper
  drm: atmel_hldc - use generic drm_connector_plug_all() helper
  drm: rcar-du - use generic drm_connector_plug_all() helper

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 ++----------------------
 drivers/gpu/drm/drm_crtc.c                   | 44 +++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_drv.c                    |  3 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c        |  9 +-----
 include/drm/drm_crtc.h                       |  3 +-
 5 files changed, 50 insertions(+), 48 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

[PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Alexey Brodkin]

As a pair to already existing drm_connector_unplug_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 generic one.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_drv.c  |  3 ++-
 include/drm/drm_crtc.h     |  3 ++-
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 65258ac..ce27420 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_unregister);
 
+/**
+ * drm_connector_plug_all - register connector userspace interfaces
+ * @dev: drm device
+ *
+ * This function registers all connector userspace interfaces in sysfs. Should
+ * be call when the device is disconnected, e.g. from an usb driver's
+ * ->connect callback.
+ */
+int drm_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;
+}
+EXPORT_SYMBOL(drm_connector_plug_all);
 
 /**
  * drm_connector_unplug_all - unregister connector userspace interfaces
@@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
 {
 	struct drm_connector *connector;
 
-	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
+	mutex_lock(&dev->mode_config.mutex);
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
 		drm_connector_unregister(connector);
 
+	mutex_unlock(&dev->mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_connector_unplug_all);
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 167c8d3..4a559c6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call
+ * drm_connector_plug_all() to register all connectors in sysfs.
  *
  * Never call this twice on any device!
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8c7fb3d..aa8789e 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 unplug all connectors from sysfs for device */
+/* helpers to plug/unplug all connectors to/from sysfs for device */
+extern int drm_connector_plug_all(struct drm_device *dev);
 extern void drm_connector_unplug_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
-- 
2.5.0

[PATCH 2/3] drm: atmel_hldc - use generic drm_connector_plug_all() helper

[Alexey Brodkin]

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

Now when drm_connector_plug_all() exists reusing it here.

And while at it getting rid of mutex in both plug() and unplug()
because they were moved to generic helper as well.

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>
---
 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..ca19ef1 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_plug_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_unplug_all(ddev);
 	drm_dev_unregister(ddev);
 	atmel_hlcdc_dc_unload(ddev);
 	drm_dev_unref(ddev);
-- 
2.5.0

[PATCH 3/3] drm: rcar-du - use generic drm_connector_plug_all() helper

[Alexey Brodkin]

Now when generic drm_connector_plug_all() helper exists we may safely
substiture with it 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: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 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 ed6006b..d5ec674 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -364,14 +364,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_plug_all(ddev);
 	if (ret < 0)
 		goto error;
 
-- 
2.5.0

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Daniel Vetter]

On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> As a pair to already existing drm_connector_unplug_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 generic one.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_drv.c  |  3 ++-
>  include/drm/drm_crtc.h     |  3 ++-
>  3 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 65258ac..ce27420 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_connector_unregister);
>  
> +/**
> + * drm_connector_plug_all - register connector userspace interfaces
> + * @dev: drm device
> + *
> + * This function registers all connector userspace interfaces in sysfs. Should
> + * be call when the device is disconnected, e.g. from an usb driver's

Still talks about disconnect ;-) Please also mention that this just calls
drm_connector_register() exactly like this including () to generate a
kerneldoc hyperlink.

> + * ->connect callback.

Returns: section is missing, specifying how this can fail. Just copy the
one from connector_register().

> + */
> +int drm_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) {

for_each_connector here please. And the s/plug/register/ naming discussion
we've had.

> +		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;
> +}
> +EXPORT_SYMBOL(drm_connector_plug_all);
>  
>  /**
>   * drm_connector_unplug_all - unregister connector userspace interfaces
> @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
>  {
>  	struct drm_connector *connector;
>  
> -	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> +	mutex_lock(&dev->mode_config.mutex);

You can't drop that FIXME, the bug is still there.

> +
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
>  		drm_connector_unregister(connector);
>  
> +	mutex_unlock(&dev->mode_config.mutex);
>  }
>  EXPORT_SYMBOL(drm_connector_unplug_all);
>  
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..4a559c6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call

It'd do 2 sentences here for simplicity, not connect them with and. Also
"... _the_ driver should ..."

> + * drm_connector_plug_all() to register all connectors in sysfs.

Maybe mention why this is separate: "This is a separate call for backwards
compatibility with drivers still using the deprecated ->load() callback,
where connectors are registered from within the ->load() callback."

Besides these details looks good.
-Daniel

>   *
>   * Never call this twice on any device!
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8c7fb3d..aa8789e 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 unplug all connectors from sysfs for device */
> +/* helpers to plug/unplug all connectors to/from sysfs for device */
> +extern int drm_connector_plug_all(struct drm_device *dev);
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>  
>  extern int drm_bridge_add(struct drm_bridge *bridge);
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Daniel Vetter]

On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> As a pair to already existing drm_connector_unplug_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 generic one.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_drv.c  |  3 ++-
>  include/drm/drm_crtc.h     |  3 ++-
>  3 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 65258ac..ce27420 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_connector_unregister);
>  
> +/**
> + * drm_connector_plug_all - register connector userspace interfaces
> + * @dev: drm device
> + *
> + * This function registers all connector userspace interfaces in sysfs. Should
> + * be call when the device is disconnected, e.g. from an usb driver's

Still talks about disconnect ;-) Please also mention that this just calls
drm_connector_register() exactly like this including () to generate a
kerneldoc hyperlink.

> + * ->connect callback.

Returns: section is missing, specifying how this can fail. Just copy the
one from connector_register().

> + */
> +int drm_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) {

for_each_connector here please. And the s/plug/register/ naming discussion
we've had.

> +		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;
> +}
> +EXPORT_SYMBOL(drm_connector_plug_all);
>  
>  /**
>   * drm_connector_unplug_all - unregister connector userspace interfaces
> @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
>  {
>  	struct drm_connector *connector;
>  
> -	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> +	mutex_lock(&dev->mode_config.mutex);

You can't drop that FIXME, the bug is still there.

> +
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
>  		drm_connector_unregister(connector);
>  
> +	mutex_unlock(&dev->mode_config.mutex);
>  }
>  EXPORT_SYMBOL(drm_connector_unplug_all);
>  
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..4a559c6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call

It'd do 2 sentences here for simplicity, not connect them with and. Also
"... _the_ driver should ..."

> + * drm_connector_plug_all() to register all connectors in sysfs.

Maybe mention why this is separate: "This is a separate call for backwards
compatibility with drivers still using the deprecated ->load() callback,
where connectors are registered from within the ->load() callback."

Besides these details looks good.
-Daniel

>   *
>   * Never call this twice on any device!
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8c7fb3d..aa8789e 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 unplug all connectors from sysfs for device */
> +/* helpers to plug/unplug all connectors to/from sysfs for device */
> +extern int drm_connector_plug_all(struct drm_device *dev);
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>  
>  extern int drm_bridge_add(struct drm_bridge *bridge);
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Alexey Brodkin]

Hi Daniel,

On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> > 
> > As a pair to already existing drm_connector_unplug_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 generic one.
> > 
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/drm_drv.c  |  3 ++-
> >  include/drm/drm_crtc.h     |  3 ++-
> >  3 files changed, 47 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 65258ac..ce27420 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
> >  }
> >  EXPORT_SYMBOL(drm_connector_unregister);
> >  
> > +/**
> > + * drm_connector_plug_all - register connector userspace interfaces
> > + * @dev: drm device
> > + *
> > + * This function registers all connector userspace interfaces in sysfs. Should
> > + * be call when the device is disconnected, e.g. from an usb driver's
> Still talks about disconnect ;-) Please also mention that this just calls
> drm_connector_register() exactly like this including () to generate a
> kerneldoc hyperlink.

Well I intentionally left in description of drm_connector_register_all():
"Should be call when the device is disconnected, e.g. from an usb driver's
 ->connect callback."

I did mean it. Or is this statement is incorrect and example of invocation of
drm_connector_register_all() should be different? Which one works better then?

> > 
> > + * ->connect callback.
> Returns: section is missing, specifying how this can fail. Just copy the
> one from connector_register().

Yeah, correct. Blind copy-paste doesn't work equally good always :(

> > 
> > + */
> > +int drm_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) {
> for_each_connector here please. And the s/plug/register/ naming discussion
> we've had.

Ok.

> > 
> > +		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;
> > +}
> > +EXPORT_SYMBOL(drm_connector_plug_all);
> >  
> >  /**
> >   * drm_connector_unplug_all - unregister connector userspace interfaces
> > @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
> >  {
> >  	struct drm_connector *connector;
> >  
> > -	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> > +	mutex_lock(&dev->mode_config.mutex);
> You can't drop that FIXME, the bug is still there.

That's clear given your explanation in the previous email.

> > 
> > +
> >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> >  		drm_connector_unregister(connector);
> >  
> > +	mutex_unlock(&dev->mode_config.mutex);
> >  }
> >  EXPORT_SYMBOL(drm_connector_unplug_all);
> >  
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 167c8d3..4a559c6 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call
> It'd do 2 sentences here for simplicity, not connect them with and. Also
> "... _the_ driver should ..."

Ok.

> > 
> > + * drm_connector_plug_all() to register all connectors in sysfs.
> Maybe mention why this is separate: "This is a separate call for backwards
> compatibility with drivers still using the deprecated ->load() callback,
> where connectors are registered from within the ->load() callback."

Ok.

-Alexey

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Daniel Vetter]

On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote:
> > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> > > 
> > > As a pair to already existing drm_connector_unplug_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 generic one.
> > > 
> > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: David Airlie <airlied@linux.ie>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/drm_drv.c  |  3 ++-
> > >  include/drm/drm_crtc.h     |  3 ++-
> > >  3 files changed, 47 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 65258ac..ce27420 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
> > >  }
> > >  EXPORT_SYMBOL(drm_connector_unregister);
> > >  
> > > +/**
> > > + * drm_connector_plug_all - register connector userspace interfaces
> > > + * @dev: drm device
> > > + *
> > > + * This function registers all connector userspace interfaces in sysfs. Should
> > > + * be call when the device is disconnected, e.g. from an usb driver's
> > Still talks about disconnect ;-) Please also mention that this just calls
> > drm_connector_register() exactly like this including () to generate a
> > kerneldoc hyperlink.
> 
> Well I intentionally left in description of drm_connector_register_all():
> "Should be call when the device is disconnected, e.g. from an usb driver's
>  ->connect callback."

You use "disconnected" for connecting stuff. That doesn't make sense to me
at all - register_all is for when you want to publish something, not for
unpublishing when the device disappears. Or maybe this is a case of lost
in translation, and you mean something else?
-Daniel
 
> I did mean it. Or is this statement is incorrect and example of invocation of
> drm_connector_register_all() should be different? Which one works better then?
> 
> > > 
> > > + * ->connect callback.
> > Returns: section is missing, specifying how this can fail. Just copy the
> > one from connector_register().
> 
> Yeah, correct. Blind copy-paste doesn't work equally good always :(
> 
> > > 
> > > + */
> > > +int drm_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) {
> > for_each_connector here please. And the s/plug/register/ naming discussion
> > we've had.
> 
> Ok.
> 
> > > 
> > > +		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;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_plug_all);
> > >  
> > >  /**
> > >   * drm_connector_unplug_all - unregister connector userspace interfaces
> > > @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
> > >  {
> > >  	struct drm_connector *connector;
> > >  
> > > -	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> > > +	mutex_lock(&dev->mode_config.mutex);
> > You can't drop that FIXME, the bug is still there.
> 
> That's clear given your explanation in the previous email.
> 
> > > 
> > > +
> > >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > >  		drm_connector_unregister(connector);
> > >  
> > > +	mutex_unlock(&dev->mode_config.mutex);
> > >  }
> > >  EXPORT_SYMBOL(drm_connector_unplug_all);
> > >  
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 167c8d3..4a559c6 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call
> > It'd do 2 sentences here for simplicity, not connect them with and. Also
> > "... _the_ driver should ..."
> 
> Ok.
> 
> > > 
> > > + * drm_connector_plug_all() to register all connectors in sysfs.
> > Maybe mention why this is separate: "This is a separate call for backwards
> > compatibility with drivers still using the deprecated ->load() callback,
> > where connectors are registered from within the ->load() callback."
> 
> Ok.
> 
> -Alexey

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Daniel Vetter]

On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote:
> > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> > > 
> > > As a pair to already existing drm_connector_unplug_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 generic one.
> > > 
> > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: David Airlie <airlied@linux.ie>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/drm_drv.c  |  3 ++-
> > >  include/drm/drm_crtc.h     |  3 ++-
> > >  3 files changed, 47 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 65258ac..ce27420 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
> > >  }
> > >  EXPORT_SYMBOL(drm_connector_unregister);
> > >  
> > > +/**
> > > + * drm_connector_plug_all - register connector userspace interfaces
> > > + * @dev: drm device
> > > + *
> > > + * This function registers all connector userspace interfaces in sysfs. Should
> > > + * be call when the device is disconnected, e.g. from an usb driver's
> > Still talks about disconnect ;-) Please also mention that this just calls
> > drm_connector_register() exactly like this including () to generate a
> > kerneldoc hyperlink.
> 
> Well I intentionally left in description of drm_connector_register_all():
> "Should be call when the device is disconnected, e.g. from an usb driver's
>  ->connect callback."

You use "disconnected" for connecting stuff. That doesn't make sense to me
at all - register_all is for when you want to publish something, not for
unpublishing when the device disappears. Or maybe this is a case of lost
in translation, and you mean something else?
-Daniel
 
> I did mean it. Or is this statement is incorrect and example of invocation of
> drm_connector_register_all() should be different? Which one works better then?
> 
> > > 
> > > + * ->connect callback.
> > Returns: section is missing, specifying how this can fail. Just copy the
> > one from connector_register().
> 
> Yeah, correct. Blind copy-paste doesn't work equally good always :(
> 
> > > 
> > > + */
> > > +int drm_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) {
> > for_each_connector here please. And the s/plug/register/ naming discussion
> > we've had.
> 
> Ok.
> 
> > > 
> > > +		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;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_plug_all);
> > >  
> > >  /**
> > >   * drm_connector_unplug_all - unregister connector userspace interfaces
> > > @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev)
> > >  {
> > >  	struct drm_connector *connector;
> > >  
> > > -	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> > > +	mutex_lock(&dev->mode_config.mutex);
> > You can't drop that FIXME, the bug is still there.
> 
> That's clear given your explanation in the previous email.
> 
> > > 
> > > +
> > >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > >  		drm_connector_unregister(connector);
> > >  
> > > +	mutex_unlock(&dev->mode_config.mutex);
> > >  }
> > >  EXPORT_SYMBOL(drm_connector_unplug_all);
> > >  
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 167c8d3..4a559c6 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call
> > It'd do 2 sentences here for simplicity, not connect them with and. Also
> > "... _the_ driver should ..."
> 
> Ok.
> 
> > > 
> > > + * drm_connector_plug_all() to register all connectors in sysfs.
> > Maybe mention why this is separate: "This is a separate call for backwards
> > compatibility with drivers still using the deprecated ->load() callback,
> > where connectors are registered from within the ->load() callback."
> 
> Ok.
> 
> -Alexey

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Alexey Brodkin]

Hi Daniel,

On Sat, 2016-03-19 at 11:02 +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote:
> > 
> > Hi Daniel,
> > 
> > On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> > > > 
> > > > 
> > > > As a pair to already existing drm_connector_unplug_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 generic one.
> > > > 
> > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Cc: David Airlie <airlied@linux.ie>
> > > > ---
> > > >  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > > >  drivers/gpu/drm/drm_drv.c  |  3 ++-
> > > >  include/drm/drm_crtc.h     |  3 ++-
> > > >  3 files changed, 47 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > > index 65258ac..ce27420 100644
> > > > --- a/drivers/gpu/drm/drm_crtc.c
> > > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_connector_unregister);
> > > >  
> > > > +/**
> > > > + * drm_connector_plug_all - register connector userspace interfaces
> > > > + * @dev: drm device
> > > > + *
> > > > + * This function registers all connector userspace interfaces in sysfs. Should
> > > > + * be call when the device is disconnected, e.g. from an usb driver's
> > > Still talks about disconnect ;-) Please also mention that this just calls
> > > drm_connector_register() exactly like this including () to generate a
> > > kerneldoc hyperlink.
> > Well I intentionally left in description of drm_connector_register_all():
> > "Should be call when the device is disconnected, e.g. from an usb driver's
> >  ->connect callback."
> You use "disconnected" for connecting stuff. That doesn't make sense to me
> at all - register_all is for when you want to publish something, not for
> unpublishing when the device disappears. Or maybe this is a case of lost
> in translation, and you mean something else?

Let me try to explain what I meant.

We execute ->connect() callback of USB device when USB device is still in "disconnected" state
(well at least that was my thought). And only when that ->connect() callback succeeds we're
entering "connected" state.

Probably above sentence is not correct. Then could you please help me with correct wording of
comment to drm_connector_register_all()?

-Alexey

Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Daniel Vetter]

On Mon, Mar 21, 2016 at 11:02:21AM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Sat, 2016-03-19 at 11:02 +0100, Daniel Vetter wrote:
> > On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote:
> > > 
> > > Hi Daniel,
> > > 
> > > On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote:
> > > > 
> > > > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote:
> > > > > 
> > > > > 
> > > > > As a pair to already existing drm_connector_unplug_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 generic one.
> > > > > 
> > > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Cc: David Airlie <airlied@linux.ie>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > > > >  drivers/gpu/drm/drm_drv.c  |  3 ++-
> > > > >  include/drm/drm_crtc.h     |  3 ++-
> > > > >  3 files changed, 47 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > > > index 65258ac..ce27420 100644
> > > > > --- a/drivers/gpu/drm/drm_crtc.c
> > > > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector)
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_connector_unregister);
> > > > >  
> > > > > +/**
> > > > > + * drm_connector_plug_all - register connector userspace interfaces
> > > > > + * @dev: drm device
> > > > > + *
> > > > > + * This function registers all connector userspace interfaces in sysfs. Should
> > > > > + * be call when the device is disconnected, e.g. from an usb driver's
> > > > Still talks about disconnect ;-) Please also mention that this just calls
> > > > drm_connector_register() exactly like this including () to generate a
> > > > kerneldoc hyperlink.
> > > Well I intentionally left in description of drm_connector_register_all():
> > > "Should be call when the device is disconnected, e.g. from an usb driver's
> > >  ->connect callback."
> > You use "disconnected" for connecting stuff. That doesn't make sense to me
> > at all - register_all is for when you want to publish something, not for
> > unpublishing when the device disappears. Or maybe this is a case of lost
> > in translation, and you mean something else?
> 
> Let me try to explain what I meant.
> 
> We execute ->connect() callback of USB device when USB device is still in "disconnected" state
> (well at least that was my thought). And only when that ->connect() callback succeeds we're
> entering "connected" state.
> 
> Probably above sentence is not correct. Then could you please help me with correct wording of
> comment to drm_connector_register_all()?

Yeah, your sentence suggests more that register_all should be called when
someone unplugs udl. What about:

"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."

And for drm_connector_unregister_all:

"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()."

Thinking about this we might want to merge drm_dev_register and
drm_connector_register_all, but that's a long-term goal which will need
some trickery to no break drivers.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch