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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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@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.

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 :-)

> > 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.

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@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?

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);


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?

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?

Br,

-- 
heikki