On Wed, 2021-08-11 at 09:49 +0000, Lin, Wayne wrote:

[Public]

quoted

-----Original Message-----
From: Lyude Paul <lyude@redhat.com>
Sent: Wednesday, August 11, 2021 4:45 AM
To: Lin, Wayne <redacted>; dri-devel@lists.freedesktop.org
Cc: Kazlauskas, Nicholas <redacted>; Wentland, Harry <
Harry.Wentland@amd.com>; Zuo, Jerry
[off-list ref]; Wu, Hersen [off-list ref]; Juston Li <
juston.li@intel.com>; Imre Deak [off-list ref];
Ville Syrjälä [off-list ref]; Daniel Vetter <
daniel.vetter@ffwll.ch>; Sean Paul [off-list ref]; Maarten Lankhorst
[off-list ref]; Maxime Ripard [off-list ref];
Thomas Zimmermann [off-list ref];
David Airlie [off-list ref]; Daniel Vetter [off-list ref]; Deucher,
Alexander [off-list ref]; Siqueira,
Rodrigo [off-list ref]; Pillai, Aurabindo <
Aurabindo.Pillai@amd.com>; Eryk Brol [off-list ref]; Bas
Nieuwenhuizen [off-list ref]; Cornij, Nikola <
Nikola.Cornij@amd.com>; Jani Nikula [off-list ref]; Manasi
Navare [off-list ref]; Ankit Nautiyal <
ankit.k.nautiyal@intel.com>; José Roberto de Souza [off-list ref];
Sean Paul [off-list ref]; Ben Skeggs [off-list ref]; 
stable@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:

quoted

[Public]

quoted

-----Original Message-----
From: Lyude Paul <lyude@redhat.com>
Sent: Wednesday, August 4, 2021 8:09 AM
To: Lin, Wayne <redacted>; dri-devel@lists.freedesktop.org
Cc: Kazlauskas, Nicholas <redacted>; Wentland,
Harry < Harry.Wentland@amd.com>; Zuo, Jerry [off-list ref]; Wu,
Hersen [off-list ref]; Juston Li < juston.li@intel.com>; Imre
Deak [off-list ref]; Ville Syrjälä
[off-list ref]; Wentland, Harry <
Harry.Wentland@amd.com>; Daniel Vetter [off-list ref];
Sean Paul [off-list ref]; Maarten Lankhorst <
maarten.lankhorst@linux.intel.com>; Maxime Ripard
[off-list ref]; Thomas Zimmermann [off-list ref]; David
Airlie [off-list ref]; Daniel Vetter [off-list ref]; Deucher,
Alexander [off-list ref]; Siqueira, Rodrigo <
Rodrigo.Siqueira@amd.com>; Pillai, Aurabindo
[off-list ref]; Eryk Brol [off-list ref]; Bas
Nieuwenhuizen [off-list ref]; Cornij, Nikola
[off-list ref]; Jani Nikula [off-list ref]; Manasi
Navare [off-list ref]; Ankit Nautiyal
[off-list ref]; José Roberto de Souza
[off-list ref]; Sean Paul [off-list ref]; Ben
Skeggs [off-list ref]; 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:

quoted

On Wed, 2021-07-21 at 00:03 +0800, Wayne Lin wrote:

quoted

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

quoted

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!

quoted

Alright - finally got some time to respond to this. So this change still
doesn't really seem correct to me (if anyone watching this thread wants to
chime in to correct me btw feel free).

JFYI - I don't think the commit is trying to break anything intentionally,
it's just that there's a lot of moving pieces with the locking here that are
easy to trip over. That being said though, besides the locking issues after
thinking about this I'm still a bit skeptical on how much this would work or
even if we would want it.

To start off - my main issue with this is that it sounds like we're basically
entirely getting rid of the disconnected state for MST connectors, and then
only exposing the connector when something is connected. Unless I'm missing
something here, the PDT can pretty much change whenever something is
connected/disconnected or across suspend/resume reprobes. To do this with the
connector API would be very different from connector probing behavior for
other connector types, which already seems like an issue to me. This would
also break the ability to force a connector to be connected/disconnected, as
there would no longer be a way to force a disconnected MST connector on.

The other thing is I'm not entirely clear still on what's trying to be
accomplished here. If you're trying to identify DRM connectors, there's
already no guaranteed consistency with connector names which means that having
less connectors doesn't really make things any easier to identify. For
actually trying to figure out more details on connectors, if this is somethig
userspace needs, this seems like something we should just be adding in the
form of connector props.

With all of this being said, this ends up just seeming like we're adding
potentially a lot of complexity to how we create connectors and the
suspend/resume reprobing code. I think it'd be good to know what the precise
usecase for this actually is, if this is something you still think is needed.

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

quoted

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 <redacted>
Cc: Imre Deak <redacted>
Cc: Ville Syrjälä <redacted>
Cc: Harry Wentland <redacted>
Cc: Daniel Vetter <redacted>
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 <redacted>
Cc: Daniel Vetter <redacted>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Nicholas Kazlauskas <redacted>
Cc: Rodrigo Siqueira <redacted>
Cc: Aurabindo Pillai <redacted>
Cc: Eryk Brol <redacted>
Cc: Bas Nieuwenhuizen <redacted>
Cc: Nikola Cornij <redacted>
Cc: Wayne Lin <redacted>
Cc: "Ville Syrjälä" <redacted>
Cc: Jani Nikula <redacted>
Cc: Manasi Navare <redacted>
Cc: Ankit Nautiyal <redacted>
Cc: "José Roberto de Souza" <redacted>
Cc: Sean Paul <redacted>
Cc: Ben Skeggs <redacted>
Cc: dri-devel@lists.freedesktop.org
Cc: <redacted> # v5.5+
Signed-off-by: Wayne Lin <redacted>
---
 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

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat