[PATCH 0/2] VBT and I_boost fixes

[PATCH 0/2] VBT and I_boost fixes

[Antti Koskipaa]

These are required for SKL PV.

I tested these on SNB and SKL.

Antti Koskipaa (2):
  drm/i915: Allow parsing of variable size child device entries from VBT
  drm/i915: Per-DDI I_boost override

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

-- 
2.3.6

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

[PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT

[Antti Koskipaa]

VBT version 196 increased the size of common_child_dev_config. The parser
code assumed that the size of this structure would not change.

So now, instead of checking for smaller size, check that the VBT entry is
not too large and memcpy only child_dev_size amount of data, leaving any
trailing entries as zero. If this is not good enough for the future,
we can always sprinkle extra version checks in there.

Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 2ff9eb0..763a636 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1022,10 +1022,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
 		return;
 	}
-	if (p_defs->child_dev_size < sizeof(*p_child)) {
+	/* Historically, child_dev_size has to be at least 33 bytes in size. */
+	if (p_defs->child_dev_size < 33) {
 		DRM_ERROR("General definiton block child device size is too small.\n");
 		return;
 	}
+	if (p_defs->child_dev_size > sizeof(*p_child)) {
+		DRM_ERROR("General definiton block child device size is too large.\n");
+		return;
+	}
 	/* get the block size of general definitions */
 	block_size = get_blocksize(p_defs);
 	/* get the number of child device */
@@ -1070,7 +1075,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 
 		child_dev_ptr = dev_priv->vbt.child_dev + count;
 		count++;
-		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
+		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
 	}
 	return;
 }
-- 
2.3.6

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

[PATCH 2/2] drm/i915: Per-DDI I_boost override

[Antti Koskipaa]

An OEM may request increased I_boost beyond the recommended values
by specifying an I_boost value to be applied to all swing entries for
a port. These override values are specified in VBT.

v2: rebase and remove unused iboost_bit variable

Issue: VIZ-5676
Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  3 +++
 drivers/gpu/drm/i915/intel_bios.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h |  9 +++++++++
 drivers/gpu/drm/i915/intel_ddi.c  | 38 ++++++++++++++++++++++++++++++--------
 4 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 768d1db..9a22a4d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1420,6 +1420,9 @@ struct ddi_vbt_port_info {
 	uint8_t supports_dvi:1;
 	uint8_t supports_hdmi:1;
 	uint8_t supports_dp:1;
+
+	uint8_t dp_boost_level;
+	uint8_t hdmi_boost_level;
 };
 
 enum psr_lines_to_wait {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 763a636..e9ced16 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -886,6 +886,17 @@ err:
 	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));
 }
 
+static u8 translate_iboost(u8 val)
+{
+	static const u8 mapping[] = { 1, 3, 7 }; /* See VBT spec */
+
+	if (val >= ARRAY_SIZE(mapping)) {
+		DRM_DEBUG_KMS("Unsupported I_boost value found in VBT (%d), display may not work properly\n", val);
+		return 0;
+	}
+	return mapping[val];
+}
+
 static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 			   const struct bdb_header *bdb)
 {
@@ -986,6 +997,16 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
 			      hdmi_level_shift);
 		info->hdmi_level_shift = hdmi_level_shift;
 	}
