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 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. 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. 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. Thanks, -- heikki
