Re: [PATCH v8 12/17] drm/dp_mst: Add branch bandwidth validation to MST atomic check

On Tue, 2019-12-03 at 09:35 -0500, 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 5e549f48ffb8..76bcbb4cd8b4 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4052,7 +4052,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;

• int prev_slots, prev_bw, req_slots, ret;

  topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state))

@@ -4063,6 +4063,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
  

@@ -4078,8 +4079,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;

@@ -4089,6 +4092,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) {

@@ -4101,6 +4107,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;

  return req_slots;

} @@ -4703,6 +4710,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);
  

Should fix the indenting here

• 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);
  

optional bikeshed: put branch and pbn_used on the same line

• 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) @@ -4834,6 +4894,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;

We should probably rename drm_dp_mst_atomic_check_topology_state() to check_payloads or something along those lines

And that's it! This looks awesome, with those small changes consider this:

Reviewed-by: Lyude Paul lyude@redhat.com

• 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 830c94b7f45d..2919d9776af3 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -502,6 +502,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;

};