Message ID | 20221108185004.2263578-1-wonchung@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2895962wru; Tue, 8 Nov 2022 11:08:23 -0800 (PST) X-Google-Smtp-Source: AMsMyM50J+1Z/fr0tP/FQ3RApdIUxwmYDDwvxit4ox2IXbrgG8m6Yak3en0nZ/oFeZc62pIjudil X-Received: by 2002:a17:907:7b94:b0:731:1b11:c241 with SMTP id ne20-20020a1709077b9400b007311b11c241mr55683103ejc.676.1667934503766; Tue, 08 Nov 2022 11:08:23 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667934503; cv=none; d=google.com; s=arc-20160816; b=Dr+mYhWpFekh+SFf16AtPrVb9HdMh43R3ZkeUPHNGCfxBj772w1NkAN/zvNHOuqmtA QFgBmc0Lg1dhpGPx+T1HgUCt0Eu++sdLyJxcLKoTWVWHiMbqGq1QmKrJq4CAM4mPiZjF sZWBxzmC36QTYmyj7NOQtE8wYnH705ZBh4kluQP84fUHAZIyyb6MX0nQmzRdg6W0mbTH qbLBS1vjakpRqHosDzr+mENuIyJCtknXzooZ2pvyUUUcbHef7w/MNKFr54vsT8/TGK0A SZKKyeg1SjV+T5Y1gvXM4KB+0REd0ZKdEDpBhBtdldUn1ajZd5vqpUvwAerdr0/DazYs eAsg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:to:from:subject:message-id:mime-version:date :dkim-signature; bh=ZB1r43VJCLEuCBOYhWr/N5mF6kTlkofIg4Tb9RTYcbk=; b=AUIRGce1onGcwfsQ0BC/mj+T44irZ8WOkXGMZY9GHWwQj3jwlgb2N6e/O+QNGpZAtC lPWybb/lanbQ7yM1s4fvJIMTHgbqkplbUb9pLXa7m1KOmhXGg4frr8rmPXmULa9GQaCG 77MEItmbfujKb40sg+DzlCRH2rG9BHn8dkErI85eYH+duTEGz3eZJ3rjvyP8TxVTxNPX xw0RZqTkvT5RHJsRyMHuRcWp9cOTRL01+cAaD7OtihumwdBis9M6ZqShXQiI8TWKV36t Aj2qlGjxkhZiP+JEzIOizMZJZZp+/dVLmQnyOKV7bfXTrTz6IwkAAdCsKLtq6kd81ZZs RVYA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=NisyEsfb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 6-20020a508e46000000b00462dca18096si12820852edx.520.2022.11.08.11.07.57; Tue, 08 Nov 2022 11:08:23 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=NisyEsfb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229597AbiKHSvP (ORCPT <rfc822;tony84727@gmail.com> + 99 others); Tue, 8 Nov 2022 13:51:15 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36824 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229730AbiKHSu6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 8 Nov 2022 13:50:58 -0500 Received: from mail-oa1-x4a.google.com (mail-oa1-x4a.google.com [IPv6:2001:4860:4864:20::4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 528EF742D6 for <linux-kernel@vger.kernel.org>; Tue, 8 Nov 2022 10:50:48 -0800 (PST) Received: by mail-oa1-x4a.google.com with SMTP id 586e51a60fabf-13bf576ffa6so7425536fac.9 for <linux-kernel@vger.kernel.org>; Tue, 08 Nov 2022 10:50:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=ZB1r43VJCLEuCBOYhWr/N5mF6kTlkofIg4Tb9RTYcbk=; b=NisyEsfbVYNv6Hsq46RUemq5+Nod7rKoNQcwWsLzOKRC3P3TWNrxadp2uEKtLNU5ff l7tXdYBT12CTvVajEmmkWrU6kkKibllyL4yBWeX/OH96i5+OPUZFDwycrxxHR26C4C2C U2Zcvf9qO4SfpoogqZ7eoImSdfA6bqHK0x7c9nVdIxLK6QMDB4N3XeL55053VhPJVQyg r9YTHT9cYnsFDYazzX+/4DcMPnmCUeUE8wPIjiC37qqaI+XwBN4JmKpgz650rNa8rlch fwdGiN0nYtwluN8Dw9C4yhMou1EfuGrMUWQZxqV18u29oweRiMENEM/g3LcEhvRdlkaV wsDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=ZB1r43VJCLEuCBOYhWr/N5mF6kTlkofIg4Tb9RTYcbk=; b=no8dCGrZ4sjKudZlnct0ufc9P0Bbo5k8KlmUPhy/0BytDf9Nty+ZKHg8CDiHLAamTH Cn7pX1vasqmrIegDGSHXbpW8D3T62CX0VBj4lTl7PoAKJT0Js0XbGjXSFjGgYOnmjgx9 SNNKYF1ViF2hdmz1+PGiOMeZhJwZu2XcNdCq9XKDfQSlOlfZohRKuv6Ka75oLe9pWGmX PMWWkqKU01MstSNB7t6fs4364go+8j/Jf5NvTO7lchW8H8AJP89J9NRZ6dGTlxTOD/+f 9INDOQyUwozs9TDX1RFdExjG/+wHOq5EPFyk44+K9rQSs5w0u1F6aQeAjihVDbuxJGzZ Ntdg== X-Gm-Message-State: ACrzQf1akNlUUUU5UP24kxIVWUZaJtooqC0+O5RgDlk6RhciPiMCn8iR 6OwVld7X2zk/Rw4JzKNk9cwu98S9YDySjQ== X-Received: from wonchungspecialist.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:1440]) (user=wonchung job=sendgmr) by 2002:a0d:df46:0:b0:36a:73db:2c2d with SMTP id i67-20020a0ddf46000000b0036a73db2c2dmr55535895ywe.130.1667933436613; Tue, 08 Nov 2022 10:50:36 -0800 (PST) Date: Tue, 8 Nov 2022 18:50:04 +0000 Mime-Version: 1.0 X-Mailer: git-send-email 2.38.1.431.g37b22c650d-goog Message-ID: <20221108185004.2263578-1-wonchung@google.com> Subject: [PATCH v4] drm/sysfs: Link DRM connectors to corresponding Type-C connectors From: Won Chung <wonchung@google.com> To: wonchung@google.com, bleung@google.com, pmalani@chromium.org, heikki.krogerus@linux.intel.com, imre.deak@intel.com, maarten.lankhorst@linux.intel.com, mripard@kernel.org, tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch, jani.nikula@linux.intel.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748956089998651268?= X-GMAIL-MSGID: =?utf-8?q?1748956089998651268?= |
Series |
[v4] drm/sysfs: Link DRM connectors to corresponding Type-C connectors
|
|
Commit Message
Won Chung
Nov. 8, 2022, 6:50 p.m. UTC
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, such as bandwidth used for a specific USB Type-C port. Signed-off-by: Won Chung <wonchung@google.com> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- 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(+)
Comments
On Tue, Nov 08, 2022 at 06:50:04PM +0000, Won Chung 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, such as bandwidth used for a > specific USB Type-C port. > > Signed-off-by: Won Chung <wonchung@google.com> > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > 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 We seem to be spinning wheels a bit here (or at least I'm missing a lot of important information from this series alone) with already at v4 but the fundamentals not answered: - where's the usb side of this, and anything we need to do in drivers? This should all be one series, or if that's too big, then a link in the cover letter for where to find all the other pieces - since I'm guessing this is for cros, will this also work on standard acpi x86 that are built for windows? arm with dt? Might be answered with the full picture - you say this helps userspace, but how? Best way here is to just point at the userspace change set that makes use of this link, code explains concepts much more precisely than lots of words, and it's also easier to review for corner cases that might be missed. That link also needs to be in the commit message/cover letter somewhere, so people can find it. In principle nothing against the idea, seems reasonable (but I'm also not sure what exact problem it's solving) - but all the detail work to make this work than an RFP to kick of some discussion is missing. And I think it's not even enough to really kick off a discussion as-is since there's really no user of this at all (in-kernel or userspace) linked. Cheers, Daniel > > > 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 430e00b16eec..6a9904fa9186 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_connector.h> > @@ -95,6 +97,34 @@ static char *drm_devnode(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"); > > /** > @@ -355,6 +385,13 @@ int drm_sysfs_connector_add(struct drm_connector *connector) > if (connector->ddc) > return sysfs_create_link(&connector->kdev->kobj, > &connector->ddc->dev.kobj, "ddc"); > + > + if (dev_fwnode(kdev)) { > + r = component_add(kdev, &typec_connector_ops); > + if (r) > + drm_err(dev, "failed to add component\n"); > + } > + > return 0; > > err_free: > @@ -367,6 +404,9 @@ void drm_sysfs_connector_remove(struct drm_connector *connector) > if (!connector->kdev) > return; > > + if (dev_fwnode(connector->kdev)) > + component_del(connector->kdev, &typec_connector_ops); > + > if (connector->ddc) > sysfs_remove_link(&connector->kdev->kobj, "ddc"); > > -- > 2.37.3.998.g577e59143f-goog >
Hi Daniel, Thank you very much for a review. On Wed, Nov 9, 2022 at 3:54 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Nov 08, 2022 at 06:50:04PM +0000, Won Chung 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, such as bandwidth used for a > > specific USB Type-C port. > > > > Signed-off-by: Won Chung <wonchung@google.com> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > 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 > > We seem to be spinning wheels a bit here (or at least I'm missing a lot of > important information from this series alone) with already at v4 but the > fundamentals not answered: > > - where's the usb side of this, and anything we need to do in drivers? > This should all be one series, or if that's too big, then a link in the > cover letter for where to find all the other pieces 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 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/ Since these are already submitted, do you think it would be a good idea to add the links in the commit message? > > - since I'm guessing this is for cros, will this also work on standard > acpi x86 that are built for windows? arm with dt? Might be answered with > the full picture Yes this is for cros, but it should work on any ACPI x86 as long as _PLD field for Type C connectors and DRM connectors are correctly added to the firmware. Since _PLD is ACPI specific, we do not have ARM with DT supported at the moment. In the future, if we find something similar to _PLD in DT, I think we can also use that in typec port-mapper for component matching. Heikki@ Can you correct me if I am incorrect or missing something? > > - you say this helps userspace, but how? Best way here is to just point at > the userspace change set that makes use of this link, code explains > concepts much more precisely than lots of words, and it's also easier to > review for corner cases that might be missed. That link also needs to be > in the commit message/cover letter somewhere, so people can find it. I do not have working code in the userspace yet since there is no symlink created between Type C connector and DRM connector at the moment. If this patch is to go through, ChromeOS will parse the symlink in DRM to find which Type C port got displays connected. First use case is metrics collection. We would like to know which port at which location is most preferred to be used for displays. We also want to see how many users charge the system on the same Type C port as displays. To answer these types of questions, we need to know which specific display uses which specific Type C connector. Second use case is to get a better idea on USB-C pin assignments. When a Type C port is in DP alt mode, there are some options on pin assignments, whether to prioritize DP bandwidth vs USB Superspeed. If we can find whether a display is actually being used by a Type C port, we can check the cases where the port prioritizes DP bandwidth over USB Superspeed while there is no display actually connected. This symlink can also be useful in the future with USB4 that makes use of DP tunnelling. Benson@ Can you add on to the use cases above if something is missing? I can try to create mock patches in ChromeOS to give an idea how userspace can utilize this symlink. Would this also work? If it sounds okay, I can work on the userspace mock patches and resend v5 with the links added to the commit message. > > In principle nothing against the idea, seems reasonable (but I'm also not > sure what exact problem it's solving) - but all the detail work to make > this work than an RFP to kick of some discussion is missing. And I think > it's not even enough to really kick off a discussion as-is since there's > really no user of this at all (in-kernel or userspace) linked. > > Cheers, Daniel > > > > > > > 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 430e00b16eec..6a9904fa9186 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_connector.h> > > @@ -95,6 +97,34 @@ static char *drm_devnode(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"); > > > > /** > > @@ -355,6 +385,13 @@ int drm_sysfs_connector_add(struct drm_connector *connector) > > if (connector->ddc) > > return sysfs_create_link(&connector->kdev->kobj, > > &connector->ddc->dev.kobj, "ddc"); > > + > > + if (dev_fwnode(kdev)) { > > + r = component_add(kdev, &typec_connector_ops); > > + if (r) > > + drm_err(dev, "failed to add component\n"); > > + } > > + > > return 0; > > > > err_free: > > @@ -367,6 +404,9 @@ void drm_sysfs_connector_remove(struct drm_connector *connector) > > if (!connector->kdev) > > return; > > > > + if (dev_fwnode(connector->kdev)) > > + component_del(connector->kdev, &typec_connector_ops); > > + > > if (connector->ddc) > > sysfs_remove_link(&connector->kdev->kobj, "ddc"); > > > > -- > > 2.37.3.998.g577e59143f-goog > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch Thank you, Won
On Thu, Nov 10, 2022 at 11:33:11AM -0800, Won Chung wrote: > Hi Daniel, > > Thank you very much for a review. > > On Wed, Nov 9, 2022 at 3:54 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Tue, Nov 08, 2022 at 06:50:04PM +0000, Won Chung 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, such as bandwidth used for a > > > specific USB Type-C port. > > > > > > Signed-off-by: Won Chung <wonchung@google.com> > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > --- > > > 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 > > > > We seem to be spinning wheels a bit here (or at least I'm missing a lot of > > important information from this series alone) with already at v4 but the > > fundamentals not answered: > > > > - where's the usb side of this, and anything we need to do in drivers? > > This should all be one series, or if that's too big, then a link in the > > cover letter for where to find all the other pieces > > 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 > > 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/ > > Since these are already submitted, do you think it would be a good > idea to add the links in the commit message? Hm yeah explaining this in the commit message with links should be good enough. Another thing, will this hit the component nesting problem? With this change we'll have drm drivers which are both aggregates and components at the same time, and last time someone tried this it all deadlocked in component.c. > > - since I'm guessing this is for cros, will this also work on standard > > acpi x86 that are built for windows? arm with dt? Might be answered with > > the full picture > > Yes this is for cros, but it should work on any ACPI x86 as long as > _PLD field for Type C connectors and DRM connectors are correctly > added to the firmware. > > Since _PLD is ACPI specific, we do not have ARM with DT supported at the moment. > In the future, if we find something similar to _PLD in DT, I think we > can also use that in typec port-mapper for component matching. > > Heikki@ Can you correct me if I am incorrect or missing something? Ok sounds good, would be good to include this in the commit message, too. > > > > > > - you say this helps userspace, but how? Best way here is to just point at > > the userspace change set that makes use of this link, code explains > > concepts much more precisely than lots of words, and it's also easier to > > review for corner cases that might be missed. That link also needs to be > > in the commit message/cover letter somewhere, so people can find it. > > I do not have working code in the userspace yet since there is no > symlink created between Type C connector and DRM connector at the > moment. > If this patch is to go through, ChromeOS will parse the symlink in DRM > to find which Type C port got displays connected. This isn't how new uapi works, we need to have the userspace together with the kernel changes. Otherwise ... how can you test that things actually work before we commit to the kernel change? Freezing down uapi on the promise that userspace will show up and that we're crossing fingers that it'll all work is not how we roll in drm. > First use case is metrics collection. > We would like to know which port at which location is most preferred > to be used for displays. > We also want to see how many users charge the system on the same Type > C port as displays. > To answer these types of questions, we need to know which specific > display uses which specific Type C connector. > > Second use case is to get a better idea on USB-C pin assignments. > When a Type C port is in DP alt mode, there are some options on pin > assignments, whether to prioritize DP bandwidth vs USB Superspeed. > If we can find whether a display is actually being used by a Type C > port, we can check the cases where the port prioritizes DP bandwidth > over USB Superspeed while there is no display actually connected. > > This symlink can also be useful in the future with USB4 that makes use > of DP tunnelling. > > Benson@ Can you add on to the use cases above if something is missing? Above should probably all be included somewhere in the commit message too. > I can try to create mock patches in ChromeOS to give an idea how > userspace can utilize this symlink. > Would this also work? drm expects fully reviewed&ready for merge patches, not just mock tests. Also ideally some igt uapi tests, but for a symlink I honestly don't know what that should be so I guess we can skip that :-) For details see our docs: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validation > If it sounds okay, I can work on the userspace mock patches and resend > v5 with the links added to the commit message. Sounds good! Cheers, Daniel > > > > > > In principle nothing against the idea, seems reasonable (but I'm also not > > sure what exact problem it's solving) - but all the detail work to make > > this work than an RFP to kick of some discussion is missing. And I think > > it's not even enough to really kick off a discussion as-is since there's > > really no user of this at all (in-kernel or userspace) linked. > > > > Cheers, Daniel > > > > > > > > > > > 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 430e00b16eec..6a9904fa9186 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_connector.h> > > > @@ -95,6 +97,34 @@ static char *drm_devnode(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"); > > > > > > /** > > > @@ -355,6 +385,13 @@ int drm_sysfs_connector_add(struct drm_connector *connector) > > > if (connector->ddc) > > > return sysfs_create_link(&connector->kdev->kobj, > > > &connector->ddc->dev.kobj, "ddc"); > > > + > > > + if (dev_fwnode(kdev)) { > > > + r = component_add(kdev, &typec_connector_ops); > > > + if (r) > > > + drm_err(dev, "failed to add component\n"); > > > + } > > > + > > > return 0; > > > > > > err_free: > > > @@ -367,6 +404,9 @@ void drm_sysfs_connector_remove(struct drm_connector *connector) > > > if (!connector->kdev) > > > return; > > > > > > + if (dev_fwnode(connector->kdev)) > > > + component_del(connector->kdev, &typec_connector_ops); > > > + > > > if (connector->ddc) > > > sysfs_remove_link(&connector->kdev->kobj, "ddc"); > > > > > > -- > > > 2.37.3.998.g577e59143f-goog > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > Thank you, > Won
Greeting, FYI, we noticed WARNING:at_drivers/base/component.c:#component_del due to commit (built with gcc-11): commit: 55495ab682271956fda4d2728480a8cd9e0afaa2 ("[PATCH v4] drm/sysfs: Link DRM connectors to corresponding Type-C connectors") url: https://github.com/intel-lab-lkp/linux/commits/Won-Chung/drm-sysfs-Link-DRM-connectors-to-corresponding-Type-C-connectors/20221109-025232 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/all/20221108185004.2263578-1-wonchung@google.com/ patch subject: [PATCH v4] drm/sysfs: Link DRM connectors to corresponding Type-C connectors in testcase: igt version: igt-x86_64-7da9f813-1_20221112 with following parameters: group: group-04 on test machine: 20 threads 1 sockets (Commet Lake) with 16G memory caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): [ 75.976323][ T2291] ------------[ cut here ]------------ [ 75.981626][ T2291] WARNING: CPU: 9 PID: 2291 at drivers/base/component.c:821 component_del+0x38a/0x4b0 [ 75.990972][ T2291] Modules linked in: i915(-) drm_buddy drm_display_helper ttm drm_kms_helper drm netconsole intel_rapl_msr intel_rapl_common btrfs blake2b_generic xor raid6_pq zstd_compress libcrc32c sd_mod x86_pkg_temp_thermal ipmi_devintf intel_powerclamp t10_pi coretemp ipmi_msghandler crc64_rocksoft_generic kvm_intel crc64_rocksoft crc64 sg kvm irqbypass crct10dif_pclmul crc32_pclmul intel_gtt crc32c_intel ghash_clmulni_intel sdhci_pci cqhci sha512_ssse3 ahci sdhci libahci rapl intel_cstate intel_wmi_thunderbolt wmi_bmof ppdev i2c_designware_platform mei_me i2c_i801 syscopyarea video parport_pc i2c_smbus libata intel_uncore sysfillrect mmc_core serio_raw mei i2c_designware_core idma64 intel_pch_thermal sysimgblt parport wmi intel_pmc_core acpi_pad acpi_tad fuse ip_tables [last unloaded: drm] [ 76.061091][ T2291] CPU: 9 PID: 2291 Comm: i915_module_loa Tainted: G W 6.1.0-rc2-00560-g55495ab68227 #1 [ 76.071896][ T2291] RIP: 0010:component_del+0x38a/0x4b0 [ 76.077103][ T2291] Code: 48 89 f8 48 c1 e8 03 42 80 3c 20 00 75 6b 48 39 6b 20 75 a4 48 c7 43 20 00 00 00 00 eb 9a 48 c7 c7 00 b1 1a 85 e8 16 ad 04 01 <0f> 0b 31 ed 5b 48 89 ef 5d 41 5c 41 5d 41 5e 41 5f e9 50 84 2b ff [ 76.096428][ T2291] RSP: 0018:ffffc90001cd7b28 EFLAGS: 00010282 [ 76.102339][ T2291] RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffffff834ef740 [ 76.110135][ T2291] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffffc90001cd7ad0 [ 76.117927][ T2291] RBP: ffffffff851ab0c0 R08: 0000000000000001 R09: ffffc90001cd7ad7 [ 76.125717][ T2291] R10: fffff5200039af5a R11: 0000000000000001 R12: ffff888433f9f800 [ 76.133516][ T2291] R13: ffffffffa034d4e0 R14: ffff888106d2f038 R15: ffffffffa0389500 [ 76.141312][ T2291] FS: 00007f1ff5227bc0(0000) GS:ffff8883cae80000(0000) knlGS:0000000000000000 [ 76.150060][ T2291] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 76.156507][ T2291] CR2: 00005648c3f42428 CR3: 000000045664a005 CR4: 00000000003706e0 [ 76.164308][ T2291] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 76.172106][ T2291] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 76.179897][ T2291] Call Trace: [ 76.183049][ T2291] <TASK> [ 76.185845][ T2291] drm_sysfs_connector_remove+0x6b/0x190 [drm] [ 76.191864][ T2291] drm_connector_unregister+0x1e2/0x2c0 [drm] [ 76.197788][ T2291] drm_connector_unregister_all+0x6d/0xf0 [drm] [ 76.203884][ T2291] ? drm_connector_free_work_fn+0x100/0x100 [drm] [ 76.210157][ T2291] ? ktime_get_mono_fast_ns+0x85/0x1d0 [ 76.215456][ T2291] drm_modeset_unregister_all+0xe/0x30 [drm] [ 76.221296][ T2291] drm_dev_unregister+0x19d/0x220 [drm] [ 76.226711][ T2291] drm_dev_unplug+0x48/0xb0 [drm] [ 76.231616][ T2291] i915_driver_remove+0xb6/0x140 [i915] [ 76.237100][ T2291] i915_pci_remove+0x39/0x80 [i915] [ 76.242216][ T2291] pci_device_remove+0x96/0x1c0 [ 76.246922][ T2291] device_release_driver_internal+0x3b9/0x600 [ 76.252819][ T2291] driver_detach+0xc0/0x180 [ 76.257170][ T2291] ? i915_pci_register_driver+0x20/0x20 [i915] [ 76.263235][ T2291] bus_remove_driver+0xe4/0x2d0 [ 76.267934][ T2291] ? d_lru_add+0xa6/0xf0 [ 76.272029][ T2291] ? i915_pci_register_driver+0x20/0x20 [i915] [ 76.278088][ T2291] pci_unregister_driver+0x26/0x260 [ 76.283133][ T2291] ? unregister_sysctl_table+0x7d/0x170 [ 76.289123][ T2291] ? i915_pci_register_driver+0x20/0x20 [i915] [ 76.295184][ T2291] i915_exit+0x7b/0xc5 [i915] [ 76.299814][ T2291] __do_sys_delete_module+0x2ea/0x510 [ 76.306059][ T2291] ? module_flags+0x300/0x300 [ 76.310581][ T2291] ? __fget_light+0x51/0x220 [ 76.315017][ T2291] ? __blkcg_punt_bio_submit+0x1b0/0x1b0 [ 76.320494][ T2291] ? _raw_spin_lock+0x81/0xd0 [ 76.325018][ T2291] ? _raw_write_lock_irq+0xd0/0xd0 [ 76.329970][ T2291] ? exit_to_user_mode_loop+0xbc/0x120 [ 76.335274][ T2291] ? exit_to_user_mode_prepare+0x89/0xd0 [ 76.340744][ T2291] do_syscall_64+0x35/0x80 [ 76.345008][ T2291] entry_SYSCALL_64_after_hwframe+0x5e/0xc8 [ 76.350732][ T2291] RIP: 0033:0x7f1ff75d74d7 [ 76.355003][ T2291] Code: 73 01 c3 48 8b 0d b9 19 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 89 19 0d 00 f7 d8 64 89 01 48 [ 76.374334][ T2291] RSP: 002b:00007ffe07f5ca38 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0 [ 76.382558][ T2291] RAX: ffffffffffffffda RBX: 00005648c3f34da0 RCX: 00007f1ff75d74d7 [ 76.390353][ T2291] RDX: 000000000000001a RSI: 0000000000000800 RDI: 00005648c3f34e08 [ 76.398145][ T2291] RBP: 0000000000000000 R08: 00000000ffffffff R09: 00007ffe07f5b790 [ 76.405938][ T2291] R10: fffffffffffffeee R11: 0000000000000206 R12: 00007f1ff7788c22 [ 76.413736][ T2291] R13: 00007ffe07f5caf0 R14: 0000000000000000 R15: 0000000000000000 [ 76.421536][ T2291] </TASK> [ 76.424422][ T2291] ---[ end trace 0000000000000000 ]--- If you fix the issue, kindly add following tag | Reported-by: kernel test robot <yujie.liu@intel.com> | Link: https://lore.kernel.org/oe-lkp/202211161829.1e6323fc-yujie.liu@intel.com To reproduce: git clone https://github.com/intel/lkp-tests.git cd lkp-tests sudo bin/lkp install job.yaml # job file is attached in this email bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run sudo bin/lkp run generated-yaml-file # if come across any failure that blocks the test, # please remove ~/.lkp and /lkp dir to run from a clean state.
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 430e00b16eec..6a9904fa9186 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_connector.h> @@ -95,6 +97,34 @@ static char *drm_devnode(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"); /** @@ -355,6 +385,13 @@ int drm_sysfs_connector_add(struct drm_connector *connector) if (connector->ddc) return sysfs_create_link(&connector->kdev->kobj, &connector->ddc->dev.kobj, "ddc"); + + if (dev_fwnode(kdev)) { + r = component_add(kdev, &typec_connector_ops); + if (r) + drm_err(dev, "failed to add component\n"); + } + return 0; err_free: @@ -367,6 +404,9 @@ void drm_sysfs_connector_remove(struct drm_connector *connector) if (!connector->kdev) return; + if (dev_fwnode(connector->kdev)) + component_del(connector->kdev, &typec_connector_ops); + if (connector->ddc) sysfs_remove_link(&connector->kdev->kobj, "ddc");