Execlists preemption & user priority

Execlists preemption & user priority

[Chris Wilson]

Some refinement from the earlier version, as Michal pointed some
workarounds we needed to prevent an obstructive break in ABI.
Preemption granularity affects how userspace should consider building
batches, so we need to match sure that as we switch preemption on, the
granulatity doesn't change. Sadly, this means we can't enable preemption
on BDW/BSW unconditionally. At least not at this time.

The usecase for this naked series is to enable preemption for pageflips.
It is also exposed to userspace via a context priority, but we reserve
for CAP_SYS_NICE processes for now as we have a very, very raw and
unfair scheduler. But it should enable display servers to squeeze that
last bit of vblank goodness out of the system.
-Chris

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

[PATCH v2 01/11] drm/i915/execlists: Move request unwinding to a separate function

[Chris Wilson]

In the future, we will want to unwind requests following a preemption
point. This requires the same steps as for unwinding upon a reset, so
extract the existing code to a separate function for later use.

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

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 61cac26a8b05..d67907a7e8e6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -348,6 +348,38 @@ lookup_priolist(struct intel_engine_cs *engine,
 	return ptr_pack_bits(p, first, 1);
 }
 
+static void unwind_wa_tail(struct drm_i915_gem_request *rq)
+{
+	rq->tail = intel_ring_wrap(rq->ring,
+				   rq->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
+	assert_ring_tail_valid(rq->ring, rq->tail);
+}
+
+static void unwind_incomplete_requests(struct intel_engine_cs *engine)
+{
+	struct drm_i915_gem_request *rq, *rn;
+
+	lockdep_assert_held(&engine->timeline->lock);
+
+	list_for_each_entry_safe_reverse(rq, rn,
+					 &engine->timeline->requests,
+					 link) {
+		struct i915_priolist *p;
+
+		if (i915_gem_request_completed(rq))
+			return;
+
+		__i915_gem_request_unsubmit(rq);
+		unwind_wa_tail(rq);
+
+		p = lookup_priolist(engine,
+				    &rq->priotree,
+				    rq->priotree.priority);
+		list_add(&rq->priotree.link,
+			 &ptr_mask_bits(p, 1)->requests);
+	}
+}
+
 static inline void
 execlists_context_status_change(struct drm_i915_gem_request *rq,
 				unsigned long status)
@@ -1382,7 +1414,6 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 			      struct drm_i915_gem_request *request)
 {
 	struct intel_engine_execlists * const execlists = &engine->execlists;
-	struct drm_i915_gem_request *rq, *rn;
 	struct intel_context *ce;
 	unsigned long flags;
 
@@ -1400,21 +1431,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	execlist_cancel_port_requests(execlists);
 
 	/* Push back any incomplete requests for replay after the reset. */
-	list_for_each_entry_safe_reverse(rq, rn,
-					 &engine->timeline->requests, link) {
-		struct i915_priolist *p;
-
-		if (i915_gem_request_completed(rq))
-			break;
-
-		__i915_gem_request_unsubmit(rq);
-
-		p = lookup_priolist(engine,
-				    &rq->priotree,
-				    rq->priotree.priority);
-		list_add(&rq->priotree.link,
-			 &ptr_mask_bits(p, 1)->requests);
-	}
+	unwind_incomplete_requests(engine);
 
 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
 
@@ -1451,10 +1468,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	intel_ring_update_space(request->ring);
 
 	/* Reset WaIdleLiteRestore:bdw,skl as well */
-	request->tail =
-		intel_ring_wrap(request->ring,
-				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
-	assert_ring_tail_valid(request->ring, request->tail);
+	unwind_wa_tail(request);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
-- 
2.14.1

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

[PATCH v2 02/11] drm/i915/execlists: Cache the last priolist lookup

[Chris Wilson]

From: Michał Winiarski <michal.winiarski@intel.com>

Avoid the repeated rbtree lookup for each request as we unwind them by
tracking the last priolist.

v2: Fix up my unhelpful suggestion of using default_priolist.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d67907a7e8e6..3998f359d4f0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -358,25 +358,31 @@ static void unwind_wa_tail(struct drm_i915_gem_request *rq)
 static void unwind_incomplete_requests(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *rq, *rn;
+	struct i915_priolist *uninitialized_var(p);
+	int last_prio = INT_MAX;
 
 	lockdep_assert_held(&engine->timeline->lock);
 
 	list_for_each_entry_safe_reverse(rq, rn,
 					 &engine->timeline->requests,
 					 link) {
-		struct i915_priolist *p;
-
 		if (i915_gem_request_completed(rq))
 			return;
 
 		__i915_gem_request_unsubmit(rq);
 		unwind_wa_tail(rq);
 
-		p = lookup_priolist(engine,
-				    &rq->priotree,
-				    rq->priotree.priority);
-		list_add(&rq->priotree.link,
-			 &ptr_mask_bits(p, 1)->requests);
+		GEM_BUG_ON(rq->priotree.priority == INT_MAX);
+		if (rq->priotree.priority != last_prio) {
+			p = lookup_priolist(engine,
+					    &rq->priotree,
+					    rq->priotree.priority);
+			p = ptr_mask_bits(p, 1);
+
+			last_prio = rq->priotree.priority;
+		}
+
+		list_add(&rq->priotree.link, &p->requests);
 	}
 }
 
-- 
2.14.1

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

[PATCH v2 03/11] drm/i915/preempt: Fix WaEnablePreemptionGranularityControlByUMD

[Chris Wilson]

From: Jeff McGee <jeff.mcgee@intel.com>

The WA applies to all production Gen9 and requires both enabling and
whitelisting of the per-context preemption control register.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a28e2a864cf1..af3fe494a429 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1077,7 +1077,9 @@ static int gen9_init_workarounds(struct intel_engine_cs *engine)
 		return ret;
 
 	/* WaEnablePreemptionGranularityControlByUMD:skl,bxt,kbl,cfl */
-	ret= wa_ring_whitelist_reg(engine, GEN8_CS_CHICKEN1);
+	I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
+		   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
+	ret = wa_ring_whitelist_reg(engine, GEN8_CS_CHICKEN1);
 	if (ret)
 		return ret;
 
@@ -1139,14 +1141,6 @@ static int skl_init_workarounds(struct intel_engine_cs *engine)
 	if (ret)
 		return ret;
 
-	/*
-	 * Actual WA is to disable percontext preemption granularity control
-	 * until D0 which is the default case so this is equivalent to
-	 * !WaDisablePerCtxtPreemptionGranularityControl:skl
-	 */
-	I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
-		   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
-
 	/* WaEnableGapsTsvCreditFix:skl */
 	I915_WRITE(GEN8_GARBCNTL, (I915_READ(GEN8_GARBCNTL) |
 				   GEN9_GAPS_TSV_CREDIT_DISABLE));
-- 
2.14.1

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

[PATCH v2 04/11] drm/i915/preempt: Default to disabled mid-command preemption levels

[Chris Wilson]

From: Michał Winiarski <michal.winiarski@intel.com>

Supporting fine-granularity preemption levels may require changes in
userspace batch buffer programming. Therefore, we need to fallback to
safe default values, rather that use hardware defaults. Userspace is
still able to enable fine-granularity, since we're whitelisting the
register controlling it in WaEnablePreemptionGranularityControlByUMD.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_reg.h        |  6 ++++++
 drivers/gpu/drm/i915/intel_engine_cs.c | 17 +++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e4c424ba5905..242970a3c185 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6993,6 +6993,12 @@ enum {
 #define GEN9_CS_DEBUG_MODE1		_MMIO(0x20ec)
 #define GEN9_CTX_PREEMPT_REG		_MMIO(0x2248)
 #define GEN8_CS_CHICKEN1		_MMIO(0x2580)
+#define GEN9_PREEMPT_3D_OBJECT_LEVEL		(1<<0)
+#define GEN9_PREEMPT_GPGPU_LEVEL(hi, lo)	(((hi) << 2) | ((lo) << 1))
+#define GEN9_PREEMPT_GPGPU_MID_THREAD_LEVEL	GEN9_PREEMPT_GPGPU_LEVEL(0, 0)
+#define GEN9_PREEMPT_GPGPU_THREAD_GROUP_LEVEL	GEN9_PREEMPT_GPGPU_LEVEL(0, 1)
+#define GEN9_PREEMPT_GPGPU_COMMAND_LEVEL	GEN9_PREEMPT_GPGPU_LEVEL(1, 0)
+#define GEN9_PREEMPT_GPGPU_LEVEL_MASK		GEN9_PREEMPT_GPGPU_LEVEL(1, 1)
 
 /* GEN7 chicken */
 #define GEN7_COMMON_SLICE_CHICKEN1		_MMIO(0x7010)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index af3fe494a429..21e037fc21b7 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1071,6 +1071,23 @@ static int gen9_init_workarounds(struct intel_engine_cs *engine)
 	I915_WRITE(GEN8_L3SQCREG4, (I915_READ(GEN8_L3SQCREG4) |
 				    GEN8_LQSC_FLUSH_COHERENT_LINES));
 
+	/* Supporting preemption with fine-granularity requires changes in the
+	 * batch buffer programming. Since we can't break old userspace, we
+	 * need to set our default preemption level to safe value. Userspace is
+	 * still able to use more fine-grained preemption levels, since in
+	 * WaEnablePreemptionGranularityControlByUMD we're whitelisting the
+	 * per-ctx register. As such, WaDisableMidCmdPreemption is not a real
+	 * HW workaround, but merely a way to start using preemption while
+	 * maintaining old contract with userspace.
+	 */
+
+	/* WaDisable3DMidCmdPreemption:skl,bxt,glk,cfl */
+	WA_CLR_BIT_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_3D_OBJECT_LEVEL);
+
+	/* WaDisableGPGPUMidCmdPreemption:skl,bxt,blk,cfl */
+	WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
+			    GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);
+
 	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
 	ret = wa_ring_whitelist_reg(engine, GEN9_CTX_PREEMPT_REG);
 	if (ret)
-- 
2.14.1

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

[PATCH v2 05/11] drm/i915/execlists: Distinguish the incomplete context notifies

[Chris Wilson]

Let the listener know that the context we just scheduled out was not
complete, and will be scheduled back in at a later point.

v2: Handle CONTEXT_STATUS_PREEMPTED in gvt by aliasing it to
CONTEXT_STATUS_OUT for the moment, gvt can expand upon the difference
later.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Zhenyu Wang" <zhenyuw@linux.intel.com>
Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gvt/scheduler.c | 1 +
 drivers/gpu/drm/i915/intel_lrc.c     | 2 +-
 drivers/gpu/drm/i915/intel_lrc.h     | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index d5892d24f0b6..f6ded475bb2c 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -174,6 +174,7 @@ static int shadow_context_status_change(struct notifier_block *nb,
 		atomic_set(&workload->shadow_ctx_active, 1);
 		break;
 	case INTEL_CONTEXT_SCHEDULE_OUT:
+	case INTEL_CONTEXT_SCHEDULE_PREEMPTED:
 		atomic_set(&workload->shadow_ctx_active, 0);
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3998f359d4f0..e3d65b49a40d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -618,7 +618,7 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
 	while (num_ports-- && port_isset(port)) {
 		struct drm_i915_gem_request *rq = port_request(port);
 
-		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
+		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
 		i915_gem_request_put(rq);
 
 		memset(port, 0, sizeof(*port));
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 314adee7127a..689fde1a63a9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -61,6 +61,7 @@
 enum {
 	INTEL_CONTEXT_SCHEDULE_IN = 0,
 	INTEL_CONTEXT_SCHEDULE_OUT,
+	INTEL_CONTEXT_SCHEDULE_PREEMPTED,
 };
 
 /* Logical Rings */
-- 
2.14.1

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

[PATCH v2 06/11] drm/i915: Introduce a preempt context

[Chris Wilson]

Add another perma-pinned context for using for preemption at any time.
We cannot just reuse the existing kernel context, as first and foremost
we need to ensure that we can preempt the kernel context itself, so
require a distinct context id. Similar to the kernel context, we may
want to interrupt execution and switch to the preempt context at any
time, and so it needs to be permanently pinned and available.

To compensate for yet another permanent allocation, we shrink the
existing context and the new context by reducing their ringbuffer to the
minimum.

v2: Assert that we never allocate a request from the preemption context.
v3: Limit perma-pin to engines that may preempt.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  4 +++-
 drivers/gpu/drm/i915/i915_gem_context.c | 42 ++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_request.c |  7 ++++++
 drivers/gpu/drm/i915/intel_engine_cs.c  | 22 +++++++++++++++--
 4 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b7cba89080ed..b6794b103255 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -783,6 +783,7 @@ struct intel_csr {
 	func(has_l3_dpf); \
 	func(has_llc); \
 	func(has_logical_ring_contexts); \
+	func(has_logical_ring_preemption); \
 	func(has_overlay); \
 	func(has_pipe_cxsr); \
 	func(has_pooled_eu); \
@@ -2250,8 +2251,9 @@ struct drm_i915_private {
 	wait_queue_head_t gmbus_wait_queue;
 
 	struct pci_dev *bridge_dev;
-	struct i915_gem_context *kernel_context;
 	struct intel_engine_cs *engine[I915_NUM_ENGINES];
+	struct i915_gem_context *kernel_context;
+	struct i915_gem_context *preempt_context;
 	struct i915_vma *semaphore;
 
 	struct drm_dma_handle *status_page_dmah;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 921ee369c74d..1518e110fd04 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -416,14 +416,29 @@ i915_gem_context_create_gvt(struct drm_device *dev)
 	return ctx;
 }
 
+static struct i915_gem_context *
+create_kernel_context(struct drm_i915_private *i915, int prio)
+{
+	struct i915_gem_context *ctx;
+
+	ctx = i915_gem_create_context(i915, NULL);
+	if (IS_ERR(ctx))
+		return ctx;
+
+	i915_gem_context_clear_bannable(ctx);
+	ctx->priority = prio;
+	ctx->ring_size = PAGE_SIZE;
+
+	return ctx;
+}
+
 int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 {
 	struct i915_gem_context *ctx;
 
 	/* Init should only be called once per module load. Eventually the
 	 * restriction on the context_disabled check can be loosened. */
-	if (WARN_ON(dev_priv->kernel_context))
-		return 0;
+	GEM_BUG_ON(dev_priv->kernel_context);
 
 	INIT_LIST_HEAD(&dev_priv->contexts.list);
 	INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
@@ -441,23 +456,30 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 	BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
 	ida_init(&dev_priv->contexts.hw_ida);
 
-	ctx = i915_gem_create_context(dev_priv, NULL);
+	/* lowest priority; idle task */
+	ctx = create_kernel_context(dev_priv, I915_PRIORITY_MIN);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
 			  PTR_ERR(ctx));
 		return PTR_ERR(ctx);
 	}
 
-	/* For easy recognisablity, we want the kernel context to be 0 and then
+	/*
+	 * For easy recognisablity, we want the kernel context to be 0 and then
 	 * all user contexts will have non-zero hw_id.
 	 */
 	GEM_BUG_ON(ctx->hw_id);
-
-	i915_gem_context_clear_bannable(ctx);
-	ctx->priority = I915_PRIORITY_MIN; /* lowest priority; idle task */
+	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
 	dev_priv->kernel_context = ctx;
 
-	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+	/* highest priority; preempting task */
+	ctx = create_kernel_context(dev_priv, INT_MAX);
+	if (IS_ERR(ctx)) {
+		DRM_ERROR("Failed to create default preempt context (error %ld)\n",
+			  PTR_ERR(ctx));
+		return PTR_ERR(ctx);
+	}
+	dev_priv->preempt_context = ctx;
 
 	DRM_DEBUG_DRIVER("%s context support initialized\n",
 			 dev_priv->engine[RCS]->context_size ? "logical" :
@@ -517,6 +539,10 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
 	context_close(ctx);
 	i915_gem_context_free(ctx);
 
+	ctx = i915_gem_context_get(fetch_and_zero(&i915->preempt_context));
+	context_close(ctx);
+	i915_gem_context_free(ctx);
+
 	/* Must free all deferred contexts (via flush_workqueue) first */
 	ida_destroy(&i915->contexts.hw_ida);
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 4eb1a76731b2..010ed68cc21b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -587,6 +587,13 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
+	/*
+	 * Preempt contexts are reserved for exclusive use to inject a
+	 * preemption context switch. They are never to be used for any trivial
+	 * request!
+	 */
+	GEM_BUG_ON(ctx == dev_priv->preempt_context);
+
 	/* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
 	 * EIO if the GPU is already wedged.
 	 */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 21e037fc21b7..02eb25ed1064 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -613,9 +613,22 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 	if (IS_ERR(ring))
 		return PTR_ERR(ring);
 
+	/*
+	 * Similarly the preempt context must always be available so that
+	 * we can interrupt the engine at any time.
+	 */
+	if (INTEL_INFO(engine->i915)->has_logical_ring_preemption) {
+		ring = engine->context_pin(engine,
+					   engine->i915->preempt_context);
+		if (IS_ERR(ring)) {
+			ret = PTR_ERR(ring);
+			goto err_unpin_kernel;
+		}
+	}
+
 	ret = intel_engine_init_breadcrumbs(engine);
 	if (ret)
-		goto err_unpin;
+		goto err_unpin_preempt;
 
 	ret = i915_gem_render_state_init(engine);
 	if (ret)
@@ -634,7 +647,10 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 	i915_gem_render_state_fini(engine);
 err_breadcrumbs:
 	intel_engine_fini_breadcrumbs(engine);
-err_unpin:
+err_unpin_preempt:
+	if (INTEL_INFO(engine->i915)->has_logical_ring_preemption)
+		engine->context_unpin(engine, engine->i915->preempt_context);
+err_unpin_kernel:
 	engine->context_unpin(engine, engine->i915->kernel_context);
 	return ret;
 }
@@ -660,6 +676,8 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	intel_engine_cleanup_cmd_parser(engine);
 	i915_gem_batch_pool_fini(&engine->batch_pool);
 
+	if (INTEL_INFO(engine->i915)->has_logical_ring_preemption)
+		engine->context_unpin(engine, engine->i915->preempt_context);
 	engine->context_unpin(engine, engine->i915->kernel_context);
 }
 
-- 
2.14.1

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

[PATCH v2 07/11] drm/i915/execlists: Move bdw GPGPU w/a to emit_bb

[Chris Wilson]

Move the re-enabling of MI arbitration from a per-bb w/a buffer to the
emission of the batch buffer itself.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e3d65b49a40d..e32109265eb9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1157,24 +1157,6 @@ static u32 *gen8_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 	return batch;
 }
 
-/*
- *  This batch is started immediately after indirect_ctx batch. Since we ensure
- *  that indirect_ctx ends on a cacheline this batch is aligned automatically.
- *
- *  The number of DWORDS written are returned using this field.
- *
- *  This batch is terminated with MI_BATCH_BUFFER_END and so we need not add padding
- *  to align it with cacheline as padding after MI_BATCH_BUFFER_END is redundant.
- */
-static u32 *gen8_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
-{
-	/* WaDisableCtxRestoreArbitration:bdw,chv */
-	*batch++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
-	*batch++ = MI_BATCH_BUFFER_END;
-
-	return batch;
-}
-
 static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 {
 	/* WaFlushCoherentL3CacheLinesAtContextSwitch:skl,bxt,glk */
@@ -1289,7 +1271,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 		break;
 	case 8:
 		wa_bb_fn[0] = gen8_init_indirectctx_bb;
-		wa_bb_fn[1] = gen8_init_perctx_bb;
+		wa_bb_fn[1] = NULL;
 		break;
 	default:
 		MISSING_CASE(INTEL_GEN(engine->i915));
@@ -1533,13 +1515,15 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
+	/* WaDisableCtxRestoreArbitration:bdw,chv */
+	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+
 	/* FIXME(BDW): Address space and security selectors. */
 	*cs++ = MI_BATCH_BUFFER_START_GEN8 |
 		(flags & I915_DISPATCH_SECURE ? 0 : BIT(8)) |
 		(flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0);
 	*cs++ = lower_32_bits(offset);
 	*cs++ = upper_32_bits(offset);
-	*cs++ = MI_NOOP;
 	intel_ring_advance(req, cs);
 
 	return 0;
-- 
2.14.1

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

[PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Chris Wilson]

With preemption, we will want to "unsubmit" a request, taking it back
from the hw and returning it to the priority sorted execution list. In
order to know where to insert it into that list, we need to remember
its adjust priority (which may change even as it was being executed).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e32109265eb9..7ac92a77aea8 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -585,8 +585,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 			}
 
 			INIT_LIST_HEAD(&rq->priotree.link);
-			rq->priotree.priority = INT_MAX;
-
 			__i915_gem_request_submit(rq);
 			trace_i915_gem_request_in(rq, port_index(port, execlists));
 			last = rq;
@@ -794,6 +792,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 				execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
 
 				trace_i915_gem_request_out(rq);
+				rq->priotree.priority = INT_MAX;
 				i915_gem_request_put(rq);
 
 				execlists_port_complete(execlists, port);
@@ -846,11 +845,15 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
 }
 
+static struct drm_i915_gem_request *pt_to_request(struct i915_priotree *pt)
+{
+	return container_of(pt, struct drm_i915_gem_request, priotree);
+}
+
 static struct intel_engine_cs *
 pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked)
 {
-	struct intel_engine_cs *engine =
-		container_of(pt, struct drm_i915_gem_request, priotree)->engine;
+	struct intel_engine_cs *engine = pt_to_request(pt)->engine;
 
 	GEM_BUG_ON(!locked);
 
@@ -904,6 +907,9 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
 		 * engines.
 		 */
 		list_for_each_entry(p, &pt->signalers_list, signal_link) {
+			if (i915_gem_request_completed(pt_to_request(p->signaler)))
+				continue;
+
 			GEM_BUG_ON(p->signaler->priority < pt->priority);
 			if (prio > READ_ONCE(p->signaler->priority))
 				list_move_tail(&p->dfs_link, &dfs);
-- 
2.14.1

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

[PATCH v2 09/11] drm/i915: Expand I915_PARAM_HAS_SCHEDULER into a capability bitmask

[Chris Wilson]

In the next few patches, we wish to enable different features for the
scheduler, some which may subtlety change ABI (e.g. allow requests to be
reordered under different circumstances). So we need to make sure
userspace is cognizant of the changes (if they care), by which we employ
the usual method of a GETPARAM. We already have an
I915_PARAM_HAS_SCHEDULER (which notes the existing ability to reorder
requests to avoid bubbles), and now we wish to extend that to be a
bitmask to describe the different capabilities implemented.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 +++--
 include/uapi/drm/i915_drm.h     | 9 ++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7056bb299dc6..db3438ba92fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -367,8 +367,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = i915_gem_mmap_gtt_version();
 		break;
 	case I915_PARAM_HAS_SCHEDULER:
-		value = dev_priv->engine[RCS] &&
-			dev_priv->engine[RCS]->schedule;
+		value = 0;
+		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule)
+			value |= I915_SCHEDULER_CAP_ENABLED;
 		break;
 	case I915_PARAM_MMAP_VERSION:
 		/* Remember to bump this if the version changes! */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index fe25a01c81f2..aa4a3b20ef6b 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -397,10 +397,17 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_MIN_EU_IN_POOL	 39
 #define I915_PARAM_MMAP_GTT_VERSION	 40
 
-/* Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
+/*
+ * Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
  * priorities and the driver will attempt to execute batches in priority order.
+ * The param returns a capability bitmask, nonzero implies that the scheduler
+ * is enabled, with different features present according to the mask.
  */
 #define I915_PARAM_HAS_SCHEDULER	 41
+#define   I915_SCHEDULER_CAP_ENABLED	(1ul << 0)
+#define   I915_SCHEDULER_CAP_PRIORITY	(1ul << 1)
+#define   I915_SCHEDULER_CAP_PREEMPTION	(1ul << 2)
+
 #define I915_PARAM_HUC_STATUS		 42
 
 /* Query whether DRM_I915_GEM_EXECBUFFER2 supports the ability to opt-out of
-- 
2.14.1

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

[PATCH v2 10/11] drm/i915/execlists: Preemption!

[Chris Wilson]

When we write to ELSP, it triggers a context preemption at the earliest
arbitration point (3DPRIMITIVE, some PIPECONTROLs, a few other
operations and the explicit MI_ARB_CHECK). If this is to the same
context, it triggers a LITE_RESTORE where the RING_TAIL is merely
updated (used currently to chain requests from the same context
together, avoiding bubbles). However, if it is to a different context, a
full context-switch is performed and it will start to execute the new
context saving the image of the old for later execution.

Previously we avoided preemption by only submitting a new context when
the old was idle. But now we wish embrace it, and if the new request has
a higher priority than the currently executing request, we write to the
ELSP regardless, thus triggering preemption, but we tell the GPU to
switch to our special preemption context (not the target). In the
context-switch interrupt handler, we know that the previous contexts
have finished execution and so can unwind all the incomplete requests
and compute the new highest priority request to execute.

It would be feasible to avoid the switch-to-idle intermediate by
programming the ELSP with the target context. The difficulty is in
tracking which request that should be whilst maintaining the dependency
change, the error comes in with coalesced requests. As we only track the
most recent request and its priority, we may run into the issue of being
tricked in preempting a high priority request that was followed by a
low priority request from the same context (e.g. for PI); worse still
that earlier request may be our own dependency and the order then broken
by preemption. By injecting the switch-to-idle and then recomputing the
priority queue, we avoid the issue with tracking in-flight coalesced
requests. Having tried the preempt-to-busy approach, and failed to find
a way around the coalesced priority issue, Michal's original proposal to
inject an idle context (based on handling GuC preemption) succeeds.

The current heuristic for deciding when to preempt are only if the new
request is of higher priority, and has the privileged priority of
greater than 0. Note that the scheduler remains unfair!

v2: Disable for gen8 (bdw/bsw) as we need additional w/a for GPGPU.
Since, the feature is now conditional and not always available when we
have a scheduler, make it known via the HAS_SCHEDULER GETPARAM (now a
capability mask).

Suggested-by: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |   9 ++-
 drivers/gpu/drm/i915/i915_irq.c         |   6 +-
 drivers/gpu/drm/i915/i915_pci.c         |   1 +
 drivers/gpu/drm/i915/intel_lrc.c        | 129 +++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +
 5 files changed, 113 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index db3438ba92fd..d1949b40f419 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -368,9 +368,16 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		break;
 	case I915_PARAM_HAS_SCHEDULER:
 		value = 0;
-		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule)
+		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
 			value |= I915_SCHEDULER_CAP_ENABLED;
+
+			if (INTEL_INFO(dev_priv)->has_logical_ring_preemption &&
+			    i915_modparams.enable_execlists &&
+			    !i915_modparams.enable_guc_submission)
+				value |= I915_SCHEDULER_CAP_PREEMPTION;
+		}
 		break;
+
 	case I915_PARAM_MMAP_VERSION:
 		/* Remember to bump this if the version changes! */
 	case I915_PARAM_HAS_GEM:
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0b7562135d1c..f4bc9ed560b1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1382,10 +1382,8 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
 	bool tasklet = false;
 
 	if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
-		if (port_count(&execlists->port[0])) {
-			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
-			tasklet = true;
-		}
+		__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
+		tasklet = true;
 	}
 
 	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index da60866b6628..931537df54f1 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -470,6 +470,7 @@ static const struct intel_device_info intel_skylake_gt4_info __initconst = {
 	.has_rc6 = 1, \
 	.has_dp_mst = 1, \
 	.has_logical_ring_contexts = 1, \
+	.has_logical_ring_preemption = 1, \
 	.has_guc = 1, \
 	.has_aliasing_ppgtt = 1, \
 	.has_full_ppgtt = 1, \
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7ac92a77aea8..9cdd55e9a9c0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -208,8 +208,8 @@
 
 /* Typical size of the average request (2 pipecontrols and a MI_BB) */
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
-
 #define WA_TAIL_DWORDS 2
+#define PREEMPT_ID 0x1
 
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine);
@@ -489,26 +489,44 @@ static void port_assign(struct execlist_port *port,
 	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
 }
 
+static void inject_preempt_context(struct intel_engine_cs *engine)
+{
+	struct intel_context *ce =
+		&engine->i915->preempt_context->engine[engine->id];
+	u32 __iomem *elsp =
+		engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
+	unsigned int n;
+
+	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
+
+	memset(ce->ring->vaddr + ce->ring->tail, 0, 8);
+	ce->ring->tail += 8;
+	ce->ring->tail &= (ce->ring->size - 1);
+	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
+
+	for (n = execlists_num_ports(&engine->execlists); --n; ) {
+		writel(0, elsp);
+		writel(0, elsp);
+	}
+	writel(upper_32_bits(ce->lrc_desc), elsp);
+	writel(lower_32_bits(ce->lrc_desc), elsp);
+}
+
+static bool can_preempt(struct intel_engine_cs *engine)
+{
+	return INTEL_INFO(engine->i915)->has_logical_ring_preemption;
+}
+
 static void execlists_dequeue(struct intel_engine_cs *engine)
 {
-	struct drm_i915_gem_request *last;
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 	struct execlist_port *port = execlists->port;
 	const struct execlist_port * const last_port =
 		&execlists->port[execlists->port_mask];
+	struct drm_i915_gem_request *last = port_request(port);
 	struct rb_node *rb;
 	bool submit = false;
 
-	last = port_request(port);
-	if (last)
-		/* WaIdleLiteRestore:bdw,skl
-		 * Apply the wa NOOPs to prevent ring:HEAD == req:TAIL
-		 * as we resubmit the request. See gen8_emit_breadcrumb()
-		 * for where we prepare the padding after the end of the
-		 * request.
-		 */
-		last->tail = last->wa_tail;
-
 	/* Hardware submission is through 2 ports. Conceptually each port
 	 * has a (RING_START, RING_HEAD, RING_TAIL) tuple. RING_START is
 	 * static for a context, and unique to each, so we only execute
@@ -533,7 +551,45 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	spin_lock_irq(&engine->timeline->lock);
 	rb = execlists->first;
 	GEM_BUG_ON(rb_first(&execlists->queue) != rb);
-	while (rb) {
+	if (!rb)
+		goto unlock;
+
+	if (last) {
+		/*
+		 * Don't resubmit or switch until all outstanding
+		 * preemptions (lite-restore) are seen. Then we
+		 * know the next preemption status we see corresponds
+		 * to this ELSP update.
+		 */
+		if (port_count(&port[0]) > 1)
+			goto unlock;
+
+		if (can_preempt(engine) &&
+		    rb_entry(rb, struct i915_priolist, node)->priority >
+		    max(last->priotree.priority, 0)) {
+			/*
+			 * Switch to our empty preempt context so
+			 * the state of the GPU is known (idle).
+			 */
+			inject_preempt_context(engine);
+			execlists->preempt = true;
+			goto unlock;
+		} else {
+			if (port_count(&port[1]))
+				goto unlock;
+
+			/* WaIdleLiteRestore:bdw,skl
+			 * Apply the wa NOOPs to prevent
+			 * ring:HEAD == req:TAIL as we resubmit the
+			 * request. See gen8_emit_breadcrumb() for
+			 * where we prepare the padding after the
+			 * end of the request.
+			 */
+			last->tail = last->wa_tail;
+		}
+	}
+
+	do {
 		struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
 		struct drm_i915_gem_request *rq, *rn;
 
@@ -596,11 +652,12 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		INIT_LIST_HEAD(&p->requests);
 		if (p->priority != I915_PRIORITY_NORMAL)
 			kmem_cache_free(engine->i915->priorities, p);
-	}
+	} while (rb);
 done:
 	execlists->first = rb;
 	if (submit)
 		port_assign(port, last);
+unlock:
 	spin_unlock_irq(&engine->timeline->lock);
 
 	if (submit)
@@ -681,13 +738,6 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
 }
 
-static bool execlists_elsp_ready(const struct intel_engine_cs *engine)
-{
-	const struct execlist_port *port = engine->execlists.port;
-
-	return port_count(&port[0]) + port_count(&port[1]) < 2;
-}
-
 /*
  * Check the unread Context Status Buffers and manage the submission of new
  * contexts to the ELSP accordingly.
@@ -696,7 +746,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 {
 	struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
 	struct intel_engine_execlists * const execlists = &engine->execlists;
-	struct execlist_port *port = execlists->port;
+	struct execlist_port * const port = execlists->port;
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	/* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -781,6 +831,23 @@ static void intel_lrc_irq_handler(unsigned long data)
 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
 				continue;
 
+			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
+			    buf[2*head + 1] == PREEMPT_ID) {
+				execlist_cancel_port_requests(execlists);
+
+				spin_lock_irq(&engine->timeline->lock);
+				unwind_incomplete_requests(engine);
+				spin_unlock_irq(&engine->timeline->lock);
+
+				GEM_BUG_ON(!execlists->preempt);
+				execlists->preempt = false;
+				continue;
+			}
+
+			if (status & GEN8_CTX_STATUS_PREEMPTED &&
+			    execlists->preempt)
+				continue;
+
 			/* Check the context/desc id for this event matches */
 			GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
 
@@ -812,7 +879,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 		}
 	}
 
-	if (execlists_elsp_ready(engine))
+	if (!execlists->preempt)
 		execlists_dequeue(engine);
 
 	intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
@@ -825,7 +892,7 @@ static void insert_request(struct intel_engine_cs *engine,
 	struct i915_priolist *p = lookup_priolist(engine, pt, prio);
 
 	list_add_tail(&pt->link, &ptr_mask_bits(p, 1)->requests);
-	if (ptr_unmask_bits(p, 1) && execlists_elsp_ready(engine))
+	if (ptr_unmask_bits(p, 1))
 		tasklet_hi_schedule(&engine->execlists.irq_tasklet);
 }
 
@@ -953,8 +1020,6 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
 	}
 
 	spin_unlock_irq(&engine->timeline->lock);
-
-	/* XXX Do we need to preempt to make room for us and our deps? */
 }
 
 static struct intel_ring *
@@ -1150,6 +1215,8 @@ static u32 *gen8_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 				       i915_ggtt_offset(engine->scratch) +
 				       2 * CACHELINE_BYTES);
 
+	*batch++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+
 	/* Pad to end of cacheline */
 	while ((unsigned long)batch % CACHELINE_BYTES)
 		*batch++ = MI_NOOP;
@@ -1165,6 +1232,8 @@ static u32 *gen8_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 
 static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 {
+	*batch++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
+
 	/* WaFlushCoherentL3CacheLinesAtContextSwitch:skl,bxt,glk */
 	batch = gen8_emit_flush_coherentl3_wa(engine, batch);
 
@@ -1210,6 +1279,8 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 		*batch++ = 0;
 	}
 
+	*batch++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+
 	/* Pad to end of cacheline */
 	while ((unsigned long)batch % CACHELINE_BYTES)
 		*batch++ = MI_NOOP;
@@ -1363,6 +1434,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 		   GT_CONTEXT_SWITCH_INTERRUPT << engine->irq_shift);
 	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
 	execlists->csb_head = -1;
+	execlists->preempt = false;
 
 	/* After a GPU reset, we may have requests to replay */
 	if (!i915_modparams.enable_guc_submission && execlists->first)
@@ -1658,7 +1730,8 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
  */
 static void gen8_emit_wa_tail(struct drm_i915_gem_request *request, u32 *cs)
 {
-	*cs++ = MI_NOOP;
+	/* Ensure there's always at least one preemption point per-request. */
+	*cs++ = MI_ARB_CHECK;
 	*cs++ = MI_NOOP;
 	request->wa_tail = intel_ring_offset(request, cs);
 }
@@ -1679,7 +1752,6 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
 
 	gen8_emit_wa_tail(request, cs);
 }
-
 static const int gen8_emit_breadcrumb_sz = 6 + WA_TAIL_DWORDS;
 
 static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
@@ -1707,7 +1779,6 @@ static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
 
 	gen8_emit_wa_tail(request, cs);
 }
-
 static const int gen8_emit_breadcrumb_render_sz = 8 + WA_TAIL_DWORDS;
 
 static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 56d7ae9f298b..891d9555dce6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -238,6 +238,8 @@ struct intel_engine_execlists {
 #define EXECLIST_MAX_PORTS 2
 	} port[EXECLIST_MAX_PORTS];
 
+	bool preempt;
+
 	/**
 	 * @port_mask: number of execlist ports - 1
 	 */
-- 
2.14.1

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

[PATCH v2 11/11] drm/i915/scheduler: Support user-defined priorities

[Chris Wilson]

Use a priority stored in the context as the initial value when
submitting a request. This allows us to change the default priority on a
per-context basis, allowing different contexts to be favoured with GPU
time at the expense of lower importance work. The user can adjust the
context's priority via I915_CONTEXT_PARAM_PRIORITY, with more positive
values being higher priority (they will be serviced earlier, after their
dependencies have been resolved). Any prerequisite work for an execbuf
will have its priority raised to match the new request as required.

Normal users can specify any value in the range of -1023 to 0 [default],
i.e. they can reduce the priority of their workloads (and temporarily
boost it back to normal if so desired).

Privileged users can specify any value in the range of -1023 to 1023,
[default is 0], i.e. they can raise their priority above all overs and
so potentially starve the system.

Note that the existing schedulers are not fair, nor load balancing, the
execution is strictly by priority on a first-come, first-served basis,
and the driver may choose to boost some requests above the range
available to users.

This priority was originally based around nice(2), but evolved to allow
clients to adjust their priority within a small range, and allow for a
privileged high priority range.

For example, this can be used to implement EGL_IMG_context_priority
https://www.khronos.org/registry/egl/extensions/IMG/EGL_IMG_context_priority.txt

	EGL_CONTEXT_PRIORITY_LEVEL_IMG determines the priority level of
        the context to be created. This attribute is a hint, as an
        implementation may not support multiple contexts at some
        priority levels and system policy may limit access to high
        priority contexts to appropriate system privilege level. The
        default value for EGL_CONTEXT_PRIORITY_LEVEL_IMG is
        EGL_CONTEXT_PRIORITY_MEDIUM_IMG."

so we can map

	PRIORITY_HIGH -> 1023 [privileged, will failback to 0]
	PRIORITY_MED -> 0 [default]
	PRIORITY_LOW -> -1023

They also map onto the priorities used by VkQueue (and a VkQueue is
essentially a timeline, our i915_gem_context under full-ppgtt).

v2: s/CAP_SYS_ADMIN/CAP_SYS_NICE/
v3: Report min/max user priorities as defines in the uapi, and rebase
internal priorities on the exposed values.

Testcase: igt/gem_exec_schedule
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  1 +
 drivers/gpu/drm/i915/i915_gem_context.c | 23 +++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_request.h | 11 ++++++++---
 include/uapi/drm/i915_drm.h             |  7 +++++++
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d1949b40f419..3bdae56b1b8d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -370,6 +370,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = 0;
 		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
 			value |= I915_SCHEDULER_CAP_ENABLED;
+			value |= I915_SCHEDULER_CAP_PRIORITY;
 
 			if (INTEL_INFO(dev_priv)->has_logical_ring_preemption &&
 			    i915_modparams.enable_execlists &&
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 1518e110fd04..08cac18da622 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -1062,6 +1062,9 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_CONTEXT_PARAM_BANNABLE:
 		args->value = i915_gem_context_is_bannable(ctx);
 		break;
+	case I915_CONTEXT_PARAM_PRIORITY:
+		args->value = ctx->priority;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -1117,6 +1120,26 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 		else
 			i915_gem_context_clear_bannable(ctx);
 		break;
+
+	case I915_CONTEXT_PARAM_PRIORITY:
+		{
+			int priority = args->value;
+
+			if (args->size)
+				ret = -EINVAL;
+			else if (!to_i915(dev)->engine[RCS]->schedule)
+				ret = -ENODEV;
+			else if (priority > I915_CONTEXT_MAX_USER_PRIORITY ||
+				 priority < I915_CONTEXT_MIN_USER_PRIORITY)
+				ret = -EINVAL;
+			else if (priority > I915_CONTEXT_DEFAULT_PRIORITY &&
+				 !capable(CAP_SYS_NICE))
+				ret = -EPERM;
+			else
+				ctx->priority = priority;
+		}
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 96eb52471dad..443f9dc0e1b7 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -30,6 +30,8 @@
 #include "i915_gem.h"
 #include "i915_sw_fence.h"
 
+#include <uapi/drm/i915_drm.h>
+
 struct drm_file;
 struct drm_i915_gem_object;
 struct drm_i915_gem_request;
@@ -69,9 +71,12 @@ struct i915_priotree {
 	struct list_head waiters_list; /* those after us, they depend upon us */
 	struct list_head link;
 	int priority;
-#define I915_PRIORITY_MAX 1024
-#define I915_PRIORITY_NORMAL 0
-#define I915_PRIORITY_MIN (-I915_PRIORITY_MAX)
+};
+
+enum {
+	I915_PRIORITY_MIN = I915_CONTEXT_MIN_USER_PRIORITY - 1,
+	I915_PRIORITY_NORMAL = I915_CONTEXT_DEFAULT_PRIORITY,
+	I915_PRIORITY_MAX = I915_CONTEXT_MAX_USER_PRIORITY + 1,
 };
 
 struct i915_gem_capture_list {
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index aa4a3b20ef6b..7266b53191ee 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -402,6 +402,9 @@ typedef struct drm_i915_irq_wait {
  * priorities and the driver will attempt to execute batches in priority order.
  * The param returns a capability bitmask, nonzero implies that the scheduler
  * is enabled, with different features present according to the mask.
+ *
+ * The initial priority for each batch is supplied by the context and is
+ * controlled via I915_CONTEXT_PARAM_PRIORITY.
  */
 #define I915_PARAM_HAS_SCHEDULER	 41
 #define   I915_SCHEDULER_CAP_ENABLED	(1ul << 0)
@@ -1367,6 +1370,10 @@ struct drm_i915_gem_context_param {
 #define I915_CONTEXT_PARAM_GTT_SIZE	0x3
 #define I915_CONTEXT_PARAM_NO_ERROR_CAPTURE	0x4
 #define I915_CONTEXT_PARAM_BANNABLE	0x5
+#define I915_CONTEXT_PARAM_PRIORITY	0x6
+#define   I915_CONTEXT_MAX_USER_PRIORITY	1023 /* inclusive */
+#define   I915_CONTEXT_DEFAULT_PRIORITY		0
+#define   I915_CONTEXT_MIN_USER_PRIORITY	-1023 /* inclusive */
 	__u64 value;
 };
 
-- 
2.14.1

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

✓ Fi.CI.BAT: success for series starting with [v2,01/11] drm/i915/execlists: Move request unwinding to a separate function

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/11] drm/i915/execlists: Move request unwinding to a separate function
URL   : https://patchwork.freedesktop.org/series/30983/
State : success

== Summary ==

Series 30983v1 series starting with [v2,01/11] drm/i915/execlists: Move request unwinding to a separate function
https://patchwork.freedesktop.org/api/1.0/series/30983/revisions/1/mbox/

Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-bdw-5557u) fdo#102473
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-cfl-s) fdo#102294
Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-warn -> PASS       (fi-glk-1) fdo#102777 +1

fdo#102473 https://bugs.freedesktop.org/show_bug.cgi?id=102473
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777

fi-bdw-5557u     total:289  pass:267  dwarn:1   dfail:0   fail:0   skip:21  time:437s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:417s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:516s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:278s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:502s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:496s
fi-cfl-s         total:289  pass:222  dwarn:35  dfail:0   fail:0   skip:32  time:537s
fi-cnl-y         total:289  pass:258  dwarn:0   dfail:0   fail:4   skip:27  time:640s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:426s
fi-glk-1         total:289  pass:259  dwarn:1   dfail:0   fail:0   skip:29  time:561s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:423s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:403s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:463s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:471s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:578s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:589s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:544s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:752s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:487s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:474s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:566s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:414s

aa884e1abdf2ffb7db8c524fc6269280734e5145 drm-tip: 2017y-09m-27d-15h-39m-07s UTC integration manifest
881ebce6c68a drm/i915/scheduler: Support user-defined priorities
7e29babc051f drm/i915/execlists: Preemption!
2ee2b66cbc55 drm/i915: Expand I915_PARAM_HAS_SCHEDULER into a capability bitmask
e440aa1cfb3b drm/i915/execlists: Keep request->priority for its lifetime
87a53d538b53 drm/i915/execlists: Move bdw GPGPU w/a to emit_bb
d45c37f2e91f drm/i915: Introduce a preempt context
fc6e993d1e56 drm/i915/execlists: Distinguish the incomplete context notifies
5f5faf86f76a drm/i915/preempt: Default to disabled mid-command preemption levels
8091c7afe694 drm/i915/preempt: Fix WaEnablePreemptionGranularityControlByUMD
cbd5945306cd drm/i915/execlists: Cache the last priolist lookup
2ab121643f85 drm/i915/execlists: Move request unwinding to a separate function

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [v2,01/11] drm/i915/execlists: Move request unwinding to a separate function

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/11] drm/i915/execlists: Move request unwinding to a separate function
URL   : https://patchwork.freedesktop.org/series/30983/
State : failure

== Summary ==

Test gem_ctx_param:
        Subgroup invalid-param-set:
                pass       -> FAIL       (shard-hsw)
        Subgroup invalid-param-get:
                pass       -> FAIL       (shard-hsw)
Test kms_flip:
        Subgroup flip-vs-wf_vblank-interruptible:
                fail       -> PASS       (shard-hsw)

shard-hsw        total:2429 pass:1329 dwarn:4   dfail:0   fail:13  skip:1083 time:9957s

== Logs ==

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

Re: [PATCH v2 05/11] drm/i915/execlists: Distinguish the incomplete context notifies

[Wang, Zhi A]

Thanks for the patch. :)

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Wednesday, September 27, 2017 7:45 PM
To: intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang <zhenyuw@linux.intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Winiarski, Michal <michal.winiarski@intel.com>; Mika Kuoppala <mika.kuoppala@linux.intel.com>; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>
Subject: [PATCH v2 05/11] drm/i915/execlists: Distinguish the incomplete context notifies

Let the listener know that the context we just scheduled out was not complete, and will be scheduled back in at a later point.

v2: Handle CONTEXT_STATUS_PREEMPTED in gvt by aliasing it to CONTEXT_STATUS_OUT for the moment, gvt can expand upon the difference later.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Zhenyu Wang" <zhenyuw@linux.intel.com>
Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gvt/scheduler.c | 1 +
 drivers/gpu/drm/i915/intel_lrc.c     | 2 +-
 drivers/gpu/drm/i915/intel_lrc.h     | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index d5892d24f0b6..f6ded475bb2c 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -174,6 +174,7 @@ static int shadow_context_status_change(struct notifier_block *nb,
 		atomic_set(&workload->shadow_ctx_active, 1);
 		break;
 	case INTEL_CONTEXT_SCHEDULE_OUT:
+	case INTEL_CONTEXT_SCHEDULE_PREEMPTED:
 		atomic_set(&workload->shadow_ctx_active, 0);
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3998f359d4f0..e3d65b49a40d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -618,7 +618,7 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
 	while (num_ports-- && port_isset(port)) {
 		struct drm_i915_gem_request *rq = port_request(port);
 
-		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
+		execlists_context_status_change(rq, 
+INTEL_CONTEXT_SCHEDULE_PREEMPTED);
 		i915_gem_request_put(rq);
 
 		memset(port, 0, sizeof(*port));
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 314adee7127a..689fde1a63a9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -61,6 +61,7 @@
 enum {
 	INTEL_CONTEXT_SCHEDULE_IN = 0,
 	INTEL_CONTEXT_SCHEDULE_OUT,
+	INTEL_CONTEXT_SCHEDULE_PREEMPTED,
 };
 
 /* Logical Rings */
--
2.14.1

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

Re: [PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Michał Winiarski]

On Wed, Sep 27, 2017 at 04:44:37PM +0000, Chris Wilson wrote:
> With preemption, we will want to "unsubmit" a request, taking it back
> from the hw and returning it to the priority sorted execution list. In
> order to know where to insert it into that list, we need to remember
> its adjust priority (which may change even as it was being executed).

This has a (positive IMO) side effect that should be mentioned here.

Starting from:
drm/i915/execlists: Unwind incomplete requests on resets

We were using the fact, that were overwriting the priority on submit to HW, to
resubmit the same requests on GPU reset.
Since now we keep track of the priority, we're going to 'resubmit' with correct
priority, making reset another "preemption" point. A quicker and more reliable
preemption! Although a bit sad for the requests that were "preempted" this way :)

Please, add the same behavior to GuC submission path.
With that, and expanded commit msg:

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index e32109265eb9..7ac92a77aea8 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -585,8 +585,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  			}
>  
>  			INIT_LIST_HEAD(&rq->priotree.link);
> -			rq->priotree.priority = INT_MAX;
> -
>  			__i915_gem_request_submit(rq);
>  			trace_i915_gem_request_in(rq, port_index(port, execlists));
>  			last = rq;
> @@ -794,6 +792,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>  				execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
>  
>  				trace_i915_gem_request_out(rq);
> +				rq->priotree.priority = INT_MAX;
>  				i915_gem_request_put(rq);
>  
>  				execlists_port_complete(execlists, port);
> @@ -846,11 +845,15 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
>  	spin_unlock_irqrestore(&engine->timeline->lock, flags);
>  }
>  
> +static struct drm_i915_gem_request *pt_to_request(struct i915_priotree *pt)
> +{
> +	return container_of(pt, struct drm_i915_gem_request, priotree);
> +}
> +
>  static struct intel_engine_cs *
>  pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked)
>  {
> -	struct intel_engine_cs *engine =
> -		container_of(pt, struct drm_i915_gem_request, priotree)->engine;
> +	struct intel_engine_cs *engine = pt_to_request(pt)->engine;
>  
>  	GEM_BUG_ON(!locked);
>  
> @@ -904,6 +907,9 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
>  		 * engines.
>  		 */
>  		list_for_each_entry(p, &pt->signalers_list, signal_link) {
> +			if (i915_gem_request_completed(pt_to_request(p->signaler)))
> +				continue;
> +
>  			GEM_BUG_ON(p->signaler->priority < pt->priority);
>  			if (prio > READ_ONCE(p->signaler->priority))
>  				list_move_tail(&p->dfs_link, &dfs);
> -- 
> 2.14.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Chris Wilson]

Quoting Michał Winiarski (2017-09-28 10:14:47)
> On Wed, Sep 27, 2017 at 04:44:37PM +0000, Chris Wilson wrote:
> > With preemption, we will want to "unsubmit" a request, taking it back
> > from the hw and returning it to the priority sorted execution list. In
> > order to know where to insert it into that list, we need to remember
> > its adjust priority (which may change even as it was being executed).
> 
> This has a (positive IMO) side effect that should be mentioned here.
> 
> Starting from:
> drm/i915/execlists: Unwind incomplete requests on resets
> 
> We were using the fact, that were overwriting the priority on submit to HW, to
> resubmit the same requests on GPU reset.
> Since now we keep track of the priority, we're going to 'resubmit' with correct
> priority, making reset another "preemption" point. A quicker and more reliable
> preemption! Although a bit sad for the requests that were "preempted" this way :)

Bah. I am not keen that reset acts as an accidental preemption point.
Hence why I kept the behaviour of resubmitting exactly was on the hw at
the time of reset to avoid the notion of preemption happening before we
were expecting any.
 
> Please, add the same behavior to GuC submission path.

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

Re: [PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Chris Wilson]

Quoting Chris Wilson (2017-09-28 10:31:00)
> Quoting Michał Winiarski (2017-09-28 10:14:47)
> > Please, add the same behavior to GuC submission path.
> 
> Why?

To expand on this, there is a large negative cost in removing the
dfs optimisation. What benefit does it bring to the execution when there
is no preemption? Afaict, none.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Chris Wilson]

Quoting Chris Wilson (2017-09-27 17:44:37)
> With preemption, we will want to "unsubmit" a request, taking it back
> from the hw and returning it to the priority sorted execution list. In
> order to know where to insert it into that list, we need to remember
> its adjust priority (which may change even as it was being executed).
s/adjust/adjusted/

"This also affects reset for execlists as we are now unsubmitting the
requests following the reset (rather than directly writing the ELSP for
the inflight contexts). This turns reset into an accidental preemption
point, as after the reset we may choose a different pair of contexts to
submit to hw.

GuC is not updated as this series doesn't add preemption to the GuC
submission, and so it can keep benefiting from the early pruning of the
DFS inside execlists_schedule() for a little longer. We also need to
find a way of reducing the cost of that DFS..."

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

Re: [PATCH v2 01/11] drm/i915/execlists: Move request unwinding to a separate function

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> In the future, we will want to unwind requests following a preemption
> point. This requires the same steps as for unwinding upon a reset, so
> extract the existing code to a separate function for later use.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/11] drm/i915/execlists: Cache the last priolist lookup

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> From: Michał Winiarski <michal.winiarski@intel.com>
> 
> Avoid the repeated rbtree lookup for each request as we unwind them by
> tracking the last priolist.
> 
> v2: Fix up my unhelpful suggestion of using default_priolist.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -358,25 +358,31 @@ static void unwind_wa_tail(struct drm_i915_gem_request *rq)
>  static void unwind_incomplete_requests(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_gem_request *rq, *rn;
> +	struct i915_priolist *uninitialized_var(p);
> +	int last_prio = INT_MAX;
>  
>  	lockdep_assert_held(&engine->timeline->lock);
>  
>  	list_for_each_entry_safe_reverse(rq, rn,
>  					 &engine->timeline->requests,
>  					 link) {
> -		struct i915_priolist *p;
> -
>  		if (i915_gem_request_completed(rq))
>  			return;
>  
>  		__i915_gem_request_unsubmit(rq);
>  		unwind_wa_tail(rq);
>  
> -		p = lookup_priolist(engine,
> -				    &rq->priotree,
> -				    rq->priotree.priority);
> -		list_add(&rq->priotree.link,
> -			 &ptr_mask_bits(p, 1)->requests);
> +		GEM_BUG_ON(rq->priotree.priority == INT_MAX);

This doesn't read aloud too logically when coming from the ring reset
codepath, at first like would seem like we're not allowing maximum
priority tasks to be unwinded (which only makes sense on the pre-empt
odepath). #define INVALID_PRIORITY INT_MAX might help

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/11] drm/i915/execlists: Cache the last priolist lookup

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-28 12:59:01)
> On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> > From: Michał Winiarski <michal.winiarski@intel.com>
> > 
> > Avoid the repeated rbtree lookup for each request as we unwind them by
> > tracking the last priolist.
> > 
> > v2: Fix up my unhelpful suggestion of using default_priolist.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -358,25 +358,31 @@ static void unwind_wa_tail(struct drm_i915_gem_request *rq)
> >  static void unwind_incomplete_requests(struct intel_engine_cs *engine)
> >  {
> >       struct drm_i915_gem_request *rq, *rn;
> > +     struct i915_priolist *uninitialized_var(p);
> > +     int last_prio = INT_MAX;
> >  
> >       lockdep_assert_held(&engine->timeline->lock);
> >  
> >       list_for_each_entry_safe_reverse(rq, rn,
> >                                        &engine->timeline->requests,
> >                                        link) {
> > -             struct i915_priolist *p;
> > -
> >               if (i915_gem_request_completed(rq))
> >                       return;
> >  
> >               __i915_gem_request_unsubmit(rq);
> >               unwind_wa_tail(rq);
> >  
> > -             p = lookup_priolist(engine,
> > -                                 &rq->priotree,
> > -                                 rq->priotree.priority);
> > -             list_add(&rq->priotree.link,
> > -                      &ptr_mask_bits(p, 1)->requests);
> > +             GEM_BUG_ON(rq->priotree.priority == INT_MAX);
> 
> This doesn't read aloud too logically when coming from the ring reset
> codepath, at first like would seem like we're not allowing maximum
> priority tasks to be unwinded (which only makes sense on the pre-empt
> odepath). #define INVALID_PRIORITY INT_MAX might help

And adding the GEM_BUG_ON() to execlists_schedule() to make sure we
never try to set it as well.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 03/11] drm/i915/preempt: Fix WaEnablePreemptionGranularityControlByUMD

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> The WA applies to all production Gen9 and requires both enabling and
> whitelisting of the per-context preemption control register.
> 
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a28e2a864cf1..af3fe494a429 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1077,7 +1077,9 @@ static int gen9_init_workarounds(struct intel_engine_cs *engine)
>  		return ret;
>  
>  	/* WaEnablePreemptionGranularityControlByUMD:skl,bxt,kbl,cfl */

To have the record correct, this applies to CNL too.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 04/11] drm/i915/preempt: Default to disabled mid-command preemption levels

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> From: Michał Winiarski <michal.winiarski@intel.com>
> 
> Supporting fine-granularity preemption levels may require changes in
> userspace batch buffer programming. Therefore, we need to fallback to
> safe default values, rather that use hardware defaults. Userspace is
> still able to enable fine-granularity, since we're whitelisting the
> register controlling it in WaEnablePreemptionGranularityControlByUMD.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +	/* Supporting preemption with fine-granularity requires changes in the
> +	 * batch buffer programming. Since we can't break old userspace, we
> +	 * need to set our default preemption level to safe value. Userspace is
> +	 * still able to use more fine-grained preemption levels, since in
> +	 * WaEnablePreemptionGranularityControlByUMD we're whitelisting the
> +	 * per-ctx register. As such, WaDisableMidCmdPreemption is not a real
> +	 * HW workaround, but merely a way to start using preemption while
> +	 * maintaining old contract with userspace.
> +	 */
> +
> +	/* WaDisable3DMidCmdPreemption:skl,bxt,glk,cfl */
> +	WA_CLR_BIT_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_3D_OBJECT_LEVEL);
> +
> +	/* WaDisableGPGPUMidCmdPreemption:skl,bxt,blk,cfl */
> +	WA_SET_FIELD_MASKED(GEN8_CS_CHICKEN1, GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> +			    GEN9_PREEMPT_GPGPU_COMMAND_LEVEL);

Lets avoid confusion by not inventing Wa names (It's not in the
database at least). This also applies to pretty much any new HW,
including CNL like the previous actual W/A.

Other than that, this is the correct thing to do so drop the false
name, and just leave this as a remark which references the other W/A.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 05/11] drm/i915/execlists: Distinguish the incomplete context notifies

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> Let the listener know that the context we just scheduled out was not
> complete, and will be scheduled back in at a later point.
> 
> v2: Handle CONTEXT_STATUS_PREEMPTED in gvt by aliasing it to
> CONTEXT_STATUS_OUT for the moment, gvt can expand upon the difference
> later.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Zhenyu Wang" <zhenyuw@linux.intel.com>
> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 06/11] drm/i915: Introduce a preempt context

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> Add another perma-pinned context for using for preemption at any time.
> We cannot just reuse the existing kernel context, as first and foremost
> we need to ensure that we can preempt the kernel context itself, so
> require a distinct context id. Similar to the kernel context, we may
> want to interrupt execution and switch to the preempt context at any
> time, and so it needs to be permanently pinned and available.
> 
> To compensate for yet another permanent allocation, we shrink the
> existing context and the new context by reducing their ringbuffer to the
> minimum.
> 
> v2: Assert that we never allocate a request from the preemption context.
> v3: Limit perma-pin to engines that may preempt.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

<SNIP>

> @@ -2250,8 +2251,9 @@ struct drm_i915_private {
>  	wait_queue_head_t gmbus_wait_queue;
>  
>  	struct pci_dev *bridge_dev;
> -	struct i915_gem_context *kernel_context;
>  	struct intel_engine_cs *engine[I915_NUM_ENGINES];

First to touch old code gets to add kerneldoc, just formulate what's in
the commit message into here.

> +	struct i915_gem_context *kernel_context;
> +	struct i915_gem_context *preempt_context;
>  	struct i915_vma *semaphore;
>  
>  	struct drm_dma_handle *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 921ee369c74d..1518e110fd04 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -416,14 +416,29 @@ i915_gem_context_create_gvt(struct drm_device *dev)
>  	return ctx;
>  }
>  
> +static struct i915_gem_context *
> +create_kernel_context(struct drm_i915_private *i915, int prio)

I may vote for 'create_internal_context' because 'kernel context' is
quite an established term. But I've got no hard option, just gotta keep
the terminology coherent (eg. in the above requested kerneldoc).

> +{
> +	struct i915_gem_context *ctx;
> +
> +	ctx = i915_gem_create_context(i915, NULL);
> +	if (IS_ERR(ctx))
> +		return ctx;
> +
> +	i915_gem_context_clear_bannable(ctx);
> +	ctx->priority = prio;
> +	ctx->ring_size = PAGE_SIZE;
> +
> +	return ctx;
> +}
> +
>  int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_gem_context *ctx;
>  
>  	/* Init should only be called once per module load. Eventually the
>  	 * restriction on the context_disabled check can be loosened. */

Commit seems to be out of date now?

> @@ -441,23 +456,30 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
>  	BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
>  	ida_init(&dev_priv->contexts.hw_ida);
>  
> -	ctx = i915_gem_create_context(dev_priv, NULL);
> +	/* lowest priority; idle task */
> +	ctx = create_kernel_context(dev_priv, I915_PRIORITY_MIN);
>  	if (IS_ERR(ctx)) {
>  		DRM_ERROR("Failed to create default global context (error %ld)\n",
>  			  PTR_ERR(ctx));
>  		return PTR_ERR(ctx);
>  	}
>  
> -	/* For easy recognisablity, we want the kernel context to be 0 and then
> +	/*
> +	 * For easy recognisablity, we want the kernel context to be 0 and then
>  	 * all user contexts will have non-zero hw_id.
>  	 */
>  	GEM_BUG_ON(ctx->hw_id);
> -
> -	i915_gem_context_clear_bannable(ctx);
> -	ctx->priority = I915_PRIORITY_MIN; /* lowest priority; idle task */
> +	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
>  	dev_priv->kernel_context = ctx;
>  
> -	GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> +	/* highest priority; preempting task */
> +	ctx = create_kernel_context(dev_priv, INT_MAX);
> +	if (IS_ERR(ctx)) {
> +		DRM_ERROR("Failed to create default preempt context (error %ld)\n",
> +			  PTR_ERR(ctx));

There's no onion teardown so kernel_context is not freed. Pleas add

> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -587,6 +587,13 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  
>  	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>  
> +	/*
> +	 * Preempt contexts are reserved for exclusive use to inject a
> +	 * preemption context switch. They are never to be used for any trivial
> +	 * request!
> +	 */
> +	GEM_BUG_ON(ctx == dev_priv->preempt_context);

Maybe check the prio here too.

> +
>  	/* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
>  	 * EIO if the GPU is already wedged.
>  	 */
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 21e037fc21b7..02eb25ed1064 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -613,9 +613,22 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
>  	if (IS_ERR(ring))
>  		return PTR_ERR(ring);
>  
> +	/*
> +	 * Similarly the preempt context must always be available so that
> +	 * we can interrupt the engine at any time.
> +	 */
> +	if (INTEL_INFO(engine->i915)->has_logical_ring_preemption) {
> +		ring = engine->context_pin(engine,
> +					   engine->i915->preempt_context);
> +		if (IS_ERR(ring)) {
> +			ret = PTR_ERR(ring);
> +			goto err_unpin_kernel;
> +		}
> +	}

Maybe add helper for the internal/kernel_context_pin and _free too,
it's unnecessary code duplication.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 06/11] drm/i915: Introduce a preempt context

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-28 13:32:09)
> On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> > Add another perma-pinned context for using for preemption at any time.
> > We cannot just reuse the existing kernel context, as first and foremost
> > we need to ensure that we can preempt the kernel context itself, so
> > require a distinct context id. Similar to the kernel context, we may
> > want to interrupt execution and switch to the preempt context at any
> > time, and so it needs to be permanently pinned and available.
> > 
> > To compensate for yet another permanent allocation, we shrink the
> > existing context and the new context by reducing their ringbuffer to the
> > minimum.
> > 
> > v2: Assert that we never allocate a request from the preemption context.
> > v3: Limit perma-pin to engines that may preempt.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> <SNIP>
> 
> > @@ -2250,8 +2251,9 @@ struct drm_i915_private {
> >       wait_queue_head_t gmbus_wait_queue;
> >  
> >       struct pci_dev *bridge_dev;
> > -     struct i915_gem_context *kernel_context;
> >       struct intel_engine_cs *engine[I915_NUM_ENGINES];
> 
> First to touch old code gets to add kerneldoc, just formulate what's in
> the commit message into here.

/* Nothing to see here, please move along. */
 
> > +     struct i915_gem_context *kernel_context;
> > +     struct i915_gem_context *preempt_context;
> >       struct i915_vma *semaphore;
> >  
> >       struct drm_dma_handle *status_page_dmah;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 921ee369c74d..1518e110fd04 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -416,14 +416,29 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> >       return ctx;
> >  }
> >  
> > +static struct i915_gem_context *
> > +create_kernel_context(struct drm_i915_private *i915, int prio)
> 
> I may vote for 'create_internal_context' because 'kernel context' is
> quite an established term. But I've got no hard option, just gotta keep
> the terminology coherent (eg. in the above requested kerneldoc).

The contexts return by this function pass i915_gem_context_is_kernel().
Atm, I think kernel_context is still the consistent terminology.
 
> > +{
> > +     struct i915_gem_context *ctx;
> > +
> > +     ctx = i915_gem_create_context(i915, NULL);
> > +     if (IS_ERR(ctx))
> > +             return ctx;
> > +
> > +     i915_gem_context_clear_bannable(ctx);
> > +     ctx->priority = prio;
> > +     ctx->ring_size = PAGE_SIZE;
> > +
> > +     return ctx;
> > +}
> > +
> >  int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> >  {
> >       struct i915_gem_context *ctx;
> >  
> >       /* Init should only be called once per module load. Eventually the
> >        * restriction on the context_disabled check can be loosened. */
> 
> Commit seems to be out of date now?

Got to keep some humour around.

> > +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> > @@ -587,6 +587,13 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> >  
> >       lockdep_assert_held(&dev_priv->drm.struct_mutex);
> >  
> > +     /*
> > +      * Preempt contexts are reserved for exclusive use to inject a
> > +      * preemption context switch. They are never to be used for any trivial
> > +      * request!
> > +      */
> > +     GEM_BUG_ON(ctx == dev_priv->preempt_context);
> 
> Maybe check the prio here too.

More a task for ->schedule().

> 
> > +
> >       /* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
> >        * EIO if the GPU is already wedged.
> >        */
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 21e037fc21b7..02eb25ed1064 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -613,9 +613,22 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> >       if (IS_ERR(ring))
> >               return PTR_ERR(ring);
> >  
> > +     /*
> > +      * Similarly the preempt context must always be available so that
> > +      * we can interrupt the engine at any time.
> > +      */
> > +     if (INTEL_INFO(engine->i915)->has_logical_ring_preemption) {
> > +             ring = engine->context_pin(engine,
> > +                                        engine->i915->preempt_context);
> > +             if (IS_ERR(ring)) {
> > +                     ret = PTR_ERR(ring);
> > +                     goto err_unpin_kernel;
> > +             }
> > +     }
> 
> Maybe add helper for the internal/kernel_context_pin and _free too,
> it's unnecessary code duplication.

What's the duplication? I'm not clear on what may be saved.

engine->context_pin()
engine->context_unpin()
??

The cleanup path I have more sympathy for, I considered doing it. But I
keep getting side tracked by the desire to eliminate the context_close()
for the kernel contexts and creating them in a closed state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 07/11] drm/i915/execlists: Move bdw GPGPU w/a to emit_bb

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> Move the re-enabling of MI arbitration from a per-bb w/a buffer to the
> emission of the batch buffer itself.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

There's something about 64b addressing mode requiring disabling ARB
during the BB_START on pre-BDW machines, but shouldn't affect
production machines.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> With preemption, we will want to "unsubmit" a request, taking it back
> from the hw and returning it to the priority sorted execution list. In
> order to know where to insert it into that list, we need to remember
> its adjust priority (which may change even as it was being executed).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>

There seems to be a whole lot of discussion around it, but this patch
itself does what it describes.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

The loop can be optimized once it's working.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 09/11] drm/i915: Expand I915_PARAM_HAS_SCHEDULER into a capability bitmask

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> In the next few patches, we wish to enable different features for the
> scheduler, some which may subtlety change ABI (e.g. allow requests to be
> reordered under different circumstances). So we need to make sure
> userspace is cognizant of the changes (if they care), by which we employ
> the usual method of a GETPARAM. We already have an
> I915_PARAM_HAS_SCHEDULER (which notes the existing ability to reorder
> requests to avoid bubbles), and now we wish to extend that to be a
> bitmask to describe the different capabilities implemented.

Please link the last Mesa series here, as this is the patch bringing in
the ABI.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +++ b/include/uapi/drm/i915_drm.h
> @@ -397,10 +397,17 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_MIN_EU_IN_POOL	 39
>  #define I915_PARAM_MMAP_GTT_VERSION	 40
>  
> -/* Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
> +/*
> + * Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
>   * priorities and the driver will attempt to execute batches in priority order.
> + * The param returns a capability bitmask, nonzero implies that the scheduler
> + * is enabled, with different features present according to the mask.
>   */
>  #define I915_PARAM_HAS_SCHEDULER	 41
> +#define   I915_SCHEDULER_CAP_ENABLED	(1ul << 0)
> +#define   I915_SCHEDULER_CAP_PRIORITY	(1ul << 1)
> +#define   I915_SCHEDULER_CAP_PREEMPTION	(1ul << 2)

There seems to be the occasional BIT() hanging under uapi (I seem to
have added the first for i915.h). So maybe we need to revert that or
convert these.

Either way;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 09/11] drm/i915: Expand I915_PARAM_HAS_SCHEDULER into a capability bitmask

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-28 14:07:35)
> On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> > In the next few patches, we wish to enable different features for the
> > scheduler, some which may subtlety change ABI (e.g. allow requests to be
> > reordered under different circumstances). So we need to make sure
> > userspace is cognizant of the changes (if they care), by which we employ
> > the usual method of a GETPARAM. We already have an
> > I915_PARAM_HAS_SCHEDULER (which notes the existing ability to reorder
> > requests to avoid bubbles), and now we wish to extend that to be a
> > bitmask to describe the different capabilities implemented.
> 
> Please link the last Mesa series here, as this is the patch bringing in
> the ABI.
> 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -397,10 +397,17 @@ typedef struct drm_i915_irq_wait {
> >  #define I915_PARAM_MIN_EU_IN_POOL     39
> >  #define I915_PARAM_MMAP_GTT_VERSION   40
> >  
> > -/* Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
> > +/*
> > + * Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
> >   * priorities and the driver will attempt to execute batches in priority order.
> > + * The param returns a capability bitmask, nonzero implies that the scheduler
> > + * is enabled, with different features present according to the mask.
> >   */
> >  #define I915_PARAM_HAS_SCHEDULER      41
> > +#define   I915_SCHEDULER_CAP_ENABLED (1ul << 0)
> > +#define   I915_SCHEDULER_CAP_PRIORITY        (1ul << 1)
> > +#define   I915_SCHEDULER_CAP_PREEMPTION      (1ul << 2)
> 
> There seems to be the occasional BIT() hanging under uapi (I seem to
> have added the first for i915.h). So maybe we need to revert that or
> convert these.

We don't have BIT() in uapi yet, afaik. Plenty of places where we could
benefit.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 08/11] drm/i915/execlists: Keep request->priority for its lifetime

[Michał Winiarski]

On Thu, Sep 28, 2017 at 11:09:14AM +0000, Chris Wilson wrote:
> Quoting Chris Wilson (2017-09-27 17:44:37)
> > With preemption, we will want to "unsubmit" a request, taking it back
> > from the hw and returning it to the priority sorted execution list. In
> > order to know where to insert it into that list, we need to remember
> > its adjust priority (which may change even as it was being executed).
> s/adjust/adjusted/
> 
> "This also affects reset for execlists as we are now unsubmitting the
> requests following the reset (rather than directly writing the ELSP for
> the inflight contexts). This turns reset into an accidental preemption
> point, as after the reset we may choose a different pair of contexts to
> submit to hw.
> 
> GuC is not updated as this series doesn't add preemption to the GuC
> submission, and so it can keep benefiting from the early pruning of the
> DFS inside execlists_schedule() for a little longer. We also need to
> find a way of reducing the cost of that DFS..."
> 

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

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

Re: [PATCH v2 10/11] drm/i915/execlists: Preemption!

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> When we write to ELSP, it triggers a context preemption at the earliest
> arbitration point (3DPRIMITIVE, some PIPECONTROLs, a few other
> operations and the explicit MI_ARB_CHECK). If this is to the same
> context, it triggers a LITE_RESTORE where the RING_TAIL is merely
> updated (used currently to chain requests from the same context
> together, avoiding bubbles). However, if it is to a different context, a
> full context-switch is performed and it will start to execute the new
> context saving the image of the old for later execution.
> 
> Previously we avoided preemption by only submitting a new context when
> the old was idle. But now we wish embrace it, and if the new request has
> a higher priority than the currently executing request, we write to the
> ELSP regardless, thus triggering preemption, but we tell the GPU to
> switch to our special preemption context (not the target). In the
> context-switch interrupt handler, we know that the previous contexts
> have finished execution and so can unwind all the incomplete requests
> and compute the new highest priority request to execute.
> 
> It would be feasible to avoid the switch-to-idle intermediate by
> programming the ELSP with the target context. The difficulty is in
> tracking which request that should be whilst maintaining the dependency
> change, the error comes in with coalesced requests. As we only track the
> most recent request and its priority, we may run into the issue of being
> tricked in preempting a high priority request that was followed by a
> low priority request from the same context (e.g. for PI);

"followed" is bit ambiguous here, depending on how you view the
ordering, wall time or ports.

> worse still
> that earlier request may be our own dependency and the order then broken
> by preemption. By injecting the switch-to-idle and then recomputing the
> priority queue, we avoid the issue with tracking in-flight coalesced
> requests. Having tried the preempt-to-busy approach, and failed to find
> a way around the coalesced priority issue, Michal's original proposal to
> inject an idle context (based on handling GuC preemption) succeeds.
> 
> The current heuristic for deciding when to preempt are only if the new
> request is of higher priority, and has the privileged priority of
> greater than 0. Note that the scheduler remains unfair!
> 
> v2: Disable for gen8 (bdw/bsw) as we need additional w/a for GPGPU.
> Since, the feature is now conditional and not always available when we
> have a scheduler, make it known via the HAS_SCHEDULER GETPARAM (now a
> capability mask).
> 
> Suggested-by: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>

<SNIP>

> @@ -489,26 +489,44 @@ static void port_assign(struct execlist_port *port,
>  	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
>  }
>  
> +static void inject_preempt_context(struct intel_engine_cs *engine)
> +{
> +	struct intel_context *ce =
> +		&engine->i915->preempt_context->engine[engine->id];
> +	u32 __iomem *elsp =
> +		engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));

engine_elsp() helper or so?

> +	unsigned int n;
> +
> +	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);

I think this could/should be done way earlier?

> +
> +	memset(ce->ring->vaddr + ce->ring->tail, 0, 8);
> +	ce->ring->tail += 8;
> +	ce->ring->tail &= (ce->ring->size - 1);
> +	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;

An awful lot of pre-expectations here, would be shame if somebody
documented them.

> +
> +	for (n = execlists_num_ports(&engine->execlists); --n; ) {

This is fine detail compared to the other loop, "<=" vs "<" (or maybe
even <= -1) would make a more clear distinction, but I'm not arguing.

> +		writel(0, elsp);
> +		writel(0, elsp);
> +	}
> +	writel(upper_32_bits(ce->lrc_desc), elsp);
> +	writel(lower_32_bits(ce->lrc_desc), elsp);

Could also be elsp_write inline helper.

> @@ -696,7 +746,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>  {
>  	struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
>  	struct intel_engine_execlists * const execlists = &engine->execlists;
> -	struct execlist_port *port = execlists->port;
> +	struct execlist_port * const port = execlists->port;
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	/* We can skip acquiring intel_runtime_pm_get() here as it was taken
> @@ -781,6 +831,23 @@ static void intel_lrc_irq_handler(unsigned long data)
>  			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>  				continue;
>  
> +			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
> +			    buf[2*head + 1] == PREEMPT_ID) {

(2 * head + 1), could be a helper again, potentially with a descriptive
name so the below comment can be removed and doesn't need to be
duplicated :)

> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -238,6 +238,8 @@ struct intel_engine_execlists {
>  #define EXECLIST_MAX_PORTS 2
>  	} port[EXECLIST_MAX_PORTS];
>  
> +	bool preempt;

Definitely want to improve the variable name, "preempting" would be the
smallest acceptable change.

Combine that with kerneldoc here and fix at memset(), this is

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/11] drm/i915/scheduler: Support user-defined priorities

[Joonas Lahtinen]

On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> Use a priority stored in the context as the initial value when
> submitting a request. This allows us to change the default priority on a
> per-context basis, allowing different contexts to be favoured with GPU
> time at the expense of lower importance work. The user can adjust the
> context's priority via I915_CONTEXT_PARAM_PRIORITY, with more positive
> values being higher priority (they will be serviced earlier, after their
> dependencies have been resolved). Any prerequisite work for an execbuf
> will have its priority raised to match the new request as required.
> 
> Normal users can specify any value in the range of -1023 to 0 [default],
> i.e. they can reduce the priority of their workloads (and temporarily
> boost it back to normal if so desired).
> 
> Privileged users can specify any value in the range of -1023 to 1023,
> [default is 0], i.e. they can raise their priority above all overs and
> so potentially starve the system.
> 
> Note that the existing schedulers are not fair, nor load balancing, the
> execution is strictly by priority on a first-come, first-served basis,
> and the driver may choose to boost some requests above the range
> available to users.
> 
> This priority was originally based around nice(2), but evolved to allow
> clients to adjust their priority within a small range, and allow for a
> privileged high priority range.
> 
> For example, this can be used to implement EGL_IMG_context_priority
> https://www.khronos.org/registry/egl/extensions/IMG/EGL_IMG_context_priority.txt
> 
> 	EGL_CONTEXT_PRIORITY_LEVEL_IMG determines the priority level of
>         the context to be created. This attribute is a hint, as an
>         implementation may not support multiple contexts at some
>         priority levels and system policy may limit access to high
>         priority contexts to appropriate system privilege level. The
>         default value for EGL_CONTEXT_PRIORITY_LEVEL_IMG is
>         EGL_CONTEXT_PRIORITY_MEDIUM_IMG."
> 
> so we can map
> 
> 	PRIORITY_HIGH -> 1023 [privileged, will failback to 0]
> 	PRIORITY_MED -> 0 [default]
> 	PRIORITY_LOW -> -1023
> 
> They also map onto the priorities used by VkQueue (and a VkQueue is
> essentially a timeline, our i915_gem_context under full-ppgtt).
> 
> v2: s/CAP_SYS_ADMIN/CAP_SYS_NICE/
> v3: Report min/max user priorities as defines in the uapi, and rebase
> internal priorities on the exposed values.
> 
> Testcase: igt/gem_exec_schedule
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We should be good to go once Mesa is.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 10/11] drm/i915/execlists: Preemption!

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-28 15:10:03)
> On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> > When we write to ELSP, it triggers a context preemption at the earliest
> > arbitration point (3DPRIMITIVE, some PIPECONTROLs, a few other
> > operations and the explicit MI_ARB_CHECK). If this is to the same
> > context, it triggers a LITE_RESTORE where the RING_TAIL is merely
> > updated (used currently to chain requests from the same context
> > together, avoiding bubbles). However, if it is to a different context, a
> > full context-switch is performed and it will start to execute the new
> > context saving the image of the old for later execution.
> > 
> > Previously we avoided preemption by only submitting a new context when
> > the old was idle. But now we wish embrace it, and if the new request has
> > a higher priority than the currently executing request, we write to the
> > ELSP regardless, thus triggering preemption, but we tell the GPU to
> > switch to our special preemption context (not the target). In the
> > context-switch interrupt handler, we know that the previous contexts
> > have finished execution and so can unwind all the incomplete requests
> > and compute the new highest priority request to execute.
> > 
> > It would be feasible to avoid the switch-to-idle intermediate by
> > programming the ELSP with the target context. The difficulty is in
> > tracking which request that should be whilst maintaining the dependency
> > change, the error comes in with coalesced requests. As we only track the
> > most recent request and its priority, we may run into the issue of being
> > tricked in preempting a high priority request that was followed by a
> > low priority request from the same context (e.g. for PI);
> 
> "followed" is bit ambiguous here, depending on how you view the
> ordering, wall time or ports.

Not in this case. Same context == same timeline, i.e. fifo. :)
 
> > worse still
> > that earlier request may be our own dependency and the order then broken
> > by preemption. By injecting the switch-to-idle and then recomputing the
> > priority queue, we avoid the issue with tracking in-flight coalesced
> > requests. Having tried the preempt-to-busy approach, and failed to find
> > a way around the coalesced priority issue, Michal's original proposal to
> > inject an idle context (based on handling GuC preemption) succeeds.
> > 
> > The current heuristic for deciding when to preempt are only if the new
> > request is of higher priority, and has the privileged priority of
> > greater than 0. Note that the scheduler remains unfair!
> > 
> > v2: Disable for gen8 (bdw/bsw) as we need additional w/a for GPGPU.
> > Since, the feature is now conditional and not always available when we
> > have a scheduler, make it known via the HAS_SCHEDULER GETPARAM (now a
> > capability mask).
> > 
> > Suggested-by: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Cc: Zhi Wang <zhi.a.wang@intel.com>
> 
> <SNIP>
> 
> > @@ -489,26 +489,44 @@ static void port_assign(struct execlist_port *port,
> >       port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> >  }
> >  
> > +static void inject_preempt_context(struct intel_engine_cs *engine)
> > +{
> > +     struct intel_context *ce =
> > +             &engine->i915->preempt_context->engine[engine->id];
> > +     u32 __iomem *elsp =
> > +             engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
> 
> engine_elsp() helper or so?

No. People doing this should suffer.

> > +     unsigned int n;
> > +
> > +     GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
> 
> I think this could/should be done way earlier?

This is the earliest point in the sequence. We assert that the value
that we stuff into the upper_32_bits(desc) will match the value we
extract from the upper_32_bits(status).

> > +
> > +     memset(ce->ring->vaddr + ce->ring->tail, 0, 8);
> > +     ce->ring->tail += 8;
> > +     ce->ring->tail &= (ce->ring->size - 1);
> > +     ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
> 
> An awful lot of pre-expectations here, would be shame if somebody
> documented them.

Like which? HW requirement for qword aligned tailed updates, some HW
requirement to ensure HEAD != TAIL. Have you seen the extensive
commentary that preceded this function?

> > @@ -696,7 +746,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> >  {
> >       struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
> >       struct intel_engine_execlists * const execlists = &engine->execlists;
> > -     struct execlist_port *port = execlists->port;
> > +     struct execlist_port * const port = execlists->port;
> >       struct drm_i915_private *dev_priv = engine->i915;
> >  
> >       /* We can skip acquiring intel_runtime_pm_get() here as it was taken
> > @@ -781,6 +831,23 @@ static void intel_lrc_irq_handler(unsigned long data)
> >                       if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
> >                               continue;
> >  
> > +                     if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
> > +                         buf[2*head + 1] == PREEMPT_ID) {
> 
> (2 * head + 1), could be a helper again, potentially with a descriptive
> name so the below comment can be removed and doesn't need to be
> duplicated :)

Which comment? The comment for the motivation behind the BUG_ON?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx