On Wed, 2021-08-04 at 07:13 +0000, Lin, Wayne wrote:
> [Public]
>
> > -----Original Message-----
> > From: Lyude Paul <lyude@xxxxxxxxxx>
> > Sent: Wednesday, August 4, 2021 8:09 AM
> > To: Lin, Wayne <Wayne.Lin@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Wentland, Harry <
> > Harry.Wentland@xxxxxxx>; Zuo, Jerry
> > <Jerry.Zuo@xxxxxxx>; Wu, Hersen <hersenxs.wu@xxxxxxx>; Juston Li <
> > juston.li@xxxxxxxxx>; Imre Deak <imre.deak@xxxxxxxxx>;
> > Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Wentland, Harry <
> > Harry.Wentland@xxxxxxx>; Daniel Vetter <daniel.vetter@xxxxxxxx>;
> > Sean Paul <sean@xxxxxxxxxx>; Maarten Lankhorst <
> > maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>;
> > Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie <airlied@xxxxxxxx>;
> > Daniel Vetter <daniel@xxxxxxxx>; Deucher,
> > Alexander <Alexander.Deucher@xxxxxxx>; Siqueira, Rodrigo <
> > Rodrigo.Siqueira@xxxxxxx>; Pillai, Aurabindo
> > <Aurabindo.Pillai@xxxxxxx>; Eryk Brol <eryk.brol@xxxxxxx>; Bas
> > Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>; Cornij, Nikola
> > <Nikola.Cornij@xxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxx>; Manasi
> > Navare <manasi.d.navare@xxxxxxxxx>; Ankit Nautiyal
> > <ankit.k.nautiyal@xxxxxxxxx>; José Roberto de Souza
> > <jose.souza@xxxxxxxxx>; Sean Paul <seanpaul@xxxxxxxxxxxx>; Ben Skeggs
> > <bskeggs@xxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 2/4] drm/dp_mst: Only create connector for connected
> > end device
> >
> > On Tue, 2021-08-03 at 19:58 -0400, Lyude Paul wrote:
> > > On Wed, 2021-07-21 at 00:03 +0800, Wayne Lin wrote:
> > > > [Why]
> > > > Currently, we will create connectors for all output ports no matter
> > > > it's connected or not. However, in MST, we can only determine
> > > > whether an output port really stands for a "connector" till it is
> > > > connected and check its peer device type as an end device.
> > >
> > > What is this commit trying to solve exactly? e.g. is AMD currently
> > > running into issues with there being too many DRM connectors or
> > > something like that?
> > > Ideally this is behavior I'd very much like us to keep as-is unless
> > > there's good reason to change it.
> Hi Lyude,
> Really appreciate for your time to elaborate in such detail. Thanks!
>
> I come up with this commit because I observed something confusing when I was
> analyzing
> MST connectors' life cycle. Take the topology instance you mentioned below
>
> Root MSTB -> Output_Port 1 -> MSTB 1.1 ->Output_Port 1(Connected w/ display)
> | -
> >Output_Port 2 (Disconnected)
> -> Output_Port 2 -> MSTB 2.1 ->Output_Port 1
> (Disconnected)
> ->
> Output_Port 2 (Disconnected)
> Which is exactly the topology of Startech DP 1-to-4 hub. There are 3 1-to-2
> branch chips
> within this hub. With our MST implementation today, we'll create drm
> connectors for all
> output ports. Hence, we totally create 6 drm connectors here. However,
> Output ports of
> Root MSTB are not connected to a stream sink. They are connected with branch
> devices.
> Thus, creating drm connector for such port looks a bit strange to me and
> increases
> complexity to tracking drm connectors. My thought is we only need to create
> drm
> connector for those connected end device. Once output port is connected then
> we can
> determine whether to add on a drm connector for this port based on the peer
> device type.
> Hence, this commit doesn't try to break the locking logic but add more
> constraints when
> We try to add drm connector. Please correct me if I misunderstand anything
> here. Thanks!
Sorry-I will respond to this soon, some more stuff came up at work so it might
take me a day or two
> > >
> > > Some context here btw - there's a lot of subtleties with MST locking
> > > that isn't immediately obvious. It's been a while since I wrote this
> > > code, but if I recall correctly one of those subtleties is that trying
> > > to create/destroy connectors on the fly when ports change types
> > > introduces a lot of potential issues with locking and some very
> > > complicated state transitions. Note that because we maintain the
> > > topology as much as possible across suspend/resumes this means there's
> > > a lot of potential state transitions with drm_dp_mst_port and
> > > drm_dp_mst_branch we need to handle that would typically be impossible
> > > to run into otherwise.
> > >
> > > An example of this, if we were to try to prune connectors based on PDT
> > > on the fly: assume we have a simple topology like this
> > >
> > > Root MSTB -> Port 1 -> MSTB 1.1 (Connected w/ display)
> > > -> Port 2 -> MSTB 2.1
> > >
> > > We suspend the system, unplug MSTB 1.1, and then resume. Once the
> > > system starts reprobing, it will notice that MSTB 1.1 has been
> > > disconnected. Since we no longer have a PDT, we decide to unregister
> > > our connector. But there's a catch! We had a display connected to MSTB
> > > 1.1, so even after unregistering the connector it's going to stay
> > > around until userspace has committed a new mode with the connector
> > > disabled.
> > >
> > > Now - assuming we're still in the same spot in the resume processs,
> > > let's assume somehow MSTB 1.1 is suddenly plugged back in. Once we've
> > > finished responding to the hotplug event, we will have created a
> > > connector for it. Now we've hit a bug - userspace hasn't removed the
> > > previous zombie connector which means we have references to the
> > > drm_dp_mst_port in our atomic state and potentially also our payload
> > > tables (?? unsure about this one).
> >
> > Whoops. One thing I totally forgot to mention here: the reason this is a
> > problem is because we'd now have two drm_connectors
> > which both have the same drm_dp_mst_port pointer.
> >
> > >
> > > So then how do we manage to add/remove connectors for input connectors
> > > on the fly? Well, that's one of the fun normally-impossible state
> > > transitions I mentioned before. According to the spec input ports are
> > > always disconnected, so we'll never receive a CSN for them. This means
> I think input ports' DisplayPort_Device_Plug_Status field is still set to 1?
> But yes,
> according to DP1.4 spec 2.11.9.3, when MST device whose DPRX detected the
> connection status change shall broadcast CSN downstream only. Hence, we'll
> never
> receive a CSN for this case.
> > > in theory the only possible way we could have a connector go from
> > > being an input connector to an output connector connector would be if
> > > the entire topology was swapped out during suspend/resume, and the
> > > input/output ports in the two topologies topology happen to be in
> > > different places.
> > > Since we only have to reprobe once during resume before we get
> > > hotplugging enabled, we're guaranteed this state transition will only
> > > happen once in this state - which means the second replug I described
> > > in the previous paragraph can never happen.
> > >
> > > Note that while I don't actually know if there's topologies with input
> > > ports at indexes other than 0, since the specification isn't super
> > > clear on this bit we play it safe and assume it is possible.
> Based on DP1.4 spec 2.5.1. Physical input ports are assigned smaller port
> numbers than physical output ports. For concentrator product, if there are 2
> input ports of it's branch device, then their port numbers are port 0 & port
> 1
> which can refer to figure 2-122 of DP1.4.
> > >
> > > Anyway-this is -all- based off my memory, so please point out anything
> > > here that I've explained that doesn't make sense or doesn't seem
> > > correct :). It's totally possible I might have misremembered something.
> Thanks again Lyude! Much appreciated for your time and help! And please
> correct me if I misunderstand anything here : )
> > >
> > > >
> > > > In current code, we have chance to create connectors for output
> > > > ports connected with branch device and these are redundant connectors.
> > > > e.g.
> > > > StarTech 1-to-4 DP hub is constructed by internal 2 layer 1-to-2
> > > > branch devices. Creating connectors for such internal output ports
> > > > are redundant.
> > > >
> > > > [How]
> > > > Put constraint on creating connector for connected end device only.
> > > >
> > > > Fixes: 6f85f73821f6 ("drm/dp_mst: Add basic topology reprobing when
> > > > resuming")
> > > > Cc: Juston Li <juston.li@xxxxxxxxx>
> > > > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Harry Wentland <hwentlan@xxxxxxx>
> > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > Cc: Sean Paul <sean@xxxxxxxxxx>
> > > > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > > > Cc: David Airlie <airlied@xxxxxxxx>
> > > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> > > > Cc: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
> > > > Cc: Aurabindo Pillai <aurabindo.pillai@xxxxxxx>
> > > > Cc: Eryk Brol <eryk.brol@xxxxxxx>
> > > > Cc: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
> > > > Cc: Nikola Cornij <nikola.cornij@xxxxxxx>
> > > > Cc: Wayne Lin <Wayne.Lin@xxxxxxx>
> > > > Cc: "Ville Syrjälä" <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > > Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> > > > Cc: "José Roberto de Souza" <jose.souza@xxxxxxxxx>
> > > > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > > > Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> > > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: <stable@xxxxxxxxxxxxxxx> # v5.5+
> > > > Signed-off-by: Wayne Lin <Wayne.Lin@xxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/drm_dp_mst_topology.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > index 51cd7f74f026..f13c7187b07f 100644
> > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > @@ -2474,7 +2474,8 @@ drm_dp_mst_handle_link_address_port(struct
> > > > drm_dp_mst_branch *mstb,
> > > >
> > > > if (port->connector)
> > > > drm_modeset_unlock(&mgr->base.lock);
> > > > - else if (!port->input)
> > > > + else if (!port->input && port->pdt != DP_PEER_DEVICE_NONE &&
> > > > + drm_dp_mst_is_end_device(port->pdt, port->mcs))
> > > > drm_dp_mst_port_add_connector(mstb, port);
> > > >
> > > > if (send_link_addr && port->mstb) { @@ -2557,6 +2558,10 @@
> > > > drm_dp_mst_handle_conn_stat(struct
> > > > drm_dp_mst_branch
> > > > *mstb,
> > > > dowork = false;
> > > > }
> > > >
> > > > + if (!port->input && !port->connector && new_pdt !=
> > > > DP_PEER_DEVICE_NONE &&
> > > > + drm_dp_mst_is_end_device(new_pdt, new_mcs))
> > > > + create_connector = true;
> > > > +
> > > > if (port->connector)
> > > > drm_modeset_unlock(&mgr->base.lock);
> > > > else if (create_connector)
> > >
> >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> Regards,
> Wayne Lin
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
