[PATCH v3 0/4] drm/omap: fix issues in omap_drv probe/remove

[PATCH v3 0/4] drm/omap: fix issues in omap_drv probe/remove

[Archit Taneja]

At the moment, the omapdrm driver doesn't work on panda/beagle when built-in
the kernel. The problem is that omapdrm doesn't defer probe if resources like
regulator/I2C etc are missing. The first patch fixes that.

The next 3 patches make sure that omapdrm module can be inserted and removed
successively, and that omapdss is left in a consistent state when omapdrm is
removed.

In the previous version of this series, there was a warning related to apply_irq
being registered seen while removing omapdrm. This was a relatively scary
warning, therefore, the "change dev_unload order" patch was added to make sure
we don't see that.

After these fixes, I still see the warning below once in a while. I don't know
how to fix it at the moment, but it's harmless and omapdrm is now usable again.
These are critical fixes which I have posted since 3.12-rcs, it would be nice if
they can go in as soon as possible.

# rmmod omapdrm.ko
[  108.826568] [drm:drm_crtc_helper_set_config],
[  108.826568] [drm:drm_crtc_helper_set_config], [CRTC:7] [FB:15] #connectors=1 (x y) (0 0)
[  108.840362] [drm:drm_crtc_helper_set_config], [CONNECTOR:4:HDMI-A-1] to [CRTC:7]
[  108.844879] [drm:drm_crtc_helper_set_config],
[  108.852813] [drm:drm_crtc_helper_set_config], [CRTC:9] [NOFB]
[  108.866821] drm_kms_helper: drm: unregistered panic notifier
[  108.901977] [drm:drm_crtc_helper_set_config],
[  108.901977] [drm:drm_crtc_helper_set_config], [CRTC:7] [NOFB]
[  108.918731] ------------[ cut here ]------------
[  108.918731] WARNING: CPU: 1 PID: 1611 at drivers/gpu/drm/drm_crtc.c:4079 drm_mode_config_cleanup+0x238/0x250 [drm]()
[  108.918731] Modules linked in: omapdrm(-) connector_hdmi encoder_tpd12s015 omapdss sysfillrect sysimgblt syscopyarea fb_sys_fops drm_kms_helper drm
[  108.918975] CPU: 1 PID: 1611 Comm: rmmod Not tainted 3.13.0-rc5-00003-gdf839f2 #137
[  108.918975] [<c0015ef8>] (unwind_backtrace+0x0/0xf0) from [<c0012c9c>] (show_stack+0x10/0x14)
[  108.918975] [<c0012c9c>] (show_stack+0x10/0x14) from [<c0591f58>] (dump_stack+0x70/0x8c)
[  108.919036] [<c0591f58>] (dump_stack+0x70/0x8c) from [<c0042f30>] (warn_slowpath_common+0x6c/0x8c)
[  108.919067] [<c0042f30>] (warn_slowpath_common+0x6c/0x8c) from [<c0042f6c>] (warn_slowpath_null+0x1c/0x24)
[  108.919067] [<c0042f6c>] (warn_slowpath_null+0x1c/0x24) from [<bf00f3a4>] (drm_mode_config_cleanup+0x238/0x250 [drm])
[  108.919067] [<bf00f3a4>] (drm_mode_config_cleanup+0x238/0x250 [drm]) from [<bf0e7454>] (dev_unload+0x40/0x94 [omapdrm])
[  108.919067] [<bf0e7454>] (dev_unload+0x40/0x94 [omapdrm]) from [<bf008830>] (drm_dev_unregister+0x24/0xb4 [drm])
[  108.919067] [<bf008830>] (drm_dev_unregister+0x24/0xb4 [drm]) from [<bf0090dc>] (drm_put_dev+0x2c/0x58 [drm])
[  108.919067] [<bf0090dc>] (drm_put_dev+0x2c/0x58 [drm]) from [<bf00a52c>] (drm_platform_exit+0x30/0x5c [drm])
[  108.919067] [<bf00a52c>] (drm_platform_exit+0x30/0x5c [drm]) from [<bf0e7604>] (pdev_remove+0x28/0x54 [omapdrm])
[  108.919067] [<bf0e7604>] (pdev_remove+0x28/0x54 [omapdrm]) from [<c037ddb8>] (platform_drv_remove+0x18/0x1c)
[  108.920074] [<c037ddb8>] (platform_drv_remove+0x18/0x1c) from [<c037c534>] (__device_release_driver+0x70/0xcc)
[  108.920074] [<c037c534>] (__device_release_driver+0x70/0xcc) from [<c037cd64>] (driver_detach+0xb4/0xb8)
[  108.920104] [<c037cd64>] (driver_detach+0xb4/0xb8) from [<c037c35c>] (bus_remove_driver+0x4c/0x90)
[  108.920135] [<c037c35c>] (bus_remove_driver+0x4c/0x90) from [<c00b179c>] (SyS_delete_module+0x120/0x1a0)
[  108.920135] [<c00b179c>] (SyS_delete_module+0x120/0x1a0) from [<c000ea60>] (ret_fast_syscall+0x0/0x48)
[  108.920135] ---[ end trace ea955a36fdb8bfbc ]---


Archit Taneja (4):
  drm/omap: fix: Defer probe if an omapdss device requests for it at
    connect
  drm/omap: fix: disconnect devices when omapdrm module is removed
  drm/omap: fix: disable encoder before destroying it
  drm/omap: fix: change dev_unload order

 drivers/gpu/drm/omapdrm/omap_crtc.c    |  5 ++
 drivers/gpu/drm/omapdrm/omap_drv.c     | 83 ++++++++++++++++++++++++----------
 drivers/gpu/drm/omapdrm/omap_drv.h     |  1 +
 drivers/gpu/drm/omapdrm/omap_encoder.c |  3 ++
 4 files changed, 67 insertions(+), 25 deletions(-)

-- 
1.8.3.2

[PATCH v3 1/4] drm/omap: fix: Defer probe if an omapdss device requests for it at connect

[Archit Taneja]

With the omapdss device model changes. omapdrm is required to call dssdriver's
connect() op to register a panel. This is currently done in omap_modeset_init()

A call to connect() can fail if the omapdss panels or the encoders(HDMI/DPI)
they connect to have some resource(like regulators, I2C adapter) missing. If
this happens, the correct approach is to defer omapdrm's probe.

omapdrm currently ignores those panels which return a non zero value when
connected. This could result in omapdrm ignoring all panels on an omap board.

The right approach would be for omapdrm to request for probe deferral when a
panel's connect op returns -EPROBE_DEFER.

In order to do this, we need to call connect() much earlier during omapdrm's
probe to prevent too many things are already done by then. We now connect the
panels during pdev_probe(), before anything else is initialized, so that we
don't need to undo too many things if a defer was requested.

Now when we enter omap_modeset_init(), we have a set of panels that have been
connected. We now proceed with registering only those panels that are already
connected.

A special case has to be considered when no panels are available to connect when
omapdrm probes. In this case too, we defer probe and expect that a panel will be
available to connect the next time.

Checking whether the panel has a driver or whether it has get_timing/read_edid
ops in omap_modeset_init() are redundant with the new display model. These can
be removed since a dssdev device will always have a driver associated with it,
and all dssdev drivers have a get_timings op.

This will mainly fix cases when omapdrm is built-in the kernel, since that's
generally where resources like regulators or I2C are unavailable because of
probe order dependencies.

In particular this fixes boot with omapdrm built-in on an omap4 panda ES board.
The regulators used by HDMI(provided by I2C based TWL regulators) aren't
initialized because I2C isn't initialized, I2C isn't initialized as it's pins
are not configured because pinctrl is yet to probe.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c |  5 +++
 drivers/gpu/drm/omapdrm/omap_drv.c  | 70 +++++++++++++++++++++++++------------
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 3 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0fd2eb1..9c01311 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
 	dss_install_mgr_ops(&mgr_ops);
 }
 
+void omap_crtc_pre_uninit(void)
+{
+	dss_uninstall_mgr_ops();
+}
+
 /* initialize crtc */
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index e7fa3cd..651f902 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -87,6 +87,43 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
 	return false;
 }
 
+static int omap_connect_dssdevs(void)
+{
+	int r;
+	struct omap_dss_device *dssdev = NULL;
+	bool no_displays = true;
+
+	for_each_dss_dev(dssdev) {
+		r = dssdev->driver->connect(dssdev);
+		if (r == -EPROBE_DEFER) {
+			omap_dss_put_device(dssdev);
+			goto cleanup;
+		} else if (r) {
+			dev_warn(dssdev->dev, "could not connect display: %s\n",
+				dssdev->name);
+		} else {
+			no_displays = false;
+		}
+	}
+
+	if (no_displays)
+		return -EPROBE_DEFER;
+
+	return 0;
+
+cleanup:
+	/*
+	 * if we are deferring probe, we disconnect the devices we previously
+	 * connected
+	 */
+	dssdev = NULL;
+
+	for_each_dss_dev(dssdev)
+		dssdev->driver->disconnect(dssdev);
+
+	return r;
+}
+
 static int omap_modeset_init(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
@@ -95,9 +132,6 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_mgrs = dss_feat_get_num_mgrs();
 	int num_crtcs;
 	int i, id = 0;
-	int r;
-
-	omap_crtc_pre_init();
 
 	drm_mode_config_init(dev);
 
@@ -119,26 +153,8 @@ static int omap_modeset_init(struct drm_device *dev)
 		enum omap_channel channel;
 		struct omap_overlay_manager *mgr;
 
-		if (!dssdev->driver) {
-			dev_warn(dev->dev, "%s has no driver.. skipping it\n",
-					dssdev->name);
-			continue;
-		}
-
-		if (!(dssdev->driver->get_timings ||
-					dssdev->driver->read_edid)) {
-			dev_warn(dev->dev, "%s driver does not support "
-				"get_timings or read_edid.. skipping it!\n",
-				dssdev->name);
-			continue;
-		}
-
-		r = dssdev->driver->connect(dssdev);
-		if (r) {
-			dev_err(dev->dev, "could not connect display: %s\n",
-					dssdev->name);
+		if (!omapdss_device_is_connected(dssdev))
 			continue;
-		}
 
 		encoder = omap_encoder_init(dev, dssdev);
 
@@ -655,9 +671,19 @@ static void pdev_shutdown(struct platform_device *device)
 
 static int pdev_probe(struct platform_device *device)
 {
+	int r;
+
 	if (omapdss_is_initialized() == false)
 		return -EPROBE_DEFER;
 
+	omap_crtc_pre_init();
+
+	r = omap_connect_dssdevs();
+	if (r) {
+		omap_crtc_pre_uninit();
+		return r;
+	}
+
 	DBG("%s", device->name);
 	return drm_platform_init(&omap_drm_driver, device);
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 0784769..f6c2236 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -158,6 +158,7 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc);
 int omap_crtc_apply(struct drm_crtc *crtc,
 		struct omap_drm_apply *apply);
 void omap_crtc_pre_init(void);
+void omap_crtc_pre_uninit(void);
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id);
 
-- 
1.8.3.2

[PATCH v3 2/4] drm/omap: fix: disconnect devices when omapdrm module is removed

[Archit Taneja]

At omapdrm probe, we install manager ops and connect omapdss devices. This
needs to be undone when omapdrm module is removed so that omapdss is in a
clean state. This ensures that we can re-insert omapdrm module, or some other
module which uses omapdss(like omapfb/omap_vout).

Currently, omapdrm's remove neither uninstalls manager ops, or disconnects
omapdss devices. We make sure that this is done in pdev_remove.

omapdrm establishes connections for omap_dss_device devices when probed. It
should also be responsible to disconnect the devices. Keeping the devices
connected can prevent the panel driver modules from unloading, it also causes
issues when we try to remove or re-insert omapdrm module.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 651f902..fca5667 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -86,6 +86,13 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
 
 	return false;
 }
+static void omap_disconnect_dssdevs(void)
+{
+	struct omap_dss_device *dssdev = NULL;
+
+	for_each_dss_dev(dssdev)
+		dssdev->driver->disconnect(dssdev);
+}
 
 static int omap_connect_dssdevs(void)
 {
@@ -116,10 +123,7 @@ cleanup:
 	 * if we are deferring probe, we disconnect the devices we previously
 	 * connected
 	 */
-	dssdev = NULL;
-
-	for_each_dss_dev(dssdev)
-		dssdev->driver->disconnect(dssdev);
+	omap_disconnect_dssdevs();
 
 	return r;
 }
@@ -693,6 +697,9 @@ static int pdev_remove(struct platform_device *device)
 	DBG("");
 	drm_platform_exit(&omap_drm_driver, device);
 
+	omap_disconnect_dssdevs();
+	omap_crtc_pre_uninit();
+
 	platform_driver_unregister(&omap_dmm_driver);
 	return 0;
 }
-- 
1.8.3.2

[PATCH v3 3/4] drm/omap: fix: disable encoder before destroying it

[Archit Taneja]

Currently, an encoder is disabled only when an apply work is queued for the
corresponding crtc. This works fine for the paths where userspace explicitly
disables crtc, this results in disabling the omapdss device in the crtc's
pre_apply function.

However, when the omapdrm module is removed, there is no work queued to ensure
that the encoder is disabled. This can result in an enabled omapdss device when
removing omapdrm. omapdss is left in an inconsistent state, and that prevents us
from using that omapdss device being used again.

Disable enabled encoders in omap_encoder_destroy, we could consider a better
place for doing this later.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_encoder.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index 6a12e89..5290a88 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -51,6 +51,9 @@ struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder)
 static void omap_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+
+	omap_encoder_set_enabled(encoder, false);
+
 	drm_encoder_cleanup(encoder);
 	kfree(omap_encoder);
 }
-- 
1.8.3.2

[PATCH v3 4/4] drm/omap: fix: change dev_unload order

[Archit Taneja]

The current dev_unload order uninits the irqs too early.

In the current sequence, it's possible that a crtc queues work(apply_worker)
to display a buffer, which registers to omap_crtc_apply_irq to notfiy the
completion of the configuration we applied.

Calling drm_vblank_cleanup and omap_drm_irq_uninstall here causes the crtc's
apply handler to never get called, which results in an incorrect state of the
apply_irq.registered parameter.

This condition occurs where there is no mode set via omapdrm, and dev_lastclose
tries to set a default fb mode via drm_fb_helper_restore_fbdev_mode. The apply
work scheduled by restore_fbdev_mode is very close in time to the disabling of
the irq handler, and hence leads to a race condition. We move the irq cleanup
at the end of the unload sequence to prevent this.

Also, the call to flush_workqueue is removed since it's called internally by
destroy_workqueue.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index fca5667..c57e99c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -517,16 +517,16 @@ static int dev_unload(struct drm_device *dev)
 	DBG("unload: dev=%p", dev);
 
 	drm_kms_helper_poll_fini(dev);
-	drm_vblank_cleanup(dev);
-	omap_drm_irq_uninstall(dev);
 
 	omap_fbdev_free(dev);
 	omap_modeset_free(dev);
 	omap_gem_deinit(dev);
 
-	flush_workqueue(priv->wq);
 	destroy_workqueue(priv->wq);
 
+	drm_vblank_cleanup(dev);
+	omap_drm_irq_uninstall(dev);
+
 	kfree(dev->dev_private);
 	dev->dev_private = NULL;
 
-- 
1.8.3.2

Re: [PATCH v3 0/4] drm/omap: fix issues in omap_drv probe/remove

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On 2014-01-02 11:19, Archit Taneja wrote:
> At the moment, the omapdrm driver doesn't work on panda/beagle when built-in
> the kernel. The problem is that omapdrm doesn't defer probe if resources like
> regulator/I2C etc are missing. The first patch fixes that.
> 
> The next 3 patches make sure that omapdrm module can be inserted and removed
> successively, and that omapdss is left in a consistent state when omapdrm is
> removed.
> 
> In the previous version of this series, there was a warning related to apply_irq
> being registered seen while removing omapdrm. This was a relatively scary
> warning, therefore, the "change dev_unload order" patch was added to make sure
> we don't see that.
> 
> After these fixes, I still see the warning below once in a while. I don't know
> how to fix it at the moment, but it's harmless and omapdrm is now usable again.
> These are critical fixes which I have posted since 3.12-rcs, it would be nice if
> they can go in as soon as possible.

These look ok to me:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]