[PATCH 1/5] drm/i915: VBT's child_device_config changes over time

[PATCH 1/5] drm/i915: VBT's child_device_config changes over time

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We currently treat the child_device_config as a simple struct, but
this is not correct: new BDB versions change the meaning of some
offsets, so the struct needs to be adjusted for each version.

Since there are too many changes (today we're in version 170!), making
a big versioned union would be too complicated, so child_device_config
is now a union of 3 things: (i) a "raw" byte array that's safe to use
anywhere; (ii)  an "old" structure that's the one we've been using and
should be safe to keep in the SDVO and TV code; and (iii) a "common"
structure that should contain only fields that are common for all the
known VBT versions.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  2 +-
 drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_bios.h | 33 +++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c   |  6 +++---
 drivers/gpu/drm/i915/intel_lvds.c |  3 ++-
 drivers/gpu/drm/i915/intel_tv.c   |  8 ++++----
 6 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 81ba5bb..ca8856a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1085,7 +1085,7 @@ struct intel_vbt_data {
 	int crt_ddc_pin;
 
 	int child_dev_num;
-	struct child_device_config *child_dev;
+	union child_device_config *child_dev;
 };
 
 enum intel_ddb_partitioning {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 6668873..33003b9 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -389,7 +389,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 {
 	struct sdvo_device_mapping *p_mapping;
 	struct bdb_general_definitions *p_defs;
-	struct child_device_config *p_child;
+	union child_device_config *p_child;
 	int i, child_device_num, count;
 	u16	block_size;
 
@@ -416,36 +416,36 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 	count = 0;
 	for (i = 0; i < child_device_num; i++) {
 		p_child = &(p_defs->devices[i]);
-		if (!p_child->device_type) {
+		if (!p_child->old.device_type) {
 			/* skip the device block if device type is invalid */
 			continue;
 		}
-		if (p_child->slave_addr != SLAVE_ADDR1 &&
-			p_child->slave_addr != SLAVE_ADDR2) {
+		if (p_child->old.slave_addr != SLAVE_ADDR1 &&
+			p_child->old.slave_addr != SLAVE_ADDR2) {
 			/*
 			 * If the slave address is neither 0x70 nor 0x72,
 			 * it is not a SDVO device. Skip it.
 			 */
 			continue;
 		}
-		if (p_child->dvo_port != DEVICE_PORT_DVOB &&
-			p_child->dvo_port != DEVICE_PORT_DVOC) {
+		if (p_child->old.dvo_port != DEVICE_PORT_DVOB &&
+			p_child->old.dvo_port != DEVICE_PORT_DVOC) {
 			/* skip the incorrect SDVO port */
 			DRM_DEBUG_KMS("Incorrect SDVO port. Skip it\n");
 			continue;
 		}
 		DRM_DEBUG_KMS("the SDVO device with slave addr %2x is found on"
 				" %s port\n",
-				p_child->slave_addr,
-				(p_child->dvo_port == DEVICE_PORT_DVOB) ?
+				p_child->old.slave_addr,
+				(p_child->old.dvo_port == DEVICE_PORT_DVOB) ?
 					"SDVOB" : "SDVOC");
-		p_mapping = &(dev_priv->sdvo_mappings[p_child->dvo_port - 1]);
+		p_mapping = &(dev_priv->sdvo_mappings[p_child->old.dvo_port - 1]);
 		if (!p_mapping->initialized) {
-			p_mapping->dvo_port = p_child->dvo_port;
-			p_mapping->slave_addr = p_child->slave_addr;
-			p_mapping->dvo_wiring = p_child->dvo_wiring;
-			p_mapping->ddc_pin = p_child->ddc_pin;
-			p_mapping->i2c_pin = p_child->i2c_pin;
+			p_mapping->dvo_port = p_child->old.dvo_port;
+			p_mapping->slave_addr = p_child->old.slave_addr;
+			p_mapping->dvo_wiring = p_child->old.dvo_wiring;
+			p_mapping->ddc_pin = p_child->old.ddc_pin;
+			p_mapping->i2c_pin = p_child->old.i2c_pin;
 			p_mapping->initialized = 1;
 			DRM_DEBUG_KMS("SDVO device: dvo=%x, addr=%x, wiring=%d, ddc_pin=%d, i2c_pin=%d\n",
 				      p_mapping->dvo_port,
@@ -457,7 +457,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
 					 "two SDVO device.\n");
 		}
-		if (p_child->slave2_addr) {
+		if (p_child->old.slave2_addr) {
 			/* Maybe this is a SDVO device with multiple inputs */
 			/* And the mapping info is not added */
 			DRM_DEBUG_KMS("there exists the slave2_addr. Maybe this"
@@ -588,7 +588,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 		       struct bdb_header *bdb)
 {
 	struct bdb_general_definitions *p_defs;
-	struct child_device_config *p_child, *child_dev_ptr;
+	union child_device_config *p_child, *child_dev_ptr;
 	int i, child_device_num, count;
 	u16	block_size;
 
@@ -616,7 +616,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	/* get the number of child device that is present */
 	for (i = 0; i < child_device_num; i++) {
 		p_child = &(p_defs->devices[i]);
-		if (!p_child->device_type) {
+		if (!p_child->common.device_type) {
 			/* skip the device block if device type is invalid */
 			continue;
 		}
@@ -636,7 +636,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	count = 0;
 	for (i = 0; i < child_device_num; i++) {
 		p_child = &(p_defs->devices[i]);
-		if (!p_child->device_type) {
+		if (!p_child->common.device_type) {
 			/* skip the device block if device type is invalid */
 			continue;
 		}
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 6e9250e..1da2bf2 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -202,7 +202,10 @@ struct bdb_general_features {
 #define DEVICE_PORT_DVOB	0x01
 #define DEVICE_PORT_DVOC	0x02
 
-struct child_device_config {
+/* We used to keep this struct but without any version control. We should avoid
+ * using it in the future, but it should be safe to keep using it in the old
+ * code. */
+struct old_child_dev_config {
 	u16 handle;
 	u16 device_type;
 	u8  device_id[10]; /* ascii string */
@@ -224,6 +227,32 @@ struct child_device_config {
 	u8  dvo_function;
 } __attribute__((packed));
 
+/* This one contains field offsets that are known to be common for all BDB
+ * versions. Notice that the meaning of the contents contents may still change,
+ * but at least the offsets are consistent. */
+struct common_child_dev_config {
+	u16 handle;
+	u16 device_type;
+	u8 not_common1[12];
+	u8 dvo_port;
+	u8 not_common2[2];
+	u8 ddc_pin;
+	u16 edid_ptr;
+} __attribute__((packed));
+
+/* This field changes depending on the BDB version, so the most reliable way to
+ * read it is by checking the BDB version and reading the raw pointer. */
+union child_device_config {
+	/* This one is safe to be used anywhere, but the code should still check
+	 * the BDB version. */
+	u8 raw[33];
+	/* This one should only be kept for legacy code. */
+	struct old_child_dev_config old;
+	/* This one should also be safe to use anywhere, even without version
+	 * checks. */
+	struct common_child_dev_config common;
+};
+
 struct bdb_general_definitions {
 	/* DDC GPIO */
 	u8 crt_ddc_gmbus_pin;
@@ -249,7 +278,7 @@ struct bdb_general_definitions {
 	 * number = (block_size - sizeof(bdb_general_definitions))/
 	 *	     sizeof(child_device_config);
 	 */
-	struct child_device_config devices[0];
+	union child_device_config devices[0];
 } __attribute__((packed));
 
 struct bdb_lvds_options {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8c70a83..20e468c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3169,7 +3169,7 @@ intel_trans_dp_port_sel(struct drm_crtc *crtc)
 bool intel_dpd_is_edp(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct child_device_config *p_child;
+	union child_device_config *p_child;
 	int i;
 
 	if (!dev_priv->vbt.child_dev_num)
@@ -3178,8 +3178,8 @@ bool intel_dpd_is_edp(struct drm_device *dev)
 	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
 		p_child = dev_priv->vbt.child_dev + i;
 
-		if (p_child->dvo_port == PORT_IDPD &&
-		    p_child->device_type == DEVICE_TYPE_eDP)
+		if (p_child->common.dvo_port == PORT_IDPD &&
+		    p_child->common.device_type == DEVICE_TYPE_eDP)
 			return true;
 	}
 	return false;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 831a5c0..277c2be 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -786,7 +786,8 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
 		return true;
 
 	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		struct child_device_config *child = dev_priv->vbt.child_dev + i;
+		union child_device_config *uchild = dev_priv->vbt.child_dev + i;
+		struct old_child_dev_config *child = &uchild->old;
 
 		/* If the device type is not LFP, continue.
 		 * We have to check both the new identifiers as well as the
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index f2c6d79..78631a2 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1510,7 +1510,7 @@ static const struct drm_encoder_funcs intel_tv_enc_funcs = {
 static int tv_is_present_in_vbt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct child_device_config *p_child;
+	union child_device_config *p_child;
 	int i, ret;
 
 	if (!dev_priv->vbt.child_dev_num)
@@ -1522,13 +1522,13 @@ static int tv_is_present_in_vbt(struct drm_device *dev)
 		/*
 		 * If the device type is not TV, continue.
 		 */
-		if (p_child->device_type != DEVICE_TYPE_INT_TV &&
-			p_child->device_type != DEVICE_TYPE_TV)
+		if (p_child->old.device_type != DEVICE_TYPE_INT_TV &&
+			p_child->old.device_type != DEVICE_TYPE_TV)
 			continue;
 		/* Only when the addin_offset is non-zero, it is regarded
 		 * as present.
 		 */
-		if (p_child->addin_offset) {
+		if (p_child->old.addin_offset) {
 			ret = 1;
 			break;
 		}
-- 
1.8.3.1

[PATCH 2/5] drm/i915: use the HDMI DDI buffer translations from VBT

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We currently use the recommended values from BSpec, but the VBT
specifies the correct value to use for the hardware we have, so use
it. We also fall back to the recommended value in case we can't find
the VBT.

In addition, this code also provides some infrastructure to parse more
information about the DDI ports. There's a lot more information we
could extract and use in the future.

v2: - Move some code to init_vbt_defaults.
v3: - Rebase
    - Clarify the "DVO Port" matching code

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  6 +++
 drivers/gpu/drm/i915/intel_bios.c | 78 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h | 13 +++++++
 drivers/gpu/drm/i915/intel_ddi.c  | 24 +++++++++++-
 4 files changed, 119 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca8856a..298c671 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1052,6 +1052,10 @@ enum modeset_restore {
 	MODESET_SUSPENDED,
 };
 
+struct ddi_vbt_port_info {
+	uint8_t hdmi_level_shift;
+};
+
 struct intel_vbt_data {
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1086,6 +1090,8 @@ struct intel_vbt_data {
 
 	int child_dev_num;
 	union child_device_config *child_dev;
+
+	struct ddi_vbt_port_info ddi_port_info[5];
 };
 
 enum intel_ddb_partitioning {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 33003b9..d7ff8fc 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -583,6 +583,72 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 	dev_priv->vbt.dsi.panel_id = mipi->panel_id;
 }
 
+static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
+			   struct bdb_header *bdb)
+{
+	union child_device_config *it, *child = NULL;
+	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
+	uint8_t hdmi_level_shift;
+	int i, j;
+	/* Each DDI port can have more than one value on the "DVO Port" field,
+	 * so look for all the possible values for each port and abort if more
+	 * than one is found. */
+	int dvo_ports[][2] = {
+		{DVO_PORT_HDMIA, DVO_PORT_DPA},
+		{DVO_PORT_HDMIB, DVO_PORT_DPB},
+		{DVO_PORT_HDMIC, DVO_PORT_DPC},
+		{DVO_PORT_HDMID, DVO_PORT_DPD},
+		{DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
+	};
+
+	/* Find the child device to use, abort if more than one found. */
+	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
+		it = dev_priv->vbt.child_dev + i;
+
+		for (j = 0; j < 2; j++) {
+			if (dvo_ports[port][j] == -1)
+				break;
+
+			if (it->common.dvo_port == dvo_ports[port][j]) {
+				if (child) {
+					DRM_ERROR("More than one child device for port %c in VBT.\n",
+						   port_name(port));
+					return;
+				}
+				child = it;
+			}
+		}
+	}
+	if (!child)
+		return;
+
+	if (bdb->version >= 158) {
+		/* The VBT HDMI level shift values match the table we have. */
+		hdmi_level_shift = child->raw[7] & 0xF;
+		if (hdmi_level_shift < 0xC) {
+			DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
+				      port_name(port),
+				      hdmi_level_shift);
+			info->hdmi_level_shift = hdmi_level_shift;
+		}
+	}
+}
+
+static void parse_ddi_ports(struct drm_i915_private *dev_priv,
+			    struct bdb_header *bdb)
+{
+	enum port port;
+
+	if (!dev_priv->vbt.child_dev_num)
+		return;
+
+	if (bdb->version < 155)
+		return;
+
+	for (port = PORT_A; port <= PORT_E; port++)
+		parse_ddi_port(dev_priv, port, bdb);
+}
+
 static void
 parse_device_mapping(struct drm_i915_private *dev_priv,
 		       struct bdb_header *bdb)
@@ -670,6 +736,16 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	dev_priv->vbt.lvds_use_ssc = 1;
 	dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
 	DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
+
+	if (HAS_DDI(dev)) {
+		enum port port;
+
+		for (port = PORT_A; port <= PORT_E; port++) {
+			/* Recommended BSpec default: 800mV 0dB. */
+			dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
+		}
+	}
+
 }
 
 static int __init intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
@@ -761,6 +837,8 @@ intel_parse_bios(struct drm_device *dev)
 	parse_driver_features(dev_priv, bdb);
 	parse_edp(dev_priv, bdb);
 	parse_mipi(dev_priv, bdb);
+	if (HAS_DDI(dev))
+		parse_ddi_ports(dev_priv, bdb);
 
 	if (bios)
 		pci_unmap_rom(pdev, bios);
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 1da2bf2..287cc5a 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -648,6 +648,19 @@ int intel_parse_bios(struct drm_device *dev);
 #define		PORT_IDPC	8
 #define		PORT_IDPD	9
 
+/* Possible values for the "DVO Port" field for versions >= 155: */
+#define DVO_PORT_HDMIA	0
+#define DVO_PORT_HDMIB	1
+#define DVO_PORT_HDMIC	2
+#define DVO_PORT_HDMID	3
+#define DVO_PORT_LVDS	4
+#define DVO_PORT_TV	5
+#define DVO_PORT_CRT	6
+#define DVO_PORT_DPB	7
+#define DVO_PORT_DPC	8
+#define DVO_PORT_DPD	9
+#define DVO_PORT_DPA	10
+
 /* MIPI DSI panel info */
 struct bdb_mipi {
 	u16 panel_id;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 6082ab2..a9887eb 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -42,7 +42,6 @@ static const u32 hsw_ddi_translations_dp[] = {
 	0x80C30FFF, 0x000B0000,
 	0x00FFFFFF, 0x00040006,
 	0x80D75FFF, 0x000B0000,
-	0x00FFFFFF, 0x00040006		/* HDMI parameters */
 };
 
 static const u32 hsw_ddi_translations_fdi[] = {
@@ -55,7 +54,22 @@ static const u32 hsw_ddi_translations_fdi[] = {
 	0x00C30FFF, 0x001E0000,
 	0x00FFFFFF, 0x00060006,
 	0x00D75FFF, 0x001E0000,
-	0x00FFFFFF, 0x00040006		/* HDMI parameters */
+};
+
+static const u32 hsw_ddi_translations_hdmi[] = {
+				/* Idx	NT mV diff	T mV diff	db  */
+	0x00FFFFFF, 0x0006000E, /* 0:	400		400		0   */
+	0x00E79FFF, 0x000E000C, /* 1:	400		500		2   */
+	0x00D75FFF, 0x0005000A, /* 2:	400		600		3.5 */
+	0x00FFFFFF, 0x0005000A, /* 3:	600		600		0   */
+	0x00E79FFF, 0x001D0007, /* 4:	600		750		2   */
+	0x00D75FFF, 0x000C0004, /* 5:	600		900		3.5 */
+	0x00FFFFFF, 0x00040006, /* 6:	800		800		0   */
+	0x80E79FFF, 0x00030002, /* 7:	800		1000		2   */
+	0x00FFFFFF, 0x00140005, /* 8:	850		850		0   */
+	0x00FFFFFF, 0x000C0004, /* 9:	900		900		0   */
+	0x00FFFFFF, 0x001C0003, /* 10:	950		950		0   */
+	0x80FFFFFF, 0x00030002, /* 11:	1000		1000		0   */
 };
 
 enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
@@ -92,12 +106,18 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port)
 	const u32 *ddi_translations = (port == PORT_E) ?
 		hsw_ddi_translations_fdi :
 		hsw_ddi_translations_dp;
+	int hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
 
 	for (i = 0, reg = DDI_BUF_TRANS(port);
 	     i < ARRAY_SIZE(hsw_ddi_translations_fdi); i++) {
 		I915_WRITE(reg, ddi_translations[i]);
 		reg += 4;
 	}
+	/* Entry 9 is for HDMI: */
+	for (i = 0; i < 2; i++) {
+		I915_WRITE(reg, hsw_ddi_translations_hdmi[hdmi_level * 2 + i]);
+		reg += 4;
+	}
 }
 
 /* Program DDI buffers translations for DP. By default, program ports A-D in DP
-- 
1.8.3.1

[PATCH 3/5] drm/i915: check the DDC and AUX bits of the VBT on DDI machines

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Our code currently assumes that port X will use the DP AUX channel X
and the DDC pin X. The VBT should tell us how things are mapped, so
add some WARNs in case we discover our assumptions are wrong (or in
case the VBT is just wrong, which is also perfectly possible).

Why would someone wire port B to AUX C and DDC D?

v2: Rebase

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v1)
---
 drivers/gpu/drm/i915/intel_bios.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index d7ff8fc..4a1f12a 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -590,6 +590,8 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
 	uint8_t hdmi_level_shift;
 	int i, j;
+	bool is_dvi, is_dp;
+	uint8_t aux_channel;
 	/* Each DDI port can have more than one value on the "DVO Port" field,
 	 * so look for all the possible values for each port and abort if more
 	 * than one is found. */
@@ -622,6 +624,31 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	if (!child)
 		return;
 
+	aux_channel = child->raw[25];
+
+	is_dvi = child->common.device_type & (1 << 4);
+	is_dp = child->common.device_type & (1 << 2);
+
+	if (is_dvi) {
+		WARN(child->common.ddc_pin == 0x05 && port != PORT_B,
+		     "Unexpected DDC pin for port B\n");
+		WARN(child->common.ddc_pin == 0x04 && port != PORT_C,
+		     "Unexpected DDC pin for port C\n");
+		WARN(child->common.ddc_pin == 0x06 && port != PORT_D,
+		     "Unexpected DDC pin for port D\n");
+	}
+
+	if (is_dp) {
+		WARN(aux_channel == 0x40 && port != PORT_A,
+		     "Unexpected AUX channel for port A\n");
+		WARN(aux_channel == 0x10 && port != PORT_B,
+		     "Unexpected AUX channel for port B\n");
+		WARN(aux_channel == 0x20 && port != PORT_C,
+		     "Unexpected AUX channel for port C\n");
+		WARN(aux_channel == 0x30 && port != PORT_D,
+		     "Unexpected AUX channel for port D\n");
+	}
+
 	if (bdb->version >= 158) {
 		/* The VBT HDMI level shift values match the table we have. */
 		hdmi_level_shift = child->raw[7] & 0xF;
-- 
1.8.3.1

[PATCH 4/5] drm/i915: add some assertions about VBT DDI port types

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Our code makes a lot of assumptions regarding what each DDI port
actually supports, and the VBT should tell us what is really happening
in the hardware. So parse the information provided by the VBT and
check if any of our assumptions is wrong.

Our driver also has a history of not really trusting the VBT, so a
WARN here could mean that:
 a) our coding assumptions are wrong
 b) the VBT is wrong
 c) we're incorrectly parsing the VBT
 d) the checks are wrong

But I really hope we won't ever trigger any of those WARNs.

v2: Don't check the redundant "Capabilities" field from byte 24 since
    it doesn't seem to be used.
v3: Rebase

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v2)
---
 drivers/gpu/drm/i915/intel_bios.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 4a1f12a..be07977 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -590,7 +590,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
 	uint8_t hdmi_level_shift;
 	int i, j;
-	bool is_dvi, is_dp;
+	bool is_dvi, is_hdmi, is_dp, is_edp, is_crt;
 	uint8_t aux_channel;
 	/* Each DDI port can have more than one value on the "DVO Port" field,
 	 * so look for all the possible values for each port and abort if more
@@ -628,6 +628,24 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 
 	is_dvi = child->common.device_type & (1 << 4);
 	is_dp = child->common.device_type & (1 << 2);
+	is_crt = child->common.device_type & (1 << 0);
+	is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
+	is_edp = is_dp && (child->common.device_type & (1 << 12));
+
+	DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
+		      port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
+
+	WARN(is_edp && is_dvi, "Internal DP port %c is TMDS compatible\n",
+	     port_name(port));
+	WARN(is_crt && port != PORT_E, "Port %c is analog\n", port_name(port));
+	WARN(is_crt && (is_dvi || is_dp),
+	     "Analog port %c is also DP or TMDS compatible\n", port_name(port));
+	WARN(is_dvi && (port == PORT_A || port == PORT_E),
+	     "Port %c is TMDS compabile\n", port_name(port));
+	WARN(!is_dvi && !is_dp && !is_crt,
+	     "Port %c is not DP/TMDS/CRT compatible\n", port_name(port));
+	WARN(is_edp && (port == PORT_B || port == PORT_C || port == PORT_E),
+	     "Port %c is internal DP\n", port_name(port));
 
 	if (is_dvi) {
 		WARN(child->common.ddc_pin == 0x05 && port != PORT_B,
-- 
1.8.3.1

[PATCH 5/5] drm/i915: don't init DP or HDMI when not supported by DDI port

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

There's no reason to init a DP connector if the encoder just supports
HDMI: we'll just waste hundreds and hundreds of cycles trying to do DP
AUX transactions to detect if there's something there. Same goes for a
DP connector that doesn't support HDMI, but I'm not sure these
actually exist.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  3 +++
 drivers/gpu/drm/i915/intel_bios.c | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_ddi.c  | 30 ++++++++++++++++++++++--------
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 298c671..8533bf1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1054,6 +1054,9 @@ enum modeset_restore {
 
 struct ddi_vbt_port_info {
 	uint8_t hdmi_level_shift;
+	bool supports_dvi;
+	bool supports_hdmi;
+	bool supports_dp;
 };
 
 struct intel_vbt_data {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index be07977..9aee153 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -632,6 +632,10 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
 	is_edp = is_dp && (child->common.device_type & (1 << 12));
 
+	info->supports_dvi = is_dvi;
+	info->supports_hdmi = is_hdmi;
+	info->supports_dp = is_dp;
+
 	DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
 		      port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
 
@@ -786,8 +790,15 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 		enum port port;
 
 		for (port = PORT_A; port <= PORT_E; port++) {
+			struct ddi_vbt_port_info *info =
+				&dev_priv->vbt.ddi_port_info[port];
+
 			/* Recommended BSpec default: 800mV 0dB. */
-			dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
+			info->hdmi_level_shift = 6;
+
+			info->supports_dvi = (port != PORT_A && port != PORT_E);
+			info->supports_hdmi = info->supports_dvi;
+			info->supports_dp = (port != PORT_E);
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a9887eb..df7f75c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1325,6 +1325,17 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	struct drm_encoder *encoder;
 	struct intel_connector *hdmi_connector = NULL;
 	struct intel_connector *dp_connector = NULL;
+	bool init_hdmi, init_dp;
+
+	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
+		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
+	init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
+	if (!init_dp && !init_hdmi) {
+		DRM_ERROR("VBT says port %c is not DVI/HDMI/DP compatible\n",
+			  port_name(port));
+		init_hdmi = true;
+		init_dp = true;
+	}
 
 	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
 	if (!intel_dig_port)
@@ -1362,19 +1373,22 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->cloneable = false;
 	intel_encoder->hot_plug = intel_ddi_hot_plug;
 
-	if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
-		drm_encoder_cleanup(encoder);
-		kfree(intel_dig_port);
-		kfree(dp_connector);
-		return;
+	if (init_dp) {
+		if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
+			drm_encoder_cleanup(encoder);
+			kfree(intel_dig_port);
+			kfree(dp_connector);
+			return;
+		}
 	}
 
-	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
+	/* In theory we don't need the encoder->type check, but leave it just in
+	 * case we have some really bad VBTs... */
+	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		hdmi_connector = kzalloc(sizeof(struct intel_connector),
 					 GFP_KERNEL);
-		if (!hdmi_connector) {
+		if (!hdmi_connector)
 			return;
-		}
 
 		intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
 		intel_hdmi_init_connector(intel_dig_port, hdmi_connector);
-- 
1.8.3.1

Re: [PATCH 5/5] drm/i915: don't init DP or HDMI when not supported by DDI port

[Chris Wilson]

On Wed, Sep 11, 2013 at 06:02:51PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> There's no reason to init a DP connector if the encoder just supports
> HDMI: we'll just waste hundreds and hundreds of cycles trying to do DP
> AUX transactions to detect if there's something there. Same goes for a
> DP connector that doesn't support HDMI, but I'm not sure these
> actually exist.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>  drivers/gpu/drm/i915/intel_bios.c | 13 ++++++++++++-
>  drivers/gpu/drm/i915/intel_ddi.c  | 30 ++++++++++++++++++++++--------
>  3 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 298c671..8533bf1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1054,6 +1054,9 @@ enum modeset_restore {
>  
>  struct ddi_vbt_port_info {
>  	uint8_t hdmi_level_shift;
> +	bool supports_dvi;
> +	bool supports_hdmi;
> +	bool supports_dp;

Might as well make these uint8_t:1;

> -	if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
> -		drm_encoder_cleanup(encoder);
> -		kfree(intel_dig_port);
> -		kfree(dp_connector);
> -		return;
> +	if (init_dp) {
> +		if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
> +			drm_encoder_cleanup(encoder);
> +			kfree(intel_dig_port);
> +			kfree(dp_connector);
> +			return;
> +		}
>  	}

You are mixing styles here. Elsewhere with init_hdmi you have, thankfully,
opted to eliminated the extra level of useless indentation. So please
use if (init_dp && !intel_dp_init_connector()).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: check the DDC and AUX bits of the VBT on DDI machines

[Chris Wilson]

On Wed, Sep 11, 2013 at 06:02:49PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Our code currently assumes that port X will use the DP AUX channel X
> and the DDC pin X. The VBT should tell us how things are mapped, so
> add some WARNs in case we discover our assumptions are wrong (or in
> case the VBT is just wrong, which is also perfectly possible).
> 
> Why would someone wire port B to AUX C and DDC D?
> 
> v2: Rebase

A WARN isn't going to be very helpful, these calls can only occur in a
single callpath, so there is is no confusion. Also it is not *ERROR*
worthy if the VBT is broken, we just get to pick up the pieces.

These are only DEBUG class warnings. If you want to get fancy, you can
put a "told you so" *ERROR* after it doesn't work.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: add some assertions about VBT DDI port types

[Chris Wilson]

On Wed, Sep 11, 2013 at 06:02:50PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Our code makes a lot of assumptions regarding what each DDI port
> actually supports, and the VBT should tell us what is really happening
> in the hardware. So parse the information provided by the VBT and
> check if any of our assumptions is wrong.
> 
> Our driver also has a history of not really trusting the VBT, so a
> WARN here could mean that:
>  a) our coding assumptions are wrong
>  b) the VBT is wrong
>  c) we're incorrectly parsing the VBT
>  d) the checks are wrong
> 
> But I really hope we won't ever trigger any of those WARNs.
> 
> v2: Don't check the redundant "Capabilities" field from byte 24 since
>     it doesn't seem to be used.
> v3: Rebase
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v2)

I have the same issue with these WARN - single callsite, no need for
disambiguation, and things we need to handle rather than bamboozle the
user. (Or if we can't, *ERROR* when it all goes wrong.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: use the HDMI DDI buffer translations from VBT

[Chris Wilson]

On Wed, Sep 11, 2013 at 06:02:48PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We currently use the recommended values from BSpec, but the VBT
> specifies the correct value to use for the hardware we have, so use
> it. We also fall back to the recommended value in case we can't find
> the VBT.
> 
> In addition, this code also provides some infrastructure to parse more
> information about the DDI ports. There's a lot more information we
> could extract and use in the future.
> 
> v2: - Move some code to init_vbt_defaults.
> v3: - Rebase
>     - Clarify the "DVO Port" matching code
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  6 +++
>  drivers/gpu/drm/i915/intel_bios.c | 78 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_bios.h | 13 +++++++
>  drivers/gpu/drm/i915/intel_ddi.c  | 24 +++++++++++-
>  4 files changed, 119 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca8856a..298c671 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1052,6 +1052,10 @@ enum modeset_restore {
>  	MODESET_SUSPENDED,
>  };
>  
> +struct ddi_vbt_port_info {
> +	uint8_t hdmi_level_shift;
> +};
> +
>  struct intel_vbt_data {
>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
> @@ -1086,6 +1090,8 @@ struct intel_vbt_data {
>  
>  	int child_dev_num;
>  	union child_device_config *child_dev;
> +
> +	struct ddi_vbt_port_info ddi_port_info[5];

s/5/PORT_E+1/ or better #define NUM_DDI_PORTS PORT_E+1.

>  };
>  
>  enum intel_ddb_partitioning {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 33003b9..d7ff8fc 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -583,6 +583,72 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  	dev_priv->vbt.dsi.panel_id = mipi->panel_id;
>  }
>  
> +static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> +			   struct bdb_header *bdb)
> +{
> +	union child_device_config *it, *child = NULL;
> +	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
> +	uint8_t hdmi_level_shift;
> +	int i, j;
> +	/* Each DDI port can have more than one value on the "DVO Port" field,
> +	 * so look for all the possible values for each port and abort if more
> +	 * than one is found. */
> +	int dvo_ports[][2] = {
> +		{DVO_PORT_HDMIA, DVO_PORT_DPA},
> +		{DVO_PORT_HDMIB, DVO_PORT_DPB},
> +		{DVO_PORT_HDMIC, DVO_PORT_DPC},
> +		{DVO_PORT_HDMID, DVO_PORT_DPD},
> +		{DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
> +	};
> +

I don't see the point of calling this function if bdb->version < 158 ?

> +	/* Find the child device to use, abort if more than one found. */
> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> +		it = dev_priv->vbt.child_dev + i;
> +
> +		for (j = 0; j < 2; j++) {
> +			if (dvo_ports[port][j] == -1)
> +				break;
> +
> +			if (it->common.dvo_port == dvo_ports[port][j]) {
> +				if (child) {
> +					DRM_ERROR("More than one child device for port %c in VBT.\n",
> +						   port_name(port));
> +					return;
> +				}
> +				child = it;
> +			}
> +		}
> +	}
> +	if (!child)
> +		return;
> +
> +	if (bdb->version >= 158) {
> +		/* The VBT HDMI level shift values match the table we have. */
> +		hdmi_level_shift = child->raw[7] & 0xF;
> +		if (hdmi_level_shift < 0xC) {
> +			DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
> +				      port_name(port),
> +				      hdmi_level_shift);
> +			info->hdmi_level_shift = hdmi_level_shift;
> +		}
> +	}
> +}
> +
> +static void parse_ddi_ports(struct drm_i915_private *dev_priv,
> +			    struct bdb_header *bdb)
> +{
> +	enum port port;
> +
> +	if (!dev_priv->vbt.child_dev_num)
> +		return;
> +
> +	if (bdb->version < 155)
> +		return;
> +
> +	for (port = PORT_A; port <= PORT_E; port++)
> +		parse_ddi_port(dev_priv, port, bdb);
> +}
> +
>  static void
>  parse_device_mapping(struct drm_i915_private *dev_priv,
>  		       struct bdb_header *bdb)
> @@ -670,6 +736,16 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
>  	dev_priv->vbt.lvds_use_ssc = 1;
>  	dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
>  	DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
> +
> +	if (HAS_DDI(dev)) {

Might as well just set the defaults anyway. I only really care if the
recommendations change between generations.

> +		enum port port;
> +
> +		for (port = PORT_A; port <= PORT_E; port++) {
> +			/* Recommended BSpec default: 800mV 0dB. */
> +			dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
> +		}
> +	}
> +
>  }
>  
>  static int __init intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
> @@ -761,6 +837,8 @@ intel_parse_bios(struct drm_device *dev)
>  	parse_driver_features(dev_priv, bdb);
>  	parse_edp(dev_priv, bdb);
>  	parse_mipi(dev_priv, bdb);
> +	if (HAS_DDI(dev))
> +		parse_ddi_ports(dev_priv, bdb);

Move the check here into parse_ddi_ports() so the calling sequence is
clear.

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: use the HDMI DDI buffer translations from VBT

[Paulo Zanoni]

2013/9/11 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Sep 11, 2013 at 06:02:48PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> We currently use the recommended values from BSpec, but the VBT
>> specifies the correct value to use for the hardware we have, so use
>> it. We also fall back to the recommended value in case we can't find
>> the VBT.
>>
>> In addition, this code also provides some infrastructure to parse more
>> information about the DDI ports. There's a lot more information we
>> could extract and use in the future.
>>
>> v2: - Move some code to init_vbt_defaults.
>> v3: - Rebase
>>     - Clarify the "DVO Port" matching code
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |  6 +++
>>  drivers/gpu/drm/i915/intel_bios.c | 78 +++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_bios.h | 13 +++++++
>>  drivers/gpu/drm/i915/intel_ddi.c  | 24 +++++++++++-
>>  4 files changed, 119 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index ca8856a..298c671 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1052,6 +1052,10 @@ enum modeset_restore {
>>       MODESET_SUSPENDED,
>>  };
>>
>> +struct ddi_vbt_port_info {
>> +     uint8_t hdmi_level_shift;
>> +};
>> +
>>  struct intel_vbt_data {
>>       struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>>       struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
>> @@ -1086,6 +1090,8 @@ struct intel_vbt_data {
>>
>>       int child_dev_num;
>>       union child_device_config *child_dev;
>> +
>> +     struct ddi_vbt_port_info ddi_port_info[5];
>
> s/5/PORT_E+1/ or better #define NUM_DDI_PORTS PORT_E+1.

Looks like I can just use the already-defined I915_MAX_PORTS. It also
matches the I915_MAX_PIPES usage.


>
>>  };
>>
>>  enum intel_ddb_partitioning {
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 33003b9..d7ff8fc 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -583,6 +583,72 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>>       dev_priv->vbt.dsi.panel_id = mipi->panel_id;
>>  }
>>
>> +static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>> +                        struct bdb_header *bdb)
>> +{
>> +     union child_device_config *it, *child = NULL;
>> +     struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
>> +     uint8_t hdmi_level_shift;
>> +     int i, j;
>> +     /* Each DDI port can have more than one value on the "DVO Port" field,
>> +      * so look for all the possible values for each port and abort if more
>> +      * than one is found. */
>> +     int dvo_ports[][2] = {
>> +             {DVO_PORT_HDMIA, DVO_PORT_DPA},
>> +             {DVO_PORT_HDMIB, DVO_PORT_DPB},
>> +             {DVO_PORT_HDMIC, DVO_PORT_DPC},
>> +             {DVO_PORT_HDMID, DVO_PORT_DPD},
>> +             {DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
>> +     };
>> +
>
> I don't see the point of calling this function if bdb->version < 158 ?

I built the whole function as a single patch, then I split it in 5
later, and chose this chunk to be the first since it was more
important than the others. So this patch looks weird, but things make
more sense if you see the next patches.


>
>> +     /* Find the child device to use, abort if more than one found. */
>> +     for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> +             it = dev_priv->vbt.child_dev + i;
>> +
>> +             for (j = 0; j < 2; j++) {
>> +                     if (dvo_ports[port][j] == -1)
>> +                             break;
>> +
>> +                     if (it->common.dvo_port == dvo_ports[port][j]) {
>> +                             if (child) {
>> +                                     DRM_ERROR("More than one child device for port %c in VBT.\n",
>> +                                                port_name(port));
>> +                                     return;
>> +                             }
>> +                             child = it;
>> +                     }
>> +             }
>> +     }
>> +     if (!child)
>> +             return;
>> +
>> +     if (bdb->version >= 158) {
>> +             /* The VBT HDMI level shift values match the table we have. */
>> +             hdmi_level_shift = child->raw[7] & 0xF;
>> +             if (hdmi_level_shift < 0xC) {
>> +                     DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
>> +                                   port_name(port),
>> +                                   hdmi_level_shift);
>> +                     info->hdmi_level_shift = hdmi_level_shift;
>> +             }
>> +     }
>> +}
>> +
>> +static void parse_ddi_ports(struct drm_i915_private *dev_priv,
>> +                         struct bdb_header *bdb)
>> +{
>> +     enum port port;
>> +
>> +     if (!dev_priv->vbt.child_dev_num)
>> +             return;
>> +
>> +     if (bdb->version < 155)
>> +             return;
>> +
>> +     for (port = PORT_A; port <= PORT_E; port++)
>> +             parse_ddi_port(dev_priv, port, bdb);
>> +}
>> +
>>  static void
>>  parse_device_mapping(struct drm_i915_private *dev_priv,
>>                      struct bdb_header *bdb)
>> @@ -670,6 +736,16 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
>>       dev_priv->vbt.lvds_use_ssc = 1;
>>       dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
>>       DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
>> +
>> +     if (HAS_DDI(dev)) {
>
> Might as well just set the defaults anyway. I only really care if the
> recommendations change between generations.

Done.

>
>> +             enum port port;
>> +
>> +             for (port = PORT_A; port <= PORT_E; port++) {
>> +                     /* Recommended BSpec default: 800mV 0dB. */
>> +                     dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
>> +             }
>> +     }
>> +
>>  }
>>
>>  static int __init intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
>> @@ -761,6 +837,8 @@ intel_parse_bios(struct drm_device *dev)
>>       parse_driver_features(dev_priv, bdb);
>>       parse_edp(dev_priv, bdb);
>>       parse_mipi(dev_priv, bdb);
>> +     if (HAS_DDI(dev))
>> +             parse_ddi_ports(dev_priv, bdb);
>
> Move the check here into parse_ddi_ports() so the calling sequence is
> clear.

Done.

Thanks for the reviews!

>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

[PATCH 2/5] drm/i915: use the HDMI DDI buffer translations from VBT

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We currently use the recommended values from BSpec, but the VBT
specifies the correct value to use for the hardware we have, so use
it. We also fall back to the recommended value in case we can't find
the VBT.

In addition, this code also provides some infrastructure to parse more
information about the DDI ports. There's a lot more information we
could extract and use in the future.

v2: - Move some code to init_vbt_defaults.
v3: - Rebase
    - Clarify the "DVO Port" matching code
v4: - Use I915_MAX_PORTS
    - Change the HAS_DDI checks
    - Replace DRM_ERROR with DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  6 +++
 drivers/gpu/drm/i915/intel_bios.c | 77 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h | 13 +++++++
 drivers/gpu/drm/i915/intel_ddi.c  | 24 +++++++++++-
 4 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca8856a..f159df0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1052,6 +1052,10 @@ enum modeset_restore {
 	MODESET_SUSPENDED,
 };
 
+struct ddi_vbt_port_info {
+	uint8_t hdmi_level_shift;
+};
+
 struct intel_vbt_data {
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1086,6 +1090,8 @@ struct intel_vbt_data {
 
 	int child_dev_num;
 	union child_device_config *child_dev;
+
+	struct ddi_vbt_port_info ddi_port_info[I915_MAX_PORTS];
 };
 
 enum intel_ddb_partitioning {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 33003b9..2f43429 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -583,6 +583,76 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 	dev_priv->vbt.dsi.panel_id = mipi->panel_id;
 }
 
+static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
+			   struct bdb_header *bdb)
+{
+	union child_device_config *it, *child = NULL;
+	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
+	uint8_t hdmi_level_shift;
+	int i, j;
+	/* Each DDI port can have more than one value on the "DVO Port" field,
+	 * so look for all the possible values for each port and abort if more
+	 * than one is found. */
+	int dvo_ports[][2] = {
+		{DVO_PORT_HDMIA, DVO_PORT_DPA},
+		{DVO_PORT_HDMIB, DVO_PORT_DPB},
+		{DVO_PORT_HDMIC, DVO_PORT_DPC},
+		{DVO_PORT_HDMID, DVO_PORT_DPD},
+		{DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
+	};
+
+	/* Find the child device to use, abort if more than one found. */
+	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
+		it = dev_priv->vbt.child_dev + i;
+
+		for (j = 0; j < 2; j++) {
+			if (dvo_ports[port][j] == -1)
+				break;
+
+			if (it->common.dvo_port == dvo_ports[port][j]) {
+				if (child) {
+					DRM_DEBUG_KMS("More than one child device for port %c in VBT.\n",
+						      port_name(port));
+					return;
+				}
+				child = it;
+			}
+		}
+	}
+	if (!child)
+		return;
+
+	if (bdb->version >= 158) {
+		/* The VBT HDMI level shift values match the table we have. */
+		hdmi_level_shift = child->raw[7] & 0xF;
+		if (hdmi_level_shift < 0xC) {
+			DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
+				      port_name(port),
+				      hdmi_level_shift);
+			info->hdmi_level_shift = hdmi_level_shift;
+		}
+	}
+}
+
+static void parse_ddi_ports(struct drm_i915_private *dev_priv,
+			    struct bdb_header *bdb)
+{
+	struct drm_device *dev = dev_priv->dev;
+	enum port port;
+
+	if (!HAS_DDI(dev))
+		return;
+
+	if (!dev_priv->vbt.child_dev_num)
+		return;
+
+	if (bdb->version < 155)
+		return;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++)
+		parse_ddi_port(dev_priv, port, bdb);
+}
+
 static void
 parse_device_mapping(struct drm_i915_private *dev_priv,
 		       struct bdb_header *bdb)
@@ -652,6 +722,7 @@ static void
 init_vbt_defaults(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
+	enum port port;
 
 	dev_priv->vbt.crt_ddc_pin = GMBUS_PORT_VGADDC;
 
@@ -670,6 +741,11 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	dev_priv->vbt.lvds_use_ssc = 1;
 	dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
 	DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		/* Recommended BSpec default: 800mV 0dB. */
+		dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
+	}
 }
 
 static int __init intel_no_opregion_vbt_callback(const struct dmi_system_id *id)
@@ -761,6 +837,7 @@ intel_parse_bios(struct drm_device *dev)
 	parse_driver_features(dev_priv, bdb);
 	parse_edp(dev_priv, bdb);
 	parse_mipi(dev_priv, bdb);
+	parse_ddi_ports(dev_priv, bdb);
 
 	if (bios)
 		pci_unmap_rom(pdev, bios);
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 1da2bf2..287cc5a 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -648,6 +648,19 @@ int intel_parse_bios(struct drm_device *dev);
 #define		PORT_IDPC	8
 #define		PORT_IDPD	9
 
+/* Possible values for the "DVO Port" field for versions >= 155: */
+#define DVO_PORT_HDMIA	0
+#define DVO_PORT_HDMIB	1
+#define DVO_PORT_HDMIC	2
+#define DVO_PORT_HDMID	3
+#define DVO_PORT_LVDS	4
+#define DVO_PORT_TV	5
+#define DVO_PORT_CRT	6
+#define DVO_PORT_DPB	7
+#define DVO_PORT_DPC	8
+#define DVO_PORT_DPD	9
+#define DVO_PORT_DPA	10
+
 /* MIPI DSI panel info */
 struct bdb_mipi {
 	u16 panel_id;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 6082ab2..a9887eb 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -42,7 +42,6 @@ static const u32 hsw_ddi_translations_dp[] = {
 	0x80C30FFF, 0x000B0000,
 	0x00FFFFFF, 0x00040006,
 	0x80D75FFF, 0x000B0000,
-	0x00FFFFFF, 0x00040006		/* HDMI parameters */
 };
 
 static const u32 hsw_ddi_translations_fdi[] = {
@@ -55,7 +54,22 @@ static const u32 hsw_ddi_translations_fdi[] = {
 	0x00C30FFF, 0x001E0000,
 	0x00FFFFFF, 0x00060006,
 	0x00D75FFF, 0x001E0000,
-	0x00FFFFFF, 0x00040006		/* HDMI parameters */
+};
+
+static const u32 hsw_ddi_translations_hdmi[] = {
+				/* Idx	NT mV diff	T mV diff	db  */
+	0x00FFFFFF, 0x0006000E, /* 0:	400		400		0   */
+	0x00E79FFF, 0x000E000C, /* 1:	400		500		2   */
+	0x00D75FFF, 0x0005000A, /* 2:	400		600		3.5 */
+	0x00FFFFFF, 0x0005000A, /* 3:	600		600		0   */
+	0x00E79FFF, 0x001D0007, /* 4:	600		750		2   */
+	0x00D75FFF, 0x000C0004, /* 5:	600		900		3.5 */
+	0x00FFFFFF, 0x00040006, /* 6:	800		800		0   */
+	0x80E79FFF, 0x00030002, /* 7:	800		1000		2   */
+	0x00FFFFFF, 0x00140005, /* 8:	850		850		0   */
+	0x00FFFFFF, 0x000C0004, /* 9:	900		900		0   */
+	0x00FFFFFF, 0x001C0003, /* 10:	950		950		0   */
+	0x80FFFFFF, 0x00030002, /* 11:	1000		1000		0   */
 };
 
 enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
@@ -92,12 +106,18 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port)
 	const u32 *ddi_translations = (port == PORT_E) ?
 		hsw_ddi_translations_fdi :
 		hsw_ddi_translations_dp;
+	int hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
 
 	for (i = 0, reg = DDI_BUF_TRANS(port);
 	     i < ARRAY_SIZE(hsw_ddi_translations_fdi); i++) {
 		I915_WRITE(reg, ddi_translations[i]);
 		reg += 4;
 	}
+	/* Entry 9 is for HDMI: */
+	for (i = 0; i < 2; i++) {
+		I915_WRITE(reg, hsw_ddi_translations_hdmi[hdmi_level * 2 + i]);
+		reg += 4;
+	}
 }
 
 /* Program DDI buffers translations for DP. By default, program ports A-D in DP
-- 
1.8.3.1

[PATCH 3/5] drm/i915: check the DDC and AUX bits of the VBT on DDI machines

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Our code currently assumes that port X will use the DP AUX channel X
and the DDC pin X. The VBT should tell us how things are mapped, so
add some WARNs in case we discover our assumptions are wrong (or in
case the VBT is just wrong, which is also perfectly possible).

Why would someone wire port B to AUX C and DDC D?

v2: Rebase
v3: Convert WARNs to DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v1)
---
 drivers/gpu/drm/i915/intel_bios.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 2f43429..12e4fd1 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -590,6 +590,8 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
 	uint8_t hdmi_level_shift;
 	int i, j;
+	bool is_dvi, is_dp;
+	uint8_t aux_channel;
 	/* Each DDI port can have more than one value on the "DVO Port" field,
 	 * so look for all the possible values for each port and abort if more
 	 * than one is found. */
@@ -622,6 +624,31 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	if (!child)
 		return;
 
+	aux_channel = child->raw[25];
+
+	is_dvi = child->common.device_type & (1 << 4);
+	is_dp = child->common.device_type & (1 << 2);
+
+	if (is_dvi) {
+		if (child->common.ddc_pin == 0x05 && port != PORT_B)
+			DRM_DEBUG_KMS("Unexpected DDC pin for port B\n");
+		if (child->common.ddc_pin == 0x04 && port != PORT_C)
+			DRM_DEBUG_KMS("Unexpected DDC pin for port C\n");
+		if (child->common.ddc_pin == 0x06 && port != PORT_D)
+			DRM_DEBUG_KMS("Unexpected DDC pin for port D\n");
+	}
+
+	if (is_dp) {
+		if (aux_channel == 0x40 && port != PORT_A)
+			DRM_DEBUG_KMS("Unexpected AUX channel for port A\n");
+		if (aux_channel == 0x10 && port != PORT_B)
+			DRM_DEBUG_KMS("Unexpected AUX channel for port B\n");
+		if (aux_channel == 0x20 && port != PORT_C)
+			DRM_DEBUG_KMS("Unexpected AUX channel for port C\n");
+		if (aux_channel == 0x30 && port != PORT_D)
+			DRM_DEBUG_KMS("Unexpected AUX channel for port D\n");
+	}
+
 	if (bdb->version >= 158) {
 		/* The VBT HDMI level shift values match the table we have. */
 		hdmi_level_shift = child->raw[7] & 0xF;
-- 
1.8.3.1

[PATCH 4/5] drm/i915: add some assertions about VBT DDI port types

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Our code makes a lot of assumptions regarding what each DDI port
actually supports, and the VBT should tell us what is really happening
in the hardware. So parse the information provided by the VBT and
check if any of our assumptions is wrong.

Our driver also has a history of not really trusting the VBT, so a
WARN here could mean that:
 a) our coding assumptions are wrong
 b) the VBT is wrong
 c) we're incorrectly parsing the VBT
 d) the checks are wrong

But I really hope we won't ever trigger any of those WARNs.

v2: Don't check the redundant "Capabilities" field from byte 24 since
    it doesn't seem to be used.
v3: Rebase
v4: Replace WARN with DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v2)
---
 drivers/gpu/drm/i915/intel_bios.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 12e4fd1..7ce1c3c 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -590,7 +590,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
 	uint8_t hdmi_level_shift;
 	int i, j;
-	bool is_dvi, is_dp;
+	bool is_dvi, is_hdmi, is_dp, is_edp, is_crt;
 	uint8_t aux_channel;
 	/* Each DDI port can have more than one value on the "DVO Port" field,
 	 * so look for all the possible values for each port and abort if more
@@ -628,6 +628,28 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 
 	is_dvi = child->common.device_type & (1 << 4);
 	is_dp = child->common.device_type & (1 << 2);
+	is_crt = child->common.device_type & (1 << 0);
+	is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
+	is_edp = is_dp && (child->common.device_type & (1 << 12));
+
+	DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
+		      port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
+
+	if (is_edp && is_dvi)
+		DRM_DEBUG_KMS("Internal DP port %c is TMDS compatible\n",
+			      port_name(port));
+	if (is_crt && port != PORT_E)
+		DRM_DEBUG_KMS("Port %c is analog\n", port_name(port));
+	if (is_crt && (is_dvi || is_dp))
+		DRM_DEBUG_KMS("Analog port %c is also DP or TMDS compatible\n",
+			      port_name(port));
+	if (is_dvi && (port == PORT_A || port == PORT_E))
+		DRM_DEBUG_KMS("Port %c is TMDS compabile\n", port_name(port));
+	if (!is_dvi && !is_dp && !is_crt)
+		DRM_DEBUG_KMS("Port %c is not DP/TMDS/CRT compatible\n",
+			      port_name(port));
+	if (is_edp && (port == PORT_B || port == PORT_C || port == PORT_E))
+		DRM_DEBUG_KMS("Port %c is internal DP\n", port_name(port));
 
 	if (is_dvi) {
 		if (child->common.ddc_pin == 0x05 && port != PORT_B)
-- 
1.8.3.1

[PATCH 5/5] drm/i915: don't init DP or HDMI when not supported by DDI port

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

There's no reason to init a DP connector if the encoder just supports
HDMI: we'll just waste hundreds and hundreds of cycles trying to do DP
AUX transactions to detect if there's something there. Same goes for a
DP connector that doesn't support HDMI, but I'm not sure these
actually exist.

v2: - Use bit fields
    - Remove useless identation level
    - Replace DRM_ERROR with DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v1)
---
 drivers/gpu/drm/i915/i915_drv.h   |  4 ++++
 drivers/gpu/drm/i915/intel_bios.c | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_ddi.c  | 20 ++++++++++++++++----
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f159df0..dd94731 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1054,6 +1054,10 @@ enum modeset_restore {
 
 struct ddi_vbt_port_info {
 	uint8_t hdmi_level_shift;
+
+	uint8_t supports_dvi:1;
+	uint8_t supports_hdmi:1;
+	uint8_t supports_dp:1;
 };
 
 struct intel_vbt_data {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 7ce1c3c..0492b6f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -632,6 +632,10 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 	is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
 	is_edp = is_dp && (child->common.device_type & (1 << 12));
 
+	info->supports_dvi = is_dvi;
+	info->supports_hdmi = is_hdmi;
+	info->supports_dp = is_dp;
+
 	DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
 		      port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
 
@@ -792,8 +796,15 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
 
 	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];
+
 		/* Recommended BSpec default: 800mV 0dB. */
-		dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
+		info->hdmi_level_shift = 6;
+
+		info->supports_dvi = (port != PORT_A && port != PORT_E);
+		info->supports_hdmi = info->supports_dvi;
+		info->supports_dp = (port != PORT_E);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a9887eb..488f4a4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1325,6 +1325,17 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	struct drm_encoder *encoder;
 	struct intel_connector *hdmi_connector = NULL;
 	struct intel_connector *dp_connector = NULL;
+	bool init_hdmi, init_dp;
+
+	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
+		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
+	init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
+	if (!init_dp && !init_hdmi) {
+		DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible\n",
+			      port_name(port));
+		init_hdmi = true;
+		init_dp = true;
+	}
 
 	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
 	if (!intel_dig_port)
@@ -1362,19 +1373,20 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->cloneable = false;
 	intel_encoder->hot_plug = intel_ddi_hot_plug;
 
-	if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
+	if (init_dp && !intel_dp_init_connector(intel_dig_port, dp_connector)) {
 		drm_encoder_cleanup(encoder);
 		kfree(intel_dig_port);
 		kfree(dp_connector);
 		return;
 	}
 
-	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
+	/* In theory we don't need the encoder->type check, but leave it just in
+	 * case we have some really bad VBTs... */
+	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		hdmi_connector = kzalloc(sizeof(struct intel_connector),
 					 GFP_KERNEL);
-		if (!hdmi_connector) {
+		if (!hdmi_connector)
 			return;
-		}
 
 		intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
 		intel_hdmi_init_connector(intel_dig_port, hdmi_connector);
-- 
1.8.3.1

Re: [PATCH 5/5] drm/i915: don't init DP or HDMI when not supported by DDI port

[Daniel Vetter]

On Thu, Sep 12, 2013 at 05:12:18PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> There's no reason to init a DP connector if the encoder just supports
> HDMI: we'll just waste hundreds and hundreds of cycles trying to do DP
> AUX transactions to detect if there's something there. Same goes for a
> DP connector that doesn't support HDMI, but I'm not sure these
> actually exist.
> 
> v2: - Use bit fields
>     - Remove useless identation level
>     - Replace DRM_ERROR with DRM_DEBUG_KMS
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> (v1)

Slurped in the entire series, thanks for the patches.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  4 ++++
>  drivers/gpu/drm/i915/intel_bios.c | 13 ++++++++++++-
>  drivers/gpu/drm/i915/intel_ddi.c  | 20 ++++++++++++++++----
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f159df0..dd94731 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1054,6 +1054,10 @@ enum modeset_restore {
>  
>  struct ddi_vbt_port_info {
>  	uint8_t hdmi_level_shift;
> +
> +	uint8_t supports_dvi:1;
> +	uint8_t supports_hdmi:1;
> +	uint8_t supports_dp:1;
>  };
>  
>  struct intel_vbt_data {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 7ce1c3c..0492b6f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -632,6 +632,10 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  	is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
>  	is_edp = is_dp && (child->common.device_type & (1 << 12));
>  
> +	info->supports_dvi = is_dvi;
> +	info->supports_hdmi = is_hdmi;
> +	info->supports_dp = is_dp;
> +
>  	DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
>  		      port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
>  
> @@ -792,8 +796,15 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
>  	DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq);
>  
>  	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
> +		struct ddi_vbt_port_info *info =
> +			&dev_priv->vbt.ddi_port_info[port];
> +
>  		/* Recommended BSpec default: 800mV 0dB. */
> -		dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
> +		info->hdmi_level_shift = 6;
> +
> +		info->supports_dvi = (port != PORT_A && port != PORT_E);
> +		info->supports_hdmi = info->supports_dvi;
> +		info->supports_dp = (port != PORT_E);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a9887eb..488f4a4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1325,6 +1325,17 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	struct drm_encoder *encoder;
>  	struct intel_connector *hdmi_connector = NULL;
>  	struct intel_connector *dp_connector = NULL;
> +	bool init_hdmi, init_dp;
> +
> +	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
> +		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
> +	init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
> +	if (!init_dp && !init_hdmi) {
> +		DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP compatible\n",
> +			      port_name(port));
> +		init_hdmi = true;
> +		init_dp = true;
> +	}
>  
>  	intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
>  	if (!intel_dig_port)
> @@ -1362,19 +1373,20 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
>  	intel_encoder->cloneable = false;
>  	intel_encoder->hot_plug = intel_ddi_hot_plug;
>  
> -	if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
> +	if (init_dp && !intel_dp_init_connector(intel_dig_port, dp_connector)) {
>  		drm_encoder_cleanup(encoder);
>  		kfree(intel_dig_port);
>  		kfree(dp_connector);
>  		return;
>  	}
>  
> -	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
> +	/* In theory we don't need the encoder->type check, but leave it just in
> +	 * case we have some really bad VBTs... */
> +	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
>  		hdmi_connector = kzalloc(sizeof(struct intel_connector),
>  					 GFP_KERNEL);
> -		if (!hdmi_connector) {
> +		if (!hdmi_connector)
>  			return;
> -		}
>  
>  		intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
>  		intel_hdmi_init_connector(intel_dig_port, hdmi_connector);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch