drm/i915/bios: store child devices in a list
diff mbox series

Message ID 20191106164531.7079-1-jani.nikula@intel.com
State New
Headers show
Series
  • drm/i915/bios: store child devices in a list
Related show

Commit Message

Jani Nikula Nov. 6, 2019, 4:45 p.m. UTC
Using the array is getting clumsy. Make things a bit more dynamic.

In code, start migrating towards calling the new struct child_device
"child" and the VBT-originating struct child_device_config "config".

Remove early returns on not having child devices when the end result
after "iterating" the empty list would be the same.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

The end goal: allow more meta information to be added to the new
child_device struct, independent of DDI port info being used or not on
the platform, and eventually migrate ddi_port_info to it as well,
unifying the stuff across platforms.

Currently it's not easily possible to associate for example the DSC
compression data to the child device for non-DDI platforms or for DSI
outputs. This lets us add the compression data (pointer) to struct
child_device.
---
 drivers/gpu/drm/i915/display/intel_bios.c | 203 ++++++++++------------
 drivers/gpu/drm/i915/i915_drv.h           |   3 +-
 2 files changed, 97 insertions(+), 109 deletions(-)

Comments

Ville Syrjala Nov. 6, 2019, 8:37 p.m. UTC | #1
On Wed, Nov 06, 2019 at 06:45:31PM +0200, Jani Nikula wrote:
> Using the array is getting clumsy. Make things a bit more dynamic.
> 
> In code, start migrating towards calling the new struct child_device
> "child" and the VBT-originating struct child_device_config "config".
> 
> Remove early returns on not having child devices when the end result
> after "iterating" the empty list would be the same.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> The end goal: allow more meta information to be added to the new
> child_device struct, independent of DDI port info being used or not on
> the platform, and eventually migrate ddi_port_info to it as well,
> unifying the stuff across platforms.
> 
> Currently it's not easily possible to associate for example the DSC
> compression data to the child device for non-DDI platforms or for DSI
> outputs. This lets us add the compression data (pointer) to struct
> child_device.
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 203 ++++++++++------------
>  drivers/gpu/drm/i915/i915_drv.h           |   3 +-
>  2 files changed, 97 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index a03f56b7b4ef..025074862ab0 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -58,6 +58,12 @@
>   * that.
>   */
>  
> +/* Wrapper for VBT child device config */
> +struct child_device {
> +	struct child_device_config config;
> +	struct list_head node;

The wrapper is a bit unfortunate. I don't suppose we could just shove
the list head into the existing struct and adjust what needs adjusting?

> +};
> +
>  #define	SLAVE_ADDR1	0x70
>  #define	SLAVE_ADDR2	0x72
>  
> @@ -449,8 +455,9 @@ static void
>  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
>  {
>  	struct sdvo_device_mapping *mapping;
> -	const struct child_device_config *child;
> -	int i, count = 0;
> +	const struct child_device_config *config;

This thing could at least can live inside the loop. Though the rename is
also a bit unfortunate, leading to a needlessly large diff. Avoiding the
wrapper struct would also avoid that. I guess another option would
be to select a different name for the wrapper pointer here and keep the
original name for the actual thing.

> +	const struct child_device *child;
> +	int count = 0;
>  
>  	/*
>  	 * Only parse SDVO mappings on gens that could have SDVO. This isn't
> @@ -461,35 +468,35 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
>  		return;
>  	}
>  
> -	for (i = 0, count = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		config = &child->config;
>  
> -		if (child->slave_addr != SLAVE_ADDR1 &&
> -		    child->slave_addr != SLAVE_ADDR2) {
> +		if (config->slave_addr != SLAVE_ADDR1 &&
> +		    config->slave_addr != SLAVE_ADDR2) {
>  			/*
>  			 * If the slave address is neither 0x70 nor 0x72,
>  			 * it is not a SDVO device. Skip it.
>  			 */
>  			continue;
>  		}
> -		if (child->dvo_port != DEVICE_PORT_DVOB &&
> -		    child->dvo_port != DEVICE_PORT_DVOC) {
> +		if (config->dvo_port != DEVICE_PORT_DVOB &&
> +		    config->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",
> -			      child->slave_addr,
> -			      (child->dvo_port == DEVICE_PORT_DVOB) ?
> +			      config->slave_addr,
> +			      (config->dvo_port == DEVICE_PORT_DVOB) ?
>  			      "SDVOB" : "SDVOC");
> -		mapping = &dev_priv->vbt.sdvo_mappings[child->dvo_port - 1];
> +		mapping = &dev_priv->vbt.sdvo_mappings[config->dvo_port - 1];
>  		if (!mapping->initialized) {
> -			mapping->dvo_port = child->dvo_port;
> -			mapping->slave_addr = child->slave_addr;
> -			mapping->dvo_wiring = child->dvo_wiring;
> -			mapping->ddc_pin = child->ddc_pin;
> -			mapping->i2c_pin = child->i2c_pin;
> +			mapping->dvo_port = config->dvo_port;
> +			mapping->slave_addr = config->slave_addr;
> +			mapping->dvo_wiring = config->dvo_wiring;
> +			mapping->ddc_pin = config->ddc_pin;
> +			mapping->i2c_pin = config->i2c_pin;
>  			mapping->initialized = 1;
>  			DRM_DEBUG_KMS("SDVO device: dvo=%x, addr=%x, wiring=%d, ddc_pin=%d, i2c_pin=%d\n",
>  				      mapping->dvo_port,
> @@ -501,7 +508,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
>  			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
>  					 "two SDVO device.\n");
>  		}
> -		if (child->slave2_addr) {
> +		if (config->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"
> @@ -1571,8 +1578,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv,
>  
>  static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
>  {
> -	const struct child_device_config *child;
> -	int i;
> +	const struct child_device *child;
>  
>  	if (!HAS_DDI(dev_priv) && !IS_CHERRYVIEW(dev_priv))
>  		return;
> @@ -1580,11 +1586,8 @@ static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
>  	if (bdb_version < 155)
>  		return;
>  
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> -
> -		parse_ddi_port(dev_priv, child, bdb_version);
> -	}
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node)
> +		parse_ddi_port(dev_priv, &child->config, bdb_version);
>  }
>  
>  static void
> @@ -1592,8 +1595,9 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>  			  const struct bdb_header *bdb)
>  {
>  	const struct bdb_general_definitions *defs;
> -	const struct child_device_config *child;
> -	int i, child_device_num, count;
> +	const struct child_device_config *config;
> +	struct child_device *child;
> +	int i, child_device_num;
>  	u8 expected_size;
>  	u16 block_size;
>  	int bus_pin;
> @@ -1629,8 +1633,8 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>  	} else if (bdb->version <= 229) {
>  		expected_size = 39;
>  	} else {
> -		expected_size = sizeof(*child);
> -		BUILD_BUG_ON(sizeof(*child) < 39);
> +		expected_size = sizeof(*config);
> +		BUILD_BUG_ON(sizeof(*config) < 39);
>  		DRM_DEBUG_DRIVER("Expected child device config size for VBT version %u not known; assuming %u\n",
>  				 bdb->version, expected_size);
>  	}
> @@ -1649,43 +1653,30 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>  
>  	/* get the number of child device */
>  	child_device_num = (block_size - sizeof(*defs)) / defs->child_dev_size;
> -	count = 0;
> -	/* get the number of child device that is present */
> -	for (i = 0; i < child_device_num; i++) {
> -		child = child_device_ptr(defs, i);
> -		if (!child->device_type)
> -			continue;
> -		count++;
> -	}
> -	if (!count) {
> -		DRM_DEBUG_KMS("no child dev is parsed from VBT\n");
> -		return;
> -	}
> -	dev_priv->vbt.child_dev = kcalloc(count, sizeof(*child), GFP_KERNEL);
> -	if (!dev_priv->vbt.child_dev) {
> -		DRM_DEBUG_KMS("No memory space for child device\n");
> -		return;
> -	}
>  
> -	dev_priv->vbt.child_dev_num = count;
> -	count = 0;
>  	for (i = 0; i < child_device_num; i++) {
> -		child = child_device_ptr(defs, i);
> -		if (!child->device_type)
> +		config = child_device_ptr(defs, i);
> +		if (!config->device_type)
>  			continue;
>  
>  		DRM_DEBUG_KMS("Found VBT child device with type 0x%x\n",
> -			      child->device_type);
> +			      config->device_type);
> +
> +		child = kmalloc(sizeof(*child), GFP_KERNEL);
>  
>  		/*
>  		 * Copy as much as we know (sizeof) and is available
> -		 * (child_dev_size) of the child device. Accessing the data must
> -		 * depend on VBT version.
> +		 * (child_dev_size) of the child device config. Accessing the
> +		 * data must depend on VBT version.
>  		 */
> -		memcpy(dev_priv->vbt.child_dev + count, child,
> -		       min_t(size_t, defs->child_dev_size, sizeof(*child)));
> -		count++;
> +		memcpy(&child->config, config,
> +		       min_t(size_t, defs->child_dev_size, sizeof(*config)));
> +
> +		list_add(&child->node, &dev_priv->vbt.child_devices);
>  	}
> +
> +	if (list_empty(&dev_priv->vbt.child_devices))
> +		DRM_DEBUG_KMS("no child dev is parsed from VBT\n");
>  }
>  
>  /* Common defaults which may be overridden by VBT. */
> @@ -1849,6 +1840,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  		return;
>  	}
>  
> +	INIT_LIST_HEAD(&dev_priv->vbt.child_devices);
> +
>  	init_vbt_defaults(dev_priv);
>  
>  	/* If the OpRegion does not have VBT, look in PCI ROM. */
> @@ -1903,9 +1896,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>   */
>  void intel_bios_driver_remove(struct drm_i915_private *dev_priv)
>  {
> -	kfree(dev_priv->vbt.child_dev);
> -	dev_priv->vbt.child_dev = NULL;
> -	dev_priv->vbt.child_dev_num = 0;
> +	struct child_device *child, *n;
> +
> +	list_for_each_entry_safe(child, n, &dev_priv->vbt.child_devices, node) {
> +		list_del(&child->node);
> +		kfree(child);
> +	}
> +
>  	kfree(dev_priv->vbt.sdvo_lvds_vbt_mode);
>  	dev_priv->vbt.sdvo_lvds_vbt_mode = NULL;
>  	kfree(dev_priv->vbt.lfp_lvds_vbt_mode);
> @@ -1929,21 +1926,22 @@ void intel_bios_driver_remove(struct drm_i915_private *dev_priv)
>   */
>  bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>  {
> -	const struct child_device_config *child;
> -	int i;
> +	const struct child_device_config *config;
> +	const struct child_device *child;
>  
>  	if (!dev_priv->vbt.int_tv_support)
>  		return false;
>  
> -	if (!dev_priv->vbt.child_dev_num)
> +	if (list_empty(&dev_priv->vbt.child_devices))
>  		return true;
>  
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		config = &child->config;
> +
>  		/*
>  		 * If the device type is not TV, continue.
>  		 */
> -		switch (child->device_type) {
> +		switch (config->device_type) {
>  		case DEVICE_TYPE_INT_TV:
>  		case DEVICE_TYPE_TV:
>  		case DEVICE_TYPE_TV_SVIDEO_COMPOSITE:
> @@ -1954,7 +1952,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>  		/* Only when the addin_offset is non-zero, it is regarded
>  		 * as present.
>  		 */
> -		if (child->addin_offset)
> +		if (config->addin_offset)
>  			return true;
>  	}
>  
> @@ -1971,32 +1969,32 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>   */
>  bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
>  {
> -	const struct child_device_config *child;
> -	int i;
> +	const struct child_device *child;
> +	const struct child_device_config *config;
>  
> -	if (!dev_priv->vbt.child_dev_num)
> +	if (list_empty(&dev_priv->vbt.child_devices))
>  		return true;
>  
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		config = &child->config;
>  
>  		/* If the device type is not LFP, continue.
>  		 * We have to check both the new identifiers as well as the
>  		 * old for compatibility with some BIOSes.
>  		 */
> -		if (child->device_type != DEVICE_TYPE_INT_LFP &&
> -		    child->device_type != DEVICE_TYPE_LFP)
> +		if (config->device_type != DEVICE_TYPE_INT_LFP &&
> +		    config->device_type != DEVICE_TYPE_LFP)
>  			continue;
>  
> -		if (intel_gmbus_is_valid_pin(dev_priv, child->i2c_pin))
> -			*i2c_pin = child->i2c_pin;
> +		if (intel_gmbus_is_valid_pin(dev_priv, config->i2c_pin))
> +			*i2c_pin = config->i2c_pin;
>  
>  		/* However, we cannot trust the BIOS writers to populate
>  		 * the VBT correctly.  Since LVDS requires additional
>  		 * information from AIM blocks, a non-zero addin offset is
>  		 * a good indicator that the LVDS is actually present.
>  		 */
> -		if (child->addin_offset)
> +		if (config->addin_offset)
>  			return true;
>  
>  		/* But even then some BIOS writers perform some black magic
> @@ -2020,7 +2018,8 @@ bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
>   */
>  bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port port)
>  {
> -	const struct child_device_config *child;
> +	const struct child_device_config *config;
> +	const struct child_device *child;
>  	static const struct {
>  		u16 dp, hdmi;
>  	} port_mapping[] = {
> @@ -2030,7 +2029,6 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>  		[PORT_E] = { DVO_PORT_DPE, DVO_PORT_HDMIE, },
>  		[PORT_F] = { DVO_PORT_DPF, DVO_PORT_HDMIF, },
>  	};
> -	int i;
>  
>  	if (HAS_DDI(dev_priv)) {
>  		const struct ddi_vbt_port_info *port_info =
> @@ -2045,16 +2043,13 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>  	if (WARN_ON(port == PORT_A) || port >= ARRAY_SIZE(port_mapping))
>  		return false;
>  
> -	if (!dev_priv->vbt.child_dev_num)
> -		return false;
> -
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		config = &child->config;
>  
> -		if ((child->dvo_port == port_mapping[port].dp ||
> -		     child->dvo_port == port_mapping[port].hdmi) &&
> -		    (child->device_type & (DEVICE_TYPE_TMDS_DVI_SIGNALING |
> -					   DEVICE_TYPE_DISPLAYPORT_OUTPUT)))
> +		if ((config->dvo_port == port_mapping[port].dp ||
> +		     config->dvo_port == port_mapping[port].hdmi) &&
> +		    (config->device_type & (DEVICE_TYPE_TMDS_DVI_SIGNALING |
> +					    DEVICE_TYPE_DISPLAYPORT_OUTPUT)))
>  			return true;
>  	}
>  
> @@ -2070,7 +2065,8 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>   */
>  bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
>  {
> -	const struct child_device_config *child;
> +	const struct child_device_config *config;
> +	const struct child_device *child;
>  	static const short port_mapping[] = {
>  		[PORT_B] = DVO_PORT_DPB,
>  		[PORT_C] = DVO_PORT_DPC,
> @@ -2078,19 +2074,15 @@ bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
>  		[PORT_E] = DVO_PORT_DPE,
>  		[PORT_F] = DVO_PORT_DPF,
>  	};
> -	int i;
>  
>  	if (HAS_DDI(dev_priv))
>  		return dev_priv->vbt.ddi_port_info[port].supports_edp;
>  
> -	if (!dev_priv->vbt.child_dev_num)
> -		return false;
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		config = &child->config;
>  
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> -
> -		if (child->dvo_port == port_mapping[port] &&
> -		    (child->device_type & DEVICE_TYPE_eDP_BITS) ==
> +		if (config->dvo_port == port_mapping[port] &&
> +		    (config->device_type & DEVICE_TYPE_eDP_BITS) ==
>  		    (DEVICE_TYPE_eDP & DEVICE_TYPE_eDP_BITS))
>  			return true;
>  	}
> @@ -2136,13 +2128,10 @@ static bool child_dev_is_dp_dual_mode(const struct child_device_config *child,
>  bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv,
>  				     enum port port)
>  {
> -	const struct child_device_config *child;
> -	int i;
> +	const struct child_device *child;
>  
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> -
> -		if (child_dev_is_dp_dual_mode(child, port))
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		if (child_dev_is_dp_dual_mode(&child->config, port))
>  			return true;
>  	}
>  
> @@ -2159,17 +2148,17 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv,
>  bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv,
>  			       enum port *port)
>  {
> -	const struct child_device_config *child;
> +	const struct child_device_config *config;
> +	const struct child_device *child;
>  	u8 dvo_port;
> -	int i;
>  
> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> -		child = dev_priv->vbt.child_dev + i;
> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
> +		config = &child->config;
>  
> -		if (!(child->device_type & DEVICE_TYPE_MIPI_OUTPUT))
> +		if (!(config->device_type & DEVICE_TYPE_MIPI_OUTPUT))
>  			continue;
>  
> -		dvo_port = child->dvo_port;
> +		dvo_port = config->dvo_port;
>  
>  		if (dvo_port == DVO_PORT_MIPIA ||
>  		    (dvo_port == DVO_PORT_MIPIB && INTEL_GEN(dev_priv) >= 11) ||
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7e0f67babe20..ae85ffa08665 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -724,8 +724,7 @@ struct intel_vbt_data {
>  
>  	int crt_ddc_pin;
>  
> -	int child_dev_num;
> -	struct child_device_config *child_dev;
> +	struct list_head child_devices;
>  
>  	struct ddi_vbt_port_info ddi_port_info[I915_MAX_PORTS];
>  	struct sdvo_device_mapping sdvo_mappings[2];
> -- 
> 2.20.1
Jani Nikula Nov. 7, 2019, 8:22 a.m. UTC | #2
On Wed, 06 Nov 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Nov 06, 2019 at 06:45:31PM +0200, Jani Nikula wrote:
>> Using the array is getting clumsy. Make things a bit more dynamic.
>> 
>> In code, start migrating towards calling the new struct child_device
>> "child" and the VBT-originating struct child_device_config "config".
>> 
>> Remove early returns on not having child devices when the end result
>> after "iterating" the empty list would be the same.
>> 
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> The end goal: allow more meta information to be added to the new
>> child_device struct, independent of DDI port info being used or not on
>> the platform, and eventually migrate ddi_port_info to it as well,
>> unifying the stuff across platforms.
>> 
>> Currently it's not easily possible to associate for example the DSC
>> compression data to the child device for non-DDI platforms or for DSI
>> outputs. This lets us add the compression data (pointer) to struct
>> child_device.
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 203 ++++++++++------------
>>  drivers/gpu/drm/i915/i915_drv.h           |   3 +-
>>  2 files changed, 97 insertions(+), 109 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index a03f56b7b4ef..025074862ab0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -58,6 +58,12 @@
>>   * that.
>>   */
>>  
>> +/* Wrapper for VBT child device config */
>> +struct child_device {
>> +	struct child_device_config config;
>> +	struct list_head node;
>
> The wrapper is a bit unfortunate. I don't suppose we could just shove
> the list head into the existing struct and adjust what needs adjusting?

The existing struct is used for serialization and the size is checked
against what's in vbt etc. I might also add stuff in the wrapper struct,
at least intermediately, so it's kind of useful. I don't really think
the wrapper is all that bad.

>
>> +};
>> +
>>  #define	SLAVE_ADDR1	0x70
>>  #define	SLAVE_ADDR2	0x72
>>  
>> @@ -449,8 +455,9 @@ static void
>>  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
>>  {
>>  	struct sdvo_device_mapping *mapping;
>> -	const struct child_device_config *child;
>> -	int i, count = 0;
>> +	const struct child_device_config *config;
>
> This thing could at least can live inside the loop. Though the rename is
> also a bit unfortunate, leading to a needlessly large diff. Avoiding the
> wrapper struct would also avoid that. I guess another option would
> be to select a different name for the wrapper pointer here and keep the
> original name for the actual thing.

The main problem with avoiding the rename is to come up with a better
name for the wrapper structure. :) Child and config seemed apt, but I do
understand the downsides. I'd just like to have names that we can use
througout. Maybe we can stick to child for struct child_device_config,
but what do we call the whole wrapper struct and local vars?
Suggestions?


BR,
Jani.


>
>> +	const struct child_device *child;
>> +	int count = 0;
>>  
>>  	/*
>>  	 * Only parse SDVO mappings on gens that could have SDVO. This isn't
>> @@ -461,35 +468,35 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
>>  		return;
>>  	}
>>  
>> -	for (i = 0, count = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		config = &child->config;
>>  
>> -		if (child->slave_addr != SLAVE_ADDR1 &&
>> -		    child->slave_addr != SLAVE_ADDR2) {
>> +		if (config->slave_addr != SLAVE_ADDR1 &&
>> +		    config->slave_addr != SLAVE_ADDR2) {
>>  			/*
>>  			 * If the slave address is neither 0x70 nor 0x72,
>>  			 * it is not a SDVO device. Skip it.
>>  			 */
>>  			continue;
>>  		}
>> -		if (child->dvo_port != DEVICE_PORT_DVOB &&
>> -		    child->dvo_port != DEVICE_PORT_DVOC) {
>> +		if (config->dvo_port != DEVICE_PORT_DVOB &&
>> +		    config->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",
>> -			      child->slave_addr,
>> -			      (child->dvo_port == DEVICE_PORT_DVOB) ?
>> +			      config->slave_addr,
>> +			      (config->dvo_port == DEVICE_PORT_DVOB) ?
>>  			      "SDVOB" : "SDVOC");
>> -		mapping = &dev_priv->vbt.sdvo_mappings[child->dvo_port - 1];
>> +		mapping = &dev_priv->vbt.sdvo_mappings[config->dvo_port - 1];
>>  		if (!mapping->initialized) {
>> -			mapping->dvo_port = child->dvo_port;
>> -			mapping->slave_addr = child->slave_addr;
>> -			mapping->dvo_wiring = child->dvo_wiring;
>> -			mapping->ddc_pin = child->ddc_pin;
>> -			mapping->i2c_pin = child->i2c_pin;
>> +			mapping->dvo_port = config->dvo_port;
>> +			mapping->slave_addr = config->slave_addr;
>> +			mapping->dvo_wiring = config->dvo_wiring;
>> +			mapping->ddc_pin = config->ddc_pin;
>> +			mapping->i2c_pin = config->i2c_pin;
>>  			mapping->initialized = 1;
>>  			DRM_DEBUG_KMS("SDVO device: dvo=%x, addr=%x, wiring=%d, ddc_pin=%d, i2c_pin=%d\n",
>>  				      mapping->dvo_port,
>> @@ -501,7 +508,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
>>  			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
>>  					 "two SDVO device.\n");
>>  		}
>> -		if (child->slave2_addr) {
>> +		if (config->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"
>> @@ -1571,8 +1578,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv,
>>  
>>  static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
>>  {
>> -	const struct child_device_config *child;
>> -	int i;
>> +	const struct child_device *child;
>>  
>>  	if (!HAS_DDI(dev_priv) && !IS_CHERRYVIEW(dev_priv))
>>  		return;
>> @@ -1580,11 +1586,8 @@ static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
>>  	if (bdb_version < 155)
>>  		return;
>>  
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> -
>> -		parse_ddi_port(dev_priv, child, bdb_version);
>> -	}
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node)
>> +		parse_ddi_port(dev_priv, &child->config, bdb_version);
>>  }
>>  
>>  static void
>> @@ -1592,8 +1595,9 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>>  			  const struct bdb_header *bdb)
>>  {
>>  	const struct bdb_general_definitions *defs;
>> -	const struct child_device_config *child;
>> -	int i, child_device_num, count;
>> +	const struct child_device_config *config;
>> +	struct child_device *child;
>> +	int i, child_device_num;
>>  	u8 expected_size;
>>  	u16 block_size;
>>  	int bus_pin;
>> @@ -1629,8 +1633,8 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>>  	} else if (bdb->version <= 229) {
>>  		expected_size = 39;
>>  	} else {
>> -		expected_size = sizeof(*child);
>> -		BUILD_BUG_ON(sizeof(*child) < 39);
>> +		expected_size = sizeof(*config);
>> +		BUILD_BUG_ON(sizeof(*config) < 39);
>>  		DRM_DEBUG_DRIVER("Expected child device config size for VBT version %u not known; assuming %u\n",
>>  				 bdb->version, expected_size);
>>  	}
>> @@ -1649,43 +1653,30 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>>  
>>  	/* get the number of child device */
>>  	child_device_num = (block_size - sizeof(*defs)) / defs->child_dev_size;
>> -	count = 0;
>> -	/* get the number of child device that is present */
>> -	for (i = 0; i < child_device_num; i++) {
>> -		child = child_device_ptr(defs, i);
>> -		if (!child->device_type)
>> -			continue;
>> -		count++;
>> -	}
>> -	if (!count) {
>> -		DRM_DEBUG_KMS("no child dev is parsed from VBT\n");
>> -		return;
>> -	}
>> -	dev_priv->vbt.child_dev = kcalloc(count, sizeof(*child), GFP_KERNEL);
>> -	if (!dev_priv->vbt.child_dev) {
>> -		DRM_DEBUG_KMS("No memory space for child device\n");
>> -		return;
>> -	}
>>  
>> -	dev_priv->vbt.child_dev_num = count;
>> -	count = 0;
>>  	for (i = 0; i < child_device_num; i++) {
>> -		child = child_device_ptr(defs, i);
>> -		if (!child->device_type)
>> +		config = child_device_ptr(defs, i);
>> +		if (!config->device_type)
>>  			continue;
>>  
>>  		DRM_DEBUG_KMS("Found VBT child device with type 0x%x\n",
>> -			      child->device_type);
>> +			      config->device_type);
>> +
>> +		child = kmalloc(sizeof(*child), GFP_KERNEL);
>>  
>>  		/*
>>  		 * Copy as much as we know (sizeof) and is available
>> -		 * (child_dev_size) of the child device. Accessing the data must
>> -		 * depend on VBT version.
>> +		 * (child_dev_size) of the child device config. Accessing the
>> +		 * data must depend on VBT version.
>>  		 */
>> -		memcpy(dev_priv->vbt.child_dev + count, child,
>> -		       min_t(size_t, defs->child_dev_size, sizeof(*child)));
>> -		count++;
>> +		memcpy(&child->config, config,
>> +		       min_t(size_t, defs->child_dev_size, sizeof(*config)));
>> +
>> +		list_add(&child->node, &dev_priv->vbt.child_devices);
>>  	}
>> +
>> +	if (list_empty(&dev_priv->vbt.child_devices))
>> +		DRM_DEBUG_KMS("no child dev is parsed from VBT\n");
>>  }
>>  
>>  /* Common defaults which may be overridden by VBT. */
>> @@ -1849,6 +1840,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>  		return;
>>  	}
>>  
>> +	INIT_LIST_HEAD(&dev_priv->vbt.child_devices);
>> +
>>  	init_vbt_defaults(dev_priv);
>>  
>>  	/* If the OpRegion does not have VBT, look in PCI ROM. */
>> @@ -1903,9 +1896,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>   */
>>  void intel_bios_driver_remove(struct drm_i915_private *dev_priv)
>>  {
>> -	kfree(dev_priv->vbt.child_dev);
>> -	dev_priv->vbt.child_dev = NULL;
>> -	dev_priv->vbt.child_dev_num = 0;
>> +	struct child_device *child, *n;
>> +
>> +	list_for_each_entry_safe(child, n, &dev_priv->vbt.child_devices, node) {
>> +		list_del(&child->node);
>> +		kfree(child);
>> +	}
>> +
>>  	kfree(dev_priv->vbt.sdvo_lvds_vbt_mode);
>>  	dev_priv->vbt.sdvo_lvds_vbt_mode = NULL;
>>  	kfree(dev_priv->vbt.lfp_lvds_vbt_mode);
>> @@ -1929,21 +1926,22 @@ void intel_bios_driver_remove(struct drm_i915_private *dev_priv)
>>   */
>>  bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>>  {
>> -	const struct child_device_config *child;
>> -	int i;
>> +	const struct child_device_config *config;
>> +	const struct child_device *child;
>>  
>>  	if (!dev_priv->vbt.int_tv_support)
>>  		return false;
>>  
>> -	if (!dev_priv->vbt.child_dev_num)
>> +	if (list_empty(&dev_priv->vbt.child_devices))
>>  		return true;
>>  
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		config = &child->config;
>> +
>>  		/*
>>  		 * If the device type is not TV, continue.
>>  		 */
>> -		switch (child->device_type) {
>> +		switch (config->device_type) {
>>  		case DEVICE_TYPE_INT_TV:
>>  		case DEVICE_TYPE_TV:
>>  		case DEVICE_TYPE_TV_SVIDEO_COMPOSITE:
>> @@ -1954,7 +1952,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>>  		/* Only when the addin_offset is non-zero, it is regarded
>>  		 * as present.
>>  		 */
>> -		if (child->addin_offset)
>> +		if (config->addin_offset)
>>  			return true;
>>  	}
>>  
>> @@ -1971,32 +1969,32 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>>   */
>>  bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
>>  {
>> -	const struct child_device_config *child;
>> -	int i;
>> +	const struct child_device *child;
>> +	const struct child_device_config *config;
>>  
>> -	if (!dev_priv->vbt.child_dev_num)
>> +	if (list_empty(&dev_priv->vbt.child_devices))
>>  		return true;
>>  
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		config = &child->config;
>>  
>>  		/* If the device type is not LFP, continue.
>>  		 * We have to check both the new identifiers as well as the
>>  		 * old for compatibility with some BIOSes.
>>  		 */
>> -		if (child->device_type != DEVICE_TYPE_INT_LFP &&
>> -		    child->device_type != DEVICE_TYPE_LFP)
>> +		if (config->device_type != DEVICE_TYPE_INT_LFP &&
>> +		    config->device_type != DEVICE_TYPE_LFP)
>>  			continue;
>>  
>> -		if (intel_gmbus_is_valid_pin(dev_priv, child->i2c_pin))
>> -			*i2c_pin = child->i2c_pin;
>> +		if (intel_gmbus_is_valid_pin(dev_priv, config->i2c_pin))
>> +			*i2c_pin = config->i2c_pin;
>>  
>>  		/* However, we cannot trust the BIOS writers to populate
>>  		 * the VBT correctly.  Since LVDS requires additional
>>  		 * information from AIM blocks, a non-zero addin offset is
>>  		 * a good indicator that the LVDS is actually present.
>>  		 */
>> -		if (child->addin_offset)
>> +		if (config->addin_offset)
>>  			return true;
>>  
>>  		/* But even then some BIOS writers perform some black magic
>> @@ -2020,7 +2018,8 @@ bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
>>   */
>>  bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port port)
>>  {
>> -	const struct child_device_config *child;
>> +	const struct child_device_config *config;
>> +	const struct child_device *child;
>>  	static const struct {
>>  		u16 dp, hdmi;
>>  	} port_mapping[] = {
>> @@ -2030,7 +2029,6 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>>  		[PORT_E] = { DVO_PORT_DPE, DVO_PORT_HDMIE, },
>>  		[PORT_F] = { DVO_PORT_DPF, DVO_PORT_HDMIF, },
>>  	};
>> -	int i;
>>  
>>  	if (HAS_DDI(dev_priv)) {
>>  		const struct ddi_vbt_port_info *port_info =
>> @@ -2045,16 +2043,13 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>>  	if (WARN_ON(port == PORT_A) || port >= ARRAY_SIZE(port_mapping))
>>  		return false;
>>  
>> -	if (!dev_priv->vbt.child_dev_num)
>> -		return false;
>> -
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		config = &child->config;
>>  
>> -		if ((child->dvo_port == port_mapping[port].dp ||
>> -		     child->dvo_port == port_mapping[port].hdmi) &&
>> -		    (child->device_type & (DEVICE_TYPE_TMDS_DVI_SIGNALING |
>> -					   DEVICE_TYPE_DISPLAYPORT_OUTPUT)))
>> +		if ((config->dvo_port == port_mapping[port].dp ||
>> +		     config->dvo_port == port_mapping[port].hdmi) &&
>> +		    (config->device_type & (DEVICE_TYPE_TMDS_DVI_SIGNALING |
>> +					    DEVICE_TYPE_DISPLAYPORT_OUTPUT)))
>>  			return true;
>>  	}
>>  
>> @@ -2070,7 +2065,8 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>>   */
>>  bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
>>  {
>> -	const struct child_device_config *child;
>> +	const struct child_device_config *config;
>> +	const struct child_device *child;
>>  	static const short port_mapping[] = {
>>  		[PORT_B] = DVO_PORT_DPB,
>>  		[PORT_C] = DVO_PORT_DPC,
>> @@ -2078,19 +2074,15 @@ bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
>>  		[PORT_E] = DVO_PORT_DPE,
>>  		[PORT_F] = DVO_PORT_DPF,
>>  	};
>> -	int i;
>>  
>>  	if (HAS_DDI(dev_priv))
>>  		return dev_priv->vbt.ddi_port_info[port].supports_edp;
>>  
>> -	if (!dev_priv->vbt.child_dev_num)
>> -		return false;
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		config = &child->config;
>>  
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> -
>> -		if (child->dvo_port == port_mapping[port] &&
>> -		    (child->device_type & DEVICE_TYPE_eDP_BITS) ==
>> +		if (config->dvo_port == port_mapping[port] &&
>> +		    (config->device_type & DEVICE_TYPE_eDP_BITS) ==
>>  		    (DEVICE_TYPE_eDP & DEVICE_TYPE_eDP_BITS))
>>  			return true;
>>  	}
>> @@ -2136,13 +2128,10 @@ static bool child_dev_is_dp_dual_mode(const struct child_device_config *child,
>>  bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv,
>>  				     enum port port)
>>  {
>> -	const struct child_device_config *child;
>> -	int i;
>> +	const struct child_device *child;
>>  
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> -
>> -		if (child_dev_is_dp_dual_mode(child, port))
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		if (child_dev_is_dp_dual_mode(&child->config, port))
>>  			return true;
>>  	}
>>  
>> @@ -2159,17 +2148,17 @@ bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv,
>>  bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv,
>>  			       enum port *port)
>>  {
>> -	const struct child_device_config *child;
>> +	const struct child_device_config *config;
>> +	const struct child_device *child;
>>  	u8 dvo_port;
>> -	int i;
>>  
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		child = dev_priv->vbt.child_dev + i;
>> +	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
>> +		config = &child->config;
>>  
>> -		if (!(child->device_type & DEVICE_TYPE_MIPI_OUTPUT))
>> +		if (!(config->device_type & DEVICE_TYPE_MIPI_OUTPUT))
>>  			continue;
>>  
>> -		dvo_port = child->dvo_port;
>> +		dvo_port = config->dvo_port;
>>  
>>  		if (dvo_port == DVO_PORT_MIPIA ||
>>  		    (dvo_port == DVO_PORT_MIPIB && INTEL_GEN(dev_priv) >= 11) ||
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 7e0f67babe20..ae85ffa08665 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -724,8 +724,7 @@ struct intel_vbt_data {
>>  
>>  	int crt_ddc_pin;
>>  
>> -	int child_dev_num;
>> -	struct child_device_config *child_dev;
>> +	struct list_head child_devices;
>>  
>>  	struct ddi_vbt_port_info ddi_port_info[I915_MAX_PORTS];
>>  	struct sdvo_device_mapping sdvo_mappings[2];
>> -- 
>> 2.20.1
Ville Syrjala Nov. 7, 2019, 10:40 a.m. UTC | #3
On Thu, Nov 07, 2019 at 10:22:10AM +0200, Jani Nikula wrote:
> On Wed, 06 Nov 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Nov 06, 2019 at 06:45:31PM +0200, Jani Nikula wrote:
> >> Using the array is getting clumsy. Make things a bit more dynamic.
> >> 
> >> In code, start migrating towards calling the new struct child_device
> >> "child" and the VBT-originating struct child_device_config "config".
> >> 
> >> Remove early returns on not having child devices when the end result
> >> after "iterating" the empty list would be the same.
> >> 
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> 
> >> ---
> >> 
> >> The end goal: allow more meta information to be added to the new
> >> child_device struct, independent of DDI port info being used or not on
> >> the platform, and eventually migrate ddi_port_info to it as well,
> >> unifying the stuff across platforms.
> >> 
> >> Currently it's not easily possible to associate for example the DSC
> >> compression data to the child device for non-DDI platforms or for DSI
> >> outputs. This lets us add the compression data (pointer) to struct
> >> child_device.
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_bios.c | 203 ++++++++++------------
> >>  drivers/gpu/drm/i915/i915_drv.h           |   3 +-
> >>  2 files changed, 97 insertions(+), 109 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> index a03f56b7b4ef..025074862ab0 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> @@ -58,6 +58,12 @@
> >>   * that.
> >>   */
> >>  
> >> +/* Wrapper for VBT child device config */
> >> +struct child_device {
> >> +	struct child_device_config config;
> >> +	struct list_head node;
> >
> > The wrapper is a bit unfortunate. I don't suppose we could just shove
> > the list head into the existing struct and adjust what needs adjusting?
> 
> The existing struct is used for serialization and the size is checked
> against what's in vbt etc. I might also add stuff in the wrapper struct,
> at least intermediately, so it's kind of useful. I don't really think
> the wrapper is all that bad.
> 
> >
> >> +};
> >> +
> >>  #define	SLAVE_ADDR1	0x70
> >>  #define	SLAVE_ADDR2	0x72
> >>  
> >> @@ -449,8 +455,9 @@ static void
> >>  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
> >>  {
> >>  	struct sdvo_device_mapping *mapping;
> >> -	const struct child_device_config *child;
> >> -	int i, count = 0;
> >> +	const struct child_device_config *config;
> >
> > This thing could at least can live inside the loop. Though the rename is
> > also a bit unfortunate, leading to a needlessly large diff. Avoiding the
> > wrapper struct would also avoid that. I guess another option would
> > be to select a different name for the wrapper pointer here and keep the
> > original name for the actual thing.
> 
> The main problem with avoiding the rename is to come up with a better
> name for the wrapper structure. :) Child and config seemed apt, but I do
> understand the downsides. I'd just like to have names that we can use
> througout. Maybe we can stick to child for struct child_device_config,
> but what do we call the whole wrapper struct and local vars?
> Suggestions?

I suck at naming things. If no better name comes up I guess we could at
least split the rename to a separate patch. Would be easier to see
what's actually changing with the introduction of the list.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index a03f56b7b4ef..025074862ab0 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -58,6 +58,12 @@ 
  * that.
  */
 
+/* Wrapper for VBT child device config */
+struct child_device {
+	struct child_device_config config;
+	struct list_head node;
+};
+
 #define	SLAVE_ADDR1	0x70
 #define	SLAVE_ADDR2	0x72
 
@@ -449,8 +455,9 @@  static void
 parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
 {
 	struct sdvo_device_mapping *mapping;
-	const struct child_device_config *child;
-	int i, count = 0;
+	const struct child_device_config *config;
+	const struct child_device *child;
+	int count = 0;
 
 	/*
 	 * Only parse SDVO mappings on gens that could have SDVO. This isn't
@@ -461,35 +468,35 @@  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
 		return;
 	}
 
-	for (i = 0, count = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		config = &child->config;
 
-		if (child->slave_addr != SLAVE_ADDR1 &&
-		    child->slave_addr != SLAVE_ADDR2) {
+		if (config->slave_addr != SLAVE_ADDR1 &&
+		    config->slave_addr != SLAVE_ADDR2) {
 			/*
 			 * If the slave address is neither 0x70 nor 0x72,
 			 * it is not a SDVO device. Skip it.
 			 */
 			continue;
 		}
-		if (child->dvo_port != DEVICE_PORT_DVOB &&
-		    child->dvo_port != DEVICE_PORT_DVOC) {
+		if (config->dvo_port != DEVICE_PORT_DVOB &&
+		    config->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",
-			      child->slave_addr,
-			      (child->dvo_port == DEVICE_PORT_DVOB) ?
+			      config->slave_addr,
+			      (config->dvo_port == DEVICE_PORT_DVOB) ?
 			      "SDVOB" : "SDVOC");
-		mapping = &dev_priv->vbt.sdvo_mappings[child->dvo_port - 1];
+		mapping = &dev_priv->vbt.sdvo_mappings[config->dvo_port - 1];
 		if (!mapping->initialized) {
-			mapping->dvo_port = child->dvo_port;
-			mapping->slave_addr = child->slave_addr;
-			mapping->dvo_wiring = child->dvo_wiring;
-			mapping->ddc_pin = child->ddc_pin;
-			mapping->i2c_pin = child->i2c_pin;
+			mapping->dvo_port = config->dvo_port;
+			mapping->slave_addr = config->slave_addr;
+			mapping->dvo_wiring = config->dvo_wiring;
+			mapping->ddc_pin = config->ddc_pin;
+			mapping->i2c_pin = config->i2c_pin;
 			mapping->initialized = 1;
 			DRM_DEBUG_KMS("SDVO device: dvo=%x, addr=%x, wiring=%d, ddc_pin=%d, i2c_pin=%d\n",
 				      mapping->dvo_port,
@@ -501,7 +508,7 @@  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
 			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
 					 "two SDVO device.\n");
 		}
-		if (child->slave2_addr) {
+		if (config->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"
@@ -1571,8 +1578,7 @@  static void parse_ddi_port(struct drm_i915_private *dev_priv,
 
 static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
 {
-	const struct child_device_config *child;
-	int i;
+	const struct child_device *child;
 
 	if (!HAS_DDI(dev_priv) && !IS_CHERRYVIEW(dev_priv))
 		return;
@@ -1580,11 +1586,8 @@  static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 bdb_version)
 	if (bdb_version < 155)
 		return;
 
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
-
-		parse_ddi_port(dev_priv, child, bdb_version);
-	}
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node)
+		parse_ddi_port(dev_priv, &child->config, bdb_version);
 }
 
 static void
@@ -1592,8 +1595,9 @@  parse_general_definitions(struct drm_i915_private *dev_priv,
 			  const struct bdb_header *bdb)
 {
 	const struct bdb_general_definitions *defs;
-	const struct child_device_config *child;
-	int i, child_device_num, count;
+	const struct child_device_config *config;
+	struct child_device *child;
+	int i, child_device_num;
 	u8 expected_size;
 	u16 block_size;
 	int bus_pin;
@@ -1629,8 +1633,8 @@  parse_general_definitions(struct drm_i915_private *dev_priv,
 	} else if (bdb->version <= 229) {
 		expected_size = 39;
 	} else {
-		expected_size = sizeof(*child);
-		BUILD_BUG_ON(sizeof(*child) < 39);
+		expected_size = sizeof(*config);
+		BUILD_BUG_ON(sizeof(*config) < 39);
 		DRM_DEBUG_DRIVER("Expected child device config size for VBT version %u not known; assuming %u\n",
 				 bdb->version, expected_size);
 	}
@@ -1649,43 +1653,30 @@  parse_general_definitions(struct drm_i915_private *dev_priv,
 
 	/* get the number of child device */
 	child_device_num = (block_size - sizeof(*defs)) / defs->child_dev_size;
-	count = 0;
-	/* get the number of child device that is present */
-	for (i = 0; i < child_device_num; i++) {
-		child = child_device_ptr(defs, i);
-		if (!child->device_type)
-			continue;
-		count++;
-	}
-	if (!count) {
-		DRM_DEBUG_KMS("no child dev is parsed from VBT\n");
-		return;
-	}
-	dev_priv->vbt.child_dev = kcalloc(count, sizeof(*child), GFP_KERNEL);
-	if (!dev_priv->vbt.child_dev) {
-		DRM_DEBUG_KMS("No memory space for child device\n");
-		return;
-	}
 
-	dev_priv->vbt.child_dev_num = count;
-	count = 0;
 	for (i = 0; i < child_device_num; i++) {
-		child = child_device_ptr(defs, i);
-		if (!child->device_type)
+		config = child_device_ptr(defs, i);
+		if (!config->device_type)
 			continue;
 
 		DRM_DEBUG_KMS("Found VBT child device with type 0x%x\n",
-			      child->device_type);
+			      config->device_type);
+
+		child = kmalloc(sizeof(*child), GFP_KERNEL);
 
 		/*
 		 * Copy as much as we know (sizeof) and is available
-		 * (child_dev_size) of the child device. Accessing the data must
-		 * depend on VBT version.
+		 * (child_dev_size) of the child device config. Accessing the
+		 * data must depend on VBT version.
 		 */
-		memcpy(dev_priv->vbt.child_dev + count, child,
-		       min_t(size_t, defs->child_dev_size, sizeof(*child)));
-		count++;
+		memcpy(&child->config, config,
+		       min_t(size_t, defs->child_dev_size, sizeof(*config)));
+
+		list_add(&child->node, &dev_priv->vbt.child_devices);
 	}
+
+	if (list_empty(&dev_priv->vbt.child_devices))
+		DRM_DEBUG_KMS("no child dev is parsed from VBT\n");
 }
 
 /* Common defaults which may be overridden by VBT. */
@@ -1849,6 +1840,8 @@  void intel_bios_init(struct drm_i915_private *dev_priv)
 		return;
 	}
 
+	INIT_LIST_HEAD(&dev_priv->vbt.child_devices);
+
 	init_vbt_defaults(dev_priv);
 
 	/* If the OpRegion does not have VBT, look in PCI ROM. */
@@ -1903,9 +1896,13 @@  void intel_bios_init(struct drm_i915_private *dev_priv)
  */
 void intel_bios_driver_remove(struct drm_i915_private *dev_priv)
 {
-	kfree(dev_priv->vbt.child_dev);
-	dev_priv->vbt.child_dev = NULL;
-	dev_priv->vbt.child_dev_num = 0;
+	struct child_device *child, *n;
+
+	list_for_each_entry_safe(child, n, &dev_priv->vbt.child_devices, node) {
+		list_del(&child->node);
+		kfree(child);
+	}
+
 	kfree(dev_priv->vbt.sdvo_lvds_vbt_mode);
 	dev_priv->vbt.sdvo_lvds_vbt_mode = NULL;
 	kfree(dev_priv->vbt.lfp_lvds_vbt_mode);
@@ -1929,21 +1926,22 @@  void intel_bios_driver_remove(struct drm_i915_private *dev_priv)
  */
 bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
 {
-	const struct child_device_config *child;
-	int i;
+	const struct child_device_config *config;
+	const struct child_device *child;
 
 	if (!dev_priv->vbt.int_tv_support)
 		return false;
 
-	if (!dev_priv->vbt.child_dev_num)
+	if (list_empty(&dev_priv->vbt.child_devices))
 		return true;
 
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		config = &child->config;
+
 		/*
 		 * If the device type is not TV, continue.
 		 */
-		switch (child->device_type) {
+		switch (config->device_type) {
 		case DEVICE_TYPE_INT_TV:
 		case DEVICE_TYPE_TV:
 		case DEVICE_TYPE_TV_SVIDEO_COMPOSITE:
@@ -1954,7 +1952,7 @@  bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
 		/* Only when the addin_offset is non-zero, it is regarded
 		 * as present.
 		 */
-		if (child->addin_offset)
+		if (config->addin_offset)
 			return true;
 	}
 
@@ -1971,32 +1969,32 @@  bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
  */
 bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
 {
-	const struct child_device_config *child;
-	int i;
+	const struct child_device *child;
+	const struct child_device_config *config;
 
-	if (!dev_priv->vbt.child_dev_num)
+	if (list_empty(&dev_priv->vbt.child_devices))
 		return true;
 
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		config = &child->config;
 
 		/* If the device type is not LFP, continue.
 		 * We have to check both the new identifiers as well as the
 		 * old for compatibility with some BIOSes.
 		 */
-		if (child->device_type != DEVICE_TYPE_INT_LFP &&
-		    child->device_type != DEVICE_TYPE_LFP)
+		if (config->device_type != DEVICE_TYPE_INT_LFP &&
+		    config->device_type != DEVICE_TYPE_LFP)
 			continue;
 
-		if (intel_gmbus_is_valid_pin(dev_priv, child->i2c_pin))
-			*i2c_pin = child->i2c_pin;
+		if (intel_gmbus_is_valid_pin(dev_priv, config->i2c_pin))
+			*i2c_pin = config->i2c_pin;
 
 		/* However, we cannot trust the BIOS writers to populate
 		 * the VBT correctly.  Since LVDS requires additional
 		 * information from AIM blocks, a non-zero addin offset is
 		 * a good indicator that the LVDS is actually present.
 		 */
-		if (child->addin_offset)
+		if (config->addin_offset)
 			return true;
 
 		/* But even then some BIOS writers perform some black magic
@@ -2020,7 +2018,8 @@  bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
  */
 bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port port)
 {
-	const struct child_device_config *child;
+	const struct child_device_config *config;
+	const struct child_device *child;
 	static const struct {
 		u16 dp, hdmi;
 	} port_mapping[] = {
@@ -2030,7 +2029,6 @@  bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
 		[PORT_E] = { DVO_PORT_DPE, DVO_PORT_HDMIE, },
 		[PORT_F] = { DVO_PORT_DPF, DVO_PORT_HDMIF, },
 	};
-	int i;
 
 	if (HAS_DDI(dev_priv)) {
 		const struct ddi_vbt_port_info *port_info =
@@ -2045,16 +2043,13 @@  bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
 	if (WARN_ON(port == PORT_A) || port >= ARRAY_SIZE(port_mapping))
 		return false;
 
-	if (!dev_priv->vbt.child_dev_num)
-		return false;
-
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		config = &child->config;
 
-		if ((child->dvo_port == port_mapping[port].dp ||
-		     child->dvo_port == port_mapping[port].hdmi) &&
-		    (child->device_type & (DEVICE_TYPE_TMDS_DVI_SIGNALING |
-					   DEVICE_TYPE_DISPLAYPORT_OUTPUT)))
+		if ((config->dvo_port == port_mapping[port].dp ||
+		     config->dvo_port == port_mapping[port].hdmi) &&
+		    (config->device_type & (DEVICE_TYPE_TMDS_DVI_SIGNALING |
+					    DEVICE_TYPE_DISPLAYPORT_OUTPUT)))
 			return true;
 	}
 
@@ -2070,7 +2065,8 @@  bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
  */
 bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
 {
-	const struct child_device_config *child;
+	const struct child_device_config *config;
+	const struct child_device *child;
 	static const short port_mapping[] = {
 		[PORT_B] = DVO_PORT_DPB,
 		[PORT_C] = DVO_PORT_DPC,
@@ -2078,19 +2074,15 @@  bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port)
 		[PORT_E] = DVO_PORT_DPE,
 		[PORT_F] = DVO_PORT_DPF,
 	};
-	int i;
 
 	if (HAS_DDI(dev_priv))
 		return dev_priv->vbt.ddi_port_info[port].supports_edp;
 
-	if (!dev_priv->vbt.child_dev_num)
-		return false;
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		config = &child->config;
 
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
-
-		if (child->dvo_port == port_mapping[port] &&
-		    (child->device_type & DEVICE_TYPE_eDP_BITS) ==
+		if (config->dvo_port == port_mapping[port] &&
+		    (config->device_type & DEVICE_TYPE_eDP_BITS) ==
 		    (DEVICE_TYPE_eDP & DEVICE_TYPE_eDP_BITS))
 			return true;
 	}
@@ -2136,13 +2128,10 @@  static bool child_dev_is_dp_dual_mode(const struct child_device_config *child,
 bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv,
 				     enum port port)
 {
-	const struct child_device_config *child;
-	int i;
+	const struct child_device *child;
 
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
-
-		if (child_dev_is_dp_dual_mode(child, port))
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		if (child_dev_is_dp_dual_mode(&child->config, port))
 			return true;
 	}
 
@@ -2159,17 +2148,17 @@  bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv,
 bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv,
 			       enum port *port)
 {
-	const struct child_device_config *child;
+	const struct child_device_config *config;
+	const struct child_device *child;
 	u8 dvo_port;
-	int i;
 
-	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
-		child = dev_priv->vbt.child_dev + i;
+	list_for_each_entry(child, &dev_priv->vbt.child_devices, node) {
+		config = &child->config;
 
-		if (!(child->device_type & DEVICE_TYPE_MIPI_OUTPUT))
+		if (!(config->device_type & DEVICE_TYPE_MIPI_OUTPUT))
 			continue;
 
-		dvo_port = child->dvo_port;
+		dvo_port = config->dvo_port;
 
 		if (dvo_port == DVO_PORT_MIPIA ||
 		    (dvo_port == DVO_PORT_MIPIB && INTEL_GEN(dev_priv) >= 11) ||
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7e0f67babe20..ae85ffa08665 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -724,8 +724,7 @@  struct intel_vbt_data {
 
 	int crt_ddc_pin;
 
-	int child_dev_num;
-	struct child_device_config *child_dev;
+	struct list_head child_devices;
 
 	struct ddi_vbt_port_info ddi_port_info[I915_MAX_PORTS];
 	struct sdvo_device_mapping sdvo_mappings[2];