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@xxxxxxx wrote:
> From: Mikita Lipski <mikita.lipski@xxxxxxx>
>
> 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@xxxxxxx>
> Cc: Harry Wentland <harry.wentland@xxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Signed-off-by: Mikita Lipski <mikita.lipski@xxxxxxx>
> ---
> 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;
> };
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
