On Fri, Mar 6, 2020 at 6:46 PM Lyude Paul lyude@redhat.com wrote:

It's already prefixed by dp_mst, so we don't really need to repeat ourselves here. One of the changes I should have picked up originally when reviewing MST DSC support.

There should be no functional changes here

Cc: Mikita Lipski mikita.lipski@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com Signed-off-by: Lyude Paul lyude@redhat.com

Reviewed-by: Alex Deucher alexander.deucher@amd.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 6c62ad8f4414..6714d8a5c558 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1937,7 +1937,7 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port *port, return parent_lct + 1; }

-static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs) +static bool drm_dp_mst_is_end_device(u8 pdt, bool mcs) { switch (pdt) { case DP_PEER_DEVICE_DP_LEGACY_CONV: @@ -1967,13 +1967,13 @@ drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,

    /* Teardown the old pdt, if there is one */
    if (port->pdt != DP_PEER_DEVICE_NONE) {

•           if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
  

•           if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
                    /*
                     * If the new PDT would also have an i2c bus,
                     * don't bother with reregistering it
                     */
                    if (new_pdt != DP_PEER_DEVICE_NONE &&
  

•                       drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs)) {
  

•                       drm_dp_mst_is_end_device(new_pdt, new_mcs)) {
                            port->pdt = new_pdt;
                            port->mcs = new_mcs;
                            return 0;
  

@@ -1993,7 +1993,7 @@ drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt, port->mcs = new_mcs;

    if (port->pdt != DP_PEER_DEVICE_NONE) {

•           if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
  

•           if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
                    /* add i2c over sideband */
                    ret = drm_dp_mst_register_i2c_bus(&port->aux);
            } else {
  

@@ -2169,7 +2169,7 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb, }

    if (port->pdt != DP_PEER_DEVICE_NONE &&

•       drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
  

•       drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
            port->cached_edid = drm_get_edid(port->connector,
                                             &port->aux.ddc);
            drm_connector_set_tile_property(port->connector);
  

-- 2.24.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

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.

On Mon, Mar 9, 2020 at 5:01 PM Lyude Paul lyude@redhat.com wrote:

Sigh, this is mostly my fault for not giving commit cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") enough scrutiny during review. The way we're checking bandwidth limitations here is mostly wrong:

For starters, drm_dp_mst_atomic_check_bw_limit() determines the pbn_limit of a branch by simply scanning each port on the current branch device, then uses the last non-zero full_pbn value that it finds. It then counts the sum of the PBN used on each branch device for that level, and compares against the full_pbn value it found before.

This is wrong because ports can and will have different PBN limitations on many hubs, especially since a number of DisplayPort hubs out there will be clever and only use the smallest link rate required for each downstream sink - potentially giving every port a different full_pbn value depending on what link rate it's trained at. This means with our current code, which max PBN value we end up with is not well defined.

Additionally, we also need to remember when checking bandwidth limitations that the top-most device in any MST topology is a branch device, not a port. This means that the first level of a topology doesn't technically have a full_pbn value that needs to be checked. Instead, we should assume that so long as our VCPI allocations fit we're within the bandwidth limitations of the primary MSTB.

We do however, want to check full_pbn on every port including those of the primary MSTB. However, it's important to keep in mind that this value represents the minimum link rate /between a port's sink or mstb, and the mstb itself/. A quick diagram to explain:

                            MSTB #1
                           /       \
                          /         \
                       Port #1    Port #2
   full_pbn for Port #1 → |          | ← full_pbn for Port #2
                       Sink #1    MSTB #2
                                     |
                                   etc...

Note that in the above diagram, the combined PBN from all VCPI allocations on said hub should not exceed the full_pbn value of port #2, and the display configuration on sink #1 should not exceed the full_pbn value of port #1. However, port #1 and port #2 can otherwise consume as much bandwidth as they want so long as their VCPI allocations still fit.

And finally - our current bandwidth checking code also makes the mistake of not checking whether something is an end device or not before trying to traverse down it.

So, let's fix it by rewriting our bandwidth checking helpers. We split the function into one part for handling branches which simply adds up the total PBN on each branch and returns it, and one for checking each port to ensure we're not going over its PBN limit. Phew.

This should fix regressions seen, where we erroneously reject display configurations due to thinking they're going over our bandwidth limits when they're not.

Changes since v1:

• Took an even closer look at how PBN limitations are supposed to be handled, and did some experimenting with Sean Paul. Ended up rewriting these helpers again, but this time they should actually be correct!

Changes since v2:

• Small indenting fix
• Fix pbn_used check in drm_dp_mst_atomic_check_port_bw_limit()

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: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com

Thanks for the detailed descriptions. The changes make sense to me, but I don't know the DP MST code that well, so patches 2-4 are: Acked-by: Alex Deucher alexander.deucher@amd.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 119 ++++++++++++++++++++------ 1 file changed, 93 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index b81ad444c24f..d2f464bdcfff 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4841,41 +4841,102 @@ static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port, return false; }

-static inline -int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch,

•                                struct drm_dp_mst_topology_state *mst_state)
  

+static int +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,

•                                 struct drm_dp_mst_topology_state *state);
  

+static int +drm_dp_mst_atomic_check_mstb_bw_limit(struct drm_dp_mst_branch *mstb,

•                                 struct drm_dp_mst_topology_state *state)
  

{

•   struct drm_dp_mst_port *port;
    struct drm_dp_vcpi_allocation *vcpi;
  

•   int pbn_limit = 0, pbn_used = 0;
  

•   struct drm_dp_mst_port *port;
  

•   int pbn_used = 0, ret;
  

•   bool found = false;
  
  

•   list_for_each_entry(port, &branch->ports, next) {
  

•           if (port->mstb)
  

•                   if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state))
  

•                           return -ENOSPC;
  

•   /* Check that we have at least one port in our state that's downstream
  

•    * of this branch, otherwise we can skip this branch
  

•    */
  

•   list_for_each_entry(vcpi, &state->vcpis, next) {
  

•           if (!vcpi->pbn ||
  

•               !drm_dp_mst_port_downstream_of_branch(vcpi->port, mstb))
  

•                   continue;
  
  

•           if (port->full_pbn > 0)
  

•                   pbn_limit = port->full_pbn;
  

•           found = true;
  

•           break;
    }
  

•   DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n",
  

•                    branch, pbn_limit);
  

•   if (!found)
  

•           return 0;
  
  

•   list_for_each_entry(vcpi, &mst_state->vcpis, next) {
  

•           if (!vcpi->pbn)
  

•                   continue;
  

•   if (mstb->port_parent)
  

•           DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] Checking bandwidth limits on [MSTB:%p]\n",
  

•                            mstb->port_parent->parent, mstb->port_parent,
  

•                            mstb);
  

•   else
  

•           DRM_DEBUG_ATOMIC("[MSTB:%p] Checking bandwidth limits\n",
  

•                            mstb);
  
  

•           if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch))
  

•                   pbn_used += vcpi->pbn;
  

•   list_for_each_entry(port, &mstb->ports, next) {
  

•           ret = drm_dp_mst_atomic_check_port_bw_limit(port, state);
  

•           if (ret < 0)
  

•                   return ret;
  

•           pbn_used += ret;
    }
  

•   DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n",
  

•                    branch, pbn_used);
  
  

•   if (pbn_used > pbn_limit) {
  

•           DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n",
  

•                            branch);
  

•   return pbn_used;
  

+}

+static int +drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,

•                                 struct drm_dp_mst_topology_state *state)
  

+{

•   struct drm_dp_vcpi_allocation *vcpi;
  

•   int pbn_used = 0;
  

•   if (port->pdt == DP_PEER_DEVICE_NONE)
  

•           return 0;
  

•   if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
  

•           bool found = false;
  

•           list_for_each_entry(vcpi, &state->vcpis, next) {
  

•                   if (vcpi->port != port)
  

•                           continue;
  

•                   if (!vcpi->pbn)
  

•                           return 0;
  

•                   found = true;
  

•                   break;
  

•           }
  

•           if (!found)
  

•                   return 0;
  

•           /* This should never happen, as it means we tried to
  

•            * set a mode before querying the full_pbn
  

•            */
  

•           if (WARN_ON(!port->full_pbn))
  

•                   return -EINVAL;
  

•           pbn_used = vcpi->pbn;
  

•   } else {
  

•           pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,
  

•                                                            state);
  

•           if (pbn_used <= 0)
  

•                   return pbn_used;
  

•   }
  

•   if (pbn_used > port->full_pbn) {
  

•           DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] required PBN of %d exceeds port limit of %d\n",
  

•                            port->parent, port, pbn_used,
  

•                            port->full_pbn);
            return -ENOSPC;
    }
  

•   return 0;
  

•   DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] uses %d out of %d PBN\n",
  

•                    port->parent, port, pbn_used, port->full_pbn);
  

•   return pbn_used;
  

}

static inline int @@ -5073,9 +5134,15 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) ret = drm_dp_mst_atomic_check_vcpi_alloc_limit(mgr, mst_state); if (ret) break;

•           ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, mst_state);
  

•           if (ret)
  

•           mutex_lock(&mgr->lock);
  

•           ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary,
  

•                                                       mst_state);
  

•           mutex_unlock(&mgr->lock);
  

•           if (ret < 0)
                    break;
  

•           else
  

•                   ret = 0;
    }
  
    return ret;
  

-- 2.24.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel