[PATCH v4 22/22] drm: omapdrm: Perform initialization/cleanup at probe/remove time
Tomi Valkeinen
tomi.valkeinen at ti.com
Fri Dec 16 12:44:26 UTC 2016
On 14/12/16 02:27, Laurent Pinchart wrote:
> 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>
> ---
> Changes since v1:
>
> - Call drm_kms_helper_poll_fini() before omap_fbdev_free() in the remove
> handler.
> - Keep storing drm_device in the platform device private data.
> ---
> drivers/gpu/drm/omapdrm/omap_connector.c | 2 -
> drivers/gpu/drm/omapdrm/omap_drv.c | 212 ++++++++++++++++---------------
> 2 files changed, 107 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
> index 691cffebb76e..f90e2d22c5ec 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> @@ -240,8 +240,6 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
> connector->interlace_allowed = 1;
> connector->doublescan_allowed = 0;
>
> - drm_connector_register(connector);
> -
> return connector;
>
> fail:
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 0a2d461d62cf..a3b37823b271 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -493,11 +493,6 @@ static int omap_modeset_init(struct drm_device *dev)
> return 0;
> }
>
> -static void omap_modeset_free(struct drm_device *dev)
> -{
> - drm_mode_config_cleanup(dev);
> -}
> -
> /*
> * drm ioctl funcs
> */
> @@ -633,95 +628,6 @@ static const struct drm_ioctl_desc ioctls[DRM_COMMAND_END - DRM_COMMAND_BASE] =
> * drm driver funcs
> */
>
> -/**
> - * load - setup chip and create an initial config
> - * @dev: DRM device
> - * @flags: startup flags
> - *
> - * The driver load routine has to do several things:
> - * - initialize the memory manager
> - * - allocate initial config memory
> - * - setup the DRM framebuffer with the allocated memory
> - */
> -static int dev_load(struct drm_device *dev, unsigned long flags)
> -{
> - struct omap_drm_platform_data *pdata = dev->dev->platform_data;
> - struct omap_drm_private *priv;
> - unsigned int i;
> - int ret;
> -
> - DBG("load: dev=%p", dev);
> -
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv)
> - return -ENOMEM;
> -
> - priv->omaprev = pdata->omaprev;
> -
> - dev->dev_private = priv;
> -
> - priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> - init_waitqueue_head(&priv->commit.wait);
> - spin_lock_init(&priv->commit.lock);
> -
> - spin_lock_init(&priv->list_lock);
> - INIT_LIST_HEAD(&priv->obj_list);
> -
> - omap_gem_init(dev);
> -
> - ret = omap_modeset_init(dev);
> - if (ret) {
> - dev_err(dev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> - dev->dev_private = NULL;
> - kfree(priv);
> - return ret;
> - }
> -
> - /* Initialize vblank handling, start with all CRTCs disabled. */
> - ret = drm_vblank_init(dev, priv->num_crtcs);
> - if (ret)
> - dev_warn(dev->dev, "could not init vblank\n");
> -
> - for (i = 0; i < priv->num_crtcs; i++)
> - drm_crtc_vblank_off(priv->crtcs[i]);
> -
> - priv->fbdev = omap_fbdev_init(dev);
> -
> - /* store off drm_device for use in pm ops */
> - dev_set_drvdata(dev->dev, dev);
> -
> - drm_kms_helper_poll_init(dev);
> -
> - return 0;
> -}
> -
> -static int dev_unload(struct drm_device *dev)
> -{
> - struct omap_drm_private *priv = dev->dev_private;
> -
> - DBG("unload: dev=%p", dev);
> -
> - drm_kms_helper_poll_fini(dev);
> -
> - if (priv->fbdev)
> - omap_fbdev_free(dev);
> -
> - omap_modeset_free(dev);
> - omap_gem_deinit(dev);
> -
> - destroy_workqueue(priv->wq);
> -
> - drm_vblank_cleanup(dev);
> - omap_drm_irq_uninstall(dev);
> -
> - kfree(dev->dev_private);
> - dev->dev_private = NULL;
> -
> - dev_set_drvdata(dev->dev, NULL);
> -
> - return 0;
> -}
> -
> static int dev_open(struct drm_device *dev, struct drm_file *file)
> {
> file->driver_priv = NULL;
> @@ -806,8 +712,6 @@ static const struct file_operations omapdriver_fops = {
> static struct drm_driver omap_drm_driver = {
> .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> DRIVER_ATOMIC,
> - .load = dev_load,
> - .unload = dev_unload,
> .open = dev_open,
> .lastclose = dev_lastclose,
> .get_vblank_counter = drm_vblank_no_hw_counter,
> @@ -837,30 +741,128 @@ static struct drm_driver omap_drm_driver = {
> .patchlevel = DRIVER_PATCHLEVEL,
> };
>
> -static int pdev_probe(struct platform_device *device)
> +static int pdev_probe(struct platform_device *pdev)
> {
> - int r;
> + struct omap_drm_platform_data *pdata = pdev->dev.platform_data;
> + struct omap_drm_private *priv;
> + struct drm_device *ddev;
> + unsigned int i;
> + int ret;
> +
> + DBG("%s", pdev->name);
>
> if (omapdss_is_initialized() == false)
> return -EPROBE_DEFER;
>
> omap_crtc_pre_init();
>
> - r = omap_connect_dssdevs();
> - if (r) {
> + ret = omap_connect_dssdevs();
> + if (ret) {
> omap_crtc_pre_uninit();
> - return r;
> + goto err_crtc_uninit;
This calls omap_crtc_pre_uninit here and in the err handler.
> + }
> +
> + /* Allocate and initialize the driver private structure. */
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + ret = -ENOMEM;
> + goto err_disconnect_dssdevs;
> + }
> +
> + priv->omaprev = pdata->omaprev;
> + priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> +
> + init_waitqueue_head(&priv->commit.wait);
> + spin_lock_init(&priv->commit.lock);
> + spin_lock_init(&priv->list_lock);
> + INIT_LIST_HEAD(&priv->obj_list);
> +
> + /* Allocate and initialize the DRM device. */
> + ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> + if (IS_ERR(ddev)) {
> + ret = PTR_ERR(ddev);
> + goto err_free_priv;
> + }
> +
> + ddev->dev_private = priv;
> + platform_set_drvdata(pdev, ddev);
> +
> + omap_gem_init(ddev);
> +
> + ret = omap_modeset_init(ddev);
> + if (ret) {
> + dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> + goto err_free_drm_dev;
> + }
> +
> + /* Initialize vblank handling, start with all CRTCs disabled. */
> + ret = drm_vblank_init(ddev, priv->num_crtcs);
> + if (ret) {
> + dev_err(&pdev->dev, "could not init vblank\n");
> + goto err_cleanup_modeset;
> }
>
> - DBG("%s", device->name);
> - return drm_platform_init(&omap_drm_driver, device);
> + for (i = 0; i < priv->num_crtcs; i++)
> + drm_crtc_vblank_off(priv->crtcs[i]);
> +
> + priv->fbdev = omap_fbdev_init(ddev);
> +
> + drm_kms_helper_poll_init(ddev);
> +
> + /*
> + * Register the DRM device with the core and the connectors with
> + * sysfs.
> + */
> + ret = drm_dev_register(ddev, 0);
> + if (ret)
> + goto err_cleanup_helpers;
> +
> + return 0;
> +
> +err_cleanup_helpers:
> + drm_kms_helper_poll_fini(ddev);
> + if (priv->fbdev)
> + omap_fbdev_free(ddev);
> + drm_vblank_cleanup(ddev);
> +err_cleanup_modeset:
> + drm_mode_config_cleanup(ddev);
> + omap_drm_irq_uninstall(ddev);
> +err_free_drm_dev:
> + omap_gem_deinit(ddev);
> + drm_dev_unref(ddev);
> +err_free_priv:
> + destroy_workqueue(priv->wq);
> + kfree(priv);
> +err_disconnect_dssdevs:
> + omap_disconnect_dssdevs();
> +err_crtc_uninit:
> + omap_crtc_pre_uninit();
> + return ret;
> }
>
> -static int pdev_remove(struct platform_device *device)
> +static int pdev_remove(struct platform_device *pdev)
> {
> + struct drm_device *ddev = platform_get_drvdata(pdev);
> + struct omap_drm_private *priv = ddev->dev_private;
> +
> DBG("");
>
> - drm_put_dev(platform_get_drvdata(device));
> + drm_dev_unregister(ddev);
> +
> + drm_kms_helper_poll_fini(ddev);
> +
> + if (priv->fbdev)
> + omap_fbdev_free(ddev);
> +
> + drm_mode_config_cleanup(ddev);
> +
> + omap_drm_irq_uninstall(ddev);
> + omap_gem_deinit(ddev);
> +
> + drm_dev_unref(ddev);
> +
> + destroy_workqueue(priv->wq);
> + kfree(priv);
>
> omap_disconnect_dssdevs();
> omap_crtc_pre_uninit();
>
The old code calls drm_vblank_cleanup(), and the probe's error handling
calls that, but not remove. Is that correct?
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161216/7cdff755/attachment.sig>
More information about the dri-devel
mailing list