Re: [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 28, 2012 at 04:43:01PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This function is supposed to be used at mode set time, so prevent
> against future mistakes by adding a WARN().
> 
> Problems when calling it when the HDMI port is enabled:
>   - It may disable sending DIPs (on IVB and older), and the register
>     specification says we shouldn't disable DIPs with the HDMI port
>     enabled.
>   - It does not wait for the VSync because the pipe is disabled at
>     mode set.
> 
> The second condition can be fixed with some additional code, but the
> first one is not so easy... Ideally, code changing the infoframes
> outside of the mode set should wait for the VSync and then directly
> call the write_infoframe functions.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Can't we extract this into a little assert_hdmi_port_disabled(dev_priv,
reg) like we have with e.g. assert_pch_hdmi_disabled? Difference would be
that we don't check for the pipe here ... Imo that would make the code
slight less verbose and the intention behind the check a bit clearer.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index f61af21..6fda359 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -334,6 +334,9 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
>  	u32 val = I915_READ(reg);
>  	u32 port;
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* If the registers were not initialized yet, they might be zeroes,
>  	 * which means we're selecting the AVI DIP and we're setting its
>  	 * frequency to once. This seems to really confuse the HW and make
> @@ -395,6 +398,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
>  	u32 val = I915_READ(reg);
>  	u32 port;
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* See the big comment in g4x_set_infoframes() */
>  	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>  
> @@ -451,6 +457,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
>  	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* See the big comment in g4x_set_infoframes() */
>  	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>  
> @@ -484,6 +493,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>  	u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* See the big comment in g4x_set_infoframes() */
>  	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>  
> @@ -516,6 +528,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
>  	u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & DDI_BUF_CTL_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	if (!intel_hdmi->has_hdmi_sink) {
>  		I915_WRITE(reg, 0);
>  		POSTING_READ(reg);
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48