[Intel-gfx] [PATCH 1/6] drm/i915: Remove 'prefault_disable' modparam

[Intel-gfx] [PATCH 1/6] drm/i915: Remove 'prefault_disable' modparam

[Chris Wilson]

The 'prefault_disable' modparam was used by IGT to prevent a few
prefaulting operations to make fault handling under struct_mutex more
prominent. With the removal of struct_mutex, this is not as important
any more and we have almost completely stopped using the parameter. The
remaining use in execbuf is now immaterial and can be dropped without
affecting coverage.

We must re-address the idea of fault injection though.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 ---
 drivers/gpu/drm/i915/i915_params.c             | 4 ----
 drivers/gpu/drm/i915/i915_params.h             | 1 -
 3 files changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 60c984e10c4a..358141e1593c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1643,9 +1643,6 @@ static int eb_prefault_relocations(const struct i915_execbuffer *eb)
 	const unsigned int count = eb->buffer_count;
 	unsigned int i;
 
-	if (unlikely(i915_modparams.prefault_disable))
-		return 0;
-
 	for (i = 0; i < count; i++) {
 		int err;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 9c8257cf88d0..add00ec1f787 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -103,10 +103,6 @@ i915_param_named(fastboot, int, 0600,
 	"(0=disabled, 1=enabled) "
 	"Default: -1 (use per-chip default)");
 
-i915_param_named_unsafe(prefault_disable, bool, 0600,
-	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
-	"For developers only.");
-
 i915_param_named_unsafe(load_detect_test, bool, 0600,
 	"Force-enable the VGA load detect code for testing (default:false). "
 	"For developers only.");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index ef4069645cb8..cb16410c2ada 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -71,7 +71,6 @@ struct drm_printer;
 	param(unsigned long, fake_lmem_start, 0, 0400) \
 	/* leave bools at the end to not create holes */ \
 	param(bool, enable_hangcheck, true, 0600) \
-	param(bool, prefault_disable, false, 0600) \
 	param(bool, load_detect_test, false, 0600) \
 	param(bool, force_reset_modeset_test, false, 0600) \
 	param(bool, error_capture, true, 0600) \
-- 
2.25.0

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

[Intel-gfx] [PATCH 2/6] drm/i915: Check current i915_vma.pin_count status first on unbind

[Chris Wilson]

Do an early rejection of a i915_vma_unbind() attempt if the i915_vma is
currently pinned, without waiting to see if the inflight operations may
unpin it. We see this problem with the shrinker trying to unbind the
active vma from inside its bind worker:

<6> [472.618968] Workqueue: events_unbound fence_work [i915]
<4> [472.618970] Call Trace:
<4> [472.618974]  ? __schedule+0x2e5/0x810
<4> [472.618978]  schedule+0x37/0xe0
<4> [472.618982]  schedule_preempt_disabled+0xf/0x20
<4> [472.618984]  __mutex_lock+0x281/0x9c0
<4> [472.618987]  ? mark_held_locks+0x49/0x70
<4> [472.618989]  ? _raw_spin_unlock_irqrestore+0x47/0x60
<4> [472.619038]  ? i915_vma_unbind+0xae/0x110 [i915]
<4> [472.619084]  ? i915_vma_unbind+0xae/0x110 [i915]
<4> [472.619122]  i915_vma_unbind+0xae/0x110 [i915]
<4> [472.619165]  i915_gem_object_unbind+0x1dc/0x400 [i915]
<4> [472.619208]  i915_gem_shrink+0x328/0x660 [i915]
<4> [472.619250]  ? i915_gem_shrink_all+0x38/0x60 [i915]
<4> [472.619282]  i915_gem_shrink_all+0x38/0x60 [i915]
<4> [472.619325]  vm_alloc_page.constprop.25+0x1aa/0x240 [i915]
<4> [472.619330]  ? rcu_read_lock_sched_held+0x4d/0x80
<4> [472.619363]  ? __alloc_pd+0xb/0x30 [i915]
<4> [472.619366]  ? module_assert_mutex_or_preempt+0xf/0x30
<4> [472.619368]  ? __module_address+0x23/0xe0
<4> [472.619371]  ? is_module_address+0x26/0x40
<4> [472.619374]  ? static_obj+0x34/0x50
<4> [472.619376]  ? lockdep_init_map+0x4d/0x1e0
<4> [472.619407]  setup_page_dma+0xd/0x90 [i915]
<4> [472.619437]  alloc_pd+0x29/0x50 [i915]
<4> [472.619470]  __gen8_ppgtt_alloc+0x443/0x6b0 [i915]
<4> [472.619503]  gen8_ppgtt_alloc+0xd7/0x300 [i915]
<4> [472.619535]  ppgtt_bind_vma+0x2a/0xe0 [i915]
<4> [472.619577]  __vma_bind+0x26/0x40 [i915]
<4> [472.619611]  fence_work+0x1c/0x90 [i915]
<4> [472.619617]  process_one_work+0x26a/0x620

Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_vma.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 84e03da0d5f9..2ffc68e18dd0 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1190,18 +1190,6 @@ int __i915_vma_unbind(struct i915_vma *vma)
 
 	lockdep_assert_held(&vma->vm->mutex);
 
-	/*
-	 * First wait upon any activity as retiring the request may
-	 * have side-effects such as unpinning or even unbinding this vma.
-	 *
-	 * XXX Actually waiting under the vm->mutex is a hinderance and
-	 * should be pipelined wherever possible. In cases where that is
-	 * unavoidable, we should lift the wait to before the mutex.
-	 */
-	ret = i915_vma_sync(vma);
-	if (ret)
-		return ret;
-
 	if (i915_vma_is_pinned(vma)) {
 		vma_print_allocator(vma, "is pinned");
 		return -EAGAIN;
@@ -1275,6 +1263,11 @@ int i915_vma_unbind(struct i915_vma *vma)
 	if (!drm_mm_node_allocated(&vma->node))
 		return 0;
 
+	if (i915_vma_is_pinned(vma)) {
+		vma_print_allocator(vma, "is pinned");
+		return -EAGAIN;
+	}
+
 	if (i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND))
 		/* XXX not always required: nop_clear_range */
 		wakeref = intel_runtime_pm_get(&vm->i915->runtime_pm);
-- 
2.25.0

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

[Intel-gfx] [PATCH 3/6] drm/i915: Tighten atomicity of i915_active_acquire vs i915_active_release

[Chris Wilson]

As we use a mutex to serialise the first acquire (as it may be a lengthy
operation), but only an atomic decrement for the release, we have to
be careful in case a second thread races and completes both
acquire/release as the first finishes its acquire.

Thread A			Thread B
i915_active_acquire		i915_active_acquire
  atomic_read() == 0		  atomic_read() == 0
  mutex_lock()			  mutex_lock()
				  atomic_read() == 0
				    ref->active();
				  atomic_inc()
				  mutex_unlock()
  atomic_read() == 1
				i915_active_release
				  atomic_dec_and_test() -> 0
				    ref->retire()
  atomic_inc() -> 1
  mutex_unlock()

So thread A has acquired the ref->active_count but since the ref was
still active at the time, it did not initialise it. By switching the
check inside the mutex to an atomic increment only if already active, we
close the race.

Fixes: c9ad602feabe ("drm/i915: Split i915_active.mutex into an irq-safe spinlock for the rbtree")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_active.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index ace55d5d4ca7..9d6830885d2e 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -416,13 +416,15 @@ int i915_active_acquire(struct i915_active *ref)
 	if (err)
 		return err;
 
-	if (!atomic_read(&ref->count) && ref->active)
-		err = ref->active(ref);
-	if (!err) {
-		spin_lock_irq(&ref->tree_lock); /* vs __active_retire() */
-		debug_active_activate(ref);
-		atomic_inc(&ref->count);
-		spin_unlock_irq(&ref->tree_lock);
+	if (likely(!i915_active_acquire_if_busy(ref))) {
+		if (ref->active)
+			err = ref->active(ref);
+		if (!err) {
+			spin_lock_irq(&ref->tree_lock); /* __active_retire() */
+			debug_active_activate(ref);
+			atomic_inc(&ref->count);
+			spin_unlock_irq(&ref->tree_lock);
+		}
 	}
 
 	mutex_unlock(&ref->mutex);
-- 
2.25.0

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

[Intel-gfx] [PATCH 4/6] drm/i915/gt: Acquire ce->active before ce->pin_count/ce->pin_mutex

[Chris Wilson]

<0> [198.668822] gem_exec-1246    0.... 193899010us : timeline_advance: timeline_advance:387 GEM_BUG_ON(!atomic_read(&tl->pin_count))
<0> [198.668859] ---------------------------------
<4> [198.669619] ------------[ cut here ]------------
<2> [198.669621] kernel BUG at drivers/gpu/drm/i915/gt/intel_timeline.c:387!
<4> [198.669703] invalid opcode: 0000 [#1] PREEMPT SMP PTI
<4> [198.669712] CPU: 0 PID: 1246 Comm: gem_exec_create Tainted: G     U  W         5.5.0-rc6-CI-CI_DRM_7755+ #1
<4> [198.669723] Hardware name:  /NUC7i5BNB, BIOS BNKBL357.86A.0054.2017.1025.1822 10/25/2017
<4> [198.669776] RIP: 0010:timeline_advance+0x7b/0xe0 [i915]
<4> [198.669785] Code: 00 48 c7 c2 10 f1 46 a0 48 c7 c7 70 1b 32 a0 e8 bb dd e7 e0 bf 01 00 00 00 e8 d1 af e7 e0 31 f6 bf 09 00 00 00 e8 35 ef d8 e0 <0f> 0b 48 c7 c1 48 fa 49 a0 ba 84 01 00 00 48 c7 c6 10 f1 46 a0 48
<4> [198.669803] RSP: 0018:ffffc900004c3a38 EFLAGS: 00010296
<4> [198.669810] RAX: ffff888270b35140 RBX: ffff88826f32ee00 RCX: 0000000000000006
<4> [198.669818] RDX: 00000000000017c5 RSI: 0000000000000000 RDI: 0000000000000009
<4> [198.669826] RBP: ffffc900004c3a64 R08: 0000000000000000 R09: 0000000000000000
<4> [198.669834] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88826f9b5980
<4> [198.669841] R13: 0000000000000cc0 R14: ffffc900004c3dc0 R15: ffff888253610068
<4> [198.669849] FS:  00007f63e663fe40(0000) GS:ffff888276c00000(0000) knlGS:0000000000000000
<4> [198.669857] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [198.669864] CR2: 00007f171f8e39a8 CR3: 000000026b1f6005 CR4: 00000000003606f0
<4> [198.669872] Call Trace:
<4> [198.669924]  intel_timeline_get_seqno+0x12/0x40 [i915]
<4> [198.669977]  __i915_request_create+0x76/0x5a0 [i915]
<4> [198.670024]  i915_request_create+0x86/0x1c0 [i915]
<4> [198.670068]  i915_gem_do_execbuffer+0xbf2/0x2500 [i915]
<4> [198.670082]  ? __lock_acquire+0x460/0x15d0
<4> [198.670128]  i915_gem_execbuffer2_ioctl+0x11f/0x470 [i915]
<4> [198.670171]  ? i915_gem_execbuffer_ioctl+0x300/0x300 [i915]
<4> [198.670181]  drm_ioctl_kernel+0xa7/0xf0
<4> [198.670188]  drm_ioctl+0x2e1/0x390
<4> [198.670233]  ? i915_gem_execbuffer_ioctl+0x300/0x300 [i915]

Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between context pin/unpin")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_context.c | 46 ++++++++++++++-----------
 drivers/gpu/drm/i915/i915_active.h      |  6 ++++
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 23137b2a8689..57e8a051ddc2 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -67,21 +67,18 @@ static int intel_context_active_acquire(struct intel_context *ce)
 {
 	int err;
 
-	err = i915_active_acquire(&ce->active);
-	if (err)
-		return err;
+	__i915_active_acquire(&ce->active);
+
+	if (intel_context_is_barrier(ce))
+		return 0;
 
 	/* Preallocate tracking nodes */
-	if (!intel_context_is_barrier(ce)) {
-		err = i915_active_acquire_preallocate_barrier(&ce->active,
-							      ce->engine);
-		if (err) {
-			i915_active_release(&ce->active);
-			return err;
-		}
-	}
+	err = i915_active_acquire_preallocate_barrier(&ce->active,
+						      ce->engine);
+	if (err)
+		i915_active_release(&ce->active);
 
-	return 0;
+	return err;
 }
 
 static void intel_context_active_release(struct intel_context *ce)
@@ -101,13 +98,19 @@ int __intel_context_do_pin(struct intel_context *ce)
 			return err;
 	}
 
-	if (mutex_lock_interruptible(&ce->pin_mutex))
-		return -EINTR;
+	err = i915_active_acquire(&ce->active);
+	if (err)
+		return err;
+
+	if (mutex_lock_interruptible(&ce->pin_mutex)) {
+		err = -EINTR;
+		goto out_release;
+	}
 
-	if (likely(!atomic_read(&ce->pin_count))) {
+	if (likely(!atomic_add_unless(&ce->pin_count, 1, 0))) {
 		err = intel_context_active_acquire(ce);
 		if (unlikely(err))
-			goto err;
+			goto out_unlock;
 
 		err = ce->ops->pin(ce);
 		if (unlikely(err))
@@ -117,18 +120,19 @@ int __intel_context_do_pin(struct intel_context *ce)
 			 ce->ring->head, ce->ring->tail);
 
 		smp_mb__before_atomic(); /* flush pin before it is visible */
+		atomic_inc(&ce->pin_count);
 	}
 
-	atomic_inc(&ce->pin_count);
 	GEM_BUG_ON(!intel_context_is_pinned(ce)); /* no overflow! */
-
-	mutex_unlock(&ce->pin_mutex);
-	return 0;
+	GEM_BUG_ON(i915_active_is_idle(&ce->active));
+	goto out_unlock;
 
 err_active:
 	intel_context_active_release(ce);
-err:
+out_unlock:
 	mutex_unlock(&ce->pin_mutex);
+out_release:
+	i915_active_release(&ce->active);
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
index b571f675c795..51e1e854ca55 100644
--- a/drivers/gpu/drm/i915/i915_active.h
+++ b/drivers/gpu/drm/i915/i915_active.h
@@ -188,6 +188,12 @@ int i915_active_acquire(struct i915_active *ref);
 bool i915_active_acquire_if_busy(struct i915_active *ref);
 void i915_active_release(struct i915_active *ref);
 
+static inline void __i915_active_acquire(struct i915_active *ref)
+{
+	GEM_BUG_ON(!atomic_read(&ref->count));
+	atomic_inc(&ref->count);
+}
+
 static inline bool
 i915_active_is_idle(const struct i915_active *ref)
 {
-- 
2.25.0

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

[Intel-gfx] [PATCH 5/6] drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore

[Chris Wilson]

If we find ourselves waiting on a MI_SEMAPHORE_WAIT, either within the
user batch or in our own preamble, the engine raises a
GT_WAIT_ON_SEMAPHORE interrupt. We can unmask that interrupt and so
respond to a semaphore wait by yielding the timeslice, if we have
another context to yield to!

The only real complication is that the interrupt is only generated for
the start of the semaphore wait, and is asynchronous to our
process_csb() -- that is, we may not have registered the timeslice before
we see the interrupt. To ensure we don't miss a potential semaphore
blocking forward progress (e.g. selftests/live_timeslice_preempt) we mark
the interrupt and apply it to the next timeslice regardless of whether it
was active at the time.

v2: We use semaphores in preempt-to-busy, within the timeslicing
implementation itself! Ergo, when we do insert a preemption due to an
expired timeslice, the new context may start with the missed semaphore
flagged by the retired context and be yielded, ad infinitum. To avoid
this, read the context id at the time of the semaphore interrupt and
only yield if that context is still active.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  9 ++++++
 drivers/gpu/drm/i915/gt/intel_gt_irq.c       | 32 +++++++++++---------
 drivers/gpu/drm/i915/gt/intel_lrc.c          | 31 ++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h              |  5 +++
 4 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 92be41a6903c..58725024ffa4 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -156,6 +156,15 @@ struct intel_engine_execlists {
 	 */
 	struct i915_priolist default_priolist;
 
+	/**
+	 * @yield: CCID at the time of the last semaphore-wait interrupt.
+	 *
+	 * Instead of leaving a semaphore busy-spinning on an engine, we would
+	 * like to switch to another ready context, i.e. yielding the semaphore
+	 * timeslice.
+	 */
+	u32 yield;
+
 	/**
 	 * @no_priolist: priority lists disabled
 	 */
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index f796bdf1ed30..6ae64a224b02 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -24,6 +24,13 @@ cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
 {
 	bool tasklet = false;
 
+	if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {
+		WRITE_ONCE(engine->execlists.yield,
+			   ENGINE_READ_FW(engine, EXECLIST_CCID));
+		if (del_timer(&engine->execlists.timer))
+			tasklet = true;
+	}
+
 	if (iir & GT_CONTEXT_SWITCH_INTERRUPT)
 		tasklet = true;
 
@@ -210,7 +217,10 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
 
 void gen11_gt_irq_postinstall(struct intel_gt *gt)
 {
-	const u32 irqs = GT_RENDER_USER_INTERRUPT | GT_CONTEXT_SWITCH_INTERRUPT;
+	const u32 irqs =
+		GT_RENDER_USER_INTERRUPT |
+		GT_CONTEXT_SWITCH_INTERRUPT |
+		GT_WAIT_SEMAPHORE_INTERRUPT;
 	struct intel_uncore *uncore = gt->uncore;
 	const u32 dmask = irqs << 16 | irqs;
 	const u32 smask = irqs << 16;
@@ -357,21 +367,15 @@ void gen8_gt_irq_postinstall(struct intel_gt *gt)
 	struct intel_uncore *uncore = gt->uncore;
 
 	/* These are interrupts we'll toggle with the ring mask register */
+	const u32 irqs =
+		GT_RENDER_USER_INTERRUPT |
+		GT_CONTEXT_SWITCH_INTERRUPT |
+		GT_WAIT_SEMAPHORE_INTERRUPT;
 	u32 gt_interrupts[] = {
-		(GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
-		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
-		 GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT |
-		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT),
-
-		(GT_RENDER_USER_INTERRUPT << GEN8_VCS0_IRQ_SHIFT |
-		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS0_IRQ_SHIFT |
-		 GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT |
-		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT),
-
+		irqs << GEN8_RCS_IRQ_SHIFT | irqs << GEN8_BCS_IRQ_SHIFT,
+		irqs << GEN8_VCS0_IRQ_SHIFT | irqs << GEN8_VCS1_IRQ_SHIFT,
 		0,
-
-		(GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT |
-		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT)
+		irqs << GEN8_VECS_IRQ_SHIFT,
 	};
 
 	gt->pm_ier = 0x0;
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index a13a8c4b65ab..6ba5a634c6e3 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1661,7 +1661,8 @@ static void defer_active(struct intel_engine_cs *engine)
 }
 
 static bool
-need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
+need_timeslice(const struct intel_engine_cs *engine,
+	       const struct i915_request *rq)
 {
 	int hint;
 
@@ -1677,6 +1678,27 @@ need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
 	return hint >= effective_prio(rq);
 }
 
+static bool
+timeslice_expired(const struct intel_engine_cs *engine,
+		  const struct i915_request *rq)
+{
+	const struct intel_engine_execlists *el = &engine->execlists;
+
+	return (timer_expired(&el->timer) ||
+		/*
+		 * Once bitten, forever smitten!
+		 *
+		 * If the active context ever busy-waited on a semaphore,
+		 * it will be treated as a hog until the end of its timeslice.
+		 * The HW only sends an interrupt on the first miss, and we
+		 * do know if that semaphore has been signaled, or even if it
+		 * is now stuck on another semaphore. Play safe, yield if it
+		 * might be stuck -- it will be given a fresh timeslice in
+		 * the near future.
+		 */
+		upper_32_bits(rq->context->lrc_desc) == READ_ONCE(el->yield));
+}
+
 static int
 switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
 {
@@ -1692,8 +1714,7 @@ timeslice(const struct intel_engine_cs *engine)
 	return READ_ONCE(engine->props.timeslice_duration_ms);
 }
 
-static unsigned long
-active_timeslice(const struct intel_engine_cs *engine)
+static unsigned long active_timeslice(const struct intel_engine_cs *engine)
 {
 	const struct i915_request *rq = *engine->execlists.active;
 
@@ -1844,7 +1865,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 			last->context->lrc_desc |= CTX_DESC_FORCE_RESTORE;
 			last = NULL;
 		} else if (need_timeslice(engine, last) &&
-			   timer_expired(&engine->execlists.timer)) {
+			   timeslice_expired(engine, last)) {
 			ENGINE_TRACE(engine,
 				     "expired last=%llx:%lld, prio=%d, hint=%d\n",
 				     last->fence.context,
@@ -2110,6 +2131,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		}
 		clear_ports(port + 1, last_port - port);
 
+		WRITE_ONCE(execlists->yield, -1);
 		execlists_submit_ports(engine);
 		set_preempt_timeout(engine);
 	} else {
@@ -4290,6 +4312,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
 
 	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
 	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
+	engine->irq_keep_mask |= GT_WAIT_SEMAPHORE_INTERRUPT << shift;
 }
 
 static void rcs_submission_override(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b93c4c18f05c..535ce7e0dc94 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3085,6 +3085,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define GT_BSD_CS_ERROR_INTERRUPT		(1 << 15)
 #define GT_BSD_USER_INTERRUPT			(1 << 12)
 #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT_S1	(1 << 11) /* hsw+; rsvd on snb, ivb, vlv */
+#define GT_WAIT_SEMAPHORE_INTERRUPT		REG_BIT(11) /* bdw+ */
 #define GT_CONTEXT_SWITCH_INTERRUPT		(1 <<  8)
 #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT	(1 <<  5) /* !snb */
 #define GT_RENDER_PIPECTL_NOTIFY_INTERRUPT	(1 <<  4)
@@ -4036,6 +4037,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define   CCID_EN			BIT(0)
 #define   CCID_EXTENDED_STATE_RESTORE	BIT(2)
 #define   CCID_EXTENDED_STATE_SAVE	BIT(3)
+
+#define EXECLIST_STATUS(base)	_MMIO((base) + 0x234)
+#define EXECLIST_CCID(base)	_MMIO((base) + 0x238)
+
 /*
  * Notes on SNB/IVB/VLV context size:
  * - Power context is saved elsewhere (LLC or stolen)
-- 
2.25.0

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

[Intel-gfx] [PATCH 6/6] drm/i915/gt: Hook up CS_MASTER_ERROR_INTERRUPT

[Chris Wilson]

Now that we have offline error capture and can reset an engine from
inside an atomic context while also preserving the GPU state for
post-mortem analysis, it is time to handle error interrupts thrown by
the command parser.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   8 +-
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  10 ++
 drivers/gpu/drm/i915/gt/intel_gt.c           |   5 +
 drivers/gpu/drm/i915/gt/intel_gt_irq.c       |  90 +++++-----
 drivers/gpu/drm/i915/gt/intel_gt_irq.h       |   3 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c          |  54 ++++--
 drivers/gpu/drm/i915/gt/selftest_lrc.c       | 163 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_gpu_error.c        |  10 +-
 drivers/gpu/drm/i915/i915_gpu_error.h        |   1 +
 drivers/gpu/drm/i915/i915_irq.c              |  10 +-
 drivers/gpu/drm/i915/i915_reg.h              |   5 +-
 11 files changed, 272 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 9b965d1f811d..39fe9a5b4820 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1293,8 +1293,14 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
 	}
 
 	if (INTEL_GEN(dev_priv) >= 6) {
-		drm_printf(m, "\tRING_IMR: %08x\n",
+		drm_printf(m, "\tRING_IMR:   0x%08x\n",
 			   ENGINE_READ(engine, RING_IMR));
+		drm_printf(m, "\tRING_ESR:   0x%08x\n",
+			   ENGINE_READ(engine, RING_ESR));
+		drm_printf(m, "\tRING_EMR:   0x%08x\n",
+			   ENGINE_READ(engine, RING_EMR));
+		drm_printf(m, "\tRING_EIR:   0x%08x\n",
+			   ENGINE_READ(engine, RING_EIR));
 	}
 
 	addr = intel_engine_get_active_head(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 58725024ffa4..c7ea986878c3 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -165,6 +165,16 @@ struct intel_engine_execlists {
 	 */
 	u32 yield;
 
+	/**
+	 * @error_interrupt: CS Master EIR
+	 *
+	 * The CS generates an interrupt when it detects an error. We capture
+	 * the first error interrupt, record the EIR and schedule the tasklet.
+	 * In the tasklet, we process the pending CS events to ensure we have
+	 * the guilty request, and then reset the engine.
+	 */
+	u32 error_interrupt;
+
 	/**
 	 * @no_priolist: priority lists disabled
 	 */
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index da2b6e2ae692..abc445f4d7a0 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -455,6 +455,11 @@ static int __engines_record_defaults(struct intel_gt *gt)
 		if (!rq)
 			continue;
 
+		if (rq->fence.error) {
+			err = -EIO;
+			goto out;
+		}
+
 		GEM_BUG_ON(!test_bit(CONTEXT_ALLOC_BIT, &rq->context->flags));
 		state = rq->context->state;
 		if (!state)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index 6ae64a224b02..82d6422f5878 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -8,6 +8,7 @@
 
 #include "i915_drv.h"
 #include "i915_irq.h"
+#include "intel_engine_pm.h"
 #include "intel_gt.h"
 #include "intel_gt_irq.h"
 #include "intel_uncore.h"
@@ -24,6 +25,21 @@ cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
 {
 	bool tasklet = false;
 
+	if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {
+		u32 eir;
+
+		eir = ENGINE_READ(engine, RING_EIR);
+		ENGINE_TRACE(engine, "CS error: %x\n", eir);
+
+		/* Disable the error interrupt until after the reset */
+		if (likely(eir)) {
+			ENGINE_WRITE(engine, RING_EMR, ~0u);
+			ENGINE_WRITE(engine, RING_EIR, eir);
+			engine->execlists.error_interrupt = eir;
+			tasklet = true;
+		}
+	}
+
 	if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {
 		WRITE_ONCE(engine->execlists.yield,
 			   ENGINE_READ_FW(engine, EXECLIST_CCID));
@@ -218,6 +234,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
 void gen11_gt_irq_postinstall(struct intel_gt *gt)
 {
 	const u32 irqs =
+		GT_CS_MASTER_ERROR_INTERRUPT |
 		GT_RENDER_USER_INTERRUPT |
 		GT_CONTEXT_SWITCH_INTERRUPT |
 		GT_WAIT_SEMAPHORE_INTERRUPT;
@@ -289,66 +306,56 @@ void gen6_gt_irq_handler(struct intel_gt *gt, u32 gt_iir)
 
 	if (gt_iir & (GT_BLT_CS_ERROR_INTERRUPT |
 		      GT_BSD_CS_ERROR_INTERRUPT |
-		      GT_RENDER_CS_MASTER_ERROR_INTERRUPT))
+		      GT_CS_MASTER_ERROR_INTERRUPT))
 		DRM_DEBUG("Command parser error, gt_iir 0x%08x\n", gt_iir);
 
 	if (gt_iir & GT_PARITY_ERROR(gt->i915))
 		gen7_parity_error_irq_handler(gt, gt_iir);
 }
 
-void gen8_gt_irq_ack(struct intel_gt *gt, u32 master_ctl, u32 gt_iir[4])
+void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl)
 {
 	void __iomem * const regs = gt->uncore->regs;
+	u32 iir;
 
 	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
-		gt_iir[0] = raw_reg_read(regs, GEN8_GT_IIR(0));
-		if (likely(gt_iir[0]))
-			raw_reg_write(regs, GEN8_GT_IIR(0), gt_iir[0]);
-	}
-
-	if (master_ctl & (GEN8_GT_VCS0_IRQ | GEN8_GT_VCS1_IRQ)) {
-		gt_iir[1] = raw_reg_read(regs, GEN8_GT_IIR(1));
-		if (likely(gt_iir[1]))
-			raw_reg_write(regs, GEN8_GT_IIR(1), gt_iir[1]);
-	}
-
-	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
-		gt_iir[2] = raw_reg_read(regs, GEN8_GT_IIR(2));
-		if (likely(gt_iir[2]))
-			raw_reg_write(regs, GEN8_GT_IIR(2), gt_iir[2]);
-	}
-
-	if (master_ctl & GEN8_GT_VECS_IRQ) {
-		gt_iir[3] = raw_reg_read(regs, GEN8_GT_IIR(3));
-		if (likely(gt_iir[3]))
-			raw_reg_write(regs, GEN8_GT_IIR(3), gt_iir[3]);
-	}
-}
-
-void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl, u32 gt_iir[4])
-{
-	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
-		cs_irq_handler(gt->engine_class[RENDER_CLASS][0],
-			       gt_iir[0] >> GEN8_RCS_IRQ_SHIFT);
-		cs_irq_handler(gt->engine_class[COPY_ENGINE_CLASS][0],
-			       gt_iir[0] >> GEN8_BCS_IRQ_SHIFT);
+		iir = raw_reg_read(regs, GEN8_GT_IIR(0));
+		if (likely(iir)) {
+			cs_irq_handler(gt->engine_class[RENDER_CLASS][0],
+				       iir >> GEN8_RCS_IRQ_SHIFT);
+			cs_irq_handler(gt->engine_class[COPY_ENGINE_CLASS][0],
+				       iir >> GEN8_BCS_IRQ_SHIFT);
+			raw_reg_write(regs, GEN8_GT_IIR(0), iir);
+		}
 	}
 
 	if (master_ctl & (GEN8_GT_VCS0_IRQ | GEN8_GT_VCS1_IRQ)) {
-		cs_irq_handler(gt->engine_class[VIDEO_DECODE_CLASS][0],
-			       gt_iir[1] >> GEN8_VCS0_IRQ_SHIFT);
-		cs_irq_handler(gt->engine_class[VIDEO_DECODE_CLASS][1],
-			       gt_iir[1] >> GEN8_VCS1_IRQ_SHIFT);
+		iir = raw_reg_read(regs, GEN8_GT_IIR(1));
+		if (likely(iir)) {
+			cs_irq_handler(gt->engine_class[VIDEO_DECODE_CLASS][0],
+				       iir >> GEN8_VCS0_IRQ_SHIFT);
+			cs_irq_handler(gt->engine_class[VIDEO_DECODE_CLASS][1],
+				       iir >> GEN8_VCS1_IRQ_SHIFT);
+			raw_reg_write(regs, GEN8_GT_IIR(1), iir);
+		}
 	}
 
 	if (master_ctl & GEN8_GT_VECS_IRQ) {
-		cs_irq_handler(gt->engine_class[VIDEO_ENHANCEMENT_CLASS][0],
-			       gt_iir[3] >> GEN8_VECS_IRQ_SHIFT);
+		iir = raw_reg_read(regs, GEN8_GT_IIR(3));
+		if (likely(iir)) {
+			cs_irq_handler(gt->engine_class[VIDEO_ENHANCEMENT_CLASS][0],
+				       iir >> GEN8_VECS_IRQ_SHIFT);
+			raw_reg_write(regs, GEN8_GT_IIR(3), iir);
+		}
 	}
 
 	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
-		gen6_rps_irq_handler(&gt->rps, gt_iir[2]);
-		guc_irq_handler(&gt->uc.guc, gt_iir[2] >> 16);
+		iir = raw_reg_read(regs, GEN8_GT_IIR(2));
+		if (likely(iir)) {
+			gen6_rps_irq_handler(&gt->rps, iir);
+			guc_irq_handler(&gt->uc.guc, iir >> 16);
+			raw_reg_write(regs, GEN8_GT_IIR(2), iir);
+		}
 	}
 }
 
@@ -368,6 +375,7 @@ void gen8_gt_irq_postinstall(struct intel_gt *gt)
 
 	/* These are interrupts we'll toggle with the ring mask register */
 	const u32 irqs =
+		GT_CS_MASTER_ERROR_INTERRUPT |
 		GT_RENDER_USER_INTERRUPT |
 		GT_CONTEXT_SWITCH_INTERRUPT |
 		GT_WAIT_SEMAPHORE_INTERRUPT;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.h b/drivers/gpu/drm/i915/gt/intel_gt_irq.h
index 8f37593712c9..886c5cf408a2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.h
@@ -36,9 +36,8 @@ void gen5_gt_enable_irq(struct intel_gt *gt, u32 mask);
 
 void gen6_gt_irq_handler(struct intel_gt *gt, u32 gt_iir);
 
-void gen8_gt_irq_ack(struct intel_gt *gt, u32 master_ctl, u32 gt_iir[4]);
+void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl);
 void gen8_gt_irq_reset(struct intel_gt *gt);
-void gen8_gt_irq_handler(struct intel_gt *gt, u32 master_ctl, u32 gt_iir[4]);
 void gen8_gt_irq_postinstall(struct intel_gt *gt);
 
 #endif /* INTEL_GT_IRQ_H */
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 6ba5a634c6e3..388d79fed722 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -2635,13 +2635,13 @@ static bool execlists_capture(struct intel_engine_cs *engine)
 	if (!cap)
 		return true;
 
+	spin_lock_irq(&engine->active.lock);
 	cap->rq = execlists_active(&engine->execlists);
-	GEM_BUG_ON(!cap->rq);
-
-	rcu_read_lock();
-	cap->rq = active_request(cap->rq->context->timeline, cap->rq);
-	cap->rq = i915_request_get_rcu(cap->rq);
-	rcu_read_unlock();
+	if (cap->rq) {
+		cap->rq = active_request(cap->rq->context->timeline, cap->rq);
+		cap->rq = i915_request_get_rcu(cap->rq);
+	}
+	spin_unlock_irq(&engine->active.lock);
 	if (!cap->rq)
 		goto err_free;
 
@@ -2680,27 +2680,25 @@ static bool execlists_capture(struct intel_engine_cs *engine)
 	return false;
 }
 
-static noinline void preempt_reset(struct intel_engine_cs *engine)
+static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
 {
 	const unsigned int bit = I915_RESET_ENGINE + engine->id;
 	unsigned long *lock = &engine->gt->reset.flags;
 
-	if (i915_modparams.reset < 3)
+	if (!intel_has_reset_engine(engine->gt))
 		return;
 
 	if (test_and_set_bit(bit, lock))
 		return;
 
+	ENGINE_TRACE(engine, "reset for %s\n", msg);
+
 	/* Mark this tasklet as disabled to avoid waiting for it to complete */
 	tasklet_disable_nosync(&engine->execlists.tasklet);
 
-	ENGINE_TRACE(engine, "preempt timeout %lu+%ums\n",
-		     READ_ONCE(engine->props.preempt_timeout_ms),
-		     jiffies_to_msecs(jiffies - engine->execlists.preempt.expires));
-
 	ring_set_paused(engine, 1); /* Freeze the current request in place */
 	if (execlists_capture(engine))
-		intel_engine_reset(engine, "preemption time out");
+		intel_engine_reset(engine, msg);
 	else
 		ring_set_paused(engine, 0);
 
@@ -2731,6 +2729,10 @@ static void execlists_submission_tasklet(unsigned long data)
 	bool timeout = preempt_timeout(engine);
 
 	process_csb(engine);
+
+	if (unlikely(engine->execlists.error_interrupt))
+		execlists_reset(engine, "CS error");
+
 	if (!READ_ONCE(engine->execlists.pending[0]) || timeout) {
 		unsigned long flags;
 
@@ -2739,8 +2741,8 @@ static void execlists_submission_tasklet(unsigned long data)
 		spin_unlock_irqrestore(&engine->active.lock, flags);
 
 		/* Recheck after serialising with direct-submission */
-		if (timeout && preempt_timeout(engine))
-			preempt_reset(engine);
+		if (unlikely(timeout && preempt_timeout(engine)))
+			execlists_reset(engine, "preemption time out");
 	}
 }
 
@@ -3365,6 +3367,25 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 	return ret;
 }
 
+static void enable_error_interrupt(struct intel_engine_cs *engine)
+{
+	u32 status;
+
+	engine->execlists.error_interrupt = 0;
+	ENGINE_WRITE(engine, RING_EMR, ~0u);
+	ENGINE_WRITE(engine, RING_EIR, ~0u); /* clear all existing errors */
+
+	status = ENGINE_READ(engine, RING_ESR);
+	if (unlikely(status)) {
+		dev_err(engine->i915->drm.dev,
+			"engine '%s' resumed still in error: %08x\n",
+			engine->name, status);
+		__intel_gt_reset(engine->gt, engine->mask);
+	}
+
+	ENGINE_WRITE(engine, RING_EMR, ~REG_BIT(0));
+}
+
 static void enable_execlists(struct intel_engine_cs *engine)
 {
 	u32 mode;
@@ -3386,6 +3407,8 @@ static void enable_execlists(struct intel_engine_cs *engine)
 			i915_ggtt_offset(engine->status_page.vma));
 	ENGINE_POSTING_READ(engine, RING_HWS_PGA);
 
+	enable_error_interrupt(engine);
+
 	engine->context_tag = 0;
 }
 
@@ -4313,6 +4336,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
 	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
 	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
 	engine->irq_keep_mask |= GT_WAIT_SEMAPHORE_INTERRUPT << shift;
+	engine->irq_keep_mask |= GT_CS_MASTER_ERROR_INTERRUPT << shift;
 }
 
 static void rcs_submission_override(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 65718ca2326e..bb7aed7bfc07 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -68,6 +68,21 @@ static void engine_heartbeat_enable(struct intel_engine_cs *engine,
 	engine->props.heartbeat_interval_ms = saved;
 }
 
+static int wait_for_submit(struct intel_engine_cs *engine,
+			   struct i915_request *rq,
+			   unsigned long timeout)
+{
+	timeout += jiffies;
+	do {
+		cond_resched();
+		intel_engine_flush_submission(engine);
+		if (i915_request_is_active(rq))
+			return 0;
+	} while (time_before(jiffies, timeout));
+
+	return -ETIME;
+}
+
 static int live_sanitycheck(void *arg)
 {
 	struct intel_gt *gt = arg;
@@ -386,6 +401,138 @@ static int live_hold_reset(void *arg)
 	return err;
 }
 
+static const char *error_repr(int err)
+{
+	return err ? "bad" : "good";
+}
+
+static int live_error_interrupt(void *arg)
+{
+	static const struct error_phase {
+		enum { GOOD = 0, BAD = -EIO } error[2];
+	} phases[] = {
+		{ { BAD,  GOOD } },
+		{ { BAD,  BAD  } },
+		{ { BAD,  GOOD } },
+		{ { GOOD, GOOD } }, /* sentinel */
+	};
+	struct intel_gt *gt = arg;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	/*
+	 * We hook up the CS_MASTER_ERROR_INTERRUPT to have forewarning
+	 * of invalid commands in user batches that will cause a GPU hang.
+	 * This is a faster mechanism than using hangcheck/heartbeats, but
+	 * only detects problems the HW knows about -- it will not warn when
+	 * we kill the HW!
+	 *
+	 * To verify our detection and reset, we throw some invalid commands
+	 * at the HW and wait for the interrupt.
+	 */
+
+	if (!intel_has_reset_engine(gt))
+		return 0;
+
+	for_each_engine(engine, gt, id) {
+		const struct error_phase *p;
+		unsigned long heartbeat;
+
+		engine_heartbeat_disable(engine, &heartbeat);
+
+		for (p = phases; p->error[0] != GOOD; p++) {
+			struct i915_request *client[ARRAY_SIZE(phases->error)];
+			int err = 0, i;
+			u32 *cs;
+
+			memset(client, 0, sizeof(*client));
+			for (i = 0; i < ARRAY_SIZE(client); i++) {
+				struct intel_context *ce;
+				struct i915_request *rq;
+
+				ce = intel_context_create(engine);
+				if (IS_ERR(ce)) {
+					err = PTR_ERR(ce);
+					goto out;
+				}
+
+				rq = intel_context_create_request(ce);
+				intel_context_put(ce);
+				if (IS_ERR(rq)) {
+					err = PTR_ERR(rq);
+					goto out;
+				}
+
+				if (rq->engine->emit_init_breadcrumb) {
+					err = rq->engine->emit_init_breadcrumb(rq);
+					if (err) {
+						i915_request_add(rq);
+						goto out;
+					}
+				}
+
+				cs = intel_ring_begin(rq, 2);
+				if (IS_ERR(cs)) {
+					i915_request_add(rq);
+					err = PTR_ERR(cs);
+					goto out;
+				}
+
+				if (p->error[i]) {
+					*cs++ = 0xdeadbeef;
+					*cs++ = 0xdeadbeef;
+				} else {
+					*cs++ = MI_NOOP;
+					*cs++ = MI_NOOP;
+				}
+
+				client[i] = i915_request_get(rq);
+				i915_request_add(rq);
+			}
+
+			err = wait_for_submit(engine, client[0], HZ / 2);
+			if (err) {
+				pr_err("%s: first request did not start within time!\n",
+				       engine->name);
+				err = -ETIME;
+				goto out;
+			}
+
+			for (i = 0; i < ARRAY_SIZE(client); i++) {
+				if (i915_request_wait(client[i], 0, HZ / 5) < 0) {
+					pr_err("%s: %s request still executing!\n",
+					       engine->name, error_repr(p->error[i]));
+					err = -ETIME;
+					goto out;
+				}
+
+				if (client[i]->fence.error != p->error[i]) {
+					pr_err("%s: %s request completed with wrong error code: %d\n",
+					       engine->name, error_repr(p->error[i]), client[i]->fence.error);
+					err = -EINVAL;
+					goto out;
+				}
+			}
+
+out:
+			for (i = 0; i < ARRAY_SIZE(client); i++)
+				if (client[i])
+					i915_request_put(client[i]);
+			if (err) {
+				pr_err("%s: failed at phase[%ld] { %d, %d }\n",
+				       engine->name, p - phases,
+				       p->error[0], p->error[1]);
+				intel_gt_set_wedged(gt);
+				return err;
+			}
+		}
+
+		engine_heartbeat_enable(engine, heartbeat);
+	}
+
+	return 0;
+}
+
 static int
 emit_semaphore_chain(struct i915_request *rq, struct i915_vma *vma, int idx)
 {
@@ -628,21 +775,6 @@ static struct i915_request *nop_request(struct intel_engine_cs *engine)
 	return rq;
 }
 
-static int wait_for_submit(struct intel_engine_cs *engine,
-			   struct i915_request *rq,
-			   unsigned long timeout)
-{
-	timeout += jiffies;
-	do {
-		cond_resched();
-		intel_engine_flush_submission(engine);
-		if (i915_request_is_active(rq))
-			return 0;
-	} while (time_before(jiffies, timeout));
-
-	return -ETIME;
-}
-
 static long timeslice_threshold(const struct intel_engine_cs *engine)
 {
 	return 2 * msecs_to_jiffies_timeout(timeslice(engine)) + 1;
@@ -3572,6 +3704,7 @@ int intel_execlists_live_selftests(struct drm_i915_private *i915)
 		SUBTEST(live_unlite_switch),
 		SUBTEST(live_unlite_preempt),
 		SUBTEST(live_hold_reset),
+		SUBTEST(live_error_interrupt),
 		SUBTEST(live_timeslice_preempt),
 		SUBTEST(live_timeslice_queue),
 		SUBTEST(live_busywait_preempt),
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 594341e27a47..dcab4723b17d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -515,6 +515,7 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
 		   (u32)(ee->acthd>>32), (u32)ee->acthd);
 	err_printf(m, "  IPEIR: 0x%08x\n", ee->ipeir);
 	err_printf(m, "  IPEHR: 0x%08x\n", ee->ipehr);
+	err_printf(m, "  ESR:   0x%08x\n", ee->esr);
 
 	error_print_instdone(m, ee);
 
@@ -1102,6 +1103,7 @@ static void engine_record_registers(struct intel_engine_coredump *ee)
 	}
 
 	if (INTEL_GEN(i915) >= 4) {
+		ee->esr = ENGINE_READ(engine, RING_ESR);
 		ee->faddr = ENGINE_READ(engine, RING_DMA_FADD);
 		ee->ipeir = ENGINE_READ(engine, RING_IPEIR);
 		ee->ipehr = ENGINE_READ(engine, RING_IPEHR);
@@ -1228,7 +1230,7 @@ static bool record_context(struct i915_gem_context_coredump *e,
 {
 	struct i915_gem_context *ctx;
 	struct task_struct *task;
-	bool capture;
+	bool simulated;
 
 	rcu_read_lock();
 	ctx = rcu_dereference(rq->context->gem_context);
@@ -1236,7 +1238,7 @@ static bool record_context(struct i915_gem_context_coredump *e,
 		ctx = NULL;
 	rcu_read_unlock();
 	if (!ctx)
-		return false;
+		return true;
 
 	rcu_read_lock();
 	task = pid_task(ctx->pid, PIDTYPE_PID);
@@ -1250,10 +1252,10 @@ static bool record_context(struct i915_gem_context_coredump *e,
 	e->guilty = atomic_read(&ctx->guilty_count);
 	e->active = atomic_read(&ctx->active_count);
 
-	capture = i915_gem_context_no_error_capture(ctx);
+	simulated = i915_gem_context_no_error_capture(ctx);
 
 	i915_gem_context_put(ctx);
-	return capture;
+	return simulated;
 }
 
 struct intel_engine_capture_vma {
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
index 41c1475e1500..4006d3b7ef93 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.h
+++ b/drivers/gpu/drm/i915/i915_gpu_error.h
@@ -75,6 +75,7 @@ struct intel_engine_coredump {
 	u32 hws;
 	u32 ipeir;
 	u32 ipehr;
+	u32 esr;
 	u32 bbstate;
 	u32 instpm;
 	u32 instps;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 94cb25ac504d..cc0b9f78990b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1609,7 +1609,6 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		u32 master_ctl, iir;
 		u32 pipe_stats[I915_MAX_PIPES] = {};
 		u32 hotplug_status = 0;
-		u32 gt_iir[4];
 		u32 ier = 0;
 
 		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
@@ -1637,7 +1636,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		ier = I915_READ(VLV_IER);
 		I915_WRITE(VLV_IER, 0);
 
-		gen8_gt_irq_ack(&dev_priv->gt, master_ctl, gt_iir);
+		gen8_gt_irq_handler(&dev_priv->gt, master_ctl);
 
 		if (iir & I915_DISPLAY_PORT_INTERRUPT)
 			hotplug_status = i9xx_hpd_irq_ack(dev_priv);
@@ -1661,8 +1660,6 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		I915_WRITE(VLV_IER, ier);
 		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
 
-		gen8_gt_irq_handler(&dev_priv->gt, master_ctl, gt_iir);
-
 		if (hotplug_status)
 			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
 
@@ -2391,7 +2388,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	struct drm_i915_private *dev_priv = arg;
 	void __iomem * const regs = dev_priv->uncore.regs;
 	u32 master_ctl;
-	u32 gt_iir[4];
 
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
@@ -2403,7 +2399,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	}
 
 	/* Find, clear, then process each source of interrupt */
-	gen8_gt_irq_ack(&dev_priv->gt, master_ctl, gt_iir);
+	gen8_gt_irq_handler(&dev_priv->gt, master_ctl);
 
 	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
 	if (master_ctl & ~GEN8_GT_IRQS) {
@@ -2414,8 +2410,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 	gen8_master_intr_enable(regs);
 
-	gen8_gt_irq_handler(&dev_priv->gt, master_ctl, gt_iir);
-
 	return IRQ_HANDLED;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 535ce7e0dc94..e5cff51343af 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2639,6 +2639,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define   GEN11_MCR_SUBSLICE_MASK	GEN11_MCR_SUBSLICE(0x7)
 #define RING_IPEIR(base)	_MMIO((base) + 0x64)
 #define RING_IPEHR(base)	_MMIO((base) + 0x68)
+#define RING_EIR(base)		_MMIO((base) + 0xb0)
+#define RING_EMR(base)		_MMIO((base) + 0xb4)
+#define RING_ESR(base)		_MMIO((base) + 0xb8)
 /*
  * On GEN4, only the render ring INSTDONE exists and has a different
  * layout than the GEN7+ version.
@@ -3089,7 +3092,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define GT_CONTEXT_SWITCH_INTERRUPT		(1 <<  8)
 #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT	(1 <<  5) /* !snb */
 #define GT_RENDER_PIPECTL_NOTIFY_INTERRUPT	(1 <<  4)
-#define GT_RENDER_CS_MASTER_ERROR_INTERRUPT	(1 <<  3)
+#define GT_CS_MASTER_ERROR_INTERRUPT		REG_BIT(3)
 #define GT_RENDER_SYNC_STATUS_INTERRUPT		(1 <<  2)
 #define GT_RENDER_DEBUG_INTERRUPT		(1 <<  1)
 #define GT_RENDER_USER_INTERRUPT		(1 <<  0)
-- 
2.25.0

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/6] drm/i915: Remove 'prefault_disable' modparam

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Remove 'prefault_disable' modparam
URL   : https://patchwork.freedesktop.org/series/72586/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
4c84192ef497 drm/i915: Remove 'prefault_disable' modparam
be5725649262 drm/i915: Check current i915_vma.pin_count status first on unbind
25508685db92 drm/i915: Tighten atomicity of i915_active_acquire vs i915_active_release
efd14c971a7b drm/i915/gt: Acquire ce->active before ce->pin_count/ce->pin_mutex
-:7: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
<0> [198.668822] gem_exec-1246    0.... 193899010us : timeline_advance: timeline_advance:387 GEM_BUG_ON(!atomic_read(&tl->pin_count))

total: 0 errors, 1 warnings, 0 checks, 89 lines checked
162d013a8266 drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore
600325c0df29 drm/i915/gt: Hook up CS_MASTER_ERROR_INTERRUPT
-:493: WARNING:LONG_LINE: line over 100 characters
#493: FILE: drivers/gpu/drm/i915/gt/selftest_lrc.c:511:
+					       engine->name, error_repr(p->error[i]), client[i]->fence.error);

total: 0 errors, 1 warnings, 0 checks, 601 lines checked

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/6] drm/i915: Remove 'prefault_disable' modparam

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Remove 'prefault_disable' modparam
URL   : https://patchwork.freedesktop.org/series/72586/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7814 -> Patchwork_16270
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16270 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16270, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_16270:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_close_race@basic-threads:
    - fi-hsw-peppy:       [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7814/fi-hsw-peppy/igt@gem_close_race@basic-threads.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/fi-hsw-peppy/igt@gem_close_race@basic-threads.html

  
Known issues
------------

  Here are the changes found in Patchwork_16270 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-skl-6700k2:      [PASS][3] -> [DMESG-WARN][4] ([i915#889])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7814/fi-skl-6700k2/igt@i915_module_load@reload-with-fault-injection.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/fi-skl-6700k2/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770r:       [PASS][5] -> [DMESG-FAIL][6] ([i915#553] / [i915#725])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7814/fi-hsw-4770r/igt@i915_selftest@live_blt.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/fi-hsw-4770r/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_gtt:
    - fi-skl-6600u:       [PASS][7] -> [TIMEOUT][8] ([fdo#111732] / [fdo#112271])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7814/fi-skl-6600u/igt@i915_selftest@live_gtt.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/fi-skl-6600u/igt@i915_selftest@live_gtt.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-kbl-x1275:       [INCOMPLETE][9] ([i915#879]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7814/fi-kbl-x1275/igt@i915_module_load@reload-with-fault-injection.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/fi-kbl-x1275/igt@i915_module_load@reload-with-fault-injection.html

  
#### Warnings ####

  * igt@gem_exec_parallel@contexts:
    - fi-byt-n2820:       [TIMEOUT][11] ([fdo#112271]) -> [FAIL][12] ([i915#694])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7814/fi-byt-n2820/igt@gem_exec_parallel@contexts.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/fi-byt-n2820/igt@gem_exec_parallel@contexts.html

  
  [fdo#111732]: https://bugs.freedesktop.org/show_bug.cgi?id=111732
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#879]: https://gitlab.freedesktop.org/drm/intel/issues/879
  [i915#889]: https://gitlab.freedesktop.org/drm/intel/issues/889


Participating hosts (44 -> 43)
------------------------------

  Additional (5): fi-bdw-gvtdvm fi-kbl-7500u fi-blb-e6850 fi-snb-2600 fi-kbl-r 
  Missing    (6): fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-icl-dsi fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7814 -> Patchwork_16270

  CI-20190529: 20190529
  CI_DRM_7814: bc626bbb5b6efa3fb6a90407e85a04ae64461db6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5384: fd6896567f7d612c76207970376d4f1e634ded55 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16270: 600325c0df29d958a7d255689004e17acd9e9d8e @ git://anongit.freedesktop.org/gfx-ci/linux


== Kernel 32bit build ==

Warning: Kernel 32bit buildtest failed:
https://intel-gfx-ci.01.org/Patchwork_16270/build_32bit.log

  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/gt/intel_lrc.o
In file included from ./include/linux/printk.h:7:0,
                 from ./include/linux/kernel.h:15,
                 from ./include/linux/interrupt.h:6,
                 from drivers/gpu/drm/i915/gt/intel_lrc.c:134:
drivers/gpu/drm/i915/gt/selftest_lrc.c: In function ‘live_error_interrupt’:
./include/linux/kern_levels.h:5:18: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘int’ [-Werror=format=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
./include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’
 #define KERN_ERR KERN_SOH "3" /* error conditions */
                  ^~~~~~~~
./include/linux/printk.h:304:9: note: in expansion of macro ‘KERN_ERR’
  printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         ^~~~~~~~
drivers/gpu/drm/i915/gt/selftest_lrc.c:522:5: note: in expansion of macro ‘pr_err’
     pr_err("%s: failed at phase[%ld] { %d, %d }\n",
     ^~~~~~
In file included from drivers/gpu/drm/i915/gt/intel_lrc.c:5284:0:
drivers/gpu/drm/i915/gt/selftest_lrc.c:522:35: note: format string is defined here
     pr_err("%s: failed at phase[%ld] { %d, %d }\n",
                                 ~~^
                                 %d
cc1: all warnings being treated as errors
scripts/Makefile.build:265: recipe for target 'drivers/gpu/drm/i915/gt/intel_lrc.o' failed
make[4]: *** [drivers/gpu/drm/i915/gt/intel_lrc.o] Error 1
scripts/Makefile.build:503: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:503: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:503: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1693: recipe for target 'drivers' failed
make: *** [drivers] Error 2


== Linux commits ==

600325c0df29 drm/i915/gt: Hook up CS_MASTER_ERROR_INTERRUPT
162d013a8266 drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore
efd14c971a7b drm/i915/gt: Acquire ce->active before ce->pin_count/ce->pin_mutex
25508685db92 drm/i915: Tighten atomicity of i915_active_acquire vs i915_active_release
be5725649262 drm/i915: Check current i915_vma.pin_count status first on unbind
4c84192ef497 drm/i915: Remove 'prefault_disable' modparam

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: warning for series starting with [1/6] drm/i915: Remove 'prefault_disable' modparam

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Remove 'prefault_disable' modparam
URL   : https://patchwork.freedesktop.org/series/72586/
State : warning

== Summary ==

CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/gt/intel_lrc.o
In file included from ./include/linux/printk.h:7:0,
                 from ./include/linux/kernel.h:15,
                 from ./include/linux/interrupt.h:6,
                 from drivers/gpu/drm/i915/gt/intel_lrc.c:134:
drivers/gpu/drm/i915/gt/selftest_lrc.c: In function ‘live_error_interrupt’:
./include/linux/kern_levels.h:5:18: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘int’ [-Werror=format=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
./include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’
 #define KERN_ERR KERN_SOH "3" /* error conditions */
                  ^~~~~~~~
./include/linux/printk.h:304:9: note: in expansion of macro ‘KERN_ERR’
  printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         ^~~~~~~~
drivers/gpu/drm/i915/gt/selftest_lrc.c:522:5: note: in expansion of macro ‘pr_err’
     pr_err("%s: failed at phase[%ld] { %d, %d }\n",
     ^~~~~~
In file included from drivers/gpu/drm/i915/gt/intel_lrc.c:5284:0:
drivers/gpu/drm/i915/gt/selftest_lrc.c:522:35: note: format string is defined here
     pr_err("%s: failed at phase[%ld] { %d, %d }\n",
                                 ~~^
                                 %d
cc1: all warnings being treated as errors
scripts/Makefile.build:265: recipe for target 'drivers/gpu/drm/i915/gt/intel_lrc.o' failed
make[4]: *** [drivers/gpu/drm/i915/gt/intel_lrc.o] Error 1
scripts/Makefile.build:503: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:503: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:503: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1693: recipe for target 'drivers' failed
make: *** [drivers] Error 2

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16270/build_32bit.log
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Check current i915_vma.pin_count status first on unbind

[Tvrtko Ursulin]

On 26/01/2020 10:23, Chris Wilson wrote:
> Do an early rejection of a i915_vma_unbind() attempt if the i915_vma is
> currently pinned, without waiting to see if the inflight operations may
> unpin it. We see this problem with the shrinker trying to unbind the
> active vma from inside its bind worker:

What is the actual problem? flush_work from a worker?

Regards,

Tvrtko

> <6> [472.618968] Workqueue: events_unbound fence_work [i915]
> <4> [472.618970] Call Trace:
> <4> [472.618974]  ? __schedule+0x2e5/0x810
> <4> [472.618978]  schedule+0x37/0xe0
> <4> [472.618982]  schedule_preempt_disabled+0xf/0x20
> <4> [472.618984]  __mutex_lock+0x281/0x9c0
> <4> [472.618987]  ? mark_held_locks+0x49/0x70
> <4> [472.618989]  ? _raw_spin_unlock_irqrestore+0x47/0x60
> <4> [472.619038]  ? i915_vma_unbind+0xae/0x110 [i915]
> <4> [472.619084]  ? i915_vma_unbind+0xae/0x110 [i915]
> <4> [472.619122]  i915_vma_unbind+0xae/0x110 [i915]
> <4> [472.619165]  i915_gem_object_unbind+0x1dc/0x400 [i915]
> <4> [472.619208]  i915_gem_shrink+0x328/0x660 [i915]
> <4> [472.619250]  ? i915_gem_shrink_all+0x38/0x60 [i915]
> <4> [472.619282]  i915_gem_shrink_all+0x38/0x60 [i915]
> <4> [472.619325]  vm_alloc_page.constprop.25+0x1aa/0x240 [i915]
> <4> [472.619330]  ? rcu_read_lock_sched_held+0x4d/0x80
> <4> [472.619363]  ? __alloc_pd+0xb/0x30 [i915]
> <4> [472.619366]  ? module_assert_mutex_or_preempt+0xf/0x30
> <4> [472.619368]  ? __module_address+0x23/0xe0
> <4> [472.619371]  ? is_module_address+0x26/0x40
> <4> [472.619374]  ? static_obj+0x34/0x50
> <4> [472.619376]  ? lockdep_init_map+0x4d/0x1e0
> <4> [472.619407]  setup_page_dma+0xd/0x90 [i915]
> <4> [472.619437]  alloc_pd+0x29/0x50 [i915]
> <4> [472.619470]  __gen8_ppgtt_alloc+0x443/0x6b0 [i915]
> <4> [472.619503]  gen8_ppgtt_alloc+0xd7/0x300 [i915]
> <4> [472.619535]  ppgtt_bind_vma+0x2a/0xe0 [i915]
> <4> [472.619577]  __vma_bind+0x26/0x40 [i915]
> <4> [472.619611]  fence_work+0x1c/0x90 [i915]
> <4> [472.619617]  process_one_work+0x26a/0x620
> 
> Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_vma.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 84e03da0d5f9..2ffc68e18dd0 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -1190,18 +1190,6 @@ int __i915_vma_unbind(struct i915_vma *vma)
>   
>   	lockdep_assert_held(&vma->vm->mutex);
>   
> -	/*
> -	 * First wait upon any activity as retiring the request may
> -	 * have side-effects such as unpinning or even unbinding this vma.
> -	 *
> -	 * XXX Actually waiting under the vm->mutex is a hinderance and
> -	 * should be pipelined wherever possible. In cases where that is
> -	 * unavoidable, we should lift the wait to before the mutex.
> -	 */
> -	ret = i915_vma_sync(vma);
> -	if (ret)
> -		return ret;
> -
>   	if (i915_vma_is_pinned(vma)) {
>   		vma_print_allocator(vma, "is pinned");
>   		return -EAGAIN;
> @@ -1275,6 +1263,11 @@ int i915_vma_unbind(struct i915_vma *vma)
>   	if (!drm_mm_node_allocated(&vma->node))
>   		return 0;
>   
> +	if (i915_vma_is_pinned(vma)) {
> +		vma_print_allocator(vma, "is pinned");
> +		return -EAGAIN;
> +	}
> +
>   	if (i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND))
>   		/* XXX not always required: nop_clear_range */
>   		wakeref = intel_runtime_pm_get(&vm->i915->runtime_pm);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Check current i915_vma.pin_count status first on unbind

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-01-27 14:15:57)
> 
> On 26/01/2020 10:23, Chris Wilson wrote:
> > Do an early rejection of a i915_vma_unbind() attempt if the i915_vma is
> > currently pinned, without waiting to see if the inflight operations may
> > unpin it. We see this problem with the shrinker trying to unbind the
> > active vma from inside its bind worker:
> 
> What is the actual problem? flush_work from a worker?

We hit the shrinker from the inside the worker (which is intended). But
what is not intended is for the shrinker to wait on any of the workers,
because of the potential recursion of waiting on one's self. The
intention was that we avoid the shrinker waiting on the vma by keeping
the vma pinned... But I only pinned the pages to prevent them being
freed and not the worker to prevent the recursive wait.

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

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Tighten atomicity of i915_active_acquire vs i915_active_release

[Tvrtko Ursulin]

On 26/01/2020 10:23, Chris Wilson wrote:
> As we use a mutex to serialise the first acquire (as it may be a lengthy
> operation), but only an atomic decrement for the release, we have to
> be careful in case a second thread races and completes both
> acquire/release as the first finishes its acquire.
> 
> Thread A			Thread B
> i915_active_acquire		i915_active_acquire
>    atomic_read() == 0		  atomic_read() == 0
>    mutex_lock()			  mutex_lock()
> 				  atomic_read() == 0
> 				    ref->active();
> 				  atomic_inc()
> 				  mutex_unlock()
>    atomic_read() == 1
> 				i915_active_release
> 				  atomic_dec_and_test() -> 0
> 				    ref->retire()
>    atomic_inc() -> 1
>    mutex_unlock()
> 
> So thread A has acquired the ref->active_count but since the ref was
> still active at the time, it did not initialise it. By switching the
> check inside the mutex to an atomic increment only if already active, we
> close the race.
> 
> Fixes: c9ad602feabe ("drm/i915: Split i915_active.mutex into an irq-safe spinlock for the rbtree")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_active.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index ace55d5d4ca7..9d6830885d2e 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -416,13 +416,15 @@ int i915_active_acquire(struct i915_active *ref)
>   	if (err)
>   		return err;
>   
> -	if (!atomic_read(&ref->count) && ref->active)
> -		err = ref->active(ref);
> -	if (!err) {
> -		spin_lock_irq(&ref->tree_lock); /* vs __active_retire() */
> -		debug_active_activate(ref);
> -		atomic_inc(&ref->count);
> -		spin_unlock_irq(&ref->tree_lock);
> +	if (likely(!i915_active_acquire_if_busy(ref))) {
> +		if (ref->active)
> +			err = ref->active(ref);
> +		if (!err) {
> +			spin_lock_irq(&ref->tree_lock); /* __active_retire() */
> +			debug_active_activate(ref);
> +			atomic_inc(&ref->count);
> +			spin_unlock_irq(&ref->tree_lock);
> +		}
>   	}
>   
>   	mutex_unlock(&ref->mutex);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Check current i915_vma.pin_count status first on unbind

[Chris Wilson]

Quoting Chris Wilson (2020-01-27 14:20:21)
> Quoting Tvrtko Ursulin (2020-01-27 14:15:57)
> > 
> > On 26/01/2020 10:23, Chris Wilson wrote:
> > > Do an early rejection of a i915_vma_unbind() attempt if the i915_vma is
> > > currently pinned, without waiting to see if the inflight operations may
> > > unpin it. We see this problem with the shrinker trying to unbind the
> > > active vma from inside its bind worker:
> > 
> > What is the actual problem? flush_work from a worker?
> 
> We hit the shrinker from the inside the worker (which is intended). But
> what is not intended is for the shrinker to wait on any of the workers,
> because of the potential recursion of waiting on one's self. The
> intention was that we avoid the shrinker waiting on the vma by keeping
> the vma pinned... But I only pinned the pages to prevent them being
> freed and not the worker to prevent the recursive wait.
> 
> Heh.

Which then ties back into the whole i915_vma is not krefed, so to
complete the destroy at the moment we need to sync with the workers.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/6] drm/i915/gt: Acquire ce->active before ce->pin_count/ce->pin_mutex

[Tvrtko Ursulin]

On 26/01/2020 10:23, Chris Wilson wrote:
> <0> [198.668822] gem_exec-1246    0.... 193899010us : timeline_advance: timeline_advance:387 GEM_BUG_ON(!atomic_read(&tl->pin_count))
> <0> [198.668859] ---------------------------------
> <4> [198.669619] ------------[ cut here ]------------
> <2> [198.669621] kernel BUG at drivers/gpu/drm/i915/gt/intel_timeline.c:387!
> <4> [198.669703] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> <4> [198.669712] CPU: 0 PID: 1246 Comm: gem_exec_create Tainted: G     U  W         5.5.0-rc6-CI-CI_DRM_7755+ #1
> <4> [198.669723] Hardware name:  /NUC7i5BNB, BIOS BNKBL357.86A.0054.2017.1025.1822 10/25/2017
> <4> [198.669776] RIP: 0010:timeline_advance+0x7b/0xe0 [i915]
> <4> [198.669785] Code: 00 48 c7 c2 10 f1 46 a0 48 c7 c7 70 1b 32 a0 e8 bb dd e7 e0 bf 01 00 00 00 e8 d1 af e7 e0 31 f6 bf 09 00 00 00 e8 35 ef d8 e0 <0f> 0b 48 c7 c1 48 fa 49 a0 ba 84 01 00 00 48 c7 c6 10 f1 46 a0 48
> <4> [198.669803] RSP: 0018:ffffc900004c3a38 EFLAGS: 00010296
> <4> [198.669810] RAX: ffff888270b35140 RBX: ffff88826f32ee00 RCX: 0000000000000006
> <4> [198.669818] RDX: 00000000000017c5 RSI: 0000000000000000 RDI: 0000000000000009
> <4> [198.669826] RBP: ffffc900004c3a64 R08: 0000000000000000 R09: 0000000000000000
> <4> [198.669834] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88826f9b5980
> <4> [198.669841] R13: 0000000000000cc0 R14: ffffc900004c3dc0 R15: ffff888253610068
> <4> [198.669849] FS:  00007f63e663fe40(0000) GS:ffff888276c00000(0000) knlGS:0000000000000000
> <4> [198.669857] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4> [198.669864] CR2: 00007f171f8e39a8 CR3: 000000026b1f6005 CR4: 00000000003606f0
> <4> [198.669872] Call Trace:
> <4> [198.669924]  intel_timeline_get_seqno+0x12/0x40 [i915]
> <4> [198.669977]  __i915_request_create+0x76/0x5a0 [i915]
> <4> [198.670024]  i915_request_create+0x86/0x1c0 [i915]
> <4> [198.670068]  i915_gem_do_execbuffer+0xbf2/0x2500 [i915]
> <4> [198.670082]  ? __lock_acquire+0x460/0x15d0
> <4> [198.670128]  i915_gem_execbuffer2_ioctl+0x11f/0x470 [i915]
> <4> [198.670171]  ? i915_gem_execbuffer_ioctl+0x300/0x300 [i915]
> <4> [198.670181]  drm_ioctl_kernel+0xa7/0xf0
> <4> [198.670188]  drm_ioctl+0x2e1/0x390
> <4> [198.670233]  ? i915_gem_execbuffer_ioctl+0x300/0x300 [i915]
> 
> Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between context pin/unpin")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_context.c | 46 ++++++++++++++-----------
>   drivers/gpu/drm/i915/i915_active.h      |  6 ++++
>   2 files changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 23137b2a8689..57e8a051ddc2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -67,21 +67,18 @@ static int intel_context_active_acquire(struct intel_context *ce)
>   {
>   	int err;
>   
> -	err = i915_active_acquire(&ce->active);
> -	if (err)
> -		return err;
> +	__i915_active_acquire(&ce->active);
> +
> +	if (intel_context_is_barrier(ce))
> +		return 0;
>   
>   	/* Preallocate tracking nodes */
> -	if (!intel_context_is_barrier(ce)) {
> -		err = i915_active_acquire_preallocate_barrier(&ce->active,
> -							      ce->engine);
> -		if (err) {
> -			i915_active_release(&ce->active);
> -			return err;
> -		}
> -	}
> +	err = i915_active_acquire_preallocate_barrier(&ce->active,
> +						      ce->engine);
> +	if (err)
> +		i915_active_release(&ce->active);
>   
> -	return 0;
> +	return err;
>   }
>   
>   static void intel_context_active_release(struct intel_context *ce)
> @@ -101,13 +98,19 @@ int __intel_context_do_pin(struct intel_context *ce)
>   			return err;
>   	}
>   
> -	if (mutex_lock_interruptible(&ce->pin_mutex))
> -		return -EINTR;
> +	err = i915_active_acquire(&ce->active);
> +	if (err)
> +		return err;
> +
> +	if (mutex_lock_interruptible(&ce->pin_mutex)) {
> +		err = -EINTR;
> +		goto out_release;
> +	}
>   
> -	if (likely(!atomic_read(&ce->pin_count))) {
> +	if (likely(!atomic_add_unless(&ce->pin_count, 1, 0))) {
>   		err = intel_context_active_acquire(ce);
>   		if (unlikely(err))
> -			goto err;
> +			goto out_unlock;
>   
>   		err = ce->ops->pin(ce);
>   		if (unlikely(err))
> @@ -117,18 +120,19 @@ int __intel_context_do_pin(struct intel_context *ce)
>   			 ce->ring->head, ce->ring->tail);
>   
>   		smp_mb__before_atomic(); /* flush pin before it is visible */
> +		atomic_inc(&ce->pin_count);
>   	}
>   
> -	atomic_inc(&ce->pin_count);
>   	GEM_BUG_ON(!intel_context_is_pinned(ce)); /* no overflow! */
> -
> -	mutex_unlock(&ce->pin_mutex);
> -	return 0;
> +	GEM_BUG_ON(i915_active_is_idle(&ce->active));
> +	goto out_unlock;
>   
>   err_active:
>   	intel_context_active_release(ce);
> -err:
> +out_unlock:
>   	mutex_unlock(&ce->pin_mutex);
> +out_release:
> +	i915_active_release(&ce->active);
>   	return err;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index b571f675c795..51e1e854ca55 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -188,6 +188,12 @@ int i915_active_acquire(struct i915_active *ref);
>   bool i915_active_acquire_if_busy(struct i915_active *ref);
>   void i915_active_release(struct i915_active *ref);
>   
> +static inline void __i915_active_acquire(struct i915_active *ref)
> +{
> +	GEM_BUG_ON(!atomic_read(&ref->count));
> +	atomic_inc(&ref->count);
> +}
> +
>   static inline bool
>   i915_active_is_idle(const struct i915_active *ref)
>   {
> 

Change looks okay but I would like to read something more in the commit 
message. Like I feel there is more to it than acquire order otherwise 
problem would be much more widespread.

Regards,

Tvrtko

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

Re: [Intel-gfx] [PATCH 4/6] drm/i915/gt: Acquire ce->active before ce->pin_count/ce->pin_mutex

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-01-27 15:33:44)
> 
> On 26/01/2020 10:23, Chris Wilson wrote:
> > <0> [198.668822] gem_exec-1246    0.... 193899010us : timeline_advance: timeline_advance:387 GEM_BUG_ON(!atomic_read(&tl->pin_count))
> > <0> [198.668859] ---------------------------------
> > <4> [198.669619] ------------[ cut here ]------------
> > <2> [198.669621] kernel BUG at drivers/gpu/drm/i915/gt/intel_timeline.c:387!
> > <4> [198.669703] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > <4> [198.669712] CPU: 0 PID: 1246 Comm: gem_exec_create Tainted: G     U  W         5.5.0-rc6-CI-CI_DRM_7755+ #1
> > <4> [198.669723] Hardware name:  /NUC7i5BNB, BIOS BNKBL357.86A.0054.2017.1025.1822 10/25/2017
> > <4> [198.669776] RIP: 0010:timeline_advance+0x7b/0xe0 [i915]
> > <4> [198.669785] Code: 00 48 c7 c2 10 f1 46 a0 48 c7 c7 70 1b 32 a0 e8 bb dd e7 e0 bf 01 00 00 00 e8 d1 af e7 e0 31 f6 bf 09 00 00 00 e8 35 ef d8 e0 <0f> 0b 48 c7 c1 48 fa 49 a0 ba 84 01 00 00 48 c7 c6 10 f1 46 a0 48
> > <4> [198.669803] RSP: 0018:ffffc900004c3a38 EFLAGS: 00010296
> > <4> [198.669810] RAX: ffff888270b35140 RBX: ffff88826f32ee00 RCX: 0000000000000006
> > <4> [198.669818] RDX: 00000000000017c5 RSI: 0000000000000000 RDI: 0000000000000009
> > <4> [198.669826] RBP: ffffc900004c3a64 R08: 0000000000000000 R09: 0000000000000000
> > <4> [198.669834] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88826f9b5980
> > <4> [198.669841] R13: 0000000000000cc0 R14: ffffc900004c3dc0 R15: ffff888253610068
> > <4> [198.669849] FS:  00007f63e663fe40(0000) GS:ffff888276c00000(0000) knlGS:0000000000000000
> > <4> [198.669857] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4> [198.669864] CR2: 00007f171f8e39a8 CR3: 000000026b1f6005 CR4: 00000000003606f0
> > <4> [198.669872] Call Trace:
> > <4> [198.669924]  intel_timeline_get_seqno+0x12/0x40 [i915]
> > <4> [198.669977]  __i915_request_create+0x76/0x5a0 [i915]
> > <4> [198.670024]  i915_request_create+0x86/0x1c0 [i915]
> > <4> [198.670068]  i915_gem_do_execbuffer+0xbf2/0x2500 [i915]
> > <4> [198.670082]  ? __lock_acquire+0x460/0x15d0
> > <4> [198.670128]  i915_gem_execbuffer2_ioctl+0x11f/0x470 [i915]
> > <4> [198.670171]  ? i915_gem_execbuffer_ioctl+0x300/0x300 [i915]
> > <4> [198.670181]  drm_ioctl_kernel+0xa7/0xf0
> > <4> [198.670188]  drm_ioctl+0x2e1/0x390
> > <4> [198.670233]  ? i915_gem_execbuffer_ioctl+0x300/0x300 [i915]
> > 
> > Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between context pin/unpin")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_context.c | 46 ++++++++++++++-----------
> >   drivers/gpu/drm/i915/i915_active.h      |  6 ++++
> >   2 files changed, 31 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 23137b2a8689..57e8a051ddc2 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -67,21 +67,18 @@ static int intel_context_active_acquire(struct intel_context *ce)
> >   {
> >       int err;
> >   
> > -     err = i915_active_acquire(&ce->active);
> > -     if (err)
> > -             return err;
> > +     __i915_active_acquire(&ce->active);
> > +
> > +     if (intel_context_is_barrier(ce))
> > +             return 0;
> >   
> >       /* Preallocate tracking nodes */
> > -     if (!intel_context_is_barrier(ce)) {
> > -             err = i915_active_acquire_preallocate_barrier(&ce->active,
> > -                                                           ce->engine);
> > -             if (err) {
> > -                     i915_active_release(&ce->active);
> > -                     return err;
> > -             }
> > -     }
> > +     err = i915_active_acquire_preallocate_barrier(&ce->active,
> > +                                                   ce->engine);
> > +     if (err)
> > +             i915_active_release(&ce->active);
> >   
> > -     return 0;
> > +     return err;
> >   }
> >   
> >   static void intel_context_active_release(struct intel_context *ce)
> > @@ -101,13 +98,19 @@ int __intel_context_do_pin(struct intel_context *ce)
> >                       return err;
> >       }
> >   
> > -     if (mutex_lock_interruptible(&ce->pin_mutex))
> > -             return -EINTR;
> > +     err = i915_active_acquire(&ce->active);
> > +     if (err)
> > +             return err;
> > +
> > +     if (mutex_lock_interruptible(&ce->pin_mutex)) {
> > +             err = -EINTR;
> > +             goto out_release;
> > +     }
> >   
> > -     if (likely(!atomic_read(&ce->pin_count))) {
> > +     if (likely(!atomic_add_unless(&ce->pin_count, 1, 0))) {
> >               err = intel_context_active_acquire(ce);
> >               if (unlikely(err))
> > -                     goto err;
> > +                     goto out_unlock;
> >   
> >               err = ce->ops->pin(ce);
> >               if (unlikely(err))
> > @@ -117,18 +120,19 @@ int __intel_context_do_pin(struct intel_context *ce)
> >                        ce->ring->head, ce->ring->tail);
> >   
> >               smp_mb__before_atomic(); /* flush pin before it is visible */
> > +             atomic_inc(&ce->pin_count);
> >       }
> >   
> > -     atomic_inc(&ce->pin_count);
> >       GEM_BUG_ON(!intel_context_is_pinned(ce)); /* no overflow! */
> > -
> > -     mutex_unlock(&ce->pin_mutex);
> > -     return 0;
> > +     GEM_BUG_ON(i915_active_is_idle(&ce->active));
> > +     goto out_unlock;
> >   
> >   err_active:
> >       intel_context_active_release(ce);
> > -err:
> > +out_unlock:
> >       mutex_unlock(&ce->pin_mutex);
> > +out_release:
> > +     i915_active_release(&ce->active);
> >       return err;
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> > index b571f675c795..51e1e854ca55 100644
> > --- a/drivers/gpu/drm/i915/i915_active.h
> > +++ b/drivers/gpu/drm/i915/i915_active.h
> > @@ -188,6 +188,12 @@ int i915_active_acquire(struct i915_active *ref);
> >   bool i915_active_acquire_if_busy(struct i915_active *ref);
> >   void i915_active_release(struct i915_active *ref);
> >   
> > +static inline void __i915_active_acquire(struct i915_active *ref)
> > +{
> > +     GEM_BUG_ON(!atomic_read(&ref->count));
> > +     atomic_inc(&ref->count);
> > +}
> > +
> >   static inline bool
> >   i915_active_is_idle(const struct i915_active *ref)
> >   {
> > 
> 
> Change looks okay but I would like to read something more in the commit 
> message. Like I feel there is more to it than acquire order otherwise 
> problem would be much more widespread.

I added the information about the issue of the mutex_lock vs unlocked
unpin, but the rest of the motion is continuing the theme of trying to
make ww_mutex easier (which you didn't hear).
https://patchwork.freedesktop.org/patch/350849/?series=72626&rev=1
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore

[Tvrtko Ursulin]

On 26/01/2020 10:23, Chris Wilson wrote:
> If we find ourselves waiting on a MI_SEMAPHORE_WAIT, either within the
> user batch or in our own preamble, the engine raises a
> GT_WAIT_ON_SEMAPHORE interrupt. We can unmask that interrupt and so
> respond to a semaphore wait by yielding the timeslice, if we have
> another context to yield to!
> 
> The only real complication is that the interrupt is only generated for
> the start of the semaphore wait, and is asynchronous to our
> process_csb() -- that is, we may not have registered the timeslice before
> we see the interrupt. To ensure we don't miss a potential semaphore
> blocking forward progress (e.g. selftests/live_timeslice_preempt) we mark
> the interrupt and apply it to the next timeslice regardless of whether it
> was active at the time.
> 
> v2: We use semaphores in preempt-to-busy, within the timeslicing
> implementation itself! Ergo, when we do insert a preemption due to an
> expired timeslice, the new context may start with the missed semaphore
> flagged by the retired context and be yielded, ad infinitum. To avoid
> this, read the context id at the time of the semaphore interrupt and
> only yield if that context is still active.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_types.h |  9 ++++++
>   drivers/gpu/drm/i915/gt/intel_gt_irq.c       | 32 +++++++++++---------
>   drivers/gpu/drm/i915/gt/intel_lrc.c          | 31 ++++++++++++++++---
>   drivers/gpu/drm/i915/i915_reg.h              |  5 +++
>   4 files changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 92be41a6903c..58725024ffa4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -156,6 +156,15 @@ struct intel_engine_execlists {
>   	 */
>   	struct i915_priolist default_priolist;
>   
> +	/**
> +	 * @yield: CCID at the time of the last semaphore-wait interrupt.
> +	 *
> +	 * Instead of leaving a semaphore busy-spinning on an engine, we would
> +	 * like to switch to another ready context, i.e. yielding the semaphore
> +	 * timeslice.
> +	 */
> +	u32 yield;
> +
>   	/**
>   	 * @no_priolist: priority lists disabled
>   	 */
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index f796bdf1ed30..6ae64a224b02 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -24,6 +24,13 @@ cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
>   {
>   	bool tasklet = false;
>   
> +	if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {
> +		WRITE_ONCE(engine->execlists.yield,
> +			   ENGINE_READ_FW(engine, EXECLIST_CCID));
> +		if (del_timer(&engine->execlists.timer))
> +			tasklet = true;

What if it fires before timeslice timer has been set up and when we miss 
to yield?

> +	}
> +
>   	if (iir & GT_CONTEXT_SWITCH_INTERRUPT)
>   		tasklet = true;
>   
> @@ -210,7 +217,10 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>   
>   void gen11_gt_irq_postinstall(struct intel_gt *gt)
>   {
> -	const u32 irqs = GT_RENDER_USER_INTERRUPT | GT_CONTEXT_SWITCH_INTERRUPT;
> +	const u32 irqs =
> +		GT_RENDER_USER_INTERRUPT |
> +		GT_CONTEXT_SWITCH_INTERRUPT |
> +		GT_WAIT_SEMAPHORE_INTERRUPT;
>   	struct intel_uncore *uncore = gt->uncore;
>   	const u32 dmask = irqs << 16 | irqs;
>   	const u32 smask = irqs << 16;
> @@ -357,21 +367,15 @@ void gen8_gt_irq_postinstall(struct intel_gt *gt)
>   	struct intel_uncore *uncore = gt->uncore;
>   
>   	/* These are interrupts we'll toggle with the ring mask register */
> +	const u32 irqs =
> +		GT_RENDER_USER_INTERRUPT |
> +		GT_CONTEXT_SWITCH_INTERRUPT |
> +		GT_WAIT_SEMAPHORE_INTERRUPT;
>   	u32 gt_interrupts[] = {
> -		(GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
> -		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
> -		 GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT |
> -		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT),
> -
> -		(GT_RENDER_USER_INTERRUPT << GEN8_VCS0_IRQ_SHIFT |
> -		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS0_IRQ_SHIFT |
> -		 GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT |
> -		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT),
> -
> +		irqs << GEN8_RCS_IRQ_SHIFT | irqs << GEN8_BCS_IRQ_SHIFT,
> +		irqs << GEN8_VCS0_IRQ_SHIFT | irqs << GEN8_VCS1_IRQ_SHIFT,
>   		0,
> -
> -		(GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT |
> -		 GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT)
> +		irqs << GEN8_VECS_IRQ_SHIFT,
>   	};
>   
>   	gt->pm_ier = 0x0;
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index a13a8c4b65ab..6ba5a634c6e3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1661,7 +1661,8 @@ static void defer_active(struct intel_engine_cs *engine)
>   }
>   
>   static bool
> -need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
> +need_timeslice(const struct intel_engine_cs *engine,
> +	       const struct i915_request *rq)

Naughty.

>   {
>   	int hint;
>   
> @@ -1677,6 +1678,27 @@ need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
>   	return hint >= effective_prio(rq);
>   }
>   
> +static bool
> +timeslice_expired(const struct intel_engine_cs *engine,
> +		  const struct i915_request *rq)
> +{
> +	const struct intel_engine_execlists *el = &engine->execlists;
> +
> +	return (timer_expired(&el->timer) ||
> +		/*
> +		 * Once bitten, forever smitten!
> +		 *
> +		 * If the active context ever busy-waited on a semaphore,
> +		 * it will be treated as a hog until the end of its timeslice.
> +		 * The HW only sends an interrupt on the first miss, and we

One interrupt per elsp submission? Then on re-submission it can repeat? 
I hope so because we recycle lrc_desc aggressively so I am wondering 
about that.

> +		 * do know if that semaphore has been signaled, or even if it
> +		 * is now stuck on another semaphore. Play safe, yield if it
> +		 * might be stuck -- it will be given a fresh timeslice in
> +		 * the near future.
> +		 */
> +		upper_32_bits(rq->context->lrc_desc) == READ_ONCE(el->yield));
> +}
> +
>   static int
>   switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
>   {
> @@ -1692,8 +1714,7 @@ timeslice(const struct intel_engine_cs *engine)
>   	return READ_ONCE(engine->props.timeslice_duration_ms);
>   }
>   
> -static unsigned long
> -active_timeslice(const struct intel_engine_cs *engine)
> +static unsigned long active_timeslice(const struct intel_engine_cs *engine)
>   {
>   	const struct i915_request *rq = *engine->execlists.active;
>   
> @@ -1844,7 +1865,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   			last->context->lrc_desc |= CTX_DESC_FORCE_RESTORE;
>   			last = NULL;
>   		} else if (need_timeslice(engine, last) &&
> -			   timer_expired(&engine->execlists.timer)) {
> +			   timeslice_expired(engine, last)) {
>   			ENGINE_TRACE(engine,
>   				     "expired last=%llx:%lld, prio=%d, hint=%d\n",

Could be useful to move tracing msg into timeslice_expired and 
distinguish between the two cases.

>   				     last->fence.context,
> @@ -2110,6 +2131,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		}
>   		clear_ports(port + 1, last_port - port);
>   
> +		WRITE_ONCE(execlists->yield, -1);
>   		execlists_submit_ports(engine);
>   		set_preempt_timeout(engine);
>   	} else {
> @@ -4290,6 +4312,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
>   
>   	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
>   	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> +	engine->irq_keep_mask |= GT_WAIT_SEMAPHORE_INTERRUPT << shift;
>   }
>   
>   static void rcs_submission_override(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b93c4c18f05c..535ce7e0dc94 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3085,6 +3085,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>   #define GT_BSD_CS_ERROR_INTERRUPT		(1 << 15)
>   #define GT_BSD_USER_INTERRUPT			(1 << 12)
>   #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT_S1	(1 << 11) /* hsw+; rsvd on snb, ivb, vlv */
> +#define GT_WAIT_SEMAPHORE_INTERRUPT		REG_BIT(11) /* bdw+ */
>   #define GT_CONTEXT_SWITCH_INTERRUPT		(1 <<  8)
>   #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT	(1 <<  5) /* !snb */
>   #define GT_RENDER_PIPECTL_NOTIFY_INTERRUPT	(1 <<  4)
> @@ -4036,6 +4037,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>   #define   CCID_EN			BIT(0)
>   #define   CCID_EXTENDED_STATE_RESTORE	BIT(2)
>   #define   CCID_EXTENDED_STATE_SAVE	BIT(3)
> +
> +#define EXECLIST_STATUS(base)	_MMIO((base) + 0x234)

This one is unused.

> +#define EXECLIST_CCID(base)	_MMIO((base) + 0x238)
> +
>   /*
>    * Notes on SNB/IVB/VLV context size:
>    * - Power context is saved elsewhere (LLC or stolen)
> 

Regards,

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

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-01-27 15:52:51)
> 
> On 26/01/2020 10:23, Chris Wilson wrote:
> > If we find ourselves waiting on a MI_SEMAPHORE_WAIT, either within the
> > user batch or in our own preamble, the engine raises a
> > GT_WAIT_ON_SEMAPHORE interrupt. We can unmask that interrupt and so
> > respond to a semaphore wait by yielding the timeslice, if we have
> > another context to yield to!
> > 
> > The only real complication is that the interrupt is only generated for
> > the start of the semaphore wait, and is asynchronous to our
> > process_csb() -- that is, we may not have registered the timeslice before
> > we see the interrupt. To ensure we don't miss a potential semaphore
> > blocking forward progress (e.g. selftests/live_timeslice_preempt) we mark
> > the interrupt and apply it to the next timeslice regardless of whether it
> > was active at the time.
> > 
> > v2: We use semaphores in preempt-to-busy, within the timeslicing
> > implementation itself! Ergo, when we do insert a preemption due to an
> > expired timeslice, the new context may start with the missed semaphore
> > flagged by the retired context and be yielded, ad infinitum. To avoid
> > this, read the context id at the time of the semaphore interrupt and
> > only yield if that context is still active.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_engine_types.h |  9 ++++++
> >   drivers/gpu/drm/i915/gt/intel_gt_irq.c       | 32 +++++++++++---------
> >   drivers/gpu/drm/i915/gt/intel_lrc.c          | 31 ++++++++++++++++---
> >   drivers/gpu/drm/i915/i915_reg.h              |  5 +++
> >   4 files changed, 59 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index 92be41a6903c..58725024ffa4 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -156,6 +156,15 @@ struct intel_engine_execlists {
> >        */
> >       struct i915_priolist default_priolist;
> >   
> > +     /**
> > +      * @yield: CCID at the time of the last semaphore-wait interrupt.
> > +      *
> > +      * Instead of leaving a semaphore busy-spinning on an engine, we would
> > +      * like to switch to another ready context, i.e. yielding the semaphore
> > +      * timeslice.
> > +      */
> > +     u32 yield;
> > +
> >       /**
> >        * @no_priolist: priority lists disabled
> >        */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > index f796bdf1ed30..6ae64a224b02 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > @@ -24,6 +24,13 @@ cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> >   {
> >       bool tasklet = false;
> >   
> > +     if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {
> > +             WRITE_ONCE(engine->execlists.yield,
> > +                        ENGINE_READ_FW(engine, EXECLIST_CCID));
> > +             if (del_timer(&engine->execlists.timer))
> > +                     tasklet = true;
> 
> What if it fires before timeslice timer has been set up and when we miss 
> to yield?

We only set the timer after the HW ack, and we can legitimately hit a
semaphore in the user payload before we see the ack. That is
demonstrated aptly by live_timeslice_preempt.

> > +     }
> > +
> >       if (iir & GT_CONTEXT_SWITCH_INTERRUPT)
> >               tasklet = true;
> >   
> > @@ -210,7 +217,10 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >   
> >   void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >   {
> > -     const u32 irqs = GT_RENDER_USER_INTERRUPT | GT_CONTEXT_SWITCH_INTERRUPT;
> > +     const u32 irqs =
> > +             GT_RENDER_USER_INTERRUPT |
> > +             GT_CONTEXT_SWITCH_INTERRUPT |
> > +             GT_WAIT_SEMAPHORE_INTERRUPT;
> >       struct intel_uncore *uncore = gt->uncore;
> >       const u32 dmask = irqs << 16 | irqs;
> >       const u32 smask = irqs << 16;
> > @@ -357,21 +367,15 @@ void gen8_gt_irq_postinstall(struct intel_gt *gt)
> >       struct intel_uncore *uncore = gt->uncore;
> >   
> >       /* These are interrupts we'll toggle with the ring mask register */
> > +     const u32 irqs =
> > +             GT_RENDER_USER_INTERRUPT |
> > +             GT_CONTEXT_SWITCH_INTERRUPT |
> > +             GT_WAIT_SEMAPHORE_INTERRUPT;
> >       u32 gt_interrupts[] = {
> > -             (GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
> > -              GT_CONTEXT_SWITCH_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
> > -              GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT |
> > -              GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT),
> > -
> > -             (GT_RENDER_USER_INTERRUPT << GEN8_VCS0_IRQ_SHIFT |
> > -              GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS0_IRQ_SHIFT |
> > -              GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT |
> > -              GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT),
> > -
> > +             irqs << GEN8_RCS_IRQ_SHIFT | irqs << GEN8_BCS_IRQ_SHIFT,
> > +             irqs << GEN8_VCS0_IRQ_SHIFT | irqs << GEN8_VCS1_IRQ_SHIFT,
> >               0,
> > -
> > -             (GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT |
> > -              GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT)
> > +             irqs << GEN8_VECS_IRQ_SHIFT,
> >       };
> >   
> >       gt->pm_ier = 0x0;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index a13a8c4b65ab..6ba5a634c6e3 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1661,7 +1661,8 @@ static void defer_active(struct intel_engine_cs *engine)
> >   }
> >   
> >   static bool
> > -need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
> > +need_timeslice(const struct intel_engine_cs *engine,
> > +            const struct i915_request *rq)
> 
> Naughty.

Adding a const! I'd go with a mere cheek than naughty.

> >   {
> >       int hint;
> >   
> > @@ -1677,6 +1678,27 @@ need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
> >       return hint >= effective_prio(rq);
> >   }
> >   
> > +static bool
> > +timeslice_expired(const struct intel_engine_cs *engine,
> > +               const struct i915_request *rq)
> > +{
> > +     const struct intel_engine_execlists *el = &engine->execlists;
> > +
> > +     return (timer_expired(&el->timer) ||
> > +             /*
> > +              * Once bitten, forever smitten!
> > +              *
> > +              * If the active context ever busy-waited on a semaphore,
> > +              * it will be treated as a hog until the end of its timeslice.
> > +              * The HW only sends an interrupt on the first miss, and we
> 
> One interrupt per elsp submission? Then on re-submission it can repeat? 
> I hope so because we recycle lrc_desc aggressively so I am wondering 
> about that.

That appears so. While waiting on a semaphore, the interrupt only fires
once not for each poll. Hmm, I shall have to check just in case each
semaphore causes a new interrupt (and we may have just got lucky due to
the effect of using edge interrupts).
 
> > +              * do know if that semaphore has been signaled, or even if it
> > +              * is now stuck on another semaphore. Play safe, yield if it
> > +              * might be stuck -- it will be given a fresh timeslice in
> > +              * the near future.
> > +              */
> > +             upper_32_bits(rq->context->lrc_desc) == READ_ONCE(el->yield));
> > +}
> > +
> >   static int
> >   switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
> >   {
> > @@ -1692,8 +1714,7 @@ timeslice(const struct intel_engine_cs *engine)
> >       return READ_ONCE(engine->props.timeslice_duration_ms);
> >   }
> >   
> > -static unsigned long
> > -active_timeslice(const struct intel_engine_cs *engine)
> > +static unsigned long active_timeslice(const struct intel_engine_cs *engine)
> >   {
> >       const struct i915_request *rq = *engine->execlists.active;
> >   
> > @@ -1844,7 +1865,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                       last->context->lrc_desc |= CTX_DESC_FORCE_RESTORE;
> >                       last = NULL;
> >               } else if (need_timeslice(engine, last) &&
> > -                        timer_expired(&engine->execlists.timer)) {
> > +                        timeslice_expired(engine, last)) {
> >                       ENGINE_TRACE(engine,
> >                                    "expired last=%llx:%lld, prio=%d, hint=%d\n",
> 
> Could be useful to move tracing msg into timeslice_expired and 
> distinguish between the two cases.

Hmm. Or just add the flag here as well, will see which looks better.

> 
> >                                    last->fence.context,
> > @@ -2110,6 +2131,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >               }
> >               clear_ports(port + 1, last_port - port);
> >   
> > +             WRITE_ONCE(execlists->yield, -1);
> >               execlists_submit_ports(engine);
> >               set_preempt_timeout(engine);
> >       } else {
> > @@ -4290,6 +4312,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
> >   
> >       engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> >       engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> > +     engine->irq_keep_mask |= GT_WAIT_SEMAPHORE_INTERRUPT << shift;
> >   }
> >   
> >   static void rcs_submission_override(struct intel_engine_cs *engine)
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b93c4c18f05c..535ce7e0dc94 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3085,6 +3085,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >   #define GT_BSD_CS_ERROR_INTERRUPT           (1 << 15)
> >   #define GT_BSD_USER_INTERRUPT                       (1 << 12)
> >   #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT_S1      (1 << 11) /* hsw+; rsvd on snb, ivb, vlv */
> > +#define GT_WAIT_SEMAPHORE_INTERRUPT          REG_BIT(11) /* bdw+ */
> >   #define GT_CONTEXT_SWITCH_INTERRUPT         (1 <<  8)
> >   #define GT_RENDER_L3_PARITY_ERROR_INTERRUPT (1 <<  5) /* !snb */
> >   #define GT_RENDER_PIPECTL_NOTIFY_INTERRUPT  (1 <<  4)
> > @@ -4036,6 +4037,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >   #define   CCID_EN                   BIT(0)
> >   #define   CCID_EXTENDED_STATE_RESTORE       BIT(2)
> >   #define   CCID_EXTENDED_STATE_SAVE  BIT(3)
> > +
> > +#define EXECLIST_STATUS(base)        _MMIO((base) + 0x234)
> 
> This one is unused.

Your eyes must be deceived, for I foresaw that complaint and added it to
intel_engine_dump :-p
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore

[Chris Wilson]

Quoting Chris Wilson (2020-01-27 16:03:11)
> Quoting Tvrtko Ursulin (2020-01-27 15:52:51)
> > 
> > On 26/01/2020 10:23, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > > index f796bdf1ed30..6ae64a224b02 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > > @@ -24,6 +24,13 @@ cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> > >   {
> > >       bool tasklet = false;
> > >   
> > > +     if (iir & GT_WAIT_SEMAPHORE_INTERRUPT) {
> > > +             WRITE_ONCE(engine->execlists.yield,
> > > +                        ENGINE_READ_FW(engine, EXECLIST_CCID));
> > > +             if (del_timer(&engine->execlists.timer))
> > > +                     tasklet = true;
> > 
> > What if it fires before timeslice timer has been set up and when we miss 
> > to yield?
> 
> We only set the timer after the HW ack, and we can legitimately hit a
> semaphore in the user payload before we see the ack. That is
> demonstrated aptly by live_timeslice_preempt.

Hmm. I thought we would see the yield such that we would dequeue and
process the new timelice regardless of the previous timer.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Check current i915_vma.pin_count status first on unbind

[Tvrtko Ursulin]

On 26/01/2020 10:23, Chris Wilson wrote:
> Do an early rejection of a i915_vma_unbind() attempt if the i915_vma is
> currently pinned, without waiting to see if the inflight operations may
> unpin it. We see this problem with the shrinker trying to unbind the
> active vma from inside its bind worker:
> 
> <6> [472.618968] Workqueue: events_unbound fence_work [i915]
> <4> [472.618970] Call Trace:
> <4> [472.618974]  ? __schedule+0x2e5/0x810
> <4> [472.618978]  schedule+0x37/0xe0
> <4> [472.618982]  schedule_preempt_disabled+0xf/0x20
> <4> [472.618984]  __mutex_lock+0x281/0x9c0
> <4> [472.618987]  ? mark_held_locks+0x49/0x70
> <4> [472.618989]  ? _raw_spin_unlock_irqrestore+0x47/0x60
> <4> [472.619038]  ? i915_vma_unbind+0xae/0x110 [i915]
> <4> [472.619084]  ? i915_vma_unbind+0xae/0x110 [i915]
> <4> [472.619122]  i915_vma_unbind+0xae/0x110 [i915]
> <4> [472.619165]  i915_gem_object_unbind+0x1dc/0x400 [i915]
> <4> [472.619208]  i915_gem_shrink+0x328/0x660 [i915]
> <4> [472.619250]  ? i915_gem_shrink_all+0x38/0x60 [i915]
> <4> [472.619282]  i915_gem_shrink_all+0x38/0x60 [i915]
> <4> [472.619325]  vm_alloc_page.constprop.25+0x1aa/0x240 [i915]
> <4> [472.619330]  ? rcu_read_lock_sched_held+0x4d/0x80
> <4> [472.619363]  ? __alloc_pd+0xb/0x30 [i915]
> <4> [472.619366]  ? module_assert_mutex_or_preempt+0xf/0x30
> <4> [472.619368]  ? __module_address+0x23/0xe0
> <4> [472.619371]  ? is_module_address+0x26/0x40
> <4> [472.619374]  ? static_obj+0x34/0x50
> <4> [472.619376]  ? lockdep_init_map+0x4d/0x1e0
> <4> [472.619407]  setup_page_dma+0xd/0x90 [i915]
> <4> [472.619437]  alloc_pd+0x29/0x50 [i915]
> <4> [472.619470]  __gen8_ppgtt_alloc+0x443/0x6b0 [i915]
> <4> [472.619503]  gen8_ppgtt_alloc+0xd7/0x300 [i915]
> <4> [472.619535]  ppgtt_bind_vma+0x2a/0xe0 [i915]
> <4> [472.619577]  __vma_bind+0x26/0x40 [i915]
> <4> [472.619611]  fence_work+0x1c/0x90 [i915]
> <4> [472.619617]  process_one_work+0x26a/0x620
> 
> Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_vma.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 84e03da0d5f9..2ffc68e18dd0 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -1190,18 +1190,6 @@ int __i915_vma_unbind(struct i915_vma *vma)
>   
>   	lockdep_assert_held(&vma->vm->mutex);
>   
> -	/*
> -	 * First wait upon any activity as retiring the request may
> -	 * have side-effects such as unpinning or even unbinding this vma.
> -	 *
> -	 * XXX Actually waiting under the vm->mutex is a hinderance and
> -	 * should be pipelined wherever possible. In cases where that is
> -	 * unavoidable, we should lift the wait to before the mutex.
> -	 */
> -	ret = i915_vma_sync(vma);
> -	if (ret)
> -		return ret;
> -
>   	if (i915_vma_is_pinned(vma)) {
>   		vma_print_allocator(vma, "is pinned");
>   		return -EAGAIN;
> @@ -1275,6 +1263,11 @@ int i915_vma_unbind(struct i915_vma *vma)
>   	if (!drm_mm_node_allocated(&vma->node))
>   		return 0;
>   
> +	if (i915_vma_is_pinned(vma)) {
> +		vma_print_allocator(vma, "is pinned");
> +		return -EAGAIN;
> +	}
> +
>   	if (i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND))
>   		/* XXX not always required: nop_clear_range */
>   		wakeref = intel_runtime_pm_get(&vm->i915->runtime_pm);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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