Re: [PATCH 4/5] drm/imx: drop deprecated load/unload drm_driver ops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 17, 2016 at 12:13:41PM +0200, Lucas Stach wrote:
> Drop the load/unload driver ops, as they are deprecated because of their
> inherent races, with devices being visible to userspace before they are
> fully initialized.
> 
> Move this code into the driver bind/unbind routines bracketed by the
> proper drm_dev_alloc/register and drm_dev_unregister/unref calls.
> 
> Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 247 ++++++++++++++++++-------------------
>  1 file changed, 121 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index c63378661e11..799a68976590 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -78,25 +78,6 @@ static void imx_drm_driver_lastclose(struct drm_device *drm)
>  	}
>  }
>  
> -static int imx_drm_driver_unload(struct drm_device *drm)
> -{
> -	struct imx_drm_device *imxdrm = drm->dev_private;
> -
> -	drm_kms_helper_poll_fini(drm);
> -
> -	if (imxdrm->fbhelper)
> -		drm_fbdev_cma_fini(imxdrm->fbhelper);
> -
> -	component_unbind_all(drm->dev, drm);
> -
> -	drm_vblank_cleanup(drm);
> -	drm_mode_config_cleanup(drm);
> -
> -	platform_set_drvdata(drm->platformdev, NULL);
> -
> -	return 0;
> -}
> -
>  static struct imx_drm_crtc *imx_drm_find_crtc(struct drm_crtc *crtc)
>  {
>  	struct imx_drm_device *imxdrm = crtc->dev->dev_private;
> @@ -223,109 +204,6 @@ static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
>  };
>  
>  /*
> - * Main DRM initialisation. This binds, initialises and registers
> - * with DRM the subcomponents of the driver.
> - */
> -static int imx_drm_driver_load(struct drm_device *drm, unsigned long flags)
> -{
> -	struct imx_drm_device *imxdrm;
> -	struct drm_connector *connector;
> -	int ret;
> -
> -	imxdrm = devm_kzalloc(drm->dev, sizeof(*imxdrm), GFP_KERNEL);
> -	if (!imxdrm)
> -		return -ENOMEM;
> -
> -	imxdrm->drm = drm;
> -
> -	drm->dev_private = imxdrm;
> -
> -	/*
> -	 * 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 and
> -	 *      drivers can well take care of their interrupts
> -	 */
> -	drm->irq_enabled = true;
> -
> -	/*
> -	 * set max width and height as default value(4096x4096).
> -	 * this value would be used to check framebuffer size limitation
> -	 * at drm_mode_addfb().
> -	 */
> -	drm->mode_config.min_width = 64;
> -	drm->mode_config.min_height = 64;
> -	drm->mode_config.max_width = 4096;
> -	drm->mode_config.max_height = 4096;
> -	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> -
> -	drm_mode_config_init(drm);
> -
> -	ret = drm_vblank_init(drm, MAX_CRTC);
> -	if (ret)
> -		goto err_kms;
> -
> -	platform_set_drvdata(drm->platformdev, drm);
> -
> -	/* Now try and bind all our sub-components */
> -	ret = component_bind_all(drm->dev, drm);
> -	if (ret)
> -		goto err_vblank;
> -
> -	/*
> -	 * All components are now added, we can publish the connector sysfs
> -	 * entries to userspace.  This will generate hotplug events and so
> -	 * userspace will expect to be able to access DRM at this point.
> -	 */
> -	list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
> -		ret = drm_connector_register(connector);
> -		if (ret) {
> -			dev_err(drm->dev,
> -				"[CONNECTOR:%d:%s] drm_connector_register failed: %d\n",
> -				connector->base.id,
> -				connector->name, ret);
> -			goto err_unbind;
> -		}
> -	}
> -
> -	/*
> -	 * All components are now initialised, so setup the fb helper.
> -	 * The fb helper takes copies of key hardware information, so the
> -	 * crtcs/connectors/encoders must not change after this point.
> -	 */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)
> -	if (legacyfb_depth != 16 && legacyfb_depth != 32) {
> -		dev_warn(drm->dev, "Invalid legacyfb_depth.  Defaulting to 16bpp\n");
> -		legacyfb_depth = 16;
> -	}
> -	drm_helper_disable_unused_functions(drm);
> -	imxdrm->fbhelper = drm_fbdev_cma_init(drm, legacyfb_depth,
> -				drm->mode_config.num_crtc, MAX_CRTC);
> -	if (IS_ERR(imxdrm->fbhelper)) {
> -		ret = PTR_ERR(imxdrm->fbhelper);
> -		imxdrm->fbhelper = NULL;
> -		goto err_unbind;
> -	}
> -#endif
> -
> -	drm_kms_helper_poll_init(drm);
> -
> -	return 0;
> -
> -err_unbind:
> -	component_unbind_all(drm->dev, drm);
> -err_vblank:
> -	drm_vblank_cleanup(drm);
> -err_kms:
> -	drm_mode_config_cleanup(drm);
> -
> -	return ret;
> -}
> -
> -/*
>   * imx_drm_add_crtc - add a new crtc
>   */
>  int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
> @@ -416,8 +294,6 @@ static const struct drm_ioctl_desc imx_drm_ioctls[] = {
>  
>  static struct drm_driver imx_drm_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
> -	.load			= imx_drm_driver_load,
> -	.unload			= imx_drm_driver_unload,
>  	.lastclose		= imx_drm_driver_lastclose,
>  	.set_busid		= drm_platform_set_busid,
>  	.gem_free_object_unlocked = drm_gem_cma_free_object,
> @@ -471,12 +347,131 @@ static int compare_of(struct device *dev, void *data)
>  
>  static int imx_drm_bind(struct device *dev)
>  {
> -	return drm_platform_init(&imx_drm_driver, to_platform_device(dev));
> +	struct drm_device *drm;
> +	struct imx_drm_device *imxdrm;
> +	int ret;
> +
> +	drm = drm_dev_alloc(&imx_drm_driver, dev);
> +	if (!drm)
> +		return -ENOMEM;
> +
> +	imxdrm = devm_kzalloc(dev, sizeof(*imxdrm), GFP_KERNEL);
> +	if (!imxdrm) {
> +		ret = -ENOMEM;
> +		goto err_unref;
> +	}
> +
> +	imxdrm->drm = drm;
> +	drm->dev_private = imxdrm;
> +
> +	/*
> +	 * 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 and
> +	 *      drivers can well take care of their interrupts
> +	 */
> +	drm->irq_enabled = true;
> +
> +	/*
> +	 * set max width and height as default value(4096x4096).
> +	 * this value would be used to check framebuffer size limitation
> +	 * at drm_mode_addfb().
> +	 */
> +	drm->mode_config.min_width = 64;
> +	drm->mode_config.min_height = 64;
> +	drm->mode_config.max_width = 4096;
> +	drm->mode_config.max_height = 4096;
> +	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> +
> +	drm_mode_config_init(drm);
> +
> +	ret = drm_vblank_init(drm, MAX_CRTC);
> +	if (ret)
> +		goto err_kms;
> +
> +	dev_set_drvdata(dev, drm);
> +
> +	/* Now try and bind all our sub-components */
> +	ret = component_bind_all(dev, drm);
> +	if (ret)
> +		goto err_vblank;
> +
> +	ret = drm_dev_register(drm, 0);

In principle this should be the last step in the init sequence, otherwise
you might have userspace fighting with your init code over the hw. Please
move down.

> +	if (ret)
> +		goto err_unbind;
> +
> +	/*
> +	 * All components are now added, we can publish the connector sysfs
> +	 * entries to userspace.  This will generate hotplug events and so
> +	 * userspace will expect to be able to access DRM at this point.
> +	 */
> +	ret = drm_connector_register_all(drm);

This (and connector_unregister_all) just became unecessary with the
patches from Chris that I merged into drm-misc today. Please remove.

> +	if (ret)
> +		goto err_unregister;
> +
> +	/*
> +	 * All components are now initialised, so setup the fb helper.
> +	 * The fb helper takes copies of key hardware information, so the
> +	 * crtcs/connectors/encoders must not change after this point.
> +	 */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)
> +	if (legacyfb_depth != 16 && legacyfb_depth != 32) {
> +		dev_warn(dev, "Invalid legacyfb_depth.  Defaulting to 16bpp\n");
> +		legacyfb_depth = 16;
> +	}
> +	drm_helper_disable_unused_functions(drm);

fyi, you need to nuke this when switching to atomic.

> +	imxdrm->fbhelper = drm_fbdev_cma_init(drm, legacyfb_depth,
> +				drm->mode_config.num_crtc, MAX_CRTC);
> +	if (IS_ERR(imxdrm->fbhelper)) {
> +		ret = PTR_ERR(imxdrm->fbhelper);
> +		imxdrm->fbhelper = NULL;
> +		goto err_unregister;
> +	}
> +#endif
> +
> +	drm_kms_helper_poll_init(drm);
> +
> +	return 0;
> +
> +err_unregister:
> +	drm_dev_unregister(drm);
> +err_unbind:
> +	component_unbind_all(drm->dev, drm);
> +err_vblank:
> +	drm_vblank_cleanup(drm);
> +err_kms:
> +	drm_mode_config_cleanup(drm);
> +err_unref:
> +	drm_dev_unref(drm);
> +
> +	return ret;
>  }
>  
>  static void imx_drm_unbind(struct device *dev)
>  {
> -	drm_put_dev(dev_get_drvdata(dev));
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct imx_drm_device *imxdrm = drm->dev_private;
> +	struct drm_fbdev_cma *fbhelper = imxdrm->fbhelper;
> +
> +	drm_kms_helper_poll_fini(drm);
> +	/* device is going down, so no need to restore fbdev modes */
> +	imxdrm->fbhelper = NULL;
> +
> +	drm_connector_unregister_all(drm);
> +	drm_dev_unregister(drm);

Same here: unregister should be first, connector_unregister_all isn't
needed any more.
-Daniel

> +
> +	if (fbhelper)
> +		drm_fbdev_cma_fini(fbhelper);
> +
> +	component_unbind_all(drm->dev, drm);
> +	dev_set_drvdata(dev, NULL);
> +
> +	drm_mode_config_cleanup(drm);
> +
> +	drm_dev_unref(drm);
>  }
>  
>  static const struct component_master_ops imx_drm_ops = {
> -- 
> 2.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux