lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Mar 2020 11:35:40 -0400 From: Mikita Lipski <mlipski@....com> To: Lyude Paul <lyude@...hat.com>, dri-devel@...ts.freedesktop.org Cc: Mikita Lipski <mikita.lipski@....com>, Hans de Goede <hdegoede@...hat.com>, Sean Paul <sean@...rly.run>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>, Alex Deucher <alexander.deucher@....com>, linux-kernel@...r.kernel.org Subject: 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 <mikita.lipski@....com> > Signed-off-by: Lyude Paul <lyude@...hat.com> > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") > Cc: Mikita Lipski <mikita.lipski@....com> > Cc: Hans de Goede <hdegoede@...hat.com> > Cc: Sean Paul <sean@...rly.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@....com
Powered by blists - more mailing lists