Re: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info into the atomic state
On Mon, 2022-08-08 at 10:02 +, Lin, Wayne wrote:
> [Public]
>
>
>
> > -Original Message-
> > From: Lyude Paul
> > Sent: Thursday, August 4, 2022 4:28 AM
> > To: Lin, Wayne ; dri-de...@lists.freedesktop.org;
> > nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
> > Cc: Ville Syrjälä ; Zuo, Jerry
> > ; Jani Nikula ; Imre Deak
> > ; Daniel Vetter ; Sean Paul
> > ; Wentland, Harry ; Li, Sun
> > peng (Leo) ; Siqueira, Rodrigo
> > ; Deucher, Alexander
> > ; Koenig, Christian
> > ; Pan, Xinhui ; David
> > Airlie ; Daniel Vetter ; Jani Nikula
> > ; Joonas Lahtinen
> > ; Rodrigo Vivi ;
> > Tvrtko Ursulin ; Ben Skeggs
> > ; Karol Herbst ; Kazlauskas,
> > Nicholas ; Li, Roman
> > ; Shih, Jude ; Simon Ser
> > ; Lakha, Bhawanpreet
> > ; Mikita Lipski ;
> > Claudio Suarez ; Chen, Ian ; Colin Ian
> > King ; Wu, Hersen ; Liu,
> > Wenjing ; Lei, Jun ; Strauss,
> > Michael ; Ma, Leo ;
> > Thomas Zimmermann ; José Roberto de Souza
> > ; He Ying ; Anshuman
> > Gupta ; Andi Shyti
> > ; Ashutosh Dixit ;
> > Juston Li ; Sean Paul ;
> > Fernando Ramos ; Luo Jiaxing
> > ; Javier Martinez Canillas ;
> > open list ; open list:INTEL DRM DRIVERS
> >
> > Subject: Re: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info
> > into the atomic state
> >
> > On Tue, 2022-07-05 at 09:10 +, Lin, Wayne wrote:
> > > > +struct drm_dp_mst_port;
> > > > +
> > > > /* DP MST stream allocation (payload bandwidth number) */
> > > > struct dc_dp_mst_stream_allocation {
> > > > uint8_t vcp_id;
> > > > /* number of slots required for the DP stream in
> > > > * transport packet */
> > > > uint8_t slot_count;
> > > > + /* The MST port this is on, this is used to associate DC MST
> > > > + payloads
> > > > with their
> > > > + * respective DRM payloads allocations, and can be ignored on non-
> > > > Linux.
> > > > + */
> > >
> > > Is it necessary for adding this new member? Since this is for setting
> > > the DC HW and not relating to drm.
> >
> > I don't entirely know, honestly. The reasons I did it:
> >
> > * Mapping things from DRM to DC and from DC to DRM is really confusing for
> >outside contributors like myself, so it wasn't even really clear to me if
> >there was another way to reconstruct the DRM context from the spots
> > where
> >we call from DC up to DM (not a typo, see next point).
> > * These DC structs for MST are already layer mixing as far as I can tell,
> >just not in an immediately obvious way. While this struct itself is for
> > DC,
> >there's multiple spots where we pass the DC payload structs down from
> > DM to
> >DC, then pass them back up from DC to DM and have to figure out how to
> >reconstruct the DRM context that we actually need to use the MST helpers
> >from that. So, that kind of further complicates the confusion of where
> >layers should be separated.
> > * As far as I'm aware with C there shouldn't be any issue with adding a
> >pointer to a struct whose contents are undefined. IMHO, this is also
> >preferable to just using void* because then at least you get some hint as
> >to the actual type of the data and avoid the possibility of casting it to
> >the wrong type. So tl;dr, on any platform even outside of Linux with a
> >reasonably compliant compiler this should still build just fine. It'll
> > even
> >give you the added bonus of warning people if they try to access the
> >contents of this member in DC on non-Linux platforms. If void* is
> > preferred
> >though I'm fine with switching it to that.
> >
> > --
> > Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
>
> Hi Lyude,
>
> Thanks for your time!
> I was thinking that our DC just mainly takes care of HW related programming.
> Struct dc_dp_mst_stream_allocation is only used to construct a copy of the
> virtual
> channel payload ID and slots count of payload allocation table determined by
> dm/drm. ID and slots are only things required for programming HW registers.
> I think there shouldn't be any spots to try to construct the DRM context from
> this dc struct since there is no such concept in HW level. Our HW should only
> take care of local DP link and it doesn't have overall topology info.
Looking at the code I wrote again and realizing I slightly misspoke, looking
at the code again I think I probably can drop this. It's likely I just got
totally lost with the DC codebase and thought this was necessary when it
wasn't. Will drop in the respin
>
> Thanks!
>
> Regards,
> Wayne
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
RE: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info into the atomic state
[Public]
> -Original Message-
> From: Lyude Paul
> Sent: Thursday, August 4, 2022 4:28 AM
> To: Lin, Wayne ; dri-de...@lists.freedesktop.org;
> nouv...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä ; Zuo, Jerry
> ; Jani Nikula ; Imre Deak
> ; Daniel Vetter ; Sean Paul
> ; Wentland, Harry ; Li, Sun
> peng (Leo) ; Siqueira, Rodrigo
> ; Deucher, Alexander
> ; Koenig, Christian
> ; Pan, Xinhui ; David
> Airlie ; Daniel Vetter ; Jani Nikula
> ; Joonas Lahtinen
> ; Rodrigo Vivi ;
> Tvrtko Ursulin ; Ben Skeggs
> ; Karol Herbst ; Kazlauskas,
> Nicholas ; Li, Roman
> ; Shih, Jude ; Simon Ser
> ; Lakha, Bhawanpreet
> ; Mikita Lipski ;
> Claudio Suarez ; Chen, Ian ; Colin Ian
> King ; Wu, Hersen ; Liu,
> Wenjing ; Lei, Jun ; Strauss,
> Michael ; Ma, Leo ;
> Thomas Zimmermann ; José Roberto de Souza
> ; He Ying ; Anshuman
> Gupta ; Andi Shyti
> ; Ashutosh Dixit ;
> Juston Li ; Sean Paul ;
> Fernando Ramos ; Luo Jiaxing
> ; Javier Martinez Canillas ;
> open list ; open list:INTEL DRM DRIVERS
>
> Subject: Re: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info
> into the atomic state
>
> On Tue, 2022-07-05 at 09:10 +, Lin, Wayne wrote:
> > > +struct drm_dp_mst_port;
> > > +
> > > /* DP MST stream allocation (payload bandwidth number) */
> > > struct dc_dp_mst_stream_allocation {
> > > uint8_t vcp_id;
> > > /* number of slots required for the DP stream in
> > > * transport packet */
> > > uint8_t slot_count;
> > > + /* The MST port this is on, this is used to associate DC MST
> > > + payloads
> > > with their
> > > + * respective DRM payloads allocations, and can be ignored on non-
> > > Linux.
> > > + */
> >
> > Is it necessary for adding this new member? Since this is for setting
> > the DC HW and not relating to drm.
>
> I don't entirely know, honestly. The reasons I did it:
>
> * Mapping things from DRM to DC and from DC to DRM is really confusing for
>outside contributors like myself, so it wasn't even really clear to me if
>there was another way to reconstruct the DRM context from the spots
> where
>we call from DC up to DM (not a typo, see next point).
> * These DC structs for MST are already layer mixing as far as I can tell,
>just not in an immediately obvious way. While this struct itself is for DC,
>there's multiple spots where we pass the DC payload structs down from
> DM to
>DC, then pass them back up from DC to DM and have to figure out how to
>reconstruct the DRM context that we actually need to use the MST helpers
>from that. So, that kind of further complicates the confusion of where
>layers should be separated.
> * As far as I'm aware with C there shouldn't be any issue with adding a
>pointer to a struct whose contents are undefined. IMHO, this is also
>preferable to just using void* because then at least you get some hint as
>to the actual type of the data and avoid the possibility of casting it to
>the wrong type. So tl;dr, on any platform even outside of Linux with a
>reasonably compliant compiler this should still build just fine. It'll even
>give you the added bonus of warning people if they try to access the
>contents of this member in DC on non-Linux platforms. If void* is preferred
>though I'm fine with switching it to that.
>
> --
> Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Hi Lyude,
Thanks for your time!
I was thinking that our DC just mainly takes care of HW related programming.
Struct dc_dp_mst_stream_allocation is only used to construct a copy of the
virtual
channel payload ID and slots count of payload allocation table determined by
dm/drm. ID and slots are only things required for programming HW registers.
I think there shouldn't be any spots to try to construct the DRM context from
this dc struct since there is no such concept in HW level. Our HW should only
take care of local DP link and it doesn't have overall topology info.
Thanks!
Regards,
Wayne
Re: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info into the atomic state
On Tue, 2022-07-05 at 09:10 +, Lin, Wayne wrote:
> > +struct drm_dp_mst_port;
> > +
> > /* DP MST stream allocation (payload bandwidth number) */
> > struct dc_dp_mst_stream_allocation {
> > uint8_t vcp_id;
> > /* number of slots required for the DP stream in
> > * transport packet */
> > uint8_t slot_count;
> > + /* The MST port this is on, this is used to associate DC MST payloads
> > with their
> > + * respective DRM payloads allocations, and can be ignored on non-
> > Linux.
> > + */
>
> Is it necessary for adding this new member? Since this is for setting the DC
> HW and not relating to drm.
I don't entirely know, honestly. The reasons I did it:
* Mapping things from DRM to DC and from DC to DRM is really confusing for
outside contributors like myself, so it wasn't even really clear to me if
there was another way to reconstruct the DRM context from the spots where
we call from DC up to DM (not a typo, see next point).
* These DC structs for MST are already layer mixing as far as I can tell,
just not in an immediately obvious way. While this struct itself is for DC,
there's multiple spots where we pass the DC payload structs down from DM to
DC, then pass them back up from DC to DM and have to figure out how to
reconstruct the DRM context that we actually need to use the MST helpers
from that. So, that kind of further complicates the confusion of where
layers should be separated.
* As far as I'm aware with C there shouldn't be any issue with adding a
pointer to a struct whose contents are undefined. IMHO, this is also
preferable to just using void* because then at least you get some hint as
to the actual type of the data and avoid the possibility of casting it to
the wrong type. So tl;dr, on any platform even outside of Linux with a
reasonably compliant compiler this should still build just fine. It'll even
give you the added bonus of warning people if they try to access the
contents of this member in DC on non-Linux platforms. If void* is preferred
though I'm fine with switching it to that.
--
Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
RE: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info into the atomic state
[Public] > -Original Message- > From: Lyude Paul > Sent: Wednesday, June 8, 2022 3:30 AM > To: dri-de...@lists.freedesktop.org; nouv...@lists.freedesktop.org; amd- > g...@lists.freedesktop.org > Cc: Lin, Wayne ; Ville Syrjälä > ; Zuo, Jerry ; Jani Nikula > ; Imre Deak ; Daniel Vetter > ; Sean Paul ; Wentland, Harry > ; Li, Sun peng (Leo) ; > Siqueira, Rodrigo ; Deucher, Alexander > ; Koenig, Christian > ; Pan, Xinhui ; David > Airlie ; Daniel Vetter ; Jani Nikula > ; Joonas Lahtinen > ; Rodrigo Vivi ; > Tvrtko Ursulin ; Ben Skeggs > ; Karol Herbst ; Kazlauskas, > Nicholas ; Li, Roman > ; Shih, Jude ; Simon Ser > ; Lakha, Bhawanpreet > ; Mikita Lipski ; > Claudio Suarez ; Chen, Ian ; Colin Ian > King ; Wu, Hersen ; Liu, > Wenjing ; Lei, Jun ; Strauss, > Michael ; Ma, Leo ; > Thomas Zimmermann ; José Roberto de Souza > ; He Ying ; Anshuman > Gupta ; Andi Shyti > ; Ashutosh Dixit ; > Juston Li ; Sean Paul ; > Fernando Ramos ; Luo Jiaxing > ; Javier Martinez Canillas ; > open list ; open list:INTEL DRM DRIVERS > > Subject: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info into > the atomic state > > Now that we've finally gotten rid of the non-atomic MST users leftover in > the kernel, we can finally get rid of all of the legacy payload code we > have and move as much as possible into the MST atomic state structs. The > main purpose of this is to make the MST code a lot less confusing to work > on, as there's a lot of duplicated logic that doesn't really need to be > here. As well, this should make introducing features like fallback link > retraining and DSC support far easier. > > Since the old payload code was pretty gnarly and there's a Lot of changes > here, I expect this might be a bit difficult to review. So to make things > as easy as possible for reviewers, I'll sum up how both the old and new > code worked here (it took me a while to figure this out too!). > > The old MST code basically worked by maintaining two different payload > tables - proposed_vcpis, and payloads. proposed_vcpis would hold the > modified payload we wanted to push to the topology, while payloads held > the > payload table that was currently programmed in hardware. Modifications to > proposed_vcpis would be handled through drm_dp_allocate_vcpi(), > drm_dp_mst_deallocate_vcpi(), and drm_dp_mst_reset_vcpi_slots(). Then, > they > would be pushed via drm_dp_mst_update_payload_step1() and > drm_dp_mst_update_payload_step2(). > > Furthermore, it's important to note how adding and removing VC payloads > actually worked with drm_dp_mst_update_payload_step1(). When a VC > payload > is removed from the VC table, all VC payloads which come after the removed > VC payload's slots must have their time slots shifted towards the start of > the table. The old code handles this by looping through the entire payload > table and recomputing the start slot for every payload in the topology from > scratch. While very much overkill, this ends up doing the right thing > because we always order the VCPIs for payloads from first to last starting > timeslot. > > It's important to also note that drm_dp_mst_update_payload_step2() isn't > actually limited to updating a single payload - the driver can use it to > queue up multiple payload changes so that as many of them can be sent as > possible before waiting for the ACT. Hi Lyude, I have concern for updating payload table multiple times for multiple payload changes before sending the ACT. Also consult with one branch vendor, they say their fw will expect receiving one ALLOCATE_PAYLOAD after setting DPCD 002c0h bit 0 (VC payload ID table updated). Thanks! > > drm_dp_mst_update_payload_step2() is pretty self explanatory and > basically > the same between the old and new code, save for the fact we don't have a > second step for deleting payloads anymore -and thus rename it to > drm_dp_mst_add_payload_step2(). > > The new payload code stores all of the current payload info within the MST > atomic state and computes as much of the state as possible ahead of time. > This has the one exception of the starting timeslots for payloads, which > can't be determined at atomic check time since the starting time slots will > vary depending on what order CRTCs are enabled in the atomic state - which > varies from driver to driver. These are still stored in the atomic MST > state, but are only copied from the old MST state during atomic commit > time. Likewise, this is when new start slots are determined. > > Adding/removing payloads now works much more closely to how things are > described in the spec. When we delete a payload, we loop through the > current list of payloads and update the start slots for any payloads whose > time slots came after the payload we just deleted. Determining the starting > time slots for new payloads being added is done by simply keeping track of > where the end of the VC table is in > drm_dp_mst_topology_mgr->next_start_slot. Additionally, it's worth noting >
