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
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); } }
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
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:
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Archit Taneja -
Rob Clark -
Semwal, Sumit -
Tomi Valkeinen