[PATCH 00/12] Preemption with GuC, second try

[PATCH 00/12] Preemption with GuC, second try

[Michał Winiarski]

I've included most of the review feedback, some of which caused additional
patches to appear. We're now using a separate object for GuC shared data (while
I was there I also added cleanup to guc stage desc pool creation). Since I
needed to iterate over clients in one additional place, I've converted them to
an array.
We're now also flushing the worker on reset and no longer using ordered wq
(using the one with max_active equal to number of engines).

Dave Gordon (1):
  drm/i915/guc: Add a second client, to be used for preemption

Michał Winiarski (11):
  drm/i915/guc: Extract GuC stage desc pool creation into a helper
  drm/i915/guc: Allocate separate shared data object for GuC
    communication
  drm/i915/guc: Initialize GuC before restarting engines
  drm/i915/guc: Add preemption action to GuC firmware interface
  drm/i915/guc: Split guc_wq_item_append
  drm/i915: Extract "emit write" part of emit breadcrumb functions
  drm/i915: Add information needed to track engine preempt state
  drm/i915/guc: Keep request->priority for its lifetime
  drm/i915: Rename helpers used for unwinding, use macro for can_preempt
  drm/i915/guc: Preemption! With GuC
  HAX Enable GuC Submission for CI

 drivers/gpu/drm/i915/i915_debugfs.c        |  11 +-
 drivers/gpu/drm/i915/i915_drv.c            |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |   2 +
 drivers/gpu/drm/i915/i915_gem.c            |  20 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |   8 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 407 ++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_params.h         |   4 +-
 drivers/gpu/drm/i915/intel_engine_cs.c     |   6 +-
 drivers/gpu/drm/i915/intel_guc.c           |   8 +-
 drivers/gpu/drm/i915/intel_guc.h           |  12 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  40 +++
 drivers/gpu/drm/i915/intel_lrc.c           |  53 ++--
 drivers/gpu/drm/i915/intel_lrc.h           |   1 -
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  54 ++++
 drivers/gpu/drm/i915/intel_uncore.c        |   2 +-
 15 files changed, 467 insertions(+), 166 deletions(-)

-- 
2.13.5

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

[PATCH 01/12] drm/i915/guc: Extract GuC stage desc pool creation into a helper

[Michał Winiarski]

Since it's a two-step process, we can have a cleaner error handling in
the caller if we do the allocations in a helper.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 65 +++++++++++++++++-------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 31381a327347..8983d53af229 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -311,6 +311,37 @@ static void guc_proc_desc_init(struct intel_guc *guc,
 	desc->priority = client->priority;
 }
 
+static int guc_stage_desc_pool_create(struct intel_guc *guc)
+{
+	struct i915_vma *vma;
+	void *vaddr;
+
+	vma = intel_guc_allocate_vma(guc,
+				     PAGE_ALIGN(sizeof(struct guc_stage_desc) *
+				     GUC_MAX_STAGE_DESCRIPTORS));
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		i915_vma_unpin_and_release(&vma);
+		return PTR_ERR(vaddr);
+	}
+
+	guc->stage_desc_pool = vma;
+	guc->stage_desc_pool_vaddr = vaddr;
+	ida_init(&guc->stage_ids);
+
+	return 0;
+}
+
+static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
+{
+	ida_destroy(&guc->stage_ids);
+	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
+	i915_vma_unpin_and_release(&guc->stage_desc_pool);
+}
+
 /*
  * Initialise/clear the stage descriptor shared with the GuC firmware.
  *
@@ -971,47 +1002,29 @@ static void guc_ads_destroy(struct intel_guc *guc)
 int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_vma *vma;
-	void *vaddr;
 	int ret;
 
 	if (guc->stage_desc_pool)
 		return 0;
 
-	vma = intel_guc_allocate_vma(guc,
-				PAGE_ALIGN(sizeof(struct guc_stage_desc) *
-					GUC_MAX_STAGE_DESCRIPTORS));
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
-
-	guc->stage_desc_pool = vma;
-
-	vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, I915_MAP_WB);
-	if (IS_ERR(vaddr)) {
-		ret = PTR_ERR(vaddr);
-		goto err_vma;
-	}
-
-	guc->stage_desc_pool_vaddr = vaddr;
+	ret = guc_stage_desc_pool_create(guc);
+	if (ret)
+		return ret;
 
 	ret = intel_guc_log_create(guc);
 	if (ret < 0)
-		goto err_vaddr;
+		goto err_stage_desc_pool;
 
 	ret = guc_ads_create(guc);
 	if (ret < 0)
 		goto err_log;
 
-	ida_init(&guc->stage_ids);
-
 	return 0;
 
 err_log:
 	intel_guc_log_destroy(guc);
-err_vaddr:
-	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
-err_vma:
-	i915_vma_unpin_and_release(&guc->stage_desc_pool);
+err_stage_desc_pool:
+	guc_stage_desc_pool_destroy(guc);
 	return ret;
 }
 
@@ -1019,11 +1032,9 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
-	ida_destroy(&guc->stage_ids);
 	guc_ads_destroy(guc);
 	intel_guc_log_destroy(guc);
-	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
-	i915_vma_unpin_and_release(&guc->stage_desc_pool);
+	guc_stage_desc_pool_destroy(guc);
 }
 
 static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
-- 
2.13.5

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

[PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Michał Winiarski]

We were using first page of kernel context render state for sharing data
with GuC. While it's justified by the fact that those pages are not used
(note, GuC still enforces this layout and refuses to work if we remove
the extra page in front), it's also confusing (why are we using this
particular page?). Let's allocate a separate object instead.

Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 36 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
 drivers/gpu/drm/i915/intel_guc.h           |  2 ++
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8983d53af229..30f026566001 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
 	memset(desc, 0, sizeof(*desc));
 }
 
+static int guc_shared_data_create(struct intel_guc *guc)
+{
+	struct i915_vma *vma;
+	void *vaddr;
+
+	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		i915_vma_unpin_and_release(&vma);
+		return PTR_ERR(vaddr);
+	}
+
+	guc->shared_data = vma;
+	guc->shared_data_vaddr = vaddr;
+
+	return 0;
+}
+
+static void guc_shared_data_destroy(struct intel_guc *guc)
+{
+	i915_gem_object_unpin_map(guc->shared_data->obj);
+	i915_vma_unpin_and_release(&guc->shared_data);
+}
+
 /* Construct a Work Item and append it to the GuC's Work Queue */
 static void guc_wq_item_append(struct i915_guc_client *client,
 			       struct drm_i915_gem_request *rq)
@@ -1011,9 +1038,13 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	if (ret)
 		return ret;
 
+	ret = guc_shared_data_create(guc);
+	if (ret)
+		goto err_stage_desc_pool;
+
 	ret = intel_guc_log_create(guc);
 	if (ret < 0)
-		goto err_stage_desc_pool;
+		goto err_shared_data;
 
 	ret = guc_ads_create(guc);
 	if (ret < 0)
@@ -1023,6 +1054,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 err_log:
 	intel_guc_log_destroy(guc);
+err_shared_data:
+	guc_shared_data_destroy(guc);
 err_stage_desc_pool:
 	guc_stage_desc_pool_destroy(guc);
 	return ret;
@@ -1034,6 +1067,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 
 	guc_ads_destroy(guc);
 	intel_guc_log_destroy(guc);
+	guc_shared_data_destroy(guc);
 	guc_stage_desc_pool_destroy(guc);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index bbe4c328e9fd..93b0bdec5882 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -187,9 +187,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
 	data[1] = GUC_POWER_D1;
-	/* first page is shared data with GuC */
-	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) +
-		  LRC_GUCSHR_PN * PAGE_SIZE;
+	data[2] = guc_ggtt_offset(guc->shared_data);
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
@@ -214,9 +212,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
-	/* first page is shared data with GuC */
-	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) +
-		  LRC_GUCSHR_PN * PAGE_SIZE;
+	data[2] = guc_ggtt_offset(guc->shared_data);
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index aa9a7b55be6e..fdbb4428b88c 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -48,6 +48,8 @@ struct intel_guc {
 	struct i915_vma *stage_desc_pool;
 	void *stage_desc_pool_vaddr;
 	struct ida stage_ids;
+	struct i915_vma *shared_data;
+	void *shared_data_vaddr;
 
 	struct i915_guc_client *execbuf_client;
 
-- 
2.13.5

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

[PATCH 03/12] drm/i915/guc: Initialize GuC before restarting engines

[Michał Winiarski]

Now that we're handling request resubmission the same way as regular
submission (from the tasklet), we can move GuC initialization earlier,
before restarting the engines. This way, we're no longer being in the
state of flux during engine restart - we're already in user requested
submission mode.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c            | 10 +++++-----
 drivers/gpu/drm/i915/i915_guc_submission.c |  7 -------
 drivers/gpu/drm/i915/intel_lrc.c           |  2 +-
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 82a10036fb38..6c9f0a151d0f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4780,6 +4780,11 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 		goto out;
 	}
 
+	/* We can't enable contexts until all firmware is loaded */
+	ret = intel_uc_init_hw(dev_priv);
+	if (ret)
+		goto out;
+
 	/* Need to do basic initialisation of all rings first: */
 	ret = __i915_gem_restart_engines(dev_priv);
 	if (ret)
@@ -4787,11 +4792,6 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 
 	intel_mocs_init_l3cc_table(dev_priv);
 
-	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_uc_init_hw(dev_priv);
-	if (ret)
-		goto out;
-
 out:
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	return ret;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 30f026566001..7e2c9136a2fa 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1186,14 +1186,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 
 	for_each_engine(engine, dev_priv, id) {
 		struct intel_engine_execlists * const execlists = &engine->execlists;
-		/* The tasklet was initialised by execlists, and may be in
-		 * a state of flux (across a reset) and so we just want to
-		 * take over the callback without changing any other state
-		 * in the tasklet.
-		 */
 		execlists->irq_tasklet.func = i915_guc_irq_handler;
-		clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
-		tasklet_schedule(&execlists->irq_tasklet);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 721432ddf403..52f4dbf2fc40 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1462,7 +1462,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 	execlists->preempt = false;
 
 	/* After a GPU reset, we may have requests to replay */
-	if (!i915_modparams.enable_guc_submission && execlists->first)
+	if (execlists->first)
 		tasklet_schedule(&execlists->irq_tasklet);
 
 	return 0;
-- 
2.13.5

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

[PATCH 04/12] drm/i915/guc: Add preemption action to GuC firmware interface

[Michał Winiarski]

We're using GuC action to request preemption. However, after requesting
preemption we need to wait for GuC to finish its own post-processing
before we start submitting our requests. Firmware is using shared
context to report its status.
Let's update GuC firmware interface with those new definitions.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h | 40 +++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 1c0a2a3de121..8f24fe70bd72 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -544,9 +544,36 @@ union guc_log_control {
 	u32 value;
 } __packed;
 
+struct guc_ctx_report {
+	u32 report_return_status;
+	u32 reserved1[64];
+	u32 affected_count;
+	u32 reserved2[2];
+} __packed;
+
+/* GuC Shared Context Data Struct */
+struct guc_shared_ctx_data {
+	u32 addr_of_last_preempted_data_low;
+	u32 addr_of_last_preempted_data_high;
+	u32 addr_of_last_preempted_data_high_tmp;
+	u32 padding;
+	u32 is_mapped_to_proxy;
+	u32 proxy_ctx_id;
+	u32 engine_reset_ctx_id;
+	u32 media_reset_count;
+	u32 reserved1[8];
+	u32 uk_last_ctx_switch_reason;
+	u32 was_reset;
+	u32 lrca_gpu_addr;
+	u64 execlist_ctx;
+	u32 reserved2[66];
+	struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
+} __packed;
+
 /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */
 enum intel_guc_action {
 	INTEL_GUC_ACTION_DEFAULT = 0x0,
+	INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2,
 	INTEL_GUC_ACTION_SAMPLE_FORCEWAKE = 0x6,
 	INTEL_GUC_ACTION_ALLOCATE_DOORBELL = 0x10,
 	INTEL_GUC_ACTION_DEALLOCATE_DOORBELL = 0x20,
@@ -562,6 +589,19 @@ enum intel_guc_action {
 	INTEL_GUC_ACTION_LIMIT
 };
 
+enum intel_guc_preempt_options {
+	INTEL_GUC_PREEMPT_OPTION_IMMEDIATE = 0x1,
+	INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q = 0x4,
+	INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q = 0x8,
+};
+
+enum intel_guc_report_status {
+	INTEL_GUC_REPORT_STATUS_UNKNOWN = 0x0,
+	INTEL_GUC_REPORT_STATUS_ACKED = 0x1,
+	INTEL_GUC_REPORT_STATUS_ERROR = 0x2,
+	INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
+};
+
 /*
  * The GuC sends its response to a command by overwriting the
  * command in SS0. The response is distinguishable from a command
-- 
2.13.5

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

[PATCH v2 05/12] drm/i915/guc: Add a second client, to be used for preemption

[Michał Winiarski]

From: Dave Gordon <david.s.gordon@intel.com>

This second client is created with priority KMD_HIGH, and marked
as preemptive. This will allow us to request preemption using GuC actions.

v2: Extract clients creation into a helper, debugfs fixups. (Michał)
Recreate doorbell on init. (Daniele)
Move clients into an array.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  11 +--
 drivers/gpu/drm/i915/i915_guc_submission.c | 107 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_guc.h           |   8 ++-
 drivers/gpu/drm/i915/intel_uncore.c        |   2 +-
 4 files changed, 90 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f7817c667958..2a3fbf2a3263 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2517,7 +2517,7 @@ static bool check_guc_submission(struct seq_file *m)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	const struct intel_guc *guc = &dev_priv->guc;
 
-	if (!guc->execbuf_client) {
+	if (!guc->client[SUBMIT]) {
 		seq_printf(m, "GuC submission %s\n",
 			   HAS_GUC_SCHED(dev_priv) ?
 			   "disabled" :
@@ -2532,6 +2532,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	const struct intel_guc *guc = &dev_priv->guc;
+	u32 i;
 
 	if (!check_guc_submission(m))
 		return 0;
@@ -2540,8 +2541,10 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
 	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
 
-	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
-	i915_guc_client_info(m, dev_priv, guc->execbuf_client);
+	for (i = 0; i < I915_GUC_NUM_CLIENTS; i++) {
+		seq_printf(m, "\nGuC client @ %p:\n", guc->client[i]);
+		i915_guc_client_info(m, dev_priv, guc->client[i]);
+	}
 
 	i915_guc_log_info(m, dev_priv);
 
@@ -2555,7 +2558,7 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	const struct intel_guc *guc = &dev_priv->guc;
 	struct guc_stage_desc *desc = guc->stage_desc_pool_vaddr;
-	struct i915_guc_client *client = guc->execbuf_client;
+	struct i915_guc_client *client = guc->client[SUBMIT];
 	unsigned int tmp;
 	int index;
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 7e2c9136a2fa..05e7e371ebe3 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -33,10 +33,11 @@
  *
  * GuC client:
  * A i915_guc_client refers to a submission path through GuC. Currently, there
- * is only one of these (the execbuf_client) and this one is charged with all
- * submissions to the GuC. This struct is the owner of a doorbell, a process
- * descriptor and a workqueue (all of them inside a single gem object that
- * contains all required pages for these elements).
+ * are two clients. One of them (SUBMIT) is charged with all submissions to the
+ * GuC, the other one (PREEMPT) is responsible for preempting the SUBMIT one.
+ * This struct is the owner of a doorbell, a process descriptor and a workqueue
+ * (all of them inside a single gem object that contains all required pages for
+ * these elements).
  *
  * GuC stage descriptor:
  * During initialization, the driver allocates a static pool of 1024 such
@@ -363,6 +364,8 @@ static void guc_stage_desc_init(struct intel_guc *guc,
 	memset(desc, 0, sizeof(*desc));
 
 	desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | GUC_STAGE_DESC_ATTR_KERNEL;
+	if (client->priority <= GUC_CLIENT_PRIORITY_HIGH)
+		desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT;
 	desc->stage_id = client->stage_id;
 	desc->priority = client->priority;
 	desc->db_id = client->doorbell_id;
@@ -552,7 +555,7 @@ static void i915_guc_submit(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_guc_client *client = guc->execbuf_client;
+	struct i915_guc_client *client = guc->client[SUBMIT];
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 	struct execlist_port *port = execlists->port;
 	const unsigned int engine_id = engine->id;
@@ -749,10 +752,11 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
  */
 static int guc_init_doorbell_hw(struct intel_guc *guc)
 {
-	struct i915_guc_client *client = guc->execbuf_client;
+	struct i915_guc_client *client = guc->client[SUBMIT];
 	bool recreate_first_client = false;
 	u16 db_id;
 	int ret;
+	u32 i;
 
 	/* For unused doorbells, make sure they are disabled */
 	for_each_clear_bit(db_id, guc->doorbell_bitmap, GUC_NUM_DOORBELLS) {
@@ -760,7 +764,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 			continue;
 
 		if (has_doorbell(client)) {
-			/* Borrow execbuf_client (we will recreate it later) */
+			/* Borrow submit client (we will recreate it later) */
 			destroy_doorbell(client);
 			recreate_first_client = true;
 		}
@@ -779,14 +783,14 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 		__update_doorbell_desc(client, client->doorbell_id);
 	}
 
-	/* Now for every client (and not only execbuf_client) make sure their
+	/* Now for every client (not only submission client) make sure their
 	 * doorbells are known by the GuC */
-	//for (client = client_list; client != NULL; client = client->next)
+	for (i = 0; i < I915_GUC_NUM_CLIENTS; i++)
 	{
-		ret = __create_doorbell(client);
+		ret = __create_doorbell(guc->client[i]);
 		if (ret) {
 			DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
-				client->stage_id, ret);
+				  guc->client[i]->stage_id, ret);
 			return ret;
 		}
 	}
@@ -913,6 +917,47 @@ static void guc_client_free(struct i915_guc_client *client)
 	kfree(client);
 }
 
+static int guc_clients_create(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_guc_client *client;
+
+	client = guc_client_alloc(dev_priv,
+				  INTEL_INFO(dev_priv)->ring_mask,
+				  GUC_CLIENT_PRIORITY_KMD_NORMAL,
+				  dev_priv->kernel_context);
+	if (IS_ERR(client)) {
+		DRM_ERROR("Failed to create GuC client for submission!\n");
+		return PTR_ERR(client);
+	}
+	guc->client[SUBMIT] = client;
+
+	client = guc_client_alloc(dev_priv,
+				  INTEL_INFO(dev_priv)->ring_mask,
+				  GUC_CLIENT_PRIORITY_KMD_HIGH,
+				  dev_priv->preempt_context);
+	if (IS_ERR(client)) {
+		DRM_ERROR("Failed to create GuC client for preemption!\n");
+		guc_client_free(guc->client[SUBMIT]);
+		guc->client[SUBMIT] = NULL;
+		return PTR_ERR(client);
+	}
+	guc->client[PREEMPT] = client;
+
+	return 0;
+}
+
+static void guc_clients_destroy(struct intel_guc *guc)
+{
+	struct i915_guc_client *client;
+	u32 i;
+
+	for (i = 0; i < I915_GUC_NUM_CLIENTS; i++) {
+		client = fetch_and_zero(&guc->client[i]);
+		guc_client_free(client);
+	}
+}
+
 static void guc_policy_init(struct guc_policy *policy)
 {
 	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
@@ -1140,7 +1185,6 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_guc_client *client = guc->execbuf_client;
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 	int err;
@@ -1158,28 +1202,29 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 		     sizeof(struct guc_wq_item) *
 		     I915_NUM_ENGINES > GUC_WQ_SIZE);
 
-	if (!client) {
-		client = guc_client_alloc(dev_priv,
-					  INTEL_INFO(dev_priv)->ring_mask,
-					  GUC_CLIENT_PRIORITY_KMD_NORMAL,
-					  dev_priv->kernel_context);
-		if (IS_ERR(client)) {
-			DRM_ERROR("Failed to create GuC client for execbuf!\n");
-			return PTR_ERR(client);
-		}
-
-		guc->execbuf_client = client;
+	/*
+	 * We're being called on both module initialization and on reset,
+	 * until this flow is changed, we're using regular client presence to
+	 * determine which case are we in, and whether we should allocate new
+	 * clients or just reset their workqueues.
+	 */
+	if (!guc->client[SUBMIT]) {
+		GEM_BUG_ON(guc->client[PREEMPT]);
+		err = guc_clients_create(guc);
+		if (err)
+			return err;
+	} else {
+		guc_reset_wq(guc->client[SUBMIT]);
+		guc_reset_wq(guc->client[PREEMPT]);
 	}
 
 	err = intel_guc_sample_forcewake(guc);
 	if (err)
-		goto err_execbuf_client;
-
-	guc_reset_wq(client);
+		goto err_free_clients;
 
 	err = guc_init_doorbell_hw(guc);
 	if (err)
-		goto err_execbuf_client;
+		goto err_free_clients;
 
 	/* Take over from manual control of ELSP (execlists) */
 	guc_interrupts_capture(dev_priv);
@@ -1191,9 +1236,8 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 
 	return 0;
 
-err_execbuf_client:
-	guc_client_free(guc->execbuf_client);
-	guc->execbuf_client = NULL;
+err_free_clients:
+	guc_clients_destroy(guc);
 	return err;
 }
 
@@ -1206,6 +1250,5 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	/* Revert back to manual ELSP submission */
 	intel_engines_reset_default_submission(dev_priv);
 
-	guc_client_free(guc->execbuf_client);
-	guc->execbuf_client = NULL;
+	guc_clients_destroy(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index fdbb4428b88c..c20ed99cbda0 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -33,6 +33,12 @@
 #include "i915_guc_reg.h"
 #include "i915_vma.h"
 
+#define I915_GUC_NUM_CLIENTS 2
+enum i915_guc_client_id {
+	SUBMIT = 0,
+	PREEMPT
+};
+
 struct intel_guc {
 	struct intel_uc_fw fw;
 	struct intel_guc_log log;
@@ -51,7 +57,7 @@ struct intel_guc {
 	struct i915_vma *shared_data;
 	void *shared_data_vaddr;
 
-	struct i915_guc_client *execbuf_client;
+	struct i915_guc_client *client[I915_GUC_NUM_CLIENTS];
 
 	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
 	/* Cyclic counter mod pagesize	*/
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b3c3f94fc7e4..0f5a433ce56a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1778,7 +1778,7 @@ bool intel_has_gpu_reset(struct drm_i915_private *dev_priv)
 bool intel_has_reset_engine(struct drm_i915_private *dev_priv)
 {
 	return (dev_priv->info.has_reset_engine &&
-		!dev_priv->guc.execbuf_client &&
+		!dev_priv->guc.client[SUBMIT] &&
 		i915_modparams.reset >= 2);
 }
 
-- 
2.13.5

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

[PATCH 06/12] drm/i915/guc: Split guc_wq_item_append

[Michał Winiarski]

We're using a special preempt context for HW to preempt into. We don't
want to emit any requests there, but we still need to wrap this context
into a valid GuC work item.
Let's cleanup the functions operating on GuC work items.
We can extract guc_request_add - responsible for adding GuC work item and
ringing the doorbell, and guc_wq_item_append - used by the function
above, not tied to the concept of gem request.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 55 ++++++++++++++++--------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 05e7e371ebe3..07cd22a7b456 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -71,7 +71,7 @@
  * WQ_TYPE_INORDER is needed to support legacy submission via GuC, which
  * represents in-order queue. The kernel driver packs ring tail pointer and an
  * ELSP context descriptor dword into Work Item.
- * See guc_wq_item_append()
+ * See guc_add_request()
  *
  * ADS:
  * The Additional Data Struct (ADS) has pointers for different buffers used by
@@ -390,7 +390,7 @@ static void guc_stage_desc_init(struct intel_guc *guc,
 		 * submission or, in other words, not using a direct submission
 		 * model) the KMD's LRCA is not used for any work submission.
 		 * Instead, the GuC uses the LRCA of the user mode context (see
-		 * guc_wq_item_append below).
+		 * guc_add_request below).
 		 */
 		lrc->context_desc = lower_32_bits(ce->lrc_desc);
 
@@ -469,22 +469,18 @@ static void guc_shared_data_destroy(struct intel_guc *guc)
 
 /* Construct a Work Item and append it to the GuC's Work Queue */
 static void guc_wq_item_append(struct i915_guc_client *client,
-			       struct drm_i915_gem_request *rq)
+			       u32 target_engine, u32 context_desc,
+			       u32 ring_tail, u32 fence_id)
 {
 	/* wqi_len is in DWords, and does not include the one-word header */
 	const size_t wqi_size = sizeof(struct guc_wq_item);
 	const u32 wqi_len = wqi_size / sizeof(u32) - 1;
-	struct intel_engine_cs *engine = rq->engine;
-	struct i915_gem_context *ctx = rq->ctx;
 	struct guc_process_desc *desc = __get_process_desc(client);
 	struct guc_wq_item *wqi;
-	u32 ring_tail, wq_off;
+	u32 wq_off;
 
 	lockdep_assert_held(&client->wq_lock);
 
-	ring_tail = intel_ring_set_tail(rq->ring, rq->tail) / sizeof(u64);
-	GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
-
 	/* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
 	 * should not have the case where structure wqi is across page, neither
 	 * wrapped to the beginning. This simplifies the implementation below.
@@ -506,15 +502,14 @@ static void guc_wq_item_append(struct i915_guc_client *client,
 	/* Now fill in the 4-word work queue item */
 	wqi->header = WQ_TYPE_INORDER |
 		      (wqi_len << WQ_LEN_SHIFT) |
-		      (engine->guc_id << WQ_TARGET_SHIFT) |
+		      (target_engine << WQ_TARGET_SHIFT) |
 		      WQ_NO_WCFLUSH_WAIT;
-
-	wqi->context_desc = lower_32_bits(intel_lr_context_descriptor(ctx, engine));
-
+	wqi->context_desc = context_desc;
 	wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
-	wqi->fence_id = rq->global_seqno;
+	GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
+	wqi->fence_id = fence_id;
 
-	/* Postincrement WQ tail for next time. */
+	/* Make the update visible to GuC */
 	WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1));
 }
 
@@ -544,6 +539,25 @@ static void guc_ring_doorbell(struct i915_guc_client *client)
 	GEM_BUG_ON(db->db_status != GUC_DOORBELL_ENABLED);
 }
 
+static void guc_add_request(struct intel_guc *guc,
+			    struct drm_i915_gem_request *rq)
+{
+	struct i915_guc_client *client = guc->client[SUBMIT];
+	struct intel_engine_cs *engine = rq->engine;
+	u32 ctx_desc = lower_32_bits(intel_lr_context_descriptor(rq->ctx, engine));
+	u32 ring_tail = intel_ring_set_tail(rq->ring, rq->tail) / sizeof(u64);
+
+	spin_lock(&client->wq_lock);
+
+	guc_wq_item_append(client, engine->guc_id, ctx_desc,
+			   ring_tail, rq->global_seqno);
+	guc_ring_doorbell(client);
+
+	client->submissions[engine->id] += 1;
+
+	spin_unlock(&client->wq_lock);
+}
+
 /**
  * i915_guc_submit() - Submit commands through GuC
  * @engine: engine associated with the commands
@@ -555,10 +569,8 @@ static void i915_guc_submit(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_guc_client *client = guc->client[SUBMIT];
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 	struct execlist_port *port = execlists->port;
-	const unsigned int engine_id = engine->id;
 	unsigned int n;
 
 	for (n = 0; n < ARRAY_SIZE(execlists->port); n++) {
@@ -572,14 +584,7 @@ static void i915_guc_submit(struct intel_engine_cs *engine)
 			if (i915_vma_is_map_and_fenceable(rq->ring->vma))
 				POSTING_READ_FW(GUC_STATUS);
 
-			spin_lock(&client->wq_lock);
-
-			guc_wq_item_append(client, rq);
-			guc_ring_doorbell(client);
-
-			client->submissions[engine_id] += 1;
-
-			spin_unlock(&client->wq_lock);
+			guc_add_request(guc, rq);
 		}
 	}
 }
-- 
2.13.5

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

[PATCH v2 07/12] drm/i915: Extract "emit write" part of emit breadcrumb functions

[Michał Winiarski]

Let's separate the "emit" part from touching any internal structures,
this way we can have a generic "emit coherent GGTT write" function.
We would like to reuse this functionality for emitting HWSP write, to
confirm that preempt-to-idle has finished.

v2: Reorder args to match emit_pipe_control, s/render/rcs (Chris)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 28 +++++++-----------------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 38 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 52f4dbf2fc40..9a7aec2c1771 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1782,10 +1782,8 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
 	/* w/a: bit 5 needs to be zero for MI_FLUSH_DW address. */
 	BUILD_BUG_ON(I915_GEM_HWS_INDEX_ADDR & (1 << 5));
 
-	*cs++ = (MI_FLUSH_DW + 1) | MI_FLUSH_DW_OP_STOREDW;
-	*cs++ = intel_hws_seqno_address(request->engine) | MI_FLUSH_DW_USE_GTT;
-	*cs++ = 0;
-	*cs++ = request->global_seqno;
+	cs = gen8_emit_ggtt_write(cs, request->global_seqno,
+				  intel_hws_seqno_address(request->engine));
 	*cs++ = MI_USER_INTERRUPT;
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
@@ -1795,24 +1793,14 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
 }
 static const int gen8_emit_breadcrumb_sz = 6 + WA_TAIL_DWORDS;
 
-static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
+static void gen8_emit_breadcrumb_rcs(struct drm_i915_gem_request *request,
 					u32 *cs)
 {
 	/* We're using qword write, seqno should be aligned to 8 bytes. */
 	BUILD_BUG_ON(I915_GEM_HWS_INDEX & 1);
 
-	/* w/a for post sync ops following a GPGPU operation we
-	 * need a prior CS_STALL, which is emitted by the flush
-	 * following the batch.
-	 */
-	*cs++ = GFX_OP_PIPE_CONTROL(6);
-	*cs++ = PIPE_CONTROL_GLOBAL_GTT_IVB | PIPE_CONTROL_CS_STALL |
-		PIPE_CONTROL_QW_WRITE;
-	*cs++ = intel_hws_seqno_address(request->engine);
-	*cs++ = 0;
-	*cs++ = request->global_seqno;
-	/* We're thrashing one dword of HWS. */
-	*cs++ = 0;
+	cs = gen8_emit_ggtt_write_rcs(cs, request->global_seqno,
+				      intel_hws_seqno_address(request->engine));
 	*cs++ = MI_USER_INTERRUPT;
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
@@ -1820,7 +1808,7 @@ 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 const int gen8_emit_breadcrumb_rcs_sz = 8 + WA_TAIL_DWORDS;
 
 static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
 {
@@ -1976,8 +1964,8 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 		engine->init_hw = gen8_init_render_ring;
 	engine->init_context = gen8_init_rcs_context;
 	engine->emit_flush = gen8_emit_flush_render;
-	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
-	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
+	engine->emit_breadcrumb = gen8_emit_breadcrumb_rcs;
+	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_rcs_sz;
 
 	ret = intel_engine_create_scratch(engine, PAGE_SIZE);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 0fedda17488c..b65f9f282ef0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -831,6 +831,44 @@ static inline u32 *gen8_emit_pipe_control(u32 *batch, u32 flags, u32 offset)
 	return batch + 6;
 }
 
+static inline u32 *
+gen8_emit_ggtt_write_rcs(u32 *cs, u32 value, u32 gtt_offset)
+{
+	/* We're using qword write, offset should be aligned to 8 bytes. */
+	GEM_BUG_ON(!IS_ALIGNED(gtt_offset, 8));
+
+	/* w/a for post sync ops following a GPGPU operation we
+	 * need a prior CS_STALL, which is emitted by the flush
+	 * following the batch.
+	 */
+	*cs++ = GFX_OP_PIPE_CONTROL(6);
+	*cs++ = PIPE_CONTROL_GLOBAL_GTT_IVB | PIPE_CONTROL_CS_STALL |
+		PIPE_CONTROL_QW_WRITE;
+	*cs++ = gtt_offset;
+	*cs++ = 0;
+	*cs++ = value;
+	/* We're thrashing one dword of HWS. */
+	*cs++ = 0;
+
+	return cs;
+}
+
+static inline u32 *
+gen8_emit_ggtt_write(u32 *cs, u32 value, u32 gtt_offset)
+{
+	/* w/a: bit 5 needs to be zero for MI_FLUSH_DW address. */
+	GEM_BUG_ON(gtt_offset & (1 << 5));
+	/* Offset should be aligned to 8 bytes for both (QW/DW) write types */
+	GEM_BUG_ON(!IS_ALIGNED(gtt_offset, 8));
+
+	*cs++ = (MI_FLUSH_DW + 1) | MI_FLUSH_DW_OP_STOREDW;
+	*cs++ = gtt_offset | MI_FLUSH_DW_USE_GTT;
+	*cs++ = 0;
+	*cs++ = value;
+
+	return cs;
+}
+
 bool intel_engine_is_idle(struct intel_engine_cs *engine);
 bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
 
-- 
2.13.5

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

[PATCH 08/12] drm/i915: Add information needed to track engine preempt state

[Michał Winiarski]

We shouldn't inspect ELSP context status (or any other bits depending on
specific submission backend) when using GuC submission.
Let's use another piece of HWSP for preempt context, to write its bit of
information, meaning that preemption has finished, and hardware is now
idle.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b65f9f282ef0..7777a05bfff5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -591,6 +591,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
  */
 #define I915_GEM_HWS_INDEX		0x30
 #define I915_GEM_HWS_INDEX_ADDR (I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT)
+#define I915_GEM_HWS_PREEMPT_INDEX	0x32
+#define I915_GEM_HWS_PREEMPT_ADDR (I915_GEM_HWS_PREEMPT_INDEX << MI_STORE_DWORD_INDEX_SHIFT)
 #define I915_GEM_HWS_SCRATCH_INDEX	0x40
 #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX << MI_STORE_DWORD_INDEX_SHIFT)
 
@@ -743,6 +745,11 @@ static inline u32 intel_hws_seqno_address(struct intel_engine_cs *engine)
 	return engine->status_page.ggtt_offset + I915_GEM_HWS_INDEX_ADDR;
 }
 
+static inline u32 intel_hws_preempt_done_address(struct intel_engine_cs *engine)
+{
+	return engine->status_page.ggtt_offset + I915_GEM_HWS_PREEMPT_ADDR;
+}
+
 /* intel_breadcrumbs.c -- user interrupt bottom-half for waiters */
 int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
 
-- 
2.13.5

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

[PATCH 09/12] drm/i915/guc: Keep request->priority for its lifetime

[Michał Winiarski]

We also want to support preemption with GuC submission backend.
In order to do that, we need to remember the priority, like we do on
execlists path.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 07cd22a7b456..947af576563b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -658,7 +658,6 @@ static void i915_guc_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));
@@ -693,6 +692,7 @@ static void i915_guc_irq_handler(unsigned long data)
 	rq = port_request(&port[0]);
 	while (rq && i915_gem_request_completed(rq)) {
 		trace_i915_gem_request_out(rq);
+		rq->priotree.priority = INT_MAX;
 		i915_gem_request_put(rq);
 
 		execlists_port_complete(execlists, port);
-- 
2.13.5

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

[PATCH 10/12] drm/i915: Rename helpers used for unwinding, use macro for can_preempt

[Michał Winiarski]

We would also like to make use of execlist_cancel_port_requests and
unwind_incomplete_requests in GuC preemption backend.
Let's rename the functions to use the correct prefixes, so that we can
simply add the declarations in the following patch.
Similar thing for applies for can_preempt, except we're introducing
HAS_LOGICAL_RING_PREEMPTION macro instad, converting other users that
were previously touching device info directly.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c        |  2 +-
 drivers/gpu/drm/i915/i915_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_engine_cs.c |  6 +++---
 drivers/gpu/drm/i915/intel_lrc.c       | 21 ++++++++-------------
 4 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66fc156b294a..78120a5d40f9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -372,7 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 			value |= I915_SCHEDULER_CAP_ENABLED;
 			value |= I915_SCHEDULER_CAP_PRIORITY;
 
-			if (INTEL_INFO(dev_priv)->has_logical_ring_preemption &&
+			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv) &&
 			    i915_modparams.enable_execlists &&
 			    !i915_modparams.enable_guc_submission)
 				value |= I915_SCHEDULER_CAP_PREEMPTION;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 799a90abd81f..d75dd54a0073 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3113,6 +3113,8 @@ intel_info(const struct drm_i915_private *dev_priv)
 
 #define HAS_LOGICAL_RING_CONTEXTS(dev_priv) \
 		((dev_priv)->info.has_logical_ring_contexts)
+#define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
+		((dev_priv)->info.has_logical_ring_preemption)
 #define USES_PPGTT(dev_priv)		(i915_modparams.enable_ppgtt)
 #define USES_FULL_PPGTT(dev_priv)	(i915_modparams.enable_ppgtt >= 2)
 #define USES_FULL_48BIT_PPGTT(dev_priv)	(i915_modparams.enable_ppgtt == 3)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 807a7aafc089..9a6176a207aa 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -617,7 +617,7 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 	 * 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) {
