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

[sashiko-bot@kernel.org]

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