Resurrecting an old thread because I am clueless and I have
questions. :)
On Tue, 17 Aug 2021, Hans de Goede <hdego...@redhat.com> wrote:
> From: Heikki Krogerus <heikki.kroge...@linux.intel.com>
>
> 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.
Is this referring to intel_didl_outputs()?
First, it's curious that intel_didl_outputs() is only called on the
resume paths, not at probe. I don't think the DIDL is set when
intel_acpi_assign_connector_fwnodes() is called. But is it only the
order that matters? Should we do intel_didl_outputs() at probe too?
Currently, we register all connectors first, move panel (as opposed to
external) connectors in front, and that's the fixed connector order
we'll use.
I am wondering if it would be possible to do what this patch does as we
register each connector, not afterwards.
It would involve something like this:
- Figure out intel_connector->acpi_device_id at connector register time
- Figure out the index for DIDL at connector register time
- Figure out connector->fwnode at connector register time
What could possibly go wrong...?
> 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).
Could someone elaborate, please?
> Changes in v2 (Hans de goede):
> - Take a reference on the fwnode which we assign to the connector,
> for ACPI nodes this is a no-op but in the future we may see
> software-fwnodes assigned to connectors which are ref-counted.
>
> Signed-off-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> Tested-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> Signed-off-by: Hans de Goede <hdego...@redhat.com>
> ---
> drivers/gpu/drm/i915/display/intel_acpi.c | 46 ++++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_acpi.h | 3 ++
> drivers/gpu/drm/i915/display/intel_display.c | 1 +
> 3 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c
> b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 7cfe91fc05f2..72cac55c0f0f 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -282,3 +282,49 @@ 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 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(drm_dev->dev, fwnode);
> + if (!fwnode)
> + break;
> +
> + 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(drm_dev->dev),
> + 0x1f, 0);
> + if (adev) {
> + connector->fwnode =
> +
> fwnode_handle_get(acpi_fwnode_handle(adev));
> + break;
> + }
> + fallthrough;
> + default:
> + connector->fwnode = fwnode_handle_get(fwnode);
Is it possible to get the struct acpi_device for all fwnodes? Does one
exist?
Specifically, I think I need a struct device that's also an ACPI device
to pass to devm_drm_panel_alloc(), so that a subsequent
drm_panel_add_follower() can use ACPI to look up the panel/connector.
BR,
Jani.
> + break;
> + }
> + }
> + drm_connector_list_iter_end(&conn_iter);
> + /*
> + * device_get_next_child_node() takes a reference on the fwnode, if
> + * we stopped iterating because we are out of connectors we need to
> + * put this, otherwise fwnode is NULL and the put is a no-op.
> + */
> + fwnode_handle_put(fwnode);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h
> b/drivers/gpu/drm/i915/display/intel_acpi.h
> index 9f197401c313..4a760a2baed9 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.h
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.h
> @@ -13,6 +13,7 @@ void intel_register_dsm_handler(void);
> void intel_unregister_dsm_handler(void);
> void intel_dsm_get_bios_data_funcs_supported(struct drm_i915_private *i915);
> 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; }
> @@ -20,6 +21,8 @@ static inline
> void intel_dsm_get_bios_data_funcs_supported(struct drm_i915_private *i915)
> { 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 a257e5dc381c..88e5fff64b8c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12561,6 +12561,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) {
--
Jani Nikula, Intel
