Re: [PATCH] drm/i915: Fix the PPT fdi lane bifurcate state handling on ivb

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 29, 2013 at 12:04:08PM +0100, Daniel Vetter wrote:
> Originally I've thought that this is leftover hw state dirt from the
> BIOS. But after way too much helpless flailing around on my part I've
> noticed that the actual bug is when we change the state of an already
> active pipe.
> 
> For example when we change the fdi lines from 2 to 3 without switching
> off outputs in-between we'll never see the crucial on->off transition
> in the ->modeset_global_resources hook the current logic relies on.
> 
> Patch version 2 got this right by instead also checking whether the
> pipe is indeed active. But that in turn broke things when pipes have
> been turned off through dpms since the bifurcate enabling is done in
> the ->crtc_mode_set callback.
> 
> To address this issues discussed with Ville in the patch review move
> the setting of the bifurcate bit into the ->crtc_enable hook. That way
> we won't wreak havoc with this state when userspace puts all other
> outputs into dpms off state. This also moves us forward with our
> overall goal to unify the modeset and dpms on paths (which we need to
> have to allow runtime pm in the dpms off state).
> 
> Unfortunately this requires us to move the bifurcate helpers around a
> bit.
> 
> Also update the commit message, I've misanalyzed the bug rather badly.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70507
> Tested-by: Jan-Michael Brummer <jan.brummer@tabos.org>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And since it now calls cpt_enable_fdi_bc_bifurcation() only when 
has_pch_encoder==true it should also fix the following scenario:

1. setcrtc pipe B -> PCH w/ fdi_lanes>2
2. setcrtc pipe C -> eDP

Previously the pipe C .mode_set() whould have called
cpt_enable_fdi_bc_bifurcation() unconditionally, which
would have killed pipe B.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++------------------
>  1 file changed, 48 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8c3bf8a89cb7..509762c85d2e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2421,9 +2421,10 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  			   FDI_FE_ERRC_ENABLE);
>  }
>  
> -static bool pipe_has_enabled_pch(struct intel_crtc *intel_crtc)
> +static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  {
> -	return intel_crtc->base.enabled && intel_crtc->config.has_pch_encoder;
> +	return crtc->base.enabled && crtc->active &&
> +		crtc->config.has_pch_encoder;
>  }
>  
>  static void ivb_modeset_global_resources(struct drm_device *dev)
> @@ -3074,6 +3075,48 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>  }
>  
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t temp;
> +
> +	temp = I915_READ(SOUTH_CHICKEN1);
> +	if (temp & FDI_BC_BIFURCATION_SELECT)
> +		return;
> +
> +	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> +	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> +
> +	temp |= FDI_BC_BIFURCATION_SELECT;
> +	DRM_DEBUG_KMS("enabling fdi C rx\n");
> +	I915_WRITE(SOUTH_CHICKEN1, temp);
> +	POSTING_READ(SOUTH_CHICKEN1);
> +}
> +
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	switch (intel_crtc->pipe) {
> +	case PIPE_A:
> +		break;
> +	case PIPE_B:
> +		if (intel_crtc->config.fdi_lanes > 2)
> +			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +		else
> +			cpt_enable_fdi_bc_bifurcation(dev);
> +
> +		break;
> +	case PIPE_C:
> +		cpt_enable_fdi_bc_bifurcation(dev);
> +
> +		break;
> +	default:
> +		BUG();
> +	}
> +}
> +
>  /*
>   * Enable PCH resources required for PCH ports:
>   *   - PCH PLLs
> @@ -3092,6 +3135,9 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  
>  	assert_pch_transcoder_disabled(dev_priv, pipe);
>  
> +	if (IS_IVYBRIDGE(dev))
> +		ivybridge_update_fdi_bc_bifurcation(intel_crtc);
> +
>  	/* Write the TU size bits before fdi link training, so that error
>  	 * detection works. */
>  	I915_WRITE(FDI_RX_TUSIZE1(pipe),
> @@ -5849,48 +5895,6 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint32_t temp;
> -
> -	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> -		return;
> -
> -	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> -	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> -
> -	temp |= FDI_BC_BIFURCATION_SELECT;
> -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> -	I915_WRITE(SOUTH_CHICKEN1, temp);
> -	POSTING_READ(SOUTH_CHICKEN1);
> -}
> -
> -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> -{
> -	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	switch (intel_crtc->pipe) {
> -	case PIPE_A:
> -		break;
> -	case PIPE_B:
> -		if (intel_crtc->config.fdi_lanes > 2)
> -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> -		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> -
> -		break;
> -	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> -
> -		break;
> -	default:
> -		BUG();
> -	}
> -}
> -
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
>  {
>  	/*
> @@ -6079,9 +6083,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  					     &intel_crtc->config.fdi_m_n);
>  	}
>  
> -	if (IS_IVYBRIDGE(dev))
> -		ivybridge_update_fdi_bc_bifurcation(intel_crtc);
> -
>  	ironlake_set_pipeconf(crtc);
>  
>  	/* Set up the display plane register */
> -- 
> 1.8.4.rc3

-- 
Ville Syrjälä
Intel OTC