Re: [PATCH 2/3] drm/i915: implement IBX hdmi transcoder select workaround

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 30 May 2012 12:31:57 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Bspec Vol 3, Part 3, Section 3.8.1.1, bit 30:
> 
> "[DevIBX] Writing to this bit only takes effect when port is enabled.
> Due to hardware issue it is required that this bit be cleared when port
> is disabled. To clear this bit software must temporarily enable this
> port on transcoder A."
> 
> Unfortunately the public Bspec misses totally out on the same language
> for HDMIB. Internal Bspec also mentions that one of the bad
> side-effects is that DPx can fail to light up on transcoder A if HDMIx
> is disabled but using transcoder B.
> 
> I've found this while reviewing Bsepc. We already implement the same
> workaround for the DP ports.
> 
> Also replace a magic 1 with PIPE_B I've found while looking through the
> code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   29 ++++++++++++++++++++++++++++-
>  1 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 4c6f141..4ebdcf1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -382,7 +382,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
>  
>  	if (HAS_PCH_CPT(dev))
>  		sdvox |= PORT_TRANS_SEL_CPT(intel_crtc->pipe);
> -	else if (intel_crtc->pipe == 1)
> +	else if (intel_crtc->pipe == PIPE_B)
>  		sdvox |= SDVO_PIPE_B_SELECT;
>  
>  	I915_WRITE(intel_hdmi->sdvox_reg, sdvox);
> @@ -405,6 +405,33 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
>  
>  	temp = I915_READ(intel_hdmi->sdvox_reg);
>  
> +	/* HW workaround for IBX, we need to move the port to transcoder A
> +	 * before disabling it. */
> +	if (HAS_PCH_IBX(dev)) {
> +		struct drm_crtc *crtc = encoder->crtc;
> +		struct intel_crtc *intel_crtc;
> +
> +		if (crtc)
> +			intel_crtc = to_intel_crtc(crtc);
> +		else
> +			intel_crtc = NULL;
If you extracted pipe = crtc ? to_intel_crtc(crtc)->pipe : -1; then that
would make the following code less ugly.
> +
> +		if (mode != DRM_MODE_DPMS_ON && (temp & SDVO_PIPE_B_SELECT)) {

Split this into a nested if:

 if (mode != DPMS_MODE_DPMS_ON) {
   if (temp & SDVO_PIPE_B_SELECT) {
    // w/a
   } 
 } else {
   // restore
 }

> +			temp &= ~SDVO_PIPE_B_SELECT;
> +			I915_WRITE(intel_hdmi->sdvox_reg, temp);
> +			POSTING_READ(intel_hdmi->sdvox_reg);

Should we not worry about the HW w/a requiring this to be written twice?

static void intel_hdmi_write_sdvox(intel_hdmi, u32 val)
 {
   struct drm_i915_private *dev_priv = intel_hdmi->base.dev->dev_private;

   I915_WRITE(intel_hdmi->sdvox_reg, val);
   POSTING_READ(intel_hdmi->sdvo_reg);

   /* HW workaround, need to write this twice for issues that may result
    * in first write getting masked.
    */
   if (dev_priv->info.has_pch_split) {
     I915_WRITE(intel_hdmi->sdvox_reg, val);
     POSTING_READ(intel_hdmi->sdvox_reg);
   }
 }
> +
> +			if (intel_crtc)
> +				intel_wait_for_vblank(dev, intel_crtc->pipe);
> +			else
> +				msleep(50);
> +		} else if (mode == DRM_MODE_DPMS_ON){
> +			/* Restore the transcoder select bit. */
> +			if (intel_crtc->pipe == PIPE_B)
> +				enable_bits |= SDVO_PIPE_B_SELECT;
> +		}
> +	}
> +
>  	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
>  	 * we do this anyway which shows more stable in testing.
>  	 */

-- 
Chris Wilson, Intel Open Source Technology Centre