Message-ID: <CAMNLLoQ=tOf4VT2_1qNX+fzHYv-8HL-WB8EVh_AAJ7BSmcm3eg@mail.gmail.com>
Date:   Mon, 24 Apr 2023 15:25:47 -0700
From:   Manasi Navare <navaremanasi@...gle.com>
To:     Won Chung <wonchung@...gle.com>
Cc:     bleung@...gle.com, pmalani@...omium.org,
        heikki.krogerus@...ux.intel.com, imre.deak@...el.com,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
        jani.nikula@...ux.intel.com, gildekel@...omium.org,
        seanpaul@...omium.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] drm/sysfs: Link DRM connectors to corresponding Type-C connectors

On Tue, Mar 28, 2023 at 6:23 PM Won Chung <wonchung@...gle.com> wrote:
>
> Create a symlink pointing to USB Type-C connector for DRM connectors
> when they are created. The link will be created only if the firmware is
> able to describe the connection beween the two connectors.
>
> Currently, even if a display uses a USB Type-C port, there is no way for
> the userspace to find which port is used for which display. With the
> symlink, display information would be accessible from Type-C connectors
> and port information would be accessible from DRM connectors.
>
> Associating the two subsystems, userspace would have potential to expose
> and utilize more complex information. ChromeOS intend to use this
> information for metrics collection. For example, we want to tell which
> port is deriving which displays. Also, combined with USB PD information,
> we can tell whether user is charging their device through display.
> Chromium patch for parsing the symlink from the kernel is at
> http://crrev.com/c/4317207.
>
> We already have a framework in typec port-mapper.c where it goes through
> component devices and runs the bind functions for those with matching
> _PLD (physical location of device).
> https://elixir.bootlin.com/linux/v5.18.1/source/drivers/usb/typec/port-mapper.c
> Since _PLD is ACPI specific field, this linking would only work on ACPI
> x86 as long as _PLD field for Type-C connectors and DRM connectors are
> correctly added to the firmware.
>
> Currently, USB ports and USB4 ports are added as components to create a
> symlink with Type C connector.
> USB:
> https://lore.kernel.org/all/20211223082349.45616-1-heikki.krogerus@linux.intel.com/
> USB4:
> https://lore.kernel.org/all/20220418175932.1809770-3-wonchung@google.com/
> So, we follow the same pattern in this patch.
>
> Signed-off-by: Won Chung <wonchung@...gle.com>
> Acked-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
> Changes from v4: (4 months ago)
> - Update commit message with an actual use case from cros userspace
> - Move component_add to before ddc check which possibly returns
> - Rebased
>
> Changes from v3:
> - Append to the commit message on why this patch is needed
>
> Changes from v2:
> - Resend the patch to dri-devel list
>
> Changes from v1:
> - Fix multiple lines to single line
>
>
>  drivers/gpu/drm/drm_sysfs.c | 40 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 183130355997..c21312e87aa2 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -11,12 +11,14 @@
>   */
>
>  #include <linux/acpi.h>
> +#include <linux/component.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/gfp.h>
>  #include <linux/i2c.h>
>  #include <linux/kdev_t.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>
>  #include <drm/drm_accel.h>
> @@ -96,6 +98,34 @@ static char *drm_devnode(const struct device *dev, umode_t *mode)
>         return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
>  }
>
> +static int typec_connector_bind(struct device *dev,
> +       struct device *typec_connector, void *data)
> +{
> +       int ret;
> +
> +       ret = sysfs_create_link(&dev->kobj, &typec_connector->kobj, "typec_connector");
> +       if (ret)
> +               return ret;
> +
> +       ret = sysfs_create_link(&typec_connector->kobj, &dev->kobj, "drm_connector");
> +       if (ret)
> +               sysfs_remove_link(&dev->kobj, "typec_connector");
> +
> +       return ret;
> +}
> +
> +static void typec_connector_unbind(struct device *dev,
> +       struct device *typec_connector, void *data)
> +{
> +       sysfs_remove_link(&typec_connector->kobj, "drm_connector");
> +       sysfs_remove_link(&dev->kobj, "typec_connector");
> +}
> +
> +static const struct component_ops typec_connector_ops = {
> +       .bind = typec_connector_bind,
> +       .unbind = typec_connector_unbind,
> +};
> +
>  static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>
>  /**
> @@ -353,9 +383,16 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>
>         connector->kdev = kdev;
>
> +       if (dev_fwnode(kdev)) {
> +               r = component_add(kdev, &typec_connector_ops);
> +               if (r)
> +                       drm_err(dev, "failed to add component\n");

Can you add more info in this drm_err message about the component that failed to
make it more intuitive

After this fix,

Reviewed-by: Manasi Navare <navaremanasi@...omium.org>

Manasi

> +       }
> +
>         if (connector->ddc)
>                 return sysfs_create_link(&connector->kdev->kobj,
>                                  &connector->ddc->dev.kobj, "ddc");
> +
>         return 0;
>
>  err_free:
> @@ -371,6 +408,9 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
>         if (connector->ddc)
>                 sysfs_remove_link(&connector->kdev->kobj, "ddc");
>
> +       if (dev_fwnode(connector->kdev))
> +               component_del(connector->kdev, &typec_connector_ops);
> +
>         DRM_DEBUG("removing \"%s\" from sysfs\n",
>                   connector->name);
>
> --
> 2.40.0.348.gf938b09366-goog
>

Powered by blists - more mailing lists