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