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

Submitted by Ville Syrjälä on Feb. 23, 2016, 4:46 p.m.

Details

Message ID 1456245988-19442-5-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show
Series "drm: DP++ adaptor support" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Ville Syrjälä Feb. 23, 2016, 4:46 p.m.
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(-)

Patch hide | download patch | download mbox

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;
+	}
+	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);
 	}

Comments

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.

- 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.

- 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);

>  	}
On Fri, 01 Apr 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> 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.
>
> - 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.
>
> - 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.

Yup, it should become intel_bios_is_dp_dual_mode() or something along
those lines in intel_bios.c. intel_vbt_defs.h is now private data to
intel_bios.c. intel_bios.h should be kept minimal.

My long term plan is to make most of dev_priv->vbt opaque pointers you
can query using helpers provided by intel_bios.c. In the future, this
should make it easier to change how we obtain and store the data. This
should also make it easier to handle setups without VBT or with limited
VBT.

BR,
Jani.

>
>> +	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);
>>  	}
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.

> 
> - 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);
> >  	}
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);

> > >  	}