| Message ID | 1351705121-17627-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 31 Oct 2012 19:38:41 +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. > > Now even the pending_flip counter is gone, so we should just > use intel_crtc_wait_for_pending_flips(), but since we're already holding > struct_mutex when we would call that function, we need another version > of it, that itself doesn't lock struct_mutex. That function call is now superfluous as you pointed out in an earlier review... But yes, not waking up the pending_flip_queue after a GPU hang is a recent bug, and could explain the lockup in https://bugs.freedesktop.org/show_bug.cgi?id=56337. -Chris
On Wed, Oct 31, 2012 at 05:43:16PM +0000, Chris Wilson wrote: > On Wed, 31 Oct 2012 19:38:41 +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. > > > > Now even the pending_flip counter is gone, so we should just > > use intel_crtc_wait_for_pending_flips(), but since we're already holding > > struct_mutex when we would call that function, we need another version > > of it, that itself doesn't lock struct_mutex. > > That function call is now superfluous as you pointed out in an earlier > review... I think we still need it when we're not doing a full modeset. Without it, intel_pipe_set_base() could overtake the flip. So the call could be moved outside of intel_pipe_set_base() so that it wouldn't be called for the full modeset path. But I suppose it's a bit more efficient to do it after we've pinned the new buffer, so simply moving it might not be the best idea. > But yes, not waking up the pending_flip_queue after a GPU hang is a > recent bug, and could explain the lockup in > https://bugs.freedesktop.org/show_bug.cgi?id=56337. OK. I suppose I'll cook up a quick patch for that as well.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 83e16f8..8f56d9a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2176,9 +2176,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, - atomic_read(&dev_priv->mm.wedged)); - /* 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 @@ -2221,6 +2218,37 @@ 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)); + + intel_finish_fb(crtc->fb); +} + static int intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *fb) @@ -2254,8 +2282,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) { @@ -2865,23 +2892,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 (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(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;