[PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[Mika Kuoppala]

To count context losses, add struct i915_ctx_hang_stats for
both i915_hw_context and drm_i915_file_private.
drm_i915_file_private is used when there is no context.

v2: renamed and cleaned up the struct (Chris Wilson, Ian Romanick)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 drivers/gpu/drm/i915/i915_drv.h |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fd8898c..8e628dc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1814,7 +1814,7 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
 	struct drm_i915_file_private *file_priv;
 
 	DRM_DEBUG_DRIVER("\n");
-	file_priv = kmalloc(sizeof(*file_priv), GFP_KERNEL);
+	file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
 	if (!file_priv)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eaa04a6..5f3da39 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -498,6 +498,13 @@ struct i915_hw_ppgtt {
 	void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
 };
 
+struct i915_ctx_hang_stats {
+	/* This context had batch pending when hang was declared */
+	unsigned batch_pending;
+
+	/* This context had batch active when hang was declared */
+	unsigned batch_active;
+};
 
 /* This must match up with the value previously used for execbuf2.rsvd1. */
 #define DEFAULT_CONTEXT_ID 0
@@ -508,6 +515,7 @@ struct i915_hw_context {
 	struct drm_i915_file_private *file_priv;
 	struct intel_ring_buffer *ring;
 	struct drm_i915_gem_object *obj;
+	struct i915_ctx_hang_stats hang_stats;
 };
 
 enum no_fbc_reason {
@@ -1367,6 +1375,8 @@ struct drm_i915_file_private {
 		struct list_head request_list;
 	} mm;
 	struct idr context_idr;
+
+	struct i915_ctx_hang_stats hang_stats;
 };
 
 #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
-- 
1.7.9.5

[PATCH 2/7] drm/i915: add i915_gem_context_get_hang_stats()

[Mika Kuoppala]

To get context hang statistics for specified context,
add i915_gem_context_get_hang_stats().

For arb-robustness, every context needs to have its own
hang statistics tracking. Added function will return
the user specified context statistics or in case of
default context, statistics from drm_i915_file_private.

v2: handle default context inside get_reset_state

v3: return struct pointer instead of passing it in as param
    (Chris Wilson)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |    4 ++++
 drivers/gpu/drm/i915/i915_gem_context.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5f3da39..5c15710 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1801,6 +1801,10 @@ static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
 	kref_put(&ctx->ref, i915_gem_context_free);
 }
 
+struct i915_ctx_hang_stats * __must_check
+i915_gem_context_get_hang_stats(struct intel_ring_buffer *ring,
+				struct drm_file *file,
+				u32 id);
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file);
 int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 39bcc08..f5ea3c1 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -303,6 +303,34 @@ static int context_idr_cleanup(int id, void *p, void *data)
 	return 0;
 }
 
+struct i915_ctx_hang_stats *
+i915_gem_context_get_hang_stats(struct intel_ring_buffer *ring,
+				struct drm_file *file,
+				u32 id)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct i915_hw_context *to;
+
+	if (dev_priv->hw_contexts_disabled)
+		return ERR_PTR(-ENOENT);
+
+	if (ring->id != RCS)
+		return ERR_PTR(-EINVAL);
+
+	if (file == NULL)
+		return ERR_PTR(-EINVAL);
+
+	if (id == DEFAULT_CONTEXT_ID)
+		return &file_priv->hang_stats;
+
+	to = i915_gem_context_get(file->driver_priv, id);
+	if (to == NULL)
+		return ERR_PTR(-ENOENT);
+
+	return &to->hang_stats;
+}
+
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
-- 
1.7.9.5

[PATCH 3/7] drm/i915: change i915_add_request to macro

[Mika Kuoppala]

Only execbuffer needed all the parameters on i915_add_request().
By putting __i915_add_request behind macro, all current callsites
become cleaner. Following patch will introduce a new parameter
for __i915_add_request. With this patch, only the relevant callsite
will reflect the change making commit smaller and easier to understand.

v2: _i915_add_request as function name (Chris Wilson)

v3: change name __i915_add_request and fix ordering of params (Ben Widawsky)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |    8 +++++---
 drivers/gpu/drm/i915/i915_gem.c            |   11 +++++------
 drivers/gpu/drm/i915/i915_gem_context.c    |    2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
 drivers/gpu/drm/i915/intel_overlay.c       |    4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    2 +-
 6 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5c15710..22dcff6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1745,9 +1745,11 @@ void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_idle(struct drm_device *dev);
-int i915_add_request(struct intel_ring_buffer *ring,
-		     struct drm_file *file,
-		     u32 *seqno);
+int __i915_add_request(struct intel_ring_buffer *ring,
+		       struct drm_file *file,
+		       u32 *seqno);
+#define i915_add_request(ring, seqno) \
+	__i915_add_request(ring, NULL, 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);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 58048d4..38e2087 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -959,7 +959,7 @@ i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
 
 	ret = 0;
 	if (seqno == ring->outstanding_lazy_request)
-		ret = i915_add_request(ring, NULL, NULL);
+		ret = i915_add_request(ring, NULL);
 
 	return ret;
 }
