[1/2] drm/i915: Kill i915_gem_execbuffer_wait_for_flips()

Commit Message

Ville Syrjälä Oct. 31, 2012, 5:38 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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ä <[e-mail]>
---
 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(-)

Comments

Ville Syrjälä Oct. 31, 2012, 7:17 p.m. UTC | #1

On Wed, Oct 31, 2012 at 07:38:40PM +0200, [e-mail] 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...

Eric Anholt Nov. 1, 2012, 12:18 a.m. UTC | #2

[e-mail] writes:

> From: Ville Syrjälä <[e-mail]>
>
> 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?

Chris Wilson Nov. 1, 2012, 8:58 a.m. UTC | #3

On Wed, 31 Oct 2012 17:18:47 -0700, Eric Anholt <[e-mail]> wrote:
> [e-mail] writes:
> 
> > From: Ville Syrjälä <[e-mail]>
> >
> > 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

Patch

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);