drm/i915/vbt: defaults handling for no VBT case

We don't use the error return for anything other than reporting and logging that there is no VBT. We can pull the logging in the function, and remove the error status return. Moreover, if we needed the information for something later on, we'd probably be better off storing the bit in dev_priv, and using it where it's needed, instead of using the error return.

While at it, improve the comments.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c   |  4 +---
 drivers/gpu/drm/i915/i915_drv.h   |  2 +-
 drivers/gpu/drm/i915/intel_bios.c | 31 ++++++++++++++++---------------
 3 files changed, 18 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +0 -0

index b1e9027a4f80..1af54717fa81 100644

The main thing are the DDI ports. If there's a VBT that says there are no outputs, we should trust that, and not have semi-random defaults. Unfortunately, the defaults have resulted in some Chromebooks without VBT to rely on this behaviour, so we split out the defaults for the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +13 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	return;
 }
 
+/* Common defaults which may be overridden by VBT. */
 static void
 init_vbt_defaults(struct drm_i915_private *dev_priv)
 {

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 			&dev_priv->vbt.ddi_port_info[port];
 
 		info->hdmi_level_shift = HDMI_LEVEL_SHIFT_UNKNOWN;
+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];
 
 		info->supports_dvi = (port != PORT_A && port != PORT_E);
 		info->supports_hdmi = info->supports_dvi;

drivers/gpu/drm/i915/intel_bios.c +0 -0

 	if (bios)
 		pci_unmap_rom(pdev, bios);

Signature · 1 line

2.1.4

Quoted 47 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says there are no outputs, we should trust that, and not have semi-random defaults. Unfortunately, the defaults have resulted in some Chromebooks without VBT to rely on this behaviour, so we split out the defaults for the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +13 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	return;
 }
 
+/* Common defaults which may be overridden by VBT. */
 static void
 init_vbt_defaults(struct drm_i915_private *dev_priv)
 {

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 			&dev_priv->vbt.ddi_port_info[port];
 
 		info->hdmi_level_shift = HDMI_LEVEL_SHIFT_UNKNOWN;
+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];
 
 		info->supports_dvi = (port != PORT_A && port != PORT_E);
 		info->supports_hdmi = info->supports_dvi;

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	parse_ddi_ports(dev_priv, bdb);
 

drivers/gpu/drm/i915/intel_bios.c void intel_bios_init(struct drm_i915_private *dev_priv) · line 1535

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +3 -1

 out:
-	if (!vbt)
+	if (!vbt) {
 		DRM_INFO("Failed to find VBIOS tables (VBT)\n");
+		init_vbt_missing_defaults(dev_priv);
+	}

So in case there is no VBT, this will set supports_DP flag on Port A. What is there is no VBT and there is no eDP on Port A?
In this case it will still try to link train on Port A and fail..? I am not sure if this case exists, but just a thought looking at it.

If such a case does not exist, then this will solve our problem of current failures because leaving defaults on Port A. So in that case it lgtm.

Regards
Manasi

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 	if (bios)
 		pci_unmap_rom(pdev, bios);

Signature · 1 line

2.1.4

Quoted 26 lines · Jani Nikula

We don't use the error return for anything other than reporting and logging that there is no VBT. We can pull the logging in the function, and remove the error status return. Moreover, if we needed the information for something later on, we'd probably be better off storing the bit in dev_priv, and using it where it's needed, instead of using the error return.

While at it, improve the comments.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c   |  4 +---
 drivers/gpu/drm/i915/i915_drv.h   |  2 +-
 drivers/gpu/drm/i915/intel_bios.c | 31 ++++++++++++++++---------------
 3 files changed, 18 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +0 -0

index b1e9027a4f80..1af54717fa81 100644
@@ -567,9 +567,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_inject_load_failure())
 		return -ENODEV;
 

drivers/gpu/drm/i915/i915_drv.c static int i915_load_modeset_init(struct drm_device *dev) · line 570

Quoted 4 lines

drivers/gpu/drm/i915/i915_drv.c +1 -3

