diff mbox

[1/4] drm/i915/guc: Move shared data allocation away from submission path

Message ID 20171211151256.17099-1-michal.winiarski@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michał Winiarski Dec. 11, 2017, 3:12 p.m. UTC 
We need shared data for actions (e.g. guc suspend/resume), and we're
using those with GuC submission disabled.
Let's introduce intel_guc_init and move shared data alloc there.

This fixes GPF during module unload with HuC, but without GuC submission:

BUG: unable to handle kernel NULL pointer dereference at 000000005aee7809
IP: intel_guc_suspend+0x34/0x140 [i915]
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP
Modules linked in: i915(O-) netconsole x86_pkg_temp_thermal
intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel
mei_me i2c_i801 mei prime_numbers [last unloaded: i915]
CPU: 2 PID: 2794 Comm: rmmod Tainted: G     U  W  O 4.15.0-rc2+ #297
Hardware name: /NUC6i5SYB, BIOS SYSKLi35.86A.0054.2016.0930.1102 09/30/2016
task: 0000000055945c61 task.stack: 00000000264ccb43
RIP: 0010:intel_guc_suspend+0x34/0x140 [i915]
RSP: 0018:ffffc90000483df8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff880829180000 RCX: 0000000000000000
RDX: 0000000000000006 RSI: ffff880844c2c938 RDI: ffff880844c2c000
RBP: ffff880829180000 R08: 00000000a29c58c1 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffa040ba40
R13: ffffffffa040bab0 R14: ffff88084a195060 R15: 000055df3ef357a0
FS:  00007ff43c043740(0000) GS:ffff88084e200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000000000f9 CR3: 000000083f179005 CR4: 00000000003606e0
Call Trace:
 i915_gem_suspend+0x9d/0x130 [i915]
 ? i915_driver_unload+0x68/0x180 [i915]
 i915_driver_unload+0x70/0x180 [i915]
 i915_pci_remove+0x15/0x20 [i915]
 pci_device_remove+0x36/0xb0
 device_release_driver_internal+0x15f/0x220
 driver_detach+0x3a/0x80
 bus_remove_driver+0x58/0xd0
 pci_unregister_driver+0x29/0x90
 SyS_delete_module+0x150/0x1e0
 entry_SYSCALL_64_fastpath+0x23/0x9a
RIP: 0033:0x7ff43b51b5c7
RSP: 002b:00007ffe6825a758 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007ff43b51b5c7
RDX: 000000000000000a RSI: 0000000000000800 RDI: 000055df3ef35808
RBP: 0000000000000000 R08: 00007ffe682596d1 R09: 0000000000000000
R10: 00007ff43b594880 R11: 0000000000000206 R12: 000055df3ef357a0
R13: 00007ffe68259740 R14: 000055df3ef35260 R15: 000055df3ef357a0
Code: 00 00 02 74 03 31 c0 c3 53 48 89 fb 48 83 ec 10 e8 52 0f
f8 ff 48 b8 01 05 00 00 02 00 00 00 48 89 44 24 04 48 8b 83 00 12 00 00 <f6> 80
f9 00 00 00 01 0f 84 a7 00 00 00 f6 80 98 00 00 00 01 0f
RIP: intel_guc_suspend+0x34/0x140 [i915] RSP: ffffc90000483df8
CR2: 00000000000000f9
---[ end trace 23a192a61d937a3e ]---

Fixes: b8e5eb960b28 ("drm/i915/guc: Allocate separate shared data object for GuC communication")
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c            | 51 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h            |  2 ++
 drivers/gpu/drm/i915/intel_guc_submission.c | 37 +--------------------
 drivers/gpu/drm/i915/intel_uc.c             | 10 +++---
 4 files changed, 60 insertions(+), 40 deletions(-)

Comments

Chris Wilson Dec. 11, 2017, 3:17 p.m. UTC | #1 
Quoting Michał Winiarski (2017-12-11 15:12:53)
> +int intel_guc_init(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +       int ret;
> +
> +       ret = guc_shared_data_create(guc);
> +       if (ret)
> +               return ret;
> +       GEM_BUG_ON(!guc->shared_data);

We don't have the early return if (allocated) now, so we don't need this
check to keep smatch quiet. You're welcome to keep it if you think it
reinforces that intel_guc_init() is responsible for allocating
guc->shared_data, which is fair enough (as I think it does help the
reader know what was allocated behind the scenes).
-Chris
Chris Wilson Dec. 11, 2017, 3:19 p.m. UTC | #2 
Quoting Michał Winiarski (2017-12-11 15:12:53)
> +int intel_guc_init(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +       int ret;
> +
> +       ret = guc_shared_data_create(guc);
> +       if (ret)
> +               return ret;
> +       GEM_BUG_ON(!guc->shared_data);
> +
> +       /* We need to notify the guc whenever we change the GGTT */
> +       i915_ggtt_enable_guc(dev_priv);

Hmm, I think we want to call i915_ggtt_invalidate() from
i915_ggtt_enable_guc(). Care to include a patch?
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 177ee69ca9b1..92ed22f38fc4 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -69,6 +69,57 @@  void intel_guc_init_early(struct intel_guc *guc)
 	guc->notify = gen8_guc_raise_irq;
 }
 
+static int guc_shared_data_create(struct intel_guc *guc)
+{
+	struct i915_vma *vma;
+	void *vaddr;
+
+	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		i915_vma_unpin_and_release(&vma);
+		return PTR_ERR(vaddr);
+	}
+
+	guc->shared_data = vma;
+	guc->shared_data_vaddr = vaddr;
+
+	return 0;
+}
+
+static void guc_shared_data_destroy(struct intel_guc *guc)
+{
+	i915_gem_object_unpin_map(guc->shared_data->obj);
+	i915_vma_unpin_and_release(&guc->shared_data);
+}
+
+int intel_guc_init(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	int ret;
+
+	ret = guc_shared_data_create(guc);
+	if (ret)
+		return ret;
+	GEM_BUG_ON(!guc->shared_data);
+
+	/* We need to notify the guc whenever we change the GGTT */
+	i915_ggtt_enable_guc(dev_priv);
+
+	return 0;
+}
+
+void intel_guc_fini(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	i915_ggtt_disable_guc(dev_priv);
+	guc_shared_data_destroy(guc);
+}
+
 static u32 get_gt_type(struct drm_i915_private *dev_priv)
 {
 	/* XXX: GT type based on PCI device ID? field seems unused by fw */
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 59856726d2bc..81659e223e11 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -119,6 +119,8 @@  static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 void intel_guc_init_early(struct intel_guc *guc);
 void intel_guc_init_send_regs(struct intel_guc *guc);
 void intel_guc_init_params(struct intel_guc *guc);
+int intel_guc_init(struct intel_guc *guc);
+void intel_guc_fini(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);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 912ff143d531..c020560c395e 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -447,33 +447,6 @@  static void guc_stage_desc_fini(struct intel_guc *guc,
 	memset(desc, 0, sizeof(*desc));
 }
 
-static int guc_shared_data_create(struct intel_guc *guc)
-{
-	struct i915_vma *vma;
-	void *vaddr;
-
-	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
-
-	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
-	if (IS_ERR(vaddr)) {
-		i915_vma_unpin_and_release(&vma);
-		return PTR_ERR(vaddr);
-	}
-
-	guc->shared_data = vma;
-	guc->shared_data_vaddr = vaddr;
-
-	return 0;
-}
-
-static void guc_shared_data_destroy(struct intel_guc *guc)
-{
-	i915_gem_object_unpin_map(guc->shared_data->obj);
-	i915_vma_unpin_and_release(&guc->shared_data);
-}
-
 /* Construct a Work Item and append it to the GuC's Work Queue */
 static void guc_wq_item_append(struct intel_guc_client *client,
 			       u32 target_engine, u32 context_desc,
@@ -1279,14 +1252,9 @@  int intel_guc_submission_init(struct intel_guc *guc)
 	 */
 	GEM_BUG_ON(!guc->stage_desc_pool);
 
-	ret = guc_shared_data_create(guc);
-	if (ret)
-		goto err_stage_desc_pool;
-	GEM_BUG_ON(!guc->shared_data);
-
 	ret = intel_guc_log_create(guc);
 	if (ret < 0)
-		goto err_shared_data;
+		goto err_stage_desc_pool;
 
 	ret = guc_preempt_work_create(guc);
 	if (ret)
@@ -1304,8 +1272,6 @@  int intel_guc_submission_init(struct intel_guc *guc)
 	guc_preempt_work_destroy(guc);
 err_log:
 	intel_guc_log_destroy(guc);
-err_shared_data:
-	guc_shared_data_destroy(guc);
 err_stage_desc_pool:
 	guc_stage_desc_pool_destroy(guc);
 	return ret;
@@ -1316,7 +1282,6 @@  void intel_guc_submission_fini(struct intel_guc *guc)
 	guc_ads_destroy(guc);
 	guc_preempt_work_destroy(guc);
 	intel_guc_log_destroy(guc);
-	guc_shared_data_destroy(guc);
 	guc_stage_desc_pool_destroy(guc);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 461047c86e0d..3040a0e00142 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -24,6 +24,7 @@ 
 
 #include "intel_uc.h"
 #include "intel_guc_submission.h"
+#include "intel_guc.h"
 #include "i915_drv.h"
 
 /* Reset GuC providing us with fresh state for both GuC and HuC.
@@ -204,8 +205,9 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
 
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(dev_priv);
+	ret = intel_guc_init(guc);
+	if (ret)
+		goto err_out;
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
 		/*
@@ -298,7 +300,7 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (USES_GUC_SUBMISSION(dev_priv))
 		intel_guc_submission_fini(guc);
 err_guc:
-	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_fini(guc);
 err_out:
 	/*
 	 * Note that there is no fallback as either user explicitly asked for
@@ -330,5 +332,5 @@  void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 		intel_guc_submission_fini(guc);
 	}
 
-	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_fini(guc);
 }