From: Mikita Lipski <mlipski@amd.com>
To: Lyude Paul <lyude@redhat.com>, dri-devel@lists.freedesktop.org
Cc: David Airlie <airlied@linux.ie>,
linux-kernel@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
Thomas Zimmermann <tzimmermann@suse.de>,
Alex Deucher <alexander.deucher@amd.com>,
Mikita Lipski <mikita.lipski@amd.com>,
Sean Paul <sean@poorly.run>
Subject: Re: [PATCH v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks
Date: Tue, 10 Mar 2020 11:35:40 -0400 [thread overview]
Message-ID: <143de1a1-c59f-6ff0-501c-1467872bc9d9@amd.com> (raw)
In-Reply-To: <20200306234623.547525-3-lyude@redhat.com>
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
next prev parent reply other threads:[~2020-03-10 15:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-06 23:46 [PATCH v2 0/4] drm/dp_mst: Fix bandwidth checking regressions from DSC patches Lyude Paul
2020-03-06 23:46 ` [PATCH v2 1/4] drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less redundant Lyude Paul
2020-03-09 21:04 ` Alex Deucher
2020-03-06 23:46 ` [PATCH v2 2/4] drm/dp_mst: Use full_pbn instead of available_pbn for bandwidth checks Lyude Paul
2020-03-10 15:35 ` Mikita Lipski [this message]
2020-03-06 23:46 ` [PATCH v2 3/4] drm/dp_mst: Reprobe path resources in CSN handler Lyude Paul
2020-03-06 23:46 ` [PATCH v2 4/4] drm/dp_mst: Rewrite and fix bandwidth limit checks Lyude Paul
2020-03-09 21:01 ` [PATCH v3] " Lyude Paul
2020-03-09 21:15 ` Alex Deucher
2020-03-10 17:51 ` Mikita Lipski
2020-03-07 12:05 ` [PATCH v2 0/4] drm/dp_mst: Fix bandwidth checking regressions from DSC patches Hans de Goede
2020-03-10 13:01 ` Mikita Lipski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=143de1a1-c59f-6ff0-501c-1467872bc9d9@amd.com \
--to=mlipski@amd.com \
--cc=airlied@linux.ie \
--cc=alexander.deucher@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lyude@redhat.com \
--cc=mikita.lipski@amd.com \
--cc=sean@poorly.run \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).