[Intel-gfx] [PATCH v2 5/9] drm/i915: Fix 12bpc HDMI enable for IBX

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> ville.syrjala at linux.intel.com
> Sent: Tuesday, May 05, 2015 7:06 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 5/9] drm/i915: Fix 12bpc HDMI enable for IBX
> 
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Follow the procedure listed in Bspec to toggle the port enable bit off
> and on when enabling HDMI with 12bpc and pixel repeat on IBX. The old
> code didn't actually enable the port before "toggling" the bit back off,
> so the whole workaround was essentially a nop.
> 
> Also take the opportunity to clarify the code by splitting the gmch
> platforms to a separate (much more straightforward) function.

Per prior reviews seen before, it would be better if there are
separate patches for fixing issue and refactoring code.

I think it is fine either way, but is bit easy for reviewing code
having separate patches.

With that,
Reviewed-by: Chandra Konduru <Chandra.konduru at intel.com>

> 
> v2: Rebased due to crtc->config changes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 78 ++++++++++++++++++++++++++---------
> ----
>  1 file changed, 53 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 2e98e33..766bdb1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -944,47 +944,73 @@ static void intel_enable_hdmi_audio(struct
> intel_encoder *encoder)
>  	intel_audio_codec_enable(encoder);
>  }
> 
> -static void intel_enable_hdmi(struct intel_encoder *encoder)
> +static void g4x_enable_hdmi(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);

Unrelated change

>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	u32 temp;
> -	u32 enable_bits = SDVO_ENABLE;
> -
> -	if (intel_crtc->config->has_audio)
> -		enable_bits |= SDVO_AUDIO_ENABLE;
> 
>  	temp = I915_READ(intel_hdmi->hdmi_reg);
> 
> -	/* HW workaround for IBX, we need to move the port to transcoder A
> -	 * before disabling it, so restore the transcoder select bit here. */
> -	if (HAS_PCH_IBX(dev))
> -		enable_bits |= SDVO_PIPE_SEL(intel_crtc->pipe);
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> 
> -	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
> -	 * we do this anyway which shows more stable in testing.
> -	 */
> -	if (HAS_PCH_SPLIT(dev)) {
> -		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
> -		POSTING_READ(intel_hdmi->hdmi_reg);
> -	}
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
> 
> -	temp |= enable_bits;
> +	if (crtc->config->has_audio)
> +		intel_enable_hdmi_audio(encoder);
> +}
> 
> +static void ibx_enable_hdmi(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	u32 temp;
> +
> +	temp = I915_READ(intel_hdmi->hdmi_reg);
> +
> +	temp |= SDVO_ENABLE;
> +	if (crtc->config->has_audio)
> +		temp |= SDVO_AUDIO_ENABLE;
> +
> +	/*
> +	 * HW workaround, need to write this twice for issue
> +	 * that may result in first write getting masked.
> +	 */
> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +	POSTING_READ(intel_hdmi->hdmi_reg);
>  	I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  	POSTING_READ(intel_hdmi->hdmi_reg);
> 
> -	/* HW workaround, need to write this twice for issue that may result
> -	 * in first write getting masked.
> +	/*
> +	 * HW workaround, need to toggle enable bit off and on
> +	 * for 12bpc with pixel repeat.
> +	 *
> +	 * FIXME: BSpec says this should be done at the end of
> +	 * of the modeset sequence, so not sure if this isn't too soon.
>  	 */
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (crtc->config->pipe_bpp > 24 &&
> +	    crtc->config->pixel_multiplier > 1) {
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		/*
> +		 * HW workaround, need to write this twice for issue
> +		 * that may result in first write getting masked.
> +		 */
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
>  		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  		POSTING_READ(intel_hdmi->hdmi_reg);
>  	}
> 
> -	if (intel_crtc->config->has_audio)
> +	if (crtc->config->has_audio)
>  		intel_enable_hdmi_audio(encoder);
>  }
> 
> @@ -1509,7 +1535,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder
> *encoder)
>  				   intel_crtc->config->has_hdmi_sink,
>  				   adjusted_mode);
> 
> -	intel_enable_hdmi(encoder);
> +	g4x_enable_hdmi(encoder);
> 
>  	vlv_wait_port_ready(dev_priv, dport, 0x0);
>  }
> @@ -1828,7 +1854,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder
> *encoder)
> 
>  	chv_powergate_phy_lanes(encoder, 0x0);
> 
> -	intel_enable_hdmi(encoder);
> +	g4x_enable_hdmi(encoder);
> 
>  	vlv_wait_port_ready(dev_priv, dport, 0x0);
>  }
> @@ -2012,8 +2038,10 @@ void intel_hdmi_init(struct drm_device *dev, int
> hdmi_reg, enum port port)
>  		intel_encoder->pre_enable = intel_hdmi_pre_enable;
>  		if (HAS_PCH_CPT(dev))
>  			intel_encoder->enable = cpt_enable_hdmi;
> +		else if (HAS_PCH_IBX(dev))
> +			intel_encoder->enable = ibx_enable_hdmi;
>  		else
> -			intel_encoder->enable = intel_enable_hdmi;
> +			intel_encoder->enable = g4x_enable_hdmi;
>  	}
> 
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;
> --
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx