[PATCH 0/8] drm/msm/dpu: various dpu resource allocation/release mismatch fix

[PATCH 0/8] drm/msm/dpu: various dpu resource allocation/release mismatch fix

[Xi Zhang]

This series fixes various resource de-allocation issues in DPU
resource cleanup code. Aim is more reliable mode set, in particular
when multiple displays/encoders are connected, to avoid blue screen
or snowy dsc displays.

Tested with v7.3 + msm-next, on Ayaneo Pocket DS (SM8550 Kalama).
Changes are not device/chip specific.

Some of the changes has lived in ROCKNIX distro for a few months.
https://github.com/ROCKNIX/distribution/pull/2716

Note: The first patch in that PR was problematic: uninitialized pointer
variable field access (since fixed), and I did not understand drm
atomic state tracking, so it wasn't doing anything at all and
is not included in this series.

The third patch linked above has more changes than what's included in
this series. The extra change did not do anything that's needed.

The following 2 commits are new and are not in ROCKNIX:

[Patch 6/8] ("drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc")
[Patch 7/8] ("drm/msm: force mode_changed on color_mgmt_changed, not just ctm")

[Patch 7/8] would fix, from my testing:
https://lore.kernel.org/all/33424a9d-10a6-4479-bba6-12f8ce60da1a@packett.cool/
See original report by Stephan Gerhold:
https://gitlab.freedesktop.org/drm/msm/-/work_items/102

Note: I'm not part of the ROCKNIX team, just contributed a few patches
there.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
Xi Zhang (8):
      drm/msm/dpu: reset unassigned resource slots to NULL on topology update
      drm/msm/dpu: save LMs in encoder mode_set for cleanup use later
      drm/msm/dpu: use common helper cleanup function in cmd mode disable
      drm/msm/dpu: Match setup_dither cleanup to the way it was set up
      drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup
      drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc
      drm/msm: force mode_changed on color_mgmt_changed, not just ctm
      drm/msm/dpu: reset DSC_CMN_MAIN_CNF in dsc disable

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 115 ++++++++++-----------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c     |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |  15 ++-
 drivers/gpu/drm/msm/msm_atomic.c                   |   3 +-
 5 files changed, 70 insertions(+), 74 deletions(-)
---
base-commit: d024361c631b3f020a26c07d683e5103404b0fbf
change-id: 20261026-msm-dpu-res-clean-f1704106453f

Best regards,
--  
Xi Zhang <xz.ca.cn@gmail.com>

[PATCH 0/8] drm/msm/dpu: various dpu resource allocation/release mismatch fix

[Xi Zhang via B4 Relay]

This series fixes various resource de-allocation issues in DPU
resource cleanup code. Aim is more reliable mode set, in particular
when multiple displays/encoders are connected, to avoid blue screen
or snowy dsc displays.

Tested with v7.3 + msm-next, on Ayaneo Pocket DS (SM8550 Kalama).
Changes are not device/chip specific.

Some of the changes has lived in ROCKNIX distro for a few months.
https://github.com/ROCKNIX/distribution/pull/2716

Note: The first patch in that PR was problematic: uninitialized pointer
variable field access (since fixed), and I did not understand drm
atomic state tracking, so it wasn't doing anything at all and
is not included in this series.

The third patch linked above has more changes than what's included in
this series. The extra change did not do anything that's needed.

The following 2 commits are new and are not in ROCKNIX:

[Patch 6/8] ("drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc")
[Patch 7/8] ("drm/msm: force mode_changed on color_mgmt_changed, not just ctm")

[Patch 7/8] would fix, from my testing:
https://lore.kernel.org/all/33424a9d-10a6-4479-bba6-12f8ce60da1a@packett.cool/
See original report by Stephan Gerhold:
https://gitlab.freedesktop.org/drm/msm/-/work_items/102

Note: I'm not part of the ROCKNIX team, just contributed a few patches
there.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
Xi Zhang (8):
      drm/msm/dpu: reset unassigned resource slots to NULL on topology update
      drm/msm/dpu: save LMs in encoder mode_set for cleanup use later
      drm/msm/dpu: use common helper cleanup function in cmd mode disable
      drm/msm/dpu: Match setup_dither cleanup to the way it was set up
      drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup
      drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc
      drm/msm: force mode_changed on color_mgmt_changed, not just ctm
      drm/msm/dpu: reset DSC_CMN_MAIN_CNF in dsc disable

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 115 ++++++++++-----------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c     |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |  15 ++-
 drivers/gpu/drm/msm/msm_atomic.c                   |   3 +-
 5 files changed, 70 insertions(+), 74 deletions(-)
---
base-commit: d024361c631b3f020a26c07d683e5103404b0fbf
change-id: 20261026-msm-dpu-res-clean-f1704106453f

Best regards,
--  
Xi Zhang <xz.ca.cn@gmail.com>

[PATCH 1/8] drm/msm/dpu: reset unassigned resource slots to NULL on topology update

[Xi Zhang]

While this is not currently the case, it may be possible that topology
for cwb and dsc resource requirements change between mode set in the
future.

e.g. when dsc support is added to DisplayPort, dsc may be optionally
enabled and disabled between different modes due to
bandwidth requirements.

Set unassigned resource slots to NULL on topology update to avoid
issues in the future. It's good practice to do so anyway.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0a6be69a3e83..8d2b4d2726f2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1205,9 +1205,13 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 						       ARRAY_SIZE(hw_pp));
 	}
 
-	for (i = 0; i < num_cwb; i++) {
-		dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
-		cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) {
+		if (i < num_cwb) {
+			dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
+			cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
+		} else {
+			dpu_enc->hw_cwb[i] = NULL;
+		}
 	}
 
 	dpu_enc->cwb_mask = cwb_mask;
@@ -1215,16 +1219,20 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 		drm_enc->crtc, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
 
-	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++)
 		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
 						: NULL;
 
 	num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 						drm_enc->crtc, DPU_HW_BLK_DSC,
 						hw_dsc, ARRAY_SIZE(hw_dsc));
-	for (i = 0; i < num_dsc; i++) {
-		dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
-		dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_dsc); i++) {
+		if (i < num_dsc) {
+			dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
+			dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
+		} else {
+			dpu_enc->hw_dsc[i] = NULL;
+		}
 	}
 
 	dpu_enc->dsc_mask = dsc_mask;

-- 
2.55.0

[PATCH 1/8] drm/msm/dpu: reset unassigned resource slots to NULL on topology update

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

While this is not currently the case, it may be possible that topology
for cwb and dsc resource requirements change between mode set in the
future.

e.g. when dsc support is added to DisplayPort, dsc may be optionally
enabled and disabled between different modes due to
bandwidth requirements.

Set unassigned resource slots to NULL on topology update to avoid
issues in the future. It's good practice to do so anyway.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0a6be69a3e83..8d2b4d2726f2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1205,9 +1205,13 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 						       ARRAY_SIZE(hw_pp));
 	}
 
-	for (i = 0; i < num_cwb; i++) {
-		dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
-		cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) {
+		if (i < num_cwb) {
+			dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
+			cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
+		} else {
+			dpu_enc->hw_cwb[i] = NULL;
+		}
 	}
 
 	dpu_enc->cwb_mask = cwb_mask;
@@ -1215,16 +1219,20 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 		drm_enc->crtc, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
 
-	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++)
 		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
 						: NULL;
 
 	num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 						drm_enc->crtc, DPU_HW_BLK_DSC,
 						hw_dsc, ARRAY_SIZE(hw_dsc));
-	for (i = 0; i < num_dsc; i++) {
-		dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
-		dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_dsc); i++) {
+		if (i < num_dsc) {
+			dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
+			dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
+		} else {
+			dpu_enc->hw_dsc[i] = NULL;
+		}
 	}
 
 	dpu_enc->dsc_mask = dsc_mask;

-- 
2.55.0

[PATCH 2/8] drm/msm/dpu: save LMs in encoder mode_set for cleanup use later

[Xi Zhang]

dpu_encoder_helper_reset_mixers, which is called during atomic_commit
disable phase, previously called
dpu_kms_get_existing_global_state, which returns the global_state
after the state swap. So the cleanup was performed on the newly
allocated resources instead of the ones that actually need reset.

Cache LMs in dpu_enc similar to other resources during mode set, and
perform the cleanup on cached LMs instead.

Also removed redundant ctl->ops.setup_blendstage during cleanup since
ctl->ops.clear_all_blendstages covers it already.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +++++++++++++++--------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 8d2b4d2726f2..09afc0681248 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -133,6 +133,7 @@ enum dpu_enc_rc_states {
  * @cur_master:		Pointer to the current master in this mode. Optimization
  *			Only valid after enable. Cleared as disable.
  * @cur_slave:		As above but for the slave encoder.
+ * @hw_mixer:		Handle to the layer mixers used for the display.
  * @hw_pp:		Handle to the pingpong blocks used for the display. No.
  *			pingpong blocks can be different than num_phys_encs.
  * @hw_cwb:		Handle to the CWB muxes used for concurrent writeback
@@ -182,6 +183,7 @@ struct dpu_encoder_virt {
 	struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
 	struct dpu_encoder_phys *cur_master;
 	struct dpu_encoder_phys *cur_slave;
+	struct dpu_hw_mixer *hw_mixer[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_cwb *hw_cwb[MAX_CWB_PER_ENC];
 	struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
@@ -1156,11 +1158,12 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms;
 	struct dpu_global_state *global_state;
+	struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_cwb[MAX_CHANNELS_PER_ENC];
-	int num_ctl, num_pp, num_dsc, num_pp_per_intf;
+	int num_ctl, num_lm, num_pp, num_dsc, num_pp_per_intf;
 	int num_cwb = 0;
 	bool is_cwb_encoder;
 	unsigned int dsc_mask = 0;
@@ -1189,6 +1192,14 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
 	/* Query resource that have been reserved in atomic check step. */
+
+	num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
+		drm_enc->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
+
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_mixer); i++)
+		dpu_enc->hw_mixer[i] = i < num_lm ? to_dpu_hw_mixer(hw_lm[i])
+						: NULL;
+
 	if (is_cwb_encoder) {
 		num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 						       drm_enc->crtc,
@@ -2183,42 +2194,34 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
 
 static void dpu_encoder_helper_reset_mixers(struct dpu_encoder_phys *phys_enc)
 {
-	int i, num_lm;
-	struct dpu_global_state *global_state;
-	struct dpu_hw_blk *hw_lm[2];
-	struct dpu_hw_mixer *hw_mixer[2];
+	int i;
 	struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
 
 	/* reset all mixers for this encoder */
 	if (ctl->ops.clear_all_blendstages)
 		ctl->ops.clear_all_blendstages(ctl);
 
-	global_state = dpu_kms_get_existing_global_state(phys_enc->dpu_kms);
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_mixer); i++) {
+		if (dpu_enc->hw_mixer[i]) {
+			if (ctl->ops.update_pending_flush_mixer)
+				ctl->ops.update_pending_flush_mixer(ctl, dpu_enc->hw_mixer[i]->idx);
 
-	num_lm = dpu_rm_get_assigned_resources(&phys_enc->dpu_kms->rm, global_state,
-		phys_enc->parent->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
-
-	for (i = 0; i < num_lm; i++) {
-		hw_mixer[i] = to_dpu_hw_mixer(hw_lm[i]);
-		if (ctl->ops.update_pending_flush_mixer)
-			ctl->ops.update_pending_flush_mixer(ctl, hw_mixer[i]->idx);
-
-		/* clear all blendstages */
-		if (ctl->ops.setup_blendstage)
-			ctl->ops.setup_blendstage(ctl, hw_mixer[i]->idx, NULL);
-
-		if (hw_mixer[i]->ops.clear_all_blendstages)
-			hw_mixer[i]->ops.clear_all_blendstages(hw_mixer[i]);
+			/* clear all blendstages */
+			if (dpu_enc->hw_mixer[i]->ops.clear_all_blendstages)
+				dpu_enc->hw_mixer[i]->ops.clear_all_blendstages(
+					dpu_enc->hw_mixer[i]);
+		}
+	}
 
-		if (ctl->ops.set_active_lms)
-			ctl->ops.set_active_lms(ctl, NULL);
+	if (ctl->ops.set_active_lms)
+		ctl->ops.set_active_lms(ctl, NULL);
 
-		if (ctl->ops.set_active_fetch_pipes)
-			ctl->ops.set_active_fetch_pipes(ctl, NULL);
+	if (ctl->ops.set_active_fetch_pipes)
+		ctl->ops.set_active_fetch_pipes(ctl, NULL);
 
-		if (ctl->ops.set_active_pipes)
-			ctl->ops.set_active_pipes(ctl, NULL);
-	}
+	if (ctl->ops.set_active_pipes)
+		ctl->ops.set_active_pipes(ctl, NULL);
 }
 
 static void dpu_encoder_dsc_pipe_clr(struct dpu_hw_ctl *ctl,

-- 
2.55.0

[PATCH 2/8] drm/msm/dpu: save LMs in encoder mode_set for cleanup use later

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

dpu_encoder_helper_reset_mixers, which is called during atomic_commit
disable phase, previously called
dpu_kms_get_existing_global_state, which returns the global_state
after the state swap. So the cleanup was performed on the newly
allocated resources instead of the ones that actually need reset.

Cache LMs in dpu_enc similar to other resources during mode set, and
perform the cleanup on cached LMs instead.

Also removed redundant ctl->ops.setup_blendstage during cleanup since
ctl->ops.clear_all_blendstages covers it already.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +++++++++++++++--------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 8d2b4d2726f2..09afc0681248 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -133,6 +133,7 @@ enum dpu_enc_rc_states {
  * @cur_master:		Pointer to the current master in this mode. Optimization
  *			Only valid after enable. Cleared as disable.
  * @cur_slave:		As above but for the slave encoder.
+ * @hw_mixer:		Handle to the layer mixers used for the display.
  * @hw_pp:		Handle to the pingpong blocks used for the display. No.
  *			pingpong blocks can be different than num_phys_encs.
  * @hw_cwb:		Handle to the CWB muxes used for concurrent writeback
@@ -182,6 +183,7 @@ struct dpu_encoder_virt {
 	struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
 	struct dpu_encoder_phys *cur_master;
 	struct dpu_encoder_phys *cur_slave;
+	struct dpu_hw_mixer *hw_mixer[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_cwb *hw_cwb[MAX_CWB_PER_ENC];
 	struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
@@ -1156,11 +1158,12 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms;
 	struct dpu_global_state *global_state;
+	struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_cwb[MAX_CHANNELS_PER_ENC];
-	int num_ctl, num_pp, num_dsc, num_pp_per_intf;
+	int num_ctl, num_lm, num_pp, num_dsc, num_pp_per_intf;
 	int num_cwb = 0;
 	bool is_cwb_encoder;
 	unsigned int dsc_mask = 0;
@@ -1189,6 +1192,14 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
 	/* Query resource that have been reserved in atomic check step. */
+
+	num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
+		drm_enc->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
+
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_mixer); i++)
+		dpu_enc->hw_mixer[i] = i < num_lm ? to_dpu_hw_mixer(hw_lm[i])
+						: NULL;
+
 	if (is_cwb_encoder) {
 		num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 						       drm_enc->crtc,
@@ -2183,42 +2194,34 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
 
 static void dpu_encoder_helper_reset_mixers(struct dpu_encoder_phys *phys_enc)
 {
-	int i, num_lm;
-	struct dpu_global_state *global_state;
-	struct dpu_hw_blk *hw_lm[2];
-	struct dpu_hw_mixer *hw_mixer[2];
+	int i;
 	struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
 
 	/* reset all mixers for this encoder */
 	if (ctl->ops.clear_all_blendstages)
 		ctl->ops.clear_all_blendstages(ctl);
 
-	global_state = dpu_kms_get_existing_global_state(phys_enc->dpu_kms);
+	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_mixer); i++) {
+		if (dpu_enc->hw_mixer[i]) {
+			if (ctl->ops.update_pending_flush_mixer)
+				ctl->ops.update_pending_flush_mixer(ctl, dpu_enc->hw_mixer[i]->idx);
 
-	num_lm = dpu_rm_get_assigned_resources(&phys_enc->dpu_kms->rm, global_state,
-		phys_enc->parent->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
-
-	for (i = 0; i < num_lm; i++) {
-		hw_mixer[i] = to_dpu_hw_mixer(hw_lm[i]);
-		if (ctl->ops.update_pending_flush_mixer)
-			ctl->ops.update_pending_flush_mixer(ctl, hw_mixer[i]->idx);
-
-		/* clear all blendstages */
-		if (ctl->ops.setup_blendstage)
-			ctl->ops.setup_blendstage(ctl, hw_mixer[i]->idx, NULL);
-
-		if (hw_mixer[i]->ops.clear_all_blendstages)
-			hw_mixer[i]->ops.clear_all_blendstages(hw_mixer[i]);
+			/* clear all blendstages */
+			if (dpu_enc->hw_mixer[i]->ops.clear_all_blendstages)
+				dpu_enc->hw_mixer[i]->ops.clear_all_blendstages(
+					dpu_enc->hw_mixer[i]);
+		}
+	}
 
-		if (ctl->ops.set_active_lms)
-			ctl->ops.set_active_lms(ctl, NULL);
+	if (ctl->ops.set_active_lms)
+		ctl->ops.set_active_lms(ctl, NULL);
 
-		if (ctl->ops.set_active_fetch_pipes)
-			ctl->ops.set_active_fetch_pipes(ctl, NULL);
+	if (ctl->ops.set_active_fetch_pipes)
+		ctl->ops.set_active_fetch_pipes(ctl, NULL);
 
-		if (ctl->ops.set_active_pipes)
-			ctl->ops.set_active_pipes(ctl, NULL);
-	}
+	if (ctl->ops.set_active_pipes)
+		ctl->ops.set_active_pipes(ctl, NULL);
 }
 
 static void dpu_encoder_dsc_pipe_clr(struct dpu_hw_ctl *ctl,

-- 
2.55.0

[PATCH 3/8] drm/msm/dpu: use common helper cleanup function in cmd mode disable

[Xi Zhang]

cmd mode encoder previously did not clean up dsc, or LMs during
disable.

Change to use the common dpu_encoder_helper_phys_cleanup function
for cmd mode disable.

Also do not set intf_cfg.mode_3d during disable:
dpu_encoder_helper_get_3d_blend_mode accesses drm_encoder->crtc->state
directly so intf_cfg.mode_3d weren't set to expected value for disable.
intf_cfg.mode_3d is also ignored by ctl->ops.reset_intf_cfg.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          |  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 10 +---------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 09afc0681248..0ab151369c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2332,8 +2332,8 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 		dpu_enc->dsc = NULL;
 	}
 
-	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
-	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+	intf_cfg.stream_sel = 0; /* Ignored by reset_intf_cfg */
+	intf_cfg.mode_3d = 0; /* Ignored by reset_intf_cfg */
 	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
 	intf_cfg.cwb = dpu_enc->cwb_mask;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 45079ee59cf6..111f9dd5e053 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -538,7 +538,6 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc)
 {
 	struct dpu_encoder_phys_cmd *cmd_enc =
 		to_dpu_encoder_phys_cmd(phys_enc);
-	struct dpu_hw_ctl *ctl;
 
 	if (phys_enc->enable_state == DPU_ENC_DISABLED) {
 		DPU_ERROR_CMDENC(cmd_enc, "already disabled\n");
@@ -566,14 +565,7 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc)
 			phys_enc->hw_pp->ops.disable_tearcheck(phys_enc->hw_pp);
 	}
 
-	if (phys_enc->hw_intf->ops.bind_pingpong_blk) {
-		phys_enc->hw_intf->ops.bind_pingpong_blk(
-				phys_enc->hw_intf,
-				PINGPONG_NONE);
-
-		ctl = phys_enc->hw_ctl;
-		ctl->ops.update_pending_flush_intf(ctl, phys_enc->hw_intf->idx);
-	}
+	dpu_encoder_helper_phys_cleanup(phys_enc);
 
 	phys_enc->enable_state = DPU_ENC_DISABLED;
 }

-- 
2.55.0

[PATCH 3/8] drm/msm/dpu: use common helper cleanup function in cmd mode disable

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

cmd mode encoder previously did not clean up dsc, or LMs during
disable.

Change to use the common dpu_encoder_helper_phys_cleanup function
for cmd mode disable.

Also do not set intf_cfg.mode_3d during disable:
dpu_encoder_helper_get_3d_blend_mode accesses drm_encoder->crtc->state
directly so intf_cfg.mode_3d weren't set to expected value for disable.
intf_cfg.mode_3d is also ignored by ctl->ops.reset_intf_cfg.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          |  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 10 +---------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 09afc0681248..0ab151369c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2332,8 +2332,8 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 		dpu_enc->dsc = NULL;
 	}
 
-	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
-	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+	intf_cfg.stream_sel = 0; /* Ignored by reset_intf_cfg */
+	intf_cfg.mode_3d = 0; /* Ignored by reset_intf_cfg */
 	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
 	intf_cfg.cwb = dpu_enc->cwb_mask;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 45079ee59cf6..111f9dd5e053 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -538,7 +538,6 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc)
 {
 	struct dpu_encoder_phys_cmd *cmd_enc =
 		to_dpu_encoder_phys_cmd(phys_enc);
-	struct dpu_hw_ctl *ctl;
 
 	if (phys_enc->enable_state == DPU_ENC_DISABLED) {
 		DPU_ERROR_CMDENC(cmd_enc, "already disabled\n");
@@ -566,14 +565,7 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc)
 			phys_enc->hw_pp->ops.disable_tearcheck(phys_enc->hw_pp);
 	}
 
-	if (phys_enc->hw_intf->ops.bind_pingpong_blk) {
-		phys_enc->hw_intf->ops.bind_pingpong_blk(
-				phys_enc->hw_intf,
-				PINGPONG_NONE);
-
-		ctl = phys_enc->hw_ctl;
-		ctl->ops.update_pending_flush_intf(ctl, phys_enc->hw_intf->idx);
-	}
+	dpu_encoder_helper_phys_cleanup(phys_enc);
 
 	phys_enc->enable_state = DPU_ENC_DISABLED;
 }

-- 
2.55.0

[PATCH 4/8] drm/msm/dpu: Match setup_dither cleanup to the way it was set up

[Xi Zhang]

Cleanup hw_pp->ops.setup_dither with NULL for all PINGPONGs,
matching the way it was setup during enable.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0ab151369c99..7d092398fc99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1320,7 +1320,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
 		unsigned bpc = dpu_enc->connector->display_info.bpc;
-		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+		for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++) {
 			if (!dpu_enc->hw_pp[i])
 				continue;
 			_dpu_encoder_setup_dither(dpu_enc->hw_pp[i], bpc);
@@ -2303,8 +2303,10 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 		}
 	}
 
-	if (phys_enc->hw_pp && phys_enc->hw_pp->ops.setup_dither)
-		phys_enc->hw_pp->ops.setup_dither(phys_enc->hw_pp, NULL);
+	if (dpu_enc->disp_info.intf_type == INTF_DSI)
+		for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++)
+			if (dpu_enc->hw_pp[i] && dpu_enc->hw_pp[i]->ops.setup_dither)
+				dpu_enc->hw_pp[i]->ops.setup_dither(dpu_enc->hw_pp[i], NULL);
 
 	if (dpu_enc->cwb_mask)
 		dpu_encoder_helper_phys_setup_cwb(phys_enc, false);

-- 
2.55.0

[PATCH 4/8] drm/msm/dpu: Match setup_dither cleanup to the way it was set up

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

Cleanup hw_pp->ops.setup_dither with NULL for all PINGPONGs,
matching the way it was setup during enable.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0ab151369c99..7d092398fc99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1320,7 +1320,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
 		unsigned bpc = dpu_enc->connector->display_info.bpc;
-		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+		for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++) {
 			if (!dpu_enc->hw_pp[i])
 				continue;
 			_dpu_encoder_setup_dither(dpu_enc->hw_pp[i], bpc);
@@ -2303,8 +2303,10 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 		}
 	}
 
-	if (phys_enc->hw_pp && phys_enc->hw_pp->ops.setup_dither)
-		phys_enc->hw_pp->ops.setup_dither(phys_enc->hw_pp, NULL);
+	if (dpu_enc->disp_info.intf_type == INTF_DSI)
+		for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++)
+			if (dpu_enc->hw_pp[i] && dpu_enc->hw_pp[i]->ops.setup_dither)
+				dpu_enc->hw_pp[i]->ops.setup_dither(dpu_enc->hw_pp[i], NULL);
 
 	if (dpu_enc->cwb_mask)
 		dpu_encoder_helper_phys_setup_cwb(phys_enc, false);

-- 
2.55.0

[PATCH 5/8] drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup

[Xi Zhang]

No functional change intended. Clean up only.

The cached hw_pp should be the same as the ones from
dpu_kms_get_existing_global_state during
dpu_encoder_helper_phys_setup_cwb.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7d092398fc99..f1d2482b787b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2363,11 +2363,6 @@ void dpu_encoder_helper_phys_setup_cwb(struct dpu_encoder_phys *phys_enc,
 	struct dpu_hw_ctl *hw_ctl;
 	struct dpu_hw_cwb_setup_cfg cwb_cfg;
 
-	struct dpu_kms *dpu_kms;
-	struct dpu_global_state *global_state;
-	struct dpu_hw_blk *rt_pp_list[MAX_CHANNELS_PER_ENC];
-	int num_pp;
-
 	if (!phys_enc->hw_wb)
 		return;
 
@@ -2379,33 +2374,20 @@ void dpu_encoder_helper_phys_setup_cwb(struct dpu_encoder_phys *phys_enc,
 		return;
 	}
 
-	dpu_kms = phys_enc->dpu_kms;
-	global_state = dpu_kms_get_existing_global_state(dpu_kms);
-	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
-					       phys_enc->parent->crtc,
-					       DPU_HW_BLK_PINGPONG, rt_pp_list,
-					       ARRAY_SIZE(rt_pp_list));
-
-	if (num_pp == 0 || num_pp > MAX_CHANNELS_PER_ENC) {
-		DPU_DEBUG_ENC(dpu_enc, "invalid num_pp %d\n", num_pp);
-		return;
-	}
-
 	/*
 	 * The CWB mux supports using LM or DSPP as tap points. For now,
 	 * always use LM tap point
 	 */
 	cwb_cfg.input = INPUT_MODE_LM_OUT;
 
-	for (int i = 0; i < MAX_CWB_PER_ENC; i++) {
+	for (int i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) {
 		hw_cwb = dpu_enc->hw_cwb[i];
 		if (!hw_cwb)
 			continue;
 
 		if (enable) {
-			struct dpu_hw_pingpong *hw_pp =
-					to_dpu_hw_pingpong(rt_pp_list[i]);
-			cwb_cfg.pp_idx = hw_pp->idx;
+			cwb_cfg.pp_idx = i < ARRAY_SIZE(dpu_enc->hw_pp) && dpu_enc->hw_pp[i] ?
+					dpu_enc->hw_pp[i]->idx : PINGPONG_NONE;
 		} else {
 			cwb_cfg.pp_idx = PINGPONG_NONE;
 		}

-- 
2.55.0

[PATCH 5/8] drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

No functional change intended. Clean up only.

The cached hw_pp should be the same as the ones from
dpu_kms_get_existing_global_state during
dpu_encoder_helper_phys_setup_cwb.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7d092398fc99..f1d2482b787b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2363,11 +2363,6 @@ void dpu_encoder_helper_phys_setup_cwb(struct dpu_encoder_phys *phys_enc,
 	struct dpu_hw_ctl *hw_ctl;
 	struct dpu_hw_cwb_setup_cfg cwb_cfg;
 
-	struct dpu_kms *dpu_kms;
-	struct dpu_global_state *global_state;
-	struct dpu_hw_blk *rt_pp_list[MAX_CHANNELS_PER_ENC];
-	int num_pp;
-
 	if (!phys_enc->hw_wb)
 		return;
 
@@ -2379,33 +2374,20 @@ void dpu_encoder_helper_phys_setup_cwb(struct dpu_encoder_phys *phys_enc,
 		return;
 	}
 
-	dpu_kms = phys_enc->dpu_kms;
-	global_state = dpu_kms_get_existing_global_state(dpu_kms);
-	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
-					       phys_enc->parent->crtc,
-					       DPU_HW_BLK_PINGPONG, rt_pp_list,
-					       ARRAY_SIZE(rt_pp_list));
-
-	if (num_pp == 0 || num_pp > MAX_CHANNELS_PER_ENC) {
-		DPU_DEBUG_ENC(dpu_enc, "invalid num_pp %d\n", num_pp);
-		return;
-	}
-
 	/*
 	 * The CWB mux supports using LM or DSPP as tap points. For now,
 	 * always use LM tap point
 	 */
 	cwb_cfg.input = INPUT_MODE_LM_OUT;
 
-	for (int i = 0; i < MAX_CWB_PER_ENC; i++) {
+	for (int i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) {
 		hw_cwb = dpu_enc->hw_cwb[i];
 		if (!hw_cwb)
 			continue;
 
 		if (enable) {
-			struct dpu_hw_pingpong *hw_pp =
-					to_dpu_hw_pingpong(rt_pp_list[i]);
-			cwb_cfg.pp_idx = hw_pp->idx;
+			cwb_cfg.pp_idx = i < ARRAY_SIZE(dpu_enc->hw_pp) && dpu_enc->hw_pp[i] ?
+					dpu_enc->hw_pp[i]->idx : PINGPONG_NONE;
 		} else {
 			cwb_cfg.pp_idx = PINGPONG_NONE;
 		}

-- 
2.55.0

[PATCH 6/8] drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc

[Xi Zhang]

Consider the scenario with 4 crtcs, 3 encoders/displays are
connected. DPU resources is nearly maxed-out, and mode set is
performed on a display which has acquired many dpu resources.
A new crtc is selected by kernel drm for the mode set,
and during atomic_check, the previous crtc won't have a chance
to release its old resources from global_state before the new
crtc tries to allocate new ones. Leading to atomic_check failure.

By having the same crtc for encoder when possible, during
atomic_check each individual crtc will always be able to release
old resources before allocating new ones.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index da3556eb6ecc..13099d6e3ac9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -879,9 +879,18 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 		}
 	}
 
-	/* All CRTCs are compatible with all encoders */
-	drm_for_each_encoder(encoder, dev)
-		encoder->possible_crtcs = (1 << dev->mode_config.num_crtc) - 1;
+	/* All CRTCs are compatible with all encoders.
+	 * Nevertheless, if enough CRTCs are available, pair up crtc/encoder
+	 * for better resource re-assignments during atomic_check
+	 * from userspace mode set.
+	 */
+	i = 0;
+	drm_for_each_encoder(encoder, dev) {
+		if (dev->mode_config.num_crtc >= num_encoders)
+			encoder->possible_crtcs = 1 << i++;
+		else
+			encoder->possible_crtcs = (1 << dev->mode_config.num_crtc) - 1;
+	}
 
 	return 0;
 }

-- 
2.55.0

[PATCH 6/8] drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

Consider the scenario with 4 crtcs, 3 encoders/displays are
connected. DPU resources is nearly maxed-out, and mode set is
performed on a display which has acquired many dpu resources.
A new crtc is selected by kernel drm for the mode set,
and during atomic_check, the previous crtc won't have a chance
to release its old resources from global_state before the new
crtc tries to allocate new ones. Leading to atomic_check failure.

By having the same crtc for encoder when possible, during
atomic_check each individual crtc will always be able to release
old resources before allocating new ones.

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index da3556eb6ecc..13099d6e3ac9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -879,9 +879,18 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 		}
 	}
 
-	/* All CRTCs are compatible with all encoders */
-	drm_for_each_encoder(encoder, dev)
-		encoder->possible_crtcs = (1 << dev->mode_config.num_crtc) - 1;
+	/* All CRTCs are compatible with all encoders.
+	 * Nevertheless, if enough CRTCs are available, pair up crtc/encoder
+	 * for better resource re-assignments during atomic_check
+	 * from userspace mode set.
+	 */
+	i = 0;
+	drm_for_each_encoder(encoder, dev) {
+		if (dev->mode_config.num_crtc >= num_encoders)
+			encoder->possible_crtcs = 1 << i++;
+		else
+			encoder->possible_crtcs = (1 << dev->mode_config.num_crtc) - 1;
+	}
 
 	return 0;
 }

-- 
2.55.0

[PATCH 7/8] drm/msm: force mode_changed on color_mgmt_changed, not just ctm

[Xi Zhang]

dpu_crtc_atomic_check calls dpu_crtc_assign_resources when
color_mgmt_changed is set, which is unsafe outside of full modeset.

Set allow_modeset and mode_changed when color_mgmt_changed is set,
not just when ctm is changed.

Also would fix
commit 42f62cd79578 ("drm/msm/dpu: try reserving the DSPP-less LM first")
which was reverted.

Closes: https://gitlab.freedesktop.org/drm/msm/-/work_items/102

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/msm_atomic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index a8babf1dbe0d..6f0f30b0728a 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -195,8 +195,7 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_commit *state)
 	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
-		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
-		    (!old_crtc_state->ctm && new_crtc_state->ctm)) {
+		if (new_crtc_state->color_mgmt_changed) {
 			new_crtc_state->mode_changed = true;
 			state->allow_modeset = true;
 		}

-- 
2.55.0

[PATCH 7/8] drm/msm: force mode_changed on color_mgmt_changed, not just ctm

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

dpu_crtc_atomic_check calls dpu_crtc_assign_resources when
color_mgmt_changed is set, which is unsafe outside of full modeset.

Set allow_modeset and mode_changed when color_mgmt_changed is set,
not just when ctm is changed.

Also would fix
commit 42f62cd79578 ("drm/msm/dpu: try reserving the DSPP-less LM first")
which was reverted.

Closes: https://gitlab.freedesktop.org/drm/msm/-/work_items/102

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/msm_atomic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index a8babf1dbe0d..6f0f30b0728a 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -195,8 +195,7 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_commit *state)
 	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
-		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
-		    (!old_crtc_state->ctm && new_crtc_state->ctm)) {
+		if (new_crtc_state->color_mgmt_changed) {
 			new_crtc_state->mode_changed = true;
 			state->allow_modeset = true;
 		}

-- 
2.55.0

[PATCH 8/8] drm/msm/dpu: reset DSC_CMN_MAIN_CNF in dsc disable

[Xi Zhang]

From observation, DSC_CMN_MAIN_CNF has hardware restriction on
differing values between paired even/odd dsc.

Reset it to 0 when dsc disable to avoid issues when a dsc pair
is later used individually for each of its dsc.

Note that currently dpu always assigns dsc in pairs if possible
for power saving. In the future, it may be desired to assign
singular dsc if possible to support high display count. e.g.
when dsc for DisplayPort support is added.

Tested on Ayaneo Pocket DS (SM8550 Kalama)

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
index b3395e9c34a1..b8a330d813e6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
@@ -78,6 +78,7 @@ static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
 
 	hw = &hw_dsc->hw;
 	sblk = hw_dsc->caps->sblk;
+	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, 0);
 	DPU_REG_WRITE(hw, sblk->ctl.base + DSC_CFG, 0);
 
 	DPU_REG_WRITE(hw, sblk->enc.base + ENC_DF_CTRL, 0);

-- 
2.55.0

[PATCH 8/8] drm/msm/dpu: reset DSC_CMN_MAIN_CNF in dsc disable

[Xi Zhang via B4 Relay]

From: Xi Zhang <xz.ca.cn@gmail.com>

From observation, DSC_CMN_MAIN_CNF has hardware restriction on
differing values between paired even/odd dsc.

Reset it to 0 when dsc disable to avoid issues when a dsc pair
is later used individually for each of its dsc.

Note that currently dpu always assigns dsc in pairs if possible
for power saving. In the future, it may be desired to assign
singular dsc if possible to support high display count. e.g.
when dsc for DisplayPort support is added.

Tested on Ayaneo Pocket DS (SM8550 Kalama)

Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
index b3395e9c34a1..b8a330d813e6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
@@ -78,6 +78,7 @@ static void dpu_hw_dsc_disable_1_2(struct dpu_hw_dsc *hw_dsc)
 
 	hw = &hw_dsc->hw;
 	sblk = hw_dsc->caps->sblk;
+	DPU_REG_WRITE(hw, DSC_CMN_MAIN_CNF, 0);
 	DPU_REG_WRITE(hw, sblk->ctl.base + DSC_CFG, 0);
 
 	DPU_REG_WRITE(hw, sblk->enc.base + ENC_DF_CTRL, 0);

-- 
2.55.0

Re: [PATCH 5/8] drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dpu: wrong pingpong index in dpu_encoder_helper_phys_setup_cwb()
--

commit 43be42587de24c595bae6d488f5806f627560896
Author: Xi Zhang <xz.ca.cn@gmail.com>

drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup

This commit replaces the global state lookup of DPU_HW_BLK_PINGPONG blocks in
dpu_encoder_helper_phys_setup_cwb() with the cached dpu_enc->hw_pp array from
dpu_encoder_virt_atomic_mode_set().

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 7d092398fc991..f1d2482b787b0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2363,11 +2363,6 @@ void dpu_encoder_helper_phys_setup_cwb(struct dpu_encoder_phys *phys_enc,
>  	struct dpu_hw_ctl *hw_ctl;
>  	struct dpu_hw_cwb_setup_cfg cwb_cfg;
>  
> -	struct dpu_kms *dpu_kms;
> -	struct dpu_global_state *global_state;
> -	struct dpu_hw_blk *rt_pp_list[MAX_CHANNELS_PER_ENC];
> -	int num_pp;
> -
>  	if (!phys_enc->hw_wb)
>  		return;
>  
> @@ -2379,33 +2374,20 @@ void dpu_encoder_helper_phys_setup_cwb(struct dpu_encoder_phys *phys_enc,
>  		return;
>  	}
>  
> -	dpu_kms = phys_enc->dpu_kms;
> -	global_state = dpu_kms_get_existing_global_state(dpu_kms);
> -	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> -					       phys_enc->parent->crtc,
> -					       DPU_HW_BLK_PINGPONG, rt_pp_list,
> -					       ARRAY_SIZE(rt_pp_list));
> -
> -	if (num_pp == 0 || num_pp > MAX_CHANNELS_PER_ENC) {
> -		DPU_DEBUG_ENC(dpu_enc, "invalid num_pp %d\n", num_pp);
> -		return;
> -	}
> -
>  	/*
>  	 * The CWB mux supports using LM or DSPP as tap points. For now,
>  	 * always use LM tap point
>  	 */
>  	cwb_cfg.input = INPUT_MODE_LM_OUT;
>  
> -	for (int i = 0; i < MAX_CWB_PER_ENC; i++) {
> +	for (int i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) {
>  		hw_cwb = dpu_enc->hw_cwb[i];
>  		if (!hw_cwb)
>  			continue;
>  
>  		if (enable) {
> -			struct dpu_hw_pingpong *hw_pp =
> -					to_dpu_hw_pingpong(rt_pp_list[i]);
> -			cwb_cfg.pp_idx = hw_pp->idx;
> +			cwb_cfg.pp_idx = i < ARRAY_SIZE(dpu_enc->hw_pp) && dpu_enc->hw_pp[i] ?
> +					dpu_enc->hw_pp[i]->idx : PINGPONG_NONE;

[Severity: High]
Does using dpu_enc->hw_pp[i]->idx here pass the dedicated CWB pingpong index
(PINGPONG_CWB_*) instead of the real-time display's pingpong index into
cwb_cfg.pp_idx?

dpu_encoder_helper_phys_setup_cwb() only configures CWB blocks when called on
a writeback encoder where dpu_enc->hw_cwb[i] is populated. In
dpu_encoder_virt_atomic_mode_set(), whenever dpu_enc->hw_cwb is populated
(is_cwb_encoder is true), dpu_enc->hw_pp is populated with
DPU_HW_BLK_DCWB_PINGPONG rather than DPU_HW_BLK_PINGPONG:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_virt_atomic_mode_set() {
	...
	if (is_cwb_encoder) {
		num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
						       drm_enc->crtc,
						       DPU_HW_BLK_DCWB_PINGPONG,
						       hw_pp, ARRAY_SIZE(hw_pp));
		num_cwb = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
						       drm_enc->crtc,
						       DPU_HW_BLK_CWB,
						       hw_cwb, ARRAY_SIZE(hw_cwb));
	} else {
		num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
						       drm_enc->crtc,
						       DPU_HW_BLK_PINGPONG, hw_pp,
						       ARRAY_SIZE(hw_pp));
	}
	...
}

In dpu_rm_get_assigned_resources(), DPU_HW_BLK_PINGPONG and
DPU_HW_BLK_DCWB_PINGPONG return mutually exclusive sets of pingpong blocks:

drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:dpu_rm_get_assigned_resources() {
	...
		if (type == DPU_HW_BLK_PINGPONG) {
			struct dpu_hw_pingpong *pp = to_dpu_hw_pingpong(hw_blks[i]);

			if (pp->idx >= PINGPONG_CWB_0)
				continue;
		}

		if (type == DPU_HW_BLK_DCWB_PINGPONG) {
			struct dpu_hw_pingpong *pp = to_dpu_hw_pingpong(hw_blks[i]);

			if (pp->idx < PINGPONG_CWB_0)
				continue;
		}
	...
}

However, dpu_hw_cwb_config() expects the real-time display's pingpong index
when programming the CWB_MUX register:

drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.c:dpu_hw_cwb_config() {
	...
	/*
	 * The CWB_MUX register takes the pingpong index for the real-time
	 * display
	 */
	if ((pp != PINGPONG_NONE) && (pp < PINGPONG_MAX))
		cwb_mux_cfg = FIELD_PREP(CWB_MUX_MASK, pp - PINGPONG_0);
	...
}

By replacing the DPU_HW_BLK_PINGPONG lookup with dpu_enc->hw_pp[i]->idx on
the CWB writeback encoder, won't this program CWB_MUX with the dedicated CWB
pingpong block index instead of the real-time display's pingpong index?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-msm-dpu-res-clean-v1-0-f46130a8a717@gmail.com?part=5

Re: [PATCH 2/8] drm/msm/dpu: save LMs in encoder mode_set for cleanup use later

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dpu: incomplete blendstage reset in dpu_encoder_helper_reset_mixers()
--

commit d3a43e897b19cb72f6d34115067a2c68f2bef217
Author: Xi Zhang <xz.ca.cn@gmail.com>

drm/msm/dpu: save LMs in encoder mode_set for cleanup use later

This commit caches layer mixers in dpu_encoder_virt during atomic_mode_set so
that dpu_encoder_helper_reset_mixers() cleans up the previously assigned
mixers rather than querying the post-swap global state. It also removes the
ctl->ops.setup_blendstage() call from dpu_encoder_helper_reset_mixers().

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 8d2b4d2726f27..09afc0681248f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

[ ... ]

> @@ -2183,42 +2194,34 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
>  
>  static void dpu_encoder_helper_reset_mixers(struct dpu_encoder_phys *phys_enc)
>  {
> -	int i, num_lm;
> -	struct dpu_global_state *global_state;
> -	struct dpu_hw_blk *hw_lm[2];
> -	struct dpu_hw_mixer *hw_mixer[2];
> +	int i;
>  	struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
>  
>  	/* reset all mixers for this encoder */
>  	if (ctl->ops.clear_all_blendstages)
>  		ctl->ops.clear_all_blendstages(ctl);
>  
> -	global_state = dpu_kms_get_existing_global_state(phys_enc->dpu_kms);
> +	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_mixer); i++) {
> +		if (dpu_enc->hw_mixer[i]) {
> +			if (ctl->ops.update_pending_flush_mixer)
> +				ctl->ops.update_pending_flush_mixer(ctl, dpu_enc->hw_mixer[i]->idx);
>  
> -	num_lm = dpu_rm_get_assigned_resources(&phys_enc->dpu_kms->rm, global_state,
> -		phys_enc->parent->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
> -
> -	for (i = 0; i < num_lm; i++) {
> -		hw_mixer[i] = to_dpu_hw_mixer(hw_lm[i]);
> -		if (ctl->ops.update_pending_flush_mixer)
> -			ctl->ops.update_pending_flush_mixer(ctl, hw_mixer[i]->idx);
> -
> -		/* clear all blendstages */
> -		if (ctl->ops.setup_blendstage)
> -			ctl->ops.setup_blendstage(ctl, hw_mixer[i]->idx, NULL);
> -
> -		if (hw_mixer[i]->ops.clear_all_blendstages)
> -			hw_mixer[i]->ops.clear_all_blendstages(hw_mixer[i]);
> +			/* clear all blendstages */
> +			if (dpu_enc->hw_mixer[i]->ops.clear_all_blendstages)
> +				dpu_enc->hw_mixer[i]->ops.clear_all_blendstages(
> +					dpu_enc->hw_mixer[i]);
> +		}
> +	}

[Severity: Medium]
Does ctl->ops.clear_all_blendstages() completely cover what
ctl->ops.setup_blendstage(ctl, hw_mixer[i]->idx, NULL) was doing here?

When dpu_encoder_virt_atomic_disable() -> dpu_encoder_helper_phys_cleanup() ->
dpu_encoder_helper_reset_mixers() runs on DPU hardware with core_major_ver < 12,
ctl->ops.clear_all_blendstages invokes dpu_hw_ctl_clear_all_blendstages() in
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c:

static void dpu_hw_ctl_clear_all_blendstages(struct dpu_hw_ctl *ctx)
{
	struct dpu_hw_blk_reg_map *c = &ctx->hw;
	int i;

	for (i = 0; i < ctx->mixer_count; i++) {
		enum dpu_lm mixer_id = ctx->mixer_hw_caps[i].id;

		DPU_REG_WRITE(c, CTL_LAYER(mixer_id), 0);
		DPU_REG_WRITE(c, CTL_LAYER_EXT(mixer_id), 0);
		DPU_REG_WRITE(c, CTL_LAYER_EXT2(mixer_id), 0);
		DPU_REG_WRITE(c, CTL_LAYER_EXT3(mixer_id), 0);
	}

	DPU_REG_WRITE(c, CTL_FETCH_PIPE_ACTIVE, 0);
}

By contrast, dpu_hw_ctl_setup_blendstage() with stage_cfg == NULL did:

	mixercfg[0] = CTL_MIXER_BORDER_OUT; /* always set BORDER_OUT */

	if (!stage_cfg)
		goto exit;
...
exit:
	DPU_REG_WRITE(c, CTL_LAYER(lm), mixercfg[0]);
	DPU_REG_WRITE(c, CTL_LAYER_EXT(lm), mixercfg[1]);
	DPU_REG_WRITE(c, CTL_LAYER_EXT2(lm), mixercfg[2]);
	DPU_REG_WRITE(c, CTL_LAYER_EXT3(lm), mixercfg[3]);
	if (ctx->mdss_ver->core_major_ver >= 9)
		DPU_REG_WRITE(c, CTL_LAYER_EXT4(lm), mixercfg[4]);

Without ctl->ops.setup_blendstage(), on DPU v9.x through v11.x platforms
CTL_LAYER_EXT4(lm) is never cleared during mixer reset (leaving any staged
SSPP_DMA4 or SSPP_DMA5 bits set), and on DPU < v5 (where
ctl->ops.reset_intf_cfg is not implemented) CTL_LAYER(lm) is left as 0
instead of having CTL_MIXER_BORDER_OUT set.

Should dpu_hw_ctl_clear_all_blendstages() be updated to also clear
CTL_LAYER_EXT4(mixer_id) on core_major_ver >= 9 (and preserve
CTL_MIXER_BORDER_OUT if needed), or should the setup_blendstage() call be
kept?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-msm-dpu-res-clean-v1-0-f46130a8a717@gmail.com?part=2

Re: [PATCH 6/8] drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm/dpu: disjoint possible_crtcs in _dpu_kms_drm_obj_init() breaks CWB
- [Medium] drm/msm/dpu: pairing encoder i to crtc i deprives encoders of cursor planes
- [Medium] drm/msm/dpu: broken possible_crtcs pairing in _dpu_kms_drm_obj_init()
--

commit 92f85d682d2387cdaeb71bf5aeb4e7434bd2e013
Author: Xi Zhang <xz.ca.cn@gmail.com>

drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc

This commit pairs each encoder with a dedicated CRTC in
_dpu_kms_drm_obj_init() when dev->mode_config.num_crtc >= num_encoders so
that modesets keep the same CRTC and release old DPU resources before
allocating new ones during atomic_check.

>     drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc
>     
>     Consider the scenario with 4 crtcs, 3 encoders/displays are
>     connected. DPU resources is nearly maxed-out, and mode set is
>     performed on a display which has acquired many dpu resources.
>     A new crtc is selected by kernel drm for the mode set,
>     and during atomic_check, the previous crtc won't have a chance
>     to release its old resources from global_state before the new
>     crtc tries to allocate new ones. Leading to atomic_check failure.
>     
>     By having the same crtc for encoder when possible, during
>     atomic_check each individual crtc will always be able to release
>     old resources before allocating new ones.
>     
>     Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>

[Severity: Medium]
Can the scenario with 4 CRTCs and 3 encoders described in the commit message
occur, and does pairing encoders to CRTCs resolve cross-CRTC resource
reassignment in dpu_crtc_atomic_check()?

In drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c:_dpu_kms_drm_obj_init(), the
number of created CRTCs is capped at num_encoders:

    max_crtc_count = min(catalog->mixer_count, num_encoders);
    ...
    max_crtc_count = min(max_crtc_count, primary_planes_idx);

    /* Create one CRTC per encoder */
    for (i = 0; i < max_crtc_count; i++) {
        crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);

so dev->mode_config.num_crtc is never greater than num_encoders.

In addition, drm_atomic_helper_check_planes() iterates over CRTCs in
ascending CRTC index order and calls dpu_crtc_atomic_check(). Inside
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c, both dpu_crtc_assign_resources()
and dpu_crtc_reassign_planes() release and immediately reserve resources per
CRTC:

    dpu_rm_release(global_state, crtc);

    if (!crtc_state->enable)
        return 0;

    topology = dpu_crtc_get_topology(crtc, dpu_kms, crtc_state);
    ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
                 crtc_state->crtc, &topology);

and:

    dpu_rm_release_all_sspp(global_state, crtc);

    return dpu_crtc_assign_planes(crtc, crtc_state);

Even with 1:1 encoder-to-CRTC pairing, if a multi-display atomic commit
disables or reduces resources on a higher-indexed CRTC while enabling or
increasing resources on a lower-indexed CRTC, won't the lower-indexed CRTC
still attempt to reserve from global_state before the higher-indexed CRTC has
released its old resources?

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index da3556eb6ecc2..13099d6e3ac95 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -879,9 +879,18 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  		}
>  	}
>  
> -	/* All CRTCs are compatible with all encoders */
> -	drm_for_each_encoder(encoder, dev)
> -		encoder->possible_crtcs = (1 << dev->mode_config.num_crtc) - 1;
> +	/* All CRTCs are compatible with all encoders.
> +	 * Nevertheless, if enough CRTCs are available, pair up crtc/encoder
> +	 * for better resource re-assignments during atomic_check
> +	 * from userspace mode set.
> +	 */
> +	i = 0;
> +	drm_for_each_encoder(encoder, dev) {
> +		if (dev->mode_config.num_crtc >= num_encoders)
> +			encoder->possible_crtcs = 1 << i++;

[Severity: High]
Does assigning encoder->possible_crtcs = 1 << i++ break Concurrent Writeback
(CWB) clone mode on platforms where catalog->cwb_count > 0?

Earlier in _dpu_kms_drm_obj_init(), DSI and Writeback encoders are marked as
clones via dpu_encoder_get_clones():

    drm_for_each_encoder(encoder, dev) {
        num_encoders++;
        if (catalog->cwb_count > 0)
            encoder->possible_clones = dpu_encoder_get_clones(encoder);
    }

where drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_get_clones()
pairs DRM_MODE_ENCODER_DSI and DRM_MODE_ENCODER_VIRTUAL encoders so both can
attach to the same drm_crtc.

When dev->mode_config.num_crtc >= num_encoders, this loop gives every encoder
a mutually exclusive single-bit possible_crtcs mask. When userspace submits
an atomic commit attaching both a DSI encoder and a Writeback encoder to the
same CRTC for CWB, drm_atomic_helper_check_modeset() ->
update_connector_routing() checks drm_encoder_crtc_ok():

    if (!drm_encoder_crtc_ok(new_encoder, new_connector_state->crtc)) {
        drm_dbg_atomic(connector->dev,
                   "[ENCODER:%d:%s] incompatible with [CRTC:%d:%s]\n",
                   new_encoder->base.id,
                   new_encoder->name,
                   new_connector_state->crtc->base.id,
                   new_connector_state->crtc->name);
        return -EINVAL;
    }

Because the DSI and Writeback encoders no longer share any common CRTC bit in
possible_crtcs, won't all CWB clone commits be rejected with -EINVAL?

[Severity: Medium]
Also in _dpu_kms_drm_obj_init(), won't locking encoder i to CRTC i prevent
displays on higher-indexed encoders from using hardware cursor planes?

Earlier in _dpu_kms_drm_obj_init(), DRM_PLANE_TYPE_CURSOR planes are only
created for SSPPs with BIT(DPU_SSPP_CURSOR) (at most 1 or 2 across DPU
catalogs):

    if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR))
        && cursor_planes_idx < max_crtc_count)
        type = DRM_PLANE_TYPE_CURSOR;

and passed to dpu_crtc_init():

    for (i = 0; i < max_crtc_count; i++) {
        crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);

Therefore, CRTCs at index i >= cursor_planes_idx are initialized with
cursor_planes[i] == NULL (so crtc->cursor == NULL).

Because _dpu_kms_setup_displays() registers encoders in fixed controller
order (DSI, DP controllers, HDMI, Writeback), an active display on an encoder
at index i >= cursor_planes_idx (such as an eDP panel on dp[3] on multi-DP
laptops) is now forced onto CRTC i where crtc->cursor is NULL, even when
CRTC 0 and CRTC 1 are unused.

When userspace calls DRM_IOCTL_MODE_CURSOR or DRM_IOCTL_MODE_CURSOR2 on that
CRTC, drivers/gpu/drm/drm_plane.c:drm_mode_cursor_common() sees
crtc->cursor == NULL:

    if (crtc->cursor) {
        ...
        ret = drm_mode_cursor_universal(crtc, req, file_priv, &ctx);
        goto out;
    }

    if (req->flags & DRM_MODE_CURSOR_BO) {
        if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
            ret = -ENXIO;
            goto out;
        }

and fails with -ENXIO (or -EFAULT for DRM_MODE_CURSOR_MOVE) because
dpu_crtc_funcs does not implement legacy cursor callbacks, while also
preventing drm_atomic_helper_update_plane() from setting
state->legacy_cursor_update for async cursor updates in can_do_async().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-msm-dpu-res-clean-v1-0-f46130a8a717@gmail.com?part=6

Re: [PATCH 1/8] drm/msm/dpu: reset unassigned resource slots to NULL on topology update

[Val Packett]

On 10/26/26 4:16 PM, Xi Zhang wrote:
> While this is not currently the case, it may be possible that topology
> for cwb and dsc resource requirements change between mode set in the
> future.
>
> e.g. when dsc support is added to DisplayPort, dsc may be optionally
> enabled and disabled between different modes due to
> bandwidth requirements.
>
> Set unassigned resource slots to NULL on topology update to avoid
> issues in the future. It's good practice to do so anyway.
>
> Signed-off-by: Xi Zhang <xz.ca.cn@gmail.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 0a6be69a3e83..8d2b4d2726f2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1205,9 +1205,13 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   						       ARRAY_SIZE(hw_pp));
>   	}
>   
> -	for (i = 0; i < num_cwb; i++) {
> -		dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
> -		cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
> +	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) {
> +		if (i < num_cwb) {
> +			dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
> +			cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
> +		} else {
> +			dpu_enc->hw_cwb[i] = NULL;
> +		}
>   	}
>   
>   	dpu_enc->cwb_mask = cwb_mask;
> @@ -1215,16 +1219,20 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   	num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>   		drm_enc->crtc, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
>   
> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
> +	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_pp); i++)
>   		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
>   						: NULL;
>   
>   	num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>   						drm_enc->crtc, DPU_HW_BLK_DSC,
>   						hw_dsc, ARRAY_SIZE(hw_dsc));
> -	for (i = 0; i < num_dsc; i++) {
> -		dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
> -		dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
> +	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_dsc); i++) {
> +		if (i < num_dsc) {
> +			dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
> +			dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
> +		} else {
> +			dpu_enc->hw_dsc[i] = NULL;
> +		}
>   	}
>   
>   	dpu_enc->dsc_mask = dsc_mask;

The radxa 7.0.11 tree which has some DP DSC patches indeed touches this 
code with "drm/msm/dpu: Clear stale DSC resources during modesets":

https://github.com/radxa/kernel/commit/d0a8968dca16965395f4089a28d48ab6acde565e

(but it only clears hw_dsc, but in the hw_pp loop, for the assigned 
resources..)

[+cc sophon]

~val