[Intel-gfx] [PATCH] [drm/i915] Don't run retire work handler while suspended

[Keith Packard]

On Tue, 2008-10-14 at 21:08 -0700, Eric Anholt wrote:

> I think the solution here is setting the suspended semaphore before
> canceling the work, to kill further execbufs.  I suspect that with that,
> the additional cancel_work semaphore isn't needed.

Yup. Also, we need to prevent work queued during the shutdown from
starting the retire work handler again. Here's a patch which does all of
that. Sorry for restructuring i915_gem_idle, but the alternative was
fairly ugly.

From bb5b95dc54cf09571d41fb53dc1d612b826dd217 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp at keithp.com>
Date: Tue, 14 Oct 2008 20:48:42 -0700
Subject: [PATCH] [drm/i915] Don't run retire work handler while suspended

At leavevt and lastclose time, cancel any pending retire work handler
invocation, and keep the retire work handler from requeuing itself if it is
currently running.

This patch restructures i915_gem_idle to perform all of these tasks instead
of having both leavevt and lastclose call a sequence of functions.

Signed-off-by: Keith Packard <keithp at keithp.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   50 +++++++++++++++++++++-----------------
 1 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a23f2c9..4eeec9d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -50,6 +50,9 @@ static int i915_gem_object_get_page_list(struct drm_gem_object *obj);
 static void i915_gem_object_free_page_list(struct drm_gem_object *obj);
 static int i915_gem_object_wait_rendering(struct drm_gem_object *obj);
 
+static void
+i915_gem_cleanup_ringbuffer(struct drm_device *dev);
+
 int
 i915_gem_init_ioctl(struct drm_device *dev, void *data,
 		    struct drm_file *file_priv)
@@ -582,7 +585,7 @@ i915_add_request(struct drm_device *dev, uint32_t flush_domains)
 	was_empty = list_empty(&dev_priv->mm.request_list);
 	list_add_tail(&request->list, &dev_priv->mm.request_list);
 
-	if (was_empty)
+	if (was_empty && !dev_priv->mm.suspended)
 		schedule_delayed_work(&dev_priv->mm.retire_work, HZ);
 	return seqno;
 }
@@ -731,7 +734,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_retire_requests(dev);
-	if (!list_empty(&dev_priv->mm.request_list))
+	if (!dev_priv->mm.suspended &&
+	    !list_empty(&dev_priv->mm.request_list))
 		schedule_delayed_work(&dev_priv->mm.retire_work, HZ);
 	mutex_unlock(&dev->struct_mutex);
 }
@@ -2227,14 +2231,24 @@ i915_gem_idle(struct drm_device *dev)
 	uint32_t seqno, cur_seqno, last_seqno;
 	int stuck, ret;
 
-	if (dev_priv->mm.suspended)
+	mutex_lock(&dev->struct_mutex);
+
+	if (dev_priv->mm.suspended || dev_priv->ring.ring_obj == NULL) {
+		mutex_unlock(&dev->struct_mutex);
 		return 0;
+	}
 
 	/* Hack!  Don't let anybody do execbuf while we don't control the chip.
 	 * We need to replace this with a semaphore, or something.
 	 */
 	dev_priv->mm.suspended = 1;
 
+	/* Cancel the retire work handler, wait for it to finish if running
+	 */
+	mutex_unlock(&dev->struct_mutex);
+	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
+	mutex_lock(&dev->struct_mutex);
+
 	i915_kernel_lost_context(dev);
 
 	/* Flush the GPU along with all non-CPU write domains
@@ -2284,13 +2298,19 @@ i915_gem_idle(struct drm_device *dev)
 
 	/* Move all buffers out of the GTT. */
 	ret = i915_gem_evict_from_list(dev, &dev_priv->mm.inactive_list);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&dev->struct_mutex);
 		return ret;
+	}
 
 	BUG_ON(!list_empty(&dev_priv->mm.active_list));
 	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
 	BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
 	BUG_ON(!list_empty(&dev_priv->mm.request_list));
+
+	i915_gem_cleanup_ringbuffer(dev);
+	mutex_unlock(&dev->struct_mutex);
+
 	return 0;
 }
 
@@ -2500,34 +2520,20 @@ i915_gem_leavevt_ioctl(struct drm_device *dev, void *data,
 {
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
 	ret = i915_gem_idle(dev);
-	if (ret == 0)
-		i915_gem_cleanup_ringbuffer(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	drm_irq_uninstall(dev);
 
-	return 0;
+	return ret;
 }
 
 void
 i915_gem_lastclose(struct drm_device *dev)
 {
 	int ret;
-	drm_i915_private_t *dev_priv = dev->dev_private;
-
-	mutex_lock(&dev->struct_mutex);
-
-	if (dev_priv->ring.ring_obj != NULL) {
-		ret = i915_gem_idle(dev);
-		if (ret)
-			DRM_ERROR("failed to idle hardware: %d\n", ret);
-
-		i915_gem_cleanup_ringbuffer(dev);
-	}
 
-	mutex_unlock(&dev->struct_mutex);
+	ret = i915_gem_idle(dev);
+	if (ret)
+		DRM_ERROR("failed to idle hardware: %d\n", ret);
 }
 
 void
-- 
1.5.6.5



-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081014/a95b2f57/attachment.sig>