* [PATCH 0/2] drm/i915: Page flip stuff @ 2012-10-31 17:38 ville.syrjala 2012-10-31 17:38 ` [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() ville.syrjala 2012-10-31 17:38 ` [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() ville.syrjala 0 siblings, 2 replies; 8+ messages in thread From: ville.syrjala @ 2012-10-31 17:38 UTC (permalink / raw) To: intel-gfx This is my quick attempt at aliminating i915_gem_execbuffer_wait_for_flips(). While doing that I noticed that intel_pipe_set_base() never actually waited for pending flips, while it probably should. I also spotted another possible problem, for which I didn't provide a patch. We wait on the pending_flip_queue based on two different variables. One is the unpin_work, which indicates pending flips, and the other is mm.wedged, which seems to indicate that the GPU is stuck. I noticed that we don't seem to call wake_up(pending_flip_queue) after clearing mm.wedged. Is that a bug, or did I just miss something? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() 2012-10-31 17:38 [PATCH 0/2] drm/i915: Page flip stuff ville.syrjala @ 2012-10-31 17:38 ` ville.syrjala 2012-10-31 19:17 ` Ville Syrjälä 2012-11-01 0:18 ` Eric Anholt 2012-10-31 17:38 ` [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() ville.syrjala 1 sibling, 2 replies; 8+ messages in thread From: ville.syrjala @ 2012-10-31 17:38 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> As per Chris Wilson's suggestion make i915_gem_execbuffer_wait_for_flips() go away. This was used to stall the GPU ring while there are pending page flips involving the relevant BO. Ie. while the BO is still being scanned out by the display controller. The recommended alternative is to use the page flip events to wait for the page flips to fully complete before reusing the BO of the old front buffer. Or use more buffers. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 7 ---- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 42 ---------------------------- drivers/gpu/drm/i915/intel_display.c | 12 +------- 3 files changed, 1 insertions(+), 60 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2fcf284..0121b34 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1053,13 +1053,6 @@ struct drm_i915_gem_object { /** for phy allocated objects */ struct drm_i915_gem_phys_object *phys_obj; - - /** - * Number of crtcs where this object is currently the fb, but - * will be page flipped away on the next vblank. When it - * reaches 0, dev_priv->pending_flip_queue will be woken up. - */ - atomic_t pending_flip; }; #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 91d43d5..2e527be 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -611,44 +611,11 @@ err: } static int -i915_gem_execbuffer_wait_for_flips(struct intel_ring_buffer *ring, u32 flips) -{ - u32 plane, flip_mask; - int ret; - - /* Check for any pending flips. As we only maintain a flip queue depth - * of 1, we can simply insert a WAIT for the next display flip prior - * to executing the batch and avoid stalling the CPU. - */ - - for (plane = 0; flips >> plane; plane++) { - if (((flips >> plane) & 1) == 0) - continue; - - if (plane) - flip_mask = MI_WAIT_FOR_PLANE_B_FLIP; - else - flip_mask = MI_WAIT_FOR_PLANE_A_FLIP; - - ret = intel_ring_begin(ring, 2); - if (ret) - return ret; - - intel_ring_emit(ring, MI_WAIT_FOR_EVENT | flip_mask); - intel_ring_emit(ring, MI_NOOP); - intel_ring_advance(ring); - } - - return 0; -} - -static int i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring, struct list_head *objects) { struct drm_i915_gem_object *obj; uint32_t flush_domains = 0; - uint32_t flips = 0; int ret; list_for_each_entry(obj, objects, exec_list) { @@ -659,18 +626,9 @@ i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring, if (obj->base.write_domain & I915_GEM_DOMAIN_CPU) i915_gem_clflush_object(obj); - if (obj->base.pending_write_domain) - flips |= atomic_read(&obj->pending_flip); - flush_domains |= obj->base.write_domain; } - if (flips) { - ret = i915_gem_execbuffer_wait_for_flips(ring, flips); - if (ret) - return ret; - } - if (flush_domains & I915_GEM_DOMAIN_CPU) intel_gtt_chipset_flush(); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cbc0035..83e16f8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2177,8 +2177,7 @@ intel_finish_fb(struct drm_framebuffer *old_fb) int ret; wait_event(dev_priv->pending_flip_queue, - atomic_read(&dev_priv->mm.wedged) || - atomic_read(&obj->pending_flip) == 0); + 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 @@ -6758,9 +6757,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev, obj = work->old_fb_obj; - atomic_clear_mask(1 << intel_crtc->plane, - &obj->pending_flip.counter); - wake_up(&dev_priv->pending_flip_queue); schedule_work(&work->work); @@ -7102,11 +7098,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, work->enable_stall_check = true; - /* Block clients from rendering to the new back buffer until - * the flip occurs and the object is no longer visible. - */ - atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip); - ret = dev_priv->display.queue_flip(dev, crtc, fb, obj); if (ret) goto cleanup_pending; @@ -7120,7 +7111,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, return 0; cleanup_pending: - atomic_sub(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip); drm_gem_object_unreference(&work->old_fb_obj->base); drm_gem_object_unreference(&obj->base); mutex_unlock(&dev->struct_mutex); -- 1.7.8.6 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() 2012-10-31 17:38 ` [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() ville.syrjala @ 2012-10-31 19:17 ` Ville Syrjälä 2012-11-01 0:18 ` Eric Anholt 1 sibling, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2012-10-31 19:17 UTC (permalink / raw) To: intel-gfx On Wed, Oct 31, 2012 at 07:38:40PM +0200, ville.syrjala@linux.intel.com wrote: > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cbc0035..83e16f8 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2177,8 +2177,7 @@ intel_finish_fb(struct drm_framebuffer *old_fb) > int ret; > > wait_event(dev_priv->pending_flip_queue, > - atomic_read(&dev_priv->mm.wedged) || > - atomic_read(&obj->pending_flip) == 0); > + atomic_read(&dev_priv->mm.wedged)); And of course this bit is total nonsense now. I had the logic inverted in my mind. So this guy actually wants to stop waiting when the GPU hangs, not the other way around. So I need to drop the whole wait_event() here. Stand by for v2... -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() 2012-10-31 17:38 ` [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() ville.syrjala 2012-10-31 19:17 ` Ville Syrjälä @ 2012-11-01 0:18 ` Eric Anholt 2012-11-01 8:58 ` Chris Wilson 1 sibling, 1 reply; 8+ messages in thread From: Eric Anholt @ 2012-11-01 0:18 UTC (permalink / raw) To: ville.syrjala, intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 715 bytes --] ville.syrjala@linux.intel.com writes: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > As per Chris Wilson's suggestion make > i915_gem_execbuffer_wait_for_flips() go away. > > This was used to stall the GPU ring while there are pending > page flips involving the relevant BO. Ie. while the BO is still > being scanned out by the display controller. > > The recommended alternative is to use the page flip events to > wait for the page flips to fully complete before reusing the BO > of the old front buffer. Or use more buffers. So, after this change, if userland submits an execbuf between having requested a pageflip and the vblank that the flip shows up on, does that exec block? [-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() 2012-11-01 0:18 ` Eric Anholt @ 2012-11-01 8:58 ` Chris Wilson 0 siblings, 0 replies; 8+ messages in thread From: Chris Wilson @ 2012-11-01 8:58 UTC (permalink / raw) To: Eric Anholt, ville.syrjala, intel-gfx [-- Attachment #1: Type: text/plain, Size: 1023 bytes --] On Wed, 31 Oct 2012 17:18:47 -0700, Eric Anholt <eric@anholt.net> wrote: > ville.syrjala@linux.intel.com writes: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > As per Chris Wilson's suggestion make > > i915_gem_execbuffer_wait_for_flips() go away. > > > > This was used to stall the GPU ring while there are pending > > page flips involving the relevant BO. Ie. while the BO is still > > being scanned out by the display controller. > > > > The recommended alternative is to use the page flip events to > > wait for the page flips to fully complete before reusing the BO > > of the old front buffer. Or use more buffers. > > So, after this change, if userland submits an execbuf between having > requested a pageflip and the vblank that the flip shows up on, does that > exec block? The code itself is dysfunctional and would cause a GPU hang on snb/ivb. Why should the kernel be imposing a policy upon userspace where none is required? -Chris -- Chris Wilson, Intel Open Source Technology Centre [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() 2012-10-31 17:38 [PATCH 0/2] drm/i915: Page flip stuff ville.syrjala 2012-10-31 17:38 ` [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() ville.syrjala @ 2012-10-31 17:38 ` ville.syrjala 2012-10-31 17:43 ` Chris Wilson 1 sibling, 1 reply; 8+ messages in thread From: ville.syrjala @ 2012-10-31 17:38 UTC (permalink / raw) To: intel-gfx 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. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 54 ++++++++++++++++++++-------------- 1 files changed, 32 insertions(+), 22 deletions(-) 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; -- 1.7.8.6 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() 2012-10-31 17:38 ` [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() ville.syrjala @ 2012-10-31 17:43 ` Chris Wilson 2012-10-31 18:17 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Chris Wilson @ 2012-10-31 17:43 UTC (permalink / raw) To: ville.syrjala, intel-gfx [-- Attachment #1: Type: text/plain, Size: 1026 bytes --] 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 -- Chris Wilson, Intel Open Source Technology Centre [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() 2012-10-31 17:43 ` Chris Wilson @ 2012-10-31 18:17 ` Ville Syrjälä 0 siblings, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2012-10-31 18:17 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-11-01 8:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-31 17:38 [PATCH 0/2] drm/i915: Page flip stuff ville.syrjala 2012-10-31 17:38 ` [PATCH 1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips() ville.syrjala 2012-10-31 19:17 ` Ville Syrjälä 2012-11-01 0:18 ` Eric Anholt 2012-11-01 8:58 ` Chris Wilson 2012-10-31 17:38 ` [PATCH 2/2] drm/i915: Wait for pending flips in intel_pipe_set_base() ville.syrjala 2012-10-31 17:43 ` Chris Wilson 2012-10-31 18:17 ` Ville Syrjälä
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.