_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
Signed-off-by: Jayant Shekhar <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 630cbaa..e81ad8c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -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));
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Dec 06, 2018 at 12:16:53PM +0530, Jayant Shekhar wrote:
> _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.
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
>
> 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
>
> Signed-off-by: Jayant Shekhar <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 630cbaa..e81ad8c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -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));
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Sean Paul, Software Engineer, Google / Chromium OS