| Message ID | 1455875288-4370-2-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, Thank you for the patch. On Friday 19 February 2016 11:47:36 Tomi Valkeinen wrote: > We occasionally see DISPC sync-lost errors when enabling and disabling > HDMI. Sometimes we get only a few, which get handled (ignored) by the > driver, but sometimes there's a flood of the errors which doesn't seem > to stop. > > The HW team has root caused this to the order in which HDMI and DISPC > are enabled/disabled. Currently we enable HDMI first, and then DISPC, > and vice versa when disabling. HW team's suggestion is to do it the > other way around. > > This patch changes the order, but this has two side effects as the pixel > clock is produced by HDMI, and the clock is not running when we > enable/disable DISPC: > > * When enabling DISPC first, we don't get vertical sync events > * When disabling DISPC last, we don't get FRAMEDONE event > > At the moment we use both of those to verify that DISPC has been > enabled/disabled properly. Thus this patch also needs to change the > omapdrm and omapdss which handle the DISPC side. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/dss/hdmi4.c | 16 ++++++++-------- > drivers/gpu/drm/omapdrm/dss/hdmi5.c | 16 ++++++++-------- > drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++ > 3 files changed, 21 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c > b/drivers/gpu/drm/omapdrm/dss/hdmi4.c index 7103c659a534..b09ce9ee82fa > 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c > @@ -214,22 +214,22 @@ static int hdmi_power_on_full(struct omap_dss_device > *dssdev) /* tv size */ > dss_mgr_set_timings(mgr, p); > > - r = hdmi_wp_video_start(&hdmi.wp); > - if (r) > - goto err_vid_enable; > - > r = dss_mgr_enable(mgr); > if (r) > goto err_mgr_enable; > > + r = hdmi_wp_video_start(&hdmi.wp); > + if (r) > + goto err_vid_enable; > + > hdmi_wp_set_irqenable(wp, > HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT); > > return 0; > > -err_mgr_enable: > - hdmi_wp_video_stop(&hdmi.wp); > err_vid_enable: > + dss_mgr_disable(mgr); > +err_mgr_enable: > hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); > err_phy_pwr: > err_phy_cfg: > @@ -246,10 +246,10 @@ static void hdmi_power_off_full(struct omap_dss_device > *dssdev) > > hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff); > > - dss_mgr_disable(mgr); > - > hdmi_wp_video_stop(&hdmi.wp); > > + dss_mgr_disable(mgr); > + > hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); > > dss_pll_disable(&hdmi.pll.pll); > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c > b/drivers/gpu/drm/omapdrm/dss/hdmi5.c index a955a2c4c061..4485a1c37bd8 > 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c > @@ -231,22 +231,22 @@ static int hdmi_power_on_full(struct omap_dss_device > *dssdev) /* tv size */ > dss_mgr_set_timings(mgr, p); > > - r = hdmi_wp_video_start(&hdmi.wp); > - if (r) > - goto err_vid_enable; > - > r = dss_mgr_enable(mgr); > if (r) > goto err_mgr_enable; > > + r = hdmi_wp_video_start(&hdmi.wp); > + if (r) > + goto err_vid_enable; > + > hdmi_wp_set_irqenable(&hdmi.wp, > HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT); > > return 0; > > -err_mgr_enable: > - hdmi_wp_video_stop(&hdmi.wp); > err_vid_enable: > + dss_mgr_disable(mgr); > +err_mgr_enable: > hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); > err_phy_pwr: > err_phy_cfg: > @@ -263,10 +263,10 @@ static void hdmi_power_off_full(struct omap_dss_device > *dssdev) > > hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff); > > - dss_mgr_disable(mgr); > - > hdmi_wp_video_stop(&hdmi.wp); > > + dss_mgr_disable(mgr); > + > hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); > > dss_pll_disable(&hdmi.pll.pll); > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2ed0754ed19e..7dd3d44a93e5 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -138,6 +138,11 @@ static void omap_crtc_set_enabled(struct drm_crtc > *crtc, bool enable) u32 framedone_irq, vsync_irq; > int ret; > > + if (omap_crtc->mgr->output->output_type == OMAP_DISPLAY_TYPE_HDMI) { > + dispc_mgr_enable(channel, enable); > + return; > + } This effectively bypasses the wait until the DISPC outputs the first vsync interrupt below. How does HDMI differ from other outputs in such a way to make the wait unneeded ? > if (dispc_mgr_is_enabled(channel) == enable) > return;
On 23/02/16 12:22, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Friday 19 February 2016 11:47:36 Tomi Valkeinen wrote: >> We occasionally see DISPC sync-lost errors when enabling and disabling >> HDMI. Sometimes we get only a few, which get handled (ignored) by the >> driver, but sometimes there's a flood of the errors which doesn't seem >> to stop. >> >> The HW team has root caused this to the order in which HDMI and DISPC >> are enabled/disabled. Currently we enable HDMI first, and then DISPC, >> and vice versa when disabling. HW team's suggestion is to do it the >> other way around. >> >> This patch changes the order, but this has two side effects as the pixel >> clock is produced by HDMI, and the clock is not running when we >> enable/disable DISPC: >> >> * When enabling DISPC first, we don't get vertical sync events >> * When disabling DISPC last, we don't get FRAMEDONE event >> >> At the moment we use both of those to verify that DISPC has been >> enabled/disabled properly. Thus this patch also needs to change the >> omapdrm and omapdss which handle the DISPC side. <snip> >> + if (omap_crtc->mgr->output->output_type == OMAP_DISPLAY_TYPE_HDMI) { >> + dispc_mgr_enable(channel, enable); >> + return; >> + } > > This effectively bypasses the wait until the DISPC outputs the first vsync > interrupt below. How does HDMI differ from other outputs in such a way to make > the wait unneeded ? There's to parts here. Enabling the output and disabling the output. Enable: We don't strictly need the wait after enable for any output. The output works after setting the enable bit. There are two reasons for the waiting: 1) A sanity check that the configuration is ok. If the config is broken (which shouldn't happen, of course, as the driver should verify the config), we won't see vsync. At the moment we only print an error in that case. 2) OMAP_DSS_CHANNEL_DIGIT is a bit problematic. That channel is used for analog tv-out (VENC) in older DSS versions, and for HDMI for more recent ones. With VENC we always get a few sync lost errors when enabling the output, so with the wait we can ignore those errors (this sync-lost ignoring is only done for OMAP_DSS_CHANNEL_DIGIT). We have seen similar sync losts with HDMI too, but apparently it is possible to support HDMI without any sync losts. That's what this patch is doing. With this patch we lose both 1) and 2) above, but 1) is not strictly needed and 2) shouldn't happen for HDMI after this patch. We could implement 1) in the HDMI driver too, using the HDMI IP's VSYNC interrupt, but I don't think it's really necessary. Disable: When disabling the output, we do want to wait until the DSS has finished the work at the end of the frame. This is done in the omap_crtc_set_enabled() function for all outputs, using FRAMEDONE interrupt when available, or VSYNC if not. For HDMI we can do it also in the HDMI driver. The HDMI IP has its own FRAMEDONE interrupt, which we wait for in hdmi_wp_video_stop(). Tomi
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c index 7103c659a534..b09ce9ee82fa 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c @@ -214,22 +214,22 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev) /* tv size */ dss_mgr_set_timings(mgr, p); - r = hdmi_wp_video_start(&hdmi.wp); - if (r) - goto err_vid_enable; - r = dss_mgr_enable(mgr); if (r) goto err_mgr_enable; + r = hdmi_wp_video_start(&hdmi.wp); + if (r) + goto err_vid_enable; + hdmi_wp_set_irqenable(wp, HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT); return 0; -err_mgr_enable: - hdmi_wp_video_stop(&hdmi.wp); err_vid_enable: + dss_mgr_disable(mgr); +err_mgr_enable: hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); err_phy_pwr: err_phy_cfg: @@ -246,10 +246,10 @@ static void hdmi_power_off_full(struct omap_dss_device *dssdev) hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff); - dss_mgr_disable(mgr); - hdmi_wp_video_stop(&hdmi.wp); + dss_mgr_disable(mgr); + hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); dss_pll_disable(&hdmi.pll.pll); diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c index a955a2c4c061..4485a1c37bd8 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c @@ -231,22 +231,22 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev) /* tv size */ dss_mgr_set_timings(mgr, p); - r = hdmi_wp_video_start(&hdmi.wp); - if (r) - goto err_vid_enable; - r = dss_mgr_enable(mgr); if (r) goto err_mgr_enable; + r = hdmi_wp_video_start(&hdmi.wp); + if (r) + goto err_vid_enable; + hdmi_wp_set_irqenable(&hdmi.wp, HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT); return 0; -err_mgr_enable: - hdmi_wp_video_stop(&hdmi.wp); err_vid_enable: + dss_mgr_disable(mgr); +err_mgr_enable: hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); err_phy_pwr: err_phy_cfg: @@ -263,10 +263,10 @@ static void hdmi_power_off_full(struct omap_dss_device *dssdev) hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff); - dss_mgr_disable(mgr); - hdmi_wp_video_stop(&hdmi.wp); + dss_mgr_disable(mgr); + hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF); dss_pll_disable(&hdmi.pll.pll); diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2ed0754ed19e..7dd3d44a93e5 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -138,6 +138,11 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable) u32 framedone_irq, vsync_irq; int ret; + if (omap_crtc->mgr->output->output_type == OMAP_DISPLAY_TYPE_HDMI) { + dispc_mgr_enable(channel, enable); + return; + } + if (dispc_mgr_is_enabled(channel) == enable) return;
We occasionally see DISPC sync-lost errors when enabling and disabling HDMI. Sometimes we get only a few, which get handled (ignored) by the driver, but sometimes there's a flood of the errors which doesn't seem to stop. The HW team has root caused this to the order in which HDMI and DISPC are enabled/disabled. Currently we enable HDMI first, and then DISPC, and vice versa when disabling. HW team's suggestion is to do it the other way around. This patch changes the order, but this has two side effects as the pixel clock is produced by HDMI, and the clock is not running when we enable/disable DISPC: * When enabling DISPC first, we don't get vertical sync events * When disabling DISPC last, we don't get FRAMEDONE event At the moment we use both of those to verify that DISPC has been enabled/disabled properly. Thus this patch also needs to change the omapdrm and omapdss which handle the DISPC side. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/dss/hdmi4.c | 16 ++++++++-------- drivers/gpu/drm/omapdrm/dss/hdmi5.c | 16 ++++++++-------- drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++ 3 files changed, 21 insertions(+), 16 deletions(-)