[PATCH 0/5 v3] timed BO wait

[PATCH 0/5 v3] timed BO wait

[Ben Widawsky]

This adds the fixes to the patches which Daniel didn't already take. New
patch to update trace events is here as well, otherwise they're pretty
much just cleanups and bug fixes from the previous try.

Ben Widawsky (5):
  drm/i915: timeout parameter for seqno wait
  drm/i915: make waiting trace events more useful
  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_dma.c         |    1 +
 drivers/gpu/drm/i915/i915_drv.h         |    6 +-
 drivers/gpu/drm/i915/i915_gem.c         |  260 ++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_trace.h       |   45 +++++-
 drivers/gpu/drm/i915/intel_overlay.c    |    4 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
 include/drm/i915_drm.h                  |    8 +
 7 files changed, 238 insertions(+), 88 deletions(-)

-- 
1.7.10

[PATCH 1/5 v2] 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.

v2: Fix rebase error (Chris)
Return proper time even in wedged + signal case (Chris + Ben)
Use timespec constructs (Ben)
Didn't take Daniel's advice regarding the Frankenstein-ness of the
  function. I did try his advice, but in the end I liked the way the
  original code looked, better.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 293f573..3c7e632 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1819,34 +1819,78 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+/**
+ * __wait_seqno - wait until execution of seqno has finished
+ * @ring: the ring expected to report seqno
+ * @seqno: duh!
+ * @interruptible: do an interruptible wait (normally yes)
+ * @timeout: in - how long to wait (NULL forever); out - how much time remaining
+ *
+ * Returns 0 if the seqno was found within the alloted time. Else returns the
+ * errno with remaining time filled in timeout argument.
+ */
 static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
-			bool interruptible)
+			bool interruptible, struct timespec *timeout)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
-	int ret = 0;
+	struct timespec temp, dummy_time={0,NSEC_PER_MSEC};
+	unsigned long before, timeout_jiffies;
+	long end;
+	bool wait_forever = false;
 
 	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
 		return 0;
 
 	trace_i915_gem_request_wait_begin(ring, seqno);
+
+	if (timeout == NULL) {
+		timeout = &dummy_time;
+		wait_forever = true;
+	}
+	timeout_jiffies = timespec_to_jiffies(timeout);
+
 	if (WARN_ON(!ring->irq_get(ring)))
 		return -ENODEV;
 
+	/* Record current jiffies in case interrupted by signal, or wedged * */
+	before = jiffies;
+
 #define EXIT_COND \
 	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
 	atomic_read(&dev_priv->mm.wedged))
+	do {
+		if (interruptible)
+			end = wait_event_interruptible_timeout(ring->irq_queue,
+							       EXIT_COND,
+							       timeout_jiffies);
+		else
+			end = wait_event_timeout(ring->irq_queue, EXIT_COND,
+						 timeout_jiffies);
 
-	if (interruptible)
-		ret = wait_event_interruptible(ring->irq_queue,
-					       EXIT_COND);
-	else
-		wait_event(ring->irq_queue, EXIT_COND);
+		if (atomic_read(&dev_priv->mm.wedged))
+			end = -EAGAIN;
+	} while(end == 0 && wait_forever);
+
+	jiffies_to_timespec(jiffies - before, &temp);
 
 	ring->irq_put(ring);
 	trace_i915_gem_request_wait_end(ring, seqno);
 #undef EXIT_COND
 
-	return ret;
+	jiffies_to_timespec(end, timeout);
+
+	switch (end) {
+	case -EAGAIN: /* Wedged */
+	case -ERESTARTSYS: /* Signal */
+		WARN_ON(timespec_compare(&temp, timeout) >= 0);
+		*timeout = timespec_sub(*timeout, temp);
+		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 +1935,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 +3023,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 2/5] drm/i915: make waiting trace events more useful

[Ben Widawsky]

With the new timeout parameter it is somewhat more useful to get trace
events for each iteration of the loop with values we care about.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c   |    7 +++---
 drivers/gpu/drm/i915/i915_trace.h |   45 +++++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3c7e632..19ef8bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1841,8 +1841,6 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
 		return 0;
 
-	trace_i915_gem_request_wait_begin(ring, seqno);
-
 	if (timeout == NULL) {
 		timeout = &dummy_time;
 		wait_forever = true;
@@ -1859,6 +1857,8 @@ 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))
 	do {
+		trace_i915_gem_seqno_wait_begin(ring, seqno, timeout_jiffies);
+
 		if (interruptible)
 			end = wait_event_interruptible_timeout(ring->irq_queue,
 							       EXIT_COND,
@@ -1869,12 +1869,13 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 
 		if (atomic_read(&dev_priv->mm.wedged))
 			end = -EAGAIN;
+
+		trace_i915_gem_seqno_wait_end(ring, seqno, end);
 	} while(end == 0 && wait_forever);
 
 	jiffies_to_timespec(jiffies - before, &temp);
 
 	ring->irq_put(ring);
-	trace_i915_gem_request_wait_end(ring, seqno);
 #undef EXIT_COND
 
 	jiffies_to_timespec(end, timeout);
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index dac7bba..2a4f535 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -311,16 +311,47 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_retire,
 	    TP_ARGS(ring, seqno)
 );
 
-DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_begin,
-	    TP_PROTO(struct intel_ring_buffer *ring, u32 seqno),
-	    TP_ARGS(ring, seqno)
-);
+TRACE_EVENT(i915_gem_seqno_wait_begin,
+	    TP_PROTO(struct intel_ring_buffer *ring, u32 seqno, unsigned long timeout),
+	    TP_ARGS(ring, seqno, timeout),
 
-DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_end,
-	    TP_PROTO(struct intel_ring_buffer *ring, u32 seqno),
-	    TP_ARGS(ring, seqno)
+	    TP_STRUCT__entry(
+		    __field(struct intel_ring_buffer *, ring)
+		    __field(u32, seqno)
+		    __field(unsigned long, timeout)
+		    ),
+
+	    TP_fast_assign(
+		    __entry->ring = ring;
+		    __entry->seqno = seqno;
+		    __entry->timeout = timeout;
+		    ),
+
+	    TP_printk("ring=%p, seqno=%u, timeout=%lu", __entry->ring,
+							__entry->seqno,
+							__entry->timeout)
 );
 
+TRACE_EVENT(i915_gem_seqno_wait_end,
+	    TP_PROTO(struct intel_ring_buffer *ring, u32 seqno, long ret),
+	    TP_ARGS(ring, seqno, ret),
+
+	    TP_STRUCT__entry(
+		    __field(struct intel_ring_buffer *, ring)
+		    __field(u32, seqno)
+		    __field(long, ret)
+		    ),
+
+	    TP_fast_assign(
+		    __entry->ring = ring;
+		    __entry->seqno = seqno;
+		    __entry->ret = ret;
+		    ),
+
+	    TP_printk("ring=%p, seqno=%u, ret=%ld", __entry->ring,
+						    __entry->seqno,
+						    __entry->ret)
+);
 DECLARE_EVENT_CLASS(i915_ring,
 	    TP_PROTO(struct intel_ring_buffer *ring),
 	    TP_ARGS(ring),
-- 
1.7.10

[PATCH 3/5 v2] drm/i915: extract some common olr+wedge code

[Ben Widawsky]

Refactor.

v2: Don't use seqno as an outparam (Chris)

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 19ef8bd..1acda9a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1819,6 +1819,57 @@ 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;
+		}
+
+		BUG_ON(seqno != request->seqno);
+	}
+
+	return ret;
+}
+
 /**
  * __wait_seqno - wait until execution of seqno has finished
  * @ring: the ring expected to report seqno
@@ -1907,34 +1958,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);
 
@@ -2000,22 +2030,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)
@@ -3203,20 +3220,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 {
+			ret = i915_gem_check_olr(obj->ring,
+						 obj->last_rendering_seqno);
 		}
 
 		/* Update the active list for the hardware's current position.
-- 
1.7.10

[PATCH 4/5 v3] drm/i915: wait render timeout ioctl

[Ben Widawsky]

This helps implement GL_ARB_sync put stops short of allowing full blow
sync objects.

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.

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)

v3: Drop lock after getting seqno to avoid ugly dance (Chris)
Quite a lot of restructuring and handle timeout == 0 (Ben)

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 |   60 +++++++++++++++++++++++++++++++++++++++
 include/drm/i915_drm.h          |    8 ++++++
 4 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 17081da..97a8f02 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2186,6 +2186,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 ca13098..dd8b33f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1220,6 +1220,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 1acda9a..8ee0b10 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1998,6 +1998,66 @@ 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;
+	struct timespec timeout;
+	u32 seqno = 0;
+	int ret = 0;
+
+	timeout = ns_to_timespec(args->timeout_ns);
+
+	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;
+
+	if (!args->timeout_ns) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = i915_gem_check_olr(ring, seqno);
+	if (ret)
+		goto out;
+
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+
+	ret = __wait_seqno(ring, seqno, true, &timeout);
+	args->timeout_ns = timespec_to_ns(&timeout);
+	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 5/5] 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 dd8b33f..62a3923 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1306,8 +1306,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 8ee0b10..a04bc2a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1950,8 +1950,7 @@ static int __wait_seqno(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;
@@ -1989,7 +1988,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);
@@ -2236,7 +2235,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)
@@ -2435,7 +2434,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 e06e46a..f670442 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;
 	i915_gem_retire_requests(dev);
@@ -447,7 +447,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;
 	i915_gem_retire_requests(dev);
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

[PATCH] intel: add a timed wait function

[Ben Widawsky]

drm_intel_gem_bo_wait(bo, timeout in ns)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 include/drm/i915_drm.h   |    8 ++++++++
 intel/intel_bufmgr.h     |    1 +
 intel/intel_bufmgr_gem.c |   25 +++++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index af3ce17..f71750d 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -191,6 +191,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)
@@ -234,6 +235,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.
@@ -876,4 +878,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_ */
diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index 45389e1..24a8002 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -174,6 +174,7 @@ int drm_intel_get_pipe_from_crtc_id(drm_intel_bufmgr *bufmgr, int crtc_id);
 
 int drm_intel_get_aperture_sizes(int fd, size_t *mappable, size_t *total);
 int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_t *timeout_ns);
 
 /* drm_intel_bufmgr_fake.c */
 drm_intel_bufmgr *drm_intel_bufmgr_fake_init(int fd,
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 3c91090..bc80150 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -1472,6 +1472,31 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo)
 	drm_intel_gem_bo_start_gtt_access(bo, 1);
 }
 
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_t *timeout_ns)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+	struct drm_i915_gem_wait wait;
+	int ret;
+
+	if (!timeout_ns)
+		return -EINVAL;
+
+	wait.bo_handle = bo_gem->gem_handle;
+	wait.timeout_ns = *timeout_ns;
+	wait.flags = 0;
+	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait);
+	if (ret)
+		return ret;
+
+	if (wait.timeout_ns == 0)
+		DBG("Wait timed out on buffer %d\n", bo_gem->gem_handle);
+
+	*timeout_ns = wait.timeout_ns;
+
+	return ret;
+}
+
 /**
  * Sets the object to the GTT read and possibly write domain, used by the X
  * 2D driver in the absence of kernel support to do drm_intel_gem_bo_map_gtt().
-- 
1.7.10

[PATCH] tests/wait render timeout test

[Ben Widawsky]

Not sure I want to include this in the make test suite, but it's useful
to both test the feature, and show how it can be used.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/gem_wait_render_timeout.c |  175 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+)
 create mode 100644 tests/gem_wait_render_timeout.c

diff --git a/tests/gem_wait_render_timeout.c b/tests/gem_wait_render_timeout.c
new file mode 100644
index 0000000..0cc33d7
--- /dev/null
+++ b/tests/gem_wait_render_timeout.c
@@ -0,0 +1,175 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *      based on gem_ringfill.c by Eric Anholt <eric@anholt.net>
+ *
+ */
+
+#include <stdio.h>
+#include "drm.h"
+#include "rendercopy.h"
+
+#define MSEC_PER_SEC	1000L
+#define USEC_PER_MSEC	1000L
+#define NSEC_PER_USEC	1000L
+#define NSEC_PER_MSEC	1000000L
+#define USEC_PER_SEC	1000000L
+#define NSEC_PER_SEC	1000000000L
+#define FSEC_PER_SEC	1000000000000000LL
+
+struct bo {
+	const char *ring;
+	drm_intel_bo *src, *dst, *tmp;
+} last_bo;
+
+static const int width = 512, height = 512;
+
+static void create_bo(drm_intel_bufmgr *bufmgr,
+		      struct bo *b,
+		      const char *ring)
+{
+	int size = 4 * width * height;
+
+	b->ring = ring;
+	b->src = drm_intel_bo_alloc(bufmgr, "src", size, 4096);
+	b->dst = drm_intel_bo_alloc(bufmgr, "dst", size, 4096);
+	b->tmp = drm_intel_bo_alloc(bufmgr, "tmp", size, 4096);
+}
+
+static void destroy_bo(struct bo *b)
+{
+	drm_intel_bo_unreference(b->src);
+	drm_intel_bo_unreference(b->tmp);
+	drm_intel_bo_unreference(b->dst);
+}
+
+static void make_work(drm_intel_bufmgr *bufmgr,
+		      struct intel_batchbuffer *batch,
+		      const char *ring,
+		      render_copyfunc_t copy)
+{
+	struct scratch_buf src, tmp, dst;
+	struct bo bo;
+	int i;
+
+	create_bo(bufmgr, &bo, ring);
+
+	src.stride = 4 * width;
+	src.tiling = 0;
+	src.data = src.cpu_mapping = NULL;
+	src.size = 4 * width * height;
+	src.num_tiles = 4 * width * height;
+	dst = tmp = src;
+
+	src.bo = bo.src;
+	tmp.bo = bo.tmp;
+	dst.bo = bo.dst;
+
+	for (i = 0; i < (width * height) >> 3; i++) {
+		int x = i % width;
+		int y = i / width;
+
+		assert(y < height);
+
+		copy(batch, &src, 0, 0, width, height, &tmp, 0, 0);
+		copy(batch, &src, x, y, 1, 1, &dst, x, y);
+	}
+
+	last_bo = bo;
+}
+
+static void blt_copy(struct intel_batchbuffer *batch,
+		     struct scratch_buf *src, unsigned src_x, unsigned src_y,
+		     unsigned w, unsigned h,
+		     struct scratch_buf *dst, unsigned dst_x, unsigned dst_y)
+{
+	BEGIN_BATCH(8);
+	OUT_BATCH(XY_SRC_COPY_BLT_CMD |
+		  XY_SRC_COPY_BLT_WRITE_ALPHA |
+		  XY_SRC_COPY_BLT_WRITE_RGB);
+	OUT_BATCH((3 << 24) | /* 32 bits */
+		  (0xcc << 16) | /* copy ROP */
+		  dst->stride);
+	OUT_BATCH((dst_y << 16) | dst_x); /* dst x1,y1 */
+	OUT_BATCH(((dst_y + h) << 16) | (dst_x + w)); /* dst x2,y2 */
+	OUT_RELOC(dst->bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH((src_y << 16) | src_x); /* src x1,y1 */
+	OUT_BATCH(src->stride);
+	OUT_RELOC(src->bo, I915_GEM_DOMAIN_RENDER, 0, 0);
+	ADVANCE_BATCH();
+
+	intel_batchbuffer_flush(batch);
+}
+
+int main(int argc, char **argv)
+{
+	drm_intel_bufmgr *bufmgr;
+	struct intel_batchbuffer *batch;
+	uint64_t orig_timeout, timeout;
+	int fd, ret;
+	bool do_signals = false;
+
+	if (argc > 2) {
+		do_signals = true;
+		orig_timeout = timeout = atoi(argv[1]) * NSEC_PER_MSEC;
+	} else if (argc > 1)
+		orig_timeout = timeout = atoi(argv[1]) * NSEC_PER_MSEC;
+	else
+		orig_timeout = timeout = 150 * NSEC_PER_MSEC;
+
+	fd = drm_open_any();
+
+	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
+	drm_intel_bufmgr_gem_enable_reuse(bufmgr);
+	batch = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
+
+	if (do_signals)
+		drmtest_fork_signal_helper();
+
+	make_work(bufmgr, batch, "blt", blt_copy);
+	ret = drm_intel_gem_bo_wait(last_bo.dst, &timeout);
+	if (ret) {
+		fprintf(stderr, "Timed wait failed %s\n", strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+
+	if (timeout == 0)
+		printf("Timeout before render completed!\n");
+	else {
+		printf("Finished with %lums time remaining\n",
+			timeout / NSEC_PER_MSEC);
+		assert(timeout <= orig_timeout);
+	}
+
+	destroy_bo(&last_bo);
+	intel_batchbuffer_free(batch);
+	drm_intel_bufmgr_destroy(bufmgr);
+
+	close(fd);
+
+	if (do_signals)
+		drmtest_stop_signal_helper();
+
+	return 0;
+}
-- 
1.7.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] 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.

v2: Fix rebase error (Chris)
Return proper time even in wedged + signal case (Chris + Ben)
Use timespec constructs (Ben)
Didn't take Daniel's advice regarding the Frankenstein-ness of the
  function. I did try his advice, but in the end I liked the way the
  original code looked, better.

v3: Make wakeups far less frequent for infinite waits (Chris)

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 293f573..70634cb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1819,34 +1819,78 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+/**
+ * __wait_seqno - wait until execution of seqno has finished
+ * @ring: the ring expected to report seqno
+ * @seqno: duh!
+ * @interruptible: do an interruptible wait (normally yes)
+ * @timeout: in - how long to wait (NULL forever); out - how much time remaining
+ *
+ * Returns 0 if the seqno was found within the alloted time. Else returns the
+ * errno with remaining time filled in timeout argument.
+ */
 static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
-			bool interruptible)
+			bool interruptible, struct timespec *timeout)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
-	int ret = 0;
+	struct timespec temp, dummy_time={1,0};
+	unsigned long before, timeout_jiffies;
+	long end;
+	bool wait_forever = false;
 
 	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
 		return 0;
 
 	trace_i915_gem_request_wait_begin(ring, seqno);
+
+	if (timeout == NULL) {
+		timeout = &dummy_time;
+		wait_forever = true;
+	}
+	timeout_jiffies = timespec_to_jiffies(timeout);
+
 	if (WARN_ON(!ring->irq_get(ring)))
 		return -ENODEV;
 
+	/* Record current jiffies in case interrupted by signal, or wedged * */
+	before = jiffies;
+
 #define EXIT_COND \
 	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
 	atomic_read(&dev_priv->mm.wedged))
+	do {
+		if (interruptible)
+			end = wait_event_interruptible_timeout(ring->irq_queue,
+							       EXIT_COND,
+							       timeout_jiffies);
+		else
+			end = wait_event_timeout(ring->irq_queue, EXIT_COND,
+						 timeout_jiffies);
 
-	if (interruptible)
-		ret = wait_event_interruptible(ring->irq_queue,
-					       EXIT_COND);
-	else
-		wait_event(ring->irq_queue, EXIT_COND);
+		if (atomic_read(&dev_priv->mm.wedged))
+			end = -EAGAIN;
+	} while(end == 0 && wait_forever);
+
+	jiffies_to_timespec(jiffies - before, &temp);
 
 	ring->irq_put(ring);
 	trace_i915_gem_request_wait_end(ring, seqno);
 #undef EXIT_COND
 
-	return ret;
+	jiffies_to_timespec(end, timeout);
+
+	switch (end) {
+	case -EAGAIN: /* Wedged */
+	case -ERESTARTSYS: /* Signal */
+		WARN_ON(timespec_compare(&temp, timeout) >= 0);
+		*timeout = timespec_sub(*timeout, temp);
+		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 +1935,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 +3023,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 2/5] drm/i915: make waiting trace events more useful

[Ben Widawsky]

v2: Don't do a trace event per loop. (Chris)
Only get blocking/non-blocking info (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_trace.h |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index dac7bba..efdf322 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -311,9 +311,27 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_retire,
 	    TP_ARGS(ring, seqno)
 );
 
-DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_begin,
+TRACE_EVENT(i915_gem_request_wait_begin,
 	    TP_PROTO(struct intel_ring_buffer *ring, u32 seqno),
-	    TP_ARGS(ring, seqno)
+	    TP_ARGS(ring, seqno),
+
+	    TP_STRUCT__entry(
+			     __field(u32, dev)
+			     __field(u32, ring)
+			     __field(u32, seqno)
+			     __field(bool, blocking)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->dev = ring->dev->primary->index;
+			   __entry->ring = ring->id;
+			   __entry->seqno = seqno;
+			   __entry->blocking =  mutex_is_locked(&ring->dev->struct_mutex);
+			   ),
+
+	    TP_printk("dev=%u, ring=%u, seqno=%u, blocking=%s",
+		      __entry->dev, __entry->ring, __entry->seqno,
+		      __entry->blocking ?  "yes" : "no")
 );
 
 DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_end,
-- 
1.7.10

[PATCH 4/5] drm/i915: wait render timeout ioctl

[Ben Widawsky]

This helps implement GL_ARB_sync put stops short of allowing full blow
sync objects.

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.

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)

v3: Drop lock after getting seqno to avoid ugly dance (Chris)

v4: check for 0 timeout after olr check to allow polling (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 |   63 +++++++++++++++++++++++++++++++++++++++
 include/drm/i915_drm.h          |    8 +++++
 4 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 17081da..97a8f02 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2186,6 +2186,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 ca13098..dd8b33f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1220,6 +1220,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 8f19cc1..cf3e506 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1997,6 +1997,69 @@ 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;
+	struct timespec timeout;
+	u32 seqno = 0;
+	int ret = 0;
+
+	timeout = ns_to_timespec(args->timeout_ns);
+
+	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;
+
+	/* Do this after OLR check to make sure we make forward progress polling
+	 * on this IOCTL with a 0 timeout (like busy ioctl)
+	 */
+	if (!args->timeout_ns) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+
+	ret = __wait_seqno(ring, seqno, true, &timeout);
+	args->timeout_ns = timespec_to_ns(&timeout);
+	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

Re: [PATCH 4/5] drm/i915: wait render timeout ioctl

[Eric Anholt]

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

On Mon, 30 Apr 2012 18:41:08 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This helps implement GL_ARB_sync put stops short of allowing full blow
                                  "but"?                           "blown"?

> sync objects.
> 
> 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.

It looks like this actually lets them wait on a BO with a timeout.  I
like it as an interface, but that's not what I expected from the commit
message.  Waiting on a request sounds like passing a seqno back out to
userland that they could wait on.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: wait render timeout ioctl

[Ben Widawsky]

On Tue, 01 May 2012 10:19:53 -0700
Eric Anholt <eric@anholt.net> wrote:

> On Mon, 30 Apr 2012 18:41:08 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > This helps implement GL_ARB_sync put stops short of allowing full blow
>                                   "but"?                           "blown"?
> 
> > sync objects.
> > 
> > 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.
> 
> It looks like this actually lets them wait on a BO with a timeout.  I
> like it as an interface, but that's not what I expected from the commit
> message.  Waiting on a request sounds like passing a seqno back out to
> userland that they could wait on.

Yeah. Chris asked me to update the comments actually, and I forgot. I
will resubmit this patch with better comments in the not too distant
future.

Re: [PATCH 3/5 v2] drm/i915: extract some common olr+wedge code

[Daniel Vetter]

On Sun, Apr 29, 2012 at 03:22:32PM -0700, Ben Widawsky wrote:
> Refactor.
> 
> v2: Don't use seqno as an outparam (Chris)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next (with some paint added), thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/5] drm/i915: make waiting trace events more useful

[Daniel Vetter]

On Mon, Apr 30, 2012 at 06:40:23PM -0700, Ben Widawsky wrote:
> v2: Don't do a trace event per loop. (Chris)
> Only get blocking/non-blocking info (Chris)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_trace.h |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index dac7bba..efdf322 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -311,9 +311,27 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_retire,
>  	    TP_ARGS(ring, seqno)
>  );
>  
> -DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_begin,
> +TRACE_EVENT(i915_gem_request_wait_begin,
>  	    TP_PROTO(struct intel_ring_buffer *ring, u32 seqno),
> -	    TP_ARGS(ring, seqno)
> +	    TP_ARGS(ring, seqno),
> +
> +	    TP_STRUCT__entry(
> +			     __field(u32, dev)
> +			     __field(u32, ring)
> +			     __field(u32, seqno)
> +			     __field(bool, blocking)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->dev = ring->dev->primary->index;
> +			   __entry->ring = ring->id;
> +			   __entry->seqno = seqno;
> +			   __entry->blocking =  mutex_is_locked(&ring->dev->struct_mutex);

For consistency I guess we can ditch the dev parameter (and even then, the
ring would uniquely identify the device). Also, I guess you need to
explicitly pass in blocking, because mutex_is_locked is rather racy -
someone else could hold the mutex while we're waiting in a non-blocking
fashion.
-Daniel

> +			   ),
> +
> +	    TP_printk("dev=%u, ring=%u, seqno=%u, blocking=%s",
> +		      __entry->dev, __entry->ring, __entry->seqno,
> +		      __entry->blocking ?  "yes" : "no")
>  );
>  
>  DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_end,
> -- 
> 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 2/5] drm/i915: make waiting trace events more useful

[Chris Wilson]

On Wed, 2 May 2012 23:12:36 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> For consistency I guess we can ditch the dev parameter (and even then, the
> ring would uniquely identify the device). Also, I guess you need to
> explicitly pass in blocking, because mutex_is_locked is rather racy -
> someone else could hold the mutex while we're waiting in a non-blocking
> fashion.

Meh, I suggested the race - I'd rather have a moment of confusion
reading the trace than reading the code in 6 months time.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: wait render timeout ioctl

[Daniel Vetter]

On Mon, Apr 30, 2012 at 06:41:08PM -0700, Ben Widawsky wrote:
> This helps implement GL_ARB_sync put stops short of allowing full blow
> sync objects.
> 
> 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.
> 
> 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)
> 
> v3: Drop lock after getting seqno to avoid ugly dance (Chris)
> 
> v4: check for 0 timeout after olr check to allow polling (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 |   63 +++++++++++++++++++++++++++++++++++++++
>  include/drm/i915_drm.h          |    8 +++++
>  4 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 17081da..97a8f02 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2186,6 +2186,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 ca13098..dd8b33f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1220,6 +1220,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 8f19cc1..cf3e506 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1997,6 +1997,69 @@ 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;
> +	struct timespec timeout;
> +	u32 seqno = 0;
> +	int ret = 0;
> +
> +	timeout = ns_to_timespec(args->timeout_ns);
> +
> +	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;
> +
> +	/* Do this after OLR check to make sure we make forward progress polling
> +	 * on this IOCTL with a 0 timeout (like busy ioctl)
> +	 */
> +	if (!args->timeout_ns) {
> +		ret = -EBUSY;

This return value is a bit inconsitency with the case where we have
timeout_ns > 0 but time out. I think we should return the same -ETIME in
both cases.
-Daniel
> +		goto out;
> +	}
> +
> +	drm_gem_object_unreference(&obj->base);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	ret = __wait_seqno(ring, seqno, true, &timeout);
> +	args->timeout_ns = timespec_to_ns(&timeout);
> +	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
> 
> _______________________________________________
> 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 2/5] drm/i915: make waiting trace events more useful

[Daniel Vetter]

On Wed, May 02, 2012 at 10:22:33PM +0100, Chris Wilson wrote:
> On Wed, 2 May 2012 23:12:36 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > For consistency I guess we can ditch the dev parameter (and even then, the
> > ring would uniquely identify the device). Also, I guess you need to
> > explicitly pass in blocking, because mutex_is_locked is rather racy -
> > someone else could hold the mutex while we're waiting in a non-blocking
> > fashion.
> 
> Meh, I suggested the race - I'd rather have a moment of confusion
> reading the trace than reading the code in 6 months time.

Ok, I see the problem and agree, let's have it slightly racy ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/5] drm/i915: timeout parameter for seqno wait

[Daniel Vetter]

On Mon, Apr 30, 2012 at 06:39:58PM -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.
> 
> v2: Fix rebase error (Chris)
> Return proper time even in wedged + signal case (Chris + Ben)
> Use timespec constructs (Ben)
> Didn't take Daniel's advice regarding the Frankenstein-ness of the
>   function. I did try his advice, but in the end I liked the way the
>   original code looked, better.
> 
> v3: Make wakeups far less frequent for infinite waits (Chris)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Ok, I've looked at this again and noticed stuff like dummy_time. Add to
that that I don't like the loop which neatly papers over any missed irqs
without yelling in the logs (which is a qa disaster given our history on
snb/ivb) and all the other differences between timout-present and infinite
sleep, I vote for __wait_seqno_timeout (which would always use the
interruptible wait).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   66 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 293f573..70634cb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1819,34 +1819,78 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> +/**
> + * __wait_seqno - wait until execution of seqno has finished
> + * @ring: the ring expected to report seqno
> + * @seqno: duh!
> + * @interruptible: do an interruptible wait (normally yes)
> + * @timeout: in - how long to wait (NULL forever); out - how much time remaining
> + *
> + * Returns 0 if the seqno was found within the alloted time. Else returns the
> + * errno with remaining time filled in timeout argument.
> + */
>  static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
> -			bool interruptible)
> +			bool interruptible, struct timespec *timeout)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	int ret = 0;
> +	struct timespec temp, dummy_time={1,0};
> +	unsigned long before, timeout_jiffies;
> +	long end;
> +	bool wait_forever = false;
>  
>  	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
>  		return 0;
>  
>  	trace_i915_gem_request_wait_begin(ring, seqno);
> +
> +	if (timeout == NULL) {
> +		timeout = &dummy_time;
> +		wait_forever = true;
> +	}
> +	timeout_jiffies = timespec_to_jiffies(timeout);
> +
>  	if (WARN_ON(!ring->irq_get(ring)))
>  		return -ENODEV;
>  
> +	/* Record current jiffies in case interrupted by signal, or wedged * */
> +	before = jiffies;
> +
>  #define EXIT_COND \
>  	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
>  	atomic_read(&dev_priv->mm.wedged))
> +	do {
> +		if (interruptible)
> +			end = wait_event_interruptible_timeout(ring->irq_queue,
> +							       EXIT_COND,
> +							       timeout_jiffies);
> +		else
> +			end = wait_event_timeout(ring->irq_queue, EXIT_COND,
> +						 timeout_jiffies);
>  
> -	if (interruptible)
> -		ret = wait_event_interruptible(ring->irq_queue,
> -					       EXIT_COND);
> -	else
> -		wait_event(ring->irq_queue, EXIT_COND);
> +		if (atomic_read(&dev_priv->mm.wedged))
> +			end = -EAGAIN;
> +	} while(end == 0 && wait_forever);
> +
> +	jiffies_to_timespec(jiffies - before, &temp);
>  
>  	ring->irq_put(ring);
>  	trace_i915_gem_request_wait_end(ring, seqno);
>  #undef EXIT_COND
>  
> -	return ret;
> +	jiffies_to_timespec(end, timeout);
> +
> +	switch (end) {
> +	case -EAGAIN: /* Wedged */
> +	case -ERESTARTSYS: /* Signal */
> +		WARN_ON(timespec_compare(&temp, timeout) >= 0);
> +		*timeout = timespec_sub(*timeout, temp);
> +		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 +1935,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 +3023,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 2/5] drm/i915: make waiting trace events more useful

[Daniel Vetter]

On Wed, May 02, 2012 at 11:36:15PM +0200, Daniel Vetter wrote:
> On Wed, May 02, 2012 at 10:22:33PM +0100, Chris Wilson wrote:
> > On Wed, 2 May 2012 23:12:36 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > For consistency I guess we can ditch the dev parameter (and even then, the
> > > ring would uniquely identify the device). Also, I guess you need to
> > > explicitly pass in blocking, because mutex_is_locked is rather racy -
> > > someone else could hold the mutex while we're waiting in a non-blocking
> > > fashion.
> > 
> > Meh, I suggested the race - I'd rather have a moment of confusion
> > reading the trace than reading the code in 6 months time.
> 
> Ok, I see the problem and agree, let's have it slightly racy ...

I've forgotten to add: Ben, please add the reasons why we decided to go
with the racy variant to the commit messages, that kind of stuff is really
important (otherwise the patch simply looks buggy).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/5] drm/i915: timeout parameter for seqno wait

[Ben Widawsky]

On Wed, 2 May 2012 23:47:35 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Apr 30, 2012 at 06:39:58PM -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.
> > 
> > v2: Fix rebase error (Chris)
> > Return proper time even in wedged + signal case (Chris + Ben)
> > Use timespec constructs (Ben)
> > Didn't take Daniel's advice regarding the Frankenstein-ness of the
> >   function. I did try his advice, but in the end I liked the way the
> >   original code looked, better.
> > 
> > v3: Make wakeups far less frequent for infinite waits (Chris)
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Ok, I've looked at this again and noticed stuff like dummy_time. Add to
> that that I don't like the loop which neatly papers over any missed irqs
> without yelling in the logs (which is a qa disaster given our history on
> snb/ivb) and all the other differences between timout-present and infinite
> sleep, I vote for __wait_seqno_timeout (which would always use the
> interruptible wait).
> -Daniel
> 

I'm confused about how the loop neatly papers over IRQs. In the wait
forever case, it sleeps for a second (and we can always make it longer).
Won't the hangcheck fire long before that to report the fact that we've
missed an IRQ? Furthermore, from an end user standpoint, masking such an
event may actually be a nice thing; just saying. It seems Chris and you
both seem to agree on this masking the problem, could one of you provide
some edification and provide the missing piece for me?

Regarding the dummy_time variable - again, forgive my ignorance, but I
don't really understand why this is so awful. I concur it's not the
prettiest code, but I honestly don't understand what's so terrible about
it.

To address the separate function request. Truly I don't feel very
strongly one way or another. I've coded both before submitting the
patches and I felt the complexity of the combined function wasn't
prohibitively confusing, it's fewer LOC, and less duplicated code. I'm
also of the opinion that waiting forever is something we never actually
want to do, and so eventually the first if (timeout == NULL) can turn
into a BUG_ON eventually.

To sum up, I guess if you can give me good explanations on the first two
points, and still want the separate functions, I'll willingly acquiesce.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/5 v3] timed BO wait

[Ben Widawsky]

The updated work is below, rebased on top of dinq.
http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=wait_rendering
git://people.freedesktop.org/~bwidawsk/drm-intel wait_rendering

I'll resubmit the patches once we settle on the review comments from
patch 1.

Thanks Chris, and Daniel for the feedback so far.

On Sun, 29 Apr 2012 15:22:29 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> This adds the fixes to the patches which Daniel didn't already take. New
> patch to update trace events is here as well, otherwise they're pretty
> much just cleanups and bug fixes from the previous try.
> 
> Ben Widawsky (5):
>   drm/i915: timeout parameter for seqno wait
>   drm/i915: make waiting trace events more useful
>   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_dma.c         |    1 +
>  drivers/gpu/drm/i915/i915_drv.h         |    6 +-
>  drivers/gpu/drm/i915/i915_gem.c         |  260 ++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_trace.h       |   45 +++++-
>  drivers/gpu/drm/i915/intel_overlay.c    |    4 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
>  include/drm/i915_drm.h                  |    8 +
>  7 files changed, 238 insertions(+), 88 deletions(-)
> 



-- 
Ben Widawsky, Intel Open Source Technology Center