[PATCH 00/12 v2] wait for BO with timeout

[PATCH 00/12 v2] wait for BO with timeout

[Ben Widawsky]

Hopefully addressed all the comments from the first series. Tests and
sample libdrm remain in the same place.

Much thanks to Chris Wilson on helping me smash this into shape.

Ben Widawsky (12):
  drm/i915: remove do_retire from i915_wait_request
  drm/i915: remove some extra retiring
  drm/i915: move vbetool invoked ier stuff
  drm/i915: kill waiting_seqno
  drm/i915: drop polled waits from i915_wait_request
  drm/i915: extract __wait_seqno from i915_wait_request
  drm/i915: remove polled wait from throttle
  drm/i915: use __wait_seqno for ring throttle
  drm/i915: timeout parameter for seqno wait
  drm/i915: extract some common olr+wedge code
  drm/i915: wait render timeout ioctl
  drm/i915: s/i915_wait_reqest/i915_wait_seqno/g

 drivers/gpu/drm/i915/i915_debugfs.c        |    4 +-
 drivers/gpu/drm/i915/i915_dma.c            |    3 +-
 drivers/gpu/drm/i915/i915_drv.h            |   11 +-
 drivers/gpu/drm/i915/i915_gem.c            |  295 +++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_gem_evict.c      |   15 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
 drivers/gpu/drm/i915/i915_irq.c            |   21 +-
 drivers/gpu/drm/i915/intel_overlay.c       |    6 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    1 -
 include/drm/i915_drm.h                     |    8 +
 12 files changed, 230 insertions(+), 142 deletions(-)

-- 
1.7.10

[PATCH 01/12 v2] drm/i915: remove do_retire from i915_wait_request

[Ben Widawsky]

This originates from a hack by me to quickly fix a bug in an earlier
patch where we needed control over whether or not waiting on a seqno
actually did any retire list processing. Since the two operations aren't
clearly related, we should pull the parameter out of the wait function,
and make the caller responsible for retiring if the action is desired.

The only function call site which did not get an explicit retire_request call
(on purpose) is i915_gem_inactive_shrink(). That code was already calling
retire_request a second time.

v2: don't modify any behavior excepit i915_gem_inactive_shrink(Daniel)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c            |    3 ++-
 drivers/gpu/drm/i915/i915_drv.h            |    5 ++---
 drivers/gpu/drm/i915/i915_gem.c            |   33 ++++++++++------------------
 drivers/gpu/drm/i915/i915_gem_evict.c      |   15 ++++++++++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
 drivers/gpu/drm/i915/intel_overlay.c       |    8 +++----
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +++-
 8 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e2983a9..36c8d5f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2013,9 +2013,10 @@ 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, true);
+	ret = i915_gpu_idle(dev);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
+	i915_gem_retire_requests(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	/* Cancel the retire work handler, which should be idle now. */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cf2c896..34d7041 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1298,14 +1298,13 @@ int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
-int __must_check i915_gpu_idle(struct drm_device *dev, bool do_retire);
+int __must_check i915_gpu_idle(struct drm_device *dev);
 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,
-				   bool do_retire);
+				   uint32_t seqno);
 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 0d53eac..25be0e0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1825,8 +1825,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
  */
 int
 i915_wait_request(struct intel_ring_buffer *ring,
-		  uint32_t seqno,
-		  bool do_retire)
+		  uint32_t seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	u32 ier;
@@ -1902,14 +1901,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
 	if (atomic_read(&dev_priv->mm.wedged))
 		ret = -EAGAIN;
 
-	/* Directly dispatch request retiring.  While we have the work queue
-	 * to handle this, the waiter on a request often wants an associated
-	 * buffer to have made it to the inactive list, and we would need
-	 * a separate wait queue to handle that.
-	 */
-	if (ret == 0 && do_retire)
-		i915_gem_retire_requests_ring(ring);
-
 	return ret;
 }
 
@@ -1931,10 +1922,10 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
 	 * it.
 	 */
 	if (obj->active) {
-		ret = i915_wait_request(obj->ring, obj->last_rendering_seqno,
-					true);
+		ret = i915_wait_request(obj->ring, obj->last_rendering_seqno);
 		if (ret)
 			return ret;
+		i915_gem_retire_requests_ring(obj->ring);
 	}
 
 	return 0;
@@ -2117,7 +2108,7 @@ i915_gem_flush_ring(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-static int i915_ring_idle(struct intel_ring_buffer *ring, bool do_retire)
+static int i915_ring_idle(struct intel_ring_buffer *ring)
 {
 	int ret;
 
@@ -2131,18 +2122,17 @@ static int i915_ring_idle(struct intel_ring_buffer *ring, bool do_retire)
 			return ret;
 	}
 
-	return i915_wait_request(ring, i915_gem_next_request_seqno(ring),
-				 do_retire);
+	return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
 }
 
-int i915_gpu_idle(struct drm_device *dev, bool do_retire)
+int i915_gpu_idle(struct drm_device *dev)
 {
 	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], do_retire);
+		ret = i915_ring_idle(&dev_priv->ring[i]);
 		if (ret)
 			return ret;
 	}
@@ -2331,9 +2321,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj)
 	}
 
 	if (obj->last_fenced_seqno) {
-		ret = i915_wait_request(obj->ring,
-					obj->last_fenced_seqno,
-					false);
+		ret = i915_wait_request(obj->ring, obj->last_fenced_seqno);
 		if (ret)
 			return ret;
 
@@ -3394,11 +3382,12 @@ i915_gem_idle(struct drm_device *dev)
 		return 0;
 	}
 
-	ret = i915_gpu_idle(dev, true);
+	ret = i915_gpu_idle(dev);
 	if (ret) {
 		mutex_unlock(&dev->struct_mutex);
 		return ret;
 	}
+	i915_gem_retire_requests(dev);
 
 	/* Under UMS, be paranoid and evict. */
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
@@ -4029,7 +4018,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, true) == 0)
+		if (i915_gpu_idle(dev) == 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 91ebb94..3bcf045 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -168,7 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj, *next;
 	bool lists_empty;
-	int ret;
+	int ret,i;
 
 	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
 		       list_empty(&dev_priv->mm.flushing_list) &&
@@ -178,11 +178,20 @@ 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, true);
+	ret = i915_gpu_idle(dev);
 	if (ret)
 		return ret;
 
+	/* The gpu_idle will flush everything in the write domain to the
+	 * active list. Then we must move everything off the active list
+	 * with retire requests.
+	 */
+	for (i = 0; i < I915_NUM_RINGS; i++)
+		if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
+			return -EBUSY;
+
+	i915_gem_retire_requests(dev);
+
 	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
 
 	/* Having flushed everything, unbind() should never raise an error */
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 68ec013..cbba0aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1220,9 +1220,10 @@ 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, true);
+			ret = i915_gpu_idle(dev);
 			if (ret)
 				goto err;
+			i915_gem_retire_requests(dev);
 
 			BUG_ON(ring->sync_seqno[i]);
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 25c8bf9..29d573c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -317,7 +317,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)) {
 			DRM_ERROR("Couldn't idle GPU\n");
 			/* Wait a bit, in hopes it avoids the hang */
 			udelay(10);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index a0b5053..e06e46a 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -225,10 +225,10 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	}
 	overlay->last_flip_req = request->seqno;
 	overlay->flip_tail = tail;
-	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req,
-				true);
+	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
+	i915_gem_retire_requests(dev);
 
 	overlay->last_flip_req = 0;
 	return 0;
@@ -447,10 +447,10 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
 	if (overlay->last_flip_req == 0)
 		return 0;
 
-	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req,
-				true);
+	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
+	i915_gem_retire_requests(dev);
 
 	if (overlay->flip_tail)
 		overlay->flip_tail(overlay);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 427b7c5..a7d97d1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1088,9 +1088,11 @@ static int intel_ring_wait_seqno(struct intel_ring_buffer *ring, u32 seqno)
 	was_interruptible = dev_priv->mm.interruptible;
 	dev_priv->mm.interruptible = false;
 
-	ret = i915_wait_request(ring, seqno, true);
+	ret = i915_wait_request(ring, seqno);
 
 	dev_priv->mm.interruptible = was_interruptible;
+	if (!ret)
+		i915_gem_retire_requests_ring(ring);
 
 	return ret;
 }
-- 
1.7.10

[PATCH 02/12] drm/i915: remove some extra retiring

[Ben Widawsky]

Not every caller of gpu_idle needs to retire requests. Try to pick only
the callers that need it. This was originally combined with the previous
patch in the first series on the mailing list.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c            |    1 -
 drivers/gpu/drm/i915/i915_gem.c            |    1 -
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    1 -
 drivers/gpu/drm/i915/intel_overlay.c       |    2 --
 4 files changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 36c8d5f..e73389d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2016,7 +2016,6 @@ int i915_driver_unload(struct drm_device *dev)
 	ret = i915_gpu_idle(dev);
 	if (ret)
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
-	i915_gem_retire_requests(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	/* Cancel the retire work handler, which should be idle now. */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 25be0e0..3b731ef 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3387,7 +3387,6 @@ i915_gem_idle(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 		return ret;
 	}
-	i915_gem_retire_requests(dev);
 
 	/* Under UMS, be paranoid and evict. */
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index cbba0aa..582f6c4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1223,7 +1223,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			ret = i915_gpu_idle(dev);
 			if (ret)
 				goto err;
-			i915_gem_retire_requests(dev);
 
 			BUG_ON(ring->sync_seqno[i]);
 		}
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e06e46a..07a5cad 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -228,7 +228,6 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
-	i915_gem_retire_requests(dev);
 
 	overlay->last_flip_req = 0;
 	return 0;
@@ -450,7 +449,6 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
 	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
-	i915_gem_retire_requests(dev);
 
 	if (overlay->flip_tail)
 		overlay->flip_tail(overlay);
-- 
1.7.10

[PATCH 03/12 v2] drm/i915: move vbetool invoked ier stuff

[Ben Widawsky]

This extra bit of interrupt enabling code doesn't belong in the wait
seqno function. If anything we should pull it out to a helper so the
throttle code can also use it. The history is a bit vague, but I am
going to attempt to just dump it, unless someone can argue otherwise.

Removing this allows for a shared lock free wait seqno function. To keep
tabs on this issue though, the IER value is stored on error capture
(recommended by Chris Wilson)

v2: fixed typo EIR->IER (Ben)
Fix some white space (Ben)
Move IER capture to globally instead of per ring (Ben)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |    1 +
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_gem.c     |   14 --------------
 drivers/gpu/drm/i915/i915_irq.c     |    8 ++++++++
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 120db46..8ed0342 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -712,6 +712,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
 		   error->time.tv_usec);
 	seq_printf(m, "PCI ID: 0x%04x\n", dev->pci_device);
 	seq_printf(m, "EIR: 0x%08x\n", error->eir);
+	seq_printf(m, "IER: 0x%08x\n", error->ier);
 	seq_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
 
 	for (i = 0; i < dev_priv->num_fence_regs; i++)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 34d7041..d0eeb28 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -173,6 +173,7 @@ struct intel_display_error_state;
 struct drm_i915_error_state {
 	u32 eir;
 	u32 pgtbl_er;
+	u32 ier;
 	u32 pipestat[I915_MAX_PIPES];
 	u32 tail[I915_NUM_RINGS];
 	u32 head[I915_NUM_RINGS];
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3b731ef..5938b9b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1828,7 +1828,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		  uint32_t seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
-	u32 ier;
 	int ret = 0;
 
 	BUG_ON(seqno == 0);
@@ -1863,19 +1862,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
 	}
 
 	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
-		if (HAS_PCH_SPLIT(ring->dev))
-			ier = I915_READ(DEIER) | I915_READ(GTIER);
-		else if (IS_VALLEYVIEW(ring->dev))
-			ier = I915_READ(GTIER) | I915_READ(VLV_IER);
-		else
-			ier = I915_READ(IER);
-		if (!ier) {
-			DRM_ERROR("something (likely vbetool) disabled "
-				  "interrupts, re-enabling\n");
-			ring->dev->driver->irq_preinstall(ring->dev);
-			ring->dev->driver->irq_postinstall(ring->dev);
-		}
-
 		trace_i915_gem_request_wait_begin(ring, seqno);
 
 		ring->waiting_seqno = seqno;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0211263..5e9d05d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1155,6 +1155,14 @@ static void i915_capture_error_state(struct drm_device *dev)
 
 	error->eir = I915_READ(EIR);
 	error->pgtbl_er = I915_READ(PGTBL_ER);
+
+	if (HAS_PCH_SPLIT(dev))
+		error->ier = I915_READ(DEIER) | I915_READ(GTIER);
+	else if (IS_VALLEYVIEW(dev))
+		error->ier = I915_READ(GTIER) | I915_READ(VLV_IER);
+	else
+		error->ier = I915_READ(IER);
+
 	for_each_pipe(pipe)
 		error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
 
-- 
1.7.10

[PATCH 04/12 v2] drm/i915: kill waiting_seqno

[Ben Widawsky]

The waiting_seqno is not terribly useful, and as such we can remove it
so that we'll be able to extract lockless code.

v2: Keep the information for error_state (Chris)
Check if ring is initialized in hangcheck (Chris)
Capture the waiting ring (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |    3 +--
 drivers/gpu/drm/i915/i915_drv.h         |    1 +
 drivers/gpu/drm/i915/i915_gem.c         |    2 --
 drivers/gpu/drm/i915/i915_irq.c         |   13 ++++++++-----
 drivers/gpu/drm/i915/intel_ringbuffer.h |    1 -
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8ed0342..9d4bf6b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -414,8 +414,6 @@ static void i915_ring_seqno_info(struct seq_file *m,
 	if (ring->get_seqno) {
 		seq_printf(m, "Current sequence (%s): %d\n",
 			   ring->name, ring->get_seqno(ring));
-		seq_printf(m, "Waiter sequence (%s):  %d\n",
-			   ring->name, ring->waiting_seqno);
 		seq_printf(m, "IRQ sequence (%s):     %d\n",
 			   ring->name, ring->irq_seqno);
 	}
@@ -687,6 +685,7 @@ static void i915_ring_error_state(struct seq_file *m,
 			   error->semaphore_mboxes[ring][1]);
 	}
 	seq_printf(m, "  seqno: 0x%08x\n", error->seqno[ring]);
+	seq_printf(m, "  waiting: %s\n", yesno(error->waiting[ring]));
 	seq_printf(m, "  ring->head: 0x%08x\n", error->cpu_ring_head[ring]);
 	seq_printf(m, "  ring->tail: 0x%08x\n", error->cpu_ring_tail[ring]);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0eeb28..d260762 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -174,6 +174,7 @@ struct drm_i915_error_state {
 	u32 eir;
 	u32 pgtbl_er;
 	u32 ier;
+	bool waiting[I915_NUM_RINGS];
 	u32 pipestat[I915_MAX_PIPES];
 	u32 tail[I915_NUM_RINGS];
 	u32 head[I915_NUM_RINGS];
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5938b9b..c29c758 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1864,7 +1864,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
 	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
 		trace_i915_gem_request_wait_begin(ring, seqno);
 
-		ring->waiting_seqno = seqno;
 		if (ring->irq_get(ring)) {
 			if (dev_priv->mm.interruptible)
 				ret = wait_event_interruptible(ring->irq_queue,
@@ -1880,7 +1879,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
 							     seqno) ||
 					   atomic_read(&dev_priv->mm.wedged), 3000))
 			ret = -EBUSY;
-		ring->waiting_seqno = 0;
 
 		trace_i915_gem_request_wait_end(ring, seqno);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5e9d05d..98494ba 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1064,6 +1064,7 @@ static void i915_record_ring_state(struct drm_device *dev,
 		error->instdone[ring->id] = I915_READ(INSTDONE);
 	}
 
+	error->waiting[ring->id] = waitqueue_active(&ring->irq_queue);
 	error->instpm[ring->id] = I915_READ(RING_INSTPM(ring->mmio_base));
 	error->seqno[ring->id] = ring->get_seqno(ring);
 	error->acthd[ring->id] = intel_ring_get_active_head(ring);
@@ -1877,14 +1878,16 @@ ring_last_seqno(struct intel_ring_buffer *ring)
 
 static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
 {
+	/* Hanghceck may trigger before ring is initialized */
+	if (ring->obj == NULL)
+		return true;
+
 	if (list_empty(&ring->request_list) ||
 	    i915_seqno_passed(ring->get_seqno(ring), ring_last_seqno(ring))) {
 		/* Issue a wake-up to catch stuck h/w. */
-		if (ring->waiting_seqno && waitqueue_active(&ring->irq_queue)) {
-			DRM_ERROR("Hangcheck timer elapsed... %s idle [waiting on %d, at %d], missed IRQ?\n",
-				  ring->name,
-				  ring->waiting_seqno,
-				  ring->get_seqno(ring));
+		if (waitqueue_active(&ring->irq_queue)) {
+			DRM_ERROR("Hangcheck timer elapsed... %s idle\n",
+				  ring->name);
 			wake_up_all(&ring->irq_queue);
 			*err = true;
 		}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index e0b25bb..d3bf259 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -60,7 +60,6 @@ struct  intel_ring_buffer {
 	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
 	u32		irq_seqno;		/* last seq seem at irq time */
 	u32		trace_irq_seqno;
-	u32		waiting_seqno;
 	u32		sync_seqno[I915_NUM_RINGS-1];
 	bool __must_check (*irq_get)(struct intel_ring_buffer *ring);
 	void		(*irq_put)(struct intel_ring_buffer *ring);
-- 
1.7.10

[PATCH 05/12 v2] drm/i915: drop polled waits from i915_wait_request

[Ben Widawsky]

The only time irq_get should fail is during unload or suspend. Both of
these points should try to quiesce the GPU before disabling interrupts
and so the atomic polling should never occur.

This was recommended by Chris Wilson as a way of reducing added
complexity to the polled wait which I introduced in an RFC patch.

09:57 < ickle_> it's only there as a fudge for waiting after irqs
after uninstalled during s&r, we aren't actually meant to hit it
09:57 < ickle_> so maybe we should just kill the code there and fix the breakage

v2: return -ENODEV instead of -EBUSY when irq_get fails

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c29c758..c5f0b03 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1864,22 +1864,19 @@ i915_wait_request(struct intel_ring_buffer *ring,
 	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
 		trace_i915_gem_request_wait_begin(ring, seqno);
 
-		if (ring->irq_get(ring)) {
-			if (dev_priv->mm.interruptible)
-				ret = wait_event_interruptible(ring->irq_queue,
-							       i915_seqno_passed(ring->get_seqno(ring), seqno)
-							       || atomic_read(&dev_priv->mm.wedged));
-			else
-				wait_event(ring->irq_queue,
-					   i915_seqno_passed(ring->get_seqno(ring), seqno)
-					   || atomic_read(&dev_priv->mm.wedged));
+		if (WARN_ON(!ring->irq_get(ring)))
+			return -ENODEV;
 
-			ring->irq_put(ring);
-		} else if (wait_for_atomic(i915_seqno_passed(ring->get_seqno(ring),
-							     seqno) ||
-					   atomic_read(&dev_priv->mm.wedged), 3000))
-			ret = -EBUSY;
+		if (dev_priv->mm.interruptible)
+			ret = wait_event_interruptible(ring->irq_queue,
+						       i915_seqno_passed(ring->get_seqno(ring), seqno)
+						       || atomic_read(&dev_priv->mm.wedged));
+		else
+			wait_event(ring->irq_queue,
+				   i915_seqno_passed(ring->get_seqno(ring), seqno)
+				   || atomic_read(&dev_priv->mm.wedged));
 
+		ring->irq_put(ring);
 		trace_i915_gem_request_wait_end(ring, seqno);
 	}
 	if (atomic_read(&dev_priv->mm.wedged))
-- 
1.7.10

[PATCH 06/12 v2] drm/i915: extract __wait_seqno from i915_wait_request

[Ben Widawsky]

i915_wait_request is actually a fairly large function encapsulating
quite a few different operations. Because being able to wait on seqnos
in various conditions is useful, extracting that bit of code to a helper
function seems useful

v2: pull the irq_get/put as well (Ben)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |   49 +++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c5f0b03..06668eb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1819,6 +1819,36 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
+			bool interruptible)
+{
+	drm_i915_private_t *dev_priv = ring->dev->dev_private;
+	int ret = 0;
+
+	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
+		return 0;
+
+	trace_i915_gem_request_wait_begin(ring, seqno);
+	if (WARN_ON(!ring->irq_get(ring)))
+		return -ENODEV;
+
+#define EXIT_COND \
+	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
+	atomic_read(&dev_priv->mm.wedged))
+
+	if (interruptible)
+		ret = wait_event_interruptible(ring->irq_queue,
+					       EXIT_COND);
+	else
+		wait_event(ring->irq_queue, EXIT_COND);
+
+	ring->irq_put(ring);
+	trace_i915_gem_request_wait_end(ring, seqno);
+#undef EXIT_COND
+
+	return ret;
+}
+
 /**
  * Waits for a sequence number to be signaled, and cleans up the
  * request and object lists appropriately for that event.
@@ -1861,24 +1891,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		seqno = request->seqno;
 	}
 
-	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
-		trace_i915_gem_request_wait_begin(ring, seqno);
-
-		if (WARN_ON(!ring->irq_get(ring)))
-			return -ENODEV;
-
-		if (dev_priv->mm.interruptible)
-			ret = wait_event_interruptible(ring->irq_queue,
-						       i915_seqno_passed(ring->get_seqno(ring), seqno)
-						       || atomic_read(&dev_priv->mm.wedged));
-		else
-			wait_event(ring->irq_queue,
-				   i915_seqno_passed(ring->get_seqno(ring), seqno)
-				   || atomic_read(&dev_priv->mm.wedged));
-
-		ring->irq_put(ring);
-		trace_i915_gem_request_wait_end(ring, seqno);
-	}
+	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
 	if (atomic_read(&dev_priv->mm.wedged))
 		ret = -EAGAIN;
 
-- 
1.7.10

[PATCH 07/12] drm/i915: remove polled wait from throttle

[Ben Widawsky]

It's about to go away anyway. Just here to help bisection.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 06668eb..4de0515 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2996,11 +2996,8 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 
 			if (ret == 0 && atomic_read(&dev_priv->mm.wedged))
 				ret = -EIO;
-		} else if (wait_for_atomic(i915_seqno_passed(ring->get_seqno(ring),
-							     seqno) ||
-				    atomic_read(&dev_priv->mm.wedged), 3000)) {
+		} else
 			ret = -EBUSY;
-		}
 	}
 
 	if (ret == 0)
-- 
1.7.10

[PATCH 08/12 v2] drm/i915: use __wait_seqno for ring throttle

[Ben Widawsky]

It turns out throttle had an almost identical  bit of code to do the
wait. Now we can call the new helper directly.  This is just a bonus,
and not needed for the overall series.

v2: remove irq_get/put which is now in __wait_seqno (Ben)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |   20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4de0515..0ae1a73 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2981,25 +2981,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	if (seqno == 0)
 		return 0;
 
-	ret = 0;
-	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
-		/* And wait for the seqno passing without holding any locks and
-		 * causing extra latency for others. This is safe as the irq
-		 * generation is designed to be run atomically and so is
-		 * lockless.
-		 */
-		if (ring->irq_get(ring)) {
-			ret = wait_event_interruptible(ring->irq_queue,
-						       i915_seqno_passed(ring->get_seqno(ring), seqno)
-						       || atomic_read(&dev_priv->mm.wedged));
-			ring->irq_put(ring);
-
-			if (ret == 0 && atomic_read(&dev_priv->mm.wedged))
-				ret = -EIO;
-		} else
-			ret = -EBUSY;
-	}
-
+	ret = __wait_seqno(ring, seqno, true);
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
-- 
1.7.10

[PATCH 09/12] drm/i915: timeout parameter for seqno wait

[Ben Widawsky]

Insert a wait parameter in the code so we can possibly timeout on a
seqno wait if need be. The code should be functionally the same as
before because all the callers will continue to retry if an arbitrary
timeout elapses.

We'd like to have nanosecond granularity, but the only way to do this is
with hrtimer, and that doesn't fit well with the needs of this code.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |   58 +++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0ae1a73..f054439 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1820,10 +1820,23 @@ i915_gem_retire_work_handler(struct work_struct *work)
 }
 
 static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
-			bool interruptible)
+			bool interruptible, long *usecs)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
-	int ret = 0;
+	bool wait_forever = false;
+	long timeout, end;
+
+	if (usecs == NULL || ((*usecs) < 0)) {
+		wait_forever = true;
+		timeout = msecs_to_jiffies(100);
+	} else
+		timeout = usecs_to_jiffies(*usecs);
+
+	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
+		return 0;
+
+	if (WARN_ON(!ring->irq_get(ring)))
+		return -ENODEV;
 
 	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
 		return 0;
@@ -1836,17 +1849,40 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
 	atomic_read(&dev_priv->mm.wedged))
 
+	trace_i915_gem_request_wait_begin(ring, seqno);
+again:
 	if (interruptible)
-		ret = wait_event_interruptible(ring->irq_queue,
-					       EXIT_COND);
+		end = wait_event_interruptible_timeout(ring->irq_queue,
+						       EXIT_COND,
+						       timeout);
 	else
-		wait_event(ring->irq_queue, EXIT_COND);
+		end = wait_event_timeout(ring->irq_queue, EXIT_COND, timeout);
+#undef EXIT_COND
+
+	if (atomic_read(&dev_priv->mm.wedged))
+		end = -EAGAIN;
+
+	if (end == 0 && wait_forever)
+		goto again;
 
-	ring->irq_put(ring);
 	trace_i915_gem_request_wait_end(ring, seqno);
-#undef EXIT_COND
+	ring->irq_put(ring);
 
-	return ret;
+	if (!wait_forever) {
+		BUG_ON(end == 0);
+		*usecs = jiffies_to_usecs(timeout - end);
+	}
+
+	switch (end) {
+	case -EAGAIN: /* Wedged */
+	case -ERESTARTSYS: /* Signal */
+		return (int)end;
+	case 0: /* Tiemout */
+		return -ETIME;
+	default: /* Completed */
+		WARN_ON(end < 0); /* We're not aware of other errors */
+		return 0;
+	}
 }
 
 /**
@@ -1891,9 +1927,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		seqno = request->seqno;
 	}
 
-	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
-	if (atomic_read(&dev_priv->mm.wedged))
-		ret = -EAGAIN;
+	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible, NULL);
 
 	return ret;
 }
@@ -2981,7 +3015,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	if (seqno == 0)
 		return 0;
 
-	ret = __wait_seqno(ring, seqno, true);
+	ret = __wait_seqno(ring, seqno, true, NULL);
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
-- 
1.7.10

[PATCH 10/12] drm/i915: extract some common olr+wedge code

[Ben Widawsky]

Refactor.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |  122 +++++++++++++++++++++------------------
 1 file changed, 65 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f054439..63eda79 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1819,6 +1819,59 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+
+static int
+i915_gem_check_wedge(struct drm_i915_private *dev_priv)
+{
+	BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	if (atomic_read(&dev_priv->mm.wedged)) {
+		struct completion *x = &dev_priv->error_completion;
+		bool recovery_complete;
+		unsigned long flags;
+
+		/* Give the error handler a chance to run. */
+		spin_lock_irqsave(&x->wait.lock, flags);
+		recovery_complete = x->done > 0;
+		spin_unlock_irqrestore(&x->wait.lock, flags);
+
+		return recovery_complete ? -EIO : -EAGAIN;
+	}
+
+	return 0;
+}
+
+/*
+ * Compare seqno against outstanding lazy request. Emit a request if they are
+ * equal. Seqno is updated with the new value if a request was emitted.
+ */
+static int
+i915_gem_check_olr(struct intel_ring_buffer *ring, u32 *seqno)
+{
+	int ret = 0;
+
+	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+
+	if (*seqno == ring->outstanding_lazy_request) {
+		struct drm_i915_gem_request *request;
+
+		request = kzalloc(sizeof(*request), GFP_KERNEL);
+		if (request == NULL)
+			return -ENOMEM;
+
+		ret = i915_add_request(ring, NULL, request);
+		if (ret) {
+			kfree(request);
+			return ret;
+		}
+
+		*seqno = request->seqno;
+	}
+
+	return ret;
+}
+
+
 static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 			bool interruptible, long *usecs)
 {
@@ -1898,34 +1951,13 @@ i915_wait_request(struct intel_ring_buffer *ring,
 
 	BUG_ON(seqno == 0);
 
-	if (atomic_read(&dev_priv->mm.wedged)) {
-		struct completion *x = &dev_priv->error_completion;
-		bool recovery_complete;
-		unsigned long flags;
-
-		/* Give the error handler a chance to run. */
-		spin_lock_irqsave(&x->wait.lock, flags);
-		recovery_complete = x->done > 0;
-		spin_unlock_irqrestore(&x->wait.lock, flags);
-
-		return recovery_complete ? -EIO : -EAGAIN;
-	}
-
-	if (seqno == ring->outstanding_lazy_request) {
-		struct drm_i915_gem_request *request;
-
-		request = kzalloc(sizeof(*request), GFP_KERNEL);
-		if (request == NULL)
-			return -ENOMEM;
-
-		ret = i915_add_request(ring, NULL, request);
-		if (ret) {
-			kfree(request);
-			return ret;
-		}
+	ret = i915_gem_check_wedge(dev_priv);
+	if (ret)
+		return ret;
 
-		seqno = request->seqno;
-	}
+	ret = i915_gem_check_olr(ring, &seqno);
+	if (ret)
+		return ret;
 
 	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible, NULL);
 
@@ -1991,22 +2023,9 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	if (seqno <= from->sync_seqno[idx])
 		return 0;
 
-	if (seqno == from->outstanding_lazy_request) {
-		struct drm_i915_gem_request *request;
-
-		request = kzalloc(sizeof(*request), GFP_KERNEL);
-		if (request == NULL)
-			return -ENOMEM;
-
-		ret = i915_add_request(from, NULL, request);
-		if (ret) {
-			kfree(request);
-			return ret;
-		}
-
-		seqno = request->seqno;
-	}
-
+	ret = i915_gem_check_olr(obj->ring, &seqno);
+	if (ret)
+		return ret;
 
 	ret = to->sync_to(to, from, seqno);
 	if (!ret)
@@ -3194,20 +3213,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 		if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
 			ret = i915_gem_flush_ring(obj->ring,
 						  0, obj->base.write_domain);
-		} else if (obj->ring->outstanding_lazy_request ==
-			   obj->last_rendering_seqno) {
-			struct drm_i915_gem_request *request;
-
-			/* This ring is not being cleared by active usage,
-			 * so emit a request to do so.
-			 */
-			request = kzalloc(sizeof(*request), GFP_KERNEL);
-			if (request) {
-				ret = i915_add_request(obj->ring, NULL, request);
-				if (ret)
-					kfree(request);
-			} else
-				ret = -ENOMEM;
+		} else {
+			u32 seqno = obj->last_rendering_seqno;
+			ret = i915_gem_check_olr(obj->ring, &seqno);
 		}
 
 		/* Update the active list for the hardware's current position.
-- 
1.7.10

[PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Ben Widawsky]

This helps implement glClientWaitSync.

Finally we can use the new timed seqno waiting function to allow
userspace to wait on a request with a timeout. This implements that
interface.

The new ioctl is very straight forward, there is a flags field which I
envision may be useful for various flush permutations of the command.

On 32bit systems, the timeout is restricted to 2^32 - 1 microseconds.

v2: ETIME/ERESTARTSYS instead of changing to EBUSY, and EGAIN (Chris)
Flush the object from the gpu write domain (Chris + Daniel)
Fix leaked refcount in good case (Chris)
Naturally align ioctl struct (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |    1 +
 drivers/gpu/drm/i915/i915_drv.h |    2 ++
 drivers/gpu/drm/i915/i915_gem.c |   55 +++++++++++++++++++++++++++++++++++++++
 include/drm/i915_drm.h          |    8 ++++++
 4 files changed, 66 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e73389d..ed0ccb7 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2189,6 +2189,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_UNLOCKED),
 };
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d260762..d415f0f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1221,6 +1221,8 @@ int i915_gem_get_tiling(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
+int i915_gem_wait_ioctl(struct drm_device *dev, void *data,
+			struct drm_file *file_priv);
 void i915_gem_load(struct drm_device *dev);
 int i915_gem_init_object(struct drm_gem_object *obj);
 int __must_check i915_gem_flush_ring(struct intel_ring_buffer *ring,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 63eda79..0dee6bc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1991,6 +1991,61 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+int
+i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
+{
+	struct drm_i915_gem_wait *args = data;
+	struct drm_i915_gem_object *obj;
+	struct intel_ring_buffer *ring = NULL;
+	long timeout;
+	u32 seqno = 0;
+	int ret = 0;
+
+	/* For now timeout is microseconds only */
+	timeout = (long)DIV_ROUND_UP_ULL(args->timeout_ns, NSEC_PER_USEC);
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->bo_handle));
+	if (&obj->base == NULL) {
+		mutex_unlock(&dev->struct_mutex);
+		return -ENOENT;
+	}
+
+	/* Need to make sure the object is flushed first. This non-obvious
+	 * flush is required to enforce that (active && !olr) == no wait
+	 * necessary.
+	 */
+	ret = i915_gem_object_flush_gpu_write_domain(obj);
+	if (ret)
+		goto out;
+
+	if (obj->active) {
+		seqno = obj->last_rendering_seqno;
+		ring = obj->ring;
+	}
+
+	if (seqno == 0)
+		 goto out;
+
+	ret = i915_gem_check_olr(ring, &seqno);
+	if (ret)
+		goto out;
+
+	mutex_unlock(&dev->struct_mutex);
+	ret = __wait_seqno(ring, seqno, true, &timeout);
+	drm_gem_object_unreference_unlocked(&obj->base);
+	args->timeout_ns = timeout * NSEC_PER_USEC;
+	return ret;
+
+out:
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
+}
+
 /**
  * i915_gem_object_sync - sync an object to a ring.
  *
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index f3f8224..8f818f8 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -200,6 +200,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_EXECBUFFER2	0x29
 #define DRM_I915_GET_SPRITE_COLORKEY	0x2a
 #define DRM_I915_SET_SPRITE_COLORKEY	0x2b
+#define DRM_I915_GEM_WAIT	0x2c
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -243,6 +244,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
 #define DRM_IOCTL_I915_SET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
 #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
+#define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -886,4 +888,10 @@ struct drm_intel_sprite_colorkey {
 	__u32 flags;
 };
 
+struct drm_i915_gem_wait {
+	__u32 bo_handle;
+	__u32 flags;
+	__u64 timeout_ns;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.10

[PATCH 12/12] drm/i915: s/i915_wait_reqest/i915_wait_seqno/g

[Ben Widawsky]

Wait request is poorly named IMO. After working with these functions for
some time, I feel it's much clearer to name the functions more
appropriately.

Of course we must update the callers to use the new name as well.

This leaves room within our namespace for a *real* wait request function
at some point.

Note to maintainer: this patch is optional.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |    4 ++--
 drivers/gpu/drm/i915/i915_gem.c         |    9 ++++-----
 drivers/gpu/drm/i915/intel_overlay.c    |    4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d415f0f..f961cc8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1307,8 +1307,8 @@ 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_wait_seqno(struct intel_ring_buffer *ring,
+				 uint32_t seqno);
 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 0dee6bc..3ae21d4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1943,8 +1943,7 @@ again:
  * request and object lists appropriately for that event.
  */
 int
-i915_wait_request(struct intel_ring_buffer *ring,
-		  uint32_t seqno)
+i915_wait_seqno(struct intel_ring_buffer *ring, uint32_t seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	int ret = 0;
@@ -1982,7 +1981,7 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
 	 * it.
 	 */
 	if (obj->active) {
-		ret = i915_wait_request(obj->ring, obj->last_rendering_seqno);
+		ret = i915_wait_seqno(obj->ring, obj->last_rendering_seqno);
 		if (ret)
 			return ret;
 		i915_gem_retire_requests_ring(obj->ring);
@@ -2224,7 +2223,7 @@ 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_wait_seqno(ring, i915_gem_next_request_seqno(ring));
 }
 
 int i915_gpu_idle(struct drm_device *dev)
@@ -2423,7 +2422,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj)
 	}
 
 	if (obj->last_fenced_seqno) {
-		ret = i915_wait_request(obj->ring, obj->last_fenced_seqno);
+		ret = i915_wait_seqno(obj->ring, obj->last_fenced_seqno);
 		if (ret)
 			return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 07a5cad..7a05a34 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -225,7 +225,7 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	}
 	overlay->last_flip_req = request->seqno;
 	overlay->flip_tail = tail;
-	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
+	ret = i915_wait_seqno(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
 
@@ -446,7 +446,7 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
 	if (overlay->last_flip_req == 0)
 		return 0;
 
-	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
+	ret = i915_wait_seqno(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a7d97d1..9bc9e12 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1088,7 +1088,7 @@ static int intel_ring_wait_seqno(struct intel_ring_buffer *ring, u32 seqno)
 	was_interruptible = dev_priv->mm.interruptible;
 	dev_priv->mm.interruptible = false;
 
-	ret = i915_wait_request(ring, seqno);
+	ret = i915_wait_seqno(ring, seqno);
 
 	dev_priv->mm.interruptible = was_interruptible;
 	if (!ret)
-- 
1.7.10

Re: [PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Thu, 26 Apr 2012 16:03:08 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> +	mutex_unlock(&dev->struct_mutex);
> +	ret = __wait_seqno(ring, seqno, true, &timeout);
> +	drm_gem_object_unreference_unlocked(&obj->base);

Once we have the seqno to wait on, we can drop the reference to the
object. The reference to the ring will be persist whilst the device is
open. Just saves doing an unsightly unlock/lock dance in the unref.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 10/12] drm/i915: extract some common olr+wedge code

[Chris Wilson]

On Thu, 26 Apr 2012 16:03:07 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> +/*
> + * Compare seqno against outstanding lazy request. Emit a request if they are
> + * equal. Seqno is updated with the new value if a request was emitted.
> + */
> +static int
> +i915_gem_check_olr(struct intel_ring_buffer *ring, u32 *seqno)
> +{
> +	int ret = 0;
> +
> +	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> +
> +	if (*seqno == ring->outstanding_lazy_request) {
> +		struct drm_i915_gem_request *request;
> +
> +		request = kzalloc(sizeof(*request), GFP_KERNEL);
> +		if (request == NULL)
> +			return -ENOMEM;
> +
> +		ret = i915_add_request(ring, NULL, request);
> +		if (ret) {
> +			kfree(request);
> +			return ret;
> +		}
> +
> +		*seqno = request->seqno;
I'd love for this to be BUG_ON(seqno != request->seqno) and so drop the
out parameter, as it would tidy all the callers up.
> +	}
> +
> +	return ret;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/12] drm/i915: timeout parameter for seqno wait

[Chris Wilson]

On Thu, 26 Apr 2012 16:03:06 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
>  static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
> -			bool interruptible)
> +			bool interruptible, long *usecs)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	int ret = 0;
> +	bool wait_forever = false;
> +	long timeout, end;
> +
> +	if (usecs == NULL || ((*usecs) < 0)) {
> +		wait_forever = true;
> +		timeout = msecs_to_jiffies(100);
> +	} else
> +		timeout = usecs_to_jiffies(*usecs);
> +
> +	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
> +		return 0;
> +
> +	if (WARN_ON(!ring->irq_get(ring)))
> +		return -ENODEV;

Rebase error, duplicated ring->irq_get/ring->get_seqno.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 02/12] drm/i915: remove some extra retiring

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:02:59PM -0700, Ben Widawsky wrote:
> Not every caller of gpu_idle needs to retire requests. Try to pick only
> the callers that need it. This was originally combined with the previous
> patch in the first series on the mailing list.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c            |    1 -
>  drivers/gpu/drm/i915/i915_gem.c            |    1 -
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    1 -
>  drivers/gpu/drm/i915/intel_overlay.c       |    2 --
>  4 files changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 36c8d5f..e73389d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2016,7 +2016,6 @@ int i915_driver_unload(struct drm_device *dev)
>  	ret = i915_gpu_idle(dev);
>  	if (ret)
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> -	i915_gem_retire_requests(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	/* Cancel the retire work handler, which should be idle now. */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 25be0e0..3b731ef 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3387,7 +3387,6 @@ i915_gem_idle(struct drm_device *dev)
>  		mutex_unlock(&dev->struct_mutex);
>  		return ret;
>  	}
> -	i915_gem_retire_requests(dev);

Imo this will still blow up resume on gen2/3 ... we may not leak any
request over a s/r cycle in general, because on resume we restart with the
seqno counting at 1. So when you then want to wait for a no longer active,
but not yet retired request after resume, that will take a while.

I also miss a bit of justification for the first hunk, but module unload
calling gpu_idle instead of gem_idle is a bit fishy in and off itself.

I think the parts below are ok, but I'm hunting an overlay regression in
dinq atm, so I'll hold of merging this.
-Daniel

>  	/* Under UMS, be paranoid and evict. */
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index cbba0aa..582f6c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1223,7 +1223,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			ret = i915_gpu_idle(dev);
>  			if (ret)
>  				goto err;
> -			i915_gem_retire_requests(dev);
>  
>  			BUG_ON(ring->sync_seqno[i]);
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index e06e46a..07a5cad 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -228,7 +228,6 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
>  	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
>  	if (ret)
>  		return ret;
> -	i915_gem_retire_requests(dev);
>  
>  	overlay->last_flip_req = 0;
>  	return 0;
> @@ -450,7 +449,6 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
>  	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
>  	if (ret)
>  		return ret;
> -	i915_gem_retire_requests(dev);
>  
>  	if (overlay->flip_tail)
>  		overlay->flip_tail(overlay);
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 01/12 v2] drm/i915: remove do_retire from i915_wait_request

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:02:58PM -0700, Ben Widawsky wrote:
> This originates from a hack by me to quickly fix a bug in an earlier
> patch where we needed control over whether or not waiting on a seqno
> actually did any retire list processing. Since the two operations aren't
> clearly related, we should pull the parameter out of the wait function,
> and make the caller responsible for retiring if the action is desired.
> 
> The only function call site which did not get an explicit retire_request call
> (on purpose) is i915_gem_inactive_shrink(). That code was already calling
> retire_request a second time.
> 
> v2: don't modify any behavior excepit i915_gem_inactive_shrink(Daniel)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 03/12 v2] drm/i915: move vbetool invoked ier stuff

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:03:00PM -0700, Ben Widawsky wrote:
> This extra bit of interrupt enabling code doesn't belong in the wait
> seqno function. If anything we should pull it out to a helper so the
> throttle code can also use it. The history is a bit vague, but I am
> going to attempt to just dump it, unless someone can argue otherwise.
> 
> Removing this allows for a shared lock free wait seqno function. To keep
> tabs on this issue though, the IER value is stored on error capture
> (recommended by Chris Wilson)
> 
> v2: fixed typo EIR->IER (Ben)
> Fix some white space (Ben)
> Move IER capture to globally instead of per ring (Ben)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next with the small addition that ier is a 16bit reg on gen2,
thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 04/12 v2] drm/i915: kill waiting_seqno

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:03:01PM -0700, Ben Widawsky wrote:
> The waiting_seqno is not terribly useful, and as such we can remove it
> so that we'll be able to extract lockless code.
> 
> v2: Keep the information for error_state (Chris)
> Check if ring is initialized in hangcheck (Chris)
> Capture the waiting ring (Chris)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Merged with the comment for the ring->obj == NULL check in hangcheck_idle
clarified, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 05/12 v2] drm/i915: drop polled waits from i915_wait_request

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:03:02PM -0700, Ben Widawsky wrote:
> The only time irq_get should fail is during unload or suspend. Both of
> these points should try to quiesce the GPU before disabling interrupts
> and so the atomic polling should never occur.
> 
> This was recommended by Chris Wilson as a way of reducing added
> complexity to the polled wait which I introduced in an RFC patch.
> 
> 09:57 < ickle_> it's only there as a fudge for waiting after irqs
> after uninstalled during s&r, we aren't actually meant to hit it
> 09:57 < ickle_> so maybe we should just kill the code there and fix the breakage
> 
> v2: return -ENODEV instead of -EBUSY when irq_get fails
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 08/12 v2] drm/i915: use __wait_seqno for ring throttle

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:03:05PM -0700, Ben Widawsky wrote:
> It turns out throttle had an almost identical  bit of code to do the
> wait. Now we can call the new helper directly.  This is just a bonus,
> and not needed for the overall series.
> 
> v2: remove irq_get/put which is now in __wait_seqno (Ben)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next up to this patch, thanks for the patches.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 09/12] drm/i915: timeout parameter for seqno wait

[Daniel Vetter]

On Thu, Apr 26, 2012 at 04:03:06PM -0700, Ben Widawsky wrote:
> Insert a wait parameter in the code so we can possibly timeout on a
> seqno wait if need be. The code should be functionally the same as
> before because all the callers will continue to retry if an arbitrary
> timeout elapses.
> 
> We'd like to have nanosecond granularity, but the only way to do this is
> with hrtimer, and that doesn't fit well with the needs of this code.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I have to admit, I'm a bit unhappy with this swiss-army-tool wait_seqno
and what it looks like. What about copy&pasting __wait_seqno_timeout,
which is always interruptible?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   58 +++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0ae1a73..f054439 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1820,10 +1820,23 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  }
>  
>  static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
> -			bool interruptible)
> +			bool interruptible, long *usecs)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	int ret = 0;
> +	bool wait_forever = false;
> +	long timeout, end;
> +
> +	if (usecs == NULL || ((*usecs) < 0)) {
> +		wait_forever = true;
> +		timeout = msecs_to_jiffies(100);
> +	} else
> +		timeout = usecs_to_jiffies(*usecs);
> +
> +	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
> +		return 0;
> +
> +	if (WARN_ON(!ring->irq_get(ring)))
> +		return -ENODEV;
>  
>  	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
>  		return 0;
> @@ -1836,17 +1849,40 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>  	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
>  	atomic_read(&dev_priv->mm.wedged))
>  
> +	trace_i915_gem_request_wait_begin(ring, seqno);
> +again:
>  	if (interruptible)
> -		ret = wait_event_interruptible(ring->irq_queue,
> -					       EXIT_COND);
> +		end = wait_event_interruptible_timeout(ring->irq_queue,
> +						       EXIT_COND,
> +						       timeout);
>  	else
> -		wait_event(ring->irq_queue, EXIT_COND);
> +		end = wait_event_timeout(ring->irq_queue, EXIT_COND, timeout);
> +#undef EXIT_COND
> +
> +	if (atomic_read(&dev_priv->mm.wedged))
> +		end = -EAGAIN;
> +
> +	if (end == 0 && wait_forever)
> +		goto again;
>  
> -	ring->irq_put(ring);
>  	trace_i915_gem_request_wait_end(ring, seqno);
> -#undef EXIT_COND
> +	ring->irq_put(ring);
>  
> -	return ret;
> +	if (!wait_forever) {
> +		BUG_ON(end == 0);
> +		*usecs = jiffies_to_usecs(timeout - end);
> +	}
> +
> +	switch (end) {
> +	case -EAGAIN: /* Wedged */
> +	case -ERESTARTSYS: /* Signal */
> +		return (int)end;
> +	case 0: /* Tiemout */
> +		return -ETIME;
> +	default: /* Completed */
> +		WARN_ON(end < 0); /* We're not aware of other errors */
> +		return 0;
> +	}
>  }
>  
>  /**
> @@ -1891,9 +1927,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
>  		seqno = request->seqno;
>  	}
>  
> -	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
> -	if (atomic_read(&dev_priv->mm.wedged))
> -		ret = -EAGAIN;
> +	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible, NULL);
>  
>  	return ret;
>  }
> @@ -2981,7 +3015,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
>  	if (seqno == 0)
>  		return 0;
>  
> -	ret = __wait_seqno(ring, seqno, true);
> +	ret = __wait_seqno(ring, seqno, true, NULL);
>  	if (ret == 0)
>  		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
>  
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Thu, 26 Apr 2012 16:03:08 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This helps implement glClientWaitSync.
> 
> Finally we can use the new timed seqno waiting function to allow
> userspace to wait on a request with a timeout. This implements that
> interface.
> 
> The new ioctl is very straight forward, there is a flags field which I
> envision may be useful for various flush permutations of the command.

What are the semantics of the ioctl? A simple use case would help
specify the interface here.

In particular, I can't tell whether the return value (timeout_ns) is
meant to be the time elapsed or the time remaining.  What value is
returned in the timeout if we are interrupted before the wait completes?
Would
  end = gettimeofday() + timeout;
  do {
    ret = i915_gem_wait(handle, 0, &timeout);
  } while (ret == -1 && errno == EINTR);
  assert(gettimeofday() <= end);
wait forever, or until the original timeout expires?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/12] drm/i915: timeout parameter for seqno wait

[Ben Widawsky]

On Fri, 27 Apr 2012 17:00:20 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Apr 26, 2012 at 04:03:06PM -0700, Ben Widawsky wrote:
> > Insert a wait parameter in the code so we can possibly timeout on a
> > seqno wait if need be. The code should be functionally the same as
> > before because all the callers will continue to retry if an
> > arbitrary timeout elapses.
> > 
> > We'd like to have nanosecond granularity, but the only way to do
> > this is with hrtimer, and that doesn't fit well with the needs of
> > this code.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I have to admit, I'm a bit unhappy with this swiss-army-tool
> wait_seqno and what it looks like. What about copy&pasting
> __wait_seqno_timeout, which is always interruptible?
> -Daniel

I'm going to put the onus on you for this bikeshed to test it, and see
how you like it. I have tried have the other function, and I felt this
one looked better. Though I assume I have to fix the rebase error that
Chris pointed out anyway. So I'll do a v3 with the separate function
*after* you confirm you really want it. Otherwise, you'll just get the
rebase fail fix.

Ben

Re: [PATCH 09/12] drm/i915: timeout parameter for seqno wait

[Ben Widawsky]

On Fri, 27 Apr 2012 15:14:10 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 26 Apr 2012 16:03:06 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> >  static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
> > -			bool interruptible)
> > +			bool interruptible, long *usecs)
> >  {
> >  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> > -	int ret = 0;
> > +	bool wait_forever = false;
> > +	long timeout, end;
> > +
> > +	if (usecs == NULL || ((*usecs) < 0)) {
> > +		wait_forever = true;
> > +		timeout = msecs_to_jiffies(100);
> > +	} else
> > +		timeout = usecs_to_jiffies(*usecs);
> > +
> > +	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
> > +		return 0;
> > +
> > +	if (WARN_ON(!ring->irq_get(ring)))
> > +		return -ENODEV;
> 
> Rebase error, duplicated ring->irq_get/ring->get_seqno.
> -Chris
> 

Got it, thanks.

Re: [PATCH 10/12] drm/i915: extract some common olr+wedge code

[Ben Widawsky]

On Fri, 27 Apr 2012 09:46:31 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 26 Apr 2012 16:03:07 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > +/*
> > + * Compare seqno against outstanding lazy request. Emit a request
> > if they are
> > + * equal. Seqno is updated with the new value if a request was
> > emitted.
> > + */
> > +static int
> > +i915_gem_check_olr(struct intel_ring_buffer *ring, u32 *seqno)
> > +{
> > +	int ret = 0;
> > +
> > +	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> > +
> > +	if (*seqno == ring->outstanding_lazy_request) {
> > +		struct drm_i915_gem_request *request;
> > +
> > +		request = kzalloc(sizeof(*request), GFP_KERNEL);
> > +		if (request == NULL)
> > +			return -ENOMEM;
> > +
> > +		ret = i915_add_request(ring, NULL, request);
> > +		if (ret) {
> > +			kfree(request);
> > +			return ret;
> > +		}
> > +
> > +		*seqno = request->seqno;
> I'd love for this to be BUG_ON(seqno != request->seqno) and so drop
> the out parameter, as it would tidy all the callers up.
> > +	}
> > +
> > +	return ret;
> -Chris
> 

Got it, thanks.

I hesitated to do this initially because of the logic in
i915_gem_object_sync. In that function, the updated seqno is used by the
ring sync code, and without the outparam, there was no way to not modify
the behavior. However after inspecting the code a bit deeper, I
understand why it's not a functional change, and the BUG_ON asserts
that.

The one bit which is still not clear to me, even after IRC, is why we
need to add the olr request at this point at all. Even if the mbox update
is delayed for a bit, I don't understand why things won't work
correctly; though I've confirmed I do get a hangcheck if I remove the
bit of code.

Comments?

Re: [PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Fri, 27 Apr 2012 09:44:55 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 26 Apr 2012 16:03:08 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > +	mutex_unlock(&dev->struct_mutex);
> > +	ret = __wait_seqno(ring, seqno, true, &timeout);
> > +	drm_gem_object_unreference_unlocked(&obj->base);
> 
> Once we have the seqno to wait on, we can drop the reference to the
> object. The reference to the ring will be persist whilst the device is
> open. Just saves doing an unsightly unlock/lock dance in the unref.
> -Chris
> 

Got it, thanks.

Re: [PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Fri, 27 Apr 2012 16:24:24 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 26 Apr 2012 16:03:08 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > This helps implement glClientWaitSync.
> > 
> > Finally we can use the new timed seqno waiting function to allow
> > userspace to wait on a request with a timeout. This implements that
> > interface.
> > 
> > The new ioctl is very straight forward, there is a flags field
> > which I envision may be useful for various flush permutations of
> > the command.
> 
> What are the semantics of the ioctl? A simple use case would help
> specify the interface here.

The first time I sent out the series, I included the libdrm and igt
test. Is this what you're looking for, or something in the commit
message?

> 
> In particular, I can't tell whether the return value (timeout_ns) is
> meant to be the time elapsed or the time remaining.  What value is
> returned in the timeout if we are interrupted before the wait
> completes? Would
>   end = gettimeofday() + timeout;
>   do {
>     ret = i915_gem_wait(handle, 0, &timeout);
>   } while (ret == -1 && errno == EINTR);
>   assert(gettimeofday() <= end);
> wait forever, or until the original timeout expires?
> -Chris
> 

The return value should give the remaining time left if the return value
was not 0, though I venture to guess you're Socratically trying to tell
me something, so I'll go through this again and find the bug.

Re: [PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Sat, 28 Apr 2012 19:11:34 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Fri, 27 Apr 2012 16:24:24 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Thu, 26 Apr 2012 16:03:08 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > This helps implement glClientWaitSync.
> > > 
> > > Finally we can use the new timed seqno waiting function to allow
> > > userspace to wait on a request with a timeout. This implements
> > > that interface.
> > > 
> > > The new ioctl is very straight forward, there is a flags field
> > > which I envision may be useful for various flush permutations of
> > > the command.
> > 
> > What are the semantics of the ioctl? A simple use case would help
> > specify the interface here.
> 
> The first time I sent out the series, I included the libdrm and igt
> test. Is this what you're looking for, or something in the commit
> message?
> 
> > 
> > In particular, I can't tell whether the return value (timeout_ns) is
> > meant to be the time elapsed or the time remaining.  What value is
> > returned in the timeout if we are interrupted before the wait
> > completes? Would
> >   end = gettimeofday() + timeout;
> >   do {
> >     ret = i915_gem_wait(handle, 0, &timeout);
> >   } while (ret == -1 && errno == EINTR);
> >   assert(gettimeofday() <= end);
> > wait forever, or until the original timeout expires?
> > -Chris
> > 
> 
> The return value should give the remaining time left if the return
> value was not 0, though I venture to guess you're Socratically trying
> to tell me something, so I'll go through this again and find the bug.


Ah, I think I see the problem...

Re: [PATCH 11/12 v2] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Sat, 28 Apr 2012 19:11:34 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, 27 Apr 2012 16:24:24 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Thu, 26 Apr 2012 16:03:08 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > This helps implement glClientWaitSync.
> > > 
> > > Finally we can use the new timed seqno waiting function to allow
> > > userspace to wait on a request with a timeout. This implements that
> > > interface.
> > > 
> > > The new ioctl is very straight forward, there is a flags field
> > > which I envision may be useful for various flush permutations of
> > > the command.
> > 
> > What are the semantics of the ioctl? A simple use case would help
> > specify the interface here.
> 
> The first time I sent out the series, I included the libdrm and igt
> test. Is this what you're looking for, or something in the commit
> message?

Whilst we will be bound by the API as used forever and a day, I found I
could not review the code against the interface for any aberrant
behaviour...

I'd at least like to some justification for the form of the interface in
the changelog and some indication of expected behaviour. That block
should be good enough also to include next to the ioctl so that we have
some ideas of what userspace is expecting when reviewing the code later.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 03/12 v2] drm/i915: move vbetool invoked ier stuff

[Jesse Barnes]

On Thu, 26 Apr 2012 16:03:00 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> This extra bit of interrupt enabling code doesn't belong in the wait
> seqno function. If anything we should pull it out to a helper so the
> throttle code can also use it. The history is a bit vague, but I am
> going to attempt to just dump it, unless someone can argue otherwise.
> 
> Removing this allows for a shared lock free wait seqno function. To
> keep tabs on this issue though, the IER value is stored on error
> capture (recommended by Chris Wilson)
> 
> v2: fixed typo EIR->IER (Ben)
> Fix some white space (Ben)
> Move IER capture to globally instead of per ring (Ben)
> 

It may be safe to remove this now.  Distros used to run vbetool at
random times, shutting things off and messing with hw.  But these days
they use the driver with a bit less molestation, so the IER check/fix
shouldn't be needed.

Jesse