| Message ID | 20200819103021.440288-1-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: fix incorrect lock state | expand |
Hi Tomi, Thank you for the patch. On Wed, Aug 19, 2020 at 01:30:21PM +0300, Tomi Valkeinen wrote: > After commit 92cc68e35863c1c61c449efa2b2daef6e9926048 ("drm/vblank: Use > spin_(un)lock_irq() in drm_crtc_vblank_on()") omapdrm locking is broken: > > WARNING: inconsistent lock state > 5.8.0-rc2-00483-g92cc68e35863 #13 Tainted: G W > -------------------------------- > inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage. > swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes: > ea98222c (&dev->event_lock#2){?.+.}-{2:2}, at: drm_handle_vblank+0x4c/0x520 [drm] > {HARDIRQ-ON-W} state was registered at: > trace_hardirqs_on+0x9c/0x1ec > _raw_spin_unlock_irq+0x20/0x58 > omap_crtc_atomic_enable+0x54/0xa0 [omapdrm] > drm_atomic_helper_commit_modeset_enables+0x218/0x270 [drm_kms_helper] > omap_atomic_commit_tail+0x48/0xc4 [omapdrm] > commit_tail+0x9c/0x190 [drm_kms_helper] > drm_atomic_helper_commit+0x154/0x188 [drm_kms_helper] > drm_client_modeset_commit_atomic+0x228/0x268 [drm] > drm_client_modeset_commit_locked+0x60/0x1d0 [drm] > drm_client_modeset_commit+0x24/0x40 [drm] > drm_fb_helper_restore_fbdev_mode_unlocked+0x54/0xa8 [drm_kms_helper] > drm_fb_helper_set_par+0x2c/0x5c [drm_kms_helper] > drm_fb_helper_hotplug_event.part.0+0xa0/0xbc [drm_kms_helper] > drm_kms_helper_hotplug_event+0x24/0x30 [drm_kms_helper] > output_poll_execute+0x1a8/0x1c0 [drm_kms_helper] > process_one_work+0x268/0x800 > worker_thread+0x30/0x4e0 > kthread+0x164/0x190 > ret_from_fork+0x14/0x20 > > The reason for this is that omapdrm calls drm_crtc_vblank_on() while > holding event_lock taken with spin_lock_irq(). > > It is not clear why drm_crtc_vblank_on() and drm_crtc_vblank_get() are > called while holding event_lock. I don't see any problem with moving > those calls outside the lock, which is what this patch does. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I've had a quick look at other callers of drm_crtc_vblank_on() and none of them call it with a spinlock held in the same function. I have however only checked locally, not up the call stack. > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index 6d40914675da..328a4a74f534 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -451,11 +451,12 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc, > if (omap_state->manually_updated) > return; > > - spin_lock_irq(&crtc->dev->event_lock); > drm_crtc_vblank_on(crtc); > + > ret = drm_crtc_vblank_get(crtc); > WARN_ON(ret != 0); > > + spin_lock_irq(&crtc->dev->event_lock); > omap_crtc_arm_event(crtc); > spin_unlock_irq(&crtc->dev->event_lock); > }
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6d40914675da..328a4a74f534 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -451,11 +451,12 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc, if (omap_state->manually_updated) return; - spin_lock_irq(&crtc->dev->event_lock); drm_crtc_vblank_on(crtc); + ret = drm_crtc_vblank_get(crtc); WARN_ON(ret != 0); + spin_lock_irq(&crtc->dev->event_lock); omap_crtc_arm_event(crtc); spin_unlock_irq(&crtc->dev->event_lock); }
After commit 92cc68e35863c1c61c449efa2b2daef6e9926048 ("drm/vblank: Use spin_(un)lock_irq() in drm_crtc_vblank_on()") omapdrm locking is broken: WARNING: inconsistent lock state 5.8.0-rc2-00483-g92cc68e35863 #13 Tainted: G W -------------------------------- inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage. swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes: ea98222c (&dev->event_lock#2){?.+.}-{2:2}, at: drm_handle_vblank+0x4c/0x520 [drm] {HARDIRQ-ON-W} state was registered at: trace_hardirqs_on+0x9c/0x1ec _raw_spin_unlock_irq+0x20/0x58 omap_crtc_atomic_enable+0x54/0xa0 [omapdrm] drm_atomic_helper_commit_modeset_enables+0x218/0x270 [drm_kms_helper] omap_atomic_commit_tail+0x48/0xc4 [omapdrm] commit_tail+0x9c/0x190 [drm_kms_helper] drm_atomic_helper_commit+0x154/0x188 [drm_kms_helper] drm_client_modeset_commit_atomic+0x228/0x268 [drm] drm_client_modeset_commit_locked+0x60/0x1d0 [drm] drm_client_modeset_commit+0x24/0x40 [drm] drm_fb_helper_restore_fbdev_mode_unlocked+0x54/0xa8 [drm_kms_helper] drm_fb_helper_set_par+0x2c/0x5c [drm_kms_helper] drm_fb_helper_hotplug_event.part.0+0xa0/0xbc [drm_kms_helper] drm_kms_helper_hotplug_event+0x24/0x30 [drm_kms_helper] output_poll_execute+0x1a8/0x1c0 [drm_kms_helper] process_one_work+0x268/0x800 worker_thread+0x30/0x4e0 kthread+0x164/0x190 ret_from_fork+0x14/0x20 The reason for this is that omapdrm calls drm_crtc_vblank_on() while holding event_lock taken with spin_lock_irq(). It is not clear why drm_crtc_vblank_on() and drm_crtc_vblank_get() are called while holding event_lock. I don't see any problem with moving those calls outside the lock, which is what this patch does. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_crtc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)