[Intel-gfx] [PATCH] drm/i915/icl+: Prevent using non-TypeC AUX channels on TypeC ports

[Imre Deak]

On Tue, Apr 28, 2020 at 12:05:35PM +0300, Jani Nikula wrote:
> On Tue, 28 Apr 2020, Imre Deak <imre.deak at intel.com> wrote:
> > On Tue, Apr 28, 2020 at 10:55:49AM +0300, Jani Nikula wrote:
> >> On Thu, 23 Apr 2020, Imre Deak <imre.deak at intel.com> wrote:
> >> > Using an AUX channel which by default belongs to a non-TypeC PHY won't
> >> > work on a TypeC PHY, since - as a side-effect besides providing an AUX
> >> > channel - the AUX channel power well affects power manangement specific
> >> > to the TypeC subsystem. Using a TypeC AUX channel on a non-TypeC PHY
> >> > would probably also cause problems, so for simplicity prevent both.
> >> >
> >> > This fixes at least an ICL-Y machine in CI, which has a buggy VBT
> >> > setting AUX-B as an alternative channel for port C.
> >> 
> >> Is it a production machine?
> >
> > Yes.

Err, I meant to say it's a pre-production machine icl-dsi. However, the
problem is not specific to whether the machine is pre-pro or not. A VBT
with this problem on a production machine would cause the same problem.

> *sigh*
> 
> Yeah I guess that settles it, we'll need this. :/
> 
> Ack.
> 
> >> Not happy about adding stuff for pre-pro machines with buggy VBT.
> >> It'll bite us later. It always has.
> >
> > If there is a buggy VBT with this issue it will cause a problem
> > somewhere down the pipeline which is difficult to track down, power well
> > timeouts, machine hangs etc. I would like to catch this early and avoid
> > having to spend time debugging these other issues.
> >
> >> Also, hate to see VBT code call into intel_display.c (intel_phy_is_tc).
> >
> > That's the way to determine if a port/PHY is TypeC on a platform or not.
> >
> > --Imre
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 84 +++++++++++++++--------
> >> >  1 file changed, 57 insertions(+), 27 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > index 839124647202..10d463723d12 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > @@ -1538,11 +1538,38 @@ static enum port get_port_by_aux_ch(struct drm_i915_private *i915, u8 aux_ch)
> >> >  	return PORT_NONE;
> >> >  }
> >> >  
> >> > +static enum aux_ch
> >> > +intel_bios_port_info_aux_ch(const struct ddi_vbt_port_info *info)
> >> > +{
> >> > +	switch (info->alternate_aux_channel) {
> >> > +	case DP_AUX_A:
> >> > +		return AUX_CH_A;
> >> > +	case DP_AUX_B:
> >> > +		return AUX_CH_B;
> >> > +	case DP_AUX_C:
> >> > +		return AUX_CH_C;
> >> > +	case DP_AUX_D:
> >> > +		return AUX_CH_D;
> >> > +	case DP_AUX_E:
> >> > +		return AUX_CH_E;
> >> > +	case DP_AUX_F:
> >> > +		return AUX_CH_F;
> >> > +	case DP_AUX_G:
> >> > +		return AUX_CH_G;
> >> > +	default:
> >> > +		MISSING_CASE(info->alternate_aux_channel);
> >> > +		return AUX_CH_A;
> >> > +	}
> >> > +}
> >> > +
> >> >  static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
> >> >  			    enum port port)
> >> >  {
> >> >  	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
> >> >  	enum port p;
> >> > +	enum aux_ch aux_ch;
> >> > +	bool aux_is_tc;
> >> > +	bool phy_is_tc;
> >> >  
> >> >  	if (!info->alternate_aux_channel)
> >> >  		return;
> >> > @@ -1571,6 +1598,35 @@ static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
> >> >  
> >> >  		info->supports_dp = false;
> >> >  		info->alternate_aux_channel = 0;
> >> > +
> >> > +		return;
> >> > +	}
> >> > +
> >> > +	aux_ch = intel_bios_port_info_aux_ch(info);
> >> > +	/* The AUX CH -> default port is a 1:1 mapping. */
> >> > +	aux_is_tc = intel_phy_is_tc(dev_priv,
> >> > +				    intel_port_to_phy(dev_priv,
> >> > +						      (enum port)aux_ch));
> >> > +	phy_is_tc = intel_phy_is_tc(dev_priv,
> >> > +				    intel_port_to_phy(dev_priv, port));
> >> > +	if (aux_is_tc != phy_is_tc) {
> >> > +		/*
> >> > +		 * Using an AUX channel which by default belongs to a TypeC
> >> > +		 * PHY can't be used for non-TypeC PHYs and vice-versa. The
> >> > +		 * reason is that TypeC AUX power wells can only be enabled in
> >> > +		 * the current TypeC mode of the PHY and have an effect on power
> >> > +		 * management specific to the TypeC subsystem.
> >> > +		 */
> >> > +		drm_dbg_kms(&dev_priv->drm,
> >> > +			    "Port %c on a %s PHY is trying to use the %s AUX CH %c, "
> >> > +			    "disabling DP support on this port.\n",
> >> > +			    port_name(port),
> >> > +			    phy_is_tc ? "TypeC" : "non-TypeC",
> >> > +			    aux_is_tc ? "TypeC" : "non-TypeC",
> >> > +			    aux_ch_name(aux_ch));
> >> > +
> >> > +		info->supports_dp = false;
> >> > +		info->alternate_aux_channel = 0;
> >> >  	}
> >> >  }
> >> >  
> >> > @@ -2595,33 +2651,7 @@ enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv,
> >> >  		return aux_ch;
> >> >  	}
> >> >  
> >> > -	switch (info->alternate_aux_channel) {
> >> > -	case DP_AUX_A:
> >> > -		aux_ch = AUX_CH_A;
> >> > -		break;
> >> > -	case DP_AUX_B:
> >> > -		aux_ch = AUX_CH_B;
> >> > -		break;
> >> > -	case DP_AUX_C:
> >> > -		aux_ch = AUX_CH_C;
> >> > -		break;
> >> > -	case DP_AUX_D:
> >> > -		aux_ch = AUX_CH_D;
> >> > -		break;
> >> > -	case DP_AUX_E:
> >> > -		aux_ch = AUX_CH_E;
> >> > -		break;
> >> > -	case DP_AUX_F:
> >> > -		aux_ch = AUX_CH_F;
> >> > -		break;
> >> > -	case DP_AUX_G:
> >> > -		aux_ch = AUX_CH_G;
> >> > -		break;
> >> > -	default:
> >> > -		MISSING_CASE(info->alternate_aux_channel);
> >> > -		aux_ch = AUX_CH_A;
> >> > -		break;
> >> > -	}
> >> > +	aux_ch = intel_bios_port_info_aux_ch(info);
> >> >  
> >> >  	drm_dbg_kms(&dev_priv->drm, "using AUX %c for port %c (VBT)\n",
> >> >  		    aux_ch_name(aux_ch), port_name(port));
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center