* [PATCH] drm/i915: fix hpd work vs. flush_work in the pageflip code deadlock
@ 2013-09-02 6:49 Daniel Vetter
2013-09-02 11:52 ` Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2013-09-02 6:49 UTC (permalink / raw
To: Intel Graphics Development
Cc: Daniel Vetter, Chris Wilson, Stuart Abercrombie, stable
Historically we've run our own driver hotplug handling in our own
work-queue, which then launched the drm core hotplug handling in the
system workqueue. This is important since we flush our own driver
workqueue in the pageflip code while hodling modeset locks, and only
the drm hotplug code grabbed these locks. But with
commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Tue Oct 23 18:23:34 2012 +0000
drm: run the hpd irq event code directly
this was changed and now we could deadlock in our flip handler if
there's a hotplug work blocking the progress of the crucial unpin
works. So this broke the careful deadlock avoidance implemented in
commit b4a98e57fc27854b5938fc8b08b68e5e68b91e1f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Thu Nov 1 09:26:26 2012 +0000
drm/i915: Flush outstanding unpin tasks before pageflipping
Since the rule thus far has been that work items on our own workqueue
may never grab modeset locks simply restore that rule again.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Stuart Abercrombie <sabercrombie@chromium.org>
Reported-by: Stuart Abercrombie <sabercrombie@chromium.org>
References: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/26239
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_irq.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c6853f..39e4dd0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1027,8 +1027,7 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
dev_priv->display.hpd_irq_setup(dev);
spin_unlock(&dev_priv->irq_lock);
- queue_work(dev_priv->wq,
- &dev_priv->hotplug_work);
+ schedule_work(&dev_priv->hotplug_work);
}
static void gmbus_irq_handler(struct drm_device *dev)
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: fix hpd work vs. flush_work in the pageflip code deadlock
2013-09-02 6:49 [PATCH] drm/i915: fix hpd work vs. flush_work in the pageflip code deadlock Daniel Vetter
@ 2013-09-02 11:52 ` Chris Wilson
2013-09-02 14:22 ` Daniel Vetter
2013-09-02 19:02 ` Daniel Vetter
0 siblings, 2 replies; 4+ messages in thread
From: Chris Wilson @ 2013-09-02 11:52 UTC (permalink / raw
To: Daniel Vetter; +Cc: Intel Graphics Development, Stuart Abercrombie, stable
On Mon, Sep 02, 2013 at 08:49:01AM +0200, Daniel Vetter wrote:
> Historically we've run our own driver hotplug handling in our own
> work-queue, which then launched the drm core hotplug handling in the
> system workqueue. This is important since we flush our own driver
> workqueue in the pageflip code while hodling modeset locks, and only
> the drm hotplug code grabbed these locks. But with
>
> commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date: Tue Oct 23 18:23:34 2012 +0000
>
> drm: run the hpd irq event code directly
>
> this was changed and now we could deadlock in our flip handler if
> there's a hotplug work blocking the progress of the crucial unpin
> works. So this broke the careful deadlock avoidance implemented in
>
> commit b4a98e57fc27854b5938fc8b08b68e5e68b91e1f
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Thu Nov 1 09:26:26 2012 +0000
>
> drm/i915: Flush outstanding unpin tasks before pageflipping
>
> Since the rule thus far has been that work items on our own workqueue
> may never grab modeset locks simply restore that rule again.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Stuart Abercrombie <sabercrombie@chromium.org>
> Reported-by: Stuart Abercrombie <sabercrombie@chromium.org>
> References: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/26239
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
That wins for simplicity, and it is indeed the only caller that requires
mode_config.lock, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Bonus would a reminder in i915_drv.h to say that we cannot put items
that require mode_config.lock onto the wq, and that they should go onto
the global workqueue instead.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] drm/i915: fix hpd work vs. flush_work in the pageflip code deadlock
2013-09-02 11:52 ` Chris Wilson
@ 2013-09-02 14:22 ` Daniel Vetter
2013-09-02 19:02 ` Daniel Vetter
1 sibling, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2013-09-02 14:22 UTC (permalink / raw
To: Intel Graphics Development
Cc: Daniel Vetter, Chris Wilson, Stuart Abercrombie, stable
Historically we've run our own driver hotplug handling in our own
work-queue, which then launched the drm core hotplug handling in the
system workqueue. This is important since we flush our own driver
workqueue in the pageflip code while hodling modeset locks, and only
the drm hotplug code grabbed these locks. But with
commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Tue Oct 23 18:23:34 2012 +0000
drm: run the hpd irq event code directly
this was changed and now we could deadlock in our flip handler if
there's a hotplug work blocking the progress of the crucial unpin
works. So this broke the careful deadlock avoidance implemented in
commit b4a98e57fc27854b5938fc8b08b68e5e68b91e1f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Thu Nov 1 09:26:26 2012 +0000
drm/i915: Flush outstanding unpin tasks before pageflipping
Since the rule thus far has been that work items on our own workqueue
may never grab modeset locks simply restore that rule again.
v2: Add a comment to the declaration of dev_priv->wq to warn readers
about the tricky implications of using it. Suggested by Chris Wilson.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Stuart Abercrombie <sabercrombie@chromium.org>
Reported-by: Stuart Abercrombie <sabercrombie@chromium.org>
References: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/26239
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
drivers/gpu/drm/i915/i915_irq.c | 3 +--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4da1f86..7bd1370 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1248,6 +1248,13 @@ typedef struct drm_i915_private {
unsigned int fsb_freq, mem_freq, is_ddr3;
+ /**
+ * wq - Driver workqueue for GEM.
+ *
+ * NOTE: Work items scheduled here are not allowed to grab any modeset
+ * locks, for otherwise the flushing done in the pageflip code will
+ * result in deadlocks.
+ */
struct workqueue_struct *wq;
/* Display functions */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c6853f..39e4dd0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1027,8 +1027,7 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
dev_priv->display.hpd_irq_setup(dev);
spin_unlock(&dev_priv->irq_lock);
- queue_work(dev_priv->wq,
- &dev_priv->hotplug_work);
+ schedule_work(&dev_priv->hotplug_work);
}
static void gmbus_irq_handler(struct drm_device *dev)
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: fix hpd work vs. flush_work in the pageflip code deadlock
2013-09-02 11:52 ` Chris Wilson
2013-09-02 14:22 ` Daniel Vetter
@ 2013-09-02 19:02 ` Daniel Vetter
1 sibling, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2013-09-02 19:02 UTC (permalink / raw
To: Chris Wilson, Daniel Vetter, Intel Graphics Development,
Stuart Abercrombie, stable
On Mon, Sep 02, 2013 at 12:52:05PM +0100, Chris Wilson wrote:
> On Mon, Sep 02, 2013 at 08:49:01AM +0200, Daniel Vetter wrote:
> > Historically we've run our own driver hotplug handling in our own
> > work-queue, which then launched the drm core hotplug handling in the
> > system workqueue. This is important since we flush our own driver
> > workqueue in the pageflip code while hodling modeset locks, and only
> > the drm hotplug code grabbed these locks. But with
> >
> > commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date: Tue Oct 23 18:23:34 2012 +0000
> >
> > drm: run the hpd irq event code directly
> >
> > this was changed and now we could deadlock in our flip handler if
> > there's a hotplug work blocking the progress of the crucial unpin
> > works. So this broke the careful deadlock avoidance implemented in
> >
> > commit b4a98e57fc27854b5938fc8b08b68e5e68b91e1f
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date: Thu Nov 1 09:26:26 2012 +0000
> >
> > drm/i915: Flush outstanding unpin tasks before pageflipping
> >
> > Since the rule thus far has been that work items on our own workqueue
> > may never grab modeset locks simply restore that rule again.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Stuart Abercrombie <sabercrombie@chromium.org>
> > Reported-by: Stuart Abercrombie <sabercrombie@chromium.org>
> > References: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/26239
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> That wins for simplicity, and it is indeed the only caller that requires
> mode_config.lock, so
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Bonus would a reminder in i915_drv.h to say that we cannot put items
> that require mode_config.lock onto the wq, and that they should go onto
> the global workqueue instead.
I've merged the updated version, thanks for your review.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-02 19:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-02 6:49 [PATCH] drm/i915: fix hpd work vs. flush_work in the pageflip code deadlock Daniel Vetter
2013-09-02 11:52 ` Chris Wilson
2013-09-02 14:22 ` Daniel Vetter
2013-09-02 19:02 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.