Re: [PATCH 01/33] drm/omap: HDMI: change enable/disable to avoid sync-losts

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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;

-- 
Regards,

Laurent Pinchart

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