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
