Hi Heikki,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Tuesday, February 5, 2019 7:24 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 Tue, Feb 05, 2019 at 02:20:34AM +0000, Michael Hsu wrote:
> > > 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.
>
> The spec does not say anything about how that MID field should be interpreted,
> and that is the problem here.
>
> The core problem here is the UCSI spec. It really needs a new version that
> considers alternate modes a bit better.
>
Agreed. UCSI spec does not specify how the connector alt mode table (returned by UCSI GET_ALTERNATE_MODES cmd with recipient==connector) can be matched with the partner alt mode table (returned by UCSI GET_ALTERNATE_MODES cmd with recipient==SOP).
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.
How about this solution... given that different ucsi ppms are allowed by ambiguous spec to have different interpretations of the connector alt mode tables' ModeID values, there should be an entry in
struct ucsi_ppm
{
...
+ int (*match_altmode_idx)(struct ucsi_ppm *, int partner_svid, int partner_mode_vdo); /* returns connector alt mode index */
};
This structure makes sense to have this function added, since in addition to the existing device-specific cmd() / sync() functions, this additional function of alt mode index matching also varies between devices.
> > 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.
>
> I don't think you can use the VDO value as a mask even with DP alt mode (let
> alone with certain other alt modes) because you need to be able to match bits
> 15:8 to bits 23:16 and vise versa in some cases.
> The matching needs to be alt mode specific in any case and I think that partially
> renders the "masks" useless.
>
> Regardless of that, you are missing the point here. The problem is that your
> PPM is exposing bit masks in those MID fields, while others are exposing the
> connector DP capabilities. There is a difference in the behaviour depending on
> PPM implementation. That's not a small problem.
>
> Perhaps equally importantly, you are using two different kinds of values in that
> same data structure depending on the request - with partners the data
> structure gives you the capabilities VDO, but with connectors you get
> something else - i.e. the returned data structure is in practice not always the
> same. You can't just change the data structure like that just when it's
> convenient for you. You only do something like that if all the alternative data
> structures are documented in some standard. That's not the case here.
>
> You have also made that hack just for the sake of one limitation in UCSI. By
> corrupting the data structure in connector case you will only know the pin
> assignment, but that you can in practice guess in case of DP, and we can even
> figure it out for certainty if needed by communicating with GPU drivers from
> the UCSI DP driver. Much bigger limitation in UCSI is that there is no way to see
> the Attention VDM payloads, let alone get an event when the partner send the
> Attention VDM. In case of DP, we can't see the DP Status VDO. On some
> platforms we could have really used the DP Status and get the Attention events
> because of HPD.
>
> > > 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
>
> If that is the case, then the UCSI code has been mixed in with the state machine
> code, and there is no real structure in the firmware code. I don't believe that
> that is the case. I'm pretty sure that changing the values that the PPM exposes
> to the OPM in the UCSI data structures requires the minimum amount of effort.
>
> > - requires QA requalification of PPM with other display monitors
>
> Now why would that be required? The actual SVID specific communication with
> the partners has not changed. There should be absolutely no changes to the
> state machines, even the policies will stay the same.
> The OPM interface simply can't be critical. Let's not forget:
>
> "The PPM is expected to function without any OS interaction"
>
> > - changing internal tools to avoid querying alt mode pin configurations
> > (or possibly require non-UCSI interface to be created to replace removed
> > feature)
>
> That sounds to me like your tools are not using the ABI defined in Linux kernel:
>
> Documentation/ABI/testing/sysfs-driver-typec-displayport
>
Actually, our tools predated the adoption of this ABI. The previous sysfs for typec had generic (protocol independent) "active" nodes which directly matched the UCSI CAM index.
> I think you need to update your tools regardless how this problem is solved.
>
> > (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?
>
> We have to make sure all the options have been considered before accepting a
> workaround like that. Once it's part of mainline, you may be off the hook, but
> me and the community are not.
>
> Even if there's no way to get rid of that mask hack, it has to be handled
> differently (in the UCSI DP driver) in any case. If we really have to accept it,
> then I do want a separate patch for it with a good explanation why it's needed
> in the commit message.
>
> Before we do anything, let's just look at our options:
>
OK, as mentioned above, we don't need the mask hack if the ucsi_ppm is allowed to supply a device-specifc function for alt mode index matching between the connector alt mode table and the partner alt mode table.
Another way to summarize the issue:
- UCSI spec requires connector alt mode index to select /deselect alt mode
- Linux displayport ABI exposes partner alt mode sysfs nodes and needs to translate partner alt mode index to UCSI's connector alt mode index
> If you need certainty regarding the selected pin-assignment, then I would
> propose initially that we communicate with the GPU/DP driver from UCSI DP
> driver. Those drivers can tell us how many DP lanes are in use, and that should
> be enough to tell are we using C or D.
>
For our device, we have certainty since the GET_CURRENT_CAM returns a different index depending on which DisplayPort pin assignment is active. But, that's internal to our device and we need not expect other devices to also reveal its current pin assignment.
> The HPD problem I mentioned above was solved by supplying the operating
> system a custom event mimicking HPD IRQ. The generic UCSI code was not
> affected by that at all. How about if you do something similar?
>
This seems like a side channel communication, and we wanted to not create extra channels - which was why we arranged our UCSI connector alt mode table the way we did.
> You would just need to be able to see the content of the Configuration VDO that
> the PPM used with the partner in your driver (ucsi_ccg,c).
> Is that an option?
>
> Note, we will in all these cases need to tweak the UCSI drivers a little, but that's
> not the issue.
>
>
> thanks,
>
> --
> heikki
Thanks,
Michael
nvpublic
