[Intel-gfx] [PATCH 0/9] drm/i915/dp_mst: Fix MST payload removal during output disabling

[Intel-gfx] [PATCH 0/9] drm/i915/dp_mst: Fix MST payload removal during output disabling

[Imre Deak]

This patchset fixes a payload tracking issue in the MST encoder's
disabling hook and adds verification for the related sw and sink state.
It also factors out a few functions to modeset CRTCs and connectors
during the atomic check phase to make it clearer where this is done.

Tested on ICL and ADLP.

Imre Deak (9):
  drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs
  drm/display/dp_mst: Handle old/new payload states in
    drm_dp_remove_payload()
  drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()
  drm/i915/dp_mst: Fix payload removal during output disabling
  drm/display/dp_mst: Fix the payload VCPI check in
    drm_dp_mst_dump_topology()
  drm/i915: Factor out helpers for modesetting CRTCs and connectors
  drm/i915/dp_mst: Move getting the MST topology state earlier to
    connector check
  drm/display/dp_mst: Add a helper to verify the MST payload state
  drm/i915/dp_mst: Verify the MST state of modesetted outputs

 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |   2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 222 +++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_atomic.c   | 126 ++++++++++
 drivers/gpu/drm/i915/display/intel_atomic.h   |   7 +
 drivers/gpu/drm/i915/display/intel_cdclk.c    |   2 +-
 drivers/gpu/drm/i915/display/intel_display.c  |  57 +----
 drivers/gpu/drm/i915/display/intel_display.h  |   2 -
 drivers/gpu/drm/i915/display/intel_dp.c       |  40 +---
 drivers/gpu/drm/i915/display/intel_dp.h       |   1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 132 ++++++++---
 drivers/gpu/drm/i915/display/intel_dp_mst.h   |   3 +
 .../drm/i915/display/intel_modeset_verify.c   |   2 +
 drivers/gpu/drm/i915/display/skl_watermark.c  |   2 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |   2 +-
 include/drm/display/drm_dp.h                  |   3 +
 include/drm/display/drm_dp_mst_helper.h       |   9 +-
 16 files changed, 472 insertions(+), 140 deletions(-)

-- 
2.37.1

[PATCH 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Imre Deak]

Add the MST topology for a CRTC to the atomic state if the driver
needs to force a modeset on the CRTC after the encoder compute config
functions are called.

Later the MST encoder's disable hook also adds the state, but that isn't
guaranteed to work (since in that hook getting the state may fail, which
can't be handled there). This should fix that, while a later patch fixes
the use of the MST state in the disable hook.

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  2 ++
 3 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 717ca3d7890d3..d3994e2a7d636 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
 		if (ret)
 			return ret;
 
+		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
+		if (ret)
+			return ret;
+
 		ret = intel_atomic_add_affected_planes(state, crtc);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 8b0e4defa3f10..ba29c294b7c1b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
 	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
 	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
 }
+
+/**
+ * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
+ * @state: atomic state
+ * @crtc: CRTC
+ *
+ * Add the MST topology state for @crtc to @state.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc)
+{
+	struct drm_connector *_connector;
+	struct drm_connector_state *conn_state;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
+		struct drm_dp_mst_topology_state *mst_state;
+		struct intel_connector *connector = to_intel_connector(_connector);
+
+		if (conn_state->crtc != &crtc->base)
+			continue;
+
+		if (!connector->mst_port)
+			continue;
+
+		mst_state = drm_atomic_get_mst_topology_state(&state->base,
+							      &connector->mst_port->mst_mgr);
+		if (IS_ERR(mst_state))
+			return PTR_ERR(mst_state);
+
+		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index f7301de6cdfb3..0cd05a9a78a25 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -18,5 +18,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
 bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.37.1

[Intel-gfx] [PATCH 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Imre Deak]

Add the MST topology for a CRTC to the atomic state if the driver
needs to force a modeset on the CRTC after the encoder compute config
functions are called.

Later the MST encoder's disable hook also adds the state, but that isn't
guaranteed to work (since in that hook getting the state may fail, which
can't be handled there). This should fix that, while a later patch fixes
the use of the MST state in the disable hook.

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  2 ++
 3 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 717ca3d7890d3..d3994e2a7d636 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
 		if (ret)
 			return ret;
 
+		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
+		if (ret)
+			return ret;
+
 		ret = intel_atomic_add_affected_planes(state, crtc);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 8b0e4defa3f10..ba29c294b7c1b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
 	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
 	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
 }
+
+/**
+ * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
+ * @state: atomic state
+ * @crtc: CRTC
+ *
+ * Add the MST topology state for @crtc to @state.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc)
+{
+	struct drm_connector *_connector;
+	struct drm_connector_state *conn_state;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
+		struct drm_dp_mst_topology_state *mst_state;
+		struct intel_connector *connector = to_intel_connector(_connector);
+
+		if (conn_state->crtc != &crtc->base)
+			continue;
+
+		if (!connector->mst_port)
+			continue;
+
+		mst_state = drm_atomic_get_mst_topology_state(&state->base,
+							      &connector->mst_port->mst_mgr);
+		if (IS_ERR(mst_state))
+			return PTR_ERR(mst_state);
+
+		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index f7301de6cdfb3..0cd05a9a78a25 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -18,5 +18,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
 bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.37.1

[PATCH 2/9] 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: 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
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 | 22 ++++++++++---------
 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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
@@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
 }
 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 ba29c294b7c1b..5f7bcb5c14847 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 2/9] 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: 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
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 | 22 ++++++++++---------
 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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
@@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
 }
 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 ba29c294b7c1b..5f7bcb5c14847 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

[Intel-gfx] [PATCH 2/9] 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: 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
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 | 22 ++++++++++---------
 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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
@@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
 }
 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 ba29c294b7c1b..5f7bcb5c14847 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 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

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().

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
 include/drm/display/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5362,18 +5362,43 @@ 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 *priv_state =
+		drm_atomic_get_old_private_obj_state(state, &mgr->base);
+
+	return priv_state ? to_dp_mst_topology_state(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 *
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 3/9] 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().

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
 include/drm/display/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5362,18 +5362,43 @@ 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 *priv_state =
+		drm_atomic_get_old_private_obj_state(state, &mgr->base);
+
+	return priv_state ? to_dp_mst_topology_state(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 *
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

[Intel-gfx] [PATCH 3/9] 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().

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
 include/drm/display/drm_dp_mst_helper.h       |  3 ++
 2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5362,18 +5362,43 @@ 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 *priv_state =
+		drm_atomic_get_old_private_obj_state(state, &mgr->base);
+
+	return priv_state ? to_dp_mst_topology_state(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 *
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 4/9] drm/i915/dp_mst: Fix payload removal during output disabling

[Imre Deak]

Use the correct old/new topology and payload states in
intel_mst_disable_dp(). So far drm_atomic_get_mst_topology_state() it
used returned either the old state, in case the state was added already
earlier during the atomic check phase or otherwise the new state (but
the latter could fail, which can't be handled in the enable/disable
hooks). After the first patch in the patchset, the state should always
get added already during the check phase, so here we can get the
old/new states without a failure.

drm_dp_remove_payload() should use time_slots from the old payload state
and vc_start_slot in the new one. It should update the new payload
states to reflect the sink's current payload table after the payload is
removed. Pass the new topology state and the old and new payload states
accordingly.

This also fixes a problem where the payload allocations for multiple MST
streams on the same link got inconsistent after a few commits, as
during payload removal the old instead of the new payload state got
updated, so the subsequent enabling sequence and commits used a stale
payload state.

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5f7bcb5c14847..800fa12a61d93 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -524,10 +524,14 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = &dig_port->dp;
 	struct intel_connector *connector =
 		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_dp_mst_topology_state *old_mst_state =
+		drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
+	struct drm_dp_mst_topology_state *new_mst_state =
+		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
+	struct drm_dp_mst_atomic_payload *old_payload =
+		drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
+	struct drm_dp_mst_atomic_payload *new_payload =
+		drm_atomic_get_mst_payload_state(new_mst_state, connector->port);
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 
 	drm_dbg_kms(&i915->drm, "active links %d\n",
@@ -535,8 +539,8 @@ 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,
-			      payload, payload);
+	drm_dp_remove_payload(&intel_dp->mst_mgr, new_mst_state,
+			      old_payload, new_payload);
 
 	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
 }
-- 
2.37.1

[Intel-gfx] [PATCH 4/9] drm/i915/dp_mst: Fix payload removal during output disabling

[Imre Deak]

Use the correct old/new topology and payload states in
intel_mst_disable_dp(). So far drm_atomic_get_mst_topology_state() it
used returned either the old state, in case the state was added already
earlier during the atomic check phase or otherwise the new state (but
the latter could fail, which can't be handled in the enable/disable
hooks). After the first patch in the patchset, the state should always
get added already during the check phase, so here we can get the
old/new states without a failure.

drm_dp_remove_payload() should use time_slots from the old payload state
and vc_start_slot in the new one. It should update the new payload
states to reflect the sink's current payload table after the payload is
removed. Pass the new topology state and the old and new payload states
accordingly.

This also fixes a problem where the payload allocations for multiple MST
streams on the same link got inconsistent after a few commits, as
during payload removal the old instead of the new payload state got
updated, so the subsequent enabling sequence and commits used a stale
payload state.

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5f7bcb5c14847..800fa12a61d93 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -524,10 +524,14 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = &dig_port->dp;
 	struct intel_connector *connector =
 		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_dp_mst_topology_state *old_mst_state =
+		drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
+	struct drm_dp_mst_topology_state *new_mst_state =
+		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
+	struct drm_dp_mst_atomic_payload *old_payload =
+		drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
+	struct drm_dp_mst_atomic_payload *new_payload =
+		drm_atomic_get_mst_payload_state(new_mst_state, connector->port);
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 
 	drm_dbg_kms(&i915->drm, "active links %d\n",
@@ -535,8 +539,8 @@ 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,
-			      payload, payload);
+	drm_dp_remove_payload(&intel_dp->mst_mgr, new_mst_state,
+			      old_payload, new_payload);
 
 	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
 }
-- 
2.37.1

[PATCH 5/9] 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: 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 81cc0c3b1e000..619f616d69e20 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4770,7 +4770,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

[Intel-gfx] [PATCH 5/9] 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: 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 81cc0c3b1e000..619f616d69e20 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4770,7 +4770,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

[Intel-gfx] [PATCH 6/9] drm/i915: Factor out helpers for modesetting CRTCs and connectors

[Imre Deak]

Factor out and add the functions to intel_atomic.c to modeset pipes or
connectors. These can be used in a few places, also making it a bit
clearer where modeset forcing is needed.

After this patch the MST topology state for modesetted CRTCs is added
already in the connector's atomic check function, but that shouldn't
matter since the state would be added later anyway in the encoders'
compute config function. Also the crtc_state->update_pipe flag will be
reset for CRTCs modesetted from a connector check function, but this
should be ok, since the flag can get set only later in
intel_crtc_check_fastset().

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c  | 126 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_atomic.h  |   7 ++
 drivers/gpu/drm/i915/display/intel_cdclk.c   |   2 +-
 drivers/gpu/drm/i915/display/intel_display.c |  57 +--------
 drivers/gpu/drm/i915/display/intel_display.h |   2 -
 drivers/gpu/drm/i915/display/intel_dp.c      |  38 ++----
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  25 +---
 drivers/gpu/drm/i915/display/skl_watermark.c |   2 +-
 8 files changed, 145 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index a9a3f3715279d..38f77f83e7329 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -38,6 +38,7 @@
 #include "intel_atomic.h"
 #include "intel_cdclk.h"
 #include "intel_display_types.h"
+#include "intel_dp_mst.h"
 #include "intel_global_state.h"
 #include "intel_hdcp.h"
 #include "intel_psr.h"
@@ -613,3 +614,128 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+
+static int modeset_pipe(struct intel_atomic_state *state,
+			struct intel_crtc *crtc, const char *reason)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct intel_crtc_state *crtc_state;
+	int ret;
+
+	drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] Full modeset due to %s\n",
+		    crtc->base.base.id, crtc->base.name, reason);
+
+	crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	crtc_state->uapi.mode_changed = true;
+	crtc_state->update_pipe = false;
+
+	ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
+	if (ret)
+		return ret;
+
+	return intel_atomic_add_affected_planes(state, crtc);
+}
+
+/**
+ * intel_atomic_modeset_connector - add a connector to the atomic state, force modeset on its CRTC if any
+ * @state: atomic state
+ * @connector: connector to add the state for
+ * @reason: the reason why the connector needs to be added
+ *
+ * Add the @connector to the atomic state with its CRTC state and force a modeset
+ * on the CRTC if any.
+ *
+ * This function can be called only before computing the new plane states.
+ *
+ * Returns 0 in case of success, a negative error code on failure.
+ */
+int intel_atomic_modeset_connector(struct intel_atomic_state *state,
+				   struct intel_connector *connector, const char *reason)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct drm_connector_state *conn_state;
+
+	drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Full modeset due to %s\n",
+		    connector->base.base.id, connector->base.name, reason);
+
+	conn_state = drm_atomic_get_connector_state(&state->base, &connector->base);
+	if (IS_ERR(conn_state))
+		return PTR_ERR(conn_state);
+
+	if (!conn_state->crtc)
+		return 0;
+
+	return modeset_pipe(state, to_intel_crtc(conn_state->crtc), reason);
+}
+
+/**
+ * intel_atomic_modeset_pipe - add a CRTC to the atomic state, force modeset on it
+ * @state: atomic state
+ * @crtc: CRTC to add the state for
+ * @reason: the reason why the CRTC needs to be added
+ *
+ * Add @crtc to the atomic state with all its connector and plane dependencies and
+ * force modeset on it.
+ *
+ * This function can be called only before computing the new plane states.
+ *
+ * Returns 0 in case of success, a negative error code on failure.
+ */
+int intel_atomic_modeset_pipe(struct intel_atomic_state *state,
+			      struct intel_crtc *crtc, const char *reason)
+{
+	int ret;
+
+	ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
+	if (ret)
+		return ret;
+
+	return modeset_pipe(state, crtc, reason);
+}
+
+/**
+ * intel_atomic_modeset_all_pipes - add all active CRTCs to the atomic state, force modeset on them
+ * @state: atomic state
+ * @reason: the reason why the CRTCs need to be added
+ *
+ * Add all the CRTCs to the atomic state and if active also add their connector and plane
+ * dependencies and force a modeset on the CRTC.
+ *
+ * This function can be called only after computing the new plane states.
+ *
+ * Returns 0 in case of success, a negative error code on failure.
+ */
+int intel_atomic_modeset_all_pipes(struct intel_atomic_state *state,
+				   const char *reason)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct intel_crtc *crtc;
+
+	/*
+	 * Add all pipes to the state, and force
+	 * a modeset on all the active ones.
+	 */
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		struct intel_crtc_state *crtc_state;
+		int ret;
+
+		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		if (!crtc_state->hw.active ||
+		    intel_crtc_needs_modeset(crtc_state))
+			continue;
+
+		ret = intel_atomic_modeset_pipe(state, crtc, reason);
+		if (ret)
+			return ret;
+
+		crtc_state->update_planes |= crtc_state->active_planes;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index 1dc439983dd94..84295d388e3cb 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -56,4 +56,11 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 			       struct intel_crtc *intel_crtc,
 			       struct intel_crtc_state *crtc_state);
 
+int intel_atomic_modeset_pipe(struct intel_atomic_state *state,
+			      struct intel_crtc *crtc, const char *reason);
+int intel_atomic_modeset_connector(struct intel_atomic_state *state,
+				   struct intel_connector *connector, const char *reason);
+int intel_atomic_modeset_all_pipes(struct intel_atomic_state *state,
+				   const char *reason);
+
 #endif /* __INTEL_ATOMIC_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 0c107a38f9d00..e4d64f82b975a 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2919,7 +2919,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 	} else if (intel_cdclk_needs_modeset(&old_cdclk_state->actual,
 					     &new_cdclk_state->actual)) {
 		/* All pipes must be switched off while we change the cdclk. */
-		ret = intel_modeset_all_pipes(state, "CDCLK change");
+		ret = intel_atomic_modeset_all_pipes(state, "CDCLK change");
 		if (ret)
 			return ret;
 
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index d3994e2a7d636..b1e895b01c111 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5901,53 +5901,6 @@ intel_verify_planes(struct intel_atomic_state *state)
 			     plane_state->uapi.visible);
 }
 
-int intel_modeset_all_pipes(struct intel_atomic_state *state,
-			    const char *reason)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_crtc *crtc;
-
-	/*
-	 * Add all pipes to the state, and force
-	 * a modeset on all the active ones.
-	 */
-	for_each_intel_crtc(&dev_priv->drm, crtc) {
-		struct intel_crtc_state *crtc_state;
-		int ret;
-
-		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-
-		if (!crtc_state->hw.active ||
-		    intel_crtc_needs_modeset(crtc_state))
-			continue;
-
-		drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n",
-			    crtc->base.base.id, crtc->base.name, reason);
-
-		crtc_state->uapi.mode_changed = true;
-		crtc_state->update_pipe = false;
-
-		ret = drm_atomic_add_affected_connectors(&state->base,
-							 &crtc->base);
-		if (ret)
-			return ret;
-
-		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
-		if (ret)
-			return ret;
-
-		ret = intel_atomic_add_affected_planes(state, crtc);
-		if (ret)
-			return ret;
-
-		crtc_state->update_planes |= crtc_state->active_planes;
-	}
-
-	return 0;
-}
-
 void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -6667,15 +6620,7 @@ static int intel_bigjoiner_add_affected_crtcs(struct intel_atomic_state *state)
 	for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, modeset_pipes) {
 		int ret;
 
-		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
-
-		crtc_state->uapi.mode_changed = true;
-
-		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
-		if (ret)
-			return ret;
-
-		ret = intel_atomic_add_affected_planes(state, crtc);
+		ret = intel_atomic_modeset_pipe(state, crtc, "big joiner");
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index ef73730f32b09..1e64f94045cdc 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -650,8 +650,6 @@ void intel_modeset_driver_remove(struct drm_i915_private *i915);
 void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915);
 void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915);
 void intel_display_resume(struct drm_device *dev);
-int intel_modeset_all_pipes(struct intel_atomic_state *state,
-			    const char *reason);
 void intel_modeset_get_crtc_power_domains(struct intel_crtc_state *crtc_state,
 					  struct intel_power_domain_mask *old_domains);
 void intel_modeset_put_crtc_power_domains(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 478653e11f4da..441fbbfa2d008 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4906,35 +4906,17 @@ static int intel_modeset_tile_group(struct intel_atomic_state *state,
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct drm_connector_list_iter conn_iter;
-	struct drm_connector *connector;
+	struct intel_connector *connector;
 	int ret = 0;
 
 	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
-	drm_for_each_connector_iter(connector, &conn_iter) {
-		struct drm_connector_state *conn_state;
-		struct intel_crtc_state *crtc_state;
-		struct intel_crtc *crtc;
-
-		if (!connector->has_tile ||
-		    connector->tile_group->id != tile_group_id)
-			continue;
-
-		conn_state = drm_atomic_get_connector_state(&state->base,
-							    connector);
-		if (IS_ERR(conn_state)) {
-			ret = PTR_ERR(conn_state);
-			break;
-		}
-
-		crtc = to_intel_crtc(conn_state->crtc);
-
-		if (!crtc)
+	for_each_intel_connector_iter(connector, &conn_iter) {
+		if (!connector->base.has_tile ||
+		    connector->base.tile_group->id != tile_group_id)
 			continue;
 
-		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
-		crtc_state->uapi.mode_changed = true;
-
-		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
+		ret = intel_atomic_modeset_connector(state, connector,
+						     "connector tile group");
 		if (ret)
 			break;
 	}
@@ -4965,13 +4947,7 @@ static int intel_modeset_affected_transcoders(struct intel_atomic_state *state,
 		if (!(transcoders & BIT(crtc_state->cpu_transcoder)))
 			continue;
 
-		crtc_state->uapi.mode_changed = true;
-
-		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
-		if (ret)
-			return ret;
-
-		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
+		ret = intel_atomic_modeset_pipe(state, crtc, "port syncing");
 		if (ret)
 			return ret;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 800fa12a61d93..5790fabc39e7e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -434,35 +434,14 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
 
 	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
 	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
-		struct intel_digital_connector_state *conn_iter_state;
-		struct intel_crtc_state *crtc_state;
-		struct intel_crtc *crtc;
-
 		if (connector_iter->mst_port != connector->mst_port ||
 		    connector_iter == connector)
 			continue;
 
-		conn_iter_state = intel_atomic_get_digital_connector_state(state,
-									   connector_iter);
-		if (IS_ERR(conn_iter_state)) {
-			ret = PTR_ERR(conn_iter_state);
-			break;
-		}
-
-		if (!conn_iter_state->base.crtc)
-			continue;
-
-		crtc = to_intel_crtc(conn_iter_state->base.crtc);
-		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
-		if (IS_ERR(crtc_state)) {
-			ret = PTR_ERR(crtc_state);
-			break;
-		}
-
-		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
+		ret = intel_atomic_modeset_connector(state, connector_iter,
+						     "MST master transcoder");
 		if (ret)
 			break;
-		crtc_state->uapi.mode_changed = true;
 	}
 	drm_connector_list_iter_end(&connector_list_iter);
 
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index ae4e9e680c2e3..1a029fb01a039 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -2481,7 +2481,7 @@ skl_compute_ddb(struct intel_atomic_state *state)
 
 		if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus) {
 			/* TODO: Implement vblank synchronized MBUS joining changes */
-			ret = intel_modeset_all_pipes(state, "MBUS joining change");
+			ret = intel_atomic_modeset_all_pipes(state, "MBUS joining change");
 			if (ret)
 				return ret;
 		}
-- 
2.37.1

[Intel-gfx] [PATCH 7/9] drm/i915/dp_mst: Move getting the MST topology state earlier to connector check

[Imre Deak]

Atm, the MST topology state for modesetted CRTCs may get added only in
the encoder's compute config function. To make this more consistent with
other encoders add these states already earlier in the connector atomic
check function and just get the new MST state in the encoder's compute
config function which shouldn't fail.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 23 ++++++++++++++-------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5790fabc39e7e..6726fd36723d7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -66,9 +66,9 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 	int bpp, slots = -EINVAL;
 	int ret = 0;
 
-	mst_state = drm_atomic_get_mst_topology_state(state, &intel_dp->mst_mgr);
-	if (IS_ERR(mst_state))
-		return PTR_ERR(mst_state);
+	mst_state = drm_atomic_get_new_mst_topology_state(state, &intel_dp->mst_mgr);
+	if (drm_WARN_ON(&i915->drm, !mst_state))
+		return -EINVAL;
 
 	crtc_state->lane_count = limits->max_lane_count;
 	crtc_state->port_clock = limits->max_rate;
@@ -254,11 +254,9 @@ static int intel_dp_mst_update_slots(struct intel_encoder *encoder,
 	u8 link_coding_cap = intel_dp_is_uhbr(crtc_state) ?
 		DP_CAP_ANSI_128B132B : DP_CAP_ANSI_8B10B;
 
-	topology_state = drm_atomic_get_mst_topology_state(conn_state->state, mgr);
-	if (IS_ERR(topology_state)) {
-		drm_dbg_kms(&i915->drm, "slot update failed\n");
-		return PTR_ERR(topology_state);
-	}
+	topology_state = drm_atomic_get_new_mst_topology_state(conn_state->state, mgr);
+	if (drm_WARN_ON(&i915->drm, !topology_state))
+		return -EINVAL;
 
 	drm_dp_mst_update_slots(topology_state, link_coding_cap);
 
@@ -465,6 +463,15 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
+	if (intel_connector_needs_modeset(state, &intel_connector->base)) {
+		struct drm_dp_mst_topology_state *mst_state;
+
+		mst_state = drm_atomic_get_mst_topology_state(&state->base,
+							      &intel_connector->mst_port->mst_mgr);
+		if (IS_ERR(mst_state))
+			return PTR_ERR(mst_state);
+	}
+
 	return drm_dp_atomic_release_time_slots(&state->base,
 						&intel_connector->mst_port->mst_mgr,
 						intel_connector->port);
-- 
2.37.1

[PATCH 8/9] 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: 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 619f616d69e20..7597d27db4fa6 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4834,6 +4834,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

[Intel-gfx] [PATCH 8/9] 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: 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 619f616d69e20..7597d27db4fa6 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4834,6 +4834,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

[Intel-gfx] [PATCH 9/9] drm/i915/dp_mst: Verify the MST state of modesetted outputs

[Imre Deak]

Verify the MST state after disabling/enabling outputs during an atomic
commit.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  |  4 ++-
 drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
 drivers/gpu/drm/i915/display/intel_dp.h       |  1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 33 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.h   |  1 +
 .../drm/i915/display/intel_modeset_verify.c   |  2 ++
 6 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b1e895b01c111..8efa3a29faafc 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7537,8 +7537,10 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	intel_check_cpu_fifo_underruns(dev_priv);
 	intel_check_pch_fifo_underruns(dev_priv);
 
-	if (state->modeset)
+	if (state->modeset) {
 		intel_verify_planes(state);
+		intel_dp_mst_verify_state(state);
+	}
 
 	intel_sagv_post_plane_update(state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 441fbbfa2d008..a87e3d04e5cb8 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4086,7 +4086,7 @@ static int intel_dp_prep_link_retrain(struct intel_dp *intel_dp,
 	return ret;
 }
 
-static bool intel_dp_is_connected(struct intel_dp *intel_dp)
+bool intel_dp_is_connected(struct intel_dp *intel_dp)
 {
 	struct intel_connector *connector = intel_dp->attached_connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index ef39e4f7a329e..1294384840190 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -102,6 +102,7 @@ void intel_read_dp_sdp(struct intel_encoder *encoder,
 		       struct intel_crtc_state *crtc_state,
 		       unsigned int type);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+bool intel_dp_is_connected(struct intel_dp *intel_dp);
 int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
 u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 				u32 link_clock, u32 lane_count,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 6726fd36723d7..821d149151ef3 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1252,3 +1252,36 @@ int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
 
 	return 0;
 }
+
+/**
+ * intel_dp_mst_verify_state - Verify the MST state for all connectors in the atomic state
+ * @state: atomic state
+ *
+ * Verify the MST SW and sink state for all modesetted MST connector in @state.
+ */
+void intel_dp_mst_verify_state(struct intel_atomic_state *state)
+{
+	struct drm_connector *_connector;
+	struct drm_connector_state *conn_state;
+	u32 encoder_mask = 0;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
+		struct intel_connector *connector = to_intel_connector(_connector);
+		struct intel_encoder *encoder;
+
+		if (!connector->mst_port ||
+		    !intel_connector_needs_modeset(state, &connector->base))
+			continue;
+
+		encoder = &dp_to_dig_port(connector->mst_port)->base;
+		if (encoder_mask & drm_encoder_index(&encoder->base))
+			continue;
+
+		encoder_mask |= drm_encoder_index(&encoder->base);
+
+		drm_dp_mst_verify_payload_state(&state->base,
+						&connector->mst_port->mst_mgr,
+						intel_dp_is_connected(connector->mst_port));
+	}
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index 0cd05a9a78a25..7f77e471ddfc3 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -20,5 +20,6 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
 int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
 					     struct intel_crtc *crtc);
+void intel_dp_mst_verify_state(struct intel_atomic_state *state);
 
 #endif /* __INTEL_DP_MST_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
index 842d70f0dfd2a..45f0d9789ef8e 100644
--- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
+++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
@@ -13,6 +13,7 @@
 #include "intel_crtc_state_dump.h"
 #include "intel_display.h"
 #include "intel_display_types.h"
+#include "intel_dp_mst.h"
 #include "intel_fdi.h"
 #include "intel_modeset_verify.h"
 #include "intel_snps_phy.h"
@@ -244,4 +245,5 @@ void intel_modeset_verify_disabled(struct drm_i915_private *dev_priv,
 	verify_encoder_state(dev_priv, state);
 	verify_connector_state(state, NULL);
 	intel_shared_dpll_verify_disabled(dev_priv);
+	intel_dp_mst_verify_state(state);
 }
-- 
2.37.1

[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/dp_mst: Fix MST payload removal during output disabling

[Patchwork]

== Series Details ==

Series: drm/i915/dp_mst: Fix MST payload removal during output disabling
URL   : https://patchwork.freedesktop.org/series/113329/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  LD [M]  drivers/gpu/drm/i915/i915.o
  LD [M]  drivers/gpu/drm/i915/kvmgt.o
  HDRTEST drivers/gpu/drm/i915/display/intel_dp_mst.h
In file included from <command-line>:
./drivers/gpu/drm/i915/display/intel_dp_mst.h:22:18: error: ‘struct intel_crtc’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
   22 |           struct intel_crtc *crtc);
      |                  ^~~~~~~~~~
./drivers/gpu/drm/i915/display/intel_dp_mst.h:21:53: error: ‘struct intel_atomic_state’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
   21 | int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
      |                                                     ^~~~~~~~~~~~~~~~~~
./drivers/gpu/drm/i915/display/intel_dp_mst.h:23:39: error: ‘struct intel_atomic_state’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
   23 | void intel_dp_mst_verify_state(struct intel_atomic_state *state);
      |                                       ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[5]: *** [drivers/gpu/drm/i915/Makefile:380: drivers/gpu/drm/i915/display/intel_dp_mst.hdrtest] Error 1
make[4]: *** [scripts/Makefile.build:504: drivers/gpu/drm/i915] Error 2
make[3]: *** [scripts/Makefile.build:504: drivers/gpu/drm] Error 2
make[2]: *** [scripts/Makefile.build:504: drivers/gpu] Error 2
make[1]: *** [scripts/Makefile.build:504: drivers] Error 2
make: *** [Makefile:2021: .] Error 2

[PATCH v2 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Imre Deak]

Add the MST topology for a CRTC to the atomic state if the driver
needs to force a modeset on the CRTC after the encoder compute config
functions are called.

Later the MST encoder's disable hook also adds the state, but that isn't
guaranteed to work (since in that hook getting the state may fail, which
can't be handled there). This should fix that, while a later patch fixes
the use of the MST state in the disable hook.

v2: Add missing forward struct declartions, caught by hdrtest.

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  4 +++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 717ca3d7890d3..d3994e2a7d636 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
 		if (ret)
 			return ret;
 
+		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
+		if (ret)
+			return ret;
+
 		ret = intel_atomic_add_affected_planes(state, crtc);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 8b0e4defa3f10..ba29c294b7c1b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
 	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
 	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
 }
+
+/**
+ * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
+ * @state: atomic state
+ * @crtc: CRTC
+ *
+ * Add the MST topology state for @crtc to @state.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc)
+{
+	struct drm_connector *_connector;
+	struct drm_connector_state *conn_state;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
+		struct drm_dp_mst_topology_state *mst_state;
+		struct intel_connector *connector = to_intel_connector(_connector);
+
+		if (conn_state->crtc != &crtc->base)
+			continue;
+
+		if (!connector->mst_port)
+			continue;
+
+		mst_state = drm_atomic_get_mst_topology_state(&state->base,
+							      &connector->mst_port->mst_mgr);
+		if (IS_ERR(mst_state))
+			return PTR_ERR(mst_state);
+
+		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index f7301de6cdfb3..f1815bb722672 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -8,6 +8,8 @@
 
 #include <linux/types.h>
 
+struct intel_atomic_state;
+struct intel_crtc;
 struct intel_crtc_state;
 struct intel_digital_port;
 struct intel_dp;
@@ -18,5 +20,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
 bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.37.1

[Intel-gfx] [PATCH v2 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Imre Deak]

Add the MST topology for a CRTC to the atomic state if the driver
needs to force a modeset on the CRTC after the encoder compute config
functions are called.

Later the MST encoder's disable hook also adds the state, but that isn't
guaranteed to work (since in that hook getting the state may fail, which
can't be handled there). This should fix that, while a later patch fixes
the use of the MST state in the disable hook.

v2: Add missing forward struct declartions, caught by hdrtest.

Cc: Lyude Paul <lyude@redhat.com>
Cc: stable@vger.kernel.org # 6.1
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  4 +++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 717ca3d7890d3..d3994e2a7d636 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
 		if (ret)
 			return ret;
 
+		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
+		if (ret)
+			return ret;
+
 		ret = intel_atomic_add_affected_planes(state, crtc);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 8b0e4defa3f10..ba29c294b7c1b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
 	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
 	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
 }
+
+/**
+ * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
+ * @state: atomic state
+ * @crtc: CRTC
+ *
+ * Add the MST topology state for @crtc to @state.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc)
+{
+	struct drm_connector *_connector;
+	struct drm_connector_state *conn_state;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
+		struct drm_dp_mst_topology_state *mst_state;
+		struct intel_connector *connector = to_intel_connector(_connector);
+
+		if (conn_state->crtc != &crtc->base)
+			continue;
+
+		if (!connector->mst_port)
+			continue;
+
+		mst_state = drm_atomic_get_mst_topology_state(&state->base,
+							      &connector->mst_port->mst_mgr);
+		if (IS_ERR(mst_state))
+			return PTR_ERR(mst_state);
+
+		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index f7301de6cdfb3..f1815bb722672 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -8,6 +8,8 @@
 
 #include <linux/types.h>
 
+struct intel_atomic_state;
+struct intel_crtc;
 struct intel_crtc_state;
 struct intel_digital_port;
 struct intel_dp;
@@ -18,5 +20,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
 bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
+int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
+					     struct intel_crtc *crtc);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.37.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/dp_mst: Fix MST payload removal during output disabling (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/dp_mst: Fix MST payload removal during output disabling (rev2)
URL   : https://patchwork.freedesktop.org/series/113329/
State : warning

== Summary ==

Error: dim checkpatch failed
3f7d65acc205 drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs
6ef41ca2e79a drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()
560a83a7af3f drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()
8f01717d8d3f drm/i915/dp_mst: Fix payload removal during output disabling
6fa9587afac7 drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()
c438e5430859 drm/i915: Factor out helpers for modesetting CRTCs and connectors
-:63: WARNING:LONG_LINE_COMMENT: line length of 105 exceeds 100 columns
#63: FILE: drivers/gpu/drm/i915/display/intel_atomic.c:643:
+ * intel_atomic_modeset_connector - add a connector to the atomic state, force modeset on its CRTC if any

total: 0 errors, 1 warnings, 0 checks, 331 lines checked
555f84d3e147 drm/i915/dp_mst: Move getting the MST topology state earlier to connector check
4aba29fbb308 drm/display/dp_mst: Add a helper to verify the MST payload state
-:154: WARNING:LONG_LINE_COMMENT: line length of 108 exceeds 100 columns
#154: FILE: drivers/gpu/drm/display/drm_dp_mst_topology.c:4969:
+ * drm_dp_mst_verify_payload_state - Verify the atomic state for payloads and the related sink payload table

total: 0 errors, 1 warnings, 0 checks, 196 lines checked
065539bb668b drm/i915/dp_mst: Verify the MST state of modesetted outputs

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/dp_mst: Fix MST payload removal during output disabling (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5299 bytes --]

== Series Details ==

Series: drm/i915/dp_mst: Fix MST payload removal during output disabling (rev2)
URL   : https://patchwork.freedesktop.org/series/113329/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12640 -> Patchwork_113329v2
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/index.html

Participating hosts (37 -> 32)
------------------------------

  Additional (1): fi-bsw-kefka 
  Missing    (6): fi-hsw-4770 fi-pnv-d510 fi-ivb-3770 fi-elk-e7500 fi-skl-6700k2 fi-snb-2600 

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - fi-bsw-nick:        NOTRUN -> [SKIP][1] ([fdo#109271]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/fi-bsw-nick/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-bsw-kefka:       NOTRUN -> [SKIP][2] ([fdo#109271]) +26 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/fi-bsw-kefka/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - {bat-adlp-9}:       [DMESG-WARN][3] -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/bat-adlp-9/igt@gem_exec_suspend@basic-s0@smem.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/bat-adlp-9/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-nick:        [INCOMPLETE][5] ([i915#7911]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/fi-bsw-nick/igt@i915_selftest@live@execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/fi-bsw-nick/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@migrate:
    - {bat-dg2-11}:       [DMESG-WARN][7] ([i915#7699]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/bat-dg2-11/igt@i915_selftest@live@migrate.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor@atomic-transitions-varying-size:
    - fi-bsw-n3050:       [FAIL][9] ([i915#2346]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/fi-bsw-n3050/igt@kms_cursor_legacy@basic-flip-after-cursor@atomic-transitions-varying-size.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/fi-bsw-n3050/igt@kms_cursor_legacy@basic-flip-after-cursor@atomic-transitions-varying-size.html

  
#### Warnings ####

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [FAIL][11] ([fdo#103375]) -> [INCOMPLETE][12] ([i915#4817])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911


Build changes
-------------

  * Linux: CI_DRM_12640 -> Patchwork_113329v2

  CI-20190529: 20190529
  CI_DRM_12640: cc7783f223ac644092bb8788f0750fc5c68aa00e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7136: 31b6af91747ad8c705399c9006cdb81cb1864146 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113329v2: cc7783f223ac644092bb8788f0750fc5c68aa00e @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

0df0c78ffaa0 drm/i915/dp_mst: Verify the MST state of modesetted outputs
7ee016e8e926 drm/display/dp_mst: Add a helper to verify the MST payload state
6aa79aba8b9d drm/i915/dp_mst: Move getting the MST topology state earlier to connector check
a137bc9cdd61 drm/i915: Factor out helpers for modesetting CRTCs and connectors
c5253fcf27b5 drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()
b5e560ea5fbb drm/i915/dp_mst: Fix payload removal during output disabling
41b769e22aea drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()
531938b86df9 drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()
c3ae029adaa1 drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/index.html

[-- Attachment #2: Type: text/html, Size: 5929 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/dp_mst: Fix MST payload removal during output disabling (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 20784 bytes --]

== Series Details ==

Series: drm/i915/dp_mst: Fix MST payload removal during output disabling (rev2)
URL   : https://patchwork.freedesktop.org/series/113329/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12640_full -> Patchwork_113329v2_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/index.html

Participating hosts (12 -> 11)
------------------------------

  Additional (1): shard-rkl0 
  Missing    (2): pig-skl-6260u pig-kbl-iris 

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace@vecs0:
    - shard-glk:          [PASS][1] -> [FAIL][2] ([i915#2842])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-glk4/igt@gem_exec_fair@basic-pace@vecs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk4/igt@gem_exec_fair@basic-pace@vecs0.html

  * igt@kms_cursor_legacy@flip-vs-cursor-busy-crc-legacy:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#7513])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-glk3/igt@kms_cursor_legacy@flip-vs-cursor-busy-crc-legacy.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk1/igt@kms_cursor_legacy@flip-vs-cursor-busy-crc-legacy.html

  * igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions:
    - shard-glk:          [PASS][5] -> [FAIL][6] ([i915#2346])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk1/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html

  * igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([i915#2122])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-glk9/igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk3/igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2.html

  * igt@perf@stress-open-close:
    - shard-glk:          [PASS][9] -> [INCOMPLETE][10] ([i915#5213])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-glk6/igt@perf@stress-open-close.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk9/igt@perf@stress-open-close.html

  * igt@runner@aborted:
    - shard-glk:          NOTRUN -> [FAIL][11] ([i915#4312])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk9/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@fbdev@unaligned-read:
    - {shard-rkl}:        [SKIP][12] ([i915#2582]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-5/igt@fbdev@unaligned-read.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@fbdev@unaligned-read.html

  * igt@feature_discovery@psr1:
    - {shard-rkl}:        [SKIP][14] ([i915#658]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-2/igt@feature_discovery@psr1.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@feature_discovery@psr1.html

  * igt@gem_ctx_persistence@legacy-engines-hang@blt:
    - {shard-rkl}:        [SKIP][16] ([i915#6252]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-5/igt@gem_ctx_persistence@legacy-engines-hang@blt.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-4/igt@gem_ctx_persistence@legacy-engines-hang@blt.html

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - {shard-rkl}:        [SKIP][18] ([fdo#109313]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-1/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-5/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html

  * igt@gem_exec_reloc@basic-write-read-noreloc:
    - {shard-rkl}:        [SKIP][20] ([i915#3281]) -> [PASS][21] +6 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-1/igt@gem_exec_reloc@basic-write-read-noreloc.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-5/igt@gem_exec_reloc@basic-write-read-noreloc.html

  * igt@gem_set_tiling_vs_pwrite:
    - {shard-rkl}:        [SKIP][22] ([i915#3282]) -> [PASS][23] +4 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-3/igt@gem_set_tiling_vs_pwrite.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-5/igt@gem_set_tiling_vs_pwrite.html

  * igt@gen9_exec_parse@bb-chained:
    - {shard-rkl}:        [SKIP][24] ([i915#2527]) -> [PASS][25] +2 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-3/igt@gen9_exec_parse@bb-chained.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-5/igt@gen9_exec_parse@bb-chained.html

  * igt@i915_module_load@reload-no-display:
    - {shard-tglu}:       [DMESG-WARN][26] -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-tglu-8/igt@i915_module_load@reload-no-display.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-tglu-7/igt@i915_module_load@reload-no-display.html

  * igt@i915_pipe_stress@stress-xrgb8888-untiled:
    - {shard-rkl}:        [SKIP][28] ([i915#4098]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-2/igt@i915_pipe_stress@stress-xrgb8888-untiled.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@i915_pipe_stress@stress-xrgb8888-untiled.html

  * igt@i915_pm_rpm@fences:
    - {shard-rkl}:        [SKIP][30] ([i915#1849]) -> [PASS][31] +1 similar issue
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-2/igt@i915_pm_rpm@fences.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@i915_pm_rpm@fences.html

  * igt@i915_pm_rpm@modeset-lpsp-stress-no-wait:
    - {shard-rkl}:        [SKIP][32] ([i915#1397]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-2/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-0:
    - {shard-rkl}:        [SKIP][34] ([i915#1845] / [i915#4098]) -> [PASS][35] +23 similar issues
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-2/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html

  * igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size:
    - shard-glk:          [FAIL][36] ([i915#2346]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-glk1/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt:
    - {shard-rkl}:        [SKIP][38] ([i915#1849] / [i915#4098]) -> [PASS][39] +13 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-3/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt.html

  * igt@kms_psr@cursor_mmap_gtt:
    - {shard-rkl}:        [SKIP][40] ([i915#1072]) -> [PASS][41] +2 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-3/igt@kms_psr@cursor_mmap_gtt.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@kms_psr@cursor_mmap_gtt.html

  * igt@kms_universal_plane@cursor-fb-leak-pipe-b:
    - {shard-rkl}:        [SKIP][42] ([i915#1845] / [i915#4070] / [i915#4098]) -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-2/igt@kms_universal_plane@cursor-fb-leak-pipe-b.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@kms_universal_plane@cursor-fb-leak-pipe-b.html

  * igt@prime_vgem@basic-fence-flip:
    - {shard-rkl}:        [SKIP][44] ([fdo#109295] / [i915#3708] / [i915#4098]) -> [PASS][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-3/igt@prime_vgem@basic-fence-flip.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-6/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-write:
    - {shard-rkl}:        [SKIP][46] ([fdo#109295] / [i915#3291] / [i915#3708]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12640/shard-rkl-3/igt@prime_vgem@basic-write.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/shard-rkl-5/igt@prime_vgem@basic-write.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110542]: https://bugs.freedesktop.org/show_bug.cgi?id=110542
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
  [fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2232]: https://gitlab.freedesktop.org/drm/intel/issues/2232
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3323]: https://gitlab.freedesktop.org/drm/intel/issues/3323
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
  [i915#3536]: https://gitlab.freedesktop.org/drm/intel/issues/3536
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3639]: https://gitlab.freedesktop.org/drm/intel/issues/3639
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
  [i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
  [i915#3825]: https://gitlab.freedesktop.org/drm/intel/issues/3825
  [i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3938]: https://gitlab.freedesktop.org/drm/intel/issues/3938
  [i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#426]: https://gitlab.freedesktop.org/drm/intel/issues/426
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#433]: https://gitlab.freedesktop.org/drm/intel/issues/433
  [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
  [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
  [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4958]: https://gitlab.freedesktop.org/drm/intel/issues/4958
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5213]: https://gitlab.freedesktop.org/drm/intel/issues/5213
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227
  [i915#6245]: https://gitlab.freedesktop.org/drm/intel/issues/6245
  [i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
  [i915#6252]: https://gitlab.freedesktop.org/drm/intel/issues/6252
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#6334]: https://gitlab.freedesktop.org/drm/intel/issues/6334
  [i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
  [i915#6403]: https://gitlab.freedesktop.org/drm/intel/issues/6403
  [i915#6463]: https://gitlab.freedesktop.org/drm/intel/issues/6463
  [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128
  [i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294
  [i915#7513]: https://gitlab.freedesktop.org/drm/intel/issues/7513
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7707]: https://gitlab.freedesktop.org/drm/intel/issues/7707
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7949]: https://gitlab.freedesktop.org/drm/intel/issues/7949


Build changes
-------------

  * Linux: CI_DRM_12640 -> Patchwork_113329v2
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_12640: cc7783f223ac644092bb8788f0750fc5c68aa00e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7136: 31b6af91747ad8c705399c9006cdb81cb1864146 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113329v2: cc7783f223ac644092bb8788f0750fc5c68aa00e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113329v2/index.html

[-- Attachment #2: Type: text/html, Size: 13562 bytes --]

Re: [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:45PM +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,

Why is that? In fact couldn't we just clear both start_slot and
pbn to 0 here?

> 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: 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
> 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 | 22 ++++++++++---------
>  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;

So I take it the only reason we even have that is the copy being done in
drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
why any of that is being done tbh. If the new payload hasn't been
allocated yet then why can't its vc_start_slots just stay at -1
until that time?

This whole thing feels a bit weird since the payload table really isn't
your normal atomic state that is computed ahead of time. Instead it just
gets built up on as we go during the actual commit. So not really sure
why we're even tracking it in atomic state...

>  
>  	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;
>  }
>  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 ba29c294b7c1b..5f7bcb5c14847 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

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:45PM +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,

Why is that? In fact couldn't we just clear both start_slot and
pbn to 0 here?

> 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: 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
> 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 | 22 ++++++++++---------
>  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;

So I take it the only reason we even have that is the copy being done in
drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
why any of that is being done tbh. If the new payload hasn't been
allocated yet then why can't its vc_start_slots just stay at -1
until that time?

This whole thing feels a bit weird since the payload table really isn't
your normal atomic state that is computed ahead of time. Instead it just
gets built up on as we go during the actual commit. So not really sure
why we're even tracking it in atomic state...

>  
>  	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;
>  }
>  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 ba29c294b7c1b..5f7bcb5c14847 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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:45PM +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,

Why is that? In fact couldn't we just clear both start_slot and
pbn to 0 here?

> 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: 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
> 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 | 22 ++++++++++---------
>  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;

So I take it the only reason we even have that is the copy being done in
drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
why any of that is being done tbh. If the new payload hasn't been
allocated yet then why can't its vc_start_slots just stay at -1
until that time?

This whole thing feels a bit weird since the payload table really isn't
your normal atomic state that is computed ahead of time. Instead it just
gets built up on as we go during the actual commit. So not really sure
why we're even tracking it in atomic state...

>  
>  	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;
>  }
>  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 ba29c294b7c1b..5f7bcb5c14847 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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Thu, Jan 26, 2023 at 07:37:04PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:45PM +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,
> 
> Why is that? In fact couldn't we just clear both start_slot and
> pbn to 0 here?

OK, so it has to be the "current" start slot. Which in this case
means new_payload since that's what's housed in the new topolpogy state
which is the one getting mutated when streams are being removed/added.

Confusing, but seems correct
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > 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: 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
> > 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 | 22 ++++++++++---------
> >  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> > @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
> 
> So I take it the only reason we even have that is the copy being done in
> drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
> why any of that is being done tbh. If the new payload hasn't been
> allocated yet then why can't its vc_start_slots just stay at -1
> until that time?
> 
> This whole thing feels a bit weird since the payload table really isn't
> your normal atomic state that is computed ahead of time. Instead it just
> gets built up on as we go during the actual commit. So not really sure
> why we're even tracking it in atomic state...
> 
> >  
> >  	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;
> >  }
> >  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 ba29c294b7c1b..5f7bcb5c14847 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
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Ville Syrjälä]

On Thu, Jan 26, 2023 at 07:37:04PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:45PM +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,
> 
> Why is that? In fact couldn't we just clear both start_slot and
> pbn to 0 here?

OK, so it has to be the "current" start slot. Which in this case
means new_payload since that's what's housed in the new topolpogy state
which is the one getting mutated when streams are being removed/added.

Confusing, but seems correct
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > 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: 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
> > 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 | 22 ++++++++++---------
> >  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> > @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
> 
> So I take it the only reason we even have that is the copy being done in
> drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
> why any of that is being done tbh. If the new payload hasn't been
> allocated yet then why can't its vc_start_slots just stay at -1
> until that time?
> 
> This whole thing feels a bit weird since the payload table really isn't
> your normal atomic state that is computed ahead of time. Instead it just
> gets built up on as we go during the actual commit. So not really sure
> why we're even tracking it in atomic state...
> 
> >  
> >  	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;
> >  }
> >  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 ba29c294b7c1b..5f7bcb5c14847 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
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Ville Syrjälä]

On Thu, Jan 26, 2023 at 11:13:10AM +0200, Imre Deak wrote:
> Add the MST topology for a CRTC to the atomic state if the driver
> needs to force a modeset on the CRTC after the encoder compute config
> functions are called.
> 
> Later the MST encoder's disable hook also adds the state, but that isn't
> guaranteed to work (since in that hook getting the state may fail, which
> can't be handled there). This should fix that, while a later patch fixes
> the use of the MST state in the disable hook.
> 
> v2: Add missing forward struct declartions, caught by hdrtest.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  4 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 717ca3d7890d3..d3994e2a7d636 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  		if (ret)
>  			return ret;
>  
> +		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
> +		if (ret)
> +			return ret;
> +
>  		ret = intel_atomic_add_affected_planes(state, crtc);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 8b0e4defa3f10..ba29c294b7c1b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
>  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
>  }
> +
> +/**
> + * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
> + * @state: atomic state
> + * @crtc: CRTC
> + *
> + * Add the MST topology state for @crtc to @state.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
> +					     struct intel_crtc *crtc)
> +{
> +	struct drm_connector *_connector;
> +	struct drm_connector_state *conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
> +		struct drm_dp_mst_topology_state *mst_state;
> +		struct intel_connector *connector = to_intel_connector(_connector);
> +
> +		if (conn_state->crtc != &crtc->base)
> +			continue;
> +
> +		if (!connector->mst_port)
> +			continue;
> +
> +		mst_state = drm_atomic_get_mst_topology_state(&state->base,
> +							      &connector->mst_port->mst_mgr);
> +		if (IS_ERR(mst_state))
> +			return PTR_ERR(mst_state);
> +
> +		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f7301de6cdfb3..f1815bb722672 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -8,6 +8,8 @@
>  
>  #include <linux/types.h>
>  
> +struct intel_atomic_state;
> +struct intel_crtc;
>  struct intel_crtc_state;
>  struct intel_digital_port;
>  struct intel_dp;
> @@ -18,5 +20,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
>  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
> +int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
> +					     struct intel_crtc *crtc);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> 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().
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
>  include/drm/display/drm_dp_mst_helper.h       |  3 ++
>  2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -5362,18 +5362,43 @@ 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 *priv_state =

I would include 'old_' in the variable name to remind the reader what it
is.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> +
> +	return priv_state ? to_dp_mst_topology_state(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 *
> 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

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> 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().
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
>  include/drm/display/drm_dp_mst_helper.h       |  3 ++
>  2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -5362,18 +5362,43 @@ 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 *priv_state =

I would include 'old_' in the variable name to remind the reader what it
is.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> +
> +	return priv_state ? to_dp_mst_topology_state(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 *
> 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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> 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().
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
>  include/drm/display/drm_dp_mst_helper.h       |  3 ++
>  2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -5362,18 +5362,43 @@ 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 *priv_state =

I would include 'old_' in the variable name to remind the reader what it
is.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> +
> +	return priv_state ? to_dp_mst_topology_state(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 *
> 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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 4/9] drm/i915/dp_mst: Fix payload removal during output disabling

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:47PM +0200, Imre Deak wrote:
> Use the correct old/new topology and payload states in
> intel_mst_disable_dp(). So far drm_atomic_get_mst_topology_state() it
> used returned either the old state, in case the state was added already
> earlier during the atomic check phase or otherwise the new state (but
> the latter could fail, which can't be handled in the enable/disable
> hooks). After the first patch in the patchset, the state should always
> get added already during the check phase, so here we can get the
> old/new states without a failure.
> 
> drm_dp_remove_payload() should use time_slots from the old payload state
> and vc_start_slot in the new one. It should update the new payload
> states to reflect the sink's current payload table after the payload is
> removed. Pass the new topology state and the old and new payload states
> accordingly.
> 
> This also fixes a problem where the payload allocations for multiple MST
> streams on the same link got inconsistent after a few commits, as
> during payload removal the old instead of the new payload state got
> updated, so the subsequent enabling sequence and commits used a stale
> payload state.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 5f7bcb5c14847..800fa12a61d93 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -524,10 +524,14 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
>  	struct intel_dp *intel_dp = &dig_port->dp;
>  	struct intel_connector *connector =
>  		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_dp_mst_topology_state *old_mst_state =
> +		drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> +	struct drm_dp_mst_topology_state *new_mst_state =
> +		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> +	struct drm_dp_mst_atomic_payload *old_payload =
> +		drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
> +	struct drm_dp_mst_atomic_payload *new_payload =
> +		drm_atomic_get_mst_payload_state(new_mst_state, connector->port);

old states could be const no?

>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  
>  	drm_dbg_kms(&i915->drm, "active links %d\n",
> @@ -535,8 +539,8 @@ 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,
> -			      payload, payload);
> +	drm_dp_remove_payload(&intel_dp->mst_mgr, new_mst_state,

Right that one needs to be 'new' to update the start_slots

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +			      old_payload, new_payload);
>  
>  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
>  }
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:33:42PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 26, 2023 at 07:37:04PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 25, 2023 at 01:48:45PM +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,
> > 
> > Why is that? In fact couldn't we just clear both start_slot and
> > pbn to 0 here?

The DP spec requires sending the actual start slot, even though the hubs
I checked ignored it and deleted all the allocation of the given VCPI
whatever start slot was used. Imo we should still not depend on this.

> OK, so it has to be the "current" start slot. Which in this case
> means new_payload since that's what's housed in the new topolpogy state
> which is the one getting mutated when streams are being removed/added.

Yes.

> Confusing, but seems correct
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > 
> > > 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: 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
> > > 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 | 22 ++++++++++---------
> > >  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> > > @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
> > 
> > So I take it the only reason we even have that is the copy being done in
> > drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
> > why any of that is being done tbh. If the new payload hasn't been
> > allocated yet then why can't its vc_start_slots just stay at -1
> > until that time?
> > 
> > This whole thing feels a bit weird since the payload table really isn't
> > your normal atomic state that is computed ahead of time. Instead it just
> > gets built up on as we go during the actual commit. So not really sure
> > why we're even tracking it in atomic state...
> > 
> > >  
> > >  	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;
> > >  }
> > >  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 ba29c294b7c1b..5f7bcb5c14847 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 2/9] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:33:42PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 26, 2023 at 07:37:04PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 25, 2023 at 01:48:45PM +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,
> > 
> > Why is that? In fact couldn't we just clear both start_slot and
> > pbn to 0 here?

The DP spec requires sending the actual start slot, even though the hubs
I checked ignored it and deleted all the allocation of the given VCPI
whatever start slot was used. Imo we should still not depend on this.

> OK, so it has to be the "current" start slot. Which in this case
> means new_payload since that's what's housed in the new topolpogy state
> which is the one getting mutated when streams are being removed/added.

Yes.

> Confusing, but seems correct
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > 
> > > 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: 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
> > > 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 | 22 ++++++++++---------
> > >  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, 19 insertions(+), 14 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 6994c9a1ed858..fed4ce6821161 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
> > > @@ -179,7 +179,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 5861b0a6247bc..ebf6e31e156e0 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,33 +3351,34 @@ 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;
> > 
> > So I take it the only reason we even have that is the copy being done in
> > drm_dp_mst_atomic_wait_for_dependencies()? I don't really understand
> > why any of that is being done tbh. If the new payload hasn't been
> > allocated yet then why can't its vc_start_slots just stay at -1
> > until that time?
> > 
> > This whole thing feels a bit weird since the payload table really isn't
> > your normal atomic state that is computed ahead of time. Instead it just
> > gets built up on as we go during the actual commit. So not really sure
> > why we're even tracking it in atomic state...
> > 
> > >  
> > >  	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;
> > >  }
> > >  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 ba29c294b7c1b..5f7bcb5c14847 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:36:20PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> > 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().
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
> >  include/drm/display/drm_dp_mst_helper.h       |  3 ++
> >  2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -5362,18 +5362,43 @@ 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 *priv_state =
> 
> I would include 'old_' in the variable name to remind the reader what it
> is.

Ok, will change it.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> > +
> > +	return priv_state ? to_dp_mst_topology_state(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 *
> > 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
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:36:20PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> > 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().
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
> >  include/drm/display/drm_dp_mst_helper.h       |  3 ++
> >  2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -5362,18 +5362,43 @@ 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 *priv_state =
> 
> I would include 'old_' in the variable name to remind the reader what it
> is.

Ok, will change it.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> > +
> > +	return priv_state ? to_dp_mst_topology_state(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 *
> > 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
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 3/9] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state()

[Imre Deak]

On Thu, Jan 26, 2023 at 08:36:20PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:46PM +0200, Imre Deak wrote:
> > 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().
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 29 +++++++++++++++++--
> >  include/drm/display/drm_dp_mst_helper.h       |  3 ++
> >  2 files changed, 30 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 ebf6e31e156e0..81cc0c3b1e000 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -5362,18 +5362,43 @@ 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 *priv_state =
> 
> I would include 'old_' in the variable name to remind the reader what it
> is.

Ok, will change it.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +		drm_atomic_get_old_private_obj_state(state, &mgr->base);
> > +
> > +	return priv_state ? to_dp_mst_topology_state(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 *
> > 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
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH v2 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Lyude Paul]

Hi! should have a chance to look at this at the start of next week

On Thu, 2023-01-26 at 11:13 +0200, Imre Deak wrote:
> Add the MST topology for a CRTC to the atomic state if the driver
> needs to force a modeset on the CRTC after the encoder compute config
> functions are called.
> 
> Later the MST encoder's disable hook also adds the state, but that isn't
> guaranteed to work (since in that hook getting the state may fail, which
> can't be handled there). This should fix that, while a later patch fixes
> the use of the MST state in the disable hook.
> 
> v2: Add missing forward struct declartions, caught by hdrtest.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  4 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 717ca3d7890d3..d3994e2a7d636 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  		if (ret)
>  			return ret;
>  
> +		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
> +		if (ret)
> +			return ret;
> +
>  		ret = intel_atomic_add_affected_planes(state, crtc);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 8b0e4defa3f10..ba29c294b7c1b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
>  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
>  }
> +
> +/**
> + * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
> + * @state: atomic state
> + * @crtc: CRTC
> + *
> + * Add the MST topology state for @crtc to @state.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
> +					     struct intel_crtc *crtc)
> +{
> +	struct drm_connector *_connector;
> +	struct drm_connector_state *conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
> +		struct drm_dp_mst_topology_state *mst_state;
> +		struct intel_connector *connector = to_intel_connector(_connector);
> +
> +		if (conn_state->crtc != &crtc->base)
> +			continue;
> +
> +		if (!connector->mst_port)
> +			continue;
> +
> +		mst_state = drm_atomic_get_mst_topology_state(&state->base,
> +							      &connector->mst_port->mst_mgr);
> +		if (IS_ERR(mst_state))
> +			return PTR_ERR(mst_state);
> +
> +		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f7301de6cdfb3..f1815bb722672 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -8,6 +8,8 @@
>  
>  #include <linux/types.h>
>  
> +struct intel_atomic_state;
> +struct intel_crtc;
>  struct intel_crtc_state;
>  struct intel_digital_port;
>  struct intel_dp;
> @@ -18,5 +20,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
>  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
> +int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
> +					     struct intel_crtc *crtc);
>  
>  #endif /* __INTEL_DP_MST_H__ */

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

Re: [PATCH v2 1/9] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs

[Lyude Paul]

Hi! should have a chance to look at this at the start of next week

On Thu, 2023-01-26 at 11:13 +0200, Imre Deak wrote:
> Add the MST topology for a CRTC to the atomic state if the driver
> needs to force a modeset on the CRTC after the encoder compute config
> functions are called.
> 
> Later the MST encoder's disable hook also adds the state, but that isn't
> guaranteed to work (since in that hook getting the state may fail, which
> can't be handled there). This should fix that, while a later patch fixes
> the use of the MST state in the disable hook.
> 
> v2: Add missing forward struct declartions, caught by hdrtest.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: stable@vger.kernel.org # 6.1
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  4 +++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 37 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  4 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 717ca3d7890d3..d3994e2a7d636 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5934,6 +5934,10 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  		if (ret)
>  			return ret;
>  
> +		ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
> +		if (ret)
> +			return ret;
> +
>  		ret = intel_atomic_add_affected_planes(state, crtc);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 8b0e4defa3f10..ba29c294b7c1b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1223,3 +1223,40 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
>  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
>  }
> +
> +/**
> + * intel_dp_mst_add_topology_state_for_crtc - add MST topology state for a CRTC
> + * @state: atomic state
> + * @crtc: CRTC
> + *
> + * Add the MST topology state for @crtc to @state.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
> +					     struct intel_crtc *crtc)
> +{
> +	struct drm_connector *_connector;
> +	struct drm_connector_state *conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(&state->base, _connector, conn_state, i) {
> +		struct drm_dp_mst_topology_state *mst_state;
> +		struct intel_connector *connector = to_intel_connector(_connector);
> +
> +		if (conn_state->crtc != &crtc->base)
> +			continue;
> +
> +		if (!connector->mst_port)
> +			continue;
> +
> +		mst_state = drm_atomic_get_mst_topology_state(&state->base,
> +							      &connector->mst_port->mst_mgr);
> +		if (IS_ERR(mst_state))
> +			return PTR_ERR(mst_state);
> +
> +		mst_state->pending_crtc_mask |= drm_crtc_mask(&crtc->base);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f7301de6cdfb3..f1815bb722672 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -8,6 +8,8 @@
>  
>  #include <linux/types.h>
>  
> +struct intel_atomic_state;
> +struct intel_crtc;
>  struct intel_crtc_state;
>  struct intel_digital_port;
>  struct intel_dp;
> @@ -18,5 +20,7 @@ int intel_dp_mst_encoder_active_links(struct intel_digital_port *dig_port);
>  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
> +int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state *state,
> +					     struct intel_crtc *crtc);
>  
>  #endif /* __INTEL_DP_MST_H__ */

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

Re: [Intel-gfx] [PATCH 4/9] drm/i915/dp_mst: Fix payload removal during output disabling

[Imre Deak]

On Thu, Jan 26, 2023 at 08:38:16PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:47PM +0200, Imre Deak wrote:
> > Use the correct old/new topology and payload states in
> > intel_mst_disable_dp(). So far drm_atomic_get_mst_topology_state() it
> > used returned either the old state, in case the state was added already
> > earlier during the atomic check phase or otherwise the new state (but
> > the latter could fail, which can't be handled in the enable/disable
> > hooks). After the first patch in the patchset, the state should always
> > get added already during the check phase, so here we can get the
> > old/new states without a failure.
> > 
> > drm_dp_remove_payload() should use time_slots from the old payload state
> > and vc_start_slot in the new one. It should update the new payload
> > states to reflect the sink's current payload table after the payload is
> > removed. Pass the new topology state and the old and new payload states
> > accordingly.
> > 
> > This also fixes a problem where the payload allocations for multiple MST
> > streams on the same link got inconsistent after a few commits, as
> > during payload removal the old instead of the new payload state got
> > updated, so the subsequent enabling sequence and commits used a stale
> > payload state.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # 6.1
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 5f7bcb5c14847..800fa12a61d93 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -524,10 +524,14 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> >  	struct intel_dp *intel_dp = &dig_port->dp;
> >  	struct intel_connector *connector =
> >  		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_dp_mst_topology_state *old_mst_state =
> > +		drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > +	struct drm_dp_mst_topology_state *new_mst_state =
> > +		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
> > +	struct drm_dp_mst_atomic_payload *old_payload =
> > +		drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
> > +	struct drm_dp_mst_atomic_payload *new_payload =
> > +		drm_atomic_get_mst_payload_state(new_mst_state, connector->port);
> 
> old states could be const no?

Yes, will change this.

> >  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> >  
> >  	drm_dbg_kms(&i915->drm, "active links %d\n",
> > @@ -535,8 +539,8 @@ 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,
> > -			      payload, payload);
> > +	drm_dp_remove_payload(&intel_dp->mst_mgr, new_mst_state,
> 
> Right that one needs to be 'new' to update the start_slots
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +			      old_payload, new_payload);
> >  
> >  	intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> >  }
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:48PM +0200, Imre Deak wrote:
> Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().
> 
> Cc: Lyude Paul <lyude@redhat.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 81cc0c3b1e000..619f616d69e20 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4770,7 +4770,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)

Why does this code even do that funny nested double loop?

>  				continue;
>  
>  			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Ville Syrjälä]

On Wed, Jan 25, 2023 at 01:48:48PM +0200, Imre Deak wrote:
> Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().
> 
> Cc: Lyude Paul <lyude@redhat.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 81cc0c3b1e000..619f616d69e20 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4770,7 +4770,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)

Why does this code even do that funny nested double loop?

>  				continue;
>  
>  			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Imre Deak]

On Fri, Jan 27, 2023 at 09:42:39PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:48PM +0200, Imre Deak wrote:
> > Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().
> > 
> > Cc: Lyude Paul <lyude@redhat.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 81cc0c3b1e000..619f616d69e20 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4770,7 +4770,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)
> 
> Why does this code even do that funny nested double loop?

The payload list is not ordered by VCPIs I think, but the printout wants
to list them in VCPI order.

> 
> >  				continue;
> >  
> >  			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 5/9] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology()

[Imre Deak]

On Fri, Jan 27, 2023 at 09:42:39PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 25, 2023 at 01:48:48PM +0200, Imre Deak wrote:
> > Fix an off-by-one error in the VCPI check in drm_dp_mst_dump_topology().
> > 
> > Cc: Lyude Paul <lyude@redhat.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 81cc0c3b1e000..619f616d69e20 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4770,7 +4770,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)
> 
> Why does this code even do that funny nested double loop?

The payload list is not ordered by VCPIs I think, but the printout wants
to list them in VCPI order.

> 
> >  				continue;
> >  
> >  			fetch_monitor_name(mgr, payload->port, name, sizeof(name));
> > -- 
> > 2.37.1
> 
> -- 
> Ville Syrjälä
> Intel