| Message ID | 20230903214150.2877023-2-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm,usb/typec: uABI for USB-C DisplayPort connectors | expand |
Hi Dmitry, On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > dev_fwnode() checks never succeed, making the respective commit NOP. That's not true. The dev->fwnode is assigned when the device is created on ACPI platforms automatically. If the drm_connector fwnode member is assigned before the device is registered, then that fwnode is assigned also to the device - see drm_connector_acpi_find_companion(). But please note that even if drm_connector does not have anything in its fwnode member, the device may still be assigned fwnode, just based on some other logic (maybe in drivers/acpi/acpi_video.c?). > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > breaks drivers already using components (as it was pointed at [1]), > resulting in a deadlock. Lockdep trace is provided below. > > Granted these two issues, it seems impractical to fix this commit in any > sane way. Revert it instead. I think there is already user space stuff that relies on these links, so I'm not sure you can just remove them like that. If the component framework is not the correct tool here, then I think you need to suggest some other way of creating them. Side note. The problem you are describing here is a limitation in the component framework - right now it's made with the idea that a device can represent a single component, but it really should allow a device to represent multiple components. I'm not saying that you should try to fix the component framework, but I just wanted to make a note about this (and this is not the only problem with the component framework). I like the component framework as a concept, but I think it needs a lot of improvements - possibly rewrite. thanks,
Hi Heikki, On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi Dmitry, > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > dev_fwnode() checks never succeed, making the respective commit NOP. > > That's not true. The dev->fwnode is assigned when the device is > created on ACPI platforms automatically. If the drm_connector fwnode > member is assigned before the device is registered, then that fwnode > is assigned also to the device - see drm_connector_acpi_find_companion(). > > But please note that even if drm_connector does not have anything in > its fwnode member, the device may still be assigned fwnode, just based > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > breaks drivers already using components (as it was pointed at [1]), > > resulting in a deadlock. Lockdep trace is provided below. > > > > Granted these two issues, it seems impractical to fix this commit in any > > sane way. Revert it instead. > > I think there is already user space stuff that relies on these links, > so I'm not sure you can just remove them like that. If the component > framework is not the correct tool here, then I think you need to > suggest some other way of creating them. The issue (that was pointed out during review) is that having a component code in the framework code can lead to lockups. With the patch #2 in place (which is the only logical way to set kdev->fwnode for non-ACPI systems) probing of drivers which use components and set drm_connector::fwnode breaks immediately. Can we move the component part to the respective drivers? With the patch 2 in place, connector->fwnode will be copied to the created kdev's fwnode pointer. Another option might be to make this drm_sysfs component registration optional. > Side note. The problem you are describing here is a limitation in the > component framework - right now it's made with the idea that a device > can represent a single component, but it really should allow a device > to represent multiple components. I'm not saying that you should try > to fix the component framework, but I just wanted to make a note about > this (and this is not the only problem with the component framework). > > I like the component framework as a concept, but I think it needs a > lot of improvements - possibly rewrite. Yes. There were several attempts to rewrite the component framework, but none succeeded up to now. Anyway, I consider rewriting components framework to be a bigger topic compared to drm connector fwnode setup. -- With best wishes Dmitry
On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > Hi Heikki, > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > Hi Dmitry, > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > That's not true. The dev->fwnode is assigned when the device is > > created on ACPI platforms automatically. If the drm_connector fwnode > > member is assigned before the device is registered, then that fwnode > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > But please note that even if drm_connector does not have anything in > > its fwnode member, the device may still be assigned fwnode, just based > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > breaks drivers already using components (as it was pointed at [1]), > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > sane way. Revert it instead. > > > > I think there is already user space stuff that relies on these links, > > so I'm not sure you can just remove them like that. If the component > > framework is not the correct tool here, then I think you need to > > suggest some other way of creating them. > > The issue (that was pointed out during review) is that having a > component code in the framework code can lead to lockups. With the > patch #2 in place (which is the only logical way to set kdev->fwnode > for non-ACPI systems) probing of drivers which use components and set > drm_connector::fwnode breaks immediately. > > Can we move the component part to the respective drivers? With the > patch 2 in place, connector->fwnode will be copied to the created > kdev's fwnode pointer. > > Another option might be to make this drm_sysfs component registration optional. You don't need to use the component framework at all if there is a better way of determining the connection between the DP and its Type-C connector (I'm assuming that that's what this series is about). You just need the symlinks, not the component. thanks,
On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > Hi Heikki, > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > Hi Dmitry, > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > member is assigned before the device is registered, then that fwnode > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > But please note that even if drm_connector does not have anything in > > > its fwnode member, the device may still be assigned fwnode, just based > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > breaks drivers already using components (as it was pointed at [1]), > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > sane way. Revert it instead. > > > > > > I think there is already user space stuff that relies on these links, > > > so I'm not sure you can just remove them like that. If the component > > > framework is not the correct tool here, then I think you need to > > > suggest some other way of creating them. > > > > The issue (that was pointed out during review) is that having a > > component code in the framework code can lead to lockups. With the > > patch #2 in place (which is the only logical way to set kdev->fwnode > > for non-ACPI systems) probing of drivers which use components and set > > drm_connector::fwnode breaks immediately. > > > > Can we move the component part to the respective drivers? With the > > patch 2 in place, connector->fwnode will be copied to the created > > kdev's fwnode pointer. > > > > Another option might be to make this drm_sysfs component registration optional. > > You don't need to use the component framework at all if there is > a better way of determining the connection between the DP and its > Type-C connector (I'm assuming that that's what this series is about). > You just need the symlinks, not the component. The problem is that right now this component registration has become mandatory. And if I set the kdev->fwnode manually (like in the patch 2), the kernel hangs inside the component code. That's why I proposed to move the components to the place where they are really necessary, e.g. i915 and amd drivers.
On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus wrote: > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus wrote: > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > member is assigned before the device is registered, then that fwnode > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > But please note that even if drm_connector does not have anything in > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > sane way. Revert it instead. > > > > > > > > I think there is already user space stuff that relies on these links, > > > > so I'm not sure you can just remove them like that. If the component > > > > framework is not the correct tool here, then I think you need to > > > > suggest some other way of creating them. > > > > > > The issue (that was pointed out during review) is that having a > > > component code in the framework code can lead to lockups. With the > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > for non-ACPI systems) probing of drivers which use components and set > > > drm_connector::fwnode breaks immediately. > > > > > > Can we move the component part to the respective drivers? With the > > > patch 2 in place, connector->fwnode will be copied to the created > > > kdev's fwnode pointer. > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > You don't need to use the component framework at all if there is > > a better way of determining the connection between the DP and its > > Type-C connector (I'm assuming that that's what this series is about). > > You just need the symlinks, not the component. > > The problem is that right now this component registration has become > mandatory. And if I set the kdev->fwnode manually (like in the patch > 2), the kernel hangs inside the component code. > That's why I proposed to move the components to the place where they > are really necessary, e.g. i915 and amd drivers. I'm all for keeping the component framework out of common code. I dislike that framework with passion, and still haven't lost all hopes of replacing it with something better.
On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > Hi Heikki, > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > Hi Dmitry, > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > member is assigned before the device is registered, then that fwnode > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > But please note that even if drm_connector does not have anything in > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > sane way. Revert it instead. > > > > > > > > I think there is already user space stuff that relies on these links, > > > > so I'm not sure you can just remove them like that. If the component > > > > framework is not the correct tool here, then I think you need to > > > > suggest some other way of creating them. > > > > > > The issue (that was pointed out during review) is that having a > > > component code in the framework code can lead to lockups. With the > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > for non-ACPI systems) probing of drivers which use components and set > > > drm_connector::fwnode breaks immediately. > > > > > > Can we move the component part to the respective drivers? With the > > > patch 2 in place, connector->fwnode will be copied to the created > > > kdev's fwnode pointer. > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > You don't need to use the component framework at all if there is > > a better way of determining the connection between the DP and its > > Type-C connector (I'm assuming that that's what this series is about). > > You just need the symlinks, not the component. > > The problem is that right now this component registration has become > mandatory. And if I set the kdev->fwnode manually (like in the patch > 2), the kernel hangs inside the component code. > That's why I proposed to move the components to the place where they > are really necessary, e.g. i915 and amd drivers. So why can't we replace the component with the method you are proposing in this series of finding out the Type-C port also with i915, AMD, or whatever driver and platform (that's the only thing that component is used for)? Determining the connection between a DP and its Type-C connector is starting to get really important, so ideally we have a common solution for that. thanks,
On Wed, Sep 06, 2023 at 03:53:14PM +0300, Laurent Pinchart wrote: > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus wrote: > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus wrote: > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > member is assigned before the device is registered, then that fwnode > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > sane way. Revert it instead. > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > so I'm not sure you can just remove them like that. If the component > > > > > framework is not the correct tool here, then I think you need to > > > > > suggest some other way of creating them. > > > > > > > > The issue (that was pointed out during review) is that having a > > > > component code in the framework code can lead to lockups. With the > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > for non-ACPI systems) probing of drivers which use components and set > > > > drm_connector::fwnode breaks immediately. > > > > > > > > Can we move the component part to the respective drivers? With the > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > kdev's fwnode pointer. > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > You don't need to use the component framework at all if there is > > > a better way of determining the connection between the DP and its > > > Type-C connector (I'm assuming that that's what this series is about). > > > You just need the symlinks, not the component. > > > > The problem is that right now this component registration has become > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > 2), the kernel hangs inside the component code. > > That's why I proposed to move the components to the place where they > > are really necessary, e.g. i915 and amd drivers. > > I'm all for keeping the component framework out of common code. I > dislike that framework with passion, and still haven't lost all hopes of > replacing it with something better. I'm not sure I share the same hate for the component framework, but I agree. It's optional anyway, so we should provide a solution that works for drivers working with and without the component framework. Maxime
On 06/09/2023 16:38, Heikki Krogerus wrote: > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: >> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus >> <heikki.krogerus@linux.intel.com> wrote: >>> >>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: >>>> Hi Heikki, >>>> >>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus >>>> <heikki.krogerus@linux.intel.com> wrote: >>>>> >>>>> Hi Dmitry, >>>>> >>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: >>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so >>>>>> dev_fwnode() checks never succeed, making the respective commit NOP. >>>>> >>>>> That's not true. The dev->fwnode is assigned when the device is >>>>> created on ACPI platforms automatically. If the drm_connector fwnode >>>>> member is assigned before the device is registered, then that fwnode >>>>> is assigned also to the device - see drm_connector_acpi_find_companion(). >>>>> >>>>> But please note that even if drm_connector does not have anything in >>>>> its fwnode member, the device may still be assigned fwnode, just based >>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?). >>>>> >>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it >>>>>> breaks drivers already using components (as it was pointed at [1]), >>>>>> resulting in a deadlock. Lockdep trace is provided below. >>>>>> >>>>>> Granted these two issues, it seems impractical to fix this commit in any >>>>>> sane way. Revert it instead. >>>>> >>>>> I think there is already user space stuff that relies on these links, >>>>> so I'm not sure you can just remove them like that. If the component >>>>> framework is not the correct tool here, then I think you need to >>>>> suggest some other way of creating them. >>>> >>>> The issue (that was pointed out during review) is that having a >>>> component code in the framework code can lead to lockups. With the >>>> patch #2 in place (which is the only logical way to set kdev->fwnode >>>> for non-ACPI systems) probing of drivers which use components and set >>>> drm_connector::fwnode breaks immediately. >>>> >>>> Can we move the component part to the respective drivers? With the >>>> patch 2 in place, connector->fwnode will be copied to the created >>>> kdev's fwnode pointer. >>>> >>>> Another option might be to make this drm_sysfs component registration optional. >>> >>> You don't need to use the component framework at all if there is >>> a better way of determining the connection between the DP and its >>> Type-C connector (I'm assuming that that's what this series is about). >>> You just need the symlinks, not the component. >> >> The problem is that right now this component registration has become >> mandatory. And if I set the kdev->fwnode manually (like in the patch >> 2), the kernel hangs inside the component code. >> That's why I proposed to move the components to the place where they >> are really necessary, e.g. i915 and amd drivers. > > So why can't we replace the component with the method you are > proposing in this series of finding out the Type-C port also with > i915, AMD, or whatever driver and platform (that's the only thing that > component is used for)? The drm/msm driver uses drm_bridge for the pipeline (including the last DP entry) and the drm_bridge_connector to create the connector. I think that enabling i915 and AMD drivers to use drm_bridge fells out of scope for this series. > Determining the connection between a DP and its Type-C connector is > starting to get really important, so ideally we have a common solution > for that. Yes. This is what we have been discussing with Simon for quite some time on #dri-devel. Unfortunately I think the solution that got merged was pretty much hastened in instead of being well-thought. For example, it is also not always possible to provide the drm_connector / typec_connector links (as you can see from the patch7. Sometimes we can only express that this is a Type-C DP connector, but we can not easily point it to the particular USB-C port. So, I'm not sure, how can we proceed here. Currently merged patch breaks drm/msm if we even try to use it by setting kdef->fwnode to drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is an ACPI-only thing, which is not expected to work in a non-ACPI cases.
On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > On 06/09/2023 16:38, Heikki Krogerus wrote: > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > Hi Heikki, > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > framework is not the correct tool here, then I think you need to > > > > > > suggest some other way of creating them. > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > component code in the framework code can lead to lockups. With the > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > kdev's fwnode pointer. > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > You don't need to use the component framework at all if there is > > > > a better way of determining the connection between the DP and its > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > You just need the symlinks, not the component. > > > > > > The problem is that right now this component registration has become > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > 2), the kernel hangs inside the component code. > > > That's why I proposed to move the components to the place where they > > > are really necessary, e.g. i915 and amd drivers. > > > > So why can't we replace the component with the method you are > > proposing in this series of finding out the Type-C port also with > > i915, AMD, or whatever driver and platform (that's the only thing that > > component is used for)? > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > entry) and the drm_bridge_connector to create the connector. I think that > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > series. > > > > Determining the connection between a DP and its Type-C connector is > > starting to get really important, so ideally we have a common solution > > for that. > > Yes. This is what we have been discussing with Simon for quite some time on > #dri-devel. > > Unfortunately I think the solution that got merged was pretty much hastened > in instead of being well-thought. For example, it is also not always > possible to provide the drm_connector / typec_connector links (as you can > see from the patch7. Sometimes we can only express that this is a Type-C DP > connector, but we can not easily point it to the particular USB-C port. > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > drm/msm if we even try to use it by setting kdef->fwnode to > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > an ACPI-only thing, which is not expected to work in a non-ACPI cases. You really have to always supply not only the Type-C ports and partners, but also the alt modes. You need them, firstly to keep things sane inside kernel, but more importantly, so they are always exposed to the user space, AND, always the same way. We have ABIs for all this stuff, including the DP alt mode. Use them. No shortcuts. So here's what you need to do. UCSI does not seem to bring you anything useful, so just disable it for now. You don't need it. Your port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so that's where you need to register all these components - the ports, partners and alt modes. You have all the needed information there. Only after you've done that we can start to look at how should the connection between the DPs and their USB Type-C connectors be handled. thanks,
On 12/09/2023 14:05, Heikki Krogerus wrote: > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: >> On 06/09/2023 16:38, Heikki Krogerus wrote: >>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: >>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus >>>> <heikki.krogerus@linux.intel.com> wrote: >>>>> >>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: >>>>>> Hi Heikki, >>>>>> >>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus >>>>>> <heikki.krogerus@linux.intel.com> wrote: >>>>>>> >>>>>>> Hi Dmitry, >>>>>>> >>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: >>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so >>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP. >>>>>>> >>>>>>> That's not true. The dev->fwnode is assigned when the device is >>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode >>>>>>> member is assigned before the device is registered, then that fwnode >>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion(). >>>>>>> >>>>>>> But please note that even if drm_connector does not have anything in >>>>>>> its fwnode member, the device may still be assigned fwnode, just based >>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?). >>>>>>> >>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it >>>>>>>> breaks drivers already using components (as it was pointed at [1]), >>>>>>>> resulting in a deadlock. Lockdep trace is provided below. >>>>>>>> >>>>>>>> Granted these two issues, it seems impractical to fix this commit in any >>>>>>>> sane way. Revert it instead. >>>>>>> >>>>>>> I think there is already user space stuff that relies on these links, >>>>>>> so I'm not sure you can just remove them like that. If the component >>>>>>> framework is not the correct tool here, then I think you need to >>>>>>> suggest some other way of creating them. >>>>>> >>>>>> The issue (that was pointed out during review) is that having a >>>>>> component code in the framework code can lead to lockups. With the >>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode >>>>>> for non-ACPI systems) probing of drivers which use components and set >>>>>> drm_connector::fwnode breaks immediately. >>>>>> >>>>>> Can we move the component part to the respective drivers? With the >>>>>> patch 2 in place, connector->fwnode will be copied to the created >>>>>> kdev's fwnode pointer. >>>>>> >>>>>> Another option might be to make this drm_sysfs component registration optional. >>>>> >>>>> You don't need to use the component framework at all if there is >>>>> a better way of determining the connection between the DP and its >>>>> Type-C connector (I'm assuming that that's what this series is about). >>>>> You just need the symlinks, not the component. >>>> >>>> The problem is that right now this component registration has become >>>> mandatory. And if I set the kdev->fwnode manually (like in the patch >>>> 2), the kernel hangs inside the component code. >>>> That's why I proposed to move the components to the place where they >>>> are really necessary, e.g. i915 and amd drivers. >>> >>> So why can't we replace the component with the method you are >>> proposing in this series of finding out the Type-C port also with >>> i915, AMD, or whatever driver and platform (that's the only thing that >>> component is used for)? >> >> The drm/msm driver uses drm_bridge for the pipeline (including the last DP >> entry) and the drm_bridge_connector to create the connector. I think that >> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this >> series. >> >> >>> Determining the connection between a DP and its Type-C connector is >>> starting to get really important, so ideally we have a common solution >>> for that. >> >> Yes. This is what we have been discussing with Simon for quite some time on >> #dri-devel. >> >> Unfortunately I think the solution that got merged was pretty much hastened >> in instead of being well-thought. For example, it is also not always >> possible to provide the drm_connector / typec_connector links (as you can >> see from the patch7. Sometimes we can only express that this is a Type-C DP >> connector, but we can not easily point it to the particular USB-C port. >> >> So, I'm not sure, how can we proceed here. Currently merged patch breaks >> drm/msm if we even try to use it by setting kdef->fwnode to >> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is >> an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > You really have to always supply not only the Type-C ports and partners, > but also the alt modes. You need them, firstly to keep things sane > inside kernel, but more importantly, so they are always exposed to the > user space, AND, always the same way. We have ABIs for all this stuff, > including the DP alt mode. Use them. No shortcuts. > > So here's what you need to do. UCSI does not seem to bring you > anything useful, so just disable it for now. You don't need it. Your > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > that's where you need to register all these components - the ports, > partners and alt modes. You have all the needed information there. To make things even more complicate, UCSI is necessary for the USB part of the story. It handles vbus and direction. > Only after you've done that we can start to look at how should the > connection between the DPs and their USB Type-C connectors be handled. But sure enough, I can add typec port registration to the altmode driver. This will solve the 'port not existing' part of the story. I'd like to hear your opinion on: - components. Using them breaks drm/msm. How can we proceed? - PATH property usage. This way we make USB-C DisplayPort behave like the MST ports.
On Mon, Sep 11, 2023 at 2:15 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > >> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > >> <heikki.krogerus@linux.intel.com> wrote: > >>> > >>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > >>>> Hi Heikki, > >>>> > >>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > >>>> <heikki.krogerus@linux.intel.com> wrote: > >>>>> > >>>>> Hi Dmitry, > >>>>> > >>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > >>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > >>>>>> dev_fwnode() checks never succeed, making the respective commit NOP. > >>>>> > >>>>> That's not true. The dev->fwnode is assigned when the device is > >>>>> created on ACPI platforms automatically. If the drm_connector fwnode > >>>>> member is assigned before the device is registered, then that fwnode > >>>>> is assigned also to the device - see drm_connector_acpi_find_companion(). > >>>>> > >>>>> But please note that even if drm_connector does not have anything in > >>>>> its fwnode member, the device may still be assigned fwnode, just based > >>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?). > >>>>> > >>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > >>>>>> breaks drivers already using components (as it was pointed at [1]), > >>>>>> resulting in a deadlock. Lockdep trace is provided below. > >>>>>> > >>>>>> Granted these two issues, it seems impractical to fix this commit in any > >>>>>> sane way. Revert it instead. > >>>>> > >>>>> I think there is already user space stuff that relies on these links, > >>>>> so I'm not sure you can just remove them like that. If the component > >>>>> framework is not the correct tool here, then I think you need to > >>>>> suggest some other way of creating them. > >>>> > >>>> The issue (that was pointed out during review) is that having a > >>>> component code in the framework code can lead to lockups. With the > >>>> patch #2 in place (which is the only logical way to set kdev->fwnode > >>>> for non-ACPI systems) probing of drivers which use components and set > >>>> drm_connector::fwnode breaks immediately. > >>>> > >>>> Can we move the component part to the respective drivers? With the > >>>> patch 2 in place, connector->fwnode will be copied to the created > >>>> kdev's fwnode pointer. > >>>> > >>>> Another option might be to make this drm_sysfs component registration optional. > >>> > >>> You don't need to use the component framework at all if there is > >>> a better way of determining the connection between the DP and its > >>> Type-C connector (I'm assuming that that's what this series is about). > >>> You just need the symlinks, not the component. > >> > >> The problem is that right now this component registration has become > >> mandatory. And if I set the kdev->fwnode manually (like in the patch > >> 2), the kernel hangs inside the component code. > >> That's why I proposed to move the components to the place where they > >> are really necessary, e.g. i915 and amd drivers. > > > > So why can't we replace the component with the method you are > > proposing in this series of finding out the Type-C port also with > > i915, AMD, or whatever driver and platform (that's the only thing that > > component is used for)? > > The drm/msm driver uses drm_bridge for the pipeline (including the last > DP entry) and the drm_bridge_connector to create the connector. I think > that enabling i915 and AMD drivers to use drm_bridge fells out of scope > for this series. > > > > Determining the connection between a DP and its Type-C connector is > > starting to get really important, so ideally we have a common solution > > for that. > > Yes. This is what we have been discussing with Simon for quite some time > on #dri-devel. > > Unfortunately I think the solution that got merged was pretty much > hastened in instead of being well-thought. For example, it is also not > always possible to provide the drm_connector / typec_connector links (as > you can see from the patch7. Sometimes we can only express that this is > a Type-C DP connector, but we can not easily point it to the particular > USB-C port. > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > drm/msm if we even try to use it by setting kdef->fwnode to > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` > is an ACPI-only thing, which is not expected to work in a non-ACPI cases. In these cases we revert and try again next cycle BR, -R > > -- > With best wishes > Dmitry >
Hi Dmitry, On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > On 12/09/2023 14:05, Heikki Krogerus wrote: > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > Hi Heikki, > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > a better way of determining the connection between the DP and its > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > You just need the symlinks, not the component. > > > > > > > > > > The problem is that right now this component registration has become > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > 2), the kernel hangs inside the component code. > > > > > That's why I proposed to move the components to the place where they > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > So why can't we replace the component with the method you are > > > > proposing in this series of finding out the Type-C port also with > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > component is used for)? > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > entry) and the drm_bridge_connector to create the connector. I think that > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > series. > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > starting to get really important, so ideally we have a common solution > > > > for that. > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > #dri-devel. > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > in instead of being well-thought. For example, it is also not always > > > possible to provide the drm_connector / typec_connector links (as you can > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > You really have to always supply not only the Type-C ports and partners, > > but also the alt modes. You need them, firstly to keep things sane > > inside kernel, but more importantly, so they are always exposed to the > > user space, AND, always the same way. We have ABIs for all this stuff, > > including the DP alt mode. Use them. No shortcuts. > > > > So here's what you need to do. UCSI does not seem to bring you > > anything useful, so just disable it for now. You don't need it. Your > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > that's where you need to register all these components - the ports, > > partners and alt modes. You have all the needed information there. > > To make things even more complicate, UCSI is necessary for the USB part of > the story. It handles vbus and direction. > > > Only after you've done that we can start to look at how should the > > connection between the DPs and their USB Type-C connectors be handled. > > But sure enough, I can add typec port registration to the altmode driver. > This will solve the 'port not existing' part of the story. > > I'd like to hear your opinion on: > > - components. Using them breaks drm/msm. How can we proceed? I don't think replacing the components is going to be a problem once you have described everything properly in you DT. I'm fairly certain now that that is the main problem here. You don't have this connection described in your DT as it should. > - PATH property usage. This way we make USB-C DisplayPort behave like the > MST ports. That looks to me like an attempt to exploit a feature that is not designed for this purposes at all. Just drop all that. The connection has to be first described in your DT, and the way you usually describe connections in DT is by using the device graph (OF graph). It seems that you have everything needed for that - the USB Type-C connectors have their own OF nodes (what you register as drm_bridges are in fact USB Type-C connectors), and presumable you also have OF nodes for all your video ports (DisplayPorts) - so applying the graph between the two really should not be a problem. The DP is endpoint for the USB Type-C connector, and vice versa. After you have everything needed in your DT, the problem here isn't actually much of a problem at all. We will have options how to move forward after that. Br,
On 12/09/2023 19:39, Dmitry Baryshkov wrote: > On 12/09/2023 14:05, Heikki Krogerus wrote: >> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: >>> On 06/09/2023 16:38, Heikki Krogerus wrote: >>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: >>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus >>>>> <heikki.krogerus@linux.intel.com> wrote: >>>>>> >>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: >>>>>>> Hi Heikki, >>>>>>> >>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus >>>>>>> <heikki.krogerus@linux.intel.com> wrote: >>>>>>>> >>>>>>>> Hi Dmitry, >>>>>>>> >>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: >>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so >>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP. >>>>>>>> >>>>>>>> That's not true. The dev->fwnode is assigned when the device is >>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode >>>>>>>> member is assigned before the device is registered, then that fwnode >>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion(). >>>>>>>> >>>>>>>> But please note that even if drm_connector does not have anything in >>>>>>>> its fwnode member, the device may still be assigned fwnode, just based >>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?). >>>>>>>> >>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it >>>>>>>>> breaks drivers already using components (as it was pointed at [1]), >>>>>>>>> resulting in a deadlock. Lockdep trace is provided below. >>>>>>>>> >>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any >>>>>>>>> sane way. Revert it instead. >>>>>>>> >>>>>>>> I think there is already user space stuff that relies on these links, >>>>>>>> so I'm not sure you can just remove them like that. If the component >>>>>>>> framework is not the correct tool here, then I think you need to >>>>>>>> suggest some other way of creating them. >>>>>>> >>>>>>> The issue (that was pointed out during review) is that having a >>>>>>> component code in the framework code can lead to lockups. With the >>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode >>>>>>> for non-ACPI systems) probing of drivers which use components and set >>>>>>> drm_connector::fwnode breaks immediately. >>>>>>> >>>>>>> Can we move the component part to the respective drivers? With the >>>>>>> patch 2 in place, connector->fwnode will be copied to the created >>>>>>> kdev's fwnode pointer. >>>>>>> >>>>>>> Another option might be to make this drm_sysfs component registration optional. >>>>>> >>>>>> You don't need to use the component framework at all if there is >>>>>> a better way of determining the connection between the DP and its >>>>>> Type-C connector (I'm assuming that that's what this series is about). >>>>>> You just need the symlinks, not the component. >>>>> >>>>> The problem is that right now this component registration has become >>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch >>>>> 2), the kernel hangs inside the component code. >>>>> That's why I proposed to move the components to the place where they >>>>> are really necessary, e.g. i915 and amd drivers. >>>> >>>> So why can't we replace the component with the method you are >>>> proposing in this series of finding out the Type-C port also with >>>> i915, AMD, or whatever driver and platform (that's the only thing that >>>> component is used for)? >>> >>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP >>> entry) and the drm_bridge_connector to create the connector. I think that >>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this >>> series. >>> >>> >>>> Determining the connection between a DP and its Type-C connector is >>>> starting to get really important, so ideally we have a common solution >>>> for that. >>> >>> Yes. This is what we have been discussing with Simon for quite some time on >>> #dri-devel. >>> >>> Unfortunately I think the solution that got merged was pretty much hastened >>> in instead of being well-thought. For example, it is also not always >>> possible to provide the drm_connector / typec_connector links (as you can >>> see from the patch7. Sometimes we can only express that this is a Type-C DP >>> connector, but we can not easily point it to the particular USB-C port. >>> >>> So, I'm not sure, how can we proceed here. Currently merged patch breaks >>> drm/msm if we even try to use it by setting kdef->fwnode to >>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is >>> an ACPI-only thing, which is not expected to work in a non-ACPI cases. >> >> You really have to always supply not only the Type-C ports and partners, >> but also the alt modes. You need them, firstly to keep things sane >> inside kernel, but more importantly, so they are always exposed to the >> user space, AND, always the same way. We have ABIs for all this stuff, >> including the DP alt mode. Use them. No shortcuts. >> >> So here's what you need to do. UCSI does not seem to bring you >> anything useful, so just disable it for now. You don't need it. Your >> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so >> that's where you need to register all these components - the ports, >> partners and alt modes. You have all the needed information there. > > To make things even more complicate, UCSI is necessary for the USB part of the story. It handles vbus and direction. On new platforms (starting from SM8450) UCSI is mandatory to have pmic_glink_altmode events triggering. Neil > >> Only after you've done that we can start to look at how should the >> connection between the DPs and their USB Type-C connectors be handled. > > But sure enough, I can add typec port registration to the altmode driver. This will solve the 'port not existing' part of the story. > > I'd like to hear your opinion on: > > - components. Using them breaks drm/msm. How can we proceed? > > - PATH property usage. This way we make USB-C DisplayPort behave like the MST ports. >
Hi Heikki, On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > > a better way of determining the connection between the DP and its > > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > > You just need the symlinks, not the component. > > > > > > > > > > > > The problem is that right now this component registration has become > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > > 2), the kernel hangs inside the component code. > > > > > > That's why I proposed to move the components to the place where they > > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > > > So why can't we replace the component with the method you are > > > > > proposing in this series of finding out the Type-C port also with > > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > > component is used for)? > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > > entry) and the drm_bridge_connector to create the connector. I think that > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > > series. > > > > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > > starting to get really important, so ideally we have a common solution > > > > > for that. > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > > #dri-devel. > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > > in instead of being well-thought. For example, it is also not always > > > > possible to provide the drm_connector / typec_connector links (as you can > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > > > You really have to always supply not only the Type-C ports and partners, > > > but also the alt modes. You need them, firstly to keep things sane > > > inside kernel, but more importantly, so they are always exposed to the > > > user space, AND, always the same way. We have ABIs for all this stuff, > > > including the DP alt mode. Use them. No shortcuts. > > > > > > So here's what you need to do. UCSI does not seem to bring you > > > anything useful, so just disable it for now. You don't need it. Your > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > > that's where you need to register all these components - the ports, > > > partners and alt modes. You have all the needed information there. > > > > To make things even more complicate, UCSI is necessary for the USB part of > > the story. It handles vbus and direction. > > > > > Only after you've done that we can start to look at how should the > > > connection between the DPs and their USB Type-C connectors be handled. > > > > But sure enough, I can add typec port registration to the altmode driver. > > This will solve the 'port not existing' part of the story. > > > > I'd like to hear your opinion on: > > > > - components. Using them breaks drm/msm. How can we proceed? > > I don't think replacing the components is going to be a problem once > you have described everything properly in you DT. I'm fairly certain now > that that is the main problem here. You don't have this connection > described in your DT as it should. We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ (for non-PMIC-GLINK platform) Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full description of USB-C connector and signal flow. In fact, thanks to this representation I can properly set 'connector->fwnode' to point to the OF node corresponding to the connector's drm_bridge. I can even propagate it to the kdef->fwnode / kdev->of_node in drm_sysfs_connector_add(). But then a component_add() call looks the kernel up. And to add on top of that, here is another reason why I think that this sysfs links ABI/implementation was not well thought. The typec_connector_ops are added to all fwnode-enabled connector devices. It doesn't even bother checking that the device is really the DP connector and that the device on the other side of fwnode link is a typec port device. The symlink is named 'typec_connector', so one can not easily extend this ABI to support SlimPort aka MyDP (which uses micro-USB-B connectors instead of USB-C). Neither can we extend it to represent MHL connections (again, micro-USB-B). > > - PATH property usage. This way we make USB-C DisplayPort behave like the > > MST ports. > > That looks to me like an attempt to exploit a feature that is not > designed for this purposes at all. Just drop all that. But why? From the docs: 'Connector path property to identify how this sink is physically connected.' So far we have been using it for MST only. But the description above also suits properly for the 'connected to the Type-C port0 device' kind of data. Then the userspace can use this property to change the representation of the controller. Or to rename it as it does for DP-MST connectors. Or just add the USB-C icon in the UI. Having this data in sysfs only requires userspace first to map the connector to the device under sysfs (which is not trivial since Xorg renames DP-MST connectors), then to look for the symlink value. Quite complicated compared to checking the DRM property. Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the schema to support 'microusb:something' values for this property. > The connection has to be first described in your DT, and the way you > usually describe connections in DT is by using the device graph (OF > graph). It seems that you have everything needed for that - the USB > Type-C connectors have their own OF nodes (what you register as > drm_bridges are in fact USB Type-C connectors), and presumable you > also have OF nodes for all your video ports (DisplayPorts) - so > applying the graph between the two really should not be a problem. The > DP is endpoint for the USB Type-C connector, and vice versa. Not quite. There is no direct connection between the USB Type-C connector and DP controller. The USB-C connector has three ports. port@0 goes to theHS-USB controller. This is simple. port@1 goes to the USB+DP PHY. All retimers and SS line muxes are included in between. And it is the USB+DP PHY that is connected to the DP and USB-SS controllers. port@2 goes to SBU lines mux (e.g. fsa4480). > After you have everything needed in your DT, the problem here isn't > actually much of a problem at all. We will have options how to move > forward after that. Could you please describe what is missing there?
Hi Neil, On Wed, Sep 13, 2023 at 11:38:19AM +0200, Neil Armstrong wrote: > On new platforms (starting from SM8450) UCSI is mandatory to have > pmic_glink_altmode events triggering. You can also populate the typec devices conditionally, only if UCSI is not supported. However, I took a peek at drivers/soc/qcom/pmic_glink_altmode.c, and it seems to be mostly is dealing with the muxes and retimer, and sending the HPD notifications to the DRM side. All that is already done in typec drivers, so there is actually a potential race here when UCSI is used. On top of that, it is sending two commands to the PMIC (ALTMODE_PAN_EN and ALTMODE_PAN_ACK). I'm pretty sure both could be handled in the UCSI glue driver (drivers/usb/typec/ucsi/ucsi_glink.c) if they are even needed when UCSI is supported. So why do you need that pmic_glibk_altmode driver at all when UCSI is supported? I don't know the hardware, so I may be missing something. thanks,
On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > Hi Heikki, > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > > > a better way of determining the connection between the DP and its > > > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > > > You just need the symlinks, not the component. > > > > > > > > > > > > > > The problem is that right now this component registration has become > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > > > 2), the kernel hangs inside the component code. > > > > > > > That's why I proposed to move the components to the place where they > > > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > > > > > So why can't we replace the component with the method you are > > > > > > proposing in this series of finding out the Type-C port also with > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > > > component is used for)? > > > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > > > entry) and the drm_bridge_connector to create the connector. I think that > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > > > series. > > > > > > > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > > > starting to get really important, so ideally we have a common solution > > > > > > for that. > > > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > > > #dri-devel. > > > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > > > in instead of being well-thought. For example, it is also not always > > > > > possible to provide the drm_connector / typec_connector links (as you can > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > > > > > You really have to always supply not only the Type-C ports and partners, > > > > but also the alt modes. You need them, firstly to keep things sane > > > > inside kernel, but more importantly, so they are always exposed to the > > > > user space, AND, always the same way. We have ABIs for all this stuff, > > > > including the DP alt mode. Use them. No shortcuts. > > > > > > > > So here's what you need to do. UCSI does not seem to bring you > > > > anything useful, so just disable it for now. You don't need it. Your > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > > > that's where you need to register all these components - the ports, > > > > partners and alt modes. You have all the needed information there. > > > > > > To make things even more complicate, UCSI is necessary for the USB part of > > > the story. It handles vbus and direction. > > > > > > > Only after you've done that we can start to look at how should the > > > > connection between the DPs and their USB Type-C connectors be handled. > > > > > > But sure enough, I can add typec port registration to the altmode driver. > > > This will solve the 'port not existing' part of the story. > > > > > > I'd like to hear your opinion on: > > > > > > - components. Using them breaks drm/msm. How can we proceed? > > > > I don't think replacing the components is going to be a problem once > > you have described everything properly in you DT. I'm fairly certain now > > that that is the main problem here. You don't have this connection > > described in your DT as it should. > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ > (for non-PMIC-GLINK platform) > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full > description of USB-C connector and signal flow. > > In fact, thanks to this representation I can properly set > 'connector->fwnode' to point to the OF node corresponding to the > connector's drm_bridge. I can even propagate it to the kdef->fwnode / > kdev->of_node in drm_sysfs_connector_add(). But then a component_add() > call looks the kernel up. > > And to add on top of that, here is another reason why I think that > this sysfs links ABI/implementation was not well thought. The > typec_connector_ops are added to all fwnode-enabled connector devices. > It doesn't even bother checking that the device is really the DP > connector and that the device on the other side of fwnode link is a > typec port device. The symlink is named 'typec_connector', so one can > not easily extend this ABI to support SlimPort aka MyDP (which uses > micro-USB-B connectors instead of USB-C). Neither can we extend it to > represent MHL connections (again, micro-USB-B). > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the > > > MST ports. > > > > That looks to me like an attempt to exploit a feature that is not > > designed for this purposes at all. Just drop all that. > > But why? From the docs: 'Connector path property to identify how this > sink is physically connected.' > > So far we have been using it for MST only. But the description above > also suits properly for the 'connected to the Type-C port0 device' > kind of data. Then the userspace can use this property to change the > representation of the controller. Or to rename it as it does for > DP-MST connectors. Or just add the USB-C icon in the UI. > > Having this data in sysfs only requires userspace first to map the > connector to the device under sysfs (which is not trivial since Xorg > renames DP-MST connectors), then to look for the symlink value. Quite > complicated compared to checking the DRM property. > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the > schema to support 'microusb:something' values for this property. > > > The connection has to be first described in your DT, and the way you > > usually describe connections in DT is by using the device graph (OF > > graph). It seems that you have everything needed for that - the USB > > Type-C connectors have their own OF nodes (what you register as > > drm_bridges are in fact USB Type-C connectors), and presumable you > > also have OF nodes for all your video ports (DisplayPorts) - so > > applying the graph between the two really should not be a problem. The > > DP is endpoint for the USB Type-C connector, and vice versa. > > Not quite. There is no direct connection between the USB Type-C > connector and DP controller. The USB-C connector has three ports. > > port@0 goes to theHS-USB controller. This is simple. > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are > included in between. And it is the USB+DP PHY that is connected to the > DP and USB-SS controllers. > > port@2 goes to SBU lines mux (e.g. fsa4480). > > > After you have everything needed in your DT, the problem here isn't > > actually much of a problem at all. We will have options how to move > > forward after that. > > Could you please describe what is missing there? We are not after the direct connections here, we are after the final endpoints. So you are missing description of the logical connection between your DP and Type-C connector. I understand that the idea is to build the graph to describe only the physical connections, but with just the physical connections you are doomed to write separate software solution for almost every single platform, even though the final endpoints are always the same (DP to Type-C). You just can not generalise the components (muxes, phys, retimers, etc.) behind USB Type-C connectors (or anything else for that matter), it's not possible. The components and their order vary on almost every single platform. On some platforms the stack of parts after the connector is also incredibly complex. Having the logical final endpoint connection described in your DT/ACPI on top of the physical connections costs very little, but at the same time it's usually the only thing that the software needs (like in this case). So, either you add one more port to your graph for the DP to Type-C connection, or, if that's not an option, then you need to describe that connection in some other way. Named references work also quite well in my experience. Br,
On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > > Hi Heikki, > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > > > > a better way of determining the connection between the DP and its > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > > > > You just need the symlinks, not the component. > > > > > > > > > > > > > > > > The problem is that right now this component registration has become > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > > > > 2), the kernel hangs inside the component code. > > > > > > > > That's why I proposed to move the components to the place where they > > > > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > > > > > > > So why can't we replace the component with the method you are > > > > > > > proposing in this series of finding out the Type-C port also with > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > > > > component is used for)? > > > > > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > > > > entry) and the drm_bridge_connector to create the connector. I think that > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > > > > series. > > > > > > > > > > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > > > > starting to get really important, so ideally we have a common solution > > > > > > > for that. > > > > > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > > > > #dri-devel. > > > > > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > > > > in instead of being well-thought. For example, it is also not always > > > > > > possible to provide the drm_connector / typec_connector links (as you can > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > > > > > > > You really have to always supply not only the Type-C ports and partners, > > > > > but also the alt modes. You need them, firstly to keep things sane > > > > > inside kernel, but more importantly, so they are always exposed to the > > > > > user space, AND, always the same way. We have ABIs for all this stuff, > > > > > including the DP alt mode. Use them. No shortcuts. > > > > > > > > > > So here's what you need to do. UCSI does not seem to bring you > > > > > anything useful, so just disable it for now. You don't need it. Your > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > > > > that's where you need to register all these components - the ports, > > > > > partners and alt modes. You have all the needed information there. > > > > > > > > To make things even more complicate, UCSI is necessary for the USB part of > > > > the story. It handles vbus and direction. > > > > > > > > > Only after you've done that we can start to look at how should the > > > > > connection between the DPs and their USB Type-C connectors be handled. > > > > > > > > But sure enough, I can add typec port registration to the altmode driver. > > > > This will solve the 'port not existing' part of the story. > > > > > > > > I'd like to hear your opinion on: > > > > > > > > - components. Using them breaks drm/msm. How can we proceed? > > > > > > I don't think replacing the components is going to be a problem once > > > you have described everything properly in you DT. I'm fairly certain now > > > that that is the main problem here. You don't have this connection > > > described in your DT as it should. > > > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ > > (for non-PMIC-GLINK platform) > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full > > description of USB-C connector and signal flow. > > > > In fact, thanks to this representation I can properly set > > 'connector->fwnode' to point to the OF node corresponding to the > > connector's drm_bridge. I can even propagate it to the kdef->fwnode / > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add() > > call looks the kernel up. > > > > And to add on top of that, here is another reason why I think that > > this sysfs links ABI/implementation was not well thought. The > > typec_connector_ops are added to all fwnode-enabled connector devices. > > It doesn't even bother checking that the device is really the DP > > connector and that the device on the other side of fwnode link is a > > typec port device. The symlink is named 'typec_connector', so one can > > not easily extend this ABI to support SlimPort aka MyDP (which uses > > micro-USB-B connectors instead of USB-C). Neither can we extend it to > > represent MHL connections (again, micro-USB-B). > > > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the > > > > MST ports. > > > > > > That looks to me like an attempt to exploit a feature that is not > > > designed for this purposes at all. Just drop all that. > > > > But why? From the docs: 'Connector path property to identify how this > > sink is physically connected.' > > > > So far we have been using it for MST only. But the description above > > also suits properly for the 'connected to the Type-C port0 device' > > kind of data. Then the userspace can use this property to change the > > representation of the controller. Or to rename it as it does for > > DP-MST connectors. Or just add the USB-C icon in the UI. > > > > Having this data in sysfs only requires userspace first to map the > > connector to the device under sysfs (which is not trivial since Xorg > > renames DP-MST connectors), then to look for the symlink value. Quite > > complicated compared to checking the DRM property. > > > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the > > schema to support 'microusb:something' values for this property. > > > > > The connection has to be first described in your DT, and the way you > > > usually describe connections in DT is by using the device graph (OF > > > graph). It seems that you have everything needed for that - the USB > > > Type-C connectors have their own OF nodes (what you register as > > > drm_bridges are in fact USB Type-C connectors), and presumable you > > > also have OF nodes for all your video ports (DisplayPorts) - so > > > applying the graph between the two really should not be a problem. The > > > DP is endpoint for the USB Type-C connector, and vice versa. > > > > Not quite. There is no direct connection between the USB Type-C > > connector and DP controller. The USB-C connector has three ports. > > > > port@0 goes to theHS-USB controller. This is simple. > > > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are > > included in between. And it is the USB+DP PHY that is connected to the > > DP and USB-SS controllers. > > > > port@2 goes to SBU lines mux (e.g. fsa4480). > > > > > After you have everything needed in your DT, the problem here isn't > > > actually much of a problem at all. We will have options how to move > > > forward after that. > > > > Could you please describe what is missing there? > > We are not after the direct connections here, we are after the final > endpoints. So you are missing description of the logical connection > between your DP and Type-C connector. Adding Krzysztof, as one of DT maintainers, to the CC list.
Hi Dmitry, On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > > > Hi Heikki, > > > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > > > > > a better way of determining the connection between the DP and its > > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > > > > > You just need the symlinks, not the component. > > > > > > > > > > > > > > > > > > The problem is that right now this component registration has become > > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > > > > > 2), the kernel hangs inside the component code. > > > > > > > > > That's why I proposed to move the components to the place where they > > > > > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > > > > > > > > > So why can't we replace the component with the method you are > > > > > > > > proposing in this series of finding out the Type-C port also with > > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > > > > > component is used for)? > > > > > > > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > > > > > entry) and the drm_bridge_connector to create the connector. I think that > > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > > > > > series. > > > > > > > > > > > > > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > > > > > starting to get really important, so ideally we have a common solution > > > > > > > > for that. > > > > > > > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > > > > > #dri-devel. > > > > > > > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > > > > > in instead of being well-thought. For example, it is also not always > > > > > > > possible to provide the drm_connector / typec_connector links (as you can > > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > > > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > > > > > > > > > You really have to always supply not only the Type-C ports and partners, > > > > > > but also the alt modes. You need them, firstly to keep things sane > > > > > > inside kernel, but more importantly, so they are always exposed to the > > > > > > user space, AND, always the same way. We have ABIs for all this stuff, > > > > > > including the DP alt mode. Use them. No shortcuts. > > > > > > > > > > > > So here's what you need to do. UCSI does not seem to bring you > > > > > > anything useful, so just disable it for now. You don't need it. Your > > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > > > > > that's where you need to register all these components - the ports, > > > > > > partners and alt modes. You have all the needed information there. > > > > > > > > > > To make things even more complicate, UCSI is necessary for the USB part of > > > > > the story. It handles vbus and direction. > > > > > > > > > > > Only after you've done that we can start to look at how should the > > > > > > connection between the DPs and their USB Type-C connectors be handled. > > > > > > > > > > But sure enough, I can add typec port registration to the altmode driver. > > > > > This will solve the 'port not existing' part of the story. > > > > > > > > > > I'd like to hear your opinion on: > > > > > > > > > > - components. Using them breaks drm/msm. How can we proceed? > > > > > > > > I don't think replacing the components is going to be a problem once > > > > you have described everything properly in you DT. I'm fairly certain now > > > > that that is the main problem here. You don't have this connection > > > > described in your DT as it should. > > > > > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ > > > (for non-PMIC-GLINK platform) > > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full > > > description of USB-C connector and signal flow. > > > > > > In fact, thanks to this representation I can properly set > > > 'connector->fwnode' to point to the OF node corresponding to the > > > connector's drm_bridge. I can even propagate it to the kdef->fwnode / > > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add() > > > call looks the kernel up. > > > > > > And to add on top of that, here is another reason why I think that > > > this sysfs links ABI/implementation was not well thought. The > > > typec_connector_ops are added to all fwnode-enabled connector devices. > > > It doesn't even bother checking that the device is really the DP > > > connector and that the device on the other side of fwnode link is a > > > typec port device. The symlink is named 'typec_connector', so one can > > > not easily extend this ABI to support SlimPort aka MyDP (which uses > > > micro-USB-B connectors instead of USB-C). Neither can we extend it to > > > represent MHL connections (again, micro-USB-B). > > > > > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the > > > > > MST ports. > > > > > > > > That looks to me like an attempt to exploit a feature that is not > > > > designed for this purposes at all. Just drop all that. > > > > > > But why? From the docs: 'Connector path property to identify how this > > > sink is physically connected.' > > > > > > So far we have been using it for MST only. But the description above > > > also suits properly for the 'connected to the Type-C port0 device' > > > kind of data. Then the userspace can use this property to change the > > > representation of the controller. Or to rename it as it does for > > > DP-MST connectors. Or just add the USB-C icon in the UI. > > > > > > Having this data in sysfs only requires userspace first to map the > > > connector to the device under sysfs (which is not trivial since Xorg > > > renames DP-MST connectors), then to look for the symlink value. Quite > > > complicated compared to checking the DRM property. > > > > > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the > > > schema to support 'microusb:something' values for this property. > > > > > > > The connection has to be first described in your DT, and the way you > > > > usually describe connections in DT is by using the device graph (OF > > > > graph). It seems that you have everything needed for that - the USB > > > > Type-C connectors have their own OF nodes (what you register as > > > > drm_bridges are in fact USB Type-C connectors), and presumable you > > > > also have OF nodes for all your video ports (DisplayPorts) - so > > > > applying the graph between the two really should not be a problem. The > > > > DP is endpoint for the USB Type-C connector, and vice versa. > > > > > > Not quite. There is no direct connection between the USB Type-C > > > connector and DP controller. The USB-C connector has three ports. > > > > > > port@0 goes to theHS-USB controller. This is simple. > > > > > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are > > > included in between. And it is the USB+DP PHY that is connected to the > > > DP and USB-SS controllers. > > > > > > port@2 goes to SBU lines mux (e.g. fsa4480). > > > > > > > After you have everything needed in your DT, the problem here isn't > > > > actually much of a problem at all. We will have options how to move > > > > forward after that. > > > > > > Could you please describe what is missing there? > > > > We are not after the direct connections here, we are after the final > > endpoints. So you are missing description of the logical connection > > between your DP and Type-C connector. > > Adding Krzysztof, as one of DT maintainers, to the CC list. > > >From what I understand, DT describes hardware. There is no description > for the 'logical' connections. > > > > > I understand that the idea is to build the graph to describe only the > > physical connections, but with just the physical connections you are > > doomed to write separate software solution for almost every single > > platform, even though the final endpoints are always the same (DP to > > Type-C). You just can not generalise the components (muxes, phys, > > retimers, etc.) behind USB Type-C connectors (or anything else for > > that matter), it's not possible. The components and their order vary > > on almost every single platform. On some platforms the stack of parts > > after the connector is also incredibly complex. > > Yes. And this is why we have a chain of DRM bridges, going from the DP > controller to the final drm_bridge at the Type-C port manager. When > there is the altmode event, it gets sent via this chain using the > normal DRM HPD event. We will not have drm bridges between the thunderbolt controller and the connector, but you still need to be able to show the connector to the user when DisplayPort is tunneled over thunderbolt. DP alt mode is only one way of getting DisplayPort through USB Type-C. You just can't make any assumptions with USB Type-C. The drm bridge chain could only solve the port/connector relationship problem from a single angle, but we need a common solution. The problem is after all completely generic. It is not DisplayPort specific or even USB Type-C specific problem. Those are just two of the many possible last endpoints for these connections that need to be aware of each other. So we really have to have a common way of getting this straight from the hardware description somehow. To me the obvious solution would be to just have a port in the graph that points directly the last endpoint regardless of what you have in between. But if that's not an option, then so be it. Then there just needs to be some other way of getting that information from DT. Maybe DT could use similar physical location object/attribute like ACPI - the DP would have matching physical location with its connector? > > Having the logical final endpoint connection described in your DT/ACPI > > on top of the physical connections costs very little, but at the same > > time it's usually the only thing that the software needs (like in this > > case). > > Maybe there is some misunderstanding here. We have this connection. We > have connector->fwnode and connector->of_node pointing to the correct > device - the last bridge in the chain. Each TCPM driver knows the > relationship between the in-built drm_bridge and the Type-C port. The > DP host controller port can be terminated with other endpoints, e.g. > eDP panel. Or there can be a non-DP host, which is then connected > through a series of bridges to the eDP or external DP port. This is > what anx78xx bridge does: it converts the HDMI link into an external > DP (SlimPort) connection. Bridge chains permit this to be handled in a > seamless way. > > What you are asking for looks like a step backwards to me: it requires > the host to know that there is a USB-C connector. > > > So, either you add one more port to your graph for the DP to Type-C > > connection, or, if that's not an option, then you need to describe > > that connection in some other way. Named references work also quite > > well in my experience. > > Named references were considered and frowned upon by DT maintainers. thanks,
On 14/09/2023 11:26, Heikki Krogerus wrote: > Hi Dmitry, > > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: >> On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus >> <heikki.krogerus@linux.intel.com> wrote: >>> >>> On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: >>>> Hi Heikki, >>>> >>>> On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus >>>> <heikki.krogerus@linux.intel.com> wrote: >>>>> On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: >>>>>> On 12/09/2023 14:05, Heikki Krogerus wrote: >>>>>>> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: >>>>>>>> On 06/09/2023 16:38, Heikki Krogerus wrote: >>>>>>>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: >>>>>>>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus >>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote: >>>>>>>>>>> >>>>>>>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: >>>>>>>>>>>> Hi Heikki, >>>>>>>>>>>> >>>>>>>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus >>>>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Dmitry, >>>>>>>>>>>>> >>>>>>>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: >>>>>>>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so >>>>>>>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP. >>>>>>>>>>>>> >>>>>>>>>>>>> That's not true. The dev->fwnode is assigned when the device is >>>>>>>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode >>>>>>>>>>>>> member is assigned before the device is registered, then that fwnode >>>>>>>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion(). >>>>>>>>>>>>> >>>>>>>>>>>>> But please note that even if drm_connector does not have anything in >>>>>>>>>>>>> its fwnode member, the device may still be assigned fwnode, just based >>>>>>>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?). >>>>>>>>>>>>> >>>>>>>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it >>>>>>>>>>>>>> breaks drivers already using components (as it was pointed at [1]), >>>>>>>>>>>>>> resulting in a deadlock. Lockdep trace is provided below. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any >>>>>>>>>>>>>> sane way. Revert it instead. >>>>>>>>>>>>> >>>>>>>>>>>>> I think there is already user space stuff that relies on these links, >>>>>>>>>>>>> so I'm not sure you can just remove them like that. If the component >>>>>>>>>>>>> framework is not the correct tool here, then I think you need to >>>>>>>>>>>>> suggest some other way of creating them. >>>>>>>>>>>> >>>>>>>>>>>> The issue (that was pointed out during review) is that having a >>>>>>>>>>>> component code in the framework code can lead to lockups. With the >>>>>>>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode >>>>>>>>>>>> for non-ACPI systems) probing of drivers which use components and set >>>>>>>>>>>> drm_connector::fwnode breaks immediately. >>>>>>>>>>>> >>>>>>>>>>>> Can we move the component part to the respective drivers? With the >>>>>>>>>>>> patch 2 in place, connector->fwnode will be copied to the created >>>>>>>>>>>> kdev's fwnode pointer. >>>>>>>>>>>> >>>>>>>>>>>> Another option might be to make this drm_sysfs component registration optional. >>>>>>>>>>> >>>>>>>>>>> You don't need to use the component framework at all if there is >>>>>>>>>>> a better way of determining the connection between the DP and its >>>>>>>>>>> Type-C connector (I'm assuming that that's what this series is about). >>>>>>>>>>> You just need the symlinks, not the component. >>>>>>>>>> >>>>>>>>>> The problem is that right now this component registration has become >>>>>>>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch >>>>>>>>>> 2), the kernel hangs inside the component code. >>>>>>>>>> That's why I proposed to move the components to the place where they >>>>>>>>>> are really necessary, e.g. i915 and amd drivers. >>>>>>>>> >>>>>>>>> So why can't we replace the component with the method you are >>>>>>>>> proposing in this series of finding out the Type-C port also with >>>>>>>>> i915, AMD, or whatever driver and platform (that's the only thing that >>>>>>>>> component is used for)? >>>>>>>> >>>>>>>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP >>>>>>>> entry) and the drm_bridge_connector to create the connector. I think that >>>>>>>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this >>>>>>>> series. >>>>>>>> >>>>>>>> >>>>>>>>> Determining the connection between a DP and its Type-C connector is >>>>>>>>> starting to get really important, so ideally we have a common solution >>>>>>>>> for that. >>>>>>>> >>>>>>>> Yes. This is what we have been discussing with Simon for quite some time on >>>>>>>> #dri-devel. >>>>>>>> >>>>>>>> Unfortunately I think the solution that got merged was pretty much hastened >>>>>>>> in instead of being well-thought. For example, it is also not always >>>>>>>> possible to provide the drm_connector / typec_connector links (as you can >>>>>>>> see from the patch7. Sometimes we can only express that this is a Type-C DP >>>>>>>> connector, but we can not easily point it to the particular USB-C port. >>>>>>>> >>>>>>>> So, I'm not sure, how can we proceed here. Currently merged patch breaks >>>>>>>> drm/msm if we even try to use it by setting kdef->fwnode to >>>>>>>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is >>>>>>>> an ACPI-only thing, which is not expected to work in a non-ACPI cases. >>>>>>> >>>>>>> You really have to always supply not only the Type-C ports and partners, >>>>>>> but also the alt modes. You need them, firstly to keep things sane >>>>>>> inside kernel, but more importantly, so they are always exposed to the >>>>>>> user space, AND, always the same way. We have ABIs for all this stuff, >>>>>>> including the DP alt mode. Use them. No shortcuts. >>>>>>> >>>>>>> So here's what you need to do. UCSI does not seem to bring you >>>>>>> anything useful, so just disable it for now. You don't need it. Your >>>>>>> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so >>>>>>> that's where you need to register all these components - the ports, >>>>>>> partners and alt modes. You have all the needed information there. >>>>>> >>>>>> To make things even more complicate, UCSI is necessary for the USB part of >>>>>> the story. It handles vbus and direction. >>>>>> >>>>>>> Only after you've done that we can start to look at how should the >>>>>>> connection between the DPs and their USB Type-C connectors be handled. >>>>>> >>>>>> But sure enough, I can add typec port registration to the altmode driver. >>>>>> This will solve the 'port not existing' part of the story. >>>>>> >>>>>> I'd like to hear your opinion on: >>>>>> >>>>>> - components. Using them breaks drm/msm. How can we proceed? >>>>> >>>>> I don't think replacing the components is going to be a problem once >>>>> you have described everything properly in you DT. I'm fairly certain now >>>>> that that is the main problem here. You don't have this connection >>>>> described in your DT as it should. >>>> >>>> We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ >>>> (for non-PMIC-GLINK platform) >>>> Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full >>>> description of USB-C connector and signal flow. >>>> >>>> In fact, thanks to this representation I can properly set >>>> 'connector->fwnode' to point to the OF node corresponding to the >>>> connector's drm_bridge. I can even propagate it to the kdef->fwnode / >>>> kdev->of_node in drm_sysfs_connector_add(). But then a component_add() >>>> call looks the kernel up. >>>> >>>> And to add on top of that, here is another reason why I think that >>>> this sysfs links ABI/implementation was not well thought. The >>>> typec_connector_ops are added to all fwnode-enabled connector devices. >>>> It doesn't even bother checking that the device is really the DP >>>> connector and that the device on the other side of fwnode link is a >>>> typec port device. The symlink is named 'typec_connector', so one can >>>> not easily extend this ABI to support SlimPort aka MyDP (which uses >>>> micro-USB-B connectors instead of USB-C). Neither can we extend it to >>>> represent MHL connections (again, micro-USB-B). >>>> >>>>>> - PATH property usage. This way we make USB-C DisplayPort behave like the >>>>>> MST ports. >>>>> >>>>> That looks to me like an attempt to exploit a feature that is not >>>>> designed for this purposes at all. Just drop all that. >>>> >>>> But why? From the docs: 'Connector path property to identify how this >>>> sink is physically connected.' >>>> >>>> So far we have been using it for MST only. But the description above >>>> also suits properly for the 'connected to the Type-C port0 device' >>>> kind of data. Then the userspace can use this property to change the >>>> representation of the controller. Or to rename it as it does for >>>> DP-MST connectors. Or just add the USB-C icon in the UI. >>>> >>>> Having this data in sysfs only requires userspace first to map the >>>> connector to the device under sysfs (which is not trivial since Xorg >>>> renames DP-MST connectors), then to look for the symlink value. Quite >>>> complicated compared to checking the DRM property. >>>> >>>> Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the >>>> schema to support 'microusb:something' values for this property. >>>> >>>>> The connection has to be first described in your DT, and the way you >>>>> usually describe connections in DT is by using the device graph (OF >>>>> graph). It seems that you have everything needed for that - the USB >>>>> Type-C connectors have their own OF nodes (what you register as >>>>> drm_bridges are in fact USB Type-C connectors), and presumable you >>>>> also have OF nodes for all your video ports (DisplayPorts) - so >>>>> applying the graph between the two really should not be a problem. The >>>>> DP is endpoint for the USB Type-C connector, and vice versa. >>>> >>>> Not quite. There is no direct connection between the USB Type-C >>>> connector and DP controller. The USB-C connector has three ports. >>>> >>>> port@0 goes to theHS-USB controller. This is simple. >>>> >>>> port@1 goes to the USB+DP PHY. All retimers and SS line muxes are >>>> included in between. And it is the USB+DP PHY that is connected to the >>>> DP and USB-SS controllers. >>>> >>>> port@2 goes to SBU lines mux (e.g. fsa4480). >>>> >>>>> After you have everything needed in your DT, the problem here isn't >>>>> actually much of a problem at all. We will have options how to move >>>>> forward after that. >>>> >>>> Could you please describe what is missing there? >>> >>> We are not after the direct connections here, we are after the final >>> endpoints. So you are missing description of the logical connection >>> between your DP and Type-C connector. >> >> Adding Krzysztof, as one of DT maintainers, to the CC list. >> >> >From what I understand, DT describes hardware. There is no description >> for the 'logical' connections. >> >>> >>> I understand that the idea is to build the graph to describe only the >>> physical connections, but with just the physical connections you are >>> doomed to write separate software solution for almost every single >>> platform, even though the final endpoints are always the same (DP to >>> Type-C). You just can not generalise the components (muxes, phys, >>> retimers, etc.) behind USB Type-C connectors (or anything else for >>> that matter), it's not possible. The components and their order vary >>> on almost every single platform. On some platforms the stack of parts >>> after the connector is also incredibly complex. >> >> Yes. And this is why we have a chain of DRM bridges, going from the DP >> controller to the final drm_bridge at the Type-C port manager. When >> there is the altmode event, it gets sent via this chain using the >> normal DRM HPD event. > > We will not have drm bridges between the thunderbolt controller and > the connector, but you still need to be able to show the connector to > the user when DisplayPort is tunneled over thunderbolt. DP alt mode is > only one way of getting DisplayPort through USB Type-C. You just can't > make any assumptions with USB Type-C. > > The drm bridge chain could only solve the port/connector relationship > problem from a single angle, but we need a common solution. The > problem is after all completely generic. It is not DisplayPort > specific or even USB Type-C specific problem. Those are just two of > the many possible last endpoints for these connections that need to be > aware of each other. > > So we really have to have a common way of getting this straight from > the hardware description somehow. > > To me the obvious solution would be to just have a port in the graph > that points directly the last endpoint regardless of what you have in > between. But if that's not an option, then so be it. Then there just > needs to be some other way of getting that information from DT. The DT must describe the HW interconnection, this is what we do, but we're allowed to parse it as we want and ignore the bridges/endpoint between the connector node and the display node, all this is implementation. We added intermediate bridge because they are part the displayport signal chain, and they may need to react to the display enable/disable/check if there's some limitations or init sequence in the retimer for example. Neil > > Maybe DT could use similar physical location object/attribute like > ACPI - the DP would have matching physical location with its connector? > >>> Having the logical final endpoint connection described in your DT/ACPI >>> on top of the physical connections costs very little, but at the same >>> time it's usually the only thing that the software needs (like in this >>> case). >> >> Maybe there is some misunderstanding here. We have this connection. We >> have connector->fwnode and connector->of_node pointing to the correct >> device - the last bridge in the chain. Each TCPM driver knows the >> relationship between the in-built drm_bridge and the Type-C port. The >> DP host controller port can be terminated with other endpoints, e.g. >> eDP panel. Or there can be a non-DP host, which is then connected >> through a series of bridges to the eDP or external DP port. This is >> what anx78xx bridge does: it converts the HDMI link into an external >> DP (SlimPort) connection. Bridge chains permit this to be handled in a >> seamless way. >> >> What you are asking for looks like a step backwards to me: it requires >> the host to know that there is a USB-C connector. >> >>> So, either you add one more port to your graph for the DP to Type-C >>> connection, or, if that's not an option, then you need to describe >>> that connection in some other way. Named references work also quite >>> well in my experience. >> >> Named references were considered and frowned upon by DT maintainers. > > thanks, >
On Thu, 14 Sept 2023 at 12:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 14/09/2023 11:26, Heikki Krogerus wrote: > > Hi Dmitry, > > > > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: > >> On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus > >> <heikki.krogerus@linux.intel.com> wrote: > >>> > >>> On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > >>>> Hi Heikki, > >>>> > >>>> On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > >>>> <heikki.krogerus@linux.intel.com> wrote: > >>>>> On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > >>>>>> On 12/09/2023 14:05, Heikki Krogerus wrote: > >>>>>>> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > >>>>>>>> On 06/09/2023 16:38, Heikki Krogerus wrote: > >>>>>>>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > >>>>>>>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > >>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote: > >>>>>>>>>>> > >>>>>>>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > >>>>>>>>>>>> Hi Heikki, > >>>>>>>>>>>> > >>>>>>>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > >>>>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>> Hi Dmitry, > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > >>>>>>>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > >>>>>>>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP. > >>>>>>>>>>>>> > >>>>>>>>>>>>> That's not true. The dev->fwnode is assigned when the device is > >>>>>>>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode > >>>>>>>>>>>>> member is assigned before the device is registered, then that fwnode > >>>>>>>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion(). > >>>>>>>>>>>>> > >>>>>>>>>>>>> But please note that even if drm_connector does not have anything in > >>>>>>>>>>>>> its fwnode member, the device may still be assigned fwnode, just based > >>>>>>>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?). > >>>>>>>>>>>>> > >>>>>>>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > >>>>>>>>>>>>>> breaks drivers already using components (as it was pointed at [1]), > >>>>>>>>>>>>>> resulting in a deadlock. Lockdep trace is provided below. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any > >>>>>>>>>>>>>> sane way. Revert it instead. > >>>>>>>>>>>>> > >>>>>>>>>>>>> I think there is already user space stuff that relies on these links, > >>>>>>>>>>>>> so I'm not sure you can just remove them like that. If the component > >>>>>>>>>>>>> framework is not the correct tool here, then I think you need to > >>>>>>>>>>>>> suggest some other way of creating them. > >>>>>>>>>>>> > >>>>>>>>>>>> The issue (that was pointed out during review) is that having a > >>>>>>>>>>>> component code in the framework code can lead to lockups. With the > >>>>>>>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode > >>>>>>>>>>>> for non-ACPI systems) probing of drivers which use components and set > >>>>>>>>>>>> drm_connector::fwnode breaks immediately. > >>>>>>>>>>>> > >>>>>>>>>>>> Can we move the component part to the respective drivers? With the > >>>>>>>>>>>> patch 2 in place, connector->fwnode will be copied to the created > >>>>>>>>>>>> kdev's fwnode pointer. > >>>>>>>>>>>> > >>>>>>>>>>>> Another option might be to make this drm_sysfs component registration optional. > >>>>>>>>>>> > >>>>>>>>>>> You don't need to use the component framework at all if there is > >>>>>>>>>>> a better way of determining the connection between the DP and its > >>>>>>>>>>> Type-C connector (I'm assuming that that's what this series is about). > >>>>>>>>>>> You just need the symlinks, not the component. > >>>>>>>>>> > >>>>>>>>>> The problem is that right now this component registration has become > >>>>>>>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch > >>>>>>>>>> 2), the kernel hangs inside the component code. > >>>>>>>>>> That's why I proposed to move the components to the place where they > >>>>>>>>>> are really necessary, e.g. i915 and amd drivers. > >>>>>>>>> > >>>>>>>>> So why can't we replace the component with the method you are > >>>>>>>>> proposing in this series of finding out the Type-C port also with > >>>>>>>>> i915, AMD, or whatever driver and platform (that's the only thing that > >>>>>>>>> component is used for)? > >>>>>>>> > >>>>>>>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP > >>>>>>>> entry) and the drm_bridge_connector to create the connector. I think that > >>>>>>>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > >>>>>>>> series. > >>>>>>>> > >>>>>>>> > >>>>>>>>> Determining the connection between a DP and its Type-C connector is > >>>>>>>>> starting to get really important, so ideally we have a common solution > >>>>>>>>> for that. > >>>>>>>> > >>>>>>>> Yes. This is what we have been discussing with Simon for quite some time on > >>>>>>>> #dri-devel. > >>>>>>>> > >>>>>>>> Unfortunately I think the solution that got merged was pretty much hastened > >>>>>>>> in instead of being well-thought. For example, it is also not always > >>>>>>>> possible to provide the drm_connector / typec_connector links (as you can > >>>>>>>> see from the patch7. Sometimes we can only express that this is a Type-C DP > >>>>>>>> connector, but we can not easily point it to the particular USB-C port. > >>>>>>>> > >>>>>>>> So, I'm not sure, how can we proceed here. Currently merged patch breaks > >>>>>>>> drm/msm if we even try to use it by setting kdef->fwnode to > >>>>>>>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > >>>>>>>> an ACPI-only thing, which is not expected to work in a non-ACPI cases. > >>>>>>> > >>>>>>> You really have to always supply not only the Type-C ports and partners, > >>>>>>> but also the alt modes. You need them, firstly to keep things sane > >>>>>>> inside kernel, but more importantly, so they are always exposed to the > >>>>>>> user space, AND, always the same way. We have ABIs for all this stuff, > >>>>>>> including the DP alt mode. Use them. No shortcuts. > >>>>>>> > >>>>>>> So here's what you need to do. UCSI does not seem to bring you > >>>>>>> anything useful, so just disable it for now. You don't need it. Your > >>>>>>> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > >>>>>>> that's where you need to register all these components - the ports, > >>>>>>> partners and alt modes. You have all the needed information there. > >>>>>> > >>>>>> To make things even more complicate, UCSI is necessary for the USB part of > >>>>>> the story. It handles vbus and direction. > >>>>>> > >>>>>>> Only after you've done that we can start to look at how should the > >>>>>>> connection between the DPs and their USB Type-C connectors be handled. > >>>>>> > >>>>>> But sure enough, I can add typec port registration to the altmode driver. > >>>>>> This will solve the 'port not existing' part of the story. > >>>>>> > >>>>>> I'd like to hear your opinion on: > >>>>>> > >>>>>> - components. Using them breaks drm/msm. How can we proceed? > >>>>> > >>>>> I don't think replacing the components is going to be a problem once > >>>>> you have described everything properly in you DT. I'm fairly certain now > >>>>> that that is the main problem here. You don't have this connection > >>>>> described in your DT as it should. > >>>> > >>>> We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ > >>>> (for non-PMIC-GLINK platform) > >>>> Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full > >>>> description of USB-C connector and signal flow. > >>>> > >>>> In fact, thanks to this representation I can properly set > >>>> 'connector->fwnode' to point to the OF node corresponding to the > >>>> connector's drm_bridge. I can even propagate it to the kdef->fwnode / > >>>> kdev->of_node in drm_sysfs_connector_add(). But then a component_add() > >>>> call looks the kernel up. > >>>> > >>>> And to add on top of that, here is another reason why I think that > >>>> this sysfs links ABI/implementation was not well thought. The > >>>> typec_connector_ops are added to all fwnode-enabled connector devices. > >>>> It doesn't even bother checking that the device is really the DP > >>>> connector and that the device on the other side of fwnode link is a > >>>> typec port device. The symlink is named 'typec_connector', so one can > >>>> not easily extend this ABI to support SlimPort aka MyDP (which uses > >>>> micro-USB-B connectors instead of USB-C). Neither can we extend it to > >>>> represent MHL connections (again, micro-USB-B). > >>>> > >>>>>> - PATH property usage. This way we make USB-C DisplayPort behave like the > >>>>>> MST ports. > >>>>> > >>>>> That looks to me like an attempt to exploit a feature that is not > >>>>> designed for this purposes at all. Just drop all that. > >>>> > >>>> But why? From the docs: 'Connector path property to identify how this > >>>> sink is physically connected.' > >>>> > >>>> So far we have been using it for MST only. But the description above > >>>> also suits properly for the 'connected to the Type-C port0 device' > >>>> kind of data. Then the userspace can use this property to change the > >>>> representation of the controller. Or to rename it as it does for > >>>> DP-MST connectors. Or just add the USB-C icon in the UI. > >>>> > >>>> Having this data in sysfs only requires userspace first to map the > >>>> connector to the device under sysfs (which is not trivial since Xorg > >>>> renames DP-MST connectors), then to look for the symlink value. Quite > >>>> complicated compared to checking the DRM property. > >>>> > >>>> Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the > >>>> schema to support 'microusb:something' values for this property. > >>>> > >>>>> The connection has to be first described in your DT, and the way you > >>>>> usually describe connections in DT is by using the device graph (OF > >>>>> graph). It seems that you have everything needed for that - the USB > >>>>> Type-C connectors have their own OF nodes (what you register as > >>>>> drm_bridges are in fact USB Type-C connectors), and presumable you > >>>>> also have OF nodes for all your video ports (DisplayPorts) - so > >>>>> applying the graph between the two really should not be a problem. The > >>>>> DP is endpoint for the USB Type-C connector, and vice versa. > >>>> > >>>> Not quite. There is no direct connection between the USB Type-C > >>>> connector and DP controller. The USB-C connector has three ports. > >>>> > >>>> port@0 goes to theHS-USB controller. This is simple. > >>>> > >>>> port@1 goes to the USB+DP PHY. All retimers and SS line muxes are > >>>> included in between. And it is the USB+DP PHY that is connected to the > >>>> DP and USB-SS controllers. > >>>> > >>>> port@2 goes to SBU lines mux (e.g. fsa4480). > >>>> > >>>>> After you have everything needed in your DT, the problem here isn't > >>>>> actually much of a problem at all. We will have options how to move > >>>>> forward after that. > >>>> > >>>> Could you please describe what is missing there? > >>> > >>> We are not after the direct connections here, we are after the final > >>> endpoints. So you are missing description of the logical connection > >>> between your DP and Type-C connector. > >> > >> Adding Krzysztof, as one of DT maintainers, to the CC list. > >> > >> >From what I understand, DT describes hardware. There is no description > >> for the 'logical' connections. > >> > >>> > >>> I understand that the idea is to build the graph to describe only the > >>> physical connections, but with just the physical connections you are > >>> doomed to write separate software solution for almost every single > >>> platform, even though the final endpoints are always the same (DP to > >>> Type-C). You just can not generalise the components (muxes, phys, > >>> retimers, etc.) behind USB Type-C connectors (or anything else for > >>> that matter), it's not possible. The components and their order vary > >>> on almost every single platform. On some platforms the stack of parts > >>> after the connector is also incredibly complex. > >> > >> Yes. And this is why we have a chain of DRM bridges, going from the DP > >> controller to the final drm_bridge at the Type-C port manager. When > >> there is the altmode event, it gets sent via this chain using the > >> normal DRM HPD event. > > > > We will not have drm bridges between the thunderbolt controller and > > the connector, but you still need to be able to show the connector to > > the user when DisplayPort is tunneled over thunderbolt. DP alt mode is > > only one way of getting DisplayPort through USB Type-C. You just can't > > make any assumptions with USB Type-C. > > > > The drm bridge chain could only solve the port/connector relationship > > problem from a single angle, but we need a common solution. The > > problem is after all completely generic. It is not DisplayPort > > specific or even USB Type-C specific problem. Those are just two of > > the many possible last endpoints for these connections that need to be > > aware of each other. > > > > So we really have to have a common way of getting this straight from > > the hardware description somehow. > > > > To me the obvious solution would be to just have a port in the graph > > that points directly the last endpoint regardless of what you have in > > between. But if that's not an option, then so be it. Then there just > > needs to be some other way of getting that information from DT. > > The DT must describe the HW interconnection, this is what we do, but > we're allowed to parse it as we want and ignore the bridges/endpoint > between the connector node and the display node, all this is implementation. > > We added intermediate bridge because they are part the displayport signal > chain, and they may need to react to the display enable/disable/check > if there's some limitations or init sequence in the retimer for example. Moreover, this way we do not have to care about exact binding of the interim devices. They can have different connections and order ports as they do see fit. Parsing the whole chain from DP controller would require having standard bindings for all retimers / muxes / switches in the chain. > > Neil > > > > > Maybe DT could use similar physical location object/attribute like > > ACPI - the DP would have matching physical location with its connector? > > > >>> Having the logical final endpoint connection described in your DT/ACPI > >>> on top of the physical connections costs very little, but at the same > >>> time it's usually the only thing that the software needs (like in this > >>> case). > >> > >> Maybe there is some misunderstanding here. We have this connection. We > >> have connector->fwnode and connector->of_node pointing to the correct > >> device - the last bridge in the chain. Each TCPM driver knows the > >> relationship between the in-built drm_bridge and the Type-C port. The > >> DP host controller port can be terminated with other endpoints, e.g. > >> eDP panel. Or there can be a non-DP host, which is then connected > >> through a series of bridges to the eDP or external DP port. This is > >> what anx78xx bridge does: it converts the HDMI link into an external > >> DP (SlimPort) connection. Bridge chains permit this to be handled in a > >> seamless way. > >> > >> What you are asking for looks like a step backwards to me: it requires > >> the host to know that there is a USB-C connector. > >> > >>> So, either you add one more port to your graph for the DP to Type-C > >>> connection, or, if that's not an option, then you need to describe > >>> that connection in some other way. Named references work also quite > >>> well in my experience. > >> > >> Named references were considered and frowned upon by DT maintainers. > > > > thanks, > > >
On Thu, 14 Sept 2023 at 12:26, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi Dmitry, > > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: > > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > > > > Hi Heikki, > > > > > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > > > > > > a better way of determining the connection between the DP and its > > > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > > > > > > You just need the symlinks, not the component. > > > > > > > > > > > > > > > > > > > > The problem is that right now this component registration has become > > > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > > > > > > 2), the kernel hangs inside the component code. > > > > > > > > > > That's why I proposed to move the components to the place where they > > > > > > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > > > > > > > > > > > So why can't we replace the component with the method you are > > > > > > > > > proposing in this series of finding out the Type-C port also with > > > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > > > > > > component is used for)? > > > > > > > > > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > > > > > > entry) and the drm_bridge_connector to create the connector. I think that > > > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > > > > > > series. > > > > > > > > > > > > > > > > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > > > > > > starting to get really important, so ideally we have a common solution > > > > > > > > > for that. > > > > > > > > > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > > > > > > #dri-devel. > > > > > > > > > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > > > > > > in instead of being well-thought. For example, it is also not always > > > > > > > > possible to provide the drm_connector / typec_connector links (as you can > > > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > > > > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > > > > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > > > > > > > > > > > You really have to always supply not only the Type-C ports and partners, > > > > > > > but also the alt modes. You need them, firstly to keep things sane > > > > > > > inside kernel, but more importantly, so they are always exposed to the > > > > > > > user space, AND, always the same way. We have ABIs for all this stuff, > > > > > > > including the DP alt mode. Use them. No shortcuts. > > > > > > > > > > > > > > So here's what you need to do. UCSI does not seem to bring you > > > > > > > anything useful, so just disable it for now. You don't need it. Your > > > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > > > > > > that's where you need to register all these components - the ports, > > > > > > > partners and alt modes. You have all the needed information there. > > > > > > > > > > > > To make things even more complicate, UCSI is necessary for the USB part of > > > > > > the story. It handles vbus and direction. > > > > > > > > > > > > > Only after you've done that we can start to look at how should the > > > > > > > connection between the DPs and their USB Type-C connectors be handled. > > > > > > > > > > > > But sure enough, I can add typec port registration to the altmode driver. > > > > > > This will solve the 'port not existing' part of the story. > > > > > > > > > > > > I'd like to hear your opinion on: > > > > > > > > > > > > - components. Using them breaks drm/msm. How can we proceed? > > > > > > > > > > I don't think replacing the components is going to be a problem once > > > > > you have described everything properly in you DT. I'm fairly certain now > > > > > that that is the main problem here. You don't have this connection > > > > > described in your DT as it should. > > > > > > > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ > > > > (for non-PMIC-GLINK platform) > > > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full > > > > description of USB-C connector and signal flow. > > > > > > > > In fact, thanks to this representation I can properly set > > > > 'connector->fwnode' to point to the OF node corresponding to the > > > > connector's drm_bridge. I can even propagate it to the kdef->fwnode / > > > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add() > > > > call looks the kernel up. > > > > > > > > And to add on top of that, here is another reason why I think that > > > > this sysfs links ABI/implementation was not well thought. The > > > > typec_connector_ops are added to all fwnode-enabled connector devices. > > > > It doesn't even bother checking that the device is really the DP > > > > connector and that the device on the other side of fwnode link is a > > > > typec port device. The symlink is named 'typec_connector', so one can > > > > not easily extend this ABI to support SlimPort aka MyDP (which uses > > > > micro-USB-B connectors instead of USB-C). Neither can we extend it to > > > > represent MHL connections (again, micro-USB-B). > > > > > > > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the > > > > > > MST ports. > > > > > > > > > > That looks to me like an attempt to exploit a feature that is not > > > > > designed for this purposes at all. Just drop all that. > > > > > > > > But why? From the docs: 'Connector path property to identify how this > > > > sink is physically connected.' > > > > > > > > So far we have been using it for MST only. But the description above > > > > also suits properly for the 'connected to the Type-C port0 device' > > > > kind of data. Then the userspace can use this property to change the > > > > representation of the controller. Or to rename it as it does for > > > > DP-MST connectors. Or just add the USB-C icon in the UI. > > > > > > > > Having this data in sysfs only requires userspace first to map the > > > > connector to the device under sysfs (which is not trivial since Xorg > > > > renames DP-MST connectors), then to look for the symlink value. Quite > > > > complicated compared to checking the DRM property. > > > > > > > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the > > > > schema to support 'microusb:something' values for this property. > > > > > > > > > The connection has to be first described in your DT, and the way you > > > > > usually describe connections in DT is by using the device graph (OF > > > > > graph). It seems that you have everything needed for that - the USB > > > > > Type-C connectors have their own OF nodes (what you register as > > > > > drm_bridges are in fact USB Type-C connectors), and presumable you > > > > > also have OF nodes for all your video ports (DisplayPorts) - so > > > > > applying the graph between the two really should not be a problem. The > > > > > DP is endpoint for the USB Type-C connector, and vice versa. > > > > > > > > Not quite. There is no direct connection between the USB Type-C > > > > connector and DP controller. The USB-C connector has three ports. > > > > > > > > port@0 goes to theHS-USB controller. This is simple. > > > > > > > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are > > > > included in between. And it is the USB+DP PHY that is connected to the > > > > DP and USB-SS controllers. > > > > > > > > port@2 goes to SBU lines mux (e.g. fsa4480). > > > > > > > > > After you have everything needed in your DT, the problem here isn't > > > > > actually much of a problem at all. We will have options how to move > > > > > forward after that. > > > > > > > > Could you please describe what is missing there? > > > > > > We are not after the direct connections here, we are after the final > > > endpoints. So you are missing description of the logical connection > > > between your DP and Type-C connector. > > > > Adding Krzysztof, as one of DT maintainers, to the CC list. > > > > >From what I understand, DT describes hardware. There is no description > > for the 'logical' connections. > > > > > > > > I understand that the idea is to build the graph to describe only the > > > physical connections, but with just the physical connections you are > > > doomed to write separate software solution for almost every single > > > platform, even though the final endpoints are always the same (DP to > > > Type-C). You just can not generalise the components (muxes, phys, > > > retimers, etc.) behind USB Type-C connectors (or anything else for > > > that matter), it's not possible. The components and their order vary > > > on almost every single platform. On some platforms the stack of parts > > > after the connector is also incredibly complex. > > > > Yes. And this is why we have a chain of DRM bridges, going from the DP > > controller to the final drm_bridge at the Type-C port manager. When > > there is the altmode event, it gets sent via this chain using the > > normal DRM HPD event. > > We will not have drm bridges between the thunderbolt controller and > the connector, but you still need to be able to show the connector to > the user when DisplayPort is tunneled over thunderbolt. DP alt mode is > only one way of getting DisplayPort through USB Type-C. You just can't > make any assumptions with USB Type-C. In the end the drm bridge chain is just a funny way to create a drm_connector. The rest of the system sees just drm_connector, no matter if internally it is intel_drm_connecttor or drm_bridge_connector. > > The drm bridge chain could only solve the port/connector relationship > problem from a single angle, but we need a common solution. The > problem is after all completely generic. It is not DisplayPort > specific or even USB Type-C specific problem. Those are just two of > the many possible last endpoints for these connections that need to be > aware of each other. > > So we really have to have a common way of getting this straight from > the hardware description somehow. I'd quite disagree with you here. This works in the x86 world, where the hardware is more or less standard. In the embedded world it is not that easy. This is why we opted for the software transcribing the hardware description. In case of ACPI the software part can be common to all the drivers. But in the embedded systems... I fear is is just not possible. > To me the obvious solution would be to just have a port in the graph > that points directly the last endpoint regardless of what you have in > between. But if that's not an option, then so be it. Then there just > needs to be some other way of getting that information from DT. Could you please point out what is wrong with generating this information on the fly? We are going to work on fixing the pmic_glink_altmode in the next couple of weeks, so that the typec port device will be always present, as you suggested. Are there any other obstacles? > > Maybe DT could use similar physical location object/attribute like > ACPI - the DP would have matching physical location with its connector? > > > > Having the logical final endpoint connection described in your DT/ACPI > > > on top of the physical connections costs very little, but at the same > > > time it's usually the only thing that the software needs (like in this > > > case). > > > > Maybe there is some misunderstanding here. We have this connection. We > > have connector->fwnode and connector->of_node pointing to the correct > > device - the last bridge in the chain. Each TCPM driver knows the > > relationship between the in-built drm_bridge and the Type-C port. The > > DP host controller port can be terminated with other endpoints, e.g. > > eDP panel. Or there can be a non-DP host, which is then connected > > through a series of bridges to the eDP or external DP port. This is > > what anx78xx bridge does: it converts the HDMI link into an external > > DP (SlimPort) connection. Bridge chains permit this to be handled in a > > seamless way. > > > > What you are asking for looks like a step backwards to me: it requires > > the host to know that there is a USB-C connector. > > > > > So, either you add one more port to your graph for the DP to Type-C > > > connection, or, if that's not an option, then you need to describe > > > that connection in some other way. Named references work also quite > > > well in my experience. > > > > Named references were considered and frowned upon by DT maintainers. > > thanks, > > -- > heikki
Hi guys, On Thu, Sep 14, 2023 at 01:40:49PM +0300, Dmitry Baryshkov wrote: > On Thu, 14 Sept 2023 at 12:26, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > Hi Dmitry, > > > > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > > > > > Hi Heikki, > > > > > > > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > > > > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > > > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these links, > > > > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > > > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional. > > > > > > > > > > > > > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > > > > > > > a better way of determining the connection between the DP and its > > > > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > > > > > > > > > You just need the symlinks, not the component. > > > > > > > > > > > > > > > > > > > > > > The problem is that right now this component registration has become > > > > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > > > > > > > > > 2), the kernel hangs inside the component code. > > > > > > > > > > > That's why I proposed to move the components to the place where they > > > > > > > > > > > are really necessary, e.g. i915 and amd drivers. > > > > > > > > > > > > > > > > > > > > So why can't we replace the component with the method you are > > > > > > > > > > proposing in this series of finding out the Type-C port also with > > > > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that > > > > > > > > > > component is used for)? > > > > > > > > > > > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > > > > > > > > > entry) and the drm_bridge_connector to create the connector. I think that > > > > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this > > > > > > > > > series. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Determining the connection between a DP and its Type-C connector is > > > > > > > > > > starting to get really important, so ideally we have a common solution > > > > > > > > > > for that. > > > > > > > > > > > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on > > > > > > > > > #dri-devel. > > > > > > > > > > > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened > > > > > > > > > in instead of being well-thought. For example, it is also not always > > > > > > > > > possible to provide the drm_connector / typec_connector links (as you can > > > > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP > > > > > > > > > connector, but we can not easily point it to the particular USB-C port. > > > > > > > > > > > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks > > > > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to > > > > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is > > > > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases. > > > > > > > > > > > > > > > > You really have to always supply not only the Type-C ports and partners, > > > > > > > > but also the alt modes. You need them, firstly to keep things sane > > > > > > > > inside kernel, but more importantly, so they are always exposed to the > > > > > > > > user space, AND, always the same way. We have ABIs for all this stuff, > > > > > > > > including the DP alt mode. Use them. No shortcuts. > > > > > > > > > > > > > > > > So here's what you need to do. UCSI does not seem to bring you > > > > > > > > anything useful, so just disable it for now. You don't need it. Your > > > > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so > > > > > > > > that's where you need to register all these components - the ports, > > > > > > > > partners and alt modes. You have all the needed information there. > > > > > > > > > > > > > > To make things even more complicate, UCSI is necessary for the USB part of > > > > > > > the story. It handles vbus and direction. > > > > > > > > > > > > > > > Only after you've done that we can start to look at how should the > > > > > > > > connection between the DPs and their USB Type-C connectors be handled. > > > > > > > > > > > > > > But sure enough, I can add typec port registration to the altmode driver. > > > > > > > This will solve the 'port not existing' part of the story. > > > > > > > > > > > > > > I'd like to hear your opinion on: > > > > > > > > > > > > > > - components. Using them breaks drm/msm. How can we proceed? > > > > > > > > > > > > I don't think replacing the components is going to be a problem once > > > > > > you have described everything properly in you DT. I'm fairly certain now > > > > > > that that is the main problem here. You don't have this connection > > > > > > described in your DT as it should. > > > > > > > > > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/ > > > > > (for non-PMIC-GLINK platform) > > > > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full > > > > > description of USB-C connector and signal flow. > > > > > > > > > > In fact, thanks to this representation I can properly set > > > > > 'connector->fwnode' to point to the OF node corresponding to the > > > > > connector's drm_bridge. I can even propagate it to the kdef->fwnode / > > > > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add() > > > > > call looks the kernel up. > > > > > > > > > > And to add on top of that, here is another reason why I think that > > > > > this sysfs links ABI/implementation was not well thought. The > > > > > typec_connector_ops are added to all fwnode-enabled connector devices. > > > > > It doesn't even bother checking that the device is really the DP > > > > > connector and that the device on the other side of fwnode link is a > > > > > typec port device. The symlink is named 'typec_connector', so one can > > > > > not easily extend this ABI to support SlimPort aka MyDP (which uses > > > > > micro-USB-B connectors instead of USB-C). Neither can we extend it to > > > > > represent MHL connections (again, micro-USB-B). > > > > > > > > > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the > > > > > > > MST ports. > > > > > > > > > > > > That looks to me like an attempt to exploit a feature that is not > > > > > > designed for this purposes at all. Just drop all that. > > > > > > > > > > But why? From the docs: 'Connector path property to identify how this > > > > > sink is physically connected.' > > > > > > > > > > So far we have been using it for MST only. But the description above > > > > > also suits properly for the 'connected to the Type-C port0 device' > > > > > kind of data. Then the userspace can use this property to change the > > > > > representation of the controller. Or to rename it as it does for > > > > > DP-MST connectors. Or just add the USB-C icon in the UI. > > > > > > > > > > Having this data in sysfs only requires userspace first to map the > > > > > connector to the device under sysfs (which is not trivial since Xorg > > > > > renames DP-MST connectors), then to look for the symlink value. Quite > > > > > complicated compared to checking the DRM property. > > > > > > > > > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the > > > > > schema to support 'microusb:something' values for this property. > > > > > > > > > > > The connection has to be first described in your DT, and the way you > > > > > > usually describe connections in DT is by using the device graph (OF > > > > > > graph). It seems that you have everything needed for that - the USB > > > > > > Type-C connectors have their own OF nodes (what you register as > > > > > > drm_bridges are in fact USB Type-C connectors), and presumable you > > > > > > also have OF nodes for all your video ports (DisplayPorts) - so > > > > > > applying the graph between the two really should not be a problem. The > > > > > > DP is endpoint for the USB Type-C connector, and vice versa. > > > > > > > > > > Not quite. There is no direct connection between the USB Type-C > > > > > connector and DP controller. The USB-C connector has three ports. > > > > > > > > > > port@0 goes to theHS-USB controller. This is simple. > > > > > > > > > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are > > > > > included in between. And it is the USB+DP PHY that is connected to the > > > > > DP and USB-SS controllers. > > > > > > > > > > port@2 goes to SBU lines mux (e.g. fsa4480). > > > > > > > > > > > After you have everything needed in your DT, the problem here isn't > > > > > > actually much of a problem at all. We will have options how to move > > > > > > forward after that. > > > > > > > > > > Could you please describe what is missing there? > > > > > > > > We are not after the direct connections here, we are after the final > > > > endpoints. So you are missing description of the logical connection > > > > between your DP and Type-C connector. > > > > > > Adding Krzysztof, as one of DT maintainers, to the CC list. > > > > > > >From what I understand, DT describes hardware. There is no description > > > for the 'logical' connections. > > > > > > > > > > > I understand that the idea is to build the graph to describe only the > > > > physical connections, but with just the physical connections you are > > > > doomed to write separate software solution for almost every single > > > > platform, even though the final endpoints are always the same (DP to > > > > Type-C). You just can not generalise the components (muxes, phys, > > > > retimers, etc.) behind USB Type-C connectors (or anything else for > > > > that matter), it's not possible. The components and their order vary > > > > on almost every single platform. On some platforms the stack of parts > > > > after the connector is also incredibly complex. > > > > > > Yes. And this is why we have a chain of DRM bridges, going from the DP > > > controller to the final drm_bridge at the Type-C port manager. When > > > there is the altmode event, it gets sent via this chain using the > > > normal DRM HPD event. > > > > We will not have drm bridges between the thunderbolt controller and > > the connector, but you still need to be able to show the connector to > > the user when DisplayPort is tunneled over thunderbolt. DP alt mode is > > only one way of getting DisplayPort through USB Type-C. You just can't > > make any assumptions with USB Type-C. > > In the end the drm bridge chain is just a funny way to create a > drm_connector. The rest of the system sees just drm_connector, no > matter if internally it is intel_drm_connecttor or > drm_bridge_connector. > > > > > The drm bridge chain could only solve the port/connector relationship > > problem from a single angle, but we need a common solution. The > > problem is after all completely generic. It is not DisplayPort > > specific or even USB Type-C specific problem. Those are just two of > > the many possible last endpoints for these connections that need to be > > aware of each other. > > > > So we really have to have a common way of getting this straight from > > the hardware description somehow. > > I'd quite disagree with you here. This works in the x86 world, where > the hardware is more or less standard. In the embedded world it is not > that easy. This is why we opted for the software transcribing the > hardware description. In case of ACPI the software part can be common > to all the drivers. But in the embedded systems... I fear is is just > not possible. > > > To me the obvious solution would be to just have a port in the graph > > that points directly the last endpoint regardless of what you have in > > between. But if that's not an option, then so be it. Then there just > > needs to be some other way of getting that information from DT. > > Could you please point out what is wrong with generating this > information on the fly? We are going to work on fixing the > pmic_glink_altmode in the next couple of weeks, so that the typec port > device will be always present, as you suggested. > Are there any other obstacles? > > > Maybe DT could use similar physical location object/attribute like > > ACPI - the DP would have matching physical location with its connector? > > > > > > Having the logical final endpoint connection described in your DT/ACPI > > > > on top of the physical connections costs very little, but at the same > > > > time it's usually the only thing that the software needs (like in this > > > > case). > > > > > > Maybe there is some misunderstanding here. We have this connection. We > > > have connector->fwnode and connector->of_node pointing to the correct > > > device - the last bridge in the chain. Each TCPM driver knows the > > > relationship between the in-built drm_bridge and the Type-C port. The > > > DP host controller port can be terminated with other endpoints, e.g. > > > eDP panel. Or there can be a non-DP host, which is then connected > > > through a series of bridges to the eDP or external DP port. This is > > > what anx78xx bridge does: it converts the HDMI link into an external > > > DP (SlimPort) connection. Bridge chains permit this to be handled in a > > > seamless way. I'm sorry, I did not read this comment carefully enough earlier. I probable have misunderstood something related to the drm bridges. I'm still not sure why would it be a problem to try to hook up the port and connector device nodes - you seem to have them ready in any case, no? But maybe it's best that you guys just prepare the next version at this point. Let's continue then. > > > What you are asking for looks like a step backwards to me: it requires > > > the host to know that there is a USB-C connector. > > > > > > > So, either you add one more port to your graph for the DP to Type-C > > > > connection, or, if that's not an option, then you need to describe > > > > that connection in some other way. Named references work also quite > > > > well in my experience. > > > > > > Named references were considered and frowned upon by DT maintainers. thanks,
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index b169b3e44a92..06662cc8d3f4 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -11,14 +11,12 @@ */ #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> @@ -98,34 +96,6 @@ 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"); /** @@ -394,16 +364,9 @@ 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 to create link to typec connector\n"); - } - if (connector->ddc) return sysfs_create_link(&connector->kdev->kobj, &connector->ddc->dev.kobj, "ddc"); - return 0; err_free: @@ -419,9 +382,6 @@ 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);
The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so dev_fwnode() checks never succeed, making the respective commit NOP. And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it breaks drivers already using components (as it was pointed at [1]), resulting in a deadlock. Lockdep trace is provided below. Granted these two issues, it seems impractical to fix this commit in any sane way. Revert it instead. [1] https://lore.kernel.org/dri-devel/Y24bcYJKGy%2Fgd5fV@phenom.ffwll.local/ ============================================ WARNING: possible recursive locking detected 6.5.0-rc6-next-20230816-10542-g090e2ca9feae-dirty #713 Tainted: G W -------------------------------------------- kworker/u16:0/11 is trying to acquire lock: ffffce0f54bea490 (component_mutex){+.+.}-{3:3}, at: __component_add+0x64/0x170 but task is already holding lock: ffffce0f54bea490 (component_mutex){+.+.}-{3:3}, at: __component_add+0x64/0x170 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(component_mutex); lock(component_mutex); *** DEADLOCK *** May be due to missing lock nesting notation 6 locks held by kworker/u16:0/11: #0: ffff5b7680008d38 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x14c/0x51c #1: ffff8000800abde0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x14c/0x51c #2: ffff5b76837a2908 (&dev->mutex){....}-{3:3}, at: __device_attach+0x38/0x188 #3: ffffce0f54bea490 (component_mutex){+.+.}-{3:3}, at: __component_add+0x64/0x170 #4: ffffce0f54bdeb40 (drm_connector_list_iter){.+.+}-{0:0}, at: drm_modeset_register_all+0x80/0x9c #5: ffff5b76866ad0d0 (&connector->mutex){+.+.}-{3:3}, at: drm_connector_register.part.0+0x28/0x104 stack backtrace: CPU: 6 PID: 11 Comm: kworker/u16:0 Tainted: G W 6.5.0-rc6-next-20230816-10542-g090e2ca9feae-dirty #713 Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) Workqueue: events_unbound deferred_probe_work_func Call trace: dump_backtrace+0x98/0xf0 show_stack+0x18/0x24 dump_stack_lvl+0x60/0xac dump_stack+0x18/0x24 print_deadlock_bug+0x254/0x340 __lock_acquire+0x105c/0x1ebc lock_acquire+0x1ec/0x314 __lock_acquire+0x105c/0x1ebc lock_acquire+0x1ec/0x314 __mutex_lock+0xa0/0x77c mutex_lock_nested+0x24/0x30 __component_add+0x64/0x170 component_add+0x14/0x20 drm_sysfs_connector_add+0x144/0x1a0 drm_connector_register.part.0+0x5c/0x104 drm_connector_register_all+0x84/0x160 drm_modeset_register_all+0x80/0x9c drm_dev_register+0x120/0x238 msm_drm_bind+0x550/0x6e0 try_to_bring_up_aggregate_device+0x164/0x1d0 __component_add+0xa8/0x170 component_add+0x14/0x20 dsi_dev_attach+0x20/0x2c dsi_host_attach+0x9c/0x144 devm_mipi_dsi_attach+0x34/0xb4 lt9611uxc_attach_dsi.isra.0+0x84/0xfc lt9611uxc_probe+0x5ac/0x66c i2c_device_probe+0x148/0x290 really_probe+0x148/0x2ac __driver_probe_device+0x78/0x12c driver_probe_device+0x3c/0x160 __device_attach_driver+0xb8/0x138 bus_for_each_drv+0x80/0xdc __device_attach+0x9c/0x188 device_initial_probe+0x14/0x20 bus_probe_device+0xac/0xb0 deferred_probe_work_func+0x8c/0xc8 process_one_work+0x1ec/0x51c worker_thread+0x1ec/0x3e4 kthread+0x120/0x124 ret_from_fork+0x10/0x20 Fixes: c5c51b242062 ("drm/sysfs: Link DRM connectors to corresponding Type-C connectors") Cc: Won Chung <wonchung@google.com> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/drm_sysfs.c | 40 ------------------------------------- 1 file changed, 40 deletions(-)