The hang checker needs to inspect whether or not the ring request list is empty
as well as if the given engine has reached or passed the most recently
submitted request. The problem with this is that the hang checker cannot grab
the struct_mutex, which is required in order to safely inspect requests since
requests might be deallocated during inspection. In the past we've had kernel
panics due to this very unsynchronized access in the hang checker.
One solution to this problem is to not inspect the requests directly since
we're only interested in the seqno of the most recently submitted request - not
the request itself. Instead the seqno of the most recently submitted request is
cached, which the hang checker then inspects, circumventing the issue of
synchronization from the hang checker entirely.
Signed-off-by: Tomas Elf <tomas.elf@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 24 ++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_irq.c | 14 ++++++--------
drivers/gpu/drm/i915/intel_lrc.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++
5 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a0bff41..98cd429 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2549,6 +2549,7 @@ void __i915_add_request(struct drm_i915_gem_request *request,
request->batch_obj = obj;
request->emitted_jiffies = jiffies;
+ ring->last_cached_seqno = request->seqno;
list_add_tail(&request->list, &ring->request_list);
trace_i915_gem_request_add(request);
@@ -2784,6 +2785,8 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
i915_gem_request_retire(request);
}
+
+ ring->last_cached_seqno = 0;
}
void i915_gem_restore_fences(struct drm_device *dev)
@@ -2846,6 +2849,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
*/
while (!list_empty(&ring->request_list)) {
struct drm_i915_gem_request *request;
+ u32 req_seqno;
request = list_first_entry(&ring->request_list,
struct drm_i915_gem_request,
@@ -2854,7 +2858,26 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
if (!i915_gem_request_completed(request, true))
break;
+ req_seqno = request->seqno;
i915_gem_request_retire(request);
+
+ /*
+ * It's ok to have a stale last_cached_seqno since the hang
+ * checker does the same comparison as below and reaches the
+ * same conclusion as we do here. If last_cached_seqno is stale
+ * and the engine seqno passes last_cached_seqno the engine is
+ * probably idle. Also, as long as newly queued requests
+ * update last_cached_seqno in __i915_add_request() we won't
+ * have to clear last_cached_seqno here. It's only when we
+ * reach the last request in the list that we have to clear
+ * last_cached_seqno. At that point it would not be ok to have
+ * zero last_cached_seqno but at the same time have the request
+ * queued up (pending removal) since then the hang checker
+ * would see an inconsistent combination of request list state
+ * and cached seqno.
+ */
+ if (req_seqno >= ring->last_cached_seqno)
+ ring->last_cached_seqno = 0;
}
/* Move any buffers on the active list that are no longer referenced
@@ -5209,6 +5232,7 @@ init_ring_lists(struct intel_engine_cs *ring)
{
INIT_LIST_HEAD(&ring->active_list);
INIT_LIST_HEAD(&ring->request_list);
+ ring->last_cached_seqno = 0;
}
void i915_init_vm(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a6fbe64..46ac99a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2484,18 +2484,16 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
}
-static struct drm_i915_gem_request *
-ring_last_request(struct intel_engine_cs *ring)
-{
- return list_entry(ring->request_list.prev,
- struct drm_i915_gem_request, list);
-}
-
static bool
ring_idle(struct intel_engine_cs *ring)
{
+ u32 current_seqno;
+
+ current_seqno = ring->get_seqno(ring, false);
+
return (list_empty(&ring->request_list) ||
- i915_gem_request_completed(ring_last_request(ring), false));
+ i915_seqno_passed(current_seqno,
+ ring->last_cached_seqno));
}
static bool
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d4f8b43..a913e45 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1720,6 +1720,7 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
ring->dev = dev;
INIT_LIST_HEAD(&ring->active_list);
INIT_LIST_HEAD(&ring->request_list);
+ ring->last_cached_seqno = 0;
i915_gem_batch_pool_init(dev, &ring->batch_pool);
init_waitqueue_head(&ring->irq_queue);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index bddc903..99d0489 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2029,6 +2029,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
ring->dev = dev;
INIT_LIST_HEAD(&ring->active_list);
INIT_LIST_HEAD(&ring->request_list);
+ ring->last_cached_seqno = 0;
INIT_LIST_HEAD(&ring->execlist_queue);
i915_gem_batch_pool_init(dev, &ring->batch_pool);
ringbuf->size = 32 * PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 0ea89ea..05d11e6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -339,6 +339,13 @@ struct intel_engine_cs {
* to encode the command length in the header).
*/
u32 (*get_cmd_length_mask)(u32 cmd_header);
+
+ /*
+ * Seqno of most recently added request.
+ * Used exclusively by hang checker to avoid grabbing lock
+ * while inspecting request list.
+ */
+ u32 last_cached_seqno;
};
bool intel_ring_initialized(struct intel_engine_cs *ring);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
