Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Sent: Monday, February 4, 2019 4:10 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 Fri, Feb 01, 2019 at 10:02:19PM +0000, Michael Hsu wrote: > > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a > > 1-based index, not a 32-bit mode VDO) can cause incorrect matches if > > the GET_ALTERNATE_MODES returns different ordering for > > recipient=connector and recipient=sop for a particular svid. > > > > For example, UCSI command GET_ALTERNATE_MODES with > recipient=connector returns > > [0] SVID=0xff01, ModeVDO=0x00000405 (Mode = 1) > > [1] SVID=0xff01, ModeVDO=0x00000805 (Mode = 2) And UCSI command > > GET_ALTERNATE_MODES with recipient=sop returns > > [0] SVID=0xff01, ModeVDO=0x00000c05 (Mode = 1) > > > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM > > returns index 1 (connector alternate mode = 1, i.e. SVID=0xff01, > > Mode=2, ModeVDO=0x00000805). > > But, the function will be unable to match it with partner_alt_mode > > corresponding to (SVID=0xff01,Mode=1). > > > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' > > bitwise AND operator when masking the partner's mode VDO (0x0c05) > > against the connector's mode VDO (0x405 or 0x805) to determine it > > there is an alternate mode match. > > No top-posting! From now on inline your comments in your replies. The > "process" guide in kernel should provide more information for you: > https://www.kernel.org/doc/html/latest/process/2.Process.html#mailing-lists > OK, got it... answering inline. > Your PPM is reporting a separate mode for every pin-assignment it supports. It > really should _not_ do that! You need to be able to get the capabilities for DP > alt mode with GET_ALTERNATE_MODE command in the format that the DP alt > mode spec defines, also with the connector. > You are not getting them like that. Now for example in both modes your > connector can only act as DisplayPort sink (i.e. a display) which I'm pretty sure > is not the case. > > With the current version of the DP alt mode spec we do not ever need more > than one mode for DP. That mode needs to show all the supported pin > assignments the device, partner or connector, supports. That is exactly how all > the other UCSI PPMs work currently. For example the PPM on Dell XPS 13 9380 > that I use for testing gives only one mode for the connector with SVID ff01. The > mode VDO (or MID in UCSI lingo) has the value 0x1c46: > > SVID=0xff01, VDO=0x1c46 > > From that you can clearly see that the connector can only act as the DisplayPort > source, and that it support pin assignments C, D and E. > > So in practice you have a firmware bug. I understand why the PPM was made > that way. That "hack" allows the PPM to workaround a limitation in UCSI where > we in practice can't tell which configuration is in use at any given time. > Unfortunately it can not be done like that. It leaves us with custom VDO values > that we would have to be interpreted separately, that only your PPM supports. > I still think it complies with the letter and spirit of the UCSI spec... There was nothing in the document to suggest that this method of implementing alt modes (i.e., assigning one connector alt mode index per DP pin configuration) is not allowed. Actually, there's also a (great) benefit with this method, the UCSI GET_SUPPORT_CAM can now be used to determine which DP pin assignments are allowed - dynamically. For example, sometimes due to another (non-DisplayPort) alt mode being active, the pins used for DP pin C might not be available, but DP pin D is still allowed. So, a call to UCSI GET_SUPPORTED_CAM would indicate that the pin C DP alt mode was not supported, but the pin D DP alt mode was currently allowed. The drawback which you mentioned "custom VDO values" is not accurate. The GET_ALTERNATE_MODES commands still returns DP-compliant mode VDOs - i.e., bits 15-8 still indicate the DP pin configurations allowed when entering that DP-compatible alternate mode. My original suggestion was to use a bitwise AND mask to make the correlation between the connector's alt mode and the partner's alt mode. This has the benefit of (1) working with PPMs which report a single connector alternate mode for all the DP pin assignments (2) working with PPMs which report separate connector alternate modes for each DP pin assignment If we use your original patch which does a simple mode index equality comparison (instead of a bitwise AND mask), it cannot handle case (2) above. > Please see if you can get the firmware (UCSI PPM) fixed. It really needs to > expose only one mode for DisplayPort alt mode in the connector, and the VDO > in it should be in the same form that it could be send to the partner, i.e. giving > the actual DisplayPort capabilities for the connector. > There is infrastructure build on top of this capability of being able to determine the DP pin assignment support using just the connector alt mode (CAM) index. So, removing the "distinct alt mode index" implementation is not feasible. But, as mentioned above, we believe a minor change (replacing == comparison of the mode index, with a "&" masking of the mode VDO) would support all cases. > We should not do anything before you ask them to fix the PPM. > Experience tells that if we workaround an issue in firmware, the firmware will > never ever get fixed. And in this case the workaround may not be as simple as > one would think. Even if we have to workaround this, it needs a separate patch > with a good explanation. And it probable does not belong to this series. Of the available choices, in terms of number of lines of C code to change + engineering resources, choice (b) is more straightforward... (a) Change the PPM firmware: do not report multiple connector alt modes per DisplayPort pin assignment - significant rewrite of firmware alt mode code - requires QA requalification of PPM with other display monitors - changing internal tools to avoid querying alt mode pin configurations (or possibly require non-UCSI interface to be created to replace removed feature) (b) Change Linux driver's alt mode comparison function Given that the proposed patch modification is a few lines of code and increases number of PPMs supported, can we re-consider it for this series of patches? > > > Thanks, > > -- > heikki Thanks, Michael nvpublic
