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