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.lip...@amd.com>
Signed-off-by: Lyude Paul <ly...@redhat.com>
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic
check")
Cc: Mikita Lipski <mikita.lip...@amd.com>
Cc: Hans de Goede <hdego...@redhat.com>
Cc: Sean Paul <s...@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.lip...@amd.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel