Re: [PATCH] drm/i915: fix VBT parsing for SDVO child device mapping

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 18 Aug 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Aug 18, 2015 at 02:28:55PM +0300, Jani Nikula wrote:
>> commit 75067ddecf21271631bc018d2fb23ddd09b66aae
>> Author: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>> Date:   Fri Jul 10 14:10:55 2015 +0300
>> 
>>     drm/i915: Per-DDI I_boost override
>> 
>> increased size of union child_device_config without taking into account
>> the size check in parse_sdvo_device_mapping(). Switch the function over
>> to using the legacy struct only.
>> 
>> Fixes: 75067ddecf21 ("drm/i915: Per-DDI I_boost override")
>> Cc: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>> Cc: David Weinehall <david.weinehall@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Also gave it a spin on my 946gz so:
> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-intel-next-fixes, thanks for the review and testing.

BR,
Jani.


>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 50 +++++++++++++++++++--------------------
>>  1 file changed, 25 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 8e46149bafdd..1b7e1a591a37 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -401,7 +401,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>>  {
>>  	struct sdvo_device_mapping *p_mapping;
>>  	const struct bdb_general_definitions *p_defs;
>> -	const union child_device_config *p_child;
>> +	const struct old_child_dev_config *child; /* legacy */
>>  	int i, child_device_num, count;
>>  	u16	block_size;
>>  
>> @@ -410,14 +410,14 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>>  		DRM_DEBUG_KMS("No general definition block is found, unable to construct sdvo mapping.\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
>> +
>> +	/*
>> +	 * Only parse SDVO mappings when the general definitions block child
>> +	 * device size matches that of the *legacy* child device config
>> +	 * struct. Thus, SDVO mapping will be skipped for newer VBT.
>>  	 */
>> -	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(*child)) {
>> +		DRM_DEBUG_KMS("Unsupported child device size for SDVO mapping.\n");
>>  		return;
>>  	}
>>  	/* get the block size of general definitions */
>> @@ -427,37 +427,37 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>>  		p_defs->child_dev_size;
>>  	count = 0;
>>  	for (i = 0; i < child_device_num; i++) {
>> -		p_child = child_device_ptr(p_defs, i);
>> -		if (!p_child->old.device_type) {
>> +		child = &child_device_ptr(p_defs, i)->old;
>> +		if (!child->device_type) {
>>  			/* skip the device block if device type is invalid */
>>  			continue;
>>  		}
>> -		if (p_child->old.slave_addr != SLAVE_ADDR1 &&
>> -			p_child->old.slave_addr != SLAVE_ADDR2) {
>> +		if (child->slave_addr != SLAVE_ADDR1 &&
>> +		    child->slave_addr != SLAVE_ADDR2) {
>>  			/*
>>  			 * If the slave address is neither 0x70 nor 0x72,
>>  			 * it is not a SDVO device. Skip it.
>>  			 */
>>  			continue;
>>  		}
>> -		if (p_child->old.dvo_port != DEVICE_PORT_DVOB &&
>> -			p_child->old.dvo_port != DEVICE_PORT_DVOC) {
>> +		if (child->dvo_port != DEVICE_PORT_DVOB &&
>> +		    child->dvo_port != DEVICE_PORT_DVOC) {
>>  			/* skip the incorrect SDVO port */
>>  			DRM_DEBUG_KMS("Incorrect SDVO port. Skip it\n");
>>  			continue;
>>  		}
>>  		DRM_DEBUG_KMS("the SDVO device with slave addr %2x is found on"
>> -				" %s port\n",
>> -				p_child->old.slave_addr,
>> -				(p_child->old.dvo_port == DEVICE_PORT_DVOB) ?
>> -					"SDVOB" : "SDVOC");
>> -		p_mapping = &(dev_priv->sdvo_mappings[p_child->old.dvo_port - 1]);
>> +			      " %s port\n",
>> +			      child->slave_addr,
>> +			      (child->dvo_port == DEVICE_PORT_DVOB) ?
>> +			      "SDVOB" : "SDVOC");
>> +		p_mapping = &(dev_priv->sdvo_mappings[child->dvo_port - 1]);
>>  		if (!p_mapping->initialized) {
>> -			p_mapping->dvo_port = p_child->old.dvo_port;
>> -			p_mapping->slave_addr = p_child->old.slave_addr;
>> -			p_mapping->dvo_wiring = p_child->old.dvo_wiring;
>> -			p_mapping->ddc_pin = p_child->old.ddc_pin;
>> -			p_mapping->i2c_pin = p_child->old.i2c_pin;
>> +			p_mapping->dvo_port = child->dvo_port;
>> +			p_mapping->slave_addr = child->slave_addr;
>> +			p_mapping->dvo_wiring = child->dvo_wiring;
>> +			p_mapping->ddc_pin = child->ddc_pin;
>> +			p_mapping->i2c_pin = child->i2c_pin;
>>  			p_mapping->initialized = 1;
>>  			DRM_DEBUG_KMS("SDVO device: dvo=%x, addr=%x, wiring=%d, ddc_pin=%d, i2c_pin=%d\n",
>>  				      p_mapping->dvo_port,
>> @@ -469,7 +469,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>>  			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
>>  					 "two SDVO device.\n");
>>  		}
>> -		if (p_child->old.slave2_addr) {
>> +		if (child->slave2_addr) {
>>  			/* Maybe this is a SDVO device with multiple inputs */
>>  			/* And the mapping info is not added */
>>  			DRM_DEBUG_KMS("there exists the slave2_addr. Maybe this"
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ville Syrjälä
> Intel OTC

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