Some good news: I actually came up with a way of handling this in the new MST
code pretty nicely, so I think we should be able to move forward without
having to disable this (although it would be very nice to know whether or not
this is necessary for amdgpu to work, since it'd still be nice to split this
into separate changes to make reviewing my big MST patch series easier. More
comments down below:
On Mon, 2022-05-02 at 18:40 -0400, Lyude Paul wrote:
> Hi! So I kinda hate to ask this, but finding this in amdgpu completely took
> me
> by surprise and unfortunately is (while technically correct) an enormous
> pain
> and not really necessary as far as I'm aware.
>
> So: it seems that amdgpu is currently the only driver that not only
> allocates/deallocates payloads, but it also increases/decreases the size of
> payloads as well. This was added in:
>
> d740e0bf8ed4 ("drm/amd/display: Add DP 2.0 MST DC Support")
>
> This is fine, except that it's totally not at all a situation that the
> current
> payload management code we have (which, is the first place this should have
> been implemented) knows how to handle, because every other driver simply
> allocates/releases payloads. Having to increase the size of payloads means
> that we no longer can count on the payload table being contiguous, and means
> we have to resort to coming up with a more complicated solution to actually
> do
> payload management atomically.
>
> I'm willing to try to do that (it should be doable by using bitmasks to
> track
> non-contiguous allocated slots), but considering:
> * This isn't actually needed for DP 2.0 to work as far as I'm aware (if I'm
> wrong please correct me! but I see no issue with just deallocating and
> allocating payloads). It's nice to have, but not necessary.
> * This was from the DSC MST series, which included a lot of code that I
> mentioned at the time needed to not live in amdgpu and be moved into
> other
> helpers. That hasn't really happened yet, and there are no signs of it
> happening from amd's side - and I just plain do not want to have to be
> the
> person to do that when I can help it. Going through amdgpu takes a
> serious
> amount of energy because of all of the abstraction layers, enough so I
> honestly didn't even notice this VC table change when I looked at the
> series this was from. (Or maybe I did, but didn't fully grasp what was
> changing at the time :\).
> * Also on that note, are we even sure that this works with subsequent VC
> changes? E.g. has anyone tested this with physical hardware? I don't know
> that I actually have the hardware to test this out, but
> drm_dp_update_payload*() absolutely doesn't understand non-contiguous
> payloads which I would think could lead to the VCPI start slots getting
> miscalculated if a payload increase/decreases (happy to give further
> explanation on this if needed). Note if this is the case, please hold off
> a
> bit before trying to fix it and consider just not doing this for the time
> being.
Sorry for being a bit vague with this! I typed this email at the end of
the workday and didn't feel like going super into detail on this. So I
guess I'll do that now (hopefully I didn't misread the MST spec
somewhere):
For reference: the issue I was mentioning here was regarding the fact
that the current payload management code we have doesn't keep track of
exactly which VC slots are in use by a payload at any given time - only
the starting slots and total slot counts of each payload. Which means
that once we increase a MST payload, since the additional VC slots will
be put at the end of the VC table regardless of the start of the
payload. As such, it's possible that said payload will no longer be
contiguous. An example, note for simplicity sake this example pretends
we only have 8 VC slots instead of 64:
Payloads: #1 == 2 slots, #2 == 1 slot, #3 == 2 slots
VC table looks like this, where each number corresponds to a VCPI and
X means unused:
|1 1 2 3 3 X X X|
We decide we need to increase payload #1 from 2 slots to 3. The MST spec
mandates that new slots always are added to the end, so we end up with
this surprising VC table:
|1 1 2 3 3 1 X X|
Now, let's say we increase payload #2 to 2 slots:
|1 1 2 3 3 1 2 X|
Surprise - the monitor for payload #1 was unplugged, so we need to
remove it's payload. Note the MST spec doesn't allow holes between
allocations, and makes branches repsonsible for automatically moving
payloads to fill in the gaps which we have to keep track of locally.
Normally the handling of holes would be fine, as our current payload
management code loops through each payload to fill in any holes. But
these payloads aren't contiguous and we only kept track of their start
slots and total slot counts. So we would end up thinking we now have a
VC table like this:
|2 2 3 3 X X X X|
But that's wrong, in reality the table will look like this on the MST
hub:
|2 3 3 2 X X X X|
Now things are broken, because we now have the wrong start slot for
payload #3.
Just figured I'd clarify :). if anyone is curious, I ended up fixing
this by adding a bitmask of assigned VC slots to our atomic payload
struct - and using that to keep track of the current start slots for
each payload. Since we have a max of 64 VC slots which fits into a u64,
this works rather elegantly!
>
> I'd /very/ much like to just disable this behavior for the time being (not
> the
> whole commit for DP 2.0 support since that'd be unreasonable, just the
> increase/decrease payload changes), and eventually have someone just
> implement
> this the proper way by adding support for this into the MST helpers and
> teaching them how to handle non-contiguous payloads. I'm happy to leave as
> much of the code intact as possible (maybe with #if 0 or whatever), and
> ideally just add some code somewhere to avoid increasing/decreasing payloads
> without a full modeset.
>
> FWIW, the WIP of my atomic MST work is here:
>
> https://gitlab.freedesktop.org/lyudess/linux/-/commits/wip/mst-atomic-only-v1
>
> I already have i915 and nouveau working with these changes JFYI.
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