+	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
 		ring = engine->context_pin(engine,
 					   engine->i915->preempt_context);
 		if (IS_ERR(ring)) {
@@ -648,7 +648,7 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 err_breadcrumbs:
 	intel_engine_fini_breadcrumbs(engine);
 err_unpin_preempt:
-	if (INTEL_INFO(engine->i915)->has_logical_ring_preemption)
+	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
 		engine->context_unpin(engine, engine->i915->preempt_context);
 err_unpin_kernel:
 	engine->context_unpin(engine, engine->i915->kernel_context);
@@ -676,7 +676,7 @@ 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)
+	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
 		engine->context_unpin(engine, engine->i915->preempt_context);
 	engine->context_unpin(engine, engine->i915->kernel_context);
 }
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 9a7aec2c1771..4f9f12e3c7f6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -354,7 +354,7 @@ static void unwind_wa_tail(struct drm_i915_gem_request *rq)
 	assert_ring_tail_valid(rq->ring, rq->tail);
 }
 
-static void unwind_incomplete_requests(struct intel_engine_cs *engine)
+static void intel_engine_unwind_incomplete_requests(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *rq, *rn;
 	struct i915_priolist *uninitialized_var(p);
@@ -515,11 +515,6 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 	elsp_write(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 intel_engine_execlists * const execlists = &engine->execlists;
@@ -567,7 +562,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		if (port_count(&port[0]) > 1)
 			goto unlock;
 
-		if (can_preempt(engine) &&
+		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
 		    rb_entry(rb, struct i915_priolist, node)->priority >
 		    max(last->priotree.priority, 0)) {
 			/*
@@ -688,7 +683,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 }
 
 static void
-execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
+execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
 {
 	struct execlist_port *port = execlists->port;
 	unsigned int num_ports = ARRAY_SIZE(execlists->port);
@@ -714,7 +709,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 	spin_lock_irqsave(&engine->timeline->lock, flags);
 
 	/* Cancel the requests on the HW and clear the ELSP tracker. */
-	execlist_cancel_port_requests(execlists);
+	execlists_cancel_port_requests(execlists);
 
 	/* Mark all executing requests as skipped. */
 	list_for_each_entry(rq, &engine->timeline->requests, link) {
@@ -856,10 +851,10 @@ static void intel_lrc_irq_handler(unsigned long data)
 
 			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
 			    buf[2*head + 1] == PREEMPT_ID) {
-				execlist_cancel_port_requests(execlists);
+				execlists_cancel_port_requests(execlists);
 
 				spin_lock_irq(&engine->timeline->lock);
-				unwind_incomplete_requests(engine);
+				intel_engine_unwind_incomplete_requests(engine);
 				spin_unlock_irq(&engine->timeline->lock);
 
 				GEM_BUG_ON(!execlists->preempt);
@@ -1519,10 +1514,10 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	 * guessing the missed context-switch events by looking at what
 	 * requests were completed.
 	 */
-	execlist_cancel_port_requests(execlists);
+	execlists_cancel_port_requests(execlists);
 
 	/* Push back any incomplete requests for replay after the reset. */
-	unwind_incomplete_requests(engine);
+	intel_engine_unwind_incomplete_requests(engine);
 
 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
 
-- 
2.13.5

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

[PATCH v2 11/12] drm/i915/guc: Preemption! With GuC

[Michał Winiarski]

Pretty similar to what we have on execlists.
We're reusing most of the GEM code, however, due to GuC quirks we need a
couple of extra bits.
Preemption is implemented as GuC action, and actions can be pretty slow.
Because of that, we're using a mutex to serialize them. Since we're
requesting preemption from the tasklet, the task of creating a workitem
and wrapping it in GuC action is delegated to a worker.

To distinguish that preemption has finished, we're using additional
piece of HWSP, and since we're not getting context switch interrupts,
we're also adding a user interrupt.

The fact that our special preempt context has completed unfortunately
doesn't mean that we're ready to submit new work. We also need to wait
for GuC to finish its own processing.

v2: Don't compile out the wait for GuC, handle workqueue flush on reset,
no need for ordered workqueue, put on a reviewer hat when looking at my own
patches (Chris)
Move struct work around in intel_guc, move user interruput outside of
conditional (Michał)
Keep ring around rather than chase though intel_context

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |   3 +-
 drivers/gpu/drm/i915/i915_gem.c            |  10 ++
 drivers/gpu/drm/i915/i915_guc_submission.c | 141 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_guc.h           |   2 +
 drivers/gpu/drm/i915/intel_lrc.c           |   4 +-
 drivers/gpu/drm/i915/intel_lrc.h           |   1 -
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   9 ++
 7 files changed, 156 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 78120a5d40f9..1d356f5c252c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -373,8 +373,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 			value |= I915_SCHEDULER_CAP_PRIORITY;
 
 			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv) &&
-			    i915_modparams.enable_execlists &&
-			    !i915_modparams.enable_guc_submission)
+			    i915_modparams.enable_execlists)
 				value |= I915_SCHEDULER_CAP_PREEMPTION;
 		}
 		break;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6c9f0a151d0f..e7d1d91cf8a4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2854,6 +2854,16 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
 	tasklet_kill(&engine->execlists.irq_tasklet);
 	tasklet_disable(&engine->execlists.irq_tasklet);
 
+	/*
+	 * We're using worker to queue preemption requests from the tasklet in
+	 * GuC submission mode.
+	 * Even though tasklet was disabled, we may still have a worker queued.
+	 * Let's make sure that all workers scheduled before disabling the
+	 * tasklet are completed before continuing with the reset.
+	 */
+	if (i915_modparams.enable_guc_submission)
+		flush_workqueue(engine->i915->guc.preempt_wq);
+
 	if (engine->irq_seqno_barrier)
 		engine->irq_seqno_barrier(engine);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 947af576563b..418451755145 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -558,6 +558,89 @@ static void guc_add_request(struct intel_guc *guc,
 	spin_unlock(&client->wq_lock);
 }
 
+#define GUC_PREEMPT_FINISHED 0x1
+#define GUC_PREEMPT_BREADCRUMB_DWORDS 0x8
+static void inject_preempt_context(struct work_struct *work)
+{
+	struct intel_engine_cs *engine =
+		container_of(work, typeof(*engine), guc_preempt_work);
+	struct drm_i915_private *dev_priv = engine->i915;
+	struct intel_guc *guc = &dev_priv->guc;
+	struct i915_guc_client *client = guc->client[PREEMPT];
+	struct intel_ring *ring = client->owner->engine[engine->id].ring;
+	u32 ctx_desc = lower_32_bits(intel_lr_context_descriptor(client->owner,
+								 engine));
+	u32 *cs = ring->vaddr + ring->tail;
+	u32 data[7];
+
+	if (engine->id == RCS) {
+		cs = gen8_emit_ggtt_write_rcs(cs, GUC_PREEMPT_FINISHED,
+				intel_hws_preempt_done_address(engine));
+	} else {
+		cs = gen8_emit_ggtt_write(cs, GUC_PREEMPT_FINISHED,
+				intel_hws_preempt_done_address(engine));
+		*cs++ = MI_NOOP;
+		*cs++ = MI_NOOP;
+	}
+	*cs++ = MI_USER_INTERRUPT;
+	*cs++ = MI_NOOP;
+
+	GEM_BUG_ON(!IS_ALIGNED(ring->size,
+			       GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32)));
+	GEM_BUG_ON((void*)cs - (ring->vaddr + ring->tail) !=
+		   GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32));
+
+	ring->tail += GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32);
+	ring->tail &= (ring->size - 1);
+
+	if (i915_vma_is_map_and_fenceable(ring->vma))
+		POSTING_READ_FW(GUC_STATUS);
+
+	spin_lock_irq(&client->wq_lock);
+	guc_wq_item_append(client, engine->guc_id, ctx_desc,
+			   ring->tail / sizeof(u64), 0);
+	spin_unlock_irq(&client->wq_lock);
+
+	data[0] = INTEL_GUC_ACTION_REQUEST_PREEMPTION;
+	data[1] = client->stage_id;
+	data[2] = INTEL_GUC_PREEMPT_OPTION_IMMEDIATE |
+		  INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q |
+		  INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q;
+	data[3] = engine->guc_id;
+	data[4] = guc->client[SUBMIT]->priority;
+	data[5] = guc->client[SUBMIT]->stage_id;
+	data[6] = guc_ggtt_offset(guc->shared_data);
+
+	if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {
+		WRITE_ONCE(engine->execlists.preempt, false);
+		tasklet_schedule(&engine->execlists.irq_tasklet);
+	}
+}
+
+/*
+ * We're using user interrupt and HWSP value to mark that preemption has
+ * finished and GPU is idle. Normally, we could unwind and continue similar to
+ * execlists submission path. Unfortunately, with GuC we also need to wait for
+ * it to finish its own postprocessing, before attempting to submit. Otherwise
+ * GuC may silently ignore our submissions, and thus we risk losing request at
+ * best, executing out-of-order and causing kernel panic at worst.
+ */
+#define GUC_PREEMPT_POSTPROCESS_DELAY_MS 10
+static void wait_for_guc_preempt_report(struct intel_engine_cs *engine)
+{
+	struct intel_guc *guc = &engine->i915->guc;
+	struct guc_shared_ctx_data *data = guc->shared_data_vaddr;
+	struct guc_ctx_report *report = &data->preempt_ctx_report[engine->guc_id];
+
+	WARN_ON(wait_for_atomic(report->report_return_status ==
+				INTEL_GUC_REPORT_STATUS_COMPLETE,
+				GUC_PREEMPT_POSTPROCESS_DELAY_MS));
+	/* GuC is expecting that we're also going to clear the affected context
+	 * counter */
+	report->affected_count = 0;
+}
+
+
 /**
  * i915_guc_submit() - Submit commands through GuC
  * @engine: engine associated with the commands
@@ -634,13 +717,28 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
 	bool submit = false;
 	struct rb_node *rb;
 
-	if (port_isset(port))
-		port++;
-
 	spin_lock_irq(&engine->timeline->lock);
 	rb = execlists->first;
 	GEM_BUG_ON(rb_first(&execlists->queue) != rb);
-	while (rb) {
+
+	if (!rb)
+		goto unlock;
+
+	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) && port_isset(port)) {
+		if (rb_entry(rb, struct i915_priolist, node)->priority >
+		    max(port_request(port)->priotree.priority, 0)) {
+			WRITE_ONCE(execlists->preempt, true);
+			queue_work(engine->i915->guc.preempt_wq,
+				   &engine->guc_preempt_work);
+			goto unlock;
+		} else if (port_isset(last_port)) {
+			goto unlock;
+		}
+
+		port++;
+	}
+
+	do {
 		struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
 		struct drm_i915_gem_request *rq, *rn;
 
@@ -670,13 +768,14 @@ static void i915_guc_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);
 		i915_guc_submit(engine);
 	}
+unlock:
 	spin_unlock_irq(&engine->timeline->lock);
 }
 
@@ -685,10 +784,23 @@ static void i915_guc_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;
-	const struct execlist_port * const last_port =
-		&execlists->port[execlists->port_mask];
 	struct drm_i915_gem_request *rq;
 
+	if (READ_ONCE(execlists->preempt) &&
+	    intel_read_status_page(engine, I915_GEM_HWS_PREEMPT_INDEX) ==
+	    GUC_PREEMPT_FINISHED) {
+		execlists_cancel_port_requests(&engine->execlists);
+
+		spin_lock_irq(&engine->timeline->lock);
+		intel_engine_unwind_incomplete_requests(engine);
+		spin_unlock_irq(&engine->timeline->lock);
+
+		wait_for_guc_preempt_report(engine);
+
+		WRITE_ONCE(execlists->preempt, false);
+		intel_write_status_page(engine, I915_GEM_HWS_PREEMPT_INDEX, 0);
+	}
+
 	rq = port_request(&port[0]);
 	while (rq && i915_gem_request_completed(rq)) {
 		trace_i915_gem_request_out(rq);
@@ -700,7 +812,7 @@ static void i915_guc_irq_handler(unsigned long data)
 		rq = port_request(&port[0]);
 	}
 
-	if (!port_isset(last_port))
+	if (!READ_ONCE(execlists->preempt))
 		i915_guc_dequeue(engine);
 }
 
@@ -1096,12 +1208,21 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	if (ret < 0)
 		goto err_shared_data;
 
+	guc->preempt_wq = alloc_workqueue("i915-guc_preempt", WQ_HIGHPRI,
+					  I915_NUM_ENGINES);
+	if (!guc->preempt_wq) {
+		ret = -ENOMEM;
+		goto err_log;
+	}
+
 	ret = guc_ads_create(guc);
 	if (ret < 0)
-		goto err_log;
+		goto err_wq;
 
 	return 0;
 
+err_wq:
+	destroy_workqueue(guc->preempt_wq);
 err_log:
 	intel_guc_log_destroy(guc);
 err_shared_data:
@@ -1116,6 +1237,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	guc_ads_destroy(guc);
+	destroy_workqueue(guc->preempt_wq);
 	intel_guc_log_destroy(guc);
 	guc_shared_data_destroy(guc);
 	guc_stage_desc_pool_destroy(guc);
@@ -1237,6 +1359,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	for_each_engine(engine, dev_priv, id) {
 		struct intel_engine_execlists * const execlists = &engine->execlists;
 		execlists->irq_tasklet.func = i915_guc_irq_handler;
+		INIT_WORK(&engine->guc_preempt_work, inject_preempt_context);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c20ed99cbda0..8af6d6579b18 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -59,6 +59,8 @@ struct intel_guc {
 
 	struct i915_guc_client *client[I915_GUC_NUM_CLIENTS];
 
+	struct workqueue_struct *preempt_wq;
+
 	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
 	/* Cyclic counter mod pagesize	*/
 	u32 db_cacheline;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4f9f12e3c7f6..f5fc41c5c8b3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -354,7 +354,7 @@ static void unwind_wa_tail(struct drm_i915_gem_request *rq)
 	assert_ring_tail_valid(rq->ring, rq->tail);
 }
 
-static void intel_engine_unwind_incomplete_requests(struct intel_engine_cs *engine)
+void intel_engine_unwind_incomplete_requests(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *rq, *rn;
 	struct i915_priolist *uninitialized_var(p);
@@ -682,7 +682,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		execlists_submit_ports(engine);
 }
 
-static void
+void
 execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
 {
 	struct execlist_port *port = execlists->port;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 689fde1a63a9..17182ce29674 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -107,7 +107,6 @@ intel_lr_context_descriptor(struct i915_gem_context *ctx,
 	return ctx->engine[engine->id].lrc_desc;
 }
 
-
 /* Execlists */
 int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
 				    int enable_execlists);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 7777a05bfff5..76d0dae1526b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -498,6 +498,8 @@ struct intel_engine_cs {
 
 	bool needs_cmd_parser;
 
+	struct work_struct guc_preempt_work;
+
 	/*
 	 * Table of commands the command parser needs to know about
 	 * for this engine.
@@ -523,6 +525,9 @@ struct intel_engine_cs {
 	u32 (*get_cmd_length_mask)(u32 cmd_header);
 };
 
+void
+execlists_cancel_port_requests(struct intel_engine_execlists * const execlists);
+
 static inline unsigned int
 execlists_num_ports(const struct intel_engine_execlists * const execlists)
 {
@@ -541,6 +546,8 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
 	memset(port + m, 0, sizeof(struct execlist_port));
 }
 
+
+
 static inline unsigned int
 intel_engine_flag(const struct intel_engine_cs *engine)
 {
@@ -708,6 +715,8 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
 u64 intel_engine_get_active_head(struct intel_engine_cs *engine);
 u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine);
 
+void intel_engine_unwind_incomplete_requests(struct intel_engine_cs *engine);
+
 static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
 {
 	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
-- 
2.13.5

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

[PATCH 12/12] HAX Enable GuC Submission for CI

[Michał Winiarski]

Also:
Revert "drm/i915/guc: Assert that we switch between known ggtt->invalidate functions"

This reverts commit 04f7b24eccdfae680a36e9825fe0d61dcd5ed528.
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++------
 drivers/gpu/drm/i915/i915_params.h  | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4c605785e2b3..a38c6baa49d7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3570,17 +3570,13 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 }
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	/* We should only be called after i915_ggtt_enable_guc() */
-	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c7292268ed43..97c06116d777 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,8 +44,8 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc_loading, 2) \
+	param(int, enable_guc_submission, 2) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.13.5

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

Re: [PATCH 10/12] drm/i915: Rename helpers used for unwinding, use macro for can_preempt

[Chris Wilson]

Quoting Michał Winiarski (2017-10-09 15:52:56)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 9a7aec2c1771..4f9f12e3c7f6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -354,7 +354,7 @@ static void unwind_wa_tail(struct drm_i915_gem_request *rq)
>         assert_ring_tail_valid(rq->ring, rq->tail);
>  }
>  
> -static void unwind_incomplete_requests(struct intel_engine_cs *engine)
> +static void intel_engine_unwind_incomplete_requests(struct intel_engine_cs *engine)

Hmm, but it only applies to execlists and shouldn't be called for
legacy. I'd cheat pass execlists and then
engine = container_of(execlists, typeof(*engine), execlists)

Then whilst you are there pull &engine->timeline->requests into a local.
That should be break even, if not a net gain in smaller code.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for Preemption with GuC, second try

[Patchwork]

== Series Details ==

Series: Preemption with GuC, second try
URL   : https://patchwork.freedesktop.org/series/31591/
State : warning

== Summary ==

Series 31591v1 Preemption with GuC, second try
https://patchwork.freedesktop.org/api/1.0/series/31591/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> SKIP       (fi-glk-1)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-glk-1)
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-threads:
                pass       -> SKIP       (fi-glk-1)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-files:
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_exec:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-default-heavy:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_basic:
        Subgroup basic-blt:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-bsd:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-render:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-vebox:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-blt:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-bsd:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-render:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-vebox:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-blt:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-bsd:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-render:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-vebox:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_create:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_fence:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-wait-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-await-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup await-hang-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup nb-await-default:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-batch-kernel-default-wb:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-pro-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-prw-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-ro-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-rw-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-set-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-wb-pro-default:
                pass       -> SKIP       (fi-glk-1)
WARNING: Long output truncated
fi-bdw-gvtdvm failed to connect after reboot
fi-pnv-d510 failed to connect after reboot

080df4bff8208a891f31629d6415ff29f6d7931a drm-tip: 2017y-10m-09d-15h-14m-40s UTC integration manifest
280e0638d5e1 HAX Enable GuC Submission for CI
b18d83257eb2 drm/i915/guc: Preemption! With GuC
40cefe25c020 drm/i915: Rename helpers used for unwinding, use macro for can_preempt
29cc5eea2bdc drm/i915/guc: Keep request->priority for its lifetime
11f5233cd128 drm/i915: Add information needed to track engine preempt state
fe2a103fa8ea drm/i915: Extract "emit write" part of emit breadcrumb functions
5efafc02bd99 drm/i915/guc: Split guc_wq_item_append
c08ea30dc5b1 drm/i915/guc: Add a second client, to be used for preemption
e635923ded30 drm/i915/guc: Add preemption action to GuC firmware interface
b3db3f128981 drm/i915/guc: Initialize GuC before restarting engines
3e71cd13551f drm/i915/guc: Allocate separate shared data object for GuC communication
b6b6aa5788a5 drm/i915/guc: Extract GuC stage desc pool creation into a helper

== Logs ==

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

Re: [PATCH 01/12] drm/i915/guc: Extract GuC stage desc pool creation into a helper

[Daniele Ceraolo Spurio]

On 09/10/17 07:52, Michał Winiarski wrote:
> Since it's a two-step process, we can have a cleaner error handling in
> the caller if we do the allocations in a helper.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] HAX Enable GuC Submission for CI

[Chris Wilson]

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

Also:
Revert "drm/i915/guc: Assert that we switch between known ggtt->invalidate functions"

This reverts commit 04f7b24eccdfae680a36e9825fe0d61dcd5ed528.
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++------
 drivers/gpu/drm/i915/i915_params.h  | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a26764af22cc..803faffd1b49 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3567,17 +3567,13 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 }
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	/* We should only be called after i915_ggtt_enable_guc() */
-	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 374d3a7cb687..3f25100218ba 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -43,8 +43,8 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc_loading, 1) \
+	param(int, enable_guc_submission, 1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.14.2

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

✗ Fi.CI.BAT: failure for Preemption with GuC, second try (rev2)

[Patchwork]

== Series Details ==

Series: Preemption with GuC, second try (rev2)
URL   : https://patchwork.freedesktop.org/series/31591/
State : failure

== Summary ==

Series 31591v2 Preemption with GuC, second try
https://patchwork.freedesktop.org/api/1.0/series/31591/revisions/2/mbox/

Test gem_sync:
        Subgroup basic-store-each:
                pass       -> FAIL       (fi-kbl-7567u)
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-legacy:
                incomplete -> PASS       (fi-glk-1)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-skl-6700k)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-cnl-y)
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-cnl-y)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-cnl-y)

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:458s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:392s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:573s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:283s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:530s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:527s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:532s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:522s
fi-cfl-s         total:289  pass:256  dwarn:1   dfail:0   fail:0   skip:32  time:561s
fi-cnl-y         total:289  pass:259  dwarn:3   dfail:0   fail:0   skip:27  time:618s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:430s
fi-glk-1         total:289  pass:258  dwarn:3   dfail:0   fail:0   skip:28  time:597s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:433s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:419s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:459s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:499s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:471s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:504s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:583s
fi-kbl-7567u     total:289  pass:264  dwarn:4   dfail:0   fail:1   skip:20  time:499s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:594s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:659s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:469s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:657s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:544s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:513s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:580s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:426s
fi-skl-gvtdvm failed to connect after reboot

080df4bff8208a891f31629d6415ff29f6d7931a drm-tip: 2017y-10m-09d-15h-14m-40s UTC integration manifest
bffdf4cc5f17 HAX Enable GuC Submission for CI
70b5ab3a52a7 drm/i915/guc: Preemption! With GuC
399cf029add0 drm/i915: Rename helpers used for unwinding, use macro for can_preempt
10ed500c103f drm/i915/guc: Keep request->priority for its lifetime
f5012623db29 drm/i915: Add information needed to track engine preempt state
5379a2da3dc6 drm/i915: Extract "emit write" part of emit breadcrumb functions
3db507ba983c drm/i915/guc: Split guc_wq_item_append
01451fc5aada drm/i915/guc: Add a second client, to be used for preemption
57df7a891e8c drm/i915/guc: Add preemption action to GuC firmware interface
185174935ffc drm/i915/guc: Initialize GuC before restarting engines
0c57a253582c drm/i915/guc: Allocate separate shared data object for GuC communication
85b03efabc74 drm/i915/guc: Extract GuC stage desc pool creation into a helper

== Logs ==

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

Re: [PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Daniele Ceraolo Spurio]

On 09/10/17 07:52, Michał Winiarski wrote:
> We were using first page of kernel context render state for sharing data
> with GuC. While it's justified by the fact that those pages are not used
> (note, GuC still enforces this layout and refuses to work if we remove
> the extra page in front), it's also confusing (why are we using this
> particular page?). Let's allocate a separate object instead.
> 
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>

+Michel (engine and watchdog reset with GuC use the shared page)

> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 36 +++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
>   drivers/gpu/drm/i915/intel_guc.h           |  2 ++
>   3 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 8983d53af229..30f026566001 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
>   	memset(desc, 0, sizeof(*desc));
>   }
>   
> +static int guc_shared_data_create(struct intel_guc *guc)
> +{
> +	struct i915_vma *vma;
> +	void *vaddr;
> +
> +	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
> +	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> +	if (IS_ERR(vaddr)) {
> +		i915_vma_unpin_and_release(&vma);
> +		return PTR_ERR(vaddr);
> +	}
> +
> +	guc->shared_data = vma;
> +	guc->shared_data_vaddr = vaddr;

I've noticed that this is now the 3rd place where we allocate and 
immediately pin a vma for guc (the other 2 being 
guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 
operations to a more common helper (and do the same for the cleanup), 
but it's probably better to do it after all the ongoing GuC re-org has 
settled down. In the meantime, this patch is:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

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

Re: [PATCH v2 11/12] drm/i915/guc: Preemption! With GuC

[Chris Wilson]

Quoting Michał Winiarski (2017-10-09 15:52:57)
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 947af576563b..418451755145 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -558,6 +558,89 @@ static void guc_add_request(struct intel_guc *guc,
>         spin_unlock(&client->wq_lock);
>  }
>  
> +#define GUC_PREEMPT_FINISHED 0x1
> +#define GUC_PREEMPT_BREADCRUMB_DWORDS 0x8
> +static void inject_preempt_context(struct work_struct *work)
> +{
> +       struct intel_engine_cs *engine =
> +               container_of(work, typeof(*engine), guc_preempt_work);
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       struct intel_guc *guc = &dev_priv->guc;
> +       struct i915_guc_client *client = guc->client[PREEMPT];
> +       struct intel_ring *ring = client->owner->engine[engine->id].ring;
> +       u32 ctx_desc = lower_32_bits(intel_lr_context_descriptor(client->owner,
> +                                                                engine));
> +       u32 *cs = ring->vaddr + ring->tail;
> +       u32 data[7];
> +
> +       if (engine->id == RCS) {
> +               cs = gen8_emit_ggtt_write_rcs(cs, GUC_PREEMPT_FINISHED,
> +                               intel_hws_preempt_done_address(engine));
> +       } else {
> +               cs = gen8_emit_ggtt_write(cs, GUC_PREEMPT_FINISHED,
> +                               intel_hws_preempt_done_address(engine));
> +               *cs++ = MI_NOOP;
> +               *cs++ = MI_NOOP;
> +       }
> +       *cs++ = MI_USER_INTERRUPT;
> +       *cs++ = MI_NOOP;
> +
> +       GEM_BUG_ON(!IS_ALIGNED(ring->size,
> +                              GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32)));
> +       GEM_BUG_ON((void*)cs - (ring->vaddr + ring->tail) !=
> +                  GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32));
> +
> +       ring->tail += GUC_PREEMPT_BREADCRUMB_DWORDS * sizeof(u32);
> +       ring->tail &= (ring->size - 1);
> +
> +       if (i915_vma_is_map_and_fenceable(ring->vma))
> +               POSTING_READ_FW(GUC_STATUS);
> +
> +       spin_lock_irq(&client->wq_lock);
> +       guc_wq_item_append(client, engine->guc_id, ctx_desc,
> +                          ring->tail / sizeof(u64), 0);
> +       spin_unlock_irq(&client->wq_lock);
> +
> +       data[0] = INTEL_GUC_ACTION_REQUEST_PREEMPTION;
> +       data[1] = client->stage_id;
> +       data[2] = INTEL_GUC_PREEMPT_OPTION_IMMEDIATE |
> +                 INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q |
> +                 INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q;
> +       data[3] = engine->guc_id;
> +       data[4] = guc->client[SUBMIT]->priority;
> +       data[5] = guc->client[SUBMIT]->stage_id;
> +       data[6] = guc_ggtt_offset(guc->shared_data);
> +
> +       if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {

Not speaking from experience or anything... But if this fails once, it's
likely to keep failing! Ergo WARN_ON_ONCE.

Broxton:
[   14.217681] [drm] INTEL_GUC_SEND: Action 0x2 failed; ret=-5 status=0xF000F000 response=0x00000010

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

Re: [PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Michel Thierry]

On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote:
> 
> 
> On 09/10/17 07:52, Michał Winiarski wrote:
>> We were using first page of kernel context render state for sharing data
>> with GuC. While it's justified by the fact that those pages are not used
>> (note, GuC still enforces this layout and refuses to work if we remove
>> the extra page in front), it's also confusing (why are we using this
>> particular page?). Let's allocate a separate object instead.
>>
>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Jeff McGee <jeff.mcgee@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
> 
> +Michel (engine and watchdog reset with GuC use the shared page)
> 
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 36 
>> +++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
>>   drivers/gpu/drm/i915/intel_guc.h           |  2 ++
>>   3 files changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 8983d53af229..30f026566001 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc 
>> *guc,
>>       memset(desc, 0, sizeof(*desc));
>>   }
>> +static int guc_shared_data_create(struct intel_guc *guc)
>> +{
>> +    struct i915_vma *vma;
>> +    void *vaddr;
>> +
>> +    vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
>> +    if (IS_ERR(vma))
>> +        return PTR_ERR(vma);
>> +
>> +    vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
>> +    if (IS_ERR(vaddr)) {
>> +        i915_vma_unpin_and_release(&vma);
>> +        return PTR_ERR(vaddr);
>> +    }
>> +
>> +    guc->shared_data = vma;
>> +    guc->shared_data_vaddr = vaddr;

Hi,

Allocating the shared_data until this point (i915_guc_submission_init) 
will be too late for GuC's watchdog.

GuC watchdog happens without i915 knowledge, so we have to pass this 
shared_data_offset during guc_params_init (in params[9] for the curious) 
instead of a h2g command; and the GuC parameters block has this note: 
"These parameters are read by the firmware on startup and cannot be 
changed thereafter".

Michał, if you plan to send another version of this, could you move it 
to guc_params_init? It isn't a big issue, I can just move it when we 
have an open source user and can upstream GuC watchdog.

Thanks,

-Michel

> 
> I've noticed that this is now the 3rd place where we allocate and 
> immediately pin a vma for guc (the other 2 being 
> guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 
> operations to a more common helper (and do the same for the cleanup), 
> but it's probably better to do it after all the ongoing GuC re-org has 
> settled down. In the meantime, this patch is:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Michel Thierry]

On 09/10/17 15:35, Michel Thierry wrote:
> On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 09/10/17 07:52, Michał Winiarski wrote:
>>> We were using first page of kernel context render state for sharing data
>>> with GuC. While it's justified by the fact that those pages are not used
>>> (note, GuC still enforces this layout and refuses to work if we remove
>>> the extra page in front), it's also confusing (why are we using this
>>> particular page?). Let's allocate a separate object instead.
>>>
>>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Jeff McGee <jeff.mcgee@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>
>> +Michel (engine and watchdog reset with GuC use the shared page)
>>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 36 
>>> +++++++++++++++++++++++++++++-
>>>   drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
>>>   drivers/gpu/drm/i915/intel_guc.h           |  2 ++
>>>   3 files changed, 39 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index 8983d53af229..30f026566001 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc 
>>> *guc,
>>>       memset(desc, 0, sizeof(*desc));
>>>   }
>>> +static int guc_shared_data_create(struct intel_guc *guc)
>>> +{
>>> +    struct i915_vma *vma;
>>> +    void *vaddr;
>>> +
>>> +    vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
>>> +    if (IS_ERR(vma))
>>> +        return PTR_ERR(vma);
>>> +
>>> +    vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
>>> +    if (IS_ERR(vaddr)) {
>>> +        i915_vma_unpin_and_release(&vma);
>>> +        return PTR_ERR(vaddr);
>>> +    }
>>> +
>>> +    guc->shared_data = vma;
>>> +    guc->shared_data_vaddr = vaddr;
> 
> Hi,
> 
> Allocating the shared_data until this point (i915_guc_submission_init) 
> will be too late for GuC's watchdog.
> 
> GuC watchdog happens without i915 knowledge, so we have to pass this 
> shared_data_offset during guc_params_init (in params[9] for the curious) 
> instead of a h2g command; and the GuC parameters block has this note: 
> "These parameters are read by the firmware on startup and cannot be 
> changed thereafter".
> 
> Michał, if you plan to send another version of this, could you move it 
> to guc_params_init? It isn't a big issue, I can just move it when we 
> have an open source user and can upstream GuC watchdog.
> 
> Thanks,
> 
> -Michel
> 

Ignore my previous reply, this is already being allocated before 
guc_params_init as it is.

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

>>
>> I've noticed that this is now the 3rd place where we allocate and 
>> immediately pin a vma for guc (the other 2 being 
>> guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 
>> operations to a more common helper (and do the same for the cleanup), 
>> but it's probably better to do it after all the ongoing GuC re-org has 
>> settled down. In the meantime, this patch is:
>>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/guc: Initialize GuC before restarting engines

[Michel Thierry]

On 09/10/17 07:52, Michał Winiarski wrote:
> Now that we're handling request resubmission the same way as regular
> submission (from the tasklet), we can move GuC initialization earlier,
> before restarting the engines. This way, we're no longer being in the
> state of flux during engine restart - we're already in user requested
> submission mode.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

The nice side-effect of this is that in order support reset engine using 
GuC submission, now the only missing bit is to use a h2g command instead 
of calling intel_gpu_reset.

Tested-by: Michel Thierry <michel.thierry@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_gem.c            | 10 +++++-----
>   drivers/gpu/drm/i915/i915_guc_submission.c |  7 -------
>   drivers/gpu/drm/i915/intel_lrc.c           |  2 +-
>   3 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 82a10036fb38..6c9f0a151d0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4780,6 +4780,11 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>   		goto out;
>   	}
>   
> +	/* We can't enable contexts until all firmware is loaded */
> +	ret = intel_uc_init_hw(dev_priv);
> +	if (ret)
> +		goto out;
> +
>   	/* Need to do basic initialisation of all rings first: */
>   	ret = __i915_gem_restart_engines(dev_priv);
>   	if (ret)
> @@ -4787,11 +4792,6 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>   
>   	intel_mocs_init_l3cc_table(dev_priv);
>   
> -	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_uc_init_hw(dev_priv);
> -	if (ret)
> -		goto out;
> -
>   out:
>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>   	return ret;
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 30f026566001..7e2c9136a2fa 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1186,14 +1186,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>   
>   	for_each_engine(engine, dev_priv, id) {
>   		struct intel_engine_execlists * const execlists = &engine->execlists;
> -		/* The tasklet was initialised by execlists, and may be in
> -		 * a state of flux (across a reset) and so we just want to
> -		 * take over the callback without changing any other state
> -		 * in the tasklet.
> -		 */
>   		execlists->irq_tasklet.func = i915_guc_irq_handler;
> -		clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> -		tasklet_schedule(&execlists->irq_tasklet);
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 721432ddf403..52f4dbf2fc40 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1462,7 +1462,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>   	execlists->preempt = false;
>   
>   	/* After a GPU reset, we may have requests to replay */
> -	if (!i915_modparams.enable_guc_submission && execlists->first)
> +	if (execlists->first)
>   		tasklet_schedule(&execlists->irq_tasklet);
>   
>   	return 0;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Michel Thierry]

On 12/10/17 13:35, Michel Thierry wrote:
> On 09/10/17 15:35, Michel Thierry wrote:
>> On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote:
>>>
>>>
>>> On 09/10/17 07:52, Michał Winiarski wrote:
>>>> We were using first page of kernel context render state for sharing 
>>>> data
>>>> with GuC. While it's justified by the fact that those pages are not 
>>>> used
>>>> (note, GuC still enforces this layout and refuses to work if we remove
>>>> the extra page in front), it's also confusing (why are we using this
>>>> particular page?). Let's allocate a separate object instead.
>>>>
>>>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Jeff McGee <jeff.mcgee@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>>
>>> +Michel (engine and watchdog reset with GuC use the shared page)
>>>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 36 
>>>> +++++++++++++++++++++++++++++-
>>>>   drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
>>>>   drivers/gpu/drm/i915/intel_guc.h           |  2 ++
>>>>   3 files changed, 39 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 8983d53af229..30f026566001 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct 
>>>> intel_guc *guc,
>>>>       memset(desc, 0, sizeof(*desc));
>>>>   }
>>>> +static int guc_shared_data_create(struct intel_guc *guc)
>>>> +{
>>>> +    struct i915_vma *vma;
>>>> +    void *vaddr;
>>>> +
>>>> +    vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
>>>> +    if (IS_ERR(vma))
>>>> +        return PTR_ERR(vma);
>>>> +
>>>> +    vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
>>>> +    if (IS_ERR(vaddr)) {
>>>> +        i915_vma_unpin_and_release(&vma);
>>>> +        return PTR_ERR(vaddr);
>>>> +    }
>>>> +
>>>> +    guc->shared_data = vma;
>>>> +    guc->shared_data_vaddr = vaddr;
>>
>> Hi,
>>
>> Allocating the shared_data until this point (i915_guc_submission_init) 
>> will be too late for GuC's watchdog.
>>
>> GuC watchdog happens without i915 knowledge, so we have to pass this 
>> shared_data_offset during guc_params_init (in params[9] for the 
>> curious) instead of a h2g command; and the GuC parameters block has 
>> this note: "These parameters are read by the firmware on startup and 
>> cannot be changed thereafter".
>>
>> Michał, if you plan to send another version of this, could you move it 
>> to guc_params_init? It isn't a big issue, I can just move it when we 
>> have an open source user and can upstream GuC watchdog.
>>
>> Thanks,
>>
>> -Michel
>>
> 
> Ignore my previous reply, this is already being allocated before 
> guc_params_init as it is.
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> 

I spoke too soon (and sorry for all the spam), the kernel_context is now 
redundant code and should be removed from the suspend & resume functions:

diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c
index 5cd9bc53e021..47c74ef0bd23 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -176,7 +176,6 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 
rsa_offset)
  int intel_guc_suspend(struct drm_i915_private *dev_priv)
  {
  	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_gem_context *ctx;
  	u32 data[3];

  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
@@ -184,8 +183,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)

  	gen9_disable_guc_interrupts(dev_priv);

-	ctx = dev_priv->kernel_context;
-
  	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
  	/* any value greater than GUC_POWER_D0 */
  	data[1] = GUC_POWER_D1;
@@ -225,7 +222,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
  int intel_guc_resume(struct drm_i915_private *dev_priv)
  {
  	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_gem_context *ctx;
  	u32 data[3];

  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
@@ -234,8 +230,6 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
  	if (i915_modparams.guc_log_level >= 0)
  		gen9_enable_guc_interrupts(dev_priv);

-	ctx = dev_priv->kernel_context;
-
  	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
  	data[1] = GUC_POWER_D0;
  	data[2] = guc_ggtt_offset(guc->shared_data);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx