[PATCH v3 1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[PATCH v3 1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[Sagar Arun Kamble]

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 2ffc966..8f2da30 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -639,7 +639,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(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
-- 
1.9.1

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

[PATCH v3 2/6] drm/i915/guc: Enable interrupts before resuming GuC during runtime resume

[Sagar Arun Kamble]

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 1fbe378..95e1c16 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

[PATCH v3 3/6] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex

[Sagar Arun Kamble]

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.

v3: Locking guc_read_update_log_buffer entirely with relay_lock. (Chris)
    Prepared intel_guc_init_early. Moved relay_lock inside relay_create
    relay_destroy, relay_file_create, guc_read_update_log_buffer. (Michal)
    Removed struct_mutex lock around guc_log_flush and removed usage
    of guc_log_has_relay() from runtime_create path as it needs
    struct_mutex lock.

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 | 147 +++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_guc_log.h |  12 +++
 drivers/gpu/drm/i915/intel_uc.c      |  26 +++++--
 drivers/gpu/drm/i915/intel_uc.h      |   4 +-
 8 files changed, 160 insertions(+), 54 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 95e1c16..1ec12ad 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..21140cc 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -64,6 +64,7 @@ void intel_guc_init_early(struct intel_guc *guc)
 {
 	intel_guc_fw_init_early(guc);
 	intel_guc_ct_init_early(&guc->ct);
+	intel_guc_log_init_early(guc);
 
 	mutex_init(&guc->send_mutex);
 	guc->send = intel_guc_send_nop;
@@ -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 8f2da30..35de889 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -153,6 +153,8 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 	if (!i915_modparams.guc_log_level)
 		return 0;
 
+	mutex_lock(&guc->log.runtime.relay_lock);
+
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
 
@@ -169,16 +171,26 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 	 */
 	if (!log_dir) {
 		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out_unlock;
 	}
 
 	ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
 	if (ret < 0 && ret != -EEXIST) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
-		return ret;
+		goto out_unlock;
 	}
 
-	return 0;
+out_unlock:
+	mutex_unlock(&guc->log.runtime.relay_lock);
+	return ret;
+}
+
+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)
@@ -188,6 +200,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 +212,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
@@ -265,6 +280,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	/* Get the pointer to shared GuC log buffer */
 	log_buf_state = src_data = guc->log.runtime.buf_addr;
 
+	mutex_lock(&guc->log.runtime.relay_lock);
+
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
 
@@ -344,6 +361,8 @@ 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,8 +382,6 @@ 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);
@@ -387,8 +404,44 @@ static int guc_log_runtime_create(struct intel_guc *guc)
 
 	guc->log.runtime.buf_addr = vaddr;
 
+	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;
+}
+
+void intel_guc_log_init_early(struct intel_guc *guc)
+{
+	mutex_init(&guc->log.runtime.relay_lock);
+	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
+}
+
+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;
+
+	mutex_lock(&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 +460,39 @@ 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);
+	mutex_unlock(&guc->log.runtime.relay_lock);
+
 	return 0;
 
-err_vaddr:
-	i915_gem_object_unpin_map(guc->log.vma->obj);
-	guc->log.runtime.buf_addr = NULL;
+err:
+	mutex_unlock(&guc->log.runtime.relay_lock);
+	/* 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)
 {
+	mutex_lock(&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))
-		return;
+	if (!guc_log_has_relay(guc))
+		goto out_unlock;
 
 	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;
+
+out_unlock:
+	mutex_unlock(&guc->log.runtime.relay_lock);
 }
 
 static int guc_log_late_setup(struct intel_guc *guc)
@@ -441,17 +500,24 @@ 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);
+		ret = intel_guc_log_relay_create(guc);
 		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;
 	}
 
 	ret = guc_log_relay_file_create(guc);
@@ -461,7 +527,11 @@ static int guc_log_late_setup(struct intel_guc *guc)
 	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:
+	intel_guc_log_relay_destroy(guc);
 err:
 	/* logging will remain off */
 	i915_modparams.guc_log_level = 0;
@@ -490,7 +560,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 +572,9 @@ static void guc_flush_logs(struct intel_guc *guc)
 	flush_work(&guc->log.runtime.flush_work);
 
 	/* Ask GuC to update the log buffer state */
+	intel_runtime_pm_get(dev_priv);
 	guc_log_flush(guc);
+	intel_runtime_pm_put(dev_priv);
 
 	/* GuC would have updated log buffer by now, so capture it */
 	guc_log_capture_logs(guc);
@@ -509,17 +585,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 +598,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 +653,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 +682,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,13 +708,13 @@ 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;
 
@@ -643,6 +724,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	gen9_disable_guc_interrupts(dev_priv);
 	intel_runtime_pm_put(dev_priv);
 
-	guc_log_runtime_destroy(&dev_priv->guc);
+	guc_log_runtime_destroy(guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	intel_guc_log_relay_destroy(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index f512cf7..c638b9d 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,9 @@ struct intel_guc_log {
 
 int intel_guc_log_create(struct intel_guc *guc);
 void intel_guc_log_destroy(struct intel_guc *guc);
+void intel_guc_log_init_early(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..e3f3509 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -236,28 +236,44 @@ 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;
+	}
+
+	ret = intel_guc_log_relay_create(guc);
+	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);
+
+	intel_guc_log_relay_destroy(guc);
 }
 
 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

[PATCH v3 4/6] drm/i915/guc: Update name and prototype of i915_guc_log_control

[Sagar Arun Kamble]

i915_guc_log_control is GuC interface and GuC APIs that are not user
facing should be named with "intel_guc" prefix hence we change name to
intel_guc_log_control. Also changed the parameter to intel_guc struct.

Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 5 +++--
 drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
 drivers/gpu/drm/i915/intel_guc_log.h | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b45be0d..c10a9e8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2467,14 +2467,15 @@ 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;
+	struct intel_guc *guc = &dev_priv->guc;
 
 	if (!HAS_GUC(dev_priv))
 		return -ENODEV;
 
-	if (!dev_priv->guc.log.vma)
+	if (!guc->log.vma)
 		return -EINVAL;
 
-	return i915_guc_log_control(dev_priv, val);
+	return intel_guc_log_control(guc, val);
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 35de889..27eb545 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -637,9 +637,9 @@ void intel_guc_log_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
-int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
+int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	bool enable_logging = control_val > 0;
 	u32 verbosity;
 	int ret;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index c638b9d..dab0e94 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -64,7 +64,7 @@ struct intel_guc_log {
 void intel_guc_log_init_early(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);
+int intel_guc_log_control(struct intel_guc *guc, 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);
 
-- 
1.9.1

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

[PATCH v3 5/6] drm/i915/guc: Fix comments style in intel_guc_log.c

[Sagar Arun Kamble]

Use consistent multi-line comment style as per guideline.

Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 49 +++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 27eb545..7c6c41b 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -30,7 +30,7 @@
 
 static void guc_log_capture_logs(struct intel_guc *guc);
 
-/**
+/*
  * DOC: GuC firmware log
  *
  * Firmware log is enabled by setting i915.guc_log_level to the positive level.
@@ -81,7 +81,8 @@ static int subbuf_start_callback(struct rchan_buf *buf,
 				 void *prev_subbuf,
 				 size_t prev_padding)
 {
-	/* Use no-overwrite mode by default, where relay will stop accepting
+	/*
+	 * Use no-overwrite mode by default, where relay will stop accepting
 	 * new data if there are no empty sub buffers left.
 	 * There is no strict synchronization enforced by relay between Consumer
 	 * and Producer. In overwrite mode, there is a possibility of getting
@@ -107,7 +108,8 @@ static struct dentry *create_buf_file_callback(const char *filename,
 {
 	struct dentry *buf_file;
 
-	/* This to enable the use of a single buffer for the relay channel and
+	/*
+	 * This to enable the use of a single buffer for the relay channel and
 	 * correspondingly have a single file exposed to User, through which
 	 * it can collect the logs in order without any post-processing.
 	 * Need to set 'is_global' even if parent is NULL for early logging.
@@ -117,7 +119,8 @@ static struct dentry *create_buf_file_callback(const char *filename,
 	if (!parent)
 		return NULL;
 
-	/* Not using the channel filename passed as an argument, since for each
+	/*
+	 * Not using the channel filename passed as an argument, since for each
 	 * channel relay appends the corresponding CPU number to the filename
 	 * passed in relay_open(). This should be fine as relay just needs a
 	 * dentry of the file associated with the channel buffer and that file's
@@ -158,7 +161,8 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
 
-	/* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is
+	/*
+	 * If /sys/kernel/debug/dri/0 location do not exist, then debugfs is
 	 * not mounted and so can't create the relay file.
 	 * The relay API seems to fit well with debugfs only, for availing relay
 	 * there are 3 requirements which can be met for debugfs file only in a
@@ -195,7 +199,8 @@ static bool guc_log_has_relay(struct intel_guc *guc)
 
 static void guc_move_to_next_buf(struct intel_guc *guc)
 {
-	/* Make sure the updates made in the sub buffer are visible when
+	/*
+	 * Make sure the updates made in the sub buffer are visible when
 	 * Consumer sees the following update to offset inside the sub buffer.
 	 */
 	smp_wmb();
@@ -215,7 +220,8 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
 	if (!guc_log_has_relay(guc))
 		return NULL;
 
-	/* Just get the base address of a new sub buffer and copy data into it
+	/*
+	 * Just get the base address of a new sub buffer and copy data into it
 	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
 	 * buffers are full. Could have used the relay_write() to indirectly
 	 * copy the data, but that would have been bit convoluted, as we need to
@@ -290,7 +296,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	dst_data += PAGE_SIZE;
 
 	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
-		/* Make a copy of the state structure, inside GuC log buffer
+		/*
+		 * Make a copy of the state structure, inside GuC log buffer
 		 * (which is uncached mapped), on the stack to avoid reading
 		 * from it multiple times.
 		 */
@@ -317,7 +324,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		memcpy(log_buf_snapshot_state, &log_buf_state_local,
 		       sizeof(struct guc_log_buffer_state));
 
-		/* The write pointer could have been updated by GuC firmware,
+		/*
+		 * The write pointer could have been updated by GuC firmware,
 		 * after sending the flush interrupt to Host, for consistency
 		 * set write pointer value to same value of sampled_write_ptr
 		 * in the snapshot buffer.
@@ -355,7 +363,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	if (log_buf_snapshot_state)
 		guc_move_to_next_buf(guc);
 	else {
-		/* Used rate limited to avoid deluge of messages, logs might be
+		/*
+		 * 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");
@@ -392,7 +401,8 @@ static int guc_log_runtime_create(struct intel_guc *guc)
 	if (ret)
 		return ret;
 
-	/* Create a WC (Uncached for read) vmalloc mapping of log
+	/*
+	 * Create a WC (Uncached for read) vmalloc mapping of log
 	 * buffer pages, so that we can directly get the data
 	 * (up-to-date) from memory.
 	 */
@@ -412,7 +422,7 @@ 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;
 
@@ -443,14 +453,16 @@ int intel_guc_log_relay_create(struct intel_guc *guc)
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = GUC_LOG_SIZE;
 
-	/* Store up to 8 snapshots, which is large enough to buffer sufficient
+	/*
+	 * Store up to 8 snapshots, which is large enough to buffer sufficient
 	 * boot time logs and provides enough leeway to User, in terms of
 	 * latency, for consuming the logs from relay. Also doesn't take
 	 * up too much memory.
 	 */
 	n_subbufs = 8;
 
-	/* Create a relay channel, so that we have buffers for storing
+	/*
+	 * Create a relay channel, so that we have buffers for storing
 	 * the GuC firmware logs, the channel will be linked with a file
 	 * later on when debugfs is registered.
 	 */
@@ -544,7 +556,8 @@ static void guc_log_capture_logs(struct intel_guc *guc)
 
 	guc_read_update_log_buffer(guc);
 
-	/* Generally device is expected to be active only at this
+	/*
+	 * Generally device is expected to be active only at this
 	 * time, so get/put should be really quick.
 	 */
 	intel_runtime_pm_get(dev_priv);
@@ -566,7 +579,8 @@ static void guc_flush_logs(struct intel_guc *guc)
 	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-	/* Before initiating the forceful flush, wait for any pending/ongoing
+	/*
+	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
 	flush_work(&guc->log.runtime.flush_work);
@@ -589,7 +603,8 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	GEM_BUG_ON(guc->log.vma);
 
-	/* We require SSE 4.1 for fast reads from the GuC log buffer and
+	/*
+	 * 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.
 	 */
-- 
1.9.1

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

[PATCH v3 6/6] [HAX] Revert "drm/i915/guc: Keep GuC log disabled by default"

[Sagar Arun Kamble]

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 7c6c41b..a076d29 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -249,7 +249,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;
@@ -367,7 +367,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++;
 	}
 
-- 
1.9.1

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

✓ Fi.CI.BAT: success for series starting with [v3,1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
URL   : https://patchwork.freedesktop.org/series/37011/
State : success

== Summary ==

Series 37011v1 series starting with [v3,1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
https://patchwork.freedesktop.org/api/1.0/series/37011/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989 +2
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                skip       -> PASS       (fi-ivb-3520m)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
                pass       -> INCOMPLETE (fi-bdw-5557u) fdo#104162

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104162 https://bugs.freedesktop.org/show_bug.cgi?id=104162

fi-bdw-5557u     total:245  pass:227  dwarn:0   dfail:0   fail:0   skip:17 
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:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:490s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:281s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:491s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:466s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:452s
fi-elk-e7500     total:224  pass:168  dwarn:9   dfail:1   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:509s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:391s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:398s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:417s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:415s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:461s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:498s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:453s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:512s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:526s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:485s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:416s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:516s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:397s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:474s
fi-pnv-d510 failed to collect. IGT log at Patchwork_7765/fi-pnv-d510/igt.log

f9207df88e2a2a5803e1ddee7738c1a1aa2fcb74 drm-tip: 2018y-01m-23d-23h-25m-55s UTC integration manifest
b6c18893698e Revert "drm/i915/guc: Keep GuC log disabled by default"
a4fb4792fbd2 drm/i915/guc: Fix comments style in intel_guc_log.c
a0cb15d86c6f drm/i915/guc: Update name and prototype of i915_guc_log_control
29d907ffd97c drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex
92c7090822b3 drm/i915/guc: Enable interrupts before resuming GuC during runtime resume
045185e800e7 drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [v3,1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/6] drm/i915/guc: Grab RPM wakelock while disabling GuC interrupts
URL   : https://patchwork.freedesktop.org/series/37011/
State : success

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                fail       -> PASS       (shard-snb) fdo#101623 +1
Test perf:
        Subgroup buffer-fill:
                fail       -> PASS       (shard-apl) fdo#103755
        Subgroup oa-exponents:
                fail       -> PASS       (shard-apl) fdo#102254
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-atomic:
                fail       -> PASS       (shard-hsw) fdo#102670
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:2753 pass:1716 dwarn:1   dfail:0   fail:22  skip:1013 time:14033s
shard-hsw        total:2753 pass:1726 dwarn:1   dfail:0   fail:10  skip:1015 time:15392s
shard-snb        total:2753 pass:1318 dwarn:1   dfail:0   fail:11  skip:1423 time:7900s
Blacklisted hosts:
shard-kbl        total:2753 pass:1840 dwarn:1   dfail:0   fail:22  skip:890 time:11051s

== Logs ==

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

Re: [PATCH v3 5/6] drm/i915/guc: Fix comments style in intel_guc_log.c

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-24 04:09:11)
> Use consistent multi-line comment style as per guideline.
> 
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc_log.c | 49 +++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 27eb545..7c6c41b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -30,7 +30,7 @@
>  
>  static void guc_log_capture_logs(struct intel_guc *guc);
>  
> -/**
> +/*
>   * DOC: GuC firmware log
>   *
>   * Firmware log is enabled by setting i915.guc_log_level to the positive level.

The double ** here indicates a kerneldoc. So this one is correct.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/6] drm/i915/guc: Fix comments style in intel_guc_log.c

[Michal Wajdeczko]

On Wed, 24 Jan 2018 10:49:02 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Sagar Arun Kamble (2018-01-24 04:09:11)
>> Use consistent multi-line comment style as per guideline.
>>
>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_log.c | 49  
>> +++++++++++++++++++++++-------------
>>  1 file changed, 32 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 27eb545..7c6c41b 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -30,7 +30,7 @@
>>
>>  static void guc_log_capture_logs(struct intel_guc *guc);
>>
>> -/**
>> +/*
>>   * DOC: GuC firmware log
>>   *
>>   * Firmware log is enabled by setting i915.guc_log_level to the  
>> positive level.
>
> The double ** here indicates a kerneldoc. So this one is correct.

The one that is wrong is in guc_log_runtime_destroy()
as it ends with **/

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

Re: [PATCH v3 5/6] drm/i915/guc: Fix comments style in intel_guc_log.c

[Sagar Arun Kamble]

On 1/24/2018 3:20 PM, Michal Wajdeczko wrote:
> On Wed, 24 Jan 2018 10:49:02 +0100, Chris Wilson 
> <chris@chris-wilson.co.uk> wrote:
>
>> Quoting Sagar Arun Kamble (2018-01-24 04:09:11)
>>> Use consistent multi-line comment style as per guideline.
>>>
>>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>  drivers/gpu/drm/i915/intel_guc_log.c | 49 
>>> +++++++++++++++++++++++-------------
>>>  1 file changed, 32 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>> index 27eb545..7c6c41b 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>> @@ -30,7 +30,7 @@
>>>
>>>  static void guc_log_capture_logs(struct intel_guc *guc);
>>>
>>> -/**
>>> +/*
>>>   * DOC: GuC firmware log
>>>   *
>>>   * Firmware log is enabled by setting i915.guc_log_level to the 
>>> positive level.
>>
>> The double ** here indicates a kerneldoc. So this one is correct.
>
yes. will update.
> The one that is wrong is in guc_log_runtime_destroy()
> as it ends with **/
>
yes. that one got introduced by patch in series. will fix.
> Michal

-- 
Thanks,
Sagar

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

Re: [PATCH v3 4/6] drm/i915/guc: Update name and prototype of i915_guc_log_control

[Michal Wajdeczko]

On Wed, 24 Jan 2018 05:09:10 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> i915_guc_log_control is GuC interface and GuC APIs that are not user
> facing should be named with "intel_guc" prefix hence we change name to
> intel_guc_log_control. Also changed the parameter to intel_guc struct.
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

with small bikeshed below

>  drivers/gpu/drm/i915/i915_debugfs.c  | 5 +++--
>  drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
>  drivers/gpu/drm/i915/intel_guc_log.h | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index b45be0d..c10a9e8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2467,14 +2467,15 @@ 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;
> +	struct intel_guc *guc = &dev_priv->guc;
> 	if (!HAS_GUC(dev_priv))
>  		return -ENODEV;
> -	if (!dev_priv->guc.log.vma)
> +	if (!guc->log.vma)
>  		return -EINVAL;

Hmm, as this looks like internal check, maybe better to move
it into intel_guc_log_control() ?

Also, I'm not sure that lack of internal vma should be reported
as -EINVAL

> -	return i915_guc_log_control(dev_priv, val);
> +	return intel_guc_log_control(guc, val);
>  }
> DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 35de889..27eb545 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -637,9 +637,9 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>  	i915_vma_unpin_and_release(&guc->log.vma);
>  }
> -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64  
> control_val)
> +int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
>  {
> -	struct intel_guc *guc = &dev_priv->guc;
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  	bool enable_logging = control_val > 0;
>  	u32 verbosity;
>  	int ret;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index c638b9d..dab0e94 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -64,7 +64,7 @@ struct intel_guc_log {
>  void intel_guc_log_init_early(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);
> +int intel_guc_log_control(struct intel_guc *guc, 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/6] drm/i915/guc: Update name and prototype of i915_guc_log_control

[Chris Wilson]

Quoting Michal Wajdeczko (2018-01-24 10:07:12)
> On Wed, 24 Jan 2018 05:09:10 +0100, Sagar Arun Kamble  
> <sagar.a.kamble@intel.com> wrote:
> 
> > i915_guc_log_control is GuC interface and GuC APIs that are not user
> > facing should be named with "intel_guc" prefix hence we change name to
> > intel_guc_log_control. Also changed the parameter to intel_guc struct.
> >
> > Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> 
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> with small bikeshed below
> 
> >  drivers/gpu/drm/i915/i915_debugfs.c  | 5 +++--
> >  drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
> >  drivers/gpu/drm/i915/intel_guc_log.h | 2 +-
> >  3 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index b45be0d..c10a9e8 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2467,14 +2467,15 @@ 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;
> > +     struct intel_guc *guc = &dev_priv->guc;
> >       if (!HAS_GUC(dev_priv))
> >               return -ENODEV;
> > -     if (!dev_priv->guc.log.vma)
> > +     if (!guc->log.vma)
> >               return -EINVAL;
> 
> Hmm, as this looks like internal check, maybe better to move
> it into intel_guc_log_control() ?
> 
> Also, I'm not sure that lack of internal vma should be reported
> as -EINVAL

Right, it's not reporting that the user's parameter was wrong, just that
the device wasn't initialised, so ENODEV seems appropriate.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/6] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-24 04:09:09)
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 8f2da30..35de889 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -153,6 +153,8 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>         if (!i915_modparams.guc_log_level)
>                 return 0;
>  
> +       mutex_lock(&guc->log.runtime.relay_lock);
> +
>         /* For now create the log file in /sys/kernel/debug/dri/0 dir */
>         log_dir = dev_priv->drm.primary->debugfs_root;
>  
> @@ -169,16 +171,26 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>          */
>         if (!log_dir) {
>                 DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
> -               return -ENODEV;
> +               ret = -ENODEV;
> +               goto out_unlock;
>         }
>  
>         ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
>         if (ret < 0 && ret != -EEXIST) {
>                 DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> -               return ret;
> +               goto out_unlock;
>         }
>  
> -       return 0;
> +out_unlock:
> +       mutex_unlock(&guc->log.runtime.relay_lock);
> +       return ret;
> +}
> +
> +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)
> @@ -188,6 +200,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 +212,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
> @@ -265,6 +280,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>         /* Get the pointer to shared GuC log buffer */
>         log_buf_state = src_data = guc->log.runtime.buf_addr;
>  
> +       mutex_lock(&guc->log.runtime.relay_lock);
> +
>         /* Get the pointer to local buffer to store the logs */
>         log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);

Hmm. The locking here tells me that we are being careful in case the
relay_chan disappears, but we don't handle the NULL pointer here.
 
> @@ -344,6 +361,8 @@ 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,8 +382,6 @@ 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);
> @@ -387,8 +404,44 @@ static int guc_log_runtime_create(struct intel_guc *guc)
>  
>         guc->log.runtime.buf_addr = vaddr;
>  
> +       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;
> +}
> +
> +void intel_guc_log_init_early(struct intel_guc *guc)
> +{
> +       mutex_init(&guc->log.runtime.relay_lock);
> +       INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
> +}
> +
> +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;
> +
> +       mutex_lock(&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 +460,39 @@ 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);
> +       mutex_unlock(&guc->log.runtime.relay_lock);
> +
>         return 0;
>  
> -err_vaddr:
> -       i915_gem_object_unpin_map(guc->log.vma->obj);
> -       guc->log.runtime.buf_addr = NULL;
> +err:
> +       mutex_unlock(&guc->log.runtime.relay_lock);
> +       /* 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)
>  {
> +       mutex_lock(&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))
> -               return;
> +       if (!guc_log_has_relay(guc))
> +               goto out_unlock;
>  
>         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;
> +
> +out_unlock:
> +       mutex_unlock(&guc->log.runtime.relay_lock);
>  }
>  
>  static int guc_log_late_setup(struct intel_guc *guc)
> @@ -441,17 +500,24 @@ 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);
> +               ret = intel_guc_log_relay_create(guc);
>                 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;
>         }
>  
>         ret = guc_log_relay_file_create(guc);
> @@ -461,7 +527,11 @@ static int guc_log_late_setup(struct intel_guc *guc)
>         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:
> +       intel_guc_log_relay_destroy(guc);
>  err:
>         /* logging will remain off */
>         i915_modparams.guc_log_level = 0;
> @@ -490,7 +560,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 +572,9 @@ static void guc_flush_logs(struct intel_guc *guc)
>         flush_work(&guc->log.runtime.flush_work);
>  
>         /* Ask GuC to update the log buffer state */
> +       intel_runtime_pm_get(dev_priv);
>         guc_log_flush(guc);
> +       intel_runtime_pm_put(dev_priv);
>  
>         /* GuC would have updated log buffer by now, so capture it */
>         guc_log_capture_logs(guc);
> @@ -509,17 +585,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 +598,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 +653,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 +682,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,13 +708,13 @@ 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;
>  
> @@ -643,6 +724,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>         gen9_disable_guc_interrupts(dev_priv);
>         intel_runtime_pm_put(dev_priv);
>  
> -       guc_log_runtime_destroy(&dev_priv->guc);
> +       guc_log_runtime_destroy(guc);
>         mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +       intel_guc_log_relay_destroy(guc);
>  }

This looks all reasonably well described by the addition of the
relay_lock and the interactions look fine. The only mistake I could see,
in the story told by this patch, was the runtime checking.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/6] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex

[Sagar Arun Kamble]

On 1/24/2018 3:48 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-01-24 04:09:09)
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 8f2da30..35de889 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -153,6 +153,8 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>>          if (!i915_modparams.guc_log_level)
>>                  return 0;
>>   
>> +       mutex_lock(&guc->log.runtime.relay_lock);
>> +
>>          /* For now create the log file in /sys/kernel/debug/dri/0 dir */
>>          log_dir = dev_priv->drm.primary->debugfs_root;
>>   
>> @@ -169,16 +171,26 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>>           */
>>          if (!log_dir) {
>>                  DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
>> -               return -ENODEV;
>> +               ret = -ENODEV;
>> +               goto out_unlock;
>>          }
>>   
>>          ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
>>          if (ret < 0 && ret != -EEXIST) {
>>                  DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
>> -               return ret;
>> +               goto out_unlock;
>>          }
>>   
>> -       return 0;
>> +out_unlock:
>> +       mutex_unlock(&guc->log.runtime.relay_lock);
>> +       return ret;
>> +}
>> +
>> +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)
>> @@ -188,6 +200,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 +212,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
>> @@ -265,6 +280,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>>          /* Get the pointer to shared GuC log buffer */
>>          log_buf_state = src_data = guc->log.runtime.buf_addr;
>>   
>> +       mutex_lock(&guc->log.runtime.relay_lock);
>> +
>>          /* Get the pointer to local buffer to store the logs */
>>          log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> Hmm. The locking here tells me that we are being careful in case the
> relay_chan disappears, but we don't handle the NULL pointer here.
>   
There is check for log_bug_snapshot_state below in for loop. But yes, we 
should return from here.
Will update.
>> @@ -344,6 +361,8 @@ 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,8 +382,6 @@ 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);
>> @@ -387,8 +404,44 @@ static int guc_log_runtime_create(struct intel_guc *guc)
>>   
>>          guc->log.runtime.buf_addr = vaddr;
>>   
>> +       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;
>> +}
>> +
>> +void intel_guc_log_init_early(struct intel_guc *guc)
>> +{
>> +       mutex_init(&guc->log.runtime.relay_lock);
>> +       INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
>> +}
>> +
>> +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;
>> +
>> +       mutex_lock(&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 +460,39 @@ 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);
>> +       mutex_unlock(&guc->log.runtime.relay_lock);
>> +
>>          return 0;
>>   
>> -err_vaddr:
>> -       i915_gem_object_unpin_map(guc->log.vma->obj);
>> -       guc->log.runtime.buf_addr = NULL;
>> +err:
>> +       mutex_unlock(&guc->log.runtime.relay_lock);
>> +       /* 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)
>>   {
>> +       mutex_lock(&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))
>> -               return;
>> +       if (!guc_log_has_relay(guc))
>> +               goto out_unlock;
>>   
>>          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;
>> +
>> +out_unlock:
>> +       mutex_unlock(&guc->log.runtime.relay_lock);
>>   }
>>   
>>   static int guc_log_late_setup(struct intel_guc *guc)
>> @@ -441,17 +500,24 @@ 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);
>> +               ret = intel_guc_log_relay_create(guc);
>>                  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;
>>          }
>>   
>>          ret = guc_log_relay_file_create(guc);
>> @@ -461,7 +527,11 @@ static int guc_log_late_setup(struct intel_guc *guc)
>>          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:
>> +       intel_guc_log_relay_destroy(guc);
>>   err:
>>          /* logging will remain off */
>>          i915_modparams.guc_log_level = 0;
>> @@ -490,7 +560,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 +572,9 @@ static void guc_flush_logs(struct intel_guc *guc)
>>          flush_work(&guc->log.runtime.flush_work);
>>   
>>          /* Ask GuC to update the log buffer state */
>> +       intel_runtime_pm_get(dev_priv);
>>          guc_log_flush(guc);
>> +       intel_runtime_pm_put(dev_priv);
>>   
>>          /* GuC would have updated log buffer by now, so capture it */
>>          guc_log_capture_logs(guc);
>> @@ -509,17 +585,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 +598,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 +653,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 +682,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,13 +708,13 @@ 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;
>>   
>> @@ -643,6 +724,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>          gen9_disable_guc_interrupts(dev_priv);
>>          intel_runtime_pm_put(dev_priv);
>>   
>> -       guc_log_runtime_destroy(&dev_priv->guc);
>> +       guc_log_runtime_destroy(guc);
>>          mutex_unlock(&dev_priv->drm.struct_mutex);
>> +
>> +       intel_guc_log_relay_destroy(guc);
>>   }
> This looks all reasonably well described by the addition of the
> relay_lock and the interactions look fine. The only mistake I could see,
> in the story told by this patch, was the runtime checking.
Could you please elaborate more on this.
> -Chris

-- 
Thanks,
Sagar

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

Re: [PATCH v3 4/6] drm/i915/guc: Update name and prototype of i915_guc_log_control

[Sagar Arun Kamble]

On 1/24/2018 3:41 PM, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2018-01-24 10:07:12)
>> On Wed, 24 Jan 2018 05:09:10 +0100, Sagar Arun Kamble
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> i915_guc_log_control is GuC interface and GuC APIs that are not user
>>> facing should be named with "intel_guc" prefix hence we change name to
>>> intel_guc_log_control. Also changed the parameter to intel_guc struct.
>>>
>>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> with small bikeshed below
>>
>>>   drivers/gpu/drm/i915/i915_debugfs.c  | 5 +++--
>>>   drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
>>>   drivers/gpu/drm/i915/intel_guc_log.h | 2 +-
>>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index b45be0d..c10a9e8 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2467,14 +2467,15 @@ 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;
>>> +     struct intel_guc *guc = &dev_priv->guc;
>>>        if (!HAS_GUC(dev_priv))
>>>                return -ENODEV;
>>> -     if (!dev_priv->guc.log.vma)
>>> +     if (!guc->log.vma)
>>>                return -EINVAL;
>> Hmm, as this looks like internal check, maybe better to move
>> it into intel_guc_log_control() ?
>>
>> Also, I'm not sure that lack of internal vma should be reported
>> as -EINVAL
> Right, it's not reporting that the user's parameter was wrong, just that
> the device wasn't initialised, so ENODEV seems appropriate.
Thanks for the review and inputs.
> -Chris

-- 
Thanks,
Sagar

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

Re: [PATCH v3 3/6] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-24 10:52:28)
> 
> 
> On 1/24/2018 3:48 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2018-01-24 04:09:09)
> >> @@ -197,7 +212,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
> >> @@ -265,6 +280,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> >>          /* Get the pointer to shared GuC log buffer */
> >>          log_buf_state = src_data = guc->log.runtime.buf_addr;
> >>   
> >> +       mutex_lock(&guc->log.runtime.relay_lock);
> >> +
> >>          /* Get the pointer to local buffer to store the logs */
> >>          log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> > Hmm. The locking here tells me that we are being careful in case the
> > relay_chan disappears, but we don't handle the NULL pointer here.
> >   
> There is check for log_bug_snapshot_state below in for loop. But yes, we 
> should return from here.
> Will update.
> >> @@ -643,6 +724,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> >>          gen9_disable_guc_interrupts(dev_priv);
> >>          intel_runtime_pm_put(dev_priv);
> >>   
> >> -       guc_log_runtime_destroy(&dev_priv->guc);
> >> +       guc_log_runtime_destroy(guc);
> >>          mutex_unlock(&dev_priv->drm.struct_mutex);
> >> +
> >> +       intel_guc_log_relay_destroy(guc);
> >>   }
> > This looks all reasonably well described by the addition of the
> > relay_lock and the interactions look fine. The only mistake I could see,
> > in the story told by this patch, was the runtime checking.
> Could you please elaborate more on this.

The previous comment :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx