| Message ID | 1409666265-19286-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 02, 2014 at 02:57:36PM +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@onelan.co.uk> > 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. I have this plan that we stop using the flip done interrupts, since they're anyway fairly broken on bdw. But I haven't really thought how that would interact with this stall checking. But I guess we can merge this stuff first and figure out the rest when someone gets around to posting a patch for killing flip done. > > v2: Cleanup, refactor, and rename > v3: Only start counting vblanks after the flip command has been seen by > the hardware. > v4: Record the seqno after we touch the ring, or else there may be no > seqno allocated yet. > v5: Rebase on mmio-flip. > > Reported-by: Simon Farnsworth <simon@farnz.org.uk> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> [v4] > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 34 +++++++--- > drivers/gpu/drm/i915/i915_irq.c | 84 +++++++------------------ > drivers/gpu/drm/i915/intel_display.c | 119 ++++++++++++++++++++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 3 + > 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 97c257d06729..f7816b4329d6 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -518,6 +518,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; > int ret; > @@ -537,6 +538,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); > @@ -544,23 +547,34 @@ 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); > } > + if (work->flip_queued_request) { > + struct i915_gem_request *rq = work->flip_queued_request; > + seq_printf(m, "Flip queued on %s at seqno %u, next seqno %u [current breadcrumb %u], completed? %d\n", > + rq->engine->name, > + rq->seqno, rq->i915->next_seqno, > + rq->engine->get_seqno(rq->engine), > + __i915_request_complete__wa(rq)); > + } else > + seq_printf(m, "Flip not associated with any ring\n"); > + seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", > + work->flip_queued_vblank, > + work->flip_ready_vblank, > + drm_vblank_count(dev, crtc->pipe)); > 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 1cd3a8ecb2f8..60ca89986499 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2090,8 +2090,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > spin_unlock(&dev_priv->irq_lock); > > for_each_pipe(dev_priv, 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); > @@ -2390,8 +2391,9 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir) > DRM_ERROR("Poison interrupt\n"); > > for_each_pipe(dev_priv, 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)) > @@ -2440,8 +2442,9 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir) > intel_opregion_asle_intr(dev); > > for_each_pipe(dev_priv, 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)) { > @@ -2596,8 +2599,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) > if (pipe_iir) { > ret = IRQ_HANDLED; > I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir); > - 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); > @@ -2900,52 +2904,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 > */ > @@ -4091,7 +4049,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); > > @@ -4102,11 +4060,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) > @@ -4274,7 +4235,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); > > @@ -4285,11 +4246,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 8496a6b6f182..18c431e4f099 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3405,6 +3405,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; > @@ -9234,7 +9257,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; > @@ -9254,23 +9276,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) > @@ -9688,6 +9696,64 @@ static int intel_default_queue_flip(struct i915_gem_request *rq, > return -ENODEV; > } > > +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 (work->flip_ready_vblank == 0) { > + if (work->flip_queued_request && > + !i915_request_complete(work->flip_queued_request)) > + return false; > + > + work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe); > + } > + > + if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3) > + return false; > + > + /* 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. We could address this by checking for a > + * non-incrementing frame counter. > + */ > + 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->flip_queued_vblank, drm_vblank_count(dev, pipe)); > + 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, > @@ -9748,12 +9814,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); > @@ -9773,8 +9847,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); > > @@ -9839,6 +9911,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > } > > work->flip_queued_request = rq; > + work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe); > work->enable_stall_check = true; > > i915_gem_track_fb(work->old_fb_obj, obj, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 18403c266682..1722673c534e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -663,6 +663,8 @@ struct intel_unpin_work { > u32 flip_count; > u32 gtt_offset; > struct i915_gem_request *flip_queued_request; > + int flip_queued_vblank; > + int flip_ready_vblank; > bool enable_stall_check; > }; > > @@ -847,6 +849,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); > > /* shared dpll functions */ > struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc); > -- > 2.1.0
On Tue, Sep 02, 2014 at 05:31:22PM +0300, Ville Syrjälä wrote: > On Tue, Sep 02, 2014 at 02:57:36PM +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@onelan.co.uk> > > 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. > > I have this plan that we stop using the flip done interrupts, > since they're anyway fairly broken on bdw. But I haven't really > thought how that would interact with this stall checking. Now that you remind me, I had a plan to rewrite this on top of that future. > But I guess we can merge this stuff first and figure out the rest > when someone gets around to posting a patch for killing flip done. Yeah, I think this is important in the short term as we have a bug causing system freezes that this at least (+ next patch) allows us to recover from. -Chris
On Tue, Sep 02, 2014 at 02:57:36PM +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@onelan.co.uk> > 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 > v3: Only start counting vblanks after the flip command has been seen by > the hardware. > v4: Record the seqno after we touch the ring, or else there may be no > seqno allocated yet. > v5: Rebase on mmio-flip. > > Reported-by: Simon Farnsworth <simon@farnz.org.uk> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> [v4] > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> This seems to be on top of the s/seqno/request/ patch which moves around flip->enable_stall_check, which this patch here also does. I'm rather confused about the resulting conflict. Especially since it took me a while to understand what happened to enable_stall_check in your request patch, so I don't think I'm qualified to resolve the conflict. Can you please rebase this one here quickly? Thanks, Daniel > --- > drivers/gpu/drm/i915/i915_debugfs.c | 34 +++++++--- > drivers/gpu/drm/i915/i915_irq.c | 84 +++++++------------------ > drivers/gpu/drm/i915/intel_display.c | 119 ++++++++++++++++++++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 3 + > 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 97c257d06729..f7816b4329d6 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -518,6 +518,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; > int ret; > @@ -537,6 +538,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); > @@ -544,23 +547,34 @@ 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); > } > + if (work->flip_queued_request) { > + struct i915_gem_request *rq = work->flip_queued_request; > + seq_printf(m, "Flip queued on %s at seqno %u, next seqno %u [current breadcrumb %u], completed? %d\n", > + rq->engine->name, > + rq->seqno, rq->i915->next_seqno, > + rq->engine->get_seqno(rq->engine), > + __i915_request_complete__wa(rq)); > + } else > + seq_printf(m, "Flip not associated with any ring\n"); > + seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", > + work->flip_queued_vblank, > + work->flip_ready_vblank, > + drm_vblank_count(dev, crtc->pipe)); > 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 1cd3a8ecb2f8..60ca89986499 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2090,8 +2090,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > spin_unlock(&dev_priv->irq_lock); > > for_each_pipe(dev_priv, 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); > @@ -2390,8 +2391,9 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir) > DRM_ERROR("Poison interrupt\n"); > > for_each_pipe(dev_priv, 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)) > @@ -2440,8 +2442,9 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir) > intel_opregion_asle_intr(dev); > > for_each_pipe(dev_priv, 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)) { > @@ -2596,8 +2599,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) > if (pipe_iir) { > ret = IRQ_HANDLED; > I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir); > - 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); > @@ -2900,52 +2904,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 > */ > @@ -4091,7 +4049,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); > > @@ -4102,11 +4060,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) > @@ -4274,7 +4235,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); > > @@ -4285,11 +4246,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 8496a6b6f182..18c431e4f099 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3405,6 +3405,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; > @@ -9234,7 +9257,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; > @@ -9254,23 +9276,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) > @@ -9688,6 +9696,64 @@ static int intel_default_queue_flip(struct i915_gem_request *rq, > return -ENODEV; > } > > +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 (work->flip_ready_vblank == 0) { > + if (work->flip_queued_request && > + !i915_request_complete(work->flip_queued_request)) > + return false; > + > + work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe); > + } > + > + if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3) > + return false; > + > + /* 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. We could address this by checking for a > + * non-incrementing frame counter. > + */ > + 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->flip_queued_vblank, drm_vblank_count(dev, pipe)); > + 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, > @@ -9748,12 +9814,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); > @@ -9773,8 +9847,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); > > @@ -9839,6 +9911,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > } > > work->flip_queued_request = rq; > + work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe); > work->enable_stall_check = true; > > i915_gem_track_fb(work->old_fb_obj, obj, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 18403c266682..1722673c534e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -663,6 +663,8 @@ struct intel_unpin_work { > u32 flip_count; > u32 gtt_offset; > struct i915_gem_request *flip_queued_request; > + int flip_queued_vblank; > + int flip_ready_vblank; > bool enable_stall_check; > }; > > @@ -847,6 +849,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); > > /* shared dpll functions */ > struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc); > -- > 2.1.0 >
On Tue, Sep 02, 2014 at 04:52:54PM +0200, Daniel Vetter wrote: > On Tue, Sep 02, 2014 at 02:57:36PM +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@onelan.co.uk> > > 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 > > v3: Only start counting vblanks after the flip command has been seen by > > the hardware. > > v4: Record the seqno after we touch the ring, or else there may be no > > seqno allocated yet. > > v5: Rebase on mmio-flip. > > > > Reported-by: Simon Farnsworth <simon@farnz.org.uk> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> [v4] > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > This seems to be on top of the s/seqno/request/ patch which moves around > flip->enable_stall_check, which this patch here also does. I'm rather > confused about the resulting conflict. Especially since it took me a while > to understand what happened to enable_stall_check in your request patch, > so I don't think I'm qualified to resolve the conflict. > > Can you please rebase this one here quickly? I have an aversion to seqno/ring combinations. That is so archaic! :) Just these two? I think keeping the requests interface for the boosting is neater than readding more seqno compares. -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 97c257d06729..f7816b4329d6 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -518,6 +518,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; int ret; @@ -537,6 +538,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); @@ -544,23 +547,34 @@ 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); } + if (work->flip_queued_request) { + struct i915_gem_request *rq = work->flip_queued_request; + seq_printf(m, "Flip queued on %s at seqno %u, next seqno %u [current breadcrumb %u], completed? %d\n", + rq->engine->name, + rq->seqno, rq->i915->next_seqno, + rq->engine->get_seqno(rq->engine), + __i915_request_complete__wa(rq)); + } else + seq_printf(m, "Flip not associated with any ring\n"); + seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", + work->flip_queued_vblank, + work->flip_ready_vblank, + drm_vblank_count(dev, crtc->pipe)); 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 1cd3a8ecb2f8..60ca89986499 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2090,8 +2090,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) spin_unlock(&dev_priv->irq_lock); for_each_pipe(dev_priv, 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); @@ -2390,8 +2391,9 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir) DRM_ERROR("Poison interrupt\n"); for_each_pipe(dev_priv, 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)) @@ -2440,8 +2442,9 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir) intel_opregion_asle_intr(dev); for_each_pipe(dev_priv, 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)) { @@ -2596,8 +2599,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) if (pipe_iir) { ret = IRQ_HANDLED; I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir); - 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); @@ -2900,52 +2904,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 */ @@ -4091,7 +4049,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); @@ -4102,11 +4060,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) @@ -4274,7 +4235,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); @@ -4285,11 +4246,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 8496a6b6f182..18c431e4f099 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3405,6 +3405,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; @@ -9234,7 +9257,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; @@ -9254,23 +9276,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) @@ -9688,6 +9696,64 @@ static int intel_default_queue_flip(struct i915_gem_request *rq, return -ENODEV; } +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 (work->flip_ready_vblank == 0) { + if (work->flip_queued_request && + !i915_request_complete(work->flip_queued_request)) + return false; + + work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe); + } + + if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3) + return false; + + /* 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. We could address this by checking for a + * non-incrementing frame counter. + */ + 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->flip_queued_vblank, drm_vblank_count(dev, pipe)); + 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, @@ -9748,12 +9814,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); @@ -9773,8 +9847,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); @@ -9839,6 +9911,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, } work->flip_queued_request = rq; + work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe); work->enable_stall_check = true; i915_gem_track_fb(work->old_fb_obj, obj, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 18403c266682..1722673c534e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -663,6 +663,8 @@ struct intel_unpin_work { u32 flip_count; u32 gtt_offset; struct i915_gem_request *flip_queued_request; + int flip_queued_vblank; + int flip_ready_vblank; bool enable_stall_check; }; @@ -847,6 +849,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); /* shared dpll functions */ struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
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@onelan.co.uk> 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 v3: Only start counting vblanks after the flip command has been seen by the hardware. v4: Record the seqno after we touch the ring, or else there may be no seqno allocated yet. v5: Rebase on mmio-flip. Reported-by: Simon Farnsworth <simon@farnz.org.uk> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> [v4] Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_debugfs.c | 34 +++++++--- drivers/gpu/drm/i915/i915_irq.c | 84 +++++++------------------ drivers/gpu/drm/i915/intel_display.c | 119 ++++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 3 + 4 files changed, 147 insertions(+), 93 deletions(-)