diff mbox

drm/i915: Fix the PPT fdi lane bifurcate state handling on ivb

Message ID 1383044648-15603-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Oct. 29, 2013, 11:04 a.m. UTC 
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>
---
 drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 47 deletions(-)

Comments

Ville Syrjälä Oct. 29, 2013, 12:46 p.m. UTC | #1 
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
Daniel Vetter Oct. 29, 2013, 12:52 p.m. UTC | #2 
On Tue, Oct 29, 2013 at 02:46:10PM +0200, Ville Syrjälä wrote:
> 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.

Geez, did that take a while for me to sink in. Somehow I've thought I've
fixed this a long time ago so that cpu edp on pipe C allows us to fully
use the fdi links on pipes A/B. But reading through the code again I've
only updated the logic in the compute_config stage, not here.

Thanks for your review, patch merged to -fixes.
-Daniel

> 
> > ---
> >  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
diff mbox

Patch

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 */