+
+	/* Parse the I_boost config for SKL and above */
+	if (bdb->version >= 196 && (child->common.flags_1 & IBOOST_ENABLE)) {
+		info->dp_boost_level = translate_iboost(child->common.iboost_level & 0xF);
+		DRM_DEBUG_KMS("VBT (e)DP boost level for port %c: %d\n",
+			      port_name(port), info->dp_boost_level);
+		info->hdmi_boost_level = translate_iboost(child->common.iboost_level >> 4);
+		DRM_DEBUG_KMS("VBT HDMI boost level for port %c: %d\n",
+			      port_name(port), info->hdmi_boost_level);
+	}
 }
 
 static void parse_ddi_ports(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index af0b476..8edd75c 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -231,6 +231,10 @@ struct old_child_dev_config {
 /* This one contains field offsets that are known to be common for all BDB
  * versions. Notice that the meaning of the contents contents may still change,
  * but at least the offsets are consistent. */
+
+/* Definitions for flags_1 */
+#define IBOOST_ENABLE (1<<3)
+
 struct common_child_dev_config {
 	u16 handle;
 	u16 device_type;
@@ -239,8 +243,13 @@ struct common_child_dev_config {
 	u8 not_common2[2];
 	u8 ddc_pin;
 	u16 edid_ptr;
+	u8 obsolete;
+	u8 flags_1;
+	u8 not_common3[13];
+	u8 iboost_level;
 } __packed;
 
+
 /* This field changes depending on the BDB version, so the most reliable way to
  * read it is by checking the BDB version and reading the raw pointer. */
 union child_device_config {
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9a40bfb..0446084 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -440,6 +440,7 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 reg;
+	u32 iboost_bit = 0;
 	int i, n_hdmi_entries, n_dp_entries, n_edp_entries, hdmi_default_entry,
 	    size;
 	int hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
@@ -466,6 +467,10 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
 		ddi_translations_hdmi =
 				skl_get_buf_trans_hdmi(dev, &n_hdmi_entries);
 		hdmi_default_entry = 8;
+		/* If we're boosting the current, set bit 31 of trans1 */
+		if (dev_priv->vbt.ddi_port_info[port].hdmi_boost_level ||
+		    dev_priv->vbt.ddi_port_info[port].dp_boost_level)
+			iboost_bit = 1<<31;
 	} else if (IS_BROADWELL(dev)) {
 		ddi_translations_fdi = bdw_ddi_translations_fdi;
 		ddi_translations_dp = bdw_ddi_translations_dp;
@@ -526,7 +531,7 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
 	}
 
 	for (i = 0, reg = DDI_BUF_TRANS(port); i < size; i++) {
-		I915_WRITE(reg, ddi_translations[i].trans1);
+		I915_WRITE(reg, ddi_translations[i].trans1 | iboost_bit);
 		reg += 4;
 		I915_WRITE(reg, ddi_translations[i].trans2);
 		reg += 4;
@@ -541,7 +546,7 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
 		hdmi_level = hdmi_default_entry;
 
 	/* Entry 9 is for HDMI: */
-	I915_WRITE(reg, ddi_translations_hdmi[hdmi_level].trans1);
+	I915_WRITE(reg, ddi_translations_hdmi[hdmi_level].trans1 | iboost_bit);
 	reg += 4;
 	I915_WRITE(reg, ddi_translations_hdmi[hdmi_level].trans2);
 	reg += 4;
@@ -2078,18 +2083,35 @@ static void skl_ddi_set_iboost(struct drm_device *dev, u32 level,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct ddi_buf_trans *ddi_translations;
 	uint8_t iboost;
+	uint8_t dp_iboost, hdmi_iboost;
 	int n_entries;
 	u32 reg;
 
+	/* VBT may override standard boost values */
+	dp_iboost = dev_priv->vbt.ddi_port_info[port].dp_boost_level;
+	hdmi_iboost = dev_priv->vbt.ddi_port_info[port].hdmi_boost_level;
+
 	if (type == INTEL_OUTPUT_DISPLAYPORT) {
-		ddi_translations = skl_get_buf_trans_dp(dev, &n_entries);
-		iboost = ddi_translations[port].i_boost;
+		if (dp_iboost) {
+			iboost = dp_iboost;
+		} else {
+			ddi_translations = skl_get_buf_trans_dp(dev, &n_entries);
+			iboost = ddi_translations[port].i_boost;
+		}
 	} else if (type == INTEL_OUTPUT_EDP) {
-		ddi_translations = skl_get_buf_trans_edp(dev, &n_entries);
-		iboost = ddi_translations[port].i_boost;
+		if (dp_iboost) {
+			iboost = dp_iboost;
+		} else {
+			ddi_translations = skl_get_buf_trans_edp(dev, &n_entries);
+			iboost = ddi_translations[port].i_boost;
+		}
 	} else if (type == INTEL_OUTPUT_HDMI) {
-		ddi_translations = skl_get_buf_trans_hdmi(dev, &n_entries);
-		iboost = ddi_translations[port].i_boost;
+		if (hdmi_iboost) {
+			iboost = hdmi_iboost;
+		} else {
+			ddi_translations = skl_get_buf_trans_hdmi(dev, &n_entries);
+			iboost = ddi_translations[port].i_boost;
+		}
 	} else {
 		return;
 	}
-- 
2.3.6

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

Re: [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa wrote:
> VBT version 196 increased the size of common_child_dev_config. The parser
> code assumed that the size of this structure would not change.
> 
> So now, instead of checking for smaller size, check that the VBT entry is
> not too large and memcpy only child_dev_size amount of data, leaving any
> trailing entries as zero. If this is not good enough for the future,
> we can always sprinkle extra version checks in there.
> 
> Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 2ff9eb0..763a636 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1022,10 +1022,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> -	if (p_defs->child_dev_size < sizeof(*p_child)) {
> +	/* Historically, child_dev_size has to be at least 33 bytes in size. */
> +	if (p_defs->child_dev_size < 33) {
>  		DRM_ERROR("General definiton block child device size is too small.\n");

"definition"

>  		return;
>  	}
> +	if (p_defs->child_dev_size > sizeof(*p_child)) {
> +		DRM_ERROR("General definiton block child device size is too large.\n");

"definition"

> +		return;
> +	}
>  	/* get the block size of general definitions */
>  	block_size = get_blocksize(p_defs);
>  	/* get the number of child device */
> @@ -1070,7 +1075,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  
>  		child_dev_ptr = dev_priv->vbt.child_dev + count;
>  		count++;
> -		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
> +		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>  	}
>  	return;
>  }
> -- 
> 2.3.6
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT

[Daniel Vetter]

On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa wrote:
> VBT version 196 increased the size of common_child_dev_config. The parser
> code assumed that the size of this structure would not change.
> 
> So now, instead of checking for smaller size, check that the VBT entry is
> not too large and memcpy only child_dev_size amount of data, leaving any
> trailing entries as zero. If this is not good enough for the future,
> we can always sprinkle extra version checks in there.
> 
> Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>

As I mentioned in the other threads I think with vbt it's not too paranoid
to double-check our assumptions. So for each vbt version range I'd like us
to check what size we exactly expect. Being super paranoid with vbt is imo
good practice since otherwise the hw teams will sneak in another update
without us realizing it.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_bios.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 2ff9eb0..763a636 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1022,10 +1022,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> -	if (p_defs->child_dev_size < sizeof(*p_child)) {
> +	/* Historically, child_dev_size has to be at least 33 bytes in size. */
> +	if (p_defs->child_dev_size < 33) {
>  		DRM_ERROR("General definiton block child device size is too small.\n");
>  		return;
>  	}
> +	if (p_defs->child_dev_size > sizeof(*p_child)) {
> +		DRM_ERROR("General definiton block child device size is too large.\n");
> +		return;
> +	}
>  	/* get the block size of general definitions */
>  	block_size = get_blocksize(p_defs);
>  	/* get the number of child device */
> @@ -1070,7 +1075,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  
>  		child_dev_ptr = dev_priv->vbt.child_dev + count;
>  		count++;
> -		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
> +		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>  	}
>  	return;
>  }
> -- 
> 2.3.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Mon, Jul 13, 2015 at 11:24:46AM +0200, Daniel Vetter wrote:
> On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa wrote:
> > VBT version 196 increased the size of common_child_dev_config. The parser
> > code assumed that the size of this structure would not change.
> > 
> > So now, instead of checking for smaller size, check that the VBT entry is
> > not too large and memcpy only child_dev_size amount of data, leaving any
> > trailing entries as zero. If this is not good enough for the future,
> > we can always sprinkle extra version checks in there.
> > 
> > Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> 
> As I mentioned in the other threads I think with vbt it's not too paranoid
> to double-check our assumptions. So for each vbt version range I'd like us
> to check what size we exactly expect. Being super paranoid with vbt is imo
> good practice since otherwise the hw teams will sneak in another update
> without us realizing it.

Antti's on vacation now for the next few weeks.  Do you need these
modifications as a pre-requisite for merging his patch, or can further
improvements be submitted separately?


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT

[Daniel Vetter]

On Tue, Jul 14, 2015 at 10:11:37AM +0300, David Weinehall wrote:
> On Mon, Jul 13, 2015 at 11:24:46AM +0200, Daniel Vetter wrote:
> > On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa wrote:
> > > VBT version 196 increased the size of common_child_dev_config. The parser
> > > code assumed that the size of this structure would not change.
> > > 
> > > So now, instead of checking for smaller size, check that the VBT entry is
> > > not too large and memcpy only child_dev_size amount of data, leaving any
> > > trailing entries as zero. If this is not good enough for the future,
> > > we can always sprinkle extra version checks in there.
> > > 
> > > Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> > 
> > As I mentioned in the other threads I think with vbt it's not too paranoid
> > to double-check our assumptions. So for each vbt version range I'd like us
> > to check what size we exactly expect. Being super paranoid with vbt is imo
> > good practice since otherwise the hw teams will sneak in another update
> > without us realizing it.
> 
> Antti's on vacation now for the next few weeks.  Do you need these
> modifications as a pre-requisite for merging his patch, or can further
> improvements be submitted separately?

This patch starts to make our vbt checking lax. I don't want to walk down
this road really, so yes I prefer if we just keep on having really strict
checks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

VBT version 196 increased the size of common_child_dev_config. The parser
code assumed that the size of this structure would not change.

The modified code now copies the amount needed based on the VBT version,
and emits a debug message if the VBT version is unknown (too new);
since the struct config block won't shrink in newer versions it should
be harmless to copy the maximum known size in such cases, so that's
what we do, but emitting the warning is probably sensible anyway.

In the longer run it might make sense to modify the parser code to
use a version/feature mapping, rather than hardcoding things like this,
but for now the variants are fairly managable.

v2: Stricter size checks

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 2ff9eb00fdec..8a1f3e1fc598 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1015,15 +1015,33 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	const union child_device_config *p_child;
 	union child_device_config *child_dev_ptr;
 	int i, child_device_num, count;
-	u16	block_size;
+	u8 expected_size;
+	u16 block_size;
 
 	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
 	if (!p_defs) {
 		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
 		return;
 	}
-	if (p_defs->child_dev_size < sizeof(*p_child)) {
-		DRM_ERROR("General definiton block child device size is too small.\n");
+	if (bdb->version < 195) {
+		expected_size = 33;
+	} else if (bdb->version == 195) {
+		expected_size = 37;
+	} else if (bdb->version <= 197) {
+		expected_size = 38;
+	} else {
+		expected_size = 38;
+		DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size);
+	}
+
+	if (expected_size > sizeof(*p_child)) {
+		DRM_ERROR("child_device_config cannot fit in p_child\n");
+		return;
+	}
+
+	if (p_defs->child_dev_size != expected_size) {
+		DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
+			  p_defs->child_dev_size, expected_size, bdb->version);
 		return;
 	}
 	/* get the block size of general definitions */
@@ -1070,7 +1088,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 
 		child_dev_ptr = dev_priv->vbt.child_dev + count;
 		count++;
-		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
+		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
 	}
 	return;
 }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Daniel Vetter]

On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
> VBT version 196 increased the size of common_child_dev_config. The parser
> code assumed that the size of this structure would not change.
> 
> The modified code now copies the amount needed based on the VBT version,
> and emits a debug message if the VBT version is unknown (too new);
> since the struct config block won't shrink in newer versions it should
> be harmless to copy the maximum known size in such cases, so that's
> what we do, but emitting the warning is probably sensible anyway.
> 
> In the longer run it might make sense to modify the parser code to
> use a version/feature mapping, rather than hardcoding things like this,
> but for now the variants are fairly managable.
> 
> v2: Stricter size checks
> 
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>

Since Chris mentioned that this should fix a regression I applied it to
drm-intel-fixes.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_bios.c |   26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 2ff9eb00fdec..8a1f3e1fc598 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1015,15 +1015,33 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	const union child_device_config *p_child;
>  	union child_device_config *child_dev_ptr;
>  	int i, child_device_num, count;
> -	u16	block_size;
> +	u8 expected_size;
> +	u16 block_size;
>  
>  	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>  	if (!p_defs) {
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> -	if (p_defs->child_dev_size < sizeof(*p_child)) {
> -		DRM_ERROR("General definiton block child device size is too small.\n");
> +	if (bdb->version < 195) {
> +		expected_size = 33;
> +	} else if (bdb->version == 195) {
> +		expected_size = 37;
> +	} else if (bdb->version <= 197) {
> +		expected_size = 38;
> +	} else {
> +		expected_size = 38;
> +		DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size);
> +	}
> +
> +	if (expected_size > sizeof(*p_child)) {
> +		DRM_ERROR("child_device_config cannot fit in p_child\n");
> +		return;
> +	}
> +
> +	if (p_defs->child_dev_size != expected_size) {
> +		DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
> +			  p_defs->child_dev_size, expected_size, bdb->version);
>  		return;
>  	}
>  	/* get the block size of general definitions */
> @@ -1070,7 +1088,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  
>  		child_dev_ptr = dev_priv->vbt.child_dev + count;
>  		count++;
> -		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
> +		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>  	}
>  	return;
>  }
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Wed, Aug 05, 2015 at 10:59:00AM +0200, Daniel Vetter wrote:
> On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
> > VBT version 196 increased the size of common_child_dev_config. The parser
> > code assumed that the size of this structure would not change.
> > 
> > The modified code now copies the amount needed based on the VBT version,
> > and emits a debug message if the VBT version is unknown (too new);
> > since the struct config block won't shrink in newer versions it should
> > be harmless to copy the maximum known size in such cases, so that's
> > what we do, but emitting the warning is probably sensible anyway.
> > 
> > In the longer run it might make sense to modify the parser code to
> > use a version/feature mapping, rather than hardcoding things like this,
> > but for now the variants are fairly managable.
> > 
> > v2: Stricter size checks
> > 
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> 
> Since Chris mentioned that this should fix a regression I applied it to
> drm-intel-fixes.

Great! Will you merge the other patch in the series to the nightly
build?


Regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Daniel Vetter]

On Wed, Aug 05, 2015 at 06:32:01PM +0300, David Weinehall wrote:
> On Wed, Aug 05, 2015 at 10:59:00AM +0200, Daniel Vetter wrote:
> > On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
> > > VBT version 196 increased the size of common_child_dev_config. The parser
> > > code assumed that the size of this structure would not change.
> > > 
> > > The modified code now copies the amount needed based on the VBT version,
> > > and emits a debug message if the VBT version is unknown (too new);
> > > since the struct config block won't shrink in newer versions it should
> > > be harmless to copy the maximum known size in such cases, so that's
> > > what we do, but emitting the warning is probably sensible anyway.
> > > 
> > > In the longer run it might make sense to modify the parser code to
> > > use a version/feature mapping, rather than hardcoding things like this,
> > > but for now the variants are fairly managable.
> > > 
> > > v2: Stricter size checks
> > > 
> > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > 
> > Since Chris mentioned that this should fix a regression I applied it to
> > drm-intel-fixes.
> 
> Great! Will you merge the other patch in the series to the nightly
> build?

I only merged this fast-tracked because it fixes a regression. For the
other patch normal review rules still apply (i.e. I won't do it).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Michel Thierry]

On 8/5/2015 9:59 AM, Daniel Vetter wrote:
> On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
>> VBT version 196 increased the size of common_child_dev_config. The parser
>> code assumed that the size of this structure would not change.
>>
>> The modified code now copies the amount needed based on the VBT version,
>> and emits a debug message if the VBT version is unknown (too new);
>> since the struct config block won't shrink in newer versions it should
>> be harmless to copy the maximum known size in such cases, so that's
>> what we do, but emitting the warning is probably sensible anyway.
>>
>> In the longer run it might make sense to modify the parser code to
>> use a version/feature mapping, rather than hardcoding things like this,
>> but for now the variants are fairly managable.
>>
>> v2: Stricter size checks
>>
>> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>
> Since Chris mentioned that this should fix a regression I applied it to
> drm-intel-fixes.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/intel_bios.c |   26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 2ff9eb00fdec..8a1f3e1fc598 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -1015,15 +1015,33 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>        const union child_device_config *p_child;
>>        union child_device_config *child_dev_ptr;
>>        int i, child_device_num, count;
>> -     u16     block_size;
>> +     u8 expected_size;
>> +     u16 block_size;
>>
>>        p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>>        if (!p_defs) {
>>                DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>                return;
>>        }
>> -     if (p_defs->child_dev_size < sizeof(*p_child)) {
>> -             DRM_ERROR("General definiton block child device size is too small.\n");
>> +     if (bdb->version < 195) {
>> +             expected_size = 33;
>> +     } else if (bdb->version == 195) {
>> +             expected_size = 37;
>> +     } else if (bdb->version <= 197) {
>> +             expected_size = 38;
>> +     } else {
>> +             expected_size = 38;
>> +             DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size);

Hi, the line above throws a warning because there are two '%u' but only 
one variable. Looks like bdb->version should be the 1st printed value.

>> +     }
>> +
>> +     if (expected_size > sizeof(*p_child)) {
>> +             DRM_ERROR("child_device_config cannot fit in p_child\n");
>> +             return;
>> +     }
>> +
>> +     if (p_defs->child_dev_size != expected_size) {
>> +             DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
>> +                       p_defs->child_dev_size, expected_size, bdb->version);
>>                return;
>>        }
>>        /* get the block size of general definitions */
>> @@ -1070,7 +1088,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>
>>                child_dev_ptr = dev_priv->vbt.child_dev + count;
>>                count++;
>> -             memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>> +             memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>>        }
>>        return;
>>   }
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Thu, Aug 06, 2015 at 02:59:10PM +0100, Michel Thierry wrote:
> On 8/5/2015 9:59 AM, Daniel Vetter wrote:
> >On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
> >>VBT version 196 increased the size of common_child_dev_config. The parser
> >>code assumed that the size of this structure would not change.
> >>
> >>The modified code now copies the amount needed based on the VBT version,
> >>and emits a debug message if the VBT version is unknown (too new);
> >>since the struct config block won't shrink in newer versions it should
> >>be harmless to copy the maximum known size in such cases, so that's
> >>what we do, but emitting the warning is probably sensible anyway.
> >>
> >>In the longer run it might make sense to modify the parser code to
> >>use a version/feature mapping, rather than hardcoding things like this,
> >>but for now the variants are fairly managable.
> >>
> >>v2: Stricter size checks
> >>
> >>Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> >
> >Since Chris mentioned that this should fix a regression I applied it to
> >drm-intel-fixes.
> >-Daniel
> >
> >>---
> >>  drivers/gpu/drm/i915/intel_bios.c |   26 ++++++++++++++++++++++----
> >>  1 file changed, 22 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >>index 2ff9eb00fdec..8a1f3e1fc598 100644
> >>--- a/drivers/gpu/drm/i915/intel_bios.c
> >>+++ b/drivers/gpu/drm/i915/intel_bios.c
> >>@@ -1015,15 +1015,33 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >>       const union child_device_config *p_child;
> >>       union child_device_config *child_dev_ptr;
> >>       int i, child_device_num, count;
> >>-     u16     block_size;
> >>+     u8 expected_size;
> >>+     u16 block_size;
> >>
> >>       p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
> >>       if (!p_defs) {
> >>               DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
> >>               return;
> >>       }
> >>-     if (p_defs->child_dev_size < sizeof(*p_child)) {
> >>-             DRM_ERROR("General definiton block child device size is too small.\n");
> >>+     if (bdb->version < 195) {
> >>+             expected_size = 33;
> >>+     } else if (bdb->version == 195) {
> >>+             expected_size = 37;
> >>+     } else if (bdb->version <= 197) {
> >>+             expected_size = 38;
> >>+     } else {
> >>+             expected_size = 38;
> >>+             DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size);
> 
> Hi, the line above throws a warning because there are two '%u' but only one
> variable. Looks like bdb->version should be the 1st printed value.

Good catch, your analysis is indeed correct.

Daniel, will you do the fixup or should I submit a fixed version?


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Per-DDI I_boost override

[David Weinehall]

On Fri, Jul 10, 2015 at 02:10:55PM +0300, Antti Koskipaa wrote:
> An OEM may request increased I_boost beyond the recommended values
> by specifying an I_boost value to be applied to all swing entries for
> a port. These override values are specified in VBT.
> 
> v2: rebase and remove unused iboost_bit variable
> 
> Issue: VIZ-5676
> Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>

Now that patch 1/2 has been updated, I'm ready to give this:

Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Thu, Aug 06, 2015 at 02:18:35PM +0200, Daniel Vetter wrote:
> On Wed, Aug 05, 2015 at 06:32:01PM +0300, David Weinehall wrote:
> > On Wed, Aug 05, 2015 at 10:59:00AM +0200, Daniel Vetter wrote:
> > > On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
> > > > VBT version 196 increased the size of common_child_dev_config. The parser
> > > > code assumed that the size of this structure would not change.
> > > > 
> > > > The modified code now copies the amount needed based on the VBT version,
> > > > and emits a debug message if the VBT version is unknown (too new);
> > > > since the struct config block won't shrink in newer versions it should
> > > > be harmless to copy the maximum known size in such cases, so that's
> > > > what we do, but emitting the warning is probably sensible anyway.
> > > > 
> > > > In the longer run it might make sense to modify the parser code to
> > > > use a version/feature mapping, rather than hardcoding things like this,
> > > > but for now the variants are fairly managable.
> > > > 
> > > > v2: Stricter size checks
> > > > 
> > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > > 
> > > Since Chris mentioned that this should fix a regression I applied it to
> > > drm-intel-fixes.
> > 
> > Great! Will you merge the other patch in the series to the nightly
> > build?
> 
> I only merged this fast-tracked because it fixes a regression. For the
> other patch normal review rules still apply (i.e. I won't do it).

I wasn't expecting fast tracking.  I'd missed the fact that patch 2/2
wasn't reviewed by anyone yet; I've done so now.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Daniel Vetter]

On Thu, Aug 06, 2015 at 05:08:55PM +0300, David Weinehall wrote:
> On Thu, Aug 06, 2015 at 02:59:10PM +0100, Michel Thierry wrote:
> > On 8/5/2015 9:59 AM, Daniel Vetter wrote:
> > >On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
> > >>VBT version 196 increased the size of common_child_dev_config. The parser
> > >>code assumed that the size of this structure would not change.
> > >>
> > >>The modified code now copies the amount needed based on the VBT version,
> > >>and emits a debug message if the VBT version is unknown (too new);
> > >>since the struct config block won't shrink in newer versions it should
> > >>be harmless to copy the maximum known size in such cases, so that's
> > >>what we do, but emitting the warning is probably sensible anyway.
> > >>
> > >>In the longer run it might make sense to modify the parser code to
> > >>use a version/feature mapping, rather than hardcoding things like this,
> > >>but for now the variants are fairly managable.
> > >>
> > >>v2: Stricter size checks
> > >>
> > >>Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > >
> > >Since Chris mentioned that this should fix a regression I applied it to
> > >drm-intel-fixes.
> > >-Daniel
> > >
> > >>---
> > >>  drivers/gpu/drm/i915/intel_bios.c |   26 ++++++++++++++++++++++----
> > >>  1 file changed, 22 insertions(+), 4 deletions(-)
> > >>
> > >>diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > >>index 2ff9eb00fdec..8a1f3e1fc598 100644
> > >>--- a/drivers/gpu/drm/i915/intel_bios.c
> > >>+++ b/drivers/gpu/drm/i915/intel_bios.c
> > >>@@ -1015,15 +1015,33 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> > >>       const union child_device_config *p_child;
> > >>       union child_device_config *child_dev_ptr;
> > >>       int i, child_device_num, count;
> > >>-     u16     block_size;
> > >>+     u8 expected_size;
> > >>+     u16 block_size;
> > >>
> > >>       p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
> > >>       if (!p_defs) {
> > >>               DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
> > >>               return;
> > >>       }
> > >>-     if (p_defs->child_dev_size < sizeof(*p_child)) {
> > >>-             DRM_ERROR("General definiton block child device size is too small.\n");
> > >>+     if (bdb->version < 195) {
> > >>+             expected_size = 33;
> > >>+     } else if (bdb->version == 195) {
> > >>+             expected_size = 37;
> > >>+     } else if (bdb->version <= 197) {
> > >>+             expected_size = 38;
> > >>+     } else {
> > >>+             expected_size = 38;
> > >>+             DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size);
> > 
> > Hi, the line above throws a warning because there are two '%u' but only one
> > variable. Looks like bdb->version should be the 1st printed value.
> 
> Good catch, your analysis is indeed correct.
> 
> Daniel, will you do the fixup or should I submit a fixed version?

Fixed up locally.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Per-DDI I_boost override

[Daniel Vetter]

On Thu, Aug 06, 2015 at 05:11:03PM +0300, David Weinehall wrote:
> On Fri, Jul 10, 2015 at 02:10:55PM +0300, Antti Koskipaa wrote:
> > An OEM may request increased I_boost beyond the recommended values
> > by specifying an I_boost value to be applied to all swing entries for
> > a port. These override values are specified in VBT.
> > 
> > v2: rebase and remove unused iboost_bit variable
> > 
> > Issue: VIZ-5676
> > Signed-off-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> 
> Now that patch 1/2 has been updated, I'm ready to give this:
> 
> Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>

Someone needs to ping me in a few weeks about this one again since atm I
can't merge it do -next, need a backmerge first.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2 addendum] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

Some more fixup is needed; the bits from Antti's patch
that actually expanded the struct to fully fit the newer
versions of the child_device_config was part of the second
patch; since that patch hasn't been merged yet we need this bit:

This applies on top of the patch you already merged
(the Iboost patch will need corresponding adjustment to
 remove the changes I split out):

Expand common_child_dev_config to be able to fit all information
defined by the latest VBT specification.

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
---
 intel_bios.c |    7 ++++++-
 intel_bios.h |    4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 990acc20771a..40e2cc4e7419 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
 		return;
 	}
+	/* Remember to keep this in sync with child_device_config;
+	 * whenever a new feature is added to BDB that causes that
+	 * struct to grow this needs to be updated too
+	 */
 	if (bdb->version < 195) {
 		expected_size = 33;
 	} else if (bdb->version == 195) {
@@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	}
 
 	if (expected_size > sizeof(*p_child)) {
-		DRM_ERROR("child_device_config cannot fit in p_child\n");
+		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
+			  expected_size, sizeof(*p_child), bdb->version);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index f7ad6a585129..71cb96f77870 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -239,6 +239,10 @@ struct common_child_dev_config {
 	u8 not_common2[2];
 	u8 ddc_pin;
 	u16 edid_ptr;
+	u8 obsolete;
+	u8 flags_1;
+	u8 not_common3[13];
+	u8 iboost_level;
 } __packed;
 
 /* This field changes depending on the BDB version, so the most reliable way to
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2 addendum] drm/i915: Allow parsing of variable size child device entries from VBT

[Jani Nikula]

On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
> Some more fixup is needed; the bits from Antti's patch
> that actually expanded the struct to fully fit the newer
> versions of the child_device_config was part of the second
> patch; since that patch hasn't been merged yet we need this bit:
>
> This applies on top of the patch you already merged
> (the Iboost patch will need corresponding adjustment to
>  remove the changes I split out):
>
> Expand common_child_dev_config to be able to fit all information
> defined by the latest VBT specification.
>
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> ---
>  intel_bios.c |    7 ++++++-
>  intel_bios.h |    4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 990acc20771a..40e2cc4e7419 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> +	/* Remember to keep this in sync with child_device_config;
> +	 * whenever a new feature is added to BDB that causes that
> +	 * struct to grow this needs to be updated too
> +	 */

BUILD_BUG_ON(something about sizeof child device config) ?

>  	if (bdb->version < 195) {
>  		expected_size = 33;
>  	} else if (bdb->version == 195) {
> @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	}
>  
>  	if (expected_size > sizeof(*p_child)) {
> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
> +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
> +			  expected_size, sizeof(*p_child), bdb->version);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index f7ad6a585129..71cb96f77870 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -239,6 +239,10 @@ struct common_child_dev_config {
>  	u8 not_common2[2];
>  	u8 ddc_pin;
>  	u16 edid_ptr;
> +	u8 obsolete;
> +	u8 flags_1;
> +	u8 not_common3[13];
> +	u8 iboost_level;
>  } __packed;
>  
>  /* This field changes depending on the BDB version, so the most reliable way to
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2 v2 addendum] drm/i915: Allow parsing of variable size child device entries from VBT

[Jani Nikula]

On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
> Some more fixup is needed; the bits from Antti's patch
> that actually expanded the struct to fully fit the newer
> versions of the child_device_config was part of the second
> patch; since that patch hasn't been merged yet we need this bit:
>
> This applies on top of the patch you already merged
> (the Iboost patch will need corresponding adjustment to
>  remove the changes I split out):
>
> Expand common_child_dev_config to be able to fit all information
> defined by the latest VBT specification.
>
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> ---
>  intel_bios.c |    7 ++++++-
>  intel_bios.h |    4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 990acc20771a..40e2cc4e7419 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> +	/* Remember to keep this in sync with child_device_config;
> +	 * whenever a new feature is added to BDB that causes that
> +	 * struct to grow this needs to be updated too
> +	 */
>  	if (bdb->version < 195) {
>  		expected_size = 33;
>  	} else if (bdb->version == 195) {
> @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	}
>  
>  	if (expected_size > sizeof(*p_child)) {
> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
> +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
> +			  expected_size, sizeof(*p_child), bdb->version);

drivers/gpu/drm/i915/intel_bios.c: In function ‘parse_device_mapping’:
drivers/gpu/drm/i915/intel_bios.c:1058:3: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
   DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
   ^
  CC [M]  drivers/gpu/drm/i915/intel_fifo_underrun.o


>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index f7ad6a585129..71cb96f77870 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -239,6 +239,10 @@ struct common_child_dev_config {
>  	u8 not_common2[2];
>  	u8 ddc_pin;
>  	u16 edid_ptr;
> +	u8 obsolete;
> +	u8 flags_1;
> +	u8 not_common3[13];
> +	u8 iboost_level;
>  } __packed;
>  
>  /* This field changes depending on the BDB version, so the most reliable way to
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2 v2 addendum] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Wed, Aug 12, 2015 at 03:13:35PM +0300, Jani Nikula wrote:
> On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
> > Some more fixup is needed; the bits from Antti's patch
> > that actually expanded the struct to fully fit the newer
> > versions of the child_device_config was part of the second
> > patch; since that patch hasn't been merged yet we need this bit:
> >
> > This applies on top of the patch you already merged
> > (the Iboost patch will need corresponding adjustment to
> >  remove the changes I split out):
> >
> > Expand common_child_dev_config to be able to fit all information
> > defined by the latest VBT specification.
> >
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> > ---
> >  intel_bios.c |    7 ++++++-
> >  intel_bios.h |    4 ++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 990acc20771a..40e2cc4e7419 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
> >  		return;
> >  	}
> > +	/* Remember to keep this in sync with child_device_config;
> > +	 * whenever a new feature is added to BDB that causes that
> > +	 * struct to grow this needs to be updated too
> > +	 */
> 
> BUILD_BUG_ON(something about sizeof child device config) ?

The idea is nice, but...  We get the size to copy (expected_size)
from the version-switch statement (so it's not available during build);
thus we cannot know at compile time whether expected_size is larger than
sizeof(child_device_config).

This is yet another argument in favour of a version:feature
table I think; that would allow for compile-time validation.

Unless someone else volunteers to refactor this code I can dig in once
I get back from vacation.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2 addendum] drm/i915: Allow parsing of variable size child device entries from VBT

[Jani Nikula]

On Thu, 13 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
> On Wed, Aug 12, 2015 at 03:13:35PM +0300, Jani Nikula wrote:
>> On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
>> > Some more fixup is needed; the bits from Antti's patch
>> > that actually expanded the struct to fully fit the newer
>> > versions of the child_device_config was part of the second
>> > patch; since that patch hasn't been merged yet we need this bit:
>> >
>> > This applies on top of the patch you already merged
>> > (the Iboost patch will need corresponding adjustment to
>> >  remove the changes I split out):
>> >
>> > Expand common_child_dev_config to be able to fit all information
>> > defined by the latest VBT specification.
>> >
>> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>> > CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>> > ---
>> >  intel_bios.c |    7 ++++++-
>> >  intel_bios.h |    4 ++++
>> >  2 files changed, 10 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> > index 990acc20771a..40e2cc4e7419 100644
>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>> >  		return;
>> >  	}
>> > +	/* Remember to keep this in sync with child_device_config;
>> > +	 * whenever a new feature is added to BDB that causes that
>> > +	 * struct to grow this needs to be updated too
>> > +	 */
>> 
>> BUILD_BUG_ON(something about sizeof child device config) ?
>
> The idea is nice, but...  We get the size to copy (expected_size)
> from the version-switch statement (so it's not available during build);
> thus we cannot know at compile time whether expected_size is larger than
> sizeof(child_device_config).
>
> This is yet another argument in favour of a version:feature
> table I think; that would allow for compile-time validation.
>
> Unless someone else volunteers to refactor this code I can dig in once
> I get back from vacation.

Right. To be clear, my question should not block this from being merged.

BR,
Jani

>
>
> Kind regards, David

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

Re: [PATCH 1/2 v2 addendum v2] drm/i915: Allow parsing of variable size child device entries from VBT

[David Weinehall]

On Wed, Aug 12, 2015 at 05:19:35PM +0300, Jani Nikula wrote:
> On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
> > Some more fixup is needed; the bits from Antti's patch
> > that actually expanded the struct to fully fit the newer
> > versions of the child_device_config was part of the second
> > patch; since that patch hasn't been merged yet we need this bit:
> >
> > This applies on top of the patch you already merged
> > (the Iboost patch will need corresponding adjustment to
> >  remove the changes I split out):
> >
> > Expand common_child_dev_config to be able to fit all information
> > defined by the latest VBT specification.
> >
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> > ---
> >  intel_bios.c |    7 ++++++-
> >  intel_bios.h |    4 ++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 990acc20771a..40e2cc4e7419 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
> >  		return;
> >  	}
> > +	/* Remember to keep this in sync with child_device_config;
> > +	 * whenever a new feature is added to BDB that causes that
> > +	 * struct to grow this needs to be updated too
> > +	 */
> >  	if (bdb->version < 195) {
> >  		expected_size = 33;
> >  	} else if (bdb->version == 195) {
> > @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >  	}
> >  
> >  	if (expected_size > sizeof(*p_child)) {
> > -		DRM_ERROR("child_device_config cannot fit in p_child\n");
> > +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
> > +			  expected_size, sizeof(*p_child), bdb->version);
> 
> drivers/gpu/drm/i915/intel_bios.c: In function ‘parse_device_mapping’:
> drivers/gpu/drm/i915/intel_bios.c:1058:3: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
>    DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>    ^
>   CC [M]  drivers/gpu/drm/i915/intel_fifo_underrun.o

Well spotted.  Or rather, stupid of me to forget that sizeof yields
size_t as type, thus necessitating %zu as conversion specifier.

Fixed version:

Expand common_child_dev_config to be able to fit all information
defined by the latest VBT specification.

v2: Use proper conversion specifier for size_t (%zu)

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>

 intel_bios.c |    7 ++++++-
 intel_bios.h |    4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 990acc20771a..5673ed53797b 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
 		return;
 	}
+	/* Remember to keep this in sync with child_device_config;
+	 * whenever a new feature is added to BDB that causes that
+	 * struct to grow this needs to be updated too
+	 */
 	if (bdb->version < 195) {
 		expected_size = 33;
 	} else if (bdb->version == 195) {
@@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	}
 
 	if (expected_size > sizeof(*p_child)) {
-		DRM_ERROR("child_device_config cannot fit in p_child\n");
+		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %zu); bdb->version: %u\n",
+			  expected_size, sizeof(*p_child), bdb->version);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index f7ad6a585129..71cb96f77870 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -239,6 +239,10 @@ struct common_child_dev_config {
 	u8 not_common2[2];
 	u8 ddc_pin;
 	u16 edid_ptr;
+	u8 obsolete;
+	u8 flags_1;
+	u8 not_common3[13];
+	u8 iboost_level;
 } __packed;
 
 /* This field changes depending on the BDB version, so the most reliable way to
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2 v2 addendum v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Timo Aaltonen]

On 13.08.2015 16:14, David Weinehall wrote:
> On Wed, Aug 12, 2015 at 05:19:35PM +0300, Jani Nikula wrote:
>> On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
>>> Some more fixup is needed; the bits from Antti's patch
>>> that actually expanded the struct to fully fit the newer
>>> versions of the child_device_config was part of the second
>>> patch; since that patch hasn't been merged yet we need this bit:
>>>
>>> This applies on top of the patch you already merged
>>> (the Iboost patch will need corresponding adjustment to
>>>  remove the changes I split out):
>>>
>>> Expand common_child_dev_config to be able to fit all information
>>> defined by the latest VBT specification.
>>>
>>> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>>> CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>>> ---
>>>  intel_bios.c |    7 ++++++-
>>>  intel_bios.h |    4 ++++
>>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> index 990acc20771a..40e2cc4e7419 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>>  		return;
>>>  	}
>>> +	/* Remember to keep this in sync with child_device_config;
>>> +	 * whenever a new feature is added to BDB that causes that
>>> +	 * struct to grow this needs to be updated too
>>> +	 */
>>>  	if (bdb->version < 195) {
>>>  		expected_size = 33;
>>>  	} else if (bdb->version == 195) {
>>> @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>>  	}
>>>  
>>>  	if (expected_size > sizeof(*p_child)) {
>>> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
>>> +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>>> +			  expected_size, sizeof(*p_child), bdb->version);
>>
>> drivers/gpu/drm/i915/intel_bios.c: In function ‘parse_device_mapping’:
>> drivers/gpu/drm/i915/intel_bios.c:1058:3: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
>>    DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>>    ^
>>   CC [M]  drivers/gpu/drm/i915/intel_fifo_underrun.o
> 
> Well spotted.  Or rather, stupid of me to forget that sizeof yields
> size_t as type, thus necessitating %zu as conversion specifier.
> 
> Fixed version:
> 
> Expand common_child_dev_config to be able to fit all information
> defined by the latest VBT specification.
> 
> v2: Use proper conversion specifier for size_t (%zu)
> 
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> 
>  intel_bios.c |    7 ++++++-
>  intel_bios.h |    4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 990acc20771a..5673ed53797b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> +	/* Remember to keep this in sync with child_device_config;
> +	 * whenever a new feature is added to BDB that causes that
> +	 * struct to grow this needs to be updated too
> +	 */
>  	if (bdb->version < 195) {
>  		expected_size = 33;
>  	} else if (bdb->version == 195) {
> @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	}
>  
>  	if (expected_size > sizeof(*p_child)) {
> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
> +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %zu); bdb->version: %u\n",
> +			  expected_size, sizeof(*p_child), bdb->version);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index f7ad6a585129..71cb96f77870 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -239,6 +239,10 @@ struct common_child_dev_config {
>  	u8 not_common2[2];
>  	u8 ddc_pin;
>  	u16 edid_ptr;
> +	u8 obsolete;
> +	u8 flags_1;
> +	u8 not_common3[13];
> +	u8 iboost_level;
>  } __packed;
>  
>  /* This field changes depending on the BDB version, so the most reliable way to

Please apply this, fixes issues on some machine(s) here. Probably also
cc:stable since it applies to BSW as well as SKL.

and the upstream bug could be referenced too, it's
https://bugs.freedesktop.org/show_bug.cgi?id=91613

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

Re: [PATCH 1/2 v2 addendum v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Jani Nikula]

On Thu, 13 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
> On Wed, Aug 12, 2015 at 05:19:35PM +0300, Jani Nikula wrote:
>> On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
>> > Some more fixup is needed; the bits from Antti's patch
>> > that actually expanded the struct to fully fit the newer
>> > versions of the child_device_config was part of the second
>> > patch; since that patch hasn't been merged yet we need this bit:
>> >
>> > This applies on top of the patch you already merged
>> > (the Iboost patch will need corresponding adjustment to
>> >  remove the changes I split out):
>> >
>> > Expand common_child_dev_config to be able to fit all information
>> > defined by the latest VBT specification.
>> >
>> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>> > CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>> > ---
>> >  intel_bios.c |    7 ++++++-
>> >  intel_bios.h |    4 ++++
>> >  2 files changed, 10 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> > index 990acc20771a..40e2cc4e7419 100644
>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>> >  		return;
>> >  	}
>> > +	/* Remember to keep this in sync with child_device_config;
>> > +	 * whenever a new feature is added to BDB that causes that
>> > +	 * struct to grow this needs to be updated too
>> > +	 */
>> >  	if (bdb->version < 195) {
>> >  		expected_size = 33;
>> >  	} else if (bdb->version == 195) {
>> > @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >  	}
>> >  
>> >  	if (expected_size > sizeof(*p_child)) {
>> > -		DRM_ERROR("child_device_config cannot fit in p_child\n");
>> > +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>> > +			  expected_size, sizeof(*p_child), bdb->version);
>> 
>> drivers/gpu/drm/i915/intel_bios.c: In function ‘parse_device_mapping’:
>> drivers/gpu/drm/i915/intel_bios.c:1058:3: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
>>    DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>>    ^
>>   CC [M]  drivers/gpu/drm/i915/intel_fifo_underrun.o
>
> Well spotted.  Or rather, stupid of me to forget that sizeof yields
> size_t as type, thus necessitating %zu as conversion specifier.
>
> Fixed version:
>
> Expand common_child_dev_config to be able to fit all information
> defined by the latest VBT specification.
>
> v2: Use proper conversion specifier for size_t (%zu)
>
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>
>  intel_bios.c |    7 ++++++-
>  intel_bios.h |    4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 990acc20771a..5673ed53797b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> +	/* Remember to keep this in sync with child_device_config;
> +	 * whenever a new feature is added to BDB that causes that
> +	 * struct to grow this needs to be updated too
> +	 */
>  	if (bdb->version < 195) {
>  		expected_size = 33;
>  	} else if (bdb->version == 195) {
> @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	}
>  
>  	if (expected_size > sizeof(*p_child)) {
> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
> +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %zu); bdb->version: %u\n",
> +			  expected_size, sizeof(*p_child), bdb->version);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index f7ad6a585129..71cb96f77870 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -239,6 +239,10 @@ struct common_child_dev_config {
>  	u8 not_common2[2];
>  	u8 ddc_pin;
>  	u16 edid_ptr;
> +	u8 obsolete;
> +	u8 flags_1;
> +	u8 not_common3[13];
> +	u8 iboost_level;

How does this impact the if (p_defs->child_dev_size != sizeof(*p_child))
check in parse_sdvo_device_mapping()?

BR,
Jani.

>  } __packed;
>  
>  /* This field changes depending on the BDB version, so the most reliable way to

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

Re: [PATCH 1/2 v2 addendum v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Jani Nikula]

On Fri, 14 Aug 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 13 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
>> On Wed, Aug 12, 2015 at 05:19:35PM +0300, Jani Nikula wrote:
>>> On Wed, 12 Aug 2015, David Weinehall <david.weinehall@linux.intel.com> wrote:
>>> > Some more fixup is needed; the bits from Antti's patch
>>> > that actually expanded the struct to fully fit the newer
>>> > versions of the child_device_config was part of the second
>>> > patch; since that patch hasn't been merged yet we need this bit:
>>> >
>>> > This applies on top of the patch you already merged
>>> > (the Iboost patch will need corresponding adjustment to
>>> >  remove the changes I split out):
>>> >
>>> > Expand common_child_dev_config to be able to fit all information
>>> > defined by the latest VBT specification.
>>> >
>>> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>>> > CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>>> > ---
>>> >  intel_bios.c |    7 ++++++-
>>> >  intel_bios.h |    4 ++++
>>> >  2 files changed, 10 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> > index 990acc20771a..40e2cc4e7419 100644
>>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> > @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>> >  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>> >  		return;
>>> >  	}
>>> > +	/* Remember to keep this in sync with child_device_config;
>>> > +	 * whenever a new feature is added to BDB that causes that
>>> > +	 * struct to grow this needs to be updated too
>>> > +	 */
>>> >  	if (bdb->version < 195) {
>>> >  		expected_size = 33;
>>> >  	} else if (bdb->version == 195) {
>>> > @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>> >  	}
>>> >  
>>> >  	if (expected_size > sizeof(*p_child)) {
>>> > -		DRM_ERROR("child_device_config cannot fit in p_child\n");
>>> > +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>>> > +			  expected_size, sizeof(*p_child), bdb->version);
>>> 
>>> drivers/gpu/drm/i915/intel_bios.c: In function ‘parse_device_mapping’:
>>> drivers/gpu/drm/i915/intel_bios.c:1058:3: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
>>>    DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %u); bdb->version: %u\n",
>>>    ^
>>>   CC [M]  drivers/gpu/drm/i915/intel_fifo_underrun.o
>>
>> Well spotted.  Or rather, stupid of me to forget that sizeof yields
>> size_t as type, thus necessitating %zu as conversion specifier.
>>
>> Fixed version:
>>
>> Expand common_child_dev_config to be able to fit all information
>> defined by the latest VBT specification.
>>
>> v2: Use proper conversion specifier for size_t (%zu)
>>
>> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>> CC: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>>
>>  intel_bios.c |    7 ++++++-
>>  intel_bios.h |    4 ++++
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 990acc20771a..5673ed53797b 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -1038,6 +1038,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>  		return;
>>  	}
>> +	/* Remember to keep this in sync with child_device_config;
>> +	 * whenever a new feature is added to BDB that causes that
>> +	 * struct to grow this needs to be updated too
>> +	 */
>>  	if (bdb->version < 195) {
>>  		expected_size = 33;
>>  	} else if (bdb->version == 195) {
>> @@ -1051,7 +1055,8 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>  	}
>>  
>>  	if (expected_size > sizeof(*p_child)) {
>> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
>> +		DRM_ERROR("child_device_config (size %u) cannot fit in p_child (size %zu); bdb->version: %u\n",
>> +			  expected_size, sizeof(*p_child), bdb->version);
>>  		return;
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index f7ad6a585129..71cb96f77870 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -239,6 +239,10 @@ struct common_child_dev_config {
>>  	u8 not_common2[2];
>>  	u8 ddc_pin;
>>  	u16 edid_ptr;
>> +	u8 obsolete;
>> +	u8 flags_1;
>> +	u8 not_common3[13];
>> +	u8 iboost_level;
>
> How does this impact the if (p_defs->child_dev_size != sizeof(*p_child))
> check in parse_sdvo_device_mapping()?

Ugh. What a mess the whole child device parsing is.

What's really broken in the current code is *assuming* some expected
size for future versions, *and* then refusing to parse if the child dev
size has grown. We should just parse *up to* min(VBT reported child dev
size, the biggest child dev we know about). The current code keeps us
doing busywork whenever someone decides to add a field at the end.

Side note, we have *two* functions and loops iterating child
devices. One for new stuff and another for old stuff. Yay.

BR,
Jani.


>
> BR,
> Jani.
>
>>  } __packed;
>>  
>>  /* This field changes depending on the BDB version, so the most reliable way to
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

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