Re: [PATCH 08/12] drm/i915: Fix the IBX transcoder B workarounds

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On 05/05/2015 07:17 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently the IBX transcoder B workarounds are not working correctly.
> Well, the HDMI one seems to be working somewhat, but the DP one is
> definitely busted.
> 
> After a bit of experimentation it looks like the best way to make this
> work is first disable the port on transcoder B, and then re-enable it
> transcoder A, and immediately disable it again.
> 
> We can also clean up the code by noting that we can't be called without
> a valid crtc. And also note that port A on ILK does not need the
> workaround, so let's check for that one too.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   | 37 ++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_hdmi.c | 50 ++++++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_sdvo.c | 41 +++++++++++++-------------------
>  3 files changed, 60 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 17b006c..3401cde 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3821,6 +3821,7 @@ static void
>  intel_dp_link_down(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>  	enum port port = intel_dig_port->port;
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3837,34 +3838,38 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	if ((IS_GEN7(dev) && port == PORT_A) ||
>  	    (HAS_PCH_CPT(dev) && port != PORT_A)) {
>  		DP &= ~DP_LINK_TRAIN_MASK_CPT;
> -		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
> +		DP |= DP_LINK_TRAIN_PAT_IDLE_CPT;
>  	} else {
>  		if (IS_CHERRYVIEW(dev))
>  			DP &= ~DP_LINK_TRAIN_MASK_CHV;
>  		else
>  			DP &= ~DP_LINK_TRAIN_MASK;
> -		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE);
> +		DP |= DP_LINK_TRAIN_PAT_IDLE;
>  	}
> +	I915_WRITE(intel_dp->output_reg, DP);
>  	POSTING_READ(intel_dp->output_reg);
>  
> -	if (HAS_PCH_IBX(dev) &&
> -	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
> -		/* Hardware workaround: leaving our transcoder select
> -		 * set to transcoder B while it's off will prevent the
> -		 * corresponding HDMI output on transcoder A.
> -		 *
> -		 * Combine this with another hardware workaround:
> -		 * transcoder select bit can only be cleared while the
> -		 * port is enabled.
> -		 */
> -		DP &= ~DP_PIPEB_SELECT;
> +	DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> +	I915_WRITE(intel_dp->output_reg, DP);
> +	POSTING_READ(intel_dp->output_reg);
> +
> +	/*
> +	 * HW workaround for IBX, we need to move the port
> +	 * to transcoder A after disabling it to allow the
> +	 * matching HDMI port to be enabled on transcoder A.
> +	 */
> +	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
> +		/* always enable with pattern 1 (as per spec) */
> +		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
> +		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
> +		I915_WRITE(intel_dp->output_reg, DP);
> +		POSTING_READ(intel_dp->output_reg);
> +
> +		DP &= ~DP_PORT_EN;
>  		I915_WRITE(intel_dp->output_reg, DP);
>  		POSTING_READ(intel_dp->output_reg);
>  	}
>  
> -	DP &= ~DP_AUDIO_OUTPUT_ENABLE;
> -	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
> -	POSTING_READ(intel_dp->output_reg);
>  	msleep(intel_dp->panel_power_down_delay);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 308015e..9b9a69e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1096,39 +1096,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	u32 temp;
> -	u32 enable_bits = SDVO_ENABLE | SDVO_AUDIO_ENABLE;
>  
>  	if (crtc->config->has_audio)
>  		intel_audio_codec_disable(encoder);
>  
>  	temp = I915_READ(intel_hdmi->hdmi_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->base.crtc;
> -		int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
> -
> -		if (temp & SDVO_PIPE_B_SELECT) {
> -			temp &= ~SDVO_PIPE_B_SELECT;
> -			I915_WRITE(intel_hdmi->hdmi_reg, temp);
> -			POSTING_READ(intel_hdmi->hdmi_reg);
> -
> -			/* Again we need to write this twice. */
> -			I915_WRITE(intel_hdmi->hdmi_reg, temp);
> -			POSTING_READ(intel_hdmi->hdmi_reg);
> -
> -			/* Transcoder selection bits only update
> -			 * effectively on vblank. */
> -			if (crtc)
> -				intel_wait_for_vblank(dev, pipe);
> -			else
> -				msleep(50);
> -		}
> -	}
> -
> -	temp &= ~enable_bits;
> -
> +	temp &= ~(SDVO_ENABLE | SDVO_AUDIO_ENABLE);
>  	I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  	POSTING_READ(intel_hdmi->hdmi_reg);
>  
> @@ -1136,6 +1110,28 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  		chv_powergate_phy_lanes(encoder, 0xf);
>  
>  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> +
> +	/*
> +	 * HW workaround for IBX, we need to move the port
> +	 * to transcoder A after disabling it to allow the
> +	 * matching DP port to be enabled on transcoder A.
> +	 */
> +	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
> +		temp &= ~SDVO_PIPE_B_SELECT;
> +		temp |= SDVO_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);
> +
> +		temp &= ~SDVO_ENABLE;
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +	}
>  }
>  
>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index e3e9c98..4a87691 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1437,6 +1437,7 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	u32 temp;
>  
>  	intel_sdvo_set_active_outputs(intel_sdvo, 0);
> @@ -1445,32 +1446,22 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>  						   DRM_MODE_DPMS_OFF);
>  
>  	temp = I915_READ(intel_sdvo->sdvo_reg);
> -	if ((temp & SDVO_ENABLE) != 0) {
> -		/* HW workaround for IBX, we need to move the port to
> -		 * transcoder A before disabling it. */
> -		if (HAS_PCH_IBX(encoder->base.dev)) {
> -			struct drm_crtc *crtc = encoder->base.crtc;
> -			int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
> -
> -			if (temp & SDVO_PIPE_B_SELECT) {
> -				temp &= ~SDVO_PIPE_B_SELECT;
> -				I915_WRITE(intel_sdvo->sdvo_reg, temp);
> -				POSTING_READ(intel_sdvo->sdvo_reg);
> -
> -				/* Again we need to write this twice. */
> -				I915_WRITE(intel_sdvo->sdvo_reg, temp);
> -				POSTING_READ(intel_sdvo->sdvo_reg);
> -
> -				/* Transcoder selection bits only update
> -				 * effectively on vblank. */
> -				if (crtc)
> -					intel_wait_for_vblank(encoder->base.dev, pipe);
> -				else
> -					msleep(50);
> -			}
> -		}
>  
> -		intel_sdvo_write_sdvox(intel_sdvo, temp & ~SDVO_ENABLE);
> +	temp &= ~SDVO_ENABLE;
> +	intel_sdvo_write_sdvox(intel_sdvo, temp);
> +
> +	/*
> +	 * HW workaround for IBX, we need to move the port
> +	 * to transcoder A after disabling it to allow the
> +	 * matching DP port to be enabled on transcoder A.
> +	 */
> +	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
> +		temp &= ~SDVO_PIPE_B_SELECT;
> +		temp |= SDVO_ENABLE;
> +		intel_sdvo_write_sdvox(intel_sdvo, temp);
> +
> +		temp &= ~SDVO_ENABLE;
> +		intel_sdvo_write_sdvox(intel_sdvo, temp);
>  	}
>  }
>  
> 

Cool, again, testing wins.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx