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 at pengutronix.de>
> ---
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
