[PATCH 00/10] wait for BO with timeout

[PATCH 00/10] wait for BO with timeout

[Ben Widawsky]

patches 1-6 are various fixes and cleanups in our existing wait_request
handling code. The purpose here is to consolidate as much code as
possible into core waiting functions, and to prepare those core
functions for atomic access.

patches 7-8 are the patches we actually care about - timeout parameters
within the core functions

patch 9 is the ioctl interface to wait on a given buffer for some amount
of time

patch 10 is just something which annoys me

The libdrm and i-g-t patches I am testing with are here:
http://cgit.freedesktop.org/~bwidawsk/drm/commit/?h=wait_render&id=719212fbdbcf478ebf2931040c6b415d2765383e

http://cgit.freedesktop.org/~bwidawsk/intel-gpu-tools/commit/?h=wait_render&id=31dcb8379d3b263a91a85db911559317321c6b7c

100ms seems to be sufficient to complete the test on my SNB.

Note to anyone who dare review this: I cannot explain why, but my i-g-t
test when set with a value to actually complete (100ms), always returns a
multiple of a 1000000 nanoseconds. This probably means a bug lurks
somewhere, but I haven't been able to find it. It is likely something
very stupid.

Also, I cannot figure out a good i-g-t test to upstream since the amount
of time to timeout can vary quite a bit. However the test above is good
for basic functionality.

Ben Widawsky (10):
  drm/i915: remove do_retire from i915_wait_request
  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: use __wait_seqno for ring throttle
  drm/i915: timeout parameter for seqno wait
  drm/i915: real wait seqno with timeout
  drm/i915: wait render timeout ioctl
  drm/i915: s/i915_wait_reqest/i915_wait_seqno/g

 drivers/gpu/drm/i915/i915_debugfs.c        |    3 +-
 drivers/gpu/drm/i915/i915_dma.c            |    3 +-
 drivers/gpu/drm/i915/i915_drv.h            |   10 +-
 drivers/gpu/drm/i915/i915_gem.c            |  207 ++++++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_evict.c      |   14 +-
 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            |   15 +-
 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                     |    9 ++
 12 files changed, 184 insertions(+), 92 deletions(-)

-- 
1.7.10

[PATCH 01/10] 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.

NOTE: this patch has a functional change. I've only made the callers
which are requiring the retirement do the retirement. This move was
blasted by Keith when I tried it before in a more subtle manner; so I am
making it very clear this time around.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c            |    2 +-
 drivers/gpu/drm/i915/i915_drv.h            |    5 ++---
 drivers/gpu/drm/i915/i915_gem.c            |   33 +++++++++-------------------
 drivers/gpu/drm/i915/i915_gem_evict.c      |   14 ++++++++++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
 drivers/gpu/drm/i915/intel_overlay.c       |    6 ++---
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +++-
 8 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a813f65..f8fdc5b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2198,7 +2198,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, true);
+	ret = i915_gpu_idle(dev);
 	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 69e1539..a6afd83 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1291,14 +1291,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 7bc4a40..3008cba 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1853,8 +1853,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;
@@ -1930,14 +1929,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;
 }
 
@@ -1959,10 +1950,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;
@@ -2145,7 +2136,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;
 
@@ -2159,18 +2150,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;
 	}
@@ -2359,12 +2349,9 @@ 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;
-
 		obj->last_fenced_seqno = 0;
 	}
 
@@ -3440,7 +3427,7 @@ 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;
@@ -4018,7 +4005,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 21a8271..df9354c 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -168,6 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	int ret;
 	bool lists_empty;
+	int i;
 
 	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
 		       list_empty(&dev_priv->mm.flushing_list) &&
@@ -177,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));
 
 	return i915_gem_evict_inactive(dev, purgeable_only);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 68ec013..582f6c4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1220,7 +1220,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, true);
+			ret = i915_gpu_idle(dev);
 			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 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 80b331c..5ade12e 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -225,8 +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,
-				true);
+	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
 	if (ret)
 		return ret;
 
@@ -447,8 +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,
-				true);
+	ret = i915_wait_request(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 12d9bc7..13eaf6a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1049,9 +1049,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/10] 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)

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 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     |    7 ++++++-
 4 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 35462df..5c4af9a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -791,6 +791,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, "EIR: 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 a6afd83..845d4af 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -163,6 +163,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 3008cba..64bbcf7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1856,7 +1856,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);
@@ -1891,19 +1890,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 ab023ca..cf0e9f0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1044,7 +1044,12 @@ static void i915_record_ring_state(struct drm_device *dev,
 		error->ipehr[ring->id] = I915_READ(IPEHR);
 		error->instdone[ring->id] = I915_READ(INSTDONE);
 	}
-
+	if (HAS_PCH_SPLIT(ring->dev))
+		error->ier = I915_READ(DEIER) | I915_READ(GTIER);
+	else if (IS_VALLEYVIEW(ring->dev))
+		error->ier = I915_READ(GTIER) | I915_READ(VLV_IER);
+	else
+		error->ier = I915_READ(IER);
 	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);
-- 
1.7.10

[PATCH 03/10] drm/i915: kill waiting_seqno

[Ben Widawsky]

It's not terribly useful and it ruins our ability to do waits atomically.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |    2 --
 drivers/gpu/drm/i915/i915_gem.c         |    2 --
 drivers/gpu/drm/i915/i915_irq.c         |    8 +++-----
 drivers/gpu/drm/i915/intel_ringbuffer.h |    1 -
 4 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5c4af9a..ee80266 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -430,8 +430,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);
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 64bbcf7..a86a6ee 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1892,7 +1892,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,
@@ -1908,7 +1907,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 cf0e9f0..52eb9ed 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1846,11 +1846,9 @@ static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
 	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 06a66ad..69798c1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -61,7 +61,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 04/10] 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

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 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 a86a6ee..6ad52d3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1892,22 +1892,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 -EBUSY;
 
-			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 05/10] 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

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ad52d3..b4a957c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1847,6 +1847,27 @@ 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;
+
+#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);
+
+#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.
@@ -1895,14 +1916,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		if (WARN_ON(!ring->irq_get(ring)))
 			return -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));
+		ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
 
 		ring->irq_put(ring);
 		trace_i915_gem_request_wait_end(ring, seqno);
-- 
1.7.10

[PATCH 06/10] 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.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b4a957c..920dbc1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3012,18 +3012,13 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 		 * 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));
+			ret = __wait_seqno(ring, seqno, true);
 			ring->irq_put(ring);
 
 			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 07/10] 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.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   41 ++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 920dbc1..7bfad04 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1847,25 +1847,36 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+#define SEQNO_WAIT_DEFAULT (3000000UL)
 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;
+	const long timeout = usecs_to_jiffies(*usecs);
+	long end;
 
 #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);
+		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
 
-	return ret;
+	switch (end) {
+	case 0: /* Tiemout */
+		return -ETIME;
+	case -ERESTARTSYS: /* Signal */
+		return -ERESTARTSYS;
+	default: /* Completed */
+		BUG_ON(end < 0); /* We're not aware of other errors */
+		*usecs = jiffies_to_usecs(timeout - end);
+		return 0;
+	}
 }
 
 /**
@@ -1916,7 +1927,12 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		if (WARN_ON(!ring->irq_get(ring)))
 			return -EBUSY;
 
-		ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
+		do {
+			long time = SEQNO_WAIT_DEFAULT;
+			ret  = __wait_seqno(ring, seqno,
+					    dev_priv->mm.interruptible,
+					    &time);
+		} while (ret == -ETIME);
 
 		ring->irq_put(ring);
 		trace_i915_gem_request_wait_end(ring, seqno);
@@ -3012,13 +3028,16 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 		 * lockless.
 		 */
 		if (ring->irq_get(ring)) {
-			ret = __wait_seqno(ring, seqno, true);
+			do {
+				long time = SEQNO_WAIT_DEFAULT;
+				ret = __wait_seqno(ring, seqno, true, &time);
+			} while (ret == -ETIME);
 			ring->irq_put(ring);
-
 			if (ret == 0 && atomic_read(&dev_priv->mm.wedged))
 				ret = -EIO;
-		} else
+		} else {
 			ret = -EBUSY;
+		}
 	}
 
 	if (ret == 0)
-- 
1.7.10

[PATCH 08/10] drm/i915: real wait seqno with timeout

[Ben Widawsky]

Move the final interesting chunk from i915_wait_seqno into a timed
variant. The timed variant can take a NULL or 0 timeout argument to
preserve the old behavior, and this allowing the old (still useful
function) to call directly into the timed variant.

This code does not require struct_mutex, therefore it can wait on events
without holding the lock.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   53 ++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7bfad04..e446d9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1879,6 +1879,40 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	}
 }
 
+static int
+i915_seqno_wait_timed(struct intel_ring_buffer *ring, u32 seqno,
+		      bool interruptible, long *timeout)
+{
+	bool wait_forever = false;
+	int ret = 0;
+
+	BUG_ON(seqno == ring->outstanding_lazy_request);
+
+	if (timeout == NULL || ((*timeout) < 0))
+		wait_forever = true;
+
+	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 -EBUSY;
+
+		do {
+			long time = wait_forever ? SEQNO_WAIT_DEFAULT : *timeout;
+			ret  = __wait_seqno(ring, seqno,
+					    interruptible,
+					    &time);
+			if (ret == 0 && timeout)
+				*timeout = time;
+		} while (ret == -ETIME && wait_forever);
+
+		ring->irq_put(ring);
+		trace_i915_gem_request_wait_end(ring, seqno);
+	}
+
+	return ret;
+}
+
 /**
  * Waits for a sequence number to be signaled, and cleans up the
  * request and object lists appropriately for that event.
@@ -1920,23 +1954,8 @@ 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 -EBUSY;
-
-		do {
-			long time = SEQNO_WAIT_DEFAULT;
-			ret  = __wait_seqno(ring, seqno,
-					    dev_priv->mm.interruptible,
-					    &time);
-		} while (ret == -ETIME);
-
-		ring->irq_put(ring);
-		trace_i915_gem_request_wait_end(ring, seqno);
-	}
+	ret = i915_seqno_wait_timed(ring, seqno, dev_priv->mm.interruptible,
+				    NULL);
 	if (atomic_read(&dev_priv->mm.wedged))
 		ret = -EAGAIN;
 
-- 
1.7.10

[PATCH 09/10] drm/i915: wait render timeout ioctl

[Ben Widawsky]

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.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 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          |    9 +++++++
 4 files changed, 67 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f8fdc5b..45a4b8b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2375,6 +2375,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 845d4af..a6ec0fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1214,6 +1214,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 e446d9c..2604beb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1989,6 +1989,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;
+	long timeout;
+	u32 seqno = 0;
+	int ret = 0;
+
+	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;
+	}
+
+	timeout = args->timeout_ns;
+
+	if (obj->active) {
+		ring = obj->ring;
+		seqno = obj->last_rendering_seqno;
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+	if (seqno == 0)
+		goto out;
+
+	BUG_ON(ring == NULL);
+
+	/* For now timeout is microseconds only */
+	if (timeout < 1000 && timeout != 0) {
+		DRM_DEBUG("Rounding value from %ld to 1000\n", timeout);
+		timeout = 1000;
+	}
+
+	timeout /= 1000;
+
+	ret = i915_seqno_wait_timed(ring, seqno, true, &timeout);
+	if (ret == -ERESTARTSYS)
+		ret = -EINTR;
+	else if (ret == -ETIME) {
+		ret = -EBUSY;
+	} else {
+		WARN_ON(timeout <= 0);
+	}
+out:
+	args->timeout_ns = timeout * 1000;
+	drm_gem_object_unreference_unlocked(&obj->base);
+	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..e365ab9 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,11 @@ struct drm_intel_sprite_colorkey {
 	__u32 flags;
 };
 
+struct drm_i915_gem_wait {
+	__u32 bo_handle;
+	__u64 timeout_ns;
+	__u32 flags;
+	__u32 rsvd;
+};
+
 #endif				/* _I915_DRM_H_ */
-- 
1.7.10

[PATCH 10/10] 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 <benjamin.widawsky@intel.com>
---
 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 a6ec0fd..65f4f33 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1299,8 +1299,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 2604beb..ae8843e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1918,8 +1918,7 @@ i915_seqno_wait_timed(struct intel_ring_buffer *ring, u32 seqno,
  * 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;
@@ -1980,7 +1979,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);
@@ -2235,7 +2234,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)
@@ -2434,7 +2433,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;
 		obj->last_fenced_seqno = 0;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 5ade12e..1de2380 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 13eaf6a..57e1a92 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1049,7 +1049,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 02/10] drm/i915: move vbetool invoked ier stuff

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:24 -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)
> 
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  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     |    7 ++++++-
>  4 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 35462df..5c4af9a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -791,6 +791,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, "EIR: 0x%08x\n", error->ier);
>  	seq_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);

Thinking about this a bit more, the error state also needs to know if we
had anybody waiting on a ring->irq_queue.
	error->waiting = 0;
	for_each_ring(ring)
		if (waitqueue_active(&ring->irq_queue)
			error->waiting |= 1 << ring->id;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/10] drm/i915: drop polled waits from i915_wait_request

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:26 -0700, Ben Widawsky <ben@bwidawsk.net> 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
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---

> +		if (WARN_ON(!ring->irq_get(ring)))
> +			return -EBUSY;

I think this is now a -ENODEV; :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:31 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> +	ret = i915_seqno_wait_timed(ring, seqno, true, &timeout);
> +	if (ret == -ERESTARTSYS)
> +		ret = -EINTR;
Don't convert it here, pass ERESTARTSYS to the system call handler which
decides how to handle it.

> +	else if (ret == -ETIME) {
> +		ret = -EBUSY;

Why the semantic change? ETIME for timer timed out still seems appropriate.

I think this whole interface is a stop-gap solution for pollable sync
objects without a clear use case. Do we have a spec for a feature
wishing to build upon this interface?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Sat, 21 Apr 2012 10:41:55 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 20 Apr 2012 18:23:31 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > +	ret = i915_seqno_wait_timed(ring, seqno, true, &timeout);
> > +	if (ret == -ERESTARTSYS)
> > +		ret = -EINTR;
> Don't convert it here, pass ERESTARTSYS to the system call handler
> which decides how to handle it.
> 
> > +	else if (ret == -ETIME) {
> > +		ret = -EBUSY;
> 
> Why the semantic change? ETIME for timer timed out still seems
> appropriate.

I must be missing something. Can you point to me where the system call
handler converts these? The only reason for the change was to prevent
passing the internal return types to user space.

> 
> I think this whole interface is a stop-gap solution for pollable sync
> objects without a clear use case. Do we have a spec for a feature
> wishing to build upon this interface?

Good point. This is primarily for glClientWaitSync. I will update the
commit message with this information.

> -Chris
> 

Re: [PATCH 04/10] drm/i915: drop polled waits from i915_wait_request

[Ben Widawsky]

On Sat, 21 Apr 2012 10:29:35 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 20 Apr 2012 18:23:26 -0700, Ben Widawsky <ben@bwidawsk.net>
> 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
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> 
> > +		if (WARN_ON(!ring->irq_get(ring)))
> > +			return -EBUSY;
> 
> I think this is now a -ENODEV; :)
> -Chris
> 
Got it. Thanks.

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

[Daniel Vetter]

On Fri, Apr 20, 2012 at 06:23:23PM -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.
> 
> NOTE: this patch has a functional change. I've only made the callers
> which are requiring the retirement do the retirement. This move was
> blasted by Keith when I tried it before in a more subtle manner; so I am
> making it very clear this time around.

See below for why it's still not a good idea to combine refactoring with
code changes ;-)

> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c            |    2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |    5 ++---
>  drivers/gpu/drm/i915/i915_gem.c            |   33 +++++++++-------------------
>  drivers/gpu/drm/i915/i915_gem_evict.c      |   14 ++++++++++--
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
>  drivers/gpu/drm/i915/intel_overlay.c       |    6 ++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +++-
>  8 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a813f65..f8fdc5b 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c

[cut]

> @@ -3440,7 +3427,7 @@ 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;


gem_idle is called by our suspend freeze function and leaking unretired
seqnos over a s/r cycle was the root cause our -rc2 regression on gen3. In
other words: I'm pretty sure this will blow up. I do like the idea of the
patch, but:

Please separate refactoring from actual code changes.

Cheers, Daniel

> @@ -4018,7 +4005,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 21a8271..df9354c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -168,6 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	int ret;
>  	bool lists_empty;
> +	int i;
>  
>  	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
>  		       list_empty(&dev_priv->mm.flushing_list) &&
> @@ -177,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));
>  
>  	return i915_gem_evict_inactive(dev, purgeable_only);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 68ec013..582f6c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1220,7 +1220,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, true);
> +			ret = i915_gpu_idle(dev);
>  			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 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 80b331c..5ade12e 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -225,8 +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,
> -				true);
> +	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
>  	if (ret)
>  		return ret;
>  
> @@ -447,8 +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,
> -				true);
> +	ret = i915_wait_request(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 12d9bc7..13eaf6a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1049,9 +1049,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
> 
> _______________________________________________
> 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/10] drm/i915: remove do_retire from i915_wait_request

[Ben Widawsky]

On Sat, 21 Apr 2012 19:17:11 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Apr 20, 2012 at 06:23:23PM -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.
> > 
> > NOTE: this patch has a functional change. I've only made the callers
> > which are requiring the retirement do the retirement. This move was
> > blasted by Keith when I tried it before in a more subtle manner; so
> > I am making it very clear this time around.
> 
> See below for why it's still not a good idea to combine refactoring
> with code changes ;-)

I think in this case it's quite obvious what went wrong if things blow
up, and the patch is so simple that separating it really makes no
difference to anybody when trying to track down a bug.

I agree without the NOTE in the comment this would be a fairly evil
thing to do, which is why Keith got upset before... 

> 
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c            |    2 +-
> >  drivers/gpu/drm/i915/i915_drv.h            |    5 ++---
> >  drivers/gpu/drm/i915/i915_gem.c            |   33
> > +++++++++-------------------
> > drivers/gpu/drm/i915/i915_gem_evict.c      |   14 ++++++++++--
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
> > drivers/gpu/drm/i915/intel_overlay.c       |    6 ++---
> > drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +++- 8 files
> > changed, 32 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c index a813f65..f8fdc5b 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> 
> [cut]
> 
> > @@ -3440,7 +3427,7 @@ 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;
> 
> 
> gem_idle is called by our suspend freeze function and leaking
> unretired seqnos over a s/r cycle was the root cause our -rc2
> regression on gen3. In other words: I'm pretty sure this will blow
> up. I do like the idea of the patch, but:
> 
> Please separate refactoring from actual code changes.

Fine.

Just curious, it blows up on gen3 only? If not, how can I make it blow
up?

> 
> Cheers, Daniel
> 

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

[Daniel Vetter]

On Sat, Apr 21, 2012 at 10:27:38AM -0700, Ben Widawsky wrote:
> Just curious, it blows up on gen3 only? If not, how can I make it blow
> up?

I think you need the gpu fencing code to make it blow up, so gen2/3.
Chris' refactoring might have changed some things in there and it doesn't
blow up any longer - I haven't tried tbh.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Sat, 21 Apr 2012 09:12:17 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Sat, 21 Apr 2012 10:41:55 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Fri, 20 Apr 2012 18:23:31 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > +	ret = i915_seqno_wait_timed(ring, seqno, true, &timeout);
> > > +	if (ret == -ERESTARTSYS)
> > > +		ret = -EINTR;
> > Don't convert it here, pass ERESTARTSYS to the system call handler
> > which decides how to handle it.
> > 
> > > +	else if (ret == -ETIME) {
> > > +		ret = -EBUSY;
> > 
> > Why the semantic change? ETIME for timer timed out still seems
> > appropriate.
> 
> I must be missing something. Can you point to me where the system call
> handler converts these? The only reason for the change was to prevent
> passing the internal return types to user space.
> 
> > 
> > I think this whole interface is a stop-gap solution for pollable sync
> > objects without a clear use case. Do we have a spec for a feature
> > wishing to build upon this interface?
> 
> Good point. This is primarily for glClientWaitSync. I will update the
> commit message with this information.
> 
> > -Chris
> > 

NVM, got them both. Thanks.

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Sat, 21 Apr 2012 09:12:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sat, 21 Apr 2012 10:41:55 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > I think this whole interface is a stop-gap solution for pollable sync
> > objects without a clear use case. Do we have a spec for a feature
> > wishing to build upon this interface?
> 
> Good point. This is primarily for glClientWaitSync. I will update the
> commit message with this information.

I read through the ARB_sync spec and this interface does seem to be a
good match for that. A slight sour test from that spec being open-ended
which might mean that we need to revise the interface in future.
However, I did realise one blindingly obvious use-case:

  drm_intel_gem_bo_wait_wait_rendering();

Currently this is used by Mesa to do its client throttling, and in
consequence also blocks X. i915_wait_ioctl() will allow mesa to only
block itself and so help reduce interface stutters, even more important
with multiple GL clients.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:31 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> +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;
> +	long timeout;
> +	u32 seqno = 0;
> +	int ret = 0;
> +
> +	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;
> +	}
> +
> +	timeout = args->timeout_ns;

We discussed on IRC whether or not we needed to flush here. For the
drm_intel_gem_bo_wait_rendering() use-case, we do need to be able to first
queue a flush.

  if (args->flags & I915_WAIT_FINISH)
	i915_gem_object_flush_gpu_write_domain(obj);

Borrowing the terminology from glFlush/glFinish. This becomes moot if
the flushing list is ever vanquished, but still a useful distinction and
required today.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Daniel Vetter]

On Sun, Apr 22, 2012 at 10:48:01AM +0100, Chris Wilson wrote:
> On Fri, 20 Apr 2012 18:23:31 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > +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;
> > +	long timeout;
> > +	u32 seqno = 0;
> > +	int ret = 0;
> > +
> > +	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;
> > +	}
> > +
> > +	timeout = args->timeout_ns;
> 
> We discussed on IRC whether or not we needed to flush here. For the
> drm_intel_gem_bo_wait_rendering() use-case, we do need to be able to first
> queue a flush.
> 
>   if (args->flags & I915_WAIT_FINISH)
> 	i915_gem_object_flush_gpu_write_domain(obj);
> 
> Borrowing the terminology from glFlush/glFinish. This becomes moot if
> the flushing list is ever vanquished, but still a useful distinction and
> required today.

Yeah, I agree that emitting any required flushes (akin to the busy ioctl)
makes sense here. While I bikeshed, the mutex_lock can be moved after
drm_gem_object_lookup - we only need the mutex when dropping the reference
(and hence the unref_unlocked variant).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Daniel Vetter]

On Fri, Apr 20, 2012 at 06:23:31PM -0700, Ben Widawsky wrote:
> 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.
> 
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---

[cut]

> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index f3f8224..e365ab9 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,11 @@ struct drm_intel_sprite_colorkey {
>  	__u32 flags;
>  };
>  
> +struct drm_i915_gem_wait {
> +	__u32 bo_handle;
> +	__u64 timeout_ns;
> +	__u32 flags;
> +	__u32 rsvd;
> +};

struct layout gone wrong, you miss a __u32 before the __u64.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 07/10] drm/i915: timeout parameter for seqno wait

[Daniel Vetter]

On Fri, Apr 20, 2012 at 06:23:29PM -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.
> 
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

Interface bikeshed: I think handling the wait_forever case in here would
be better, the do {} while (-ETIME) loops don't look to cute. That way we
can also ditch the rather arbitrary SEQNO_WAIT_DEFAULT.

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   41 ++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 920dbc1..7bfad04 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1847,25 +1847,36 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> +#define SEQNO_WAIT_DEFAULT (3000000UL)
>  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;
> +	const long timeout = usecs_to_jiffies(*usecs);
> +	long end;
>  
>  #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);
> +		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
>  
> -	return ret;
> +	switch (end) {
> +	case 0: /* Tiemout */
> +		return -ETIME;
> +	case -ERESTARTSYS: /* Signal */
> +		return -ERESTARTSYS;
> +	default: /* Completed */
> +		BUG_ON(end < 0); /* We're not aware of other errors */

WARN_ON, we won't die right away when this ever happens.

> +		*usecs = jiffies_to_usecs(timeout - end);

Safe when the ioctl restarting works different, you also need this for the
-ERESTARTSYS case. And it doesn't hurt for the timeout case.

> +		return 0;
> +	}
>  }
>  
>  /**
> @@ -1916,7 +1927,12 @@ i915_wait_request(struct intel_ring_buffer *ring,
>  		if (WARN_ON(!ring->irq_get(ring)))
>  			return -EBUSY;
>  
> -		ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
> +		do {
> +			long time = SEQNO_WAIT_DEFAULT;
> +			ret  = __wait_seqno(ring, seqno,
> +					    dev_priv->mm.interruptible,
> +					    &time);
> +		} while (ret == -ETIME);
>  
>  		ring->irq_put(ring);
>  		trace_i915_gem_request_wait_end(ring, seqno);
> @@ -3012,13 +3028,16 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
>  		 * lockless.
>  		 */
>  		if (ring->irq_get(ring)) {
> -			ret = __wait_seqno(ring, seqno, true);
> +			do {
> +				long time = SEQNO_WAIT_DEFAULT;
> +				ret = __wait_seqno(ring, seqno, true, &time);
> +			} while (ret == -ETIME);
>  			ring->irq_put(ring);
> -
>  			if (ret == 0 && atomic_read(&dev_priv->mm.wedged))
>  				ret = -EIO;
> -		} else
> +		} else {
>  			ret = -EBUSY;
> +		}
>  	}
>  
>  	if (ret == 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 03/10] drm/i915: kill waiting_seqno

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:25 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cf0e9f0..52eb9ed 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1846,11 +1846,9 @@ static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)

So ring->waiting_seqno was also in effect checking that the ring was
initialised here. Inappropriately, I might add.

Anyway we need:

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:31 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> +out:
> +	args->timeout_ns = timeout * 1000;
Multiple paths reach this point and a single one converted timeout to
microseconds.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/10] drm/i915: use __wait_seqno for ring throttle

[Chris Wilson]

On Fri, 20 Apr 2012 18:23:28 -0700, Ben Widawsky <ben@bwidawsk.net> 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.

If you defer this patch to later in the series you can simply this
function even further by using i915_seqno_wait_timed(). As the only
known user of the API, I can vouch it will not change userspace
behaviour.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 03/10] drm/i915: kill waiting_seqno

[Ben Widawsky]

On Sun, 22 Apr 2012 14:46:05 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 20 Apr 2012 18:23:25 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c index cf0e9f0..52eb9ed 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1846,11 +1846,9 @@ static bool i915_hangcheck_ring_idle(struct
> > intel_ring_buffer *ring, bool *err)
> 
> So ring->waiting_seqno was also in effect checking that the ring was
> initialised here. Inappropriately, I might add.
> 
> Anyway we need:
> 
>   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;
> >  		}
> 

Got it. Thanks.

Re: [PATCH 09/10] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Sun, 22 Apr 2012 14:45:13 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Apr 20, 2012 at 06:23:31PM -0700, Ben Widawsky wrote:
> > 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.
> > 
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> 
> [cut]
> 
> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > index f3f8224..e365ab9 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,11 @@ struct drm_intel_sprite_colorkey {
> >  	__u32 flags;
> >  };
> >  
> > +struct drm_i915_gem_wait {
> > +	__u32 bo_handle;
> > +	__u64 timeout_ns;
> > +	__u32 flags;
> > +	__u32 rsvd;
> > +};
> 
> struct layout gone wrong, you miss a __u32 before the __u64.
> -Daniel

Got it, thanks. I'm just planning to drop the rsvd field actually.