[v8,12/17] drm/dp_mst: Add branch bandwidth validation to MST atomic check
diff mbox series

Message ID 20191203143530.27262-13-mikita.lipski@amd.com
State New
Headers show
Series
  • DSC MST support for DRM and AMDGPU
Related show

Commit Message

Lipski, Mikita Dec. 3, 2019, 2:35 p.m. UTC
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(-)

Comments

Harry Wentland Dec. 5, 2019, 7:28 p.m. UTC | #1
On 2019-12-03 9:35 a.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 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;
>  

'ret' is unused here.

Harry

>  	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);
> +
> +	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)
> @@ -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;
> +		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;
>  };
>
Lyude Paul Dec. 7, 2019, 12:30 a.m. UTC | #2
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;
>  };

Patch
diff mbox series

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);
+
+	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)
@@ -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;
+		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;
 };