[PATCH 0/2] drm/i915/vbt: defaults handling for no VBT case

[PATCH 0/2] drm/i915/vbt: defaults handling for no VBT case

[Jani Nikula]

This is my proposed alternative to [1].

BR,
Jani.


[1] http://patchwork.freedesktop.org/patch/msgid/1488546102-27789-1-git-send-email-manasi.d.navare@intel.com


Jani Nikula (2):
  drm/i915/vbt: don't propagate errors from intel_bios_init()
  drm/i915/vbt: split out defaults that are set when there is no VBT

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

-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915/vbt: don't propagate errors from intel_bios_init()

[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 <manasi.d.navare@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b1e9027a4f80..1af54717fa81 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -567,9 +567,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_inject_load_failure())
 		return -ENODEV;
 
-	ret = intel_bios_init(dev_priv);
-	if (ret)
-		DRM_INFO("failed to find VBIOS tables\n");
+	intel_bios_init(dev_priv);
 
 	/* If we have > 1 VGA cards, then we need to arbitrate access
 	 * to the common VGA resources.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3002996ddbed..4fc8ca340d27 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3714,7 +3714,7 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 extern void intel_i2c_reset(struct drm_i915_private *dev_priv);
 
 /* intel_bios.c */
-int intel_bios_init(struct drm_i915_private *dev_priv);
+void intel_bios_init(struct drm_i915_private *dev_priv);
 bool intel_bios_is_valid_vbt(const void *buf, size_t size);
 bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv);
 bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin);
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index e144f033f4b5..710988d72253 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1462,36 +1462,35 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
  * intel_bios_init - find VBT and initialize settings from the BIOS
  * @dev_priv: i915 device instance
  *
- * Loads the Video BIOS and checks that the VBT exists.  Sets scratch registers
- * to appropriate values.
- *
- * Returns 0 on success, nonzero on failure.
+ * Parse and initialize settings from the Video BIOS Tables (VBT). If the VBT
+ * was not found in ACPI OpRegion, try to find it in PCI ROM first. Also
+ * initialize some defaults if the VBT is not present at all.
  */
-int
-intel_bios_init(struct drm_i915_private *dev_priv)
+void intel_bios_init(struct drm_i915_private *dev_priv)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	const struct vbt_header *vbt = dev_priv->opregion.vbt;
 	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 
-	if (HAS_PCH_NOP(dev_priv))
-		return -ENODEV;
+	if (HAS_PCH_NOP(dev_priv)) {
+		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
+		return;
+	}
 
 	init_vbt_defaults(dev_priv);
 
+	/* If the OpRegion does not have VBT, look in PCI ROM. */
 	if (!vbt) {
 		size_t size;
 
 		bios = pci_map_rom(pdev, &size);
 		if (!bios)
-			return -1;
+			goto out;
 
 		vbt = find_vbt(bios, size);
-		if (!vbt) {
-			pci_unmap_rom(pdev, bios);
-			return -1;
-		}
+		if (!vbt)
+			goto out;
 
 		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
 	}
@@ -1516,10 +1515,12 @@ intel_bios_init(struct drm_i915_private *dev_priv)
 	parse_mipi_sequence(dev_priv, bdb);
 	parse_ddi_ports(dev_priv, bdb);
 
+out:
+	if (!vbt)
+		DRM_INFO("Failed to find VBIOS tables (VBT)\n");
+
 	if (bios)
 		pci_unmap_rom(pdev, bios);
-
-	return 0;
 }
 
 /**
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/vbt: don't propagate errors from intel_bios_init()

[Chris Wilson]

On Fri, Mar 10, 2017 at 03:27:57PM +0200, Jani Nikula wrote:
> 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 <manasi.d.navare@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b1e9027a4f80..1af54717fa81 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -567,9 +567,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (i915_inject_load_failure())
>  		return -ENODEV;
>  
> -	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 <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/vbt: don't propagate errors from intel_bios_init()

[Jani Nikula]

On Fri, 10 Mar 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Mar 10, 2017 at 03:27:57PM +0200, Jani Nikula wrote:
>> 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 <manasi.d.navare@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  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(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index b1e9027a4f80..1af54717fa81 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -567,9 +567,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>>  	if (i915_inject_load_failure())
>>  		return -ENODEV;
>>  
>> -	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 <chris@chris-wilson.co.uk>

Pushed patch 1, thanks for the review.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[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 <manasi.d.navare@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 710988d72253..639d45c1dd2e 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -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);
 
 out:
-	if (!vbt)
+	if (!vbt) {
 		DRM_INFO("Failed to find VBIOS tables (VBT)\n");
+		init_vbt_missing_defaults(dev_priv);
+	}
 
 	if (bios)
 		pci_unmap_rom(pdev, bios);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Manasi Navare]

On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
> 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 <manasi.d.navare@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 710988d72253..639d45c1dd2e 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -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);
>  
>  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


>  
>  	if (bios)
>  		pci_unmap_rom(pdev, bios);
> -- 
> 2.1.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Jani Nikula]

On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
>> 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 <manasi.d.navare@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 710988d72253..639d45c1dd2e 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -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);
>>  
>>  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.


> 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
>
>
>>  
>>  	if (bios)
>>  		pci_unmap_rom(pdev, bios);
>> -- 
>> 2.1.4
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Manasi Navare]

On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
> On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
> >> 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 <manasi.d.navare@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> index 710988d72253..639d45c1dd2e 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -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);
> >>  
> >>  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.
> 
>

Ok agreed. In that case Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Regards
Manasi

 
> > 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
> >
> >
> >>  
> >>  	if (bios)
> >>  		pci_unmap_rom(pdev, bios);
> >> -- 
> >> 2.1.4
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Jani Nikula]

On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
>> On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
>> >> 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 <manasi.d.navare@intel.com>
>> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
>> >>  1 file changed, 16 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> >> index 710988d72253..639d45c1dd2e 100644
>> >> --- a/drivers/gpu/drm/i915/intel_bios.c
>> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> >> @@ -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);
>> >>  
>> >>  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.
>> 
>>
>
> Ok agreed. In that case Reviewed-by: Manasi Navare
> <manasi.d.navare@intel.com>

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.


>
> Regards
> Manasi
>
>  
>> > 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
>> >
>> >
>> >>  
>> >>  	if (bios)
>> >>  		pci_unmap_rom(pdev, bios);
>> >> -- 
>> >> 2.1.4
>> >> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Paulo Zanoni]

Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:
> On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > 
> > On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
> > > 
> > > On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com>
> > > wrote:
> > > > 
> > > > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
> > > > > 
> > > > > 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 <manasi.d.navare@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> > > > > b/drivers/gpu/drm/i915/intel_bios.c
> > > > > index 710988d72253..639d45c1dd2e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > > @@ -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);
> > > > >  
> > > > >  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.
> > > 
> > > 
> > 
> > Ok agreed. In that case Reviewed-by: Manasi Navare
> > <manasi.d.navare@intel.com>
> 
> 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.

> 
> BR,
> Jani.
> 
> 
> > 
> > 
> > Regards
> > Manasi
> > 
> >  
> > > 
> > > > 
> > > > 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
> > > > 
> > > > 
> > > > > 
> > > > >  
> > > > >  	if (bios)
> > > > >  		pci_unmap_rom(pdev, bios);
> > > > > -- 
> > > > > 2.1.4
> > > > > 
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Technology Center
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Jani Nikula]

On Fri, 17 Mar 2017, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:
>> On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > 
>> > On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
>> > > 
>> > > On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com>
>> > > wrote:
>> > > > 
>> > > > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
>> > > > > 
>> > > > > 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 <manasi.d.navare@intel.com>
>> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > > > > ---
>> > > > >  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
>> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c
>> > > > > b/drivers/gpu/drm/i915/intel_bios.c
>> > > > > index 710988d72253..639d45c1dd2e 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > > > > @@ -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);
>> > > > >  
>> > > > >  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.
>> > > 
>> > > 
>> > 
>> > Ok agreed. In that case Reviewed-by: Manasi Navare
>> > <manasi.d.navare@intel.com>
>> 
>> 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'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.


> 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.
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> > 
>> > 
>> > Regards
>> > Manasi
>> > 
>> >  
>> > > 
>> > > > 
>> > > > 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
>> > > > 
>> > > > 
>> > > > > 
>> > > > >  
>> > > > >  	if (bios)
>> > > > >  		pci_unmap_rom(pdev, bios);
>> > > > > -- 
>> > > > > 2.1.4
>> > > > > 
>> > > 
>> > > -- 
>> > > Jani Nikula, Intel Open Source Technology Center
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Paulo Zanoni]

Em Seg, 2017-03-20 às 11:38 +0200, Jani Nikula escreveu:
> On Fri, 17 Mar 2017, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> > 
> > Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:
> > > 
> > > On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare@intel.com>
> > > wrote:
> > > > 
> > > > 
> > > > On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
> > > > > 
> > > > > 
> > > > > On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com
> > > > > >
> > > > > wrote:
> > > > > > 
> > > > > > 
> > > > > > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 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 <manasi.d.navare@intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
> > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > b/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > index 710988d72253..639d45c1dd2e 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > @@ -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);
> > > > > > >  
> > > > > > >  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.
> > > > > 
> > > > > 
> > > > 
> > > > Ok agreed. In that case Reviewed-by: Manasi Navare
> > > > <manasi.d.navare@intel.com>
> > > 
> > > 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'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.


>  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.
> 
> 
> > 
> > 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.
> > 
> > > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > Regards
> > > > Manasi
> > > > 
> > > >  
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 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
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > >  
> > > > > > >  	if (bios)
> > > > > > >  		pci_unmap_rom(pdev, bios);
> > > > > > > -- 
> > > > > > > 2.1.4
> > > > > > > 
> > > > > 
> > > > > -- 
> > > > > Jani Nikula, Intel Open Source Technology Center
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Manasi Navare]

On Mon, Mar 20, 2017 at 01:57:51PM -0300, Paulo Zanoni wrote:
> Em Seg, 2017-03-20 às 11:38 +0200, Jani Nikula escreveu:
> > On Fri, 17 Mar 2017, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> > > 
> > > Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:
> > > > 
> > > > On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare@intel.com>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
> > > > > > 
> > > > > > 
> > > > > > On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@intel.com
> > > > > > >
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 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 <manasi.d.navare@intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
> > > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > index 710988d72253..639d45c1dd2e 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > @@ -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);
> > > > > > > >  
> > > > > > > >  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.
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Ok agreed. In that case Reviewed-by: Manasi Navare
> > > > > <manasi.d.navare@intel.com>
> > > > 
> > > > 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'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.
> 
>

In the no-output machine cases, like the SKL desktop with on eDP on
Port A, it reports child_device_num=0 and so we should not call the
init_vbt_missing_defaults() function at all. I think we need a way to find
if the VBT was malformed/parese error.

Regards
Manasi

 
> >  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.
> > 
> > 
> > > 
> > > 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.
> > > 
> > > > 
> > > > 
> > > > BR,
> > > > Jani.
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Regards
> > > > > Manasi
> > > > > 
> > > > >  
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 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
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > >  
> > > > > > > >  	if (bios)
> > > > > > > >  		pci_unmap_rom(pdev, bios);
> > > > > > > > -- 
> > > > > > > > 2.1.4
> > > > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > Jani Nikula, Intel Open Source Technology Center
> > > > 
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915/vbt: defaults handling for no VBT case

[Patchwork]

== Series Details ==

Series: drm/i915/vbt: defaults handling for no VBT case
URL   : https://patchwork.freedesktop.org/series/21061/
State : failure

== Summary ==

Series 21061v1 drm/i915/vbt: defaults handling for no VBT case
https://patchwork.freedesktop.org/api/1.0/series/21061/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> INCOMPLETE (fi-byt-j1900)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 462s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 601s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 532s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 556s
fi-byt-j1900     total:241  pass:214  dwarn:0   dfail:0   fail:0   skip:26  time: 0s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 503s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 445s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 505s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 493s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 475s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 484s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 587s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 491s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 507s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 548s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 431s

2095bbc9d234d71fa44fd9181597431e2653058c drm-tip: 2017y-03m-10d-15h-03m-17s UTC integration manifest
d047866 drm/i915/vbt: split out defaults that are set when there is no VBT
d6576e7 drm/i915/vbt: don't propagate errors from intel_bios_init()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4138/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/vbt: defaults handling for no VBT case

[Patchwork]

== Series Details ==

Series: drm/i915/vbt: defaults handling for no VBT case
URL   : https://patchwork.freedesktop.org/series/21061/
State : success

== Summary ==

Series 21061v1 drm/i915/vbt: defaults handling for no VBT case
https://patchwork.freedesktop.org/api/1.0/series/21061/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s3:
                fail       -> DMESG-WARN (fi-kbl-7500u) fdo#100124
        Subgroup basic-s4-devices:
                fail       -> PASS       (fi-kbl-7500u) fdo#100099
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-kbl-7500u) fdo#100123
        Subgroup suspend-read-crc-pipe-b:
                fail       -> PASS       (fi-kbl-7500u) fdo#100123
        Subgroup suspend-read-crc-pipe-c:
                fail       -> PASS       (fi-kbl-7500u) fdo#100044

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100124 https://bugs.freedesktop.org/show_bug.cgi?id=100124
fdo#100099 https://bugs.freedesktop.org/show_bug.cgi?id=100099
fdo#100123 https://bugs.freedesktop.org/show_bug.cgi?id=100123
fdo#100044 https://bugs.freedesktop.org/show_bug.cgi?id=100044

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 462s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 610s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 528s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 548s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 503s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 508s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 429s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 445s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 504s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 483s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 471s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 475s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 590s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 490s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 505s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 551s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 423s

9f5422bbf45b2d8a0bb0538b08335c4ba7c2fba5 drm-tip: 2017y-03m-13d-08h-27m-09s UTC integration manifest
f55d292 drm/i915/vbt: split out defaults that are set when there is no VBT
88af841 drm/i915/vbt: don't propagate errors from intel_bios_init()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4149/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx