drm/dp_mst: Improve VCPI helpers, use in nouveau

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

Reviewed-by Daniel Vetter <[email protected]>

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

include/drm/drm_dp_mst_helper.h +2 -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;
+

include/drm/drm_dp_mst_helper.h +65 -0

+		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
+ * state
+ * @__i: int iteration cursor, for macro-internal use
+ *
+ * This iterates over all DRM DP MST topology managers in an atomic update,
+ * tracking both old and new state. This is useful in places where the state
+ * delta needs to be considered, for example in atomic check functions.
+ */
+#define for_each_oldnew_mst_mgr_in_state(__state, mgr, old_state, new_state, __i) \
+	for ((__i) = 0; (__i) < (__state)->num_private_objs; (__i)++) \
+		for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), &(old_state), &(new_state), (__i)))
+
+/**
+ * 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 · 6 lines

2.19.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 v6:
- Keep a kref to all of the ports we have allocations on. This required a good bit of changing to when we call drm_dp_find_vcpi_slots(), mainly that we need to ensure that we only redo VCPI allocations on actual mode or CRTC changes, not crtc_state->active changes. Additionally, we no longer take the registration of the DRM connector for each port into account because so long as we have a kref to the port in the new or previous atomic state, the connector will stay registered.

- Use the small changes to drm_dp_put_port() to add even more error checking to make misusage of the helpers more obvious. I added this after having to chase down various use-after-free conditions that started popping up from the new helpers so no one else has to troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both not be called on the same port in a single atomic check phase (but multiple calls to one or the other is OK)

Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI allocations happen in a state. This makes the next change I'm about to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure that when ports are destroyed and still have VCPI allocations in the topology state, the only state changes allowed are releasing said ports' VCPI. This prevents a state with a mix of VCPI allocations from destroyed ports, and allocations from valid ports.

Changes since v3:
- Don't release VCPI allocations in the topology state immediately in drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip over them in drm_dp_mst_duplicate_state(). This makes it so drm_dp_atomic_release_vcpi_slots() is still idempotent while also throwing warnings if the driver messes up it's book keeping and tries to release VCPI slots on a port that doesn't have any pre-existing VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"

Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into it's own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth requirements. This is needed because originally we relied on the state duplication functions to prune any stale ports from the new state, which would prevent us from incorrectly considering their bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet

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 | 294 +++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  64 +++---
 include/drm/drm_dp_mst_helper.h       |  24 ++-
 4 files changed, 319 insertions(+), 67 deletions(-)

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

index 00fbe7a2699d..183d0e832ced 100644
@@ -2617,21 +2617,42 @@ 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
  *
- * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * 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 &drm_encoder_helper_funcs.atomic_check() callback to change the
+ * current VCPI allocation for the new state, but only when
+ * &drm_crtc_state.mode_changed or &drm_crtc_state.connectors_changed is set
+ * to ensure compatibility with userspace applications that still use the
+ * legacy modesetting UAPI.
+ *
+ * Allocations set by this function are not checked against the bandwidth
+ * restraints of @mgr until the driver calls drm_dp_mst_atomic_check().
+ *
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
+ *
+ * See also:
+ * drm_dp_atomic_release_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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))

@@ -2640,20 +2661,60 @@ 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;
+	topology_state->vcpi_allocated = true;
+
+	/* 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;
+
+			/*
+			 * This should never happen, unless the driver tries

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

+
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	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);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
+		vcpi->port = drm_dp_get_validated_port_ref(mgr, port);
+		if (!vcpi->port) {
+			ret = -EINVAL;
+			kfree(vcpi);
+			goto out;
+		}
+		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);

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

@@ -2661,31 +2722,66 @@ 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
+ * @port: The port to release the VCPI slots from
  *
- * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
- * negative error code
+ * 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 &drm_connector_helper_funcs.atomic_check() callback when the
+ * connector will no longer have VCPI allocated (e.g. because it's CRTC was
+ * removed) when it had VCPI allocated in the previous atomic state.
+ *
+ * It is OK to call this even if @port has been removed from the system.
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic check
+ * phase.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 *pos;
+	bool found = false;
 
 	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(pos, &topology_state->vcpis, next) {
 
+		if (pos->port == port) {
+			found = true;
+			break;
+		}
+	}
+	if (WARN_ON(!found)) {
+		DRM_ERROR("no VCPI for [MST PORT:%p] found in mst state %p\n",
+			  port, &topology_state->base);
+		return -EINVAL;

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

+			DRM_ERROR("cannot allocate and release VCPI on [MST PORT:%p] in the same state\n",
+				  port);
+			return -EINVAL;
+		}
+		pos->vcpi = 0;
+	}
 
 	return 0;

}

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

@@ -3115,15 +3211,45 @@ 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_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	INIT_LIST_HEAD(&state->vcpis);
+	state->vcpi_allocated = false;
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		/* Prune leftover freed VCPI allocations */
+		if (!pos->vcpi)
+			continue;
+
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail;
+
+		/* Take a plain old kref here and don't validate the port, as
+		 * it's topology might not even exist anymore
+		 */
+		kref_get(&vcpi->port->kref);
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next) {
+		drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

@@ -3131,14 +3257,116 @@ 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;
+
+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next) {
+		/* We only keep references to ports with non-zero VCPIs */
+		if (pos->vcpi)
+			drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+static inline int
+drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,

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

+		DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p allocates no VCPI, check skipped\n",
+				 mgr, &mst_state->base);
+		return 0;
+	}
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Releasing VCPI is always OK-even if the port is gone */
+		if (!vcpi->vcpi) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] releases all VCPI slots\n",

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

+
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+				 vcpi->port, vcpi->vcpi);
+
+		avail_slots -= vcpi->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough VCPI slots in mst state %p (avail=%d)\n",
+					 vcpi->port, mst_state,
+					 avail_slots + vcpi->vcpi);
+			ret = -ENOSPC;
+			goto port_fail;
+		}
+
+		drm_dp_put_port(port);
+	}
+	DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
+			 mgr, mst_state, avail_slots,
+			 63 - avail_slots);
+
+	return 0;
+port_fail:
+	drm_dp_put_port(port);
+	return ret;
+}
+
+/**
+ * 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
+ *
+ * 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
+ * &drm_mode_config_funcs.atomic_check() callback.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_atomic_release_vcpi_slots()
+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */
+int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
+	int i, ret = 0;
+
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);
+
+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

@@ -3216,13 +3444,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 +4 -0

index 132e978227fb..8e85e04a5948 100644
@@ -12592,6 +12592,10 @@ static int intel_atomic_check(struct drm_device *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);
 

drivers/gpu/drm/i915/intel_dp_mst.c +6 -1

index 4de247ddf05f..be2d1ac7d87d 100644
@@ -41,8 +41,13 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct drm_connector *connector = conn_state->connector;
 	void *port = to_intel_connector(connector)->port;
 	struct drm_atomic_state *state = pipe_config->base.state;
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
+	struct drm_crtc_state *new_crtc_state = &pipe_config->base;
 	int bpp;
-	int lane_count, slots = 0;
+	int lane_count, slots =
+		to_intel_crtc_state(old_crtc_state)->dp_m_n.tu;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
 	bool constant_n = drm_dp_has_quirk(&intel_dp->desc,

@@ -77,8 +82,12 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

drivers/gpu/drm/i915/intel_dp_mst.c +8 -4

 		slots = drm_dp_atomic_find_vcpi_slots(state,
 						      &intel_dp->mst_mgr,
 						      port,

@@ -107,36 +116,39 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
-static int intel_dp_mst_atomic_check(struct drm_connector *connector,
-		struct drm_connector_state *new_conn_state)
+static int
+intel_dp_mst_atomic_check(struct drm_connector *connector,
+			  struct drm_connector_state *new_conn_state)
 {
+	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;
 	struct drm_atomic_state *state = new_conn_state->state;
-	struct drm_connector_state *old_conn_state;
-	struct drm_crtc *old_crtc;

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+
+	return drm_dp_atomic_release_vcpi_slots(state, mgr, port);
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,

include/drm/drm_dp_mst_helper.h +0 -0

index 3faceb66f5cb..2392e12b6bce 100644

include/drm/drm_dp_mst_helper.h +16 -7

+	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;
+	u8 vcpi_allocated;
 	struct drm_dp_mst_topology_mgr *mgr;
 };
 

@@ -619,14 +626,17 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct drm_dp_mst_topology_mgr *mgr);
-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);
-int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
-				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots);
+int __must_check
+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);
+int __must_check
+drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				 struct drm_dp_mst_topology_mgr *mgr,
+				 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);
+int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state *state);
 
 extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
 

Signature · 1 line

2.19.1

Quoted 35 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.

Quoted 9 lines

Changes since v6:
- Keep a kref to all of the ports we have allocations on. This required a good bit of changing to when we call drm_dp_find_vcpi_slots(), mainly that we need to ensure that we only redo VCPI allocations on actual mode or CRTC changes, not crtc_state->active changes. Additionally, we no longer take the registration of the DRM connector for each port into account because so long as we have a kref to the port in the new or previous atomic state, the connector will stay registered.

I write an entire pile of small nitpits (still included most of them below), until I realized this here wont work. Delaying the call to destroy the connector (well, unregister it really) wreaks the design we've come up with thus far, resulting in most of my comments here.

Instead, all we need to do is delay the kfree(port) at the bottom of drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the pointer value to stay valid, as a lookup key. It doesn't care at all about anything actually stored in there. So the only thing we need to delay is the kfree. I think the simplest way to achieve this is to add a 2nd kref (port->kfree_ref or something like that), with on reference held by the port itself (released in drm_dp_destroy_port), and the other one held as-needed by the vcpi allocation lists.

I think if we go with this design instead of retrofitting a semantic change of the port lifetime itself, all the complications I complain about below should disappear.

Piles of comments below.

Cheers, Daniel

Quoted 132 lines

- Use the small changes to drm_dp_put_port() to add even more error checking to make misusage of the helpers more obvious. I added this after having to chase down various use-after-free conditions that started popping up from the new helpers so no one else has to troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both not be called on the same port in a single atomic check phase (but multiple calls to one or the other is OK)

Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI allocations happen in a state. This makes the next change I'm about to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure that when ports are destroyed and still have VCPI allocations in the topology state, the only state changes allowed are releasing said ports' VCPI. This prevents a state with a mix of VCPI allocations from destroyed ports, and allocations from valid ports.

Changes since v3:
- Don't release VCPI allocations in the topology state immediately in drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip over them in drm_dp_mst_duplicate_state(). This makes it so drm_dp_atomic_release_vcpi_slots() is still idempotent while also throwing warnings if the driver messes up it's book keeping and tries to release VCPI slots on a port that doesn't have any pre-existing VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"

Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into it's own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth requirements. This is needed because originally we relied on the state duplication functions to prune any stale ports from the new state, which would prevent us from incorrectly considering their bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet

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 | 294 +++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  64 +++---
 include/drm/drm_dp_mst_helper.h       |  24 ++-
 4 files changed, 319 insertions(+), 67 deletions(-)

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

index 00fbe7a2699d..183d0e832ced 100644
@@ -2617,21 +2617,42 @@ 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
  *
- * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * 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 &drm_encoder_helper_funcs.atomic_check() callback to change the
+ * current VCPI allocation for the new state, but only when
+ * &drm_crtc_state.mode_changed or &drm_crtc_state.connectors_changed is set
+ * to ensure compatibility with userspace applications that still use the
+ * legacy modesetting UAPI.
+ *
+ * Allocations set by this function are not checked against the bandwidth
+ * restraints of @mgr until the driver calls drm_dp_mst_atomic_check().
+ *
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
+ *
+ * See also:
+ * drm_dp_atomic_release_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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))

@@ -2640,20 +2661,60 @@ 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;
+	topology_state->vcpi_allocated = true;
+
+	/* 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;
+
+			/*
+			 * This should never happen, unless the driver tries

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

Quoted 10 lines

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

+			 * releasing and allocating the same VCPI allocation,
+			 * which is an error
+			 */
+			if (WARN_ON(!prev_slots)) {
 
 
+				DRM_ERROR("cannot allocate and release VCPI on [MST PORT:%p] in the same state\n",
+					  port);
+				return -EINVAL;
+			}

Allowing atomic changes to output state (like disabling it on one crtc, and then enabling on the other, thus both releasing and re-allocating the vcpi in one go) is kinda the point of atomic. So definitely can't EINVAL here.

Sounds like we need more testcases in igt?

Quoted 104 lines

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

+
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	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);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
+		vcpi->port = drm_dp_get_validated_port_ref(mgr, port);
+		if (!vcpi->port) {
+			ret = -EINVAL;
+			kfree(vcpi);
+			goto out;
+		}
+		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);

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

@@ -2661,31 +2722,66 @@ 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
+ * @port: The port to release the VCPI slots from
  *
- * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
- * negative error code
+ * 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 &drm_connector_helper_funcs.atomic_check() callback when the
+ * connector will no longer have VCPI allocated (e.g. because it's CRTC was
+ * removed) when it had VCPI allocated in the previous atomic state.
+ *
+ * It is OK to call this even if @port has been removed from the system.
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic check
+ * phase.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 *pos;
+	bool found = false;
 
 	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(pos, &topology_state->vcpis, next) {
 
+		if (pos->port == port) {
+			found = true;
+			break;
+		}
+	}
+	if (WARN_ON(!found)) {
+		DRM_ERROR("no VCPI for [MST PORT:%p] found in mst state %p\n",
+			  port, &topology_state->base);
+		return -EINVAL;

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

Quoted 10 lines

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

+	}
+
+	DRM_DEBUG_ATOMIC("[MST PORT:%p] VCPI %d -> 0\n", port, pos->vcpi);
+	if (pos->vcpi) {
+		/*
+		 * This should be safe-as the previous state will still hold a
+		 * reference to this port until we swap states. If it's not,
+		 * the driver has a bookkeeping error
+		 */
+		if (WARN_ON(drm_dp_put_port(port))) {

The usual way to do this (instead of adding a return value to the normal _put function) is something like

kref_put(port, function_that_with_just_a_WARNING_DRM_ERROR)

avoids the need for patch 2.

Quoted 76 lines

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

+			DRM_ERROR("cannot allocate and release VCPI on [MST PORT:%p] in the same state\n",
+				  port);
+			return -EINVAL;
+		}
+		pos->vcpi = 0;
+	}
 
 	return 0;

}

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

@@ -3115,15 +3211,45 @@ 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_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	INIT_LIST_HEAD(&state->vcpis);
+	state->vcpi_allocated = false;
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		/* Prune leftover freed VCPI allocations */
+		if (!pos->vcpi)
+			continue;
+
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail;
+
+		/* Take a plain old kref here and don't validate the port, as
+		 * it's topology might not even exist anymore
+		 */
+		kref_get(&vcpi->port->kref);
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next) {
+		drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

@@ -3131,14 +3257,116 @@ 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;
+
+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next) {
+		/* We only keep references to ports with non-zero VCPIs */
+		if (pos->vcpi)
+			drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+static inline int
+drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,

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

Quoted 10 lines

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

+				       struct drm_dp_mst_topology_state *mst_state)
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	struct drm_dp_mst_port *port;
+	int avail_slots = 63, ret;
+
+	/* There's no possible scenario where releasing VCPI or keeping it the
+	 * same would make the state invalid
+	 */
+	if (!mst_state->vcpi_allocated) {

Do we need this? If not I'm always in favour of removing code if it's not needed :-)

Quoted 9 lines

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

+		DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p allocates no VCPI, check skipped\n",
+				 mgr, &mst_state->base);
+		return 0;
+	}
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Releasing VCPI is always OK-even if the port is gone */
+		if (!vcpi->vcpi) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] releases all VCPI slots\n",

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

Quoted 10 lines

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

+					 vcpi->port);
+			continue;
+		}
+
+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone but still has VCPI, cannot add new VCPI\n",
+					 vcpi->port);
+			return -EINVAL;
+		}

For find_vcpi we've already done this. For unplugged vcpi, we can't do this here I think - otherwise if you enable 2 mst outputs on the same branch, and unplug both, old non-atomic userspace can never ever again disable them because it can't disable both. The check (to skip already deallocated ports) above isn't enough to handle that.

Quoted 122 lines

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

+
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+				 vcpi->port, vcpi->vcpi);
+
+		avail_slots -= vcpi->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough VCPI slots in mst state %p (avail=%d)\n",
+					 vcpi->port, mst_state,
+					 avail_slots + vcpi->vcpi);
+			ret = -ENOSPC;
+			goto port_fail;
+		}
+
+		drm_dp_put_port(port);
+	}
+	DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
+			 mgr, mst_state, avail_slots,
+			 63 - avail_slots);
+
+	return 0;
+port_fail:
+	drm_dp_put_port(port);
+	return ret;
+}
+
+/**
+ * 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
+ *
+ * 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
+ * &drm_mode_config_funcs.atomic_check() callback.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_atomic_release_vcpi_slots()
+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */
+int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
+	int i, ret = 0;
+
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);
+
+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

@@ -3216,13 +3444,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 +4 -0

index 132e978227fb..8e85e04a5948 100644
@@ -12592,6 +12592,10 @@ static int intel_atomic_check(struct drm_device *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);
 

drivers/gpu/drm/i915/intel_dp_mst.c +6 -1

index 4de247ddf05f..be2d1ac7d87d 100644
@@ -41,8 +41,13 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct drm_connector *connector = conn_state->connector;
 	void *port = to_intel_connector(connector)->port;
 	struct drm_atomic_state *state = pipe_config->base.state;
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
+	struct drm_crtc_state *new_crtc_state = &pipe_config->base;
 	int bpp;
-	int lane_count, slots = 0;
+	int lane_count, slots =
+		to_intel_crtc_state(old_crtc_state)->dp_m_n.tu;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
 	bool constant_n = drm_dp_has_quirk(&intel_dp->desc,

@@ -77,8 +82,12 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 90

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +6 -2

 	pipe_config->pbn = mst_pbn;
 
-	/* Zombie connectors can't have VCPI slots */
-	if (!drm_connector_is_unregistered(connector)) {
+	/* Only change VCPI allocation on actual mode changes, to prevent us
+	 * from trying to allocate VCPI to ports that no longer exist when we
+	 * may just be trying to disable DPMS on them
+	 */
+	if (new_crtc_state->mode_changed ||
+	    new_crtc_state->connectors_changed) {

Why exactly is this needed? We only call compute_config on the enocoder when enabling an output. Not when disabling one. Similar for the atomic helpers and the various encoder callbacks (you have the same change in the nouveau code). Did this actually blow up while testing?

Quoted 21 lines

drivers/gpu/drm/i915/intel_dp_mst.c +8 -4

 		slots = drm_dp_atomic_find_vcpi_slots(state,
 						      &intel_dp->mst_mgr,
 						      port,

@@ -107,36 +116,39 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
-static int intel_dp_mst_atomic_check(struct drm_connector *connector,
-		struct drm_connector_state *new_conn_state)
+static int
+intel_dp_mst_atomic_check(struct drm_connector *connector,
+			  struct drm_connector_state *new_conn_state)
 {
+	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;
 	struct drm_atomic_state *state = new_conn_state->state;
-	struct drm_connector_state *old_conn_state;
-	struct drm_crtc *old_crtc;

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 120

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +5 -4

-	struct drm_crtc_state *crtc_state;
-	int slots, ret = 0;
+	struct drm_connector_state *old_conn_state =
+		drm_atomic_get_old_connector_state(state, connector);
+	struct drm_crtc *old_crtc = old_conn_state->crtc,
+			*new_crtc = new_conn_state->crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 
-	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
-	old_crtc = old_conn_state->crtc;

Bikeshed: Reworking all the above hides the real changes quite a bit.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 139

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +3 -6

 	if (!old_crtc)
-		return ret;
+		return 0;
 
-	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;
+	old_crtc_state = drm_atomic_get_old_crtc_state(state, old_crtc);
+	if (!old_crtc_state ||

!old_crtc_state would be a framework bug. Just remove that check.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 141

Quoted 2 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+	    !to_intel_crtc_state(old_crtc_state)->dp_m_n.tu)
+		return 0;

Hm, why do we need this? Having a DP port previously enabled with a 0 tu sounds like a pretty big driver bug.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 146

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_mst.c +4 -2

-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+	/* If we switched CRTCs, clear the previous one's allocation */
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
+	if (new_crtc_state->connectors_changed && !new_crtc_state->enable)
+		to_intel_crtc_state(new_crtc_state)->dp_m_n.tu = 0;

No idea why we need to clear this here? Shouldn't we just clear this unconditionally if we release the allocation.

Also naming convention, I'd call this old_crtc_new_state, and maybe old_crtc_old_state. To make it clear these are the old/new states for old_crtc. Instead of old/new state for the new crtc.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 149

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -7

-		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;
-	}
-	return ret;
+	if (new_crtc)
+		return 0;

Not sure why we shouldn't release this? If we handle this in helpers, it would remove piles of special casing code here I think. Plus remove special code in the hlpers.

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+
+	return drm_dp_atomic_release_vcpi_slots(state, mgr, port);
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,

include/drm/drm_dp_mst_helper.h +0 -0

index 3faceb66f5cb..2392e12b6bce 100644

include/drm/drm_dp_mst_helper.h struct drm_dp_payload { · line 409

Quoted 4 lines

include/drm/drm_dp_mst_helper.h +1 -0

@@ -406,9 +406,16 @@ 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 {

I think a comment here that this pointer holds a full reference (even past when the connector is unplugged) would be good. Otherwise the loop in release_vcpi would be buggy.

Quoted 40 lines

include/drm/drm_dp_mst_helper.h +16 -7

+	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;
+	u8 vcpi_allocated;
 	struct drm_dp_mst_topology_mgr *mgr;
 };
 

@@ -619,14 +626,17 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct drm_dp_mst_topology_mgr *mgr);
-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);
-int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
-				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots);
+int __must_check
+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);
+int __must_check
+drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				 struct drm_dp_mst_topology_mgr *mgr,
+				 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);
+int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state *state);
 
 extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
 

Signature · 1 line

2.19.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 60 lines · Daniel Vetter

Quoted 35 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.

Quoted 9 lines

Changes since v6:
- Keep a kref to all of the ports we have allocations on. This required a good bit of changing to when we call drm_dp_find_vcpi_slots(), mainly that we need to ensure that we only redo VCPI allocations on actual mode or CRTC changes, not crtc_state->active changes. Additionally, we no longer take the registration of the DRM connector for each port into account because so long as we have a kref to the port in the new or previous atomic state, the connector will stay registered.

I write an entire pile of small nitpits (still included most of them below), until I realized this here wont work. Delaying the call to destroy the connector (well, unregister it really) wreaks the design we've come up with thus far, resulting in most of my comments here.

Instead, all we need to do is delay the kfree(port) at the bottom of drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the pointer value to stay valid, as a lookup key. It doesn't care at all about anything actually stored in there. So the only thing we need to delay is the kfree. I think the simplest way to achieve this is to add a 2nd kref (port->kfree_ref or something like that), with on reference held by the port itself (released in drm_dp_destroy_port), and the other one held as-needed by the vcpi allocation lists.

Quoted 3 lines

I think if we go with this design instead of retrofitting a semantic change of the port lifetime itself, all the complications I complain about below should disappear.

In the above I meant drm_dp_destroy_port or drm_dp_destroy_connector_work.

Aside: I think creating a kref for the final kfree would also solve a bunch of other issues in a much neater way: In

commit f038c5b99fc1332f558b495d136d4f433ee65caa

Author: Lyude Paul <[email protected]>
Date:   Tue Nov 13 17:46:14 2018 -0500

drm/dp_mst: Skip validating ports during destruction, just ref

we could use that kfree reference to make sure the port pointer is alive. This of course means that drm_dp_update_payload_part1() would also need to use the kfree reference for the vcpi allocations. And probably everywhere else (e.g. in your nouveau series for payload information).

That would give us a very clear separation overall between "port can still be used because it's not yet unplugged" vs. "port only hangs around because a bunch of vcpi allocations and other payload things upstream of the port might still need the port lookup key to free resources". Instead of again sprinkling complicated conditions and magic tricks all over the code to figure out whethe we should allow the get_validate_port to succeed or not, or the modeset to succeed or not.

-Daniel

Quoted 729 lines

Piles of comments below.

Cheers, Daniel

Quoted 146 lines

- Use the small changes to drm_dp_put_port() to add even more error checking to make misusage of the helpers more obvious. I added this after having to chase down various use-after-free conditions that started popping up from the new helpers so no one else has to troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both not
be
called on the same port in a single atomic check phase (but multiple calls to one or the other is OK)

Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI allocations happen in a state. This makes the next change I'm about to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure that when ports are destroyed and still have VCPI allocations in the topology state, the only state changes allowed are releasing said ports' VCPI. This prevents a state with a mix of VCPI allocations from destroyed ports, and allocations from valid ports.

Changes since v3:
- Don't release VCPI allocations in the topology state immediately in drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip over them in drm_dp_mst_duplicate_state(). This makes it so drm_dp_atomic_release_vcpi_slots() is still idempotent while also throwing warnings if the driver messes up it's book keeping and tries to release VCPI slots on a port that doesn't have any pre-existing VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"

Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into it's own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth requirements. This is needed because originally we relied on the state duplication functions to prune any stale ports from the new state, which would prevent us from incorrectly considering their bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() -
danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet

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 | 294 +++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  64 +++---
 include/drm/drm_dp_mst_helper.h       |  24 ++-
 4 files changed, 319 insertions(+), 67 deletions(-)

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

index 00fbe7a2699d..183d0e832ced 100644
@@ -2617,21 +2617,42 @@ 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
*
- * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * 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 &drm_encoder_helper_funcs.atomic_check() callback to change the
+ * current VCPI allocation for the new state, but only when
+ * &drm_crtc_state.mode_changed or &drm_crtc_state.connectors_changed is set
+ * to ensure compatibility with userspace applications that still use the
+ * legacy modesetting UAPI.
+ *
+ * Allocations set by this function are not checked against the bandwidth
+ * restraints of @mgr until the driver calls drm_dp_mst_atomic_check().
+ *
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
+ *
+ * See also:
+ * drm_dp_atomic_release_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 +11 -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))

@@ -2640,20 +2661,60 @@ 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;
+	topology_state->vcpi_allocated = true;
+
+	/* 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;
+
+			/*

drivers/gpu/drm/drm_dp_mst_topology.c int drm_dp_atomic_find_vcpi_slots(struct · line 2684

Quoted 10 lines

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

+			 * This should never happen, unless the driver tries
+			 * releasing and allocating the same VCPI allocation,
+			 * which is an error
+			 */
+			if (WARN_ON(!prev_slots)) {
+				DRM_ERROR("cannot allocate and release VCPI on
 [MST PORT:%p] in the same state\n",
+					  port);
+				return -EINVAL;
+			}

Allowing atomic changes to output state (like disabling it on one crtc, and then enabling on the other, thus both releasing and re-allocating the vcpi in one go) is kinda the point of atomic. So definitely can't EINVAL here.

Sounds like we need more testcases in igt?

Quoted 110 lines

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

+
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	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);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
+		vcpi->port = drm_dp_get_validated_port_ref(mgr, port);
+		if (!vcpi->port) {
+			ret = -EINVAL;
+			kfree(vcpi);
+			goto out;
+		}
+		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);

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

@@ -2661,31 +2722,66 @@ 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
+ * @port: The port to release the VCPI slots from
  *
- * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state-

Quoted 1 line

avail_slots or

- * negative error code
+ * 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 &drm_connector_helper_funcs.atomic_check() callback when the
+ * connector will no longer have VCPI allocated (e.g. because it's CRTC was
+ * removed) when it had VCPI allocated in the previous atomic state.
+ *
+ * It is OK to call this even if @port has been removed from the system.
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic check
+ * phase.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 +13 -10

 				     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 *pos;
+	bool found = false;
 
 	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(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			found = true;
+			break;
+		}
+	}
+	if (WARN_ON(!found)) {
+		DRM_ERROR("no VCPI for [MST PORT:%p] found in mst state %p\n",
+			  port, &topology_state->base);
+		return -EINVAL;

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

Quoted 10 lines

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

+	}
+
+	DRM_DEBUG_ATOMIC("[MST PORT:%p] VCPI %d -> 0\n", port, pos->vcpi);
+	if (pos->vcpi) {
+		/*
+		 * This should be safe-as the previous state will still hold a
+		 * reference to this port until we swap states. If it's not,
+		 * the driver has a bookkeeping error
+		 */
+		if (WARN_ON(drm_dp_put_port(port))) {

The usual way to do this (instead of adding a return value to the normal _put function) is something like

kref_put(port, function_that_with_just_a_WARNING_DRM_ERROR)

avoids the need for patch 2.

Quoted 81 lines

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

+			DRM_ERROR("cannot allocate and release VCPI on [MST

PORT:%p] in the same state\n",

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

+				  port);
+			return -EINVAL;
+		}
+		pos->vcpi = 0;
+	}
 
 	return 0;
 }

@@ -3115,15 +3211,45 @@ 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_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	INIT_LIST_HEAD(&state->vcpis);
+	state->vcpi_allocated = false;
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		/* Prune leftover freed VCPI allocations */
+		if (!pos->vcpi)
+			continue;
+
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail;
+
+		/* Take a plain old kref here and don't validate the port, as
+		 * it's topology might not even exist anymore
+		 */
+		kref_get(&vcpi->port->kref);
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next) {
+		drm_dp_put_port(pos->port);
+		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 +11 -1

@@ -3131,14 +3257,116 @@ 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;
+
+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next) {
+		/* We only keep references to ports with non-zero VCPIs */
+		if (pos->vcpi)
+			drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+static inline int
+drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr
 *mgr,
+				       struct drm_dp_mst_topology_state

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

Quoted 10 lines

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

 *mst_state)
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	struct drm_dp_mst_port *port;
+	int avail_slots = 63, ret;
+
+	/* There's no possible scenario where releasing VCPI or keeping it the
+	 * same would make the state invalid
+	 */
+	if (!mst_state->vcpi_allocated) {

Do we need this? If not I'm always in favour of removing code if it's not needed :-)

Quoted 12 lines

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

+		DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p allocates no VCPI,
 check skipped\n",
+				 mgr, &mst_state->base);
+		return 0;
+	}
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Releasing VCPI is always OK-even if the port is gone */
+		if (!vcpi->vcpi) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] releases all VCPI
 slots\n",
+					 vcpi->port);

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

Quoted 10 lines

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

+			continue;
+		}
+
+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone but still has
 VCPI, cannot add new VCPI\n",
+					 vcpi->port);
+			return -EINVAL;
+		}

For find_vcpi we've already done this. For unplugged vcpi, we can't do this here I think - otherwise if you enable 2 mst outputs on the same branch, and unplug both, old non-atomic userspace can never ever again disable them because it can't disable both. The check (to skip already deallocated ports) above isn't enough to handle that.

Quoted 135 lines

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

+
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+				 vcpi->port, vcpi->vcpi);
+
+		avail_slots -= vcpi->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough VCPI slots

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

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

+					 vcpi->port, mst_state,
+					 avail_slots + vcpi->vcpi);
+			ret = -ENOSPC;
+			goto port_fail;
+		}
+
+		drm_dp_put_port(port);
+	}
+	DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
+			 mgr, mst_state, avail_slots,
+			 63 - avail_slots);
+
+	return 0;
+port_fail:
+	drm_dp_put_port(port);
+	return ret;
+}
+
+/**
+ * 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
+ *
+ * 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
+ * &drm_mode_config_funcs.atomic_check() callback.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_atomic_release_vcpi_slots()
+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */

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

+int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
+	int i, ret = 0;
+
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);
+
+const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs =

{

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

 	.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

@@ -3216,13 +3444,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 +4 -0

index 132e978227fb..8e85e04a5948 100644
@@ -12592,6 +12592,10 @@ static int intel_atomic_check(struct drm_device
 *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

drivers/gpu/drm/i915/intel_dp_mst.c +6 -1

index 4de247ddf05f..be2d1ac7d87d 100644
@@ -41,8 +41,13 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	struct drm_connector *connector = conn_state->connector;
 	void *port = to_intel_connector(connector)->port;
 	struct drm_atomic_state *state = pipe_config->base.state;
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
+	struct drm_crtc_state *new_crtc_state = &pipe_config->base;
 	int bpp;
-	int lane_count, slots = 0;
+	int lane_count, slots =
+		to_intel_crtc_state(old_crtc_state)->dp_m_n.tu;
 	const struct drm_display_mode *adjusted_mode = &pipe_config-
 >base.adjusted_mode;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0

 	int mst_pbn;
 	bool constant_n = drm_dp_has_quirk(&intel_dp->desc,

@@ -77,8 +82,12 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 91

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +6 -2

 	pipe_config->pbn = mst_pbn;
 
-	/* Zombie connectors can't have VCPI slots */
-	if (!drm_connector_is_unregistered(connector)) {
+	/* Only change VCPI allocation on actual mode changes, to prevent us
+	 * from trying to allocate VCPI to ports that no longer exist when we
+	 * may just be trying to disable DPMS on them
+	 */
+	if (new_crtc_state->mode_changed ||
+	    new_crtc_state->connectors_changed) {

Why exactly is this needed? We only call compute_config on the enocoder when enabling an output. Not when disabling one. Similar for the atomic helpers and the various encoder callbacks (you have the same change in the nouveau code). Did this actually blow up while testing?

Quoted 22 lines

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0

 		slots = drm_dp_atomic_find_vcpi_slots(state,
 						      &intel_dp->mst_mgr,

drivers/gpu/drm/i915/intel_dp_mst.c +8 -4

 						      port,

@@ -107,36 +116,39 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	return true;
 }
 
-static int intel_dp_mst_atomic_check(struct drm_connector *connector,
-		struct drm_connector_state *new_conn_state)
+static int
+intel_dp_mst_atomic_check(struct drm_connector *connector,
+			  struct drm_connector_state *new_conn_state)
 {
+	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;
 	struct drm_atomic_state *state = new_conn_state->state;
-	struct drm_connector_state *old_conn_state;
-	struct drm_crtc *old_crtc;

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 121

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +5 -4

-	struct drm_crtc_state *crtc_state;
-	int slots, ret = 0;
+	struct drm_connector_state *old_conn_state =
+		drm_atomic_get_old_connector_state(state, connector);
+	struct drm_crtc *old_crtc = old_conn_state->crtc,
+			*new_crtc = new_conn_state->crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 
-	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
-	old_crtc = old_conn_state->crtc;

Bikeshed: Reworking all the above hides the real changes quite a bit.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 139

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +3 -6

 	if (!old_crtc)
-		return ret;
+		return 0;
 
-	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;
+	old_crtc_state = drm_atomic_get_old_crtc_state(state, old_crtc);
+	if (!old_crtc_state ||

!old_crtc_state would be a framework bug. Just remove that check.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 142

Quoted 2 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+	    !to_intel_crtc_state(old_crtc_state)->dp_m_n.tu)
+		return 0;

Hm, why do we need this? Having a DP port previously enabled with a 0 tu sounds like a pretty big driver bug.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 146

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_mst.c +4 -2

-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+	/* If we switched CRTCs, clear the previous one's allocation */
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
+	if (new_crtc_state->connectors_changed && !new_crtc_state->enable)
+		to_intel_crtc_state(new_crtc_state)->dp_m_n.tu = 0;

No idea why we need to clear this here? Shouldn't we just clear this unconditionally if we release the allocation.

Also naming convention, I'd call this old_crtc_new_state, and maybe old_crtc_old_state. To make it clear these are the old/new states for old_crtc. Instead of old/new state for the new crtc.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 149

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -7

-		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;
-	}
-	return ret;
+	if (new_crtc)
+		return 0;

Not sure why we shouldn't release this? If we handle this in helpers, it would remove piles of special casing code here I think. Plus remove special code in the hlpers.

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+
+	return drm_dp_atomic_release_vcpi_slots(state, mgr, port);
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,

include/drm/drm_dp_mst_helper.h +0 -0

index 3faceb66f5cb..2392e12b6bce 100644

include/drm/drm_dp_mst_helper.h struct drm_dp_payload { · line 410

Quoted 5 lines

include/drm/drm_dp_mst_helper.h +1 -0

@@ -406,9 +406,16 @@ 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 {

I think a comment here that this pointer holds a full reference (even past when the connector is unplugged) would be good. Otherwise the loop in release_vcpi would be buggy.

Quoted 46 lines

include/drm/drm_dp_mst_helper.h +16 -7

+	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;
+	u8 vcpi_allocated;
 	struct drm_dp_mst_topology_mgr *mgr;
 };

@@ -619,14 +626,17 @@ void drm_dp_mst_topology_mgr_suspend(struct
 drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr
 *mgr);
 struct drm_dp_mst_topology_state
 *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct
 drm_dp_mst_topology_mgr *mgr);
-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);
-int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
-				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots);
+int __must_check
+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);
+int __must_check
+drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				 struct drm_dp_mst_topology_mgr *mgr,
+				 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);
+int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state

*state);

extern const struct drm_private_state_funcs
drm_dp_mst_topology_state_funcs;

Signature · 1 line

2.19.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 81 lines · Daniel Vetter

Quoted 60 lines · Daniel Vetter

Quoted 35 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.

Quoted 9 lines

Changes since v6:
- Keep a kref to all of the ports we have allocations on. This required a good bit of changing to when we call drm_dp_find_vcpi_slots(), mainly that we need to ensure that we only redo VCPI allocations on actual mode or CRTC changes, not crtc_state->active changes. Additionally, we no longer take the registration of the DRM connector for each port into account because so long as we have a kref to the port in the new or previous atomic state, the connector will stay registered.

I write an entire pile of small nitpits (still included most of them below), until I realized this here wont work. Delaying the call to destroy the connector (well, unregister it really) wreaks the design we've come up with thus far, resulting in most of my comments here.

Instead, all we need to do is delay the kfree(port) at the bottom of drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the pointer value to stay valid, as a lookup key. It doesn't care at all about anything actually stored in there. So the only thing we need to delay is the kfree. I think the simplest way to achieve this is to add a 2nd kref (port->kfree_ref or something like that), with on reference held by the port itself (released in drm_dp_destroy_port), and the other one held as-needed by the vcpi allocation lists.

Quoted 3 lines

I think if we go with this design instead of retrofitting a semantic change of the port lifetime itself, all the complications I complain about below should disappear.

In the above I meant drm_dp_destroy_port or drm_dp_destroy_connector_work.

Aside: I think creating a kref for the final kfree would also solve a bunch of other issues in a much neater way: In

commit f038c5b99fc1332f558b495d136d4f433ee65caa

Author: Lyude Paul <[email protected]>
Date:   Tue Nov 13 17:46:14 2018 -0500

drm/dp_mst: Skip validating ports during destruction, just ref

we could use that kfree reference to make sure the port pointer is alive. This of course means that drm_dp_update_payload_part1() would also need to use the kfree reference for the vcpi allocations. And probably everywhere else (e.g. in your nouveau series for payload information).

Quoted 7 lines

That would give us a very clear separation overall between "port can still be used because it's not yet unplugged" vs. "port only hangs around because a bunch of vcpi allocations and other payload things upstream of the port might still need the port lookup key to free resources". Instead of again sprinkling complicated conditions and magic tricks all over the code to figure out whethe we should allow the get_validate_port to succeed or not, or the modeset to succeed or not.

mhm, I've been experimenting with this and I think I agree this is definitely the right solution to go with. It'll end up decomplicating a lot of this :)

Quoted 731 lines

-Daniel

Quoted 729 lines

Piles of comments below.

Cheers, Daniel

Quoted 146 lines

- Use the small changes to drm_dp_put_port() to add even more error checking to make misusage of the helpers more obvious. I added this after having to chase down various use-after-free conditions that started popping up from the new helpers so no one else has to troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both not
be
called on the same port in a single atomic check phase (but multiple calls to one or the other is OK)

Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI allocations happen in a state. This makes the next change I'm about to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure that when ports are destroyed and still have VCPI allocations in the topology state, the only state changes allowed are releasing said ports' VCPI. This prevents a state with a mix of VCPI allocations from destroyed ports, and allocations from valid ports.

Changes since v3:
- Don't release VCPI allocations in the topology state immediately in drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip over them in drm_dp_mst_duplicate_state(). This makes it so drm_dp_atomic_release_vcpi_slots() is still idempotent while also throwing warnings if the driver messes up it's book keeping and tries to release VCPI slots on a port that doesn't have any pre-existing VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"

Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into it's own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth requirements. This is needed because originally we relied on the state duplication functions to prune any stale ports from the new state, which would prevent us from incorrectly considering their bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() -
danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet

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 | 294 +++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  64 +++---
 include/drm/drm_dp_mst_helper.h       |  24 ++-
 4 files changed, 319 insertions(+), 67 deletions(-)

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

index 00fbe7a2699d..183d0e832ced 100644
@@ -2617,21 +2617,42 @@ 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
*
- * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * 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 &drm_encoder_helper_funcs.atomic_check() callback to change the
+ * current VCPI allocation for the new state, but only when
+ * &drm_crtc_state.mode_changed or &drm_crtc_state.connectors_changed is set
+ * to ensure compatibility with userspace applications that still use the
+ * legacy modesetting UAPI.
+ *
+ * Allocations set by this function are not checked against the bandwidth
+ * restraints of @mgr until the driver calls drm_dp_mst_atomic_check().
+ *
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
+ *
+ * See also:
+ * drm_dp_atomic_release_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 +11 -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))

@@ -2640,20 +2661,60 @@ 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;
+	topology_state->vcpi_allocated = true;
+
+	/* 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;
+
+			/*

drivers/gpu/drm/drm_dp_mst_topology.c int drm_dp_atomic_find_vcpi_slots(struct · line 2684

Quoted 10 lines

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

+			 * This should never happen, unless the driver tries
+			 * releasing and allocating the same VCPI allocation,
+			 * which is an error
+			 */
+			if (WARN_ON(!prev_slots)) {
+				DRM_ERROR("cannot allocate and release VCPI on
 [MST PORT:%p] in the same state\n",
+					  port);
+				return -EINVAL;
+			}

Allowing atomic changes to output state (like disabling it on one crtc, and then enabling on the other, thus both releasing and re-allocating the vcpi in one go) is kinda the point of atomic. So definitely can't EINVAL here.

Sounds like we need more testcases in igt?

Quoted 110 lines

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

+
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	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);
+
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
+		vcpi->port = drm_dp_get_validated_port_ref(mgr, port);
+		if (!vcpi->port) {
+			ret = -EINVAL;
+			kfree(vcpi);
+			goto out;
+		}
+		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);

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

@@ -2661,31 +2722,66 @@ 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
+ * @port: The port to release the VCPI slots from
  *
- * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state-

Quoted 1 line

avail_slots or

- * negative error code
+ * 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 &drm_connector_helper_funcs.atomic_check() callback when the
+ * connector will no longer have VCPI allocated (e.g. because it's CRTC was
+ * removed) when it had VCPI allocated in the previous atomic state.
+ *
+ * It is OK to call this even if @port has been removed from the system.
+ * Additionally, it is OK to call this function multiple times on the same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic check
+ * phase.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 +13 -10

 				     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 *pos;
+	bool found = false;
 
 	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(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			found = true;
+			break;
+		}
+	}
+	if (WARN_ON(!found)) {
+		DRM_ERROR("no VCPI for [MST PORT:%p] found in mst state %p\n",
+			  port, &topology_state->base);
+		return -EINVAL;

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

Quoted 10 lines

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

+	}
+
+	DRM_DEBUG_ATOMIC("[MST PORT:%p] VCPI %d -> 0\n", port, pos->vcpi);
+	if (pos->vcpi) {
+		/*
+		 * This should be safe-as the previous state will still hold a
+		 * reference to this port until we swap states. If it's not,
+		 * the driver has a bookkeeping error
+		 */
+		if (WARN_ON(drm_dp_put_port(port))) {

The usual way to do this (instead of adding a return value to the normal _put function) is something like

kref_put(port, function_that_with_just_a_WARNING_DRM_ERROR)

avoids the need for patch 2.

Quoted 81 lines

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

+			DRM_ERROR("cannot allocate and release VCPI on [MST

PORT:%p] in the same state\n",

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

+				  port);
+			return -EINVAL;
+		}
+		pos->vcpi = 0;
+	}
 
 	return 0;
 }

