On Tue, Jun 10, 2014 at 11:04:00AM +0100, Chris Wilson wrote:
> Long ago, back in the racy haydays of 915gm interrupt handling, page
> flips would occasionally go astray and leave the hardware stuck, and the
> display not updating. This annoyed people who relied on their systems
> being able to display continuously updating information 24/7, and so
> some code to detect when the driver missed the page flip completion
> signal was added. Until recently, it was presumed that the interrupt
> handling was now flawless, but once again Simon Farnsworth has found a
> system whose display will stall. Reinstate the pageflip stall detection,
> which works by checking to see if the hardware has been updated to the
> new framebuffer address following each vblank. If the hardware is
> scanning out from the new framebuffer, but we still think the flip is
> pending, then we kick our driver into submision.
>
> This is a continuation of the effort started with
> commit 4e5359cd053bfb7d8dabe4a63624a5726848ffbc
> Author: Simon Farnsworth <[email protected]>
> Date: Wed Sep 1 17:47:52 2010 +0100
>
> drm/i915: Avoid pageflipping freeze when we miss the flip prepare
> interrupt
>
> This now includes a belt-and-braces approach to make sure the driver
> (or the hardware) doesn't miss an interrupt and cause us to stop
> updating the display should the unthinkable happen and the pageflip fail -
> i.e.
> that the user is able to continue submitting flips.
>
> v2: Cleanup, refactor, and rename
>
> Reported-by: Simon Farnsworth <[email protected]>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502
> Signed-off-by: Chris Wilson <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 23 +++---
> drivers/gpu/drm/i915/i915_irq.c | 85 +++++++---------------
> drivers/gpu/drm/i915/intel_display.c | 130
> ++++++++++++++++++++++++++++------
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 4 files changed, 147 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index e8ea1ef..a5f0a26 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -581,6 +581,7 @@ static int i915_gem_pageflip_info(struct seq_file *m,
> void *data)
> {
> struct drm_info_node *node = m->private;
> struct drm_device *dev = node->minor->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned long flags;
> struct intel_crtc *crtc;
>
> @@ -595,6 +596,8 @@ static int i915_gem_pageflip_info(struct seq_file *m,
> void *data)
> seq_printf(m, "No flip due on pipe %c (plane %c)\n",
> pipe, plane);
> } else {
> + u32 addr;
> +
> if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
> seq_printf(m, "Flip queued on pipe %c (plane
> %c)\n",
> pipe, plane);
> @@ -602,23 +605,23 @@ static int i915_gem_pageflip_info(struct seq_file *m,
> void *data)
> seq_printf(m, "Flip pending (waiting for vsync)
> on pipe %c (plane %c)\n",
> pipe, plane);
> }
> + seq_printf(m, "Flip queued on frame %d, now %d\n",
> + work->sbc,
> atomic_read(&dev->vblank[crtc->pipe].count));
> if (work->enable_stall_check)
> seq_puts(m, "Stall check enabled, ");
> else
> seq_puts(m, "Stall check waiting for page flip
> ioctl, ");
> seq_printf(m, "%d prepares\n",
> atomic_read(&work->pending));
>
> - if (work->old_fb_obj) {
> - struct drm_i915_gem_object *obj =
> work->old_fb_obj;
> - if (obj)
> - seq_printf(m, "Old framebuffer
> gtt_offset 0x%08lx\n",
> -
> i915_gem_obj_ggtt_offset(obj));
> - }
> + if (INTEL_INFO(dev)->gen >= 4)
> + addr =
> I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
> + else
> + addr = I915_READ(DSPADDR(crtc->plane));
> + seq_printf(m, "Current scanout address 0x%08x\n", addr);
> +
> if (work->pending_flip_obj) {
> - struct drm_i915_gem_object *obj =
> work->pending_flip_obj;
> - if (obj)
> - seq_printf(m, "New framebuffer
> gtt_offset 0x%08lx\n",
> -
> i915_gem_obj_ggtt_offset(obj));
> + seq_printf(m, "New framebuffer address
> 0x%08lx\n", (long)work->gtt_offset);
> + seq_printf(m, "MMIO update completed? %d\n",
> addr == work->gtt_offset);
> }
> }
> spin_unlock_irqrestore(&dev->event_lock, flags);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 229e1f1..c3db85f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1802,8 +1802,9 @@ static void valleyview_pipestat_irq_handler(struct
> drm_device *dev, u32 iir)
> spin_unlock(&dev_priv->irq_lock);
>
> for_each_pipe(pipe) {
> - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> - intel_pipe_handle_vblank(dev, pipe);
> + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
> intel_prepare_page_flip(dev, pipe);
> @@ -2079,8 +2080,9 @@ static void ilk_display_irq_handler(struct drm_device
> *dev, u32 de_iir)
> DRM_ERROR("Poison interrupt\n");
>
> for_each_pipe(pipe) {
> - if (de_iir & DE_PIPE_VBLANK(pipe))
> - intel_pipe_handle_vblank(dev, pipe);
> + if (de_iir & DE_PIPE_VBLANK(pipe) &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
> false))
> @@ -2129,8 +2131,9 @@ static void ivb_display_irq_handler(struct drm_device
> *dev, u32 de_iir)
> intel_opregion_asle_intr(dev);
>
> for_each_pipe(pipe) {
> - if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
> - intel_pipe_handle_vblank(dev, pipe);
> + if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)) &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> /* plane/pipes map 1:1 on ilk+ */
> if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> @@ -2265,8 +2268,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> continue;
>
> pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> - if (pipe_iir & GEN8_PIPE_VBLANK)
> - intel_pipe_handle_vblank(dev, pipe);
> +
> + if (pipe_iir & GEN8_PIPE_VBLANK &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
> intel_prepare_page_flip(dev, pipe);
> @@ -2575,52 +2580,6 @@ void i915_handle_error(struct drm_device *dev, bool
> wedged,
> schedule_work(&dev_priv->gpu_error.work);
> }
>
> -static void __always_unused i915_pageflip_stall_check(struct drm_device
> *dev, int pipe)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct drm_i915_gem_object *obj;
> - struct intel_unpin_work *work;
> - unsigned long flags;
> - bool stall_detected;
> -
> - /* Ignore early vblank irqs */
> - if (intel_crtc == NULL)
> - return;
> -
> - spin_lock_irqsave(&dev->event_lock, flags);
> - work = intel_crtc->unpin_work;
> -
> - if (work == NULL ||
> - atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE ||
> - !work->enable_stall_check) {
> - /* Either the pending flip IRQ arrived, or we're too early.
> Don't check */
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> - return;
> - }
> -
> - /* Potential stall - if we see that the flip has happened, assume a
> missed interrupt */
> - obj = work->pending_flip_obj;
> - if (INTEL_INFO(dev)->gen >= 4) {
> - int dspsurf = DSPSURF(intel_crtc->plane);
> - stall_detected = I915_HI_DISPBASE(I915_READ(dspsurf)) ==
> - i915_gem_obj_ggtt_offset(obj);
> - } else {
> - int dspaddr = DSPADDR(intel_crtc->plane);
> - stall_detected = I915_READ(dspaddr) ==
> (i915_gem_obj_ggtt_offset(obj) +
> - crtc->y *
> crtc->primary->fb->pitches[0] +
> - crtc->x *
> crtc->primary->fb->bits_per_pixel/8);
> - }
> -
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - if (stall_detected) {
> - DRM_DEBUG_DRIVER("Pageflip stall detected\n");
> - intel_prepare_page_flip(dev, intel_crtc->plane);
> - }
> -}
> -
> /* Called from drm generic code, passed 'crtc' which
> * we use as a pipe index
> */
> @@ -3726,7 +3685,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> return false;
>
> if ((iir & flip_pending) == 0)
> - return false;
> + goto check_page_flip;
>
> intel_prepare_page_flip(dev, plane);
>
> @@ -3737,11 +3696,14 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> * an interrupt per se, we watch for the change at vblank.
> */
> if (I915_READ16(ISR) & flip_pending)
> - return false;
> + goto check_page_flip;
>
> intel_finish_page_flip(dev, pipe);
> -
> return true;
> +
> +check_page_flip:
> + intel_check_page_flip(dev, pipe);
> + return false;
> }
>
> static irqreturn_t i8xx_irq_handler(int irq, void *arg)
> @@ -3911,7 +3873,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
> return false;
>
> if ((iir & flip_pending) == 0)
> - return false;
> + goto check_page_flip;
>
> intel_prepare_page_flip(dev, plane);
>
> @@ -3922,11 +3884,14 @@ static bool i915_handle_vblank(struct drm_device *dev,
> * an interrupt per se, we watch for the change at vblank.
> */
> if (I915_READ(ISR) & flip_pending)
> - return false;
> + goto check_page_flip;
>
> intel_finish_page_flip(dev, pipe);
> -
> return true;
> +
> +check_page_flip:
> + intel_check_page_flip(dev, pipe);
> + return false;
> }
>
> static irqreturn_t i915_irq_handler(int irq, void *arg)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..b87b4ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3330,6 +3330,29 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
> return false;
> }
>
> +static void page_flip_completed(struct intel_crtc *intel_crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> + struct intel_unpin_work *work = intel_crtc->unpin_work;
> +
> + /* ensure that the unpin work is consistent wrt ->pending. */
> + smp_rmb();
> + intel_crtc->unpin_work = NULL;
> +
> + if (work->event)
> + drm_send_vblank_event(intel_crtc->base.dev,
> + intel_crtc->pipe,
> + work->event);
> +
> + drm_crtc_vblank_put(&intel_crtc->base);
> +
> + wake_up_all(&dev_priv->pending_flip_queue);
> + queue_work(dev_priv->wq, &work->work);
> +
> + trace_i915_flip_complete(intel_crtc->plane,
> + work->pending_flip_obj);
> +}
> +
> void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -8895,7 +8918,6 @@ static void intel_unpin_work_fn(struct work_struct
> *__work)
> static void do_intel_finish_page_flip(struct drm_device *dev,
> struct drm_crtc *crtc)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_unpin_work *work;
> unsigned long flags;
> @@ -8915,23 +8937,9 @@ static void do_intel_finish_page_flip(struct
> drm_device *dev,
> return;
> }
>
> - /* and that the unpin work is consistent wrt ->pending. */
> - smp_rmb();
> -
> - intel_crtc->unpin_work = NULL;
> -
> - if (work->event)
> - drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
> -
> - drm_crtc_vblank_put(crtc);
> + page_flip_completed(intel_crtc);
>
> spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - wake_up_all(&dev_priv->pending_flip_queue);
> -
> - queue_work(dev_priv->wq, &work->work);
> -
> - trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
> }
>
> void intel_finish_page_flip(struct drm_device *dev, int pipe)
> @@ -9009,11 +9017,17 @@ void intel_prepare_page_flip(struct drm_device *dev,
> int plane)
> spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> +static inline int crtc_sbc(struct intel_crtc *crtc)
> +{
> + return atomic_read(&crtc->base.dev->vblank[crtc->pipe].count);
> +}
Still says 'sbc' which doesn't make sense to me.
> +
> static inline void intel_mark_page_flip_active(struct intel_crtc *intel_crtc)
> {
> /* Ensure that the work item is consistent when activating it ... */
> smp_wmb();
> atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> + intel_crtc->unpin_work->sbc = crtc_sbc(intel_crtc);
> /* and that it is marked active as soon as the irq could fire. */
> smp_wmb();
> }
> @@ -9265,6 +9279,69 @@ static int intel_default_queue_flip(struct drm_device
> *dev,
> return -ENODEV;
> }
>
> +static bool i915_gem_object_is_idle(struct drm_i915_gem_object *obj,
> + bool readonly)
> +{
> + struct intel_engine_cs *ring = obj->ring;
> + u32 seqno = readonly ? obj->last_write_seqno : obj->last_read_seqno;
> +
> + if (ring == NULL || seqno == 0)
> + return true;
> +
> + return i915_seqno_passed(ring->get_seqno(ring, true), seqno);
> +}
> +
> +static bool __intel_pageflip_stall_check(struct drm_device *dev,
> + struct drm_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_unpin_work *work = intel_crtc->unpin_work;
> + u32 addr;
> +
> + if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
> + return true;
> +
> + if (!work->enable_stall_check)
> + return false;
> +
> + if ((crtc_sbc(intel_crtc) - intel_crtc->unpin_work->sbc) < 3)
> + return false;
> +
> + if (!i915_gem_object_is_idle(work->pending_flip_obj, true))
> + return false;
I take it this is done to mitigate my premature flip completion
concerns? Should work for the common case I suppose. Although if
someone does something like this "write,read(s),flip" it could
still complete the flip too soon. Waiting for last_read_seqno would
avoid that.
And double checking the hardware flip counter should avoid this
problem entirely. We already have it sampled in unpin_work->flip_count
for the mmio vs. cs flip race so it should be easy to check it here as
well. I suppose having both the flip counter and seqno checks should
provide the best protection for all gens.
As far as the seqno check goes, I was wondering if we should sample
the seqno when submitting the flip? I'm slightly worried how this will
work if userspace submitted a flip and already managed to pile more
rendering on top. This code would then wait for the seqno for those
later rendering operations.
> +
> + /* Potential stall - if we see that the flip has happened, assume a
> missed interrupt */
> + if (INTEL_INFO(dev)->gen >= 4)
> + addr = I915_HI_DISPBASE(I915_READ(DSPSURF(intel_crtc->plane)));
> + else
> + addr = I915_READ(DSPADDR(intel_crtc->plane));
> +
> + /* There is a potential issue here with a false positive after a flip
> + * to the same address.
> + */
> + return addr == work->gtt_offset;
> +}
> +
> +void intel_check_page_flip(struct drm_device *dev, int pipe)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + unsigned long flags;
> +
> + if (crtc == NULL)
> + return;
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
> + WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
> + intel_crtc->unpin_work->sbc, crtc_sbc(intel_crtc));
> + page_flip_completed(intel_crtc);
> + }
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> +}
> +
> static int intel_crtc_page_flip(struct drm_crtc *crtc,
> struct drm_framebuffer *fb,
> struct drm_pending_vblank_event *event,
> @@ -9312,12 +9389,20 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> /* We borrow the event spin lock for protecting unpin_work */
> spin_lock_irqsave(&dev->event_lock, flags);
> if (intel_crtc->unpin_work) {
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> - kfree(work);
> - drm_crtc_vblank_put(crtc);
> + /* Before declaring the flip queue wedged, check if
> + * the hardware completed the operation behind our backs.
> + */
> + if (__intel_pageflip_stall_check(dev, crtc)) {
> + DRM_DEBUG_DRIVER("flip queue: previous flip completed,
> continuing\n");
> + page_flip_completed(intel_crtc);
> + } else {
> + DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> + spin_unlock_irqrestore(&dev->event_lock, flags);
>
> - DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> - return -EBUSY;
> + drm_crtc_vblank_put(crtc);
> + kfree(work);
> + return -EBUSY;
> + }
> }
> intel_crtc->unpin_work = work;
> spin_unlock_irqrestore(&dev->event_lock, flags);
> @@ -9337,8 +9422,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> work->pending_flip_obj = obj;
>
> - work->enable_stall_check = true;
> -
> atomic_inc(&intel_crtc->unpin_work_count);
> intel_crtc->reset_counter =
> atomic_read(&dev_priv->gpu_error.reset_counter);
>
> @@ -9361,6 +9444,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> work->gtt_offset =
> i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> + work->enable_stall_check = true;
>
> ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
> page_flip_flags);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 78d4124..ac902ad 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -619,6 +619,7 @@ struct intel_unpin_work {
> u32 flip_count;
> u32 gtt_offset;
> bool enable_stall_check;
> + int sbc;
> };
>
> struct intel_set_config {
> @@ -763,6 +764,7 @@ __intel_framebuffer_create(struct drm_device *dev,
> void intel_prepare_page_flip(struct drm_device *dev, int plane);
> void intel_finish_page_flip(struct drm_device *dev, int pipe);
> void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> +void intel_check_page_flip(struct drm_device *dev, int pipe);
> struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> void assert_shared_dpll(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll,
> --
> 1.7.9.5
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
