drm/dp_mst: Add some atomic state iterator macros

PATCH New Lyude Paul 5 participants 15 messages Nov 7, 2018 4 messages not in the archive

Subsystems (4)

DRM drivers DRM misc Intel i915 nouveau

lore

Share & tools

lore

Lyude Paul

author maintainer

2018-11-07 01:21 UTC #0 lore raw

v3 cover drm/dp_mst: Add some atomic state iterator macros

This patchset does some cleaning up of the atomic VCPI helpers for MST, and converts nouveau over to using them. I would have included amdgpu in this patch as well, but at the moment moving them over to the atomic helpers is nontrivial.

Cc Daniel Vetter <[email protected]>

Lyude Paul (5):
drm/dp_mst: Add some atomic state iterator macros drm/dp_mst: Start tracking per-port VCPI allocations drm/dp_mst: Check payload count in drm_dp_mst_atomic_check() drm/nouveau: Use atomic VCPI helpers for MST
drm/dp_mst: Stop unsetting mstc->port, check connector registration

 drivers/gpu/drm/drm_dp_mst_topology.c   | 229 ++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c    |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c     |  31 ++--
 drivers/gpu/drm/nouveau/dispnv50/disp.c |  65 +++++--
 include/drm/drm_dp_mst_helper.h         | 100 ++++++++++-
 5 files changed, 359 insertions(+), 70 deletions(-)

Signature · 6 lines

2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

A message that is not in the archive (sent off-list or not yet received): 20181107012116.17332-1-lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Lyude Paul

2018-11-07 01:21 UTC #2 lore raw

PATCH v3 1/5 drm/dp_mst: Add some atomic state iterator macros

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 +77 -0

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

Lyude Paul

2018-11-07 01:21 UTC #3 lore raw

PATCH v3 2/5 drm/dp_mst: Start tracking per-port VCPI allocations

In reply to a message not in the archive

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 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 | 222 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h       |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)

drivers/gpu/drm/drm_dp_mst_topology.c +188 ~~-34~~

index 8c3cfac437f4..74823afb262e 100644
@@ -2614,21 +2614,34 @@ 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. The allocations are not checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * 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))

@@ -2637,20 +2650,41 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
 
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
+
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);

@@ -2658,32 +2692,50 @@ 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).
+ *
+ * It is OK to call this even if @port has been removed from the system, in
+ * which case it will just amount to a no-op.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * 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 *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3164,34 @@ 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);
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail_alloc;
+
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

@@ -3128,14 +3199,99 @@ 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)
+		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 *mst_state)
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	struct drm_dp_mst_port *port;
+	int avail_slots = 63, ret;
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Ports that no longer exist shouldn't be counted towards the
+		 * bandwidth requirements for the state, so skip them
+		 */
+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping checks\n",
+					 vcpi->port);
+			continue;
+		}
+
+		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

@@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644
@@ -12553,6 +12553,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 +17 ~~-14~~

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

include/drm/drm_dp_mst_helper.h +16 -7

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

@@ -619,14 +625,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 · 6 lines

2.19.1

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

Daniel Vetter

2018-11-07 20:59 UTC #4 lore raw

Re: [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

Quoted 211 lines · Lyude Paul

There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */
That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.
Changes since 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 | 222 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h       |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +70 ~~-27~~
index 8c3cfac437f4..74823afb262e 100644
@@ -2614,21 +2614,34 @@ 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. The allocations are not checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * 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))
@@ -2637,20 +2650,41 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
 
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
+
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
 
@@ -2658,32 +2692,50 @@ 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).
+ *
+ * It is OK to call this even if @port has been removed from the system, in
+ * which case it will just amount to a no-op.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * 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 *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",

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

Quoted 10 lines

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

+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */

This case here has me a bit worried. I thought with your changes we'll always have the port pointer still around, to be able to clean things up properly. If this is not the case then we'd potentially leak the vcpi allocation for an unplugged port.

Otoh if the driver releases the allocation for a port twice it's probably mixed up it's own book-keeping. We should WARN about that case.

This is kinda fallout from your change to not prune allocations on duplicate, and I think will further clean up how this is handled.

So with the comment removed and replaced by a WARNING(1,"no vcpi allocation found for port %p\n", port) or something like that this should be solid.

Quoted 55 lines

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

 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3164,34 @@ 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);
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail_alloc;
+
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

@@ -3128,14 +3199,99 @@ 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)
+		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 3221

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;
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Ports that no longer exist shouldn't be counted towards the
+		 * bandwidth requirements for the state, so skip them
+		 */

Scenario:
- 2 mst sinks on a single host side port.
- 1st connector is active, 2nd connector is off.
- 1st connector is hot-unplugged, but userspace hasn't realized this yet, so not yet come around to do the disabling modeset.
- At the same time userspace decides to enable the 2nd output, which forces a modeset and pulls the topology state in.
- This function here decides to only look at the vcpi allocation for the 2nd connector because the first one is gone already.
- Unfortunately lighting up both connectors at the same time isn't possible, because too many vcpi. But since userspace didn't ask for a modeset on the now unplugged first connector.
-> Modeset goes boom because we enable too many vcpi.

But with the change in how we release vcpi allocations there's no need to ignore disconnected stuff here, and we should be able to check vcpi limits by just checking all of them.

I didn't see anything else serious, so with the above two things resolved this has my Reviewed-by: Daniel Vetter <[email protected]>

One bikeshed below, because I need to prove I scrolled to the end :-)

Quoted 97 lines

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

+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping checks\n",
+					 vcpi->port);
+			continue;
+		}
+
+		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

@@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_check(struct drm_device *dev, · line 12558

Quoted 6 lines

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

@@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct drm_device *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;

Complete bikeshed, so feel free to ignore: I'd stuff this into intel_modeset_checks(). Feel free to ignore.

Cheers, Daniel

Quoted 102 lines

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

+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

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

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

include/drm/drm_dp_mst_helper.h +16 -7

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

@@ -619,14 +625,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 · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

A message that is not in the archive (sent off-list or not yet received): 20181107205930.GY21967-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org

Lyude Paul

2018-11-07 21:23 UTC #6 lore raw

Re: [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

Quoted 244 lines · Daniel Vetter

Quoted 168 lines · Lyude Paul
There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */
That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.
Changes since 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 | 222 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h       |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +2 -3
index 8c3cfac437f4..74823afb262e 100644
@@ -2614,21 +2614,34 @@ 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. The allocations are not checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * 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 +32 ~~-10~~
 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn)
 {
 	struct drm_dp_mst_topology_state *topology_state;
-	int req_slots;
+	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+	int prev_slots, req_slots, ret;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
@@ -2637,20 +2650,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
 drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
 
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
+
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2664
Quoted 8 lines
drivers/gpu/drm/drm_dp_mst_topology.c +1 -3
@@ -2658,32 +2692,50 @@ 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
Quoted 47 lines
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).
+ *
+ * It is OK to call this even if @port has been removed from the system, in
+ * which case it will just amount to a no-op.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * 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 +6 ~~-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 *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2738
Quoted 10 lines
drivers/gpu/drm/drm_dp_mst_topology.c +9 -0
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
This case here has me a bit worried. I thought with your changes we'll always have the port pointer still around, to be able to clean things up properly. If this is not the case then we'd potentially leak the vcpi allocation for an unplugged port.
Otoh if the driver releases the allocation for a port twice it's probably mixed up it's own book-keeping. We should WARN about that case.

Quoted 6 lines

This is kinda fallout from your change to not prune allocations on duplicate, and I think will further clean up how this is handled.
So with the comment removed and replaced by a WARNING(1,"no vcpi allocation found for port %p\n", port) or something like that this should be solid.

Hm; that comment is leftover from the fallout of moving the port verification logic out of the state duplicate functions, but I don't think this would nessecarily be a bookkeeping error. If we're going to support fallback link retraining, we'll need to make sure that every driver's drm_connector->atomic_check() function is idempotent so that if the atomic helpers have to pull in more CRTCs to perform modesets on then userspace originally added, the second check on the DRM connector doesn't cause any issues. Remember: if we're trying to retrain an MST topology at a lower link rate then before, that means changing the PBN divisor which in turn means that the only valid actions that can be taken are recalculating every active VCPI allocation at once in order to prevent state inconsistencies, or disabling all of the displays (since going from >0 VCPI to 0 VCPI should always be safe).

We /could/ just leave this up to the driver to do the bookkeeping on this, but that seems rather redundant. We could add extra bookkeeping in the atomic check though so that instead of just deleting the structs for each VCPI allocation when they go to 0, we instead just set them to 0 then free them somewhere else.

Quoted 323 lines

Quoted 61 lines

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

 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3164,34 @@ 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);
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail_alloc;
+
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }

static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

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

@@ -3128,14 +3199,99 @@ 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)
+		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 3224

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;
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Ports that no longer exist shouldn't be counted towards the
+		 * bandwidth requirements for the state, so skip them
+		 */

But with the change in how we release vcpi allocations there's no need to ignore disconnected stuff here, and we should be able to check vcpi limits by just checking all of them.

I didn't see anything else serious, so with the above two things resolved this has my Reviewed-by: Daniel Vetter <[email protected]>

One bikeshed below, because I need to prove I scrolled to the end :-)

Quoted 104 lines

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

+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping
 checks\n",
+					 vcpi->port);
+			continue;
+		}
+
+		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.
+ */

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

+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

@@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_check(struct drm_device · line 12559

Quoted 7 lines

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

@@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct drm_device
 *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;

Complete bikeshed, so feel free to ignore: I'd stuff this into intel_modeset_checks(). Feel free to ignore.

Cheers, Daniel

Quoted 111 lines

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

+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

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

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

}

include/drm/drm_dp_mst_helper.h +16 -7

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

@@ -619,14 +625,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 · 7 lines

Cheers,
	Lyude Paul

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

Lyude Paul

author maintainer

2018-11-07 21:39 UTC #7 lore raw

Re: [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

Quoted 274 lines · Lyude Paul

Quoted 251 lines · Daniel Vetter
Quoted 173 lines · Lyude Paul
There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */
That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.
Changes since 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 | 222 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h       |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +0 -1
index 8c3cfac437f4..74823afb262e 100644
@@ -2614,21 +2614,34 @@ 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. The allocations are not checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * 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 +32 ~~-10~~
 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn)
 {
 	struct drm_dp_mst_topology_state *topology_state;
-	int req_slots;
+	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+	int prev_slots, req_slots, ret;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
@@ -2637,20 +2650,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
 drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
 
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
+
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2664
Quoted 8 lines
drivers/gpu/drm/drm_dp_mst_topology.c +1 -3
@@ -2658,32 +2692,50 @@ 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
Quoted 49 lines
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).
+ *
+ * It is OK to call this even if @port has been removed from the system,
in
+ * which case it will just amount to a no-op.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * 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 +6 ~~-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 *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2738
Quoted 10 lines
drivers/gpu/drm/drm_dp_mst_topology.c +9 -0
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
This case here has me a bit worried. I thought with your changes we'll always have the port pointer still around, to be able to clean things up properly. If this is not the case then we'd potentially leak the vcpi allocation for an unplugged port.
Otoh if the driver releases the allocation for a port twice it's probably mixed up it's own book-keeping. We should WARN about that case.
Quoted 6 lines
This is kinda fallout from your change to not prune allocations on duplicate, and I think will further clean up how this is handled.
So with the comment removed and replaced by a WARNING(1,"no vcpi allocation found for port %p\n", port) or something like that this should be solid.
Hm; that comment is leftover from the fallout of moving the port verification
logic out of the state duplicate functions, but I don't think this would nessecarily be a bookkeeping error. If we're going to support fallback link retraining, we'll need to make sure that every driver's drm_connector->atomic_check() function is idempotent so that if the atomic helpers have to pull in more CRTCs to perform modesets on then userspace originally added, the second check on the DRM connector doesn't cause any issues. Remember: if we're trying to retrain an MST topology at a lower link rate then before, that means changing the PBN divisor which in turn means that the only valid actions that can be taken are recalculating every active VCPI allocation at once in order to prevent state inconsistencies, or disabling all of the displays (since going from >0 VCPI to 0 VCPI should always be safe).

Quoted 5 lines

We /could/ just leave this up to the driver to do the bookkeeping on this, but that seems rather redundant. We could add extra bookkeeping in the atomic check though so that instead of just deleting the structs for each VCPI allocation when they go to 0, we instead just set them to 0 then free them somewhere else.

jfyi; I think it would go something like this:

drm_dp_atomic_release_vcpi_slots():

for each vcpi:
if vcpi.port == port:
vcpi.vcpi = 0
return 0;

drm_error("No previous VCPI allocation found! Help!!!\n"); return -EINVAL;

Then we'd need to add something to prune the VCPI allocations in the state, which the driver would call after all atomic checks/mode validations have been performed, including drm_dp_mst_atomic_check()

drm_dp_mst_atomic_check_finish():
for each vcpi:

    if vcpi.vcpi == 0:
      del vcpi

Then, that way we've made sure that multiple VCPI frees in the same check are OK, but freeing VCPI that never existed actually throws errors

Quoted 328 lines

Quoted 61 lines

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

 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3164,34 @@ 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);
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail_alloc;
+
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }

static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

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

@@ -3128,14 +3199,99 @@ 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)
+		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 3224

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;
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Ports that no longer exist shouldn't be counted towards the
+		 * bandwidth requirements for the state, so skip them
+		 */

But with the change in how we release vcpi allocations there's no need to ignore disconnected stuff here, and we should be able to check vcpi limits by just checking all of them.

I didn't see anything else serious, so with the above two things resolved this has my Reviewed-by: Daniel Vetter <[email protected]>

One bikeshed below, because I need to prove I scrolled to the end :-)

Quoted 107 lines

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

+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping
 checks\n",
+					 vcpi->port);
+			continue;
+		}
+
+		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.
+ */

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

@@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_check(struct drm_device · line 12559

Quoted 7 lines

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

@@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct drm_device
 *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;

Complete bikeshed, so feel free to ignore: I'd stuff this into intel_modeset_checks(). Feel free to ignore.

Cheers, Daniel

Quoted 113 lines

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

+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

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

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

}

include/drm/drm_dp_mst_helper.h +16 -7

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

@@ -619,14 +625,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 · 7 lines

Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): aa94ffb871956a5f235be27194e58073ce20ce57.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Daniel Vetter

2018-11-07 21:59 UTC #9 lore raw

Re: [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

Quoted 297 lines · Lyude Paul

Quoted 274 lines · Lyude Paul
Quoted 251 lines · Daniel Vetter
Quoted 173 lines · Lyude Paul
There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release. */
That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.
Changes since 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 | 222 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h       |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +0 -1
index 8c3cfac437f4..74823afb262e 100644
@@ -2614,21 +2614,34 @@ 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. The allocations are not checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * 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 +32 ~~-10~~
 				  struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_dp_mst_port *port, int pbn)
 {
 	struct drm_dp_mst_topology_state *topology_state;
-	int req_slots;
+	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+	int prev_slots, req_slots, ret;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
@@ -2637,20 +2650,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
 drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+		      port->connector->base.id, port->connector->name, port,
+		      prev_slots, req_slots);
 
+	/* Add the new allocation to the state */
+	if (!vcpi) {
+		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		vcpi->port = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
+
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2664
Quoted 8 lines
drivers/gpu/drm/drm_dp_mst_topology.c +1 -3
@@ -2658,32 +2692,50 @@ 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
Quoted 49 lines
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).
+ *
+ * It is OK to call this even if @port has been removed from the system,
in
+ * which case it will just amount to a no-op.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * 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 +6 ~~-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 *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
 
-	/* We cannot rely on port->vcpi.num_slots to update
-	 * topology_state->avail_slots as the port may not exist if the parent
-	 * branch device was unplugged. This should be fixed by tracking
-	 * per-port slot allocation in drm_dp_mst_topology_state instead of
-	 * depending on the caller to tell us how many slots to release.
-	 */
-	topology_state->avail_slots += slots;
-	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
-			slots, topology_state->avail_slots);
+	list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
drivers/gpu/drm/drm_dp_mst_topology.c EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); · line 2738
Quoted 10 lines
drivers/gpu/drm/drm_dp_mst_topology.c +9 -0
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port was
+	 * destroyed since the last atomic commit. That's OK!
+	 */
This case here has me a bit worried. I thought with your changes we'll always have the port pointer still around, to be able to clean things up properly. If this is not the case then we'd potentially leak the vcpi allocation for an unplugged port.
Otoh if the driver releases the allocation for a port twice it's probably mixed up it's own book-keeping. We should WARN about that case.
Quoted 6 lines
This is kinda fallout from your change to not prune allocations on duplicate, and I think will further clean up how this is handled.
So with the comment removed and replaced by a WARNING(1,"no vcpi allocation found for port %p\n", port) or something like that this should be solid.
Hm; that comment is leftover from the fallout of moving the port verification
logic out of the state duplicate functions, but I don't think this would nessecarily be a bookkeeping error. If we're going to support fallback link retraining, we'll need to make sure that every driver's drm_connector->atomic_check() function is idempotent so that if the atomic helpers have to pull in more CRTCs to perform modesets on then userspace originally added, the second check on the DRM connector doesn't cause any issues. Remember: if we're trying to retrain an MST topology at a lower link rate then before, that means changing the PBN divisor which in turn means that the only valid actions that can be taken are recalculating every active VCPI allocation at once in order to prevent state inconsistencies, or disabling all of the displays (since going from >0 VCPI to 0 VCPI should always be safe).
Quoted 5 lines
We /could/ just leave this up to the driver to do the bookkeeping on this, but that seems rather redundant. We could add extra bookkeeping in the atomic check though so that instead of just deleting the structs for each VCPI allocation when they go to 0, we instead just set them to 0 then free them somewhere else.
jfyi; I think it would go something like this:
drm_dp_atomic_release_vcpi_slots():
for each vcpi:
if vcpi.port == port:
vcpi.vcpi = 0
return 0;
drm_error("No previous VCPI allocation found! Help!!!\n"); return -EINVAL;
Then we'd need to add something to prune the VCPI allocations in the state, which the driver would call after all atomic checks/mode validations have been performed, including drm_dp_mst_atomic_check()

Quoted 7 lines

drm_dp_mst_atomic_check_finish():
for each vcpi:
    if vcpi.vcpi == 0:
      del vcpi
Then, that way we've made sure that multiple VCPI frees in the same check are OK, but freeing VCPI that never existed actually throws errors

Not sure that's needed at all. The way link training works is we just ask userspace to do the heavy lifting for us, by issuing a new modeset. If that means mode pruning, we also ask userspace to do the heavy lifting for us, again by issuing a new modeset. Kernel never needs to retrain if that means recomputing state stuff or pruning modes or other fun problems. It doesn't even need to retrain if that means shutting down a bunch of links, it can just punt that to userspace and let it retrain at a convenient time.

Of course that retrain might then result in a modeset, which might fail if the connector is entirely gone by then.

Otoh I might just be talking nonsense right now, it's a bit late :-)

Cheers, Daniel

Quoted 332 lines

Quoted 328 lines

Quoted 61 lines

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

 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);

@@ -3112,15 +3164,34 @@ 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);
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail_alloc;
+
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }

static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,

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

@@ -3128,14 +3199,99 @@ 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)
+		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 3224

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;
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Ports that no longer exist shouldn't be counted towards the
+		 * bandwidth requirements for the state, so skip them
+		 */

But with the change in how we release vcpi allocations there's no need to ignore disconnected stuff here, and we should be able to check vcpi limits by just checking all of them.

I didn't see anything else serious, so with the above two things resolved this has my Reviewed-by: Daniel Vetter <[email protected]>

One bikeshed below, because I need to prove I scrolled to the end :-)

Quoted 107 lines

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

+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping
 checks\n",
+					 vcpi->port);
+			continue;
+		}
+
+		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

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

@@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_check(struct drm_device · line 12559

Quoted 7 lines

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

@@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct drm_device
 *dev,
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;

Complete bikeshed, so feel free to ignore: I'd stuff this into intel_modeset_checks(). Feel free to ignore.

Cheers, Daniel

Quoted 113 lines

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

+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);

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

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

}

include/drm/drm_dp_mst_helper.h +16 -7

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

@@ -619,14 +625,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 · 2 lines

Cheers,
	Lyude Paul

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 20181107215939.GA21967-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org

Lyude Paul

2018-11-07 22:41 UTC #11 lore raw

Re: [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

Quoted 360 lines · Daniel Vetter

Quoted 168 lines · Lyude Paul
Quoted 167 lines · Lyude Paul
Quoted 166 lines · Daniel Vetter
Quoted 165 lines · Lyude Paul
There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c:
	/* We cannot rely on port->vcpi.num_slots to update
	 * topology_state->avail_slots as the port may not exist if
the parent
* branch device was unplugged. This should be fixed by
tracking
* per-port slot allocation in drm_dp_mst_topology_state
instead of
* depending on the caller to tell us how many slots to
release.
	 */
That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the
order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks
with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
each port. This allows us to ensure idempotency without having to rely
on the driver as much. Additionally: the driver doesn't need to do any
kind of VCPI slot tracking anymore if it doesn't need it for it's own
internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new
VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST.
Changes since 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 | 222 ++++++++++++++++++++++-
---
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h       |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +0 -1
index 8c3cfac437f4..74823afb262e 100644
@@ -2614,21 +2614,34 @@ 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. The allocations are not
checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * 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 +11 -6
 	if (IS_ERR(topology_state))
@@ -2637,20 +2650,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
 drm_atomic_state *state,
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
 		return -EINVAL;
-	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
-	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
-			req_slots, topology_state->avail_slots);
 
-	if (req_slots > topology_state->avail_slots) {
-		drm_dp_put_port(port);
-		return -ENOSPC;
+	/* Find the current allocation for this port, if any */
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			vcpi = pos;
+			prev_slots = vcpi->vcpi;
+			break;
+		}
 	}
+	if (!vcpi)
+		prev_slots = 0;
+
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/drm_dp_mst_topology.c int drm_dp_atomic_find_vcpi_slots(struct · line 2671
Quoted 6 lines
drivers/gpu/drm/drm_dp_mst_topology.c +2 -2
-	topology_state->avail_slots -= req_slots;
-	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state-
 >avail_slots);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d ->
 %d\n",
+		      port->connector->base.id, port->connector-
name,
Quoted 166 lines
Quoted 142 lines
Quoted 119 lines
Quoted 27 lines
drivers/gpu/drm/drm_dp_mst_topology.c +17 -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 = port;
+		list_add(&vcpi->next, &topology_state->vcpis);
+	}
+	vcpi->vcpi = req_slots;
+
+	ret = req_slots;
+out:
 	drm_dp_put_port(port);
-	return req_slots;
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
@@ -2658,32 +2692,50 @@
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
  * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi
Quoted 8 lines
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
Quoted 70 lines
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).
+ *
+ * It is OK to call this even if @port has been removed from the system,
in
+ * which case it will just amount to a no-op.
+ *
+ * See also:
+ * drm_dp_atomic_find_vcpi_slots()
+ * 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 +2 -1
-				     int slots)
+				     struct drm_dp_mst_port *port)
 {
 	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_dp_vcpi_allocation *tmp, *pos;
 
 	topology_state = drm_atomic_get_mst_topology_state(state,
mgr);
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 +1 -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_safe(pos, tmp, &topology_state->vcpis,
next) {
drivers/gpu/drm/drm_dp_mst_topology.c +10 -0
+		if (pos->port == port) {
+			list_del(&pos->next);
+			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
+				      port, pos->vcpi);
 
+			kfree(pos);
+			return 0;
+		}
+	}
+
+	/* If no allocation was found, all that means is that the port
was
drivers/gpu/drm/drm_dp_mst_topology.c
Quoted 2 lines
drivers/gpu/drm/drm_dp_mst_topology.c +2 -0
+	 * destroyed since the last atomic commit. That's OK!
+	 */
This case here has me a bit worried. I thought with your changes we'll always have the port pointer still around, to be able to clean things up
properly. If this is not the case then we'd potentially leak the vcpi allocation for an unplugged port.
Otoh if the driver releases the allocation for a port twice it's probably
mixed up it's own book-keeping. We should WARN about that case.
Quoted 7 lines
This is kinda fallout from your change to not prune allocations on duplicate, and I think will further clean up how this is handled.
So with the comment removed and replaced by a WARNING(1,"no vcpi allocation found for port %p\n", port) or something like that this should
be solid.
Hm; that comment is leftover from the fallout of moving the port verification
logic out of the state duplicate functions, but I don't think this would nessecarily be a bookkeeping error. If we're going to support fallback link retraining, we'll need to make sure that every driver's drm_connector->atomic_check() function is idempotent so that if the atomic helpers have to pull in more CRTCs to perform modesets on then userspace originally added, the second check on the DRM connector doesn't cause any issues. Remember: if we're trying to retrain an MST topology at a lower link rate then before, that means changing the PBN divisor which in turn means that the only valid actions that can be taken are recalculating every active VCPI allocation at once in order to prevent state inconsistencies, or disabling all of the displays (since going from >0 VCPI to 0 VCPI should always be safe).
Quoted 5 lines
We /could/ just leave this up to the driver to do the bookkeeping on this, but that seems rather redundant. We could add extra bookkeeping in the atomic check though so that instead of just deleting the structs for each VCPI allocation when they go to 0, we instead just set them to 0 then free them somewhere else.
jfyi; I think it would go something like this:
drm_dp_atomic_release_vcpi_slots():
for each vcpi:
if vcpi.port == port:
vcpi.vcpi = 0
return 0;
drm_error("No previous VCPI allocation found! Help!!!\n"); return -EINVAL;
Then we'd need to add something to prune the VCPI allocations in the state,
which the driver would call after all atomic checks/mode validations have been
performed, including drm_dp_mst_atomic_check()
Quoted 8 lines
drm_dp_mst_atomic_check_finish():
for each vcpi:
    if vcpi.vcpi == 0:
      del vcpi
Then, that way we've made sure that multiple VCPI frees in the same check are
OK, but freeing VCPI that never existed actually throws errors
Not sure that's needed at all. The way link training works is we just ask userspace to do the heavy lifting for us, by issuing a new modeset. If that means mode pruning, we also ask userspace to do the heavy lifting for us, again by issuing a new modeset. Kernel never needs to retrain if that means recomputing state stuff or pruning modes or other fun problems. It doesn't even need to retrain if that means shutting down a bunch of links, it can just punt that to userspace and let it retrain at a convenient time.

Quoted 4 lines

Of course that retrain might then result in a modeset, which might fail if the connector is entirely gone by then.
Otoh I might just be talking nonsense right now, it's a bit late :-)

hehe-it's fine. Either way, I'll try to explain it a little better to make this easier to understand for tommorrow.

So: the VCPI allocations are all calculated based on the topology's PBN divisor, which itself is calculated based on the link rate and bandwidth. For a topology running at 5.4 GBit/s x 4 lanes, the pbn_divisor would be 40. For 2.7GBit/s x 4 lanes, it would be 20. The higher the PBN divisor, the more VCPI tokens are needed for each active display, and vice versa.

So: let's say we have a topology like this:

DP-1 @ 5.4 GBit/s w/ pbn_div == 40:
|- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI = 14
|- DP-5: 1920x1080 @ 60Hz bpp=24, VCPI = 14

As a note: because DP-4 and DP-5 share the same DP link since they're on the same topology, I'm assuming that LINK_STATUS_BAD is set at once for all of them.

If DP-4 and DP-5's connection status suddenly went LINK_STATUS_BAD, we'd need to recalculate everything to fit in 2.7 GBit/s with a pbn_div of
20. This is the important part: remember that since everything's sharing the same link, that means they're also sharing the same PBN divisor. Which means that if userspace were to try to fix this by changing one display at a time, say:

DP-1 @ 2.7 GBit/s w/ pbn_div == 20: (Invalid state)
|- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI=28 |- DP-5: 1920x1080 @ 60Hz bpp=24, VCPI=14

This wouldn't work at all: the 14 VCPI slots that DP-5 has now correspond to half the amount of real-world bandwidth then they did previously. This would be broken, and additionally the MST spec mandates that in the event that the all payloads need to be recalculated on link rate changes before programming new payloads.. So, that means we're only left with a few ways to fix this. Change DP-4 and DP-5 at the same time:

DP-1 @ 2.7 GBit/s w/ pbn_div == 20: (Link good, valid state)
|- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI=28
|- DP-5: 1920x1080 @ 60Hz bpp=24, VCPI=28

Or disable each connector. This can be done all at once, or one at a time since we're not programming new payloads:

DP-1 @ 5.4 GBit/s w/ pbn_div == 40: (Link bad, valid state)
|- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI = 14
|- DP-5: <disabled>

DP-1 @ 2.7 GBit/s w/ pbn_div == 20: (Link good, valid state)
|- DP-4: <disabled>
|- DP-5: <disabled>

What this means for atomic userspace is that while it is a bit complicated, we do actually need to add restrictions on what modesets it tries to do on topologies that need fallback retraining to prevent it from setting an invalid state like the one shown above. Part of this means that if userspace tries to do a modeset on one connector on a topology, we have to force a modeset on all of the other connectors on the topology to ensure that they still fit within the VCPI restrictions after having their allocations recalculated. /THIS/ is where having everything that the atomic MST helpers do be idempotent comes in handy as it would solve a situation like this:

(assuming the current setup is identical to the diagrams I listed above)

Atomic state containing:
- mgr w/ pbn_div == 40 (link @ 5.4 GBit/s)
- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI=14
- LINK_STATUS bad -> good (requires modeset to apply new VCPI of 28)

DP-4 encoder.atomic_check() -> OK
DP-4.atomic_check() -> OK

mgr.atomic_check() -> nono wait a second userspace!!
- DP-4 can't be changed without changing DP-5, so:
state.add_connector(DP-5) DP-5.mode_changed = true
need_recheck = true

state.recheck():
# Rerun checks now
DP-4 encoder.atomic_check() -> OK
DP-4.atomic_check() -> OK
DP-5 encoder.atomic_check() -> OK
DP-5.atomic_check() -> OK
mgr.atomic_check() -> OK

As you can see: DP-4 needed to get checked a second time, which means it's VCPI would be freed twice in the same state, but nothing would actually be leaking! That's pretty much the whole usecase for something like this, so that drivers don't have to track where they are in their atomic check() since that's a lot more error prone to have rewritten once for each MST driver.

Quoted 367 lines

Cheers, Daniel
Quoted 363 lines
Quoted 359 lines
Quoted 359 lines
Quoted 72 lines
drivers/gpu/drm/drm_dp_mst_topology.c +0 -0
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
@@ -3112,15 +3164,34 @@ 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 +23 -1
+	INIT_LIST_HEAD(&state->vcpis);
+
+	list_for_each_entry(pos, &old_state->vcpis, next) {
+		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
+		if (!vcpi)
+			goto fail_alloc;
+
+		list_add(&vcpi->next, &state->vcpis);
+	}
+
 	return &state->base;
+
+fail_alloc:
+	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+		kfree(pos);
+	kfree(state);
+
+	return NULL;
 }
 
 static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
@@ -3128,14 +3199,99 @@ 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)
+		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 +7 -0
+{
+	struct drm_dp_vcpi_allocation *vcpi;
+	struct drm_dp_mst_port *port;
+	int avail_slots = 63, ret;
+
+	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
+		/* Ports that no longer exist shouldn't be counted
towards the
drivers/gpu/drm/drm_dp_mst_topology.c
Quoted 2 lines
drivers/gpu/drm/drm_dp_mst_topology.c +2 -0
+		 * bandwidth requirements for the state, so skip them
+		 */
Scenario:
- 2 mst sinks on a single host side port.
- 1st connector is active, 2nd connector is off.
- 1st connector is hot-unplugged, but userspace hasn't realized this yet,
so not yet come around to do the disabling modeset.
- At the same time userspace decides to enable the 2nd output, which forces a modeset and pulls the topology state in.
- This function here decides to only look at the vcpi allocation for the
2nd connector because the first one is gone already.
- Unfortunately lighting up both connectors at the same time isn't possible, because too many vcpi. But since userspace didn't ask for
a
modeset on the now unplugged first connector.
-> Modeset goes boom because we enable too many vcpi.
But with the change in how we release vcpi allocations there's no need to
ignore disconnected stuff here, and we should be able to check vcpi limits
by just checking all of them.
I didn't see anything else serious, so with the above two things resolved
this has my Reviewed-by: Daniel Vetter <[email protected]>
One bikeshed below, because I need to prove I scrolled to the end :-)
Quoted 117 lines
drivers/gpu/drm/drm_dp_mst_topology.c +3 -0
+		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
+		if (!port) {
+			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale,
skipping
checks\n",
drivers/gpu/drm/drm_dp_mst_topology.c +5 -0
+					 vcpi->port);
+			continue;
+		}
+
+		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
@@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644
@@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct
drm_device
*dev,
drivers/gpu/drm/i915/intel_display.c
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
 				       "[modeset]" : "[fastset]");
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
Complete bikeshed, so feel free to ignore: I'd stuff this into intel_modeset_checks(). Feel free to ignore.
Cheers, Daniel
Quoted 126 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);
drivers/gpu/drm/i915/intel_dp_mst.c +9 ~~-14~~
index 8b71d64ebd9d..aaf904738b78 100644
@@ -114,28 +114,31 @@ static int intel_dp_mst_atomic_check(struct
 drm_connector *connector,
 	struct drm_connector_state *old_conn_state;
 	struct drm_crtc *old_crtc;
 	struct drm_crtc_state *crtc_state;
-	int slots, ret = 0;
+	struct intel_connector *intel_connector =
+		to_intel_connector(connector);
+	struct drm_dp_mst_topology_mgr *mgr =
+		&intel_connector->mst_port->mst_mgr;
+	struct drm_dp_mst_port *port = intel_connector->port;
+	int ret = 0;
 
 	old_conn_state = drm_atomic_get_old_connector_state(state,
 connector);
 	old_crtc = old_conn_state->crtc;
 	if (!old_crtc)
 		return ret;
 
-	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
-	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
-	if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
-		struct drm_dp_mst_topology_mgr *mgr;
-		struct drm_encoder *old_encoder;
+	/* Free VCPI, since compute_config() won't be run */
+	if (!new_conn_state->crtc) {
+		crtc_state = drm_atomic_get_new_crtc_state(state,
 old_crtc);
 
-		old_encoder = old_conn_state->best_encoder;
-		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
-
-		ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
 slots);
-		if (ret)
-			DRM_DEBUG_KMS("failed releasing %d vcpi
 slots:%d\n",
 slots, ret);
-		else
-			to_intel_crtc_state(crtc_state)->dp_m_n.tu =
0;
drivers/gpu/drm/i915/intel_dp_mst.c +1 -0
+		ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
port);
drivers/gpu/drm/i915/intel_dp_mst.c +2 -0
+		if (ret) {
+			DRM_DEBUG_KMS("failed releasing vcpi slots:
%d\n",
drivers/gpu/drm/i915/intel_dp_mst.c +5 -0
+				      ret);
+			return ret;
+		}
+		to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
 	}
+
 	return ret;
 }
include/drm/drm_dp_mst_helper.h +7 -1
index 3faceb66f5cb..08b67948b8f9 100644
@@ -406,9 +406,15 @@ struct drm_dp_payload {
 
 #define to_dp_mst_topology_state(x) container_of(x, struct
 drm_dp_mst_topology_state, base)
 
+struct drm_dp_vcpi_allocation {
+	struct drm_dp_mst_port *port;
+	int vcpi;
+	struct list_head next;
+};
+
 struct drm_dp_mst_topology_state {
 	struct drm_private_state base;
-	int avail_slots;
+	struct list_head vcpis;
 	struct drm_dp_mst_topology_mgr *mgr;
 };
@@ -619,14 +625,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);
extern const struct drm_private_state_funcs
drm_dp_mst_topology_state_funcs;
Signature · 1 line
2.19.1
Signature · 2 lines
Cheers,
	Lyude Paul

Signature · 7 lines

Cheers,
	Lyude Paul

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

Lyude Paul

2018-11-07 01:21 UTC #12 lore raw

PATCH v3 3/5 drm/dp_mst: Check payload count in drm_dp_mst_atomic_check()

In reply to a message not in the archive

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

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

Reviewed-by Daniel Vetter <[email protected]>

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

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

index 74823afb262e..8ef0c5f5c7cf 100644
@@ -3213,7 +3213,7 @@ drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,
 {
 	struct drm_dp_vcpi_allocation *vcpi;
 	struct drm_dp_mst_port *port;
-	int avail_slots = 63, ret;
+	int avail_slots = 63, payload_count = 0, ret;
 
 	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
 		/* Ports that no longer exist shouldn't be counted towards the

@@ -3238,6 +3238,13 @@ drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,
 			goto port_fail;
 		}
 
+		if (++payload_count > mgr->max_payloads) {
+			DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p has too many payloads (max=%d)\n",
+					 mgr, mst_state, mgr->max_payloads);
+			ret = -EINVAL;
+			goto port_fail;
+		}
+
 		drm_dp_put_port(port);
 	}
 	DRM_DEBUG_ATOMIC("[MST MGR:%p] mst_state %p vcpi avail=%d used=%d\n",

Signature · 6 lines

2.19.1

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

Lyude Paul

2018-11-07 01:21 UTC #13 lore raw

PATCH v3 4/5 drm/nouveau: Use atomic VCPI helpers for MST

In reply to a message not in the archive

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

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

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

Acked-by Daniel Vetter <[email protected]>

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

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

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

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

@@ -2109,6 +2141,10 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 			return ret;
 	}
 
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		return ret;
+
 	return 0;
 }

Signature · 6 lines

2.19.1

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

Lyude Paul

author maintainer

2018-11-07 01:21 UTC #14 lore raw

PATCH v3 5/5 drm/dp_mst: Stop unsetting mstc->port, check connector registration

In reply to Lyude Paul

Same thing we did in i915, but for nouveau now.

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

Cc Daniel Vetter <[email protected]>

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

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

index 0469ef9e1a54..ff04a56b5949 100644
@@ -698,8 +698,10 @@ nv50_msto_cleanup(struct nv50_msto *msto)
 	struct nv50_mstm *mstm = mstc->mstm;
 
 	NV_ATOMIC(drm, "%s: msto cleanup\n", msto->encoder.name);
-	if (mstc->port && mstc->port->vcpi.vcpi > 0 && !nv50_msto_payload(msto))
+	if (!drm_connector_is_unregistered(&mstc->connector) &&
+	    mstc->port->vcpi.vcpi > 0 && !nv50_msto_payload(msto))
 		drm_dp_mst_deallocate_vcpi(&mstm->mgr, mstc->port);
+
 	if (msto->disabled) {
 		msto->mstc = NULL;
 		msto->head = NULL;

@@ -725,7 +727,8 @@ nv50_msto_prepare(struct nv50_msto *msto)
 	};
 
 	NV_ATOMIC(drm, "%s: msto prepare\n", msto->encoder.name);
-	if (mstc->port && mstc->port->vcpi.vcpi > 0) {
+	if (!drm_connector_is_unregistered(&mstc->connector) &&
+	    mstc->port->vcpi.vcpi > 0) {
 		struct drm_dp_payload *payload = nv50_msto_payload(msto);
 		if (payload) {
 			args.vcpi.start_slot = payload->start_slot;

@@ -828,7 +831,7 @@ nv50_msto_disable(struct drm_encoder *encoder)
 	struct nv50_mstc *mstc = msto->mstc;
 	struct nv50_mstm *mstm = mstc->mstm;
 
-	if (mstc->port)
+	if (!drm_connector_is_unregistered(&mstc->connector))
 		drm_dp_mst_reset_vcpi_slots(&mstm->mgr, mstc->port);
 
 	mstm->outp->update(mstm->outp, msto->head->base.index, NULL, 0, 0);

@@ -967,7 +970,7 @@ nv50_mstc_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status conn_status;
 	int ret;
 
-	if (!mstc->port)
+	if (drm_connector_is_unregistered(&mstc->connector))
 		return connector_status_disconnected;
 
 	ret = pm_runtime_get_sync(connector->dev->dev);

@@ -1105,10 +1108,6 @@ nv50_mstm_destroy_connector(struct drm_dp_mst_topology_mgr *mgr,
 
 	drm_fb_helper_remove_one_connector(&drm->fbcon->helper, &mstc->connector);
 
-	drm_modeset_lock(&drm->dev->mode_config.connection_mutex, NULL);
-	mstc->port = NULL;
-	drm_modeset_unlock(&drm->dev->mode_config.connection_mutex);
-
 	drm_connector_put(&mstc->connector);
 }

Signature · 6 lines

2.19.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patchwork

2018-11-07 14:23 UTC #15 lore raw

✗ Fi.CI.CHECKPATCH: warning for drm/dp_mst: Add some atomic state iterator macros

In reply to Lyude Paul

== Series Details ==

Series: drm/dp_mst: Add some atomic state iterator macros
URL : https://patchwork.freedesktop.org/series/52130/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f9a06ea3b5c0 drm/dp_mst: Add some atomic state iterator macros
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

-:55: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__state' - possible side-effects? #55: FILE: include/drm/drm_dp_mst_helper.h:669:
+#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)))

-:55: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__i' - possible side-effects? #55: FILE: include/drm/drm_dp_mst_helper.h:669:
+#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)))

-:57: WARNING:LONG_LINE: line over 100 characters
#57: FILE: include/drm/drm_dp_mst_helper.h:671:
+ for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), &(old_state), &(new_state), (__i)))

-:72: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__state' - possible side-effects? #72: FILE: include/drm/drm_dp_mst_helper.h:686:
+#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)))

-:72: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__i' - possible side-effects? #72: FILE: include/drm/drm_dp_mst_helper.h:686:
+#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)))

-:74: WARNING:LONG_LINE: line over 100 characters
#74: FILE: include/drm/drm_dp_mst_helper.h:688:
+ for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), &(old_state), NULL, (__i)))

-:90: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__state' - possible side-effects? #90: FILE: include/drm/drm_dp_mst_helper.h:704:
+#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)))

-:90: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__i' - possible side-effects? #90: FILE: include/drm/drm_dp_mst_helper.h:704:
+#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)))

-:92: WARNING:LONG_LINE: line over 100 characters
#92: FILE: include/drm/drm_dp_mst_helper.h:706:
+ for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), NULL, &(new_state), (__i)))

total: 0 errors, 4 warnings, 6 checks, 81 lines checked 2f6f91409c1d drm/dp_mst: Start tracking per-port VCPI allocations 3595cf182bc3 drm/dp_mst: Check payload count in drm_dp_mst_atomic_check() 65ad86529a15 drm/nouveau: Use atomic VCPI helpers for MST b58c9476ea09 drm/dp_mst: Stop unsetting mstc->port, check connector registration

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2018-11-07 14:25 UTC #16 lore raw

✗ Fi.CI.SPARSE: warning for drm/dp_mst: Add some atomic state iterator macros

In reply to Lyude Paul

== Series Details ==

Series: drm/dp_mst: Add some atomic state iterator macros
URL : https://patchwork.freedesktop.org/series/52130/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/dp_mst: Add some atomic state iterator macros
Okay!

Commit: drm/dp_mst: Start tracking per-port VCPI allocations +./include/linux/slab.h:332:43: warning: dubious: x & !y

Commit: drm/dp_mst: Check payload count in drm_dp_mst_atomic_check() Okay!

Commit: drm/nouveau: Use atomic VCPI helpers for MST Okay!

Commit: drm/dp_mst: Stop unsetting mstc->port, check connector registration Okay!

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2018-11-07 14:39 UTC #17 lore raw

✓ Fi.CI.BAT: success for drm/dp_mst: Add some atomic state iterator macros

In reply to Lyude Paul

== Series Details ==

Series: drm/dp_mst: Add some atomic state iterator macros
URL : https://patchwork.freedesktop.org/series/52130/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5097 -> Patchwork_10749 =

== Summary - SUCCESS ==

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/52130/revisions/1/mbox/

== Known issues ==

Here are the changes found in Patchwork_10749 that come from known issues:

=== IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-cfl-8109u:       INCOMPLETE (fdo#106070) -> PASS

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-cfl-8109u:       DMESG-WARN (fdo#107345) -> PASS +2

fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718

== Participating hosts (52 -> 46) ==

Missing (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600

== Build changes ==

* Linux: CI_DRM_5097 -> Patchwork_10749

CI_DRM_5097: c20dfc4f015dfd41246beb7d72338aa50543a5ef @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4711: cc41f4c921e56c62c85ec5349c47022ae9b5f008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10749: b58c9476ea09620366de5a2ba7ecbcd01a5a8962 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

b58c9476ea09 drm/dp_mst: Stop unsetting mstc->port, check connector registration 65ad86529a15 drm/nouveau: Use atomic VCPI helpers for MST 3595cf182bc3 drm/dp_mst: Check payload count in drm_dp_mst_atomic_check() 2f6f91409c1d drm/dp_mst: Start tracking per-port VCPI allocations f9a06ea3b5c0 drm/dp_mst: Add some atomic state iterator macros

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10749/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2018-11-07 22:15 UTC #18 lore raw

✓ Fi.CI.IGT: success for drm/dp_mst: Add some atomic state iterator macros

In reply to Lyude Paul

== Series Details ==

Series: drm/dp_mst: Add some atomic state iterator macros
URL : https://patchwork.freedesktop.org/series/52130/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5097_full -> Patchwork_10749_full =

== Summary - WARNING ==

Minor unknown changes coming with Patchwork_10749_full need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_10749_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

== Possible new issues ==

Here are the unknown changes that may have been introduced in Patchwork_10749_full:

=== IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

== Known issues ==

Here are the changes found in Patchwork_10749_full that come from known issues:

=== IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_store@basic-vebox:
      shard-kbl:          PASS -> DMESG-WARN (fdo#105602, fdo#103558) +9

    igt@gem_workarounds@suspend-resume:
      shard-skl:          PASS -> INCOMPLETE (fdo#107773, fdo#104108)

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956) +1

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
      shard-apl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_color@pipe-b-legacy-gamma:
      shard-apl:          PASS -> FAIL (fdo#104782)

    igt@kms_cursor_crc@cursor-128x128-onscreen:
      shard-apl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-128x42-sliding:
      shard-apl:          NOTRUN -> FAIL (fdo#103232) +1

    igt@kms_flip@dpms-off-confusion-interruptible:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-skl:          PASS -> FAIL (fdo#105363)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-apl:          PASS -> FAIL (fdo#103167) +3

    igt@kms_plane_alpha_blend@pipe-b-alpha-transparant-fb:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@kms_properties@connector-properties-atomic:
      shard-skl:          NOTRUN -> FAIL (fdo#108642)

    igt@pm_rpm@system-suspend-execbuf:
      shard-skl:          PASS -> INCOMPLETE (fdo#107773, fdo#104108, fdo#107807)

    igt@prime_busy@basic-wait-before-default:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    
    ==== Possible fixes ====

    igt@gem_ctx_isolation@vecs0-s3:
      shard-skl:          INCOMPLETE (fdo#107773, fdo#104108) -> PASS

    igt@gem_exec_reuse@baggage:
      shard-apl:          DMESG-WARN -> PASS

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-kbl:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_color@pipe-a-ctm-max:
      shard-apl:          FAIL (fdo#108147) -> PASS

    igt@kms_cursor_crc@cursor-256x256-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +5

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-skl:          FAIL (fdo#105363) -> PASS

    igt@kms_flip_tiling@flip-to-yf-tiled:
      shard-skl:          FAIL (fdo#107931) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-render:
      shard-skl:          FAIL (fdo#105682) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          FAIL (fdo#103167) -> PASS +3

    igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-pwrite:
      shard-skl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS +1

    igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
      shard-skl:          FAIL (fdo#107815) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
      shard-glk:          FAIL (fdo#103166) -> PASS

    igt@kms_vblank@pipe-a-accuracy-idle:
      shard-skl:          FAIL (fdo#102583) -> PASS

    igt@pm_rpm@dpms-lpsp:
      shard-skl:          INCOMPLETE (fdo#107807) -> PASS

== Participating hosts (6 -> 6) ==

No changes in participating hosts

== Build changes ==

* Linux: CI_DRM_5097 -> Patchwork_10749

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10749/shards.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx