drm/msm/dpu: Fix vblank refcount mismatch
PATCH 1/1 drm/msm/dpu: Fix vblank refcount mismatch
_dpu_crtc_vblank_enable_no_lock releases crtc_lock as its needed for power handle operations. This opens up a window where in a thread running dpu_crtc_disable and a thread running dpu_crtc_vblank can race in using dpu_crtc->enabled.
dpu_crtc_disable will change the state, where as dpu_crtc_vblank use the variable. The fix is to cache the crtc enabled state while holding the lock and use it as a gate in calling _dpu_crtc_vblank_enable_no_lock.
This issue was introduced with the commit cf871c48 (drm/msm/dpu: Remove suspend state tracking from crtc).
Below are stack traces of thread 1 and thread 2 in good case and bad case:
Bad case:
-------------
Thread 1
dpu_encoder_phys_vid_control_vblank_irq+0xd0/0x170 dpu_encoder_register_vblank_callback+0xb8/0x100 _dpu_crtc_vblank_enable_no_lock+0x240/0x288 dpu_crtc_disable+0xc4/0x288 drm_atomic_helper_commit_modeset_disables+0x19c/0x350 msm_atomic_commit_tail+0x48/0x144 commit_tail+0x44/0x70 drm_atomic_helper_commit+0xf0/0xf8 drm_atomic_commit+0x40/0x4c drm_mode_atomic_ioctl+0x374/0x90c drm_ioctl_kernel+0xac/0xec drm_ioctl+0x218/0x384 drm_compat_ioctl+0xd8/0xe8
Thread 2:
dpu_encoder_phys_vid_control_vblank_irq+0x74/0x170 dpu_encoder_register_vblank_callback+0xb8/0x100 _dpu_crtc_vblank_enable_no_lock+0x240/0x288 dpu_crtc_vblank+0xa8/0x118 dpu_kms_disable_vblank+0x20/0x2c vblank_ctrl_worker+0xa0/0xe0 kthread_worker_fn+0xe4/0x1a4 kthread+0x11c/0x12c ret_from_fork+0x10/0x18
Good case:
--------------
Thread 1:
dpu_encoder_phys_vid_control_vblank_irq+0xd0/0x170 dpu_encoder_phys_vid_irq_control+0xc8/0x110 _dpu_encoder_irq_control+0x48/0xa0 _dpu_encoder_resource_control_helper+0xb4/0x10c dpu_encoder_resource_control+0x4e0/0x664 dpu_encoder_virt_enable+0xb8/0x120 dpu_kms_encoder_enable+0x34/0xcc drm_atomic_helper_commit_modeset_enables+0x120/0x1b8 msm_atomic_commit_tail+0x5c/0x144 commit_tail+0x44/0x70 drm_atomic_helper_commit+0xf0/0xf8 drm_atomic_commit+0x40/0x4c drm_mode_atomic_ioctl+0x374/0x90c
Thread 2:
dpu_crtc_vblank+0xc8/0x118 dpu_kms_disable_vblank+0x20/0x2c vblank_ctrl_worker+0xa0/0xe0 kthread_worker_fn+0xe4/0x1a4 kthread+0x11c/0x12c
--- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +14 -3
@@ -877,6 +877,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)struct drm_encoder *encoder;struct msm_drm_private *priv;unsigned long flags; +bool crtc_en;if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {DPU_ERROR ("invalid crtc\n" );
@@ -901,11 +902,21 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) atomic_read(&dpu_crtc->frame_pending));trace_dpu_crtc_disable (DRMID (crtc), false, dpu_crtc); -if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { -_dpu_crtc_vblank_enable_no_lock (dpu_crtc, false); - } + +/* +* Cache vblank enabled before calling _dpu_crtc_vblank_enable_no_lock, +* because we release crtc_lock inside and acquire it back. While lock +* is released, there are cases where dpu_crtc_vblank comes in between +* while disable is going on. dpu_crtc_vblank further calls +* _dpu_crtc_vblank_enable_no_lock which tries vblank disable again +* resulting in refcount mismatch. +*/ + crtc_en = dpu_crtc->enabled; dpu_crtc->enabled = false; +if (crtc_en && dpu_crtc->vblank_requested) +_dpu_crtc_vblank_enable_no_lock (dpu_crtc, false); +if (atomic_read (&dpu_crtc->frame_pending)) {trace_dpu_crtc_disable_frame_pending (DRMID (crtc),atomic_read (&dpu_crtc->frame_pending));
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Sean Paul
reviewerQuoted 4 lines · Jayant Shekhar
_dpu_crtc_vblank_enable_no_lockreleasescrtc_lockas its needed for power handle operations. This opens up a window where in a thread runningdpu_crtc_disableand a thread runningdpu_crtc_vblankcan race in usingdpu_crtc->enabled.
Looks like you're using an old kernel. Both power_handle and vblank_enable_no_lock were removed.
If you want something more up-to-date for testing, grab the mtp-testing branch from my dpu-staging tree [1]
Sean
[1]- https://gitlab.freedesktop.org/seanpaul/dpu-staging.git
Quoted 110 lines
dpu_crtc_disablewill change the state, where asdpu_crtc_vblankuse the variable. The fix is to cache the crtc enabled state while holding the lock and use it as a gate in calling_dpu_crtc_vblank_enable_no_lock.This issue was introduced with the commit
cf871c48(drm/msm/dpu: Remove suspend state tracking from crtc).Below are stack traces of thread 1 and thread 2 in good case and bad case:
Bad case:
-------------
Thread 1dpu_encoder_phys_vid_control_vblank_irq+0xd0/0x170 dpu_encoder_register_vblank_callback+0xb8/0x100 _dpu_crtc_vblank_enable_no_lock+0x240/0x288 dpu_crtc_disable+0xc4/0x288 drm_atomic_helper_commit_modeset_disables+0x19c/0x350 msm_atomic_commit_tail+0x48/0x144 commit_tail+0x44/0x70 drm_atomic_helper_commit+0xf0/0xf8 drm_atomic_commit+0x40/0x4c drm_mode_atomic_ioctl+0x374/0x90c drm_ioctl_kernel+0xac/0xec drm_ioctl+0x218/0x384 drm_compat_ioctl+0xd8/0xe8Thread 2:
dpu_encoder_phys_vid_control_vblank_irq+0x74/0x170 dpu_encoder_register_vblank_callback+0xb8/0x100 _dpu_crtc_vblank_enable_no_lock+0x240/0x288 dpu_crtc_vblank+0xa8/0x118 dpu_kms_disable_vblank+0x20/0x2c vblank_ctrl_worker+0xa0/0xe0 kthread_worker_fn+0xe4/0x1a4 kthread+0x11c/0x12c ret_from_fork+0x10/0x18Good case:
--------------
Thread 1:dpu_encoder_phys_vid_control_vblank_irq+0xd0/0x170 dpu_encoder_phys_vid_irq_control+0xc8/0x110 _dpu_encoder_irq_control+0x48/0xa0 _dpu_encoder_resource_control_helper+0xb4/0x10c dpu_encoder_resource_control+0x4e0/0x664 dpu_encoder_virt_enable+0xb8/0x120 dpu_kms_encoder_enable+0x34/0xcc drm_atomic_helper_commit_modeset_enables+0x120/0x1b8 msm_atomic_commit_tail+0x5c/0x144 commit_tail+0x44/0x70 drm_atomic_helper_commit+0xf0/0xf8 drm_atomic_commit+0x40/0x4c drm_mode_atomic_ioctl+0x374/0x90cThread 2:
dpu_crtc_vblank+0xc8/0x118 dpu_kms_disable_vblank+0x20/0x2c vblank_ctrl_worker+0xa0/0xe0 kthread_worker_fn+0xe4/0x1a4 kthread+0x11c/0x12cSigned-off-by Jayant Shekhar <[email protected]>--- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +14
-3@@ -877,6 +877,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)struct drm_encoder *encoder;struct msm_drm_private *priv;unsigned long flags; +bool crtc_en;if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {DPU_ERROR ("invalid crtc\n" );@@ -901,11 +902,21 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) atomic_read(&dpu_crtc->frame_pending));trace_dpu_crtc_disable (DRMID (crtc), false, dpu_crtc); -if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { -_dpu_crtc_vblank_enable_no_lock (dpu_crtc, false); - } + +/* +* Cache vblank enabled before calling _dpu_crtc_vblank_enable_no_lock, +* because we release crtc_lock inside and acquire it back. While lock +* is released, there are cases where dpu_crtc_vblank comes in between +* while disable is going on. dpu_crtc_vblank further calls +* _dpu_crtc_vblank_enable_no_lock which tries vblank disable again +* resulting in refcount mismatch. +*/ + crtc_en = dpu_crtc->enabled; dpu_crtc->enabled = false; +if (crtc_en && dpu_crtc->vblank_requested) +_dpu_crtc_vblank_enable_no_lock (dpu_crtc, false); +if (atomic_read (&dpu_crtc->frame_pending)) {trace_dpu_crtc_disable_frame_pending (DRMID (crtc),atomic_read (&dpu_crtc->frame_pending));Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno