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