-	ret = intel_bios_init(dev_priv);
-	if (ret)
-		DRM_INFO("failed to find VBIOS tables\n");
+	intel_bios_init(dev_priv);

Sold.

Reviewed-by Chris Wilson <[email protected]>

Quoted 48 lines · Manasi Navare

Quoted 47 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says there are no outputs, we should trust that, and not have semi-random defaults. Unfortunately, the defaults have resulted in some Chromebooks without VBT to rely on this behaviour, so we split out the defaults for the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +13 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	return;
 }
 
+/* Common defaults which may be overridden by VBT. */
 static void
 init_vbt_defaults(struct drm_i915_private *dev_priv)
 {

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 			&dev_priv->vbt.ddi_port_info[port];
 
 		info->hdmi_level_shift = HDMI_LEVEL_SHIFT_UNKNOWN;
+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];
 
 		info->supports_dvi = (port != PORT_A && port != PORT_E);
 		info->supports_hdmi = info->supports_dvi;

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	parse_ddi_ports(dev_priv, bdb);
 

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.c void intel_bios_init(struct drm_i915_private *dev_priv) · line 1535

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +3 -1

 out:
-	if (!vbt)
+	if (!vbt) {
 		DRM_INFO("Failed to find VBIOS tables (VBT)\n");
+		init_vbt_missing_defaults(dev_priv);
+	}

So in case there is no VBT, this will set supports_DP flag on Port A. What is there is no VBT and there is no eDP on Port A?
In this case it will still try to link train on Port A and fail..? I am not sure if this case exists, but just a thought looking at it.

It's possible the case exists, but the point is that the behaviour for the no-VBT case remains the same before and after this patch.

BR,
Jani.

Quoted 9 lines

If such a case does not exist, then this will solve our problem of current failures because leaving defaults on Port A. So in that case it lgtm.

Regards
Manasi

Quoted 1 line

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 	if (bios)
 		pci_unmap_rom(pdev, bios);

Signature · 1 line

2.1.4

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Quoted 64 lines · Jani Nikula

Quoted 48 lines · Manasi Navare

Quoted 47 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says there are no outputs, we should trust that, and not have semi-random defaults. Unfortunately, the defaults have resulted in some Chromebooks without VBT to rely on this behaviour, so we split out the defaults for the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +13 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	return;
 }
 
+/* Common defaults which may be overridden by VBT. */
 static void
 init_vbt_defaults(struct drm_i915_private *dev_priv)
 {

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 			&dev_priv->vbt.ddi_port_info[port];
 
 		info->hdmi_level_shift = HDMI_LEVEL_SHIFT_UNKNOWN;
+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];
 
 		info->supports_dvi = (port != PORT_A && port != PORT_E);
 		info->supports_hdmi = info->supports_dvi;

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	parse_ddi_ports(dev_priv, bdb);
 

Quoted 11 lines

drivers/gpu/drm/i915/intel_bios.c void intel_bios_init(struct drm_i915_private *dev_priv) · line 1535

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +3 -1

 out:
-	if (!vbt)
+	if (!vbt) {
 		DRM_INFO("Failed to find VBIOS tables (VBT)\n");
+		init_vbt_missing_defaults(dev_priv);
+	}

So in case there is no VBT, this will set supports_DP flag on Port A. What is there is no VBT and there is no eDP on Port A?
In this case it will still try to link train on Port A and fail..? I am not sure if this case exists, but just a thought looking at it.

It's possible the case exists, but the point is that the behaviour for the no-VBT case remains the same before and after this patch.

Quoted 4 lines

BR,
Jani.

Ok agreed. In that case Reviewed-by: Manasi Navare <[email protected]>

Regards
Manasi

Quoted 17 lines

Quoted 14 lines

If such a case does not exist, then this will solve our problem of current failures because leaving defaults on Port A. So in that case it lgtm.

Regards
Manasi

Quoted 6 lines

