Hi, On 5/4/21 9:52 AM, Andy Shevchenko wrote: > > > On Monday, May 3, 2021, Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> wrote: > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx <mailto:heikki.krogerus@xxxxxxxxxxxxxxx>> > > On Intel platforms we know that the ACPI connector device > node order will follow the order the driver (i915) decides. > The decision is made using the custom Intel ACPI OpRegion > (intel_opregion.c), though the driver does not actually know > that the values it sends to ACPI there are used for > associating a device node for the connectors, and assigning > address for them. > > In reality that custom Intel ACPI OpRegion actually violates > ACPI specification (we supply dynamic information to objects > that are defined static, for example _ADR), however, it > makes assigning correct connector node for a connector entry > straightforward (it's one-on-one mapping). > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx <mailto:heikki.krogerus@xxxxxxxxxxxxxxx>> > [hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>: Move intel_acpi_assign_connector_fwnodes() to > intel_acpi.c] > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> > --- > drivers/gpu/drm/i915/display/intel_acpi.c | 40 ++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_acpi.h | 3 ++ > drivers/gpu/drm/i915/display/intel_display.c | 1 + > 3 files changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c > index 833d0c1be4f1..9f266dfda7dd 100644 > --- a/drivers/gpu/drm/i915/display/intel_acpi.c > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c > @@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv) > } > drm_connector_list_iter_end(&conn_iter); > } > + > +/* NOTE: The connector order must be final before this is called. */ > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_device *drm_dev = &i915->drm; > + struct device *kdev = &drm_dev->pdev->dev; > + struct fwnode_handle *fwnode = NULL; > + struct drm_connector *connector; > + struct acpi_device *adev; > + > + drm_connector_list_iter_begin(drm_dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + /* Always getting the next, even when the last was not used. */ > + fwnode = device_get_next_child_node(kdev, fwnode); > + if (!fwnode) > + break; > > > > Who is dropping reference counting on fwnode ? We are dealing with ACPI fwnode-s here and those are not ref-counted, they are embedded inside a struct acpi_device and their lifetime is tied to that struct. They should probably still be ref-counted (with the count never dropping to 0) so that the generic fwnode functions behave the same anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() in drivers/acpi/property.c which is the get_next_child_node() implementation for ACPI fwnode-s. > I’m in the middle of a pile of fixes for fwnode refcounting when for_each_child or get_next_child is used. So, please double check you drop a reference. The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node() do not mention anything about these functions returning a reference. So I think we need to first make up our mind here how we want this all to work and then fix the actual implementation and docs before fixing callers. Regards, Hans > > > + > + switch (connector->connector_type) { > + case DRM_MODE_CONNECTOR_LVDS: > + case DRM_MODE_CONNECTOR_eDP: > + case DRM_MODE_CONNECTOR_DSI: > + /* > + * Integrated displays have a specific address 0x1f on > + * most Intel platforms, but not on all of them. > + */ > + adev = acpi_find_child_device(ACPI_COMPANION(kdev), > + 0x1f, 0); > + if (adev) { > + connector->fwnode = acpi_fwnode_handle(adev); > + break; > + } > + fallthrough; > + default: > + connector->fwnode = fwnode; > + break; > + } > + } > + drm_connector_list_iter_end(&conn_iter); > +} > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h > index e8b068661d22..d2435691f4b5 100644 > --- a/drivers/gpu/drm/i915/display/intel_acpi.h > +++ b/drivers/gpu/drm/i915/display/intel_acpi.h > @@ -12,11 +12,14 @@ struct drm_i915_private; > void intel_register_dsm_handler(void); > void intel_unregister_dsm_handler(void); > void intel_acpi_device_id_update(struct drm_i915_private *i915); > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915); > #else > static inline void intel_register_dsm_handler(void) { return; } > static inline void intel_unregister_dsm_handler(void) { return; } > static inline > void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; } > +static inline > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { return; } > #endif /* CONFIG_ACPI */ > > #endif /* __INTEL_ACPI_H__ */ > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 828ef4c5625f..87cad549632c 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915) > > drm_modeset_lock_all(dev); > intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); > + intel_acpi_assign_connector_fwnodes(i915); > drm_modeset_unlock_all(dev); > > for_each_intel_crtc(dev, crtc) { > -- > 2.31.1 > > > > -- > With Best Regards, > Andy Shevchenko > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
