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 <[email protected]> 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/[email protected]?part=1
