On Fri, Jun 17, 2016 at 8:48 PM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
