* [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path
@ 2017-12-11 15:12 Michał Winiarski
2017-12-11 15:12 ` [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex Michał Winiarski
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Michał Winiarski @ 2017-12-11 15:12 UTC (permalink / raw)
To: intel-gfx
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(-)
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);
}
--
2.14.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex
2017-12-11 15:12 [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Michał Winiarski
@ 2017-12-11 15:12 ` Michał Winiarski
2017-12-11 15:26 ` Chris Wilson
2017-12-11 15:15 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Michał Winiarski
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Michał Winiarski @ 2017-12-11 15:12 UTC (permalink / raw)
To: intel-gfx
This gets rid of the following lockdep splat:
======================================================
WARNING: possible circular locking dependency detected
4.15.0-rc2-CI-Patchwork_7428+ #1 Not tainted
------------------------------------------------------
debugfs_test/1351 is trying to acquire lock:
(&dev->struct_mutex){+.+.}, at: [<000000009d90d1a3>] i915_mutex_lock_interruptible+0x47/0x130 [i915]
but task is already holding lock:
(&mm->mmap_sem){++++}, at: [<000000005df01c1e>] __do_page_fault+0x106/0x560
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #6 (&mm->mmap_sem){++++}:
__might_fault+0x63/0x90
_copy_to_user+0x1e/0x70
filldir+0x8c/0xf0
dcache_readdir+0xeb/0x160
iterate_dir+0xe6/0x150
SyS_getdents+0xa0/0x130
entry_SYSCALL_64_fastpath+0x1c/0x89
-> #5 (&sb->s_type->i_mutex_key#5){++++}:
lockref_get+0x9/0x20
-> #4 ((completion)&req.done){+.+.}:
wait_for_common+0x54/0x210
devtmpfs_create_node+0x130/0x150
device_add+0x5ad/0x5e0
device_create_groups_vargs+0xd4/0xe0
device_create+0x35/0x40
msr_device_create+0x22/0x40
cpuhp_invoke_callback+0xc5/0xbf0
cpuhp_thread_fun+0x167/0x210
smpboot_thread_fn+0x17f/0x270
kthread+0x173/0x1b0
ret_from_fork+0x24/0x30
-> #3 (cpuhp_state-up){+.+.}:
cpuhp_issue_call+0x132/0x1c0
__cpuhp_setup_state_cpuslocked+0x12f/0x2a0
__cpuhp_setup_state+0x3a/0x50
page_writeback_init+0x3a/0x5c
start_kernel+0x393/0x3e2
secondary_startup_64+0xa5/0xb0
-> #2 (cpuhp_state_mutex){+.+.}:
__mutex_lock+0x81/0x9b0
__cpuhp_setup_state_cpuslocked+0x4b/0x2a0
__cpuhp_setup_state+0x3a/0x50
page_alloc_init+0x1f/0x26
start_kernel+0x139/0x3e2
secondary_startup_64+0xa5/0xb0
-> #1 (cpu_hotplug_lock.rw_sem){++++}:
cpus_read_lock+0x34/0xa0
apply_workqueue_attrs+0xd/0x40
__alloc_workqueue_key+0x2c7/0x4e1
intel_guc_submission_init+0x10c/0x650 [i915]
intel_uc_init_hw+0x29e/0x460 [i915]
i915_gem_init_hw+0xca/0x290 [i915]
i915_gem_init+0x115/0x3a0 [i915]
i915_driver_load+0x9a8/0x16c0 [i915]
i915_pci_probe+0x2e/0x90 [i915]
pci_device_probe+0x9c/0x120
driver_probe_device+0x2a3/0x480
__driver_attach+0xd9/0xe0
bus_for_each_dev+0x57/0x90
bus_add_driver+0x168/0x260
driver_register+0x52/0xc0
do_one_initcall+0x39/0x150
do_init_module+0x56/0x1ef
load_module+0x231c/0x2d70
SyS_finit_module+0xa5/0xe0
entry_SYSCALL_64_fastpath+0x1c/0x89
-> #0 (&dev->struct_mutex){+.+.}:
lock_acquire+0xaf/0x200
__mutex_lock+0x81/0x9b0
i915_mutex_lock_interruptible+0x47/0x130 [i915]
i915_gem_fault+0x201/0x760 [i915]
__do_fault+0x15/0x70
__handle_mm_fault+0x85b/0xe40
handle_mm_fault+0x14f/0x2f0
__do_page_fault+0x2d1/0x560
page_fault+0x22/0x30
other info that might help us debug this:
Chain exists of:
&dev->struct_mutex --> &sb->s_type->i_mutex_key#5 --> &mm->mmap_sem
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem);
lock(&sb->s_type->i_mutex_key#5);
lock(&mm->mmap_sem);
lock(&dev->struct_mutex);
*** DEADLOCK ***
1 lock held by debugfs_test/1351:
#0: (&mm->mmap_sem){++++}, at: [<000000005df01c1e>] __do_page_fault+0x106/0x560
stack backtrace:
CPU: 2 PID: 1351 Comm: debugfs_test Not tainted 4.15.0-rc2-CI-Patchwork_7428+ #1
Hardware name: /NUC6i5SYB, BIOS SYSKLi35.86A.0057.2017.0119.1758 01/19/2017
Call Trace:
dump_stack+0x5f/0x86
print_circular_bug+0x230/0x3b0
check_prev_add+0x439/0x7b0
? lockdep_init_map_crosslock+0x20/0x20
? unwind_get_return_address+0x16/0x30
? __lock_acquire+0x1385/0x15a0
__lock_acquire+0x1385/0x15a0
lock_acquire+0xaf/0x200
? i915_mutex_lock_interruptible+0x47/0x130 [i915]
__mutex_lock+0x81/0x9b0
? i915_mutex_lock_interruptible+0x47/0x130 [i915]
? i915_mutex_lock_interruptible+0x47/0x130 [i915]
? i915_mutex_lock_interruptible+0x47/0x130 [i915]
i915_mutex_lock_interruptible+0x47/0x130 [i915]
? __pm_runtime_resume+0x4f/0x80
i915_gem_fault+0x201/0x760 [i915]
__do_fault+0x15/0x70
__handle_mm_fault+0x85b/0xe40
handle_mm_fault+0x14f/0x2f0
__do_page_fault+0x2d1/0x560
page_fault+0x22/0x30
RIP: 0033:0x7f98d6f49116
RSP: 002b:00007ffd6ffc3278 EFLAGS: 00010283
RAX: 00007f98d39a2bc0 RBX: 0000000000000000 RCX: 0000000000001680
RDX: 0000000000001680 RSI: 00007ffd6ffc3400 RDI: 00007f98d39a2bc0
RBP: 00007ffd6ffc33a0 R08: 0000000000000000 R09: 00000000000005a0
R10: 000055e847c2a830 R11: 0000000000000002 R12: 0000000000000001
R13: 000055e847c1d040 R14: 00007ffd6ffc3400 R15: 00007f98d6752ba0
Testcase: igt/debugfs_test/read_all_entries
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 1 +
drivers/gpu/drm/i915/i915_gem.c | 4 ++
drivers/gpu/drm/i915/intel_guc.c | 57 +++++++++++++++++++++++
drivers/gpu/drm/i915/intel_guc.h | 2 +
drivers/gpu/drm/i915/intel_guc_log.c | 23 ---------
drivers/gpu/drm/i915/intel_guc_submission.c | 72 ++++++++---------------------
drivers/gpu/drm/i915/intel_guc_submission.h | 2 +
drivers/gpu/drm/i915/intel_uc.c | 26 +++++++++++
drivers/gpu/drm/i915/intel_uc.h | 2 +
9 files changed, 112 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5b1fd5f1defb..53678031ed61 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -621,6 +621,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
i915_gem_contexts_fini(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
+ intel_uc_fini_wq(dev_priv);
i915_gem_cleanup_userptr(dev_priv);
i915_gem_drain_freed_objects(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fcc9b53864f0..4458a65ea580 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5151,6 +5151,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
if (ret)
return ret;
+ ret = intel_uc_init_wq(dev_priv);
+ if (ret)
+ return ret;
+
/* This is just a security blanket to placate dragons.
* On some systems, we very sporadically observe that the first TLBs
* used by the CS may be stale, despite us poking the TLB reset. If
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 92ed22f38fc4..3c6bf5a34c3c 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -69,6 +69,63 @@ void intel_guc_init_early(struct intel_guc *guc)
guc->notify = gen8_guc_raise_irq;
}
+int intel_guc_init_wq(struct intel_guc *guc)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+ /*
+ * GuC log buffer flush work item has to do register access to
+ * send the ack to GuC and this work item, if not synced before
+ * suspend, can potentially get executed after the GFX device is
+ * suspended.
+ * By marking the WQ as freezable, we don't have to bother about
+ * flushing of this work item from the suspend hooks, the pending
+ * work item if any will be either executed before the suspend
+ * or scheduled later on resume. This way the handling of work
+ * item can be kept same between system suspend & rpm suspend.
+ */
+ guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+ WQ_HIGHPRI | WQ_FREEZABLE);
+ if (!guc->log.runtime.flush_wq)
+ return -ENOMEM;
+
+ /*
+ * Even though both sending GuC action, and adding a new workitem to
+ * GuC workqueue are serialized (each with its own locking), since
+ * we're using mutliple engines, it's possible that we're going to
+ * issue a preempt request with two (or more - each for different
+ * engine) workitems in GuC queue. In this situation, GuC may submit
+ * all of them, which will make us very confused.
+ * Our preemption contexts may even already be complete - before we
+ * even had the chance to sent the preempt action to GuC!. Rather
+ * than introducing yet another lock, we can just use ordered workqueue
+ * to make sure we're always sending a single preemption request with a
+ * single workitem.
+ */
+ if (HAS_LOGICAL_RING_PREEMPTION(dev_priv) &&
+ USES_GUC_SUBMISSION(dev_priv)) {
+ guc->preempt_wq = alloc_ordered_workqueue("i915-guc_preempt",
+ WQ_HIGHPRI);
+ if (!guc->preempt_wq) {
+ destroy_workqueue(guc->log.runtime.flush_wq);
+ return -ENOMEM;
+ }
+ }
+
+ return 0;
+}
+
+void intel_guc_fini_wq(struct intel_guc *guc)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+ if (HAS_LOGICAL_RING_PREEMPTION(dev_priv) &&
+ USES_GUC_SUBMISSION(dev_priv))
+ destroy_workqueue(guc->preempt_wq);
+
+ destroy_workqueue(guc->log.runtime.flush_wq);
+}
+
static int guc_shared_data_create(struct intel_guc *guc)
{
struct i915_vma *vma;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 81659e223e11..52856a97477d 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_wq(struct intel_guc *guc);
+void intel_guc_fini_wq(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);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 1a2c5eed9929..eaedd63e3819 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -411,30 +411,8 @@ static int guc_log_runtime_create(struct intel_guc *guc)
guc->log.runtime.relay_chan = guc_log_relay_chan;
INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
-
- /*
- * GuC log buffer flush work item has to do register access to
- * send the ack to GuC and this work item, if not synced before
- * suspend, can potentially get executed after the GFX device is
- * suspended.
- * By marking the WQ as freezable, we don't have to bother about
- * flushing of this work item from the suspend hooks, the pending
- * work item if any will be either executed before the suspend
- * or scheduled later on resume. This way the handling of work
- * item can be kept same between system suspend & rpm suspend.
- */
- guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
- WQ_HIGHPRI | WQ_FREEZABLE);
- if (!guc->log.runtime.flush_wq) {
- DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
- ret = -ENOMEM;
- goto err_relaychan;
- }
-
return 0;
-err_relaychan:
- relay_close(guc->log.runtime.relay_chan);
err_vaddr:
i915_gem_object_unpin_map(guc->log.vma->obj);
guc->log.runtime.buf_addr = NULL;
@@ -450,7 +428,6 @@ static void guc_log_runtime_destroy(struct intel_guc *guc)
if (!guc_log_has_runtime(guc))
return;
- destroy_workqueue(guc->log.runtime.flush_wq);
relay_close(guc->log.runtime.relay_chan);
i915_gem_object_unpin_map(guc->log.vma->obj);
guc->log.runtime.buf_addr = NULL;
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index c020560c395e..1c0d0b09ac9e 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1187,57 +1187,15 @@ static void guc_ads_destroy(struct intel_guc *guc)
i915_vma_unpin_and_release(&guc->ads_vma);
}
-static int guc_preempt_work_create(struct intel_guc *guc)
-{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
- struct intel_engine_cs *engine;
- enum intel_engine_id id;
-
- /*
- * Even though both sending GuC action, and adding a new workitem to
- * GuC workqueue are serialized (each with its own locking), since
- * we're using mutliple engines, it's possible that we're going to
- * issue a preempt request with two (or more - each for different
- * engine) workitems in GuC queue. In this situation, GuC may submit
- * all of them, which will make us very confused.
- * Our preemption contexts may even already be complete - before we
- * even had the chance to sent the preempt action to GuC!. Rather
- * than introducing yet another lock, we can just use ordered workqueue
- * to make sure we're always sending a single preemption request with a
- * single workitem.
- */
- guc->preempt_wq = alloc_ordered_workqueue("i915-guc_preempt",
- WQ_HIGHPRI);
- if (!guc->preempt_wq)
- return -ENOMEM;
-
- for_each_engine(engine, dev_priv, id) {
- guc->preempt_work[id].engine = engine;
- INIT_WORK(&guc->preempt_work[id].work, inject_preempt_context);
- }
-
- return 0;
-}
-
-static void guc_preempt_work_destroy(struct intel_guc *guc)
-{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
- struct intel_engine_cs *engine;
- enum intel_engine_id id;
-
- for_each_engine(engine, dev_priv, id)
- cancel_work_sync(&guc->preempt_work[id].work);
-
- destroy_workqueue(guc->preempt_wq);
- guc->preempt_wq = NULL;
-}
-
/*
* Set up the memory resources to be shared with the GuC (via the GGTT)
* at firmware loading time.
*/
int intel_guc_submission_init(struct intel_guc *guc)
{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+ struct intel_engine_cs *engine;
+ enum intel_engine_id id;
int ret;
if (guc->stage_desc_pool)
@@ -1256,20 +1214,20 @@ int intel_guc_submission_init(struct intel_guc *guc)
if (ret < 0)
goto err_stage_desc_pool;
- ret = guc_preempt_work_create(guc);
- if (ret)
- goto err_log;
- GEM_BUG_ON(!guc->preempt_wq);
-
ret = guc_ads_create(guc);
if (ret < 0)
- goto err_wq;
+ goto err_log;
GEM_BUG_ON(!guc->ads_vma);
+ if (HAS_LOGICAL_RING_PREEMPTION(dev_priv)) {
+ for_each_engine(engine, dev_priv, id) {
+ guc->preempt_work[id].engine = engine;
+ INIT_WORK(&guc->preempt_work[id].work, inject_preempt_context);
+ }
+ }
+
return 0;
-err_wq:
- guc_preempt_work_destroy(guc);
err_log:
intel_guc_log_destroy(guc);
err_stage_desc_pool:
@@ -1279,8 +1237,14 @@ int intel_guc_submission_init(struct intel_guc *guc)
void intel_guc_submission_fini(struct intel_guc *guc)
{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+ struct intel_engine_cs *engine;
+ enum intel_engine_id id;
+
+ for_each_engine(engine, dev_priv, id)
+ cancel_work_sync(&guc->preempt_work[id].work);
+
guc_ads_destroy(guc);
- guc_preempt_work_destroy(guc);
intel_guc_log_destroy(guc);
guc_stage_desc_pool_destroy(guc);
}
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h b/drivers/gpu/drm/i915/intel_guc_submission.h
index 021fe85c8f71..fb081cefef93 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
@@ -77,5 +77,7 @@ int intel_guc_submission_init(struct intel_guc *guc);
int intel_guc_submission_enable(struct intel_guc *guc);
void intel_guc_submission_disable(struct intel_guc *guc);
void intel_guc_submission_fini(struct intel_guc *guc);
+int intel_guc_preempt_work_create(struct intel_guc *guc);
+void intel_guc_preempt_work_destroy(struct intel_guc *guc);
#endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 3040a0e00142..785850838a44 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -188,6 +188,32 @@ static void guc_disable_communication(struct intel_guc *guc)
guc->send = intel_guc_send_nop;
}
+int intel_uc_init_wq(struct drm_i915_private *dev_priv)
+{
+ int ret;
+
+ if (!USES_GUC(dev_priv))
+ return 0;
+
+ ret = intel_guc_init_wq(&dev_priv->guc);
+ if (ret) {
+ DRM_ERROR("Couldn't allocate workqueues for GuC\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+void intel_uc_fini_wq(struct drm_i915_private *dev_priv)
+{
+ if (!USES_GUC(dev_priv))
+ return;
+
+ GEM_BUG_ON(!HAS_GUC(dev_priv));
+
+ intel_guc_fini_wq(&dev_priv->guc);
+}
+
int intel_uc_init_hw(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7a59e2486e9e..53edfeaf56b0 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -33,6 +33,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv);
void intel_uc_init_mmio(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_wq(struct drm_i915_private *dev_priv);
+void intel_uc_fini_wq(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);
--
2.14.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw
2017-12-11 15:12 [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Michał Winiarski
2017-12-11 15:12 ` [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex Michał Winiarski
@ 2017-12-11 15:15 ` Michał Winiarski
2017-12-11 15:15 ` [PATCH 4/4] HAX Enable GuC Submission for CI Michał Winiarski
2017-12-11 15:37 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Chris Wilson
2017-12-11 15:17 ` [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Chris Wilson
2017-12-11 15:19 ` Chris Wilson
3 siblings, 2 replies; 8+ messages in thread
From: Michał Winiarski @ 2017-12-11 15:15 UTC (permalink / raw)
To: intel-gfx
After GPU reset, GuC HW needs to be reinitialized (with FW reload).
Unfortunately, we're doing some extra work there (mostly allocating stuff),
work that can be moved to guc_init and called once at driver load time.
As a side effect we're no longer hitting an assert in
i915_ggtt_enable_guc on suspend/resume.
References: 04f7b24eccdf ("drm/i915/guc: Assert that we switch between known ggtt->invalidate functions")
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>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 1 +
drivers/gpu/drm/i915/i915_gem.c | 4 +++
drivers/gpu/drm/i915/intel_uc.c | 68 ++++++++++++++++++++++++++++-------------
drivers/gpu/drm/i915/intel_uc.h | 2 ++
4 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 53678031ed61..bf5f010d8b47 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -617,6 +617,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
mutex_lock(&dev_priv->drm.struct_mutex);
intel_uc_fini_hw(dev_priv);
+ intel_uc_fini(dev_priv);
i915_gem_cleanup_engines(dev_priv);
i915_gem_contexts_fini(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4458a65ea580..6767f9d86588 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5178,6 +5178,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
intel_init_gt_powersave(dev_priv);
+ ret = intel_uc_init(dev_priv);
+ if (ret)
+ goto out_unlock;
+
ret = i915_gem_init_hw(dev_priv);
if (ret)
goto out_unlock;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 785850838a44..e12d690d79a2 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -214,26 +214,23 @@ void intel_uc_fini_wq(struct drm_i915_private *dev_priv)
intel_guc_fini_wq(&dev_priv->guc);
}
-int intel_uc_init_hw(struct drm_i915_private *dev_priv)
+int intel_uc_init(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
- struct intel_huc *huc = &dev_priv->huc;
- int ret, attempts;
+ int ret;
if (!USES_GUC(dev_priv))
return 0;
- if (!HAS_GUC(dev_priv)) {
- ret = -ENODEV;
- goto err_out;
- }
+ if (!HAS_GUC(dev_priv))
+ return -ENODEV;
guc_disable_communication(guc);
gen9_reset_guc_interrupts(dev_priv);
ret = intel_guc_init(guc);
if (ret)
- goto err_out;
+ return ret;
if (USES_GUC_SUBMISSION(dev_priv)) {
/*
@@ -241,10 +238,44 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
* if we are planning to enable submission later
*/
ret = intel_guc_submission_init(guc);
- if (ret)
- goto err_guc;
+ if (ret) {
+ intel_guc_fini(guc);
+ return ret;
+ }
}
+ return 0;
+}
+
+void intel_uc_fini(struct drm_i915_private *dev_priv)
+{
+ struct intel_guc *guc = &dev_priv->guc;
+
+ if (!USES_GUC(dev_priv))
+ return;
+
+ GEM_BUG_ON(!HAS_GUC(dev_priv));
+
+ if (USES_GUC_SUBMISSION(dev_priv))
+ intel_guc_submission_fini(guc);
+
+ intel_guc_fini(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 (!USES_GUC(dev_priv))
+ return 0;
+
+ GEM_BUG_ON(!HAS_GUC(dev_priv));
+
+ guc_disable_communication(guc);
+ gen9_reset_guc_interrupts(dev_priv);
+
/* init WOPCM */
I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
I915_WRITE(DMA_GUC_WOPCM_OFFSET,
@@ -264,12 +295,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
*/
ret = __intel_uc_reset_hw(dev_priv);
if (ret)
- goto err_submission;
+ goto err_out;
if (USES_HUC(dev_priv)) {
ret = intel_huc_init_hw(huc);
if (ret)
- goto err_submission;
+ goto err_out;
}
intel_guc_init_params(guc);
@@ -322,11 +353,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
guc_disable_communication(guc);
err_log_capture:
guc_capture_load_err_log(guc);
-err_submission:
- if (USES_GUC_SUBMISSION(dev_priv))
- intel_guc_submission_fini(guc);
-err_guc:
- intel_guc_fini(guc);
err_out:
/*
* Note that there is no fallback as either user explicitly asked for
@@ -348,15 +374,13 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
if (!USES_GUC(dev_priv))
return;
+ GEM_BUG_ON(!HAS_GUC(dev_priv));
+
if (USES_GUC_SUBMISSION(dev_priv))
intel_guc_submission_disable(guc);
guc_disable_communication(guc);
- if (USES_GUC_SUBMISSION(dev_priv)) {
+ if (USES_GUC_SUBMISSION(dev_priv))
gen9_disable_guc_interrupts(dev_priv);
- intel_guc_submission_fini(guc);
- }
-
- intel_guc_fini(guc);
}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 53edfeaf56b0..8a7249722ef1 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -37,6 +37,8 @@ int intel_uc_init_wq(struct drm_i915_private *dev_priv);
void intel_uc_fini_wq(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_uc_init(struct drm_i915_private *dev_priv);
+void intel_uc_fini(struct drm_i915_private *dev_priv);
static inline bool intel_uc_is_using_guc(void)
{
--
2.14.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/4] HAX Enable GuC Submission for CI
2017-12-11 15:15 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Michał Winiarski
@ 2017-12-11 15:15 ` Michał Winiarski
2017-12-11 15:37 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Chris Wilson
1 sibling, 0 replies; 8+ messages in thread
From: Michał Winiarski @ 2017-12-11 15:15 UTC (permalink / raw)
To: intel-gfx
---
drivers/gpu/drm/i915/i915_params.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 792ce26d7449..9725c5ad8ac6 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,7 +45,7 @@
param(int, disable_power_well, -1) \
param(int, enable_ips, 1) \
param(int, invert_brightness, 0) \
- param(int, enable_guc, 0) \
+ param(int, enable_guc, -1) \
param(int, guc_log_level, -1) \
param(char *, guc_firmware_path, NULL) \
param(char *, huc_firmware_path, NULL) \
--
2.14.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path
2017-12-11 15:12 [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Michał Winiarski
2017-12-11 15:12 ` [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex Michał Winiarski
2017-12-11 15:15 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Michał Winiarski
@ 2017-12-11 15:17 ` Chris Wilson
2017-12-11 15:19 ` Chris Wilson
3 siblings, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2017-12-11 15:17 UTC (permalink / raw)
To: Michał Winiarski, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path
2017-12-11 15:12 [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Michał Winiarski
` (2 preceding siblings ...)
2017-12-11 15:17 ` [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Chris Wilson
@ 2017-12-11 15:19 ` Chris Wilson
3 siblings, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2017-12-11 15:19 UTC (permalink / raw)
To: Michał Winiarski, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex
2017-12-11 15:12 ` [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex Michał Winiarski
@ 2017-12-11 15:26 ` Chris Wilson
0 siblings, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2017-12-11 15:26 UTC (permalink / raw)
To: Michał Winiarski, intel-gfx
Quoting Michał Winiarski (2017-12-11 15:12:54)
> @@ -1256,20 +1214,20 @@ int intel_guc_submission_init(struct intel_guc *guc)
> if (ret < 0)
> goto err_stage_desc_pool;
>
> - ret = guc_preempt_work_create(guc);
> - if (ret)
> - goto err_log;
> - GEM_BUG_ON(!guc->preempt_wq);
> -
> ret = guc_ads_create(guc);
> if (ret < 0)
> - goto err_wq;
> + goto err_log;
> GEM_BUG_ON(!guc->ads_vma);
>
> + if (HAS_LOGICAL_RING_PREEMPTION(dev_priv)) {
> + for_each_engine(engine, dev_priv, id) {
> + guc->preempt_work[id].engine = engine;
> + INIT_WORK(&guc->preempt_work[id].work, inject_preempt_context);
> + }
> + }
> +
> return 0;
>
> -err_wq:
> - guc_preempt_work_destroy(guc);
> err_log:
> intel_guc_log_destroy(guc);
> err_stage_desc_pool:
> @@ -1279,8 +1237,14 @@ int intel_guc_submission_init(struct intel_guc *guc)
>
> void intel_guc_submission_fini(struct intel_guc *guc)
> {
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> +
> + for_each_engine(engine, dev_priv, id)
> + cancel_work_sync(&guc->preempt_work[id].work);
> +
You should then unconditionally initialise the preempt_work[id].work as
well. Which isn't a problem as it is just pure initialisation and no
allocation.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw
2017-12-11 15:15 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Michał Winiarski
2017-12-11 15:15 ` [PATCH 4/4] HAX Enable GuC Submission for CI Michał Winiarski
@ 2017-12-11 15:37 ` Chris Wilson
1 sibling, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2017-12-11 15:37 UTC (permalink / raw)
To: Michał Winiarski, intel-gfx
Quoting Michał Winiarski (2017-12-11 15:15:20)
> After GPU reset, GuC HW needs to be reinitialized (with FW reload).
> Unfortunately, we're doing some extra work there (mostly allocating stuff),
> work that can be moved to guc_init and called once at driver load time.
>
> As a side effect we're no longer hitting an assert in
> i915_ggtt_enable_guc on suspend/resume.
>
> References: 04f7b24eccdf ("drm/i915/guc: Assert that we switch between known ggtt->invalidate functions")
> 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>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
> @@ -241,10 +238,44 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> * if we are planning to enable submission later
> */
> ret = intel_guc_submission_init(guc);
> - if (ret)
> - goto err_guc;
> + if (ret) {
> + intel_guc_fini(guc);
> + return ret;
> + }
> }
>
> + return 0;
> +}
> +
> +void intel_uc_fini(struct drm_i915_private *dev_priv)
I have a fondness for laying out the code in roughly the order it should
be used in the API, quite often reverse order to keep forward decls to a
minimum.
Looks like it is a clean split and does what you say on the tin,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-11 15:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-11 15:12 [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Michał Winiarski
2017-12-11 15:12 ` [PATCH 2/4] drm/i915/guc: Move GuC workqueue allocations outside of the mutex Michał Winiarski
2017-12-11 15:26 ` Chris Wilson
2017-12-11 15:15 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Michał Winiarski
2017-12-11 15:15 ` [PATCH 4/4] HAX Enable GuC Submission for CI Michał Winiarski
2017-12-11 15:37 ` [PATCH 3/4] drm/i915/guc: Extract guc_init from guc_init_hw Chris Wilson
2017-12-11 15:17 ` [PATCH 1/4] drm/i915/guc: Move shared data allocation away from submission path Chris Wilson
2017-12-11 15:19 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.