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.
> 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
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:
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.
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?
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
