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

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

 



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




[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