Re: [PATCH 5/8] drm/i915: Associate ACPI connector nodes with connector entries (v2)

[Jani Nikula <jani.nikula@linux.intel.com>]

Resurrecting an old thread because I am clueless and I have
questions. :)

On Tue, 17 Aug 2021, Hans de Goede <hdegoede@redhat.com> wrote:
> From: Heikki Krogerus <heikki.krogerus@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.krogerus@linux.intel.com>
> Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@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