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.
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'.
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. Then, a previously 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.
>
> 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, we can prepare a patch to translate the PPM's returned alt mode list (which contains different CAM index for each DP pin assignment) into a compressed table. We would also need to do the reverse transformation.
Method (1). In ucsi_ppm structure...
struct ucsi_ppm {
+ int (*canonize_alt_mode_table)(struct typec_altmode *altmodes); /* compresses multiple DP alt modes into single alt mode */
+ int (*get_ppm_cam)(struct typec_altmode *altmodes, int cam, unsigned long am_specific);
};
Method (2). Hook directly into ucsi_ccg.c's command / response handler. When there is a response from ppm to GET_ALTERNATE_MODES (recipient = connector), it substitutes the return value with the single SVID/ModeID entry. When it gets a UCSI SET_CURRENT_CAM command, it will translate the index value from the ucsi/displayport.c's index into the ppm's value. This isolates changes within ucsi_ccg.c and does not require addition of function callbacks to the ucsi_ppm structure.
>
> That does not leave much use for the separated modes for the two pin
> assignments. The only thing they are providing in any case is the guarantee of
> the initial pin assignment, but as said, that we can guess in any case.
>
> Sorry for the long explanation.
>
>
> Br,
>
> --
> heikki
Thanks,
Michael
nvpublic
