From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 01/10] drm/i915: Check for a stalled page flip after each vblank Date: Tue, 2 Sep 2014 16:52:54 +0200 Message-ID: <20140902145253.GC15520@phenom.ffwll.local> References: <1409666265-19286-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 74B836E0CB for ; Tue, 2 Sep 2014 07:52:31 -0700 (PDT) Received: by mail-wi0-f182.google.com with SMTP id z2so8038512wiv.15 for ; Tue, 02 Sep 2014 07:52:30 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1409666265-19286-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > Date: Wed Sep 1 17:47:52 2010 +0100 > = > drm/i915: Avoid pageflipping freeze when we miss the flip prepare int= errupt > = > 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 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D75502 > Signed-off-by: Chris Wilson > Cc: Daniel Vetter > Cc: Ville Syrj=E4l=E4 > Reviewed-by: Ville Syrj=E4l=E4 [v4] > Signed-off-by: Daniel Vetter 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/i= 915_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 =3D m->private; > struct drm_device *dev =3D node->minor->dev; > + struct drm_i915_private *dev_priv =3D 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 =3D 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 =3D 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 >=3D 4) > + addr =3D I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane))); > + else > + addr =3D 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 =3D 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_o= ffset); > + seq_printf(m, "MMIO update completed? %d\n", addr =3D=3D 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_devi= ce *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_devi= ce *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 =3D 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, boo= l 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 =3D dev->dev_private; > - struct drm_crtc *crtc =3D dev_priv->pipe_to_crtc_mapping[pipe]; > - struct intel_crtc *intel_crtc =3D 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 =3D=3D NULL) > - return; > - > - spin_lock_irqsave(&dev->event_lock, flags); > - work =3D intel_crtc->unpin_work; > - > - if (work =3D=3D NULL || > - atomic_read(&work->pending) >=3D INTEL_FLIP_COMPLETE || > - !work->enable_stall_check) { > - /* Either the pending flip IRQ arrived, or we're too early. Don't chec= k */ > - spin_unlock_irqrestore(&dev->event_lock, flags); > - return; > - } > - > - /* Potential stall - if we see that the flip has happened, assume a mis= sed interrupt */ > - obj =3D work->pending_flip_obj; > - if (INTEL_INFO(dev)->gen >=3D 4) { > - int dspsurf =3D DSPSURF(intel_crtc->plane); > - stall_detected =3D I915_HI_DISPBASE(I915_READ(dspsurf)) =3D=3D > - i915_gem_obj_ggtt_offset(obj); > - } else { > - int dspaddr =3D DSPADDR(intel_crtc->plane); > - stall_detected =3D I915_READ(dspaddr) =3D=3D (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 *d= ev, > return false; > = > if ((iir & flip_pending) =3D=3D 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 *d= ev, > return false; > = > if ((iir & flip_pending) =3D=3D 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 =3D to_i915(intel_crtc->base.dev); > + struct intel_unpin_work *work =3D intel_crtc->unpin_work; > + > + /* ensure that the unpin work is consistent wrt ->pending. */ > + smp_rmb(); > + intel_crtc->unpin_work =3D 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 =3D 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 =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D 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_d= evice *dev, > return; > } > = > - /* and that the unpin work is consistent wrt ->pending. */ > - smp_rmb(); > - > - intel_crtc->unpin_work =3D 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_ge= m_request *rq, > return -ENODEV; > } > = > +static bool __intel_pageflip_stall_check(struct drm_device *dev, > + struct drm_crtc *crtc) > +{ > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + struct intel_unpin_work *work =3D intel_crtc->unpin_work; > + u32 addr; > + > + if (atomic_read(&work->pending) >=3D INTEL_FLIP_COMPLETE) > + return true; > + > + if (!work->enable_stall_check) > + return false; > + > + if (work->flip_ready_vblank =3D=3D 0) { > + if (work->flip_queued_request && > + !i915_request_complete(work->flip_queued_request)) > + return false; > + > + work->flip_ready_vblank =3D 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 >=3D 4) > + addr =3D I915_HI_DISPBASE(I915_READ(DSPSURF(intel_crtc->plane))); > + else > + addr =3D 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 =3D=3D work->gtt_offset; > +} > + > +void intel_check_page_flip(struct drm_device *dev, int pipe) > +{ > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct drm_crtc *crtc =3D dev_priv->pipe_to_crtc_mapping[pipe]; > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + unsigned long flags; > + > + if (crtc =3D=3D 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, pi= pe)); > + 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 =3D work; > spin_unlock_irqrestore(&dev->event_lock, flags); > @@ -9773,8 +9847,6 @@ static int intel_crtc_page_flip(struct drm_crtc *cr= tc, > = > work->pending_flip_obj =3D obj; > = > - work->enable_stall_check =3D true; > - > atomic_inc(&intel_crtc->unpin_work_count); > intel_crtc->reset_counter =3D atomic_read(&dev_priv->gpu_error.reset_co= unter); > = > @@ -9839,6 +9911,7 @@ static int intel_crtc_page_flip(struct drm_crtc *cr= tc, > } > = > work->flip_queued_request =3D rq; > + work->flip_queued_vblank =3D drm_vblank_count(dev, intel_crtc->pipe); > work->enable_stall_check =3D 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/inte= l_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 *c= rtc); > -- = > 2.1.0 > = -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch