On Thu, 07 Nov 2019, Ville Syrjälä <[email protected]> wrote: > On Thu, Nov 07, 2019 at 06:08:53PM +0200, Jani Nikula wrote: >> Using the array is getting clumsy. Make things a bit more dynamic. >> >> Remove early returns on not having child devices when the end result >> after "iterating" the empty list would be the same. >> >> v2: >> - stick to previous naming of child devices (Ville) >> - use kzalloc, handle failure >> - initialize list head earlier to keep intel_bios_driver_remove() safe >> >> Cc: Ville Syrjala <[email protected]> >> Signed-off-by: Jani Nikula <[email protected]> >> >> --- >> >> 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 | 123 ++++++++++------------ >> drivers/gpu/drm/i915/i915_drv.h | 3 +- >> 2 files changed, 58 insertions(+), 68 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c >> b/drivers/gpu/drm/i915/display/intel_bios.c >> index c19b234bebe6..37b944397b75 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 display_device_data { >> + struct child_device_config child; >> + 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 display_device_data *devdata; >> const struct child_device_config *child; >> - int i, count = 0; >> + int count = 0; >> >> /* >> * Only parse SDVO mappings on gens that could have SDVO. This isn't >> @@ -461,8 +468,8 @@ 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(devdata, &dev_priv->vbt.display_devices, node) { >> + child = &devdata->child; >> >> if (child->slave_addr != SLAVE_ADDR1 && >> child->slave_addr != SLAVE_ADDR2) { >> @@ -1572,8 +1579,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 display_device_data *devdata; >> >> if (!HAS_DDI(dev_priv) && !IS_CHERRYVIEW(dev_priv)) >> return; >> @@ -1581,11 +1587,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(devdata, &dev_priv->vbt.display_devices, node) >> + parse_ddi_port(dev_priv, &devdata->child, bdb_version); >> } >> >> static void >> @@ -1593,8 +1596,9 @@ parse_general_definitions(struct drm_i915_private >> *dev_priv, >> const struct bdb_header *bdb) >> { >> const struct bdb_general_definitions *defs; >> + struct display_device_data *devdata; >> const struct child_device_config *child; >> - int i, child_device_num, count; >> + int i, child_device_num; >> u8 expected_size; >> u16 block_size; >> int bus_pin; >> @@ -1650,26 +1654,7 @@ 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) >> @@ -1678,15 +1663,23 @@ parse_general_definitions(struct drm_i915_private >> *dev_priv, >> DRM_DEBUG_KMS("Found VBT child device with type 0x%x\n", >> child->device_type); >> >> + devdata = kzalloc(sizeof(*devdata), GFP_KERNEL); >> + if (!devdata) >> + break; >> + >> /* >> * 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, >> + memcpy(&devdata->child, child, >> min_t(size_t, defs->child_dev_size, sizeof(*child))); >> - count++; >> + >> + list_add(&devdata->node, &dev_priv->vbt.display_devices); > > Seems like we want list_add_tail() here or we'll reverse the child dev > order and get the port A vs. port E bug right back.
D'oh! Thanks. BR, Jani. > > Otherwise > Reviewed-by: Ville Syrjälä <[email protected]> > >> } >> + >> + if (list_empty(&dev_priv->vbt.display_devices)) >> + DRM_DEBUG_KMS("no child dev is parsed from VBT\n"); >> } >> >> /* Common defaults which may be overridden by VBT. */ >> @@ -1836,6 +1829,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv) >> const struct bdb_header *bdb; >> u8 __iomem *bios = NULL; >> >> + INIT_LIST_HEAD(&dev_priv->vbt.display_devices); >> + >> if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv)) { >> DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n"); >> return; >> @@ -1895,9 +1890,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 display_device_data *devdata, *n; >> + >> + list_for_each_entry_safe(devdata, n, &dev_priv->vbt.display_devices, >> node) { >> + list_del(&devdata->node); >> + kfree(devdata); >> + } >> + >> kfree(dev_priv->vbt.sdvo_lvds_vbt_mode); >> dev_priv->vbt.sdvo_lvds_vbt_mode = NULL; >> kfree(dev_priv->vbt.lfp_lvds_vbt_mode); >> @@ -1921,17 +1920,18 @@ 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 display_device_data *devdata; >> const struct child_device_config *child; >> - int i; >> >> if (!dev_priv->vbt.int_tv_support) >> return false; >> >> - if (!dev_priv->vbt.child_dev_num) >> + if (list_empty(&dev_priv->vbt.display_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(devdata, &dev_priv->vbt.display_devices, node) { >> + child = &devdata->child; >> + >> /* >> * If the device type is not TV, continue. >> */ >> @@ -1963,14 +1963,14 @@ 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 display_device_data *devdata; >> const struct child_device_config *child; >> - int i; >> >> - if (!dev_priv->vbt.child_dev_num) >> + if (list_empty(&dev_priv->vbt.display_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(devdata, &dev_priv->vbt.display_devices, node) { >> + child = &devdata->child; >> >> /* If the device type is not LFP, continue. >> * We have to check both the new identifiers as well as the >> @@ -2012,6 +2012,7 @@ 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 display_device_data *devdata; >> const struct child_device_config *child; >> static const struct { >> u16 dp, hdmi; >> @@ -2022,7 +2023,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 = >> @@ -2037,11 +2037,8 @@ 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(devdata, &dev_priv->vbt.display_devices, node) { >> + child = &devdata->child; >> >> if ((child->dvo_port == port_mapping[port].dp || >> child->dvo_port == port_mapping[port].hdmi) && >> @@ -2062,6 +2059,7 @@ 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 display_device_data *devdata; >> const struct child_device_config *child; >> static const short port_mapping[] = { >> [PORT_B] = DVO_PORT_DPB, >> @@ -2070,16 +2068,12 @@ 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; >> - >> - for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { >> - child = dev_priv->vbt.child_dev + i; >> + list_for_each_entry(devdata, &dev_priv->vbt.display_devices, node) { >> + child = &devdata->child; >> >> if (child->dvo_port == port_mapping[port] && >> (child->device_type & DEVICE_TYPE_eDP_BITS) == >> @@ -2128,13 +2122,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 display_device_data *devdata; >> >> - 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(devdata, &dev_priv->vbt.display_devices, node) { >> + if (child_dev_is_dp_dual_mode(&devdata->child, port)) >> return true; >> } >> >> @@ -2151,12 +2142,12 @@ 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 display_device_data *devdata; >> const struct child_device_config *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(devdata, &dev_priv->vbt.display_devices, node) { >> + child = &devdata->child; >> >> if (!(child->device_type & DEVICE_TYPE_MIPI_OUTPUT)) >> continue; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 67bdfe6de3fa..2c0674a86dd8 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -720,8 +720,7 @@ struct intel_vbt_data { >> >> int crt_ddc_pin; >> >> - int child_dev_num; >> - struct child_device_config *child_dev; >> + struct list_head display_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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
