drm/dp_mst: Improve VCPI helpers, use in nouveau

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

---
 include/drm/drm_dp_mst_helper.h | 77 +++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

include/drm/drm_dp_mst_helper.h +31 -0

index 59f005b419cf..3faceb66f5cb 100644
@@ -628,4 +628,81 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_port *port, bool power_up);
 
+extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
+
+static inline bool
+__drm_dp_mst_state_iter_get(struct drm_atomic_state *state,
+			    struct drm_dp_mst_topology_mgr **mgr,
+			    struct drm_dp_mst_topology_state **old_state,
+			    struct drm_dp_mst_topology_state **new_state,
+			    int i)
+{
+	struct __drm_private_objs_state *objs_state = &state->private_objs[i];
+
+	if (objs_state->ptr->funcs != &drm_dp_mst_topology_state_funcs)
+		return false;
+
+	*mgr = to_dp_mst_topology_mgr(objs_state->ptr);
+	if (old_state)
+		*old_state = to_dp_mst_topology_state(objs_state->old_state);
+	if (new_state)
+		*new_state = to_dp_mst_topology_state(objs_state->new_state);
+
+	return true;
+}
+
+/**
+ * for_each_oldnew_mst_mgr_in_state - iterate over all DP MST topology
+ * managers in an atomic update
+ * @__state: &struct drm_atomic_state pointer
+ * @mgr: &struct drm_dp_mst_topology_mgr iteration cursor
+ * @old_state: &struct drm_dp_mst_topology_state iteration cursor for the old
+ * state
+ * @new_state: &struct drm_dp_mst_topology_state iteration cursor for the new

include/drm/drm_dp_mst_helper.h +36 -0

+
+/**
+ * for_each_old_mst_mgr_in_state - iterate over all DP MST topology managers
+ * in an atomic update
+ * @__state: &struct drm_atomic_state pointer
+ * @mgr: &struct drm_dp_mst_topology_mgr iteration cursor
+ * @old_state: &struct drm_dp_mst_topology_state iteration cursor for the old
+ * state
+ * @__i: int iteration cursor, for macro-internal use
+ *
+ * This iterates over all DRM DP MST topology managers in an atomic update,
+ * tracking only the old state. This is useful in disable functions, where we
+ * need the old state the hardware is still in.
+ */
+#define for_each_old_mst_mgr_in_state(__state, mgr, old_state, __i) \
+	for ((__i) = 0; (__i) < (__state)->num_private_objs; (__i)++) \
+		for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), &(old_state), NULL, (__i)))
+
+/**
+ * for_each_new_mst_mgr_in_state - iterate over all DP MST topology managers
+ * in an atomic update
+ * @__state: &struct drm_atomic_state pointer
+ * @mgr: &struct drm_dp_mst_topology_mgr iteration cursor
+ * @new_state: &struct drm_dp_mst_topology_state iteration cursor for the new
+ * state
+ * @__i: int iteration cursor, for macro-internal use
+ *
+ * This iterates over all DRM DP MST topology managers in an atomic update,
+ * tracking only the new state. This is useful in enable functions, where we
+ * need the new state the hardware should be in when the atomic commit
+ * operation has completed.
+ */
+#define for_each_new_mst_mgr_in_state(__state, mgr, new_state, __i) \
+	for ((__i) = 0; (__i) < (__state)->num_private_objs; (__i)++) \
+		for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), NULL, &(new_state), (__i)))
+
 #endif

Signature · 1 line

2.17.2

There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:

/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */

That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.

So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.

Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.

Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.

Changes since v1:
- Don't use the now-removed ->atomic_check() for private objects hook, just give drivers a function to call themselves

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/drm_dp_mst_topology.c | 190 +++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c  |   8 ++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 +++--
 include/drm/drm_dp_mst_helper.h       |  11 +-
 4 files changed, 192 insertions(+), 48 deletions(-)

drivers/gpu/drm/drm_dp_mst_topology.c +0 -0

index 8c3cfac437f4..dcfab7536914 100644
@@ -2614,21 +2614,33 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 }
 
 /**

drivers/gpu/drm/drm_dp_mst_topology.c +5 -8

+ *
  * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * Total slots in the atomic state assigned for this port, or a negative error
+ * code if the port no longer exists
  */
 int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn)
 {
 	struct drm_dp_mst_topology_state *topology_state;
-	int req_slots;
+	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+	int prev_slots, req_slots, ret;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))

@@ -2637,20 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;

drivers/gpu/drm/drm_dp_mst_topology.c +21 -4

+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
 
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
 

@@ -2658,32 +2691,46 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
  * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
  * @state: global atomic state
  * @mgr: MST topology manager for the port
- * @slots: number of vcpi slots to release

drivers/gpu/drm/drm_dp_mst_topology.c +24 -14

  *
  * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
- * negative error code
+ * 0 if all slots for this port were added back to
+ * &drm_dp_mst_topology_state->avail_slots or negative error code
  */
 int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots)
+				     struct drm_dp_mst_port *port)
 {
 	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_dp_vcpi_allocation *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3159,50 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 static struct drm_private_state *
 drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
 {
-	struct drm_dp_mst_topology_state *state;
+	struct drm_dp_mst_topology_state *state, *old_state =
+		to_dp_mst_topology_state(obj->state);
+	struct drm_dp_mst_topology_mgr *mgr = old_state->mgr;
+	struct drm_dp_mst_port *port;
+	struct drm_dp_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	state->mgr = mgr;

drivers/gpu/drm/drm_dp_mst_topology.c +18 -0

+		if (!vcpi) {
+			drm_dp_put_port(port);
+			goto fail_alloc;
+		}
+
+		vcpi->port = port;
+		vcpi->vcpi = pos->vcpi;
+		list_add(&vcpi->next, &state->vcpis);
+		drm_dp_put_port(port);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

drivers/gpu/drm/drm_dp_mst_topology.c +3 -1

+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next)
+		kfree(pos);
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+/**

drivers/gpu/drm/drm_dp_mst_topology.c +19 -0

+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */
+int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr = state->mgr;
+	struct drm_dp_vcpi_allocation *pos;
+	int avail_slots = 63;
+
+	list_for_each_entry(pos, &state->vcpis, next) {
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+				 pos->port, pos->vcpi);
+
+		avail_slots -= pos->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough vcpi slots in state %p (avail=%d)\n",
+					 pos->port, state,

drivers/gpu/drm/drm_dp_mst_topology.c +5 -4

+
+const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs = {
 	.atomic_duplicate_state = drm_dp_mst_duplicate_state,
 	.atomic_destroy_state = drm_dp_mst_destroy_state,
 };
+EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
 
 /**
  * drm_atomic_get_mst_topology_state: get MST topology state

@@ -3213,13 +3341,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 		return -ENOMEM;
 
 	mst_state->mgr = mgr;
-
-	/* max. time slots - one slot for MTP header */
-	mst_state->avail_slots = 63;
+	INIT_LIST_HEAD(&mst_state->vcpis);
 
 	drm_atomic_private_obj_init(&mgr->base,
 				    &mst_state->base,
-				    &mst_state_funcs);
+				    &drm_dp_mst_topology_state_funcs);
 
 	return 0;
 }

drivers/gpu/drm/i915/intel_display.c +8 -0

index fe045abb6472..f66af1465686 100644
@@ -12484,6 +12484,8 @@ static int intel_atomic_check(struct drm_device *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *crtc_state;
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
 	int ret, i;
 	bool any_ms = false;
 

@@ -12534,6 +12536,12 @@ static int intel_atomic_check(struct drm_device *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check(mst_state);
+		if (ret)
+			return ret;
+	}
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);
 

drivers/gpu/drm/i915/intel_dp_mst.c +17 -14

index 8b71d64ebd9d..aaf904738b78 100644
@@ -114,28 +114,31 @@ static int intel_dp_mst_atomic_check(struct drm_connector *connector,
 	struct drm_connector_state *old_conn_state;
 	struct drm_crtc *old_crtc;
 	struct drm_crtc_state *crtc_state;
-	int slots, ret = 0;
+	struct intel_connector *intel_connector =
+		to_intel_connector(connector);
+	struct drm_dp_mst_topology_mgr *mgr =
+		&intel_connector->mst_port->mst_mgr;
+	struct drm_dp_mst_port *port = intel_connector->port;
+	int ret = 0;
 
 	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
 	old_crtc = old_conn_state->crtc;
 	if (!old_crtc)
 		return ret;
 
-	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
-	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
-	if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
-		struct drm_dp_mst_topology_mgr *mgr;
-		struct drm_encoder *old_encoder;
+	/* Free VCPI, since compute_config() won't be run */
+	if (!new_conn_state->crtc) {
+		crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
 
-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
-
-		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
-		if (ret)
-			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
-		else
-			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
+		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, port);
+		if (ret) {
+			DRM_DEBUG_KMS("failed releasing vcpi slots: %d\n",
+				      ret);
+			return ret;
+		}
+		to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
 	}
+
 	return ret;
 }
 

include/drm/drm_dp_mst_helper.h +7 -1

index 3faceb66f5cb..0aa7d3658013 100644
@@ -406,9 +406,15 @@ struct drm_dp_payload {
 
 #define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
 
+struct drm_dp_vcpi_allocation {
+	struct drm_dp_mst_port *port;
+	int vcpi;
+	struct list_head next;
+};
+
 struct drm_dp_mst_topology_state {
 	struct drm_private_state base;
-	int avail_slots;
+	struct list_head vcpis;
 	struct drm_dp_mst_topology_mgr *mgr;
 };
 

include/drm/drm_dp_mst_helper.h +0 -0

 extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
 

Signature · 1 line

2.17.2

Quoted 66 lines · Lyude Paul

There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:

/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */

That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.

So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.

Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.

Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.

Changes since v1:
- Don't use the now-removed ->atomic_check() for private objects hook, just give drivers a function to call themselves

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/drm_dp_mst_topology.c | 190 +++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c  |   8 ++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 +++--
 include/drm/drm_dp_mst_helper.h       |  11 +-
 4 files changed, 192 insertions(+), 48 deletions(-)

drivers/gpu/drm/drm_dp_mst_topology.c +2 -1

index 8c3cfac437f4..dcfab7536914 100644
@@ -2614,21 +2614,33 @@ static int drm_dp_init_vcpi(struct
 drm_dp_mst_topology_mgr *mgr,
 }
 
 /**
- * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the state
  * @state: global atomic state
  * @mgr: MST topology manager for the port
  * @port: port to find vcpi slots for
  * @pbn: bandwidth required for the mode in PBN
  *
+ * Allocates VCPI slots to @port, replacing any previous VCPI allocations

Quoted 5 lines

it
+ * may have had. Any atomic drivers which support MST must call this function
+ * in their atomic_check() handlers to change the current VCPI allocation for

Maybe do a nice kerneldoc reference to the right atomic_check here.

Quoted 2 lines

+ * the new state. After the ->atomic_check() hooks of the driver and all other

This will upset the kerneldoc parser I think.

Quoted 7 lines

+ * mode objects in the state have been called, DRM will check the final VCPI
+ * allocations to ensure that they will fit into the available bandwidth on
+ * the topology.
+ *
+ * See also: drm_dp_atomic_release_vcpi_slots()

Also need to reference drm_dp_mst_atomic_check() here and that drivers must call it or nothing happens.

Quoted 36 lines

+ *
* RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * Total slots in the atomic state assigned for this port, or a negative error
+ * code if the port no longer exists
*/
int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,

drivers/gpu/drm/drm_dp_mst_topology.c +8 -7

 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn)
 {
 	struct drm_dp_mst_topology_state *topology_state;
-	int req_slots;
+	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+	int prev_slots, req_slots, ret;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))

@@ -2637,20 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
 drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;

drivers/gpu/drm/drm_dp_mst_topology.c int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, · line 2662

Quoted 4 lines

drivers/gpu/drm/drm_dp_mst_topology.c +3 -0

+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;

For robustness should we warn here, since drivers forgetting to release vcpi slots is kinda a bug? Or do we need to have this to make life easier for driver writers?

Quoted 41 lines

drivers/gpu/drm/drm_dp_mst_topology.c +23 -4

+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
 
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);

@@ -2658,32 +2691,46 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
  * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
  * @state: global atomic state
  * @mgr: MST topology manager for the port
- * @slots: number of vcpi slots to release
+ *
+ * Releases any VCPI slots that have been allocated to a port in the

atomic
+ * state. Any atomic drivers which support MST must call this function in
+ * their connector's atomic_check() handler when the connector will no

Quoted 8 lines

longer
+ * have VCPI allocated (e.g. because it's CRTC was removed).
+ *
+ * It is OK to call this even if @port has been removed from the system, in
+ * which case it will just amount to a no-op.
+ *
+ * See also: drm_dp_atomic_find_vcpi_slots()

Same comments as with the _find_vcpi_slots() function.

Quoted 67 lines

*
* RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
- * negative error code
+ * 0 if all slots for this port were added back to
+ * &drm_dp_mst_topology_state->avail_slots or negative error code */
int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,

drivers/gpu/drm/drm_dp_mst_topology.c +23 -12

 				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots)
