[PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

Atm, drm_dp_remove_payload() uses the same payload state to both get the
vc_start_slot required for the payload removal DPCD message and to
deduct time_slots from vc_start_slot of all payloads after the one being
removed.

The above isn't always correct, as vc_start_slot must be the up-to-date
version contained in the new payload state, but time_slots must be the
one used when the payload was previously added, contained in the old
payload state. The new payload's time_slots can change vs. the old one
if the current atomic commit changes the corresponding mode.

This patch let's drivers pass the old and new payload states to
drm_dp_remove_payload(), but keeps these the same for now in all drivers
not to change the behavior. A follow-up i915 patch will pass in that
driver the correct old and new states to the function.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Karol Herbst <kherbst@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
 include/drm/display/drm_dp_mst_helper.h       |  3 ++-
 5 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index a50319fc42b11..180d3893b68da 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
 	if (enable)
 		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
 	else
-		drm_dp_remove_payload(mst_mgr, mst_state, payload);
+		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
 
 	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
 	 * AUX message. The sequence is slot 1-63 allocated sequence for each
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 847c10aa2098c..1990ff5dc7ddd 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
  * drm_dp_remove_payload() - Remove an MST payload
  * @mgr: Manager to use.
  * @mst_state: The MST atomic state
- * @payload: The payload to write
+ * @old_payload: The payload with its old state
+ * @new_payload: The payload to write
  *
  * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
  * the starting time slots of all other payloads which would have been shifted towards the start of
@@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
  */
 void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   struct drm_dp_mst_topology_state *mst_state,
-			   struct drm_dp_mst_atomic_payload *payload)
+			   const struct drm_dp_mst_atomic_payload *old_payload,
+			   struct drm_dp_mst_atomic_payload *new_payload)
 {
 	struct drm_dp_mst_atomic_payload *pos;
 	bool send_remove = false;
 
 	/* We failed to make the payload, so nothing to do */
-	if (payload->vc_start_slot == -1)
+	if (new_payload->vc_start_slot == -1)
 		return;
 
 	mutex_lock(&mgr->lock);
-	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
+	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
 	mutex_unlock(&mgr->lock);
 
 	if (send_remove)
-		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
+		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
 	else
 		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
-			    payload->vcpi);
+			    new_payload->vcpi);
 
 	list_for_each_entry(pos, &mst_state->payloads, next) {
-		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
-			pos->vc_start_slot -= payload->time_slots;
+		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
+			pos->vc_start_slot -= old_payload->time_slots;
 	}
-	payload->vc_start_slot = -1;
+	new_payload->vc_start_slot = -1;
 
 	mgr->payload_count--;
-	mgr->next_start_slot -= payload->time_slots;
+	mgr->next_start_slot -= old_payload->time_slots;
 
-	if (payload->delete)
-		drm_dp_mst_put_port_malloc(payload->port);
+	if (new_payload->delete)
+		drm_dp_mst_put_port_malloc(new_payload->port);
 }
 EXPORT_SYMBOL(drm_dp_remove_payload);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
 		to_intel_connector(old_conn_state->connector);
 	struct drm_dp_mst_topology_state *mst_state =
 		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
+	struct drm_dp_mst_atomic_payload *payload =
+		drm_atomic_get_mst_payload_state(mst_state, connector->port);
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 
 	drm_dbg_kms(&i915->drm, "active links %d\n",
@@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
 	intel_hdcp_disable(intel_mst->connector);
 
 	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
-			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
+			      payload, payload);
 
 	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
 }
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index edcb2529b4025..ed9d374147b8d 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
 
 	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
 	if (msto->disabled) {
-		drm_dp_remove_payload(mgr, mst_state, payload);
+		drm_dp_remove_payload(mgr, mst_state, payload, payload);
 
 		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
 	} else {
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 41fd8352ab656..f5eb9aa152b14 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
 			     struct drm_dp_mst_atomic_payload *payload);
 void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   struct drm_dp_mst_topology_state *mst_state,
-			   struct drm_dp_mst_atomic_payload *payload);
+			   const struct drm_dp_mst_atomic_payload *old_payload,
+			   struct drm_dp_mst_atomic_payload *new_payload);
 
 int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
-- 
2.37.1

[PATCH v2 03/17] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Imre Deak]

Add a function to get the old MST topology state, required by a
follow-up i915 patch.

While at it clarify the code comment of
drm_atomic_get_new_mst_topology_state() and add _new prefix
to the new state pointer to remind about its difference from the old
state.

v2: Use old_/new_ prefixes for the state pointers. (Ville)

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 33 ++++++++++++++++---
 include/drm/display/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 1990ff5dc7ddd..38dab76ae69ea 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5364,28 +5364,53 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
 }
 EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
 
+/**
+ * drm_atomic_get_old_mst_topology_state: get old MST topology state in atomic state, if any
+ * @state: global atomic state
+ * @mgr: MST topology manager, also the private object in this case
+ *
+ * This function wraps drm_atomic_get_old_private_obj_state() passing in the MST atomic
+ * state vtable so that the private object state returned is that of a MST
+ * topology object.
+ *
+ * Returns:
+ *
+ * The old MST topology state, or NULL if there's no topology state for this MST mgr
+ * in the global atomic state
+ */
+struct drm_dp_mst_topology_state *
+drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
+				      struct drm_dp_mst_topology_mgr *mgr)
+{
+	struct drm_private_state *old_priv_state =
+		drm_atomic_get_old_private_obj_state(state, &mgr->base);
+
+	return old_priv_state ? to_dp_mst_topology_state(old_priv_state) : NULL;
+}
+EXPORT_SYMBOL(drm_atomic_get_old_mst_topology_state);
+
 /**
  * drm_atomic_get_new_mst_topology_state: get new MST topology state in atomic state, if any
  * @state: global atomic state
  * @mgr: MST topology manager, also the private object in this case
  *
- * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
+ * This function wraps drm_atomic_get_new_private_obj_state() passing in the MST atomic
  * state vtable so that the private object state returned is that of a MST
  * topology object.
  *
  * Returns:
  *
- * The MST topology state, or NULL if there's no topology state for this MST mgr
+ * The new MST topology state, or NULL if there's no topology state for this MST mgr
  * in the global atomic state
  */
 struct drm_dp_mst_topology_state *
 drm_atomic_get_new_mst_topology_state(struct drm_atomic_state *state,
 				      struct drm_dp_mst_topology_mgr *mgr)
 {
-	struct drm_private_state *priv_state =
+	struct drm_private_state *new_priv_state =
 		drm_atomic_get_new_private_obj_state(state, &mgr->base);
 
-	return priv_state ? to_dp_mst_topology_state(priv_state) : NULL;
+	return new_priv_state ? to_dp_mst_topology_state(new_priv_state) : NULL;
 }
 EXPORT_SYMBOL(drm_atomic_get_new_mst_topology_state);
 
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index f5eb9aa152b14..32c764fb9cb56 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -868,6 +868,9 @@ struct drm_dp_mst_topology_state *
 drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 				  struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *
+drm_atomic_get_old_mst_topology_state(struct drm_atomic_state *state,
+				      struct drm_dp_mst_topology_mgr *mgr);
+struct drm_dp_mst_topology_state *
 drm_atomic_get_new_mst_topology_state(struct drm_atomic_state *state,
 				      struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_atomic_payload *
-- 
2.37.1

[PATCH v2 05/17] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Imre Deak]

Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 38dab76ae69ea..8787df19f428b 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4772,7 +4772,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 		list_for_each_entry(payload, &state->payloads, next) {
 			char name[14];
 
-			if (payload->vcpi != i || payload->delete)
+			if (payload->vcpi != i + 1 || payload->delete)
 				continue;
 
 			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
-- 
2.37.1

[PATCH v2 09/17] drm/display/dp_mst: Add a helper to verify the MST payload state

[Imre Deak]

Add a function drivers can use to verify the MST payload state tracking
and compare this to the sink's payload table.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 169 ++++++++++++++++++
 include/drm/display/drm_dp.h                  |   3 +
 include/drm/display/drm_dp_mst_helper.h       |   3 +
 3 files changed, 175 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 0c04b96ae614c..e57dd16955d52 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4847,6 +4847,175 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 }
 EXPORT_SYMBOL(drm_dp_mst_dump_topology);
 
+static bool verify_mst_payload_state(struct drm_dp_mst_topology_state *mst_state)
+{
+	struct drm_dp_mst_topology_mgr *mgr = mst_state->mgr;
+	struct drm_dp_mst_atomic_payload *payload;
+	int payload_count = 0;
+	u64 time_slot_mask = 0;
+	u32 vcpi_mask = 0;
+	int last_set;
+
+	if (BITS_PER_TYPE(time_slot_mask) < mst_state->total_avail_slots)
+		return false;
+
+	list_for_each_entry(payload, &mst_state->payloads, next) {
+		u64 mask;
+
+		if (payload->vc_start_slot == -1)
+			continue;
+
+		if (!payload->time_slots)
+			return false;
+
+		if (payload->vc_start_slot < mst_state->start_slot)
+			return false;
+
+		if (payload->vc_start_slot + payload->time_slots - mst_state->start_slot >
+		    mst_state->total_avail_slots)
+			return false;
+
+		mask = GENMASK_ULL(payload->vc_start_slot + payload->time_slots - 1,
+				   payload->vc_start_slot);
+
+		if (time_slot_mask & mask)
+			return false;
+
+		time_slot_mask |= mask;
+
+		if (payload->vcpi < 1 ||
+		    payload->vcpi & ~DP_PAYLOAD_ID_MASK ||
+		    payload->vcpi > BITS_PER_TYPE(vcpi_mask))
+			return false;
+		if (BIT(payload->vcpi - 1) & vcpi_mask)
+			return false;
+		vcpi_mask |= BIT(payload->vcpi - 1);
+
+		payload_count++;
+	}
+
+	if (payload_count != mgr->payload_count)
+		return false;
+
+	last_set = fls64(time_slot_mask);
+
+	if (last_set &&
+	    GENMASK_ULL(last_set - 1, mst_state->start_slot) != time_slot_mask)
+		return false;
+
+	if (max(mst_state->start_slot, mgr->next_start_slot) !=
+	    max_t(int, mst_state->start_slot, last_set))
+		return false;
+
+	return true;
+}
+
+static int get_payload_table_vcpi(const u8 *table, int slot)
+{
+	if (slot == 0)
+		return FIELD_GET(DP_PAYLOAD_ID_SLOT0_5_0_MASK, table[0]) |
+		       (FIELD_GET(DP_PAYLOAD_ID_SLOT0_6, table[1]) << 6);
+	else
+		return FIELD_GET(DP_PAYLOAD_ID_MASK, table[slot]);
+}
+
+static bool verify_mst_payload_table(struct drm_dp_mst_topology_state *mst_state,
+				     const u8 *payload_table)
+{
+	struct drm_dp_mst_topology_mgr *mgr = mst_state->mgr;
+	struct drm_dp_mst_atomic_payload *payload;
+	int i;
+
+	list_for_each_entry(payload, &mst_state->payloads, next) {
+		if (payload->vc_start_slot == -1)
+			continue;
+
+		if (payload->vc_start_slot + payload->time_slots > DP_PAYLOAD_TABLE_SIZE)
+			return false;
+
+		for (i = 0; i < payload->time_slots; i++)
+			if (get_payload_table_vcpi(payload_table,
+						   payload->vc_start_slot + i) != payload->vcpi)
+				return false;
+	}
+
+	for (i = max(mgr->next_start_slot, mst_state->start_slot);
+	     i < DP_PAYLOAD_TABLE_SIZE;
+	     i++) {
+		if (get_payload_table_vcpi(payload_table, i) != 0)
+			return false;
+	}
+
+	return true;
+}
+
+static void print_mst_payload_state(struct drm_dp_mst_topology_mgr *mgr,
+				    struct drm_dp_mst_topology_state *mst_state,
+				    const u8 *payload_table)
+{
+	struct drm_dp_mst_atomic_payload *payload;
+	int i = 0;
+
+	drm_dbg(mgr->dev,
+		"Payload state: start_slot %d total_avail_slots %d next_start_slot %d payload_count %d\n",
+		mst_state->start_slot, mst_state->total_avail_slots,
+		mgr->next_start_slot, mgr->payload_count);
+
+	list_for_each_entry(payload, &mst_state->payloads, next) {
+		drm_dbg(mgr->dev,
+			"  Payload#%d: port %p VCPI %d delete %d vc_start_slot %d time_slots %d\n",
+			i, payload->port, payload->vcpi,
+			payload->delete, payload->vc_start_slot, payload->time_slots);
+		i++;
+	}
+
+	if (!payload_table)
+		return;
+
+	drm_dbg(mgr->dev, "Payload table:\n");
+	print_hex_dump(KERN_DEBUG, "  Ptbl ",
+		       DUMP_PREFIX_OFFSET, 16, 1,
+		       payload_table, DP_PAYLOAD_TABLE_SIZE, false);
+}
+
+/**
+ * drm_dp_mst_verify_payload_state - Verify the atomic state for payloads and the related sink payload table
+ * @state: atomic state
+ * @mgr: manager to verify the state for
+ * @verify_sink: %true if the sink payload table needs to be verified as well
+ *
+ * Verify @mgr's atomic state tracking all its payloads and optionally the
+ * related sink payload table.
+ */
+void drm_dp_mst_verify_payload_state(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     bool verify_sink)
+{
+	struct drm_dp_mst_topology_state *mst_state;
+	u8 payload_table[DP_PAYLOAD_TABLE_SIZE];
+
+	mst_state = drm_atomic_get_new_mst_topology_state(state, mgr);
+	if (drm_WARN_ON(mgr->dev, !mst_state))
+		return;
+
+	if (drm_WARN_ON(mgr->dev, !verify_mst_payload_state(mst_state))) {
+		print_mst_payload_state(mgr, mst_state, NULL);
+		return;
+	}
+
+	if (!verify_sink)
+		return;
+
+	if (!dump_dp_payload_table(mgr, payload_table))
+		return;
+
+	if (!verify_mst_payload_table(mst_state, payload_table)) {
+		drm_err(mgr->dev, "MST payload state mismatches payload table\n");
+		print_mst_payload_state(mgr, mst_state, payload_table);
+	}
+}
+EXPORT_SYMBOL(drm_dp_mst_verify_payload_state);
+
 static void drm_dp_tx_work(struct work_struct *work)
 {
 	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, tx_work);
diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 632376c291db6..bcc5183188a68 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -925,9 +925,12 @@
 #define DP_PAYLOAD_TABLE_UPDATE_STATUS      0x2c0   /* 1.2 MST */
 # define DP_PAYLOAD_TABLE_UPDATED           (1 << 0)
 # define DP_PAYLOAD_ACT_HANDLED             (1 << 1)
+# define DP_PAYLOAD_ID_SLOT0_5_0_MASK	    (0x3f << 2)
 
 #define DP_VC_PAYLOAD_ID_SLOT_1             0x2c1   /* 1.2 MST */
 /* up to ID_SLOT_63 at 0x2ff */
+# define DP_PAYLOAD_ID_SLOT0_6		    (1 << 7)
+# define DP_PAYLOAD_ID_MASK		    0x7f
 
 /* Source Device-specific */
 #define DP_SOURCE_OUI			    0x300
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 32c764fb9cb56..44c6710ebf315 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -848,6 +848,9 @@ int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
 void drm_dp_mst_dump_topology(struct seq_file *m,
 			      struct drm_dp_mst_topology_mgr *mgr);
+void drm_dp_mst_verify_payload_state(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     bool verify_sink);
 
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int __must_check
-- 
2.37.1

[PATCH v2 11/17] drm/display/dp_mst: Add helpers to query for payload allocation errors

[Imre Deak]

Add a way for drivers to query if allocating time slots for any payloads
in a given MST topology failed. This is needed by a follow-up i915 patch
verifying the SW vs. HW state of the MST topology.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 35 ++++++++++++++++---
 include/drm/display/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index e57dd16955d52..f2081f3fad0da 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3306,15 +3306,14 @@ int drm_dp_add_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 			     struct drm_dp_mst_atomic_payload *payload)
 {
 	struct drm_dp_mst_port *port;
-	int ret;
+	int ret = 0;
 
 	port = drm_dp_mst_topology_get_port_validated(mgr, payload->port);
 	if (!port) {
 		drm_dbg_kms(mgr->dev,
 			    "VCPI %d for port %p not in topology, not creating a payload\n",
 			    payload->vcpi, payload->port);
-		payload->vc_start_slot = -1;
-		return 0;
+		goto alloc_fail;
 	}
 
 	if (mgr->payload_count == 0)
@@ -3327,14 +3326,21 @@ int drm_dp_add_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 	if (ret < 0) {
 		drm_warn(mgr->dev, "Failed to create MST payload for port %p: %d\n",
 			 payload->port, ret);
-		payload->vc_start_slot = -1;
-		return ret;
+		goto alloc_fail;
 	}
 
+	payload->alloc_failed = false;
+
 	mgr->payload_count++;
 	mgr->next_start_slot += payload->time_slots;
 
 	return 0;
+
+alloc_fail:
+	payload->vc_start_slot = -1;
+	payload->alloc_failed = true;
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_add_payload_part1);
 
@@ -3423,6 +3429,25 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
 }
 EXPORT_SYMBOL(drm_dp_add_payload_part2);
 
+/**
+ * drm_dp_mst_has_payload_alloc_errors - Query for payload allocation errors
+ * @mst_state: The MST atomic state
+ *
+ * Returns %true if the allocation of any of the payloads in @mst_state
+ * failed, %false otherwise.
+ */
+bool drm_dp_mst_has_payload_alloc_errors(const struct drm_dp_mst_topology_state *mst_state)
+{
+	struct drm_dp_mst_atomic_payload *pos;
+
+	list_for_each_entry(pos, &mst_state->payloads, next)
+		if (pos->alloc_failed)
+			return true;
+
+	return false;
+}
+EXPORT_SYMBOL(drm_dp_mst_has_payload_alloc_errors);
+
 static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_port *port,
 				 int offset, int size, u8 *bytes)
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 44c6710ebf315..53b251b264e89 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -568,6 +568,8 @@ struct drm_dp_mst_atomic_payload {
 	bool delete : 1;
 	/** @dsc_enabled: Whether or not this payload has DSC enabled */
 	bool dsc_enabled : 1;
+	/** @alloc_failed: Whether or not allocating this payload failed */
+	bool alloc_failed : 1;
 
 	/** @next: The list node for this payload */
 	struct list_head next;
@@ -843,6 +845,7 @@ void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   struct drm_dp_mst_topology_state *mst_state,
 			   const struct drm_dp_mst_atomic_payload *old_payload,
 			   struct drm_dp_mst_atomic_payload *new_payload);
+bool drm_dp_mst_has_payload_alloc_errors(const struct drm_dp_mst_topology_state *mst_state);
 
 int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
-- 
2.37.1

[PATCH v2 12/17] drm/display/dp_mst: Add helpers to query payload allocation properties

[Imre Deak]

Add helper functions to query the virtual channel and time slots for a
payload and the current payload count and total allocated time slots in
an MST topology. These are needed by a follow-up i915 patch verifying
the SW vs. HW state of the MST topology.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +++++++++++++++++++
 include/drm/display/drm_dp_mst_helper.h       | 21 +++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index f2081f3fad0da..47605f67578ad 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3448,6 +3448,42 @@ bool drm_dp_mst_has_payload_alloc_errors(const struct drm_dp_mst_topology_state
 }
 EXPORT_SYMBOL(drm_dp_mst_has_payload_alloc_errors);
 
+/**
+ * drm_dp_mst_payload_vchannel - Return the DP virtual channel for a payload
+ * @mst_state: The MST atomic state containing @payload
+ * @payload: The payload to get the virtual channel for
+ *
+ * Return the DP virtual channel for @payload. The virtual channel is a
+ * contiguous range of MST Transmission Units on the DP main lanes between
+ * the source DPTX and the first downstream MST hub DPRX. Accordingly the
+ * channel is determined by the payload's position on the payload list
+ * ordered by VC start slot.
+ *
+ * Returns the 0-based virtual channel of @payload if it's in @mst_state with
+ * its time slots being allocated, or -1 otherwise.
+ */
+int drm_dp_mst_payload_vchannel(const struct drm_dp_mst_topology_state *mst_state,
+				const struct drm_dp_mst_atomic_payload *payload)
+{
+	struct drm_dp_mst_atomic_payload *pos;
+	int vc = 0;
+	bool found = false;
+
+	list_for_each_entry(pos, &mst_state->payloads, next) {
+		if (pos->vc_start_slot == -1)
+			continue;
+
+		if (pos == payload)
+			found = true;
+
+		if (pos->vc_start_slot < payload->vc_start_slot)
+			vc++;
+	}
+
+	return found ? vc : -1;
+}
+EXPORT_SYMBOL(drm_dp_mst_payload_vchannel);
+
 static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_port *port,
 				 int offset, int size, u8 *bytes)
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 53b251b264e89..bb7c595096fed 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -846,6 +846,27 @@ void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   const struct drm_dp_mst_atomic_payload *old_payload,
 			   struct drm_dp_mst_atomic_payload *new_payload);
 bool drm_dp_mst_has_payload_alloc_errors(const struct drm_dp_mst_topology_state *mst_state);
+int drm_dp_mst_payload_vchannel(const struct drm_dp_mst_topology_state *mst_state,
+				const struct drm_dp_mst_atomic_payload *payload);
+
+static inline int
+drm_dp_mst_payload_count(const struct drm_dp_mst_topology_state *mst_state)
+{
+	return mst_state->mgr->payload_count;
+}
+
+static inline int
+drm_dp_mst_allocated_time_slots(const struct drm_dp_mst_topology_state *mst_state)
+{
+	return drm_dp_mst_payload_count(mst_state) ?
+		mst_state->mgr->next_start_slot - mst_state->start_slot : 0;
+}
+
+static inline int
+drm_dp_mst_payload_time_slots(const struct drm_dp_mst_atomic_payload *payload)
+{
+	return payload->time_slots;
+}
 
 int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
 
-- 
2.37.1

[PATCH v2 13/17] drm/display/dp_mst: Export the DP_PAYLOAD_TABLE_SIZE definition

[Imre Deak]

In a follow-up workaround for a payload allocation problem the driver
uses DP_PAYLOAD_TABLE_SIZE to determine when the workaround is needed,
so export the definition.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 --
 include/drm/display/drm_dp.h                  | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 47605f67578ad..d0e929df7d088 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4771,8 +4771,6 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
 	}
 }
 
-#define DP_PAYLOAD_TABLE_SIZE		64
-
 static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
 				  char *buf)
 {
diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index bcc5183188a68..8f18f66ff5bc3 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -932,6 +932,8 @@
 # define DP_PAYLOAD_ID_SLOT0_6		    (1 << 7)
 # define DP_PAYLOAD_ID_MASK		    0x7f
 
+#define DP_PAYLOAD_TABLE_SIZE		    64
+
 /* Source Device-specific */
 #define DP_SOURCE_OUI			    0x300
 
-- 
2.37.1

[PATCH v2 14/17] drm/display/dp_mst: Factor out a helper to reset the payload table

[Imre Deak]

A follow-up patch adds a workaround for a payload allocation problem in
a DELL monitor. For this the driver needs to reset the payload table in
the monitor's MST hub, factor out a helper to do this.

While at it use DP_PAYLOAD_TABLE_SIZE in the reset command, instead of
open coding it.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 8 +++++++-
 include/drm/display/drm_dp_mst_helper.h       | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index d0e929df7d088..eb170389cf9bc 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3429,6 +3429,12 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
 }
 EXPORT_SYMBOL(drm_dp_add_payload_part2);
 
+int drm_dp_mst_reset_payload_table(struct drm_dp_mst_topology_mgr *mgr)
+{
+	return drm_dp_dpcd_write_payload(mgr, 0, 0, DP_PAYLOAD_TABLE_SIZE - 1);
+}
+EXPORT_SYMBOL(drm_dp_mst_reset_payload_table);
+
 /**
  * drm_dp_mst_has_payload_alloc_errors - Query for payload allocation errors
  * @mst_state: The MST atomic state
@@ -3699,7 +3705,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 			goto out_unlock;
 
 		/* Write reset payload */
-		drm_dp_dpcd_write_payload(mgr, 0, 0, 0x3f);
+		drm_dp_mst_reset_payload_table(mgr);
 
 		queue_work(system_long_wq, &mgr->work);
 
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index bb7c595096fed..2d15399df2950 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -845,6 +845,7 @@ void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
 			   struct drm_dp_mst_topology_state *mst_state,
 			   const struct drm_dp_mst_atomic_payload *old_payload,
 			   struct drm_dp_mst_atomic_payload *new_payload);
+int drm_dp_mst_reset_payload_table(struct drm_dp_mst_topology_mgr *mgr);
 bool drm_dp_mst_has_payload_alloc_errors(const struct drm_dp_mst_topology_state *mst_state);
 int drm_dp_mst_payload_vchannel(const struct drm_dp_mst_topology_state *mst_state,
 				const struct drm_dp_mst_atomic_payload *payload);
-- 
2.37.1

[PATCH v2 15/17] drm/dp: Add a quirk for a DELL P2715Q MST payload allocation problem

[Imre Deak]

The DELL P2715Q monitor's MST hub has a payload allocation problem,
where the VCPI used to reserve the last two time slots (at position
0x3e, 0x3f) in the hub's payload table, this VCPI can't be reused for
later payload configurations. To work around the problem in a follow-up
patch the driver needs to reset the payload table after all payloads
have been freed; add a quirk the driver can use to detect the monitor
and apply the WA if needed.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 2 ++
 include/drm/display/drm_dp_helper.h     | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 16565a0a5da6d..e2cf4b4fe11ea 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2244,6 +2244,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
 	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) },
 	/* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */
 	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) },
+	/* DELL P2715Q MST payload table must be reset after the two last payload slots get allocated. */
+	{ OUI(0x00, 0xe0, 0x4c), DEVICE_ID('D', 'p', '1', '.', '2', 0), true, BIT(DP_DPCD_QUIRK_MST_PAYLOAD_TABLE_RESET_WA) },
 };
 
 #undef OUI
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index ab55453f2d2cd..fcd445921ec9d 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -620,6 +620,14 @@ enum drm_dp_quirk {
 	 * the DP_MAX_LINK_RATE register reporting a lower max multiplier.
 	 */
 	DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS,
+	/**
+	 * @DP_DPCD_QUIRK_MST_PAYLOAD_TABLE_RESET_WA:
+	 *
+	 * An bug in the MST branch device's payload allocation logic requires
+	 * the payload table to be reset after its last two payload slots get
+	 * allocated.
+	 */
+	DP_DPCD_QUIRK_MST_PAYLOAD_TABLE_RESET_WA,
 };
 
 /**
-- 
2.37.1

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Lyude Paul]

On Tue, 2023-01-31 at 17:05 +0200, Imre Deak wrote:
> Atm, drm_dp_remove_payload() uses the same payload state to both get the
> vc_start_slot required for the payload removal DPCD message and to
> deduct time_slots from vc_start_slot of all payloads after the one being
> removed.
> 
> The above isn't always correct, as vc_start_slot must be the up-to-date
> version contained in the new payload state, but time_slots must be the
> one used when the payload was previously added, contained in the old
> payload state. The new payload's time_slots can change vs. the old one
> if the current atomic commit changes the corresponding mode.
> 
> This patch let's drivers pass the old and new payload states to
> drm_dp_remove_payload(), but keeps these the same for now in all drivers
> not to change the behavior. A follow-up i915 patch will pass in that
> driver the correct old and new states to the function.

Oh wow, this was definitely a mistake on my part, thanks for catching this!
TBH, I think this behavior is correct so (now that I actually have a setup
capable of testing amdgpu's MST fully thanks to gitlab issue 2171…) if you'd
like to change it on other drivers as well I can test it fully. Or feel free
to leave it to me, shouldn't be too difficult I think :).

For 0-2:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Karol Herbst <kherbst@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org # 6.1
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
>  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
>  5 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index a50319fc42b11..180d3893b68da 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>  	if (enable)
>  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
>  	else
> -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
>  
>  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
>  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 847c10aa2098c..1990ff5dc7ddd 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
>   * drm_dp_remove_payload() - Remove an MST payload
>   * @mgr: Manager to use.
>   * @mst_state: The MST atomic state
> - * @payload: The payload to write
> + * @old_payload: The payload with its old state
> + * @new_payload: The payload to write
>   *
>   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
>   * the starting time slots of all other payloads which would have been shifted towards the start of
> @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
>   */
>  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
>  			   struct drm_dp_mst_topology_state *mst_state,
> -			   struct drm_dp_mst_atomic_payload *payload)
> +			   const struct drm_dp_mst_atomic_payload *old_payload,
> +			   struct drm_dp_mst_atomic_payload *new_payload)
>  {
>  	struct drm_dp_mst_atomic_payload *pos;
>  	bool send_remove = false;
>  
>  	/* We failed to make the payload, so nothing to do */
> -	if (payload->vc_start_slot == -1)
> +	if (new_payload->vc_start_slot == -1)
>  		return;
>  
>  	mutex_lock(&mgr->lock);
> -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
>  	mutex_unlock(&mgr->lock);
>  
>  	if (send_remove)
> -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
>  	else
>  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> -			    payload->vcpi);
> +			    new_payload->vcpi);
>  
>  	list_for_each_entry(pos, &mst_state->payloads, next) {
> -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> -			pos->vc_start_slot -= payload->time_slots;
> +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> +			pos->vc_start_slot -= old_payload->time_slots;
>  	}
> -	payload->vc_start_slot = -1;
> +	new_payload->vc_start_slot = -1;
>  
>  	mgr->payload_count--;
> -	mgr->next_start_slot -= payload->time_slots;
> +	mgr->next_start_slot -= old_payload->time_slots;
>  
> -	if (payload->delete)
> -		drm_dp_mst_put_port_malloc(payload->port);
> +	if (new_payload->delete)
> +		drm_dp_mst_put_port_malloc(new_payload->port);
>  }
>  EXPORT_SYMBOL(drm_dp_remove_payload);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
>  		to_intel_connector(old_conn_state->connector);
>  	struct drm_dp_mst_topology_state *mst_state =
>  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> +	struct drm_dp_mst_atomic_payload *payload =
> +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  
>  	drm_dbg_kms(&i915->drm, "active links %d\n",
> @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
>  	intel_hdcp_disable(intel_mst->connector);
>  
>  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> +			      payload, payload);
>  
>  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
>  }
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b4025..ed9d374147b8d 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
>  
>  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
>  	if (msto->disabled) {
> -		drm_dp_remove_payload(mgr, mst_state, payload);
> +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
>  
>  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
>  	} else {
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 41fd8352ab656..f5eb9aa152b14 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
>  			     struct drm_dp_mst_atomic_payload *payload);
>  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
>  			   struct drm_dp_mst_topology_state *mst_state,
> -			   struct drm_dp_mst_atomic_payload *payload);
> +			   const struct drm_dp_mst_atomic_payload *old_payload,
> +			   struct drm_dp_mst_atomic_payload *new_payload);
>  
>  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

On Tue, Jan 31, 2023 at 06:13:10PM -0500, Lyude Paul wrote:
> On Tue, 2023-01-31 at 17:05 +0200, Imre Deak wrote:
> > Atm, drm_dp_remove_payload() uses the same payload state to both get the
> > vc_start_slot required for the payload removal DPCD message and to
> > deduct time_slots from vc_start_slot of all payloads after the one being
> > removed.
> > 
> > The above isn't always correct, as vc_start_slot must be the up-to-date
> > version contained in the new payload state, but time_slots must be the
> > one used when the payload was previously added, contained in the old
> > payload state. The new payload's time_slots can change vs. the old one
> > if the current atomic commit changes the corresponding mode.
> > 
> > This patch let's drivers pass the old and new payload states to
> > drm_dp_remove_payload(), but keeps these the same for now in all drivers
> > not to change the behavior. A follow-up i915 patch will pass in that
> > driver the correct old and new states to the function.
> 
> Oh wow, this was definitely a mistake on my part, thanks for catching this!
> TBH, I think this behavior is correct so (now that I actually have a setup
> capable of testing amdgpu's MST fully thanks to gitlab issue 2171…) if you'd
> like to change it on other drivers as well I can test it fully.

I only checked that the other drivers pass the new payload state to
drm_dp_remove_payload(), so not sure how that works atm if the same
commit has to both remove the payload (with the old time_slots value)
and add it back (with a new time_slots value). Maybe that can't happen
in those drivers, or time_slots get updated between remove and readd.

> Or feel free to leave it to me, shouldn't be too difficult I think :).

Yes, this patch should have no functional change, so please check what
would apply to other drivers as well.

Could you also check Ville's comment about storing start_slot elsewhere
than the atomic state (leaving only time_slots there). I wonder if that
would work, at least it would simplify things I think.

> For 0-2:
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks.

> 
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Cc: Karol Herbst <kherbst@redhat.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Cc: dri-devel@lists.freedesktop.org
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> >  5 files changed, 21 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > index a50319fc42b11..180d3893b68da 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> >  	if (enable)
> >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> >  	else
> > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
> >  
> >  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
> >  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 847c10aa2098c..1990ff5dc7ddd 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> >   * drm_dp_remove_payload() - Remove an MST payload
> >   * @mgr: Manager to use.
> >   * @mst_state: The MST atomic state
> > - * @payload: The payload to write
> > + * @old_payload: The payload with its old state
> > + * @new_payload: The payload to write
> >   *
> >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> >   */
> >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> >  			   struct drm_dp_mst_topology_state *mst_state,
> > -			   struct drm_dp_mst_atomic_payload *payload)
> > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > +			   struct drm_dp_mst_atomic_payload *new_payload)
> >  {
> >  	struct drm_dp_mst_atomic_payload *pos;
> >  	bool send_remove = false;
> >  
> >  	/* We failed to make the payload, so nothing to do */
> > -	if (payload->vc_start_slot == -1)
> > +	if (new_payload->vc_start_slot == -1)
> >  		return;
> >  
> >  	mutex_lock(&mgr->lock);
> > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> >  	mutex_unlock(&mgr->lock);
> >  
> >  	if (send_remove)
> > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> >  	else
> >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > -			    payload->vcpi);
> > +			    new_payload->vcpi);
> >  
> >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > -			pos->vc_start_slot -= payload->time_slots;
> > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > +			pos->vc_start_slot -= old_payload->time_slots;
> >  	}
> > -	payload->vc_start_slot = -1;
> > +	new_payload->vc_start_slot = -1;
> >  
> >  	mgr->payload_count--;
> > -	mgr->next_start_slot -= payload->time_slots;
> > +	mgr->next_start_slot -= old_payload->time_slots;
> >  
> > -	if (payload->delete)
> > -		drm_dp_mst_put_port_malloc(payload->port);
> > +	if (new_payload->delete)
> > +		drm_dp_mst_put_port_malloc(new_payload->port);
> >  }
> >  EXPORT_SYMBOL(drm_dp_remove_payload);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> >  		to_intel_connector(old_conn_state->connector);
> >  	struct drm_dp_mst_topology_state *mst_state =
> >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > +	struct drm_dp_mst_atomic_payload *payload =
> > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> >  
> >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> >  	intel_hdcp_disable(intel_mst->connector);
> >  
> >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > +			      payload, payload);
> >  
> >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> >  }
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b4025..ed9d374147b8d 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> >  
> >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> >  	if (msto->disabled) {
> > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> >  
> >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> >  	} else {
> > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > index 41fd8352ab656..f5eb9aa152b14 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> >  			     struct drm_dp_mst_atomic_payload *payload);
> >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> >  			   struct drm_dp_mst_topology_state *mst_state,
> > -			   struct drm_dp_mst_atomic_payload *payload);
> > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > +			   struct drm_dp_mst_atomic_payload *new_payload);
> >  
> >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> >  
> 
> -- 
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
> 

Re: [PATCH v2 11/17] drm/display/dp_mst: Add helpers to query for payload allocation errors

[Dan Carpenter]

Hi Imre,

url:    https://github.com/intel-lab-lkp/linux/commits/Imre-Deak/drm-i915-dp_mst-Add-the-MST-topology-state-for-modesetted-CRTCs/20230131-230853
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20230131150548.1614458-12-imre.deak%40intel.com
patch subject: [PATCH v2 11/17] drm/display/dp_mst: Add helpers to query for payload allocation errors
config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20230202/202302021855.yyqIeQ2o-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

smatch warnings:
drivers/gpu/drm/display/drm_dp_mst_topology.c:3316 drm_dp_add_payload_part1() warn: missing error code 'ret'

vim +/ret +3316 drivers/gpu/drm/display/drm_dp_mst_topology.c

4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3304  int drm_dp_add_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3305  			     struct drm_dp_mst_topology_state *mst_state,
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3306  			     struct drm_dp_mst_atomic_payload *payload)
ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3307  {
ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3308  	struct drm_dp_mst_port *port;
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3309  	int ret = 0;
706246c761ddd3 drivers/gpu/drm/drm_dp_mst_topology.c         Lyude Paul       2018-12-13  3310  
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3311  	port = drm_dp_mst_topology_get_port_validated(mgr, payload->port);
33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3312  	if (!port) {
33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3313  		drm_dbg_kms(mgr->dev,
33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3314  			    "VCPI %d for port %p not in topology, not creating a payload\n",
33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3315  			    payload->vcpi, payload->port);
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31 @3316  		goto alloc_fail;

Hard to tell if this is an error path or a success path.

33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3317  	}
cfe9f90358d97a drivers/gpu/drm/drm_dp_mst_topology.c         Lyude Paul       2019-01-10  3318  
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3319  	if (mgr->payload_count == 0)
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3320  		mgr->next_start_slot = mst_state->start_slot;
3769e4c0af5b82 drivers/gpu/drm/drm_dp_mst_topology.c         Wayne Lin        2021-06-16  3321  
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3322  	payload->vc_start_slot = mgr->next_start_slot;
cfe9f90358d97a drivers/gpu/drm/drm_dp_mst_topology.c         Lyude Paul       2019-01-10  3323  
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3324  	ret = drm_dp_create_payload_step1(mgr, payload);
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3325  	drm_dp_mst_topology_put_port(port);
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3326  	if (ret < 0) {
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3327  		drm_warn(mgr->dev, "Failed to create MST payload for port %p: %d\n",
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3328  			 payload->port, ret);
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3329  		goto alloc_fail;
ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3330  	}
dfda0df3426483 drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-08-06  3331  
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3332  	payload->alloc_failed = false;
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3333  
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3334  	mgr->payload_count++;
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3335  	mgr->next_start_slot += payload->time_slots;
ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3336  
4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3337  	return 0;
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3338  
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3339  alloc_fail:
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3340  	payload->vc_start_slot = -1;
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3341  	payload->alloc_failed = true;
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3342  
5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3343  	return ret;
ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3344  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 11/17] drm/display/dp_mst: Add helpers to query for payload allocation errors

[Imre Deak]

Hi,

On Thu, Feb 02, 2023 at 03:15:46PM +0300, Dan Carpenter wrote:
> Hi Imre,
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Imre-Deak/drm-i915-dp_mst-Add-the-MST-topology-state-for-modesetted-CRTCs/20230131-230853
> base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
> patch link:    https://lore.kernel.org/r/20230131150548.1614458-12-imre.deak%40intel.com
> patch subject: [PATCH v2 11/17] drm/display/dp_mst: Add helpers to query for payload allocation errors
> config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20230202/202302021855.yyqIeQ2o-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> 
> smatch warnings:
> drivers/gpu/drm/display/drm_dp_mst_topology.c:3316 drm_dp_add_payload_part1() warn: missing error code 'ret'
> 
> vim +/ret +3316 drivers/gpu/drm/display/drm_dp_mst_topology.c
> 
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3304  int drm_dp_add_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3305  			     struct drm_dp_mst_topology_state *mst_state,
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3306  			     struct drm_dp_mst_atomic_payload *payload)
> ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3307  {
> ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3308  	struct drm_dp_mst_port *port;
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3309  	int ret = 0;
> 706246c761ddd3 drivers/gpu/drm/drm_dp_mst_topology.c         Lyude Paul       2018-12-13  3310  
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3311  	port = drm_dp_mst_topology_get_port_validated(mgr, payload->port);
> 33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3312  	if (!port) {
> 33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3313  		drm_dbg_kms(mgr->dev,
> 33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3314  			    "VCPI %d for port %p not in topology, not creating a payload\n",
> 33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3315  			    payload->vcpi, payload->port);
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31 @3316  		goto alloc_fail;
> 
> Hard to tell if this is an error path or a success path.

thanks for the report. The function before the change in this patchset
returned 0 in this case, so I didn't want to change that. Looking at the
callers none of them uses the return value (except for printing an error
message). I think returning an error code in this case as well would be
more consistent, but that change should be a follow-up.

> 33f960e23c29d1 drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2022-12-14  3317  	}
> cfe9f90358d97a drivers/gpu/drm/drm_dp_mst_topology.c         Lyude Paul       2019-01-10  3318  
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3319  	if (mgr->payload_count == 0)
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3320  		mgr->next_start_slot = mst_state->start_slot;
> 3769e4c0af5b82 drivers/gpu/drm/drm_dp_mst_topology.c         Wayne Lin        2021-06-16  3321  
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3322  	payload->vc_start_slot = mgr->next_start_slot;
> cfe9f90358d97a drivers/gpu/drm/drm_dp_mst_topology.c         Lyude Paul       2019-01-10  3323  
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3324  	ret = drm_dp_create_payload_step1(mgr, payload);
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3325  	drm_dp_mst_topology_put_port(port);
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3326  	if (ret < 0) {
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3327  		drm_warn(mgr->dev, "Failed to create MST payload for port %p: %d\n",
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3328  			 payload->port, ret);
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3329  		goto alloc_fail;
> ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3330  	}
> dfda0df3426483 drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-08-06  3331  
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3332  	payload->alloc_failed = false;
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3333  
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3334  	mgr->payload_count++;
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3335  	mgr->next_start_slot += payload->time_slots;
> ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3336  
> 4d07b0bc403403 drivers/gpu/drm/display/drm_dp_mst_topology.c Lyude Paul       2022-08-17  3337  	return 0;
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3338  
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3339  alloc_fail:
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3340  	payload->vc_start_slot = -1;
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3341  	payload->alloc_failed = true;
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3342  
> 5c4df7ffba973b drivers/gpu/drm/display/drm_dp_mst_topology.c Imre Deak        2023-01-31  3343  	return ret;
> ad7f8a1f9ced7f drivers/gpu/drm/drm_dp_mst_topology.c         Dave Airlie      2014-06-05  3344  }
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
> 

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Lyude Paul]

On Wed, 2023-02-01 at 17:04 +0200, Imre Deak wrote:
> 
> Yes, this patch should have no functional change, so please check what
> would apply to other drivers as well.
> 
> Could you also check Ville's comment about storing start_slot elsewhere
> than the atomic state (leaving only time_slots there). I wonder if that
> would work, at least it would simplify things I think.

Ideally it'd be nice if we could have all this info in the atomic commit, but
yeah - you're not the first person to find this a bit confusing. FWIW though,
the way we store start_slot right now is intended to follow the same kind of
behavior we have with atomic CRTC commit dependencies, I think I also made it
that way so all of the state would be in a single place but there's no hard
requirement for it.

So if you guys think it'd be better design-wise to store this something else,
I've got no strong feelings either way
> 
> > For 0-2:
> > 
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> 
> Thanks.
> 
> > 
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > Cc: Karol Herbst <kherbst@redhat.com>
> > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > > Cc: stable@vger.kernel.org # 6.1
> > > Cc: dri-devel@lists.freedesktop.org
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
> > >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> > >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> > >  5 files changed, 21 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > index a50319fc42b11..180d3893b68da 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> > >  	if (enable)
> > >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> > >  	else
> > > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
> > >  
> > >  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
> > >  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > index 847c10aa2098c..1990ff5dc7ddd 100644
> > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > >   * drm_dp_remove_payload() - Remove an MST payload
> > >   * @mgr: Manager to use.
> > >   * @mst_state: The MST atomic state
> > > - * @payload: The payload to write
> > > + * @old_payload: The payload with its old state
> > > + * @new_payload: The payload to write
> > >   *
> > >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> > >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > > @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > >   */
> > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > -			   struct drm_dp_mst_atomic_payload *payload)
> > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > +			   struct drm_dp_mst_atomic_payload *new_payload)
> > >  {
> > >  	struct drm_dp_mst_atomic_payload *pos;
> > >  	bool send_remove = false;
> > >  
> > >  	/* We failed to make the payload, so nothing to do */
> > > -	if (payload->vc_start_slot == -1)
> > > +	if (new_payload->vc_start_slot == -1)
> > >  		return;
> > >  
> > >  	mutex_lock(&mgr->lock);
> > > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> > >  	mutex_unlock(&mgr->lock);
> > >  
> > >  	if (send_remove)
> > > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> > >  	else
> > >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > > -			    payload->vcpi);
> > > +			    new_payload->vcpi);
> > >  
> > >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > > -			pos->vc_start_slot -= payload->time_slots;
> > > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > > +			pos->vc_start_slot -= old_payload->time_slots;
> > >  	}
> > > -	payload->vc_start_slot = -1;
> > > +	new_payload->vc_start_slot = -1;
> > >  
> > >  	mgr->payload_count--;
> > > -	mgr->next_start_slot -= payload->time_slots;
> > > +	mgr->next_start_slot -= old_payload->time_slots;
> > >  
> > > -	if (payload->delete)
> > > -		drm_dp_mst_put_port_malloc(payload->port);
> > > +	if (new_payload->delete)
> > > +		drm_dp_mst_put_port_malloc(new_payload->port);
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_remove_payload);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > >  		to_intel_connector(old_conn_state->connector);
> > >  	struct drm_dp_mst_topology_state *mst_state =
> > >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > > +	struct drm_dp_mst_atomic_payload *payload =
> > > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> > >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > >  
> > >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > >  	intel_hdcp_disable(intel_mst->connector);
> > >  
> > >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > > +			      payload, payload);
> > >  
> > >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> > >  }
> > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > index edcb2529b4025..ed9d374147b8d 100644
> > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> > >  
> > >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> > >  	if (msto->disabled) {
> > > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> > >  
> > >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> > >  	} else {
> > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > > index 41fd8352ab656..f5eb9aa152b14 100644
> > > --- a/include/drm/display/drm_dp_mst_helper.h
> > > +++ b/include/drm/display/drm_dp_mst_helper.h
> > > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> > >  			     struct drm_dp_mst_atomic_payload *payload);
> > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > -			   struct drm_dp_mst_atomic_payload *payload);
> > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > +			   struct drm_dp_mst_atomic_payload *new_payload);
> > >  
> > >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> > >  
> > 
> > -- 
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> > 
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

On Mon, Feb 06, 2023 at 07:42:32PM -0500, Lyude Paul wrote:
> On Wed, 2023-02-01 at 17:04 +0200, Imre Deak wrote:
> > 
> > Yes, this patch should have no functional change, so please check what
> > would apply to other drivers as well.
> > 
> > Could you also check Ville's comment about storing start_slot elsewhere
> > than the atomic state (leaving only time_slots there). I wonder if that
> > would work, at least it would simplify things I think.
> 
> Ideally it'd be nice if we could have all this info in the atomic commit, but
> yeah - you're not the first person to find this a bit confusing. FWIW though,
> the way we store start_slot right now is intended to follow the same kind of
> behavior we have with atomic CRTC commit dependencies, I think I also made it
> that way so all of the state would be in a single place but there's no hard
> requirement for it.

As I understood the atomic state should be precomputed during atomic
check and not changed later during the commit phase. In case of
start_slot this would mean knowing in advance the actual (driver
specific) disabling / enabling sequence of the payloads, not sure how
feasible it is to figure that out. start_slot also changes dynamically
as each payload is disabled, so these intermediate versions of the field
would need to be tracked somehow separately from the final (precomputed)
versions.

> So if you guys think it'd be better design-wise to store this something else,
> I've got no strong feelings either way
> > 
> > > For 0-2:
> > > 
> > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > 
> > Thanks.
> > 
> > > 
> > > > 
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > > Cc: Karol Herbst <kherbst@redhat.com>
> > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > > > Cc: stable@vger.kernel.org # 6.1
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> > > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
> > > >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> > > >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> > > >  5 files changed, 21 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > index a50319fc42b11..180d3893b68da 100644
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> > > >  	if (enable)
> > > >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> > > >  	else
> > > > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > > > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
> > > >  
> > > >  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
> > > >  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > index 847c10aa2098c..1990ff5dc7ddd 100644
> > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > >   * drm_dp_remove_payload() - Remove an MST payload
> > > >   * @mgr: Manager to use.
> > > >   * @mst_state: The MST atomic state
> > > > - * @payload: The payload to write
> > > > + * @old_payload: The payload with its old state
> > > > + * @new_payload: The payload to write
> > > >   *
> > > >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> > > >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > > > @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > >   */
> > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > -			   struct drm_dp_mst_atomic_payload *payload)
> > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > +			   struct drm_dp_mst_atomic_payload *new_payload)
> > > >  {
> > > >  	struct drm_dp_mst_atomic_payload *pos;
> > > >  	bool send_remove = false;
> > > >  
> > > >  	/* We failed to make the payload, so nothing to do */
> > > > -	if (payload->vc_start_slot == -1)
> > > > +	if (new_payload->vc_start_slot == -1)
> > > >  		return;
> > > >  
> > > >  	mutex_lock(&mgr->lock);
> > > > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > > > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> > > >  	mutex_unlock(&mgr->lock);
> > > >  
> > > >  	if (send_remove)
> > > > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > > > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> > > >  	else
> > > >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > > > -			    payload->vcpi);
> > > > +			    new_payload->vcpi);
> > > >  
> > > >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > > > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > > > -			pos->vc_start_slot -= payload->time_slots;
> > > > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > > > +			pos->vc_start_slot -= old_payload->time_slots;
> > > >  	}
> > > > -	payload->vc_start_slot = -1;
> > > > +	new_payload->vc_start_slot = -1;
> > > >  
> > > >  	mgr->payload_count--;
> > > > -	mgr->next_start_slot -= payload->time_slots;
> > > > +	mgr->next_start_slot -= old_payload->time_slots;
> > > >  
> > > > -	if (payload->delete)
> > > > -		drm_dp_mst_put_port_malloc(payload->port);
> > > > +	if (new_payload->delete)
> > > > +		drm_dp_mst_put_port_malloc(new_payload->port);
> > > >  }
> > > >  EXPORT_SYMBOL(drm_dp_remove_payload);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > >  		to_intel_connector(old_conn_state->connector);
> > > >  	struct drm_dp_mst_topology_state *mst_state =
> > > >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > > > +	struct drm_dp_mst_atomic_payload *payload =
> > > > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> > > >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > > >  
> > > >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > > > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > >  	intel_hdcp_disable(intel_mst->connector);
> > > >  
> > > >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > > > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > > > +			      payload, payload);
> > > >  
> > > >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> > > >  }
> > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > index edcb2529b4025..ed9d374147b8d 100644
> > > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> > > >  
> > > >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> > > >  	if (msto->disabled) {
> > > > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > > > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> > > >  
> > > >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> > > >  	} else {
> > > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > > > index 41fd8352ab656..f5eb9aa152b14 100644
> > > > --- a/include/drm/display/drm_dp_mst_helper.h
> > > > +++ b/include/drm/display/drm_dp_mst_helper.h
> > > > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> > > >  			     struct drm_dp_mst_atomic_payload *payload);
> > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > -			   struct drm_dp_mst_atomic_payload *payload);
> > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > +			   struct drm_dp_mst_atomic_payload *new_payload);
> > > >  
> > > >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> > > >  
> > > 
> > > -- 
> > > Cheers,
> > >  Lyude Paul (she/her)
> > >  Software Engineer at Red Hat
> > > 
> > 
> 
> -- 
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
> 

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Lyude Paul]

On Tue, 2023-02-07 at 14:11 +0200, Imre Deak wrote:
> On Mon, Feb 06, 2023 at 07:42:32PM -0500, Lyude Paul wrote:
> > On Wed, 2023-02-01 at 17:04 +0200, Imre Deak wrote:
> > > 
> > > Yes, this patch should have no functional change, so please check what
> > > would apply to other drivers as well.
> > > 
> > > Could you also check Ville's comment about storing start_slot elsewhere
> > > than the atomic state (leaving only time_slots there). I wonder if that
> > > would work, at least it would simplify things I think.
> > 
> > Ideally it'd be nice if we could have all this info in the atomic commit, but
> > yeah - you're not the first person to find this a bit confusing. FWIW though,
> > the way we store start_slot right now is intended to follow the same kind of
> > behavior we have with atomic CRTC commit dependencies, I think I also made it
> > that way so all of the state would be in a single place but there's no hard
> > requirement for it.
> 
> As I understood the atomic state should be precomputed during atomic
> check and not changed later during the commit phase. In case of
> start_slot this would mean knowing in advance the actual (driver
> specific) disabling / enabling sequence of the payloads, not sure how
> feasible it is to figure that out. start_slot also changes dynamically

It isn't feasible afaict, which was the main motivation for having the strange
update procedure - this is pretty much something that needs to be determined
at commit time.

> as each payload is disabled, so these intermediate versions of the field
> would need to be tracked somehow separately from the final (precomputed)
> versions.

FWIW, the current way this works is that we call
drm_dp_mst_atomic_wait_for_dependencies() in order to ensure that no one's
currently writing to the start_slot field (remember, at this point we may not
have atomic locks held anymore). Then at that point, start_slot in the new
atomic state should hold the current in-hw start_slot value. start_slot isn't
actually set to the new starting slot until drm_dp_add_payload_part1(). That
of course as you pointed out, doesn't help us unless we read all of the
start_slot values before we start changing payloads - since disabling payloads
inevitably changes the start slot of payloads that come afterwards.

I'll admit I'm a bit surprised this logic was wrong - if I recall the whole
reason I assumed this was OK when writing this code was that since we're
updating one payload at a time, ACT, repeat, that the start slots of each
payload in hardware _would_ actually change in the same way we modify them in
helpers. e.g., my expectation was if we had a bunch of payloads like this:

Payload #1: 15 slots, start_slot=0
Payload #2: 15 slots, start_slot=15
Payload #3: 15 slots, start_slot=30

And then disabled say, payload #1, that immediately after we get the ACT that
the payload table in hardware would look like this:

Payload #2: 15 slots, start_slot=0
Payload #3: 15 slots, start_slot=15

But it sounds like it actually would look like this in the real world?

Payload #2: 15 slots, start_slot=15
Payload #3: 15 slots, start_slot=30

So I'm curious, is there something I missed here? At what point does the MST
hub at the other end decide that it's time to move the start slots back? (keep
in mind, the MST specification does explicitly mention that there should never
be holes in the payload table - so something has to be shifting the payloads
back).

> 
> > So if you guys think it'd be better design-wise to store this something else,
> > I've got no strong feelings either way
> > > 
> > > > For 0-2:
> > > > 
> > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > 
> > > Thanks.
> > > 
> > > > 
> > > > > 
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > > > Cc: Karol Herbst <kherbst@redhat.com>
> > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > > > > Cc: stable@vger.kernel.org # 6.1
> > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> > > > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
> > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
> > > > >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> > > > >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> > > > >  5 files changed, 21 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > index a50319fc42b11..180d3893b68da 100644
> > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> > > > >  	if (enable)
> > > > >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> > > > >  	else
> > > > > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > > > > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
> > > > >  
> > > > >  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
> > > > >  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > index 847c10aa2098c..1990ff5dc7ddd 100644
> > > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > > >   * drm_dp_remove_payload() - Remove an MST payload
> > > > >   * @mgr: Manager to use.
> > > > >   * @mst_state: The MST atomic state
> > > > > - * @payload: The payload to write
> > > > > + * @old_payload: The payload with its old state
> > > > > + * @new_payload: The payload to write
> > > > >   *
> > > > >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> > > > >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > > > > @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > > >   */
> > > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > > -			   struct drm_dp_mst_atomic_payload *payload)
> > > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > > +			   struct drm_dp_mst_atomic_payload *new_payload)
> > > > >  {
> > > > >  	struct drm_dp_mst_atomic_payload *pos;
> > > > >  	bool send_remove = false;
> > > > >  
> > > > >  	/* We failed to make the payload, so nothing to do */
> > > > > -	if (payload->vc_start_slot == -1)
> > > > > +	if (new_payload->vc_start_slot == -1)
> > > > >  		return;
> > > > >  
> > > > >  	mutex_lock(&mgr->lock);
> > > > > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > > > > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> > > > >  	mutex_unlock(&mgr->lock);
> > > > >  
> > > > >  	if (send_remove)
> > > > > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > > > > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> > > > >  	else
> > > > >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > > > > -			    payload->vcpi);
> > > > > +			    new_payload->vcpi);
> > > > >  
> > > > >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > > > > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > > > > -			pos->vc_start_slot -= payload->time_slots;
> > > > > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > > > > +			pos->vc_start_slot -= old_payload->time_slots;
> > > > >  	}
> > > > > -	payload->vc_start_slot = -1;
> > > > > +	new_payload->vc_start_slot = -1;
> > > > >  
> > > > >  	mgr->payload_count--;
> > > > > -	mgr->next_start_slot -= payload->time_slots;
> > > > > +	mgr->next_start_slot -= old_payload->time_slots;
> > > > >  
> > > > > -	if (payload->delete)
> > > > > -		drm_dp_mst_put_port_malloc(payload->port);
> > > > > +	if (new_payload->delete)
> > > > > +		drm_dp_mst_put_port_malloc(new_payload->port);
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_dp_remove_payload);
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > > >  		to_intel_connector(old_conn_state->connector);
> > > > >  	struct drm_dp_mst_topology_state *mst_state =
> > > > >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > > > > +	struct drm_dp_mst_atomic_payload *payload =
> > > > > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> > > > >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > > > >  
> > > > >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > > > > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > > >  	intel_hdcp_disable(intel_mst->connector);
> > > > >  
> > > > >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > > > > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > > > > +			      payload, payload);
> > > > >  
> > > > >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> > > > >  }
> > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > index edcb2529b4025..ed9d374147b8d 100644
> > > > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> > > > >  
> > > > >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> > > > >  	if (msto->disabled) {
> > > > > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > > > > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> > > > >  
> > > > >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> > > > >  	} else {
> > > > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > > > > index 41fd8352ab656..f5eb9aa152b14 100644
> > > > > --- a/include/drm/display/drm_dp_mst_helper.h
> > > > > +++ b/include/drm/display/drm_dp_mst_helper.h
> > > > > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> > > > >  			     struct drm_dp_mst_atomic_payload *payload);
> > > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > > -			   struct drm_dp_mst_atomic_payload *payload);
> > > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > > +			   struct drm_dp_mst_atomic_payload *new_payload);
> > > > >  
> > > > >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> > > > >  
> > > > 
> > > > -- 
> > > > Cheers,
> > > >  Lyude Paul (she/her)
> > > >  Software Engineer at Red Hat
> > > > 
> > > 
> > 
> > -- 
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> > 
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

On Tue, Feb 07, 2023 at 07:21:48PM -0500, Lyude Paul wrote:
> On Tue, 2023-02-07 at 14:11 +0200, Imre Deak wrote:
> > On Mon, Feb 06, 2023 at 07:42:32PM -0500, Lyude Paul wrote:
> > > On Wed, 2023-02-01 at 17:04 +0200, Imre Deak wrote:
> > > > 
> > > > Yes, this patch should have no functional change, so please check what
> > > > would apply to other drivers as well.
> > > > 
> > > > Could you also check Ville's comment about storing start_slot elsewhere
> > > > than the atomic state (leaving only time_slots there). I wonder if that
> > > > would work, at least it would simplify things I think.
> > > 
> > > Ideally it'd be nice if we could have all this info in the atomic commit, but
> > > yeah - you're not the first person to find this a bit confusing. FWIW though,
> > > the way we store start_slot right now is intended to follow the same kind of
> > > behavior we have with atomic CRTC commit dependencies, I think I also made it
> > > that way so all of the state would be in a single place but there's no hard
> > > requirement for it.
> > 
> > As I understood the atomic state should be precomputed during atomic
> > check and not changed later during the commit phase. In case of
> > start_slot this would mean knowing in advance the actual (driver
> > specific) disabling / enabling sequence of the payloads, not sure how
> > feasible it is to figure that out. start_slot also changes dynamically
> 
> It isn't feasible afaict, which was the main motivation for having the strange
> update procedure - this is pretty much something that needs to be determined
> at commit time.
>
> > as each payload is disabled, so these intermediate versions of the field
> > would need to be tracked somehow separately from the final (precomputed)
> > versions.
> 
> FWIW, the current way this works is that we call
> drm_dp_mst_atomic_wait_for_dependencies() in order to ensure that no one's
> currently writing to the start_slot field (remember, at this point we may not
> have atomic locks held anymore). Then at that point, start_slot in the new
> atomic state should hold the current in-hw start_slot value. start_slot isn't
> actually set to the new starting slot until drm_dp_add_payload_part1(). That
> of course as you pointed out, doesn't help us unless we read all of the
> start_slot values before we start changing payloads - since disabling payloads
> inevitably changes the start slot of payloads that come afterwards.
> 
> I'll admit I'm a bit surprised this logic was wrong - if I recall the whole
> reason I assumed this was OK when writing this code was that since we're
> updating one payload at a time, ACT, repeat, that the start slots of each
> payload in hardware _would_ actually change in the same way we modify them in
> helpers. e.g., my expectation was if we had a bunch of payloads like this:
> 
> Payload #1: 15 slots, start_slot=0
> Payload #2: 15 slots, start_slot=15
> Payload #3: 15 slots, start_slot=30
> 
> And then disabled say, payload #1, that immediately after we get the ACT that
> the payload table in hardware would look like this:
> 
> Payload #2: 15 slots, start_slot=0
> Payload #3: 15 slots, start_slot=15

The above is the actual and expected HW state state yes.

> But it sounds like it actually would look like this in the real world?
> 
> Payload #2: 15 slots, start_slot=15
> Payload #3: 15 slots, start_slot=30

No, the problem currently is not that start_slot of the subsequent
payloads are not shifted towards the beginning. Rather the atomic state
doesn't get updated properly, becoming out of sync with the HW. For
instance in a commit resizing payload #1, in the commit phase
(intel_atomic_commit_tail()) will begin by removing payload #1. The
initial state is

            old payload state         new payload state
Payload #1: 15 slots, start_slot=0    20 slots, start_slot=0
Payload #2: 15 slots, start_slot=15   15 slots, start_slot=15
Payload #3: 15 slots, start_slot=30   15 slots, start_slot=30

mgr->next_start_slot = 45

intel_mst_disable_dp() will pass the old MST and payload state to
drm_dp_remove_payload(): The MST state was added during atomic check,
since payload #1 changed, then intel_atomic_commit() ->
drm_atomic_helper_swap_state() sets the MST current state (returned by
drm_atomic_get_mst_topology_state()) to point to the old state. So at
the point drm_dp_remove_payload() returns we have:

            old payload state         new payload state
Payload #1: 15 slots, start_slot=-1   20 slots, start_slot=0
Payload #2: 15 slots, start_slot=0    15 slots, start_slot=15
Payload #3: 15 slots, start_slot=15   15 slots, start_slot=30

mgr->next_start_slot = 30

then after re-enabling payload #1, after drm_dp_add_payload_part1()
returns (passing to it the new MST and payload state) we have:

            old payload state         new payload state
Payload #1: 15 slots, start_slot=-1   20 slots, start_slot=30
Payload #2: 15 slots, start_slot=0    15 slots, start_slot=15
Payload #3: 15 slots, start_slot=15   15 slots, start_slot=30

mgr->next_start_slot = 50

So in the new SW state payload #1 and #3 incorrectly overlap, with the
actual HW state being:

Payload #1: 20 slots, start_slot=30
Payload #2: 15 slots, start_slot=0
Payload #3: 15 slots, start_slot=15

A subsequent commit will see the wrong start_slot in the SW state for
payload #2 (15) and #3 (30).

> So I'm curious, is there something I missed here? At what point does the MST
> hub at the other end decide that it's time to move the start slots back?

The hub shifts back payloads after the DPCD write command to 0x1c0 -
0x1c2 to remove a payload. (The HW OTOH does the corresponding shift at
the point of disabling the stream, in intel_mst_post_disable_dp() ->
intel_disable_transcoder() for i915).

> (keep in mind, the MST specification does explicitly mention that
> there should never be holes in the payload table - so something has to
> be shifting the payloads back).

Right, the hubs I checked conform to this.

> > > So if you guys think it'd be better design-wise to store this something else,
> > > I've got no strong feelings either way
> > > > 
> > > > > For 0-2:
> > > > > 
> > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > 
> > > > Thanks.
> > > > 
> > > > > 
> > > > > > 
> > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > > > > Cc: Karol Herbst <kherbst@redhat.com>
> > > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > > > > > Cc: stable@vger.kernel.org # 6.1
> > > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > ---
> > > > > >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> > > > > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
> > > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
> > > > > >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> > > > > >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> > > > > >  5 files changed, 21 insertions(+), 16 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > > index a50319fc42b11..180d3893b68da 100644
> > > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > > @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> > > > > >  	if (enable)
> > > > > >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> > > > > >  	else
> > > > > > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > > > > > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
> > > > > >  
> > > > > >  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
> > > > > >  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > index 847c10aa2098c..1990ff5dc7ddd 100644
> > > > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > > > >   * drm_dp_remove_payload() - Remove an MST payload
> > > > > >   * @mgr: Manager to use.
> > > > > >   * @mst_state: The MST atomic state
> > > > > > - * @payload: The payload to write
> > > > > > + * @old_payload: The payload with its old state
> > > > > > + * @new_payload: The payload to write
> > > > > >   *
> > > > > >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> > > > > >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > > > > > @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > > > >   */
> > > > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > > > -			   struct drm_dp_mst_atomic_payload *payload)
> > > > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > > > +			   struct drm_dp_mst_atomic_payload *new_payload)
> > > > > >  {
> > > > > >  	struct drm_dp_mst_atomic_payload *pos;
> > > > > >  	bool send_remove = false;
> > > > > >  
> > > > > >  	/* We failed to make the payload, so nothing to do */
> > > > > > -	if (payload->vc_start_slot == -1)
> > > > > > +	if (new_payload->vc_start_slot == -1)
> > > > > >  		return;
> > > > > >  
> > > > > >  	mutex_lock(&mgr->lock);
> > > > > > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > > > > > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> > > > > >  	mutex_unlock(&mgr->lock);
> > > > > >  
> > > > > >  	if (send_remove)
> > > > > > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > > > > > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> > > > > >  	else
> > > > > >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > > > > > -			    payload->vcpi);
> > > > > > +			    new_payload->vcpi);
> > > > > >  
> > > > > >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > > > > > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > > > > > -			pos->vc_start_slot -= payload->time_slots;
> > > > > > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > > > > > +			pos->vc_start_slot -= old_payload->time_slots;
> > > > > >  	}
> > > > > > -	payload->vc_start_slot = -1;
> > > > > > +	new_payload->vc_start_slot = -1;
> > > > > >  
> > > > > >  	mgr->payload_count--;
> > > > > > -	mgr->next_start_slot -= payload->time_slots;
> > > > > > +	mgr->next_start_slot -= old_payload->time_slots;
> > > > > >  
> > > > > > -	if (payload->delete)
> > > > > > -		drm_dp_mst_put_port_malloc(payload->port);
> > > > > > +	if (new_payload->delete)
> > > > > > +		drm_dp_mst_put_port_malloc(new_payload->port);
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(drm_dp_remove_payload);
> > > > > >  
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > > > >  		to_intel_connector(old_conn_state->connector);
> > > > > >  	struct drm_dp_mst_topology_state *mst_state =
> > > > > >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > > > > > +	struct drm_dp_mst_atomic_payload *payload =
> > > > > > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> > > > > >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > > > > >  
> > > > > >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > > > > > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > > > >  	intel_hdcp_disable(intel_mst->connector);
> > > > > >  
> > > > > >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > > > > > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > > > > > +			      payload, payload);
> > > > > >  
> > > > > >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> > > > > >  }
> > > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > > index edcb2529b4025..ed9d374147b8d 100644
> > > > > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> > > > > >  
> > > > > >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> > > > > >  	if (msto->disabled) {
> > > > > > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > > > > > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> > > > > >  
> > > > > >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> > > > > >  	} else {
> > > > > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > > > > > index 41fd8352ab656..f5eb9aa152b14 100644
> > > > > > --- a/include/drm/display/drm_dp_mst_helper.h
> > > > > > +++ b/include/drm/display/drm_dp_mst_helper.h
> > > > > > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> > > > > >  			     struct drm_dp_mst_atomic_payload *payload);
> > > > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > > > -			   struct drm_dp_mst_atomic_payload *payload);
> > > > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > > > +			   struct drm_dp_mst_atomic_payload *new_payload);
> > > > > >  
> > > > > >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> > > > > >  
> > > > > 
> > > > > -- 
> > > > > Cheers,
> > > > >  Lyude Paul (she/her)
> > > > >  Software Engineer at Red Hat
> > > > > 
> > > > 
> > > 
> > > -- 
> > > Cheers,
> > >  Lyude Paul (she/her)
> > >  Software Engineer at Red Hat
> > > 
> > 
> 
> -- 
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
> 

Re: [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Lyude Paul]

On Wed, 2023-02-08 at 09:41 +0200, Imre Deak wrote:
> On Tue, Feb 07, 2023 at 07:21:48PM -0500, Lyude Paul wrote:
> > On Tue, 2023-02-07 at 14:11 +0200, Imre Deak wrote:
> > 
> > And then disabled say, payload #1, that immediately after we get the ACT that
> > the payload table in hardware would look like this:
> > 
> > Payload #2: 15 slots, start_slot=0
> > Payload #3: 15 slots, start_slot=15
> 
> The above is the actual and expected HW state state yes.
> 
> > But it sounds like it actually would look like this in the real world?
> > 
> > Payload #2: 15 slots, start_slot=15
> > Payload #3: 15 slots, start_slot=30
> 
> No, the problem currently is not that start_slot of the subsequent
> payloads are not shifted towards the beginning. Rather the atomic state
> doesn't get updated properly, becoming out of sync with the HW. For
> instance in a commit resizing payload #1, in the commit phase
> (intel_atomic_commit_tail()) will begin by removing payload #1. The
> initial state is
> 
>             old payload state         new payload state
> Payload #1: 15 slots, start_slot=0    20 slots, start_slot=0
> Payload #2: 15 slots, start_slot=15   15 slots, start_slot=15
> Payload #3: 15 slots, start_slot=30   15 slots, start_slot=30
> 
> mgr->next_start_slot = 45
> 
> intel_mst_disable_dp() will pass the old MST and payload state to
> drm_dp_remove_payload(): The MST state was added during atomic check,
> since payload #1 changed, then intel_atomic_commit() ->
> drm_atomic_helper_swap_state() sets the MST current state (returned by
> drm_atomic_get_mst_topology_state()) to point to the old state. So at

OK - this took me a while to wrap my head around but you're completely right.
It appears I totally misunderstood where the state swapping actually happens
during the check -> commit sequence. I think if that's how things work too
then yeah, it definitely might not be a bad idea to move the start slot out of
the atomic state :P. I guess we could just keep this in the mst manager struct
instead of the commit state and make the rules for access be the same: protect
them through commit ordering, and document that the proper way of accessing
start values outside of the context of an atomic commit (if this was needed
for some reason) is:

* grab mst lock
* call drm_dp_mst_atomic_wait_for_dependencies()
* read values under lock

Thank y'all again so much for helping out with this! It is super appreciated,
and once you guys push these patches upstream I will look into adopting this
for nouveau. I already poked some folks from AMD as well to make sure they're
keeping an eye on this (although looking at the Cc I realize they were already
added a while ago, whoops lol). 

> the point drm_dp_remove_payload() returns we have:
> 
>             old payload state         new payload state
> Payload #1: 15 slots, start_slot=-1   20 slots, start_slot=0
> Payload #2: 15 slots, start_slot=0    15 slots, start_slot=15
> Payload #3: 15 slots, start_slot=15   15 slots, start_slot=30
> 
> mgr->next_start_slot = 30
> 
> then after re-enabling payload #1, after drm_dp_add_payload_part1()
> returns (passing to it the new MST and payload state) we have:
> 
>             old payload state         new payload state
> Payload #1: 15 slots, start_slot=-1   20 slots, start_slot=30
> Payload #2: 15 slots, start_slot=0    15 slots, start_slot=15
> Payload #3: 15 slots, start_slot=15   15 slots, start_slot=30
> 
> mgr->next_start_slot = 50
> 
> So in the new SW state payload #1 and #3 incorrectly overlap, with the
> actual HW state being:
> 
> Payload #1: 20 slots, start_slot=30
> Payload #2: 15 slots, start_slot=0
> Payload #3: 15 slots, start_slot=15
> 
> A subsequent commit will see the wrong start_slot in the SW state for
> payload #2 (15) and #3 (30).
> 
> > So I'm curious, is there something I missed here? At what point does the MST
> > hub at the other end decide that it's time to move the start slots back?
> 
> The hub shifts back payloads after the DPCD write command to 0x1c0 -
> 0x1c2 to remove a payload. (The HW OTOH does the corresponding shift at
> the point of disabling the stream, in intel_mst_post_disable_dp() ->
> intel_disable_transcoder() for i915).
> 
> > (keep in mind, the MST specification does explicitly mention that
> > there should never be holes in the payload table - so something has to
> > be shifting the payloads back).
> 
> Right, the hubs I checked conform to this.

> 
> > > > So if you guys think it'd be better design-wise to store this something else,
> > > > I've got no strong feelings either way
> > > > > 
> > > > > > For 0-2:
> > > > > > 
> > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > > > > > Cc: Karol Herbst <kherbst@redhat.com>
> > > > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > > > > > > Cc: stable@vger.kernel.org # 6.1
> > > > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > ---
> > > > > > >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
> > > > > > >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++---------
> > > > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  4 ++-
> > > > > > >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 +-
> > > > > > >  include/drm/display/drm_dp_mst_helper.h       |  3 ++-
> > > > > > >  5 files changed, 21 insertions(+), 16 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > > > index a50319fc42b11..180d3893b68da 100644
> > > > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > > > > @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
> > > > > > >  	if (enable)
> > > > > > >  		drm_dp_add_payload_part1(mst_mgr, mst_state, payload);
> > > > > > >  	else
> > > > > > > -		drm_dp_remove_payload(mst_mgr, mst_state, payload);
> > > > > > > +		drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);
> > > > > > >  
> > > > > > >  	/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
> > > > > > >  	 * AUX message. The sequence is slot 1-63 allocated sequence for each
> > > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > > index 847c10aa2098c..1990ff5dc7ddd 100644
> > > > > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > > @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > > > > >   * drm_dp_remove_payload() - Remove an MST payload
> > > > > > >   * @mgr: Manager to use.
> > > > > > >   * @mst_state: The MST atomic state
> > > > > > > - * @payload: The payload to write
> > > > > > > + * @old_payload: The payload with its old state
> > > > > > > + * @new_payload: The payload to write
> > > > > > >   *
> > > > > > >   * Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
> > > > > > >   * the starting time slots of all other payloads which would have been shifted towards the start of
> > > > > > > @@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
> > > > > > >   */
> > > > > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > > > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > > > > -			   struct drm_dp_mst_atomic_payload *payload)
> > > > > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > > > > +			   struct drm_dp_mst_atomic_payload *new_payload)
> > > > > > >  {
> > > > > > >  	struct drm_dp_mst_atomic_payload *pos;
> > > > > > >  	bool send_remove = false;
> > > > > > >  
> > > > > > >  	/* We failed to make the payload, so nothing to do */
> > > > > > > -	if (payload->vc_start_slot == -1)
> > > > > > > +	if (new_payload->vc_start_slot == -1)
> > > > > > >  		return;
> > > > > > >  
> > > > > > >  	mutex_lock(&mgr->lock);
> > > > > > > -	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
> > > > > > > +	send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary);
> > > > > > >  	mutex_unlock(&mgr->lock);
> > > > > > >  
> > > > > > >  	if (send_remove)
> > > > > > > -		drm_dp_destroy_payload_step1(mgr, mst_state, payload);
> > > > > > > +		drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
> > > > > > >  	else
> > > > > > >  		drm_dbg_kms(mgr->dev, "Payload for VCPI %d not in topology, not sending remove\n",
> > > > > > > -			    payload->vcpi);
> > > > > > > +			    new_payload->vcpi);
> > > > > > >  
> > > > > > >  	list_for_each_entry(pos, &mst_state->payloads, next) {
> > > > > > > -		if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)
> > > > > > > -			pos->vc_start_slot -= payload->time_slots;
> > > > > > > +		if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
> > > > > > > +			pos->vc_start_slot -= old_payload->time_slots;
> > > > > > >  	}
> > > > > > > -	payload->vc_start_slot = -1;
> > > > > > > +	new_payload->vc_start_slot = -1;
> > > > > > >  
> > > > > > >  	mgr->payload_count--;
> > > > > > > -	mgr->next_start_slot -= payload->time_slots;
> > > > > > > +	mgr->next_start_slot -= old_payload->time_slots;
> > > > > > >  
> > > > > > > -	if (payload->delete)
> > > > > > > -		drm_dp_mst_put_port_malloc(payload->port);
> > > > > > > +	if (new_payload->delete)
> > > > > > > +		drm_dp_mst_put_port_malloc(new_payload->port);
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL(drm_dp_remove_payload);
> > > > > > >  
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > > index f3cb12dcfe0a7..dc4e5ff1dbb31 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > > @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > > > > >  		to_intel_connector(old_conn_state->connector);
> > > > > > >  	struct drm_dp_mst_topology_state *mst_state =
> > > > > > >  		drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > > > > > > +	struct drm_dp_mst_atomic_payload *payload =
> > > > > > > +		drm_atomic_get_mst_payload_state(mst_state, connector->port);
> > > > > > >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > > > > > >  
> > > > > > >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > > > > > > @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> > > > > > >  	intel_hdcp_disable(intel_mst->connector);
> > > > > > >  
> > > > > > >  	drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
> > > > > > > -			      drm_atomic_get_mst_payload_state(mst_state, connector->port));
> > > > > > > +			      payload, payload);
> > > > > > >  
> > > > > > >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> > > > > > >  }
> > > > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > > > index edcb2529b4025..ed9d374147b8d 100644
> > > > > > > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > > > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > > > > > > @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state,
> > > > > > >  
> > > > > > >  	// TODO: Figure out if we want to do a better job of handling VCPI allocation failures here?
> > > > > > >  	if (msto->disabled) {
> > > > > > > -		drm_dp_remove_payload(mgr, mst_state, payload);
> > > > > > > +		drm_dp_remove_payload(mgr, mst_state, payload, payload);
> > > > > > >  
> > > > > > >  		nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0);
> > > > > > >  	} else {
> > > > > > > diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> > > > > > > index 41fd8352ab656..f5eb9aa152b14 100644
> > > > > > > --- a/include/drm/display/drm_dp_mst_helper.h
> > > > > > > +++ b/include/drm/display/drm_dp_mst_helper.h
> > > > > > > @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> > > > > > >  			     struct drm_dp_mst_atomic_payload *payload);
> > > > > > >  void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr,
> > > > > > >  			   struct drm_dp_mst_topology_state *mst_state,
> > > > > > > -			   struct drm_dp_mst_atomic_payload *payload);
> > > > > > > +			   const struct drm_dp_mst_atomic_payload *old_payload,
> > > > > > > +			   struct drm_dp_mst_atomic_payload *new_payload);
> > > > > > >  
> > > > > > >  int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> > > > > > >  
> > > > > > 
> > > > > > -- 
> > > > > > Cheers,
> > > > > >  Lyude Paul (she/her)
> > > > > >  Software Engineer at Red Hat
> > > > > > 
> > > > > 
> > > > 
> > > > -- 
> > > > Cheers,
> > > >  Lyude Paul (she/her)
> > > >  Software Engineer at Red Hat
> > > > 
> > > 
> > 
> > -- 
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> > 
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat