Re: [PATCH v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
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
Signed-off-by: Lyude Paul
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic
check")
Cc: Mikita Lipski
Cc: Hans de Goede
Cc: Sean Paul
---
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(>base.lock, NULL);
drm_dp_send_enum_path_resources(mgr, mstb, port);
drm_modeset_unlock(>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, >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);
[PATCH v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
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
Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic
check")
Cc: Mikita Lipski
Cc: Hans de Goede
Cc: Sean Paul
---
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(>base.lock, NULL);
drm_dp_send_enum_path_resources(mgr, mstb, port);
drm_modeset_unlock(>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, >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",