+				     struct drm_dp_mst_port *port)
 {
 	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_dp_vcpi_allocation *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3159,50 @@ static void drm_dp_destroy_connector_work(struct
 work_struct *work)
 static struct drm_private_state *
 drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
 {
-	struct drm_dp_mst_topology_state *state;
+	struct drm_dp_mst_topology_state *state, *old_state =
+		to_dp_mst_topology_state(obj->state);
+	struct drm_dp_mst_topology_mgr *mgr = old_state->mgr;
+	struct drm_dp_mst_port *port;
+	struct drm_dp_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	state->mgr = mgr;
+	INIT_LIST_HEAD(&state->vcpis);

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_destroy_connector_work(struct · line 3186

Quoted 10 lines

drivers/gpu/drm/drm_dp_mst_topology.c +9 -0

+
+	/* Copy over the VCPI allocations for ports that still exist */
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		port = drm_dp_get_validated_port_ref(mgr, pos->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone, vcpi %d ->
 0\n",
+					 pos->port, pos->vcpi);
+			continue;
+		}

Hm, in general we have 0 validation in the duplicate_state functions, Just dump copying, with minimal pointer/refcount updates.

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_destroy_connector_work(struct · line 3188

Quoted 2 lines

drivers/gpu/drm/drm_dp_mst_topology.c +2 -0

+
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);

kmemdump + updating the list. Just in case we ever add more stuff here. Probably needs an explicit memset to avoid list debugging.

At least that's the style we use everywhere else.

Quoted 23 lines

drivers/gpu/drm/drm_dp_mst_topology.c +18 -0

+		if (!vcpi) {
+			drm_dp_put_port(port);
+			goto fail_alloc;
+		}
+
+		vcpi->port = port;
+		vcpi->vcpi = pos->vcpi;
+		list_add(&vcpi->next, &state->vcpis);
+		drm_dp_put_port(port);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }

static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_mst_destroy_state(struct · line 3215

Quoted 6 lines

drivers/gpu/drm/drm_dp_mst_topology.c +2 -0

@@ -3128,14 +3210,60 @@ static void drm_dp_mst_destroy_state(struct
 drm_private_obj *obj,
 {
 	struct drm_dp_mst_topology_state *mst_state =
 		to_dp_mst_topology_state(state);
+	struct drm_dp_vcpi_allocation *pos, *tmp;
+

WARN_ON(!list_empty());

I think this not being empty on mgr teardown would be a driver bug. All the ports/branch devices should be gone by now, and also all the modeset state should have been torn down, using drm_atomic_helper_shutdown() or friends.

Quoted 13 lines

drivers/gpu/drm/drm_dp_mst_topology.c +4 -1

+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next)
+		kfree(pos);
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+/**
+ * drm_dp_mst_atomic_check - Check that the new state of an MST topology

in an
+ * atomic update is valid
+ * @state: Pointer to the new &struct drm_dp_mst_topology_state
+ *

Quoted 8 lines

+ * Checks the given topology state for an atomic update to ensure that it's
+ * valid. This includes checking whether there's enough bandwidth to support
+ * the new VCPI allocations in the atomic update.
+ *
+ * Any atomic drivers supporting DP MST must make sure to call this after
+ * checking the rest of their state in their ->atomic_check() callback.

&drm_mode_config_funcs.atomic_check

Quoted 20 lines

+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */

drivers/gpu/drm/drm_dp_mst_topology.c +13 -0

+int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr = state->mgr;
+	struct drm_dp_vcpi_allocation *pos;
+	int avail_slots = 63;
+
+	list_for_each_entry(pos, &state->vcpis, next) {
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+				 pos->port, pos->vcpi);
+
+		avail_slots -= pos->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough vcpi slots

in state %p (avail=%d)\n",

drivers/gpu/drm/drm_dp_mst_topology.c +1 -0

+					 pos->port, state,

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_mst_destroy_state(struct drm_private_obj *obj, · line 3260

Quoted 10 lines

drivers/gpu/drm/drm_dp_mst_topology.c +10 -0

+					 avail_slots + pos->vcpi);
+			return -ENOSPC;
+		}
+	}
+	DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p vcpi avail=%d used=%d\n",
+			 mgr, state, avail_slots, 63 - avail_slots);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);

Commented on patch 4 already, but I think this would look a bit simpler if we (also/instead, up to you really) expose a helper that checks all mgr in a drm_atomic_state. No need to have the exact same loop in each driver.

Quoted 132 lines

drivers/gpu/drm/drm_dp_mst_topology.c +5 -4

+
+const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs = {
 	.atomic_duplicate_state = drm_dp_mst_duplicate_state,
 	.atomic_destroy_state = drm_dp_mst_destroy_state,
 };
+EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
 
 /**
  * drm_atomic_get_mst_topology_state: get MST topology state

@@ -3213,13 +3341,11 @@ int drm_dp_mst_topology_mgr_init(struct
 drm_dp_mst_topology_mgr *mgr,
 		return -ENOMEM;
 
 	mst_state->mgr = mgr;
-
-	/* max. time slots - one slot for MTP header */
-	mst_state->avail_slots = 63;
+	INIT_LIST_HEAD(&mst_state->vcpis);
 
 	drm_atomic_private_obj_init(&mgr->base,
 				    &mst_state->base,
-				    &mst_state_funcs);
+				    &drm_dp_mst_topology_state_funcs);
 
 	return 0;

}

drivers/gpu/drm/i915/intel_display.c +8 -0

index fe045abb6472..f66af1465686 100644
@@ -12484,6 +12484,8 @@ static int intel_atomic_check(struct drm_device
 *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *crtc_state;
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
 	int ret, i;
 	bool any_ms = false;

@@ -12534,6 +12536,12 @@ static int intel_atomic_check(struct drm_device
 *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check(mst_state);
+		if (ret)
+			return ret;
+	}
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

drivers/gpu/drm/i915/intel_dp_mst.c +17 -14

index 8b71d64ebd9d..aaf904738b78 100644
@@ -114,28 +114,31 @@ static int intel_dp_mst_atomic_check(struct
 drm_connector *connector,
 	struct drm_connector_state *old_conn_state;
 	struct drm_crtc *old_crtc;
 	struct drm_crtc_state *crtc_state;
-	int slots, ret = 0;
+	struct intel_connector *intel_connector =
+		to_intel_connector(connector);
+	struct drm_dp_mst_topology_mgr *mgr =
+		&intel_connector->mst_port->mst_mgr;
+	struct drm_dp_mst_port *port = intel_connector->port;
+	int ret = 0;
 
 	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
 	old_crtc = old_conn_state->crtc;
 	if (!old_crtc)
 		return ret;
 
-	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
-	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
-	if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
-		struct drm_dp_mst_topology_mgr *mgr;
-		struct drm_encoder *old_encoder;
+	/* Free VCPI, since compute_config() won't be run */
+	if (!new_conn_state->crtc) {
+		crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
 
-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
-
-		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
-		if (ret)
-			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n",
 slots, ret);
-		else
-			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
+		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, port);
+		if (ret) {
+			DRM_DEBUG_KMS("failed releasing vcpi slots: %d\n",
+				      ret);
+			return ret;
+		}
+		to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
 	}
+
 	return ret;

}

include/drm/drm_dp_mst_helper.h +7 -1

index 3faceb66f5cb..0aa7d3658013 100644
@@ -406,9 +406,15 @@ struct drm_dp_payload {
 
 #define to_dp_mst_topology_state(x) container_of(x, struct
 drm_dp_mst_topology_state, base)
 
+struct drm_dp_vcpi_allocation {
+	struct drm_dp_mst_port *port;
+	int vcpi;
+	struct list_head next;
+};
+
 struct drm_dp_mst_topology_state {
 	struct drm_private_state base;
-	int avail_slots;
+	struct list_head vcpis;
 	struct drm_dp_mst_topology_mgr *mgr;
 };

include/drm/drm_dp_mst_helper.h int drm_dp_atomic_find_vcpi_slots(struct · line 631

Quoted 2 lines

include/drm/drm_dp_mst_helper.h +0 -0

@@ -624,9 +630,10 @@ int drm_dp_atomic_find_vcpi_slots(struct
 drm_atomic_state *state,
 				  struct drm_dp_mst_port *port, int pbn);

I think annotating the relase/find functions with __must_check would be good.

include/drm/drm_dp_mst_helper.h int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, · line 635

Quoted 6 lines

include/drm/drm_dp_mst_helper.h +1 -1

 int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots);
+				     struct drm_dp_mst_port *port);
 int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_port *port, bool power_up);

Same here.

include/drm/drm_dp_mst_helper.h int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, · line 636

Quoted 1 line

include/drm/drm_dp_mst_helper.h +1 -0

+int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state);

With the nits addressed:

Reviewed-by Daniel Vetter <[email protected]>

Quoted 7 lines

include/drm/drm_dp_mst_helper.h +0 -0

 extern const struct drm_private_state_funcs
 drm_dp_mst_topology_state_funcs;

Signature · 2 lines

-- 
2.17.2

Quoted 56 lines · Lyude Paul

There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:

/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */

That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.

So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.

Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.

Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.

Changes since v1:
- Don't use the now-removed ->atomic_check() for private objects hook, just give drivers a function to call themselves

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/drm_dp_mst_topology.c | 190 +++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c  |   8 ++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 +++--
 include/drm/drm_dp_mst_helper.h       |  11 +-
 4 files changed, 192 insertions(+), 48 deletions(-)

drivers/gpu/drm/drm_dp_mst_topology.c +0 -0

index 8c3cfac437f4..dcfab7536914 100644
@@ -2614,21 +2614,33 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 }
 
 /**

drivers/gpu/drm/drm_dp_mst_topology.c static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, · line 2625

Quoted 10 lines

drivers/gpu/drm/drm_dp_mst_topology.c +4 -1

- * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the state
  * @state: global atomic state
  * @mgr: MST topology manager for the port
  * @port: port to find vcpi slots for
  * @pbn: bandwidth required for the mode in PBN
  *
+ * Allocates VCPI slots to @port, replacing any previous VCPI allocations it
+ * may have had. Any atomic drivers which support MST must call this function
+ * in their atomic_check() handlers to change the current VCPI allocation for

Maybe do a nice kerneldoc reference to the right atomic_check here.

drivers/gpu/drm/drm_dp_mst_topology.c static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, · line 2626

Quoted 1 line

drivers/gpu/drm/drm_dp_mst_topology.c +1 -0

+ * the new state. After the ->atomic_check() hooks of the driver and all other

This will upset the kerneldoc parser I think.

drivers/gpu/drm/drm_dp_mst_topology.c static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, · line 2631

Quoted 5 lines

drivers/gpu/drm/drm_dp_mst_topology.c +5 -0

+ * mode objects in the state have been called, DRM will check the final VCPI
+ * allocations to ensure that they will fit into the available bandwidth on
+ * the topology.
+ *
+ * See also: drm_dp_atomic_release_vcpi_slots()

Also need to reference drm_dp_mst_atomic_check() here and that drivers must call it or nothing happens.

Quoted 28 lines

drivers/gpu/drm/drm_dp_mst_topology.c +5 -8

+ *
  * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * Total slots in the atomic state assigned for this port, or a negative error
+ * code if the port no longer exists
  */
 int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn)
 {
 	struct drm_dp_mst_topology_state *topology_state;
-	int req_slots;
+	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+	int prev_slots, req_slots, ret;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))

@@ -2637,20 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;

drivers/gpu/drm/drm_dp_mst_topology.c int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, · line 2662

Quoted 10 lines

drivers/gpu/drm/drm_dp_mst_topology.c +9 -0

+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;

For robustness should we warn here, since drivers forgetting to release vcpi slots is kinda a bug? Or do we need to have this to make life easier for driver writers?

Quoted 35 lines

drivers/gpu/drm/drm_dp_mst_topology.c +21 -4

+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
 
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
 

@@ -2658,32 +2691,46 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
  * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
  * @state: global atomic state
  * @mgr: MST topology manager for the port
- * @slots: number of vcpi slots to release

drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2703

Quoted 10 lines

drivers/gpu/drm/drm_dp_mst_topology.c +10 -0

+ *
+ * Releases any VCPI slots that have been allocated to a port in the atomic
+ * state. Any atomic drivers which support MST must call this function in
+ * their connector's atomic_check() handler when the connector will no longer
+ * have VCPI allocated (e.g. because it's CRTC was removed).
+ *
+ * It is OK to call this even if @port has been removed from the system, in
+ * which case it will just amount to a no-op.
+ *
+ * See also: drm_dp_atomic_find_vcpi_slots()

Same comments as with the _find_vcpi_slots() function.

Quoted 64 lines

drivers/gpu/drm/drm_dp_mst_topology.c +24 -14

  *
  * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
- * negative error code
+ * 0 if all slots for this port were added back to
+ * &drm_dp_mst_topology_state->avail_slots or negative error code
  */
 int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots)
