Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 > 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 > > > 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 > > > > > 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 > > > > > > > > > > > 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 > > > > > > > > > > > > > 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 > > > >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Thu, 14 Sept 2023 at 12: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 > > 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 > > > > 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 > > > > > > > > > > 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 > > > > > > > > > > > > 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Thu, 14 Sept 2023 at 12:35, Neil Armstrong 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 > >> 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 > 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 > >> 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 > 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 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 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 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 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 > 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 > > > 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 > > > > > > > > > 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 > > > > > > > > > > > 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 > > > > > > >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus 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 > > 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 > > > > > > > > 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 > > > > > > > > > > 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 > > > >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 > 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 > > > > > > > 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 > > > > > > > > > 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 > > >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi Heikki, On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus 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 > > > > > > 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 > > > > > > > > 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 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 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)
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 > > > > > 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 > > > > > > > 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 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 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 > > > 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 > > > > > 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 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 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. -- With best wishes Dmitry
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 signature.asc Description: PGP signature
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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: > > > Hi Heikki, > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > 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, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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. -- Regards, Laurent Pinchart
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus 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 > > 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. -- With best wishes Dmitry
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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 > 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, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi Heikki, On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus 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
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
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, -- heikki
