[Public]
Hi Lyude,
Thanks for raising this!
Please see my comments inline : )
________________________________________
> From: Zuo, Jerry <Jerry.Zuo@xxxxxxx>
> Sent: Thursday, May 5, 2022 10:30
> To: Lyude Paul; Lin, Wayne; Wentland, Harry; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: Do we really need to increase/decrease MST VC payloads?
>
> [AMD Official Use Only - General]
>
> Hi Lyude:
>
> Sorry for replying late.
>
> 1. The payload increase/decrease routines are not for DP2.
> 2. mst_bw_update is not used in amdgpu_dm, so those two functions are not getting used for now. I leave it there simply for future possible hook up.
>
> Regards,
> Jerry
>
> > -----Original Message-----
> > From: Lyude Paul <lyude@xxxxxxxxxx>
> > Sent: Wednesday, May 4, 2022 5:17 PM
> > To: Lin, Wayne <Wayne.Lin@xxxxxxx>; Wentland, Harry
> > <Harry.Wentland@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Zuo, Jerry
> > <Jerry.Zuo@xxxxxxx>
> > Subject: Re: Do we really need to increase/decrease MST VC payloads?
> >
> > 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:
I think this shouldn't happen. Please refer to the figure 2-98 of DP spec 1.4.
The payload id table should be changed to below I think.
|1 1 1 2 3 3 X X|
I skimmed over a bit of the implementation in drm. I think it should work fine
except that we have to revise function drm_dp_init_vcpi() a bit.
Since increasing/decreasing time slots should still use the same payload id,
we should add logic to skip trying to assign a new payload id under this case.
> >
> > |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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl
> > > ab.freedesktop.org%2Flyudess%2Flinux%2F-%2Fcommits%2Fwip%2Fmst-
> > atomic-
> > > only-
> > v1&data=05%7C01%7Cjerry.zuo%40amd.com%7Cf669121b53414c0dbc9
> > 40
> > >
> > 8da2e137e85%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6378
> > 729585141
> > >
> > 60092%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> > MzIiLCJB
> > >
> > TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=IBn%2BF5
> > 0r9WIeUfG
> > > 9MUGStbACr5Kolu3PB5K0dyiiYwg%3D&reserved=0
> > >
> > > I already have i915 and nouveau working with these changes JFYI.
> >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
--
Regards,
Wayne Lin
