[RFC 0/3] solving omapdrm/omapdss layering issues

[RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

From: Rob Clark <rob@ti.com>

I've been working for the better part of the week on solving some of
the omapdss vs kms mismatches, which is one of the bigger remaining
issues in the TODO before moving omapdrm out of staging.

The biggest place that this shows up is in GO bit handling.  Basically,
some of the scanout related registers in DSS hw block are only shadow
registers, and if the GO bit is set during the vblank the hw copies
into the actual registers (not accessible to CPU) and clears the GO
bit.  When the GO bit is set, there is no safe way to update these
registers without undefined results.  So omapdss tries to be friendly
and abstract this, by buffering up the updated state, and applying it
on the next vblank once the GO bit is cleared.  But this causes all
sorts of mayhem at the omapdrm layer, which would like to unpin the
previous scanout buffer(s) on the next vblank (or endwin) irq.  Due
to the buffering in omapdss, we have no way to know on a vblank if we
have switched to the scanout buffer or not.  Basically it works ok as
long as userspace is only ever updating on layer (either crtc or drm
plane) at a time.  But throw together hw mouse cursor (drm plane)
plus a window manager like compiz which does page flips, or wayland
(weston drm compositor) with hw composition (drm plane), and things
start to fail in a big way.

I've tried a few approaches to preserve the omapdss more or less as it
is, by adding callbacks for when GO bit is cleared, etc.  But the
sequencing of setting up connector/encoder/crtc is not really what
omapdss expects, and would generally end up confusing the "apply"
layer in omapdss (it would end up not programming various registers
because various dirty flags would get cleared, for example mgr updated
before overlay connected, etc).

Finally, in frustration, this afternoon I hit upon an idea.  Why not
just use the dispc code in omapdss, which is basically a stateless
layer of helper functions, and bypass the stateful layer of omapdss.
Since KMS helper functions already give us the correct sequence for
setting up the hardware, this turned out to be rather easy.  And fit
it quite nicely with my mechanism to queue up updates when the GO bit
is not clear.  And, unlike my previous attempts, it actually worked..
not only that, but it worked on the first boot!

So I am pretty happy about how this is shaping up.  Not only is it
simpler that my previous attepmts, and solves a few tricky buffer
unpin related issues.  But it also makes it very easy to wire in the
missing userspace vblank event handling without resorting to duct-
tape.

Obviously there is stuff still missing, and some hacks.  This is
really just a proof of concept at this stage.  But I wanted to send an
RFC so we could start discussing how to move forward.  Ie. could we
reasonably add support to build dispc as a library of stateless helper
functions, sharing it and the panel drivers between omapdrm and the
legacy omapdss based drivers.  Or is there no clean way to do that, in
which case we should just copy the code we need into omapdrm, and
leave the deprecated omapdss as it is for legacy drivers.

Rob Clark (3):
  OMAPDSS: expose dispc for use from omapdrm
  omap2+: use dss_dispc hwmod for omapdrm
  drm/omap: use dispc directly

 arch/arm/mach-omap2/drm.c                |   24 +-
 drivers/staging/omapdrm/Makefile         |    1 +
 drivers/staging/omapdrm/omap_connector.c |   20 +-
 drivers/staging/omapdrm/omap_crtc.c      |   42 ++--
 drivers/staging/omapdrm/omap_drv.c       |   70 +-----
 drivers/staging/omapdrm/omap_drv.h       |   64 ++++-
 drivers/staging/omapdrm/omap_encoder.c   |  213 ++++++++++++++---
 drivers/staging/omapdrm/omap_irq.c       |  133 +++++++++++
 drivers/staging/omapdrm/omap_plane.c     |  382 +++++++++---------------------
 drivers/video/omap2/dss/apply.c          |    4 +-
 drivers/video/omap2/dss/dispc.c          |   76 ++++--
 drivers/video/omap2/dss/dss.h            |    2 +
 drivers/video/omap2/dss/hdmi.c           |   18 +-
 13 files changed, 628 insertions(+), 421 deletions(-)
 create mode 100644 drivers/staging/omapdrm/omap_irq.c

-- 
1.7.9.5

[RFC 1/3] OMAPDSS: expose dispc for use from omapdrm

[Rob Clark]

From: Rob Clark <rob@ti.com>

Not very clean, just for proof of concept.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/video/omap2/dss/apply.c |    4 ++-
 drivers/video/omap2/dss/dispc.c |   76 ++++++++++++++++++++++++++++++---------
 drivers/video/omap2/dss/dss.h   |    2 ++
 drivers/video/omap2/dss/hdmi.c  |   18 +++++-----
 4 files changed, 73 insertions(+), 27 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index ab22cc2..c9a9b80 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -699,6 +699,8 @@ static void dss_write_regs(void)
 
 static void dss_set_go_bits(void)
 {
+	dump_stack();
+#if 0
 	const int num_mgrs = omap_dss_get_num_overlay_managers();
 	int i;
 
@@ -722,7 +724,7 @@ static void dss_set_go_bits(void)
 
 		dispc_mgr_go(mgr->id);
 	}
-
+#endif
 }
 
 static void mgr_clear_shadow_dirty(struct omap_overlay_manager *mgr)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index e6ea47e..bdb0bde 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -352,7 +352,7 @@ static void dispc_restore_context(void)
 	if (dss_has_feature(FEAT_MGR_LCD2))
 		RR(CONTROL2);
 	/* clear spurious SYNC_LOST_DIGIT interrupts */
-	dispc_write_reg(DISPC_IRQSTATUS, DISPC_IRQ_SYNC_LOST_DIGIT);
+	dispc_clear_irqs(DISPC_IRQ_SYNC_LOST_DIGIT);
 
 	/*
 	 * enable last so IRQs won't trigger before
@@ -376,6 +376,7 @@ int dispc_runtime_get(void)
 	WARN_ON(r < 0);
 	return r < 0 ? r : 0;
 }
+EXPORT_SYMBOL_GPL(dispc_runtime_get);
 
 void dispc_runtime_put(void)
 {
@@ -386,6 +387,7 @@ void dispc_runtime_put(void)
 	r = pm_runtime_put_sync(&dispc.pdev->dev);
 	WARN_ON(r < 0 && r != -ENOSYS);
 }
+EXPORT_SYMBOL_GPL(dispc_runtime_put);
 
 static inline bool dispc_mgr_is_lcd(enum omap_channel channel)
 {
@@ -410,6 +412,7 @@ u32 dispc_mgr_get_vsync_irq(enum omap_channel channel)
 		return 0;
 	}
 }
+EXPORT_SYMBOL_GPL(dispc_mgr_get_vsync_irq);
 
 u32 dispc_mgr_get_framedone_irq(enum omap_channel channel)
 {
@@ -440,6 +443,7 @@ bool dispc_mgr_go_busy(enum omap_channel channel)
 	else
 		return REG_GET(DISPC_CONTROL, bit, bit) == 1;
 }
+EXPORT_SYMBOL_GPL(dispc_mgr_go_busy);
 
 void dispc_mgr_go(enum omap_channel channel)
 {
@@ -483,6 +487,7 @@ void dispc_mgr_go(enum omap_channel channel)
 	else
 		REG_FLD_MOD(DISPC_CONTROL, 1, bit, bit);
 }
+EXPORT_SYMBOL_GPL(dispc_mgr_go);
 
 static void dispc_ovl_write_firh_reg(enum omap_plane plane, int reg, u32 value)
 {
@@ -844,6 +849,7 @@ void dispc_ovl_set_channel_out(enum omap_plane plane, enum omap_channel channel)
 	}
 	dispc_write_reg(DISPC_OVL_ATTRIBUTES(plane), val);
 }
+EXPORT_SYMBOL_GPL(dispc_ovl_set_channel_out);
 
 static enum omap_channel dispc_ovl_get_channel_out(enum omap_plane plane)
 {
@@ -2236,6 +2242,7 @@ int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dispc_ovl_setup);
 
 int dispc_ovl_enable(enum omap_plane plane, bool enable)
 {
@@ -2256,6 +2263,7 @@ static void dispc_disable_isr(void *data, u32 mask)
 	struct completion *compl = data;
 	complete(compl);
 }
+EXPORT_SYMBOL_GPL(dispc_ovl_enable);
 
 static void _enable_lcd_out(enum omap_channel channel, bool enable)
 {
@@ -2318,6 +2326,10 @@ static void _enable_digit_out(bool enable)
 	dispc_read_reg(DISPC_CONTROL);
 }
 
+/* TODO revisit how this and dispc_mgr_enable_lcd_out() should
+ * work w/ omapdrm handling the irqs..  ideally we'd just have
+ * a dispc helper fxn to call from the omapdrm irq handling.
+ */
 static void dispc_mgr_enable_digit_out(bool enable)
 {
 	struct completion frame_done_completion;
@@ -2381,7 +2393,7 @@ static void dispc_mgr_enable_digit_out(bool enable)
 		unsigned long flags;
 		spin_lock_irqsave(&dispc.irq_lock, flags);
 		dispc.irq_error_mask |= DISPC_IRQ_SYNC_LOST_DIGIT;
-		dispc_write_reg(DISPC_IRQSTATUS, DISPC_IRQ_SYNC_LOST_DIGIT);
+		dispc_clear_irqs(DISPC_IRQ_SYNC_LOST_DIGIT);
 		_omap_dispc_set_irqs();
 		spin_unlock_irqrestore(&dispc.irq_lock, flags);
 	}
@@ -2410,6 +2422,7 @@ void dispc_mgr_enable(enum omap_channel channel, bool enable)
 	else
 		BUG();
 }
+EXPORT_SYMBOL_GPL(dispc_mgr_enable);
 
 void dispc_lcd_enable_signal_polarity(bool act_high)
 {
@@ -2529,6 +2542,7 @@ void dispc_mgr_setup(enum omap_channel channel,
 		dispc_mgr_set_cpr_coef(channel, &info->cpr_coefs);
 	}
 }
+EXPORT_SYMBOL_GPL(dispc_mgr_setup);
 
 void dispc_mgr_set_tft_data_lines(enum omap_channel channel, u8 data_lines)
 {
@@ -2705,6 +2719,7 @@ void dispc_mgr_set_timings(enum omap_channel channel,
 
 	dispc_mgr_set_size(channel, t.x_res, t.y_res);
 }
+EXPORT_SYMBOL_GPL(dispc_mgr_set_timings);
 
 static void dispc_mgr_set_lcd_divisor(enum omap_channel channel, u16 lck_div,
 		u16 pck_div)
@@ -3224,11 +3239,33 @@ int dispc_mgr_get_clock_div(enum omap_channel channel,
 	return 0;
 }
 
+u32 dispc_read_irqs(void)
+{
+	return dispc_read_reg(DISPC_IRQSTATUS);
+}
+EXPORT_SYMBOL_GPL(dispc_read_irqs);
+
+void dispc_clear_irqs(u32 mask)
+{
+	dispc_write_reg(DISPC_IRQSTATUS, mask);
+}
+EXPORT_SYMBOL_GPL(dispc_clear_irqs);
+
+void dispc_set_irqs(u32 mask)
+{
+	u32 old_mask = dispc_read_reg(DISPC_IRQENABLE);
+
+	/* clear the irqstatus for newly enabled irqs */
+	dispc_clear_irqs((mask ^ old_mask) & mask);
+
+	dispc_write_reg(DISPC_IRQENABLE, mask);
+}
+EXPORT_SYMBOL_GPL(dispc_set_irqs);
+
 /* dispc.irq_lock has to be locked by the caller */
 static void _omap_dispc_set_irqs(void)
 {
 	u32 mask;
-	u32 old_mask;
 	int i;
 	struct omap_dispc_isr_data *isr_data;
 
@@ -3243,11 +3280,7 @@ static void _omap_dispc_set_irqs(void)
 		mask |= isr_data->mask;
 	}
 
-	old_mask = dispc_read_reg(DISPC_IRQENABLE);
-	/* clear the irqstatus for newly enabled irqs */
-	dispc_write_reg(DISPC_IRQSTATUS, (mask ^ old_mask) & mask);
-
-	dispc_write_reg(DISPC_IRQENABLE, mask);
+	dispc_set_irqs(mask);
 }
 
 int omap_dispc_register_isr(omap_dispc_isr_t isr, void *arg, u32 mask)
@@ -3380,7 +3413,7 @@ static irqreturn_t omap_dispc_irq_handler(int irq, void *arg)
 
 	spin_lock(&dispc.irq_lock);
 
-	irqstatus = dispc_read_reg(DISPC_IRQSTATUS);
+	irqstatus = dispc_read_irqs();
 	irqenable = dispc_read_reg(DISPC_IRQENABLE);
 
 	/* IRQ is not for us */
@@ -3402,9 +3435,9 @@ static irqreturn_t omap_dispc_irq_handler(int irq, void *arg)
 #endif
 	/* Ack the interrupt. Do it here before clocks are possibly turned
 	 * off */
-	dispc_write_reg(DISPC_IRQSTATUS, irqstatus);
+	dispc_clear_irqs(irqstatus);
 	/* flush posted write */
-	dispc_read_reg(DISPC_IRQSTATUS);
+	dispc_read_irqs();
 
 	/* make a copy and unlock, so that isrs can unregister
 	 * themselves */
@@ -3597,6 +3630,17 @@ int omap_dispc_wait_for_irq_interruptible_timeout(u32 irqmask,
 	return 0;
 }
 
+u32 dispc_error_irqs(void)
+{
+	u32 mask = DISPC_IRQ_MASK_ERROR;
+	if (dss_has_feature(FEAT_MGR_LCD2))
+		mask |= DISPC_IRQ_SYNC_LOST2;
+	if (dss_feat_get_num_ovls() > 3)
+		mask |= DISPC_IRQ_VID3_FIFO_UNDERFLOW;
+	return mask;
+}
+EXPORT_SYMBOL_GPL(dispc_error_irqs);
+
 static void _omap_dispc_initialize_irq(void)
 {
 	unsigned long flags;
@@ -3605,15 +3649,11 @@ static void _omap_dispc_initialize_irq(void)
 
 	memset(dispc.registered_isr, 0, sizeof(dispc.registered_isr));
 
-	dispc.irq_error_mask = DISPC_IRQ_MASK_ERROR;
-	if (dss_has_feature(FEAT_MGR_LCD2))
-		dispc.irq_error_mask |= DISPC_IRQ_SYNC_LOST2;
-	if (dss_feat_get_num_ovls() > 3)
-		dispc.irq_error_mask |= DISPC_IRQ_VID3_FIFO_UNDERFLOW;
+	dispc.irq_error_mask = dispc_error_irqs();
 
 	/* there's SYNC_LOST_DIGIT waiting after enabling the DSS,
 	 * so clear it */
-	dispc_write_reg(DISPC_IRQSTATUS, dispc_read_reg(DISPC_IRQSTATUS));
+	dispc_clear_irqs(dispc_read_irqs());
 
 	_omap_dispc_set_irqs();
 
@@ -3690,6 +3730,7 @@ static int __init omap_dispchw_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+#if 0
 	dispc.irq = platform_get_irq(dispc.pdev, 0);
 	if (dispc.irq < 0) {
 		DSSERR("platform_get_irq failed\n");
@@ -3702,6 +3743,7 @@ static int __init omap_dispchw_probe(struct platform_device *pdev)
 		DSSERR("request_irq failed\n");
 		return r;
 	}
+#endif
 
 	clk = clk_get(&pdev->dev, "fck");
 	if (IS_ERR(clk)) {
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index dd1092c..31affc9 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -370,6 +370,8 @@ int dispc_init_platform_driver(void) __init;
 void dispc_uninit_platform_driver(void) __exit;
 void dispc_dump_clocks(struct seq_file *s);
 void dispc_irq_handler(void);
+u32 dispc_read_irqs(void);
+void dispc_clear_irqs(u32 mask);
 
 int dispc_runtime_get(void);
 void dispc_runtime_put(void);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 26a2430..8ca7d71 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -296,7 +296,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	if (r)
 		return r;
 
-	dss_mgr_disable(dssdev->manager);
+//	dss_mgr_disable(dssdev->manager);
 
 	p = &dssdev->panel.timings;
 
@@ -350,20 +350,20 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	dispc_enable_gamma_table(0);
 
 	/* tv size */
-	dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
+//	dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 
 	r = hdmi.ip_data.ops->video_enable(&hdmi.ip_data);
 	if (r)
 		goto err_vid_enable;
 
-	r = dss_mgr_enable(dssdev->manager);
-	if (r)
-		goto err_mgr_enable;
+//	r = dss_mgr_enable(dssdev->manager);
+//	if (r)
+//		goto err_mgr_enable;
 
 	return 0;
 
-err_mgr_enable:
-	hdmi.ip_data.ops->video_disable(&hdmi.ip_data);
+//err_mgr_enable:
+//	hdmi.ip_data.ops->video_disable(&hdmi.ip_data);
 err_vid_enable:
 	hdmi.ip_data.ops->phy_disable(&hdmi.ip_data);
 	hdmi.ip_data.ops->pll_disable(&hdmi.ip_data);
@@ -374,7 +374,7 @@ err:
 
 static void hdmi_power_off(struct omap_dss_device *dssdev)
 {
-	dss_mgr_disable(dssdev->manager);
+//	dss_mgr_disable(dssdev->manager);
 
 	hdmi.ip_data.ops->video_disable(&hdmi.ip_data);
 	hdmi.ip_data.ops->phy_disable(&hdmi.ip_data);
@@ -413,7 +413,7 @@ void omapdss_hdmi_display_set_timing(struct omap_dss_device *dssdev)
 		if (r)
 			DSSERR("failed to power on device\n");
 	} else {
-		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
+//		dss_mgr_set_timings(dssdev->manager, &dssdev->panel.timings);
 	}
 }
 
-- 
1.7.9.5

[RFC 2/3] omap2+: use dss_dispc hwmod for omapdrm

[Rob Clark]

From: Rob Clark <rob@ti.com>

We need this so that platform_get_irq() works when drm core sets up the
irq handling.

Signed-off-by: Rob Clark <rob@ti.com>
---
 arch/arm/mach-omap2/drm.c |   24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-omap2/drm.c b/arch/arm/mach-omap2/drm.c
index 4ab7b6a..f7a2444 100644
--- a/arch/arm/mach-omap2/drm.c
+++ b/arch/arm/mach-omap2/drm.c
@@ -33,18 +33,6 @@
 #include <plat/drm.h>
 
 #if defined(CONFIG_DRM_OMAP) || (CONFIG_DRM_OMAP_MODULE)
-
-static struct omap_drm_platform_data omapdrm_platdata;
-
-static struct platform_device omap_drm_device = {
-		.dev = {
-			.coherent_dma_mask = DMA_BIT_MASK(32),
-			.platform_data = &omapdrm_platdata,
-		},
-		.name = "omapdrm",
-		.id = 0,
-};
-
 static int __init omap_init_drm(void)
 {
 	struct omap_hwmod *oh = NULL;
@@ -60,8 +48,16 @@ static int __init omap_init_drm(void)
 			oh->name);
 	}
 
-	return platform_device_register(&omap_drm_device);
+	/* lookup and populate DSS information: */
+	oh = omap_hwmod_lookup("dss_dispc");
+	pdev = omap_device_build("omapdrm", -1, oh, NULL, 0, NULL, 0,
+			false);
+	WARN(IS_ERR(pdev), "Could not build omap_device for omapdrm\n");
 
+	if (!pdev)
+		return -EINVAL;
+
+	return 0;
 }
 
 arch_initcall(omap_init_drm);
@@ -69,12 +65,14 @@ arch_initcall(omap_init_drm);
 void __init omapdrm_reserve_vram(void)
 {
 #ifdef CONFIG_CMA
+#if 0 /* TODO add this back for omap3 */
 	/*
 	 * Create private 32MiB contiguous memory area for omapdrm.0 device
 	 * TODO revisit size.. if uc/wc buffers are allocated from CMA pages
 	 * then the amount of memory we need goes up..
 	 */
 	dma_declare_contiguous(&omap_drm_device.dev, 32 * SZ_1M, 0, 0);
+#endif
 #else
 #  warning "CMA is not enabled, there may be limitations about scanout buffer allocations on OMAP3 and earlier"
 #endif
-- 
1.7.9.5

[RFC 3/3] drm/omap: use dispc directly

[Rob Clark]

From: Rob Clark <rob@ti.com>

This is still work in progress, but it nicely solves the omapdss vs drm
impedence mismatches, and properly fixes unpin confusion vs GO bit status.
As an added bonus, we also no longer leave the last overlay buffer pinned.
Adding the previously missing vblank event handling for userspace should
be pretty trivial now too.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/staging/omapdrm/Makefile         |    1 +
 drivers/staging/omapdrm/omap_connector.c |   20 +-
 drivers/staging/omapdrm/omap_crtc.c      |   42 ++--
 drivers/staging/omapdrm/omap_drv.c       |   70 +-----
 drivers/staging/omapdrm/omap_drv.h       |   64 ++++-
 drivers/staging/omapdrm/omap_encoder.c   |  213 ++++++++++++++---
 drivers/staging/omapdrm/omap_irq.c       |  133 +++++++++++
 drivers/staging/omapdrm/omap_plane.c     |  382 +++++++++---------------------
 8 files changed, 544 insertions(+), 381 deletions(-)
 create mode 100644 drivers/staging/omapdrm/omap_irq.c

diff --git a/drivers/staging/omapdrm/Makefile b/drivers/staging/omapdrm/Makefile
index 1ca0e00..d85e058 100644
--- a/drivers/staging/omapdrm/Makefile
+++ b/drivers/staging/omapdrm/Makefile
@@ -5,6 +5,7 @@
 
 ccflags-y := -Iinclude/drm -Werror
 omapdrm-y := omap_drv.o \
+	omap_irq.o \
 	omap_debugfs.o \
 	omap_crtc.o \
 	omap_plane.o \
diff --git a/drivers/staging/omapdrm/omap_connector.c b/drivers/staging/omapdrm/omap_connector.c
index 5e2856c..577ae32 100644
--- a/drivers/staging/omapdrm/omap_connector.c
+++ b/drivers/staging/omapdrm/omap_connector.c
@@ -284,16 +284,17 @@ static const struct drm_connector_helper_funcs omap_connector_helper_funcs = {
 };
 
 /* called from encoder when mode is set, to propagate settings to the dssdev */
-void omap_connector_mode_set(struct drm_connector *connector,
-		struct drm_display_mode *mode)
+int omap_connector_mode_set(struct drm_connector *connector,
+		struct drm_display_mode *mode,
+		struct omap_video_timings *timings)
 {
 	struct drm_device *dev = connector->dev;
 	struct omap_connector *omap_connector = to_omap_connector(connector);
 	struct omap_dss_device *dssdev = omap_connector->dssdev;
 	struct omap_dss_driver *dssdrv = dssdev->driver;
-	struct omap_video_timings timings;
+	int ret;
 
-	copy_timings_drm_to_omap(&timings, mode);
+	copy_timings_drm_to_omap(timings, mode);
 
 	DBG("%s: set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x",
 			omap_connector->dssdev->name,
@@ -303,12 +304,15 @@ void omap_connector_mode_set(struct drm_connector *connector,
 			mode->vdisplay, mode->vsync_start,
 			mode->vsync_end, mode->vtotal, mode->type, mode->flags);
 
-	if (dssdrv->check_timings(dssdev, &timings)) {
-		dev_err(dev->dev, "could not set timings\n");
-		return;
+	ret = dssdrv->check_timings(dssdev, timings);
+	if (ret) {
+		dev_err(dev->dev, "could not set timings: %d\n", ret);
+		return ret;
 	}
 
-	dssdrv->set_timings(dssdev, &timings);
+	dssdrv->set_timings(dssdev, timings);
+
+	return 0;
 }
 
 /* flush an area of the framebuffer (in case of manual update display that
diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
index 7479dcb..3024dcd 100644
--- a/drivers/staging/omapdrm/omap_crtc.c
+++ b/drivers/staging/omapdrm/omap_crtc.c
@@ -33,7 +33,6 @@ struct omap_crtc {
 
 	/* if there is a pending flip, these will be non-null: */
 	struct drm_pending_vblank_event *event;
-	struct drm_framebuffer *old_fb;
 };
 
 static void omap_crtc_destroy(struct drm_crtc *crtc)
@@ -78,7 +77,8 @@ static int omap_crtc_mode_set(struct drm_crtc *crtc,
 	return omap_plane_mode_set(plane, crtc, crtc->fb,
 			0, 0, mode->hdisplay, mode->vdisplay,
 			x << 16, y << 16,
-			mode->hdisplay << 16, mode->vdisplay << 16);
+			mode->hdisplay << 16, mode->vdisplay << 16,
+			NULL, NULL);
 }
 
 static void omap_crtc_prepare(struct drm_crtc *crtc)
@@ -102,10 +102,11 @@ static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_plane *plane = omap_crtc->plane;
 	struct drm_display_mode *mode = &crtc->mode;
 
-	return plane->funcs->update_plane(plane, crtc, crtc->fb,
+	return omap_plane_mode_set(plane, crtc, crtc->fb,
 			0, 0, mode->hdisplay, mode->vdisplay,
 			x << 16, y << 16,
-			mode->hdisplay << 16, mode->vdisplay << 16);
+			mode->hdisplay << 16, mode->vdisplay << 16,
+			NULL, NULL);
 }
 
 static void omap_crtc_load_lut(struct drm_crtc *crtc)
@@ -154,17 +155,17 @@ static void page_flip_cb(void *arg)
 {
 	struct drm_crtc *crtc = arg;
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
-	struct drm_framebuffer *old_fb = omap_crtc->old_fb;
-
-	omap_crtc->old_fb = NULL;
-
-	omap_crtc_mode_set_base(crtc, crtc->x, crtc->y, old_fb);
+	struct drm_plane *plane = omap_crtc->plane;
+	struct drm_display_mode *mode = &crtc->mode;
 
-	/* really we'd like to setup the callback atomically w/ setting the
-	 * new scanout buffer to avoid getting stuck waiting an extra vblank
-	 * cycle.. for now go for correctness and later figure out speed..
-	 */
-	omap_plane_on_endwin(omap_crtc->plane, vblank_cb, crtc);
+	// XXX this needs to be shunted off to a worker.. at
+	// least right now we are making assumptions that everything is
+	// synchronized on mode_config.mutex
+	omap_plane_mode_set(plane, crtc, crtc->fb,
+			0, 0, mode->hdisplay, mode->vdisplay,
+			crtc->x << 16, crtc->y << 16,
+			mode->hdisplay << 16, mode->vdisplay << 16,
+			vblank_cb, arg);
 }
 
 static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
@@ -181,7 +182,6 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	omap_crtc->old_fb = crtc->fb;
 	omap_crtc->event = event;
 	crtc->fb = fb;
 
@@ -222,6 +222,18 @@ static const struct drm_crtc_helper_funcs omap_crtc_helper_funcs = {
 	.load_lut = omap_crtc_load_lut,
 };
 
+struct drm_encoder *omap_crtc_attached_encoder(struct drm_crtc *crtc)
+{
+	struct omap_drm_private *priv = crtc->dev->dev_private;
+	int i;
+
+	for (i = 0; i < priv->num_encoders; i++)
+		if (priv->encoders[i]->crtc == crtc)
+			return priv->encoders[i];
+
+	return NULL;
+}
+
 /* initialize crtc */
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct omap_overlay *ovl, int id)
diff --git a/drivers/staging/omapdrm/omap_drv.c b/drivers/staging/omapdrm/omap_drv.c
index df31c89..2a67e4e 100644
--- a/drivers/staging/omapdrm/omap_drv.c
+++ b/drivers/staging/omapdrm/omap_drv.c
@@ -419,7 +419,7 @@ static int omap_modeset_init(struct drm_device *dev)
 
 	dev->mode_config.funcs = &omap_mode_config_funcs;
 
-	return 0;
+	return drm_irq_install(dev);
 }
 
 static void omap_modeset_free(struct drm_device *dev)
@@ -756,7 +756,9 @@ static void dev_lastclose(struct drm_device *dev)
 				priv->rotation_prop, 0);
 	}
 
+	mutex_lock(&dev->mode_config.mutex);
 	ret = drm_fb_helper_restore_fbdev_mode(priv->fbdev);
+	mutex_unlock(&dev->mode_config.mutex);
 	if (ret)
 		DBG("failed to restore crtc mode");
 }
@@ -780,60 +782,6 @@ static void dev_postclose(struct drm_device *dev, struct drm_file *file)
 	DBG("postclose: dev=%p, file=%p", dev, file);
 }
 
-/**
- * enable_vblank - enable vblank interrupt events
- * @dev: DRM device
- * @crtc: which irq to enable
- *
- * Enable vblank interrupts for @crtc.  If the device doesn't have
- * a hardware vblank counter, this routine should be a no-op, since
- * interrupts will have to stay on to keep the count accurate.
- *
- * RETURNS
- * Zero on success, appropriate errno if the given @crtc's vblank
- * interrupt cannot be enabled.
- */
-static int dev_enable_vblank(struct drm_device *dev, int crtc)
-{
-	DBG("enable_vblank: dev=%p, crtc=%d", dev, crtc);
-	return 0;
-}
-
-/**
- * disable_vblank - disable vblank interrupt events
- * @dev: DRM device
- * @crtc: which irq to enable
- *
- * Disable vblank interrupts for @crtc.  If the device doesn't have
- * a hardware vblank counter, this routine should be a no-op, since
- * interrupts will have to stay on to keep the count accurate.
- */
-static void dev_disable_vblank(struct drm_device *dev, int crtc)
-{
-	DBG("disable_vblank: dev=%p, crtc=%d", dev, crtc);
-}
-
-static irqreturn_t dev_irq_handler(DRM_IRQ_ARGS)
-{
-	return IRQ_HANDLED;
-}
-
-static void dev_irq_preinstall(struct drm_device *dev)
-{
-	DBG("irq_preinstall: dev=%p", dev);
-}
-
-static int dev_irq_postinstall(struct drm_device *dev)
-{
-	DBG("irq_postinstall: dev=%p", dev);
-	return 0;
-}
-
-static void dev_irq_uninstall(struct drm_device *dev)
-{
-	DBG("irq_uninstall: dev=%p", dev);
-}
-
 static const struct vm_operations_struct omap_gem_vm_ops = {
 	.fault = omap_gem_fault,
 	.open = omap_gem_vm_open,
@@ -863,12 +811,12 @@ static struct drm_driver omap_drm_driver = {
 		.preclose = dev_preclose,
 		.postclose = dev_postclose,
 		.get_vblank_counter = drm_vblank_count,
-		.enable_vblank = dev_enable_vblank,
-		.disable_vblank = dev_disable_vblank,
-		.irq_preinstall = dev_irq_preinstall,
-		.irq_postinstall = dev_irq_postinstall,
-		.irq_uninstall = dev_irq_uninstall,
-		.irq_handler = dev_irq_handler,
+		.enable_vblank = omap_irq_enable_vblank,
+		.disable_vblank = omap_irq_disable_vblank,
+		.irq_preinstall = omap_irq_preinstall,
+		.irq_postinstall = omap_irq_postinstall,
+		.irq_uninstall = omap_irq_uninstall,
+		.irq_handler = omap_irq_handler,
 		.reclaim_buffers = drm_core_reclaim_buffers,
 #ifdef CONFIG_DEBUG_FS
 		.debugfs_init = omap_debugfs_init,
diff --git a/drivers/staging/omapdrm/omap_drv.h b/drivers/staging/omapdrm/omap_drv.h
index 799dd46..5cd3b1e 100644
--- a/drivers/staging/omapdrm/omap_drv.h
+++ b/drivers/staging/omapdrm/omap_drv.h
@@ -28,6 +28,31 @@
 #include <plat/drm.h>
 #include "omap_drm.h"
 
+/* APIs we need from dispc.. TODO omapdss should export these */
+void dispc_clear_irqs(u32 mask);
+u32 dispc_read_irqs(void);
+void dispc_set_irqs(u32 mask);
+u32 dispc_error_irqs(void);
+int dispc_runtime_get(void);
+int dispc_runtime_put(void);
+
+void dispc_mgr_enable(enum omap_channel channel, bool enable);
+void dispc_mgr_setup(enum omap_channel channel,
+		struct omap_overlay_manager_info *info);
+void dispc_mgr_set_timings(enum omap_channel channel,
+		struct omap_video_timings *timings);
+void dispc_mgr_go(enum omap_channel channel);
+bool dispc_mgr_go_busy(enum omap_channel channel);
+u32 dispc_mgr_get_vsync_irq(enum omap_channel channel);
+
+int dispc_ovl_enable(enum omap_plane plane, bool enable);
+void dispc_ovl_set_channel_out(enum omap_plane plane,
+		enum omap_channel channel);
+int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
+		bool ilace, bool replication,
+		const struct omap_video_timings *mgr_timings);
+
+
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 #define VERB(fmt, ...) if (0) DRM_DEBUG(fmt, ##__VA_ARGS__) /* verbose debug */
 
@@ -81,6 +106,21 @@ struct omap_drm_window {
 	uint32_t src_w, src_h;
 };
 
+/* Once GO bit is set, we can't make further updates to shadowed registers
+ * until the GO bit is cleared.  So various parts in the kms code that need
+ * to update shadowed registers queue up a pair of callbacks, pre_apply
+ * which is called before setting GO bit, and post_apply that is called
+ * after GO bit is cleared.  The encoder manages the queuing, and everyone
+ * else goes thru omap_encoder_apply() using these callbacks so that the
+ * code which has to deal w/ GO bit state is centralized.
+ */
+struct omap_drm_apply {
+	struct list_head pending_node, queued_node;
+	bool queued;
+	void (*pre_apply)(struct omap_drm_apply *apply);
+	void (*post_apply)(struct omap_drm_apply *apply);
+};
+
 #ifdef CONFIG_DEBUG_FS
 int omap_debugfs_init(struct drm_minor *minor);
 void omap_debugfs_cleanup(struct drm_minor *minor);
@@ -89,9 +129,19 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m);
 void omap_gem_describe_objects(struct list_head *list, struct seq_file *m);
 #endif
 
+int omap_irq_enable_vblank(struct drm_device *dev, int crtc);
+void omap_irq_disable_vblank(struct drm_device *dev, int crtc);
+irqreturn_t omap_irq_handler(DRM_IRQ_ARGS);
+void omap_irq_preinstall(struct drm_device *dev);
+int omap_irq_postinstall(struct drm_device *dev);
+void omap_irq_uninstall(struct drm_device *dev);
+void omap_irq_enable(uint32_t mask);
+void omap_irq_disable(uint32_t mask);
+
 struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev);
 void omap_fbdev_free(struct drm_device *dev);
 
+struct drm_encoder *omap_crtc_attached_encoder(struct drm_crtc *crtc);
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct omap_overlay *ovl, int id);
 
@@ -104,8 +154,7 @@ int omap_plane_mode_set(struct drm_plane *plane,
 		int crtc_x, int crtc_y,
 		unsigned int crtc_w, unsigned int crtc_h,
 		uint32_t src_x, uint32_t src_y,
-		uint32_t src_w, uint32_t src_h);
-void omap_plane_on_endwin(struct drm_plane *plane,
+		uint32_t src_w, uint32_t src_h,
 		void (*fxn)(void *), void *arg);
 void omap_plane_install_properties(struct drm_plane *plane,
 		struct drm_mode_object *obj);
@@ -116,6 +165,12 @@ struct drm_encoder *omap_encoder_init(struct drm_device *dev,
 		struct omap_overlay_manager *mgr);
 struct omap_overlay_manager *omap_encoder_get_manager(
 		struct drm_encoder *encoder);
+const struct omap_video_timings *omap_encoder_timings(
+		struct drm_encoder *encoder);
+enum omap_channel omap_encoder_channel(struct drm_encoder *encoder);
+void omap_encoder_vblank(struct drm_encoder *encoder, uint32_t irqstatus);
+int omap_encoder_apply(struct drm_encoder *encoder,
+		struct omap_drm_apply *apply);
 struct drm_encoder *omap_connector_attached_encoder(
 		struct drm_connector *connector);
 enum drm_connector_status omap_connector_detect(
@@ -123,8 +178,9 @@ enum drm_connector_status omap_connector_detect(
 
 struct drm_connector *omap_connector_init(struct drm_device *dev,
 		int connector_type, struct omap_dss_device *dssdev);
-void omap_connector_mode_set(struct drm_connector *connector,
-		struct drm_display_mode *mode);
+int omap_connector_mode_set(struct drm_connector *connector,
+		struct drm_display_mode *mode,
+		struct omap_video_timings *timings);
 void omap_connector_flush(struct drm_connector *connector,
 		int x, int y, int w, int h);
 
diff --git a/drivers/staging/omapdrm/omap_encoder.c b/drivers/staging/omapdrm/omap_encoder.c
index 06c52cb..d56d14d 100644
--- a/drivers/staging/omapdrm/omap_encoder.c
+++ b/drivers/staging/omapdrm/omap_encoder.c
@@ -22,6 +22,9 @@
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
 
+#include <linux/list.h>
+
+
 /*
  * encoder funcs
  */
@@ -30,13 +33,32 @@
 
 struct omap_encoder {
 	struct drm_encoder base;
-	struct omap_overlay_manager *mgr;
+	struct omap_overlay_manager *mgr; // TODO remove
+
+	const char *name;
+	enum omap_channel channel;
+	struct omap_overlay_manager_info info;
+
+	struct omap_video_timings timings;
+	bool enabled, timings_valid;
+	uint32_t irqmask;
+
+	struct omap_drm_apply apply;
+
+	/* list of in-progress apply's: */
+	struct list_head pending_applies;
+
+	/* list of queued apply's: */
+	struct list_head queued_applies;
+
+	/* for handling queued and in-progress applies: */
+	struct work_struct work;
 };
 
 static void omap_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
-	DBG("%s", omap_encoder->mgr->name);
+	DBG("%s", omap_encoder->name);
 	drm_encoder_cleanup(encoder);
 	kfree(omap_encoder);
 }
@@ -44,7 +66,14 @@ static void omap_encoder_destroy(struct drm_encoder *encoder)
 static void omap_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
-	DBG("%s: %d", omap_encoder->mgr->name, mode);
+	bool enabled = (mode == DRM_MODE_DPMS_ON);
+
+	DBG("%s: %d", omap_encoder->name, mode);
+
+	if (enabled != omap_encoder->enabled) {
+		omap_encoder->enabled = enabled;
+		omap_encoder_apply(encoder, &omap_encoder->apply);
+	}
 }
 
 static bool omap_encoder_mode_fixup(struct drm_encoder *encoder,
@@ -52,7 +81,7 @@ static bool omap_encoder_mode_fixup(struct drm_encoder *encoder,
 				  struct drm_display_mode *adjusted_mode)
 {
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
-	DBG("%s", omap_encoder->mgr->name);
+	DBG("%s", omap_encoder->name);
 	return true;
 }
 
@@ -67,13 +96,14 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder,
 
 	mode = adjusted_mode;
 
-	DBG("%s: set mode: %dx%d", omap_encoder->mgr->name,
+	DBG("%s: set mode: %dx%d", omap_encoder->name,
 			mode->hdisplay, mode->vdisplay);
 
 	for (i = 0; i < priv->num_connectors; i++) {
 		struct drm_connector *connector = priv->connectors[i];
 		if (connector->encoder == encoder) {
-			omap_connector_mode_set(connector, mode);
+			omap_encoder->timings_valid = omap_connector_mode_set(
+					connector, mode, &omap_encoder->timings) == 0;
 		}
 	}
 }
@@ -83,7 +113,7 @@ static void omap_encoder_prepare(struct drm_encoder *encoder)
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
 	struct drm_encoder_helper_funcs *encoder_funcs =
 				encoder->helper_private;
-	DBG("%s", omap_encoder->mgr->name);
+	DBG("%s", omap_encoder->name);
 	encoder_funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
 }
 
@@ -92,8 +122,7 @@ static void omap_encoder_commit(struct drm_encoder *encoder)
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
 	struct drm_encoder_helper_funcs *encoder_funcs =
 				encoder->helper_private;
-	DBG("%s", omap_encoder->mgr->name);
-	omap_encoder->mgr->apply(omap_encoder->mgr);
+	DBG("%s", omap_encoder->name);
 	encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
 }
 
@@ -109,6 +138,32 @@ static const struct drm_encoder_helper_funcs omap_encoder_helper_funcs = {
 	.commit = omap_encoder_commit,
 };
 
+static void omap_encoder_pre_apply(struct omap_drm_apply *apply)
+{
+	struct omap_encoder *omap_encoder =
+			container_of(apply, struct omap_encoder, apply);
+
+	DBG("%s: enabled=%d, timings_valid=%d", omap_encoder->name,
+			omap_encoder->enabled,
+			omap_encoder->timings_valid);
+
+	if (!omap_encoder->enabled || !omap_encoder->timings_valid) {
+		dispc_mgr_enable(omap_encoder->channel, false);
+		return;
+	}
+
+	dispc_mgr_setup(omap_encoder->channel, &omap_encoder->info);
+	dispc_mgr_set_timings(omap_encoder->channel,
+			&omap_encoder->timings);
+	dispc_mgr_enable(omap_encoder->channel, true);
+}
+
+static void omap_encoder_post_apply(struct omap_drm_apply *apply)
+{
+	/* nothing needed for post-apply */
+}
+
+// TODO remove
 struct omap_overlay_manager *omap_encoder_get_manager(
 		struct drm_encoder *encoder)
 {
@@ -116,14 +171,113 @@ struct omap_overlay_manager *omap_encoder_get_manager(
 	return omap_encoder->mgr;
 }
 
+const struct omap_video_timings *omap_encoder_timings(
+		struct drm_encoder *encoder)
+{
+	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+	return &omap_encoder->timings;
+}
+
+enum omap_channel omap_encoder_channel(struct drm_encoder *encoder)
+{
+	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+	return omap_encoder->channel;
+}
+
+void omap_encoder_vblank(struct drm_encoder *encoder, uint32_t irqstatus)
+{
+	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+
+	if (irqstatus & omap_encoder->irqmask) {
+		DBG("%s: it's for me!", omap_encoder->name);
+		if (!dispc_mgr_go_busy(omap_encoder->channel)) {
+			struct omap_drm_private *priv =
+					encoder->dev->dev_private;
+			DBG("%s: apply done", omap_encoder->name);
+			omap_irq_disable(omap_encoder->irqmask);
+			queue_work(priv->wq, &omap_encoder->work);
+		}
+	}
+}
+
+static void apply_worker(struct work_struct *work)
+{
+	struct omap_encoder *omap_encoder =
+			container_of(work, struct omap_encoder, work);
+	struct drm_encoder *encoder = &omap_encoder->base;
+	struct drm_device *dev = encoder->dev;
+	struct omap_drm_apply *apply, *n;
+	bool need_apply;
+
+	/*
+	 * Synchronize everything on mode_config.mutex, to keep
+	 * the callbacks and list modification all serialized
+	 * with respect to modesetting ioctls from userspace.
+	 */
+	mutex_lock(&dev->mode_config.mutex);
+	dispc_runtime_get();
+
+	/* finish up previous apply's: */
+	list_for_each_entry_safe(apply, n,
+			&omap_encoder->pending_applies, pending_node) {
+	}
+
+	need_apply = !list_empty(&omap_encoder->queued_applies);
+
+	/* then handle the next round of of queued apply's: */
+	list_for_each_entry_safe(apply, n,
+			&omap_encoder->queued_applies, queued_node) {
+		apply->pre_apply(apply);
+		list_del(&apply->queued_node);
+		apply->queued = false;
+		list_add_tail(&apply->pending_node,
+				&omap_encoder->pending_applies);
+	}
+
+	if (need_apply) {
+		DBG("%s: GO", omap_encoder->name);
+		omap_irq_enable(omap_encoder->irqmask);
+		dispc_mgr_go(omap_encoder->channel);
+	}
+	dispc_runtime_put();
+	mutex_unlock(&dev->mode_config.mutex);
+}
+
+int omap_encoder_apply(struct drm_encoder *encoder,
+		struct omap_drm_apply *apply)
+{
+	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+	struct drm_device *dev = encoder->dev;
+
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+
+	/* no need to queue it again if it is already queued: */
+	if (apply->queued)
+		return 0;
+
+	apply->queued = true;
+	list_add_tail(&apply->queued_node, &omap_encoder->queued_applies);
+
+	/*
+	 * If there are no currently pending updates, then go ahead and
+	 * kick the worker immediately, otherwise it will run again when
+	 * the current update finishes.
+	 */
+	if (list_empty(&omap_encoder->pending_applies)) {
+		struct omap_drm_private *priv = encoder->dev->dev_private;
+		queue_work(priv->wq, &omap_encoder->work);
+	}
+
+	return 0;
+}
+
 /* initialize encoder */
 struct drm_encoder *omap_encoder_init(struct drm_device *dev,
 		struct omap_overlay_manager *mgr)
 {
 	struct drm_encoder *encoder = NULL;
 	struct omap_encoder *omap_encoder;
-	struct omap_overlay_manager_info info;
-	int ret;
+	struct omap_overlay_manager_info *info;
 
 	DBG("%s", mgr->name);
 
@@ -133,32 +287,33 @@ struct drm_encoder *omap_encoder_init(struct drm_device *dev,
 		goto fail;
 	}
 
+	omap_encoder->name = mgr->name;
+	omap_encoder->channel = mgr->id;
+	omap_encoder->irqmask =
+			dispc_mgr_get_vsync_irq(mgr->id);
 	omap_encoder->mgr = mgr;
+
 	encoder = &omap_encoder->base;
 
+	INIT_WORK(&omap_encoder->work, apply_worker);
+	INIT_LIST_HEAD(&omap_encoder->pending_applies);
+	INIT_LIST_HEAD(&omap_encoder->queued_applies);
+
+	omap_encoder->apply.pre_apply  = omap_encoder_pre_apply;
+	omap_encoder->apply.post_apply = omap_encoder_post_apply;
+
 	drm_encoder_init(dev, encoder, &omap_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS);
 	drm_encoder_helper_add(encoder, &omap_encoder_helper_funcs);
 
-	mgr->get_manager_info(mgr, &info);
-
-	/* TODO: fix hard-coded setup.. */
-	info.default_color = 0x00000000;
-	info.trans_key = 0x00000000;
-	info.trans_key_type = OMAP_DSS_COLOR_KEY_GFX_DST;
-	info.trans_enabled = false;
-
-	ret = mgr->set_manager_info(mgr, &info);
-	if (ret) {
-		dev_err(dev->dev, "could not set manager info\n");
-		goto fail;
-	}
+	info = &omap_encoder->info;
+	mgr->get_manager_info(mgr, info);
 
-	ret = mgr->apply(mgr);
-	if (ret) {
-		dev_err(dev->dev, "could not apply\n");
-		goto fail;
-	}
+	/* TODO: fix hard-coded setup.. add properties! */
+	info->default_color = 0x00000000;
+	info->trans_key = 0x00000000;
+	info->trans_key_type = OMAP_DSS_COLOR_KEY_GFX_DST;
+	info->trans_enabled = false;
 
 	return encoder;
 
diff --git a/drivers/staging/omapdrm/omap_irq.c b/drivers/staging/omapdrm/omap_irq.c
new file mode 100644
index 0000000..28450ff
--- /dev/null
+++ b/drivers/staging/omapdrm/omap_irq.c
@@ -0,0 +1,133 @@
+/*
+ * drivers/staging/omapdrm/omap_irq.c
+ *
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Rob Clark <rob.clark@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "omap_drv.h"
+
+/* TODO move to priv.. */
+static u32 error_mask;
+static u32 current_mask;
+
+/**
+ * enable_vblank - enable vblank interrupt events
+ * @dev: DRM device
+ * @crtc: which irq to enable
+ *
+ * Enable vblank interrupts for @crtc.  If the device doesn't have
+ * a hardware vblank counter, this routine should be a no-op, since
+ * interrupts will have to stay on to keep the count accurate.
+ *
+ * RETURNS
+ * Zero on success, appropriate errno if the given @crtc's vblank
+ * interrupt cannot be enabled.
+ */
+int omap_irq_enable_vblank(struct drm_device *dev, int crtc)
+{
+	DBG("dev=%p, crtc=%d", dev, crtc);
+	return 0;
+}
+
+/**
+ * disable_vblank - disable vblank interrupt events
+ * @dev: DRM device
+ * @crtc: which irq to enable
+ *
+ * Disable vblank interrupts for @crtc.  If the device doesn't have
+ * a hardware vblank counter, this routine should be a no-op, since
+ * interrupts will have to stay on to keep the count accurate.
+ */
+void omap_irq_disable_vblank(struct drm_device *dev, int crtc)
+{
+	DBG("dev=%p, crtc=%d", dev, crtc);
+}
+
+/* call w/ pm runtime held */
+void omap_irq_enable(uint32_t mask)
+{
+	current_mask |= mask;
+	dispc_set_irqs(current_mask);
+}
+
+/* call w/ pm runtime held */
+void omap_irq_disable(uint32_t mask)
+{
+	current_mask &= ~mask;
+	dispc_set_irqs(current_mask);
+}
+
+irqreturn_t omap_irq_handler(DRM_IRQ_ARGS)
+{
+	struct drm_device *dev = (struct drm_device *) arg;
+	struct omap_drm_private *priv = dev->dev_private;
+	u32 irqstatus;
+
+	irqstatus = dispc_read_irqs();
+	dispc_clear_irqs(irqstatus);
+	dispc_read_irqs();            /* flush posted write */
+
+	if (irqstatus & error_mask) {
+		DBG("errors: %08x", irqstatus & error_mask);
+		/* TODO */
+	}
+
+	DBG("irqs: %08x", irqstatus & ~error_mask);
+
+#define VBLANK (DISPC_IRQ_VSYNC|DISPC_IRQ_VSYNC2|DISPC_IRQ_EVSYNC_ODD|DISPC_IRQ_EVSYNC_EVEN)
+	if (irqstatus & VBLANK) {
+		unsigned int i;
+		for (i = 0; i < priv->num_encoders; i++)
+			omap_encoder_vblank(priv->encoders[i], irqstatus);
+
+		/* TODO we could probably dispatch to CRTC's and handle
+		 * page-flip events w/out the callback between omap_plane
+		 * and omap_crtc.. that would be a bit cleaner.
+		 */
+	}
+
+	return IRQ_HANDLED;
+}
+
+void omap_irq_preinstall(struct drm_device *dev)
+{
+	DBG("dev=%p", dev);
+
+	dispc_runtime_get();
+	error_mask = dispc_error_irqs();
+
+	/* for now ignore DISPC_IRQ_SYNC_LOST_DIGIT.. really I think
+	 * we just need to ignore it while enabling tv-out
+	 */
+	error_mask &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
+
+	dispc_clear_irqs(0xffffffff);
+	dispc_runtime_put();
+}
+
+int omap_irq_postinstall(struct drm_device *dev)
+{
+	DBG("dev=%p", dev);
+	dispc_runtime_get();
+	omap_irq_enable(error_mask);
+	dispc_runtime_put();
+	return 0;
+}
+
+void omap_irq_uninstall(struct drm_device *dev)
+{
+	DBG("dev=%p", dev);
+}
diff --git a/drivers/staging/omapdrm/omap_plane.c b/drivers/staging/omapdrm/omap_plane.c
index 6931d06..a8d0df0 100644
--- a/drivers/staging/omapdrm/omap_plane.c
+++ b/drivers/staging/omapdrm/omap_plane.c
@@ -41,12 +41,14 @@ struct callback {
 
 struct omap_plane {
 	struct drm_plane base;
-	struct omap_overlay *ovl;
+	int plane;  /* TODO rename omap_plane -> omap_plane_id in omapdss so I can use the enum */
+	const char *name;
 	struct omap_overlay_info info;
+	struct omap_drm_apply apply;
 
 	/* position/orientation of scanout within the fb: */
 	struct omap_drm_window win;
-
+	bool enabled;
 
 	/* last fb that we pinned: */
 	struct drm_framebuffer *pinned_fb;
@@ -54,189 +56,13 @@ struct omap_plane {
 	uint32_t nformats;
 	uint32_t formats[32];
 
-	/* for synchronizing access to unpins fifo */
-	struct mutex unpin_mutex;
-
-	/* set of bo's pending unpin until next END_WIN irq */
+	/* set of bo's pending unpin until next post_apply() */
 	DECLARE_KFIFO_PTR(unpin_fifo, struct drm_gem_object *);
-	int num_unpins, pending_num_unpins;
-
-	/* for deferred unpin when we need to wait for scanout complete irq */
-	struct work_struct work;
-
-	/* callback on next endwin irq */
-	struct callback endwin;
-};
 
-/* map from ovl->id to the irq we are interested in for scanout-done */
-static const uint32_t id2irq[] = {
-		[OMAP_DSS_GFX]    = DISPC_IRQ_GFX_END_WIN,
-		[OMAP_DSS_VIDEO1] = DISPC_IRQ_VID1_END_WIN,
-		[OMAP_DSS_VIDEO2] = DISPC_IRQ_VID2_END_WIN,
-		[OMAP_DSS_VIDEO3] = DISPC_IRQ_VID3_END_WIN,
+	// XXX maybe get rid of this and handle vblank in crtc too?
+	struct callback apply_done_cb;
 };
 
-static void dispc_isr(void *arg, uint32_t mask)
-{
-	struct drm_plane *plane = arg;
-	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_drm_private *priv = plane->dev->dev_private;
-
-	omap_dispc_unregister_isr(dispc_isr, plane,
-			id2irq[omap_plane->ovl->id]);
-
-	queue_work(priv->wq, &omap_plane->work);
-}
-
-static void unpin_worker(struct work_struct *work)
-{
-	struct omap_plane *omap_plane =
-			container_of(work, struct omap_plane, work);
-	struct callback endwin;
-
-	mutex_lock(&omap_plane->unpin_mutex);
-	DBG("unpinning %d of %d", omap_plane->num_unpins,
-			omap_plane->num_unpins + omap_plane->pending_num_unpins);
-	while (omap_plane->num_unpins > 0) {
-		struct drm_gem_object *bo = NULL;
-		int ret = kfifo_get(&omap_plane->unpin_fifo, &bo);
-		WARN_ON(!ret);
-		omap_gem_put_paddr(bo);
-		drm_gem_object_unreference_unlocked(bo);
-		omap_plane->num_unpins--;
-	}
-	endwin = omap_plane->endwin;
-	omap_plane->endwin.fxn = NULL;
-	mutex_unlock(&omap_plane->unpin_mutex);
-
-	if (endwin.fxn)
-		endwin.fxn(endwin.arg);
-}
-
-static void install_irq(struct drm_plane *plane)
-{
-	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_overlay *ovl = omap_plane->ovl;
-	int ret;
-
-	ret = omap_dispc_register_isr(dispc_isr, plane, id2irq[ovl->id]);
-
-	/*
-	 * omapdss has upper limit on # of registered irq handlers,
-	 * which we shouldn't hit.. but if we do the limit should
-	 * be raised or bad things happen:
-	 */
-	WARN_ON(ret == -EBUSY);
-}
-
-/* push changes down to dss2 */
-static int commit(struct drm_plane *plane)
-{
-	struct drm_device *dev = plane->dev;
-	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_overlay *ovl = omap_plane->ovl;
-	struct omap_overlay_info *info = &omap_plane->info;
-	int ret;
-
-	DBG("%s", ovl->name);
-	DBG("%dx%d -> %dx%d (%d)", info->width, info->height, info->out_width,
-			info->out_height, info->screen_width);
-	DBG("%d,%d %08x %08x", info->pos_x, info->pos_y,
-			info->paddr, info->p_uv_addr);
-
-	/* NOTE: do we want to do this at all here, or just wait
-	 * for dpms(ON) since other CRTC's may not have their mode
-	 * set yet, so fb dimensions may still change..
-	 */
-	ret = ovl->set_overlay_info(ovl, info);
-	if (ret) {
-		dev_err(dev->dev, "could not set overlay info\n");
-		return ret;
-	}
-
-	mutex_lock(&omap_plane->unpin_mutex);
-	omap_plane->num_unpins += omap_plane->pending_num_unpins;
-	omap_plane->pending_num_unpins = 0;
-	mutex_unlock(&omap_plane->unpin_mutex);
-
-	/* our encoder doesn't necessarily get a commit() after this, in
-	 * particular in the dpms() and mode_set_base() cases, so force the
-	 * manager to update:
-	 *
-	 * could this be in the encoder somehow?
-	 */
-	if (ovl->manager) {
-		ret = ovl->manager->apply(ovl->manager);
-		if (ret) {
-			dev_err(dev->dev, "could not apply settings\n");
-			return ret;
-		}
-
-		/*
-		 * NOTE: really this should be atomic w/ mgr->apply() but
-		 * omapdss does not expose such an API
-		 */
-		if (omap_plane->num_unpins > 0)
-			install_irq(plane);
-
-	} else {
-		struct omap_drm_private *priv = dev->dev_private;
-		queue_work(priv->wq, &omap_plane->work);
-	}
-
-
-	if (ovl->is_enabled(ovl)) {
-		omap_framebuffer_flush(plane->fb, info->pos_x, info->pos_y,
-				info->out_width, info->out_height);
-	}
-
-	return 0;
-}
-
-/* when CRTC that we are attached to has potentially changed, this checks
- * if we are attached to proper manager, and if necessary updates.
- */
-static void update_manager(struct drm_plane *plane)
-{
-	struct omap_drm_private *priv = plane->dev->dev_private;
-	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_overlay *ovl = omap_plane->ovl;
-	struct omap_overlay_manager *mgr = NULL;
-	int i;
-
-	if (plane->crtc) {
-		for (i = 0; i < priv->num_encoders; i++) {
-			struct drm_encoder *encoder = priv->encoders[i];
-			if (encoder->crtc == plane->crtc) {
-				mgr = omap_encoder_get_manager(encoder);
-				break;
-			}
-		}
-	}
-
-	if (ovl->manager != mgr) {
-		bool enabled = ovl->is_enabled(ovl);
-
-		/* don't switch things around with enabled overlays: */
-		if (enabled)
-			omap_plane_dpms(plane, DRM_MODE_DPMS_OFF);
-
-		if (ovl->manager) {
-			DBG("disconnecting %s from %s", ovl->name,
-					ovl->manager->name);
-			ovl->unset_manager(ovl);
-		}
-
-		if (mgr) {
-			DBG("connecting %s to %s", ovl->name, mgr->name);
-			ovl->set_manager(ovl, mgr);
-		}
-
-		if (enabled && mgr)
-			omap_plane_dpms(plane, DRM_MODE_DPMS_ON);
-	}
-}
-
 static void unpin(void *arg, struct drm_gem_object *bo)
 {
 	struct drm_plane *plane = arg;
@@ -244,7 +70,6 @@ static void unpin(void *arg, struct drm_gem_object *bo)
 
 	if (kfifo_put(&omap_plane->unpin_fifo,
 			(const struct drm_gem_object **)&bo)) {
-		omap_plane->pending_num_unpins++;
 		/* also hold a ref so it isn't free'd while pinned */
 		drm_gem_object_reference(bo);
 	} else {
@@ -264,9 +89,7 @@ static int update_pin(struct drm_plane *plane, struct drm_framebuffer *fb)
 
 		DBG("%p -> %p", pinned_fb, fb);
 
-		mutex_lock(&omap_plane->unpin_mutex);
 		ret = omap_framebuffer_replace(pinned_fb, fb, plane, unpin);
-		mutex_unlock(&omap_plane->unpin_mutex);
 
 		if (ret) {
 			dev_err(plane->dev->dev, "could not swap %p -> %p\n",
@@ -281,31 +104,92 @@ static int update_pin(struct drm_plane *plane, struct drm_framebuffer *fb)
 	return 0;
 }
 
-/* update parameters that are dependent on the framebuffer dimensions and
- * position within the fb that this plane scans out from. This is called
- * when framebuffer or x,y base may have changed.
- */
-static void update_scanout(struct drm_plane *plane)
+static void omap_plane_pre_apply(struct omap_drm_apply *apply)
 {
-	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_overlay_info *info = &omap_plane->info;
+	struct omap_plane *omap_plane =
+			container_of(apply, struct omap_plane, apply);
 	struct omap_drm_window *win = &omap_plane->win;
+	struct drm_plane *plane = &omap_plane->base;
+	struct drm_device *dev = plane->dev;
+	struct drm_encoder *encoder = plane->crtc ?
+			omap_crtc_attached_encoder(plane->crtc) : NULL;
+	struct omap_overlay_info *info = &omap_plane->info;
+	bool enabled = omap_plane->enabled && encoder;
+	bool ilace, replication;
 	int ret;
 
-	ret = update_pin(plane, plane->fb);
-	if (ret) {
-		dev_err(plane->dev->dev,
-			"could not pin fb: %d\n", ret);
-		omap_plane_dpms(plane, DRM_MODE_DPMS_OFF);
+	DBG("%s", omap_plane->name);
+
+	/* if fb has changed, pin new fb: */
+	update_pin(plane, enabled ? plane->fb : NULL);
+
+	if (!enabled) {
+		dispc_ovl_enable(omap_plane->plane, false);
 		return;
 	}
 
+	/* update scanout: */
 	omap_framebuffer_update_scanout(plane->fb, win, info);
 
-	DBG("%s: %d,%d: %08x %08x (%d)", omap_plane->ovl->name,
-			win->src_x, win->src_y,
-			(u32)info->paddr, (u32)info->p_uv_addr,
+	DBG("%dx%d -> %dx%d (%d)", info->width, info->height,
+			info->out_width, info->out_height,
 			info->screen_width);
+	DBG("%d,%d %08x %08x", info->pos_x, info->pos_y,
+			info->paddr, info->p_uv_addr);
+
+	/* TODO: */
+	ilace = false;
+	replication = false;
+
+	/* and finally, update omapdss: */
+	ret = dispc_ovl_setup(omap_plane->plane, info, ilace,
+			replication, omap_encoder_timings(encoder));
+	if (ret) {
+		dev_err(dev->dev, "dispc_ovl_setup failed: %d\n", ret);
+		return;
+	}
+
+	dispc_ovl_enable(omap_plane->plane, true);
+	dispc_ovl_set_channel_out(omap_plane->plane,
+			omap_encoder_channel(encoder));
+}
+
+static void omap_plane_post_apply(struct omap_drm_apply *apply)
+{
+	struct omap_plane *omap_plane =
+			container_of(apply, struct omap_plane, apply);
+	struct drm_plane *plane = &omap_plane->base;
+	struct omap_overlay_info *info = &omap_plane->info;
+	struct drm_gem_object *bo = NULL;
+	struct callback cb;
+
+	cb = omap_plane->apply_done_cb;
+	omap_plane->apply_done_cb.fxn = NULL;
+
+	while (kfifo_get(&omap_plane->unpin_fifo, &bo)) {
+		omap_gem_put_paddr(bo);
+		drm_gem_object_unreference_unlocked(bo);
+	}
+
+	if (cb.fxn)
+		cb.fxn(cb.arg);
+
+	if (omap_plane->enabled) {
+		omap_framebuffer_flush(plane->fb, info->pos_x, info->pos_y,
+				info->out_width, info->out_height);
+	}
+}
+
+static int apply(struct drm_plane *plane)
+{
+	if (plane->crtc) {
+		struct omap_plane *omap_plane = to_omap_plane(plane);
+		struct drm_encoder *encoder =
+				omap_crtc_attached_encoder(plane->crtc);
+		if (encoder)
+			return omap_encoder_apply(encoder, &omap_plane->apply);
+	}
+	return 0;
 }
 
 int omap_plane_mode_set(struct drm_plane *plane,
@@ -313,7 +197,8 @@ int omap_plane_mode_set(struct drm_plane *plane,
 		int crtc_x, int crtc_y,
 		unsigned int crtc_w, unsigned int crtc_h,
 		uint32_t src_x, uint32_t src_y,
-		uint32_t src_w, uint32_t src_h)
+		uint32_t src_w, uint32_t src_h,
+		void (*fxn)(void *), void *arg)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct omap_drm_window *win = &omap_plane->win;
@@ -329,17 +214,13 @@ int omap_plane_mode_set(struct drm_plane *plane,
 	win->src_w = src_w >> 16;
 	win->src_h = src_h >> 16;
 
-	/* note: this is done after this fxn returns.. but if we need
-	 * to do a commit/update_scanout, etc before this returns we
-	 * need the current value.
-	 */
+	omap_plane->apply_done_cb.fxn = fxn;
+	omap_plane->apply_done_cb.arg = arg;
+
 	plane->fb = fb;
 	plane->crtc = crtc;
 
-	update_scanout(plane);
-	update_manager(plane);
-
-	return 0;
+	return apply(plane);
 }
 
 static int omap_plane_update(struct drm_plane *plane,
@@ -349,9 +230,12 @@ static int omap_plane_update(struct drm_plane *plane,
 		uint32_t src_x, uint32_t src_y,
 		uint32_t src_w, uint32_t src_h)
 {
-	omap_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h,
-			src_x, src_y, src_w, src_h);
-	return omap_plane_dpms(plane, DRM_MODE_DPMS_ON);
+	struct omap_plane *omap_plane = to_omap_plane(plane);
+	omap_plane->enabled = true;
+	return omap_plane_mode_set(plane, crtc, fb,
+			crtc_x, crtc_y, crtc_w, crtc_h,
+			src_x, src_y, src_w, src_h,
+			NULL, NULL);
 }
 
 static int omap_plane_disable(struct drm_plane *plane)
@@ -364,10 +248,10 @@ static int omap_plane_disable(struct drm_plane *plane)
 static void omap_plane_destroy(struct drm_plane *plane)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
-	DBG("%s", omap_plane->ovl->name);
+	DBG("%s", omap_plane->name);
 	omap_plane_disable(plane);
 	drm_plane_cleanup(plane);
-	WARN_ON(omap_plane->pending_num_unpins + omap_plane->num_unpins > 0);
+	WARN_ON(!kfifo_is_empty(&omap_plane->unpin_fifo));
 	kfifo_free(&omap_plane->unpin_fifo);
 	kfree(omap_plane);
 }
@@ -375,37 +259,15 @@ static void omap_plane_destroy(struct drm_plane *plane)
 int omap_plane_dpms(struct drm_plane *plane, int mode)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_overlay *ovl = omap_plane->ovl;
-	int r;
-
-	DBG("%s: %d", omap_plane->ovl->name, mode);
+	bool enabled = (mode == DRM_MODE_DPMS_ON);
+	int ret = 0;
 
-	if (mode == DRM_MODE_DPMS_ON) {
-		update_scanout(plane);
-		r = commit(plane);
-		if (!r)
-			r = ovl->enable(ovl);
-	} else {
-		struct omap_drm_private *priv = plane->dev->dev_private;
-		r = ovl->disable(ovl);
-		update_pin(plane, NULL);
-		queue_work(priv->wq, &omap_plane->work);
+	if (enabled != omap_plane->enabled) {
+		omap_plane->enabled = enabled;
+		ret = apply(plane);
 	}
 
-	return r;
-}
-
-void omap_plane_on_endwin(struct drm_plane *plane,
-		void (*fxn)(void *), void *arg)
-{
-	struct omap_plane *omap_plane = to_omap_plane(plane);
-
-	mutex_lock(&omap_plane->unpin_mutex);
-	omap_plane->endwin.fxn = fxn;
-	omap_plane->endwin.arg = arg;
-	mutex_unlock(&omap_plane->unpin_mutex);
-
-	install_irq(plane);
+	return ret;
 }
 
 /* helper to install properties which are common to planes and crtcs */
@@ -443,15 +305,9 @@ int omap_plane_set_property(struct drm_plane *plane,
 	int ret = -EINVAL;
 
 	if (property == priv->rotation_prop) {
-		struct omap_overlay *ovl = omap_plane->ovl;
-
-		DBG("%s: rotation: %02x", ovl->name, (uint32_t)val);
+		DBG("%s: rotation: %02x", omap_plane->name, (uint32_t)val);
 		omap_plane->win.rotation = val;
-
-		if (ovl->is_enabled(ovl))
-			ret = omap_plane_dpms(plane, DRM_MODE_DPMS_ON);
-		else
-			ret = 0;
+		ret = apply(plane);
 	}
 
 	return ret;
@@ -471,36 +327,35 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 {
 	struct drm_plane *plane = NULL;
 	struct omap_plane *omap_plane;
+	struct omap_overlay_info *info;
 	int ret;
 
 	DBG("%s: possible_crtcs=%08x, priv=%d", ovl->name,
 			possible_crtcs, priv);
 
-	/* friendly reminder to update table for future hw: */
-	WARN_ON(ovl->id >= ARRAY_SIZE(id2irq));
-
 	omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL);
 	if (!omap_plane) {
 		dev_err(dev->dev, "could not allocate plane\n");
 		goto fail;
 	}
 
-	mutex_init(&omap_plane->unpin_mutex);
-
 	ret = kfifo_alloc(&omap_plane->unpin_fifo, 16, GFP_KERNEL);
 	if (ret) {
 		dev_err(dev->dev, "could not allocate unpin FIFO\n");
 		goto fail;
 	}
 
-	INIT_WORK(&omap_plane->work, unpin_worker);
-
 	omap_plane->nformats = omap_framebuffer_get_formats(
 			omap_plane->formats, ARRAY_SIZE(omap_plane->formats),
 			ovl->supported_modes);
-	omap_plane->ovl = ovl;
+	omap_plane->plane = ovl->id;
+	omap_plane->name = ovl->name;
+
 	plane = &omap_plane->base;
 
+	omap_plane->apply.pre_apply  = omap_plane_pre_apply;
+	omap_plane->apply.post_apply = omap_plane_post_apply;
+
 	drm_plane_init(dev, plane, possible_crtcs, &omap_plane_funcs,
 			omap_plane->formats, omap_plane->nformats, priv);
 
@@ -509,11 +364,12 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	/* get our starting configuration, set defaults for parameters
 	 * we don't currently use, etc:
 	 */
-	ovl->get_overlay_info(ovl, &omap_plane->info);
-	omap_plane->info.rotation_type = OMAP_DSS_ROT_DMA;
-	omap_plane->info.rotation = OMAP_DSS_ROT_0;
-	omap_plane->info.global_alpha = 0xff;
-	omap_plane->info.mirror = 0;
+	info = &omap_plane->info;
+	ovl->get_overlay_info(ovl, info);
+	info->rotation_type = OMAP_DSS_ROT_DMA;
+	info->rotation = OMAP_DSS_ROT_0;
+	info->global_alpha = 0xff;
+	info->mirror = 0;
 
 	/* Set defaults depending on whether we are a CRTC or overlay
 	 * layer.
@@ -525,8 +381,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	else
 		omap_plane->info.zorder = ovl->id;
 
-	update_manager(plane);
-
 	return plane;
 
 fail:
-- 
1.7.9.5

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Tomi Valkeinen]

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

Hi,

On Fri, 2012-07-27 at 20:07 -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> I've been working for the better part of the week on solving some of
> the omapdss vs kms mismatches, which is one of the bigger remaining
> issues in the TODO before moving omapdrm out of staging.
> 
> The biggest place that this shows up is in GO bit handling.  Basically,
> some of the scanout related registers in DSS hw block are only shadow
> registers, and if the GO bit is set during the vblank the hw copies
> into the actual registers (not accessible to CPU) and clears the GO
> bit.  When the GO bit is set, there is no safe way to update these
> registers without undefined results.  So omapdss tries to be friendly
> and abstract this, by buffering up the updated state, and applying it

It's not really about being friendly. Omapdss tries to do as little as
possible, while still supporting all its HW features. Shadow registers
are a bit tricky creating this mess.

> on the next vblank once the GO bit is cleared.  But this causes all
> sorts of mayhem at the omapdrm layer, which would like to unpin the
> previous scanout buffer(s) on the next vblank (or endwin) irq.  Due
> to the buffering in omapdss, we have no way to know on a vblank if we
> have switched to the scanout buffer or not.  Basically it works ok as
> long as userspace is only ever updating on layer (either crtc or drm
> plane) at a time.  But throw together hw mouse cursor (drm plane)
> plus a window manager like compiz which does page flips, or wayland
> (weston drm compositor) with hw composition (drm plane), and things
> start to fail in a big way.
> 
> I've tried a few approaches to preserve the omapdss more or less as it
> is, by adding callbacks for when GO bit is cleared, etc.  But the
> sequencing of setting up connector/encoder/crtc is not really what
> omapdss expects, and would generally end up confusing the "apply"
> layer in omapdss (it would end up not programming various registers
> because various dirty flags would get cleared, for example mgr updated
> before overlay connected, etc).

Can you give more info what the problem is? It shouldn't end up not
programming registers, except if there's a bug there.

Didn't the apply-id stuff I proposed some months ago have enough stuff
to make this work?

The thing about shadow registers is that we need to manage them in one
central place. And the same shadow registers are used for both the
composition stuff (overlays etc) and output stuff (video timings &
configuration). If omapdrm handles the composition shadow registers, it
also needs to handle all the other shadow registers.

> Finally, in frustration, this afternoon I hit upon an idea.  Why not
> just use the dispc code in omapdss, which is basically a stateless
> layer of helper functions, and bypass the stateful layer of omapdss.

If you do this, you'll need to implement all the stuff of the stateful
layer in omapdrm. You can't just call the dispc funcs and expect things
to work reliably. Things like enabling/disabling overlays with fifomerge
requires possibly multiple vsyncs. And output related shadow registers
may be changed separately from the composition side.

The apply.c is not there to make the life of the user of omapdss's easy,
it's there to make the DSS hardware work properly =).

> Since KMS helper functions already give us the correct sequence for
> setting up the hardware, this turned out to be rather easy.  And fit
> it quite nicely with my mechanism to queue up updates when the GO bit
> is not clear.  And, unlike my previous attempts, it actually worked..
> not only that, but it worked on the first boot!

Well, not having read the omapdrm code, I'm not in the best position to
comment, but I fear that while it seems to work, you have lots of corner
cases where you'll get glitches. We had a lot simpler configuration
model in the omapdss for long time, until the small corner cases started
to pile up and new features started to cause problems, and I wrote the
current apply mechanism.

> So I am pretty happy about how this is shaping up.  Not only is it
> simpler that my previous attepmts, and solves a few tricky buffer
> unpin related issues.  But it also makes it very easy to wire in the
> missing userspace vblank event handling without resorting to duct-
> tape.

Why is giving an event from omapdss at vsync "duct-tapy"?

> Obviously there is stuff still missing, and some hacks.  This is
> really just a proof of concept at this stage.  But I wanted to send an
> RFC so we could start discussing how to move forward.  Ie. could we
> reasonably add support to build dispc as a library of stateless helper
> functions, sharing it and the panel drivers between omapdrm and the
> legacy omapdss based drivers.  Or is there no clean way to do that, in
> which case we should just copy the code we need into omapdrm, and
> leave the deprecated omapdss as it is for legacy drivers.

I agree that we need to get omapdrm work well, as it's (or will be) the
most important display framework. And if managing that requires us to
combine omapdss and omapdrm, I'm fine with it.

But, again, so far I don't understand why it's so difficult to have them
separate with omapdss giving notifications of important events, and also
how combining the two drivers would fix any issues (though I agree it
could simplify some code somewhat).

So what I propose is first to look at the problems you have, so that I
understand what the problem is with omapdss-omapdrm interaction.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> On Fri, 2012-07-27 at 20:07 -0500, Rob Clark wrote:
>> From: Rob Clark <rob@ti.com>
>>
>> I've been working for the better part of the week on solving some of
>> the omapdss vs kms mismatches, which is one of the bigger remaining
>> issues in the TODO before moving omapdrm out of staging.
>>
>> The biggest place that this shows up is in GO bit handling.  Basically,
>> some of the scanout related registers in DSS hw block are only shadow
>> registers, and if the GO bit is set during the vblank the hw copies
>> into the actual registers (not accessible to CPU) and clears the GO
>> bit.  When the GO bit is set, there is no safe way to update these
>> registers without undefined results.  So omapdss tries to be friendly
>> and abstract this, by buffering up the updated state, and applying it
>
> It's not really about being friendly. Omapdss tries to do as little as
> possible, while still supporting all its HW features. Shadow registers
> are a bit tricky creating this mess.

What I mean by 'friendly' is it tries to abstract this for simple
users, like an fbdev driver.  But this really quickly breaks down w/ a
more sophisticated user.  Which is why I've been more in favor of
making omapdss less of a layer.  The idea of using it as some helper
functions which handle a bit the variation of different generations of
hw while not abstracting the fundamental operating concepts of DSS IP
block (ie. GO bit stuff) seems perfect to me.  So dispc plus
dss_feature stuff seems like just what I'm looking for.

>> on the next vblank once the GO bit is cleared.  But this causes all
>> sorts of mayhem at the omapdrm layer, which would like to unpin the
>> previous scanout buffer(s) on the next vblank (or endwin) irq.  Due
>> to the buffering in omapdss, we have no way to know on a vblank if we
>> have switched to the scanout buffer or not.  Basically it works ok as
>> long as userspace is only ever updating on layer (either crtc or drm
>> plane) at a time.  But throw together hw mouse cursor (drm plane)
>> plus a window manager like compiz which does page flips, or wayland
>> (weston drm compositor) with hw composition (drm plane), and things
>> start to fail in a big way.
>>
>> I've tried a few approaches to preserve the omapdss more or less as it
>> is, by adding callbacks for when GO bit is cleared, etc.  But the
>> sequencing of setting up connector/encoder/crtc is not really what
>> omapdss expects, and would generally end up confusing the "apply"
>> layer in omapdss (it would end up not programming various registers
>> because various dirty flags would get cleared, for example mgr updated
>> before overlay connected, etc).
>
> Can you give more info what the problem is? It shouldn't end up not
> programming registers, except if there's a bug there.

Yeah, it is probably just a bug.. and a bug could be fixed.  But with
all that extra code it is certainly a lot harder to debug.  So 'could'
and 'should' are maybe two different things.

> Didn't the apply-id stuff I proposed some months ago have enough stuff
> to make this work?

I guess the approach I was trying was similar to that proposal.. it
probably could be made to work.  But I am really not a big fan of
unnecessary complexity.  And unnecessary layering adds complexity.
This is why in general most of the drm folks have preferred an
approach of helper fxns rather than layers.  And I tend to agree with
them.

> The thing about shadow registers is that we need to manage them in one
> central place. And the same shadow registers are used for both the
> composition stuff (overlays etc) and output stuff (video timings &
> configuration). If omapdrm handles the composition shadow registers, it
> also needs to handle all the other shadow registers.

Yup.. I'm handling it in a central place :-)

basically all the register programming is coming through the 'struct
omap_drm_apply' mechanism, so it is all aligned to vblank/framedone
and GO bit status.  So probably that isn't strictly needed, because
I'm treating all registers as shadow'd registers, but that seemed like
a clean approach.

>> Finally, in frustration, this afternoon I hit upon an idea.  Why not
>> just use the dispc code in omapdss, which is basically a stateless
>> layer of helper functions, and bypass the stateful layer of omapdss.
>
> If you do this, you'll need to implement all the stuff of the stateful
> layer in omapdrm. You can't just call the dispc funcs and expect things
> to work reliably. Things like enabling/disabling overlays with fifomerge
> requires possibly multiple vsyncs. And output related shadow registers
> may be changed separately from the composition side.

All the dispc fxn calls (except whatever is done directly from the
'omap_dss_device', which I haven't converted over yet) are done
synchronized to the ovl mgrs GO bit.  I haven't hooked up fifomerge
yet, although looking at the apply code in omapdss, that looks like it
should be pretty straightforward to hook into the same omap_drm_apply
mechanism.

> The apply.c is not there to make the life of the user of omapdss's easy,
> it's there to make the DSS hardware work properly =).
>
>> Since KMS helper functions already give us the correct sequence for
>> setting up the hardware, this turned out to be rather easy.  And fit
>> it quite nicely with my mechanism to queue up updates when the GO bit
>> is not clear.  And, unlike my previous attempts, it actually worked..
>> not only that, but it worked on the first boot!
>
> Well, not having read the omapdrm code, I'm not in the best position to
> comment, but I fear that while it seems to work, you have lots of corner
> cases where you'll get glitches. We had a lot simpler configuration
> model in the omapdss for long time, until the small corner cases started
> to pile up and new features started to cause problems, and I wrote the
> current apply mechanism.

I think you'd like drm/kms if you start to get into it.. and really it
is much better if folks like you who really know the hw well, and have
lot of experience with the corner cases are working at the drm/kms
layer and not staying beneath an omapdss arbitrary layer.  I have a
pretty good idea of what is needed from userspace and graphics
standpoint, but not your level of experience w/ DSS and related IP
blocks and all different sorts of crazy display panel technology.  So
I could really use your help at the drm/kms layer :-)

And btw, I think the current mapping of drm_encoder to mgr in omapdrm
is not correct.  I'm just in the process of shuffling things around.
I think drm/kms actually maps quite nicely to the underlying hardware
with the following arrangement:

 drm_plane -> ovl
 drm_crtc -> mgr
 drm_encoder -> DSI/DPI/HDMI/VENC encoder
 drm_connector -> pretty much what we call a panel driver today

>> So I am pretty happy about how this is shaping up.  Not only is it
>> simpler that my previous attepmts, and solves a few tricky buffer
>> unpin related issues.  But it also makes it very easy to wire in the
>> missing userspace vblank event handling without resorting to duct-
>> tape.
>
> Why is giving an event from omapdss at vsync "duct-tapy"?

well, at a minimum, it is duplicating at omapdss a lot of what drm
irq/vblank infrastructure already provides.

>> Obviously there is stuff still missing, and some hacks.  This is
>> really just a proof of concept at this stage.  But I wanted to send an
>> RFC so we could start discussing how to move forward.  Ie. could we
>> reasonably add support to build dispc as a library of stateless helper
>> functions, sharing it and the panel drivers between omapdrm and the
>> legacy omapdss based drivers.  Or is there no clean way to do that, in
>> which case we should just copy the code we need into omapdrm, and
>> leave the deprecated omapdss as it is for legacy drivers.
>
> I agree that we need to get omapdrm work well, as it's (or will be) the
> most important display framework. And if managing that requires us to
> combine omapdss and omapdrm, I'm fine with it.
>
> But, again, so far I don't understand why it's so difficult to have them
> separate with omapdss giving notifications of important events, and also
> how combining the two drivers would fix any issues (though I agree it
> could simplify some code somewhat).

I do think with enough debugging it *could* be made to work w/
separate omapdss layer.  But I'm a big fan or 'simpler is better'..
or maybe to put it another way, there is plenty of necessary
complexity (like managing memory/dmm in a dynamic way synchronized
with display/gpu/etc), so lets not make the problem more complex with
unnecessary complexity.

> So what I propose is first to look at the problems you have, so that I
> understand what the problem is with omapdss-omapdrm interaction.

It's quite likely that I'm not doing a very good job of explaining it.
 And unfortunately the current GO related issues aren't something that
show up w/ a simple fliptest/modetest.. they are showing up w/ more
complex userspace like compiz or wayland.  Although maybe I could
enhance modetest to the point where it triggers the same problems.

It would be quite useful if you could look at the omap_drm_apply
mechanism I had in omapdrm, because that seems like a quite
straightforward way to deal w/ shadowed registers.  I think it will
get a bit cleaner w/ moving the mgr code into the crtc, so I should
have an updated version of my current omapdrm-on-dispc patch later
today.

BR,
-R

>  Tomi
>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Tomi Valkeinen]

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

On Tue, 2012-07-31 at 09:45 -0500, Rob Clark wrote:
> On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> > It's not really about being friendly. Omapdss tries to do as little as
> > possible, while still supporting all its HW features. Shadow registers
> > are a bit tricky creating this mess.
> 
> What I mean by 'friendly' is it tries to abstract this for simple
> users, like an fbdev driver.  But this really quickly breaks down w/ a

No, that's not what omapdss tries to do. I'm not trying to hide the
shadow registers and the GO bit behind the omapdss API, I'm just trying
to make it work.

The omapdss API was made with omapfb, so it's true that the API may not
be good for omapdrm. But I'm happy to change the API.

> And btw, I think the current mapping of drm_encoder to mgr in omapdrm
> is not correct.  I'm just in the process of shuffling things around.
> I think drm/kms actually maps quite nicely to the underlying hardware
> with the following arrangement:
> 
>  drm_plane -> ovl
>  drm_crtc -> mgr
>  drm_encoder -> DSI/DPI/HDMI/VENC encoder
>  drm_connector -> pretty much what we call a panel driver today

Hmm, what was the arrangement earlier?

I guess the fact is that DRM concepts do not really match the OMAP DSS
hardware, and we'll have to use whatever gives us least problems.

> It would be quite useful if you could look at the omap_drm_apply
> mechanism I had in omapdrm, because that seems like a quite
> straightforward way to deal w/ shadowed registers.  I think it will

Yes, it seems straightforward, but it's not =).

I had a look at your omapdrm-on-dispc-2 branch. What you are doing there
is quite similar to what omapdss was doing earlier. It's not going to
work reliably with multiple outputs and fifomerge.

Configuring things like overlay color mode are quite simple. They only
affect that one overlay. Also things like manager default bg color are
simple, they affect only that one manager.

But enabling/disabling an overlay or a manager, changing the destination
mgr of an overlay, fifomerge... Those are not simple. You can't do them
directly, as you do in your branch.

As an example, consider the case of enabling an overlay (vid1), and
moving fifo buffers from currently enabled overlay (gfx) to vid1: you'll
first need to take the fifo buffers from gfx, set GO, and wait for the
settings to take effect. Only then you can set the fifo buffers for
vid1, enable it and set GO bit.

I didn't write omapdss's apply.c for fun or to make omapfb simpler. I
made it because the shadow register system is complex, and we need to
handle the tricky cases somewhere.

So, as I said before, I believe you'll just end up writing similar code
to what is currently in apply.c. It won't be as simple as your current
branch.

Also, as I mentioned earlier, you'll also need to handle the output side
of the shadow registers. These come from the output drivers (DPI, DSI,
etc, and indirectly from panel drivers). They are not currently handled
in the best manner in omapdss, but Archit is working on that and in his
version apply.c will handle also those properly.

About your code, I see you have these pre and post apply callbacks that
handle the configuration. Wouldn't it be rather easy to have omapdss's
apply.c call these?

And then one thing I don't think you've considered is manual update
displays. Of course, one option is to not support those with omapdrm,
but that's quite a big decision. omapdss's apply.c handles those also.

Also, can you check again my mail "Re: OMAPDSS vsyncs/apply" Sat, 12 May
2012 10:01:24, about the request_config() suggestion. I think that would
be somewhat similar to your pre/post callbacks. I'll try to write some
prototype for the request_config suggestion so that it's easier to
understand.

 Tomi



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-07-31 at 09:45 -0500, Rob Clark wrote:
>> On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>> > It's not really about being friendly. Omapdss tries to do as little as
>> > possible, while still supporting all its HW features. Shadow registers
>> > are a bit tricky creating this mess.
>>
>> What I mean by 'friendly' is it tries to abstract this for simple
>> users, like an fbdev driver.  But this really quickly breaks down w/ a
>
> No, that's not what omapdss tries to do. I'm not trying to hide the
> shadow registers and the GO bit behind the omapdss API, I'm just trying
> to make it work.
>
> The omapdss API was made with omapfb, so it's true that the API may not
> be good for omapdrm. But I'm happy to change the API.
>
>> And btw, I think the current mapping of drm_encoder to mgr in omapdrm
>> is not correct.  I'm just in the process of shuffling things around.
>> I think drm/kms actually maps quite nicely to the underlying hardware
>> with the following arrangement:
>>
>>  drm_plane -> ovl
>>  drm_crtc -> mgr
>>  drm_encoder -> DSI/DPI/HDMI/VENC encoder
>>  drm_connector -> pretty much what we call a panel driver today
>
> Hmm, what was the arrangement earlier?

it was previously:

  plane -> ovl
  crtc -> placeholder
  encoder -> mgr
  connector -> dssdev (encoder+panel)

although crtc is really the point where you should enable/disable
vblank irqs, so the new arrangement is somewhat cleaner (although on
my branch the encoder/connector part are not finished yet)

> I guess the fact is that DRM concepts do not really match the OMAP DSS
> hardware, and we'll have to use whatever gives us least problems.

Actually, I think it does map fairly well to the hardware.. at least
more so than to omapdss ;-)

The one area that kms mismatches a bit is decoupling of ovl from mgr
that we have in our hw..  I've partially solved that a while back w/
the patch in drm to add "private planes" so the omap_crtc internally
uses an omap_plane.  It isn't exposed to userspace to be able to
re-use the planes from unused crtcs, although I have some ideas about
that (but not yet time to work on it).

>> It would be quite useful if you could look at the omap_drm_apply
>> mechanism I had in omapdrm, because that seems like a quite
>> straightforward way to deal w/ shadowed registers.  I think it will
>
> Yes, it seems straightforward, but it's not =).
>
> I had a look at your omapdrm-on-dispc-2 branch. What you are doing there
> is quite similar to what omapdss was doing earlier. It's not going to
> work reliably with multiple outputs and fifomerge.
>
> Configuring things like overlay color mode are quite simple. They only
> affect that one overlay. Also things like manager default bg color are
> simple, they affect only that one manager.
>
> But enabling/disabling an overlay or a manager, changing the destination
> mgr of an overlay, fifomerge... Those are not simple. You can't do them
> directly, as you do in your branch.
>
> As an example, consider the case of enabling an overlay (vid1), and
> moving fifo buffers from currently enabled overlay (gfx) to vid1: you'll
> first need to take the fifo buffers from gfx, set GO, and wait for the
> settings to take effect. Only then you can set the fifo buffers for
> vid1, enable it and set GO bit.

hmm, it does sound like it needs a bit of a state machine to deal with
multi-step updates.. although that makes races more of a problem,
which was something I was trying hard to avoid.

For enabling/disabling an output (manager+encoder), this is relatively
infrequent, so it can afford to block to avoid races.  (Like userspace
enabling and then rapidly disabling an output part way through the
enable.)  But enabling/disabling an overlay, or adjusting position or
scanout address must not block.  And ideally, if possible, switching
an overlay between two managers should not block.

For fifomerge, if I understand correctly, it shouldn't really be
needed for functionality, but mainly as a power optimization?  If this
is the case I wonder about an approach of disabling fifomerge when
there are ongoing setting changes, and then setting it after things
settle down?  I'll have to think about it, but I was trying to avoid
needing a multi-step state machine to avoid the associated race
conditions, but if this is not possible then it is not possible.

> I didn't write omapdss's apply.c for fun or to make omapfb simpler. I
> made it because the shadow register system is complex, and we need to
> handle the tricky cases somewhere.
>
> So, as I said before, I believe you'll just end up writing similar code
> to what is currently in apply.c. It won't be as simple as your current
> branch.
>
> Also, as I mentioned earlier, you'll also need to handle the output side
> of the shadow registers. These come from the output drivers (DPI, DSI,
> etc, and indirectly from panel drivers). They are not currently handled
> in the best manner in omapdss, but Archit is working on that and in his
> version apply.c will handle also those properly.

The encoder/connector part of things is something that I have not
tackled yet.. but I expect if there is something that can handle
fifomerge, etc, then it should also be usable from the encoder code.

I need to have a closer look at the patches from Archit (I assume you
are talking about the series he sent earlier today) and see if that
makes things easier for me to map properly to kms encoder/connector.

> About your code, I see you have these pre and post apply callbacks that
> handle the configuration. Wouldn't it be rather easy to have omapdss's
> apply.c call these?

Possibly.. really what I am working on now is a proof of concept.  But
I think that once it works properly, if there is a way to shuffle
things around to get more re-use from omapfb/etc, then that would be a
good idea.  I'm not opposed to that.  But we at least need to figure
out how to get it working properly for drm/kms's needs.

> And then one thing I don't think you've considered is manual update
> displays. Of course, one option is to not support those with omapdrm,
> but that's quite a big decision. omapdss's apply.c handles those also.

well, mainly because it is only proof of concept so far, and I don't
actually have any hardware w/ a manual update display.  But I think
manual update needs some more work at a few layers.  We need userspace
xorg driver to call DRM_IOCTL_MODE_DIRTYFB at the appropriate times
(in case it is doing front buffer rendering), then on kernel side we
need to defer until gpu access has finished (similar to how a
page_flip is handled).  After that, if I understand properly, we can
use the same apply mechanism to kick the encoder to push the update
out to the display.

> Also, can you check again my mail "Re: OMAPDSS vsyncs/apply" Sat, 12 May
> 2012 10:01:24, about the request_config() suggestion. I think that would
> be somewhat similar to your pre/post callbacks. I'll try to write some
> prototype for the request_config suggestion so that it's easier to
> understand.

I'll look again, but as far as I remember that at least wasn't
addressing the performance issues from making overlay enable/disable
synchronous.  And fixing that would, I expect, trigger the same
problems that I already spent a few days debugging before switching
over to handle apply in omapdrm.

BR,
-R

>  Tomi
>
>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Archit Taneja]

Hi,

On Wednesday 01 August 2012 07:55 PM, Rob Clark wrote:
> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Tue, 2012-07-31 at 09:45 -0500, Rob Clark wrote:
>>> On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>>>> It's not really about being friendly. Omapdss tries to do as little as
>>>> possible, while still supporting all its HW features. Shadow registers
>>>> are a bit tricky creating this mess.
>>>
>>> What I mean by 'friendly' is it tries to abstract this for simple
>>> users, like an fbdev driver.  But this really quickly breaks down w/ a
>>
>> No, that's not what omapdss tries to do. I'm not trying to hide the
>> shadow registers and the GO bit behind the omapdss API, I'm just trying
>> to make it work.
>>
>> The omapdss API was made with omapfb, so it's true that the API may not
>> be good for omapdrm. But I'm happy to change the API.
>>
>>> And btw, I think the current mapping of drm_encoder to mgr in omapdrm
>>> is not correct.  I'm just in the process of shuffling things around.
>>> I think drm/kms actually maps quite nicely to the underlying hardware
>>> with the following arrangement:
>>>
>>>   drm_plane -> ovl
>>>   drm_crtc -> mgr
>>>   drm_encoder -> DSI/DPI/HDMI/VENC encoder
>>>   drm_connector -> pretty much what we call a panel driver today
>>
>> Hmm, what was the arrangement earlier?
>
> it was previously:
>
>    plane -> ovl
>    crtc -> placeholder
>    encoder -> mgr
>    connector -> dssdev (encoder+panel)
>
> although crtc is really the point where you should enable/disable
> vblank irqs, so the new arrangement is somewhat cleaner (although on
> my branch the encoder/connector part are not finished yet)
>
>> I guess the fact is that DRM concepts do not really match the OMAP DSS
>> hardware, and we'll have to use whatever gives us least problems.
>
> Actually, I think it does map fairly well to the hardware.. at least
> more so than to omapdss ;-)
>
> The one area that kms mismatches a bit is decoupling of ovl from mgr
> that we have in our hw..  I've partially solved that a while back w/
> the patch in drm to add "private planes" so the omap_crtc internally
> uses an omap_plane.  It isn't exposed to userspace to be able to
> re-use the planes from unused crtcs, although I have some ideas about
> that (but not yet time to work on it).
>
>>> It would be quite useful if you could look at the omap_drm_apply
>>> mechanism I had in omapdrm, because that seems like a quite
>>> straightforward way to deal w/ shadowed registers.  I think it will
>>
>> Yes, it seems straightforward, but it's not =).
>>
>> I had a look at your omapdrm-on-dispc-2 branch. What you are doing there
>> is quite similar to what omapdss was doing earlier. It's not going to
>> work reliably with multiple outputs and fifomerge.
>>
>> Configuring things like overlay color mode are quite simple. They only
>> affect that one overlay. Also things like manager default bg color are
>> simple, they affect only that one manager.
>>
>> But enabling/disabling an overlay or a manager, changing the destination
>> mgr of an overlay, fifomerge... Those are not simple. You can't do them
>> directly, as you do in your branch.
>>
>> As an example, consider the case of enabling an overlay (vid1), and
>> moving fifo buffers from currently enabled overlay (gfx) to vid1: you'll
>> first need to take the fifo buffers from gfx, set GO, and wait for the
>> settings to take effect. Only then you can set the fifo buffers for
>> vid1, enable it and set GO bit.
>
> hmm, it does sound like it needs a bit of a state machine to deal with
> multi-step updates.. although that makes races more of a problem,
> which was something I was trying hard to avoid.
>
> For enabling/disabling an output (manager+encoder), this is relatively
> infrequent, so it can afford to block to avoid races.  (Like userspace
> enabling and then rapidly disabling an output part way through the
> enable.)  But enabling/disabling an overlay, or adjusting position or
> scanout address must not block.  And ideally, if possible, switching
> an overlay between two managers should not block.
>
> For fifomerge, if I understand correctly, it shouldn't really be
> needed for functionality, but mainly as a power optimization?  If this
> is the case I wonder about an approach of disabling fifomerge when
> there are ongoing setting changes, and then setting it after things
> settle down?  I'll have to think about it, but I was trying to avoid
> needing a multi-step state machine to avoid the associated race
> conditions, but if this is not possible then it is not possible.
>
>> I didn't write omapdss's apply.c for fun or to make omapfb simpler. I
>> made it because the shadow register system is complex, and we need to
>> handle the tricky cases somewhere.
>>
>> So, as I said before, I believe you'll just end up writing similar code
>> to what is currently in apply.c. It won't be as simple as your current
>> branch.
>>
>> Also, as I mentioned earlier, you'll also need to handle the output side
>> of the shadow registers. These come from the output drivers (DPI, DSI,
>> etc, and indirectly from panel drivers). They are not currently handled
>> in the best manner in omapdss, but Archit is working on that and in his
>> version apply.c will handle also those properly.
>
> The encoder/connector part of things is something that I have not
> tackled yet.. but I expect if there is something that can handle
> fifomerge, etc, then it should also be usable from the encoder code.
>
> I need to have a closer look at the patches from Archit (I assume you
> are talking about the series he sent earlier today) and see if that
> makes things easier for me to map properly to kms encoder/connector.

I guess the work Tomi is talking about is already merged in 3.6. It 
ensures that interface drivers(DSI/HDMI) don't do direct DISPC register 
writes on overlay managers. For example, when HDMI's timings are 
changed, the TV manager's DISPC_SIZE_DIGIT needs to be configured, and 
it's a shadow register. There was no guarantee previously that when the 
HDMI driver writes to this register the GO bit of the TV manager is clear.

The stuff I posted today is a part of a bigger series, it's final aim is 
to have an entity in omapdss which is an equivalent of drm_encoder in 
your new drm arrangement, i.e, an entity which represents an interface. 
We call it outputs, a manager would now connect to an output instead of 
a panel, and the output would now connect to the panel. So the 
connection will be like:

ovl->manager->output->device

The whole set is in the tree below, I'm posting the set out in smaller 
parts.

git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git 
out_work_23_july

Archit

>
>> About your code, I see you have these pre and post apply callbacks that
>> handle the configuration. Wouldn't it be rather easy to have omapdss's
>> apply.c call these?
>
> Possibly.. really what I am working on now is a proof of concept.  But
> I think that once it works properly, if there is a way to shuffle
> things around to get more re-use from omapfb/etc, then that would be a
> good idea.  I'm not opposed to that.  But we at least need to figure
> out how to get it working properly for drm/kms's needs.
>
>> And then one thing I don't think you've considered is manual update
>> displays. Of course, one option is to not support those with omapdrm,
>> but that's quite a big decision. omapdss's apply.c handles those also.
>
> well, mainly because it is only proof of concept so far, and I don't
> actually have any hardware w/ a manual update display.  But I think
> manual update needs some more work at a few layers.  We need userspace
> xorg driver to call DRM_IOCTL_MODE_DIRTYFB at the appropriate times
> (in case it is doing front buffer rendering), then on kernel side we
> need to defer until gpu access has finished (similar to how a
> page_flip is handled).  After that, if I understand properly, we can
> use the same apply mechanism to kick the encoder to push the update
> out to the display.
>
>> Also, can you check again my mail "Re: OMAPDSS vsyncs/apply" Sat, 12 May
>> 2012 10:01:24, about the request_config() suggestion. I think that would
>> be somewhat similar to your pre/post callbacks. I'll try to write some
>> prototype for the request_config suggestion so that it's easier to
>> understand.
>
> I'll look again, but as far as I remember that at least wasn't
> addressing the performance issues from making overlay enable/disable
> synchronous.  And fixing that would, I expect, trigger the same
> problems that I already spent a few days debugging before switching
> over to handle apply in omapdrm.
>
> BR,
> -R
>
>>   Tomi
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Wed, Aug 1, 2012 at 11:46 AM, Archit Taneja <archit@ti.com> wrote:
> Hi,
>
>
> On Wednesday 01 August 2012 07:55 PM, Rob Clark wrote:
>>
>> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
>> wrote:
>>>
>>> On Tue, 2012-07-31 at 09:45 -0500, Rob Clark wrote:
>>>>
>>>> On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> wrote:
>>>
>>>
>>>>> It's not really about being friendly. Omapdss tries to do as little as
>>>>> possible, while still supporting all its HW features. Shadow registers
>>>>> are a bit tricky creating this mess.
>>>>
>>>>
>>>> What I mean by 'friendly' is it tries to abstract this for simple
>>>> users, like an fbdev driver.  But this really quickly breaks down w/ a
>>>
>>>
>>> No, that's not what omapdss tries to do. I'm not trying to hide the
>>> shadow registers and the GO bit behind the omapdss API, I'm just trying
>>> to make it work.
>>>
>>> The omapdss API was made with omapfb, so it's true that the API may not
>>> be good for omapdrm. But I'm happy to change the API.
>>>
>>>> And btw, I think the current mapping of drm_encoder to mgr in omapdrm
>>>> is not correct.  I'm just in the process of shuffling things around.
>>>> I think drm/kms actually maps quite nicely to the underlying hardware
>>>> with the following arrangement:
>>>>
>>>>   drm_plane -> ovl
>>>>   drm_crtc -> mgr
>>>>   drm_encoder -> DSI/DPI/HDMI/VENC encoder
>>>>   drm_connector -> pretty much what we call a panel driver today
>>>
>>>
>>> Hmm, what was the arrangement earlier?
>>
>>
>> it was previously:
>>
>>    plane -> ovl
>>    crtc -> placeholder
>>    encoder -> mgr
>>    connector -> dssdev (encoder+panel)
>>
>> although crtc is really the point where you should enable/disable
>> vblank irqs, so the new arrangement is somewhat cleaner (although on
>> my branch the encoder/connector part are not finished yet)
>>
>>> I guess the fact is that DRM concepts do not really match the OMAP DSS
>>> hardware, and we'll have to use whatever gives us least problems.
>>
>>
>> Actually, I think it does map fairly well to the hardware.. at least
>> more so than to omapdss ;-)
>>
>> The one area that kms mismatches a bit is decoupling of ovl from mgr
>> that we have in our hw..  I've partially solved that a while back w/
>> the patch in drm to add "private planes" so the omap_crtc internally
>> uses an omap_plane.  It isn't exposed to userspace to be able to
>> re-use the planes from unused crtcs, although I have some ideas about
>> that (but not yet time to work on it).
>>
>>>> It would be quite useful if you could look at the omap_drm_apply
>>>> mechanism I had in omapdrm, because that seems like a quite
>>>> straightforward way to deal w/ shadowed registers.  I think it will
>>>
>>>
>>> Yes, it seems straightforward, but it's not =).
>>>
>>> I had a look at your omapdrm-on-dispc-2 branch. What you are doing there
>>> is quite similar to what omapdss was doing earlier. It's not going to
>>> work reliably with multiple outputs and fifomerge.
>>>
>>> Configuring things like overlay color mode are quite simple. They only
>>> affect that one overlay. Also things like manager default bg color are
>>> simple, they affect only that one manager.
>>>
>>> But enabling/disabling an overlay or a manager, changing the destination
>>> mgr of an overlay, fifomerge... Those are not simple. You can't do them
>>> directly, as you do in your branch.
>>>
>>> As an example, consider the case of enabling an overlay (vid1), and
>>> moving fifo buffers from currently enabled overlay (gfx) to vid1: you'll
>>> first need to take the fifo buffers from gfx, set GO, and wait for the
>>> settings to take effect. Only then you can set the fifo buffers for
>>> vid1, enable it and set GO bit.
>>
>>
>> hmm, it does sound like it needs a bit of a state machine to deal with
>> multi-step updates.. although that makes races more of a problem,
>> which was something I was trying hard to avoid.
>>
>> For enabling/disabling an output (manager+encoder), this is relatively
>> infrequent, so it can afford to block to avoid races.  (Like userspace
>> enabling and then rapidly disabling an output part way through the
>> enable.)  But enabling/disabling an overlay, or adjusting position or
>> scanout address must not block.  And ideally, if possible, switching
>> an overlay between two managers should not block.
>>
>> For fifomerge, if I understand correctly, it shouldn't really be
>> needed for functionality, but mainly as a power optimization?  If this
>> is the case I wonder about an approach of disabling fifomerge when
>> there are ongoing setting changes, and then setting it after things
>> settle down?  I'll have to think about it, but I was trying to avoid
>> needing a multi-step state machine to avoid the associated race
>> conditions, but if this is not possible then it is not possible.
>>
>>> I didn't write omapdss's apply.c for fun or to make omapfb simpler. I
>>> made it because the shadow register system is complex, and we need to
>>> handle the tricky cases somewhere.
>>>
>>> So, as I said before, I believe you'll just end up writing similar code
>>> to what is currently in apply.c. It won't be as simple as your current
>>> branch.
>>>
>>> Also, as I mentioned earlier, you'll also need to handle the output side
>>> of the shadow registers. These come from the output drivers (DPI, DSI,
>>> etc, and indirectly from panel drivers). They are not currently handled
>>> in the best manner in omapdss, but Archit is working on that and in his
>>> version apply.c will handle also those properly.
>>
>>
>> The encoder/connector part of things is something that I have not
>> tackled yet.. but I expect if there is something that can handle
>> fifomerge, etc, then it should also be usable from the encoder code.
>>
>> I need to have a closer look at the patches from Archit (I assume you
>> are talking about the series he sent earlier today) and see if that
>> makes things easier for me to map properly to kms encoder/connector.
>
>
> I guess the work Tomi is talking about is already merged in 3.6. It ensures
> that interface drivers(DSI/HDMI) don't do direct DISPC register writes on
> overlay managers. For example, when HDMI's timings are changed, the TV
> manager's DISPC_SIZE_DIGIT needs to be configured, and it's a shadow
> register. There was no guarantee previously that when the HDMI driver writes
> to this register the GO bit of the TV manager is clear.

ahh, ok.. actually I've commented out (I think) all of the mgr
register updates from the HDMI driver for my prototype.  These already
get set properly from the kms crtc (going through GO bit / apply
mechanism to synchronize w/ GO bit), so I don't even need the
interface/panel driver to set this stuff up.

> The stuff I posted today is a part of a bigger series, it's final aim is to
> have an entity in omapdss which is an equivalent of drm_encoder in your new
> drm arrangement, i.e, an entity which represents an interface. We call it
> outputs, a manager would now connect to an output instead of a panel, and
> the output would now connect to the panel. So the connection will be like:

Ok.. this would help.  I'll take a look.  I do request that
interfaces/panels don't set any mgr/timing related registers.  I had
to comment all this stuff out in my prototype.  Really we want to set
the timings separately on the crtc (mgr) / encoder (interface) /
connector (panel.. not sure if it is needed, though?).  KMS will take
care of propagating the timings through the pipeline.

BR,
-R

> ovl->manager->output->device
>
> The whole set is in the tree below, I'm posting the set out in smaller
> parts.
>
> git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git
> out_work_23_july
>
> Archit
>
>>
>>> About your code, I see you have these pre and post apply callbacks that
>>> handle the configuration. Wouldn't it be rather easy to have omapdss's
>>> apply.c call these?
>>
>>
>> Possibly.. really what I am working on now is a proof of concept.  But
>> I think that once it works properly, if there is a way to shuffle
>> things around to get more re-use from omapfb/etc, then that would be a
>> good idea.  I'm not opposed to that.  But we at least need to figure
>> out how to get it working properly for drm/kms's needs.
>>
>>> And then one thing I don't think you've considered is manual update
>>> displays. Of course, one option is to not support those with omapdrm,
>>> but that's quite a big decision. omapdss's apply.c handles those also.
>>
>>
>> well, mainly because it is only proof of concept so far, and I don't
>> actually have any hardware w/ a manual update display.  But I think
>> manual update needs some more work at a few layers.  We need userspace
>> xorg driver to call DRM_IOCTL_MODE_DIRTYFB at the appropriate times
>> (in case it is doing front buffer rendering), then on kernel side we
>> need to defer until gpu access has finished (similar to how a
>> page_flip is handled).  After that, if I understand properly, we can
>> use the same apply mechanism to kick the encoder to push the update
>> out to the display.
>>
>>> Also, can you check again my mail "Re: OMAPDSS vsyncs/apply" Sat, 12 May
>>> 2012 10:01:24, about the request_config() suggestion. I think that would
>>> be somewhat similar to your pre/post callbacks. I'll try to write some
>>> prototype for the request_config suggestion so that it's easier to
>>> understand.
>>
>>
>> I'll look again, but as far as I remember that at least wasn't
>> addressing the performance issues from making overlay enable/disable
>> synchronous.  And fixing that would, I expect, trigger the same
>> problems that I already spent a few days debugging before switching
>> over to handle apply in omapdrm.
>>
>> BR,
>> -R
>>
>>>   Tomi
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Wed, Aug 1, 2012 at 11:53 AM, Rob Clark <rob.clark@linaro.org> wrote:
> On Wed, Aug 1, 2012 at 11:46 AM, Archit Taneja <archit@ti.com> wrote:
>> Hi,
>>
>>
>> On Wednesday 01 August 2012 07:55 PM, Rob Clark wrote:
>>>
>>> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> wrote:
>>>>
>>>> On Tue, 2012-07-31 at 09:45 -0500, Rob Clark wrote:
>>>>>
>>>>> On Tue, Jul 31, 2012 at 8:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>> wrote:
>>>>
>>>>
>>>>>> It's not really about being friendly. Omapdss tries to do as little as
>>>>>> possible, while still supporting all its HW features. Shadow registers
>>>>>> are a bit tricky creating this mess.
>>>>>
>>>>>
>>>>> What I mean by 'friendly' is it tries to abstract this for simple
>>>>> users, like an fbdev driver.  But this really quickly breaks down w/ a
>>>>
>>>>
>>>> No, that's not what omapdss tries to do. I'm not trying to hide the
>>>> shadow registers and the GO bit behind the omapdss API, I'm just trying
>>>> to make it work.
>>>>
>>>> The omapdss API was made with omapfb, so it's true that the API may not
>>>> be good for omapdrm. But I'm happy to change the API.
>>>>
>>>>> And btw, I think the current mapping of drm_encoder to mgr in omapdrm
>>>>> is not correct.  I'm just in the process of shuffling things around.
>>>>> I think drm/kms actually maps quite nicely to the underlying hardware
>>>>> with the following arrangement:
>>>>>
>>>>>   drm_plane -> ovl
>>>>>   drm_crtc -> mgr
>>>>>   drm_encoder -> DSI/DPI/HDMI/VENC encoder
>>>>>   drm_connector -> pretty much what we call a panel driver today
>>>>
>>>>
>>>> Hmm, what was the arrangement earlier?
>>>
>>>
>>> it was previously:
>>>
>>>    plane -> ovl
>>>    crtc -> placeholder
>>>    encoder -> mgr
>>>    connector -> dssdev (encoder+panel)
>>>
>>> although crtc is really the point where you should enable/disable
>>> vblank irqs, so the new arrangement is somewhat cleaner (although on
>>> my branch the encoder/connector part are not finished yet)
>>>
>>>> I guess the fact is that DRM concepts do not really match the OMAP DSS
>>>> hardware, and we'll have to use whatever gives us least problems.
>>>
>>>
>>> Actually, I think it does map fairly well to the hardware.. at least
>>> more so than to omapdss ;-)
>>>
>>> The one area that kms mismatches a bit is decoupling of ovl from mgr
>>> that we have in our hw..  I've partially solved that a while back w/
>>> the patch in drm to add "private planes" so the omap_crtc internally
>>> uses an omap_plane.  It isn't exposed to userspace to be able to
>>> re-use the planes from unused crtcs, although I have some ideas about
>>> that (but not yet time to work on it).
>>>
>>>>> It would be quite useful if you could look at the omap_drm_apply
>>>>> mechanism I had in omapdrm, because that seems like a quite
>>>>> straightforward way to deal w/ shadowed registers.  I think it will
>>>>
>>>>
>>>> Yes, it seems straightforward, but it's not =).
>>>>
>>>> I had a look at your omapdrm-on-dispc-2 branch. What you are doing there
>>>> is quite similar to what omapdss was doing earlier. It's not going to
>>>> work reliably with multiple outputs and fifomerge.
>>>>
>>>> Configuring things like overlay color mode are quite simple. They only
>>>> affect that one overlay. Also things like manager default bg color are
>>>> simple, they affect only that one manager.
>>>>
>>>> But enabling/disabling an overlay or a manager, changing the destination
>>>> mgr of an overlay, fifomerge... Those are not simple. You can't do them
>>>> directly, as you do in your branch.
>>>>
>>>> As an example, consider the case of enabling an overlay (vid1), and
>>>> moving fifo buffers from currently enabled overlay (gfx) to vid1: you'll
>>>> first need to take the fifo buffers from gfx, set GO, and wait for the
>>>> settings to take effect. Only then you can set the fifo buffers for
>>>> vid1, enable it and set GO bit.
>>>
>>>
>>> hmm, it does sound like it needs a bit of a state machine to deal with
>>> multi-step updates.. although that makes races more of a problem,
>>> which was something I was trying hard to avoid.
>>>
>>> For enabling/disabling an output (manager+encoder), this is relatively
>>> infrequent, so it can afford to block to avoid races.  (Like userspace
>>> enabling and then rapidly disabling an output part way through the
>>> enable.)  But enabling/disabling an overlay, or adjusting position or
>>> scanout address must not block.  And ideally, if possible, switching
>>> an overlay between two managers should not block.
>>>
>>> For fifomerge, if I understand correctly, it shouldn't really be
>>> needed for functionality, but mainly as a power optimization?  If this
>>> is the case I wonder about an approach of disabling fifomerge when
>>> there are ongoing setting changes, and then setting it after things
>>> settle down?  I'll have to think about it, but I was trying to avoid
>>> needing a multi-step state machine to avoid the associated race
>>> conditions, but if this is not possible then it is not possible.
>>>
>>>> I didn't write omapdss's apply.c for fun or to make omapfb simpler. I
>>>> made it because the shadow register system is complex, and we need to
>>>> handle the tricky cases somewhere.
>>>>
>>>> So, as I said before, I believe you'll just end up writing similar code
>>>> to what is currently in apply.c. It won't be as simple as your current
>>>> branch.
>>>>
>>>> Also, as I mentioned earlier, you'll also need to handle the output side
>>>> of the shadow registers. These come from the output drivers (DPI, DSI,
>>>> etc, and indirectly from panel drivers). They are not currently handled
>>>> in the best manner in omapdss, but Archit is working on that and in his
>>>> version apply.c will handle also those properly.
>>>
>>>
>>> The encoder/connector part of things is something that I have not
>>> tackled yet.. but I expect if there is something that can handle
>>> fifomerge, etc, then it should also be usable from the encoder code.
>>>
>>> I need to have a closer look at the patches from Archit (I assume you
>>> are talking about the series he sent earlier today) and see if that
>>> makes things easier for me to map properly to kms encoder/connector.
>>
>>
>> I guess the work Tomi is talking about is already merged in 3.6. It ensures
>> that interface drivers(DSI/HDMI) don't do direct DISPC register writes on
>> overlay managers. For example, when HDMI's timings are changed, the TV
>> manager's DISPC_SIZE_DIGIT needs to be configured, and it's a shadow
>> register. There was no guarantee previously that when the HDMI driver writes
>> to this register the GO bit of the TV manager is clear.
>
> ahh, ok.. actually I've commented out (I think) all of the mgr
> register updates from the HDMI driver for my prototype.  These already
> get set properly from the kms crtc (going through GO bit / apply
> mechanism to synchronize w/ GO bit), so I don't even need the
> interface/panel driver to set this stuff up.
>
>> The stuff I posted today is a part of a bigger series, it's final aim is to
>> have an entity in omapdss which is an equivalent of drm_encoder in your new
>> drm arrangement, i.e, an entity which represents an interface. We call it
>> outputs, a manager would now connect to an output instead of a panel, and
>> the output would now connect to the panel. So the connection will be like:
>
> Ok.. this would help.  I'll take a look.  I do request that
> interfaces/panels don't set any mgr/timing related registers.  I had

sorry, that should say "any mgr/ovl related registers".. basically not
having any under-the-hood connections between the entities at the
omapdss level would be ideal

BR,
-R

> to comment all this stuff out in my prototype.  Really we want to set
> the timings separately on the crtc (mgr) / encoder (interface) /
> connector (panel.. not sure if it is needed, though?).  KMS will take
> care of propagating the timings through the pipeline.
>
> BR,
> -R
>
>> ovl->manager->output->device
>>
>> The whole set is in the tree below, I'm posting the set out in smaller
>> parts.
>>
>> git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git
>> out_work_23_july
>>
>> Archit
>>
>>>
>>>> About your code, I see you have these pre and post apply callbacks that
>>>> handle the configuration. Wouldn't it be rather easy to have omapdss's
>>>> apply.c call these?
>>>
>>>
>>> Possibly.. really what I am working on now is a proof of concept.  But
>>> I think that once it works properly, if there is a way to shuffle
>>> things around to get more re-use from omapfb/etc, then that would be a
>>> good idea.  I'm not opposed to that.  But we at least need to figure
>>> out how to get it working properly for drm/kms's needs.
>>>
>>>> And then one thing I don't think you've considered is manual update
>>>> displays. Of course, one option is to not support those with omapdrm,
>>>> but that's quite a big decision. omapdss's apply.c handles those also.
>>>
>>>
>>> well, mainly because it is only proof of concept so far, and I don't
>>> actually have any hardware w/ a manual update display.  But I think
>>> manual update needs some more work at a few layers.  We need userspace
>>> xorg driver to call DRM_IOCTL_MODE_DIRTYFB at the appropriate times
>>> (in case it is doing front buffer rendering), then on kernel side we
>>> need to defer until gpu access has finished (similar to how a
>>> page_flip is handled).  After that, if I understand properly, we can
>>> use the same apply mechanism to kick the encoder to push the update
>>> out to the display.
>>>
>>>> Also, can you check again my mail "Re: OMAPDSS vsyncs/apply" Sat, 12 May
>>>> 2012 10:01:24, about the request_config() suggestion. I think that would
>>>> be somewhat similar to your pre/post callbacks. I'll try to write some
>>>> prototype for the request_config suggestion so that it's easier to
>>>> understand.
>>>
>>>
>>> I'll look again, but as far as I remember that at least wasn't
>>> addressing the performance issues from making overlay enable/disable
>>> synchronous.  And fixing that would, I expect, trigger the same
>>> problems that I already spent a few days debugging before switching
>>> over to handle apply in omapdrm.
>>>
>>> BR,
>>> -R
>>>
>>>>   Tomi
>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Tomi Valkeinen]

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

On Wed, 2012-08-01 at 11:53 -0500, Rob Clark wrote:

> Ok.. this would help.  I'll take a look.  I do request that
> interfaces/panels don't set any mgr/timing related registers.  I had
> to comment all this stuff out in my prototype.  Really we want to set
> the timings separately on the crtc (mgr) / encoder (interface) /
> connector (panel.. not sure if it is needed, though?).  KMS will take
> care of propagating the timings through the pipeline.

If we only had auto-update displays, and only the video timings were
shadow register, it'd work. Unfortunately we have other registers as
shadow registers also, like DISPC_CONTROL1, DISPC_CONFIG1 and
DISPC_DIVISOR1.

But we should think if this could be somehow be changed, so that all the
shadow register info would come from one place. I do find it a bit
unlikely with a quick thought, though.

Well, hmm. Perhaps... Omapdrm (or omapfb etc) doesn't really need to
know about the values of those registers, it just needs to control the
GO bit. So perhaps if we had some method to inform omapdrm that these
things have changed, and omapdrm would then set the GO bit as soon as
possible.

But there are some tricky stuff, like the divisors... Well, we need to
think about this.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Wed, Aug 1, 2012 at 12:38 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2012-08-01 at 11:53 -0500, Rob Clark wrote:
>
>> Ok.. this would help.  I'll take a look.  I do request that
>> interfaces/panels don't set any mgr/timing related registers.  I had
>> to comment all this stuff out in my prototype.  Really we want to set
>> the timings separately on the crtc (mgr) / encoder (interface) /
>> connector (panel.. not sure if it is needed, though?).  KMS will take
>> care of propagating the timings through the pipeline.
>
> If we only had auto-update displays, and only the video timings were
> shadow register, it'd work. Unfortunately we have other registers as
> shadow registers also, like DISPC_CONTROL1, DISPC_CONFIG1 and
> DISPC_DIVISOR1.

well, I was kinda thinking we just do all the register access from the
corresponding crtc (mgr)'s GO/apply sequencing..  so if, for example,
you change resolution, then the plane, crtc, encoder, panel all queue
up via omap_crtc_apply() on their associated crtc, and then at the
right time from pre_apply() fxns call the appropriate omapdss/dispc
function(s) for register updates.

I think that would work well for everything but mgr enable/disable
(which is infrequent, so ok to block for a few vblanks), and
fifomerge, which I'm a bit on the fence about.

> But we should think if this could be somehow be changed, so that all the
> shadow register info would come from one place. I do find it a bit
> unlikely with a quick thought, though.
>
> Well, hmm. Perhaps... Omapdrm (or omapfb etc) doesn't really need to
> know about the values of those registers, it just needs to control the
> GO bit. So perhaps if we had some method to inform omapdrm that these
> things have changed, and omapdrm would then set the GO bit as soon as
> possible.

Well, what I'm doing now is basically, if I update anything in any of
the omap_*_info structs, I schedule an apply, and from pre_apply
callback push the changes down to dispc.. I was thinking to follow the
same for encoder and probably connector.  (Not sure if doing things
like setting timings at hdmi need to be GO bit sync'd?  Maybe this
could be bypassed for the connector, but if not it is easy enough just
to use the same mechanism that the plane/crtc/encoder are already
using.)

> But there are some tricky stuff, like the divisors... Well, we need to
> think about this.

I think, if I understand properly, the most tricky thing is the shared
clocks.. although I'm not really sure if we can actually change things
like the core clock when you plug in a 2nd display w/out loosing sync
on the first?  That's seems like a tricky thing either way.  Anything
else, that is only effecting a single crtc->encoder->connector chain
can have register programming sync'd to that mgr's GO bit.

BR,
-R

>  Tomi
>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Tomi Valkeinen]

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

On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> > I guess the fact is that DRM concepts do not really match the OMAP DSS
> > hardware, and we'll have to use whatever gives us least problems.
> 
> Actually, I think it does map fairly well to the hardware.. at least
> more so than to omapdss ;-)

Hm, I'm not sure I understand, omapdss concepts map directly to the
hardware.

> The one area that kms mismatches a bit is decoupling of ovl from mgr
> that we have in our hw..  I've partially solved that a while back w/

What do you mean with that? Ovls and mgrs are one entity in KMS? Didn't
the drm_plane stuff separate these?

> For enabling/disabling an output (manager+encoder), this is relatively
> infrequent, so it can afford to block to avoid races.  (Like userspace
> enabling and then rapidly disabling an output part way through the
> enable.)  But enabling/disabling an overlay, or adjusting position or
> scanout address must not block.  And ideally, if possible, switching
> an overlay between two managers should not block.

Adjusting the position or address of the buffer are simple, they can be
easily done without any blocking.

But ovl enable/disable and switching an ovl to another mgr do (possibly)
take multiple vsyncs (and in the switch case, vsyncs of two separate
outputs). So if those do not block, we'll need to handle them as a state
machine and try to avoid races etc. It'll be "interesting".

However, we can sometimes do those operations immediately. So I think we
should have these conditional fast-paths in the code, and do them in
non-blocking manner when possible.

> I'll look again, but as far as I remember that at least wasn't
> addressing the performance issues from making overlay enable/disable

Right, it wasn't addressing those issues. But your branch doesn't really
address those issues either, as it doesn't handle the problems related
to ovl enable/disable.

> synchronous.  And fixing that would, I expect, trigger the same
> problems that I already spent a few days debugging before switching
> over to handle apply in omapdrm.

I was under the impression that the apply mechanism, the caching and
setting of the configs, was the major issue you had. But you're hinting
that the actual problem is related to ovl enable/disable? I haven't
tried fixing the ovl enable/disable, as I didn't know it's an issue for
omapdrm. Or are they both as big issues?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
>> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>> > I guess the fact is that DRM concepts do not really match the OMAP DSS
>> > hardware, and we'll have to use whatever gives us least problems.
>>
>> Actually, I think it does map fairly well to the hardware.. at least
>> more so than to omapdss ;-)
>
> Hm, I'm not sure I understand, omapdss concepts map directly to the
> hardware.

I think it is mainly exposing the encoder and panel as two separate
entities.. which seems to be what Archit is working on

in case of something like DVI bridge from DPI, this seems pretty
straightforward.. only the connector needs to know about DDC stuff,
which i2c to use and that sort of thing.  So at kms level we would
have (for example) an omap_dpi_encoder which would be the same for DPI
panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
through the code to see how this would work.  Honestly I've looked
less at this part of code and encoder related registers in the TRM,
compared to the ovl/mgr parts, but at least from the 'DSS overview'
picture in the TRM it seems to make sense ;-)

KMS even exposes the idea that certain crtcs can connect to only
certain encoders.  Or that you could you could have certain connectors
switched between encoders.  For example if you had a hw w/ DPI out,
and some mux to switch that back and forth between a DPI lcd panel and
a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
this, but it is in theory possible.)  So we could expose possible
video chain topologies to userspace in this way.

The other thing is that we don't need to propagate timings from the
panel up to the mgr at the dss level.. kms is already handling this
for us.  In my latest version, which I haven't pushed, I removed the
'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.

>> The one area that kms mismatches a bit is decoupling of ovl from mgr
>> that we have in our hw..  I've partially solved that a while back w/
>
> What do you mean with that? Ovls and mgrs are one entity in KMS? Didn't
> the drm_plane stuff separate these?

yes and no.. it is in our omapdrm implementation, because each crtc
has it's own private plane assigned.  Basically the purpose is that we
can't break the interface to existing KMS userspace, which expects a
CRTC to include the dma scanout engine.  But it means at the moment we
can't re-use these planes from crtc's that are not in use.

I have some ideas about how to expose this to userspace in a backwards
compatible way, so a userspace that is aware of this can re-use planes
from crtcs that are not in use.  There is at least one other SoC
platform (STE, IIRC?) that has similar flexibility in hw, so I think
this is a worthwhile thing to do.. but just haven't gotten to it yet.

>> For enabling/disabling an output (manager+encoder), this is relatively
>> infrequent, so it can afford to block to avoid races.  (Like userspace
>> enabling and then rapidly disabling an output part way through the
>> enable.)  But enabling/disabling an overlay, or adjusting position or
>> scanout address must not block.  And ideally, if possible, switching
>> an overlay between two managers should not block.
>
> Adjusting the position or address of the buffer are simple, they can be
> easily done without any blocking.
>
> But ovl enable/disable and switching an ovl to another mgr do (possibly)
> take multiple vsyncs (and in the switch case, vsyncs of two separate
> outputs). So if those do not block, we'll need to handle them as a state
> machine and try to avoid races etc. It'll be "interesting".

ok, I see the problem.  Really the one thing I'm not handling properly
is disconnecting a plane from one crtc and connecting to another.  The
disconnect should synchronize on the outgoing crtc's vblank/GO, and
connect on the incoming crtc.  But this is not a frequent operation,
so I think the easy solution here is to block on the
connect-to-new-crtc if the disconnect is still pending.  I'd prefer
this to introducing intermediate states.

A simple enable/disable without changing crtc does not need to block.
If usespace disables and then re-enables before the vblank, we just
apply whatever is the most recent state at the vblank.  Meaning
enable->disable->enable, the middle disable might just get skipped.
This is fine, and actually desirable.

> However, we can sometimes do those operations immediately. So I think we
> should have these conditional fast-paths in the code, and do them in
> non-blocking manner when possible.
>
>> I'll look again, but as far as I remember that at least wasn't
>> addressing the performance issues from making overlay enable/disable
>
> Right, it wasn't addressing those issues. But your branch doesn't really
> address those issues either, as it doesn't handle the problems related
> to ovl enable/disable.
>
>> synchronous.  And fixing that would, I expect, trigger the same
>> problems that I already spent a few days debugging before switching
>> over to handle apply in omapdrm.
>
> I was under the impression that the apply mechanism, the caching and
> setting of the configs, was the major issue you had. But you're hinting
> that the actual problem is related to ovl enable/disable? I haven't
> tried fixing the ovl enable/disable, as I didn't know it's an issue for
> omapdrm. Or are they both as big issues?

I think the problem was there were some cases, like ovl updates before
setting the mgr, where the user_info_dirty flag would be cleared but
the registers not updated.  This is what I meant by sequence of
operations at KMS level confusing omapdss.  This should be fixable
with some debugging.  Although getting rid of the state tracking at
omapdss level altogether was a much simpler solution, and is the one I
prefer :-)

BR,
-R

>  Tomi
>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Tomi Valkeinen]

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

On Thu, 2012-08-02 at 07:45 -0500, Rob Clark wrote:
> On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
> >> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> >> > I guess the fact is that DRM concepts do not really match the OMAP DSS
> >> > hardware, and we'll have to use whatever gives us least problems.
> >>
> >> Actually, I think it does map fairly well to the hardware.. at least
> >> more so than to omapdss ;-)
> >
> > Hm, I'm not sure I understand, omapdss concepts map directly to the
> > hardware.
> 
> I think it is mainly exposing the encoder and panel as two separate
> entities.. which seems to be what Archit is working on

I still don't follow =) They are separate entities. Omapdss models the
HW quite directly, I think. It doesn't expose everything, though, as the
output drivers (dsi.c, dpi.c etc) are used via the panel drivers.

> in case of something like DVI bridge from DPI, this seems pretty
> straightforward.. only the connector needs to know about DDC stuff,
> which i2c to use and that sort of thing.  So at kms level we would
> have (for example) an omap_dpi_encoder which would be the same for DPI
> panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
> through the code to see how this would work.  Honestly I've looked
> less at this part of code and encoder related registers in the TRM,
> compared to the ovl/mgr parts, but at least from the 'DSS overview'
> picture in the TRM it seems to make sense ;-)
> 
> KMS even exposes the idea that certain crtcs can connect to only
> certain encoders.  Or that you could you could have certain connectors
> switched between encoders.  For example if you had a hw w/ DPI out,
> and some mux to switch that back and forth between a DPI lcd panel and
> a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
> this, but it is in theory possible.)  So we could expose possible
> video chain topologies to userspace in this way.

OMAP3 SDP board has such a setup, with manual switch to select between
LCD and DVI.

> The other thing is that we don't need to propagate timings from the
> panel up to the mgr at the dss level.. kms is already handling this
> for us.  In my latest version, which I haven't pushed, I removed the
> 'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.

You're thinking only about simple DPI cases. Consider this example, with
a DSI-to-DP bridge chip. What we have is the following flow of data:

DISPC -> DSI -> DSI-2-DP -> DP monitor

The timings you are thinking about are in the DISPC, but here they are
only one part of the link. And here the DISPC timings are not actually
the timings what the user is interested about. The user wants his
timings to be between DSI-2-DP chip and the DP monitor.

Timings programmed to DISPC are not the same. The timings for DISPC come
from the DSI driver, and they may be very different than the user's
timings. With DSI video mode, the DISPC timings would have some
resemblance to the user's timings, mainly the time to send one line
would be the same. With DSI cmd mode, the DISPC timings would be
something totally different, most likely with 0 blank times and as fast
pixel clock as possible.

What omapdss does currently is that you set the user's timings to the
right side of the chain, which propagate back to DSS. This allows the
DSI-2-DP bridge use DSI timings that work optimally for the bridge, and
DSI driver will use DISPC timings that work optimally for it.

And it's not only about timings above, but also other settings related
to the busses between the components. Clock dividers, polarities, stuff
like that.

> I think the problem was there were some cases, like ovl updates before
> setting the mgr, where the user_info_dirty flag would be cleared but
> the registers not updated.  This is what I meant by sequence of

Hmm, I'd appreciate more info about this if you can give. Sounds like a
bug, that shouldn't happen.

So you mean that you have an ovl, with no manager set. And you change
the settings of the ovl before assigning it to a mgr? That's something I
haven't really tested, so it could bug, true.

> operations at KMS level confusing omapdss.  This should be fixable
> with some debugging.  Although getting rid of the state tracking at
> omapdss level altogether was a much simpler solution, and is the one I
> prefer :-)

Yes, I don't prefer the state tracking either (we didn't have it in
earlier versions of omapdss), but I still don't see an option to it if
we want to support all the stuff we have.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Thu, Aug 2, 2012 at 8:15 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-08-02 at 07:45 -0500, Rob Clark wrote:
>> On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
>> >> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> >
>> >> > I guess the fact is that DRM concepts do not really match the OMAP DSS
>> >> > hardware, and we'll have to use whatever gives us least problems.
>> >>
>> >> Actually, I think it does map fairly well to the hardware.. at least
>> >> more so than to omapdss ;-)
>> >
>> > Hm, I'm not sure I understand, omapdss concepts map directly to the
>> > hardware.
>>
>> I think it is mainly exposing the encoder and panel as two separate
>> entities.. which seems to be what Archit is working on
>
> I still don't follow =) They are separate entities. Omapdss models the
> HW quite directly, I think. It doesn't expose everything, though, as the
> output drivers (dsi.c, dpi.c etc) are used via the panel drivers.

right.. so we just need to expose the output drivers as separate
entities, and let omapdrm propagate information such as timings
between them

>> in case of something like DVI bridge from DPI, this seems pretty
>> straightforward.. only the connector needs to know about DDC stuff,
>> which i2c to use and that sort of thing.  So at kms level we would
>> have (for example) an omap_dpi_encoder which would be the same for DPI
>> panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
>> through the code to see how this would work.  Honestly I've looked
>> less at this part of code and encoder related registers in the TRM,
>> compared to the ovl/mgr parts, but at least from the 'DSS overview'
>> picture in the TRM it seems to make sense ;-)
>>
>> KMS even exposes the idea that certain crtcs can connect to only
>> certain encoders.  Or that you could you could have certain connectors
>> switched between encoders.  For example if you had a hw w/ DPI out,
>> and some mux to switch that back and forth between a DPI lcd panel and
>> a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
>> this, but it is in theory possible.)  So we could expose possible
>> video chain topologies to userspace in this way.
>
> OMAP3 SDP board has such a setup, with manual switch to select between
> LCD and DVI.

ahh, good to know.. so I'm not crazy for wanting to expose this
possibility to userspace

>> The other thing is that we don't need to propagate timings from the
>> panel up to the mgr at the dss level.. kms is already handling this
>> for us.  In my latest version, which I haven't pushed, I removed the
>> 'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.
>
> You're thinking only about simple DPI cases. Consider this example, with
> a DSI-to-DP bridge chip. What we have is the following flow of data:
>
> DISPC -> DSI -> DSI-2-DP -> DP monitor
>
> The timings you are thinking about are in the DISPC, but here they are
> only one part of the link. And here the DISPC timings are not actually
> the timings what the user is interested about. The user wants his
> timings to be between DSI-2-DP chip and the DP monitor.
>
> Timings programmed to DISPC are not the same. The timings for DISPC come
> from the DSI driver, and they may be very different than the user's
> timings. With DSI video mode, the DISPC timings would have some
> resemblance to the user's timings, mainly the time to send one line
> would be the same. With DSI cmd mode, the DISPC timings would be
> something totally different, most likely with 0 blank times and as fast
> pixel clock as possible.

hmm, well kms already has a concept of adjusted_timings, which could
perhaps be used here to propagate the timings between crtc->encoder..
although the order is probably backwards from what we want (it comes
from the crtc to the encoder.. and if I understand properly we want it
the other way and actually possibly from the connector).  But that
isn't to say that internally in omapdrm the crtc couldn't get the
adjusted timings from the connector.  So I still think the parameter
flow doesn't need to be 'under the hood' in omapdss.

And fwiw, the adjusted_timings stuff is handled by drm_crtc_helper
fxns, so if the way the core kms handles it isn't what we want, we can
just plug in our own fxn instead of using drm_crtc_helper_set_mode(),
so that isn't really a big problem.

> What omapdss does currently is that you set the user's timings to the
> right side of the chain, which propagate back to DSS. This allows the
> DSI-2-DP bridge use DSI timings that work optimally for the bridge, and
> DSI driver will use DISPC timings that work optimally for it.
>
> And it's not only about timings above, but also other settings related
> to the busses between the components. Clock dividers, polarities, stuff
> like that.

I expect we could handle other settings in the same way as the timings.

>> I think the problem was there were some cases, like ovl updates before
>> setting the mgr, where the user_info_dirty flag would be cleared but
>> the registers not updated.  This is what I meant by sequence of
>
> Hmm, I'd appreciate more info about this if you can give. Sounds like a
> bug, that shouldn't happen.
>
> So you mean that you have an ovl, with no manager set. And you change
> the settings of the ovl before assigning it to a mgr? That's something I
> haven't really tested, so it could bug, true.
>
>> operations at KMS level confusing omapdss.  This should be fixable
>> with some debugging.  Although getting rid of the state tracking at
>> omapdss level altogether was a much simpler solution, and is the one I
>> prefer :-)
>
> Yes, I don't prefer the state tracking either (we didn't have it in
> earlier versions of omapdss), but I still don't see an option to it if
> we want to support all the stuff we have.

well, we do have to do state tracking *somewhere*.. I just prefer to
do it only in one layer instead of two ;-)

BR,
-R

>  Tomi
>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Semwal, Sumit]

Hi Rob, Tomi,
On Thu, Aug 2, 2012 at 7:46 PM, Rob Clark <rob.clark@linaro.org> wrote:
> On Thu, Aug 2, 2012 at 8:15 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Thu, 2012-08-02 at 07:45 -0500, Rob Clark wrote:
>>> On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> > On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
>>> >> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> >
>>> >> > I guess the fact is that DRM concepts do not really match the OMAP DSS
>>> >> > hardware, and we'll have to use whatever gives us least problems.
>>> >>
>>> >> Actually, I think it does map fairly well to the hardware.. at least
>>> >> more so than to omapdss ;-)
>>> >
>>> > Hm, I'm not sure I understand, omapdss concepts map directly to the
>>> > hardware.
>>>
>>> I think it is mainly exposing the encoder and panel as two separate
>>> entities.. which seems to be what Archit is working on
>>
>> I still don't follow =) They are separate entities. Omapdss models the
>> HW quite directly, I think. It doesn't expose everything, though, as the
>> output drivers (dsi.c, dpi.c etc) are used via the panel drivers.
>
> right.. so we just need to expose the output drivers as separate
> entities, and let omapdrm propagate information such as timings
> between them
>
>>> in case of something like DVI bridge from DPI, this seems pretty
>>> straightforward.. only the connector needs to know about DDC stuff,
>>> which i2c to use and that sort of thing.  So at kms level we would
>>> have (for example) an omap_dpi_encoder which would be the same for DPI
>>> panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
>>> through the code to see how this would work.  Honestly I've looked
>>> less at this part of code and encoder related registers in the TRM,
>>> compared to the ovl/mgr parts, but at least from the 'DSS overview'
>>> picture in the TRM it seems to make sense ;-)
>>>
>>> KMS even exposes the idea that certain crtcs can connect to only
>>> certain encoders.  Or that you could you could have certain connectors
>>> switched between encoders.  For example if you had a hw w/ DPI out,
>>> and some mux to switch that back and forth between a DPI lcd panel and
>>> a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
>>> this, but it is in theory possible.)  So we could expose possible
>>> video chain topologies to userspace in this way.
>>
>> OMAP3 SDP board has such a setup, with manual switch to select between
>> LCD and DVI.
>
> ahh, good to know.. so I'm not crazy for wanting to expose this
> possibility to userspace
>
>>> The other thing is that we don't need to propagate timings from the
>>> panel up to the mgr at the dss level.. kms is already handling this
>>> for us.  In my latest version, which I haven't pushed, I removed the
>>> 'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.
>>
>> You're thinking only about simple DPI cases. Consider this example, with
>> a DSI-to-DP bridge chip. What we have is the following flow of data:
>>
>> DISPC -> DSI -> DSI-2-DP -> DP monitor
>>
>> The timings you are thinking about are in the DISPC, but here they are
>> only one part of the link. And here the DISPC timings are not actually
>> the timings what the user is interested about. The user wants his
>> timings to be between DSI-2-DP chip and the DP monitor.
>>
>> Timings programmed to DISPC are not the same. The timings for DISPC come
>> from the DSI driver, and they may be very different than the user's
>> timings. With DSI video mode, the DISPC timings would have some
>> resemblance to the user's timings, mainly the time to send one line
>> would be the same. With DSI cmd mode, the DISPC timings would be
>> something totally different, most likely with 0 blank times and as fast
>> pixel clock as possible.
>
> hmm, well kms already has a concept of adjusted_timings, which could
> perhaps be used here to propagate the timings between crtc->encoder..
> although the order is probably backwards from what we want (it comes
> from the crtc to the encoder.. and if I understand properly we want it
> the other way and actually possibly from the connector).  But that
> isn't to say that internally in omapdrm the crtc couldn't get the
> adjusted timings from the connector.  So I still think the parameter
> flow doesn't need to be 'under the hood' in omapdss.
>
> And fwiw, the adjusted_timings stuff is handled by drm_crtc_helper
> fxns, so if the way the core kms handles it isn't what we want, we can
> just plug in our own fxn instead of using drm_crtc_helper_set_mode(),
> so that isn't really a big problem.
>
>> What omapdss does currently is that you set the user's timings to the
>> right side of the chain, which propagate back to DSS. This allows the
>> DSI-2-DP bridge use DSI timings that work optimally for the bridge, and
>> DSI driver will use DISPC timings that work optimally for it.
>>
>> And it's not only about timings above, but also other settings related
>> to the busses between the components. Clock dividers, polarities, stuff
>> like that.
>
> I expect we could handle other settings in the same way as the timings.
>
>>> I think the problem was there were some cases, like ovl updates before
>>> setting the mgr, where the user_info_dirty flag would be cleared but
>>> the registers not updated.  This is what I meant by sequence of
>>
>> Hmm, I'd appreciate more info about this if you can give. Sounds like a
>> bug, that shouldn't happen.
>>
>> So you mean that you have an ovl, with no manager set. And you change
>> the settings of the ovl before assigning it to a mgr? That's something I
>> haven't really tested, so it could bug, true.
>>
>>> operations at KMS level confusing omapdss.  This should be fixable
>>> with some debugging.  Although getting rid of the state tracking at
>>> omapdss level altogether was a much simpler solution, and is the one I
>>> prefer :-)
>>
>> Yes, I don't prefer the state tracking either (we didn't have it in
>> earlier versions of omapdss), but I still don't see an option to it if
>> we want to support all the stuff we have.
>
> well, we do have to do state tracking *somewhere*.. I just prefer to
> do it only in one layer instead of two ;-)
>
Let me add a little more to the 'boiling pot' ;) - what about
writeback? Writeback, from DSS perspective, is an integral part of the
DSS offering, is needed for many compelling use cases, and needs to
sync with the overlays and the managers (depending on mode) for state
information.

As such, Omapdss needs to worry about the way writeback is structured,
and the way it interacts with the other entities in the system.

Even though AFAIK DRM doesn't provide a capture mechanism to help
support writeback through omapdrm, we still need to provide a clean
way to use the writeback path with the display ones that we've talked
about here till now.

> BR,
> -R

Best,
~Sumit.
>>  Tomi
>>

Re: [RFC 0/3] solving omapdrm/omapdss layering issues

[Rob Clark]

On Fri, Aug 3, 2012 at 1:01 AM, Semwal, Sumit <sumit.semwal@ti.com> wrote:
> Hi Rob, Tomi,
> On Thu, Aug 2, 2012 at 7:46 PM, Rob Clark <rob.clark@linaro.org> wrote:
>> On Thu, Aug 2, 2012 at 8:15 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> On Thu, 2012-08-02 at 07:45 -0500, Rob Clark wrote:
>>>> On Thu, Aug 2, 2012 at 2:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>> > On Wed, 2012-08-01 at 09:25 -0500, Rob Clark wrote:
>>>> >> On Wed, Aug 1, 2012 at 4:21 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>> >
>>>> >> > I guess the fact is that DRM concepts do not really match the OMAP DSS
>>>> >> > hardware, and we'll have to use whatever gives us least problems.
>>>> >>
>>>> >> Actually, I think it does map fairly well to the hardware.. at least
>>>> >> more so than to omapdss ;-)
>>>> >
>>>> > Hm, I'm not sure I understand, omapdss concepts map directly to the
>>>> > hardware.
>>>>
>>>> I think it is mainly exposing the encoder and panel as two separate
>>>> entities.. which seems to be what Archit is working on
>>>
>>> I still don't follow =) They are separate entities. Omapdss models the
>>> HW quite directly, I think. It doesn't expose everything, though, as the
>>> output drivers (dsi.c, dpi.c etc) are used via the panel drivers.
>>
>> right.. so we just need to expose the output drivers as separate
>> entities, and let omapdrm propagate information such as timings
>> between them
>>
>>>> in case of something like DVI bridge from DPI, this seems pretty
>>>> straightforward.. only the connector needs to know about DDC stuff,
>>>> which i2c to use and that sort of thing.  So at kms level we would
>>>> have (for example) an omap_dpi_encoder which would be the same for DPI
>>>> panel (connector) or DPI->DVI bridge.  For HDMI I'm still looking
>>>> through the code to see how this would work.  Honestly I've looked
>>>> less at this part of code and encoder related registers in the TRM,
>>>> compared to the ovl/mgr parts, but at least from the 'DSS overview'
>>>> picture in the TRM it seems to make sense ;-)
>>>>
>>>> KMS even exposes the idea that certain crtcs can connect to only
>>>> certain encoders.  Or that you could you could have certain connectors
>>>> switched between encoders.  For example if you had a hw w/ DPI out,
>>>> and some mux to switch that back and forth between a DPI lcd panel and
>>>> a DPI->DVI bridge.  (Ok, I'm not aware of any board that actually does
>>>> this, but it is in theory possible.)  So we could expose possible
>>>> video chain topologies to userspace in this way.
>>>
>>> OMAP3 SDP board has such a setup, with manual switch to select between
>>> LCD and DVI.
>>
>> ahh, good to know.. so I'm not crazy for wanting to expose this
>> possibility to userspace
>>
>>>> The other thing is that we don't need to propagate timings from the
>>>> panel up to the mgr at the dss level.. kms is already handling this
>>>> for us.  In my latest version, which I haven't pushed, I removed the
>>>> 'struct omap_overlay_mgr' ptr from 'struct omap_dss_device'.
>>>
>>> You're thinking only about simple DPI cases. Consider this example, with
>>> a DSI-to-DP bridge chip. What we have is the following flow of data:
>>>
>>> DISPC -> DSI -> DSI-2-DP -> DP monitor
>>>
>>> The timings you are thinking about are in the DISPC, but here they are
>>> only one part of the link. And here the DISPC timings are not actually
>>> the timings what the user is interested about. The user wants his
>>> timings to be between DSI-2-DP chip and the DP monitor.
>>>
>>> Timings programmed to DISPC are not the same. The timings for DISPC come
>>> from the DSI driver, and they may be very different than the user's
>>> timings. With DSI video mode, the DISPC timings would have some
>>> resemblance to the user's timings, mainly the time to send one line
>>> would be the same. With DSI cmd mode, the DISPC timings would be
>>> something totally different, most likely with 0 blank times and as fast
>>> pixel clock as possible.
>>
>> hmm, well kms already has a concept of adjusted_timings, which could
>> perhaps be used here to propagate the timings between crtc->encoder..
>> although the order is probably backwards from what we want (it comes
>> from the crtc to the encoder.. and if I understand properly we want it
>> the other way and actually possibly from the connector).  But that
>> isn't to say that internally in omapdrm the crtc couldn't get the
>> adjusted timings from the connector.  So I still think the parameter
>> flow doesn't need to be 'under the hood' in omapdss.
>>
>> And fwiw, the adjusted_timings stuff is handled by drm_crtc_helper
>> fxns, so if the way the core kms handles it isn't what we want, we can
>> just plug in our own fxn instead of using drm_crtc_helper_set_mode(),
>> so that isn't really a big problem.
>>
>>> What omapdss does currently is that you set the user's timings to the
>>> right side of the chain, which propagate back to DSS. This allows the
>>> DSI-2-DP bridge use DSI timings that work optimally for the bridge, and
>>> DSI driver will use DISPC timings that work optimally for it.
>>>
>>> And it's not only about timings above, but also other settings related
>>> to the busses between the components. Clock dividers, polarities, stuff
>>> like that.
>>
>> I expect we could handle other settings in the same way as the timings.
>>
>>>> I think the problem was there were some cases, like ovl updates before
>>>> setting the mgr, where the user_info_dirty flag would be cleared but
>>>> the registers not updated.  This is what I meant by sequence of
>>>
>>> Hmm, I'd appreciate more info about this if you can give. Sounds like a
>>> bug, that shouldn't happen.
>>>
>>> So you mean that you have an ovl, with no manager set. And you change
>>> the settings of the ovl before assigning it to a mgr? That's something I
>>> haven't really tested, so it could bug, true.
>>>
>>>> operations at KMS level confusing omapdss.  This should be fixable
>>>> with some debugging.  Although getting rid of the state tracking at
>>>> omapdss level altogether was a much simpler solution, and is the one I
>>>> prefer :-)
>>>
>>> Yes, I don't prefer the state tracking either (we didn't have it in
>>> earlier versions of omapdss), but I still don't see an option to it if
>>> we want to support all the stuff we have.
>>
>> well, we do have to do state tracking *somewhere*.. I just prefer to
>> do it only in one layer instead of two ;-)
>>
> Let me add a little more to the 'boiling pot' ;) - what about
> writeback? Writeback, from DSS perspective, is an integral part of the
> DSS offering, is needed for many compelling use cases, and needs to
> sync with the overlays and the managers (depending on mode) for state
> information.
>
> As such, Omapdss needs to worry about the way writeback is structured,
> and the way it interacts with the other entities in the system.
>
> Even though AFAIK DRM doesn't provide a capture mechanism to help
> support writeback through omapdrm, we still need to provide a clean
> way to use the writeback path with the display ones that we've talked
> about here till now.

well, first: all the fancy features in the world don't matter if
fundamental use cases like multi-layer updates don't work.

but that said, writeback doesn't strictly change anything.
Configuring the input side of writeback (ie input to dss) *must* be
done through drm in order to not bypass the authentication mechanism
in drm.  We can't have non-authenticated clients configuring things so
that the contents of the screen get written back to memory, that is a
security hole.  After that, you maybe just need to provide some fxns
for the (presumably v4l capture) interface can configure the output
side of WB.  I guess this is roughly analogous to how hdmi audio
works.  It could be that we register a v4l2 driver within omapdrm, or
that we export some fxns that a v4l2 driver outside of omapdrm can
use.  Probably we should look at how hdmi audio and alsa hook together
in other drivers to see what the right precedent is to follow.

BR,
-R

>> BR,
>> -R
>
> Best,
> ~Sumit.
>>>  Tomi
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html