Nouveau - Aug 2022 - [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

[Public]


> -----Original Message-----
> From: Lyude Paul <lyude at redhat.com>
> Sent: Thursday, August 4, 2022 4:28 AM
> To: Lin, Wayne <Wayne.Lin at amd.com>; dri-devel at
lists.freedesktop.org;
> nouveau at lists.freedesktop.org; amd-gfx at lists.freedesktop.org
> Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>; Zuo, Jerry
> <Jerry.Zuo at amd.com>; Jani Nikula <jani.nikula at intel.com>;
Imre Deak
> <imre.deak at intel.com>; Daniel Vetter <daniel.vetter at
ffwll.ch>; Sean Paul
> <sean at poorly.run>; Wentland, Harry <Harry.Wentland at
amd.com>; Li, Sun
> peng (Leo) <Sunpeng.Li at amd.com>; Siqueira, Rodrigo
> <Rodrigo.Siqueira at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; Pan, Xinhui <Xinhui.Pan at
amd.com>; David
> Airlie <airlied at linux.ie>; Daniel Vetter <daniel at
ffwll.ch>; Jani Nikula
> <jani.nikula at linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen at linux.intel.com>; Rodrigo Vivi <rodrigo.vivi
at intel.com>;
> Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>; Ben Skeggs
> <bskeggs at redhat.com>; Karol Herbst <kherbst at redhat.com>;
Kazlauskas,
> Nicholas <Nicholas.Kazlauskas at amd.com>; Li, Roman
> <Roman.Li at amd.com>; Shih, Jude <Jude.Shih at amd.com>; Simon
Ser
> <contact at emersion.fr>; Lakha, Bhawanpreet
> <Bhawanpreet.Lakha at amd.com>; Mikita Lipski <mikita.lipski at
amd.com>;
> Claudio Suarez <cssk at net-c.es>; Chen, Ian <Ian.Chen at
amd.com>; Colin Ian
> King <colin.king at intel.com>; Wu, Hersen <hersenxs.wu at
amd.com>; Liu,
> Wenjing <Wenjing.Liu at amd.com>; Lei, Jun <Jun.Lei at
amd.com>; Strauss,
> Michael <Michael.Strauss at amd.com>; Ma, Leo <Hanghong.Ma at
amd.com>;
> Thomas Zimmermann <tzimmermann at suse.de>; Jos? Roberto de Souza
> <jose.souza at intel.com>; He Ying <heying24 at huawei.com>;
Anshuman
> Gupta <anshuman.gupta at intel.com>; Andi Shyti
> <andi.shyti at linux.intel.com>; Ashutosh Dixit <ashutosh.dixit at
intel.com>;
> Juston Li <juston.li at intel.com>; Sean Paul <seanpaul at
chromium.org>;
> Fernando Ramos <greenfoo at u92.eu>; Luo Jiaxing
> <luojiaxing at huawei.com>; Javier Martinez Canillas <javierm at
redhat.com>;
> open list <linux-kernel at vger.kernel.org>; open list:INTEL DRM
DRIVERS
> <intel-gfx at lists.freedesktop.org>
> 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.

Thanks!

Regards,
Wayne