Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Sent: Tuesday, February 12, 2019 7:22 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, > > On Mon, Feb 11, 2019 at 11:43:17PM +0000, Michael Hsu wrote: > > 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. > > Now the user can read the vdo from DP alt mode, and always know that it > contains the DisplayPort capabilities, just like it should. If we were to add > support for your PPM as is, the user would loose guarantee for that. The vdo > would then mean something completely different with your PPM in case of the > connectors. > > The user would then have to be able to, firstly identify the hardware, and then > of course know how that handle it. > > Basically the user space would have to deal with your hardware completely > separately. And only with your hardware. > > <snip> > > > > 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." > > Yes, we may have multiple modes defined in future versions of the DP alt mode > spec, but the connector, just like the partners attached to it, will still have only > one mode per definition. > > Right now there is only one mode defined, but you still return two for the > connector. > > > 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. > > The current patch does have a bug, and it needs to be fixed, but that does not > fix the issue you have with the two DP modes for the connector. We are talking > about two separate issues here. > > > > > 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. > > Note that you can't just mix that into the ucsi_ccg.c. You are not the only one > using the Cypress CCGx USB PD controllers. I think even we use them on some > of our platforms, but on our platform the PD controller is usually attached to > the embedded controller, so the operating systems continues to access the PPM > using the ACPI mailbox. > > But something like that has to be done. I just realised that the hardware is > already being sold to the customers, so I don't think we have a choice. The > problem is that once the workaround is in, the firmware will never ever be > fixed :-(. > We can't change our PPM firmware since we have too many devices out their already. But, we'll be submitting future patches on top of yours which address the objections you've mentioned already in this thread. > > > > > 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 > > I think for clarity's sake I better fix the mode index issue now and send second > version of these patches so we can concentrate on the bigger problem, the two > connector DP alt modes. > Agreed. Please update your patch for the mode index issue (being able to reliably match partner mode index with the connector mode index returned by UCSI). > > 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 > > No! We must not hide stuff like that from the user. Even if the connector does > not support a partner mode, we still show it to the user space. > OK, but this means the "active" sysfs node for a partner mode (which is not supported by GET_SUPPORTED_CAM) will always show "no". And writing "yes" to it will silently fail... Did this intentional? Having some partner sysfs "active" nodes which do nothing without any warning to user that they don't work? > This rule should apply to everything else. For example, even when the > connector does not support Audio Accessory devices, a partner device should > still pop up. > > Nothing is preventing the user from comparing the connector and partner and > determine things from that on its own, for example if audio accessory or an > alternate mode is supported or not, but it means the interface must follow the > rules. No exceptions. > > As a shortcut with the alternate modes, the user can determine if a partner > mode is supported by simply checking if it's linked to a connector mode (there > is a symlink pointing to the connector mode), or alternatively by checking if the > mode is bind to a driver. > OK, we can come up with some method of interfacing our PPM firmware's mode table with your patch's expectation (of only one alt mode index per DP alt mode). Let's treat that independently from your set of patches. > > 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. > > We can not put user experience aside. The user space must know what are the > capabilities of the platform as well as the capabilities of the devices attached to > the platform, and what is going on in general. > > One of the big problems with USB Type-C is that the user has no idea what does > his/her hardware support and not support. That we can not fix, but we must do > everything to make sure that it will at least always be possible to supply > information to the user. > > If a connector does not support Audio Accessory, Thunderbolt, or whatever, the > user should always be informed by the operating system that you just plugged > this or that device and, sorry, but it's not supported on this connector, > whenever the user tries to attach one of those devices. > > Otherwise the user will simply assume that everything is working, the OS did > not say anything, and then start scratching his/her head... Why isn't it working? > > The Billboard Device Class does not help here as it only tells the capabilities of > the partner device. > > > > > > > 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. > > Unfortunately the other players clearly interpreted the situation differently. > > I think spec update is needed even regardless of this issue. I'll see if I can start a > dialog first inside Intel regarding the limitations and lack of clear definition in > some parts of the current UCSI spec. > > > thanks, > > -- > heikki Thanks, Michael nvpublic
