Hi Heikki,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Friday, February 8, 2019 5:48 AM
> To: Michael Hsu <mhsu@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Ajay Gupta
> <ajayg@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate
> modes
>
> Hi Michael,
>
> On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote:
> > Hi Heikki,
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > Sent: Thursday, February 7, 2019 5:16 AM
> > > To: Michael Hsu <mhsu@xxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Ajay Gupta
> > > <ajayg@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for
> > > alternate modes
> > >
> > > Hi Michael,
> > >
> > > On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote:
> > > > Your existing patch matches the SVID and then requires one-for-one
> > > > ordering in case either alt mode table has multiple mode entries
> > > > for a
> > > particular SVID.
> > >
> > > And we'll fix that, but that does not solve the issue that I'm
> > > talking about. There are two separate issues here. That problem is a
> > > bug in the driver, but the major problem still is that you have
> > > separate alternate modes for the two DP pin assignments, and fixing the
> index handling does not help with that.
> > >
> > > According to the DP alt mode standard you do not have a separate
> > > mode for every supported DṔ pin assignment. When you change the DP
> > > alt mode pin assignment, the current mode is not exited and a new one
> entered.
> > > You stay in the DisplayPort mode, and simply re-configure using the
> > > DisplayPort Configure command. In your implementation however, the
> > > mode is existed, and a new one entered. So your implementation is
> > > not made according to the DisplayPort Alt Mode standard.
> > >
> > > I guess I have been able to explain just how big of a problem that
> > > is, and also how exactly we will have to handle it... Let's look at
> > > this from user space perspective.
> > >
> > > The user space should not need to be aware of the method used to
> > > interface with the USB Type-C connectors on the system. The kernel
> > > needs to hide that and supply unified interface to the user space
> > > which looks and behaves the same, _always_.
> > >
> > > In case of the DP alt mode, the user space should be allowed to
> > > expect the behaviour to follow the DP alt mode spec, so when the
> > > user space selects another pin assignment for the DP alt mode
> > > adapter, it should be a pass-fail operation without any side
> > > effects, just like explained in the VDM flows in the DP alt mode spec.
> > >
> > > But now with your PPM, there is a major side effect. Every time the
> > > user space selects a new DP pin assignment, the current mode is
> > > actually exited and a new mode entered. That is not standard
> > > behaviour. The user space will not just ignore the mode being exited.
> >
> > Not exactly true... If you look on a USB PD analyzer at the type c
> > connector's CC lines, you would *not* see and "Exit Mode" VDMs - just "DP
> Configure" VDMs.
>
> So your PPM does not actually reflect the behaviour of the hardware and
> firmware. You are not actually changing the mode when you change the pin
> assignment, yet you still want the operating system to think it has to change the
> mode every time it changes the pin assignment.
>
> That means the two modes you have are for completely customised software
> (firmware) representation of the DisplayPort alt mode support that does not
> follow the standard, and as we can now see, reflect even the actual behaviour.
>
> If your connector could act as the sink, how many DP alt modes would return to
> the partner as a response to Discover Modes with DP SVID?
> You would return only one regardless of how many pin assignments you
> support, right? That's what you need to return to the OS as well, as source or
> sink.
>
> > The "active" sysfs node with the value of "yes" would change paths
> > (since another UCSI CAM index is active), but that's because there was
> > a UCSI connector status change.
> >
> > > It has to react to it, most likely by assuming there was a fatal error
> somewhere.
> > > So in practice the user space is now forced to handle your connector
> > > as a special case.
> >
> > The expected user behavior is to read every sysfs node of the form...
> > /sys/class/typec/.../svid
> > /sys/class/typec/.../vdo
> > And if it decides that's the alt mode it wants to activate, it writes 1 to
> > /sys/class/typec/.../active
> > And optionally, reads above sysfs file to confirm that it did indeed
> > change to 'yes'.
> > It would treat it as an failure to enter alt mode if it was read as
> > still 'no'.
>
> Do you understand that those steps you can take _only_ on your hardware in
> order to change the DP pin assignment? They work only with your PPM. The
> approach is completely custom.
>
I disagree, most user-mode applications would read the contents of the 'svid' and 'vdo'
sysfs nodes and then decide if they want to enter that alternate mode - by writing
to the corresponding 'active' node.
Question: If the application does not examine
both the 'svid' and 'vdo' nodes, what other node do they read before deciding whether
to enter that alternate mode or not?
Question2: Why even report this 'svid' and 'vdo' values if it was not the intent of the
type c driver design to allow applications to inspect them and act on them?
> The other UCSI PPMs handle this part correctly. There is always only one
> DisplayPort alt mode for the connector, regardless of how many DP pin
> assignment the connector support. Your PPM is the only exception.
>
In the absence of any express or implied prohibition on this implementation
(in ucsi, type c, or usb PD specifications), it would be better driver design to
support this (be flexible in what you accept as input, be strict in what you output).
> > The unexpected behavior which you are talking about is if the user
> > uses the displayport pin assignment sysfs node to switch from pin C to pin D.
>
> Note that the sysfs attribute file for the DP pin assignment is the _standard_
> interface. That sysfs file is the interface the user space will use for changing the
> DisplayPort pin assignment.
>
No problem... With the use of '&' masking between the UCSI connector alt mode
vdo with the UCSI partner alt mode vdo, they DP sysfs pin assignment nodes would
work.
What is a problem is requiring one for one mode index matching between the UCSI
connector alt mode table with the UCSI partner alt mode table.
According to DP over Type C spec,
"Future versions of this Standard may describe other modes associated with DP_SID. Such modes
will be identified by having a non-zero value in bits 31:24 of the VDO. The DFP_U shall examine
the list of modes returned until it finds 0s in bits 31:24 of the VDO and a non-zero value in bits 23:0
of the VDO (i.e., DisplayPort capabilities). The DFP_U and UFP_U shall use the corresponding
offset (indexed from 1) as the Object Position in the Enter Mode, DisplayPort Configure,
DisplayPort Status Update, Attention, and Exit Mode commands."
Above requirement means that for SVID 0xff01, it is possible for multiple Mode VDOs to be
associated with it. Hence, the latest patch's algorithm (of matching mode index will not work).
To illustrate:
Alt mode adapter responds to Discover Modes VDM (for SVID 0xff01) with following:
(a) 0x00000c05
==> this has svid 0xff01, partner mode index 1
(b) 0x01xxxxxxx --> non-zero value in bits 31 to 24 indicate future DP alt mode
==> this has svid 0xff01, partner mode index 2
But GPU PPM supports only (b) hence its UCSI GET_ALTERNATE_MODES (recipient = connector) returns
only one connecter alt mode:
(1) 0x01xxxxxx --> GPU supports only extended DP alt mode
==> this has svid 0xff01, connector mode index 1
The patch's algorithm of '==' comparison with mode index would yield the wrong correlation between connector and partner alt modes.
Note how using '&' mask between the UCSI's connector mode VDO and UCSI's partner mode VDO would work in all cases.
> > Then, a reviously active mode would turn to 'no' and another active
> > sysfs node would change to 'yes'.
> >
> > But, user-mode application has to be able to expect this anyways. (This =
> > some active sysfs node changing even though the application did not
> > cause it.) For example, a TypeC-to-DP dongle is attached. This dongle
> > is also a hub so a downstream USB superspeed device can be connected
> > to it. When there is no downstream USB device attached, DisplayPort
> > Pin C (4 lanes of video) are supported. But, user plugs in a USB
> > device to the dongle - then the dongle will switch from pin C to pin D
> > automatically to allow the USB superspeed lanes to be connected. In
> > this case, I'd expect your displayport ABI to reflect the automatic
> > pin assignment change from C to D caused not by any sysfs activity,
> > but simply plugging in USB device to the typec alt mode adapter. In
> > our particular case, in addition to the "pin" sysfs node changing from
> > "[C] D E" to "C [D] E", the "active" sysfs node with the "yes" value
> > would change from one path to another. So, applications must expect values
> of sysfs nodes under /sys/class/typec to change by itself.
>
> We are not talking about just some sysfs files here. Every alternate mode will
> have a kernel object representing them. When a change happens on those
> objects, for example if a mode is entered or exited (regardless of was it as a
> result of user action or not), or the DP pin assignment gets changed, the kernel
> generates events for those objects. Those events are the primary source of
> initial information in user space.
>
> The problem is that changing the DP pin assignment has different meaning
> than exiting and entering the modes. Again, changing the DP pin assignment
> does not cause the mode to be exited and another to be entered. That is how
> the standard defines it (and that is the reason why we have the separate sysfs
> attribute file for the DP pin assignment in the first place), and that's how the
> user space will expect the interface to behave.
>
> With your PPM however the pin assignment is completely coupled with the
> connector mode. If the pin assignment is changed, the mode gets changed, and
> if the mode is changed, the pin assignment gets changed.
> That is simply not the standard behaviour.
>
Keep in mind that the previous type c ABI did not have this restriction.
This new set of patches for creation of the displayport ABI implicitly added this.
However, one of my previous suggestions would maintain this invariance (i.e., changing pin assignments would not cause alt mode change).
- basically, when PPM returns alt mode table with separate entries for each mode VDO, the ucsi_ccg.c driver would compress the alt mode table before presenting it to ucsi/displayport.c.
So, if this is your only objection, we can accept this. Need to change struct ucsi_ppm to add function pointers to translate ppm's alt mode table into the format that ucsi/displayport.c required.
> > > To protect the user space from that, and to keep the interface we
> > > provide to it consistent and predictable, we would have to handle
> > > your PPM completely separately. The user space will see only one
> > > connector DP alt mode no matter what. Even if you are unable change
> > > the PPM, the driver will still have to expose the two connector DP
> > > alternate modes as one connector alternate mode to the user space.
> >
> > If this is acceptable to you,
>
> It really isn't, but it will be something that we have to do unless you guys can fix
> your firmware. This will be a large quirk that we would have to implement and
> then maintain, just to workaround yet another firmware issue.
>
> Kernel is already full of workarounds like that, so I'm begging you, please get
> the firmware fixed. Surely you now see that it does not exactly comply with the
> DP alt mode standard.
This "workaround" was actually required by the following design "flaw" in the current patch:
- you create partner sysfs nodes using UCSI GET_ALT_MODES (recipient == SOP)
- UCSI SET_NEW_CAM requires *connector* alt mode index
- problem: how to correlate *connector* alt mode index with *partner* alt mode index
in the absence of any device-independent means to do this in the UCSI spec
Here's a method of avoiding the above dilemma:
- create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with recipient == connector)
- also query UCSI GET_SUPPORTED_CAM
- create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit for a particular connector alt mode index
You might consider it wrong to populate partner sysfs nodes with connector alt modes, but it makes sense
If you ask the question: why report partner alt modes which are not supported at the connector side? You'd never
be able to enter that partner alt mode anyways, unless both the connector and partner mutually supported it.
>
> Why would you need the two DP alt modes for the connector?
>
As mentioned earlier, this was due to limitation of UCSI which did not allow querying of current DP pin assignment.
We maintained compliance with UCSI, yet overcame its limitations using this connector alt mode table arrangement.
If you hadn’t objected so strongly to this technique, we would've probably recommended this technique to all our
technical partners.
Maybe even asked Intel to bless this scheme when they publish the next UCSI spec update.
>
> Br,
>
> --
> heikki
Thanks,
Michael
nvpublic