+				     struct drm_dp_mst_port *port)
 {
 	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_dp_vcpi_allocation *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3159,50 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 static struct drm_private_state *
 drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
 {
-	struct drm_dp_mst_topology_state *state;
+	struct drm_dp_mst_topology_state *state, *old_state =
+		to_dp_mst_topology_state(obj->state);
+	struct drm_dp_mst_topology_mgr *mgr = old_state->mgr;
+	struct drm_dp_mst_port *port;
+	struct drm_dp_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	state->mgr = mgr;

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_destroy_connector_work(struct work_struct *work) · line 3184

Quoted 10 lines

drivers/gpu/drm/drm_dp_mst_topology.c +10 -0

+	INIT_LIST_HEAD(&state->vcpis);
+
+	/* Copy over the VCPI allocations for ports that still exist */
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		port = drm_dp_get_validated_port_ref(mgr, pos->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone, vcpi %d -> 0\n",
+					 pos->port, pos->vcpi);
+			continue;
+		}

Hm, in general we have 0 validation in the duplicate_state functions, Just dump copying, with minimal pointer/refcount updates.

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_destroy_connector_work(struct work_struct *work) · line 3186

Quoted 2 lines

drivers/gpu/drm/drm_dp_mst_topology.c +2 -0

+
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);

kmemdump + updating the list. Just in case we ever add more stuff here. Probably needs an explicit memset to avoid list debugging.

At least that's the style we use everywhere else.

Quoted 23 lines

drivers/gpu/drm/drm_dp_mst_topology.c +18 -0

+		if (!vcpi) {
+			drm_dp_put_port(port);
+			goto fail_alloc;
+		}
+
+		vcpi->port = port;
+		vcpi->vcpi = pos->vcpi;
+		list_add(&vcpi->next, &state->vcpis);
+		drm_dp_put_port(port);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

drivers/gpu/drm/drm_dp_mst_topology.c static void drm_dp_mst_destroy_state(struct drm_private_obj *obj, · line 3214

Quoted 5 lines

drivers/gpu/drm/drm_dp_mst_topology.c +2 -0

@@ -3128,14 +3210,60 @@ static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
 {
 	struct drm_dp_mst_topology_state *mst_state =
 		to_dp_mst_topology_state(state);
+	struct drm_dp_vcpi_allocation *pos, *tmp;
+

WARN_ON(!list_empty());

I think this not being empty on mgr teardown would be a driver bug. All the ports/branch devices should be gone by now, and also all the modeset state should have been torn down, using drm_atomic_helper_shutdown() or friends.

It occurred to me that we never actually check this! So let's start doing that.

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/gpu/drm/drm_dp_mst_topology.c +7 -1

index dcfab7536914..8bb03700e199 100644
@@ -3238,7 +3238,7 @@ int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state)
 {
 	struct drm_dp_mst_topology_mgr *mgr = state->mgr;
 	struct drm_dp_vcpi_allocation *pos;
-	int avail_slots = 63;
+	int avail_slots = 63, payload_count = 0;
 
 	list_for_each_entry(pos, &state->vcpis, next) {
 		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",

@@ -3251,6 +3251,12 @@ int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state)
 					 avail_slots + pos->vcpi);
 			return -ENOSPC;
 		}
+
+		if (++payload_count > mgr->max_payloads) {
+			DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p has too many payloads (max=%d)\n",
+					 mgr, state, mgr->max_payloads);
+			return -EINVAL;
+		}
 	}
 	DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p vcpi avail=%d used=%d\n",
 			 mgr, state, avail_slots, 63 - avail_slots);

Signature · 1 line

2.17.2

Quoted 5 lines · Lyude Paul

It occurred to me that we never actually check this! So let's start doing that.

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

One thought on testing all this: I think long term some unti tests, where we have a fake driver doing fake mst branch/ports and a bunch of allocations and then checking that it all works and validates would be nice. Longer term project ofc, and maybe after Kunit has been merged ...

-Daniel

Quoted 37 lines

---
 drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/gpu/drm/drm_dp_mst_topology.c +1 -1

index dcfab7536914..8bb03700e199 100644
@@ -3238,7 +3238,7 @@ int drm_dp_mst_atomic_check(struct
 drm_dp_mst_topology_state *state)
 {
 	struct drm_dp_mst_topology_mgr *mgr = state->mgr;
 	struct drm_dp_vcpi_allocation *pos;
-	int avail_slots = 63;
+	int avail_slots = 63, payload_count = 0;
 
 	list_for_each_entry(pos, &state->vcpis, next) {

drivers/gpu/drm/drm_dp_mst_topology.c +6 -0

 		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",

@@ -3251,6 +3251,12 @@ int drm_dp_mst_atomic_check(struct
 drm_dp_mst_topology_state *state)
 					 avail_slots + pos->vcpi);
 			return -ENOSPC;
 		}
+
+		if (++payload_count > mgr->max_payloads) {
+			DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p has too many
 payloads (max=%d)\n",
+					 mgr, state, mgr->max_payloads);
+			return -EINVAL;
+		}
 	}

drivers/gpu/drm/drm_dp_mst_topology.c +0 -0

 	DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p vcpi avail=%d used=%d\n",
 			 mgr, state, avail_slots, 63 - avail_slots);

Signature · 1 line

2.17.2

Currently, nouveau uses the yolo method of setting up MST displays: it uses the old VCPI helpers (drm_dp_find_vcpi_slots()) for computing the display configuration. These helpers don't take care to make sure they take a reference to the mstb port that they're checking, and additionally don't actually check whether or not the topology still has enough bandwidth to provide the VCPI tokens required.

So, drop usage of the old helpers and move entirely over to the atomic helpers.

Signed-off-by Lyude Paul <[email protected]>

Cc Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 54 +++++++++++++++++++++----
 1 file changed, 47 insertions(+), 7 deletions(-)

drivers/gpu/drm/nouveau/dispnv50/disp.c +41 -7

index 6cbbae3f438b..37503319e5b1 100644
@@ -747,16 +747,22 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 		       struct drm_crtc_state *crtc_state,
 		       struct drm_connector_state *conn_state)
 {
-	struct nv50_mstc *mstc = nv50_mstc(conn_state->connector);
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_connector *connector = conn_state->connector;
+	struct nv50_mstc *mstc = nv50_mstc(connector);
 	struct nv50_mstm *mstm = mstc->mstm;
-	int bpp = conn_state->connector->display_info.bpc * 3;
+	int bpp = connector->display_info.bpc * 3;
 	int slots;
 
-	mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, bpp);
-
-	slots = drm_dp_find_vcpi_slots(&mstm->mgr, mstc->pbn);
-	if (slots < 0)
-		return slots;
+	mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock,
+					 bpp);
+	/* Zombies don't need VCPI */
+	if (!drm_connector_is_unregistered(connector)) {
+		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
+						      mstc->port, mstc->pbn);
+		if (slots < 0)
+			return slots;
+	}
 
 	return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
 					   mstc->native);

@@ -920,12 +926,38 @@ nv50_mstc_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
+static int
+nv50_mstc_atomic_check(struct drm_connector *connector,
+		       struct drm_connector_state *new_conn_state)
+{
+	struct drm_atomic_state *state = new_conn_state->state;
+	struct nv50_mstc *mstc = nv50_mstc(connector);
+	struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr;
+	struct drm_connector_state *old_conn_state;
+	struct drm_crtc *old_crtc;
+
+	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
+	old_crtc = old_conn_state->crtc;
+
+	/* We only need to release VCPI here if we're going from having a CRTC
+	 * on this connector, to not having one
+	 */
+	if (!old_crtc || new_conn_state->crtc)
+		return 0;
+
+	/* Release the previous VCPI allocation since the encoder's
+	 * atomic_check() won't be called
+	 */
+	return drm_dp_atomic_release_vcpi_slots(state, mgr, mstc->port);
+}
+
 static const struct drm_connector_helper_funcs
 nv50_mstc_help = {
 	.get_modes = nv50_mstc_get_modes,
 	.mode_valid = nv50_mstc_mode_valid,
 	.best_encoder = nv50_mstc_best_encoder,
 	.atomic_best_encoder = nv50_mstc_atomic_best_encoder,
+	.atomic_check = nv50_mstc_atomic_check,
 };
 
 static enum drm_connector_status

@@ -2084,6 +2116,8 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	struct drm_connector *connector;
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
 	int ret, i;
 
 	/* We need to handle colour management on a per-plane basis. */

drivers/gpu/drm/nouveau/dispnv50/disp.c +1 -0

+
 	return 0;
 }
 

Signature · 1 line

2.17.2