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

["Lin, Wayne" <Wayne.Lin@amd.com>]

[Public]

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