On Tue, Dec 13, 2016 at 2:34 PM, Laurent Pinchart
<laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> wrote:
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> 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.
>
This looks reasonable to me, but I also don't have any exynos hardware
(that's running anything close to mainline).
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> 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);
> +
> + 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