drivers/gpu/drm/i915/intel_bios.c +0 -0

 	if (bios)
 		pci_unmap_rom(pdev, bios);

Signature · 1 line

2.1.4

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Quoted 89 lines · Jani Nikula

Quoted 86 lines

Quoted 67 lines · Manasi Navare

Quoted 62 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says there are
no outputs, we should trust that, and not have semi-random defaults. Unfortunately, the defaults have resulted in some Chromebooks
without VBT to rely on this behaviour, so we split out the defaults for
the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-

 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +0 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct
 drm_i915_private *dev_priv,
  	return;
  }

+/* Common defaults which may be overridden by VBT. */ static void
 init_vbt_defaults(struct drm_i915_private *dev_priv) {

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct
 drm_i915_private *dev_priv)
  			&dev_priv->vbt.ddi_port_info[port];

  info->hdmi_level_shift =
HDMI_LEVEL_SHIFT_UNKNOWN;

drivers/gpu/drm/i915/intel_bios.c +12 -0

+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];

  info->supports_dvi = (port != PORT_A && port != PORT_E);
  info->supports_hdmi = info->supports_dvi;

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct
 drm_i915_private *dev_priv)
  	parse_ddi_ports(dev_priv, bdb);

 out:

drivers/gpu/drm/i915/intel_bios.c +1 -1

-	if (!vbt)
+	if (!vbt) {

  DRM_INFO("Failed to find VBIOS tables (VBT)\n");

drivers/gpu/drm/i915/intel_bios.c void intel_bios_init(struct drm_i915_private *dev_priv) · line 1535

Quoted 2 lines

drivers/gpu/drm/i915/intel_bios.c +2 -0

+		init_vbt_missing_defaults(dev_priv);
+	}

Quoted 7 lines

So in case there is no VBT, this will set supports_DP flag on Port A.
What is there is no VBT and there is no eDP on Port A? In this case it will still try to link train on Port A and fail..?
I am not sure if this case exists, but just a thought looking at it.

It's possible the case exists, but the point is that the behaviour for
the no-VBT case remains the same before and after this patch.

BR,
Jani.

Quoted 2 lines

Ok agreed. In that case Reviewed-by: Manasi Navare <[email protected]>

Pushed to drm-intel-next-queued, thanks for the review.

I really hope there are no machines out there that have a crippled VBT
with no child device config. I guess we'll find out...

BR,
Jani.

Quoted 28 lines

Regards
Manasi

Quoted 22 lines

Quoted 19 lines

If such a case does not exist, then this will solve our problem of
current failures because leaving defaults on Port A. So in that case
it lgtm.

Regards
Manasi

Quoted 7 lines

  if (bios)
  pci_unmap_rom(pdev, bios);
--
2.1.4

--
Jani Nikula, Intel Open Source Technology Center

Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:

Quoted 90 lines · Manasi Navare

Quoted 89 lines · Jani Nikula

Quoted 86 lines

Quoted 67 lines · Manasi Navare

Quoted 62 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says there are
no outputs, we should trust that, and not have semi-random defaults. Unfortunately, the defaults have resulted in some Chromebooks
without VBT to rely on this behaviour, so we split out the defaults for
the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-

 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +0 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct
 drm_i915_private *dev_priv,
  	return;
  }

+/* Common defaults which may be overridden by VBT. */ static void
 init_vbt_defaults(struct drm_i915_private *dev_priv) {

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct
 drm_i915_private *dev_priv)
  			&dev_priv->vbt.ddi_port_info[port];

  info->hdmi_level_shift =
HDMI_LEVEL_SHIFT_UNKNOWN;

drivers/gpu/drm/i915/intel_bios.c +12 -0

+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+		struct ddi_vbt_port_info *info =
+			&dev_priv->vbt.ddi_port_info[port];

  info->supports_dvi = (port != PORT_A && port != PORT_E);
  info->supports_hdmi = info->supports_dvi;

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct
 drm_i915_private *dev_priv)
  	parse_ddi_ports(dev_priv, bdb);

 out:

drivers/gpu/drm/i915/intel_bios.c +1 -1

-	if (!vbt)
+	if (!vbt) {

  DRM_INFO("Failed to find VBIOS tables (VBT)\n");

drivers/gpu/drm/i915/intel_bios.c void intel_bios_init(struct drm_i915_private *dev_priv) · line 1535

Quoted 2 lines

drivers/gpu/drm/i915/intel_bios.c +2 -0

+		init_vbt_missing_defaults(dev_priv);
+	}

Quoted 7 lines

So in case there is no VBT, this will set supports_DP flag on Port A.
What is there is no VBT and there is no eDP on Port A? In this case it will still try to link train on Port A and fail..?
I am not sure if this case exists, but just a thought looking at it.

It's possible the case exists, but the point is that the behaviour for
the no-VBT case remains the same before and after this patch.

BR,
Jani.

Quoted 8 lines

Quoted 2 lines

Ok agreed. In that case Reviewed-by: Manasi Navare <[email protected]>

Pushed to drm-intel-next-queued, thanks for the review.

I really hope there are no machines out there that have a crippled VBT
with no child device config. I guess we'll find out...

I have access to this very interesting machine with DDB version 163 and a child device size config that's 1 instead of the expected 33.

So what happens here is that since the VBT is supposed to be valid we don't end up calling init_vbt_missing_defauilts(). We return early from parse_device_mapping(), which means we don't set vbt.child_dev_num, which means that parse_ddi_port() returns early. So info->supports_* stays false, and intel_ddi_init() fails.

Given your commit message it seems that we should properly be able to distinguish between "VBT correctly says that there's no output" and "VBT is drunk and should go home" in order to fix this problem.

I can confirm that reverting this patch makes display great again^w^w work again. So unfortunately I'll have to call regression on this patch.

Quoted 34 lines

BR,
Jani.

Quoted 28 lines

Regards
Manasi

Quoted 22 lines

Quoted 19 lines

If such a case does not exist, then this will solve our problem of
current failures because leaving defaults on Port A. So in that case
it lgtm.

Regards
Manasi

Quoted 7 lines

  if (bios)
  pci_unmap_rom(pdev, bios);
--
2.1.4

--
Jani Nikula, Intel Open Source Technology Center

Quoted 135 lines

Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:

Quoted 113 lines

Quoted 102 lines · Manasi Navare

Quoted 99 lines · Jani Nikula

On Sat, 11 Mar 2017, Manasi Navare <[email protected]

Quoted 1 line

Quoted 81 lines · wrote:

Quoted 73 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says
there are
no outputs, we should trust that, and not have semi- random
defaults. Unfortunately, the defaults have resulted in some
Chromebooks
without VBT to rely on this behaviour, so we split out the
defaults for
the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-

 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +0 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct
 drm_i915_private *dev_priv,
  	return;
  }

+/* Common defaults which may be overridden by VBT. */ static void
 init_vbt_defaults(struct drm_i915_private *dev_priv) {

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct
 drm_i915_private *dev_priv)

  &dev_priv-

Quoted 1 line

vbt.ddi_port_info[port];

  info->hdmi_level_shift =
HDMI_LEVEL_SHIFT_UNKNOWN;

drivers/gpu/drm/i915/intel_bios.c +6 -0

+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private

*dev_priv)

drivers/gpu/drm/i915/intel_bios.c +4 -0

+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS;

port++) {

drivers/gpu/drm/i915/intel_bios.c +2 -0

+		struct ddi_vbt_port_info *info =
+			&dev_priv-

Quoted 1 line

vbt.ddi_port_info[port];

  info->supports_dvi = (port != PORT_A && port
!= PORT_E);
  info->supports_hdmi = info-

Quoted 1 line

supports_dvi;

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct
 drm_i915_private *dev_priv)
  	parse_ddi_ports(dev_priv, bdb);

 out:

drivers/gpu/drm/i915/intel_bios.c +1 -1

