Do we really need to increase/decrease MST VC payloads?

Thu May 5 02:30:21 UTC 2022

[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 at redhat.com>
> Sent: Wednesday, May 4, 2022 5:17 PM
> To: Lin, Wayne <Wayne.Lin at amd.com>; Wentland, Harry
> <Harry.Wentland at amd.com>; amd-gfx at lists.freedesktop.org; Zuo, Jerry
> <Jerry.Zuo at amd.com>
> 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:
> 
>         |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