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

[Damien Lespiau <damien.lespiau@intel.com>]

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