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

[Lyude Paul]

On Thu, 2022-05-05 at 10:02 +0000, Lin, Wayne wrote:
> 
> 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.

I'm not sure about that tbh, since the example you gave also still mentions VC
#1 which is supposed to be disabled here.

FWIW, I was alerted to this pecularity when I noticed Figure 2-93 from chapter
2.6.5 of the DP 2.0 spec. Notice that on the diagram it mentions time slots 1-
20 allocated to VC1, time slots 21-32 being allocated to VC2, and then
(confusingly in a different color) mentions that time slots 33-47 are
allocated to VC1 again.

At first I honestly thought this was a typo in the spec, but it's done
multiple times and it _does_ actually make sense when you think about it for a
little while. Consider that the advantage of having VC allocations work this
way would be that you're able to increase the time slots allocated to a VC
without having to even change their start slots - meaning those other payloads
don't need to be disrupted in any way. This is only as far as I can tell if
you have non-contiguous payloads.

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

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat