[PATCH] drm/msm/dpu: stop all video interfaces before cleaning up a split encoder

[PATCH] drm/msm/dpu: stop all video interfaces before cleaning up a split encoder

[Joonhoe Kim]

dpu_encoder_virt_atomic_disable() disables the physical encoders one by
one. For a video-mode master, dpu_encoder_phys_vid_disable() stops its
timing engine, waits for the frame to finish and then runs
dpu_encoder_helper_phys_cleanup(), which resets the CTL. With a split
display (two interfaces driven from one CTL, e.g. bonded DSI) that CTL
is shared with the slave, whose timing engine is still running at that
point: the source pipe starts fetching the slave's next frame and is
left stalled half-way through it (on SM8850, SSPP_CMN_STATUS 0x10030
with the fetch and unpack counters frozen, where an idle pipe shows
0x10003).

The stall is cleared by a power collapse of the MDSS core GDSC, which
normally happens between a disable and the next enable, so it goes
unnoticed. When MDSS stays powered across the disable -- a full modeset
within one commit, or another runtime-active user of MDSS such as the
DP controller -- the next enable of the bonded DSI panel scans out
nothing: the DPU keeps committing frames, the layer mixers produce no
output (CRC 0), and the panel shows black with the backlight on. A CTL
reset at enable does not clear it.

Stop the timing engine of every video interface of the encoder before
any of them is cleaned up. The master's existing wait for the frame to
complete then covers both halves.

Seen on a Lenovo Legion Tab Y700 gen 5 (TB323FU, SM8850) with a bonded
DSI video-mode panel (CSOT PP8807HB1-1). It reproduces without any
external display by keeping MDSS runtime-active:

  echo on > /sys/bus/platform/devices/9800000.display-subsystem/power/control

then DPMS off and on from the compositor: black 3/3 before this change.
Every full modeset (e.g. a refresh rate change) went black the same way.
With this change: DPMS off/on 13/13 and full modesets 4/4 show the
picture, and an attached DP display is unaffected. Only tested on this
device.

Fixes: 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine disable")
Assisted-by: LLM
Signed-off-by: Joonhoe Kim <26rote@gmail.com>
---
Saim Shujah's "drm/msm/dpu: clear pending flush state before physical cleanup"
(https://lore.kernel.org/all/20260826182459.1506522-1-saimzst@gmail.com/)
alone does not help here: still black 3/3 with only that change.

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1f20695f81e3..a14156408126 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1380,6 +1380,35 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
 	mutex_unlock(&dpu_enc->enc_lock);
 }
 
+/*
+ * Stop the timing engine of every video-mode interface of the encoder before
+ * any of them is cleaned up. With a split display (two interfaces on one CTL,
+ * e.g. bonded DSI) the master's cleanup resets the shared CTL while the
+ * slave's timing engine would still be running; the source pipes then start
+ * fetching the slave's next frame and stall half-way through it. The stall
+ * survives until the MDSS core GDSC is power-collapsed, so when something else
+ * keeps MDSS powered (an active DP controller) the next enable scans out
+ * nothing.
+ */
+static void dpu_encoder_stop_video_timing(struct dpu_encoder_virt *dpu_enc)
+{
+	unsigned long lock_flags;
+	int i;
+
+	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+		if (phys->intf_mode != INTF_MODE_VIDEO || !phys->hw_intf ||
+		    !phys->hw_intf->ops.enable_timing ||
+		    phys->enable_state == DPU_ENC_DISABLED)
+			continue;
+
+		spin_lock_irqsave(phys->enc_spinlock, lock_flags);
+		phys->hw_intf->ops.enable_timing(phys->hw_intf, 0);
+		spin_unlock_irqrestore(phys->enc_spinlock, lock_flags);
+	}
+}
+
 static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
 					struct drm_atomic_commit *state)
 {
@@ -1412,6 +1441,9 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
 
 	dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_PRE_STOP);
 
+	if (dpu_enc->num_phys_encs > 1)
+		dpu_encoder_stop_video_timing(dpu_enc);
+
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 

base-commit: 6375e61c01e93e35ee7acd336a689ac1fae4b509
-- 
2.43.0

Re: [PATCH] drm/msm/dpu: stop all video interfaces before cleaning up a split encoder

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dpu: race condition causes 84ms timeout during display disable
--

commit 9a915625643ca9da58dbf62e57c9c0b79c93bceb
Author: Joonhoe Kim <26rote@gmail.com>

drm/msm/dpu: stop all video interfaces before cleaning up a split encoder

dpu_encoder_virt_atomic_disable() disables the physical encoders one by
one. For a video-mode master, dpu_encoder_phys_vid_disable() stops its
timing engine, waits for the frame to finish and then runs
dpu_encoder_helper_phys_cleanup(), which resets the CTL. With a split
display that CTL is shared with the slave, whose timing engine is still
running at that point, causing the source pipe to stall.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1f20695f81e35..a14156408126b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
[ ... ]
> @@ -1380,6 +1380,35 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>  	mutex_unlock(&dpu_enc->enc_lock);
>  }
>  
> +/*
> + * Stop the timing engine of every video-mode interface of the encoder before
> + * any of them is cleaned up. With a split display (two interfaces on one CTL,
> + * e.g. bonded DSI) the master's cleanup resets the shared CTL while the
> + * slave's timing engine would still be running; the source pipes then start
> + * fetching the slave's next frame and stall half-way through it. The stall
> + * survives until the MDSS core GDSC is power-collapsed, so when something else
> + * keeps MDSS powered (an active DP controller) the next enable scans out
> + * nothing.
> + */
> +static void dpu_encoder_stop_video_timing(struct dpu_encoder_virt *dpu_enc)
> +{
> +	unsigned long lock_flags;
> +	int i;
> +
> +	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> +		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> +		if (phys->intf_mode != INTF_MODE_VIDEO || !phys->hw_intf ||
> +		    !phys->hw_intf->ops.enable_timing ||
> +		    phys->enable_state == DPU_ENC_DISABLED)
> +			continue;
> +
> +		spin_lock_irqsave(phys->enc_spinlock, lock_flags);
> +		phys->hw_intf->ops.enable_timing(phys->hw_intf, 0);
> +		spin_unlock_irqrestore(phys->enc_spinlock, lock_flags);

[Severity: Medium]
Will this lead to an 84ms timeout during display disable?

By stopping the timing engine here without incrementing pending_kickoff_cnt,
we open a window where a final vsync interrupt could fire.

In dpu_encoder_phys_vid_vblank_irq(), the IRQ handler explicitly ignores the
vsync if pending_kickoff_cnt is 0:

	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
		atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);

If the final vsync fires before dpu_encoder_phys_vid_disable() is called, it
will be ignored.

Then, when dpu_encoder_phys_vid_disable() runs, it increments the counter
and waits for a vsync that already happened:

	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
	phys_enc->hw_intf->ops.enable_timing(phys_enc->hw_intf, 0);
	if (dpu_encoder_phys_vid_is_master(phys_enc))
		dpu_encoder_phys_inc_pending(phys_enc);
	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);

	/* Wait for a vsync so we know the ENABLE=0 latched... */
	if (dpu_encoder_phys_vid_is_master(phys_enc)) {
		ret = dpu_encoder_phys_vid_wait_for_tx_complete(phys_enc);

Could this result in the vsync being permanently missed, causing the wait
to timeout?

> +	}
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929130621.943-1-26rote@gmail.com?part=1

Re: [PATCH] drm/msm/dpu: stop all video interfaces before cleaning up a split encoder

[Dmitry Baryshkov]

On Tue, Sep 29, 2026 at 10:06:21PM +0900, Joonhoe Kim wrote:
> dpu_encoder_virt_atomic_disable() disables the physical encoders one by
> one. For a video-mode master, dpu_encoder_phys_vid_disable() stops its
> timing engine, waits for the frame to finish and then runs
> dpu_encoder_helper_phys_cleanup(), which resets the CTL. With a split
> display (two interfaces driven from one CTL, e.g. bonded DSI) that CTL
> is shared with the slave, whose timing engine is still running at that
> point: the source pipe starts fetching the slave's next frame and is
> left stalled half-way through it (on SM8850, SSPP_CMN_STATUS 0x10030
> with the fetch and unpack counters frozen, where an idle pipe shows
> 0x10003).
> 
> The stall is cleared by a power collapse of the MDSS core GDSC, which
> normally happens between a disable and the next enable, so it goes
> unnoticed. When MDSS stays powered across the disable -- a full modeset
> within one commit, or another runtime-active user of MDSS such as the
> DP controller -- the next enable of the bonded DSI panel scans out
> nothing: the DPU keeps committing frames, the layer mixers produce no
> output (CRC 0), and the panel shows black with the backlight on. A CTL
> reset at enable does not clear it.
> 
> Stop the timing engine of every video interface of the encoder before
> any of them is cleaned up. The master's existing wait for the frame to
> complete then covers both halves.
> 
> Seen on a Lenovo Legion Tab Y700 gen 5 (TB323FU, SM8850) with a bonded
> DSI video-mode panel (CSOT PP8807HB1-1). It reproduces without any
> external display by keeping MDSS runtime-active:
> 
>   echo on > /sys/bus/platform/devices/9800000.display-subsystem/power/control
> 
> then DPMS off and on from the compositor: black 3/3 before this change.
> Every full modeset (e.g. a refresh rate change) went black the same way.
> With this change: DPMS off/on 13/13 and full modesets 4/4 show the
> picture, and an attached DP display is unaffected. Only tested on this
> device.
> 
> Fixes: 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine disable")
> Assisted-by: LLM
> Signed-off-by: Joonhoe Kim <26rote@gmail.com>
> ---
> Saim Shujah's "drm/msm/dpu: clear pending flush state before physical cleanup"
> (https://lore.kernel.org/all/20260826182459.1506522-1-saimzst@gmail.com/)
> alone does not help here: still black 3/3 with only that change.
> 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1f20695f81e3..a14156408126 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1380,6 +1380,35 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>  	mutex_unlock(&dpu_enc->enc_lock);
>  }
>  
> +/*
> + * Stop the timing engine of every video-mode interface of the encoder before
> + * any of them is cleaned up. With a split display (two interfaces on one CTL,
> + * e.g. bonded DSI) the master's cleanup resets the shared CTL while the
> + * slave's timing engine would still be running; the source pipes then start
> + * fetching the slave's next frame and stall half-way through it. The stall
> + * survives until the MDSS core GDSC is power-collapsed, so when something else
> + * keeps MDSS powered (an active DP controller) the next enable scans out
> + * nothing.
> + */

Please instruct your AI to stop generating the narrative comments which
duplicate commit messages. Otherwise LGTM (please respond to Sashiko
though).

> +static void dpu_encoder_stop_video_timing(struct dpu_encoder_virt *dpu_enc)
> +{
> +	unsigned long lock_flags;
> +	int i;
> +
> +	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> +		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> +		if (phys->intf_mode != INTF_MODE_VIDEO || !phys->hw_intf ||
> +		    !phys->hw_intf->ops.enable_timing ||
> +		    phys->enable_state == DPU_ENC_DISABLED)
> +			continue;
> +
> +		spin_lock_irqsave(phys->enc_spinlock, lock_flags);
> +		phys->hw_intf->ops.enable_timing(phys->hw_intf, 0);
> +		spin_unlock_irqrestore(phys->enc_spinlock, lock_flags);
> +	}
> +}
> +
>  static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
>  					struct drm_atomic_commit *state)
>  {

-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm/dpu: stop all video interfaces before cleaning up a split encoder

[Joonhoe Kim]

On Tue, 29 Sep 2026 13:18:40 +0000, sashiko-bot@kernel.org wrote:
> Could this result in the vsync being permanently missed, causing the wait
> to timeout?

Yes, the window is real; I did not hit it in 13 cycles, but nothing
prevents it. v2 stopped only the slave interfaces up front, and v3
follows Dmitry's suggestion and disables the slave before the master,
so no timing engine is stopped outside its own disable path:
https://lore.kernel.org/all/20261002130712.50612-1-26rote@gmail.com/

Thanks,
Joonhoe