| Message ID | 20191116220128.16598-13-mikita.lipski@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSC MST support for DRM and AMDGPU | expand |
I'm not well versed in MST bw validation, which might explain my confusion here - so bear with me :) ... On 2019-11-16 5:01 p.m., mikita.lipski@amd.com wrote: > From: Mikita Lipski <mikita.lipski@amd.com> > > Adding PBN attribute to drm_dp_vcpi_allocation structure to > keep track of how much bandwidth each Port requires. > Adding drm_dp_mst_atomic_check_bw_limit to verify that > state's bandwidth needs doesn't exceed available bandwidth. > The funtion is called in drm_dp_mst_atomic_check after > drm_dp_mst_atomic_check_topology_state to fully verify that > the proposed topology is supported. > > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Lyude Paul <lyude@redhat.com> > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 67 ++++++++++++++++++++++++++- > include/drm/drm_dp_mst_helper.h | 1 + > 2 files changed, 66 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 98cc93d5ddd7..5072c1e3dcfe 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -3243,7 +3243,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, > { > struct drm_dp_mst_topology_state *topology_state; > struct drm_dp_vcpi_allocation *pos, *vcpi = NULL; > - int prev_slots, req_slots, ret; > + int prev_slots, prev_bw, req_slots, ret; > > topology_state = drm_atomic_get_mst_topology_state(state, mgr); > if (IS_ERR(topology_state)) > @@ -3254,6 +3254,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, > if (pos->port == port) { > vcpi = pos; > prev_slots = vcpi->vcpi; > + prev_bw = vcpi->pbn; > > /* > * This should never happen, unless the driver tries > @@ -3269,8 +3270,10 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, > break; > } > } > - if (!vcpi) > + if (!vcpi) { > prev_slots = 0; > + prev_bw = 0; > + } > > if (pbn_div <= 0) > pbn_div = mgr->pbn_div; > @@ -3280,6 +3283,9 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, > DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] VCPI %d -> %d\n", > port->connector->base.id, port->connector->name, > port, prev_slots, req_slots); > + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] PBN %d -> %d\n", > + port->connector->base.id, port->connector->name, > + port, prev_bw, pbn); > > /* Add the new allocation to the state */ > if (!vcpi) { > @@ -3292,6 +3298,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, > list_add(&vcpi->next, &topology_state->vcpis); > } > vcpi->vcpi = req_slots; > + vcpi->pbn = pbn; > > ret = req_slots; > return ret; > @@ -3837,6 +3844,59 @@ static void drm_dp_mst_destroy_state(struct drm_private_obj *obj, > kfree(mst_state); > } > > +static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port, > + struct drm_dp_mst_branch *branch) > +{ > + while (port->parent) { > + if (port->parent == branch) > + return true; > + > + if (port->parent->port_parent) > + port = port->parent->port_parent; > + else > + break; > + } > + 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) > +{ > + struct drm_dp_mst_port *port; > + struct drm_dp_vcpi_allocation *vcpi; > + int pbn_limit = 0, pbn_used = 0; > + > + list_for_each_entry(port, &branch->ports, next) { > + if (port->mstb) { > + if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state)) > + return -EINVAL; > + } > + if (port->available_pbn > 0) > + pbn_limit = port->available_pbn; Shouldn't the pbn_limit be the pbn limit of *this* mstb's upstream port (branch->port_parent) and not the downstream ports? Otherwise, wouldn't this mean we're only considering the 'last' downstream port in this mstb when saving the pbn_limit? (or perhapse you meant to use '+=' rather than '=') > + } > + DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n", > + branch, > + pbn_limit); > + > + list_for_each_entry(vcpi, &mst_state->vcpis, next) { > + if (!vcpi->pbn) > + continue; > + > + if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch)) > + pbn_used += vcpi->pbn; > + } It's not clear to me how this will behave when recursively called. The above iteration over branch->ports looks to be checking that a downstream MSTB is bandwidth-valid. Wouldn't this iteration here repeat the validation work again, since drm_dp_mst_port_downstream_of_branch() will return true for *all* downstream ports (including the ones we recursed on above)? It wouldn't be wrong exactly, but not sure if that's intended. Thanks, Leo > + 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 -EINVAL; > + } > + return 0; > +} > + > static inline int > drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_topology_state *mst_state) > @@ -3968,6 +4028,9 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) > ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state); > if (ret) > break; > + ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, mst_state); > + if (ret) > + break; > } > > return ret; > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index b1b00de3083b..5a119a40ed5a 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -431,6 +431,7 @@ struct drm_dp_payload { > struct drm_dp_vcpi_allocation { > struct drm_dp_mst_port *port; > int vcpi; > + int pbn; > bool dsc_enabled; > struct list_head next; > }; >
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 98cc93d5ddd7..5072c1e3dcfe 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3243,7 +3243,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, { struct drm_dp_mst_topology_state *topology_state; struct drm_dp_vcpi_allocation *pos, *vcpi = NULL; - int prev_slots, req_slots, ret; + int prev_slots, prev_bw, req_slots, ret; topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state)) @@ -3254,6 +3254,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, if (pos->port == port) { vcpi = pos; prev_slots = vcpi->vcpi; + prev_bw = vcpi->pbn; /* * This should never happen, unless the driver tries @@ -3269,8 +3270,10 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, break; } } - if (!vcpi) + if (!vcpi) { prev_slots = 0; + prev_bw = 0; + } if (pbn_div <= 0) pbn_div = mgr->pbn_div; @@ -3280,6 +3283,9 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] VCPI %d -> %d\n", port->connector->base.id, port->connector->name, port, prev_slots, req_slots); + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] PBN %d -> %d\n", + port->connector->base.id, port->connector->name, + port, prev_bw, pbn); /* Add the new allocation to the state */ if (!vcpi) { @@ -3292,6 +3298,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, list_add(&vcpi->next, &topology_state->vcpis); } vcpi->vcpi = req_slots; + vcpi->pbn = pbn; ret = req_slots; return ret; @@ -3837,6 +3844,59 @@ static void drm_dp_mst_destroy_state(struct drm_private_obj *obj, kfree(mst_state); } +static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port, + struct drm_dp_mst_branch *branch) +{ + while (port->parent) { + if (port->parent == branch) + return true; + + if (port->parent->port_parent) + port = port->parent->port_parent; + else + break; + } + 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) +{ + struct drm_dp_mst_port *port; + struct drm_dp_vcpi_allocation *vcpi; + int pbn_limit = 0, pbn_used = 0; + + list_for_each_entry(port, &branch->ports, next) { + if (port->mstb) { + if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state)) + return -EINVAL; + } + if (port->available_pbn > 0) + pbn_limit = port->available_pbn; + } + DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n", + branch, + pbn_limit); + + list_for_each_entry(vcpi, &mst_state->vcpis, next) { + if (!vcpi->pbn) + continue; + + if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch)) + pbn_used += vcpi->pbn; + } + 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 -EINVAL; + } + return 0; +} + static inline int drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_topology_state *mst_state) @@ -3968,6 +4028,9 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state); if (ret) break; + ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, mst_state); + if (ret) + break; } return ret; diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index b1b00de3083b..5a119a40ed5a 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -431,6 +431,7 @@ struct drm_dp_payload { struct drm_dp_vcpi_allocation { struct drm_dp_mst_port *port; int vcpi; + int pbn; bool dsc_enabled; struct list_head next; };