Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

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

On Wed, Mar 11, 2015 at 01:35:43PM +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 the pch
> transcoder. The mode set consistency check should prevent us from
> disabling the bit if pipe C is enabled so the change should be safe.
> 
> Note that this doens't affect the logic that prevents the bit being
> set while a pipe is active, since the patch retains the behavior of
> only chaging the bit if necessary. Because of the checks during mode
> set, the first change would necessarily happen with both pipes B and
> C disabled, and any subsequent write would be skipped.
> 
> v2: Only change the bit during pch trancoder enable. (Ville)


Actually what I had inb mind was something like this:

pch_enable()
{
	if (pipe == B && fdi_lanes > 2)
		disable_bifurcation()
	...
}

pch_disable()
{
	...
	if (pipe == B && fdi_lanes > 2)
		enable_bifurcation()
}

So we only change it when enabling/disabling pipe B, never for pipe C.
Hence it remains enabled whenever pipe B doesn't need >2 FDI lanes.


> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
>  1 file changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..bfbd829 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3153,32 +3153,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,20 +3808,23 @@ 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_set_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);
>  }
> @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
>  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);
> +			cpt_set_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	default:
> @@ -13056,8 +13032,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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx