[PATCH 1/5] drm/imx: disable outputs in lastclose when framebuffer emulation is disabled

[PATCH 1/5] drm/imx: disable outputs in lastclose when framebuffer emulation is disabled

[Lucas Stach]

If there is no framebuffer mode that can be restored, all outputs should
be disabled in order to avoid information leaks.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 82656654fb21..c63378661e11 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -63,7 +63,19 @@ static void imx_drm_driver_lastclose(struct drm_device *drm)
 {
 	struct imx_drm_device *imxdrm = drm->dev_private;
 
-	drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
+	if (imxdrm->fbhelper) {
+		drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
+	} else {
+		struct drm_connector *connector;
+
+		/* no kernel mode to go back to, disable all outputs */
+		drm_modeset_lock_all(drm);
+		drm_for_each_connector(connector, drm)
+			connector->encoder = NULL;
+		drm_modeset_unlock_all(drm);
+
+		drm_helper_disable_unused_functions(drm);
+	}
 }
 
 static int imx_drm_driver_unload(struct drm_device *drm)
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/imx: imx-ldb: check return code on panel attach

[Lucas Stach]

Check the return code on panel attach. Avoids a kernel crash later
on if the attach failed.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index beff793bb717..48166df14042 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -427,8 +427,12 @@ static int imx_ldb_register(struct drm_device *drm,
 	drm_connector_init(drm, &imx_ldb_ch->connector,
 			   &imx_ldb_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
 
-	if (imx_ldb_ch->panel)
-		drm_panel_attach(imx_ldb_ch->panel, &imx_ldb_ch->connector);
+	if (imx_ldb_ch->panel) {
+		ret = drm_panel_attach(imx_ldb_ch->panel,
+				       &imx_ldb_ch->connector);
+		if (ret)
+			return ret;
+	}
 
 	drm_mode_connector_attach_encoder(&imx_ldb_ch->connector,
 			&imx_ldb_ch->encoder);
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/imx: imx-ldb: detach panel on unbind

[Lucas Stach]

Make sure to leave a clean panel state behind and allow to
properly attach to the panel again on a rebind.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 48166df14042..9e117a654417 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -671,6 +671,9 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
 	for (i = 0; i < 2; i++) {
 		struct imx_ldb_channel *channel = &imx_ldb->channel[i];
 
+		if (channel->panel)
+			drm_panel_detach(channel->panel);
+
 		if (!channel->connector.funcs)
 			continue;
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas Stach]

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);
+	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);
+	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);
+	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);
+
+	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

[PATCH 5/5] drm/imx: don't destroy mode objects manually on driver unbind

[Lucas Stach]

Instead let drm_mode_config_cleanup() do the work when taking down
the master device. This requires all cleanup functions to be
properly hooked up to the mode object .destroy callback.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/dw-hdmi.c       | 3 ---
 drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
 drivers/gpu/drm/imx/imx-ldb.c          | 6 ------
 drivers/gpu/drm/imx/imx-tve.c          | 3 ---
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 9 ++++++---
 drivers/gpu/drm/imx/parallel-display.c | 3 ---
 6 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index c9d941283d30..5f97977f7e5c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1834,9 +1834,6 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
 	/* Disable all interrupts */
 	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
 
-	hdmi->connector.funcs->destroy(&hdmi->connector);
-	hdmi->encoder->funcs->destroy(hdmi->encoder);
-
 	clk_disable_unprepare(hdmi->iahb_clk);
 	clk_disable_unprepare(hdmi->isfr_clk);
 	i2c_put_adapter(hdmi->ddc);
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 799a68976590..71e33666cae8 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -466,11 +466,11 @@ static void imx_drm_unbind(struct device *dev)
 	if (fbhelper)
 		drm_fbdev_cma_fini(fbhelper);
 
+	drm_mode_config_cleanup(drm);
+
 	component_unbind_all(drm->dev, drm);
 	dev_set_drvdata(dev, NULL);
 
-	drm_mode_config_cleanup(drm);
-
 	drm_dev_unref(drm);
 }
 
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 9e117a654417..b49948d51110 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -674,12 +674,6 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
 		if (channel->panel)
 			drm_panel_detach(channel->panel);
 
-		if (!channel->connector.funcs)
-			continue;
-
-		channel->connector.funcs->destroy(&channel->connector);
-		channel->encoder.funcs->destroy(&channel->encoder);
-
 		kfree(channel->edid);
 		i2c_put_adapter(channel->ddc);
 	}
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index baf788121287..3973b2c85e2d 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -688,9 +688,6 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
 {
 	struct imx_tve *tve = dev_get_drvdata(dev);
 
-	tve->connector.funcs->destroy(&tve->connector);
-	tve->encoder.funcs->destroy(&tve->encoder);
-
 	if (!IS_ERR(tve->dac_reg))
 		regulator_disable(tve->dac_reg);
 }
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index fc040417e1e8..4d5dbea48cef 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -228,9 +228,14 @@ put_vblank:
 	return ret;
 }
 
+static void ipu_crtc_cleanup(struct drm_crtc *crtc)
+{
+	imx_drm_remove_crtc(to_ipu_crtc(crtc)->imx_crtc);
+}
+
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_crtc_helper_set_config,
-	.destroy = drm_crtc_cleanup,
+	.destroy = ipu_crtc_cleanup,
 	.page_flip = ipu_page_flip,
 };
 
@@ -551,8 +556,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master,
 {
 	struct ipu_crtc *ipu_crtc = dev_get_drvdata(dev);
 
-	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
-
 	destroy_workqueue(ipu_crtc->flip_queue);
 	ipu_plane_put_resources(ipu_crtc->plane[0]);
 	ipu_put_resources(ipu_crtc);
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 2d1fd02cd3d6..a1fa8fc91336 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -248,9 +248,6 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
 {
 	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
 
-	imxpd->encoder.funcs->destroy(&imxpd->encoder);
-	imxpd->connector.funcs->destroy(&imxpd->connector);
-
 	kfree(imxpd->edid);
 }
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/imx: disable outputs in lastclose when framebuffer emulation is disabled

[Daniel Vetter]

On Fri, Jun 17, 2016 at 12:13:38PM +0200, Lucas Stach wrote:
> If there is no framebuffer mode that can be restored, all outputs should
> be disabled in order to avoid information leaks.

No, this was a short-term regression that's now fixed again. When a client
closes or calls rmfb, we make sure that buffer isn't in use any more. This
shouldn't be needed any more.

Also, a generic fix in driver code. Tsk! ;-)

Cheers, Daniel

> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 82656654fb21..c63378661e11 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -63,7 +63,19 @@ static void imx_drm_driver_lastclose(struct drm_device *drm)
>  {
>  	struct imx_drm_device *imxdrm = drm->dev_private;
>  
> -	drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
> +	if (imxdrm->fbhelper) {
> +		drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
> +	} else {
> +		struct drm_connector *connector;
> +
> +		/* no kernel mode to go back to, disable all outputs */
> +		drm_modeset_lock_all(drm);
> +		drm_for_each_connector(connector, drm)
> +			connector->encoder = NULL;
> +		drm_modeset_unlock_all(drm);
> +
> +		drm_helper_disable_unused_functions(drm);
> +	}
>  }
>  
>  static int imx_drm_driver_unload(struct drm_device *drm)
> -- 
> 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

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

[Daniel Vetter]

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.

> +	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

Re: [PATCH 5/5] drm/imx: don't destroy mode objects manually on driver unbind

[Philipp Zabel]

Am Freitag, den 17.06.2016, 12:13 +0200 schrieb Lucas Stach:
> Instead let drm_mode_config_cleanup() do the work when taking down
> the master device. This requires all cleanup functions to be
> properly hooked up to the mode object .destroy callback.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c       | 3 ---
>  drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
>  drivers/gpu/drm/imx/imx-ldb.c          | 6 ------
>  drivers/gpu/drm/imx/imx-tve.c          | 3 ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c       | 9 ++++++---
>  drivers/gpu/drm/imx/parallel-display.c | 3 ---
>  6 files changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index c9d941283d30..5f97977f7e5c 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1834,9 +1834,6 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
>  	/* Disable all interrupts */
>  	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
>  
> -	hdmi->connector.funcs->destroy(&hdmi->connector);
> -	hdmi->encoder->funcs->destroy(hdmi->encoder);
> -
>  	clk_disable_unprepare(hdmi->iahb_clk);
>  	clk_disable_unprepare(hdmi->isfr_clk);
>  	i2c_put_adapter(hdmi->ddc);

The rockchip driver already calls drm_mode_config_cleanup after
component_unbind_all, so I this change should work for the rockchip
driver as is?

> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 799a68976590..71e33666cae8 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -466,11 +466,11 @@ static void imx_drm_unbind(struct device *dev)
>  	if (fbhelper)
>  		drm_fbdev_cma_fini(fbhelper);
>  
> +	drm_mode_config_cleanup(drm);
> +
>  	component_unbind_all(drm->dev, drm);
>  	dev_set_drvdata(dev, NULL);
>  
> -	drm_mode_config_cleanup(drm);
> -
>  	drm_dev_unref(drm);
>  }

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/imx: imx-ldb: detach panel on unbind

[Philipp Zabel]

Am Freitag, den 17.06.2016, 12:13 +0200 schrieb Lucas Stach:
> Make sure to leave a clean panel state behind and allow to
> properly attach to the panel again on a rebind.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-ldb.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 48166df14042..9e117a654417 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -671,6 +671,9 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
>  	for (i = 0; i < 2; i++) {
>  		struct imx_ldb_channel *channel = &imx_ldb->channel[i];
>  
> +		if (channel->panel)
> +			drm_panel_detach(channel->panel);

I think this should also be done in the bind error path.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ying Liu]

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

Re: [PATCH 2/5] drm/imx: imx-ldb: check return code on panel attach

[Philipp Zabel]

Am Freitag, den 17.06.2016, 12:13 +0200 schrieb Lucas Stach:
> Check the return code on panel attach. Avoids a kernel crash later
> on if the attach failed.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-ldb.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index beff793bb717..48166df14042 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -427,8 +427,12 @@ static int imx_ldb_register(struct drm_device *drm,
>  	drm_connector_init(drm, &imx_ldb_ch->connector,
>  			   &imx_ldb_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
>  
> -	if (imx_ldb_ch->panel)
> -		drm_panel_attach(imx_ldb_ch->panel, &imx_ldb_ch->connector);
> +	if (imx_ldb_ch->panel) {
> +		ret = drm_panel_attach(imx_ldb_ch->panel,
> +				       &imx_ldb_ch->connector);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	drm_mode_connector_attach_encoder(&imx_ldb_ch->connector,
>  			&imx_ldb_ch->encoder);

Applied, thanks.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel