[PATCH] drm/msm/dpu: delete timeout timer when encoder is not busy

[PATCH] drm/msm/dpu: delete timeout timer when encoder is not busy

[Jun Nie]

There is a spurious timeout error message under the following reproduction
steps:
1. Run "modetest -M msm -r" and press CTRL+Z to pause it.
2. Run "while true; do rtcwake -m mem -s 3 -v; sleep 2; done"

Resulting error message:
[  124.018206] [drm:dpu_encoder_virt_atomic_disable:1425] [dpu error]enc35 timeout pending

dpu_crtc_commit_kickoff() calls dpu_encoder_kickoff() to set frame busy
bits, and then arms the frame done timer. If the frame done IRQ fires
between these two steps, the IRQ handler clears the busy bits before the
timer is armed. As a result, the timer runs with busy bits already cleared
and is never cancelled by subsequent IRQ handlers, leading to a false
timeout warning.

Delete the pending timer when all busy bits are cleared in the frame done
callback to prevent false timeouts.

Fixes: 95bbde1d0d07 ("drm/msm/dpu: Start frame done timer after encoder kickoff")
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 +++++++++++++++
 1 file changed, 15 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..d83d38797331 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1573,6 +1573,21 @@ void dpu_encoder_frame_done_callback(
 			| DPU_ENCODER_FRAME_EVENT_PANEL_DEAD)) {
 
 		if (!dpu_enc->frame_busy_mask[0]) {
+			/*
+			 * dpu_crtc_commit_kickoff calls dpu_encoder_kickoff to
+			 * mark busy bits, starts the framedone timer next.
+			 * It is possibile that irq happens between the 2
+			 * operations. Thus timer is running with busy bits
+			 * cleared by irq handler and timer will not be deleted
+			 * anymore. Then false timeout introduces unnecessary
+			 * confusion and visual defect.
+			 * delete the timer here to fix it.
+			 */
+			if (atomic_read(&dpu_enc->frame_done_timeout_ms)) {
+				atomic_set(&dpu_enc->frame_done_timeout_ms, 0);
+				timer_delete(&dpu_enc->frame_done_timer);
+			}
+
 			/**
 			 * suppress frame_done without waiter,
 			 * likely autorefresh
-- 
2.53.0

Re: [PATCH] drm/msm/dpu: delete timeout timer when encoder is not busy

[Jun Nie]

Jun Nie <jun.nie@linaro.org> 于2026年9月3日周四 22:30写道：
>
> There is a spurious timeout error message under the following reproduction
> steps:
> 1. Run "modetest -M msm -r" and press CTRL+Z to pause it.
> 2. Run "while true; do rtcwake -m mem -s 3 -v; sleep 2; done"
>
> Resulting error message:
> [  124.018206] [drm:dpu_encoder_virt_atomic_disable:1425] [dpu error]enc35 timeout pending
>
> dpu_crtc_commit_kickoff() calls dpu_encoder_kickoff() to set frame busy
> bits, and then arms the frame done timer. If the frame done IRQ fires
> between these two steps, the IRQ handler clears the busy bits before the
> timer is armed. As a result, the timer runs with busy bits already cleared
> and is never cancelled by subsequent IRQ handlers, leading to a false
> timeout warning.
>
> Delete the pending timer when all busy bits are cleared in the frame done
> callback to prevent false timeouts.
>

Another option is to wrap the enc_spinlock lock/unlock pair in dpu_encoder.c
and expose it to dpu_crtc.c to make the sequence of dpu_encoder_kickoff()
and dpu_encoder_start_frame_done_timer() atomic against the IRQ handler.
However, this breaks encoder self-containment to some extent. Furthermore,
initial testing revealed a deadlock involving additional locks beyond
dpu_enc->enc_spinlock.

- Jun

Re: [PATCH] drm/msm/dpu: delete timeout timer when encoder is not busy

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in the lockless timer deletion allows an IRQ handler to inadvertently delete the watchdog timer for a newly started frame on another CPU.
- [Medium] The patch fails to address the root cause of the race condition and relies on a subsequent, unrelated IRQ to mask the bug, leaving the original false timeout issue unfixed in many scenarios.

Pre-existing issues:
- [High] Missing synchronization in `dpu_encoder_virt_atomic_disable` allows the driver to disable hardware resources while the timer callback is still executing, leading to crashes.
--

commit 91dd8b2e3a127693bc37ea9250d6d6a4bd503cf0
Author: Jun Nie <jun.nie@linaro.org>

drm/msm/dpu: delete timeout timer when encoder is not busy

This commit attempts to fix a spurious timeout error message during encoder
disable. It adds a check in the frame done IRQ callback to delete the
watchdog timer if the frame busy bits are cleared.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1f20695f81e35..d83d387973311 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1573,6 +1573,21 @@ void dpu_encoder_frame_done_callback(
>  			| DPU_ENCODER_FRAME_EVENT_PANEL_DEAD)) {
>  
>  		if (!dpu_enc->frame_busy_mask[0]) {
> +			/*
> +			 * dpu_crtc_commit_kickoff calls dpu_encoder_kickoff to
> +			 * mark busy bits, starts the framedone timer next.
> +			 * It is possibile that irq happens between the 2
> +			 * operations. Thus timer is running with busy bits
> +			 * cleared by irq handler and timer will not be deleted
> +			 * anymore. Then false timeout introduces unnecessary
> +			 * confusion and visual defect.
> +			 * delete the timer here to fix it.
> +			 */
> +			if (atomic_read(&dpu_enc->frame_done_timeout_ms)) {
> +				atomic_set(&dpu_enc->frame_done_timeout_ms, 0);
> +				timer_delete(&dpu_enc->frame_done_timer);
> +			}

[Severity: Medium]
Does this patch rely on a subsequent IRQ to fix the false timeout in all
scenarios?

If no subsequent IRQ (like autorefresh or a TE interrupt) fires after the
timer is armed with an empty frame_busy_mask, the timer will expire normally.
When it expires, dpu_encoder_frame_done_timeout() returns early because
frame_busy_mask is empty, but it fails to clear frame_done_timeout_ms:

dpu_encoder_frame_done_timeout() {
    ...
    if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc) {
        return;
    }
    ...
}

Later, dpu_encoder_virt_atomic_disable() checks frame_done_timeout_ms,
finds it non-zero, and still falsely prints the 'timeout pending' error.
Will this leave the bug unfixed for standard operations that do not trigger
extra IRQs?

[Severity: High]
Could this lockless timer deletion introduce a race condition with a new
commit?

CPU1 handling a spurious IRQ in dpu_encoder_frame_done_callback()
evaluates if (!dpu_enc->frame_busy_mask[0]) as true locklessly.

Concurrently, CPU2 starts a new commit:
dpu_encoder_kickoff() sets frame_busy_mask to 1.
dpu_encoder_start_frame_done_timer() arms the timer and sets
frame_done_timeout_ms to 500.

CPU1 continues execution, reads frame_done_timeout_ms (now 500 from CPU2),
sets it to 0, and calls timer_delete().

Does this silently delete the newly armed watchdog timer for CPU2's commit,
leaving it without watchdog protection if the hardware hangs?

[Severity: High]
This is a pre-existing issue, but does dpu_encoder_virt_atomic_disable()
lack synchronization when disabling hardware resources?

dpu_encoder_frame_done_timeout() clears frame_done_timeout_ms via
atomic_xchg() early in its execution:

dpu_encoder_frame_done_timeout() {
    ...
    } else if (!atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
        return;
    }
    ...
}

If dpu_encoder_virt_atomic_disable() runs concurrently, it checks:

    if (atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
        timer_delete_sync(&dpu_enc->frame_done_timer);
    }

If the timeout callback has already cleared the variable, the disable path
sees 0, skips timer_delete_sync(), and proceeds to disable clocks and power
domains. Meanwhile, the timeout callback is still executing and accesses
hardware registers, which can cause a synchronous external abort when clocks
are disabled. Should this path always synchronize with the timer callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903143004.1026181-1-jun.nie@linaro.org?part=1