@@ -2000,10 +2000,9 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 	return 0;
 }
 
-int
-i915_add_request(struct intel_ring_buffer *ring,
-		 struct drm_file *file,
-		 u32 *out_seqno)
+int __i915_add_request(struct intel_ring_buffer *ring,
+		       struct drm_file *file,
+		       u32 *out_seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
@@ -2280,7 +2279,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	idle = true;
 	for_each_ring(ring, dev_priv, i) {
 		if (ring->gpu_caches_dirty)
-			i915_add_request(ring, NULL, NULL);
+			i915_add_request(ring, NULL);
 
 		idle &= list_empty(&ring->request_list);
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f5ea3c1..ff47145 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -455,7 +455,7 @@ static int do_switch(struct i915_hw_context *to)
 		from->obj->dirty = 1;
 		BUG_ON(from->obj->ring != ring);
 
-		ret = i915_add_request(ring, NULL, NULL);
+		ret = i915_add_request(ring, NULL);
 		if (ret) {
 			/* Too late, we've already scheduled a context switch.
 			 * Try to undo the change so that the hw state is
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c98333d..d79ac7a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -802,7 +802,7 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 	ring->gpu_caches_dirty = true;
 
 	/* Add a breadcrumb for the completion of the batch buffer */
-	(void)i915_add_request(ring, file, NULL);
+	(void)__i915_add_request(ring, file, NULL);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 836794b..a369881 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -217,7 +217,7 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	int ret;
 
 	BUG_ON(overlay->last_flip_req);
-	ret = i915_add_request(ring, NULL, &overlay->last_flip_req);
+	ret = i915_add_request(ring, &overlay->last_flip_req);
 	if (ret)
 		return ret;
 
@@ -286,7 +286,7 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
 	intel_ring_emit(ring, flip_addr);
 	intel_ring_advance(ring);
 
-	return i915_add_request(ring, NULL, &overlay->last_flip_req);
+	return i915_add_request(ring, &overlay->last_flip_req);
 }
 
 static void intel_overlay_release_old_vid_tail(struct intel_overlay *overlay)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a3cfa35..e51ab55 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1512,7 +1512,7 @@ int intel_ring_idle(struct intel_ring_buffer *ring)
 
 	/* We need to add any requests required to flush the objects and ring */
 	if (ring->outstanding_lazy_request) {
-		ret = i915_add_request(ring, NULL, NULL);
+		ret = i915_add_request(ring, NULL);
 		if (ret)
 			return ret;
 	}
-- 
1.7.9.5

[PATCH 4/7] drm/i915: add batch bo to i915_add_request()

[Mika Kuoppala]

In order to track down a batch buffer and context which
caused the ring to hang, store reference to bo into the request struct.
Request can also cause gpu to hang after the batch in the flush section
in the ring. To detect this add start of the flush portion offset into the
request.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |   11 +++++++++--
 drivers/gpu/drm/i915/i915_gem.c            |    6 +++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    7 ++++---
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 22dcff6..8bc399c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1352,12 +1352,18 @@ struct drm_i915_gem_request {
 	/** GEM sequence number associated with this request. */
 	uint32_t seqno;
 
-	/** Postion in the ringbuffer of the end of the request */
+	/** Position in the ringbuffer of the start of the request */
+	u32 head;
+
+	/** Position in the ringbuffer of the end of the request */
 	u32 tail;
 
 	/** Context related to this request */
 	struct i915_hw_context *ctx;
 
+	/** Batch buffer related to this request if any */
+	struct drm_i915_gem_object *batch_obj;
+
 	/** Time at which this request was emitted, in jiffies. */
 	unsigned long emitted_jiffies;
 
@@ -1747,9 +1753,10 @@ int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_idle(struct drm_device *dev);
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
+		       struct drm_i915_gem_object *batch_obj,
 		       u32 *seqno);
 #define i915_add_request(ring, seqno) \
-	__i915_add_request(ring, NULL, seqno);
+	__i915_add_request(ring, NULL, NULL, 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);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 38e2087..5be7846 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2002,14 +2002,16 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
+		       struct drm_i915_gem_object *obj,
 		       u32 *out_seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
-	u32 request_ring_position;
+	u32 request_ring_position, request_start;
 	int was_empty;
 	int ret;
 
+	request_start = intel_ring_get_tail(ring);
 	/*
 	 * Emit any outstanding flushes - execbuf can fail to emit the flush
 	 * after having emitted the batchbuffer command. Hence we need to fix
@@ -2041,8 +2043,10 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 
 	request->seqno = intel_ring_get_seqno(ring);
 	request->ring = ring;
+	request->head = request_start;
 	request->tail = request_ring_position;
 	request->ctx = ring->last_context;
+	request->batch_obj = obj;
 
 	if (request->ctx)
 		i915_gem_context_reference(request->ctx);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d79ac7a..87a3227 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -796,13 +796,14 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
 static void
 i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 				    struct drm_file *file,
-				    struct intel_ring_buffer *ring)
+				    struct intel_ring_buffer *ring,
+				    struct drm_i915_gem_object *obj)
 {
 	/* Unconditionally force add_request to emit a full flush. */
 	ring->gpu_caches_dirty = true;
 
 	/* Add a breadcrumb for the completion of the batch buffer */
-	(void)__i915_add_request(ring, file, NULL);
+	(void)__i915_add_request(ring, file, obj, NULL);
 }
 
 static int
@@ -1083,7 +1084,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
 
 	i915_gem_execbuffer_move_to_active(&eb->objects, ring);
-	i915_gem_execbuffer_retire_commands(dev, file, ring);
+	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
 
 err:
 	eb_destroy(eb);
-- 
1.7.9.5

[PATCH 5/7] drm/i915: store ring hangcheck action

[Mika Kuoppala]

For guilty batchbuffer analysis later on when rings are reset,
store what state the ring was on when hang was declared.
This helps to weed out the waiting rings from the active ones.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c         |    8 ++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b26243f..208e675 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2380,7 +2380,8 @@ static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv)
 		ring->hangcheck.deadlock = false;
 }
 
-static enum { wait, active, kick, hung } ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
+static enum intel_ring_hangcheck_action
+ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2483,7 +2484,10 @@ void i915_hangcheck_elapsed(unsigned long data)
 				 * being repeatedly kicked and so responsible
 				 * for stalling the machine.
 				 */
-				switch (ring_stuck(ring, acthd)) {
+				ring->hangcheck.action = ring_stuck(ring,
+								    acthd);
+
+				switch (ring->hangcheck.action) {
 				case wait:
 					score = 0;
 					break;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a3e9610..799f04c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -37,11 +37,14 @@ struct  intel_hw_status_page {
 #define I915_READ_SYNC_0(ring) I915_READ(RING_SYNC_0((ring)->mmio_base))
 #define I915_READ_SYNC_1(ring) I915_READ(RING_SYNC_1((ring)->mmio_base))
 
+enum intel_ring_hangcheck_action { wait, active, kick, hung };
+
 struct intel_ring_hangcheck {
 	bool deadlock;
 	u32 seqno;
 	u32 acthd;
 	int score;
+	enum intel_ring_hangcheck_action action;
 };
 
 struct  intel_ring_buffer {
-- 
1.7.9.5

[PATCH 6/7] drm/i915: find guilty batch buffer on ring resets

[Mika Kuoppala]

After hang check timer has declared gpu to be hang,
rings are reset. In ring reset, when clearing
request list, do post mortem analysis to find out
the guilty batch buffer.

Select requests for further analysis by inspecting
the completed sequence number which has been updated
into the HWS page. If request was completed, it can't
be related to the hang.

For noncompleted requests mark the batch as guilty
if the ring was not waiting and the ring head was
stuck inside the buffer object or in the flush region
right after the batch. For everything else, mark
them as innocents.

v2: Fixed a typo in commit message (Ville Syrjälä)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   88 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5be7846..6144f0b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2103,6 +2103,85 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
 	spin_unlock(&file_priv->mm.lock);
 }
 
+static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object *obj)
+{
+	if (acthd >= obj->gtt_offset &&
+	    acthd < obj->gtt_offset + obj->base.size)
+		return true;
+
+	return false;
+}
+
+static bool i915_head_inside_request(u32 acthd, u32 rs, u32 re)
+{
+	if (rs < re) {
+		if (acthd >= rs && acthd < re)
+			return true;
+	} else if (rs > re) {
+		if (acthd >= rs || acthd < re)
+			return true;
+	}
+
+	return false;
+}
+
+static bool i915_request_guilty(struct drm_i915_gem_request *request,
+				const u32 acthd, bool *inside)
+{
+	if (request->batch_obj) {
+		if (i915_head_inside_object(acthd, request->batch_obj)) {
+			*inside = true;
+			return true;
+		}
+	}
+
+	if (i915_head_inside_request(acthd, request->head, request->tail)) {
+		*inside = false;
+		return true;
+	}
+
+	return false;
+}
+
+static void i915_set_reset_status(struct intel_ring_buffer *ring,
+				  struct drm_i915_gem_request *request,
+				  u32 acthd)
+{
+	struct i915_ctx_hang_stats *hs = NULL;
+	bool inside, guilty;
+
+	/* Innocent until proven guilty */
+	guilty = false;
+
+	if (ring->hangcheck.last_action != wait &&
+	    i915_request_guilty(request, acthd, &inside)) {
+		DRM_ERROR("%s hung %s bo (0x%x ctx %d) at 0x%x\n",
+			  ring->name,
+			  inside ? "inside" : "flushing",
+			  request->batch_obj ?
+			  request->batch_obj->gtt_offset : 0,
+			  request->ctx ? request->ctx->id : 0,
+			  acthd);
+
+		guilty = true;
+	}
+
+	/* If contexts are disabled or this is the default context, use
+	 * file_priv->reset_state
+	 */
+	if (request->ctx && request->ctx->id != DEFAULT_CONTEXT_ID)
+		hs = &request->ctx->hang_stats;
+	else if (request->file_priv)
+		hs = &request->file_priv->hang_stats;
+
+	if (hs) {
+		if (guilty)
+			hs->batch_active++;
+		else
+			hs->batch_pending++;
+	}
+}
+
 static void i915_gem_free_request(struct drm_i915_gem_request *request)
 {
 	list_del(&request->list);
@@ -2117,6 +2196,12 @@ static void i915_gem_free_request(struct drm_i915_gem_request *request)
 static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 				      struct intel_ring_buffer *ring)
 {
+	u32 completed_seqno;
+	u32 acthd;
+
+	acthd = intel_ring_get_active_head(ring);
+	completed_seqno = ring->get_seqno(ring, false);
+
 	while (!list_empty(&ring->request_list)) {
 		struct drm_i915_gem_request *request;
 
@@ -2124,6 +2209,9 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 					   struct drm_i915_gem_request,
 					   list);
 
+		if (request->seqno > completed_seqno)
+			i915_set_reset_status(ring, request, acthd);
+
 		i915_gem_free_request(request);
 	}
 
-- 
1.7.9.5

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

[PATCH 7/7] drm/i915: refuse to submit more batchbuffers from guilty context

[Mika Kuoppala]

If context has recently submitted a faulty batchbuffers guilty of
gpu hang and decides to keep submitting more crap, ban it permanently.

v2: Store guilty ban status bool in gpu_error instead of pointers
    that might become danling before hang is declared.

v3: Use return value for banned status instead of stashing state
    into gpu_error (Chris Wilson)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |    6 +++--
 drivers/gpu/drm/i915/i915_drv.h            |    8 ++++++-
 drivers/gpu/drm/i915/i915_gem.c            |   34 ++++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   13 +++++++++++
 4 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c3e4f29..70b64fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -867,6 +867,7 @@ int i915_reset(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	bool simulated;
+	bool ctx_banned;
 	int ret;
 
 	if (!i915_try_reset)
@@ -874,11 +875,12 @@ int i915_reset(struct drm_device *dev)
 
 	mutex_lock(&dev->struct_mutex);
 
-	i915_gem_reset(dev);
+	ctx_banned = i915_gem_reset(dev);
 
 	simulated = dev_priv->gpu_error.stop_rings != 0;
 
-	if (!simulated && get_seconds() - dev_priv->gpu_error.last_reset < 5) {
+	if (!(simulated || ctx_banned) &&
+	    get_seconds() - dev_priv->gpu_error.last_reset < 5) {
 		DRM_ERROR("GPU hanging too fast, declaring wedged!\n");
 		ret = -ENODEV;
 	} else {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8bc399c..364afff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -504,6 +504,12 @@ struct i915_ctx_hang_stats {
 
 	/* This context had batch active when hang was declared */
 	unsigned batch_active;
+
+	/* Time when this context was last blamed for a GPU reset */
+	unsigned long batch_active_reset_ts;
+
+	/* This context is banned to submit more work */
+	bool banned;
 };
 
 /* This must match up with the value previously used for execbuf2.rsvd1. */
@@ -1738,7 +1744,7 @@ static inline bool i915_terminally_wedged(struct i915_gpu_error *error)
 	return atomic_read(&error->reset_counter) == I915_WEDGED;
 }
 
-void i915_gem_reset(struct drm_device *dev);
+bool i915_gem_reset(struct drm_device *dev);
 void i915_gem_clflush_object(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
 					    uint32_t read_domains,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6144f0b..3ecf1fe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2143,15 +2143,15 @@ static bool i915_request_guilty(struct drm_i915_gem_request *request,
 	return false;
 }
 
-static void i915_set_reset_status(struct intel_ring_buffer *ring,
+static bool i915_set_reset_status(struct intel_ring_buffer *ring,
 				  struct drm_i915_gem_request *request,
 				  u32 acthd)
 {
 	struct i915_ctx_hang_stats *hs = NULL;
-	bool inside, guilty;
+	bool inside, guilty, banned;
 
 	/* Innocent until proven guilty */
-	guilty = false;
+	guilty = banned = false;
 
 	if (ring->hangcheck.last_action != wait &&
 	    i915_request_guilty(request, acthd, &inside)) {
@@ -2175,11 +2175,20 @@ static void i915_set_reset_status(struct intel_ring_buffer *ring,
 		hs = &request->file_priv->hang_stats;
 
 	if (hs) {
-		if (guilty)
+		if (guilty) {
+			if (!hs->banned &&
+			    get_seconds() - hs->batch_active_reset_ts < 15) {
+				hs->banned = banned = true;
+				DRM_ERROR("context hanging too fast, declaring banned\n");
+			}
 			hs->batch_active++;
-		else
+			hs->batch_active_reset_ts = get_seconds();
+		} else {
 			hs->batch_pending++;
+		}
 	}
+
+	return banned;
 }
 
 static void i915_gem_free_request(struct drm_i915_gem_request *request)
@@ -2193,11 +2202,12 @@ static void i915_gem_free_request(struct drm_i915_gem_request *request)
 	kfree(request);
 }
 
-static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
+static bool i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 				      struct intel_ring_buffer *ring)
 {
 	u32 completed_seqno;
 	u32 acthd;
+	bool ctx_banned = false;
 
 	acthd = intel_ring_get_active_head(ring);
 	completed_seqno = ring->get_seqno(ring, false);
@@ -2210,7 +2220,8 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 					   list);
 
 		if (request->seqno > completed_seqno)
-			i915_set_reset_status(ring, request, acthd);
+			ctx_banned |= i915_set_reset_status(ring,
+							    request, acthd);
 
 		i915_gem_free_request(request);
 	}
@@ -2224,6 +2235,8 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 
 		i915_gem_object_move_to_inactive(obj);
 	}
+
+	return ctx_banned;
 }
 
 static void i915_gem_reset_fences(struct drm_device *dev)
@@ -2247,15 +2260,16 @@ static void i915_gem_reset_fences(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 }
 
-void i915_gem_reset(struct drm_device *dev)
+bool i915_gem_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct intel_ring_buffer *ring;
 	int i;
+	bool ctx_banned = false;
 
 	for_each_ring(ring, dev_priv, i)
-		i915_gem_reset_ring_lists(dev_priv, ring);
+		ctx_banned |= i915_gem_reset_ring_lists(dev_priv, ring);
 
 	/* Move everything out of the GPU domains to ensure we do any
 	 * necessary invalidation upon reuse.
@@ -2269,6 +2283,8 @@ void i915_gem_reset(struct drm_device *dev)
 
 	/* The fence registers are invalidated so clear them out */
 	i915_gem_reset_fences(dev);
+
+	return ctx_banned;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 87a3227..7fcd6c0 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -842,6 +842,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *batch_obj;
 	struct drm_clip_rect *cliprects = NULL;
 	struct intel_ring_buffer *ring;
+	struct i915_ctx_hang_stats *hs;
 	u32 ctx_id = i915_execbuffer2_get_context_id(*args);
 	u32 exec_start, exec_len;
 	u32 mask, flags;
@@ -1033,6 +1034,18 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto err;
 
+	hs = i915_gem_context_get_hang_stats(&dev_priv->ring[RCS],
+					     file, ctx_id);
+	if (IS_ERR(hs)) {
+		ret = PTR_ERR(hs);
+		goto err;
+	}
+
+	if (hs->banned) {
+		ret = -EIO;
+		goto err;
+	}
+
 	ret = i915_switch_context(ring, file, ctx_id);
 	if (ret)
 		goto err;
-- 
1.7.9.5

Re: [PATCH 4/7] drm/i915: add batch bo to i915_add_request()

[Chris Wilson]

On Wed, Jun 12, 2013 at 12:35:31PM +0300, Mika Kuoppala wrote:
> In order to track down a batch buffer and context which
> caused the ring to hang, store reference to bo into the request struct.
> Request can also cause gpu to hang after the batch in the flush section
> in the ring. To detect this add start of the flush portion offset into the
> request.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> @@ -2041,8 +2043,10 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>  
>  	request->seqno = intel_ring_get_seqno(ring);
>  	request->ring = ring;
> +	request->head = request_start;
>  	request->tail = request_ring_position;
>  	request->ctx = ring->last_context;
> +	request->batch_obj = obj;

This could do with a comment explaining the lifetimes of the request vs
batch_obj, and so justifying why we do not need a reference count.

/* Whilst this request exists, batch_obj will be on the
 * active_list, and so will hold the active reference. Only when this
 * request is retired will the the batch_obj be moved onto the
 * inactive_list and lose its active reference. Hence we do not need
 * to explicitly hold another reference here.
 */
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7] drm/i915: find guilty batch buffer on ring resets

[Chris Wilson]

On Wed, Jun 12, 2013 at 12:35:33PM +0300, Mika Kuoppala wrote:
> After hang check timer has declared gpu to be hang,
s/hang/hung/ :-p

> rings are reset. In ring reset, when clearing
> request list, do post mortem analysis to find out
> the guilty batch buffer.
> 
> Select requests for further analysis by inspecting
> the completed sequence number which has been updated
> into the HWS page. If request was completed, it can't
> be related to the hang.
> 
> For noncompleted requests mark the batch as guilty
> if the ring was not waiting and the ring head was
> stuck inside the buffer object or in the flush region
> right after the batch. For everything else, mark
> them as innocents.
> 
> v2: Fixed a typo in commit message (Ville Syrjälä)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---

> +static bool i915_head_inside_request(u32 acthd, u32 rs, u32 re)
> +{

Be kind to your reader and use request_start, request_end or just start,
end.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[Chris Wilson]

On Wed, Jun 12, 2013 at 12:35:28PM +0300, Mika Kuoppala wrote:
> To count context losses, add struct i915_ctx_hang_stats for
> both i915_hw_context and drm_i915_file_private.
> drm_i915_file_private is used when there is no context.
> 
> v2: renamed and cleaned up the struct (Chris Wilson, Ian Romanick)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Only minor bikesheds on the series, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/7] drm/i915: add batch bo to i915_add_request()

[Mika Kuoppala]

In order to track down a batch buffer and context which
caused the ring to hang, store reference to bo into the request struct.
Request can also cause gpu to hang after the batch in the flush section
in the ring. To detect this add start of the flush portion offset into the
request.

v2: Included comment about request vs batch_obj lifetimes (Chris Wilson)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |   11 +++++++++--
 drivers/gpu/drm/i915/i915_gem.c            |   13 ++++++++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    7 ++++---
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 22dcff6..8bc399c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1352,12 +1352,18 @@ struct drm_i915_gem_request {
 	/** GEM sequence number associated with this request. */
 	uint32_t seqno;
 
-	/** Postion in the ringbuffer of the end of the request */
+	/** Position in the ringbuffer of the start of the request */
+	u32 head;
+
+	/** Position in the ringbuffer of the end of the request */
 	u32 tail;
 
 	/** Context related to this request */
 	struct i915_hw_context *ctx;
 
+	/** Batch buffer related to this request if any */
+	struct drm_i915_gem_object *batch_obj;
+
 	/** Time at which this request was emitted, in jiffies. */
 	unsigned long emitted_jiffies;
 
@@ -1747,9 +1753,10 @@ int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_idle(struct drm_device *dev);
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
+		       struct drm_i915_gem_object *batch_obj,
 		       u32 *seqno);
 #define i915_add_request(ring, seqno) \
-	__i915_add_request(ring, NULL, seqno);
+	__i915_add_request(ring, NULL, NULL, 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);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 38e2087..dc32fae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2002,14 +2002,16 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
+		       struct drm_i915_gem_object *obj,
 		       u32 *out_seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
-	u32 request_ring_position;
+	u32 request_ring_position, request_start;
 	int was_empty;
 	int ret;
 
+	request_start = intel_ring_get_tail(ring);
 	/*
 	 * Emit any outstanding flushes - execbuf can fail to emit the flush
 	 * after having emitted the batchbuffer command. Hence we need to fix
@@ -2041,8 +2043,17 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 
 	request->seqno = intel_ring_get_seqno(ring);
 	request->ring = ring;
+	request->head = request_start;
 	request->tail = request_ring_position;
 	request->ctx = ring->last_context;
+	request->batch_obj = obj;
+
+	/* Whilst this request exists, batch_obj will be on the
+	 * active_list, and so will hold the active reference. Only when this
+	 * request is retired will the the batch_obj be moved onto the
+	 * inactive_list and lose its active reference. Hence we do not need
+	 * to explicitly hold another reference here.
+	 */
 
 	if (request->ctx)
 		i915_gem_context_reference(request->ctx);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d79ac7a..87a3227 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -796,13 +796,14 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
 static void
 i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 				    struct drm_file *file,
-				    struct intel_ring_buffer *ring)
+				    struct intel_ring_buffer *ring,
+				    struct drm_i915_gem_object *obj)
 {
 	/* Unconditionally force add_request to emit a full flush. */
 	ring->gpu_caches_dirty = true;
 
 	/* Add a breadcrumb for the completion of the batch buffer */
-	(void)__i915_add_request(ring, file, NULL);
+	(void)__i915_add_request(ring, file, obj, NULL);
 }
 
 static int
@@ -1083,7 +1084,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
 
 	i915_gem_execbuffer_move_to_active(&eb->objects, ring);
-	i915_gem_execbuffer_retire_commands(dev, file, ring);
+	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
 
 err:
 	eb_destroy(eb);
-- 
1.7.9.5

[PATCH 6/7] drm/i915: find guilty batch buffer on ring resets

[Mika Kuoppala]

After hang check timer has declared gpu to be hung,
rings are reset. In ring reset, when clearing
request list, do post mortem analysis to find out
the guilty batch buffer.

Select requests for further analysis by inspecting
the completed sequence number which has been updated
into the HWS page. If request was completed, it can't
be related to the hang.

For noncompleted requests mark the batch as guilty
if the ring was not waiting and the ring head was
stuck inside the buffer object or in the flush region
right after the batch. For everything else, mark
them as innocents.

v2: Fixed a typo in commit message (Ville Syrjälä)

v3: - more descriptive function parameters (Chris Wilson)
    - use masked head address when inspecting if request is in ring
    - s/hangcheck.last_action/hangcheck.action
    - added comment about unmasked head hitting batch_obj range

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   97 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dc32fae..9b20fbb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2110,6 +2110,94 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
 	spin_unlock(&file_priv->mm.lock);
 }
 
+static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object *obj)
+{
+	if (acthd >= obj->gtt_offset &&
+	    acthd < obj->gtt_offset + obj->base.size)
+		return true;
+
+	return false;
+}
+
+static bool i915_head_inside_request(const u32 acthd_unmasked,
+				     const u32 request_start,
+				     const u32 request_end)
+{
+	const u32 acthd = acthd_unmasked & HEAD_ADDR;
+
+	if (request_start < request_end) {
+		if (acthd >= request_start && acthd < request_end)
+			return true;
+	} else if (request_start > request_end) {
+		if (acthd >= request_start || acthd < request_end)
+			return true;
+	}
+
+	return false;
+}
+
+static bool i915_request_guilty(struct drm_i915_gem_request *request,
+				const u32 acthd, bool *inside)
+{
+	/* There is a possibility that unmasked head address
+	 * pointing inside the ring, matches the batch_obj address range.
+	 * However this is extremely unlikely.
+	 */
+
+	if (request->batch_obj) {
+		if (i915_head_inside_object(acthd, request->batch_obj)) {
+			*inside = true;
+			return true;
+		}
+	}
+
+	if (i915_head_inside_request(acthd, request->head, request->tail)) {
+		*inside = false;
+		return true;
+	}
+
+	return false;
+}
+
+static void i915_set_reset_status(struct intel_ring_buffer *ring,
+				  struct drm_i915_gem_request *request,
+				  u32 acthd)
+{
+	struct i915_ctx_hang_stats *hs = NULL;
+	bool inside, guilty;
+
+	/* Innocent until proven guilty */
+	guilty = false;
+
+	if (ring->hangcheck.action != wait &&
+	    i915_request_guilty(request, acthd, &inside)) {
+		DRM_ERROR("%s hung %s bo (0x%x ctx %d) at 0x%x\n",
+			  ring->name,
+			  inside ? "inside" : "flushing",
+			  request->batch_obj ?
+			  request->batch_obj->gtt_offset : 0,
+			  request->ctx ? request->ctx->id : 0,
+			  acthd);
+
+		guilty = true;
+	}
+
+	/* If contexts are disabled or this is the default context, use
+	 * file_priv->reset_state
+	 */
+	if (request->ctx && request->ctx->id != DEFAULT_CONTEXT_ID)
+		hs = &request->ctx->hang_stats;
+	else if (request->file_priv)
+		hs = &request->file_priv->hang_stats;
+
+	if (hs) {
+		if (guilty)
+			hs->batch_active++;
+		else
+			hs->batch_pending++;
+	}
+}
+
 static void i915_gem_free_request(struct drm_i915_gem_request *request)
 {
 	list_del(&request->list);
@@ -2124,6 +2212,12 @@ static void i915_gem_free_request(struct drm_i915_gem_request *request)
 static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 				      struct intel_ring_buffer *ring)
 {
+	u32 completed_seqno;
+	u32 acthd;
+
+	acthd = intel_ring_get_active_head(ring);
+	completed_seqno = ring->get_seqno(ring, false);
+
 	while (!list_empty(&ring->request_list)) {
 		struct drm_i915_gem_request *request;
 
@@ -2131,6 +2225,9 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 					   struct drm_i915_gem_request,
 					   list);
 
+		if (request->seqno > completed_seqno)
+			i915_set_reset_status(ring, request, acthd);
+
 		i915_gem_free_request(request);
 	}
 
-- 
1.7.9.5

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

Re: [PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[Ben Widawsky]

On Wed, Jun 12, 2013 at 12:35:28PM +0300, Mika Kuoppala wrote:
> To count context losses, add struct i915_ctx_hang_stats for
> both i915_hw_context and drm_i915_file_private.
> drm_i915_file_private is used when there is no context.
> 
> v2: renamed and cleaned up the struct (Chris Wilson, Ian Romanick)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
I don't have time to do a proper review before Daniel wants to merge
these, and Chris has already reviewed it.

1-6 are:
Acked-by: Ben Widawsky <ben@bwidawsk.net>

I don't really like the behavior of 7. At least, I'd like to make it
something that can be disabled via debugfs, sysfs, or module parameter.
(I'd very much prefer it to be opt-in also).  TBH , I only read it very
fast, and I'm not horribly opposed to it, just a bunch of complexity for
IMO little gain. Presumably the problem it's trying to solve should be
fixed with a fix to ddx, mesa, libva, client, whatever.  In the embedded
case, the same thing applies.  Banning the guilty doesn't make the user
experience any better. So the only thing I see is DoS, but we've never
*really* made that our priority anyway, so, meh.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[Chris Wilson]

On Wed, Jun 12, 2013 at 02:13:26PM -0700, Ben Widawsky wrote:
> On Wed, Jun 12, 2013 at 12:35:28PM +0300, Mika Kuoppala wrote:
> > To count context losses, add struct i915_ctx_hang_stats for
> > both i915_hw_context and drm_i915_file_private.
> > drm_i915_file_private is used when there is no context.
> > 
> > v2: renamed and cleaned up the struct (Chris Wilson, Ian Romanick)
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> I don't have time to do a proper review before Daniel wants to merge
> these, and Chris has already reviewed it.
> 
> 1-6 are:
> Acked-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I don't really like the behavior of 7. At least, I'd like to make it
> something that can be disabled via debugfs, sysfs, or module parameter.
> (I'd very much prefer it to be opt-in also).  TBH , I only read it very
> fast, and I'm not horribly opposed to it, just a bunch of complexity for
> IMO little gain. Presumably the problem it's trying to solve should be
> fixed with a fix to ddx, mesa, libva, client, whatever.  In the embedded
> case, the same thing applies.  Banning the guilty doesn't make the user
> experience any better. So the only thing I see is DoS, but we've never
> *really* made that our priority anyway, so, meh.

Right, it is policy. But it is existing policy. Ultimately we want to
get as much of that decision out of the kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[Daniel Vetter]

On Wed, Jun 12, 2013 at 11:44:51PM +0100, Chris Wilson wrote:
> On Wed, Jun 12, 2013 at 02:13:26PM -0700, Ben Widawsky wrote:
> > On Wed, Jun 12, 2013 at 12:35:28PM +0300, Mika Kuoppala wrote:
> > > To count context losses, add struct i915_ctx_hang_stats for
> > > both i915_hw_context and drm_i915_file_private.
> > > drm_i915_file_private is used when there is no context.
> > > 
> > > v2: renamed and cleaned up the struct (Chris Wilson, Ian Romanick)
> > > 
> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > >
> > I don't have time to do a proper review before Daniel wants to merge
> > these, and Chris has already reviewed it.
> > 
> > 1-6 are:
> > Acked-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > I don't really like the behavior of 7. At least, I'd like to make it
> > something that can be disabled via debugfs, sysfs, or module parameter.
> > (I'd very much prefer it to be opt-in also).  TBH , I only read it very
> > fast, and I'm not horribly opposed to it, just a bunch of complexity for
> > IMO little gain. Presumably the problem it's trying to solve should be
> > fixed with a fix to ddx, mesa, libva, client, whatever.  In the embedded
> > case, the same thing applies.  Banning the guilty doesn't make the user
> > experience any better. So the only thing I see is DoS, but we've never
> > *really* made that our priority anyway, so, meh.
> 
> Right, it is policy. But it is existing policy. Ultimately we want to
> get as much of that decision out of the kernel.

Merged the entire series with a little note added to this patch explaining
the justification for it. Thanks for patches and review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/7] drm/i915: add struct i915_ctx_hang_stats

[Daniel Vetter]

On Thu, Jun 13, 2013 at 11:53:13AM +0200, Daniel Vetter wrote:
> On Wed, Jun 12, 2013 at 11:44:51PM +0100, Chris Wilson wrote:
> > On Wed, Jun 12, 2013 at 02:13:26PM -0700, Ben Widawsky wrote:
> > > On Wed, Jun 12, 2013 at 12:35:28PM +0300, Mika Kuoppala wrote:
> > > > To count context losses, add struct i915_ctx_hang_stats for
> > > > both i915_hw_context and drm_i915_file_private.
> > > > drm_i915_file_private is used when there is no context.
> > > > 
> > > > v2: renamed and cleaned up the struct (Chris Wilson, Ian Romanick)
> > > > 
> > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > > >
> > > I don't have time to do a proper review before Daniel wants to merge
> > > these, and Chris has already reviewed it.
> > > 
> > > 1-6 are:
> > > Acked-by: Ben Widawsky <ben@bwidawsk.net>
> > > 
> > > I don't really like the behavior of 7. At least, I'd like to make it
> > > something that can be disabled via debugfs, sysfs, or module parameter.
> > > (I'd very much prefer it to be opt-in also).  TBH , I only read it very
> > > fast, and I'm not horribly opposed to it, just a bunch of complexity for
> > > IMO little gain. Presumably the problem it's trying to solve should be
> > > fixed with a fix to ddx, mesa, libva, client, whatever.  In the embedded
> > > case, the same thing applies.  Banning the guilty doesn't make the user
> > > experience any better. So the only thing I see is DoS, but we've never
> > > *really* made that our priority anyway, so, meh.
> > 
> > Right, it is policy. But it is existing policy. Ultimately we want to
> > get as much of that decision out of the kernel.
> 
> Merged the entire series with a little note added to this patch explaining
> the justification for it. Thanks for patches and review.

I think a careful review of the locking would be good here. I see a few
cases:
- dev_priv->hangcheck stats which are only touched by the hangcheck timer
  ever. Safe since the hangcheck timer is non-reentrant, but would be good
  to add a comment to those things.
- Data shared between hangcheck and reset work. Probably needs spinlock
  protection, I'd add a new one. I know that it's rather unlikely that
  we'll race, but if we e.g. increase the hangcheck rate a lot we might
  fire the next hangcheck before the reset work has completed. Or the
  scheduler could be really annoying and not give the work cpu time for a
  while.
- Guilty context stats in the context structs. Atm this is protected by
  dev->struct_mutex I think, but I'd like to not proliferate the use of
  that lock if possible (there's _way_ too much legacy bagadge attached to
  this thing). So I'd vote for the introduction of a new mutex.
- Anyting else I've missed in my quick review.

I think a patch for each class of data explaining how it's accessed and
how it's protected exactly (both in the commit message and with a short
comment in the headers) would be really fine.

Volunteered?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch