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

[PATCH v2 0/4] 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 bandwidth-check related as far as I
can tell (I found some other regressions unrelated to AMD's DSC patches
which I'll be sending out patches for shortly). 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 (4):
  drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less
    redundant
  drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
  drm/dp_mst: Reprobe path resources in CSN handler
  drm/dp_mst: Rewrite and fix bandwidth limit checks

 drivers/gpu/drm/drm_dp_mst_topology.c | 185 ++++++++++++++++++--------
 include/drm/drm_dp_mst_helper.h       |   4 +-
 2 files changed, 129 insertions(+), 60 deletions(-)

-- 
2.24.1

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

[PATCH v2 1/4] 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: 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 6c62ad8f4414..6714d8a5c558 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 v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks

[Lyude Paul]

DisplayPort specifications are fun. For a while, it's been really
unclear to us what available_pbn actually does. There's a somewhat vague
explanation in the DisplayPort spec (starting from 1.2) that partially
explains it:

  The minimum payload bandwidth number supported by the path. Each node
  updates this number with its available payload bandwidth number if its
  payload bandwidth number is less than that in the Message Transaction
  reply.

So, it sounds like available_pbn represents the smallest link rate in
use between the source and the branch device. Cool, so full_pbn is just
the highest possible PBN that the branch device supports right?

Well, we assumed that for quite a while until Sean Paul noticed that on
some MST hubs, available_pbn will actually get set to 0 whenever there's
any active payloads on the respective branch device. This caused quite a
bit of confusion since clearing the payload ID table would end up fixing
the available_pbn value.

So, we just went with that until commit cd82d82cbc04 ("drm/dp_mst: Add
branch bandwidth validation to MST atomic check") started breaking
people's setups due to us getting erroneous available_pbn values. So, we
did some more digging and got confused until we finally looked at the
definition for full_pbn:

  The bandwidth of the link at the trained link rate and lane count
  between the DP Source device and the DP Sink device with no time slots
  allocated to VC Payloads, represented as a Payload Bandwidth Number. As
  with the Available_Payload_Bandwidth_Number, this number is determined
  by the link with the lowest lane count and link rate.

That's what we get for not reading specs closely enough, hehe. So, since
full_pbn is definitely what we want for doing bandwidth restriction
checks - let's start using that instead and ignore available_pbn
entirely.

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: Hans de Goede <hdegoede@redhat.com>
Cc: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 15 +++++++--------
 include/drm/drm_dp_mst_helper.h       |  4 ++--
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 6714d8a5c558..79ebb871230b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2306,7 +2306,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 								port);
 			}
 		} else {
-			port->available_pbn = 0;
+			port->full_pbn = 0;
 		}
 	}
 
@@ -2401,7 +2401,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
 		if (port->ddps) {
 			dowork = true;
 		} else {
-			port->available_pbn = 0;
+			port->full_pbn = 0;
 		}
 	}
 
@@ -2553,7 +2553,7 @@ static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mg
 		if (port->input || !port->ddps)
 			continue;
 
-		if (!port->available_pbn) {
+		if (!port->full_pbn) {
 			drm_modeset_lock(&mgr->base.lock, NULL);
 			drm_dp_send_enum_path_resources(mgr, mstb, port);
 			drm_modeset_unlock(&mgr->base.lock);
@@ -2999,8 +2999,7 @@ drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 				      path_res->port_number,
 				      path_res->full_payload_bw_number,
 				      path_res->avail_payload_bw_number);
-			port->available_pbn =
-				path_res->avail_payload_bw_number;
+			port->full_pbn = path_res->full_payload_bw_number;
 			port->fec_capable = path_res->fec_capable;
 		}
 	}
@@ -3585,7 +3584,7 @@ drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb)
 
 	list_for_each_entry(port, &mstb->ports, next) {
 		/* The PBN for each port will also need to be re-probed */
-		port->available_pbn = 0;
+		port->full_pbn = 0;
 
 		if (port->mstb)
 			drm_dp_mst_topology_mgr_invalidate_mstb(port->mstb);
@@ -4853,8 +4852,8 @@ int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,
 			if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state))
 				return -ENOSPC;
 
-		if (port->available_pbn > 0)
-			pbn_limit = port->available_pbn;
+		if (port->full_pbn > 0)
+			pbn_limit = port->full_pbn;
 	}
 	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
 			 branch, pbn_limit);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 5483f888712a..1d4c996cb92d 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -81,7 +81,7 @@ struct drm_dp_vcpi {
  * &drm_dp_mst_topology_mgr.base.lock.
  * @num_sdp_stream_sinks: Number of stream sinks. Protected by
  * &drm_dp_mst_topology_mgr.base.lock.
- * @available_pbn: Available bandwidth for this port. Protected by
+ * @full_pbn: Max possible bandwidth for this port. Protected by
  * &drm_dp_mst_topology_mgr.base.lock.
  * @next: link to next port on this branch device
  * @aux: i2c aux transport to talk to device connected to this port, protected
@@ -126,7 +126,7 @@ struct drm_dp_mst_port {
 	u8 dpcd_rev;
 	u8 num_sdp_streams;
 	u8 num_sdp_stream_sinks;
-	uint16_t available_pbn;
+	uint16_t full_pbn;
 	struct list_head next;
 	/**
 	 * @mstb: the branch device connected to this port, if there is one.
-- 
2.24.1

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

[PATCH v2 3/4] drm/dp_mst: Reprobe path resources in CSN handler

[Lyude Paul]

We used to punt off reprobing path resources to the link address probe
work, but now that we handle CSNs asynchronously from the driver's HPD
handling we can do whatever the heck we want from the CSN!

So, reprobe the path resources from drm_dp_mst_handle_conn_stat(). Also,
get rid of the path resource reprobing code in
drm_dp_check_and_send_link_address() since it's needlessly complicated
when we already reprobe path resources from
drm_dp_handle_link_address_port(). And finally, teach
drm_dp_send_enum_path_resources() to return 1 on PBN changes so we know
if we need to send another hotplug or not.

This fixes issues where we've indicated to userspace that a port has
just been connected, before we actually probed it's available PBN -
something that results in unexpected atomic check failures.

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: Hans de Goede <hdegoede@redhat.com>
Cc: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 48 ++++++++++++++-------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 79ebb871230b..b81ad444c24f 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2299,12 +2299,16 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 		mutex_unlock(&mgr->lock);
 	}
 
-	if (old_ddps != port->ddps) {
-		if (port->ddps) {
-			if (!port->input) {
-				drm_dp_send_enum_path_resources(mgr, mstb,
-								port);
-			}
+	/*
+	 * Reprobe PBN caps on both hotplug, and when re-probing the link
+	 * for our parent mstb
+	 */
+	if (old_ddps != port->ddps || !created) {
+		if (port->ddps && !port->input) {
+			ret = drm_dp_send_enum_path_resources(mgr, mstb,
+							      port);
+			if (ret == 1)
+				changed = true;
 		} else {
 			port->full_pbn = 0;
 		}
@@ -2398,11 +2402,10 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
 	port->ddps = conn_stat->displayport_device_plug_status;
 
 	if (old_ddps != port->ddps) {
-		if (port->ddps) {
-			dowork = true;
-		} else {
+		if (port->ddps && !port->input)
+			drm_dp_send_enum_path_resources(mgr, mstb, port);
+		else
 			port->full_pbn = 0;
-		}
 	}
 
 	new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat->peer_device_type;
@@ -2553,13 +2556,6 @@ static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mg
 		if (port->input || !port->ddps)
 			continue;
 
-		if (!port->full_pbn) {
-			drm_modeset_lock(&mgr->base.lock, NULL);
-			drm_dp_send_enum_path_resources(mgr, mstb, port);
-			drm_modeset_unlock(&mgr->base.lock);
-			changed = true;
-		}
-
 		if (port->mstb)
 			mstb_child = drm_dp_mst_topology_get_mstb_validated(
 			    mgr, port->mstb);
@@ -2987,6 +2983,7 @@ drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 
 	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
 	if (ret > 0) {
+		ret = 0;
 		path_res = &txmsg->reply.u.path_resources;
 
 		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) {
@@ -2999,13 +2996,22 @@ drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 				      path_res->port_number,
 				      path_res->full_payload_bw_number,
 				      path_res->avail_payload_bw_number);
+
+			/*
+			 * If something changed, make sure we send a
+			 * hotplug
+			 */
+			if (port->full_pbn != path_res->full_payload_bw_number ||
+			    port->fec_capable != path_res->fec_capable)
+				ret = 1;
+
 			port->full_pbn = path_res->full_payload_bw_number;
 			port->fec_capable = path_res->fec_capable;
 		}
 	}
 
 	kfree(txmsg);
-	return 0;
+	return ret;
 }
 
 static struct drm_dp_mst_port *drm_dp_get_last_connected_port_to_mstb(struct drm_dp_mst_branch *mstb)
@@ -3582,13 +3588,9 @@ drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb)
 	/* The link address will need to be re-sent on resume */
 	mstb->link_address_sent = false;
 
-	list_for_each_entry(port, &mstb->ports, next) {
-		/* The PBN for each port will also need to be re-probed */
-		port->full_pbn = 0;
-
+	list_for_each_entry(port, &mstb->ports, next)
 		if (port->mstb)
 			drm_dp_mst_topology_mgr_invalidate_mstb(port->mstb);
-	}
 }
 
 /**
-- 
2.24.1

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

[PATCH v2 4/4] 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:

For starters, drm_dp_mst_atomic_check_bw_limit() determines the
pbn_limit of a branch by simply scanning each port on the current branch
device, then uses the last non-zero full_pbn value that it finds. It
then counts the sum of the PBN used on each branch device for that
level, and compares against the full_pbn value it found before.

This is wrong because ports can and will have different PBN limitations
on many hubs, especially since a number of DisplayPort hubs out there
will be clever and only use the smallest link rate required for each
downstream sink - potentially giving every port a different full_pbn
value depending on what link rate it's trained at. This means with our
current code, which max PBN value we end up with is not well defined.

Additionally, we also need to remember when checking bandwidth
limitations that the top-most device in any MST topology is a branch
device, not a port. This means that the first level of a topology
doesn't technically have a full_pbn value that needs to be checked.
Instead, we should assume that so long as our VCPI allocations fit we're
within the bandwidth limitations of the primary MSTB.

We do however, want to check full_pbn on every port including those of
the primary MSTB. However, it's important to keep in mind that this
value represents the minimum link rate /between a port's sink or mstb,
and the mstb itself/. A quick diagram to explain:

                                MSTB #1
                               /       \
                              /         \
                           Port #1    Port #2
       full_pbn for Port #1 → |          | ← full_pbn for Port #2
                           Sink #1    MSTB #2
                                         |
                                       etc...

Note that in the above diagram, the combined PBN from all VCPI
allocations on said hub should not exceed the full_pbn value of port #2,
and the display configuration on sink #1 should not exceed the full_pbn
value of port #1. However, port #1 and port #2 can otherwise consume as
much bandwidth as they want so long as their VCPI allocations still fit.

And finally - our current bandwidth checking code also makes the mistake
of not checking whether something is an end device or not before trying
to traverse down it.

So, let's fix it by rewriting our bandwidth checking helpers. We split
the function into one part for handling branches which simply adds up
the total PBN on each branch and returns it, and one for checking each
port to ensure we're not going over its PBN limit. Phew.

This should fix regressions seen, where we erroneously reject display
configurations due to thinking they're going over our bandwidth limits
when they're not.

Changes since v1:
* Took an even closer look at how PBN limitations are supposed to be
  handled, and did some experimenting with Sean Paul. Ended up rewriting
  these helpers again, but this time they should actually be correct!

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: Sean Paul <seanpaul@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 120 ++++++++++++++++++++------
 1 file changed, 94 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index b81ad444c24f..322f7b2c9c96 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4841,41 +4841,103 @@ 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_port_bw_limit(struct drm_dp_mst_port *port,
+				      struct drm_dp_mst_topology_state *state);
+
+static int
+drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,
+				      struct drm_dp_mst_topology_state *state)
 {
-	struct drm_dp_mst_port *port;
 	struct drm_dp_vcpi_allocation *vcpi;
-	int pbn_limit = 0, pbn_used = 0;
+	struct drm_dp_mst_port *port;
+	int pbn_used = 0, ret;
+	bool found = false;
 
-	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;
+	/* Check that we have at least one port in our state that's downstream
+	 * of this branch, otherwise we can skip this branch
+	 */
+	list_for_each_entry(vcpi, &state->vcpis, next) {
+		if (!vcpi->pbn ||
+		    !drm_dp_mst_port_downstream_of_branch(vcpi->port,
+							  mstb))
+			continue;
 
-		if (port->full_pbn > 0)
-			pbn_limit = port->full_pbn;
+		found = true;
+		break;
 	}
-	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
-			 branch, pbn_limit);
+	if (!found)
+		return 0;
 
-	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
-		if (!vcpi->pbn)
-			continue;
+	if (mstb->port_parent)
+		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] Checking bandwidth limits on [MSTB:%p]\n",
+				 mstb->port_parent->parent, mstb->port_parent,
+				 mstb);
+	else
+		DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
+				 mstb);
 
-		if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
-			pbn_used += vcpi->pbn;
+	list_for_each_entry(port, &mstb->ports, next) {
+		ret = drm_dp_mst_atomic_check_port_bw_limit(port, state);
+		if (ret < 0)
+			return ret;
+
+		pbn_used += ret;
 	}
-	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
-			 branch, pbn_used);
 
-	if (pbn_used > pbn_limit) {
-		DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n",
-				 branch);
+	return pbn_used;
+}
+
+static int
+drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
+				      struct drm_dp_mst_topology_state *state)
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	int pbn_used = 0;
+
+	if (port->pdt == DP_PEER_DEVICE_NONE)
+		return 0;
+
+	if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
+		bool found = false;
+
+		list_for_each_entry(vcpi, &state->vcpis, next) {
+			if (vcpi->port != port)
+				continue;
+			if (!vcpi->pbn)
+				return 0;
+
+			found = true;
+			break;
+		}
+		if (!found)
+			return 0;
+
+		/* This should never happen, as it means we tried to
+		 * set a mode before querying the full_pbn
+		 */
+		if (WARN_ON(!port->full_pbn))
+			return -EINVAL;
+
+		pbn_used = vcpi->pbn;
+	} else {
+		pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,
+								 state);
+		if (pbn_used <= 0)
+			return 0;
+	}
+
+	if (pbn_used > port->full_pbn) {
+		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] required PBN of %d exceeds port limit of %d\n",
+				 port->parent, port, pbn_used,
+				 port->full_pbn);
 		return -ENOSPC;
 	}
-	return 0;
+
+	DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] uses %d out of %d PBN\n",
+			 port->parent, port, pbn_used, port->full_pbn);
+
+	return pbn_used;
 }
 
 static inline int
@@ -5073,9 +5135,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_mstb_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 v2 0/4] drm/dp_mst: Fix bandwidth checking regressions from DSC patches

[Hans de Goede]

Hi,

On 3/7/20 12:46 AM, 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 bandwidth-check related as far as I
> can tell (I found some other regressions unrelated to AMD's DSC patches
> which I'll be sending out patches for shortly). Note that I don't have
> any DSC displays locally yet, so if someone from AMD could sanity check
> this I would appreciate it ♥.

I can confirm that this series fixes only of the 2 FHD monitors on
my Lenovo TB3 gen 2 dock lighting up, thank you!

This series is:

Tested-by: Hans de Goede <hdegoede@redhat.com>

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 (4):
>    drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less
>      redundant
>    drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
>    drm/dp_mst: Reprobe path resources in CSN handler
>    drm/dp_mst: Rewrite and fix bandwidth limit checks
> 
>   drivers/gpu/drm/drm_dp_mst_topology.c | 185 ++++++++++++++++++--------
>   include/drm/drm_dp_mst_helper.h       |   4 +-
>   2 files changed, 129 insertions(+), 60 deletions(-)
> 

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

[PATCH v3] 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:

For starters, drm_dp_mst_atomic_check_bw_limit() determines the
pbn_limit of a branch by simply scanning each port on the current branch
device, then uses the last non-zero full_pbn value that it finds. It
then counts the sum of the PBN used on each branch device for that
level, and compares against the full_pbn value it found before.

This is wrong because ports can and will have different PBN limitations
on many hubs, especially since a number of DisplayPort hubs out there
will be clever and only use the smallest link rate required for each
downstream sink - potentially giving every port a different full_pbn
value depending on what link rate it's trained at. This means with our
current code, which max PBN value we end up with is not well defined.

Additionally, we also need to remember when checking bandwidth
limitations that the top-most device in any MST topology is a branch
device, not a port. This means that the first level of a topology
doesn't technically have a full_pbn value that needs to be checked.
Instead, we should assume that so long as our VCPI allocations fit we're
within the bandwidth limitations of the primary MSTB.

We do however, want to check full_pbn on every port including those of
the primary MSTB. However, it's important to keep in mind that this
value represents the minimum link rate /between a port's sink or mstb,
and the mstb itself/. A quick diagram to explain:

                                MSTB #1
                               /       \
                              /         \
                           Port #1    Port #2
       full_pbn for Port #1 → |          | ← full_pbn for Port #2
                           Sink #1    MSTB #2
                                         |
                                       etc...

Note that in the above diagram, the combined PBN from all VCPI
allocations on said hub should not exceed the full_pbn value of port #2,
and the display configuration on sink #1 should not exceed the full_pbn
value of port #1. However, port #1 and port #2 can otherwise consume as
much bandwidth as they want so long as their VCPI allocations still fit.

And finally - our current bandwidth checking code also makes the mistake
of not checking whether something is an end device or not before trying
to traverse down it.

So, let's fix it by rewriting our bandwidth checking helpers. We split
the function into one part for handling branches which simply adds up
the total PBN on each branch and returns it, and one for checking each
port to ensure we're not going over its PBN limit. Phew.

This should fix regressions seen, where we erroneously reject display
configurations due to thinking they're going over our bandwidth limits
when they're not.

Changes since v1:
* Took an even closer look at how PBN limitations are supposed to be
  handled, and did some experimenting with Sean Paul. Ended up rewriting
  these helpers again, but this time they should actually be correct!
Changes since v2:
* Small indenting fix
* Fix pbn_used check in drm_dp_mst_atomic_check_port_bw_limit()

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: Sean Paul <seanpaul@google.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 119 ++++++++++++++++++++------
 1 file changed, 93 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index b81ad444c24f..d2f464bdcfff 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4841,41 +4841,102 @@ 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_port_bw_limit(struct drm_dp_mst_port *port,
+				      struct drm_dp_mst_topology_state *state);
+
+static int
+drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,
+				      struct drm_dp_mst_topology_state *state)
 {
-	struct drm_dp_mst_port *port;
 	struct drm_dp_vcpi_allocation *vcpi;
-	int pbn_limit = 0, pbn_used = 0;
+	struct drm_dp_mst_port *port;
+	int pbn_used = 0, ret;
+	bool found = false;
 
-	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;
+	/* Check that we have at least one port in our state that's downstream
+	 * of this branch, otherwise we can skip this branch
+	 */
+	list_for_each_entry(vcpi, &state->vcpis, next) {
+		if (!vcpi->pbn ||
+		    !drm_dp_mst_port_downstream_of_branch(vcpi->port, mstb))
+			continue;
 
-		if (port->full_pbn > 0)
-			pbn_limit = port->full_pbn;
+		found = true;
+		break;
 	}
-	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
-			 branch, pbn_limit);
+	if (!found)
+		return 0;
 
-	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
-		if (!vcpi->pbn)
-			continue;
+	if (mstb->port_parent)
+		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] Checking bandwidth limits on [MSTB:%p]\n",
+				 mstb->port_parent->parent, mstb->port_parent,
+				 mstb);
+	else
+		DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
+				 mstb);
 
-		if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
-			pbn_used += vcpi->pbn;
+	list_for_each_entry(port, &mstb->ports, next) {
+		ret = drm_dp_mst_atomic_check_port_bw_limit(port, state);
+		if (ret < 0)
+			return ret;
+
+		pbn_used += ret;
 	}
-	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
-			 branch, pbn_used);
 
-	if (pbn_used > pbn_limit) {
-		DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n",
-				 branch);
+	return pbn_used;
+}
+
+static int
+drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
+				      struct drm_dp_mst_topology_state *state)
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	int pbn_used = 0;
+
+	if (port->pdt == DP_PEER_DEVICE_NONE)
+		return 0;
+
+	if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
+		bool found = false;
+
+		list_for_each_entry(vcpi, &state->vcpis, next) {
+			if (vcpi->port != port)
+				continue;
+			if (!vcpi->pbn)
+				return 0;
+
+			found = true;
+			break;
+		}
+		if (!found)
+			return 0;
+
+		/* This should never happen, as it means we tried to
+		 * set a mode before querying the full_pbn
+		 */
+		if (WARN_ON(!port->full_pbn))
+			return -EINVAL;
+
+		pbn_used = vcpi->pbn;
+	} else {
+		pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,
+								 state);
+		if (pbn_used <= 0)
+			return pbn_used;
+	}
+
+	if (pbn_used > port->full_pbn) {
+		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] required PBN of %d exceeds port limit of %d\n",
+				 port->parent, port, pbn_used,
+				 port->full_pbn);
 		return -ENOSPC;
 	}
-	return 0;
+
+	DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] uses %d out of %d PBN\n",
+			 port->parent, port, pbn_used, port->full_pbn);
+
+	return pbn_used;
 }
 
 static inline int
@@ -5073,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_mstb_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 v2 1/4] drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less redundant

[Alex Deucher]

On Fri, Mar 6, 2020 at 6:46 PM Lyude Paul <lyude@redhat.com> wrote:
>
> 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: Sean Paul <seanpaul@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.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 6c62ad8f4414..6714d8a5c558 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Mon, Mar 9, 2020 at 5:01 PM Lyude Paul <lyude@redhat.com> wrote:
>
> 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:
>
> For starters, drm_dp_mst_atomic_check_bw_limit() determines the
> pbn_limit of a branch by simply scanning each port on the current branch
> device, then uses the last non-zero full_pbn value that it finds. It
> then counts the sum of the PBN used on each branch device for that
> level, and compares against the full_pbn value it found before.
>
> This is wrong because ports can and will have different PBN limitations
> on many hubs, especially since a number of DisplayPort hubs out there
> will be clever and only use the smallest link rate required for each
> downstream sink - potentially giving every port a different full_pbn
> value depending on what link rate it's trained at. This means with our
> current code, which max PBN value we end up with is not well defined.
>
> Additionally, we also need to remember when checking bandwidth
> limitations that the top-most device in any MST topology is a branch
> device, not a port. This means that the first level of a topology
> doesn't technically have a full_pbn value that needs to be checked.
> Instead, we should assume that so long as our VCPI allocations fit we're
> within the bandwidth limitations of the primary MSTB.
>
> We do however, want to check full_pbn on every port including those of
> the primary MSTB. However, it's important to keep in mind that this
> value represents the minimum link rate /between a port's sink or mstb,
> and the mstb itself/. A quick diagram to explain:
>
>                                 MSTB #1
>                                /       \
>                               /         \
>                            Port #1    Port #2
>        full_pbn for Port #1 → |          | ← full_pbn for Port #2
>                            Sink #1    MSTB #2
>                                          |
>                                        etc...
>
> Note that in the above diagram, the combined PBN from all VCPI
> allocations on said hub should not exceed the full_pbn value of port #2,
> and the display configuration on sink #1 should not exceed the full_pbn
> value of port #1. However, port #1 and port #2 can otherwise consume as
> much bandwidth as they want so long as their VCPI allocations still fit.
>
> And finally - our current bandwidth checking code also makes the mistake
> of not checking whether something is an end device or not before trying
> to traverse down it.
>
> So, let's fix it by rewriting our bandwidth checking helpers. We split
> the function into one part for handling branches which simply adds up
> the total PBN on each branch and returns it, and one for checking each
> port to ensure we're not going over its PBN limit. Phew.
>
> This should fix regressions seen, where we erroneously reject display
> configurations due to thinking they're going over our bandwidth limits
> when they're not.
>
> Changes since v1:
> * Took an even closer look at how PBN limitations are supposed to be
>   handled, and did some experimenting with Sean Paul. Ended up rewriting
>   these helpers again, but this time they should actually be correct!
> Changes since v2:
> * Small indenting fix
> * Fix pbn_used check in drm_dp_mst_atomic_check_port_bw_limit()
>
> 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: Sean Paul <seanpaul@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

Thanks for the detailed descriptions.  The changes make sense to me,
but I don't know the DP MST code that well, so patches 2-4 are:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 119 ++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index b81ad444c24f..d2f464bdcfff 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4841,41 +4841,102 @@ 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_port_bw_limit(struct drm_dp_mst_port *port,
> +                                     struct drm_dp_mst_topology_state *state);
> +
> +static int
> +drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,
> +                                     struct drm_dp_mst_topology_state *state)
>  {
> -       struct drm_dp_mst_port *port;
>         struct drm_dp_vcpi_allocation *vcpi;
> -       int pbn_limit = 0, pbn_used = 0;
> +       struct drm_dp_mst_port *port;
> +       int pbn_used = 0, ret;
> +       bool found = false;
>
> -       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;
> +       /* Check that we have at least one port in our state that's downstream
> +        * of this branch, otherwise we can skip this branch
> +        */
> +       list_for_each_entry(vcpi, &state->vcpis, next) {
> +               if (!vcpi->pbn ||
> +                   !drm_dp_mst_port_downstream_of_branch(vcpi->port, mstb))
> +                       continue;
>
> -               if (port->full_pbn > 0)
> -                       pbn_limit = port->full_pbn;
> +               found = true;
> +               break;
>         }
> -       DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
> -                        branch, pbn_limit);
> +       if (!found)
> +               return 0;
>
> -       list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> -               if (!vcpi->pbn)
> -                       continue;
> +       if (mstb->port_parent)
> +               DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] Checking bandwidth limits on [MSTB:%p]\n",
> +                                mstb->port_parent->parent, mstb->port_parent,
> +                                mstb);
> +       else
> +               DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
> +                                mstb);
>
> -               if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
> -                       pbn_used += vcpi->pbn;
> +       list_for_each_entry(port, &mstb->ports, next) {
> +               ret = drm_dp_mst_atomic_check_port_bw_limit(port, state);
> +               if (ret < 0)
> +                       return ret;
> +
> +               pbn_used += ret;
>         }
> -       DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
> -                        branch, pbn_used);
>
> -       if (pbn_used > pbn_limit) {
> -               DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n",
> -                                branch);
> +       return pbn_used;
> +}
> +
> +static int
> +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
> +                                     struct drm_dp_mst_topology_state *state)
> +{
> +       struct drm_dp_vcpi_allocation *vcpi;
> +       int pbn_used = 0;
> +
> +       if (port->pdt == DP_PEER_DEVICE_NONE)
> +               return 0;
> +
> +       if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
> +               bool found = false;
> +
> +               list_for_each_entry(vcpi, &state->vcpis, next) {
> +                       if (vcpi->port != port)
> +                               continue;
> +                       if (!vcpi->pbn)
> +                               return 0;
> +
> +                       found = true;
> +                       break;
> +               }
> +               if (!found)
> +                       return 0;
> +
> +               /* This should never happen, as it means we tried to
> +                * set a mode before querying the full_pbn
> +                */
> +               if (WARN_ON(!port->full_pbn))
> +                       return -EINVAL;
> +
> +               pbn_used = vcpi->pbn;
> +       } else {
> +               pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,
> +                                                                state);
> +               if (pbn_used <= 0)
> +                       return pbn_used;
> +       }
> +
> +       if (pbn_used > port->full_pbn) {
> +               DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] required PBN of %d exceeds port limit of %d\n",
> +                                port->parent, port, pbn_used,
> +                                port->full_pbn);
>                 return -ENOSPC;
>         }
> -       return 0;
> +
> +       DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] uses %d out of %d PBN\n",
> +                        port->parent, port, pbn_used, port->full_pbn);
> +
> +       return pbn_used;
>  }
>
>  static inline int
> @@ -5073,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_mstb_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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mikita Lipski]

On 3/6/20 6:46 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 bandwidth-check related as far as I
> can tell (I found some other regressions unrelated to AMD's DSC patches
> which I'll be sending out patches for shortly). Note that I don't have
> any DSC displays locally yet, so if someone from AMD could sanity check
> this I would appreciate it ♥.

The series is tested and verified with MST DSC Realtek board.
Tested-by: Mikita Lipski <mikita.lipski@amd.com>

> 
> 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 (4):
>    drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less
>      redundant
>    drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
>    drm/dp_mst: Reprobe path resources in CSN handler
>    drm/dp_mst: Rewrite and fix bandwidth limit checks
> 
>   drivers/gpu/drm/drm_dp_mst_topology.c | 185 ++++++++++++++++++--------
>   include/drm/drm_dp_mst_helper.h       |   4 +-
>   2 files changed, 129 insertions(+), 60 deletions(-)
> 

-- 
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lipski@amd.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks

[Mikita Lipski]

On 3/6/20 6:46 PM, Lyude Paul wrote:
> DisplayPort specifications are fun. For a while, it's been really
> unclear to us what available_pbn actually does. There's a somewhat vague
> explanation in the DisplayPort spec (starting from 1.2) that partially
> explains it:
> 
>    The minimum payload bandwidth number supported by the path. Each node
>    updates this number with its available payload bandwidth number if its
>    payload bandwidth number is less than that in the Message Transaction
>    reply.
> 
> So, it sounds like available_pbn represents the smallest link rate in
> use between the source and the branch device. Cool, so full_pbn is just
> the highest possible PBN that the branch device supports right?
> 
> Well, we assumed that for quite a while until Sean Paul noticed that on
> some MST hubs, available_pbn will actually get set to 0 whenever there's
> any active payloads on the respective branch device. This caused quite a
> bit of confusion since clearing the payload ID table would end up fixing
> the available_pbn value.
> 
> So, we just went with that until commit cd82d82cbc04 ("drm/dp_mst: Add
> branch bandwidth validation to MST atomic check") started breaking
> people's setups due to us getting erroneous available_pbn values. So, we
> did some more digging and got confused until we finally looked at the
> definition for full_pbn:
> 
>    The bandwidth of the link at the trained link rate and lane count
>    between the DP Source device and the DP Sink device with no time slots
>    allocated to VC Payloads, represented as a Payload Bandwidth Number. As
>    with the Available_Payload_Bandwidth_Number, this number is determined
>    by the link with the lowest lane count and link rate.
> 
> That's what we get for not reading specs closely enough, hehe. So, since
> full_pbn is definitely what we want for doing bandwidth restriction
> checks - let's start using that instead and ignore available_pbn
> entirely.
> 


Thank you for the fix and a very detailed explanation.
I was really confused by available_pbn and why it wouldn't get updated 
and was searching for the solution in wrong places. But I'm glad you 
were able to identify the solution.
I still think the port should have an available_pbn value so it could be 
updated when driver parses RESOURCE_STATUS_NOTIFY and 
ENUM_PATH_RESOURCES messages.
With that being said it is a great find. Thanks.

Reviewed-by: Mikita Lipski <mikita.lipski@amd.com>

> 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: Hans de Goede <hdegoede@redhat.com>
> Cc: Sean Paul <sean@poorly.run>
> ---
>   drivers/gpu/drm/drm_dp_mst_topology.c | 15 +++++++--------
>   include/drm/drm_dp_mst_helper.h       |  4 ++--
>   2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 6714d8a5c558..79ebb871230b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2306,7 +2306,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>   								port);
>   			}
>   		} else {
> -			port->available_pbn = 0;
> +			port->full_pbn = 0;
>   		}
>   	}
>   
> @@ -2401,7 +2401,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
>   		if (port->ddps) {
>   			dowork = true;
>   		} else {
> -			port->available_pbn = 0;
> +			port->full_pbn = 0;
>   		}
>   	}
>   
> @@ -2553,7 +2553,7 @@ static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mg
>   		if (port->input || !port->ddps)
>   			continue;
>   
> -		if (!port->available_pbn) {
> +		if (!port->full_pbn) {
>   			drm_modeset_lock(&mgr->base.lock, NULL);
>   			drm_dp_send_enum_path_resources(mgr, mstb, port);
>   			drm_modeset_unlock(&mgr->base.lock);
> @@ -2999,8 +2999,7 @@ drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
>   				      path_res->port_number,
>   				      path_res->full_payload_bw_number,
>   				      path_res->avail_payload_bw_number);
> -			port->available_pbn =
> -				path_res->avail_payload_bw_number;
> +			port->full_pbn = path_res->full_payload_bw_number;
>   			port->fec_capable = path_res->fec_capable;
>   		}
>   	}
> @@ -3585,7 +3584,7 @@ drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb)
>   
>   	list_for_each_entry(port, &mstb->ports, next) {
>   		/* The PBN for each port will also need to be re-probed */
> -		port->available_pbn = 0;
> +		port->full_pbn = 0;
>   
>   		if (port->mstb)
>   			drm_dp_mst_topology_mgr_invalidate_mstb(port->mstb);
> @@ -4853,8 +4852,8 @@ int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,
>   			if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state))
>   				return -ENOSPC;
>   
> -		if (port->available_pbn > 0)
> -			pbn_limit = port->available_pbn;
> +		if (port->full_pbn > 0)
> +			pbn_limit = port->full_pbn;
>   	}
>   	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
>   			 branch, pbn_limit);
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 5483f888712a..1d4c996cb92d 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -81,7 +81,7 @@ struct drm_dp_vcpi {
>    * &drm_dp_mst_topology_mgr.base.lock.
>    * @num_sdp_stream_sinks: Number of stream sinks. Protected by
>    * &drm_dp_mst_topology_mgr.base.lock.
> - * @available_pbn: Available bandwidth for this port. Protected by
> + * @full_pbn: Max possible bandwidth for this port. Protected by
>    * &drm_dp_mst_topology_mgr.base.lock.
>    * @next: link to next port on this branch device
>    * @aux: i2c aux transport to talk to device connected to this port, protected
> @@ -126,7 +126,7 @@ struct drm_dp_mst_port {
>   	u8 dpcd_rev;
>   	u8 num_sdp_streams;
>   	u8 num_sdp_stream_sinks;
> -	uint16_t available_pbn;
> +	uint16_t full_pbn;
>   	struct list_head next;
>   	/**
>   	 * @mstb: the branch device connected to this port, if there is one.
> 

-- 
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lipski@amd.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mikita Lipski]

On 3/9/20 5:01 PM, Lyude Paul wrote:
> 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:
> 
> For starters, drm_dp_mst_atomic_check_bw_limit() determines the
> pbn_limit of a branch by simply scanning each port on the current branch
> device, then uses the last non-zero full_pbn value that it finds. It
> then counts the sum of the PBN used on each branch device for that
> level, and compares against the full_pbn value it found before.
> 
> This is wrong because ports can and will have different PBN limitations
> on many hubs, especially since a number of DisplayPort hubs out there
> will be clever and only use the smallest link rate required for each
> downstream sink - potentially giving every port a different full_pbn
> value depending on what link rate it's trained at. This means with our
> current code, which max PBN value we end up with is not well defined.
> 
> Additionally, we also need to remember when checking bandwidth
> limitations that the top-most device in any MST topology is a branch
> device, not a port. This means that the first level of a topology
> doesn't technically have a full_pbn value that needs to be checked.
> Instead, we should assume that so long as our VCPI allocations fit we're
> within the bandwidth limitations of the primary MSTB.
> 
> We do however, want to check full_pbn on every port including those of
> the primary MSTB. However, it's important to keep in mind that this
> value represents the minimum link rate /between a port's sink or mstb,
> and the mstb itself/. A quick diagram to explain:
> 
>                                  MSTB #1
>                                 /       \
>                                /         \
>                             Port #1    Port #2
>         full_pbn for Port #1 → |          | ← full_pbn for Port #2
>                             Sink #1    MSTB #2
>                                           |
>                                         etc...
> 
> Note that in the above diagram, the combined PBN from all VCPI
> allocations on said hub should not exceed the full_pbn value of port #2,
> and the display configuration on sink #1 should not exceed the full_pbn
> value of port #1. However, port #1 and port #2 can otherwise consume as
> much bandwidth as they want so long as their VCPI allocations still fit.
> 
> And finally - our current bandwidth checking code also makes the mistake
> of not checking whether something is an end device or not before trying
> to traverse down it.
> 
> So, let's fix it by rewriting our bandwidth checking helpers. We split
> the function into one part for handling branches which simply adds up
> the total PBN on each branch and returns it, and one for checking each
> port to ensure we're not going over its PBN limit. Phew.
> 
> This should fix regressions seen, where we erroneously reject display
> configurations due to thinking they're going over our bandwidth limits
> when they're not.
> 
> Changes since v1:
> * Took an even closer look at how PBN limitations are supposed to be
>    handled, and did some experimenting with Sean Paul. Ended up rewriting
>    these helpers again, but this time they should actually be correct!
> Changes since v2:
> * Small indenting fix
> * Fix pbn_used check in drm_dp_mst_atomic_check_port_bw_limit()
> 

Thank you for rewriting the bandwidth check helper!

My initial understanding of available_pbn was completely wrong and 
therefore the bandwidth validation was not doing what it intended.
This version is much cleaner and  easier to follow than the initial one, 
since you're separating branch and port validation into 2 different 
functions, and also go down the device topology rather than starting 
from the end nodes. Also the explanation and the diagram help a lot to 
understand how it should have be done initially.

This change makes sense and looks correct to me, therefore:
Reviewed-by: Mikita Lipski <mikita.lipski@amd.com>

Thanks,
Mikita


> 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: Sean Paul <seanpaul@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/gpu/drm/drm_dp_mst_topology.c | 119 ++++++++++++++++++++------
>   1 file changed, 93 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index b81ad444c24f..d2f464bdcfff 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4841,41 +4841,102 @@ 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_port_bw_limit(struct drm_dp_mst_port *port,
> +				      struct drm_dp_mst_topology_state *state);
> +
> +static int
> +drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,
> +				      struct drm_dp_mst_topology_state *state)
>   {
> -	struct drm_dp_mst_port *port;
>   	struct drm_dp_vcpi_allocation *vcpi;
> -	int pbn_limit = 0, pbn_used = 0;
> +	struct drm_dp_mst_port *port;
> +	int pbn_used = 0, ret;
> +	bool found = false;
>   
> -	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;
> +	/* Check that we have at least one port in our state that's downstream
> +	 * of this branch, otherwise we can skip this branch
> +	 */
> +	list_for_each_entry(vcpi, &state->vcpis, next) {
> +		if (!vcpi->pbn ||
> +		    !drm_dp_mst_port_downstream_of_branch(vcpi->port, mstb))
> +			continue;
>   
> -		if (port->full_pbn > 0)
> -			pbn_limit = port->full_pbn;
> +		found = true;
> +		break;
>   	}
> -	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
> -			 branch, pbn_limit);
> +	if (!found)
> +		return 0;
>   
> -	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> -		if (!vcpi->pbn)
> -			continue;
> +	if (mstb->port_parent)
> +		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] Checking bandwidth limits on [MSTB:%p]\n",
> +				 mstb->port_parent->parent, mstb->port_parent,
> +				 mstb);
> +	else
> +		DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
> +				 mstb);
>   
> -		if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
> -			pbn_used += vcpi->pbn;
> +	list_for_each_entry(port, &mstb->ports, next) {
> +		ret = drm_dp_mst_atomic_check_port_bw_limit(port, state);
> +		if (ret < 0)
> +			return ret;
> +
> +		pbn_used += ret;
>   	}
> -	DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
> -			 branch, pbn_used);
>   
> -	if (pbn_used > pbn_limit) {
> -		DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n",
> -				 branch);
> +	return pbn_used;
> +}
> +
> +static int
> +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
> +				      struct drm_dp_mst_topology_state *state)
> +{
> +	struct drm_dp_vcpi_allocation *vcpi;
> +	int pbn_used = 0;
> +
> +	if (port->pdt == DP_PEER_DEVICE_NONE)
> +		return 0;
> +
> +	if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
> +		bool found = false;
> +
> +		list_for_each_entry(vcpi, &state->vcpis, next) {
> +			if (vcpi->port != port)
> +				continue;
> +			if (!vcpi->pbn)
> +				return 0;
> +
> +			found = true;
> +			break;
> +		}
> +		if (!found)
> +			return 0;
> +
> +		/* This should never happen, as it means we tried to
> +		 * set a mode before querying the full_pbn
> +		 */
> +		if (WARN_ON(!port->full_pbn))
> +			return -EINVAL;
> +
> +		pbn_used = vcpi->pbn;
> +	} else {
> +		pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,
> +								 state);
> +		if (pbn_used <= 0)
> +			return pbn_used;
> +	}
> +
> +	if (pbn_used > port->full_pbn) {
> +		DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] required PBN of %d exceeds port limit of %d\n",
> +				 port->parent, port, pbn_used,
> +				 port->full_pbn);
>   		return -ENOSPC;
>   	}
> -	return 0;
> +
> +	DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] uses %d out of %d PBN\n",
> +			 port->parent, port, pbn_used, port->full_pbn);
> +
> +	return pbn_used;
>   }
>   
>   static inline int
> @@ -5073,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_mstb_bw_limit(mgr->mst_primary,
> +							    mst_state);
> +		mutex_unlock(&mgr->lock);
> +		if (ret < 0)
>   			break;
> +		else
> +			ret = 0;
>   	}
>   
>   	return ret;
> 

-- 
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lipski@amd.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel