Re: [PATCH v2] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 05, 2012 at 03:20:53PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
>     just wraps intel_crtc_wait_for_pending_flips_locked().

Sorry for the long delay in looking at this. One bikeshed here: I prefer
the patch changelog before the sob lines so that it gets included in the
commit message - most often it's rather interesting read, especially for
patches that take a few revisions to get right. More substantial comment
below.

>  drivers/gpu/drm/i915/intel_display.c |   76 ++++++++++++++++++---------------
>  1 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1a38267..a18e6e6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2228,6 +2228,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 (atomic_read(&dev_priv->mm.wedged))
> +		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_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));

I think we also need to add a dev_priv->mm.wedged check here, since the
gpu might die and never execute the pageflip. Otoh we don't complete any
pageflips that never executed due to a gpu hang, so maybe also add a big
FIXME. But with the wedged check we should at least not hang in an
non-interruptible wait.

The other thing is that the wait_even in finish_fb is not superflous,
since we should never see a framebuffer with pending flips for _this_ crtc
(it could have a pending flip on another crtc). So I think that code in
finish_fb should die, leaving just the comment and the finish_gpu call.

Cheers, Daniel

PS: Testcase would be awesome, but I have no ideas beyond what we already
have in flip_test unfortunately ...
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch