[PATCH] drm/i915: Fix the VBT child device parsing for BSW

[PATCH] drm/i915: Fix the VBT child device parsing for BSW

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Recent BSW VBT has a VBT child device size 37 bytes instead of the 33
bytes our code assumes. This means we fail to parse the VBT and thus
fail to detect eDP ports properly and just register them as DP ports
instead.

Fix it up by using the reported child device size from the VBT instead
of assuming it matches out struct defintions.

The latest spec I have shows that the child device size should be 36
bytes for rev >= 195, however on my BSW the size is actually 37 bytes.
And our current struct definition is 33 bytes.

Feels like the entire VBT parses would need to be rewritten to handle
changes in the layout better, but for now I've decided to do just the
bare minimum to get my eDP port back.

Cc: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 26 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_bios.h |  4 ++--
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index c684085..4cbd325 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -447,6 +447,12 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
 	}
 }
 
+static union child_device_config *
+child_device_ptr(struct bdb_general_definitions *p_defs, int i)
+{
+	return (void *) &p_defs->devices[i * p_defs->child_dev_size];
+}
+
 static void
 parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 			  struct bdb_header *bdb)
@@ -476,10 +482,10 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 	block_size = get_blocksize(p_defs);
 	/* get the number of child device */
 	child_device_num = (block_size - sizeof(*p_defs)) /
-				sizeof(*p_child);
+		p_defs->child_dev_size;
 	count = 0;
 	for (i = 0; i < child_device_num; i++) {
-		p_child = &(p_defs->devices[i]);
+		p_child = child_device_ptr(p_defs, i);
 		if (!p_child->old.device_type) {
 			/* skip the device block if device type is invalid */
 			continue;
@@ -1067,25 +1073,19 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
 		return;
 	}
-	/* judge whether the size of child device meets the requirements.
-	 * If the child device size obtained from general definition block
-	 * is different with sizeof(struct child_device_config), skip the
-	 * parsing of sdvo device info
-	 */
-	if (p_defs->child_dev_size != sizeof(*p_child)) {
-		/* different child dev size . Ignore it */
-		DRM_DEBUG_KMS("different child size is found. Invalid.\n");
+	if (p_defs->child_dev_size < sizeof(*p_child)) {
+		DRM_ERROR("General definiton block child device size is too small.\n");
 		return;
 	}
 	/* get the block size of general definitions */
 	block_size = get_blocksize(p_defs);
 	/* get the number of child device */
 	child_device_num = (block_size - sizeof(*p_defs)) /
-				sizeof(*p_child);
+				p_defs->child_dev_size;
 	count = 0;
 	/* get the number of child device that is present */
 	for (i = 0; i < child_device_num; i++) {
-		p_child = &(p_defs->devices[i]);
+		p_child = child_device_ptr(p_defs, i);
 		if (!p_child->common.device_type) {
 			/* skip the device block if device type is invalid */
 			continue;
@@ -1105,7 +1105,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	dev_priv->vbt.child_dev_num = count;
 	count = 0;
 	for (i = 0; i < child_device_num; i++) {
-		p_child = &(p_defs->devices[i]);
+		p_child = child_device_ptr(p_defs, i);
 		if (!p_child->common.device_type) {
 			/* skip the device block if device type is invalid */
 			continue;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 6afd5be..af0b476 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -277,9 +277,9 @@ struct bdb_general_definitions {
 	 * And the device num is related with the size of general definition
 	 * block. It is obtained by using the following formula:
 	 * number = (block_size - sizeof(bdb_general_definitions))/
-	 *	     sizeof(child_device_config);
+	 *	     defs->child_dev_size;
 	 */
-	union child_device_config devices[0];
+	uint8_t devices[0];
 } __packed;
 
 /* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
-- 
2.0.5

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

Re: [PATCH] drm/i915: Fix the VBT child device parsing for BSW

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6051
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  304/304              304/304
IVB                                  339/339              339/339
BYT                                  287/287              287/287
HSW                                  362/362              362/362
BDW                                  310/310              310/310
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix the VBT child device parsing for BSW

[Damien Lespiau]

On Wed, Mar 25, 2015 at 06:45:58PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Recent BSW VBT has a VBT child device size 37 bytes instead of the 33
> bytes our code assumes. This means we fail to parse the VBT and thus
> fail to detect eDP ports properly and just register them as DP ports
> instead.
> 
> Fix it up by using the reported child device size from the VBT instead
> of assuming it matches out struct defintions.
> 
> The latest spec I have shows that the child device size should be 36
> bytes for rev >= 195, however on my BSW the size is actually 37 bytes.
> And our current struct definition is 33 bytes.
> 
> Feels like the entire VBT parses would need to be rewritten to handle
> changes in the layout better, but for now I've decided to do just the
> bare minimum to get my eDP port back.
> 
> Cc: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Two minor comments, but, in any case, this is:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_bios.c | 26 +++++++++++++-------------
>  drivers/gpu/drm/i915/intel_bios.h |  4 ++--
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index c684085..4cbd325 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -447,6 +447,12 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static union child_device_config *
> +child_device_ptr(struct bdb_general_definitions *p_defs, int i)
> +{
> +	return (void *) &p_defs->devices[i * p_defs->child_dev_size];
> +}

We could use a child_device_num() helper as well.

>  static void
>  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>  			  struct bdb_header *bdb)
> @@ -476,10 +482,10 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>  	block_size = get_blocksize(p_defs);
>  	/* get the number of child device */
>  	child_device_num = (block_size - sizeof(*p_defs)) /
> -				sizeof(*p_child);
> +		p_defs->child_dev_size;
>  	count = 0;
>  	for (i = 0; i < child_device_num; i++) {
> -		p_child = &(p_defs->devices[i]);
> +		p_child = child_device_ptr(p_defs, i);
>  		if (!p_child->old.device_type) {
>  			/* skip the device block if device type is invalid */
>  			continue;
> @@ -1067,25 +1073,19 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> -	/* judge whether the size of child device meets the requirements.
> -	 * If the child device size obtained from general definition block
> -	 * is different with sizeof(struct child_device_config), skip the
> -	 * parsing of sdvo device info
> -	 */
> -	if (p_defs->child_dev_size != sizeof(*p_child)) {
> -		/* different child dev size . Ignore it */
> -		DRM_DEBUG_KMS("different child size is found. Invalid.\n");
> +	if (p_defs->child_dev_size < sizeof(*p_child)) {
> +		DRM_ERROR("General definiton block child device size is too small.\n");

definition

>  		return;
>  	}
>  	/* get the block size of general definitions */
>  	block_size = get_blocksize(p_defs);
>  	/* get the number of child device */
>  	child_device_num = (block_size - sizeof(*p_defs)) /
> -				sizeof(*p_child);
> +				p_defs->child_dev_size;
>  	count = 0;
>  	/* get the number of child device that is present */
>  	for (i = 0; i < child_device_num; i++) {
> -		p_child = &(p_defs->devices[i]);
> +		p_child = child_device_ptr(p_defs, i);
>  		if (!p_child->common.device_type) {
>  			/* skip the device block if device type is invalid */
>  			continue;
> @@ -1105,7 +1105,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	dev_priv->vbt.child_dev_num = count;
>  	count = 0;
>  	for (i = 0; i < child_device_num; i++) {
> -		p_child = &(p_defs->devices[i]);
> +		p_child = child_device_ptr(p_defs, i);
>  		if (!p_child->common.device_type) {
>  			/* skip the device block if device type is invalid */
>  			continue;
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 6afd5be..af0b476 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -277,9 +277,9 @@ struct bdb_general_definitions {
>  	 * And the device num is related with the size of general definition
>  	 * block. It is obtained by using the following formula:
>  	 * number = (block_size - sizeof(bdb_general_definitions))/
> -	 *	     sizeof(child_device_config);
> +	 *	     defs->child_dev_size;
>  	 */
> -	union child_device_config devices[0];
> +	uint8_t devices[0];
>  } __packed;
>  
>  /* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
> -- 
> 2.0.5
> 
> _______________________________________________
> 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] drm/i915: Fix the VBT child device parsing for BSW

[Damien Lespiau]

On Wed, Mar 25, 2015 at 06:45:58PM +0200, ville.syrjala@linux.intel.com wrote:
> +static union child_device_config *
> +child_device_ptr(struct bdb_general_definitions *p_defs, int i)
> +{
> +	return (void *) &p_defs->devices[i * p_defs->child_dev_size];
> +}

Actually this looks wrong. We're doing:

  p_defs->devices + sizeof(union child_device_config) * i * child_dev_size

instead of:

  (u8)p_defs->devices + i * child_dev_size

?

I wondered about the (void *) cast precedence over [], because gcc
treats void * pointer arithmetic with a size of 1, but [] will be done
first apparently.

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

Re: [PATCH] drm/i915: Fix the VBT child device parsing for BSW

[Ville Syrjälä]

On Thu, Apr 09, 2015 at 09:49:25AM +0100, Damien Lespiau wrote:
> On Wed, Mar 25, 2015 at 06:45:58PM +0200, ville.syrjala@linux.intel.com wrote:
> > +static union child_device_config *
> > +child_device_ptr(struct bdb_general_definitions *p_defs, int i)
> > +{
> > +	return (void *) &p_defs->devices[i * p_defs->child_dev_size];
> > +}
> 
> Actually this looks wrong. We're doing:
> 
>   p_defs->devices + sizeof(union child_device_config) * i * child_dev_size
> 
> instead of:
> 
>   (u8)p_defs->devices + i * child_dev_size
> 
> ?

The patch also had:
-     union child_device_config devices[0];
+     uint8_t devices[0];

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix the VBT child device parsing for BSW

[Damien Lespiau]

On Thu, Apr 09, 2015 at 01:37:10PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 09, 2015 at 09:49:25AM +0100, Damien Lespiau wrote:
> > On Wed, Mar 25, 2015 at 06:45:58PM +0200, ville.syrjala@linux.intel.com wrote:
> > > +static union child_device_config *
> > > +child_device_ptr(struct bdb_general_definitions *p_defs, int i)
> > > +{
> > > +	return (void *) &p_defs->devices[i * p_defs->child_dev_size];
> > > +}
> > 
> > Actually this looks wrong. We're doing:
> > 
> >   p_defs->devices + sizeof(union child_device_config) * i * child_dev_size
> > 
> > instead of:
> > 
> >   (u8)p_defs->devices + i * child_dev_size
> > 
> > ?
> 
> The patch also had:
> -     union child_device_config devices[0];
> +     uint8_t devices[0];

Ah, yes! oops

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

Re: [PATCH] drm/i915: Fix the VBT child device parsing for BSW

[Daniel Vetter]

On Thu, Apr 09, 2015 at 11:56:36AM +0100, Damien Lespiau wrote:
> On Thu, Apr 09, 2015 at 01:37:10PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 09, 2015 at 09:49:25AM +0100, Damien Lespiau wrote:
> > > On Wed, Mar 25, 2015 at 06:45:58PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > +static union child_device_config *
> > > > +child_device_ptr(struct bdb_general_definitions *p_defs, int i)
> > > > +{
> > > > +	return (void *) &p_defs->devices[i * p_defs->child_dev_size];
> > > > +}
> > > 
> > > Actually this looks wrong. We're doing:
> > > 
> > >   p_defs->devices + sizeof(union child_device_config) * i * child_dev_size
> > > 
> > > instead of:
> > > 
> > >   (u8)p_defs->devices + i * child_dev_size
> > > 
> > > ?
> > 
> > The patch also had:
> > -     union child_device_config devices[0];
> > +     uint8_t devices[0];
> 
> Ah, yes! oops

With the r-b reinstantiated I merged the patch, thanks.
-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