public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] Revert "drm/msm/dpu: try reserving the DSPP-less LM first"
@ 2026-02-13 23:02 Dmitry Baryshkov
  2026-02-14  3:59 ` Claude review: " Claude Code Review Bot
  2026-02-14  3:59 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Dmitry Baryshkov @ 2026-02-13 23:02 UTC (permalink / raw)
  To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
	Konrad Dybcio
  Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel, Val Packett

This reverts commit 42f62cd79578 ("drm/msm/dpu: try reserving the
DSPP-less LM first"). It seems on later DPUs using higher LMs require
some additional setup or conflicts with the hardware defaults. Val (and
other developers) reported blue screen on Hamoa (X1E80100) laptops.
Revert the offending commit until we understand, what is the issue.

Fixes: 42f62cd79578 ("drm/msm/dpu: try reserving the DSPP-less LM first")
Reported-by: Val Packett <val@packett.cool>
Closes: https://lore.kernel.org/r/33424a9d-10a6-4479-bba6-12f8ce60da1a@packett.cool
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 52 +++++++++-------------------------
 1 file changed, 14 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 451a4fcf3e65..7e77d88f8959 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -350,26 +350,28 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
 	return true;
 }
 
-static bool dpu_rm_find_lms(struct dpu_rm *rm,
-			    struct dpu_global_state *global_state,
-			    uint32_t crtc_id, bool skip_dspp,
-			    struct msm_display_topology *topology,
-			    int *lm_idx, int *pp_idx, int *dspp_idx)
+static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
+			       struct dpu_global_state *global_state,
+			       uint32_t crtc_id,
+			       struct msm_display_topology *topology)
 
 {
+	int lm_idx[MAX_BLOCKS];
+	int pp_idx[MAX_BLOCKS];
+	int dspp_idx[MAX_BLOCKS] = {0};
 	int i, lm_count = 0;
 
+	if (!topology->num_lm) {
+		DPU_ERROR("zero LMs in topology\n");
+		return -EINVAL;
+	}
+
 	/* Find a primary mixer */
 	for (i = 0; i < ARRAY_SIZE(rm->mixer_blks) &&
 			lm_count < topology->num_lm; i++) {
 		if (!rm->mixer_blks[i])
 			continue;
 
-		if (skip_dspp && to_dpu_hw_mixer(rm->mixer_blks[i])->cap->dspp) {
-			DPU_DEBUG("Skipping LM_%d, skipping LMs with DSPPs\n", i);
-			continue;
-		}
-
 		/*
 		 * Reset lm_count to an even index. This will drop the previous
 		 * primary mixer if failed to find its peer.
@@ -408,38 +410,12 @@ static bool dpu_rm_find_lms(struct dpu_rm *rm,
 		}
 	}
 
-	return lm_count == topology->num_lm;
-}
-
-static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
-			       struct dpu_global_state *global_state,
-			       uint32_t crtc_id,
-			       struct msm_display_topology *topology)
-
-{
-	int lm_idx[MAX_BLOCKS];
-	int pp_idx[MAX_BLOCKS];
-	int dspp_idx[MAX_BLOCKS] = {0};
-	int i;
-	bool found;
-
-	if (!topology->num_lm) {
-		DPU_ERROR("zero LMs in topology\n");
-		return -EINVAL;
-	}
-
-	/* Try using non-DSPP LM blocks first */
-	found = dpu_rm_find_lms(rm, global_state, crtc_id, !topology->num_dspp,
-				topology, lm_idx, pp_idx, dspp_idx);
-	if (!found && !topology->num_dspp)
-		found = dpu_rm_find_lms(rm, global_state, crtc_id, false,
-					topology, lm_idx, pp_idx, dspp_idx);
-	if (!found) {
+	if (lm_count != topology->num_lm) {
 		DPU_DEBUG("unable to find appropriate mixers\n");
 		return -ENAVAIL;
 	}
 
-	for (i = 0; i < topology->num_lm; i++) {
+	for (i = 0; i < lm_count; i++) {
 		global_state->mixer_to_crtc_id[lm_idx[i]] = crtc_id;
 		global_state->pingpong_to_crtc_id[pp_idx[i]] = crtc_id;
 		global_state->dspp_to_crtc_id[dspp_idx[i]] =

---
base-commit: 50c4a49f7292b33b454ea1a16c4f77d6965405dc
change-id: 20260214-revert-dspp-less-8426dee24b92

Best regards,
-- 
With best wishes
Dmitry


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: Revert "drm/msm/dpu: try reserving the DSPP-less LM first"
  2026-02-13 23:02 [PATCH] Revert "drm/msm/dpu: try reserving the DSPP-less LM first" Dmitry Baryshkov
@ 2026-02-14  3:59 ` Claude Code Review Bot
  2026-02-14  3:59 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-02-14  3:59 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: Revert "drm/msm/dpu: try reserving the DSPP-less LM first"
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 1
Reviewed: 2026-02-14T13:59:51.835481

---

This is a single-patch revert of commit 42f62cd79578 ("drm/msm/dpu: try reserving the DSPP-less LM first"), which introduced an optimization to prefer layer mixers (LMs) without DSPPs when DSPPs weren't needed by the topology. The revert is motivated by reports of blue screens on Hamoa (X1E80100) laptops, indicating that the optimization exposed a hardware dependency or initialization issue with higher-numbered LMs on newer DPU generations.

The revert is clean and straightforward. It removes the `dpu_rm_find_lms()` helper function and inlines the mixer search loop back into `_dpu_rm_reserve_lms()`, removing the `skip_dspp` parameter and the two-pass search strategy. The resulting code matches what the pre-optimization code looked like, restoring the simple linear scan of mixer blocks.

No significant issues found. The patch is a correct revert addressing a real regression.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: Revert "drm/msm/dpu: try reserving the DSPP-less LM first"
  2026-02-13 23:02 [PATCH] Revert "drm/msm/dpu: try reserving the DSPP-less LM first" Dmitry Baryshkov
  2026-02-14  3:59 ` Claude review: " Claude Code Review Bot
@ 2026-02-14  3:59 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-02-14  3:59 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

The commit message is clear about the motivation: blue screens on X1E80100 laptops, with a proper Fixes tag and Reported-by/Closes links.

The revert collapses the two-function design (`dpu_rm_find_lms` + `_dpu_rm_reserve_lms`) back into a single `_dpu_rm_reserve_lms` function. Looking at the diff:

> -static bool dpu_rm_find_lms(struct dpu_rm *rm,
> -			    struct dpu_global_state *global_state,
> -			    uint32_t crtc_id, bool skip_dspp,
> -			    struct msm_display_topology *topology,
> -			    int *lm_idx, int *pp_idx, int *dspp_idx)
> +static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> +			       struct dpu_global_state *global_state,
> +			       uint32_t crtc_id,
> +			       struct msm_display_topology *topology)

The separate `dpu_rm_find_lms` function is removed and its loop body is folded back into `_dpu_rm_reserve_lms`, with the `skip_dspp` filtering removed entirely:

> -		if (skip_dspp && to_dpu_hw_mixer(rm->mixer_blks[i])->cap->dspp) {
> -			DPU_DEBUG("Skipping LM_%d, skipping LMs with DSPPs\n", i);
> -			continue;
> -		}

This was the core of the optimization -- skipping DSPP-capable LMs when the topology doesn't need DSPPs. Removing this restores the original behavior of allocating LMs in their natural order.

> -	/* Try using non-DSPP LM blocks first */
> -	found = dpu_rm_find_lms(rm, global_state, crtc_id, !topology->num_dspp,
> -				topology, lm_idx, pp_idx, dspp_idx);
> -	if (!found && !topology->num_dspp)
> -		found = dpu_rm_find_lms(rm, global_state, crtc_id, false,
> -					topology, lm_idx, pp_idx, dspp_idx);

The two-pass fallback strategy (try without DSPP-capable LMs first, then fall back to any LM) is eliminated.

> -	for (i = 0; i < topology->num_lm; i++) {
> +	for (i = 0; i < lm_count; i++) {

This change in the reservation loop from `topology->num_lm` to `lm_count` is equivalent since the code reaches this point only when `lm_count == topology->num_lm` (verified by the check just above). No functional difference.

One minor note on the commit message: "It seems on later DPUs using higher LMs require some additional setup or conflicts with the hardware defaults" -- this is slightly awkward grammar ("using higher LMs require" could be "using higher LMs requires"), but it's understandable and this is a cosmetic observation.

No correctness issues found. The revert is a clean, mechanical undo of the original optimization and correctly restores the previous LM allocation behavior.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-02-14  3:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-13 23:02 [PATCH] Revert "drm/msm/dpu: try reserving the DSPP-less LM first" Dmitry Baryshkov
2026-02-14  3:59 ` Claude review: " Claude Code Review Bot
2026-02-14  3:59 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox