On Wed, 06 Nov 2019, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>
>> ---
>>
>> 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
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
