Re: [PATCH 4/4] drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Sex, 2016-04-01 às 17:45 +0300, Ville Syrjälä escreveu:
> On Thu, Mar 31, 2016 at 10:06:37PM +0000, Zanoni, Paulo R wrote:
> > 
> > Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
> > escreveu:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > DP dual mode type 1 DVI adaptors aren't required to implement any
> > > registers, so it's a bit hard to detect them. The best way would
> > > be to check the state of the CONFIG1 pin, but we have no way to
> > > do that. So as a last resort, check the VBT to see if the HDMI
> > > port is in fact a dual mode capable DP port.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_bios.h |  3 +++
> > >  drivers/gpu/drm/i915/intel_dp.c   | 28
> > > ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
> > >  4 files changed, 53 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_bios.h
> > > b/drivers/gpu/drm/i915/intel_bios.h
> > > index 350d4e0f75a4..50d1659efe47 100644
> > > --- a/drivers/gpu/drm/i915/intel_bios.h
> > > +++ b/drivers/gpu/drm/i915/intel_bios.h
> > > @@ -730,6 +730,7 @@ struct bdb_psr {
> > >  #define	 DEVICE_TYPE_INT_TV	0x1009
> > >  #define	 DEVICE_TYPE_HDMI	0x60D2
> > >  #define	 DEVICE_TYPE_DP		0x68C6
> > > +#define	 DEVICE_TYPE_DP_DUAL_MODE	0x60D6
> > >  #define	 DEVICE_TYPE_eDP	0x78C6
> > >  
> > >  #define  DEVICE_TYPE_CLASS_EXTENSION	(1 << 15)
> > > @@ -764,6 +765,8 @@ struct bdb_psr {
> > >  	 DEVICE_TYPE_DISPLAYPORT_OUTPUT | \
> > >  	 DEVICE_TYPE_ANALOG_OUTPUT)
> > >  
> > > +#define DEVICE_TYPE_DP_DUAL_MODE_BITS
> > > ~DEVICE_TYPE_NOT_HDMI_OUTPUT
> > > +
> > >  /* define the DVO port for HDMI output type */
> > >  #define		DVO_B		1
> > >  #define		DVO_C		2
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index cbc06596659a..f3edacf517ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5104,6 +5104,34 @@ bool intel_dp_is_edp(struct drm_device
> > > *dev,
> > > enum port port)
> > >  	return false;
> > >  }
> > >  
> > > +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv,
> > > enum
> > > port port)
> > > +{
> > > +	const union child_device_config *p_child;
> > > +	int i;
> > > +	static const short port_mapping[] = {
> > > +		[PORT_B] = DVO_PORT_DPB,
> > > +		[PORT_C] = DVO_PORT_DPC,
> > > +		[PORT_D] = DVO_PORT_DPD,
> > > +		[PORT_E] = DVO_PORT_DPE,
> > > +	};
> > > +
> > > +	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
> > > +		return false;
> > > +
> > > +	if (!dev_priv->vbt.child_dev_num)
> > > +		return false;
> > > +
> > > +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> > > +		p_child = &dev_priv->vbt.child_dev[i];
> > > +
> > > +		if (p_child->common.dvo_port ==
> > > port_mapping[port]
> > > &&
> > > +		    (p_child->common.device_type &
> > > DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
> > > +		    (DEVICE_TYPE_DP_DUAL_MODE &
> > > DEVICE_TYPE_DP_DUAL_MODE_BITS))
> > > +			return true;
> > > +	}
> > Some thoughts:
> > 
> > This is going to be implemented for all VBT versions. Since there's
> > no
> > real history about anything before version 155, is this really what
> > we
> > want? A huge part of the "we don't trust the VBT" culture we have
> > on
> > our team is because of those old versions being completely
> > unreliable.
> > If this is the case, we could make this implementation just be a
> > small
> > patch in parse_ddi_port(). I'm kinda afraid we may somehow break
> > old
> > machines yet again.
> I don't think it matters much. ILK being the oldest platform with
> DP++
> capable of >165MHz, and at least my ILK here already has VBT version
> 163. Also this device type stuff was there before 155 already. And
> the is_edp() thing has worked for us mostly fine so if we things the
> same way it seems unlikely we get too many problems.

> 
> > 
> > 
> > - Instead of creating these complicated bit masks, why don't we
> > just
> > specifically check "if bit 2 and bit 4 are enabled, we're using an
> > adaptor"? Much simpler IMHO.
> I'm not sure it's a good idea to trust that some crappy BIOS doesn't
> just set whatever random bits on a random port. So a more
> conservative approach seems like a better idea to me. Also it matches
> how we do the is_edp() check.

Funcion intel_ddi_init() already only looks at the specific bits when
deciding whether to initialize DP or HDMI. OTOH that's just for HSW+
which has a somewhat-trustable VBT.

Anyway, I have no further comments for the series. Feel free to send v2
based on the discussion and your conclusions.

> 
> > 
> > 
> > - Jani's recent patch suggests you may want to move this function
> > to
> > intel_bios.c in order to avoid including intel_vbt_defs.h from
> > intel_hdmi.c. Anyway, you'll have to rebase.
> > 
> > > 
> > > +	return false;
> > > +}
> > > +
> > >  void
> > >  intel_dp_add_properties(struct intel_dp *intel_dp, struct
> > > drm_connector *connector)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3ca29a181e64..c7d1ea4dbe42 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1248,6 +1248,7 @@ int intel_dp_sink_crc(struct intel_dp
> > > *intel_dp, u8 *crc);
> > >  bool intel_dp_compute_config(struct intel_encoder *encoder,
> > >  			     struct intel_crtc_state
> > > *pipe_config);
> > >  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> > > +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv,
> > > enum
> > > port port);
> > >  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port,
> > >  				  bool long_hpd);
> > >  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 660a65f48fd8..1476f3afb7e2 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1390,14 +1390,33 @@ intel_hdmi_unset_edid(struct
> > > drm_connector
> > > *connector)
> > >  }
> > >  
> > >  static void
> > > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
> > > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector,
> > > bool
> > > has_edid)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(connector-
> > > >dev);
> > >  	struct intel_hdmi *hdmi =
> > > intel_attached_hdmi(connector);
> > > +	enum port port = hdmi_to_dig_port(hdmi)->port;
> > >  	struct i2c_adapter *adapter =
> > >  		intel_gmbus_get_adapter(dev_priv, hdmi-
> > > >ddc_bus);
> > >  	enum drm_dp_dual_mode_type type =
> > > drm_dp_dual_mode_detect(adapter);
> > >  
> > > +	/*
> > > +	 * Type 1 DVI adaptors are not required to implement any
> > > +	 * registers, so we can't always detect their presence.
> > > +	 * Ideally we should be able to check the state of the
> > > +	 * CONFIG1 pin, but no such luck on our hardware.
> > > +	 *
> > > +	 * The only method left to us is to check the VBT to see
> > > +	 * if the port is a dual mode capable DP port. But let's
> > > +	 * only do that when we sucesfully read the EDID, to
> > > avoid
> > > +	 * confusing log messages about DP dual mode adaptors
> > > when
> > > +	 * there's nothing connected to the port.
> > > +	 */
> > > +	if (type == DRM_DP_DUAL_MODE_NONE && has_edid &&
> > > +	    intel_dp_is_dual_mode(dev_priv, port)) {
> > > +		DRM_DEBUG_KMS("Assuming DP dual mode adaptor
> > > presence based on VBT\n");
> > > +		type = DRM_DP_DUAL_MODE_TYPE1_DVI;
> > > +	}
> > > +
> > >  	if (type == DRM_DP_DUAL_MODE_NONE)
> > >  		return;
> > >  
> > > @@ -1441,7 +1460,7 @@ intel_hdmi_set_edid(struct drm_connector
> > > *connector, bool force)
> > >  				    intel_gmbus_get_adapter(dev_
> > > priv
> > > ,
> > >  				    intel_hdmi->ddc_bus));
> > >  
> > > -		intel_hdmi_dp_dual_mode_detect(connector);
> > > +		intel_hdmi_dp_dual_mode_detect(connector, edid
> > > !=
> > > NULL);
> > >  
> > >  		intel_display_power_put(dev_priv,
> > > POWER_DOMAIN_GMBUS);
> > >  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx