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

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

[Imre Deak]

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

[PATCH 2/2] drm/i915: lock event_lock for drm_vblank_off()

[Imre Deak]

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

Re: [PATCH 2/2] drm/i915: lock event_lock for drm_vblank_off()

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Imre Deak]

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

Re: [PATCH 2/2] drm/i915: lock event_lock for drm_vblank_off()

[Imre Deak]

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
> 

[PATCH v2 0/4] drm/exynos, intel: fix locking for flip/vbl event list

[Imre Deak]

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

-- 
1.7.9.5

[PATCH v2 1/4] drm/exynos: hold event_lock while accessing pageflip_event_list

[Imre Deak]

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;
 		}
-- 
1.7.9.5

[PATCH v2 2/4] drm/exynos: call drm_vblank_put for each queued flip event

[Imre Deak]

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

[PATCH v2 3/4] drm/exynos: fix lockdep for event_lock wrt. vbl_time_lock

[Imre Deak]

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

[PATCH v2 4/4] drm: hold event_lock while accessing vblank_event_list

[Imre Deak]

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

Re: [PATCH v2 0/4] drm/exynos, intel: fix locking for flip/vbl event list

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 1834 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 2558 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/4] drm/exynos,intel: fix locking for flip/vbl event list

[Imre Deak]

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

Re: [PATCH v2 0/4] drm/exynos, intel: fix locking for flip/vbl event list

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 1694 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 2749 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/4] drm/exynos, intel: fix locking for flip/vbl event list

[Rob Clark]

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
>

Re: [PATCH v2 0/4] drm/exynos, intel: fix locking for flip/vbl event list

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 2559 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 4298 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel