diff mbox

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

Message ID 1351721019-8040-1-git-send-email-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Imre Deak Oct. 31, 2012, 10:03 p.m. UTC
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.

Comments

Imre Deak Nov. 1, 2012, 9:22 a.m. UTC | #1
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);
Imre Deak Nov. 2, 2012, 11:30 a.m. UTC | #2
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(-)
Inki Dae Nov. 7, 2012, 9:31 a.m. UTC | #3
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
>
Imre Deak Nov. 7, 2012, 10:43 a.m. UTC | #4
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
Inki Dae Nov. 7, 2012, 4:25 p.m. UTC | #5
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
>
Rob Clark Nov. 7, 2012, 4:28 p.m. UTC | #6
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
>
Inki Dae Nov. 7, 2012, 4:42 p.m. UTC | #7
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
>
diff mbox

Patch

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