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

[Jani Nikula]

On Fri, 23 May 2025, Heikki Krogerus <heikki.krogerus at linux.intel.com> wrote:
> Hi Jani,
>
> On Fri, May 23, 2025 at 12:28:11PM +0300, Jani Nikula wrote:
>> 
>> Resurrecting an old thread because I am clueless and I have
>> questions. :)
>> 
>> On Tue, 17 Aug 2021, Hans de Goede <hdegoede at redhat.com> wrote:
>> > From: Heikki Krogerus <heikki.krogerus at 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.
>
> That would be ideal. I did not know how to do that at the time.
>
>> 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...?
>
> Probable nothing :-)

I guess we'd still have to maintain the same order, somehow? That's
still a problem.

>
>> > 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?
>
> If you evaluated the _ADR (address) before i915 was loaded you got
> different value than what you got after the driver was loaded. In
> practice it meant that you could not determine which ACPI device node
> would end up representing which connector before the driver was fully
> loaded.
>
> But is the intel operation region even used anymore? I can't see its
> UUID in the tables of the MTL or even RPL systems that we have.

What does it mean? What we write in DIDL doesn't matter for such
systems? What does that mean for the ordering?

I'm just clueless about this. :/

> In any case, this is not a huge problem I think.
>
>> > 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 at linux.intel.com>
>> > Tested-by: Heikki Krogerus <heikki.krogerus at linux.intel.com>
>> > Signed-off-by: Hans de Goede <hdegoede at 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?
>
> If it is_acpi_node(fwnode) then yes, you can use
> to_acpi_device_node(fwnode)
>
> Each connector does have an ACPI device node in the ACPI tables under
> the parent GFX controller device, so yes one should always 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.
>
> You could already do something like this to make the code work with
> both ACPI and DT:
>
> +static struct drm_panel *drm_find_panel(const struct fwnode_handle *fwnode)
> +{
> +       struct drm_panel *panel;
> +
> +       if (!fwnode_device_is_available(fwnode))
> +               return ERR_PTR(-ENODEV);
> +
> +       mutex_lock(&panel_lock);
> +
> +       list_for_each_entry(panel, &panel_list, list) {
> +               if (dev_fwnode(panel->dev) == fwnode) {
> +                       mutex_unlock(&panel_lock);
> +                       return panel;
> +               }
> +       }
> +
> +       mutex_unlock(&panel_lock);
> +       return ERR_PTR(-EPROBE_DEFER);
> +}
> +
>  int drm_panel_add_follower(struct device *follower_dev,
>                            struct drm_panel_follower *follower)
>  {
> -       struct device_node *panel_np;
> +       struct fwnode_handle *fwnode;
>         struct drm_panel *panel;
>         int ret;
>  
> -       panel_np = of_parse_phandle(follower_dev->of_node, "panel", 0);
> -       if (!panel_np)
> +       fwnode = fwnode_find_reference(dev_fwnode(follower_dev), "panel", 0);
> +       if (!fwnode)
>                 return -ENODEV;
>  
> -       panel = of_drm_find_panel(panel_np);
> -       of_node_put(panel_np);
> +       panel = drm_find_panel(fwnode);
> +       fwnode_handle_put(fwnode);
>         if (IS_ERR(panel))
>                 return PTR_ERR(panel);

Cool, thanks!

> But that would only work if the follower_dev had a _DSD device
> property named "panel" that has a reference to the ACPI device node
> that represents the panel as its value. Is the panel here the same
> thing as the connector?

For our purposes I think it would be. But I don't know what that
reference would look like, or what the connector ACPI things look like.

> I'm pretty sure there is no such device property (or is there?), so
> how are you going to find the correct panel ACPI node for the
> follower_dev?

I'm being told such thing doesn't exist, but could be defined. Again,
I'm clueless.


BR,
Jani.


-- 
Jani Nikula, Intel