@@ -3115,15 +3211,45 @@ 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_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
+	INIT_LIST_HEAD(&state->vcpis);
+	state->vcpi_allocated = false;
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		/* Prune leftover freed VCPI allocations */
+		if (!pos->vcpi)
+			continue;
+
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail;
+
+		/* Take a plain old kref here and don't validate the port, as
+		 * it's topology might not even exist anymore
+		 */
+		kref_get(&vcpi->port->kref);
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next) {
+		drm_dp_put_port(pos->port);
+		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 +11 -1

@@ -3131,14 +3257,116 @@ 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;
+
+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next) {
+		/* We only keep references to ports with non-zero VCPIs */
+		if (pos->vcpi)
+			drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+static inline int
+drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr
 *mgr,
+				       struct drm_dp_mst_topology_state

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

Quoted 10 lines

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

 *mst_state)
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	struct drm_dp_mst_port *port;
+	int avail_slots = 63, ret;
+
+	/* There's no possible scenario where releasing VCPI or keeping it the
+	 * same would make the state invalid
+	 */
+	if (!mst_state->vcpi_allocated) {

Do we need this? If not I'm always in favour of removing code if it's not needed :-)

Quoted 12 lines

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

+		DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p allocates no VCPI,
 check skipped\n",
+				 mgr, &mst_state->base);
+		return 0;
+	}
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Releasing VCPI is always OK-even if the port is gone */
+		if (!vcpi->vcpi) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] releases all VCPI
 slots\n",
+					 vcpi->port);

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

Quoted 10 lines

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

+			continue;
+		}
+
+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone but still has
 VCPI, cannot add new VCPI\n",
+					 vcpi->port);
+			return -EINVAL;
+		}

For find_vcpi we've already done this. For unplugged vcpi, we can't do this here I think - otherwise if you enable 2 mst outputs on the same branch, and unplug both, old non-atomic userspace can never ever again disable them because it can't disable both. The check (to skip already deallocated ports) above isn't enough to handle that.

Quoted 135 lines

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

+
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+				 vcpi->port, vcpi->vcpi);
+
+		avail_slots -= vcpi->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough VCPI slots

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

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

+					 vcpi->port, mst_state,
+					 avail_slots + vcpi->vcpi);
+			ret = -ENOSPC;
+			goto port_fail;
+		}
+
+		drm_dp_put_port(port);
+	}
+	DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
+			 mgr, mst_state, avail_slots,
+			 63 - avail_slots);
+
+	return 0;
+port_fail:
+	drm_dp_put_port(port);
+	return ret;
+}
+
+/**
+ * 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
+ *
+ * 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
+ * &drm_mode_config_funcs.atomic_check() callback.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_atomic_release_vcpi_slots()
+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */

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

+int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
+	int i, ret = 0;
+
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);
+
+const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs =

{

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

 	.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

@@ -3216,13 +3444,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 +4 -0

index 132e978227fb..8e85e04a5948 100644
@@ -12592,6 +12592,10 @@ static int intel_atomic_check(struct drm_device
 *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

drivers/gpu/drm/i915/intel_dp_mst.c +6 -1

index 4de247ddf05f..be2d1ac7d87d 100644
@@ -41,8 +41,13 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	struct drm_connector *connector = conn_state->connector;
 	void *port = to_intel_connector(connector)->port;
 	struct drm_atomic_state *state = pipe_config->base.state;
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
+	struct drm_crtc_state *new_crtc_state = &pipe_config->base;
 	int bpp;
-	int lane_count, slots = 0;
+	int lane_count, slots =
+		to_intel_crtc_state(old_crtc_state)->dp_m_n.tu;
 	const struct drm_display_mode *adjusted_mode = &pipe_config-
 >base.adjusted_mode;

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0

 	int mst_pbn;
 	bool constant_n = drm_dp_has_quirk(&intel_dp->desc,

@@ -77,8 +82,12 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 91

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +6 -2

 	pipe_config->pbn = mst_pbn;
 
-	/* Zombie connectors can't have VCPI slots */
-	if (!drm_connector_is_unregistered(connector)) {
+	/* Only change VCPI allocation on actual mode changes, to prevent us
+	 * from trying to allocate VCPI to ports that no longer exist when we
+	 * may just be trying to disable DPMS on them
+	 */
+	if (new_crtc_state->mode_changed ||
+	    new_crtc_state->connectors_changed) {

Why exactly is this needed? We only call compute_config on the enocoder when enabling an output. Not when disabling one. Similar for the atomic helpers and the various encoder callbacks (you have the same change in the nouveau code). Did this actually blow up while testing?

Quoted 22 lines

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0

 		slots = drm_dp_atomic_find_vcpi_slots(state,
 						      &intel_dp->mst_mgr,

drivers/gpu/drm/i915/intel_dp_mst.c +8 -4

 						      port,

@@ -107,36 +116,39 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	return true;
 }
 
-static int intel_dp_mst_atomic_check(struct drm_connector *connector,
-		struct drm_connector_state *new_conn_state)
+static int
+intel_dp_mst_atomic_check(struct drm_connector *connector,
+			  struct drm_connector_state *new_conn_state)
 {
+	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;
 	struct drm_atomic_state *state = new_conn_state->state;
-	struct drm_connector_state *old_conn_state;
-	struct drm_crtc *old_crtc;

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 121

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +5 -4

-	struct drm_crtc_state *crtc_state;
-	int slots, ret = 0;
+	struct drm_connector_state *old_conn_state =
+		drm_atomic_get_old_connector_state(state, connector);
+	struct drm_crtc *old_crtc = old_conn_state->crtc,
+			*new_crtc = new_conn_state->crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 
-	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
-	old_crtc = old_conn_state->crtc;

Bikeshed: Reworking all the above hides the real changes quite a bit.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 139

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +3 -6

 	if (!old_crtc)
-		return ret;
+		return 0;
 
-	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;
+	old_crtc_state = drm_atomic_get_old_crtc_state(state, old_crtc);
+	if (!old_crtc_state ||

!old_crtc_state would be a framework bug. Just remove that check.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 142

Quoted 2 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+	    !to_intel_crtc_state(old_crtc_state)->dp_m_n.tu)
+		return 0;

Hm, why do we need this? Having a DP port previously enabled with a 0 tu sounds like a pretty big driver bug.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 146

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_mst.c +4 -2

-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+	/* If we switched CRTCs, clear the previous one's allocation */
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
+	if (new_crtc_state->connectors_changed && !new_crtc_state->enable)
+		to_intel_crtc_state(new_crtc_state)->dp_m_n.tu = 0;

No idea why we need to clear this here? Shouldn't we just clear this unconditionally if we release the allocation.

Also naming convention, I'd call this old_crtc_new_state, and maybe old_crtc_old_state. To make it clear these are the old/new states for old_crtc. Instead of old/new state for the new crtc.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct intel_encoder *encoder, · line 149

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -7

-		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;
-	}
-	return ret;
+	if (new_crtc)
+		return 0;

Not sure why we shouldn't release this? If we handle this in helpers, it would remove piles of special casing code here I think. Plus remove special code in the hlpers.

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+
+	return drm_dp_atomic_release_vcpi_slots(state, mgr, port);
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,

include/drm/drm_dp_mst_helper.h +0 -0

index 3faceb66f5cb..2392e12b6bce 100644

include/drm/drm_dp_mst_helper.h struct drm_dp_payload { · line 410

Quoted 5 lines

include/drm/drm_dp_mst_helper.h +1 -0

@@ -406,9 +406,16 @@ 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 {

I think a comment here that this pointer holds a full reference (even past when the connector is unplugged) would be good. Otherwise the loop in release_vcpi would be buggy.

Quoted 46 lines

include/drm/drm_dp_mst_helper.h +16 -7

+	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;
+	u8 vcpi_allocated;
 	struct drm_dp_mst_topology_mgr *mgr;
 };

@@ -619,14 +626,17 @@ void drm_dp_mst_topology_mgr_suspend(struct
 drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr
 *mgr);
 struct drm_dp_mst_topology_state
 *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct
 drm_dp_mst_topology_mgr *mgr);
-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);
-int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
-				     struct drm_dp_mst_topology_mgr *mgr,
-				     int slots);
+int __must_check
+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);
+int __must_check
+drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				 struct drm_dp_mst_topology_mgr *mgr,
+				 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);
+int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state

*state);

extern const struct drm_private_state_funcs
drm_dp_mst_topology_state_funcs;

Signature · 1 line

2.19.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Signature · 2 lines

Cheers,
	Lyude Paul

Quoted 120 lines · Lyude Paul

Quoted 108 lines · Daniel Vetter

Quoted 82 lines · Daniel Vetter

Quoted 47 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.

Quoted 13 lines

Changes since v6:
- Keep a kref to all of the ports we have allocations on. This
required
a good bit of changing to when we call drm_dp_find_vcpi_slots(), mainly that we need to ensure that we only redo VCPI allocations
on
actual mode or CRTC changes, not crtc_state->active changes. Additionally, we no longer take the registration of the DRM
connector
for each port into account because so long as we have a kref to
the
port in the new or previous atomic state, the connector will stay registered.

I write an entire pile of small nitpits (still included most of them below), until I realized this here wont work. Delaying the call to destroy
the connector (well, unregister it really) wreaks the design we've come up
with thus far, resulting in most of my comments here.

Instead, all we need to do is delay the kfree(port) at the bottom of drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the pointer value to stay valid, as a lookup key. It doesn't care at all about
anything actually stored in there. So the only thing we need to delay is
the kfree. I think the simplest way to achieve this is to add a 2nd kref
(port->kfree_ref or something like that), with on reference held by the
port itself (released in drm_dp_destroy_port), and the other one held as-needed by the vcpi allocation lists.

