Skip to content

Commit

Permalink
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
Browse files Browse the repository at this point in the history
Currently both drm_irq.c and several drivers call drm_vblank_put()
while holding event_lock. Now that drm_vblank_put() can disable the
vblank interrupt directly it may need to grab vbl_lock and
vblank_time_lock. That causes deadlocks since we take the locks
in the opposite order in two places in drm_irq.c. So let's make
sure the locking order is always event_lock->vbl_lock->vblank_time_lock.

In drm_vblank_off() pull up event_lock from underneath vbl_lock. Hold
the event_lock across the whole operation to make sure we only send
out the events that were on the queue when we disabled the interrupt,
and not ones that got added just after (assuming drm_vblank_on() already
managed to get called somewhere between).

To sort the other deadlock pull the event_lock out from
drm_handle_vblank_events() into drm_handle_vblank() to be taken outside
vblank_time_lock. Add the appropriate assert_spin_locked() to
drm_handle_vblank_events().

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
  • Loading branch information
Ville Syrjälä authored and Daniel Vetter committed Aug 6, 2014
1 parent 8a51d5b commit 56cc279
Showing 1 changed file with 25 additions and 22 deletions.
47 changes: 25 additions & 22 deletions drivers/gpu/drm/drm_irq.c
Original file line number Diff line number Diff line change
Expand Up @@ -1037,14 +1037,25 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
unsigned long irqflags;
unsigned int seq;

spin_lock_irqsave(&dev->vbl_lock, irqflags);
spin_lock_irqsave(&dev->event_lock, irqflags);

spin_lock(&dev->vbl_lock);
vblank_disable_and_save(dev, crtc);
wake_up(&vblank->queue);

/*
* Prevent subsequent drm_vblank_get() from re-enabling
* the vblank interrupt by bumping the refcount.
*/
if (!vblank->inmodeset) {
atomic_inc(&vblank->refcount);
vblank->inmodeset = 1;
}
spin_unlock(&dev->vbl_lock);

/* Send any queued vblank events, lest the natives grow disquiet */
seq = drm_vblank_count_and_time(dev, crtc, &now);

spin_lock(&dev->event_lock);
list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
if (e->pipe != crtc)
continue;
Expand All @@ -1055,18 +1066,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
drm_vblank_put(dev, e->pipe);
send_vblank_event(dev, e, seq, &now);
}
spin_unlock(&dev->event_lock);

/*
* Prevent subsequent drm_vblank_get() from re-enabling
* the vblank interrupt by bumping the refcount.
*/
if (!vblank->inmodeset) {
atomic_inc(&vblank->refcount);
vblank->inmodeset = 1;
}

spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
spin_unlock_irqrestore(&dev->event_lock, irqflags);
}
EXPORT_SYMBOL(drm_vblank_off);

Expand Down Expand Up @@ -1446,12 +1446,11 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
{
struct drm_pending_vblank_event *e, *t;
struct timeval now;
unsigned long flags;
unsigned int seq;

seq = drm_vblank_count_and_time(dev, crtc, &now);
assert_spin_locked(&dev->event_lock);

spin_lock_irqsave(&dev->event_lock, flags);
seq = drm_vblank_count_and_time(dev, crtc, &now);

list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
if (e->pipe != crtc)
Expand All @@ -1467,8 +1466,6 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
send_vblank_event(dev, e, seq, &now);
}

spin_unlock_irqrestore(&dev->event_lock, flags);

trace_drm_vblank_event(crtc, seq);
}

Expand All @@ -1491,15 +1488,18 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
if (!dev->num_crtcs)
return false;

spin_lock_irqsave(&dev->event_lock, irqflags);

/* Need timestamp lock to prevent concurrent execution with
* vblank enable/disable, as this would cause inconsistent
* or corrupted timestamps and vblank counts.
*/
spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
spin_lock(&dev->vblank_time_lock);

/* Vblank irq handling disabled. Nothing to do. */
if (!vblank->enabled) {
spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
spin_unlock(&dev->vblank_time_lock);
spin_unlock_irqrestore(&dev->event_lock, irqflags);
return false;
}

Expand Down Expand Up @@ -1539,10 +1539,13 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
crtc, (int) diff_ns);
}

spin_unlock(&dev->vblank_time_lock);

wake_up(&vblank->queue);
drm_handle_vblank_events(dev, crtc);

spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
spin_unlock_irqrestore(&dev->event_lock, irqflags);

return true;
}
EXPORT_SYMBOL(drm_handle_vblank);

0 comments on commit 56cc279

Please sign in to comment.