Re: [RESEND RFC 18/18] drm/display/dp_mst: Move all payload info into the atomic state

[Lyude Paul]

On Mon, 2022-08-08 at 10:02 +0000, Lin, Wayne wrote:
> [Public]
> 
> 
> 
> > -----Original Message-----
> > From: Lyude Paul <lyude@xxxxxxxxxx>
> > Sent: Thursday, August 4, 2022 4:28 AM
> > To: Lin, Wayne <Wayne.Lin@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> > nouveau@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Zuo, Jerry
> > <Jerry.Zuo@xxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxx>; Imre Deak
> > <imre.deak@xxxxxxxxx>; Daniel Vetter <daniel.vetter@xxxxxxxx>; Sean Paul
> > <sean@xxxxxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>; Li, Sun
> > peng (Leo) <Sunpeng.Li@xxxxxxx>; Siqueira, Rodrigo
> > <Rodrigo.Siqueira@xxxxxxx>; Deucher, Alexander
> > <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> > <Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>; David
> > Airlie <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Jani Nikula
> > <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> > <joonas.lahtinen@xxxxxxxxxxxxxxx>; Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>;
> > Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>; Ben Skeggs
> > <bskeggs@xxxxxxxxxx>; Karol Herbst <kherbst@xxxxxxxxxx>; Kazlauskas,
> > Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Li, Roman
> > <Roman.Li@xxxxxxx>; Shih, Jude <Jude.Shih@xxxxxxx>; Simon Ser
> > <contact@xxxxxxxxxxx>; Lakha, Bhawanpreet
> > <Bhawanpreet.Lakha@xxxxxxx>; Mikita Lipski <mikita.lipski@xxxxxxx>;
> > Claudio Suarez <cssk@xxxxxxxx>; Chen, Ian <Ian.Chen@xxxxxxx>; Colin Ian
> > King <colin.king@xxxxxxxxx>; Wu, Hersen <hersenxs.wu@xxxxxxx>; Liu,
> > Wenjing <Wenjing.Liu@xxxxxxx>; Lei, Jun <Jun.Lei@xxxxxxx>; Strauss,
> > Michael <Michael.Strauss@xxxxxxx>; Ma, Leo <Hanghong.Ma@xxxxxxx>;
> > Thomas Zimmermann <tzimmermann@xxxxxxx>; José Roberto de Souza
> > <jose.souza@xxxxxxxxx>; He Ying <heying24@xxxxxxxxxx>; Anshuman
> > Gupta <anshuman.gupta@xxxxxxxxx>; Andi Shyti
> > <andi.shyti@xxxxxxxxxxxxxxx>; Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>;
> > Juston Li <juston.li@xxxxxxxxx>; Sean Paul <seanpaul@xxxxxxxxxxxx>;
> > Fernando Ramos <greenfoo@xxxxxx>; Luo Jiaxing
> > <luojiaxing@xxxxxxxxxx>; Javier Martinez Canillas <javierm@xxxxxxxxxx>;
> > open list <linux-kernel@xxxxxxxxxxxxxxx>; open list:INTEL DRM DRIVERS
> > <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>
> > 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 +0000, 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