On Fri, May 30, 2014 at 05:16:34PM +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 <simon.farnsworth@xxxxxxxxxxxx> > 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. > > Reported-by: Simon Farnsworth <simon@xxxxxxxxxxxx> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502 > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 33 ++++++++--- > drivers/gpu/drm/i915/i915_irq.c | 85 ++++++++------------------- > drivers/gpu/drm/i915/intel_display.c | 109 ++++++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_drv.h | 2 + > 4 files changed, 144 insertions(+), 85 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 0b063c03d188..a05a33ab4b33 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; > > @@ -602,23 +603,37 @@ 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)); > + { > + u32 addr; > + > + if (INTEL_INFO(dev)->gen >= 4) > + addr = DSPSURF(crtc->plane); > + else > + addr = DSPADDR(crtc->plane); > + > + seq_printf(m, "Current scanout address 0x%08x\n", > + I915_READ(addr)); "current" makes me think of the live address. But right now I can't think of a better term to use there. > } I don't like the extra {} block. Such things always give me an impression that this is a debug hack someone forgot to remove. > + > 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)); > + bool complete; > + > + seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset); > + > + if (INTEL_INFO(dev)->gen >= 4) > + complete = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane))) == work->gtt_offset; > + else > + complete = I915_READ(DSPADDR(crtc->plane)) == work->gtt_offset; > + > + seq_printf(m, "MMIO update completed? %d\n", complete); > } > } > 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 bce9afbc34bb..3be22794b53c 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); Can't we stick this call into intel_pipe_handle_vblank()? > > 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; Ah right this guy might not appreciate the check in intel_pipe_handle_vblank(). Oh well. > } > > 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 318747b444c6..764b277e5937 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3303,6 +3303,22 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev) > return false; > } > > +static void page_flip_completed(struct drm_i915_private *dev_priv, > + struct intel_crtc *intel_crtc, > + struct intel_unpin_work *work) > +{ assert_spin_locked(event_lock) ? > + if (work->event) > + drm_send_vblank_event(dev_priv->dev, > + intel_crtc->pipe, > + work->event); > + > + 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; > @@ -8848,21 +8864,12 @@ static void do_intel_finish_page_flip(struct drm_device *dev, > > /* 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); > - > + page_flip_completed(dev_priv, intel_crtc, work); > drm_crtc_vblank_put(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) > @@ -8940,11 +8947,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); > +} msc? > + > 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(); > } > @@ -9196,6 +9209,58 @@ static int intel_default_queue_flip(struct drm_device *dev, > return -ENODEV; > } > > +static bool __intel_pageflip_stall_check(struct drm_device *dev, > + struct drm_crtc *crtc, > + struct intel_unpin_work *work) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_i915_gem_object *obj; > + bool stall_detected; > + > + 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; > + > + /* 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)) == work->gtt_offset; > + } else { > + int dspaddr = DSPADDR(intel_crtc->plane); > + stall_detected = I915_READ(dspaddr) == work->gtt_offset; > + } Hmm. There's a slight danger of premature flip completion if the user issues a flip to the same address, and it really takes longer than three frames to complete. The hardware flip counter could be used to avoid that. I guess for for pre-ctg there isn't much we can do except hope that flips never take more than three frames. > + > + return stall_detected; > +} > + > +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, intel_crtc->unpin_work)) { > + WARN_ONCE(1, "Kicking stuck page flip\n"); > + drm_vblank_put(dev, intel_crtc->pipe); > + page_flip_completed(dev_priv, intel_crtc, intel_crtc->unpin_work); > + intel_crtc->unpin_work = NULL; > + } > + 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, > @@ -9243,12 +9308,25 @@ 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); > + bool stall; > + > + /* Before declaring the flip queue wedged, check if > + * the hardware completed the operation behind our backs. > + */ > + stall = __intel_pageflip_stall_check(dev, crtc, > + intel_crtc->unpin_work); > drm_crtc_vblank_put(crtc); > > - DRM_DEBUG_DRIVER("flip queue: crtc already busy\n"); > - return -EBUSY; > + if (stall) { > + DRM_DEBUG_DRIVER("flip queue: previous flip completed, continuing\n"); > + page_flip_completed(dev_priv, intel_crtc, intel_crtc->unpin_work); I was going to say you're leaking the vblank ref here until I saw that it was dropped earlier. Feels a bit strange to basically drop the ref you just got for this flip, and take over the ref from the earlier flip. I'd just move the drm_vblank_put() call into page_flip_completed(). > + } else { > + DRM_DEBUG_DRIVER("flip queue: crtc already busy\n"); > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + kfree(work); > + return -EBUSY; > + } > } > intel_crtc->unpin_work = work; > spin_unlock_irqrestore(&dev->event_lock, flags); > @@ -9268,8 +9346,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); > > @@ -9292,6 +9368,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 c597b0da93b6..b17c295fe967 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 { > @@ -761,6 +762,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, > -- > 2.0.0.rc4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
