[Intel-gfx] [PATCH] drm/i915/cnl: Force DDI_A_4_LANES when needed.

[Ville Syrjälä]

On Fri, Oct 20, 2017 at 04:11:27PM -0700, Rodrigo Vivi wrote:
> As we faced in BXT, on CNL DDI_A_4_LANES is not
> set as expected when system is boot with multiple
> monitors connected. This result in wrong lane
> setup impacting the max data rate available and
> consequently blocking modeset on eDP, resulting
> in a blank screen.
> 
> Most of CNL SKUs don't support DDI-E.
> The only SKU that supports DDI-E is the same
> that supports the full A/E split called DDI-F.
> 
> Also when DDI-F is used DDI-E cannot be used because
> they share Interrupts. So DDI-E is almost useless.
> Anyways let's consider this is possible and rely on
> VBT for that.
> 
> Since this become a trend this commit also adds
> an extra commit message so in the future we have
> an extra insight when we are dealing with this
> same case.
> 
> This patch was initialy start by Clint, but required
> many changes including full commit message. So
> Credits entirely to Clint for finding this.
> 
> Suggested-by: Clint Taylor <clinton.a.taylor at intel.com>
> Cc: Clint Taylor <clinton.a.taylor at intel.com>
> Cc: Mika Kahola <mika.kahola at intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index adf51b328844..8acacf800a24 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2803,17 +2803,29 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	}
>  
>  	/*
> -	 * Bspec says that DDI_A_4_LANES is the only supported configuration
> -	 * for Broxton.  Yet some BIOS fail to set this bit on port A if eDP
> -	 * wasn't lit up at boot.  Force this bit on in our internal
> +	 * Some BIOS might fail to set this bit on port A if eDP
> +	 * wasn't lit up at boot.  Force this bit when needed on in our internal
>  	 * configuration so that we use the proper lane count for our
>  	 * calculations.
>  	 */
> -	if (IS_GEN9_LP(dev_priv) && port == PORT_A) {
> -		if (!(intel_dig_port->saved_port_bits & DDI_A_4_LANES)) {
> -			DRM_DEBUG_KMS("BXT BIOS forgot to set DDI_A_4_LANES for port A; fixing\n");
> +	if (port == PORT_A &&
> +	    !(intel_dig_port->saved_port_bits & DDI_A_4_LANES)) {
> +
> +		/* Broxton: Bspec says that DDI_A_4_LANES is the only supported
> +		 *          configuration
> +		 * Cannonlake: Most of SKUs don't support DDI_E, and the only
> +		 *             one who does also have a full A/E split called
> +		 *             DDI_F what makes DDI_E useless. However for this
> +		 *             case let's trust VBT info.
> +		 */
> +		if (IS_GEN9_LP(dev_priv) ||
> +		    ((IS_CANNONLAKE(dev_priv)) &&
> +		     !intel_bios_is_port_present(dev_priv, PORT_E))) {

Can you extract all these messy conditions into some kind of (ideally
less convoluted) helper function? In the end I think it would be cool if
this part of the code ends up looking something like:

if (intel_ddi_a_force_4_lanes(dig_port)) {
	DRM_DEBUG_KMS(...);
	dig_port->saved_port_bits |= DDI_A_4_LANES;
	max_lanes = 4;
}


> +			DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n");
>  			intel_dig_port->saved_port_bits |= DDI_A_4_LANES;
>  			max_lanes = 4;
> +		} else {
> +			DRM_DEBUG_KMS("DDI A configured for 2 lanes\n");
>  		}
>  	}
>  
> -- 
> 2.13.5

-- 
Ville Syrjälä
Intel OTC