diff mbox

drm/i915: Simplify the way BC bifurcation state consistency is kept

Message ID 1425990754-8723-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ander Conselvan de Oliveira March 10, 2015, 12:32 p.m. UTC 
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling or
disabling the pch transcoder. The checks before the mode set should
ensure that the configuration is valid, so it should be safe to do it
so.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---

On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> With atomic we need to probably look at crtc_state->mode_changed. As an
> interim solution we have the same information available in the
> modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.

I looked into that solution, but involves passing modeset_pipes way down
into the call stack, so I started looking for a different solution. Do you
see any caveat with this approach?

Thanks,
Ander

---
 drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

Comments

Ville Syrjala March 10, 2015, 1:03 p.m. UTC | #1 
On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling or
> disabling the pch transcoder. The checks before the mode set should
> ensure that the configuration is valid, so it should be safe to do it
> so.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> 
> On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > With atomic we need to probably look at crtc_state->mode_changed. As an
> > interim solution we have the same information available in the
> > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> 
> I looked into that solution, but involves passing modeset_pipes way down
> into the call stack, so I started looking for a different solution. Do you
> see any caveat with this approach?
> 
> Thanks,
> Ander
> 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..eca5a41 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active);
>  
>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>  {
> @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  					    enum pipe pipe)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *crtc =
> +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  	uint32_t reg, val;
>  
>  	/* FDI relies on the transcoder */
> @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
>  		I915_WRITE(reg, val);
>  	}
> +
> +	if (IS_IVYBRIDGE(dev))
> +		ivybridge_update_fdi_bc_bifurcation(crtc, false);
>  }
>  
>  static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		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 = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,41 +3815,44 @@ 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)
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		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");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
>  
> -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active)
>  {
>  	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);
> +		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> +			cpt_enable_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_enable_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
>  

It sees could now change the bifurcation while pipe B is active (but only
using two lanes). Not sure if the old code did that too, or if it might
cause problems.

Also depending on the order you disable the pipes you might end up with
bifurcation enabled or disabled.

It seems to me that the simplest solution should be to have bifurcation
enabled at all times, except when pipe B really needs the four lanes.
Then the hardware state would only need to be changed when
enabling/disabling pipe B with four lanes. Rest of the time
crtc->enabled and fdi_lanes should be able to tell us which
configuration is possible and which not. Or am I missing something?

Well, eventually we want to tie this into the atomic state so that we
can actaully have pipe B drop into two lane mode if pipe C needs the
lanes (and pipe B can still operate with two lanes). But I guess that's
still not achievable with the current code.


>  		break;
>  	default:
> @@ -3895,7 +3879,7 @@ 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);
> +		ivybridge_update_fdi_bc_bifurcation(intel_crtc, true);
>  
>  	/* Write the TU size bits before fdi link training, so that error
>  	 * detection works. */
> @@ -13056,8 +13040,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter March 10, 2015, 7:10 p.m. UTC | #2 
On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling or
> disabling the pch transcoder. The checks before the mode set should
> ensure that the configuration is valid, so it should be safe to do it
> so.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> 
> On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > With atomic we need to probably look at crtc_state->mode_changed. As an
> > interim solution we have the same information available in the
> > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> 
> I looked into that solution, but involves passing modeset_pipes way down
> into the call stack, so I started looking for a different solution. Do you
> see any caveat with this approach?

Moving things to the disable hook would be great since that's yet another
special case remove. It wasnt' possible back when I've done this, and I
think the reason was that we still had a ->mode_set callback before the
crtc_enable hook. But that's all gone now, so as long as the bifurcate
update is done early enough I think this'll work.

Maybe discuss this with Ville locally - he provided all the feedback for
my original patch too? Random bikesheds below, I definitely need to think
about this more.

Cheers, Daniel

> 
> Thanks,
> Ander
> 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..eca5a41 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active);

Imo just move the functions instead of forward declarations.

>  
>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>  {
> @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  					    enum pipe pipe)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *crtc =
> +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  	uint32_t reg, val;
>  
>  	/* FDI relies on the transcoder */
> @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
>  		I915_WRITE(reg, val);
>  	}
> +
> +	if (IS_IVYBRIDGE(dev))
> +		ivybridge_update_fdi_bc_bifurcation(crtc, false);
>  }
>  
>  static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		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 = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,41 +3815,44 @@ 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)
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)

enable feels misnamed now. Maybe s/enable/set/ instead?

>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		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");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
>  
> -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active)
>  {
>  	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);
> +		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> +			cpt_enable_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_enable_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
>  
>  		break;
>  	default:
> @@ -3895,7 +3879,7 @@ 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);
> +		ivybridge_update_fdi_bc_bifurcation(intel_crtc, true);
>  
>  	/* Write the TU size bits before fdi link training, so that error
>  	 * detection works. */
> @@ -13056,8 +13040,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.1.0
>
Daniel Vetter March 10, 2015, 7:14 p.m. UTC | #3 
On Tue, Mar 10, 2015 at 03:03:59PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> > Remove the global modeset resource function that would disable the
> > bifurcation bit, and instead enable/disable it when enabling or
> > disabling the pch transcoder. The checks before the mode set should
> > ensure that the configuration is valid, so it should be safe to do it
> > so.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> > 
> > On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > > With atomic we need to probably look at crtc_state->mode_changed. As an
> > > interim solution we have the same information available in the
> > > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> > 
> > I looked into that solution, but involves passing modeset_pipes way down
> > into the call stack, so I started looking for a different solution. Do you
> > see any caveat with this approach?
> > 
> > Thanks,
> > Ander
> > 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
> >  1 file changed, 21 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4008bf4..eca5a41 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
> >  			    const struct intel_crtc_state *pipe_config);
> >  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
> >  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> > +						bool pipe_active);
> >  
> >  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
> >  {
> > @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
> >  					    enum pipe pipe)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_crtc *crtc =
> > +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >  	uint32_t reg, val;
> >  
> >  	/* FDI relies on the transcoder */
> > @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
> >  		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
> >  		I915_WRITE(reg, val);
> >  	}
> > +
> > +	if (IS_IVYBRIDGE(dev))
> > +		ivybridge_update_fdi_bc_bifurcation(crtc, false);
> >  }
> >  
> >  static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> > @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> >  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
> >  }
> >  
> > -static void ivb_modeset_global_resources(struct drm_device *dev)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *pipe_B_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > -	struct intel_crtc *pipe_C_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> > -	uint32_t temp;
> > -
> > -	/*
> > -	 * When everything is off disable fdi C so that we could enable fdi B
> > -	 * with all lanes. Note that we don't care about enabled pipes without
> > -	 * an enabled pch encoder.
> > -	 */
> > -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> > -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> > -		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 = I915_READ(SOUTH_CHICKEN1);
> > -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> > -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> > -		I915_WRITE(SOUTH_CHICKEN1, temp);
> > -	}
> > -}
> > -
> >  /* The FDI link training functions for ILK/Ibexpeak. */
> >  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
> >  {
> > @@ -3834,41 +3815,44 @@ 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)
> > +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t temp;
> >  
> >  	temp = I915_READ(SOUTH_CHICKEN1);
> > -	if (temp & FDI_BC_BIFURCATION_SELECT)
> > +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
> >  		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");
> > +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> > +	if (enable)
> > +		temp |= FDI_BC_BIFURCATION_SELECT;
> > +
> > +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
> >  	I915_WRITE(SOUTH_CHICKEN1, temp);
> >  	POSTING_READ(SOUTH_CHICKEN1);
> >  }
> >  
> > -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> > +						bool pipe_active)
> >  {
> >  	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);
> > +		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> > +			cpt_enable_fdi_bc_bifurcation(dev, false);
> >  		else
> > -			cpt_enable_fdi_bc_bifurcation(dev);
> > +			cpt_enable_fdi_bc_bifurcation(dev, true);
> >  
> >  		break;
> >  	case PIPE_C:
> > -		cpt_enable_fdi_bc_bifurcation(dev);
> > +		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
> >  
> 
> It sees could now change the bifurcation while pipe B is active (but only
> using two lanes). Not sure if the old code did that too, or if it might
> cause problems.

It shouldn't be possible and it'll anger the hw.
cpt_enable_fdi_bc_bifurcation has WARN_ONs to make sure the fdi rx for
both pch transcoder B & C are off to make sure we don't get this wrong.

> Also depending on the order you disable the pipes you might end up with
> bifurcation enabled or disabled.
> 
> It seems to me that the simplest solution should be to have bifurcation
> enabled at all times, except when pipe B really needs the four lanes.
> Then the hardware state would only need to be changed when
> enabling/disabling pipe B with four lanes. Rest of the time
> crtc->enabled and fdi_lanes should be able to tell us which
> configuration is possible and which not. Or am I missing something?
> 
> Well, eventually we want to tie this into the atomic state so that we
> can actaully have pipe B drop into two lane mode if pipe C needs the
> lanes (and pipe B can still operate with two lanes). But I guess that's
> still not achievable with the current code.

Checking fdi lane constraints is already done in the compute_config code.
And it assumes that it can always get the max, i.e. if pipe B is only
using 2 lanes it'll just enable pipe C. Which implies that we indeed have
to aggressive enable the bifurcate bit (since atm we don't support a
modeset on pipe B). For atomic modeset we'd just need to extend that code
to work with all pipes (like all the other compute_config code). That
shouldn't be a lot fo trouble (since we can always throw the crtc for the
other pipe into the update in atomic_check functions).
-Daniel
Shuang He March 10, 2015, 8:40 p.m. UTC | #4 
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5925
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              282/282              280/282
ILK                                  308/308              308/308
SNB                                  307/307              307/307
IVB                                  375/375              375/375
BYT                                  294/294              294/294
HSW                                  385/385              385/385
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gem_userptr_blits_minor-normal-sync      DMESG_WARN(2)PASS(3)      DMESG_WARN(2)
*PNV  igt_gem_userptr_blits_minor-unsync-normal      PASS(4)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..eca5a41 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -101,6 +101,8 @@  static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
+static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
+						bool pipe_active);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -1956,6 +1958,8 @@  static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
 					    enum pipe pipe)
 {
 	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 	uint32_t reg, val;
 
 	/* FDI relies on the transcoder */
@@ -1980,6 +1984,9 @@  static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
 		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
 		I915_WRITE(reg, val);
 	}
+
+	if (IS_IVYBRIDGE(dev))
+		ivybridge_update_fdi_bc_bifurcation(crtc, false);
 }
 
 static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
@@ -3153,32 +3160,6 @@  static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 	return crtc->base.state->enable && crtc->config->has_pch_encoder;
 }
 
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *pipe_B_crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
-	struct intel_crtc *pipe_C_crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
-	uint32_t temp;
-
-	/*
-	 * When everything is off disable fdi C so that we could enable fdi B
-	 * with all lanes. Note that we don't care about enabled pipes without
-	 * an enabled pch encoder.
-	 */
-	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
-	    !pipe_has_enabled_pch(pipe_C_crtc)) {
-		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 = I915_READ(SOUTH_CHICKEN1);
-		temp &= ~FDI_BC_BIFURCATION_SELECT;
-		DRM_DEBUG_KMS("disabling fdi C rx\n");
-		I915_WRITE(SOUTH_CHICKEN1, temp);
-	}
-}
-
 /* The FDI link training functions for ILK/Ibexpeak. */
 static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 {
@@ -3834,41 +3815,44 @@  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)
+static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t temp;
 
 	temp = I915_READ(SOUTH_CHICKEN1);
-	if (temp & FDI_BC_BIFURCATION_SELECT)
+	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
 		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");
+	temp &= ~FDI_BC_BIFURCATION_SELECT;
+	if (enable)
+		temp |= FDI_BC_BIFURCATION_SELECT;
+
+	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
 	I915_WRITE(SOUTH_CHICKEN1, temp);
 	POSTING_READ(SOUTH_CHICKEN1);
 }
 
-static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
+static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
+						bool pipe_active)
 {
 	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);
+		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
+			cpt_enable_fdi_bc_bifurcation(dev, false);
 		else
-			cpt_enable_fdi_bc_bifurcation(dev);
+			cpt_enable_fdi_bc_bifurcation(dev, true);
 
 		break;
 	case PIPE_C:
-		cpt_enable_fdi_bc_bifurcation(dev);
+		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
 
 		break;
 	default:
@@ -3895,7 +3879,7 @@  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);
+		ivybridge_update_fdi_bc_bifurcation(intel_crtc, true);
 
 	/* Write the TU size bits before fdi link training, so that error
 	 * detection works. */
@@ -13056,8 +13040,6 @@  static void intel_init_display(struct drm_device *dev)
 	} else if (IS_IVYBRIDGE(dev)) {
 		/* FIXME: detect B0+ stepping and use auto training */
 		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
-		dev_priv->display.modeset_global_resources =
-			ivb_modeset_global_resources;
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 	} else if (IS_VALLEYVIEW(dev)) {