* [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
@ 2018-01-23 15:42 Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts Sagar Arun Kamble
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Sagar Arun Kamble @ 2018-01-23 15:42 UTC (permalink / raw)
To: intel-gfx
This patch fixes lockdep issue due to circular locking dependency of
struct_mutex, i_mutex_key, mmap_sem, relay_channels_mutex.
For GuC log relay channel we create debugfs file that requires i_mutex_key
lock and we are doing that under struct_mutex. So we introduced newer
dependency as:
&dev->struct_mutex --> &sb->s_type->i_mutex_key#3 --> &mm->mmap_sem
However, there is dependency from mmap_sem to struct_mutex. Hence we
separate the relay create/destroy operation from under struct_mutex.
======================================================
WARNING: possible circular locking dependency detected
4.15.0-rc6-CI-Patchwork_7614+ #1 Not tainted
------------------------------------------------------
debugfs_test/1388 is trying to acquire lock:
(&dev->struct_mutex){+.+.}, at: [<00000000d5e1d915>] i915_mutex_lock_interruptible+0x47/0x130 [i915]
but task is already holding lock:
(&mm->mmap_sem){++++}, at: [<0000000029a9c131>] __do_page_fault+0x106/0x560
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&mm->mmap_sem){++++}:
_copy_to_user+0x1e/0x70
filldir+0x8c/0xf0
dcache_readdir+0xeb/0x160
iterate_dir+0xdc/0x140
SyS_getdents+0xa0/0x130
entry_SYSCALL_64_fastpath+0x1c/0x89
-> #2 (&sb->s_type->i_mutex_key#3){++++}:
start_creating+0x59/0x110
__debugfs_create_file+0x2e/0xe0
relay_create_buf_file+0x62/0x80
relay_late_setup_files+0x84/0x250
guc_log_late_setup+0x4f/0x110 [i915]
i915_guc_log_register+0x32/0x40 [i915]
i915_driver_load+0x7b6/0x1720 [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
-> #1 (relay_channels_mutex){+.+.}:
relay_open+0x12c/0x2b0
intel_guc_log_runtime_create+0xab/0x230 [i915]
intel_guc_init+0x81/0x120 [i915]
intel_uc_init+0x29/0xa0 [i915]
i915_gem_init+0x182/0x530 [i915]
i915_driver_load+0xaa9/0x1720 [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){+.+.}:
__mutex_lock+0x81/0x9b0
i915_mutex_lock_interruptible+0x47/0x130 [i915]
i915_gem_fault+0x201/0x790 [i915]
__do_fault+0x15/0x70
__handle_mm_fault+0x677/0xdc0
handle_mm_fault+0x14f/0x2f0
__do_page_fault+0x2d1/0x560
page_fault+0x4c/0x60
other info that might help us debug this:
Chain exists of:
&dev->struct_mutex --> &sb->s_type->i_mutex_key#3 --> &mm->mmap_sem
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem);
lock(&sb->s_type->i_mutex_key#3);
lock(&mm->mmap_sem);
lock(&dev->struct_mutex);
*** DEADLOCK ***
1 lock held by debugfs_test/1388:
#0: (&mm->mmap_sem){++++}, at: [<0000000029a9c131>] __do_page_fault+0x106/0x560
stack backtrace:
CPU: 2 PID: 1388 Comm: debugfs_test Not tainted 4.15.0-rc6-CI-Patchwork_7614+ #1
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./J4205-ITX, BIOS P1.10 09/29/2016
Call Trace:
dump_stack+0x5f/0x86
print_circular_bug.isra.18+0x1d0/0x2c0
__lock_acquire+0x14ae/0x1b60
? lock_acquire+0xaf/0x200
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/0x790 [i915]
__do_fault+0x15/0x70
? _raw_spin_unlock+0x29/0x40
__handle_mm_fault+0x677/0xdc0
handle_mm_fault+0x14f/0x2f0
__do_page_fault+0x2d1/0x560
? page_fault+0x36/0x60
page_fault+0x4c/0x60
v2: Added lock protection to guc->log.runtime.relay_chan (Chris)
Fixed locking inside guc_flush_logs uncovered by new lockdep.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104693
Testcase: igt/debugfs_test/read_all_entries # with enable_guc=1 and guc_log_level=1
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 12 +--
drivers/gpu/drm/i915/i915_drv.c | 2 +-
drivers/gpu/drm/i915/i915_gem.c | 4 +-
drivers/gpu/drm/i915/intel_guc.c | 7 +-
drivers/gpu/drm/i915/intel_guc_log.c | 140 ++++++++++++++++++++++++++++-------
drivers/gpu/drm/i915/intel_guc_log.h | 11 +++
drivers/gpu/drm/i915/intel_uc.c | 31 ++++++--
drivers/gpu/drm/i915/intel_uc.h | 4 +-
8 files changed, 161 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 80dc679..b45be0d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2467,7 +2467,6 @@ static int i915_guc_log_control_get(void *data, u64 *val)
static int i915_guc_log_control_set(void *data, u64 val)
{
struct drm_i915_private *dev_priv = data;
- int ret;
if (!HAS_GUC(dev_priv))
return -ENODEV;
@@ -2475,16 +2474,7 @@ static int i915_guc_log_control_set(void *data, u64 val)
if (!dev_priv->guc.log.vma)
return -EINVAL;
- ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
- if (ret)
- return ret;
-
- intel_runtime_pm_get(dev_priv);
- ret = i915_guc_log_control(dev_priv, val);
- intel_runtime_pm_put(dev_priv);
-
- mutex_unlock(&dev_priv->drm.struct_mutex);
- return ret;
+ return i915_guc_log_control(dev_priv, val);
}
DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1fbe378..29048b9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -626,7 +626,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);
+ intel_uc_fini_misc(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 7f0684c..34d0d51 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5193,7 +5193,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- ret = intel_uc_init_wq(dev_priv);
+ ret = intel_uc_init_misc(dev_priv);
if (ret)
return ret;
@@ -5289,7 +5289,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
mutex_unlock(&dev_priv->drm.struct_mutex);
- intel_uc_fini_wq(dev_priv);
+ intel_uc_fini_misc(dev_priv);
if (ret != -EIO)
i915_gem_cleanup_userptr(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index ea30e7c..cab3c98 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -66,6 +66,7 @@ void intel_guc_init_early(struct intel_guc *guc)
intel_guc_ct_init_early(&guc->ct);
mutex_init(&guc->send_mutex);
+ mutex_init(&guc->log.runtime.relay_lock);
guc->send = intel_guc_send_nop;
guc->notify = gen8_guc_raise_irq;
}
@@ -87,8 +88,10 @@ int intel_guc_init_wq(struct intel_guc *guc)
*/
guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
WQ_HIGHPRI | WQ_FREEZABLE);
- if (!guc->log.runtime.flush_wq)
+ if (!guc->log.runtime.flush_wq) {
+ DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
return -ENOMEM;
+ }
/*
* Even though both sending GuC action, and adding a new workitem to
@@ -109,6 +112,8 @@ int intel_guc_init_wq(struct intel_guc *guc)
WQ_HIGHPRI);
if (!guc->preempt_wq) {
destroy_workqueue(guc->log.runtime.flush_wq);
+ DRM_ERROR("Couldn't allocate workqueue for GuC "
+ "preemption\n");
return -ENOMEM;
}
}
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 2ffc966..573d96d 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -181,6 +181,13 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
return 0;
}
+static bool guc_log_has_relay(struct intel_guc *guc)
+{
+ lockdep_assert_held(&guc->log.runtime.relay_lock);
+
+ return guc->log.runtime.relay_chan != NULL;
+}
+
static void guc_move_to_next_buf(struct intel_guc *guc)
{
/* Make sure the updates made in the sub buffer are visible when
@@ -188,6 +195,9 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
*/
smp_wmb();
+ if (!guc_log_has_relay(guc))
+ return;
+
/* All data has been written, so now move the offset of sub buffer. */
relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
@@ -197,7 +207,7 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
static void *guc_get_write_buffer(struct intel_guc *guc)
{
- if (!guc->log.runtime.relay_chan)
+ if (!guc_log_has_relay(guc))
return NULL;
/* Just get the base address of a new sub buffer and copy data into it
@@ -266,7 +276,9 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
log_buf_state = src_data = guc->log.runtime.buf_addr;
/* Get the pointer to local buffer to store the logs */
+ mutex_lock(&guc->log.runtime.relay_lock);
log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
/* Actual logs are present from the 2nd page */
src_data += PAGE_SIZE;
@@ -335,6 +347,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
dst_data += buffer_size;
}
+ mutex_lock(&guc->log.runtime.relay_lock);
if (log_buf_snapshot_state)
guc_move_to_next_buf(guc);
else {
@@ -344,6 +357,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
guc->log.capture_miss_count++;
}
+ mutex_unlock(&guc->log.runtime.relay_lock);
}
static void capture_logs_work(struct work_struct *work)
@@ -363,12 +377,12 @@ static int guc_log_runtime_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
void *vaddr;
- struct rchan *guc_log_relay_chan;
- size_t n_subbufs, subbuf_size;
int ret;
lockdep_assert_held(&dev_priv->drm.struct_mutex);
+ GEM_BUG_ON(!guc_log_has_relay(guc));
+
GEM_BUG_ON(guc_log_has_runtime(guc));
ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
@@ -387,8 +401,40 @@ static int guc_log_runtime_create(struct intel_guc *guc)
guc->log.runtime.buf_addr = vaddr;
+ INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
+
+ return 0;
+}
+
+static void guc_log_runtime_destroy(struct intel_guc *guc)
+{
+ /*
+ * It's possible that the runtime stuff was never allocated because
+ * GuC log was disabled at the boot time.
+ **/
+ if (!guc_log_has_runtime(guc))
+ return;
+
+ i915_gem_object_unpin_map(guc->log.vma->obj);
+ guc->log.runtime.buf_addr = NULL;
+}
+
+int intel_guc_log_relay_create(struct intel_guc *guc)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+ struct rchan *guc_log_relay_chan;
+ size_t n_subbufs, subbuf_size;
+ int ret;
+
+ if (!i915_modparams.guc_log_level)
+ return 0;
+
+ lockdep_assert_held(&guc->log.runtime.relay_lock);
+
+ GEM_BUG_ON(guc_log_has_relay(guc));
+
/* Keep the size of sub buffers same as shared log buffer */
- subbuf_size = guc->log.vma->obj->base.size;
+ subbuf_size = GUC_LOG_SIZE;
/* Store up to 8 snapshots, which is large enough to buffer sufficient
* boot time logs and provides enough leeway to User, in terms of
@@ -407,33 +453,33 @@ static int guc_log_runtime_create(struct intel_guc *guc)
DRM_ERROR("Couldn't create relay chan for GuC logging\n");
ret = -ENOMEM;
- goto err_vaddr;
+ goto err;
}
GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
guc->log.runtime.relay_chan = guc_log_relay_chan;
- INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
return 0;
-err_vaddr:
- i915_gem_object_unpin_map(guc->log.vma->obj);
- guc->log.runtime.buf_addr = NULL;
+err:
+ /* logging will be off */
+ i915_modparams.guc_log_level = 0;
return ret;
}
-static void guc_log_runtime_destroy(struct intel_guc *guc)
+void intel_guc_log_relay_destroy(struct intel_guc *guc)
{
+ lockdep_assert_held(&guc->log.runtime.relay_lock);
+
/*
- * It's possible that the runtime stuff was never allocated because
+ * It's possible that the relay was never allocated because
* GuC log was disabled at the boot time.
*/
- if (!guc_log_has_runtime(guc))
+ if (!guc_log_has_relay(guc))
return;
relay_close(guc->log.runtime.relay_chan);
- i915_gem_object_unpin_map(guc->log.vma->obj);
- guc->log.runtime.buf_addr = NULL;
+ guc->log.runtime.relay_chan = NULL;
}
static int guc_log_late_setup(struct intel_guc *guc)
@@ -441,27 +487,48 @@ static int guc_log_late_setup(struct intel_guc *guc)
struct drm_i915_private *dev_priv = guc_to_i915(guc);
int ret;
- lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
if (!guc_log_has_runtime(guc)) {
/*
* If log was disabled at boot time, then setup needed to handle
* log buffer flush interrupts would not have been done yet, so
* do that now.
*/
- ret = guc_log_runtime_create(guc);
+ GEM_BUG_ON(guc_log_has_relay(guc));
+
+ mutex_lock(&guc->log.runtime.relay_lock);
+ ret = intel_guc_log_relay_create(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
+
if (ret)
goto err;
+
+ mutex_lock(&dev_priv->drm.struct_mutex);
+ intel_runtime_pm_get(dev_priv);
+ ret = guc_log_runtime_create(guc);
+ intel_runtime_pm_put(dev_priv);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
+
+ if (ret)
+ goto err_relay;
}
+ mutex_lock(&guc->log.runtime.relay_lock);
ret = guc_log_relay_file_create(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
+
if (ret)
goto err_runtime;
return 0;
err_runtime:
+ mutex_lock(&dev_priv->drm.struct_mutex);
guc_log_runtime_destroy(guc);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
+err_relay:
+ mutex_lock(&guc->log.runtime.relay_lock);
+ intel_guc_log_relay_destroy(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
err:
/* logging will remain off */
i915_modparams.guc_log_level = 0;
@@ -490,7 +557,11 @@ static void guc_flush_logs(struct intel_guc *guc)
return;
/* First disable the interrupts, will be renabled afterwards */
+ mutex_lock(&dev_priv->drm.struct_mutex);
+ intel_runtime_pm_get(dev_priv);
gen9_disable_guc_interrupts(dev_priv);
+ intel_runtime_pm_put(dev_priv);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
/* Before initiating the forceful flush, wait for any pending/ongoing
* flush to complete otherwise forceful flush may not actually happen.
@@ -498,7 +569,11 @@ static void guc_flush_logs(struct intel_guc *guc)
flush_work(&guc->log.runtime.flush_work);
/* Ask GuC to update the log buffer state */
+ mutex_lock(&dev_priv->drm.struct_mutex);
+ intel_runtime_pm_get(dev_priv);
guc_log_flush(guc);
+ intel_runtime_pm_put(dev_priv);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
/* GuC would have updated log buffer by now, so capture it */
guc_log_capture_logs(guc);
@@ -509,17 +584,10 @@ int intel_guc_log_create(struct intel_guc *guc)
struct i915_vma *vma;
unsigned long offset;
u32 flags;
- u32 size;
int ret;
GEM_BUG_ON(guc->log.vma);
- /* The first page is to save log buffer state. Allocate one
- * extra page for others in case for overlap */
- size = (1 + GUC_LOG_DPC_PAGES + 1 +
- GUC_LOG_ISR_PAGES + 1 +
- GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
-
/* We require SSE 4.1 for fast reads from the GuC log buffer and
* it should be present on the chipsets supporting GuC based
* submisssions.
@@ -529,7 +597,7 @@ int intel_guc_log_create(struct intel_guc *guc)
goto err;
}
- vma = intel_guc_allocate_vma(guc, size);
+ vma = intel_guc_allocate_vma(guc, GUC_LOG_SIZE);
if (IS_ERR(vma)) {
ret = PTR_ERR(vma);
goto err;
@@ -584,7 +652,15 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
return 0;
verbosity = enable_logging ? control_val - 1 : 0;
+
+ ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
+ if (ret)
+ return ret;
+ intel_runtime_pm_get(dev_priv);
ret = guc_log_control(guc, enable_logging, verbosity);
+ intel_runtime_pm_put(dev_priv);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
+
if (ret < 0) {
DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
return ret;
@@ -605,7 +681,11 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
}
/* GuC logging is currently the only user of Guc2Host interrupts */
+ mutex_lock(&dev_priv->drm.struct_mutex);
+ intel_runtime_pm_get(dev_priv);
gen9_enable_guc_interrupts(dev_priv);
+ intel_runtime_pm_put(dev_priv);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
} else {
/*
* Once logging is disabled, GuC won't generate logs & send an
@@ -627,19 +707,23 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
return;
- mutex_lock(&dev_priv->drm.struct_mutex);
guc_log_late_setup(&dev_priv->guc);
- mutex_unlock(&dev_priv->drm.struct_mutex);
}
void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
{
+ struct intel_guc *guc = &dev_priv->guc;
+
if (!USES_GUC_SUBMISSION(dev_priv))
return;
mutex_lock(&dev_priv->drm.struct_mutex);
/* GuC logging is currently the only user of Guc2Host interrupts */
gen9_disable_guc_interrupts(dev_priv);
- guc_log_runtime_destroy(&dev_priv->guc);
+ guc_log_runtime_destroy(guc);
mutex_unlock(&dev_priv->drm.struct_mutex);
+
+ mutex_lock(&guc->log.runtime.relay_lock);
+ intel_guc_log_relay_destroy(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
}
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index f512cf7..378d489 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -32,6 +32,13 @@
struct drm_i915_private;
struct intel_guc;
+/*
+ * The first page is to save log buffer state. Allocate one
+ * extra page for others in case for overlap
+ */
+#define GUC_LOG_SIZE ((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \
+ 1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT)
+
struct intel_guc_log {
u32 flags;
struct i915_vma *vma;
@@ -41,6 +48,8 @@ struct intel_guc_log {
struct workqueue_struct *flush_wq;
struct work_struct flush_work;
struct rchan *relay_chan;
+ /* To serialize the access to relay_chan */
+ struct mutex relay_lock;
} runtime;
/* logging related stats */
u32 capture_miss_count;
@@ -52,6 +61,8 @@ struct intel_guc_log {
int intel_guc_log_create(struct intel_guc *guc);
void intel_guc_log_destroy(struct intel_guc *guc);
+int intel_guc_log_relay_create(struct intel_guc *guc);
+void intel_guc_log_relay_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);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index f78a17b..b119e94 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -236,28 +236,49 @@ 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 intel_uc_init_misc(struct drm_i915_private *dev_priv)
{
+ struct intel_guc *guc = &dev_priv->guc;
int ret;
if (!USES_GUC(dev_priv))
return 0;
- ret = intel_guc_init_wq(&dev_priv->guc);
+ ret = intel_guc_init_wq(guc);
if (ret) {
DRM_ERROR("Couldn't allocate workqueues for GuC\n");
- return ret;
+ goto err;
+ }
+
+ mutex_lock(&guc->log.runtime.relay_lock);
+ ret = intel_guc_log_relay_create(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
+
+ if (ret) {
+ DRM_ERROR("Couldn't allocate relay for GuC log\n");
+ goto err_relay;
}
return 0;
+
+err_relay:
+ intel_guc_fini_wq(guc);
+err:
+ return ret;
}
-void intel_uc_fini_wq(struct drm_i915_private *dev_priv)
+void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
{
+ struct intel_guc *guc = &dev_priv->guc;
+
if (!USES_GUC(dev_priv))
return;
- intel_guc_fini_wq(&dev_priv->guc);
+ intel_guc_fini_wq(guc);
+
+ mutex_lock(&guc->log.runtime.relay_lock);
+ intel_guc_log_relay_destroy(guc);
+ mutex_unlock(&guc->log.runtime.relay_lock);
}
int intel_uc_init(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 8a72497..f2984e0 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -33,8 +33,8 @@
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_misc(struct drm_i915_private *dev_priv);
+void intel_uc_fini_misc(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);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
@ 2018-01-23 15:42 ` Sagar Arun Kamble
2018-01-23 15:48 ` Chris Wilson
2018-01-23 15:42 ` [PATCH v2 3/4] drm/i915/guc: Enable interrupts before resuming GuC during runtime resume Sagar Arun Kamble
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Sagar Arun Kamble @ 2018-01-23 15:42 UTC (permalink / raw)
To: intel-gfx
Disabling GuC interrupts involves access to GuC IRQ control registers
hence ensure device is RPM awake.
v2: Add comment about need to synchronize flush work and log runtime
destroy
v3: Moved patch earlier in the series and removed comment about future
work. (Tvrtko)
v4-v5: Rebase.
v6: Added assert_rpm_wakelock_held() to gen9_*_guc_interrupts. (Chris)
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
drivers/gpu/drm/i915/intel_guc_log.c | 3 +++
2 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3517c65..85c46a2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -452,6 +452,8 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
{
+ assert_rpm_wakelock_held(dev_priv);
+
spin_lock_irq(&dev_priv->irq_lock);
gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
spin_unlock_irq(&dev_priv->irq_lock);
@@ -459,6 +461,8 @@ void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
{
+ assert_rpm_wakelock_held(dev_priv);
+
spin_lock_irq(&dev_priv->irq_lock);
if (!dev_priv->guc.interrupts_enabled) {
WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
@@ -471,6 +475,8 @@ void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
{
+ assert_rpm_wakelock_held(dev_priv);
+
spin_lock_irq(&dev_priv->irq_lock);
dev_priv->guc.interrupts_enabled = false;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 573d96d..be9f2ca 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -719,7 +719,10 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
mutex_lock(&dev_priv->drm.struct_mutex);
/* GuC logging is currently the only user of Guc2Host interrupts */
+ intel_runtime_pm_get(dev_priv);
gen9_disable_guc_interrupts(dev_priv);
+ intel_runtime_pm_put(dev_priv);
+
guc_log_runtime_destroy(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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] drm/i915/guc: Enable interrupts before resuming GuC during runtime resume
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts Sagar Arun Kamble
@ 2018-01-23 15:42 ` Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default" Sagar Arun Kamble
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Sagar Arun Kamble @ 2018-01-23 15:42 UTC (permalink / raw)
To: intel-gfx
GuC log streaming needs interrupts enabled prior to GuC resume but
runtime pm interrupt setup was happening post GuC resume. Fix it.
While at it, fix the unwinding of steps in the runtime suspend path.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104695
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 29048b9..1ec12ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2600,6 +2600,11 @@ static int intel_runtime_suspend(struct device *kdev)
intel_runtime_pm_enable_interrupts(dev_priv);
+ intel_guc_resume(dev_priv);
+
+ i915_gem_init_swizzling(dev_priv);
+ i915_gem_restore_fences(dev_priv);
+
enable_rpm_wakeref_asserts(dev_priv);
return ret;
@@ -2665,8 +2670,6 @@ 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);
-
if (IS_GEN9_LP(dev_priv)) {
bxt_disable_dc9(dev_priv);
bxt_display_core_init(dev_priv, true);
@@ -2681,6 +2684,10 @@ static int intel_runtime_resume(struct device *kdev)
intel_uncore_runtime_resume(dev_priv);
+ intel_runtime_pm_enable_interrupts(dev_priv);
+
+ intel_guc_resume(dev_priv);
+
/*
* No point of rolling back things in case of an error, as the best
* we can do is to hope that things will still work (and disable RPM).
@@ -2688,8 +2695,6 @@ static int intel_runtime_resume(struct device *kdev)
i915_gem_init_swizzling(dev_priv);
i915_gem_restore_fences(dev_priv);
- intel_runtime_pm_enable_interrupts(dev_priv);
-
/*
* On VLV/CHV display interrupts are part of the display
* power well, so hpd is reinitialized from there. For
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default"
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 3/4] drm/i915/guc: Enable interrupts before resuming GuC during runtime resume Sagar Arun Kamble
@ 2018-01-23 15:42 ` Sagar Arun Kamble
2018-01-23 16:06 ` Michal Wajdeczko
2018-01-23 15:46 ` [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Chris Wilson
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Sagar Arun Kamble @ 2018-01-23 15:42 UTC (permalink / raw)
To: intel-gfx
Comment DRM_ERROR_RATELIMIT outputs as log capturer won't be
running.
This reverts commit bd724318b682587ad2f989ab8e0f7b3d4486ced5.
---
drivers/gpu/drm/i915/i915_params.h | 2 +-
drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 430f5f9..c963603 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -48,7 +48,7 @@
param(int, enable_ips, 1) \
param(int, invert_brightness, 0) \
param(int, enable_guc, 0) \
- param(int, guc_log_level, 0) \
+ param(int, guc_log_level, -1) \
param(char *, guc_firmware_path, NULL) \
param(char *, huc_firmware_path, NULL) \
param(int, mmio_debug, 0) \
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index be9f2ca..7604451 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -238,7 +238,7 @@ static bool guc_check_log_buf_overflow(struct intel_guc *guc,
/* buffer_full_cnt is a 4 bit counter */
guc->log.total_overflow_count[type] += 16;
}
- DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
+ //DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
}
return overflow;
@@ -354,7 +354,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
/* Used rate limited to avoid deluge of messages, logs might be
* getting consumed by User at a slow rate.
*/
- DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
+ //DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
guc->log.capture_miss_count++;
}
mutex_unlock(&guc->log.runtime.relay_lock);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
` (2 preceding siblings ...)
2018-01-23 15:42 ` [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default" Sagar Arun Kamble
@ 2018-01-23 15:46 ` Chris Wilson
2018-01-23 16:01 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] " Patchwork
2018-01-23 16:44 ` [PATCH v2 1/4] " Michal Wajdeczko
5 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2018-01-23 15:46 UTC (permalink / raw)
To: Sagar Arun Kamble, intel-gfx
Quoting Sagar Arun Kamble (2018-01-23 15:42:17)
> static void *guc_get_write_buffer(struct intel_guc *guc)
> {
> - if (!guc->log.runtime.relay_chan)
> + if (!guc_log_has_relay(guc))
> return NULL;
>
> /* Just get the base address of a new sub buffer and copy data into it
> @@ -266,7 +276,9 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> log_buf_state = src_data = guc->log.runtime.buf_addr;
>
> /* Get the pointer to local buffer to store the logs */
> + mutex_lock(&guc->log.runtime.relay_lock);
> log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> + mutex_unlock(&guc->log.runtime.relay_lock);
Since dst_data/log_buf_snapshot_state are pointing into the relay_chan
they are only valid underneath the relay_lock (we don't have any
references or whatnot).
> /* Actual logs are present from the 2nd page */
> src_data += PAGE_SIZE;
> @@ -335,6 +347,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> dst_data += buffer_size;
> }
>
> + mutex_lock(&guc->log.runtime.relay_lock);
> if (log_buf_snapshot_state)
> guc_move_to_next_buf(guc);
> else {
> @@ -344,6 +357,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
> guc->log.capture_miss_count++;
> }
> + mutex_unlock(&guc->log.runtime.relay_lock);
I.e. it's no good just reacquiring the lock as the state may have
perished in between.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
2018-01-23 15:42 ` [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts Sagar Arun Kamble
@ 2018-01-23 15:48 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2018-01-23 15:48 UTC (permalink / raw)
To: Sagar Arun Kamble, intel-gfx
Quoting Sagar Arun Kamble (2018-01-23 15:42:18)
> Disabling GuC interrupts involves access to GuC IRQ control registers
> hence ensure device is RPM awake.
>
> v2: Add comment about need to synchronize flush work and log runtime
> destroy
>
> v3: Moved patch earlier in the series and removed comment about future
> work. (Tvrtko)
>
> v4-v5: Rebase.
>
> v6: Added assert_rpm_wakelock_held() to gen9_*_guc_interrupts. (Chris)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Looks self-consistent, so let's hope it doesn't generate any warnings ;)
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] 11+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
` (3 preceding siblings ...)
2018-01-23 15:46 ` [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Chris Wilson
@ 2018-01-23 16:01 ` Patchwork
2018-01-23 16:44 ` [PATCH v2 1/4] " Michal Wajdeczko
5 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-01-23 16:01 UTC (permalink / raw)
To: Sagar Arun Kamble; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
URL : https://patchwork.freedesktop.org/series/36980/
State : warning
== Summary ==
Series 36980v1 series starting with [v2,1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
https://patchwork.freedesktop.org/api/1.0/series/36980/revisions/1/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-fail -> PASS (fi-elk-e7500) fdo#103989 +2
Test drv_module_reload:
Subgroup basic-reload:
pass -> DMESG-WARN (fi-skl-guc)
Subgroup basic-no-display:
pass -> DMESG-WARN (fi-skl-guc)
Subgroup basic-reload-inject:
pass -> DMESG-WARN (fi-skl-guc)
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:421s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:429s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:380s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:494s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:487s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:489s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:474s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:455s
fi-elk-e7500 total:224 pass:169 dwarn:9 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:284s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:516s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:394s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:400s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:415s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:448s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:412s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:462s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:498s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:502s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:584s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:428s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:509s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:529s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:494s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:491s
fi-skl-guc total:288 pass:257 dwarn:3 dfail:0 fail:0 skip:28 time:420s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:432s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:527s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:403s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:577s
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:475s
cbdcbeb1eb0965d3b9afc88561e9da7e11ce1d82 drm-tip: 2018y-01m-23d-13h-00m-40s UTC integration manifest
a6ff60a6b193 Revert "drm/i915/guc: Keep GuC log disabled by default"
5a10169f1ca1 drm/i915/guc: Enable interrupts before resuming GuC during runtime resume
aaf3f3a257bf drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
95958514665d drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7752/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default"
2018-01-23 15:42 ` [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default" Sagar Arun Kamble
@ 2018-01-23 16:06 ` Michal Wajdeczko
2018-01-23 16:32 ` Sagar Arun Kamble
0 siblings, 1 reply; 11+ messages in thread
From: Michal Wajdeczko @ 2018-01-23 16:06 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Tue, 23 Jan 2018 16:42:20 +0100, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> Comment DRM_ERROR_RATELIMIT outputs as log capturer won't be
> running.
>
> This reverts commit bd724318b682587ad2f989ab8e0f7b3d4486ced5.
> ---
> drivers/gpu/drm/i915/i915_params.h | 2 +-
> drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 430f5f9..c963603 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -48,7 +48,7 @@
> param(int, enable_ips, 1) \
> param(int, invert_brightness, 0) \
> param(int, enable_guc, 0) \
> - param(int, guc_log_level, 0) \
> + param(int, guc_log_level, -1) \
If this is HAX for CI then OK, but otherwise maybe we
should leave log disabled(0) to match disabled(0) GuC ;)
Then in the future we can change both to auto(-1)
> param(char *, guc_firmware_path, NULL) \
> param(char *, huc_firmware_path, NULL) \
> param(int, mmio_debug, 0) \
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index be9f2ca..7604451 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -238,7 +238,7 @@ static bool guc_check_log_buf_overflow(struct
> intel_guc *guc,
> /* buffer_full_cnt is a 4 bit counter */
> guc->log.total_overflow_count[type] += 16;
> }
> - DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
> + //DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
I'm not sure that we should leave dead code
And using C++ style comments is not good too
(unless this patch is just a HAX)
> }
> return overflow;
> @@ -354,7 +354,7 @@ static void guc_read_update_log_buffer(struct
> intel_guc *guc)
> /* Used rate limited to avoid deluge of messages, logs might be
> * getting consumed by User at a slow rate.
> */
> - DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
> + //DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
> guc->log.capture_miss_count++;
> }
> mutex_unlock(&guc->log.runtime.relay_lock);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default"
2018-01-23 16:06 ` Michal Wajdeczko
@ 2018-01-23 16:32 ` Sagar Arun Kamble
0 siblings, 0 replies; 11+ messages in thread
From: Sagar Arun Kamble @ 2018-01-23 16:32 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 1/23/2018 9:36 PM, Michal Wajdeczko wrote:
> On Tue, 23 Jan 2018 16:42:20 +0100, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> Comment DRM_ERROR_RATELIMIT outputs as log capturer won't be
>> running.
>>
>> This reverts commit bd724318b682587ad2f989ab8e0f7b3d4486ced5.
>> ---
>> drivers/gpu/drm/i915/i915_params.h | 2 +-
>> drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.h
>> b/drivers/gpu/drm/i915/i915_params.h
>> index 430f5f9..c963603 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -48,7 +48,7 @@
>> param(int, enable_ips, 1) \
>> param(int, invert_brightness, 0) \
>> param(int, enable_guc, 0) \
>> - param(int, guc_log_level, 0) \
>> + param(int, guc_log_level, -1) \
>
> If this is HAX for CI then OK, but otherwise maybe we
> should leave log disabled(0) to match disabled(0) GuC ;)
>
> Then in the future we can change both to auto(-1)
>
Yes. This is HAX. Missed tag in this rev.
>> param(char *, guc_firmware_path, NULL) \
>> param(char *, huc_firmware_path, NULL) \
>> param(int, mmio_debug, 0) \
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index be9f2ca..7604451 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -238,7 +238,7 @@ static bool guc_check_log_buf_overflow(struct
>> intel_guc *guc,
>> /* buffer_full_cnt is a 4 bit counter */
>> guc->log.total_overflow_count[type] += 16;
>> }
>> - DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
>> + //DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
>
> I'm not sure that we should leave dead code
> And using C++ style comments is not good too
> (unless this patch is just a HAX)
>
>> }
>> return overflow;
>> @@ -354,7 +354,7 @@ static void guc_read_update_log_buffer(struct
>> intel_guc *guc)
>> /* Used rate limited to avoid deluge of messages, logs might be
>> * getting consumed by User at a slow rate.
>> */
>> - DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
>> + //DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
>> guc->log.capture_miss_count++;
>> }
>> mutex_unlock(&guc->log.runtime.relay_lock);
--
Thanks,
Sagar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
` (4 preceding siblings ...)
2018-01-23 16:01 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] " Patchwork
@ 2018-01-23 16:44 ` Michal Wajdeczko
2018-01-23 16:48 ` Chris Wilson
5 siblings, 1 reply; 11+ messages in thread
From: Michal Wajdeczko @ 2018-01-23 16:44 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Tue, 23 Jan 2018 16:42:17 +0100, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> This patch fixes lockdep issue due to circular locking dependency of
> struct_mutex, i_mutex_key, mmap_sem, relay_channels_mutex.
> For GuC log relay channel we create debugfs file that requires
> i_mutex_key
> lock and we are doing that under struct_mutex. So we introduced newer
> dependency as:
> &dev->struct_mutex --> &sb->s_type->i_mutex_key#3 --> &mm->mmap_sem
> However, there is dependency from mmap_sem to struct_mutex. Hence we
> separate the relay create/destroy operation from under struct_mutex.
>
... <snip>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 80dc679..b45be0d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2467,7 +2467,6 @@ static int i915_guc_log_control_get(void *data,
> u64 *val)
> static int i915_guc_log_control_set(void *data, u64 val)
> {
> struct drm_i915_private *dev_priv = data;
> - int ret;
> if (!HAS_GUC(dev_priv))
> return -ENODEV;
> @@ -2475,16 +2474,7 @@ static int i915_guc_log_control_set(void *data,
> u64 val)
> if (!dev_priv->guc.log.vma)
> return -EINVAL;
> - ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> - if (ret)
> - return ret;
> -
> - intel_runtime_pm_get(dev_priv);
> - ret = i915_guc_log_control(dev_priv, val);
> - intel_runtime_pm_put(dev_priv);
> -
> - mutex_unlock(&dev_priv->drm.struct_mutex);
> - return ret;
> + return i915_guc_log_control(dev_priv, val);
I hope that one day we change signature of this function to
int intel_guc_log_control(struct intel_guc *guc, u64 control);
... <snip>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index ea30e7c..cab3c98 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -66,6 +66,7 @@ void intel_guc_init_early(struct intel_guc *guc)
> intel_guc_ct_init_early(&guc->ct);
> mutex_init(&guc->send_mutex);
> + mutex_init(&guc->log.runtime.relay_lock);
Maybe this can be done in intel_guc_loc.c (or .h) as
void intel_guc_log_init_early() { }
> guc->send = intel_guc_send_nop;
> guc->notify = gen8_guc_raise_irq;
> }
> @@ -87,8 +88,10 @@ int intel_guc_init_wq(struct intel_guc *guc)
> */
> guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> WQ_HIGHPRI | WQ_FREEZABLE);
> - if (!guc->log.runtime.flush_wq)
> + if (!guc->log.runtime.flush_wq) {
> + DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
> return -ENOMEM;
> + }
> /*
> * Even though both sending GuC action, and adding a new workitem to
> @@ -109,6 +112,8 @@ int intel_guc_init_wq(struct intel_guc *guc)
> WQ_HIGHPRI);
> if (!guc->preempt_wq) {
> destroy_workqueue(guc->log.runtime.flush_wq);
> + DRM_ERROR("Couldn't allocate workqueue for GuC "
> + "preemption\n");
> return -ENOMEM;
> }
> }
... <snip>
> static void capture_logs_work(struct work_struct *work)
> @@ -363,12 +377,12 @@ static int guc_log_runtime_create(struct intel_guc
> *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> void *vaddr;
> - struct rchan *guc_log_relay_chan;
> - size_t n_subbufs, subbuf_size;
> int ret;
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
> + GEM_BUG_ON(!guc_log_has_relay(guc));
> +
Do we need this line?
> GEM_BUG_ON(guc_log_has_runtime(guc));
> ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
> @@ -387,8 +401,40 @@ static int guc_log_runtime_create(struct intel_guc
> *guc)
> guc->log.runtime.buf_addr = vaddr;
> + INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
I'm not sure about other BKMs, but I prefer to not delay such
initialization
and perform them in functions like init_early
> +
> + return 0;
> +}
> +
> +static void guc_log_runtime_destroy(struct intel_guc *guc)
> +{
> + /*
> + * It's possible that the runtime stuff was never allocated because
> + * GuC log was disabled at the boot time.
> + **/
Is this correct comment style ?
> + if (!guc_log_has_runtime(guc))
> + return;
> +
> + i915_gem_object_unpin_map(guc->log.vma->obj);
> + guc->log.runtime.buf_addr = NULL;
> +}
> +
... <snip>
> @@ -605,7 +681,11 @@ int i915_guc_log_control(struct drm_i915_private
> *dev_priv, u64 control_val)
> }
> /* GuC logging is currently the only user of Guc2Host interrupts */
> + mutex_lock(&dev_priv->drm.struct_mutex);
> + intel_runtime_pm_get(dev_priv);
> gen9_enable_guc_interrupts(dev_priv);
> + intel_runtime_pm_put(dev_priv);
> + mutex_unlock(&dev_priv->drm.struct_mutex);
Maybe pm_get/lock/xxx/unlock/pm_put would be better order ?
... <snip>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index f78a17b..b119e94 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -236,28 +236,49 @@ 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 intel_uc_init_misc(struct drm_i915_private *dev_priv)
> {
> + struct intel_guc *guc = &dev_priv->guc;
> int ret;
> if (!USES_GUC(dev_priv))
> return 0;
> - ret = intel_guc_init_wq(&dev_priv->guc);
> + ret = intel_guc_init_wq(guc);
> if (ret) {
> DRM_ERROR("Couldn't allocate workqueues for GuC\n");
> - return ret;
> + goto err;
> + }
> +
Hmm, maybe below code
> + mutex_lock(&guc->log.runtime.relay_lock);
> + ret = intel_guc_log_relay_create(guc);
> + mutex_unlock(&guc->log.runtime.relay_lock);
> +
> + if (ret) {
> + DRM_ERROR("Couldn't allocate relay for GuC log\n");
> + goto err_relay;
can be done in intel_guc_log.c as kind of init function ?
> }
> return 0;
> +
> +err_relay:
> + intel_guc_fini_wq(guc);
> +err:
> + return ret;
> }
> -void intel_uc_fini_wq(struct drm_i915_private *dev_priv)
> +void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
> {
> + struct intel_guc *guc = &dev_priv->guc;
> +
> if (!USES_GUC(dev_priv))
> return;
> - intel_guc_fini_wq(&dev_priv->guc);
> + intel_guc_fini_wq(guc);
> +
> + mutex_lock(&guc->log.runtime.relay_lock);
> + intel_guc_log_relay_destroy(guc);
> + mutex_unlock(&guc->log.runtime.relay_lock);
Maybe lock/unlock can be done inside intel_guc_log_relay_destroy ?
same with intel_guc_log_relay_create.
> }
> int intel_uc_init(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index 8a72497..f2984e0 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -33,8 +33,8 @@
> 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_misc(struct drm_i915_private *dev_priv);
> +void intel_uc_fini_misc(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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
2018-01-23 16:44 ` [PATCH v2 1/4] " Michal Wajdeczko
@ 2018-01-23 16:48 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2018-01-23 16:48 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx, Sagar Arun Kamble
Quoting Michal Wajdeczko (2018-01-23 16:44:33)
> On Tue, 23 Jan 2018 16:42:17 +0100, Sagar Arun Kamble
> > @@ -605,7 +681,11 @@ int i915_guc_log_control(struct drm_i915_private
> > *dev_priv, u64 control_val)
> > }
> > /* GuC logging is currently the only user of Guc2Host interrupts */
> > + mutex_lock(&dev_priv->drm.struct_mutex);
> > + intel_runtime_pm_get(dev_priv);
> > gen9_enable_guc_interrupts(dev_priv);
> > + intel_runtime_pm_put(dev_priv);
> > + mutex_unlock(&dev_priv->drm.struct_mutex);
>
> Maybe pm_get/lock/xxx/unlock/pm_put would be better order ?
There's no strong reason to prefer one nesting or the other. So pick a
pattern for guc_interrupts and stick to it :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-01-23 16:48 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-23 15:42 [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 2/4] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts Sagar Arun Kamble
2018-01-23 15:48 ` Chris Wilson
2018-01-23 15:42 ` [PATCH v2 3/4] drm/i915/guc: Enable interrupts before resuming GuC during runtime resume Sagar Arun Kamble
2018-01-23 15:42 ` [PATCH v2 4/4] Revert "drm/i915/guc: Keep GuC log disabled by default" Sagar Arun Kamble
2018-01-23 16:06 ` Michal Wajdeczko
2018-01-23 16:32 ` Sagar Arun Kamble
2018-01-23 15:46 ` [PATCH v2 1/4] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex Chris Wilson
2018-01-23 16:01 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] " Patchwork
2018-01-23 16:44 ` [PATCH v2 1/4] " Michal Wajdeczko
2018-01-23 16:48 ` 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.