Re: [PATCH 1/2] drm: fix order of event_lock wrt. vblank_time_clock

On Thu, 2012-11-01 at 00:03 +0200, Imre Deak wrote:

drm_vblank_off() requires callers to hold the event_lock, while itself locking vbl_time and vblank_time_lock. This defines a dependency chain that conflicts with the one in drm_handle_vblank() where we first lock vblank_time_lock and then the event_lock. Fix this by reversing the locking order in drm_handle_vblank().

This should've triggered a lockdep warning in the exynos driver, the rest of the drivers were ok, since they are not using drm_vblank_off(), or as in the case of the intel driver were not holding the event_lock when calling drm_vblank_off(). This latter issue is addressed in the next patch.

I just realized this is better solved by fixing the lock order in the exynos driver. That way we can take the event_lock close to what it really locks. Fixing things there will also leave the other drivers unaffected.

I'll follow up with v2 doing this.

--Imre

Signed-off-by: Imre Deak imre.deak@intel.com

drivers/gpu/drm/drm_irq.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)

Tested with i915, the rest of the drivers should be checked with lockdep enabled.

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3a3d0ce..2193d4a 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1236,17 +1236,21 @@ done: return ret; }

+/**

• • drm_handle_vblank_events - send pending vblank events
• • @dev: DRM device
• • @crtc: crtc where the vblank(s) happened
• • Must be called with @dev->event_lock held.
• */

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);

• spin_lock_irqsave(&dev->event_lock, flags);

• list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { if (e->pipe != crtc) continue;

@@ -1266,8 +1270,6 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) e->event.sequence); }

• spin_unlock_irqrestore(&dev->event_lock, flags);
• trace_drm_vblank_event(crtc, seq);

}

@@ -1285,21 +1287,22 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) s64 diff_ns; struct timeval tvblank; unsigned long irqflags;

• bool ret = false;

  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 (!dev->vblank_enabled[crtc]) {

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

• return false;
  

• }

• if (!dev->vblank_enabled[crtc])

• goto unlock_ret;
  
  

  /* Fetch corresponding timestamp for this vblank interval from

  • driver and store it in proper slot of timestamp ringbuffer.

@@ -1340,7 +1343,12 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) DRM_WAKEUP(&dev->vbl_queue[crtc]); drm_handle_vblank_events(dev, crtc);

• spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
• return true;

• ret = true;

+unlock_ret:

• spin_unlock(&dev->vblank_time_lock);
• spin_unlock_irqrestore(&dev->event_lock, irqflags);
• return ret;

} EXPORT_SYMBOL(drm_handle_vblank);