[PATCH 0/3] More Haswell VBT code

[PATCH 0/3] More Haswell VBT code

[Paulo Zanoni]

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

Hi

These 3 patches are on top of the 2 patches I recently sent to the list:
  [1/2] drm/i915: VBT's child_device_config changes over time
  [2/2] drm/i915: use the HDMI DDI buffer translations from VBT

Patch 1 is IMHO a "must have" since it allows us to discover if our current
assumptions are wrong. If we ever get a "black screen" bug report with one of
the WARNs added by this patch, then we'll have to write some additional code.

Patch 2 is just a bunch of WARNs that should really not ever appear in the log
files. If we ever see these WARNs on a machine, we should probably assume its
VBT is broken, and this may be a really good hint on why the user's eDP panel is
just black.

Patch 3 is just an "optimization" that will make our code not try to do DP AUX
transactions on HDMI-only ports.

I know some of us have really strong negative feelings about the VBT, but we
currently already rely on the VBT for some important display features (e.g.,
lots of SDVO, eDP and LVDS info, CRT DDC, etc.), and the WARNs added by this
series should actually help us decide if the bug reporter's VBT should be
trusted or not. Depending on the bug reports we get, we may consider adding a
"i915.trust_vbt" option or maybe even machine-specific quirks.

Thanks,
Paulo

Paulo Zanoni (3):
  drm/i915: check the DDC and AUX bits of the VBT on DDI machines
  drm/i915: add some assertions about VBT DDI port types
  drm/i915: don't init DP or HDMI when not supported by DDI port

 drivers/gpu/drm/i915/i915_drv.h   |  3 ++
 drivers/gpu/drm/i915/intel_bios.c | 58 ++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ddi.c  | 30 ++++++++++++++------
 3 files changed, 82 insertions(+), 9 deletions(-)

-- 
1.8.1.2

[PATCH 1/3] 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?

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 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 79bb651..a6700ab 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -575,6 +575,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;
 	int dvo_ports[][2] = {
 		{0, 10}, {1, 7}, {2, 8}, {3, 9}, {6, /* Unused */ 0},
 	};
@@ -598,6 +600,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.1.2

[PATCH 2/3] 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.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 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 a6700ab..4003dbf 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -575,7 +575,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;
 	int dvo_ports[][2] = {
 		{0, 10}, {1, 7}, {2, 8}, {3, 9}, {6, /* Unused */ 0},
@@ -604,6 +604,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.1.2

[PATCH 3/3] 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>
---
 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 645dd74..8720f31 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1041,6 +1041,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 4003dbf..cd823b9 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -608,6 +608,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);
 
@@ -762,8 +766,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 ece226d..d344977 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.1.2

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

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Wed, Aug 28, 2013 at 12:45 PM, Paulo Zanoni <przanoni@gmail.com> 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?
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  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 79bb651..a6700ab 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -575,6 +575,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;
>         int dvo_ports[][2] = {
>                 {0, 10}, {1, 7}, {2, 8}, {3, 9}, {6, /* Unused */ 0},
>         };
> @@ -598,6 +600,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.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Wed, Aug 28, 2013 at 12:45 PM, Paulo Zanoni <przanoni@gmail.com> 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.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  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 a6700ab..4003dbf 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -575,7 +575,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;
>         int dvo_ports[][2] = {
>                 {0, 10}, {1, 7}, {2, 8}, {3, 9}, {6, /* Unused */ 0},
> @@ -604,6 +604,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.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Rodrigo Vivi]

Do we really trust VBT that much?

Anyways,
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Wed, Aug 28, 2013 at 12:45 PM, Paulo Zanoni <przanoni@gmail.com> 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>
> ---
>  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 645dd74..8720f31 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1041,6 +1041,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 4003dbf..cd823b9 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -608,6 +608,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);
>
> @@ -762,8 +766,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 ece226d..d344977 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.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Paulo Zanoni]

2013/9/3 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> Do we really trust VBT that much?

We already trust it for some very important things, like eDP. Also, if
the VBT is wrong we'll start seeing all sorts of WARNs due to the
previous patches. Anyway, we can always revert...

Thanks for the reviews!

>
> Anyways,
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> On Wed, Aug 28, 2013 at 12:45 PM, Paulo Zanoni <przanoni@gmail.com> 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>
>> ---
>>  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 645dd74..8720f31 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1041,6 +1041,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 4003dbf..cd823b9 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -608,6 +608,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);
>>
>> @@ -762,8 +766,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 ece226d..d344977 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.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Paulo Zanoni