| Message ID | 1351721019-8040-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(-)
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 >
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, 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.