Re: [PATCH 3/4] drm/i915: Clean up DDI DDC/AUX CH sanitation

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

On Tue, Oct 11, 2016 at 08:52:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Now that we use the AUX and GMBUS assignment from VBT for all ports,
> let's clean up the sanitization of the port information a bit.
> Previosuly we only did this for port E, and only complained about a
> non-standard assignment for the other ports. But as we know that
> non-standard assignments are a fact of life, let's expand the
> sanitization to all the ports.
> 
> v2: Include a commit message, fix up the comments a bit
> 
> Cc: stable@vger.kernel.org
> Cc: Maarten Maathuis <madman2003@gmail.com>
> Tested-by: Maarten Maathuis <madman2003@gmail.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=97877
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 116 +++++++++++++++++++++++---------------
>  1 file changed, 71 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 83667e8cdd6b..6d1ffa815e97 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1035,6 +1035,71 @@ static u8 translate_iboost(u8 val)
>  	return mapping[val];
>  }
>  
> +static void sanitize_ddc_pin(struct drm_i915_private *dev_priv,
> +			     enum port port)
> +{
> +	const struct ddi_vbt_port_info *info =
> +		&dev_priv->vbt.ddi_port_info[port];
> +	enum port p;
> +

Hmm. And I just realized this code is probably slightly garbage. If the
current port doesn't have alternate_ddc_pin/alternate_aux_channel, then
we'd proceed to clobber any lower port without the thing as well. Now,
potentially all DDI platforms have this stuff fully decked out in VBT so
it might not matter in practice. But better safe that sorry, so I'll
send out a new version of this patch that checks here for the zero
case in both of these functions.

> +	for_each_port_masked(p, (1 << port) - 1) {
> +		struct ddi_vbt_port_info *i = &dev_priv->vbt.ddi_port_info[p];
> +
> +		if (info->alternate_ddc_pin != i->alternate_ddc_pin)
> +			continue;
> +
> +		DRM_DEBUG_KMS("port %c trying to use the same DDC pin (0x%x) as port %c, "
> +			      "disabling port %c DVI/HDMI support\n",
> +			      port_name(p), i->alternate_ddc_pin,
> +			      port_name(port), port_name(p));
> +
> +		/*
> +		 * If we have multiple ports supposedly sharing the
> +		 * pin, then dvi/hdmi couldn't exist on the shared
> +		 * port. Otherwise they share the same ddc bin and
> +		 * system couldn't communicate with them separately.
> +		 *
> +		 * Due to parsing the ports in alphabetical order,
> +		 * a higher port will always clobber a lower one.
> +		 */
> +		i->supports_dvi = false;
> +		i->supports_hdmi = false;
> +		i->alternate_ddc_pin = 0;
> +	}
> +}
> +
> +static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
> +			    enum port port)
> +{
> +	const struct ddi_vbt_port_info *info =
> +		&dev_priv->vbt.ddi_port_info[port];
> +	enum port p;
> +
> +	for_each_port_masked(p, (1 << port) - 1) {
> +		struct ddi_vbt_port_info *i = &dev_priv->vbt.ddi_port_info[p];
> +
> +		if (info->alternate_aux_channel != i->alternate_aux_channel)
> +			continue;
> +
> +		DRM_DEBUG_KMS("port %c trying to use the same AUX CH (0x%x) as port %c, "
> +			      "disabling port %c DP support\n",
> +			      port_name(p), i->alternate_aux_channel,
> +			      port_name(port), port_name(p));
> +
> +		/*
> +		 * If we have multiple ports supposedlt sharing the
> +		 * aux channel, then DP couldn't exist on the shared
> +		 * port. Otherwise they share the same aux channel
> +		 * and system couldn't communicate with them separately.
> +		 *
> +		 * Due to parsing the ports in alphabetical order,
> +		 * a higher port will always clobber a lower one.
> +		 */
> +		i->supports_dp = false;
> +		i->alternate_aux_channel = 0;
> +	}
> +}
> +
>  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  			   const struct bdb_header *bdb)
>  {
> @@ -1109,54 +1174,15 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  		DRM_DEBUG_KMS("Port %c is internal DP\n", port_name(port));
>  
>  	if (is_dvi) {
> -		if (port == PORT_E) {
> -			info->alternate_ddc_pin = ddc_pin;
> -			/* if DDIE share ddc pin with other port, then
> -			 * dvi/hdmi couldn't exist on the shared port.
> -			 * Otherwise they share the same ddc bin and system
> -			 * couldn't communicate with them seperately. */
> -			if (ddc_pin == DDC_PIN_B) {
> -				dev_priv->vbt.ddi_port_info[PORT_B].supports_dvi = 0;
> -				dev_priv->vbt.ddi_port_info[PORT_B].supports_hdmi = 0;
> -			} else if (ddc_pin == DDC_PIN_C) {
> -				dev_priv->vbt.ddi_port_info[PORT_C].supports_dvi = 0;
> -				dev_priv->vbt.ddi_port_info[PORT_C].supports_hdmi = 0;
> -			} else if (ddc_pin == DDC_PIN_D) {
> -				dev_priv->vbt.ddi_port_info[PORT_D].supports_dvi = 0;
> -				dev_priv->vbt.ddi_port_info[PORT_D].supports_hdmi = 0;
> -			}
> -		} else if (ddc_pin == DDC_PIN_B && port != PORT_B)
> -			DRM_DEBUG_KMS("Unexpected DDC pin for port B\n");
> -		else if (ddc_pin == DDC_PIN_C && port != PORT_C)
> -			DRM_DEBUG_KMS("Unexpected DDC pin for port C\n");
> -		else if (ddc_pin == DDC_PIN_D && port != PORT_D)
> -			DRM_DEBUG_KMS("Unexpected DDC pin for port D\n");
> +		info->alternate_ddc_pin = ddc_pin;
> +
> +		sanitize_ddc_pin(dev_priv, port);
>  	}
>  
>  	if (is_dp) {
> -		if (port == PORT_E) {
> -			info->alternate_aux_channel = aux_channel;
> -			/* if DDIE share aux channel with other port, then
> -			 * DP couldn't exist on the shared port. Otherwise
> -			 * they share the same aux channel and system
> -			 * couldn't communicate with them seperately. */
> -			if (aux_channel == DP_AUX_A)
> -				dev_priv->vbt.ddi_port_info[PORT_A].supports_dp = 0;
> -			else if (aux_channel == DP_AUX_B)
> -				dev_priv->vbt.ddi_port_info[PORT_B].supports_dp = 0;
> -			else if (aux_channel == DP_AUX_C)
> -				dev_priv->vbt.ddi_port_info[PORT_C].supports_dp = 0;
> -			else if (aux_channel == DP_AUX_D)
> -				dev_priv->vbt.ddi_port_info[PORT_D].supports_dp = 0;
> -		}
> -		else if (aux_channel == DP_AUX_A && port != PORT_A)
> -			DRM_DEBUG_KMS("Unexpected AUX channel for port A\n");
> -		else if (aux_channel == DP_AUX_B && port != PORT_B)
> -			DRM_DEBUG_KMS("Unexpected AUX channel for port B\n");
> -		else if (aux_channel == DP_AUX_C && port != PORT_C)
> -			DRM_DEBUG_KMS("Unexpected AUX channel for port C\n");
> -		else if (aux_channel == DP_AUX_D && port != PORT_D)
> -			DRM_DEBUG_KMS("Unexpected AUX channel for port D\n");
> +		info->alternate_aux_channel = aux_channel;
> +
> +		sanitize_aux_ch(dev_priv, port);
>  	}
>  
>  	if (bdb->version >= 158) {
> -- 
> 2.7.4

-- 
Ville Syrj�l�
Intel OTC