[PATCH] drm/msm/dp: Skip push_idle in atomic_disable if display is not powered on

[PATCH] drm/msm/dp: Skip push_idle in atomic_disable if display is not powered on

[Yongxing Mou]

On eDP interfaces with no panel connected, panel-simple-dp-aux fails to
read DPCD over AUX during probe, leaving hpd_state at ST_DISCONNECTED.
msm_dp_bridge_atomic_enable() exits early as a result, so
msm_dp_display_enable() is never called and power_on remains false with
link clocks never enabled.

During system suspend, drm_atomic_helper_suspend() still invokes
atomic_disable() for all registered bridges. The resulting write to
REG_DP_STATE_CTRL against an unpowered controller triggers a NoC slave
error, causing a fatal crash.

Guard push_idle with a power_on check to skip the register access when the
display was never enabled.

Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 1b3cbf4016ef..51b60eb9c2df 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1660,6 +1660,13 @@ void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
+	/*
+	 * push_idle accesses link registers which require link clocks to be on.
+	 * Skip if the display was never powered on (e.g. eDP with no panel).
+	 */
+	if (!dp->power_on)
+		return;
+
 	msm_dp_ctrl_push_idle(msm_dp_display->ctrl);
 }
 

---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260511-edp_suspend-0e45d0110633

Best regards,
-- 
Yongxing Mou <yongxing.mou@oss.qualcomm.com>

Re: [PATCH] drm/msm/dp: Skip push_idle in atomic_disable if display is not powered on

[Dmitry Baryshkov]

On Mon, May 11, 2026 at 04:36:03PM +0800, Yongxing Mou wrote:
> On eDP interfaces with no panel connected, panel-simple-dp-aux fails to
> read DPCD over AUX during probe, leaving hpd_state at ST_DISCONNECTED.
> msm_dp_bridge_atomic_enable() exits early as a result, so

Why is atomic_enable() called for disconnected bridge?

> msm_dp_display_enable() is never called and power_on remains false with
> link clocks never enabled.
> 
> During system suspend, drm_atomic_helper_suspend() still invokes
> atomic_disable() for all registered bridges. The resulting write to
> REG_DP_STATE_CTRL against an unpowered controller triggers a NoC slave
> error, causing a fatal crash.
> 
> Guard push_idle with a power_on check to skip the register access when the
> display was never enabled.
> 
> Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

Fixes?

> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 1b3cbf4016ef..51b60eb9c2df 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1660,6 +1660,13 @@ void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
>  
>  	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>  
> +	/*
> +	 * push_idle accesses link registers which require link clocks to be on.
> +	 * Skip if the display was never powered on (e.g. eDP with no panel).
> +	 */
> +	if (!dp->power_on)
> +		return;
> +
>  	msm_dp_ctrl_push_idle(msm_dp_display->ctrl);
>  }
>  
> 
> ---
> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> change-id: 20260511-edp_suspend-0e45d0110633
> 
> Best regards,
> -- 
> Yongxing Mou <yongxing.mou@oss.qualcomm.com>
> 

-- 
With best wishes
Dmitry

Claude review: drm/msm/dp: Skip push_idle in atomic_disable if display is not powered on

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dp: Skip push_idle in atomic_disable if display is not powered on
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
Patches: 2
Reviewed: 2026-05-16T15:42:51.497192

---

This is a single-patch fix for a real crash scenario on Qualcomm eDP interfaces. When an eDP connector has no panel attached, the probe-time DPCD read fails, leaving `hpd_state` at `ST_DISCONNECTED`. Consequently `msm_dp_bridge_atomic_enable()` exits early, `power_on` stays `false`, and link clocks are never enabled. During system suspend, `drm_atomic_helper_suspend()` still calls `atomic_disable()` on all registered bridges, and `msm_dp_ctrl_push_idle()` writes to `REG_DP_STATE_CTRL` on an unpowered controller — triggering a NoC slave error and a fatal crash.

The fix is straightforward and correct in principle: guard `msm_dp_ctrl_push_idle()` with a `power_on` check. This is consistent with how `msm_dp_display_disable()` (called from the subsequent `atomic_post_disable`) already guards itself with `if (!msm_dp_display->power_on) return 0;`.

**Verdict: The patch looks correct and the approach is reasonable.** One minor concern below.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dp: Skip push_idle in atomic_disable if display is not powered on

[Claude Code Review Bot]

Patch Review

**Commit message:** Well-written. It clearly explains the root cause (eDP with no panel → DPCD read failure → `ST_DISCONNECTED` → `atomic_enable` exits early → `power_on` stays false → `push_idle` writes to unpowered register → NoC error). Good.

**Code change:**

```c
+	if (!dp->power_on)
+		return;
+
 	msm_dp_ctrl_push_idle(msm_dp_display->ctrl);
```

**Correctness:** The guard is logically correct. If `power_on` is `false`, the link clocks are not enabled, so writing to `REG_DP_STATE_CTRL` via `msm_dp_ctrl_push_idle()` would access unpowered hardware. The check is consistent with the guard in `msm_dp_display_disable()` which does the same pattern (`if (!msm_dp_display->power_on) return 0;`).

**Minor concern — early return skips post_disable flow:** The `atomic_disable` → `atomic_post_disable` sequence means that even when we return early here, `msm_dp_bridge_atomic_post_disable()` will still be called. That function calls `msm_dp_display_disable()`, which has its own `power_on` guard, so the early return here is safe — `atomic_post_disable` won't crash either. The `pm_runtime_put_sync()` at the end of `atomic_post_disable` could be called without a matching `pm_runtime_get`, but looking at the enable path, `pm_runtime_get_sync` is called in `atomic_pre_enable`, which would also have been called before `atomic_post_disable` during the suspend teardown. So this should be balanced correctly.

**Nit — comment length:** The 3-line comment is fine for explaining a non-obvious guard, though it could be slightly more concise. This is a minor style preference and not a blocker.

**No other issues found.** The patch is minimal, well-scoped, and addresses a real crash. It could benefit from a `Fixes:` tag referencing the commit that introduced the unconditional `msm_dp_ctrl_push_idle()` call in `atomic_disable`, which would help stable backporting.

---
Generated by Claude Code Patch Reviewer