Re: [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/4/21 9:52 AM, Andy Shevchenko wrote:
> 
> 
> On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> 
>     From: Heikki Krogerus <heikki.krogerus@linux.intel.com <mailto: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.
> 
>     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@linux.intel.com <mailto:heikki.krogerus@linux.intel.com>>
>     [hdegoede@redhat.com <mailto:hdegoede@redhat.com>: Move intel_acpi_assign_connector_fwnodes() to
>      intel_acpi.c]
>     Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>     ---
>      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
> 
>