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

Hans de Goede
2021-05-05T09:07:38Z

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