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

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 at amd.com>
Cc: Alex Deucher <alexander.deucher at amd.com>
Cc: Sean Paul <seanpaul at google.com>
Cc: Hans de Goede <hdegoede at 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

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 at amd.com>
Cc: Alex Deucher <alexander.deucher at amd.com>
Cc: Sean Paul <seanpaul at google.com>
Cc: Hans de Goede <hdegoede at redhat.com>
Signed-off-by: Lyude Paul <lyude at 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

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 at redhat.com>
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST
atomic check")
Cc: Mikita Lipski <mikita.lipski at amd.com>
Cc: Alex Deucher <alexander.deucher at amd.com>
Cc: Sean Paul <seanpaul at google.com>
Cc: Hans de Goede <hdegoede at 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

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 at redhat.com>
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST
atomic check")
Cc: Mikita Lipski <mikita.lipski at amd.com>
Cc: Alex Deucher <alexander.deucher at amd.com>
Cc: Sean Paul <seanpaul at google.com>
Cc: Hans de Goede <hdegoede at 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

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 at redhat.com>
> Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to
MST atomic check")
> Cc: Mikita Lipski <mikita.lipski at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Sean Paul <seanpaul at google.com>
> Cc: Hans de Goede <hdegoede at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Ville Syrj?l?
Intel

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 at 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 at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Sean Paul <seanpaul at google.com>
> Cc: Hans de Goede <hdegoede at 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(-)
>