Re: [PATCH v12 1/3] drm: rockchip: Add basic drm driver

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 19, 2014 at 10:04:13AM +0100, Boris Brezillon wrote:
> AFAIU, the suggestion was to split drm_connector_init and
> drm_connector_register calls:
>  - drm_connector_init call should still be part of the load procedure
>    (this function adds the connector to the connector list which is used
>    by drm_mode_group_init_legacy_group)
>  - drm_connector_register should be called after the device has been
>    registered
> 
> Here what I've done and it seems to work:
> 
> 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 int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> {
> 	struct drm_device *ddev;
> 	int ret;
> 
> 	ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
> 	if (!ddev)
> 		return -ENOMEM;
> 
> 	ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
> 	if (ret)
> 		goto err_unref;
> 
> 	ret = atmel_hlcdc_dc_load(ddev);
> 	if (ret)
> 		goto err_unref;
> 
> 	ret = drm_dev_register(ddev, 0);
> 	if (ret)
> 		goto err_unload;
> 
> 	ret = atmel_hlcdc_dc_connector_plug_all(ddev);
> 	if (ret)
> 		goto err_unregister;
> 
> 	return 0;
> 
> err_unregister:
> 	drm_dev_unregister(ddev);
> 
> err_unload:
> 	atmel_hlcdc_dc_unload(ddev);
> 
> err_unref:
> 	drm_dev_unref(ddev);
> 
> 	return ret;
> }
> 
> Daniel, can you confirm that's what you had in mind ?

Yup. To be able to have race-free driver load we need to split object
into an _init step (allocates structs and links to kernel-internal lists)
and _register (makes the object userspace-visible through sysfs and
dev-node kms object lookup idr).

This entire mess is all still fallout from the dark ages of the drm
midlayer and we'll probably have fun with this for another few years ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch