[Intel-gfx] [PATCH v3 2/4] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Daniel Vetter]

On Tue, Nov 27, 2012 at 08:34:56PM +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> intel_pipe_set_base() never actually waited for any pending page flips
> on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> the current front buffer. But the pending flips were actually tracked
> in the BO of the previous front buffer, so the call to intel_finish_fb()
> never did anything useful.
> 
> intel_crtc_wait_for_pending_flips() is the current _working_ way to wait
> for pending page flips. So use it in intel_pipe_set_base() too. Some
> refactoring was necessary to avoid locking struct_mutex twice.
> 
> v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
>     just wraps intel_crtc_wait_for_pending_flips_locked().
> 
> v3: Kill leftover wait_event() in intel_finish_fb()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   80 +++++++++++++++++----------------
>  1 files changed, 41 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8c2d810..ea710af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2238,10 +2238,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
>  	bool was_interruptible = dev_priv->mm.interruptible;
>  	int ret;
>  
> -	wait_event(dev_priv->pending_flip_queue,
> -		   i915_reset_in_progress(&dev_priv->gpu_error) ||
> -		   atomic_read(&obj->pending_flip) == 0);
> -
>  	/* Big Hammer, we also need to ensure that any pending
>  	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>  	 * current scanout is retired before unpinning the old
> @@ -2284,6 +2280,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
>  	}
>  }
>  
> +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (i915_reset_in_progress(&dev_priv->gpu_error))
> +		return false;

This check is not enough, since a full gpu reset might have happened while
we didn't look. Which means that we'll still be waiting forever. To really
close all gaps and races I think we need to full multi-state transistions
like it's already implemented in __wait_seqno (minus the first kick, since
no one is holding the dev->struct_mutex while waiting for a pageflip to
complete). So
- we need a reset_counter here like in wait_seqno
- need to reset the unpin_work state (and fire off any pending drm events
  while at it) to unblock kernel waiters and userspace

Also: igt test highly preferred ;-) Best would be to convert the hangman
test over to the subtest infrastructure (maybe easier when first ported
python and using argpars, but I don't mind bash's getopt support) and then
adding a new subtestcase which exercises flips vs. hangs.

Yours, Daniel

> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	return pending;
> +}
> +
> +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (crtc->fb == NULL)
> +		return;
> +
> +	wait_event(dev_priv->pending_flip_queue,
> +		   !intel_crtc_has_pending_flip(crtc));
> +
> +	intel_finish_fb(crtc->fb);
> +}
> +
> +static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	intel_crtc_wait_for_pending_flips_locked(crtc);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
>  static int
>  intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		    struct drm_framebuffer *fb)
> @@ -2317,8 +2353,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		return ret;
>  	}
>  
> -	if (crtc->fb)
> -		intel_finish_fb(crtc->fb);
> +	intel_crtc_wait_for_pending_flips_locked(crtc);
>  
>  	ret = dev_priv->display.update_plane(crtc, fb, x, y);
>  	if (ret) {
> @@ -2950,39 +2985,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
>  	udelay(100);
>  }
>  
> -static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long flags;
> -	bool pending;
> -
> -	if (i915_reset_in_progress(&dev_priv->gpu_error))
> -		return false;
> -
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> -	return pending;
> -}
> -
> -static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	if (crtc->fb == NULL)
> -		return;
> -
> -	wait_event(dev_priv->pending_flip_queue,
> -		   !intel_crtc_has_pending_flip(crtc));
> -
> -	mutex_lock(&dev->struct_mutex);
> -	intel_finish_fb(crtc->fb);
> -	mutex_unlock(&dev->struct_mutex);
> -}
> -
>  static bool ironlake_crtc_driving_pch(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -- 
> 1.7.8.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch