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

From: Ying Liu <gnuiyl@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: patchwork-lst@pengutronix.de, kernel@pengutronix.de,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 4/5] drm/imx: drop deprecated load/unload drm_driver ops
Date: Fri, 24 Jun 2016 15:46:55 +0800	[thread overview]
Message-ID: <CAOcKUNVknndZs+Cp7gL2A4LjyKvbvYFXN_1NWWssx-KWAwqJOA@mail.gmail.com> (raw)
In-Reply-To: <20160617124838.GQ23520@phenom.ffwll.local>

On Fri, Jun 17, 2016 at 8:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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@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.

I've got this done in my imx-drm atomic conversion v2 patch set.

Regards,
Liu Ying

>
>> +     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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel