[PATCH 1/2] drm: exynos: Perform initialization/cleanup at probe/remove time

Daniel Vetter daniel at ffwll.ch
Tue Dec 13 21:10:58 UTC 2016


On Tue, Dec 13, 2016 at 09:34:05PM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> The drm driver .load() operation is prone to race conditions as it
> initializes the driver after registering the device nodes. Its usage is
> deprecated, inline it in the probe function and call drm_dev_alloc() and
> drm_dev_register() explicitly.
> 
> For consistency inline the .unload() handler in the remove function as
> well.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  drivers/gpu/drm/exynos/exynos_dp.c       |   1 -
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c  |   1 -
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 245 ++++++++++++++++---------------
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c  |   1 -
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c |   1 -
>  drivers/gpu/drm/exynos/exynos_hdmi.c     |   1 -
>  6 files changed, 127 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 528229faffe4..b839f065f4b3 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -102,7 +102,6 @@ static int exynos_dp_bridge_attach(struct analogix_dp_plat_data *plat_data,
>  	struct drm_encoder *encoder = &dp->encoder;
>  	int ret;
>  
> -	drm_connector_register(connector);
>  	dp->connector = connector;
>  
>  	/* Pre-empt DP connector creation if there's a bridge */
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> index ad6b73c7fc59..3aab71a485ba 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> @@ -114,7 +114,6 @@ static int exynos_dpi_create_connector(struct drm_encoder *encoder)
>  	}
>  
>  	drm_connector_helper_add(connector, &exynos_dpi_connector_helper_funcs);
> -	drm_connector_register(connector);
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 739180ac3da5..bcd3d1db53eb 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -90,120 +90,6 @@ static void exynos_drm_atomic_work(struct work_struct *work)
>  
>  static struct device *exynos_drm_get_dma_device(void);
>  
> -static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct exynos_drm_private *private;
> -	struct drm_encoder *encoder;
> -	unsigned int clone_mask;
> -	int cnt, ret;
> -
> -	private = kzalloc(sizeof(struct exynos_drm_private), GFP_KERNEL);
> -	if (!private)
> -		return -ENOMEM;
> -
> -	init_waitqueue_head(&private->wait);
> -	spin_lock_init(&private->lock);
> -
> -	dev_set_drvdata(dev->dev, dev);
> -	dev->dev_private = (void *)private;
> -
> -	/* the first real CRTC device is used for all dma mapping operations */
> -	private->dma_dev = exynos_drm_get_dma_device();
> -	if (!private->dma_dev) {
> -		DRM_ERROR("no device found for DMA mapping operations.\n");
> -		ret = -ENODEV;
> -		goto err_free_private;
> -	}
> -	DRM_INFO("Exynos DRM: using %s device for DMA mapping operations\n",
> -		 dev_name(private->dma_dev));
> -
> -	/* create common IOMMU mapping for all devices attached to Exynos DRM */
> -	ret = drm_create_iommu_mapping(dev);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to create iommu mapping.\n");
> -		goto err_free_private;
> -	}
> -
> -	drm_mode_config_init(dev);
> -
> -	exynos_drm_mode_config_init(dev);
> -
> -	/* setup possible_clones. */
> -	cnt = 0;
> -	clone_mask = 0;
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> -		clone_mask |= (1 << (cnt++));
> -
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> -		encoder->possible_clones = clone_mask;
> -
> -	platform_set_drvdata(dev->platformdev, dev);
> -
> -	/* Try to bind all sub drivers. */
> -	ret = component_bind_all(dev->dev, dev);
> -	if (ret)
> -		goto err_mode_config_cleanup;
> -
> -	ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
> -	if (ret)
> -		goto err_unbind_all;
> -
> -	/* Probe non kms sub drivers and virtual display driver. */
> -	ret = exynos_drm_device_subdrv_probe(dev);
> -	if (ret)
> -		goto err_cleanup_vblank;
> -
> -	drm_mode_config_reset(dev);
> -
> -	/*
> -	 * enable drm irq mode.
> -	 * - with irq_enabled = true, we can use the vblank feature.
> -	 *
> -	 * P.S. note that we wouldn't use drm irq handler but
> -	 *	just specific driver own one instead because
> -	 *	drm framework supports only one irq handler.
> -	 */
> -	dev->irq_enabled = true;
> -
> -	/* init kms poll for handling hpd */
> -	drm_kms_helper_poll_init(dev);
> -
> -	/* force connectors detection */
> -	drm_helper_hpd_irq_event(dev);
> -
> -	return 0;
> -
> -err_cleanup_vblank:
> -	drm_vblank_cleanup(dev);
> -err_unbind_all:
> -	component_unbind_all(dev->dev, dev);
> -err_mode_config_cleanup:
> -	drm_mode_config_cleanup(dev);
> -	drm_release_iommu_mapping(dev);
> -err_free_private:
> -	kfree(private);
> -
> -	return ret;
> -}
> -
> -static int exynos_drm_unload(struct drm_device *dev)
> -{
> -	exynos_drm_device_subdrv_remove(dev);
> -
> -	exynos_drm_fbdev_fini(dev);
> -	drm_kms_helper_poll_fini(dev);
> -
> -	drm_vblank_cleanup(dev);
> -	component_unbind_all(dev->dev, dev);
> -	drm_mode_config_cleanup(dev);
> -	drm_release_iommu_mapping(dev);
> -
> -	kfree(dev->dev_private);
> -	dev->dev_private = NULL;
> -
> -	return 0;
> -}
> -
>  static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs)
>  {
>  	bool pending;
> @@ -373,8 +259,6 @@ static const struct file_operations exynos_drm_driver_fops = {
>  static struct drm_driver exynos_drm_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
>  				  | DRIVER_ATOMIC | DRIVER_RENDER,
> -	.load			= exynos_drm_load,
> -	.unload			= exynos_drm_unload,
>  	.open			= exynos_drm_open,
>  	.preclose		= exynos_drm_preclose,
>  	.lastclose		= exynos_drm_lastclose,
> @@ -552,12 +436,137 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>  
>  static int exynos_drm_bind(struct device *dev)
>  {
> -	return drm_platform_init(&exynos_drm_driver, to_platform_device(dev));
> +	struct exynos_drm_private *private;
> +	struct drm_encoder *encoder;
> +	struct drm_device *drm;
> +	unsigned int clone_mask;
> +	int cnt, ret;
> +
> +	drm = drm_dev_alloc(&exynos_drm_driver, dev);
> +	if (IS_ERR(drm))
> +		return PTR_ERR(drm);
> +
> +	private = kzalloc(sizeof(struct exynos_drm_private), GFP_KERNEL);
> +	if (!private) {
> +		ret = -ENOMEM;
> +		goto err_free_drm;
> +	}
> +
> +	init_waitqueue_head(&private->wait);
> +	spin_lock_init(&private->lock);
> +
> +	dev_set_drvdata(drm->dev, drm);
> +	drm->dev_private = (void *)private;
> +
> +	/* the first real CRTC device is used for all dma mapping operations */
> +	private->dma_dev = exynos_drm_get_dma_device();
> +	if (!private->dma_dev) {
> +		DRM_ERROR("no device found for DMA mapping operations.\n");
> +		ret = -ENODEV;
> +		goto err_free_private;
> +	}
> +	DRM_INFO("Exynos DRM: using %s device for DMA mapping operations\n",
> +		 dev_name(private->dma_dev));
> +
> +	/* create common IOMMU mapping for all devices attached to Exynos DRM */
> +	ret = drm_create_iommu_mapping(drm);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to create iommu mapping.\n");
> +		goto err_free_private;
> +	}
> +
> +	drm_mode_config_init(drm);
> +
> +	exynos_drm_mode_config_init(drm);
> +
> +	/* setup possible_clones. */
> +	cnt = 0;
> +	clone_mask = 0;
> +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
> +		clone_mask |= (1 << (cnt++));
> +
> +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
> +		encoder->possible_clones = clone_mask;
> +
> +	platform_set_drvdata(drm->platformdev, drm);
> +
> +	/* Try to bind all sub drivers. */
> +	ret = component_bind_all(drm->dev, drm);
> +	if (ret)
> +		goto err_mode_config_cleanup;
> +
> +	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> +	if (ret)
> +		goto err_unbind_all;
> +
> +	/* Probe non kms sub drivers and virtual display driver. */
> +	ret = exynos_drm_device_subdrv_probe(drm);
> +	if (ret)
> +		goto err_cleanup_vblank;
> +
> +	drm_mode_config_reset(drm);
> +
> +	/*
> +	 * enable drm irq mode.
> +	 * - with irq_enabled = true, we can use the vblank feature.
> +	 *
> +	 * P.S. note that we wouldn't use drm irq handler but
> +	 *	just specific driver own one instead because
> +	 *	drm framework supports only one irq handler.
> +	 */
> +	drm->irq_enabled = true;
> +
> +	/* init kms poll for handling hpd */
> +	drm_kms_helper_poll_init(drm);
> +
> +	/* force connectors detection */
> +	drm_helper_hpd_irq_event(drm);
> +
> +	/* register the DRM device */
> +	ret = drm_dev_register(drm, 0);
> +	if (ret < 0)
> +		goto err_cleanup_fbdev;
> +
> +	return 0;
> +
> +err_cleanup_fbdev:
> +	exynos_drm_fbdev_fini(drm);
> +	drm_kms_helper_poll_fini(drm);
> +	exynos_drm_device_subdrv_remove(drm);
> +err_cleanup_vblank:
> +	drm_vblank_cleanup(drm);
> +err_unbind_all:
> +	component_unbind_all(drm->dev, drm);
> +err_mode_config_cleanup:
> +	drm_mode_config_cleanup(drm);
> +	drm_release_iommu_mapping(drm);
> +err_free_private:
> +	kfree(private);
> +err_free_drm:
> +	drm_dev_unref(drm);
> +
> +	return ret;
>  }
>  
>  static void exynos_drm_unbind(struct device *dev)
>  {
> -	drm_put_dev(dev_get_drvdata(dev));
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +
> +	drm_dev_unregister(drm);
> +
> +	exynos_drm_device_subdrv_remove(drm);
> +
> +	exynos_drm_fbdev_fini(drm);
> +	drm_kms_helper_poll_fini(drm);

Unbind order is inverted from the error paths in the probe function, but
meh, preexisting.

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>o

... because I really want to see drm_platform.c gone! Feel free to push to
drm-misc, you haz commit rights after all ;-)
-Daniel

> +
> +	component_unbind_all(drm->dev, drm);
> +	drm_mode_config_cleanup(drm);
> +	drm_release_iommu_mapping(drm);
> +
> +	kfree(drm->dev_private);
> +	drm->dev_private = NULL;
> +
> +	drm_dev_unref(drm);
>  }
>  
>  static const struct component_master_ops exynos_drm_ops = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index e07cb1fe4860..7e803fe2352f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1587,7 +1587,6 @@ static int exynos_dsi_create_connector(struct drm_encoder *encoder)
>  	}
>  
>  	drm_connector_helper_add(connector, &exynos_dsi_connector_helper_funcs);
> -	drm_connector_register(connector);
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 57fe514d5c5b..6bbb0ea8a6af 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -359,7 +359,6 @@ static int vidi_create_connector(struct drm_encoder *encoder)
>  	}
>  
>  	drm_connector_helper_add(connector, &vidi_connector_helper_funcs);
> -	drm_connector_register(connector);
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 5ed8b1effe71..3bc70f40cb7d 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -909,7 +909,6 @@ static int hdmi_create_connector(struct drm_encoder *encoder)
>  	}
>  
>  	drm_connector_helper_add(connector, &hdmi_connector_helper_funcs);
> -	drm_connector_register(connector);
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
>  	return 0;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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


More information about the dri-devel mailing list