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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@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.
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@xxxxxxxxxx/
(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,
