[PATCH 0/4] GuC Fixes and change for v9+ Firmwares

[PATCH 0/4] GuC Fixes and change for v9+ Firmwares

[Sagar Arun Kamble]

This series fixes bugs in suspend/unload/reset path with GuC enabled.
With v9+ firmware new type of fast (Default/Critical) logging is to
be enabled by default. A patch enables that logging by default.
Once GuC v9+ firmwares are posted to 01.org, change to update the
default firmware version and if needed change to enabled GuC submission
will be added later.

Cc: Spotswood John A <john.a.spotswood@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>

Sagar Arun Kamble (4):
  drm/i915: Separate GuC/HuC specific functionality from intel_uc
  drm/i915/guc: Fix GuC interaction in reset/suspend scenarios
  drm/i915/guc: Fix GuC HW/SW state cleanup in unload path
  drm/i915/guc: Enable default/critical logging in GuC by default from
    GuC v9

 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/i915_drv.c            |  13 +-
 drivers/gpu/drm/i915/i915_drv.h            |   1 +
 drivers/gpu/drm/i915/i915_gem.c            |  85 ++++++-
 drivers/gpu/drm/i915/i915_guc_submission.c |  53 -----
 drivers/gpu/drm/i915/intel_guc.c           | 358 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           | 206 +++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  14 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    |  11 +-
 drivers/gpu/drm/i915/intel_guc_log.c       |   7 +-
 drivers/gpu/drm/i915/intel_huc.c           |  50 +---
 drivers/gpu/drm/i915/intel_huc.h           |  38 +++
 drivers/gpu/drm/i915/intel_uc.c            | 169 ++------------
 drivers/gpu/drm/i915/intel_uc.h            | 254 +-------------------
 drivers/gpu/drm/i915/intel_uc_common.h     | 102 ++++++++
 15 files changed, 843 insertions(+), 519 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc.h
 create mode 100644 drivers/gpu/drm/i915/intel_huc.h
 create mode 100644 drivers/gpu/drm/i915/intel_uc_common.h

-- 
1.9.1

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

[PATCH 1/4] drm/i915: Separate GuC/HuC specific functionality from intel_uc

[Sagar Arun Kamble]

Removed unnecessary intel_uc.h includes as it is present in i915_drv.h.
Created intel_guc.c and intel_guc.h for placing GuC specific code.
Created intel_huc.h to refer to HuC specific functions.

v2: Prepared intel_uc_common.h. huc_auth code declaration adjusted.
Moved enable/disable_communication to intel_uc.c (Michal)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/i915_drv.c            |   1 -
 drivers/gpu/drm/i915/i915_guc_submission.c |   1 -
 drivers/gpu/drm/i915/intel_guc.c           | 193 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           | 200 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c    |   1 -
 drivers/gpu/drm/i915/intel_huc.c           |  50 +-----
 drivers/gpu/drm/i915/intel_huc.h           |  38 +++++
 drivers/gpu/drm/i915/intel_uc.c            | 128 +--------------
 drivers/gpu/drm/i915/intel_uc.h            | 254 +----------------------------
 drivers/gpu/drm/i915/intel_uc_common.h     | 101 ++++++++++++
 11 files changed, 545 insertions(+), 423 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc.h
 create mode 100644 drivers/gpu/drm/i915/intel_huc.h
 create mode 100644 drivers/gpu/drm/i915/intel_uc_common.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 892f52b..efc5b30 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -59,6 +59,7 @@ i915-y += i915_cmd_parser.o \
 
 # general-purpose microcontroller (GuC) support
 i915-y += intel_uc.o \
+	  intel_guc.o \
 	  intel_guc_ct.o \
 	  intel_guc_log.o \
 	  intel_guc_loader.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f10a078..2ae730c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -50,7 +50,6 @@
 #include "i915_trace.h"
 #include "i915_vgpu.h"
 #include "intel_drv.h"
-#include "intel_uc.h"
 
 static struct drm_driver driver;
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 48a1e93..602ae8a 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -23,7 +23,6 @@
  */
 #include <linux/circ_buf.h>
 #include "i915_drv.h"
-#include "intel_uc.h"
 
 #include <trace/events/dma_fence.h>
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
new file mode 100644
index 0000000..978a0e3
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -0,0 +1,193 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "i915_drv.h"
+
+int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
+{
+	WARN(1, "Unexpected send: action=%#x\n", *action);
+	return -ENODEV;
+}
+
+static void gen8_guc_raise_irq(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+}
+
+void intel_guc_init_early(struct intel_guc *guc)
+{
+	intel_guc_ct_init_early(&guc->ct);
+
+	mutex_init(&guc->send_mutex);
+	guc->send = intel_guc_send_nop;
+	guc->notify = gen8_guc_raise_irq;
+}
+
+static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
+{
+	GEM_BUG_ON(!guc->send_regs.base);
+	GEM_BUG_ON(!guc->send_regs.count);
+	GEM_BUG_ON(i >= guc->send_regs.count);
+
+	return _MMIO(guc->send_regs.base + 4 * i);
+}
+
+void intel_guc_init_send_regs(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	enum forcewake_domains fw_domains = 0;
+	unsigned int i;
+
+	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
+	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
+
+	for (i = 0; i < guc->send_regs.count; i++) {
+		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+					guc_send_reg(guc, i),
+					FW_REG_READ | FW_REG_WRITE);
+	}
+	guc->send_regs.fw_domains = fw_domains;
+}
+
+/*
+ * This function implements the MMIO based host to GuC interface.
+ */
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	u32 status;
+	int i;
+	int ret;
+
+	GEM_BUG_ON(!len);
+	GEM_BUG_ON(len > guc->send_regs.count);
+
+	/* If CT is available, we expect to use MMIO only during init/fini */
+	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
+		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
+		*action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
+
+	mutex_lock(&guc->send_mutex);
+	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
+
+	for (i = 0; i < len; i++)
+		I915_WRITE(guc_send_reg(guc, i), action[i]);
+
+	POSTING_READ(guc_send_reg(guc, i - 1));
+
+	intel_guc_notify(guc);
+
+	/*
+	 * No GuC command should ever take longer than 10ms.
+	 * Fast commands should still complete in 10us.
+	 */
+	ret = __intel_wait_for_register_fw(dev_priv,
+					   guc_send_reg(guc, 0),
+					   INTEL_GUC_RECV_MASK,
+					   INTEL_GUC_RECV_MASK,
+					   10, 10, &status);
+	if (status != INTEL_GUC_STATUS_SUCCESS) {
+		/*
+		 * Either the GuC explicitly returned an error (which
+		 * we convert to -EIO here) or no response at all was
+		 * received within the timeout limit (-ETIMEDOUT)
+		 */
+		if (ret != -ETIMEDOUT)
+			ret = -EIO;
+
+		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
+			 " ret=%d status=0x%08X response=0x%08X\n",
+			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
+	}
+
+	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
+	mutex_unlock(&guc->send_mutex);
+
+	return ret;
+}
+
+int intel_guc_sample_forcewake(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	u32 action[2];
+
+	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
+	/* WaRsDisableCoarsePowerGating:skl,bxt */
+	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
+		action[1] = 0;
+	else
+		/* bit 0 and 1 are for Render and Media domain separately */
+		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
+
+	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+}
+
+/**
+ * intel_guc_auth_huc() - authenticate HuC
+ *
+ * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
+ * authenticate_huc interface.
+ */
+void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_vma *vma;
+	int ret;
+	u32 data[2];
+
+	vma = i915_gem_object_ggtt_pin(huc_fw->obj, NULL, 0, 0,
+				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+	if (IS_ERR(vma)) {
+		DRM_ERROR("failed to pin huc fw object %d\n",
+				(int)PTR_ERR(vma));
+		return;
+	}
+
+	/* Specify auth action and where public signature is. */
+	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
+	data[1] = guc_ggtt_offset(vma) + huc_fw->rsa_offset;
+
+	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
+	if (ret) {
+		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
+		goto out;
+	}
+
+	/* Check authentication status, it should be done by now */
+	ret = intel_wait_for_register(dev_priv,
+				HUC_STATUS2,
+				HUC_FW_VERIFIED,
+				HUC_FW_VERIFIED,
+				50);
+
+	if (ret) {
+		DRM_ERROR("HuC: Authentication failed %d\n", ret);
+		goto out;
+	}
+
+out:
+	i915_vma_unpin(vma);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
new file mode 100644
index 0000000..b329830
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -0,0 +1,200 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+#ifndef _INTEL_GUC_H_
+#define _INTEL_GUC_H_
+
+#include "intel_guc_fwif.h"
+#include "i915_guc_reg.h"
+#include "intel_ringbuffer.h"
+#include "intel_guc_ct.h"
+#include "i915_vma.h"
+
+struct drm_i915_gem_request;
+
+/*
+ * This structure primarily describes the GEM object shared with the GuC.
+ * The specs sometimes refer to this object as a "GuC context", but we use
+ * the term "client" to avoid confusion with hardware contexts. This
+ * GEM object is held for the entire lifetime of our interaction with
+ * the GuC, being allocated before the GuC is loaded with its firmware.
+ * Because there's no way to update the address used by the GuC after
+ * initialisation, the shared object must stay pinned into the GGTT as
+ * long as the GuC is in use. We also keep the first page (only) mapped
+ * into kernel address space, as it includes shared data that must be
+ * updated on every request submission.
+ *
+ * The single GEM object described here is actually made up of several
+ * separate areas, as far as the GuC is concerned. The first page (kept
+ * kmap'd) includes the "process descriptor" which holds sequence data for
+ * the doorbell, and one cacheline which actually *is* the doorbell; a
+ * write to this will "ring the doorbell" (i.e. send an interrupt to the
+ * GuC). The subsequent  pages of the client object constitute the work
+ * queue (a circular array of work items), again described in the process
+ * descriptor. Work queue pages are mapped momentarily as required.
+ *
+ * We also keep a few statistics on failures. Ideally, these should all
+ * be zero!
+ *   no_wq_space: times that the submission pre-check found no space was
+ *                available in the work queue (note, the queue is shared,
+ *                not per-engine). It is OK for this to be nonzero, but
+ *                it should not be huge!
+ *   b_fail: failed to ring the doorbell. This should never happen, unless
+ *           somehow the hardware misbehaves, or maybe if the GuC firmware
+ *           crashes? We probably need to reset the GPU to recover.
+ *   retcode: errno from last guc_submit()
+ */
+struct i915_guc_client {
+	struct i915_vma *vma;
+	void *vaddr;
+	struct i915_gem_context *owner;
+	struct intel_guc *guc;
+
+	uint32_t engines;		/* bitmap of (host) engine ids	*/
+	uint32_t priority;
+	u32 stage_id;
+	uint32_t proc_desc_offset;
+
+	u16 doorbell_id;
+	unsigned long doorbell_offset;
+	u32 doorbell_cookie;
+
+	spinlock_t wq_lock;
+	uint32_t wq_offset;
+	uint32_t wq_size;
+	uint32_t wq_tail;
+	uint32_t wq_rsvd;
+	uint32_t no_wq_space;
+
+	/* Per-engine counts of GuC submissions */
+	uint64_t submissions[I915_NUM_ENGINES];
+};
+
+struct intel_guc_log {
+	uint32_t flags;
+	struct i915_vma *vma;
+	/* The runtime stuff gets created only when GuC logging gets enabled */
+	struct {
+		void *buf_addr;
+		struct workqueue_struct *flush_wq;
+		struct work_struct flush_work;
+		struct rchan *relay_chan;
+	} runtime;
+	/* logging related stats */
+	u32 capture_miss_count;
+	u32 flush_interrupt_count;
+	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
+	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
+	u32 flush_count[GUC_MAX_LOG_BUFFER];
+};
+
+struct intel_guc {
+	struct intel_uc_fw fw;
+	struct intel_guc_log log;
+	struct intel_guc_ct ct;
+
+	/* Log snapshot if GuC errors during load */
+	struct drm_i915_gem_object *load_err_log;
+
+	/* intel_guc_recv interrupt related state */
+	bool interrupts_enabled;
+
+	struct i915_vma *ads_vma;
+	struct i915_vma *stage_desc_pool;
+	void *stage_desc_pool_vaddr;
+	struct ida stage_ids;
+
+	struct i915_guc_client *execbuf_client;
+
+	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
+	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
+
+	/* GuC's FW specific registers used in MMIO send */
+	struct {
+		u32 base;
+		unsigned int count;
+		enum forcewake_domains fw_domains;
+	} send_regs;
+
+	/* To serialize the intel_guc_send actions */
+	struct mutex send_mutex;
+
+	/* GuC's FW specific send function */
+	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
+
+	/* GuC's FW specific notify function */
+	void (*notify)(struct intel_guc *guc);
+};
+
+static inline u32 guc_ggtt_offset(struct i915_vma *vma)
+{
+	u32 offset = i915_ggtt_offset(vma);
+
+	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
+	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
+	return offset;
+}
+
+/* intel_guc.c */
+void intel_guc_init_early(struct intel_guc *guc);
+int intel_guc_sample_forcewake(struct intel_guc *guc);
+void intel_guc_init_send_regs(struct intel_guc *guc);
+int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw);
+
+static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
+				 u32 len)
+{
+	return guc->send(guc, action, len);
+}
+
+static inline void intel_guc_notify(struct intel_guc *guc)
+{
+	guc->notify(guc);
+}
+
+/* intel_guc_loader.c */
+int intel_guc_select_fw(struct intel_guc *guc);
+int intel_guc_init_hw(struct intel_guc *guc);
+int intel_guc_suspend(struct drm_i915_private *dev_priv);
+int intel_guc_resume(struct drm_i915_private *dev_priv);
+u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
+
+/* i915_guc_submission.c */
+int i915_guc_submission_init(struct drm_i915_private *dev_priv);
+int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
+int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
+void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
+void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
+void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
+struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
+
+/* intel_guc_log.c */
+int intel_guc_log_create(struct intel_guc *guc);
+void intel_guc_log_destroy(struct intel_guc *guc);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
+void i915_guc_log_register(struct drm_i915_private *dev_priv);
+void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
+
+#endif
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 8b0ae7f..81e03a6 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -27,7 +27,6 @@
  *    Alex Dai <yu.dai@intel.com>
  */
 #include "i915_drv.h"
-#include "intel_uc.h"
 
 /**
  * DOC: GuC-specific firmware loader
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6145fa0..f9459f5 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -21,9 +21,7 @@
  * IN THE SOFTWARE.
  *
  */
-#include <linux/firmware.h>
 #include "i915_drv.h"
-#include "intel_uc.h"
 
 /**
  * DOC: HuC Firmware
@@ -225,54 +223,14 @@ void intel_huc_init_hw(struct intel_huc *huc)
 }
 
 /**
- * intel_guc_auth_huc() - authenticate ucode
- * @dev_priv: the drm_i915_device
- *
- * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
- * authenticate_huc interface.
+ * intel_auth_huc() - authenticate HuC ucode
  */
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
+void intel_huc_auth(struct intel_huc *huc)
 {
-	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_huc *huc = &dev_priv->huc;
-	struct i915_vma *vma;
-	int ret;
-	u32 data[2];
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return;
 
-	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
-				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
-	if (IS_ERR(vma)) {
-		DRM_ERROR("failed to pin huc fw object %d\n",
-				(int)PTR_ERR(vma));
-		return;
-	}
-
-	/* Specify auth action and where public signature is. */
-	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
-	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
-
-	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
-	if (ret) {
-		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
-		goto out;
-	}
-
-	/* Check authentication status, it should be done by now */
-	ret = intel_wait_for_register(dev_priv,
-				HUC_STATUS2,
-				HUC_FW_VERIFIED,
-				HUC_FW_VERIFIED,
-				50);
-
-	if (ret) {
-		DRM_ERROR("HuC: Authentication failed %d\n", ret);
-		goto out;
-	}
-
-out:
-	i915_vma_unpin(vma);
+	intel_guc_auth_huc(&dev_priv->guc, &huc->fw);
 }
-
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
new file mode 100644
index 0000000..e90952b
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+#ifndef _INTEL_HUC_H_
+#define _INTEL_HUC_H_
+
+struct intel_huc {
+	/* Generic uC firmware management */
+	struct intel_uc_fw fw;
+
+	/* HuC-specific additions */
+};
+
+/* intel_huc.c */
+void intel_huc_select_fw(struct intel_huc *huc);
+void intel_huc_init_hw(struct intel_huc *huc);
+void intel_huc_auth(struct intel_huc *huc);
+#endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..a3fc4c8 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -23,7 +23,6 @@
  */
 
 #include "i915_drv.h"
-#include "intel_uc.h"
 #include <linux/firmware.h>
 
 /* Cleans up uC firmware by releasing the firmware GEM obj.
@@ -94,22 +93,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
 }
 
-static void gen8_guc_raise_irq(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
-}
-
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
-	intel_guc_ct_init_early(&guc->ct);
-
-	mutex_init(&guc->send_mutex);
-	guc->send = intel_guc_send_nop;
-	guc->notify = gen8_guc_raise_irq;
+	intel_guc_init_early(guc);
 }
 
 static void fetch_uc_fw(struct drm_i915_private *dev_priv,
@@ -262,32 +250,6 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 	__intel_uc_fw_fini(&dev_priv->huc.fw);
 }
 
-static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
-{
-	GEM_BUG_ON(!guc->send_regs.base);
-	GEM_BUG_ON(!guc->send_regs.count);
-	GEM_BUG_ON(i >= guc->send_regs.count);
-
-	return _MMIO(guc->send_regs.base + 4 * i);
-}
-
-static void guc_init_send_regs(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	enum forcewake_domains fw_domains = 0;
-	unsigned int i;
-
-	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
-	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
-
-	for (i = 0; i < guc->send_regs.count; i++) {
-		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
-					guc_send_reg(guc, i),
-					FW_REG_READ | FW_REG_WRITE);
-	}
-	guc->send_regs.fw_domains = fw_domains;
-}
-
 static void guc_capture_load_err_log(struct intel_guc *guc)
 {
 	if (!guc->log.vma || i915.guc_log_level < 0)
@@ -295,8 +257,6 @@ static void guc_capture_load_err_log(struct intel_guc *guc)
 
 	if (!guc->load_err_log)
 		guc->load_err_log = i915_gem_object_get(guc->log.vma->obj);
-
-	return;
 }
 
 static void guc_free_load_err_log(struct intel_guc *guc)
@@ -309,8 +269,6 @@ static int guc_enable_communication(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	guc_init_send_regs(guc);
-
 	if (HAS_GUC_CT(dev_priv))
 		return intel_guc_enable_ct(guc);
 
@@ -331,6 +289,7 @@ static void guc_disable_communication(struct intel_guc *guc)
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_huc *huc = &dev_priv->huc;
 	int ret, attempts;
 
 	if (!i915.enable_guc_loading)
@@ -386,11 +345,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
+	intel_guc_init_send_regs(guc);
+
 	ret = guc_enable_communication(guc);
 	if (ret)
 		goto err_log_capture;
 
-	intel_guc_auth_huc(dev_priv);
+	intel_huc_auth(huc);
 	if (i915.enable_guc_submission) {
 		if (i915.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
@@ -458,82 +419,3 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	i915_ggtt_disable_guc(dev_priv);
 }
-
-int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
-{
-	WARN(1, "Unexpected send: action=%#x\n", *action);
-	return -ENODEV;
-}
-
-/*
- * This function implements the MMIO based host to GuC interface.
- */
-int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	u32 status;
-	int i;
-	int ret;
-
-	GEM_BUG_ON(!len);
-	GEM_BUG_ON(len > guc->send_regs.count);
-
-	/* If CT is available, we expect to use MMIO only during init/fini */
-	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
-		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
-		*action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
-
-	mutex_lock(&guc->send_mutex);
-	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
-
-	for (i = 0; i < len; i++)
-		I915_WRITE(guc_send_reg(guc, i), action[i]);
-
-	POSTING_READ(guc_send_reg(guc, i - 1));
-
-	intel_guc_notify(guc);
-
-	/*
-	 * No GuC command should ever take longer than 10ms.
-	 * Fast commands should still complete in 10us.
-	 */
-	ret = __intel_wait_for_register_fw(dev_priv,
-					   guc_send_reg(guc, 0),
-					   INTEL_GUC_RECV_MASK,
-					   INTEL_GUC_RECV_MASK,
-					   10, 10, &status);
-	if (status != INTEL_GUC_STATUS_SUCCESS) {
-		/*
-		 * Either the GuC explicitly returned an error (which
-		 * we convert to -EIO here) or no response at all was
-		 * received within the timeout limit (-ETIMEDOUT)
-		 */
-		if (ret != -ETIMEDOUT)
-			ret = -EIO;
-
-		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
-			 " ret=%d status=0x%08X response=0x%08X\n",
-			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
-	}
-
-	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
-	mutex_unlock(&guc->send_mutex);
-
-	return ret;
-}
-
-int intel_guc_sample_forcewake(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	u32 action[2];
-
-	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
-	/* WaRsDisableCoarsePowerGating:skl,bxt */
-	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
-		action[1] = 0;
-	else
-		/* bit 0 and 1 are for Render and Media domain separately */
-		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
-
-	return intel_guc_send(guc, action, ARRAY_SIZE(action));
-}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 22ae52b..c87a2b4 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -24,256 +24,8 @@
 #ifndef _INTEL_UC_H_
 #define _INTEL_UC_H_
 
-#include "intel_guc_fwif.h"
-#include "i915_guc_reg.h"
-#include "intel_ringbuffer.h"
-#include "intel_guc_ct.h"
-#include "i915_vma.h"
-
-struct drm_i915_gem_request;
-
-/*
- * This structure primarily describes the GEM object shared with the GuC.
- * The specs sometimes refer to this object as a "GuC context", but we use
- * the term "client" to avoid confusion with hardware contexts. This
- * GEM object is held for the entire lifetime of our interaction with
- * the GuC, being allocated before the GuC is loaded with its firmware.
- * Because there's no way to update the address used by the GuC after
- * initialisation, the shared object must stay pinned into the GGTT as
- * long as the GuC is in use. We also keep the first page (only) mapped
- * into kernel address space, as it includes shared data that must be
- * updated on every request submission.
- *
- * The single GEM object described here is actually made up of several
- * separate areas, as far as the GuC is concerned. The first page (kept
- * kmap'd) includes the "process descriptor" which holds sequence data for
- * the doorbell, and one cacheline which actually *is* the doorbell; a
- * write to this will "ring the doorbell" (i.e. send an interrupt to the
- * GuC). The subsequent  pages of the client object constitute the work
- * queue (a circular array of work items), again described in the process
- * descriptor. Work queue pages are mapped momentarily as required.
- *
- * We also keep a few statistics on failures. Ideally, these should all
- * be zero!
- *   no_wq_space: times that the submission pre-check found no space was
- *                available in the work queue (note, the queue is shared,
- *                not per-engine). It is OK for this to be nonzero, but
- *                it should not be huge!
- *   b_fail: failed to ring the doorbell. This should never happen, unless
- *           somehow the hardware misbehaves, or maybe if the GuC firmware
- *           crashes? We probably need to reset the GPU to recover.
- *   retcode: errno from last guc_submit()
- */
-struct i915_guc_client {
-	struct i915_vma *vma;
-	void *vaddr;
-	struct i915_gem_context *owner;
-	struct intel_guc *guc;
-
-	uint32_t engines;		/* bitmap of (host) engine ids	*/
-	uint32_t priority;
-	u32 stage_id;
-	uint32_t proc_desc_offset;
-
-	u16 doorbell_id;
-	unsigned long doorbell_offset;
-	u32 doorbell_cookie;
-
-	spinlock_t wq_lock;
-	uint32_t wq_offset;
-	uint32_t wq_size;
-	uint32_t wq_tail;
-	uint32_t wq_rsvd;
-	uint32_t no_wq_space;
-
-	/* Per-engine counts of GuC submissions */
-	uint64_t submissions[I915_NUM_ENGINES];
-};
-
-enum intel_uc_fw_status {
-	INTEL_UC_FIRMWARE_FAIL = -1,
-	INTEL_UC_FIRMWARE_NONE = 0,
-	INTEL_UC_FIRMWARE_PENDING,
-	INTEL_UC_FIRMWARE_SUCCESS
-};
-
-/* User-friendly representation of an enum */
-static inline
-const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
-{
-	switch (status) {
-	case INTEL_UC_FIRMWARE_FAIL:
-		return "FAIL";
-	case INTEL_UC_FIRMWARE_NONE:
-		return "NONE";
-	case INTEL_UC_FIRMWARE_PENDING:
-		return "PENDING";
-	case INTEL_UC_FIRMWARE_SUCCESS:
-		return "SUCCESS";
-	}
-	return "<invalid>";
-}
-
-enum intel_uc_fw_type {
-	INTEL_UC_FW_TYPE_GUC,
-	INTEL_UC_FW_TYPE_HUC
-};
-
-/* User-friendly representation of an enum */
-static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
-{
-	switch (type) {
-	case INTEL_UC_FW_TYPE_GUC:
-		return "GuC";
-	case INTEL_UC_FW_TYPE_HUC:
-		return "HuC";
-	}
-	return "uC";
-}
-
-/*
- * This structure encapsulates all the data needed during the process
- * of fetching, caching, and loading the firmware image into the GuC.
- */
-struct intel_uc_fw {
-	const char *path;
-	size_t size;
-	struct drm_i915_gem_object *obj;
-	enum intel_uc_fw_status fetch_status;
-	enum intel_uc_fw_status load_status;
-
-	uint16_t major_ver_wanted;
-	uint16_t minor_ver_wanted;
-	uint16_t major_ver_found;
-	uint16_t minor_ver_found;
-
-	enum intel_uc_fw_type type;
-	uint32_t header_size;
-	uint32_t header_offset;
-	uint32_t rsa_size;
-	uint32_t rsa_offset;
-	uint32_t ucode_size;
-	uint32_t ucode_offset;
-};
-
-struct intel_guc_log {
-	uint32_t flags;
-	struct i915_vma *vma;
-	/* The runtime stuff gets created only when GuC logging gets enabled */
-	struct {
-		void *buf_addr;
-		struct workqueue_struct *flush_wq;
-		struct work_struct flush_work;
-		struct rchan *relay_chan;
-	} runtime;
-	/* logging related stats */
-	u32 capture_miss_count;
-	u32 flush_interrupt_count;
-	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
-	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
-	u32 flush_count[GUC_MAX_LOG_BUFFER];
-};
-
-struct intel_guc {
-	struct intel_uc_fw fw;
-	struct intel_guc_log log;
-	struct intel_guc_ct ct;
-
-	/* Log snapshot if GuC errors during load */
-	struct drm_i915_gem_object *load_err_log;
-
-	/* intel_guc_recv interrupt related state */
-	bool interrupts_enabled;
-
-	struct i915_vma *ads_vma;
-	struct i915_vma *stage_desc_pool;
-	void *stage_desc_pool_vaddr;
-	struct ida stage_ids;
-
-	struct i915_guc_client *execbuf_client;
-
-	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
-	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
-
-	/* GuC's FW specific registers used in MMIO send */
-	struct {
-		u32 base;
-		unsigned int count;
-		enum forcewake_domains fw_domains;
-	} send_regs;
-
-	/* To serialize the intel_guc_send actions */
-	struct mutex send_mutex;
-
-	/* GuC's FW specific send function */
-	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
-
-	/* GuC's FW specific notify function */
-	void (*notify)(struct intel_guc *guc);
-};
-
-struct intel_huc {
-	/* Generic uC firmware management */
-	struct intel_uc_fw fw;
-
-	/* HuC-specific additions */
-};
-
-/* intel_uc.c */
-void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
-void intel_uc_init_early(struct drm_i915_private *dev_priv);
-void intel_uc_init_fw(struct drm_i915_private *dev_priv);
-void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
-int intel_uc_init_hw(struct drm_i915_private *dev_priv);
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
-int intel_guc_sample_forcewake(struct intel_guc *guc);
-int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
-int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
-
-static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
-{
-	return guc->send(guc, action, len);
-}
-
-static inline void intel_guc_notify(struct intel_guc *guc)
-{
-	guc->notify(guc);
-}
-
-/* intel_guc_loader.c */
-int intel_guc_select_fw(struct intel_guc *guc);
-int intel_guc_init_hw(struct intel_guc *guc);
-int intel_guc_suspend(struct drm_i915_private *dev_priv);
-int intel_guc_resume(struct drm_i915_private *dev_priv);
-u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
-
-/* i915_guc_submission.c */
-int i915_guc_submission_init(struct drm_i915_private *dev_priv);
-int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
-int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
-void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
-void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
-struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
-
-/* intel_guc_log.c */
-int intel_guc_log_create(struct intel_guc *guc);
-void intel_guc_log_destroy(struct intel_guc *guc);
-int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
-void i915_guc_log_register(struct drm_i915_private *dev_priv);
-void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
-
-static inline u32 guc_ggtt_offset(struct i915_vma *vma)
-{
-	u32 offset = i915_ggtt_offset(vma);
-	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
-	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
-	return offset;
-}
-
-/* intel_huc.c */
-void intel_huc_select_fw(struct intel_huc *huc);
-void intel_huc_init_hw(struct intel_huc *huc);
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
+#include <intel_uc_common.h>
+#include <intel_guc.h>
+#include <intel_huc.h>
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h
new file mode 100644
index 0000000..f454f73
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_uc_common.h
@@ -0,0 +1,101 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+#ifndef _INTEL_UC_COMMON_H_
+#define _INTEL_UC_COMMON_H_
+
+enum intel_uc_fw_status {
+	INTEL_UC_FIRMWARE_FAIL = -1,
+	INTEL_UC_FIRMWARE_NONE = 0,
+	INTEL_UC_FIRMWARE_PENDING,
+	INTEL_UC_FIRMWARE_SUCCESS
+};
+
+/* User-friendly representation of an enum */
+static inline
+const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
+{
+	switch (status) {
+	case INTEL_UC_FIRMWARE_FAIL:
+		return "FAIL";
+	case INTEL_UC_FIRMWARE_NONE:
+		return "NONE";
+	case INTEL_UC_FIRMWARE_PENDING:
+		return "PENDING";
+	case INTEL_UC_FIRMWARE_SUCCESS:
+		return "SUCCESS";
+	}
+	return "<invalid>";
+}
+
+enum intel_uc_fw_type {
+	INTEL_UC_FW_TYPE_GUC,
+	INTEL_UC_FW_TYPE_HUC
+};
+
+/* User-friendly representation of an enum */
+static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
+{
+	switch (type) {
+	case INTEL_UC_FW_TYPE_GUC:
+		return "GuC";
+	case INTEL_UC_FW_TYPE_HUC:
+		return "HuC";
+	}
+	return "uC";
+}
+
+/*
+ * This structure encapsulates all the data needed during the process
+ * of fetching, caching, and loading the firmware image into the GuC.
+ */
+struct intel_uc_fw {
+	const char *path;
+	size_t size;
+	struct drm_i915_gem_object *obj;
+	enum intel_uc_fw_status fetch_status;
+	enum intel_uc_fw_status load_status;
+
+	uint16_t major_ver_wanted;
+	uint16_t minor_ver_wanted;
+	uint16_t major_ver_found;
+	uint16_t minor_ver_found;
+
+	enum intel_uc_fw_type type;
+	uint32_t header_size;
+	uint32_t header_offset;
+	uint32_t rsa_size;
+	uint32_t rsa_offset;
+	uint32_t ucode_size;
+	uint32_t ucode_offset;
+};
+
+/* intel_uc.c */
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
+void intel_uc_init_early(struct drm_i915_private *dev_priv);
+void intel_uc_init_fw(struct drm_i915_private *dev_priv);
+void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
+int intel_uc_init_hw(struct drm_i915_private *dev_priv);
+void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+
+#endif
-- 
1.9.1

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

[PATCH 2/4] drm/i915/guc: Fix GuC interaction in reset/suspend scenarios

[Sagar Arun Kamble]

Tearing down of guc_ggtt_invalidate/guc_interrupts/guc_communication
setup should happen towards end of reset/suspend as these are
setup back again during recovery/resume.

Prepared helpers intel_guc_pause and intel_guc_unpause that will do
teardown/bringup of this setup along with suspension/resumption of GuC if
loaded. Moved intel_guc_suspend, intel_guc_resume to intel_guc.c.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |   6 +-
 drivers/gpu/drm/i915/i915_gem.c            |   6 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  52 ----------
 drivers/gpu/drm/i915/intel_guc.c           | 152 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |   9 +-
 drivers/gpu/drm/i915/intel_uc.c            |  29 +-----
 6 files changed, 169 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2ae730c..b2e8f95 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1690,8 +1690,6 @@ static int i915_drm_resume(struct drm_device *dev)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_resume(dev_priv);
-
 	intel_modeset_init_hw(dev);
 
 	spin_lock_irq(&dev_priv->irq_lock);
@@ -2486,7 +2484,7 @@ static int intel_runtime_suspend(struct device *kdev)
 	 */
 	i915_gem_runtime_suspend(dev_priv);
 
-	intel_guc_suspend(dev_priv);
+	intel_guc_runtime_suspend(&dev_priv->guc);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
@@ -2571,7 +2569,7 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(dev_priv))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
-	intel_guc_resume(dev_priv);
+	intel_guc_runtime_resume(&dev_priv->guc);
 
 	if (IS_GEN9_LP(dev_priv)) {
 		bxt_disable_dc9(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e4cc08b..977500f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2846,6 +2846,8 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
 
 	i915_gem_revoke_fences(dev_priv);
 
+	intel_guc_reset_prepare(&dev_priv->guc);
+
 	return err;
 }
 
@@ -4574,8 +4576,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	i915_gem_contexts_lost(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_suspend(dev_priv);
-
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
 
@@ -4592,6 +4592,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
 		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
 
+	intel_guc_system_suspend(&dev_priv->guc);
+
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
 	 * expects the system to be in execlists mode on startup,
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 602ae8a..2f977ab 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1287,55 +1287,3 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	guc_client_free(guc->execbuf_client);
 	guc->execbuf_client = NULL;
 }
-
-/**
- * intel_guc_suspend() - notify GuC entering suspend state
- * @dev_priv:	i915 device private
- */
-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)
-		return 0;
-
-	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;
-	/* first page is shared data with GuC */
-	data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
-
-	return intel_guc_send(guc, data, ARRAY_SIZE(data));
-}
-
-/**
- * intel_guc_resume() - notify GuC resuming from suspend state
- * @dev_priv:	i915 device private
- */
-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)
-		return 0;
-
-	if (i915.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;
-	/* first page is shared data with GuC */
-	data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
-
-	return intel_guc_send(guc, data, ARRAY_SIZE(data));
-}
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 978a0e3..1fd8599 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -191,3 +191,155 @@ void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw)
 out:
 	i915_vma_unpin(vma);
 }
+
+int intel_guc_enable_communication(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	if (HAS_GUC_CT(dev_priv))
+		return intel_guc_enable_ct(guc);
+
+	guc->send = intel_guc_send_mmio;
+	return 0;
+}
+
+void intel_guc_disable_communication(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	if (HAS_GUC_CT(dev_priv))
+		intel_guc_disable_ct(guc);
+
+	guc->send = intel_guc_send_nop;
+}
+
+/**
+ * intel_guc_suspend() - notify GuC entering suspend state
+ */
+static int intel_guc_suspend(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_gem_context *ctx;
+	u32 data[3];
+
+	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return 0;
+
+	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;
+	/* first page is shared data with GuC */
+	data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
+
+	return intel_guc_send(guc, data, ARRAY_SIZE(data));
+}
+
+/**
+ * intel_guc_resume() - notify GuC resuming from suspend state
+ */
+static int intel_guc_resume(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_gem_context *ctx;
+	u32 data[3];
+
+	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return 0;
+
+	ctx = dev_priv->kernel_context;
+
+	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);
+
+	return intel_guc_send(guc, data, ARRAY_SIZE(data));
+}
+
+static void intel_guc_sanitize(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_disable_communication(guc);
+	gen9_disable_guc_interrupts(dev_priv);
+}
+
+void intel_guc_reset_prepare(struct intel_guc *guc)
+{
+	if (!i915.enable_guc_loading)
+		return;
+
+	intel_guc_sanitize(guc);
+	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
+}
+
+static int intel_guc_pause(struct intel_guc *guc)
+{
+	int ret = 0;
+
+	ret = intel_guc_suspend(guc);
+	intel_guc_sanitize(guc);
+
+	return ret;
+}
+
+static int intel_guc_unpause(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	int ret = 0;
+
+	if (i915.guc_log_level >= 0)
+		gen9_enable_guc_interrupts(dev_priv);
+	intel_guc_enable_communication(guc);
+	i915_ggtt_enable_guc(dev_priv);
+	ret = intel_guc_resume(guc);
+
+	return ret;
+}
+
+int intel_guc_runtime_suspend(struct intel_guc *guc)
+{
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	return intel_guc_pause(guc);
+}
+
+int intel_guc_runtime_resume(struct intel_guc *guc)
+{
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	return intel_guc_unpause(guc);
+}
+
+int intel_guc_system_suspend(struct intel_guc *guc)
+{
+	int ret = 0;
+
+	if (!i915.enable_guc_loading)
+		return ret;
+
+	ret = intel_guc_pause(guc);
+	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
+
+	return ret;
+}
+
+int intel_guc_system_resume(struct intel_guc *guc)
+{
+	int ret = 0;
+
+	if (!i915.enable_guc_loading)
+		return ret;
+
+	/*
+	 * Placeholder for GuC resume from system suspend/freeze states.
+	 * Currently full reinitialization of GEM and GuC happens along
+	 * these paths, Hence this function is doing nothing.
+	 */
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b329830..bf4dda0 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -162,6 +162,13 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
 void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw);
+int intel_guc_enable_communication(struct intel_guc *guc);
+void intel_guc_disable_communication(struct intel_guc *guc);
+void intel_guc_reset_prepare(struct intel_guc *guc);
+int intel_guc_runtime_suspend(struct intel_guc *guc);
+int intel_guc_runtime_resume(struct intel_guc *guc);
+int intel_guc_system_suspend(struct intel_guc *guc);
+int intel_guc_system_resume(struct intel_guc *guc);
 
 static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
 				 u32 len)
@@ -177,8 +184,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 /* intel_guc_loader.c */
 int intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
-int intel_guc_suspend(struct drm_i915_private *dev_priv);
-int intel_guc_resume(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index a3fc4c8..30c004c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -265,27 +265,6 @@ static void guc_free_load_err_log(struct intel_guc *guc)
 		i915_gem_object_put(guc->load_err_log);
 }
 
-static int guc_enable_communication(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	if (HAS_GUC_CT(dev_priv))
-		return intel_guc_enable_ct(guc);
-
-	guc->send = intel_guc_send_mmio;
-	return 0;
-}
-
-static void guc_disable_communication(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	if (HAS_GUC_CT(dev_priv))
-		intel_guc_disable_ct(guc);
-
-	guc->send = intel_guc_send_nop;
-}
-
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -295,7 +274,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (!i915.enable_guc_loading)
 		return 0;
 
-	guc_disable_communication(guc);
+	intel_guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
 
 	/* We need to notify the guc whenever we change the GGTT */
@@ -347,7 +326,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	intel_guc_init_send_regs(guc);
 
-	ret = guc_enable_communication(guc);
+	ret = intel_guc_enable_communication(guc);
 	if (ret)
 		goto err_log_capture;
 
@@ -373,7 +352,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * marks the GPU as wedged until reset).
 	 */
 err_interrupts:
-	guc_disable_communication(guc);
+	intel_guc_disable_communication(guc);
 	gen9_disable_guc_interrupts(dev_priv);
 err_log_capture:
 	guc_capture_load_err_log(guc);
@@ -410,7 +389,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	if (i915.enable_guc_submission)
 		i915_guc_submission_disable(dev_priv);
 
-	guc_disable_communication(&dev_priv->guc);
+	intel_guc_disable_communication(&dev_priv->guc);
 
 	if (i915.enable_guc_submission) {
 		gen9_disable_guc_interrupts(dev_priv);
-- 
1.9.1

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

[PATCH 3/4] drm/i915/guc: Fix GuC HW/SW state cleanup in unload path

[Sagar Arun Kamble]

Teardown of GuC HW/SW state was not properly done in unload path.
During unload, we can rely on intel_guc_reset_prepare being done
as part of i915_gem_suspend for disabling GuC interfaces.
We will have to disable GuC submission prior to suspend as that involves
communication with GuC to destroy doorbell. So intel_uc_fini_hw has to
be called as part of i915_gem_suspend during unload as that really
takes care of finishing the GuC operations. Created new parameter for
i915_gem_suspend to handle unload/suspend path w.r.t gem and GuC suspend.
GuC related allocations are cleaned up as part of intel_uc_cleanup_hw.

v2: Prepared i915_gem_unload. (Michal)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c        |  6 +--
 drivers/gpu/drm/i915/i915_drv.h        |  1 +
 drivers/gpu/drm/i915/i915_gem.c        | 79 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.c       | 13 ++++++
 drivers/gpu/drm/i915/intel_guc.h       |  1 +
 drivers/gpu/drm/i915/intel_uc.c        | 14 +++---
 drivers/gpu/drm/i915/intel_uc_common.h |  1 +
 7 files changed, 103 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b2e8f95..b6cc2fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -601,7 +601,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
 	i915_gem_drain_workqueue(dev_priv);
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	intel_uc_fini_hw(dev_priv);
+	intel_uc_cleanup_hw(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_contexts_fini(dev_priv);
 	i915_gem_cleanup_userptr(dev_priv);
@@ -682,7 +682,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	return 0;
 
 cleanup_gem:
-	if (i915_gem_suspend(dev_priv))
+	if (i915_gem_unload(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 	i915_gem_fini(dev_priv);
 cleanup_uc:
@@ -1375,7 +1375,7 @@ void i915_driver_unload(struct drm_device *dev)
 
 	i915_driver_unregister(dev_priv);
 
-	if (i915_gem_suspend(dev_priv))
+	if (i915_gem_unload(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 064bf0f..570e71e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3628,6 +3628,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
 			   unsigned int flags);
 int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
+int __must_check i915_gem_unload(struct drm_i915_private *dev_priv);
 void i915_gem_resume(struct drm_i915_private *dev_priv);
 int i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 977500f..24cefe9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4624,6 +4624,85 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+int i915_gem_unload(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = &dev_priv->drm;
+	int ret;
+
+	intel_runtime_pm_get(dev_priv);
+	intel_suspend_gt_powersave(dev_priv);
+
+	mutex_lock(&dev->struct_mutex);
+
+	/* We have to flush all the executing contexts to main memory so
+	 * that they can saved in the hibernation image. To ensure the last
+	 * context image is coherent, we have to switch away from it. That
+	 * leaves the dev_priv->kernel_context still active when
+	 * we actually suspend, and its image in memory may not match the GPU
+	 * state. Fortunately, the kernel_context is disposable and we do
+	 * not rely on its state.
+	 */
+	ret = i915_gem_switch_to_kernel_context(dev_priv);
+	if (ret)
+		goto err_unlock;
+
+	ret = i915_gem_wait_for_idle(dev_priv,
+				     I915_WAIT_INTERRUPTIBLE |
+				     I915_WAIT_LOCKED);
+	if (ret)
+		goto err_unlock;
+
+	assert_kernel_context_is_current(dev_priv);
+	i915_gem_contexts_lost(dev_priv);
+	mutex_unlock(&dev->struct_mutex);
+
+	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
+	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
+
+	/* As the idle_work is rearming if it detects a race, play safe and
+	 * repeat the flush until it is definitely idle.
+	 */
+	while (flush_delayed_work(&dev_priv->gt.idle_work))
+		;
+
+	/* Assert that we successfully flushed all the work and
+	 * reset the GPU back to its idle, low power state.
+	 */
+	WARN_ON(dev_priv->gt.awake);
+	WARN_ON(!intel_engines_are_idle(dev_priv));
+
+	/* Handle GuC suspension in case of unloading/system suspend */
+	intel_uc_fini_hw(dev_priv);
+
+	/*
+	 * Neither the BIOS, ourselves or any other kernel
+	 * expects the system to be in execlists mode on startup,
+	 * so we need to reset the GPU back to legacy mode. And the only
+	 * known way to disable logical contexts is through a GPU reset.
+	 *
+	 * So in order to leave the system in a known default configuration,
+	 * always reset the GPU upon unload and suspend. Afterwards we then
+	 * clean up the GEM state tracking, flushing off the requests and
+	 * leaving the system in a known idle state.
+	 *
+	 * Note that is of the upmost importance that the GPU is idle and
+	 * all stray writes are flushed *before* we dismantle the backing
+	 * storage for the pinned objects.
+	 *
+	 * However, since we are uncertain that resetting the GPU on older
+	 * machines is a good idea, we don't - just in case it leaves the
+	 * machine in an unusable condition.
+	 */
+	i915_gem_sanitize(dev_priv);
+	goto out_rpm_put;
+
+err_unlock:
+	mutex_unlock(&dev->struct_mutex);
+out_rpm_put:
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
 void i915_gem_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 1fd8599..da81c37 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -343,3 +343,16 @@ int intel_guc_system_resume(struct intel_guc *guc)
 	 */
 	return ret;
 }
+
+void intel_guc_fini(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_device *dev = &dev_priv->drm;
+
+	if (i915.enable_guc_submission) {
+		mutex_lock(&dev->struct_mutex);
+		i915_guc_submission_disable(dev_priv);
+		mutex_unlock(&dev->struct_mutex);
+		intel_guc_reset_prepare(guc);
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index bf4dda0..1b3305f 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -169,6 +169,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 int intel_guc_runtime_resume(struct intel_guc *guc);
 int intel_guc_system_suspend(struct intel_guc *guc);
 int intel_guc_system_resume(struct intel_guc *guc);
+void intel_guc_fini(struct intel_guc *guc);
 
 static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
 				 u32 len)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 30c004c..9216c73 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -381,20 +381,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
+	intel_guc_fini(&dev_priv->guc);
+}
+
+void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv)
+{
 	guc_free_load_err_log(&dev_priv->guc);
 
 	if (!i915.enable_guc_loading)
 		return;
 
 	if (i915.enable_guc_submission)
-		i915_guc_submission_disable(dev_priv);
-
-	intel_guc_disable_communication(&dev_priv->guc);
-
-	if (i915.enable_guc_submission) {
-		gen9_disable_guc_interrupts(dev_priv);
 		i915_guc_submission_fini(dev_priv);
-	}
-
-	i915_ggtt_disable_guc(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h
index f454f73..c6e873d 100644
--- a/drivers/gpu/drm/i915/intel_uc_common.h
+++ b/drivers/gpu/drm/i915/intel_uc_common.h
@@ -97,5 +97,6 @@ struct intel_uc_fw {
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv);
 
 #endif
-- 
1.9.1

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

[PATCH 4/4] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Sagar Arun Kamble]

With GuC v9, new type of Default/critical logging in GuC to enable
capturing minimal important logs in production systems efficiently.
This patch enables this logging in GuC by default always. It should
be noted that streaming support with half-full interrupt mechanism
that is present for normal logging is not present for this type of
logging.

v2: Emulating GuC critical logging through i915.guc_log_level. Setting
this to 0 will make GuC critical logging ON and setting it to 1-4 will
communicate log level of 0-3 to GuC.

v3: Commit message update. Enable default/critical logging in GuC always.
Fixed RPM wake during guc_log_unregister in the unload path.

Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
Cc: Fry Gregory P <gregory.p.fry@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Spotswood John A <john.a.spotswood@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h   | 14 +++++++++++++-
 drivers/gpu/drm/i915/intel_guc_loader.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_guc_log.c    |  7 ++++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 5fa2860..353b081 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -132,6 +132,7 @@
 #define   GUC_WQ_TRACK_ENABLED		(1 << 8)
 #define   GUC_ADS_ENABLED		(1 << 9)
 #define   GUC_DEBUG_RESERVED		(1 << 10)
+#define   GUC_V9_CRITICAL_LOGGING_DISABLED	(1 << 10)
 #define   GUC_ADS_ADDR_SHIFT		11
 #define   GUC_ADS_ADDR_MASK		0xfffff800
 
@@ -139,6 +140,16 @@
 
 #define GUC_CTL_MAX_DWORDS		(SOFT_SCRATCH_COUNT - 2) /* [1..14] */
 
+/*
+ * Critical logging in GuC is to be enabled always from GuC v9+.
+ * (for KBL - v9.39+)
+ */
+#define NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw)	\
+	(((IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv)) && \
+				    guc_fw->major_ver_found >= 9) || \
+	  (IS_KABYLAKE(dev_priv) && guc_fw->major_ver_found >= 9 && \
+				    guc_fw->minor_ver_found >= 39))
+
 /**
  * DOC: GuC Firmware Layout
  *
@@ -539,7 +550,8 @@ struct guc_log_buffer_state {
 		u32 logging_enabled:1;
 		u32 reserved1:3;
 		u32 verbosity:4;
-		u32 reserved2:24;
+		u32 critical_logging_enabled:1;
+		u32 reserved2:23;
 	};
 	u32 value;
 } __packed;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 81e03a6..d2b027c 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -106,6 +106,7 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
 static void guc_params_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	u32 params[GUC_CTL_MAX_DWORDS];
 	int i;
 
@@ -136,6 +137,15 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
 	} else
 		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 
+	/*
+	 * GuC has critical/default logging level which is to be enabled
+	 * always from GuC v9 onwards.
+	 */
+	if (NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw)) {
+		params[GUC_CTL_DEBUG] &=
+			~GUC_V9_CRITICAL_LOGGING_DISABLED;
+	}
+
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (i915.enable_guc_submission) {
 		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 16d3b87..6877e34 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -589,7 +589,7 @@ void intel_guc_log_destroy(struct intel_guc *guc)
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 {
 	struct intel_guc *guc = &dev_priv->guc;
-
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	union guc_log_control log_param;
 	int ret;
 
@@ -603,6 +603,9 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
 		return 0;
 
+	if (NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw))
+		log_param.critical_logging_enabled = 1;
+
 	ret = guc_log_control(guc, log_param.value);
 	if (ret < 0) {
 		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
@@ -655,8 +658,10 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
+	intel_runtime_pm_get(dev_priv);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	gen9_disable_guc_interrupts(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 	guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
-- 
1.9.1

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

✓ Fi.CI.BAT: success for GuC Fixes and change for v9+ Firmwares

[Patchwork]

== Series Details ==

Series: GuC Fixes and change for v9+ Firmwares
URL   : https://patchwork.freedesktop.org/series/29651/
State : success

== Summary ==

Series 29651v1 GuC Fixes and change for v9+ Firmwares
https://patchwork.freedesktop.org/api/1.0/series/29651/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-snb-2600) fdo#100215 +1
        Subgroup basic-flip-after-cursor-varying-size:
                fail       -> PASS       (fi-hsw-4770) fdo#102402 +1

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102402 https://bugs.freedesktop.org/show_bug.cgi?id=102402

fi-bdw-5557u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-bdw-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:444s
fi-blb-e6850     total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  time:360s
fi-bsw-n3050     total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  time:557s
fi-bwr-2160      total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 time:253s
fi-bxt-j4205     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:526s
fi-byt-j1900     total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  time:522s
fi-byt-n2820     total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  time:514s
fi-elk-e7500     total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  time:431s
fi-glk-2a        total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:614s
fi-hsw-4770      total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:458s
fi-hsw-4770r     total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:425s
fi-ilk-650       total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:423s
fi-ivb-3520m     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:505s
fi-ivb-3770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:474s
fi-kbl-7500u     total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:515s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:599s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:598s
fi-pnv-d510      total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:525s
fi-skl-6260u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:473s
fi-skl-6700k     total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:532s
fi-skl-6770hq    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:501s
fi-skl-gvtdvm    total:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  time:441s
fi-skl-x1585l    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:486s
fi-snb-2520m     total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  time:549s
fi-snb-2600      total:288  pass:249  dwarn:0   dfail:0   fail:1   skip:38  time:404s

ccf4ca2d93383fe1a234aba83df9c21400216433 drm-tip: 2017y-08m-31d-18h-37m-46s UTC integration manifest
4599587b12e5 drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9
3333d4e54028 drm/i915/guc: Fix GuC HW/SW state cleanup in unload path
9b7ae898972f drm/i915/guc: Fix GuC interaction in reset/suspend scenarios
31f4b546abf3 drm/i915: Separate GuC/HuC specific functionality from intel_uc

== Logs ==

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

✗ Fi.CI.IGT: failure for GuC Fixes and change for v9+ Firmwares

[Patchwork]

== Series Details ==

Series: GuC Fixes and change for v9+ Firmwares
URL   : https://patchwork.freedesktop.org/series/29651/
State : failure

== Summary ==

Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252 +1
        Subgroup oa-formats:
                pass       -> FAIL       (shard-hsw)
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2265 pass:1234 dwarn:0   dfail:0   fail:15  skip:1016 time:9638s

== Logs ==

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

Re: [PATCH 3/4] drm/i915/guc: Fix GuC HW/SW state cleanup in unload path

[Michał Winiarski]

On Fri, Sep 01, 2017 at 11:02:11AM +0530, Sagar Arun Kamble wrote:
> Teardown of GuC HW/SW state was not properly done in unload path.
> During unload, we can rely on intel_guc_reset_prepare being done
> as part of i915_gem_suspend for disabling GuC interfaces.
> We will have to disable GuC submission prior to suspend as that involves
> communication with GuC to destroy doorbell. So intel_uc_fini_hw has to
> be called as part of i915_gem_suspend during unload as that really
> takes care of finishing the GuC operations. Created new parameter for
> i915_gem_suspend to handle unload/suspend path w.r.t gem and GuC suspend.
> GuC related allocations are cleaned up as part of intel_uc_cleanup_hw.

Is this still accurate description? After changes from v2?

> 
> v2: Prepared i915_gem_unload. (Michal)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c        |  6 +--
>  drivers/gpu/drm/i915/i915_drv.h        |  1 +
>  drivers/gpu/drm/i915/i915_gem.c        | 79 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.c       | 13 ++++++
>  drivers/gpu/drm/i915/intel_guc.h       |  1 +
>  drivers/gpu/drm/i915/intel_uc.c        | 14 +++---
>  drivers/gpu/drm/i915/intel_uc_common.h |  1 +
>  7 files changed, 103 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b2e8f95..b6cc2fe 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -601,7 +601,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>  	i915_gem_drain_workqueue(dev_priv);
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> -	intel_uc_fini_hw(dev_priv);
> +	intel_uc_cleanup_hw(dev_priv);
>  	i915_gem_cleanup_engines(dev_priv);
>  	i915_gem_contexts_fini(dev_priv);
>  	i915_gem_cleanup_userptr(dev_priv);
> @@ -682,7 +682,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	return 0;
>  
>  cleanup_gem:
> -	if (i915_gem_suspend(dev_priv))
> +	if (i915_gem_unload(dev_priv))
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  	i915_gem_fini(dev_priv);
>  cleanup_uc:
> @@ -1375,7 +1375,7 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	i915_driver_unregister(dev_priv);
>  
> -	if (i915_gem_suspend(dev_priv))
> +	if (i915_gem_unload(dev_priv))
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 064bf0f..570e71e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3628,6 +3628,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>  			   unsigned int flags);
>  int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
> +int __must_check i915_gem_unload(struct drm_i915_private *dev_priv);
>  void i915_gem_resume(struct drm_i915_private *dev_priv);
>  int i915_gem_fault(struct vm_fault *vmf);
>  int i915_gem_object_wait(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 977500f..24cefe9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4624,6 +4624,85 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> +int i915_gem_unload(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	int ret;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	intel_suspend_gt_powersave(dev_priv);
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	/* We have to flush all the executing contexts to main memory so
> +	 * that they can saved in the hibernation image. To ensure the last
> +	 * context image is coherent, we have to switch away from it. That
> +	 * leaves the dev_priv->kernel_context still active when
> +	 * we actually suspend, and its image in memory may not match the GPU
> +	 * state. Fortunately, the kernel_context is disposable and we do
> +	 * not rely on its state.
> +	 */
> +	ret = i915_gem_switch_to_kernel_context(dev_priv);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = i915_gem_wait_for_idle(dev_priv,
> +				     I915_WAIT_INTERRUPTIBLE |
> +				     I915_WAIT_LOCKED);
> +	if (ret)
> +		goto err_unlock;
> +
> +	assert_kernel_context_is_current(dev_priv);
> +	i915_gem_contexts_lost(dev_priv);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> +	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
> +
> +	/* As the idle_work is rearming if it detects a race, play safe and
> +	 * repeat the flush until it is definitely idle.
> +	 */
> +	while (flush_delayed_work(&dev_priv->gt.idle_work))
> +		;
> +
> +	/* Assert that we successfully flushed all the work and
> +	 * reset the GPU back to its idle, low power state.
> +	 */
> +	WARN_ON(dev_priv->gt.awake);
> +	WARN_ON(!intel_engines_are_idle(dev_priv));
> +
> +	/* Handle GuC suspension in case of unloading/system suspend */
> +	intel_uc_fini_hw(dev_priv);
> +
> +	/*
> +	 * Neither the BIOS, ourselves or any other kernel
> +	 * expects the system to be in execlists mode on startup,
> +	 * so we need to reset the GPU back to legacy mode. And the only
> +	 * known way to disable logical contexts is through a GPU reset.
> +	 *
> +	 * So in order to leave the system in a known default configuration,
> +	 * always reset the GPU upon unload and suspend. Afterwards we then
> +	 * clean up the GEM state tracking, flushing off the requests and
> +	 * leaving the system in a known idle state.
> +	 *
> +	 * Note that is of the upmost importance that the GPU is idle and
> +	 * all stray writes are flushed *before* we dismantle the backing
> +	 * storage for the pinned objects.
> +	 *
> +	 * However, since we are uncertain that resetting the GPU on older
> +	 * machines is a good idea, we don't - just in case it leaves the
> +	 * machine in an unusable condition.
> +	 */
> +	i915_gem_sanitize(dev_priv);
> +	goto out_rpm_put;
> +
> +err_unlock:
> +	mutex_unlock(&dev->struct_mutex);
> +out_rpm_put:
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
> +}
> +

^^^ This is just copypasta of i915_gem_suspend
One line has changed... (well, I think it's no longer up-to-date with drm-tip,
so there's more, but s/intel_guc_system_suspend/intel_uc_fini_hw was the
intention, right?)
We can do better.

>  void i915_gem_resume(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 1fd8599..da81c37 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -343,3 +343,16 @@ int intel_guc_system_resume(struct intel_guc *guc)
>  	 */
>  	return ret;
>  }
> +
> +void intel_guc_fini(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct drm_device *dev = &dev_priv->drm;
> +
> +	if (i915.enable_guc_submission) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_guc_submission_disable(dev_priv);
> +		mutex_unlock(&dev->struct_mutex);
> +		intel_guc_reset_prepare(guc);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index bf4dda0..1b3305f 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -169,6 +169,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  int intel_guc_runtime_resume(struct intel_guc *guc);
>  int intel_guc_system_suspend(struct intel_guc *guc);
>  int intel_guc_system_resume(struct intel_guc *guc);
> +void intel_guc_fini(struct intel_guc *guc);
>  
>  static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
>  				 u32 len)
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 30c004c..9216c73 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -381,20 +381,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  {
> +	intel_guc_fini(&dev_priv->guc);
> +}
> +
> +void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv)
> +{
>  	guc_free_load_err_log(&dev_priv->guc);

Not really part of the patch... But still - why are we touching this here?
Isn't guc->log->vma is created only if GuC is loaded?
>  
>  	if (!i915.enable_guc_loading)
>  		return;
>  
>  	if (i915.enable_guc_submission)
> -		i915_guc_submission_disable(dev_priv);
> -
> -	intel_guc_disable_communication(&dev_priv->guc);
> -
> -	if (i915.enable_guc_submission) {
> -		gen9_disable_guc_interrupts(dev_priv);
>  		i915_guc_submission_fini(dev_priv);
> -	}
> -
> -	i915_ggtt_disable_guc(dev_priv);
>  }

We now have intel_uc_fini_hw and intel_uc_cleanup_hw.

intel_uc_fini_hw is called by i915_gem_unload, intel_uc_cleanup_hw is called by
i915_gem_fini - so, it's in that order, we're first calling fini, then calling
cleanup.
Both are only used on unload path, while intel_uc_init_hw, is called on load,
resume, and reset (by i915_gem_init_hw)
We should probably redo the naming at this point... Perhaps stick with the
unload for rather than fini_hw? Or at least comment on the ordering.

-Michał

> diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h
> index f454f73..c6e873d 100644
> --- a/drivers/gpu/drm/i915/intel_uc_common.h
> +++ b/drivers/gpu/drm/i915/intel_uc_common.h
> @@ -97,5 +97,6 @@ struct intel_uc_fw {
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> +void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv);
>  
>  #endif
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/guc: Fix GuC HW/SW state cleanup in unload path

[Kamble, Sagar A]

On 9/5/2017 8:24 PM, Michał Winiarski wrote:
> On Fri, Sep 01, 2017 at 11:02:11AM +0530, Sagar Arun Kamble wrote:
>> Teardown of GuC HW/SW state was not properly done in unload path.
>> During unload, we can rely on intel_guc_reset_prepare being done
>> as part of i915_gem_suspend for disabling GuC interfaces.
>> We will have to disable GuC submission prior to suspend as that involves
>> communication with GuC to destroy doorbell. So intel_uc_fini_hw has to
>> be called as part of i915_gem_suspend during unload as that really
>> takes care of finishing the GuC operations. Created new parameter for
>> i915_gem_suspend to handle unload/suspend path w.r.t gem and GuC suspend.
>> GuC related allocations are cleaned up as part of intel_uc_cleanup_hw.
> Is this still accurate description? After changes from v2?
No. Sorry. Will update this in the new version.
>
>> v2: Prepared i915_gem_unload. (Michal)
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c        |  6 +--
>>   drivers/gpu/drm/i915/i915_drv.h        |  1 +
>>   drivers/gpu/drm/i915/i915_gem.c        | 79 ++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc.c       | 13 ++++++
>>   drivers/gpu/drm/i915/intel_guc.h       |  1 +
>>   drivers/gpu/drm/i915/intel_uc.c        | 14 +++---
>>   drivers/gpu/drm/i915/intel_uc_common.h |  1 +
>>   7 files changed, 103 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index b2e8f95..b6cc2fe 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -601,7 +601,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>>   	i915_gem_drain_workqueue(dev_priv);
>>   
>>   	mutex_lock(&dev_priv->drm.struct_mutex);
>> -	intel_uc_fini_hw(dev_priv);
>> +	intel_uc_cleanup_hw(dev_priv);
>>   	i915_gem_cleanup_engines(dev_priv);
>>   	i915_gem_contexts_fini(dev_priv);
>>   	i915_gem_cleanup_userptr(dev_priv);
>> @@ -682,7 +682,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>>   	return 0;
>>   
>>   cleanup_gem:
>> -	if (i915_gem_suspend(dev_priv))
>> +	if (i915_gem_unload(dev_priv))
>>   		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>>   	i915_gem_fini(dev_priv);
>>   cleanup_uc:
>> @@ -1375,7 +1375,7 @@ void i915_driver_unload(struct drm_device *dev)
>>   
>>   	i915_driver_unregister(dev_priv);
>>   
>> -	if (i915_gem_suspend(dev_priv))
>> +	if (i915_gem_unload(dev_priv))
>>   		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>>   
>>   	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 064bf0f..570e71e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3628,6 +3628,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>>   int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>>   			   unsigned int flags);
>>   int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
>> +int __must_check i915_gem_unload(struct drm_i915_private *dev_priv);
>>   void i915_gem_resume(struct drm_i915_private *dev_priv);
>>   int i915_gem_fault(struct vm_fault *vmf);
>>   int i915_gem_object_wait(struct drm_i915_gem_object *obj,
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 977500f..24cefe9 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4624,6 +4624,85 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>>   	return ret;
>>   }
>>   
>> +int i915_gem_unload(struct drm_i915_private *dev_priv)
>> +{
>> +	struct drm_device *dev = &dev_priv->drm;
>> +	int ret;
>> +
>> +	intel_runtime_pm_get(dev_priv);
>> +	intel_suspend_gt_powersave(dev_priv);
>> +
>> +	mutex_lock(&dev->struct_mutex);
>> +
>> +	/* We have to flush all the executing contexts to main memory so
>> +	 * that they can saved in the hibernation image. To ensure the last
>> +	 * context image is coherent, we have to switch away from it. That
>> +	 * leaves the dev_priv->kernel_context still active when
>> +	 * we actually suspend, and its image in memory may not match the GPU
>> +	 * state. Fortunately, the kernel_context is disposable and we do
>> +	 * not rely on its state.
>> +	 */
>> +	ret = i915_gem_switch_to_kernel_context(dev_priv);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = i915_gem_wait_for_idle(dev_priv,
>> +				     I915_WAIT_INTERRUPTIBLE |
>> +				     I915_WAIT_LOCKED);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	assert_kernel_context_is_current(dev_priv);
>> +	i915_gem_contexts_lost(dev_priv);
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>> +	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>> +	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
>> +
>> +	/* As the idle_work is rearming if it detects a race, play safe and
>> +	 * repeat the flush until it is definitely idle.
>> +	 */
>> +	while (flush_delayed_work(&dev_priv->gt.idle_work))
>> +		;
>> +
>> +	/* Assert that we successfully flushed all the work and
>> +	 * reset the GPU back to its idle, low power state.
>> +	 */
>> +	WARN_ON(dev_priv->gt.awake);
>> +	WARN_ON(!intel_engines_are_idle(dev_priv));
>> +
>> +	/* Handle GuC suspension in case of unloading/system suspend */
>> +	intel_uc_fini_hw(dev_priv);
>> +
>> +	/*
>> +	 * Neither the BIOS, ourselves or any other kernel
>> +	 * expects the system to be in execlists mode on startup,
>> +	 * so we need to reset the GPU back to legacy mode. And the only
>> +	 * known way to disable logical contexts is through a GPU reset.
>> +	 *
>> +	 * So in order to leave the system in a known default configuration,
>> +	 * always reset the GPU upon unload and suspend. Afterwards we then
>> +	 * clean up the GEM state tracking, flushing off the requests and
>> +	 * leaving the system in a known idle state.
>> +	 *
>> +	 * Note that is of the upmost importance that the GPU is idle and
>> +	 * all stray writes are flushed *before* we dismantle the backing
>> +	 * storage for the pinned objects.
>> +	 *
>> +	 * However, since we are uncertain that resetting the GPU on older
>> +	 * machines is a good idea, we don't - just in case it leaves the
>> +	 * machine in an unusable condition.
>> +	 */
>> +	i915_gem_sanitize(dev_priv);
>> +	goto out_rpm_put;
>> +
>> +err_unlock:
>> +	mutex_unlock(&dev->struct_mutex);
>> +out_rpm_put:
>> +	intel_runtime_pm_put(dev_priv);
>> +	return ret;
>> +}
>> +
> ^^^ This is just copypasta of i915_gem_suspend
> One line has changed... (well, I think it's no longer up-to-date with drm-tip,
> so there's more, but s/intel_guc_system_suspend/intel_uc_fini_hw was the
> intention, right?)
> We can do better.
Yes. This is just a line change. Earlier, I parameterised 
i915_gem_suspend to differentiate suspend vs unload.
Michal suggested if we could separate these paths for gem. I am okay any 
of these paths. Let me know your thoughts.
>
>>   void i915_gem_resume(struct drm_i915_private *dev_priv)
>>   {
>>   	struct drm_device *dev = &dev_priv->drm;
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
>> index 1fd8599..da81c37 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -343,3 +343,16 @@ int intel_guc_system_resume(struct intel_guc *guc)
>>   	 */
>>   	return ret;
>>   }
>> +
>> +void intel_guc_fini(struct intel_guc *guc)
>> +{
>> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +	struct drm_device *dev = &dev_priv->drm;
>> +
>> +	if (i915.enable_guc_submission) {
>> +		mutex_lock(&dev->struct_mutex);
>> +		i915_guc_submission_disable(dev_priv);
>> +		mutex_unlock(&dev->struct_mutex);
>> +		intel_guc_reset_prepare(guc);
>> +	}
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
>> index bf4dda0..1b3305f 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -169,6 +169,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>>   int intel_guc_runtime_resume(struct intel_guc *guc);
>>   int intel_guc_system_suspend(struct intel_guc *guc);
>>   int intel_guc_system_resume(struct intel_guc *guc);
>> +void intel_guc_fini(struct intel_guc *guc);
>>   
>>   static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
>>   				 u32 len)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>> index 30c004c..9216c73 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -381,20 +381,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>   
>>   void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>>   {
>> +	intel_guc_fini(&dev_priv->guc);
>> +}
>> +
>> +void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv)
>> +{
>>   	guc_free_load_err_log(&dev_priv->guc);
> Not really part of the patch... But still - why are we touching this here?
> Isn't guc->log->vma is created only if GuC is loaded?
This is to differentiate the fini and cleanup. guc->log->vma is create 
only if GuC is loaded.
I will move this past guc_loading check below.
>>   
>>   	if (!i915.enable_guc_loading)
>>   		return;
>>   
>>   	if (i915.enable_guc_submission)
>> -		i915_guc_submission_disable(dev_priv);
>> -
>> -	intel_guc_disable_communication(&dev_priv->guc);
>> -
>> -	if (i915.enable_guc_submission) {
>> -		gen9_disable_guc_interrupts(dev_priv);
>>   		i915_guc_submission_fini(dev_priv);
>> -	}
>> -
>> -	i915_ggtt_disable_guc(dev_priv);
>>   }
> We now have intel_uc_fini_hw and intel_uc_cleanup_hw.
>
> intel_uc_fini_hw is called by i915_gem_unload, intel_uc_cleanup_hw is called by
> i915_gem_fini - so, it's in that order, we're first calling fini, then calling
> cleanup.
> Both are only used on unload path, while intel_uc_init_hw, is called on load,
> resume, and reset (by i915_gem_init_hw)
> We should probably redo the naming at this point... Perhaps stick with the
> unload for rather than fini_hw? Or at least comment on the ordering.
Sure. I will make this unload then.
>
> -Michał
>
>> diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h
>> index f454f73..c6e873d 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_common.h
>> +++ b/drivers/gpu/drm/i915/intel_uc_common.h
>> @@ -97,5 +97,6 @@ struct intel_uc_fw {
>>   void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>>   int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>>   void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
>> +void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv);
>>   
>>   #endif
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/4] drm/i915: Separate GuC/HuC specific functionality from intel_uc

[Michał Winiarski]

On Fri, Sep 01, 2017 at 11:02:09AM +0530, Sagar Arun Kamble wrote:
> Removed unnecessary intel_uc.h includes as it is present in i915_drv.h.
> Created intel_guc.c and intel_guc.h for placing GuC specific code.
> Created intel_huc.h to refer to HuC specific functions.
> 
> v2: Prepared intel_uc_common.h. huc_auth code declaration adjusted.
> Moved enable/disable_communication to intel_uc.c (Michal)

In v2 you also renamed things, moved things around (and addressed all of the
other review comments from Michał).

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_drv.c            |   1 -
>  drivers/gpu/drm/i915/i915_guc_submission.c |   1 -
>  drivers/gpu/drm/i915/intel_guc.c           | 193 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h           | 200 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_loader.c    |   1 -
>  drivers/gpu/drm/i915/intel_huc.c           |  50 +-----
>  drivers/gpu/drm/i915/intel_huc.h           |  38 +++++
>  drivers/gpu/drm/i915/intel_uc.c            | 128 +--------------
>  drivers/gpu/drm/i915/intel_uc.h            | 254 +----------------------------
>  drivers/gpu/drm/i915/intel_uc_common.h     | 101 ++++++++++++
>  11 files changed, 545 insertions(+), 423 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_guc.c
>  create mode 100644 drivers/gpu/drm/i915/intel_guc.h
>  create mode 100644 drivers/gpu/drm/i915/intel_huc.h
>  create mode 100644 drivers/gpu/drm/i915/intel_uc_common.h

[SNIP]

> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 22ae52b..c87a2b4 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -24,256 +24,8 @@
>  #ifndef _INTEL_UC_H_
>  #define _INTEL_UC_H_
>  

[SNIP]

> -/* intel_huc.c */
> -void intel_huc_select_fw(struct intel_huc *huc);
> -void intel_huc_init_hw(struct intel_huc *huc);
> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
> +#include <intel_uc_common.h>
> +#include <intel_guc.h>
> +#include <intel_huc.h>

^^^
Will this build? (well... it passed BAT, but it doesn't compile on my box).
drivers/gpu/drm/i915 is not -I, so we should use quote marks, not angle
brackets.

Separate header, why? Can't we merge intel_uc_common.h with intel_uc.h?

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

Re: [PATCH 1/4] drm/i915: Separate GuC/HuC specific functionality from intel_uc

[Kamble, Sagar A]

On 9/7/2017 5:54 PM, Michał Winiarski wrote:
> On Fri, Sep 01, 2017 at 11:02:09AM +0530, Sagar Arun Kamble wrote:
>> Removed unnecessary intel_uc.h includes as it is present in i915_drv.h.
>> Created intel_guc.c and intel_guc.h for placing GuC specific code.
>> Created intel_huc.h to refer to HuC specific functions.
>>
>> v2: Prepared intel_uc_common.h. huc_auth code declaration adjusted.
>> Moved enable/disable_communication to intel_uc.c (Michal)
> In v2 you also renamed things, moved things around (and addressed all of the
> other review comments from Michał).
Yes. Sorry. Will update in the next revision.
>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile              |   1 +
>>   drivers/gpu/drm/i915/i915_drv.c            |   1 -
>>   drivers/gpu/drm/i915/i915_guc_submission.c |   1 -
>>   drivers/gpu/drm/i915/intel_guc.c           | 193 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc.h           | 200 +++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc_loader.c    |   1 -
>>   drivers/gpu/drm/i915/intel_huc.c           |  50 +-----
>>   drivers/gpu/drm/i915/intel_huc.h           |  38 +++++
>>   drivers/gpu/drm/i915/intel_uc.c            | 128 +--------------
>>   drivers/gpu/drm/i915/intel_uc.h            | 254 +----------------------------
>>   drivers/gpu/drm/i915/intel_uc_common.h     | 101 ++++++++++++
>>   11 files changed, 545 insertions(+), 423 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_guc.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_guc.h
>>   create mode 100644 drivers/gpu/drm/i915/intel_huc.h
>>   create mode 100644 drivers/gpu/drm/i915/intel_uc_common.h
> [SNIP]
>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>> index 22ae52b..c87a2b4 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -24,256 +24,8 @@
>>   #ifndef _INTEL_UC_H_
>>   #define _INTEL_UC_H_
>>   
> [SNIP]
>
>> -/* intel_huc.c */
>> -void intel_huc_select_fw(struct intel_huc *huc);
>> -void intel_huc_init_hw(struct intel_huc *huc);
>> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
>> +#include <intel_uc_common.h>
>> +#include <intel_guc.h>
>> +#include <intel_huc.h>
> ^^^
> Will this build? (well... it passed BAT, but it doesn't compile on my box).
> drivers/gpu/drm/i915 is not -I, so we should use quote marks, not angle
> brackets.
>
> Separate header, why? Can't we merge intel_uc_common.h with intel_uc.h?
>
> -Michał
Will change these to quote marks. I added this as separate header to not 
declare struct intel_uc_fw in same header as struct intel_guc and struct 
intel_huc.
We can merge but then it will take #include for intel_guc.h and 
intel_huc.h to the end of file.
If we want to keep intel_guc.h and intel_huc.h at the top, they should 
reference pointer to struct intel_uc_fw.
For readability I feel having intel_uc_common.h also seems intuitive.

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

Re: [PATCH 1/4] drm/i915: Separate GuC/HuC specific functionality from intel_uc

[Michal Wajdeczko]

On Fri, Sep 01, 2017 at 11:02:09AM +0530, Sagar Arun Kamble wrote:
> Removed unnecessary intel_uc.h includes as it is present in i915_drv.h.
> Created intel_guc.c and intel_guc.h for placing GuC specific code.
> Created intel_huc.h to refer to HuC specific functions.
> 
> v2: Prepared intel_uc_common.h. huc_auth code declaration adjusted.
> Moved enable/disable_communication to intel_uc.c (Michal)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_drv.c            |   1 -
>  drivers/gpu/drm/i915/i915_guc_submission.c |   1 -
>  drivers/gpu/drm/i915/intel_guc.c           | 193 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h           | 200 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_loader.c    |   1 -
>  drivers/gpu/drm/i915/intel_huc.c           |  50 +-----
>  drivers/gpu/drm/i915/intel_huc.h           |  38 +++++
>  drivers/gpu/drm/i915/intel_uc.c            | 128 +--------------
>  drivers/gpu/drm/i915/intel_uc.h            | 254 +----------------------------
>  drivers/gpu/drm/i915/intel_uc_common.h     | 101 ++++++++++++
>  11 files changed, 545 insertions(+), 423 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_guc.c
>  create mode 100644 drivers/gpu/drm/i915/intel_guc.h
>  create mode 100644 drivers/gpu/drm/i915/intel_huc.h
>  create mode 100644 drivers/gpu/drm/i915/intel_uc_common.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 892f52b..efc5b30 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -59,6 +59,7 @@ i915-y += i915_cmd_parser.o \
>  
>  # general-purpose microcontroller (GuC) support
>  i915-y += intel_uc.o \
> +	  intel_guc.o \
>  	  intel_guc_ct.o \
>  	  intel_guc_log.o \
>  	  intel_guc_loader.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f10a078..2ae730c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -50,7 +50,6 @@
>  #include "i915_trace.h"
>  #include "i915_vgpu.h"
>  #include "intel_drv.h"
> -#include "intel_uc.h"
>  
>  static struct drm_driver driver;
>  
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 48a1e93..602ae8a 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -23,7 +23,6 @@
>   */
>  #include <linux/circ_buf.h>
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  
>  #include <trace/events/dma_fence.h>
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> new file mode 100644
> index 0000000..978a0e3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -0,0 +1,193 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "i915_drv.h"
> +
> +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> +{
> +	WARN(1, "Unexpected send: action=%#x\n", *action);
> +	return -ENODEV;
> +}
> +
> +static void gen8_guc_raise_irq(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> +}
> +
> +void intel_guc_init_early(struct intel_guc *guc)
> +{
> +	intel_guc_ct_init_early(&guc->ct);
> +
> +	mutex_init(&guc->send_mutex);
> +	guc->send = intel_guc_send_nop;
> +	guc->notify = gen8_guc_raise_irq;
> +}
> +
> +static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> +{
> +	GEM_BUG_ON(!guc->send_regs.base);
> +	GEM_BUG_ON(!guc->send_regs.count);
> +	GEM_BUG_ON(i >= guc->send_regs.count);
> +
> +	return _MMIO(guc->send_regs.base + 4 * i);
> +}
> +
> +void intel_guc_init_send_regs(struct intel_guc *guc)

Do we need to expose it outside this file?

> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	enum forcewake_domains fw_domains = 0;
> +	unsigned int i;
> +
> +	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> +	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> +
> +	for (i = 0; i < guc->send_regs.count; i++) {
> +		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> +					guc_send_reg(guc, i),
> +					FW_REG_READ | FW_REG_WRITE);
> +	}
> +	guc->send_regs.fw_domains = fw_domains;
> +}
> +
> +/*
> + * This function implements the MMIO based host to GuC interface.
> + */
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 status;
> +	int i;
> +	int ret;
> +
> +	GEM_BUG_ON(!len);
> +	GEM_BUG_ON(len > guc->send_regs.count);
> +
> +	/* If CT is available, we expect to use MMIO only during init/fini */
> +	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
> +		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> +		*action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
> +
> +	mutex_lock(&guc->send_mutex);
> +	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
> +
> +	for (i = 0; i < len; i++)
> +		I915_WRITE(guc_send_reg(guc, i), action[i]);
> +
> +	POSTING_READ(guc_send_reg(guc, i - 1));
> +
> +	intel_guc_notify(guc);
> +
> +	/*
> +	 * No GuC command should ever take longer than 10ms.
> +	 * Fast commands should still complete in 10us.
> +	 */
> +	ret = __intel_wait_for_register_fw(dev_priv,
> +					   guc_send_reg(guc, 0),
> +					   INTEL_GUC_RECV_MASK,
> +					   INTEL_GUC_RECV_MASK,
> +					   10, 10, &status);
> +	if (status != INTEL_GUC_STATUS_SUCCESS) {
> +		/*
> +		 * Either the GuC explicitly returned an error (which
> +		 * we convert to -EIO here) or no response at all was
> +		 * received within the timeout limit (-ETIMEDOUT)
> +		 */
> +		if (ret != -ETIMEDOUT)
> +			ret = -EIO;
> +
> +		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
> +			 " ret=%d status=0x%08X response=0x%08X\n",
> +			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
> +	}
> +
> +	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> +	mutex_unlock(&guc->send_mutex);
> +
> +	return ret;
> +}
> +
> +int intel_guc_sample_forcewake(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 action[2];
> +
> +	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
> +	/* WaRsDisableCoarsePowerGating:skl,bxt */
> +	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> +		action[1] = 0;
> +	else
> +		/* bit 0 and 1 are for Render and Media domain separately */
> +		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
> +
> +	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> +}
> +
> +/**
> + * intel_guc_auth_huc() - authenticate HuC
> + *
> + * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
> + * authenticate_huc interface.
> + */
> +void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct i915_vma *vma;
> +	int ret;
> +	u32 data[2];
> +
> +	vma = i915_gem_object_ggtt_pin(huc_fw->obj, NULL, 0, 0,
> +				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +	if (IS_ERR(vma)) {
> +		DRM_ERROR("failed to pin huc fw object %d\n",
> +				(int)PTR_ERR(vma));
> +		return;
> +	}
> +
> +	/* Specify auth action and where public signature is. */
> +	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
> +	data[1] = guc_ggtt_offset(vma) + huc_fw->rsa_offset;
> +
> +	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
> +	if (ret) {
> +		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
> +		goto out;
> +	}
> +
> +	/* Check authentication status, it should be done by now */
> +	ret = intel_wait_for_register(dev_priv,
> +				HUC_STATUS2,
> +				HUC_FW_VERIFIED,
> +				HUC_FW_VERIFIED,
> +				50);

This wait should stay in huc.c

> +
> +	if (ret) {
> +		DRM_ERROR("HuC: Authentication failed %d\n", ret);
> +		goto out;
> +	}
> +
> +out:
> +	i915_vma_unpin(vma);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> new file mode 100644
> index 0000000..b329830
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +#ifndef _INTEL_GUC_H_
> +#define _INTEL_GUC_H_
> +
> +#include "intel_guc_fwif.h"
> +#include "i915_guc_reg.h"
> +#include "intel_ringbuffer.h"
> +#include "intel_guc_ct.h"
> +#include "i915_vma.h"
> +
> +struct drm_i915_gem_request;
> +
> +/*
> + * This structure primarily describes the GEM object shared with the GuC.
> + * The specs sometimes refer to this object as a "GuC context", but we use
> + * the term "client" to avoid confusion with hardware contexts. This
> + * GEM object is held for the entire lifetime of our interaction with
> + * the GuC, being allocated before the GuC is loaded with its firmware.
> + * Because there's no way to update the address used by the GuC after
> + * initialisation, the shared object must stay pinned into the GGTT as
> + * long as the GuC is in use. We also keep the first page (only) mapped
> + * into kernel address space, as it includes shared data that must be
> + * updated on every request submission.
> + *
> + * The single GEM object described here is actually made up of several
> + * separate areas, as far as the GuC is concerned. The first page (kept
> + * kmap'd) includes the "process descriptor" which holds sequence data for
> + * the doorbell, and one cacheline which actually *is* the doorbell; a
> + * write to this will "ring the doorbell" (i.e. send an interrupt to the
> + * GuC). The subsequent  pages of the client object constitute the work
> + * queue (a circular array of work items), again described in the process
> + * descriptor. Work queue pages are mapped momentarily as required.
> + *
> + * We also keep a few statistics on failures. Ideally, these should all
> + * be zero!
> + *   no_wq_space: times that the submission pre-check found no space was
> + *                available in the work queue (note, the queue is shared,
> + *                not per-engine). It is OK for this to be nonzero, but
> + *                it should not be huge!
> + *   b_fail: failed to ring the doorbell. This should never happen, unless
> + *           somehow the hardware misbehaves, or maybe if the GuC firmware
> + *           crashes? We probably need to reset the GPU to recover.
> + *   retcode: errno from last guc_submit()
> + */
> +struct i915_guc_client {
> +	struct i915_vma *vma;
> +	void *vaddr;
> +	struct i915_gem_context *owner;
> +	struct intel_guc *guc;
> +
> +	uint32_t engines;		/* bitmap of (host) engine ids	*/
> +	uint32_t priority;
> +	u32 stage_id;
> +	uint32_t proc_desc_offset;
> +
> +	u16 doorbell_id;
> +	unsigned long doorbell_offset;
> +	u32 doorbell_cookie;
> +
> +	spinlock_t wq_lock;
> +	uint32_t wq_offset;
> +	uint32_t wq_size;
> +	uint32_t wq_tail;
> +	uint32_t wq_rsvd;
> +	uint32_t no_wq_space;
> +
> +	/* Per-engine counts of GuC submissions */
> +	uint64_t submissions[I915_NUM_ENGINES];
> +};
> +
> +struct intel_guc_log {
> +	uint32_t flags;
> +	struct i915_vma *vma;
> +	/* The runtime stuff gets created only when GuC logging gets enabled */
> +	struct {
> +		void *buf_addr;
> +		struct workqueue_struct *flush_wq;
> +		struct work_struct flush_work;
> +		struct rchan *relay_chan;
> +	} runtime;
> +	/* logging related stats */
> +	u32 capture_miss_count;
> +	u32 flush_interrupt_count;
> +	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
> +	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
> +	u32 flush_count[GUC_MAX_LOG_BUFFER];
> +};
> +
> +struct intel_guc {
> +	struct intel_uc_fw fw;
> +	struct intel_guc_log log;
> +	struct intel_guc_ct ct;
> +
> +	/* Log snapshot if GuC errors during load */
> +	struct drm_i915_gem_object *load_err_log;
> +
> +	/* intel_guc_recv interrupt related state */
> +	bool interrupts_enabled;
> +
> +	struct i915_vma *ads_vma;
> +	struct i915_vma *stage_desc_pool;
> +	void *stage_desc_pool_vaddr;
> +	struct ida stage_ids;
> +
> +	struct i915_guc_client *execbuf_client;
> +
> +	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> +	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
> +
> +	/* GuC's FW specific registers used in MMIO send */
> +	struct {
> +		u32 base;
> +		unsigned int count;
> +		enum forcewake_domains fw_domains;
> +	} send_regs;
> +
> +	/* To serialize the intel_guc_send actions */
> +	struct mutex send_mutex;
> +
> +	/* GuC's FW specific send function */
> +	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> +
> +	/* GuC's FW specific notify function */
> +	void (*notify)(struct intel_guc *guc);
> +};
> +
> +static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> +{
> +	u32 offset = i915_ggtt_offset(vma);
> +
> +	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> +	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> +	return offset;
> +}
> +
> +/* intel_guc.c */
> +void intel_guc_init_early(struct intel_guc *guc);
> +int intel_guc_sample_forcewake(struct intel_guc *guc);
> +void intel_guc_init_send_regs(struct intel_guc *guc);
> +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
> +void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw);
> +
> +static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
> +				 u32 len)
> +{
> +	return guc->send(guc, action, len);
> +}
> +
> +static inline void intel_guc_notify(struct intel_guc *guc)
> +{
> +	guc->notify(guc);
> +}
> +
> +/* intel_guc_loader.c */
> +int intel_guc_select_fw(struct intel_guc *guc);
> +int intel_guc_init_hw(struct intel_guc *guc);
> +int intel_guc_suspend(struct drm_i915_private *dev_priv);
> +int intel_guc_resume(struct drm_i915_private *dev_priv);

Is this correct ?

> +u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> +
> +/* i915_guc_submission.c */
> +int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> +int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
> +int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
> +void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
> +void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> +void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> +struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
> +
> +/* intel_guc_log.c */
> +int intel_guc_log_create(struct intel_guc *guc);
> +void intel_guc_log_destroy(struct intel_guc *guc);
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
> +void i915_guc_log_register(struct drm_i915_private *dev_priv);
> +void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8b0ae7f..81e03a6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -27,7 +27,6 @@
>   *    Alex Dai <yu.dai@intel.com>
>   */
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  
>  /**
>   * DOC: GuC-specific firmware loader
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 6145fa0..f9459f5 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -21,9 +21,7 @@
>   * IN THE SOFTWARE.
>   *
>   */
> -#include <linux/firmware.h>
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  
>  /**
>   * DOC: HuC Firmware
> @@ -225,54 +223,14 @@ void intel_huc_init_hw(struct intel_huc *huc)
>  }
>  
>  /**
> - * intel_guc_auth_huc() - authenticate ucode
> - * @dev_priv: the drm_i915_device
> - *
> - * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
> - * authenticate_huc interface.
> + * intel_auth_huc() - authenticate HuC ucode
>   */
> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
> +void intel_huc_auth(struct intel_huc *huc)
>  {
> -	struct intel_guc *guc = &dev_priv->guc;
> -	struct intel_huc *huc = &dev_priv->huc;
> -	struct i915_vma *vma;
> -	int ret;
> -	u32 data[2];
> +	struct drm_i915_private *dev_priv = huc_to_i915(huc);
>  
>  	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return;
>  
> -	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> -				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> -	if (IS_ERR(vma)) {
> -		DRM_ERROR("failed to pin huc fw object %d\n",
> -				(int)PTR_ERR(vma));
> -		return;
> -	}
> -
> -	/* Specify auth action and where public signature is. */
> -	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
> -	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
> -
> -	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
> -	if (ret) {
> -		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
> -		goto out;
> -	}

Hmm, I think only this chunk that prepares and sends guc action shall be
moved to intel_guc.c as intel_guc_auth_huc()

Rest of this function shall stay here as it has pure Huc specific bits.

> -
> -	/* Check authentication status, it should be done by now */
> -	ret = intel_wait_for_register(dev_priv,
> -				HUC_STATUS2,
> -				HUC_FW_VERIFIED,
> -				HUC_FW_VERIFIED,
> -				50);
> -
> -	if (ret) {
> -		DRM_ERROR("HuC: Authentication failed %d\n", ret);
> -		goto out;
> -	}
> -
> -out:
> -	i915_vma_unpin(vma);
> +	intel_guc_auth_huc(&dev_priv->guc, &huc->fw);
>  }
> -
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> new file mode 100644
> index 0000000..e90952b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +#ifndef _INTEL_HUC_H_
> +#define _INTEL_HUC_H_
> +

