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

Wed Aug 11 09:49:49 UTC 2021

[Public]

> -----Original Message-----
> From: Lyude Paul <lyude at redhat.com>
> Sent: Wednesday, August 11, 2021 4:45 AM
> To: Lin, Wayne <Wayne.Lin at amd.com>; dri-devel at lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Wentland, Harry <Harry.Wentland at amd.com>; Zuo, Jerry
> <Jerry.Zuo at amd.com>; Wu, Hersen <hersenxs.wu at amd.com>; Juston Li <juston.li at intel.com>; Imre Deak <imre.deak at intel.com>;
> Ville Syrjälä <ville.syrjala at linux.intel.com>; Daniel Vetter <daniel.vetter at ffwll.ch>; Sean Paul <sean at poorly.run>; Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com>; Maxime Ripard <mripard at kernel.org>; Thomas Zimmermann <tzimmermann at suse.de>;
> David Airlie <airlied at linux.ie>; Daniel Vetter <daniel at ffwll.ch>; Deucher, Alexander <Alexander.Deucher at amd.com>; Siqueira,
> Rodrigo <Rodrigo.Siqueira at amd.com>; Pillai, Aurabindo <Aurabindo.Pillai at amd.com>; Eryk Brol <eryk.brol at amd.com>; Bas
> Nieuwenhuizen <bas at basnieuwenhuizen.nl>; Cornij, Nikola <Nikola.Cornij at amd.com>; Jani Nikula <jani.nikula at intel.com>; Manasi
> Navare <manasi.d.navare at intel.com>; Ankit Nautiyal <ankit.k.nautiyal at intel.com>; José Roberto de Souza <jose.souza at intel.com>;
> Sean Paul <seanpaul at chromium.org>; Ben Skeggs <bskeggs at redhat.com>; stable at vger.kernel.org
> Subject: Re: [PATCH 2/4] drm/dp_mst: Only create connector for connected end device
>
> On Wed, 2021-08-04 at 07:13 +0000, Lin, Wayne wrote:
> > [Public]
> >
> > > -----Original Message-----
> > > From: Lyude Paul <lyude at redhat.com>
> > > Sent: Wednesday, August 4, 2021 8:09 AM
> > > To: Lin, Wayne <Wayne.Lin at amd.com>; dri-devel at lists.freedesktop.org
> > > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Wentland,
> > > Harry < Harry.Wentland at amd.com>; Zuo, Jerry <Jerry.Zuo at amd.com>; Wu,
> > > Hersen <hersenxs.wu at amd.com>; Juston Li < juston.li at intel.com>; Imre
> > > Deak <imre.deak at intel.com>; Ville Syrjälä
> > > <ville.syrjala at linux.intel.com>; Wentland, Harry <
> > > Harry.Wentland at amd.com>; Daniel Vetter <daniel.vetter at ffwll.ch>;
> > > Sean Paul <sean at poorly.run>; Maarten Lankhorst <
> > > maarten.lankhorst at linux.intel.com>; Maxime Ripard
> > > <mripard at kernel.org>; Thomas Zimmermann <tzimmermann at suse.de>; David
> > > Airlie <airlied at linux.ie>; Daniel Vetter <daniel at ffwll.ch>; Deucher,
> > > Alexander <Alexander.Deucher at amd.com>; Siqueira, Rodrigo <
> > > Rodrigo.Siqueira at amd.com>; Pillai, Aurabindo
> > > <Aurabindo.Pillai at amd.com>; Eryk Brol <eryk.brol at amd.com>; Bas
> > > Nieuwenhuizen <bas at basnieuwenhuizen.nl>; Cornij, Nikola
> > > <Nikola.Cornij at amd.com>; Jani Nikula <jani.nikula at intel.com>; Manasi
> > > Navare <manasi.d.navare at intel.com>; Ankit Nautiyal
> > > <ankit.k.nautiyal at intel.com>; José Roberto de Souza
> > > <jose.souza at intel.com>; Sean Paul <seanpaul at chromium.org>; Ben
> > > Skeggs <bskeggs at redhat.com>; stable at 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!
>
> Sorry-I will respond to this soon, some more stuff came up at work so it might take me a day or two
No worries. Much appreciated for your time!
>
> > > >
> > > > 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 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
> > Regards,
> > Wayne Lin
> >
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
--
Regards,
Wayne Lin