[PATCH 2/4] drm/dp_mst: Only create connector for connected end device

[Lyude Paul]

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

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 at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Harry Wentland <hwentlan at amd.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Sean Paul <sean at poorly.run>
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> Cc: Aurabindo Pillai <aurabindo.pillai at amd.com>
> Cc: Eryk Brol <eryk.brol at amd.com>
> Cc: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> Cc: Nikola Cornij <nikola.cornij at amd.com>
> Cc: Wayne Lin <Wayne.Lin at amd.com>
> Cc: "Ville Syrjälä" <ville.syrjala at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> Cc: "José Roberto de Souza" <jose.souza at intel.com>
> Cc: Sean Paul <seanpaul at chromium.org>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: dri-devel at lists.freedesktop.org
> Cc: <stable at vger.kernel.org> # v5.5+
> Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> ---
>  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