In intel_guc.h you explicitly included set of required headers.
Either do the same here, or assume that they are always available.
(as being included by intel_uc.h as it was done previously?)

> +struct intel_huc {
> +	/* Generic uC firmware management */
> +	struct intel_uc_fw fw;
> +
> +	/* HuC-specific additions */
> +};
> +
> +/* intel_huc.c */
> +void intel_huc_select_fw(struct intel_huc *huc);
> +void intel_huc_init_hw(struct intel_huc *huc);
> +void intel_huc_auth(struct intel_huc *huc);
> +#endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 0178ba4..a3fc4c8 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -23,7 +23,6 @@
>   */
>  
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  #include <linux/firmware.h>
>  
>  /* Cleans up uC firmware by releasing the firmware GEM obj.
> @@ -94,22 +93,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>  }
>  
> -static void gen8_guc_raise_irq(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> -}
> -
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
>  
> -	intel_guc_ct_init_early(&guc->ct);
> -
> -	mutex_init(&guc->send_mutex);
> -	guc->send = intel_guc_send_nop;
> -	guc->notify = gen8_guc_raise_irq;
> +	intel_guc_init_early(guc);
>  }
>  
>  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> @@ -262,32 +250,6 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  	__intel_uc_fw_fini(&dev_priv->huc.fw);
>  }
>  
> -static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> -{
> -	GEM_BUG_ON(!guc->send_regs.base);
> -	GEM_BUG_ON(!guc->send_regs.count);
> -	GEM_BUG_ON(i >= guc->send_regs.count);
> -
> -	return _MMIO(guc->send_regs.base + 4 * i);
> -}
> -
> -static void guc_init_send_regs(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	enum forcewake_domains fw_domains = 0;
> -	unsigned int i;
> -
> -	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> -	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> -
> -	for (i = 0; i < guc->send_regs.count; i++) {
> -		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> -					guc_send_reg(guc, i),
> -					FW_REG_READ | FW_REG_WRITE);
> -	}
> -	guc->send_regs.fw_domains = fw_domains;
> -}
> -
>  static void guc_capture_load_err_log(struct intel_guc *guc)
>  {
>  	if (!guc->log.vma || i915.guc_log_level < 0)
> @@ -295,8 +257,6 @@ static void guc_capture_load_err_log(struct intel_guc *guc)
>  
>  	if (!guc->load_err_log)
>  		guc->load_err_log = i915_gem_object_get(guc->log.vma->obj);
> -
> -	return;
>  }
>  
>  static void guc_free_load_err_log(struct intel_guc *guc)
> @@ -309,8 +269,6 @@ static int guc_enable_communication(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  
> -	guc_init_send_regs(guc);
> -
>  	if (HAS_GUC_CT(dev_priv))
>  		return intel_guc_enable_ct(guc);
>  
> @@ -331,6 +289,7 @@ static void guc_disable_communication(struct intel_guc *guc)
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> +	struct intel_huc *huc = &dev_priv->huc;
>  	int ret, attempts;
>  
>  	if (!i915.enable_guc_loading)
> @@ -386,11 +345,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  	if (ret)
>  		goto err_log_capture;
>  
> +	intel_guc_init_send_regs(guc);
> +
>  	ret = guc_enable_communication(guc);
>  	if (ret)
>  		goto err_log_capture;
>  
> -	intel_guc_auth_huc(dev_priv);
> +	intel_huc_auth(huc);
>  	if (i915.enable_guc_submission) {
>  		if (i915.guc_log_level >= 0)
>  			gen9_enable_guc_interrupts(dev_priv);
> @@ -458,82 +419,3 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  
>  	i915_ggtt_disable_guc(dev_priv);
>  }
> -
> -int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> -{
> -	WARN(1, "Unexpected send: action=%#x\n", *action);
> -	return -ENODEV;
> -}
> -
> -/*
> - * This function implements the MMIO based host to GuC interface.
> - */
> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 status;
> -	int i;
> -	int ret;
> -
> -	GEM_BUG_ON(!len);
> -	GEM_BUG_ON(len > guc->send_regs.count);
> -
> -	/* If CT is available, we expect to use MMIO only during init/fini */
> -	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
> -		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> -		*action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
> -
> -	mutex_lock(&guc->send_mutex);
> -	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
> -
> -	for (i = 0; i < len; i++)
> -		I915_WRITE(guc_send_reg(guc, i), action[i]);
> -
> -	POSTING_READ(guc_send_reg(guc, i - 1));
> -
> -	intel_guc_notify(guc);
> -
> -	/*
> -	 * No GuC command should ever take longer than 10ms.
> -	 * Fast commands should still complete in 10us.
> -	 */
> -	ret = __intel_wait_for_register_fw(dev_priv,
> -					   guc_send_reg(guc, 0),
> -					   INTEL_GUC_RECV_MASK,
> -					   INTEL_GUC_RECV_MASK,
> -					   10, 10, &status);
> -	if (status != INTEL_GUC_STATUS_SUCCESS) {
> -		/*
> -		 * Either the GuC explicitly returned an error (which
> -		 * we convert to -EIO here) or no response at all was
> -		 * received within the timeout limit (-ETIMEDOUT)
> -		 */
> -		if (ret != -ETIMEDOUT)
> -			ret = -EIO;
> -
> -		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
> -			 " ret=%d status=0x%08X response=0x%08X\n",
> -			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
> -	}
> -
> -	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> -	mutex_unlock(&guc->send_mutex);
> -
> -	return ret;
> -}
> -
> -int intel_guc_sample_forcewake(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 action[2];
> -
> -	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
> -	/* WaRsDisableCoarsePowerGating:skl,bxt */
> -	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> -		action[1] = 0;
> -	else
> -		/* bit 0 and 1 are for Render and Media domain separately */
> -		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
> -
> -	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> -}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 22ae52b..c87a2b4 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -24,256 +24,8 @@
>  #ifndef _INTEL_UC_H_
>  #define _INTEL_UC_H_
>  
> -#include "intel_guc_fwif.h"
> -#include "i915_guc_reg.h"
> -#include "intel_ringbuffer.h"
> -#include "intel_guc_ct.h"
> -#include "i915_vma.h"
> -
> -struct drm_i915_gem_request;
> -
> -/*
> - * This structure primarily describes the GEM object shared with the GuC.
> - * The specs sometimes refer to this object as a "GuC context", but we use
> - * the term "client" to avoid confusion with hardware contexts. This
> - * GEM object is held for the entire lifetime of our interaction with
> - * the GuC, being allocated before the GuC is loaded with its firmware.
> - * Because there's no way to update the address used by the GuC after
> - * initialisation, the shared object must stay pinned into the GGTT as
> - * long as the GuC is in use. We also keep the first page (only) mapped
> - * into kernel address space, as it includes shared data that must be
> - * updated on every request submission.
> - *
> - * The single GEM object described here is actually made up of several
> - * separate areas, as far as the GuC is concerned. The first page (kept
> - * kmap'd) includes the "process descriptor" which holds sequence data for
> - * the doorbell, and one cacheline which actually *is* the doorbell; a
> - * write to this will "ring the doorbell" (i.e. send an interrupt to the
> - * GuC). The subsequent  pages of the client object constitute the work
> - * queue (a circular array of work items), again described in the process
> - * descriptor. Work queue pages are mapped momentarily as required.
> - *
> - * We also keep a few statistics on failures. Ideally, these should all
> - * be zero!
> - *   no_wq_space: times that the submission pre-check found no space was
> - *                available in the work queue (note, the queue is shared,
> - *                not per-engine). It is OK for this to be nonzero, but
> - *                it should not be huge!
> - *   b_fail: failed to ring the doorbell. This should never happen, unless
> - *           somehow the hardware misbehaves, or maybe if the GuC firmware
> - *           crashes? We probably need to reset the GPU to recover.
> - *   retcode: errno from last guc_submit()
> - */
> -struct i915_guc_client {
> -	struct i915_vma *vma;
> -	void *vaddr;
> -	struct i915_gem_context *owner;
> -	struct intel_guc *guc;
> -
> -	uint32_t engines;		/* bitmap of (host) engine ids	*/
> -	uint32_t priority;
> -	u32 stage_id;
> -	uint32_t proc_desc_offset;
> -
> -	u16 doorbell_id;
> -	unsigned long doorbell_offset;
> -	u32 doorbell_cookie;
> -
> -	spinlock_t wq_lock;
> -	uint32_t wq_offset;
> -	uint32_t wq_size;
> -	uint32_t wq_tail;
> -	uint32_t wq_rsvd;
> -	uint32_t no_wq_space;
> -
> -	/* Per-engine counts of GuC submissions */
> -	uint64_t submissions[I915_NUM_ENGINES];
> -};
> -
> -enum intel_uc_fw_status {
> -	INTEL_UC_FIRMWARE_FAIL = -1,
> -	INTEL_UC_FIRMWARE_NONE = 0,
> -	INTEL_UC_FIRMWARE_PENDING,
> -	INTEL_UC_FIRMWARE_SUCCESS
> -};
> -
> -/* User-friendly representation of an enum */
> -static inline
> -const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
> -{
> -	switch (status) {
> -	case INTEL_UC_FIRMWARE_FAIL:
> -		return "FAIL";
> -	case INTEL_UC_FIRMWARE_NONE:
> -		return "NONE";
> -	case INTEL_UC_FIRMWARE_PENDING:
> -		return "PENDING";
> -	case INTEL_UC_FIRMWARE_SUCCESS:
> -		return "SUCCESS";
> -	}
> -	return "<invalid>";
> -}
> -
> -enum intel_uc_fw_type {
> -	INTEL_UC_FW_TYPE_GUC,
> -	INTEL_UC_FW_TYPE_HUC
> -};
> -
> -/* User-friendly representation of an enum */
> -static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
> -{
> -	switch (type) {
> -	case INTEL_UC_FW_TYPE_GUC:
> -		return "GuC";
> -	case INTEL_UC_FW_TYPE_HUC:
> -		return "HuC";
> -	}
> -	return "uC";
> -}
> -
> -/*
> - * This structure encapsulates all the data needed during the process
> - * of fetching, caching, and loading the firmware image into the GuC.
> - */
> -struct intel_uc_fw {
> -	const char *path;
> -	size_t size;
> -	struct drm_i915_gem_object *obj;
> -	enum intel_uc_fw_status fetch_status;
> -	enum intel_uc_fw_status load_status;
> -
> -	uint16_t major_ver_wanted;
> -	uint16_t minor_ver_wanted;
> -	uint16_t major_ver_found;
> -	uint16_t minor_ver_found;
> -
> -	enum intel_uc_fw_type type;
> -	uint32_t header_size;
> -	uint32_t header_offset;
> -	uint32_t rsa_size;
> -	uint32_t rsa_offset;
> -	uint32_t ucode_size;
> -	uint32_t ucode_offset;
> -};
> -
> -struct intel_guc_log {
> -	uint32_t flags;
> -	struct i915_vma *vma;
> -	/* The runtime stuff gets created only when GuC logging gets enabled */
> -	struct {
> -		void *buf_addr;
> -		struct workqueue_struct *flush_wq;
> -		struct work_struct flush_work;
> -		struct rchan *relay_chan;
> -	} runtime;
> -	/* logging related stats */
> -	u32 capture_miss_count;
> -	u32 flush_interrupt_count;
> -	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
> -	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
> -	u32 flush_count[GUC_MAX_LOG_BUFFER];
> -};
> -
> -struct intel_guc {
> -	struct intel_uc_fw fw;
> -	struct intel_guc_log log;
> -	struct intel_guc_ct ct;
> -
> -	/* Log snapshot if GuC errors during load */
> -	struct drm_i915_gem_object *load_err_log;
> -
> -	/* intel_guc_recv interrupt related state */
> -	bool interrupts_enabled;
> -
> -	struct i915_vma *ads_vma;
> -	struct i915_vma *stage_desc_pool;
> -	void *stage_desc_pool_vaddr;
> -	struct ida stage_ids;
> -
> -	struct i915_guc_client *execbuf_client;
> -
> -	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> -	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
> -
> -	/* GuC's FW specific registers used in MMIO send */
> -	struct {
> -		u32 base;
> -		unsigned int count;
> -		enum forcewake_domains fw_domains;
> -	} send_regs;
> -
> -	/* To serialize the intel_guc_send actions */
> -	struct mutex send_mutex;
> -
> -	/* GuC's FW specific send function */
> -	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> -
> -	/* GuC's FW specific notify function */
> -	void (*notify)(struct intel_guc *guc);
> -};
> -
> -struct intel_huc {
> -	/* Generic uC firmware management */
> -	struct intel_uc_fw fw;
> -
> -	/* HuC-specific additions */
> -};
> -
> -/* intel_uc.c */
> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
> -void intel_uc_init_early(struct drm_i915_private *dev_priv);
> -void intel_uc_init_fw(struct drm_i915_private *dev_priv);
> -void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
> -int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> -void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> -int intel_guc_sample_forcewake(struct intel_guc *guc);
> -int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
> -
> -static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
> -{
> -	return guc->send(guc, action, len);
> -}
> -
> -static inline void intel_guc_notify(struct intel_guc *guc)
> -{
> -	guc->notify(guc);
> -}
> -
> -/* intel_guc_loader.c */
> -int intel_guc_select_fw(struct intel_guc *guc);
> -int intel_guc_init_hw(struct intel_guc *guc);
> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
> -int intel_guc_resume(struct drm_i915_private *dev_priv);
> -u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> -
> -/* i915_guc_submission.c */
> -int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> -int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
> -int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
> -void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
> -void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> -struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
> -
> -/* intel_guc_log.c */
> -int intel_guc_log_create(struct intel_guc *guc);
> -void intel_guc_log_destroy(struct intel_guc *guc);
> -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
> -void i915_guc_log_register(struct drm_i915_private *dev_priv);
> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> -
> -static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> -{
> -	u32 offset = i915_ggtt_offset(vma);
> -	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> -	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> -	return offset;
> -}
> -
> -/* intel_huc.c */
> -void intel_huc_select_fw(struct intel_huc *huc);
> -void intel_huc_init_hw(struct intel_huc *huc);
> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
> +#include <intel_uc_common.h>
> +#include <intel_guc.h>
> +#include <intel_huc.h>
>  

If you insist of having separate headers for guc and huc, then maybe
instead of introducing yet another common header file, just leave all common
uc staff here and include uc.h header in both intel_guc.h and intel_huc.h.


>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h
> new file mode 100644
> index 0000000..f454f73
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_uc_common.h
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +#ifndef _INTEL_UC_COMMON_H_
> +#define _INTEL_UC_COMMON_H_
> +
> +enum intel_uc_fw_status {
> +	INTEL_UC_FIRMWARE_FAIL = -1,
> +	INTEL_UC_FIRMWARE_NONE = 0,
> +	INTEL_UC_FIRMWARE_PENDING,
> +	INTEL_UC_FIRMWARE_SUCCESS
> +};
> +
> +/* User-friendly representation of an enum */
> +static inline
> +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
> +{
> +	switch (status) {
> +	case INTEL_UC_FIRMWARE_FAIL:
> +		return "FAIL";
> +	case INTEL_UC_FIRMWARE_NONE:
> +		return "NONE";
> +	case INTEL_UC_FIRMWARE_PENDING:
> +		return "PENDING";
> +	case INTEL_UC_FIRMWARE_SUCCESS:
> +		return "SUCCESS";
> +	}
> +	return "<invalid>";
> +}
> +
> +enum intel_uc_fw_type {
> +	INTEL_UC_FW_TYPE_GUC,
> +	INTEL_UC_FW_TYPE_HUC
> +};
> +
> +/* User-friendly representation of an enum */
> +static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
> +{
> +	switch (type) {
> +	case INTEL_UC_FW_TYPE_GUC:
> +		return "GuC";
> +	case INTEL_UC_FW_TYPE_HUC:
> +		return "HuC";
> +	}
> +	return "uC";
> +}
> +
> +/*
> + * This structure encapsulates all the data needed during the process
> + * of fetching, caching, and loading the firmware image into the GuC.
> + */
> +struct intel_uc_fw {
> +	const char *path;
> +	size_t size;
> +	struct drm_i915_gem_object *obj;
> +	enum intel_uc_fw_status fetch_status;
> +	enum intel_uc_fw_status load_status;
> +
> +	uint16_t major_ver_wanted;
> +	uint16_t minor_ver_wanted;
> +	uint16_t major_ver_found;
> +	uint16_t minor_ver_found;
> +
> +	enum intel_uc_fw_type type;
> +	uint32_t header_size;
> +	uint32_t header_offset;
> +	uint32_t rsa_size;
> +	uint32_t rsa_offset;
> +	uint32_t ucode_size;
> +	uint32_t ucode_offset;
> +};
> +
> +/* intel_uc.c */
> +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
> +void intel_uc_init_early(struct drm_i915_private *dev_priv);
> +void intel_uc_init_fw(struct drm_i915_private *dev_priv);
> +void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> +void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> +
> +#endif
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/guc: Fix GuC interaction in reset/suspend scenarios

[Michal Wajdeczko]

On Fri, Sep 01, 2017 at 11:02:10AM +0530, Sagar Arun Kamble wrote:
> Tearing down of guc_ggtt_invalidate/guc_interrupts/guc_communication
> setup should happen towards end of reset/suspend as these are
> setup back again during recovery/resume.
> 
> Prepared helpers intel_guc_pause and intel_guc_unpause that will do
> teardown/bringup of this setup along with suspension/resumption of GuC if
> loaded. Moved intel_guc_suspend, intel_guc_resume to intel_guc.c.

Please try to limit number of changes in single patch.
Also describe idea behind system/runtime functions.

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c            |   6 +-
>  drivers/gpu/drm/i915/i915_gem.c            |   6 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c |  52 ----------
>  drivers/gpu/drm/i915/intel_guc.c           | 152 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h           |   9 +-
>  drivers/gpu/drm/i915/intel_uc.c            |  29 +-----
>  6 files changed, 169 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2ae730c..b2e8f95 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1690,8 +1690,6 @@ static int i915_drm_resume(struct drm_device *dev)
>  	}
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	intel_guc_resume(dev_priv);
> -
>  	intel_modeset_init_hw(dev);
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> @@ -2486,7 +2484,7 @@ static int intel_runtime_suspend(struct device *kdev)
>  	 */
>  	i915_gem_runtime_suspend(dev_priv);
>  
> -	intel_guc_suspend(dev_priv);
> +	intel_guc_runtime_suspend(&dev_priv->guc);
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  
> @@ -2571,7 +2569,7 @@ static int intel_runtime_resume(struct device *kdev)
>  	if (intel_uncore_unclaimed_mmio(dev_priv))
>  		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
>  
> -	intel_guc_resume(dev_priv);
> +	intel_guc_runtime_resume(&dev_priv->guc);
>  
>  	if (IS_GEN9_LP(dev_priv)) {
>  		bxt_disable_dc9(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e4cc08b..977500f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2846,6 +2846,8 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
>  
>  	i915_gem_revoke_fences(dev_priv);
>  
> +	intel_guc_reset_prepare(&dev_priv->guc);
> +
>  	return err;
>  }
>  
> @@ -4574,8 +4576,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	i915_gem_contexts_lost(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	intel_guc_suspend(dev_priv);
> -
>  	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>  	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
>  
> @@ -4592,6 +4592,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>  		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>  
> +	intel_guc_system_suspend(&dev_priv->guc);
> +
>  	/*
>  	 * Neither the BIOS, ourselves or any other kernel
>  	 * expects the system to be in execlists mode on startup,
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 602ae8a..2f977ab 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1287,55 +1287,3 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>  	guc_client_free(guc->execbuf_client);
>  	guc->execbuf_client = NULL;
>  }
> -
> -/**
> - * intel_guc_suspend() - notify GuC entering suspend state
> - * @dev_priv:	i915 device private
> - */
> -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)
> -		return 0;
> -
> -	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;
> -	/* first page is shared data with GuC */
> -	data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
> -
> -	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> -}
> -
> -/**
> - * intel_guc_resume() - notify GuC resuming from suspend state
> - * @dev_priv:	i915 device private
> - */
> -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)
> -		return 0;
> -
> -	if (i915.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;
> -	/* first page is shared data with GuC */
> -	data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
> -
> -	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> -}
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 978a0e3..1fd8599 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -191,3 +191,155 @@ void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw)
>  out:
>  	i915_vma_unpin(vma);
>  }
> +
> +int intel_guc_enable_communication(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	if (HAS_GUC_CT(dev_priv))
> +		return intel_guc_enable_ct(guc);
> +
> +	guc->send = intel_guc_send_mmio;
> +	return 0;
> +}
> +
> +void intel_guc_disable_communication(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	if (HAS_GUC_CT(dev_priv))
> +		intel_guc_disable_ct(guc);
> +
> +	guc->send = intel_guc_send_nop;
> +}
> +
> +/**
> + * intel_guc_suspend() - notify GuC entering suspend state
> + */
> +static int intel_guc_suspend(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct i915_gem_context *ctx;
> +	u32 data[3];
> +
> +	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +		return 0;
> +
> +	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;
> +	/* first page is shared data with GuC */
> +	data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
> +
> +	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> +}
> +
> +/**
> + * intel_guc_resume() - notify GuC resuming from suspend state
> + */
> +static int intel_guc_resume(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct i915_gem_context *ctx;
> +	u32 data[3];
> +
> +	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +		return 0;
> +
> +	ctx = dev_priv->kernel_context;
> +
> +	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);
> +
> +	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> +}
> +
> +static void intel_guc_sanitize(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	i915_ggtt_disable_guc(dev_priv);
> +	intel_guc_disable_communication(guc);
> +	gen9_disable_guc_interrupts(dev_priv);
> +}
> +
> +void intel_guc_reset_prepare(struct intel_guc *guc)
> +{
> +	if (!i915.enable_guc_loading)
> +		return;
> +
> +	intel_guc_sanitize(guc);
> +	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
> +}
> +
> +static int intel_guc_pause(struct intel_guc *guc)
> +{
> +	int ret = 0;
> +
> +	ret = intel_guc_suspend(guc);
> +	intel_guc_sanitize(guc);
> +
> +	return ret;
> +}
> +
> +static int intel_guc_unpause(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	int ret = 0;
> +
> +	if (i915.guc_log_level >= 0)
> +		gen9_enable_guc_interrupts(dev_priv);
> +	intel_guc_enable_communication(guc);
> +	i915_ggtt_enable_guc(dev_priv);
> +	ret = intel_guc_resume(guc);
> +
> +	return ret;
> +}
> +
> +int intel_guc_runtime_suspend(struct intel_guc *guc)
> +{
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	return intel_guc_pause(guc);
> +}
> +
> +int intel_guc_runtime_resume(struct intel_guc *guc)
> +{
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	return intel_guc_unpause(guc);
> +}
> +
> +int intel_guc_system_suspend(struct intel_guc *guc)
> +{
> +	int ret = 0;
> +
> +	if (!i915.enable_guc_loading)
> +		return ret;
> +
> +	ret = intel_guc_pause(guc);
> +	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
> +
> +	return ret;
> +}
> +
> +int intel_guc_system_resume(struct intel_guc *guc)
> +{
> +	int ret = 0;
> +
> +	if (!i915.enable_guc_loading)
> +		return ret;
> +
> +	/*
> +	 * Placeholder for GuC resume from system suspend/freeze states.
> +	 * Currently full reinitialization of GEM and GuC happens along
> +	 * these paths, Hence this function is doing nothing.
> +	 */
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index b329830..bf4dda0 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -162,6 +162,13 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
>  void intel_guc_auth_huc(struct intel_guc *guc, struct intel_uc_fw *huc_fw);
> +int intel_guc_enable_communication(struct intel_guc *guc);
> +void intel_guc_disable_communication(struct intel_guc *guc);
> +void intel_guc_reset_prepare(struct intel_guc *guc);
> +int intel_guc_runtime_suspend(struct intel_guc *guc);
> +int intel_guc_runtime_resume(struct intel_guc *guc);
> +int intel_guc_system_suspend(struct intel_guc *guc);
> +int intel_guc_system_resume(struct intel_guc *guc);
>  
>  static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
>  				 u32 len)
> @@ -177,8 +184,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
>  /* intel_guc_loader.c */
>  int intel_guc_select_fw(struct intel_guc *guc);
>  int intel_guc_init_hw(struct intel_guc *guc);
> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
> -int intel_guc_resume(struct drm_i915_private *dev_priv);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index a3fc4c8..30c004c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -265,27 +265,6 @@ static void guc_free_load_err_log(struct intel_guc *guc)
>  		i915_gem_object_put(guc->load_err_log);
>  }
>  
> -static int guc_enable_communication(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	if (HAS_GUC_CT(dev_priv))
> -		return intel_guc_enable_ct(guc);
> -
> -	guc->send = intel_guc_send_mmio;
> -	return 0;
> -}
> -
> -static void guc_disable_communication(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	if (HAS_GUC_CT(dev_priv))
> -		intel_guc_disable_ct(guc);
> -
> -	guc->send = intel_guc_send_nop;
> -}

Why above functions are moved in this patch ?

> -
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> @@ -295,7 +274,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  	if (!i915.enable_guc_loading)
>  		return 0;
>  
> -	guc_disable_communication(guc);
> +	intel_guc_disable_communication(guc);
>  	gen9_reset_guc_interrupts(dev_priv);
>  
>  	/* We need to notify the guc whenever we change the GGTT */
> @@ -347,7 +326,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  	intel_guc_init_send_regs(guc);
>  
> -	ret = guc_enable_communication(guc);
> +	ret = intel_guc_enable_communication(guc);
>  	if (ret)
>  		goto err_log_capture;
>  
> @@ -373,7 +352,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  	 * marks the GPU as wedged until reset).
>  	 */
>  err_interrupts:
> -	guc_disable_communication(guc);
> +	intel_guc_disable_communication(guc);
>  	gen9_disable_guc_interrupts(dev_priv);
>  err_log_capture:
>  	guc_capture_load_err_log(guc);
> @@ -410,7 +389,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  	if (i915.enable_guc_submission)
>  		i915_guc_submission_disable(dev_priv);
>  
> -	guc_disable_communication(&dev_priv->guc);
> +	intel_guc_disable_communication(&dev_priv->guc);
>  
>  	if (i915.enable_guc_submission) {
>  		gen9_disable_guc_interrupts(dev_priv);
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/guc: Fix GuC HW/SW state cleanup in unload path

[Michal Wajdeczko]

On Fri, Sep 01, 2017 at 11:02:11AM +0530, Sagar Arun Kamble wrote:
> Teardown of GuC HW/SW state was not properly done in unload path.
> During unload, we can rely on intel_guc_reset_prepare being done
> as part of i915_gem_suspend for disabling GuC interfaces.
> We will have to disable GuC submission prior to suspend as that involves
> communication with GuC to destroy doorbell. So intel_uc_fini_hw has to
> be called as part of i915_gem_suspend during unload as that really
> takes care of finishing the GuC operations. Created new parameter for
> i915_gem_suspend to handle unload/suspend path w.r.t gem and GuC suspend.
> GuC related allocations are cleaned up as part of intel_uc_cleanup_hw.
> 
> v2: Prepared i915_gem_unload. (Michal)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c        |  6 +--
>  drivers/gpu/drm/i915/i915_drv.h        |  1 +
>  drivers/gpu/drm/i915/i915_gem.c        | 79 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.c       | 13 ++++++
>  drivers/gpu/drm/i915/intel_guc.h       |  1 +
>  drivers/gpu/drm/i915/intel_uc.c        | 14 +++---
>  drivers/gpu/drm/i915/intel_uc_common.h |  1 +
>  7 files changed, 103 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b2e8f95..b6cc2fe 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -601,7 +601,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>  	i915_gem_drain_workqueue(dev_priv);
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> -	intel_uc_fini_hw(dev_priv);
> +	intel_uc_cleanup_hw(dev_priv);
>  	i915_gem_cleanup_engines(dev_priv);
>  	i915_gem_contexts_fini(dev_priv);
>  	i915_gem_cleanup_userptr(dev_priv);
> @@ -682,7 +682,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	return 0;
>  
>  cleanup_gem:
> -	if (i915_gem_suspend(dev_priv))
> +	if (i915_gem_unload(dev_priv))
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  	i915_gem_fini(dev_priv);
>  cleanup_uc:
> @@ -1375,7 +1375,7 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	i915_driver_unregister(dev_priv);
>  
> -	if (i915_gem_suspend(dev_priv))
> +	if (i915_gem_unload(dev_priv))
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 064bf0f..570e71e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3628,6 +3628,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>  			   unsigned int flags);
>  int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
> +int __must_check i915_gem_unload(struct drm_i915_private *dev_priv);
>  void i915_gem_resume(struct drm_i915_private *dev_priv);
>  int i915_gem_fault(struct vm_fault *vmf);
>  int i915_gem_object_wait(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 977500f..24cefe9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4624,6 +4624,85 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> +int i915_gem_unload(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	int ret;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	intel_suspend_gt_powersave(dev_priv);
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	/* We have to flush all the executing contexts to main memory so
> +	 * that they can saved in the hibernation image. To ensure the last
> +	 * context image is coherent, we have to switch away from it. That
> +	 * leaves the dev_priv->kernel_context still active when
> +	 * we actually suspend, and its image in memory may not match the GPU
> +	 * state. Fortunately, the kernel_context is disposable and we do
> +	 * not rely on its state.
> +	 */
> +	ret = i915_gem_switch_to_kernel_context(dev_priv);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = i915_gem_wait_for_idle(dev_priv,
> +				     I915_WAIT_INTERRUPTIBLE |
> +				     I915_WAIT_LOCKED);
> +	if (ret)
> +		goto err_unlock;
> +
> +	assert_kernel_context_is_current(dev_priv);
> +	i915_gem_contexts_lost(dev_priv);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> +	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
> +
> +	/* As the idle_work is rearming if it detects a race, play safe and
> +	 * repeat the flush until it is definitely idle.
> +	 */
> +	while (flush_delayed_work(&dev_priv->gt.idle_work))
> +		;
> +
> +	/* Assert that we successfully flushed all the work and
> +	 * reset the GPU back to its idle, low power state.
> +	 */
> +	WARN_ON(dev_priv->gt.awake);
> +	WARN_ON(!intel_engines_are_idle(dev_priv));
> +
> +	/* Handle GuC suspension in case of unloading/system suspend */
> +	intel_uc_fini_hw(dev_priv);
> +
> +	/*
> +	 * Neither the BIOS, ourselves or any other kernel
> +	 * expects the system to be in execlists mode on startup,
> +	 * so we need to reset the GPU back to legacy mode. And the only
> +	 * known way to disable logical contexts is through a GPU reset.
> +	 *
> +	 * So in order to leave the system in a known default configuration,
> +	 * always reset the GPU upon unload and suspend. Afterwards we then
> +	 * clean up the GEM state tracking, flushing off the requests and
> +	 * leaving the system in a known idle state.
> +	 *
> +	 * Note that is of the upmost importance that the GPU is idle and
> +	 * all stray writes are flushed *before* we dismantle the backing
> +	 * storage for the pinned objects.
> +	 *
> +	 * However, since we are uncertain that resetting the GPU on older
> +	 * machines is a good idea, we don't - just in case it leaves the
> +	 * machine in an unusable condition.
> +	 */
> +	i915_gem_sanitize(dev_priv);
> +	goto out_rpm_put;
> +
> +err_unlock:
> +	mutex_unlock(&dev->struct_mutex);
> +out_rpm_put:
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
> +}
> +
>  void i915_gem_resume(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 1fd8599..da81c37 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -343,3 +343,16 @@ int intel_guc_system_resume(struct intel_guc *guc)
>  	 */
>  	return ret;
>  }
> +
> +void intel_guc_fini(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct drm_device *dev = &dev_priv->drm;
> +
> +	if (i915.enable_guc_submission) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_guc_submission_disable(dev_priv);
> +		mutex_unlock(&dev->struct_mutex);
> +		intel_guc_reset_prepare(guc);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index bf4dda0..1b3305f 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -169,6 +169,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  int intel_guc_runtime_resume(struct intel_guc *guc);
>  int intel_guc_system_suspend(struct intel_guc *guc);
>  int intel_guc_system_resume(struct intel_guc *guc);
> +void intel_guc_fini(struct intel_guc *guc);

Do we have corresponding guc_init() ?

>  
>  static inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
>  				 u32 len)
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 30c004c..9216c73 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -381,20 +381,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv)

As you have below "uc_cleanup_hw" then maybe this function shall also be renamed ?

>  {
> +	intel_guc_fini(&dev_priv->guc);
> +}
> +
> +void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv)
> +{
>  	guc_free_load_err_log(&dev_priv->guc);
>  
>  	if (!i915.enable_guc_loading)
>  		return;
>  
>  	if (i915.enable_guc_submission)
> -		i915_guc_submission_disable(dev_priv);
> -
> -	intel_guc_disable_communication(&dev_priv->guc);
> -
> -	if (i915.enable_guc_submission) {
> -		gen9_disable_guc_interrupts(dev_priv);
>  		i915_guc_submission_fini(dev_priv);
> -	}
> -
> -	i915_ggtt_disable_guc(dev_priv);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h
> index f454f73..c6e873d 100644
> --- a/drivers/gpu/drm/i915/intel_uc_common.h
> +++ b/drivers/gpu/drm/i915/intel_uc_common.h
> @@ -97,5 +97,6 @@ struct intel_uc_fw {
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> +void intel_uc_cleanup_hw(struct drm_i915_private *dev_priv);
>  
>  #endif
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Michal Wajdeczko]

On Fri, Sep 01, 2017 at 11:02:12AM +0530, Sagar Arun Kamble wrote:
> With GuC v9, new type of Default/critical logging in GuC to enable
> capturing minimal important logs in production systems efficiently.
> This patch enables this logging in GuC by default always. It should
> be noted that streaming support with half-full interrupt mechanism
> that is present for normal logging is not present for this type of
> logging.
> 
> v2: Emulating GuC critical logging through i915.guc_log_level. Setting
> this to 0 will make GuC critical logging ON and setting it to 1-4 will
> communicate log level of 0-3 to GuC.
> 
> v3: Commit message update. Enable default/critical logging in GuC always.
> Fixed RPM wake during guc_log_unregister in the unload path.
> 
> Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
> Cc: Fry Gregory P <gregory.p.fry@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Spotswood John A <john.a.spotswood@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fwif.h   | 14 +++++++++++++-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_guc_log.c    |  7 ++++++-
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 5fa2860..353b081 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -132,6 +132,7 @@
>  #define   GUC_WQ_TRACK_ENABLED		(1 << 8)
>  #define   GUC_ADS_ENABLED		(1 << 9)
>  #define   GUC_DEBUG_RESERVED		(1 << 10)
> +#define   GUC_V9_CRITICAL_LOGGING_DISABLED	(1 << 10)

It looks that GUC_DEBUG_RESERVED was never used, so maybe just remove old
name and add new one without V9 prefix ?

>  #define   GUC_ADS_ADDR_SHIFT		11
>  #define   GUC_ADS_ADDR_MASK		0xfffff800
>  
> @@ -139,6 +140,16 @@
>  
>  #define GUC_CTL_MAX_DWORDS		(SOFT_SCRATCH_COUNT - 2) /* [1..14] */
>  
> +/*
> + * Critical logging in GuC is to be enabled always from GuC v9+.
> + * (for KBL - v9.39+)
> + */
> +#define NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw)	\
> +	(((IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv)) && \
> +				    guc_fw->major_ver_found >= 9) || \
> +	  (IS_KABYLAKE(dev_priv) && guc_fw->major_ver_found >= 9 && \
> +				    guc_fw->minor_ver_found >= 39))
> +

As this is about Guc caps, then maybe simpler:

#define GUC_NEEDS_CRITICAL_LOGGING(guc)	\
	(((IS_SKYLAKE(guc_to_i915(guc)) || IS_BROXTON(guc_to_i915(guc)) && \
				    guc->fw.major_ver_found >= 9) || \
	  (IS_KABYLAKE(guc_to_i915(guc)) && guc->fw.major_ver_found >= 9 && \
				    guc->fw.minor_ver_found >= 39))


>  /**
>   * DOC: GuC Firmware Layout
>   *
> @@ -539,7 +550,8 @@ struct guc_log_buffer_state {
>  		u32 logging_enabled:1;
>  		u32 reserved1:3;
>  		u32 verbosity:4;
> -		u32 reserved2:24;
> +		u32 critical_logging_enabled:1;
> +		u32 reserved2:23;
>  	};
>  	u32 value;
>  } __packed;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 81e03a6..d2b027c 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -106,6 +106,7 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
>  static void guc_params_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	u32 params[GUC_CTL_MAX_DWORDS];
>  	int i;
>  
> @@ -136,6 +137,15 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
>  	} else
>  		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>  
> +	/*
> +	 * GuC has critical/default logging level which is to be enabled
> +	 * always from GuC v9 onwards.
> +	 */
> +	if (NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw)) {
> +		params[GUC_CTL_DEBUG] &=
> +			~GUC_V9_CRITICAL_LOGGING_DISABLED;

Maybe I'm missing something, but when this bit was set that you have to clear it now?


> +	}
> +
>  	/* If GuC submission is enabled, set up additional parameters here */
>  	if (i915.enable_guc_submission) {
>  		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 16d3b87..6877e34 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -589,7 +589,7 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> -
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	union guc_log_control log_param;
>  	int ret;
>  
> @@ -603,6 +603,9 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>  	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>  		return 0;
>  
> +	if (NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw))
> +		log_param.critical_logging_enabled = 1;
> +
>  	ret = guc_log_control(guc, log_param.value);
>  	if (ret < 0) {
>  		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
> @@ -655,8 +658,10 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> +	intel_runtime_pm_get(dev_priv);
>  	/* GuC logging is currently the only user of Guc2Host interrupts */
>  	gen9_disable_guc_interrupts(dev_priv);
> +	intel_runtime_pm_put(dev_priv);

Can this RPM be moved to separate patch ?

>  	guc_log_runtime_destroy(&dev_priv->guc);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx