[PATCH 0/3] drm/dp_mst: Fix bandwidth checking regressions from DSC patches

[PATCH 0/3] drm/dp_mst: Fix bandwidth checking regressions from DSC patches

[Lyude Paul]

AMD's patch series for adding DSC support to the MST helpers
unfortunately introduced a few regressions into the kernel that I didn't
get around to fixing until just now. I would have reverted the changes
earlier, but seeing as that would have reverted all of amd's DSC support
+ everything that was done on top of that I realllllly wanted to avoid
doing that.

Anyway, this should fix everything as far as I can tell. Note that I
don't have any DSC displays locally yet, so if someone from AMD could
sanity check this I would appreciate it ♥.

Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>

Lyude Paul (3):
  drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less
    redundant
  drm/dp_mst: Don't show connectors as connected before probing
    available PBN
  drm/dp_mst: Rewrite and fix bandwidth limit checks

 drivers/gpu/drm/drm_dp_mst_topology.c | 124 ++++++++++++++++++++------
 1 file changed, 96 insertions(+), 28 deletions(-)

-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less redundant

[Lyude Paul]

It's already prefixed by dp_mst, so we don't really need to repeat
ourselves here. One of the changes I should have picked up originally
when reviewing MST DSC support.

There should be no functional changes here

Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 61e7beada832..207eef08d12c 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1937,7 +1937,7 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port *port,
 	return parent_lct + 1;
 }
 
-static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs)
+static bool drm_dp_mst_is_end_device(u8 pdt, bool mcs)
 {
 	switch (pdt) {
 	case DP_PEER_DEVICE_DP_LEGACY_CONV:
@@ -1967,13 +1967,13 @@ drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
 
 	/* Teardown the old pdt, if there is one */
 	if (port->pdt != DP_PEER_DEVICE_NONE) {
-		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
+		if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
 			/*
 			 * If the new PDT would also have an i2c bus,
 			 * don't bother with reregistering it
 			 */
 			if (new_pdt != DP_PEER_DEVICE_NONE &&
-			    drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs)) {
+			    drm_dp_mst_is_end_device(new_pdt, new_mcs)) {
 				port->pdt = new_pdt;
 				port->mcs = new_mcs;
 				return 0;
@@ -1993,7 +1993,7 @@ drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
 	port->mcs = new_mcs;
 
 	if (port->pdt != DP_PEER_DEVICE_NONE) {
-		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
+		if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
 			/* add i2c over sideband */
 			ret = drm_dp_mst_register_i2c_bus(&port->aux);
 		} else {
@@ -2169,7 +2169,7 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
 	}
 
 	if (port->pdt != DP_PEER_DEVICE_NONE &&
-	    drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
+	    drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
 		port->cached_edid = drm_get_edid(port->connector,
 						 &port->aux.ddc);
 		drm_connector_set_tile_property(port->connector);
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Lyude Paul]

It's next to impossible for us to do connector probing on topologies
without occasionally racing with userspace, since creating a connector
itself causes a hotplug event which we have to send before probing the
available PBN of a connector. Even if we didn't have this hotplug event
sent, there's still always a chance that userspace started probing
connectors before we finished probing the topology.

This can be a problem when validating a new MST state since the
connector will be shown as connected briefly, but without any available
PBN - causing any atomic state which would enable said connector to fail
with -ENOSPC. So, let's simply workaround this by telling userspace new
MST connectors are disconnected until we've finished probing their PBN.
Since we always send a hotplug event at the end of the link address
probing process, userspace will still know to reprobe the connector when
we're ready.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check")
Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 207eef08d12c..7b0ff0cff954 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
 			ret = connector_status_connected;
 		break;
 	}
+
+	/* We don't want to tell userspace the port is actually plugged into
+	 * anything until we've finished probing it's available_pbn, otherwise
+	 * userspace will see racy atomic check failures
+	 *
+	 * Since we always send a hotplug at the end of probing topology
+	 * state, we can just let userspace reprobe this connector later.
+	 */
+	if (ret == connector_status_connected && !port->available_pbn) {
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN not probed)\n",
+			      connector->base.id, connector->name);
+		ret = connector_status_disconnected;
+	}
 out:
 	drm_dp_mst_topology_put_port(port);
 	return ret;
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/dp_mst: Rewrite and fix bandwidth limit checks

[Lyude Paul]

Sigh, this is mostly my fault for not giving commit cd82d82cbc04
("drm/dp_mst: Add branch bandwidth validation to MST atomic check")
enough scrutiny during review. The way we're checking bandwidth
limitations here is mostly wrong.

First things first, we need to follow the locking conventions for MST.
Whenever traversing downwards (upwards is always safe) in the topology,
we need to hold &mgr->lock to prevent the topology from changing under
us. We don't currently do that when performing bandwidth limit checks.

Next we need to figure out the actual PBN limit for the primary MSTB.
Here we actually want to use the highest available_pbn value we can find
on each level of the topology, then make sure that the combined sum of
allocated PBN on each port of the branch device doesn't exceed that
amount. Currently, we just loop through each level of the topology and
use the last non-zero PBN we find.

Once we've done that, we then want to traverse down each branch device
we find in the topology with at least one downstream port that has PBN
allocated in our atomic state, and repeat the whole process on each
level of the topology as we travel down. While doing this, we need to
take care to avoid attempting to traverse down end devices. We don't
currently do this, although I'm not actually sure whether or not this
broke anything before.

Since there's a bit too many issues here to try to fix one by one, and
the drm_dp_mst_atomic_check_bw_limit() code is not entirely clear on all
of these pain points anyway, let's just take the easy way out and
rewrite the whole function. Additionally, we also add a kernel warning
if we find that any ports we performed bandwidth limit checks on didn't
actually have available_pbn populated - as this is always a bug in the
MST helpers.

This should fix regressions seen on nouveau, i915 and amdgpu where we
erroneously reject atomic states that should fit within bandwidth
limitations.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check")
Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 101 ++++++++++++++++++++------
 1 file changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 7b0ff0cff954..87dc7c92d339 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4853,41 +4853,90 @@ static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port,
 	return false;
 }
 
-static inline
-int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,
-				     struct drm_dp_mst_topology_state *mst_state)
+static int
+drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,
+				 struct drm_dp_mst_topology_state *mst_state)
 {
 	struct drm_dp_mst_port *port;
 	struct drm_dp_vcpi_allocation *vcpi;
-	int pbn_limit = 0, pbn_used = 0;
+	int pbn_limit = 0, pbn_used = 0, ret;
 
-	list_for_each_entry(port, &branch->ports, next) {
-		if (port->mstb)
-			if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state))
-				return -ENOSPC;
+	if (branch->port_parent)
+		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] checking [MSTB:%p]\n",
+				 branch->port_parent->parent,
+				 branch->port_parent, branch);
+	else
+		DRM_DEBUG_ATOMIC("Checking [MSTB:%p]\n", branch);
 
-		if (port->available_pbn > 0)
+	list_for_each_entry(port, &branch->ports, next) {
+		/* Since each port shares a link, the highest PBN we find
+		 * should be assumed to be the limit for this branch device
+		 */
+		if (pbn_limit < port->available_pbn)
 			pbn_limit = port->available_pbn;
-	}
-	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
-			 branch, pbn_limit);
 
-	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
-		if (!vcpi->pbn)
+		if (port->pdt == DP_PEER_DEVICE_NONE)
 			continue;
 
-		if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
-			pbn_used += vcpi->pbn;
+		if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
+			list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+				if (vcpi->port != port)
+					continue;
+				if (!vcpi->pbn)
+					break;
+
+				/* This should never happen, as it means we
+				 * tried to set a mode before querying the
+				 * available_pbn
+				 */
+				if (WARN_ON(!port->available_pbn))
+					return -EINVAL;
+
+				if (vcpi->pbn > port->available_pbn) {
+					DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] %d exceeds available PBN of %d\n",
+							 branch, port,
+							 vcpi->pbn,
+							 port->available_pbn);
+					return -ENOSPC;
+				}
+
+				DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] using %d PBN\n",
+						 branch, port, vcpi->pbn);
+				pbn_used += vcpi->pbn;
+				break;
+			}
+		} else {
+			list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+				if (!vcpi->pbn ||
+				    !drm_dp_mst_port_downstream_of_branch(vcpi->port,
+									  port->mstb))
+					continue;
+
+				ret = drm_dp_mst_atomic_check_bw_limit(port->mstb,
+								       mst_state);
+				if (ret < 0)
+					return ret;
+
+				pbn_used += ret;
+				break;
+			}
+		}
 	}
-	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
-			 branch, pbn_used);
+	if (!pbn_used)
+		return 0;
+
+	DRM_DEBUG_ATOMIC("[MSTB:%p] has total available PBN of %d\n",
+			 branch, pbn_limit);
 
 	if (pbn_used > pbn_limit) {
-		DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n",
-				 branch);
+		DRM_DEBUG_ATOMIC("[MSTB:%p] Not enough bandwidth (need: %d)\n",
+				 branch, pbn_used);
 		return -ENOSPC;
 	}
-	return 0;
+
+	DRM_DEBUG_ATOMIC("[MSTB:%p] using %d PBN\n", branch, pbn_used);
+
+	return pbn_used;
 }
 
 static inline int
@@ -5085,9 +5134,15 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
 		ret = drm_dp_mst_atomic_check_vcpi_alloc_limit(mgr, mst_state);
 		if (ret)
 			break;
-		ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, mst_state);
-		if (ret)
+
+		mutex_lock(&mgr->lock);
+		ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary,
+						       mst_state);
+		mutex_unlock(&mgr->lock);
+		if (ret < 0)
 			break;
+		else
+			ret = 0;
 	}
 
 	return ret;
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Ville Syrjälä]

On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> It's next to impossible for us to do connector probing on topologies
> without occasionally racing with userspace, since creating a connector
> itself causes a hotplug event which we have to send before probing the
> available PBN of a connector. Even if we didn't have this hotplug event
> sent, there's still always a chance that userspace started probing
> connectors before we finished probing the topology.
> 
> This can be a problem when validating a new MST state since the
> connector will be shown as connected briefly, but without any available
> PBN - causing any atomic state which would enable said connector to fail
> with -ENOSPC. So, let's simply workaround this by telling userspace new
> MST connectors are disconnected until we've finished probing their PBN.
> Since we always send a hotplug event at the end of the link address
> probing process, userspace will still know to reprobe the connector when
> we're ready.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check")
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 207eef08d12c..7b0ff0cff954 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
>  			ret = connector_status_connected;
>  		break;
>  	}
> +
> +	/* We don't want to tell userspace the port is actually plugged into
> +	 * anything until we've finished probing it's available_pbn, otherwise

"its"

Why is the connector even registered before we've finished the probe?

> +	 * userspace will see racy atomic check failures
> +	 *
> +	 * Since we always send a hotplug at the end of probing topology
> +	 * state, we can just let userspace reprobe this connector later.
> +	 */
> +	if (ret == connector_status_connected && !port->available_pbn) {
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN not probed)\n",
> +			      connector->base.id, connector->name);
> +		ret = connector_status_disconnected;
> +	}
>  out:
>  	drm_dp_mst_topology_put_port(port);
>  	return ret;
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm/dp_mst: Fix bandwidth checking regressions from DSC patches

[Hans de Goede]

Hi Lyude,

On 3/4/20 11:36 PM, Lyude Paul wrote:
> AMD's patch series for adding DSC support to the MST helpers
> unfortunately introduced a few regressions into the kernel that I didn't
> get around to fixing until just now. I would have reverted the changes
> earlier, but seeing as that would have reverted all of amd's DSC support
> + everything that was done on top of that I realllllly wanted to avoid
> doing that.
> 
> Anyway, this should fix everything as far as I can tell. Note that I
> don't have any DSC displays locally yet, so if someone from AMD could
> sanity check this I would appreciate it ♥.

Thank you for trying to fix this, unfortunately for me this is not fixed,
with this series. My setup:

5.6-rc4 + your 3 patches (+ some unrelated patches outside of drm)

-Lenovo x1 7th gen +
  Lenovo TB3 dock gen 2 +
  2 external 1920x1080@60 monitors connected to the 2 HDMI interfaces on the dock
-System booted with the LID closed, so that the firmware/BIOS has already
  initialized both monitors when the kernel boots

This should be fairly easy to reproduce on a similar setup, other
users are seeing similar problems when connecting more then 1 monitor
to DP-MST docks, see e.g. :

https://bugzilla.redhat.com/show_bug.cgi?id=1809681
https://bugzilla.redhat.com/show_bug.cgi?id=1810070

Let me know if you want me to collect some drm.debug logs, I guess
if you do, you want me to use drm.debug=0x114 ?

Regards,

Hans





> 
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> 
> Lyude Paul (3):
>    drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less
>      redundant
>    drm/dp_mst: Don't show connectors as connected before probing
>      available PBN
>    drm/dp_mst: Rewrite and fix bandwidth limit checks
> 
>   drivers/gpu/drm/drm_dp_mst_topology.c | 124 ++++++++++++++++++++------
>   1 file changed, 96 insertions(+), 28 deletions(-)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Lyude Paul]

On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
> On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> > It's next to impossible for us to do connector probing on topologies
> > without occasionally racing with userspace, since creating a connector
> > itself causes a hotplug event which we have to send before probing the
> > available PBN of a connector. Even if we didn't have this hotplug event
> > sent, there's still always a chance that userspace started probing
> > connectors before we finished probing the topology.
> > 
> > This can be a problem when validating a new MST state since the
> > connector will be shown as connected briefly, but without any available
> > PBN - causing any atomic state which would enable said connector to fail
> > with -ENOSPC. So, let's simply workaround this by telling userspace new
> > MST connectors are disconnected until we've finished probing their PBN.
> > Since we always send a hotplug event at the end of the link address
> > probing process, userspace will still know to reprobe the connector when
> > we're ready.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST
> > atomic check")
> > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Sean Paul <seanpaul@google.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 207eef08d12c..7b0ff0cff954 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector
> > *connector,
> >  			ret = connector_status_connected;
> >  		break;
> >  	}
> > +
> > +	/* We don't want to tell userspace the port is actually plugged into
> > +	 * anything until we've finished probing it's available_pbn, otherwise
> 
> "its"
> 
> Why is the connector even registered before we've finished the probe?
> 
Oops, I'm not sure how I did this by accident but the explanation I gave in
the commit message was uh, completely wrong. I must have forgotten that I made
sure we didn't expose connectors before probing their PBN back when I started
my MST cleanup....

So: despite what I said before it's not actually when new connectors are
created, it's when downstream hotplugs happen which means that the conenctor's
always going to be there before we probe the available_pbn. I did just notice
though that we send a hotplug on connection status notifications even before
we've finished the PBN probe, so I might be able to improve on that as well.
We still definitely want to report the connector as disconnected before we
have the available PBN though, in case another probe was already going before
we got the connection status notification.

I'll make sure to fixup the explanation in the commit message on the next
respin

> > +	 * userspace will see racy atomic check failures
> > +	 *
> > +	 * Since we always send a hotplug at the end of probing topology
> > +	 * state, we can just let userspace reprobe this connector later.
> > +	 */
> > +	if (ret == connector_status_connected && !port->available_pbn) {
> > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN not
> > probed)\n",
> > +			      connector->base.id, connector->name);
> > +		ret = connector_status_disconnected;
> > +	}
> >  out:
> >  	drm_dp_mst_topology_put_port(port);
> >  	return ret;
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Ville Syrjälä]

On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
> On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
> > On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> > > It's next to impossible for us to do connector probing on topologies
> > > without occasionally racing with userspace, since creating a connector
> > > itself causes a hotplug event which we have to send before probing the
> > > available PBN of a connector. Even if we didn't have this hotplug event
> > > sent, there's still always a chance that userspace started probing
> > > connectors before we finished probing the topology.
> > > 
> > > This can be a problem when validating a new MST state since the
> > > connector will be shown as connected briefly, but without any available
> > > PBN - causing any atomic state which would enable said connector to fail
> > > with -ENOSPC. So, let's simply workaround this by telling userspace new
> > > MST connectors are disconnected until we've finished probing their PBN.
> > > Since we always send a hotplug event at the end of the link address
> > > probing process, userspace will still know to reprobe the connector when
> > > we're ready.
> > > 
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST
> > > atomic check")
> > > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Sean Paul <seanpaul@google.com>
> > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 207eef08d12c..7b0ff0cff954 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector
> > > *connector,
> > >  			ret = connector_status_connected;
> > >  		break;
> > >  	}
> > > +
> > > +	/* We don't want to tell userspace the port is actually plugged into
> > > +	 * anything until we've finished probing it's available_pbn, otherwise
> > 
> > "its"
> > 
> > Why is the connector even registered before we've finished the probe?
> > 
> Oops, I'm not sure how I did this by accident but the explanation I gave in
> the commit message was uh, completely wrong. I must have forgotten that I made
> sure we didn't expose connectors before probing their PBN back when I started
> my MST cleanup....
> 
> So: despite what I said before it's not actually when new connectors are
> created, it's when downstream hotplugs happen which means that the conenctor's
> always going to be there before we probe the available_pbn.

Not sure I understand. You're saying this is going to change for already
existing connectors when something else gets plugged in, and either we
zero it out during the probe or it always was zero to begin with for
whatever reason?

> I did just notice
> though that we send a hotplug on connection status notifications even before
> we've finished the PBN probe, so I might be able to improve on that as well.
> We still definitely want to report the connector as disconnected before we
> have the available PBN though, in case another probe was already going before
> we got the connection status notification.
> 
> I'll make sure to fixup the explanation in the commit message on the next
> respin
> 
> > > +	 * userspace will see racy atomic check failures
> > > +	 *
> > > +	 * Since we always send a hotplug at the end of probing topology
> > > +	 * state, we can just let userspace reprobe this connector later.
> > > +	 */
> > > +	if (ret == connector_status_connected && !port->available_pbn) {
> > > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN not
> > > probed)\n",
> > > +			      connector->base.id, connector->name);
> > > +		ret = connector_status_disconnected;
> > > +	}
> > >  out:
> > >  	drm_dp_mst_topology_put_port(port);
> > >  	return ret;
> > > -- 
> > > 2.24.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Associate Software Engineer at Red Hat

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Lyude Paul]

On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
> On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
> > On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> > > > It's next to impossible for us to do connector probing on topologies
> > > > without occasionally racing with userspace, since creating a connector
> > > > itself causes a hotplug event which we have to send before probing the
> > > > available PBN of a connector. Even if we didn't have this hotplug
> > > > event
> > > > sent, there's still always a chance that userspace started probing
> > > > connectors before we finished probing the topology.
> > > > 
> > > > This can be a problem when validating a new MST state since the
> > > > connector will be shown as connected briefly, but without any
> > > > available
> > > > PBN - causing any atomic state which would enable said connector to
> > > > fail
> > > > with -ENOSPC. So, let's simply workaround this by telling userspace
> > > > new
> > > > MST connectors are disconnected until we've finished probing their
> > > > PBN.
> > > > Since we always send a hotplug event at the end of the link address
> > > > probing process, userspace will still know to reprobe the connector
> > > > when
> > > > we're ready.
> > > > 
> > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to
> > > > MST
> > > > atomic check")
> > > > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > Cc: Sean Paul <seanpaul@google.com>
> > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > index 207eef08d12c..7b0ff0cff954 100644
> > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector
> > > > *connector,
> > > >  			ret = connector_status_connected;
> > > >  		break;
> > > >  	}
> > > > +
> > > > +	/* We don't want to tell userspace the port is actually
> > > > plugged into
> > > > +	 * anything until we've finished probing it's available_pbn,
> > > > otherwise
> > > 
> > > "its"
> > > 
> > > Why is the connector even registered before we've finished the probe?
> > > 
> > Oops, I'm not sure how I did this by accident but the explanation I gave
> > in
> > the commit message was uh, completely wrong. I must have forgotten that I
> > made
> > sure we didn't expose connectors before probing their PBN back when I
> > started
> > my MST cleanup....
> > 
> > So: despite what I said before it's not actually when new connectors are
> > created, it's when downstream hotplugs happen which means that the
> > conenctor's
> > always going to be there before we probe the available_pbn.
> 
> Not sure I understand. You're saying this is going to change for already
> existing connectors when something else gets plugged in, and either we
> zero it out during the probe or it always was zero to begin with for
> whatever reason?

So: you just made me realize that I'm not actually sure whether there's any
point to us clearing port->available_pbn here since the available_pbn (at
least the value that we cache on initial link address probing for bandwidth
constraint checking) shouldn't actually change on a port just because of a
hotplug. I bet this is probably causing more problems on it's own as well,
since reprobing the available_pbn might actually give us a value that reflects
allocations on other ports that are already in place.

So: I think what I'm going to do instead is make it so that we never clear
port->available_pbn; mainly to make things less complicated during
suspend/resume, since we want to make sure there's always some sort of PBN
value populated even during the middle of reprobing the link address on
resume. That way we don't have to pretend that it's ever disconnected either.
Will send a respin in a bit.

> 
> > I did just notice
> > though that we send a hotplug on connection status notifications even
> > before
> > we've finished the PBN probe, so I might be able to improve on that as
> > well.
> > We still definitely want to report the connector as disconnected before we
> > have the available PBN though, in case another probe was already going
> > before
> > we got the connection status notification.
> > 
> > I'll make sure to fixup the explanation in the commit message on the next
> > respin
> > 
> > > > +	 * userspace will see racy atomic check failures
> > > > +	 *
> > > > +	 * Since we always send a hotplug at the end of probing
> > > > topology
> > > > +	 * state, we can just let userspace reprobe this connector
> > > > later.
> > > > +	 */
> > > > +	if (ret == connector_status_connected && !port->available_pbn) 
> > > > {
> > > > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN
> > > > not
> > > > probed)\n",
> > > > +			      connector->base.id, connector->name);
> > > > +		ret = connector_status_disconnected;
> > > > +	}
> > > >  out:
> > > >  	drm_dp_mst_topology_put_port(port);
> > > >  	return ret;
> > > > -- 
> > > > 2.24.1
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > -- 
> > Cheers,
> > 	Lyude Paul (she/her)
> > 	Associate Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Lyude Paul]

On Thu, 2020-03-05 at 13:52 -0500, Lyude Paul wrote:
> On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
> > On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
> > > On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
> > > > On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> > > > > It's next to impossible for us to do connector probing on topologies
> > > > > without occasionally racing with userspace, since creating a
> > > > > connector
> > > > > itself causes a hotplug event which we have to send before probing
> > > > > the
> > > > > available PBN of a connector. Even if we didn't have this hotplug
> > > > > event
> > > > > sent, there's still always a chance that userspace started probing
> > > > > connectors before we finished probing the topology.
> > > > > 
> > > > > This can be a problem when validating a new MST state since the
> > > > > connector will be shown as connected briefly, but without any
> > > > > available
> > > > > PBN - causing any atomic state which would enable said connector to
> > > > > fail
> > > > > with -ENOSPC. So, let's simply workaround this by telling userspace
> > > > > new
> > > > > MST connectors are disconnected until we've finished probing their
> > > > > PBN.
> > > > > Since we always send a hotplug event at the end of the link address
> > > > > probing process, userspace will still know to reprobe the connector
> > > > > when
> > > > > we're ready.
> > > > > 
> > > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to
> > > > > MST
> > > > > atomic check")
> > > > > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > Cc: Sean Paul <seanpaul@google.com>
> > > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > index 207eef08d12c..7b0ff0cff954 100644
> > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector
> > > > > *connector,
> > > > >  			ret = connector_status_connected;
> > > > >  		break;
> > > > >  	}
> > > > > +
> > > > > +	/* We don't want to tell userspace the port is actually
> > > > > plugged into
> > > > > +	 * anything until we've finished probing it's available_pbn,
> > > > > otherwise
> > > > 
> > > > "its"
> > > > 
> > > > Why is the connector even registered before we've finished the probe?
> > > > 
> > > Oops, I'm not sure how I did this by accident but the explanation I gave
> > > in
> > > the commit message was uh, completely wrong. I must have forgotten that
> > > I
> > > made
> > > sure we didn't expose connectors before probing their PBN back when I
> > > started
> > > my MST cleanup....
> > > 
> > > So: despite what I said before it's not actually when new connectors are
> > > created, it's when downstream hotplugs happen which means that the
> > > conenctor's
> > > always going to be there before we probe the available_pbn.
> > 
> > Not sure I understand. You're saying this is going to change for already
> > existing connectors when something else gets plugged in, and either we
> > zero it out during the probe or it always was zero to begin with for
> > whatever reason?
> 
> So: you just made me realize that I'm not actually sure whether there's any
> point to us clearing port->available_pbn here since the available_pbn (at
> least the value that we cache on initial link address probing for bandwidth
> constraint checking) shouldn't actually change on a port just because of a
> hotplug. I bet this is probably causing more problems on it's own as well,
> since reprobing the available_pbn might actually give us a value that
> reflects
> allocations on other ports that are already in place.
> 
> So: I think what I'm going to do instead is make it so that we never clear
> port->available_pbn; mainly to make things less complicated during
> suspend/resume, since we want to make sure there's always some sort of PBN
> value populated even during the middle of reprobing the link address on
> resume. That way we don't have to pretend that it's ever disconnected
> either.
> Will send a respin in a bit.
Wait, nope, I believe I am the fool here - _supposedly_ available bw is
supposed to reflect the smallest link rate that occurs in a patch to a branch
device. I think, me and sean paul are looking at this a bit more closely. I
think I might need to do some more playing around with my hubs to make sure
this value is actually what we think it is because unfortunately the spec is
pretty vague on this from what I can tell :( 

> 
> > > I did just notice
> > > though that we send a hotplug on connection status notifications even
> > > before
> > > we've finished the PBN probe, so I might be able to improve on that as
> > > well.
> > > We still definitely want to report the connector as disconnected before
> > > we
> > > have the available PBN though, in case another probe was already going
> > > before
> > > we got the connection status notification.
> > > 
> > > I'll make sure to fixup the explanation in the commit message on the
> > > next
> > > respin
> > > 
> > > > > +	 * userspace will see racy atomic check failures
> > > > > +	 *
> > > > > +	 * Since we always send a hotplug at the end of probing
> > > > > topology
> > > > > +	 * state, we can just let userspace reprobe this connector
> > > > > later.
> > > > > +	 */
> > > > > +	if (ret == connector_status_connected && !port-
> > > > > >available_pbn) 
> > > > > {
> > > > > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN
> > > > > not
> > > > > probed)\n",
> > > > > +			      connector->base.id, connector->name);
> > > > > +		ret = connector_status_disconnected;
> > > > > +	}
> > > > >  out:
> > > > >  	drm_dp_mst_topology_put_port(port);
> > > > >  	return ret;
> > > > > -- 
> > > > > 2.24.1
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > -- 
> > > Cheers,
> > > 	Lyude Paul (she/her)
> > > 	Associate Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Lyude Paul]

On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
> On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
> > On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> > > > It's next to impossible for us to do connector probing on topologies
> > > > without occasionally racing with userspace, since creating a connector
> > > > itself causes a hotplug event which we have to send before probing the
> > > > available PBN of a connector. Even if we didn't have this hotplug
> > > > event
> > > > sent, there's still always a chance that userspace started probing
> > > > connectors before we finished probing the topology.
> > > > 
> > > > This can be a problem when validating a new MST state since the
> > > > connector will be shown as connected briefly, but without any
> > > > available
> > > > PBN - causing any atomic state which would enable said connector to
> > > > fail
> > > > with -ENOSPC. So, let's simply workaround this by telling userspace
> > > > new
> > > > MST connectors are disconnected until we've finished probing their
> > > > PBN.
> > > > Since we always send a hotplug event at the end of the link address
> > > > probing process, userspace will still know to reprobe the connector
> > > > when
> > > > we're ready.
> > > > 
> > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to
> > > > MST
> > > > atomic check")
> > > > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > Cc: Sean Paul <seanpaul@google.com>
> > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > index 207eef08d12c..7b0ff0cff954 100644
> > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector
> > > > *connector,
> > > >  			ret = connector_status_connected;
> > > >  		break;
> > > >  	}
> > > > +
> > > > +	/* We don't want to tell userspace the port is actually
> > > > plugged into
> > > > +	 * anything until we've finished probing it's available_pbn,
> > > > otherwise
> > > 
> > > "its"
> > > 
> > > Why is the connector even registered before we've finished the probe?
> > > 
> > Oops, I'm not sure how I did this by accident but the explanation I gave
> > in
> > the commit message was uh, completely wrong. I must have forgotten that I
> > made
> > sure we didn't expose connectors before probing their PBN back when I
> > started
> > my MST cleanup....
> > 
> > So: despite what I said before it's not actually when new connectors are
> > created, it's when downstream hotplugs happen which means that the
> > conenctor's
> > always going to be there before we probe the available_pbn.
> 
> Not sure I understand. You're saying this is going to change for already
> existing connectors when something else gets plugged in, and either we
> zero it out during the probe or it always was zero to begin with for
> whatever reason?

ok-me and Sean Paul did some playing around with available_pbn and full_pbn
(I'll get into this one in a moment), and I also played around with a couple
of different hubs and have a much better understanding of how this should work
now.

So: first off tl;dr available_pbn is absolutely not what we want in basically
any scenario, we actually want to use the full_pbn field that we get when
sending ENUM_PATH_RESOURCES. Second, full_pbn represents the _smallest_ bandwidth limitation encountered in the path between the root MSTB and each connected sink. Remember that there's technically a DisplayPort link trained between each branch device going down the topology, so that bandwidth limitation basically equates to "what is the lowest trained link rate that exists down the path to this port?". This also means that full_pbn will potentially change every time a new connector is plugged in, as some hubs will be clever and optimize the link rate they decide to use. Likewise, since there's not going to be anything trained on a disconnected port (e.g. ddps=0) there's no point in keeping full_pbn around for disconnected ports, since otherwise we might let userspace see a connected port with a stale full_pbn value.

So-IMHO the behavior of not letting connectors show as connected until we also
have their full_pbn probed should definitely be the right solution here.
Especially if we want to eventually start pruning modes based on full_pbn at
some point in the future.

> 
> > I did just notice
> > though that we send a hotplug on connection status notifications even
> > before
> > we've finished the PBN probe, so I might be able to improve on that as
> > well.
> > We still definitely want to report the connector as disconnected before we
> > have the available PBN though, in case another probe was already going
> > before
> > we got the connection status notification.
> > 
> > I'll make sure to fixup the explanation in the commit message on the next
> > respin
> > 
> > > > +	 * userspace will see racy atomic check failures
> > > > +	 *
> > > > +	 * Since we always send a hotplug at the end of probing
> > > > topology
> > > > +	 * state, we can just let userspace reprobe this connector
> > > > later.
> > > > +	 */
> > > > +	if (ret == connector_status_connected && !port->available_pbn) 
> > > > {
> > > > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN
> > > > not
> > > > probed)\n",
> > > > +			      connector->base.id, connector->name);
> > > > +		ret = connector_status_disconnected;
> > > > +	}
> > > >  out:
> > > >  	drm_dp_mst_topology_put_port(port);
> > > >  	return ret;
> > > > -- 
> > > > 2.24.1
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > -- 
> > Cheers,
> > 	Lyude Paul (she/her)
> > 	Associate Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

[Lyude Paul]

final correction: I probably could actually get rid of this patch and do this
a bit more nicely by just making sure that we reprobe path resources for
connectors while under drm_dp_mst_topology_mgr->base.lock on CSNs, instead of
punting it off to the link address work like we seem to be doing. So, going to
try doing that instead.

On Fri, 2020-03-06 at 15:03 -0500, Lyude Paul wrote:
> On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
> > On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
> > > On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
> > > > On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
> > > > > It's next to impossible for us to do connector probing on topologies
> > > > > without occasionally racing with userspace, since creating a
> > > > > connector
> > > > > itself causes a hotplug event which we have to send before probing
> > > > > the
> > > > > available PBN of a connector. Even if we didn't have this hotplug
> > > > > event
> > > > > sent, there's still always a chance that userspace started probing
> > > > > connectors before we finished probing the topology.
> > > > > 
> > > > > This can be a problem when validating a new MST state since the
> > > > > connector will be shown as connected briefly, but without any
> > > > > available
> > > > > PBN - causing any atomic state which would enable said connector to
> > > > > fail
> > > > > with -ENOSPC. So, let's simply workaround this by telling userspace
> > > > > new
> > > > > MST connectors are disconnected until we've finished probing their
> > > > > PBN.
> > > > > Since we always send a hotplug event at the end of the link address
> > > > > probing process, userspace will still know to reprobe the connector
> > > > > when
> > > > > we're ready.
> > > > > 
> > > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to
> > > > > MST
> > > > > atomic check")
> > > > > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > Cc: Sean Paul <seanpaul@google.com>
> > > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > index 207eef08d12c..7b0ff0cff954 100644
> > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector
> > > > > *connector,
> > > > >  			ret = connector_status_connected;
> > > > >  		break;
> > > > >  	}
> > > > > +
> > > > > +	/* We don't want to tell userspace the port is actually
> > > > > plugged into
> > > > > +	 * anything until we've finished probing it's available_pbn,
> > > > > otherwise
> > > > 
> > > > "its"
> > > > 
> > > > Why is the connector even registered before we've finished the probe?
> > > > 
> > > Oops, I'm not sure how I did this by accident but the explanation I gave
> > > in
> > > the commit message was uh, completely wrong. I must have forgotten that
> > > I
> > > made
> > > sure we didn't expose connectors before probing their PBN back when I
> > > started
> > > my MST cleanup....
> > > 
> > > So: despite what I said before it's not actually when new connectors are
> > > created, it's when downstream hotplugs happen which means that the
> > > conenctor's
> > > always going to be there before we probe the available_pbn.
> > 
> > Not sure I understand. You're saying this is going to change for already
> > existing connectors when something else gets plugged in, and either we
> > zero it out during the probe or it always was zero to begin with for
> > whatever reason?
> 
> ok-me and Sean Paul did some playing around with available_pbn and full_pbn
> (I'll get into this one in a moment), and I also played around with a couple
> of different hubs and have a much better understanding of how this should
> work
> now.
> 
> So: first off tl;dr available_pbn is absolutely not what we want in
> basically
> any scenario, we actually want to use the full_pbn field that we get when
> sending ENUM_PATH_RESOURCES. Second, full_pbn represents the _smallest_
> bandwidth limitation encountered in the path between the root MSTB and each
> connected sink. Remember that there's technically a DisplayPort link trained
> between each branch device going down the topology, so that bandwidth
> limitation basically equates to "what is the lowest trained link rate that
> exists down the path to this port?". This also means that full_pbn will
> potentially change every time a new connector is plugged in, as some hubs
> will be clever and optimize the link rate they decide to use. Likewise,
> since there's not going to be anything trained on a disconnected port (e.g.
> ddps=0) there's no point in keeping full_pbn around for disconnected ports,
> since otherwise we might let userspace see a connected port with a stale
> full_pbn value.
> 
> So-IMHO the behavior of not letting connectors show as connected until we
> also
> have their full_pbn probed should definitely be the right solution here.
> Especially if we want to eventually start pruning modes based on full_pbn at
> some point in the future.
> 
> > > I did just notice
> > > though that we send a hotplug on connection status notifications even
> > > before
> > > we've finished the PBN probe, so I might be able to improve on that as
> > > well.
> > > We still definitely want to report the connector as disconnected before
> > > we
> > > have the available PBN though, in case another probe was already going
> > > before
> > > we got the connection status notification.
> > > 
> > > I'll make sure to fixup the explanation in the commit message on the
> > > next
> > > respin
> > > 
> > > > > +	 * userspace will see racy atomic check failures
> > > > > +	 *
> > > > > +	 * Since we always send a hotplug at the end of probing
> > > > > topology
> > > > > +	 * state, we can just let userspace reprobe this connector
> > > > > later.
> > > > > +	 */
> > > > > +	if (ret == connector_status_connected && !port-
> > > > > >available_pbn) 
> > > > > {
> > > > > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN
> > > > > not
> > > > > probed)\n",
> > > > > +			      connector->base.id, connector->name);
> > > > > +		ret = connector_status_disconnected;
> > > > > +	}
> > > > >  out:
> > > > >  	drm_dp_mst_topology_put_port(port);
> > > > >  	return ret;
> > > > > -- 
> > > > > 2.24.1
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > -- 
> > > Cheers,
> > > 	Lyude Paul (she/her)
> > > 	Associate Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel