[PATCH 1/2] drm/i915: argument for deferring retirement

[PATCH 1/2] drm/i915: argument for deferring retirement

[Ben Widawsky]

Sometimes it may be the case when we idle the gpu or wait on something
we don't actually want to process the retiring list. This patch allows
callers to choose the behavior.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c            |    2 +-
 drivers/gpu/drm/i915/i915_drv.h            |    8 +++++---
 drivers/gpu/drm/i915/i915_gem.c            |   21 +++++++++++----------
 drivers/gpu/drm/i915/i915_gem_evict.c      |    2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
 6 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 8122738..1efc953 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2131,7 +2131,7 @@ int i915_driver_unload(struct drm_device *dev)
 		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
 
 	mutex_lock(&dev->struct_mutex);
-	ret = i915_gpu_idle(dev);
+	ret = i915_gpu_idle(dev, false);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f02a5f5..4fbe117 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1179,13 +1179,15 @@ void i915_gem_do_init(struct drm_device *dev,
 		      unsigned long start,
 		      unsigned long mappable_end,
 		      unsigned long end);
-int __must_check i915_gpu_idle(struct drm_device *dev);
+int __must_check i915_gpu_idle(struct drm_device *dev, bool defer_retirement);
 int __must_check i915_gem_idle(struct drm_device *dev);
 int __must_check i915_add_request(struct intel_ring_buffer *ring,
 				  struct drm_file *file,
 				  struct drm_i915_gem_request *request);
-int __must_check i915_wait_request(struct intel_ring_buffer *ring,
-				   uint32_t seqno);
+int __must_check i915_gem_wait_request(struct intel_ring_buffer *ring,
+				   uint32_t seqno,
+				   bool defer_retirement);
+#define i915_wait_request(ring, seqno) i915_gem_wait_request(ring, seqno, false)
 int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
 int __must_check
 i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index eb98a7f..ad16de9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1942,8 +1942,9 @@ i915_gem_retire_work_handler(struct work_struct *work)
  * request and object lists appropriately for that event.
  */
 int
-i915_wait_request(struct intel_ring_buffer *ring,
-		  uint32_t seqno)
+i915_gem_wait_request(struct intel_ring_buffer *ring,
+		  uint32_t seqno,
+		  bool defer_retirement)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	u32 ier;
@@ -2027,7 +2028,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
 	 * buffer to have made it to the inactive list, and we would need
 	 * a separate wait queue to handle that.
 	 */
-	if (ret == 0)
+	if (ret == 0 && !defer_retirement)
 		i915_gem_retire_requests_ring(ring);
 
 	return ret;
@@ -2172,7 +2173,7 @@ i915_gem_flush_ring(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-static int i915_ring_idle(struct intel_ring_buffer *ring)
+static int i915_ring_idle(struct intel_ring_buffer *ring, bool defer_retirement)
 {
 	int ret;
 
@@ -2186,18 +2187,18 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
 			return ret;
 	}
 
-	return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
+	return i915_gem_wait_request(ring, i915_gem_next_request_seqno(ring),
+				     defer_retirement);
 }
 
-int
-i915_gpu_idle(struct drm_device *dev)
+int i915_gpu_idle(struct drm_device *dev, bool defer_retirement)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	int ret, i;
 
 	/* Flush everything onto the inactive list. */
 	for (i = 0; i < I915_NUM_RINGS; i++) {
-		ret = i915_ring_idle(&dev_priv->ring[i]);
+		ret = i915_ring_idle(&dev_priv->ring[i], defer_retirement);
 		if (ret)
 			return ret;
 	}
@@ -3710,7 +3711,7 @@ i915_gem_idle(struct drm_device *dev)
 		return 0;
 	}
 
-	ret = i915_gpu_idle(dev);
+	ret = i915_gpu_idle(dev, false);
 	if (ret) {
 		mutex_unlock(&dev->struct_mutex);
 		return ret;
@@ -4201,7 +4202,7 @@ rescan:
 		 * This has a dramatic impact to reduce the number of
 		 * OOM-killer events whilst running the GPU aggressively.
 		 */
-		if (i915_gpu_idle(dev) == 0)
+		if (i915_gpu_idle(dev, false) == 0)
 			goto rescan;
 	}
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index ead5d00..2a6247b 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -195,7 +195,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 	trace_i915_gem_evict_everything(dev, purgeable_only);
 
 	/* Flush everything (on to the inactive lists) and evict */
-	ret = i915_gpu_idle(dev);
+	ret = i915_gpu_idle(dev, false);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c01cb20..0c01596 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1186,7 +1186,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			 * so every billion or so execbuffers, we need to stall
 			 * the GPU in order to reset the counters.
 			 */
-			ret = i915_gpu_idle(dev);
+			ret = i915_gpu_idle(dev, false);
 			if (ret)
 				goto err;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6042c5e..11bddd5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -55,7 +55,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
 
 	if (unlikely(dev_priv->mm.gtt->do_idle_maps)) {
 		dev_priv->mm.interruptible = false;
-		if (i915_gpu_idle(dev_priv->dev)) {
+		if (i915_gpu_idle(dev_priv->dev, false)) {
 			DRM_ERROR("Couldn't idle GPU\n");
 			/* Wait a bit, in hopes it avoids the hang */
 			udelay(10);
-- 
1.7.8.4

[PATCH 2/2] drm/i915: drm/i915: Fix recursive calls to unmap

[Ben Widawsky]

After the ILK vt-d workaround patches it became clear that we had
introduced a bug.  Chris Wilson tracked down the issue to recursive
calls to unmap. This happens because we try to optimize waiting on
requests by calling retire requests after the wait, which may drop the
last reference on an object and end up freeing the object (and then
unmap the object from the gtt).

After the last patch we can now choose to defer processing the retire
list.

Kudos to Chris Wilson for tracking this one down.

This fixes tests/gem_linear_blits in intel-gpu-tools.

v2: v1 used a global, v2 directly modified the call chain.

v3: As Keith Packard pointed out, v2 changed retirement behavior. While
it was intentional, it wasn't related to the bugfix, and so has been
removed.  To do this, changed i915_wait_request to be a macro to assure
old behavior is kept.

v4: Rebased and renamed strictly_idle to defer_retirement.

V5: Don't use a macro port new code, just change all the callers.
Also had a badly named argument in the function definition.

This patch fixes gem_unref_active_buffers from i-g-t in the non-VTd
case (ie. do_idle_maps forced to true).

Reported-by: guang.a.yang@intel.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42180
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 11bddd5..e050b90 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -55,7 +55,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
 
 	if (unlikely(dev_priv->mm.gtt->do_idle_maps)) {
 		dev_priv->mm.interruptible = false;
-		if (i915_gpu_idle(dev_priv->dev, false)) {
+		if (i915_gpu_idle(dev_priv->dev, true)) {
 			DRM_ERROR("Couldn't idle GPU\n");
 			/* Wait a bit, in hopes it avoids the hang */
 			udelay(10);
-- 
1.7.8.4

Re: [PATCH 1/2] drm/i915: argument for deferring retirement

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

On Tue, 24 Jan 2012 14:42:02 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> Sometimes it may be the case when we idle the gpu or wait on something
> we don't actually want to process the retiring list. This patch allows
> callers to choose the behavior.

<bikeshed>
!defer_retirement sounds like a double-negative to me, might be better
to have the parameter called 'do_retire' and change usage to match?
</bikeshed>

In any case, this looks easy to understand to me.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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

Re: [PATCH 2/2] drm/i915: drm/i915: Fix recursive calls to unmap

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 396 bytes --]

On Tue, 24 Jan 2012 14:42:03 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:

> This patch fixes gem_unref_active_buffers from i-g-t in the non-VTd
> case (ie. do_idle_maps forced to true).

Nice, one-line fix.

(if you agree with my <bikeshed> in the previous patch, this will have
to be changed to match)

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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

Re: [PATCH 1/2] drm/i915: argument for deferring retirement

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 526 bytes --]

On Tue, Jan 24, 2012 at 20:42, Ben Widawsky <ben@bwidawsk.net> wrote:

> Sometimes it may be the case when we idle the gpu or wait on something
> we don't actually want to process the retiring list. This patch allows
> callers to choose the behavior.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

(I agree with Keith comments though - defer retirement could be renamed to
do_retire or force_retire or similar..).

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 931 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

Re: [PATCH 2/2] drm/i915: drm/i915: Fix recursive calls to unmap

[Ben Widawsky]

On 01/24/12 15:00, Keith Packard wrote:
> On Tue, 24 Jan 2012 14:42:03 -0800, Ben Widawsky<ben@bwidawsk.net>  wrote:
>
>> This patch fixes gem_unref_active_buffers from i-g-t in the non-VTd
>> case (ie. do_idle_maps forced to true).
>
> Nice, one-line fix.
>
> (if you agree with my<bikeshed>  in the previous patch, this will have
> to be changed to match)
>
> Reviewed-by: Keith Packard<keithp@keithp.com>
>

I agree with the bikeshed, however I am not sure what you expect changed 
here. do_idle_maps is still forced to true to test the fix. So I'm going 
to assume you just read the comment too quickly and resubmit with the 
updated patches.

[PATCH 2/2] drm/i915: drm/i915: Fix recursive calls to unmap

[Ben Widawsky]

After the ILK vt-d workaround patches it became clear that we had
introduced a bug.  Chris Wilson tracked down the issue to recursive
calls to unmap. This happens because we try to optimize waiting on
requests by calling retire requests after the wait, which may drop the
last reference on an object and end up freeing the object (and then
unmap the object from the gtt).

After the last patch we can now choose to defer processing the retire
list.

Kudos to Chris Wilson for tracking this one down.

This patch fixes gem_unref_active_buffers from i-g-t. It was tested by
forcing do_idle_maps to true.

This also fixes tests/gem_linear_blits in intel-gpu-tools.

Reported-by: guang.a.yang@intel.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42180
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e050b90..11bddd5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -55,7 +55,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
 
 	if (unlikely(dev_priv->mm.gtt->do_idle_maps)) {
 		dev_priv->mm.interruptible = false;
-		if (i915_gpu_idle(dev_priv->dev, true)) {
+		if (i915_gpu_idle(dev_priv->dev, false)) {
 			DRM_ERROR("Couldn't idle GPU\n");
 			/* Wait a bit, in hopes it avoids the hang */
 			udelay(10);
-- 
1.7.8.4

Re: [PATCH 2/2] drm/i915: drm/i915: Fix recursive calls to unmap

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 437 bytes --]

On Tue, 24 Jan 2012 20:02:55 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:

> I agree with the bikeshed, however I am not sure what you expect changed 
> here. do_idle_maps is still forced to true to test the fix. So I'm going 
> to assume you just read the comment too quickly and resubmit with the 
> updated patches.

Just that the boolean sense of the new parameter is reversed, nothing else.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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