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

[Daniel Vetter <daniel@ffwll.ch>]

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