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.
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);
drm_vblank_off() requires callers to hold the event_lock.
Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b453901..56f1119 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3413,6 +3413,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; int plane = intel_crtc->plane; + unsigned long flags; u32 reg, temp;
@@ -3423,7 +3424,11 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc); + + spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe); + spin_unlock_irqrestore(&dev->event_lock, flags); + intel_crtc_update_cursor(crtc, false);
intel_disable_plane(dev_priv, plane, pipe); @@ -3495,6 +3500,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) int plane = intel_crtc->plane; enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; bool is_pch_port; + unsigned long flags;
if (!intel_crtc->active) return; @@ -3505,7 +3511,11 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc); + + spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe); + spin_unlock_irqrestore(&dev->event_lock, flags); + intel_crtc_update_cursor(crtc, false);
intel_disable_plane(dev_priv, plane, pipe); @@ -3617,6 +3627,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; int plane = intel_crtc->plane; + unsigned long flags;
if (!intel_crtc->active) @@ -3627,7 +3638,11 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc); + + spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe); + spin_unlock_irqrestore(&dev->event_lock, flags); + intel_crtc_dpms_overlay(intel_crtc, false); intel_crtc_update_cursor(crtc, false);
On Thu, Nov 01, 2012 at 12:03:39AM +0200, Imre Deak wrote:
drm_vblank_off() requires callers to hold the event_lock.
Signed-off-by: Imre Deak imre.deak@intel.com
Hm, do we put this code through its paces already in flip_test by scheduling a vblank wait in the future (a second or so), and then disabling the display? -Daniel
drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b453901..56f1119 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3413,6 +3413,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; int plane = intel_crtc->plane;
- unsigned long flags; u32 reg, temp;
@@ -3423,7 +3424,11 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc);
spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe);
spin_unlock_irqrestore(&dev->event_lock, flags);
intel_crtc_update_cursor(crtc, false);
intel_disable_plane(dev_priv, plane, pipe);
@@ -3495,6 +3500,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) int plane = intel_crtc->plane; enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; bool is_pch_port;
unsigned long flags;
if (!intel_crtc->active) return;
@@ -3505,7 +3511,11 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc);
spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe);
spin_unlock_irqrestore(&dev->event_lock, flags);
intel_crtc_update_cursor(crtc, false);
intel_disable_plane(dev_priv, plane, pipe);
@@ -3617,6 +3627,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; int plane = intel_crtc->plane;
unsigned long flags;
if (!intel_crtc->active)
@@ -3627,7 +3638,11 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc);
- spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe);
- spin_unlock_irqrestore(&dev->event_lock, flags);
- intel_crtc_dpms_overlay(intel_crtc, false); intel_crtc_update_cursor(crtc, false);
-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 2012-10-31 at 23:46 +0100, Daniel Vetter wrote:
On Thu, Nov 01, 2012 at 12:03:39AM +0200, Imre Deak wrote:
drm_vblank_off() requires callers to hold the event_lock.
Signed-off-by: Imre Deak imre.deak@intel.com
Hm, do we put this code through its paces already in flip_test by scheduling a vblank wait in the future (a second or so), and then disabling the display?
There isn't such a test case yet, but this bug is triggered with what we have already, the 'wait-for-vblank + dpms off' test. But it might make sense to delay the vblank as you say, in case disabling takes a long time and the vblank arrives before we get to drm_vblank_off(). I can add something for that.
--Imre
-Daniel
drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b453901..56f1119 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3413,6 +3413,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; int plane = intel_crtc->plane;
- unsigned long flags; u32 reg, temp;
@@ -3423,7 +3424,11 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc);
spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe);
spin_unlock_irqrestore(&dev->event_lock, flags);
intel_crtc_update_cursor(crtc, false);
intel_disable_plane(dev_priv, plane, pipe);
@@ -3495,6 +3500,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) int plane = intel_crtc->plane; enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; bool is_pch_port;
unsigned long flags;
if (!intel_crtc->active) return;
@@ -3505,7 +3511,11 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc);
spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe);
spin_unlock_irqrestore(&dev->event_lock, flags);
intel_crtc_update_cursor(crtc, false);
intel_disable_plane(dev_priv, plane, pipe);
@@ -3617,6 +3627,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) struct intel_encoder *encoder; int pipe = intel_crtc->pipe; int plane = intel_crtc->plane;
unsigned long flags;
if (!intel_crtc->active)
@@ -3627,7 +3638,11 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc);
- spin_lock_irqsave(&dev->event_lock, flags); drm_vblank_off(dev, pipe);
- spin_unlock_irqrestore(&dev->event_lock, flags);
- intel_crtc_dpms_overlay(intel_crtc, false); intel_crtc_update_cursor(crtc, false);
-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
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);
The patchset adds the missing event_lock when accessing the vblank_event_list in drm_vblank_off() and as preparation for this also fixes a few other issues in the exynos driver.
This is also a dependency for Rob Clark's drm_send_vblank_event() rework as that would trigger a warning for the unhold event_lock without this changeset.
The exynos changes are only compile tested, the rest is tested on an Intel IVB machine on top of drm-intel-nightly + drm_send_vblank_event() rework, with i-g-t/flip_test.
In v2: - Instead of fixing the event_lock vs. vblank_time_lock lockdep issue in drm_handle_vblank(), fix it in the exynos driver. This looks like the more logical thing to do and this way we also have a smaller impact on the rest of DRM code.
Imre Deak (4): drm/exynos: hold event_lock while accessing pageflip_event_list drm/exynos: call drm_vblank_put for each queued flip event drm/exynos: fix lockdep for event_lock wrt. vbl_time_lock drm: hold event_lock while accessing vblank_event_list
drivers/gpu/drm/drm_irq.c | 3 +++ drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 +++++ drivers/gpu/drm/exynos/exynos_drm_fimd.c | 20 +------------------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 20 +------------------- drivers/gpu/drm/exynos/exynos_mixer.c | 11 +---------- 5 files changed, 11 insertions(+), 48 deletions(-)
Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index fce245f..2efa4b0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -236,16 +236,21 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc, goto out; }
+ spin_lock_irq(&dev->event_lock); list_add_tail(&event->base.link, &dev_priv->pageflip_event_list); + spin_unlock_irq(&dev->event_lock);
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x, crtc->y, NULL); if (ret) { crtc->fb = old_fb; + + spin_lock_irq(&dev->event_lock); drm_vblank_put(dev, exynos_crtc->pipe); list_del(&event->base.link); + spin_unlock_irq(&dev->event_lock);
goto out; }
It's guaranteed that for each event on pageflip_event_list we have called drm_vblank_get() - see exynos_drm_crtc_page_flip() - so checking for this is redundant.
Also we need to call drm_vblank_put() for each event on the list, not only once, otherwise we'd leak vblank references if there are multiple events on the list.
Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 8 +------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 8 +------- drivers/gpu/drm/exynos/exynos_mixer.c | 11 +---------- 3 files changed, 3 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 130a2b5..e69b7c4 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -642,17 +642,11 @@ static void fimd_finish_pageflip(struct drm_device *drm_dev, int crtc)
list_move_tail(&e->base.link, &e->base.file_priv->event_list); wake_up_interruptible(&e->base.file_priv->event_wait); + drm_vblank_put(drm_dev, crtc); }
if (is_checked) { /* - * call drm_vblank_put only in case that drm_vblank_get was - * called. - */ - if (atomic_read(&drm_dev->vblank_refcount[crtc]) > 0) - drm_vblank_put(drm_dev, crtc); - - /* * don't off vblank if vblank_disable_allowed is 1, * because vblank would be off by timer handler. */ diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index e4b8a8f..e26d455 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -401,17 +401,11 @@ static void vidi_finish_pageflip(struct drm_device *drm_dev, int crtc)
list_move_tail(&e->base.link, &e->base.file_priv->event_list); wake_up_interruptible(&e->base.file_priv->event_wait); + drm_vblank_put(drm_dev, crtc); }
if (is_checked) { /* - * call drm_vblank_put only in case that drm_vblank_get was - * called. - */ - if (atomic_read(&drm_dev->vblank_refcount[crtc]) > 0) - drm_vblank_put(drm_dev, crtc); - - /* * don't off vblank if vblank_disable_allowed is 1, * because vblank would be off by timer handler. */ diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 614b2e9..5720e82 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -884,7 +884,6 @@ static void mixer_finish_pageflip(struct drm_device *drm_dev, int crtc) struct drm_pending_vblank_event *e, *t; struct timeval now; unsigned long flags; - bool is_checked = false;
spin_lock_irqsave(&drm_dev->event_lock, flags);
@@ -894,7 +893,6 @@ static void mixer_finish_pageflip(struct drm_device *drm_dev, int crtc) if (crtc != e->pipe) continue;
- is_checked = true; do_gettimeofday(&now); e->event.sequence = 0; e->event.tv_sec = now.tv_sec; @@ -902,16 +900,9 @@ static void mixer_finish_pageflip(struct drm_device *drm_dev, int crtc)
list_move_tail(&e->base.link, &e->base.file_priv->event_list); wake_up_interruptible(&e->base.file_priv->event_wait); + drm_vblank_put(drm_dev, crtc); }
- if (is_checked) - /* - * call drm_vblank_put only in case that drm_vblank_get was - * called. - */ - if (atomic_read(&drm_dev->vblank_refcount[crtc]) > 0) - drm_vblank_put(drm_dev, crtc); - spin_unlock_irqrestore(&drm_dev->event_lock, flags); }
Currently the exynos driver calls drm_vblank_off() with the event_lock held, while drm_vblank_off() will lock vbl_time and vblank_time_lock. This lock dependency chain conflicts with the one in drm_handle_vblank() where we first lock vblank_time_lock and then the event_lock.
Fix this by removing the above drm_vblank_off() calls which are in fact never executed: drm_dev->vblank_disable_allowed is only ever non-zero during driver init, until it's set in {fimd,vidi}_subdrv_probe. Both the driver init and open code is protected by drm_global_mutex, so the earliest page flip ioctl can happen only after vblank_disable_allowed is set to 1. Thus {fimd,vidi}_finish_pageflip - with pending flip events - will always get called with vblank_disable_allowed being 1.
Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 12 ------------ drivers/gpu/drm/exynos/exynos_drm_vidi.c | 12 ------------ 2 files changed, 24 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index e69b7c4..2eb131c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -623,7 +623,6 @@ static void fimd_finish_pageflip(struct drm_device *drm_dev, int crtc) struct drm_pending_vblank_event *e, *t; struct timeval now; unsigned long flags; - bool is_checked = false;
spin_lock_irqsave(&drm_dev->event_lock, flags);
@@ -633,8 +632,6 @@ static void fimd_finish_pageflip(struct drm_device *drm_dev, int crtc) if (crtc != e->pipe) continue;
- is_checked = true; - do_gettimeofday(&now); e->event.sequence = 0; e->event.tv_sec = now.tv_sec; @@ -645,15 +642,6 @@ static void fimd_finish_pageflip(struct drm_device *drm_dev, int crtc) drm_vblank_put(drm_dev, crtc); }
- if (is_checked) { - /* - * don't off vblank if vblank_disable_allowed is 1, - * because vblank would be off by timer handler. - */ - if (!drm_dev->vblank_disable_allowed) - drm_vblank_off(drm_dev, crtc); - } - spin_unlock_irqrestore(&drm_dev->event_lock, flags); }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index e26d455..4b0c16b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -382,7 +382,6 @@ static void vidi_finish_pageflip(struct drm_device *drm_dev, int crtc) struct drm_pending_vblank_event *e, *t; struct timeval now; unsigned long flags; - bool is_checked = false;
spin_lock_irqsave(&drm_dev->event_lock, flags);
@@ -392,8 +391,6 @@ static void vidi_finish_pageflip(struct drm_device *drm_dev, int crtc) if (crtc != e->pipe) continue;
- is_checked = true; - do_gettimeofday(&now); e->event.sequence = 0; e->event.tv_sec = now.tv_sec; @@ -404,15 +401,6 @@ static void vidi_finish_pageflip(struct drm_device *drm_dev, int crtc) drm_vblank_put(drm_dev, crtc); }
- if (is_checked) { - /* - * don't off vblank if vblank_disable_allowed is 1, - * because vblank would be off by timer handler. - */ - if (!drm_dev->vblank_disable_allowed) - drm_vblank_off(drm_dev, crtc); - } - spin_unlock_irqrestore(&drm_dev->event_lock, flags); }
Currently the only users of drm_vblank_off() are i915 and gma500, neither of which holds the event_lock when calling this function. Fix this by holding the event_lock while traversing the list.
Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/drm_irq.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3a3d0ce..c6cd558 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -949,6 +949,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
/* 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; @@ -965,6 +967,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) trace_drm_vblank_event_delivered(e->base.pid, e->pipe, e->event.sequence); } + spin_unlock(&dev->event_lock);
spin_unlock_irqrestore(&dev->vbl_lock, irqflags); }
2012/11/2 Imre Deak imre.deak@intel.com
The patchset adds the missing event_lock when accessing the vblank_event_list in drm_vblank_off() and as preparation for this also fixes a few other issues in the exynos driver.
This is also a dependency for Rob Clark's drm_send_vblank_event() rework as that would trigger a warning for the unhold event_lock without this changeset.
The exynos changes are only compile tested, the rest is tested on an Intel IVB machine on top of drm-intel-nightly + drm_send_vblank_event() rework, with i-g-t/flip_test.
Hi Imre,
Works fine. But we should wait for Rob's patch set to be merged to -next, and this may be rebased on top of latest Rob's patch set again.
Thanks, Inki Dae
In v2:
- Instead of fixing the event_lock vs. vblank_time_lock lockdep issue in drm_handle_vblank(), fix it in the exynos driver. This looks like the more logical thing to do and this way we also have a smaller impact on the rest of DRM code.
Imre Deak (4): drm/exynos: hold event_lock while accessing pageflip_event_list drm/exynos: call drm_vblank_put for each queued flip event drm/exynos: fix lockdep for event_lock wrt. vbl_time_lock drm: hold event_lock while accessing vblank_event_list
drivers/gpu/drm/drm_irq.c | 3 +++ drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 +++++ drivers/gpu/drm/exynos/exynos_drm_fimd.c | 20 +------------------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 20 +------------------- drivers/gpu/drm/exynos/exynos_mixer.c | 11 +---------- 5 files changed, 11 insertions(+), 48 deletions(-)
-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 2012-11-07 at 18:31 +0900, Inki Dae wrote:
2012/11/2 Imre Deak imre.deak@intel.com The patchset adds the missing event_lock when accessing the vblank_event_list in drm_vblank_off() and as preparation for this also fixes a few other issues in the exynos driver. This is also a dependency for Rob Clark's drm_send_vblank_event() rework as that would trigger a warning for the unhold event_lock without this changeset. The exynos changes are only compile tested, the rest is tested on an Intel IVB machine on top of drm-intel-nightly + drm_send_vblank_event() rework, with i-g-t/flip_test. Hi Imre, Works fine. But we should wait for Rob's patch set to be merged to -next, and this may be rebased on top of latest Rob's patch set again.
Ok, thanks for checking this. I assume then that this patchset will get merged through your tree.
I think Rob's patchset depends on this, so ideally this should go first. Otherwise the i915 driver would trigger the WARN in his patchset due to the unheld event_lock.
--Imre
2012/11/7 Imre Deak imre.deak@intel.com
On Wed, 2012-11-07 at 18:31 +0900, Inki Dae wrote:
2012/11/2 Imre Deak imre.deak@intel.com The patchset adds the missing event_lock when accessing the vblank_event_list in drm_vblank_off() and as preparation for this also fixes a few other issues in the exynos driver. This is also a dependency for Rob Clark's drm_send_vblank_event() rework as that would trigger a warning for the unhold event_lock without this changeset. The exynos changes are only compile tested, the rest is tested on an Intel IVB machine on top of drm-intel-nightly + drm_send_vblank_event() rework, with i-g-t/flip_test. Hi Imre, Works fine. But we should wait for Rob's patch set to be merged to -next, and this may be rebased on top of latest Rob's patch set again.
Ok, thanks for checking this. I assume then that this patchset will get merged through your tree.
I think Rob's patchset depends on this, so ideally this should go first. Otherwise the i915 driver would trigger the WARN in his patchset due to the unheld event_lock.
Ok, but I merge it first, shouldn't Rob's patch set be rebased? Anyway this is minor issue so I could resolve it. And it seems like that your patch set has no dependency of Rob's. I mean that your patch set worked fine without Rob's.
Thanks, Inki Dae
--Imre
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Nov 7, 2012 at 10:25 AM, Inki Dae inki.dae@samsung.com wrote:
2012/11/7 Imre Deak imre.deak@intel.com
On Wed, 2012-11-07 at 18:31 +0900, Inki Dae wrote:
2012/11/2 Imre Deak imre.deak@intel.com The patchset adds the missing event_lock when accessing the vblank_event_list in drm_vblank_off() and as preparation for this also fixes a few other issues in the exynos driver. This is also a dependency for Rob Clark's drm_send_vblank_event() rework as that would trigger a warning for the unhold event_lock without this changeset. The exynos changes are only compile tested, the rest is tested on an Intel IVB machine on top of drm-intel-nightly + drm_send_vblank_event() rework, with i-g-t/flip_test. Hi Imre, Works fine. But we should wait for Rob's patch set to be merged to -next, and this may be rebased on top of latest Rob's patch set again.
Ok, thanks for checking this. I assume then that this patchset will get merged through your tree.
I think Rob's patchset depends on this, so ideally this should go first. Otherwise the i915 driver would trigger the WARN in his patchset due to the unheld event_lock.
Ok, but I merge it first, shouldn't Rob's patch set be rebased? Anyway this is minor issue so I could resolve it. And it seems like that your patch set has no dependency of Rob's. I mean that your patch set worked fine without Rob's.
I think there should be no hard dependency on my patch set.. the only connection is that my patchset without this patch will start showing the WARN_ON() traces
BR, -R
Thanks, Inki Dae
--Imre
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/11/8 Rob Clark rob.clark@linaro.org
On Wed, Nov 7, 2012 at 10:25 AM, Inki Dae inki.dae@samsung.com wrote:
2012/11/7 Imre Deak imre.deak@intel.com
On Wed, 2012-11-07 at 18:31 +0900, Inki Dae wrote:
2012/11/2 Imre Deak imre.deak@intel.com The patchset adds the missing event_lock when accessing the vblank_event_list in drm_vblank_off() and as preparation for this also fixes a few other issues in the exynos driver. This is also a dependency for Rob Clark's drm_send_vblank_event() rework as that would trigger a warning for the unhold event_lock without this changeset. The exynos changes are only compile tested, the rest is tested on an Intel IVB machine on top of drm-intel-nightly + drm_send_vblank_event() rework, with i-g-t/flip_test. Hi Imre, Works fine. But we should wait for Rob's patch set to be merged to -next, and this may be rebased on top of latest Rob's patch set again.
Ok, thanks for checking this. I assume then that this patchset will get merged through your tree.
I think Rob's patchset depends on this, so ideally this should go first. Otherwise the i915 driver would trigger the WARN in his patchset due to the unheld event_lock.
Ok, but I merge it first, shouldn't Rob's patch set be rebased? Anyway
this
is minor issue so I could resolve it. And it seems like that your patch
set
has no dependency of Rob's. I mean that your patch set worked fine
without
Rob's.
I think there should be no hard dependency on my patch set.. the only connection is that my patchset without this patch will start showing the WARN_ON() traces
Right, My concern was just merge conflict.
BR, -R
Thanks, Inki Dae
--Imre
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Imre Deak -
Inki Dae -
Rob Clark