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

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

[Lyude Paul]

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

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

[Lyude Paul]

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

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

[Zuo, Jerry]

[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@redhat.com>
> Sent: Wednesday, May 4, 2022 5:17 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; amd-gfx@lists.freedesktop.org; Zuo, Jerry
> <Jerry.Zuo@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&amp;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&amp;sdata=IBn%2BF5
> 0r9WIeUfG
> > 9MUGStbACr5Kolu3PB5K0dyiiYwg%3D&amp;reserved=0
> >
> > I already have i915 and nouveau working with these changes JFYI.
> 
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat

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

[Lin, Wayne]

[Public]

Hi Lyude,

Thanks for raising this!
Please see my comments inline : )
________________________________________
> From: Zuo, Jerry <Jerry.Zuo@amd.com>
> Sent: Thursday, May 5, 2022 10:30
> To: Lyude Paul; Lin, Wayne; Wentland, Harry; amd-gfx@lists.freedesktop.org
> 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@redhat.com>
> > Sent: Wednesday, May 4, 2022 5:17 PM
> > To: Lin, Wayne <Wayne.Lin@amd.com>; Wentland, Harry
> > <Harry.Wentland@amd.com>; amd-gfx@lists.freedesktop.org; Zuo, Jerry
> > <Jerry.Zuo@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:

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&amp;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&amp;sdata=IBn%2BF5
> > 0r9WIeUfG
> > > 9MUGStbACr5Kolu3PB5K0dyiiYwg%3D&amp;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

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

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

[Lin, Wayne]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Wednesday, May 11, 2022 6:14 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>;
> Wentland, Harry <Harry.Wentland@amd.com>; amd-
> gfx@lists.freedesktop.org
> Subject: Re: Do we really need to increase/decrease MST VC payloads?
> 
> 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.
> 
Thank you Lyude.

I think that should be just a typo since remaining 2 payload portions of the
"De-allocation" case at the bottom of that figure are represented in different
colors. Besides, I think we should just take the idea that it illustrates in the figure
for increasing/decreasing time lots. Which only increase extra slots continuously.  

I also consult with a branch device vendor, their DP Rx HW design only supports
1 start/end per stream. We have consistent consensus for this. However, the
most confident way is having VESA to clarify this.

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

Ideally, other payloads shouldn't get disrupted. Source will send out ACT symbols
to synchronize DP Rx with the new updated payload ID table. Operations on
DPCD 0x1C1 & 0x1C2 won't have physical signal take effect immediately. 

I think increasing time slots non-contiguously will have DP Rx to have more HW
registers and have more complicated design which will increase their cost.
> 
> >
> > > >
> > > >         |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&amp;data=05%7C01%7Cjerry.zuo%40amd.com%7Cf669121b53414c0dbc9
> > > > 40
> > > > >
> > > >
> 8da2e137e85%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6378
> > > > 729585141
> > > > >
> > > >
> 60092%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> u
> > > > MzIiLCJB
> > > > >
> > > >
> TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=IBn%2BF5
> > > > 0r9WIeUfG
> > > > > 9MUGStbACr5Kolu3PB5K0dyiiYwg%3D&amp;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
--
Regards,
Wayne Lin