Re: [PATCH 01/10] drm/i915: Check for a stalled page flip after each vblank

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.

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@xxxxxxxxxxxx>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> [v4]
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux