lkml.org 
[lkml]   [2020]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
From
Date


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

\
 
 \ /
  Last update: 2020-03-10 16:39    [W:0.101 / U:0.892 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site