Re: [Intel-gfx] [PATCH] drm/i915: Assuming non-DP++ port if dvo_port is HDMI and there's no AUX ch specified in the VBT

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

On Mon, Nov 14, 2016 at 08:15:56AM +0100, Daniel Vetter wrote:
> On Fri, Nov 11, 2016 at 07:14:24PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > My heuristic for detecting type 1 DVI DP++ adaptors based on the VBT
> > port information apparently didn't survive the reality of buggy VBTs.
> > In this particular case we have a machine with a natice HDMI port, but
> > the VBT telsl us it's a DP++ port based on its capabilities.
> > 
> > The dvo_port information in VBT does claim that we're dealing with a
> > HDMI port though, but we have other machines which do the same even
> > when they actually have DP++ ports. So that piece of information alone
> > isn't sufficient to tell the two apart.
> > 
> > After staring at a bunch of VBTs from various machines, I have to
> > conclude that the only other semi-reliable clue we can use is the
> > presence of the AUX channel in the VBT. On this particular machine
> > AUX channel is specified as zero, whereas on some of the other machines
> > which listed the DP++ port as HDMI have a non-zero AUX channel.
> > 
> > I've also seen VBTs which have dvo_port a DP but have a zero AUX
> > channel. I believe those we need to treat as DP ports, so we'll limit
> > the AUX channel check to just the cases where dvo_port is HDMI.
> > 
> > If we encounter any more serious failures with this heuristic I think
> > we'll have to have to throw it out entirely. But that could mean that
> > there is a risk of type 1 DVI dongle users getting greeted by a
> > black screen, so I'd rather not go there unless absolutely necessary.
> > 
> > Cc: Daniel Otero <daniel.otero@outlook.com>
> > Cc: stable@vger.kernel.org
> > Tested-by: Daniel Otero <daniel.otero@outlook.com>
> > Fixes: d61992565bd3 ("drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97994
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_bios.c     | 32 ++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_vbt_defs.h |  3 ++-
> >  2 files changed, 26 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 5ab646ef8c9f..33ed05186810 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1147,7 +1147,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> >  	if (!child)
> >  		return;
> >  
> > -	aux_channel = child->raw[25];
> > +	aux_channel = child->common.aux_channel;
> >  	ddc_pin = child->common.ddc_pin;
> >  
> >  	is_dvi = child->common.device_type & DEVICE_TYPE_TMDS_DVI_SIGNALING;
> > @@ -1677,7 +1677,8 @@ bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
> >  	return false;
> >  }
> >  
> > -bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum port port)
> > +static bool child_dev_is_dp_dual_mode(const union child_device_config *p_child,
> > +				      enum port port)
> >  {
> >  	static const struct {
> >  		u16 dp, hdmi;
> > @@ -1691,22 +1692,37 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum por
> >  		[PORT_D] = { DVO_PORT_DPD, DVO_PORT_HDMID, },
> >  		[PORT_E] = { DVO_PORT_DPE, DVO_PORT_HDMIE, },
> >  	};
> > -	int i;
> >  
> >  	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
> >  		return false;
> >  
> > -	if (!dev_priv->vbt.child_dev_num)
> > +	if ((p_child->common.device_type & DEVICE_TYPE_DP_DUAL_MODE_BITS) !=
> > +	    (DEVICE_TYPE_DP_DUAL_MODE & DEVICE_TYPE_DP_DUAL_MODE_BITS))
> > +		return false;
> > +
> > +	if (p_child->common.dvo_port == port_mapping[port].dp)
> > +		return true;
> > +
> > +	/* Only accept a HDMI dvo_port as DP++ if it has an AUX channel */
> > +	if (p_child->common.dvo_port == port_mapping[port].hdmi &&
> > +	    p_child->common.aux_channel != 0)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum port port)
> > +{
> > +	int i;
> > +
> > +	if (port == PORT_A)
> >  		return false;
> 
> We check for PORT_A twice now. Otherwise contains what it says on the tin,
> but no idea whether this is the perfect solution either. Also need to
> check vbt docs for the right bit, but assuming that checks out (I'll
> report on irc), and with the above nitpick fixed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

11:34 < danvet> vsyrjala, vbt seems to check out too

v2: Remove the duplicate PORT_A check (Daniel)
    Fix some typos in the commit message

Pushed to dinq. Thanks for the review.

> 
> >  
> >  	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> >  		const union child_device_config *p_child =
> >  			&dev_priv->vbt.child_dev[i];
> >  
> > -		if ((p_child->common.dvo_port == port_mapping[port].dp ||
> > -		     p_child->common.dvo_port == port_mapping[port].hdmi) &&
> > -		    (p_child->common.device_type & DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
> > -		    (DEVICE_TYPE_DP_DUAL_MODE & DEVICE_TYPE_DP_DUAL_MODE_BITS))
> > +		if (child_dev_is_dp_dual_mode(p_child, port))
> >  			return true;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
> > index 68db9621f1f0..8886cab19f98 100644
> > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> > @@ -280,7 +280,8 @@ struct common_child_dev_config {
> >  	u8 dp_support:1;
> >  	u8 tmds_support:1;
> >  	u8 support_reserved:5;
> > -	u8 not_common3[12];
> > +	u8 aux_channel;
> > +	u8 not_common3[11];
> >  	u8 iboost_level;
> >  } __packed;
> >  
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrj�l�
Intel OTC