Quoted 4 lines

I think if we go with this design instead of retrofitting a semantic change of the port lifetime itself, all the complications I complain about
below should disappear.

In the above I meant drm_dp_destroy_port or
drm_dp_destroy_connector_work.

Aside: I think creating a kref for the final kfree would also solve a bunch of other issues in a much neater way: In

commit f038c5b99fc1332f558b495d136d4f433ee65caa

Author: Lyude Paul <[email protected]>
Date:   Tue Nov 13 17:46:14 2018 -0500

drm/dp_mst: Skip validating ports during destruction, just ref

we could use that kfree reference to make sure the port pointer is alive.
This of course means that drm_dp_update_payload_part1() would also need to
use the kfree reference for the vcpi allocations. And probably everywhere
else (e.g. in your nouveau series for payload information).

Quoted 10 lines

That would give us a very clear separation overall between "port can still
be used because it's not yet unplugged" vs. "port only hangs around because a bunch of vcpi allocations and other payload things upstream of the port might still need the port lookup key to free resources". Instead
of again sprinkling complicated conditions and magic tricks all over the code to figure out whethe we should allow the get_validate_port to succeed
or not, or the modeset to succeed or not.

Quoted 4 lines

mhm, I've been experimenting with this and I think I agree this is definitely
the right solution to go with. It'll end up decomplicating a lot of this :)

Since it looks like we'll use the kfree_ref quite a bit, wrappers for that would be useful. Still not sure what to call those, best I could come up with is dp_port_malloc_get/put. Plus a comment explaining what it does. It's going to be a bit of wtf-is-this no matter what :-/ -Daniel

Quoted 829 lines

Quoted 829 lines

-Daniel

Quoted 826 lines

Piles of comments below.

Cheers, Daniel

Quoted 177 lines

- Use the small changes to drm_dp_put_port() to add even more error checking to make misusage of the helpers more obvious. I added
this
after having to chase down various use-after-free conditions that started popping up from the new helpers so no one else has to troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both
not
be
called on the same port in a single atomic check phase (but
multiple
calls to one or the other is OK)

Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI allocations happen in a state. This makes the next change I'm
about
to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure
that
when ports are destroyed and still have VCPI allocations in the topology state, the only state changes allowed are releasing said ports' VCPI. This prevents a state with a mix of VCPI allocations from destroyed ports, and allocations from valid ports.

Changes since v3:
- Don't release VCPI allocations in the topology state immediately
in
drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and
skip
over them in drm_dp_mst_duplicate_state(). This makes it so drm_dp_atomic_release_vcpi_slots() is still idempotent while also throwing warnings if the driver messes up it's book keeping and
tries
to release VCPI slots on a port that doesn't have any pre-
existing
VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"

Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into
it's
own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth requirements. This is needed because originally we relied on the state duplication functions to prune any stale ports from the new state, which would prevent us from incorrectly considering their bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() -
danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet

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 | 294
+++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  64 +++---
 include/drm/drm_dp_mst_helper.h       |  24 ++-
 4 files changed, 319 insertions(+), 67 deletions(-)

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

index 00fbe7a2699d..183d0e832ced 100644
@@ -2617,21 +2617,42 @@ 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
*
- * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * 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 &drm_encoder_helper_funcs.atomic_check() callback to change
the
+ * current VCPI allocation for the new state, but only when
+ * &drm_crtc_state.mode_changed or &drm_crtc_state.connectors_changed
is set
+ * to ensure compatibility with userspace applications that still use
the
+ * legacy modesetting UAPI.
+ *
+ * Allocations set by this function are not checked against the bandwidth
+ * restraints of @mgr until the driver calls drm_dp_mst_atomic_check().
+ *
+ * Additionally, it is OK to call this function multiple times on the
same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
+ *
+ * See also:
+ * drm_dp_atomic_release_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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 +0 -0

 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int

pbn)
{

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

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

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

 	if (IS_ERR(topology_state))

@@ -2640,20 +2661,60 @@ 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;
+	topology_state->vcpi_allocated = true;
+
+	/* 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;
+
+			/*
+			 * This should never happen, unless the driver

tries

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

+			 * releasing and allocating the same VCPI

allocation,

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

+			 * which is an error
+			 */
+			if (WARN_ON(!prev_slots)) {
+				DRM_ERROR("cannot allocate and release

VCPI on
[MST PORT:%p] in the same state\n",

drivers/gpu/drm/drm_dp_mst_topology.c

Quoted 3 lines

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

+					  port);
+				return -EINVAL;
+			}

Allowing atomic changes to output state (like disabling it on one crtc,
and then enabling on the other, thus both releasing and re-allocating the
vcpi in one go) is kinda the point of atomic. So definitely can't EINVAL
here.

Sounds like we need more testcases in igt?

Quoted 140 lines

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

+
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state-

Quoted 1 line

avail_slots);

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

+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] VCPI %d ->

%d\n",

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

+			 port->connector->base.id, port->connector-

Quoted 1 line

name,

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

+			 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;
+		}
 
+		vcpi->port = drm_dp_get_validated_port_ref(mgr, port);
+		if (!vcpi->port) {
+			ret = -EINVAL;
+			kfree(vcpi);
+			goto out;
+		}
+		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);

@@ -2661,31 +2722,66 @@
 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
+ * @port: The port to release the VCPI slots from
*
- * RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state-

Quoted 1 line

avail_slots or

- * negative error code
+ * 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 &drm_connector_helper_funcs.atomic_check() callback when the
+ * connector will no longer have VCPI allocated (e.g. because it's CRTC
was
+ * removed) when it had VCPI allocated in the previous atomic state.
+ *
+ * It is OK to call this even if @port has been removed from the system.
+ * Additionally, it is OK to call this function multiple times on the
same
+ * @port as needed. It is not OK however, to call this function and
+ * drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic
check
+ * phase.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * 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,

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

-				     int slots)
+				     struct drm_dp_mst_port *port)
 {
 	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_dp_vcpi_allocation *pos;
+	bool found = false;
 
 	topology_state = drm_atomic_get_mst_topology_state(state,

mgr);

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

 	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

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

-	 * branch device was unplugged. This should be fixed by

tracking

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

-	 * per-port slot allocation in drm_dp_mst_topology_state

instead of

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

-	 * depending on the caller to tell us how many slots to

release.

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

-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			found = true;
+			break;
+		}
+	}
+	if (WARN_ON(!found)) {
+		DRM_ERROR("no VCPI for [MST PORT:%p] found in mst

state %p\n",

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

+			  port, &topology_state->base);
+		return -EINVAL;
+	}
+
+	DRM_DEBUG_ATOMIC("[MST PORT:%p] VCPI %d -> 0\n", port, pos-

Quoted 1 line

vcpi);

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

+	if (pos->vcpi) {
+		/*
+		 * This should be safe-as the previous state will

still hold a

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

+		 * reference to this port until we swap states. If

it's not,

drivers/gpu/drm/drm_dp_mst_topology.c

Quoted 3 lines

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

+		 * the driver has a bookkeeping error
+		 */
+		if (WARN_ON(drm_dp_put_port(port))) {

The usual way to do this (instead of adding a return value to the normal
_put function) is something like

kref_put(port, function_that_with_just_a_WARNING_DRM_ERROR)

avoids the need for patch 2.

Quoted 96 lines

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

+			DRM_ERROR("cannot allocate and release VCPI on

[MST
PORT:%p] in the same state\n",

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

+				  port);
+			return -EINVAL;
+		}
+		pos->vcpi = 0;
+	}
 
 	return 0;
 }

@@ -3115,15 +3211,45 @@ 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) {

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

-	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_vcpi_allocation *pos, *vcpi;
 
-	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state-

Quoted 1 line

base);

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

+	INIT_LIST_HEAD(&state->vcpis);
+	state->vcpi_allocated = false;
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		/* Prune leftover freed VCPI allocations */
+		if (!pos->vcpi)
+			continue;
+
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail;
+
+		/* Take a plain old kref here and don't validate the

port, as

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

+		 * it's topology might not even exist anymore
+		 */
+		kref_get(&vcpi->port->kref);
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next) {
+		drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

@@ -3131,14 +3257,116 @@ 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;
+
+	list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next) {
+		/* We only keep references to ports with non-zero

VCPIs */

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

+		if (pos->vcpi)
+			drm_dp_put_port(pos->port);
+		kfree(pos);
+	}
 
 	kfree(mst_state);
 }
 
-static const struct drm_private_state_funcs mst_state_funcs = {
+static inline int
+drm_dp_mst_atomic_check_topology_state(struct

drm_dp_mst_topology_mgr
*mgr,

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

+				       struct

drm_dp_mst_topology_state
*mst_state)

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

+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	struct drm_dp_mst_port *port;
+	int avail_slots = 63, ret;
+
+	/* There's no possible scenario where releasing VCPI or

keeping it the

drivers/gpu/drm/drm_dp_mst_topology.c

Quoted 3 lines

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

+	 * same would make the state invalid
+	 */
+	if (!mst_state->vcpi_allocated) {

Do we need this? If not I'm always in favour of removing code if it's not
needed :-)

Quoted 23 lines

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

+		DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p allocates no

VCPI,
check skipped\n",

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

+				 mgr, &mst_state->base);
+		return 0;
+	}
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Releasing VCPI is always OK-even if the port is

gone */

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

+		if (!vcpi->vcpi) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] releases all

VCPI
slots\n",

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

+					 vcpi->port);
+			continue;
+		}
+
+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone but

still has
VCPI, cannot add new VCPI\n",

drivers/gpu/drm/drm_dp_mst_topology.c

Quoted 3 lines

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

+					 vcpi->port);
+			return -EINVAL;
+		}

For find_vcpi we've already done this. For unplugged vcpi, we can't do this here I think - otherwise if you enable 2 mst outputs on the same branch, and unplug both, old non-atomic userspace can never ever again disable them because it can't disable both. The check (to skip already deallocated ports) above isn't enough to handle that.

Quoted 152 lines

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

+
+		DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi

slots\n",

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

+				 vcpi->port, vcpi->vcpi);
+
+		avail_slots -= vcpi->vcpi;
+		if (avail_slots < 0) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough

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

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

+					 vcpi->port, mst_state,
+					 avail_slots + vcpi->vcpi);
+			ret = -ENOSPC;
+			goto port_fail;
+		}
+
+		drm_dp_put_port(port);
+	}
+	DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d

used=%d\n",

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

+			 mgr, mst_state, avail_slots,
+			 63 - avail_slots);
+
+	return 0;
+port_fail:
+	drm_dp_put_port(port);
+	return ret;
+}
+
+/**
+ * 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
+ *
+ * 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
+ * &drm_mode_config_funcs.atomic_check() callback.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * drm_dp_atomic_release_vcpi_slots()
+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */

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

+int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
+{
+	struct drm_dp_mst_topology_mgr *mgr;
+	struct drm_dp_mst_topology_state *mst_state;
+	int i, ret = 0;
+
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		ret = drm_dp_mst_atomic_check_topology_state(mgr,

mst_state);

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

+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);
+
+const struct drm_private_state_funcs

drm_dp_mst_topology_state_funcs =
{

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

 	.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

@@ -3216,13 +3444,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 +0 -0

index 132e978227fb..8e85e04a5948 100644
@@ -12592,6 +12592,10 @@ static int intel_atomic_check(struct

drm_device
*dev,

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

 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

drivers/gpu/drm/i915/intel_dp_mst.c +6 -1

index 4de247ddf05f..be2d1ac7d87d 100644
@@ -41,8 +41,13 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	struct drm_connector *connector = conn_state->connector;
 	void *port = to_intel_connector(connector)->port;
 	struct drm_atomic_state *state = pipe_config->base.state;
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
+	struct drm_crtc_state *new_crtc_state = &pipe_config->base;
 	int bpp;
-	int lane_count, slots = 0;
+	int lane_count, slots =
+		to_intel_crtc_state(old_crtc_state)->dp_m_n.tu;
 	const struct drm_display_mode *adjusted_mode = &pipe_config-
 > base.adjusted_mode;

drivers/gpu/drm/i915/intel_dp_mst.c +1 -2

 	int mst_pbn;
 	bool constant_n = drm_dp_has_quirk(&intel_dp->desc,

@@ -77,8 +82,12 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
 bpp);
 	pipe_config->pbn = mst_pbn;
 
-	/* Zombie connectors can't have VCPI slots */
-	if (!drm_connector_is_unregistered(connector)) {
+	/* Only change VCPI allocation on actual mode changes, to

prevent us

drivers/gpu/drm/i915/intel_dp_mst.c +1 -0

+	 * from trying to allocate VCPI to ports that no longer exist

when we

drivers/gpu/drm/i915/intel_dp_mst.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_dp_mst.c +4 -0

+	 * may just be trying to disable DPMS on them
+	 */
+	if (new_crtc_state->mode_changed ||
+	    new_crtc_state->connectors_changed) {

Why exactly is this needed? We only call compute_config on the enocoder
when enabling an output. Not when disabling one. Similar for the atomic
helpers and the various encoder callbacks (you have the same change in the
nouveau code). Did this actually blow up while testing?

Quoted 25 lines

drivers/gpu/drm/i915/intel_dp_mst.c +0 -0

 		slots = drm_dp_atomic_find_vcpi_slots(state,
 						      &intel_dp-

Quoted 1 line

mst_mgr,

drivers/gpu/drm/i915/intel_dp_mst.c +8 -5

 						      port,

@@ -107,36 +116,39 @@ static bool intel_dp_mst_compute_config(struct
 intel_encoder *encoder,
 	return true;
 }
 
-static int intel_dp_mst_atomic_check(struct drm_connector
 *connector,
-		struct drm_connector_state *new_conn_state)
+static int
+intel_dp_mst_atomic_check(struct drm_connector *connector,
+			  struct drm_connector_state *new_conn_state)
 {
+	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;
 	struct drm_atomic_state *state = new_conn_state->state;
-	struct drm_connector_state *old_conn_state;
-	struct drm_crtc *old_crtc;
-	struct drm_crtc_state *crtc_state;

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 123

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +5 -3

-	int slots, ret = 0;
+	struct drm_connector_state *old_conn_state =
+		drm_atomic_get_old_connector_state(state, connector);
+	struct drm_crtc *old_crtc = old_conn_state->crtc,
+			*new_crtc = new_conn_state->crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 
-	old_conn_state = drm_atomic_get_old_connector_state(state,
 connector);
-	old_crtc = old_conn_state->crtc;

Bikeshed: Reworking all the above hides the real changes quite a bit.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 143

Quoted 12 lines

drivers/gpu/drm/i915/intel_dp_mst.c +3 -6

 	if (!old_crtc)
-		return ret;
+		return 0;
 
-	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;
+	old_crtc_state = drm_atomic_get_old_crtc_state(state,
 old_crtc);
+	if (!old_crtc_state ||

!old_crtc_state would be a framework bug. Just remove that check.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 145

Quoted 2 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+	    !to_intel_crtc_state(old_crtc_state)->dp_m_n.tu)
+		return 0;

Hm, why do we need this? Having a DP port previously enabled with a 0 tu
sounds like a pretty big driver bug.

drivers/gpu/drm/i915/intel_dp_mst.c static bool intel_dp_mst_compute_config(struct · line 152

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +4 -2

-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+	/* If we switched CRTCs, clear the previous one's allocation
 */
+	new_crtc_state = drm_atomic_get_new_crtc_state(state,
 old_crtc);
+	if (new_crtc_state->connectors_changed && !new_crtc_state-
 >enable)
+		to_intel_crtc_state(new_crtc_state)->dp_m_n.tu = 0;

No idea why we need to clear this here? Shouldn't we just clear this unconditionally if we release the allocation.

Also naming convention, I'd call this old_crtc_new_state, and maybe old_crtc_old_state. To make it clear these are the old/new states for old_crtc. Instead of old/new state for the new crtc.

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp_mst.c +0 -3

-		ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
 slots);
-		if (ret)
-			DRM_DEBUG_KMS("failed releasing %d vcpi
 slots:%d\n",

slots, ret);

drivers/gpu/drm/i915/intel_dp_mst.c +0 -2

-		else
-			to_intel_crtc_state(crtc_state)->dp_m_n.tu =

0;

drivers/gpu/drm/i915/intel_dp_mst.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -2

-	}
-	return ret;
+	if (new_crtc)
+		return 0;

Not sure why we shouldn't release this? If we handle this in helpers, it
would remove piles of special casing code here I think. Plus remove special code in the hlpers.

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

+
+	return drm_dp_atomic_release_vcpi_slots(state, mgr, port);
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,

include/drm/drm_dp_mst_helper.h +0 -0

index 3faceb66f5cb..2392e12b6bce 100644

include/drm/drm_dp_mst_helper.h struct drm_dp_payload { · line 410

Quoted 5 lines

include/drm/drm_dp_mst_helper.h +1 -0

@@ -406,9 +406,16 @@ 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 {

I think a comment here that this pointer holds a full reference (even past
when the connector is unplugged) would be good. Otherwise the loop in release_vcpi would be buggy.

Quoted 46 lines

include/drm/drm_dp_mst_helper.h +7 -1

+	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;
+	u8 vcpi_allocated;
 	struct drm_dp_mst_topology_mgr *mgr;
 };

@@ -619,14 +626,17 @@ void drm_dp_mst_topology_mgr_suspend(struct
 drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr
 *mgr);
 struct drm_dp_mst_topology_state
 *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    st

ruct
drm_dp_mst_topology_mgr *mgr);

include/drm/drm_dp_mst_helper.h +0 -3

-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);
-int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,

include/drm/drm_dp_mst_helper.h +0 -1

-				     struct drm_dp_mst_topology_mgr

*mgr,

include/drm/drm_dp_mst_helper.h +8 -1

-				     int slots);
+int __must_check
+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);
+int __must_check
+drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				 struct drm_dp_mst_topology_mgr *mgr,
+				 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);
+int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state *state);

Quoted 6 lines

extern const struct drm_private_state_funcs
drm_dp_mst_topology_state_funcs;

--
2.19.1

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch