Before queuing the flip but crucially after attaching the unpin-work to
the crtc, we continue to setup the unpin-work. However, should the
hardware fire early, we see the connected unpin-work and queue the task.
The task then promptly runs and unpins the fb before we finish taking
the required references or even pinning it... Havoc.
To close the race, we use the flip-pending atomic to indicate when the
flip is finally setup and enqueued. So during the flip-done processing,
we can check more accurately whether the flip was expected.
v2: Add the appropriate mb() to ensure that the writes to the page-flip
worker are complete prior to marking it active and emitting the MI_FLIP.
On the read side, the mb should be enforced by the spinlocks.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
[danvet: Review the barriers a bit, we need a write barrier both
before and after updating ->pending. Similarly we need a read barrier
in the interrupt handler both before and after reading ->pending. With
well-ordered irqs only one barrier in each place should be required,
but since this patch explicitly sets out to combat spurious interrupts
with is staged activation of the unpin work we need to go full-bore on
the barriers, too. Discussed with Chris Wilson on irc and changes
acked by him.]
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
OTC Commit Hash:
commit e7d841ca03b7ab668620045cd7b428eda9f41601
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
The patch add additional changes which is missing in EMGD to allow the above
patch to port in. The patch helps to fix the driver hang when running 3d
navigation in dual display mode using weston. This capture in HSD 206265.
Signed-off-by: Chon Ming Lee <chon.ming.lee@xxxxxxxxx>
---
include/igd_display_pipeline.h | 4 +++
src/core/emgd_crtc.c | 63 +++++++++++++++++++++++++++++++++-------
src/core/emgd_drv.c | 2 ++
src/core/i915/i915_emgd_helper.c | 4 +--
4 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/include/igd_display_pipeline.h b/include/igd_display_pipeline.h
index 77a3161..f868d22 100644
--- a/include/igd_display_pipeline.h
+++ b/include/igd_display_pipeline.h
@@ -562,6 +562,10 @@ typedef struct _emgd_page_flip_work {
/* Userspace event to send back upon flip completion. */
struct drm_pending_vblank_event *event;
+ atomic_t pending;
+#define INTEL_FLIP_INACTIVE 0
+#define INTEL_FLIP_PENDING 1
+#define INTEL_FLIP_COMPLETE 2
} emgd_page_flip_work_t;
#endif
diff --git a/src/core/emgd_crtc.c b/src/core/emgd_crtc.c
index b0bc62b..88fcb88 100755
--- a/src/core/emgd_crtc.c
+++ b/src/core/emgd_crtc.c
@@ -1074,6 +1074,31 @@ static void emgd_crtc_load_lut(struct drm_crtc *crtc)
+void intel_prepare_page_flip(struct drm_device *dev, int crtcnum)
+{
+ drm_emgd_private_t *priv = dev->dev_private;
+ emgd_crtc_t *emgd_crtc = priv->crtcs[crtcnum];
+ unsigned long flags;
+
+ /* NB: An MMIO update of the plane base pointer will also
+ * generate a page-flip completion irq, i.e. every modeset
+ * is also accompanied by a spurious intel_prepare_page_flip().
+ */
+ spin_lock_irqsave(&dev->event_lock, flags);
+ if (emgd_crtc->flip_pending)
+ atomic_inc_not_zero(&emgd_crtc->flip_pending->pending);
+ spin_unlock_irqrestore(&dev->event_lock, flags);
+}
+
+static inline void intel_mark_page_flip_active(emgd_crtc_t *emgd_crtc)
+{
+ /* Ensure that the work item is consistent when activating it ... */
+ smp_wmb();
+ atomic_set(&emgd_crtc->flip_pending->pending, INTEL_FLIP_PENDING);
+ /* and that it is marked active as soon as the irq could fire. */
+ smp_wmb();
+}
+
/*
* crtc_pageflip_handler()
*
@@ -1110,17 +1135,23 @@ int crtc_pageflip_handler(struct drm_device *dev, int crtcnum)
do_gettimeofday(&now);
- /* Protect access to the CRTC structure */
- spin_lock_irqsave(&emgd_crtc->crtc_lock, flags);
+ spin_lock_irqsave(&dev->event_lock, flags);
+ page_flip_work = emgd_crtc->flip_pending;
+
+ /* Ensure we don't miss a work->pending update ... */
+ smp_rmb();
/* If there is nothing to do, then do nothing */
- if (NULL == emgd_crtc->flip_pending) {
+ if (NULL == page_flip_work || atomic_read(&page_flip_work->pending) <
+ INTEL_FLIP_COMPLETE) {
EMGD_DEBUG("No pending page flip");
- spin_unlock_irqrestore(&emgd_crtc->crtc_lock, flags);
+ spin_unlock_irqrestore(&dev->event_lock, flags);
return 0;
}
- page_flip_work = emgd_crtc->flip_pending;
+ /* and that the unpin work is consistent wrt ->pending. */
+ smp_rmb();
+
emgd_crtc->flip_pending = NULL;
/* Flip is now complete; send userspace event, if requested */
@@ -1139,7 +1170,7 @@ int crtc_pageflip_handler(struct drm_device *dev, int crtcnum)
if (emgd_crtc->igd_plane->plane_features & IGD_PLANE_IS_PLANEB) {
plane_select = 1;
}
- spin_unlock_irqrestore(&emgd_crtc->crtc_lock, flags);
+ spin_unlock_irqrestore(&dev->event_lock, flags);
atomic_clear_mask(1 << plane_select,
&page_flip_work->old_fb_obj->pending_flip.counter);
@@ -1314,8 +1345,6 @@ static int emgd_crtc_page_flip(struct drm_crtc *crtc,
return -ENOMEM;
}
- mutex_lock(&crtc->dev->struct_mutex);
-
page_flip_work->event = event;
page_flip_work->dev = crtc->dev;
page_flip_work->old_fb_obj = emgd_old_fb->bo;
@@ -1325,7 +1354,6 @@ static int emgd_crtc_page_flip(struct drm_crtc *crtc,
ret = drm_vblank_get(crtc->dev, emgd_crtc->pipe);
if (ret) {
kfree(page_flip_work);
- mutex_unlock(&crtc->dev->struct_mutex);
return ret;
}
@@ -1336,8 +1364,6 @@ static int emgd_crtc_page_flip(struct drm_crtc *crtc,
drm_vblank_put(dev, emgd_crtc->pipe);
kfree(page_flip_work);
- mutex_unlock(&crtc->dev->struct_mutex);
-
EMGD_ERROR("flip queue: crtc already busy");
return -EBUSY;
}
@@ -1350,6 +1376,19 @@ static int emgd_crtc_page_flip(struct drm_crtc *crtc,
flush_workqueue(priv->wq);
}
+ ret = i915_mutex_lock_interruptible(dev);
+ if (ret) {
+ spin_lock_irqsave(&emgd_crtc->crtc_lock, flags);
+ emgd_crtc->flip_pending = NULL;
+ spin_unlock_irqrestore(&emgd_crtc->crtc_lock, flags);
+
+ drm_vblank_put(crtc->dev, emgd_crtc->pipe);
+
+ kfree(page_flip_work);
+
+ return ret;
+ }
+
page_flip_work->new_fb_obj = emgd_new_fb->bo;
@@ -1372,6 +1411,8 @@ static int emgd_crtc_page_flip(struct drm_crtc *crtc,
ret = context->mode_dispatch->kms_queue_flip(emgd_crtc, emgd_new_fb,
emgd_new_fb->bo, plane_select);
+ intel_mark_page_flip_active(emgd_crtc);
+
if (ret) {
atomic_dec(&emgd_crtc->unpin_work_count);
atomic_sub(1 << plane_select,
diff --git a/src/core/emgd_drv.c b/src/core/emgd_drv.c
index 83c18b1..4bba236 100644
--- a/src/core/emgd_drv.c
+++ b/src/core/emgd_drv.c
@@ -2417,9 +2417,11 @@ irqreturn_t emgd_driver_irq_handler(int irq, void *arg)
if (int_data.irq_display) {
if (int_data.irq_display & IGD_IRQ_DISP_FLIP_A) {
+ intel_prepare_page_flip(dev, 0);
crtc_pageflip_handler(dev, 0);
}
if (int_data.irq_display & IGD_IRQ_DISP_FLIP_B) {
+ intel_prepare_page_flip(dev, 1);
crtc_pageflip_handler(dev, 1);
}
diff --git a/src/core/i915/i915_emgd_helper.c b/src/core/i915/i915_emgd_helper.c
index ac0eb6a..2b51995 100644
--- a/src/core/i915/i915_emgd_helper.c
+++ b/src/core/i915/i915_emgd_helper.c
@@ -743,9 +743,9 @@ static void intel_display_handle_reset(struct drm_device *dev)
if (emgd_crtc->flip_pending)
atomic_inc(&emgd_crtc->unpin_work_count);
+ intel_prepare_page_flip(dev, emgd_crtc->pipe);
crtc_pageflip_handler(dev, emgd_crtc->pipe);
-
- }
+ }
}
static void i915_error_wake_up(struct drm_i915_private *dev_priv,
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html