-	if (!vbt)
+	if (!vbt) {

  DRM_INFO("Failed to find VBIOS tables (VBT)\n");

drivers/gpu/drm/i915/intel_bios.c

Quoted 2 lines

drivers/gpu/drm/i915/intel_bios.c +2 -0

+		init_vbt_missing_defaults(dev_priv);
+	}

So in case there is no VBT, this will set supports_DP flag

Quoted 8 lines

on
Port A.
What is there is no VBT and there is no eDP on Port A? In this case it will still try to link train on Port A and fail..?
I am not sure if this case exists, but just a thought looking
at it.

It's possible the case exists, but the point is that the behaviour for
the no-VBT case remains the same before and after this patch.

Quoted 7 lines

Quoted 4 lines

BR,
Jani.

Ok agreed. In that case Reviewed-by: Manasi Navare <[email protected]>

Quoted 6 lines

Pushed to drm-intel-next-queued, thanks for the review.

I really hope there are no machines out there that have a crippled
VBT
with no child device config. I guess we'll find out...

I have access to this very interesting machine with DDB version 163 and
a child device size config that's 1 instead of the expected 33.

So what happens here is that since the VBT is supposed to be valid we
don't end up calling init_vbt_missing_defauilts(). We return early from
parse_device_mapping(), which means we don't set vbt.child_dev_num, which means that parse_ddi_port() returns early. So info-

Quoted 1 line

supports_*

stays false, and intel_ddi_init() fails.

Quoted 4 lines

Given your commit message it seems that we should properly be able to
distinguish between "VBT correctly says that there's no output" and "VBT is drunk and should go home" in order to fix this problem.

I'm not sure it's possible to distinguish between the two. I thought we'd be able to rely on the former.

If we have to change
"init_vbt_missing_defaults" to "init_child_dev_missing_defaults", then
we'll never be able to handle the case where vbt correctly states there
are no child devices. :(

BR,
Jani.

Quoted 47 lines

I can confirm that reverting this patch makes display great again^w^w
work again. So unfortunately I'll have to call regression on this patch.

Quoted 41 lines

BR,
Jani.

Quoted 34 lines

Regards
Manasi

Quoted 27 lines

Quoted 12 lines

If such a case does not exist, then this will solve our problem
of
current failures because leaving defaults on Port A. So in that
case
it lgtm.

Quoted 12 lines

Regards
Manasi

Quoted 8 lines

  if (bios)
  pci_unmap_rom(pdev, bios);
--
2.1.4

--
Jani Nikula, Intel Open Source Technology Center

Em Seg, 2017-03-20 às 11:38 +0200, Jani Nikula escreveu:

Quoted 136 lines · Paulo Zanoni

Quoted 135 lines

Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:

Quoted 113 lines

Quoted 102 lines · Manasi Navare

Quoted 99 lines · Jani Nikula

On Sat, 11 Mar 2017, Manasi Navare <[email protected]

Quoted 1 line

Quoted 81 lines · wrote:

Quoted 73 lines · Jani Nikula

The main thing are the DDI ports. If there's a VBT that says
there are
no outputs, we should trust that, and not have semi- random
defaults. Unfortunately, the defaults have resulted in some
Chromebooks
without VBT to rely on this behaviour, so we split out the
defaults for
the missing VBT case.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-

 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_bios.c +0 -0

index 710988d72253..639d45c1dd2e 100644
@@ -1341,6 +1341,7 @@ parse_device_mapping(struct
 drm_i915_private *dev_priv,
  	return;
  }

+/* Common defaults which may be overridden by VBT. */ static void
 init_vbt_defaults(struct drm_i915_private *dev_priv) {

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1377,6 +1378,18 @@ init_vbt_defaults(struct
 drm_i915_private *dev_priv)

  &dev_priv-

Quoted 1 line

vbt.ddi_port_info[port];

  info->hdmi_level_shift =
HDMI_LEVEL_SHIFT_UNKNOWN;

drivers/gpu/drm/i915/intel_bios.c +6 -0

+	}
+}
+
+/* Defaults to initialize only if there is no VBT. */
+static void
+init_vbt_missing_defaults(struct drm_i915_private

*dev_priv)

drivers/gpu/drm/i915/intel_bios.c +4 -0

+{
+	enum port port;
+
+	for (port = PORT_A; port < I915_MAX_PORTS;

port++) {

drivers/gpu/drm/i915/intel_bios.c +2 -0

+		struct ddi_vbt_port_info *info =
+			&dev_priv-

Quoted 1 line

vbt.ddi_port_info[port];

  info->supports_dvi = (port != PORT_A && port
!= PORT_E);
  info->supports_hdmi = info-

Quoted 1 line

supports_dvi;

drivers/gpu/drm/i915/intel_bios.c +0 -0

@@ -1516,8 +1529,10 @@ void intel_bios_init(struct
 drm_i915_private *dev_priv)
  	parse_ddi_ports(dev_priv, bdb);

 out:

drivers/gpu/drm/i915/intel_bios.c +1 -1

-	if (!vbt)
+	if (!vbt) {

  DRM_INFO("Failed to find VBIOS tables (VBT)\n");

drivers/gpu/drm/i915/intel_bios.c

Quoted 2 lines

drivers/gpu/drm/i915/intel_bios.c +2 -0

+		init_vbt_missing_defaults(dev_priv);
+	}

So in case there is no VBT, this will set supports_DP flag

Quoted 8 lines

on
Port A.
What is there is no VBT and there is no eDP on Port A? In this case it will still try to link train on Port A and fail..?
I am not sure if this case exists, but just a thought looking
at it.

It's possible the case exists, but the point is that the behaviour for
the no-VBT case remains the same before and after this patch.

Quoted 7 lines

Quoted 4 lines

BR,
Jani.

Ok agreed. In that case Reviewed-by: Manasi Navare <[email protected]>

Quoted 6 lines

Pushed to drm-intel-next-queued, thanks for the review.

I really hope there are no machines out there that have a crippled
VBT
with no child device config. I guess we'll find out...

I have access to this very interesting machine with DDB version 163 and
a child device size config that's 1 instead of the expected 33.

So what happens here is that since the VBT is supposed to be valid we
don't end up calling init_vbt_missing_defauilts(). We return early from
parse_device_mapping(), which means we don't set vbt.child_dev_num, which means that parse_ddi_port() returns early. So info-

Quoted 1 line

supports_*

stays false, and intel_ddi_init() fails.

Quoted 7 lines

Quoted 4 lines

Given your commit message it seems that we should properly be able to
distinguish between "VBT correctly says that there's no output" and "VBT is drunk and should go home" in order to fix this problem.

I'm not sure it's possible to distinguish between the two. I thought we'd be able to rely on the former.

Are you sure? The code I'm hitting is that child_dev_size != expected size, which translates to "we failed to parse the VBT". Is this exactly what you see in the no-output machines? I would expect no-output machines would have a VBT that actually parses correctly but intentionally leaves you with child_device_num = 0.

Then, what we'd need to do would be to make those parse_something functions return error codes when the VBT was malformed, so we'd be able to differentiate between intentional child_dev_num=0 or parse failures.

Quoted 55 lines

If we have to change
"init_vbt_missing_defaults" to "init_child_dev_missing_defaults", then
we'll never be able to handle the case where vbt correctly states there
are no child devices. :(

BR,
Jani.

Quoted 44 lines

I can confirm that reverting this patch makes display great again^w^w
work again. So unfortunately I'll have to call regression on this patch.

Quoted 38 lines

BR,
Jani.

Quoted 32 lines

Regards
Manasi

Quoted 25 lines

Quoted 24 lines

If such a case does not exist, then this will solve our problem
of
current failures because leaving defaults on Port A. So in that
case
it lgtm.

Regards
Manasi

Quoted 8 lines

  if (bios)
  pci_unmap_rom(pdev, bios);
--
2.1.4

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

--
Jani Nikula, Intel Open Source Technology Center