[PATCH v3 01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[PATCH v3 01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Michał Winiarski]

The GuC log contains a separate space used for crash dump.
We even get a separate notification for it. While we're not handling
crash differently yet, it makes sense to decouple the two right now to
simplify the following patches.

v2: Move guc_log_flush_irq_disable up to avoid movement in following
    patches (Sagar).
v3: s/guc_log_flush_irq_*/guc_flush_log_msg_*, rebase after mass rename

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v2)
---
 drivers/gpu/drm/i915/intel_guc.c     | 25 ++++++++++---------------
 drivers/gpu/drm/i915/intel_guc.h     |  2 ++
 drivers/gpu/drm/i915/intel_guc_log.c | 31 +++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_uc.c      | 14 +++++---------
 4 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index e70bf654d21e..3af603536b1b 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -67,6 +67,7 @@ void intel_guc_init_early(struct intel_guc *guc)
 	intel_guc_log_init_early(&guc->log);
 
 	mutex_init(&guc->send_mutex);
+	spin_lock_init(&guc->irq_lock);
 	guc->send = intel_guc_send_nop;
 	guc->notify = gen8_guc_raise_irq;
 }
@@ -368,7 +369,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 void intel_guc_to_host_event_handler(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	u32 msg, flush;
+	u32 msg, val;
 
 	/*
 	 * Sample the log buffer flush related bits & clear them out now
@@ -381,24 +382,18 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
 	 * could happen that GuC sets the bit for 2nd interrupt but Host
 	 * clears out the bit on handling the 1st interrupt.
 	 */
-
-	msg = I915_READ(SOFT_SCRATCH(15));
-	flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
-		       INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
-	if (flush) {
-		/* Clear the message bits that are handled */
-		I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
-
-		/* Handle flush interrupt in bottom half */
+	spin_lock(&guc->irq_lock);
+	val = I915_READ(SOFT_SCRATCH(15));
+	msg = val & guc->msg_enabled_mask;
+	I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
+	spin_unlock(&guc->irq_lock);
+
+	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
+		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
 		queue_work(guc->log.runtime.flush_wq,
 			   &guc->log.runtime.flush_work);
 
 		guc->log.flush_interrupt_count++;
-	} else {
-		/*
-		 * Not clearing of unhandled event bits won't result in
-		 * re-triggering of the interrupt.
-		 */
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index cdb649a9a4cf..9a95d1518aa9 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -56,7 +56,9 @@ struct intel_guc {
 	struct drm_i915_gem_object *load_err_log;
 
 	/* intel_guc_recv interrupt related state */
+	spinlock_t irq_lock;
 	bool interrupts_enabled;
+	unsigned int msg_enabled_mask;
 
 	struct i915_vma *ads_vma;
 	struct i915_vma *stage_desc_pool;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 1c2127bc3878..1e209fcf90e1 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -73,6 +73,22 @@ static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+static void guc_flush_log_msg_enable(struct intel_guc *guc)
+{
+	spin_lock_irq(&guc->irq_lock);
+	guc->msg_enabled_mask |= INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
+				 INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED;
+	spin_unlock_irq(&guc->irq_lock);
+}
+
+static void guc_flush_log_msg_disable(struct intel_guc *guc)
+{
+	spin_lock_irq(&guc->irq_lock);
+	guc->msg_enabled_mask &= ~(INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
+				   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED);
+	spin_unlock_irq(&guc->irq_lock);
+}
+
 static inline struct intel_guc *log_to_guc(struct intel_guc_log *log)
 {
 	return container_of(log, struct intel_guc, log);
@@ -709,12 +725,7 @@ int intel_guc_log_register(struct intel_guc_log *log)
 	if (ret)
 		goto err_runtime;
 
-	/* GuC logging is currently the only user of Guc2Host interrupts */
-	mutex_lock(&i915->drm.struct_mutex);
-	intel_runtime_pm_get(i915);
-	gen9_enable_guc_interrupts(i915);
-	intel_runtime_pm_put(i915);
-	mutex_unlock(&i915->drm.struct_mutex);
+	guc_flush_log_msg_enable(guc);
 
 	return 0;
 
@@ -733,6 +744,8 @@ void intel_guc_log_unregister(struct intel_guc_log *log)
 	struct intel_guc *guc = log_to_guc(log);
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 
+	guc_flush_log_msg_disable(guc);
+
 	/*
 	 * Once logging is disabled, GuC won't generate logs & send an
 	 * interrupt. But there could be some data in the log buffer
@@ -742,12 +755,6 @@ void intel_guc_log_unregister(struct intel_guc_log *log)
 	guc_flush_logs(log);
 
 	mutex_lock(&i915->drm.struct_mutex);
-
-	/* GuC logging is currently the only user of Guc2Host interrupts */
-	intel_runtime_pm_get(i915);
-	gen9_disable_guc_interrupts(i915);
-	intel_runtime_pm_put(i915);
-
 	guc_log_runtime_destroy(log);
 	mutex_unlock(&i915->drm.struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 104c03ae2742..765b86a53f19 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -247,6 +247,8 @@ static int guc_enable_communication(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
+	gen9_enable_guc_interrupts(dev_priv);
+
 	if (HAS_GUC_CT(dev_priv))
 		return intel_guc_enable_ct(guc);
 
@@ -261,6 +263,8 @@ static void guc_disable_communication(struct intel_guc *guc)
 	if (HAS_GUC_CT(dev_priv))
 		intel_guc_disable_ct(guc);
 
+	gen9_disable_guc_interrupts(dev_priv);
+
 	guc->send = intel_guc_send_nop;
 }
 
@@ -413,12 +417,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		if (i915_modparams.guc_log_level)
-			gen9_enable_guc_interrupts(dev_priv);
-
 		ret = intel_guc_submission_enable(guc);
 		if (ret)
-			goto err_interrupts;
+			goto err_communication;
 	}
 
 	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
@@ -433,8 +434,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	/*
 	 * We've failed to load the firmware :(
 	 */
-err_interrupts:
-	gen9_disable_guc_interrupts(dev_priv);
 err_communication:
 	guc_disable_communication(guc);
 err_log_capture:
@@ -464,9 +463,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
-
-	if (USES_GUC_SUBMISSION(dev_priv))
-		gen9_disable_guc_interrupts(dev_priv);
 }
 
 int intel_uc_suspend(struct drm_i915_private *i915)
-- 
2.14.3

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

[PATCH v3 02/13] drm/i915/guc: Log runtime should consist of both mapping and relay

[Michał Winiarski]

Currently, we're treating relay and mapping of GuC log as a separate
concepts. We're also using inconsistent locking, sometimes using
relay_lock, sometimes using struct mutex.
Let's correct that. Anything touching the runtime is now serialized
using runtime.lock, while we're still using struct mutex as inner lock
for mapping.
We're still racy in setting the log level - but we'll take care of that
in the following patches.

v2: Tidy locking (Sagar)
v3: Remove obsoleted comment (Sagar)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 125 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
 2 files changed, 38 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 1e209fcf90e1..b82866bfbef5 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -176,10 +176,7 @@ static int guc_log_relay_file_create(struct intel_guc_log *log)
 	struct dentry *log_dir;
 	int ret;
 
-	if (!i915_modparams.guc_log_level)
-		return 0;
-
-	mutex_lock(&log->runtime.relay_lock);
+	lockdep_assert_held(&log->runtime.lock);
 
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
@@ -198,29 +195,17 @@ static int guc_log_relay_file_create(struct intel_guc_log *log)
 	 */
 	if (!log_dir) {
 		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
-		ret = -ENODEV;
-		goto out_unlock;
+		return -ENODEV;
 	}
 
 	ret = relay_late_setup_files(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);
-		goto out_unlock;
+		return ret;
 	}
 
-	ret = 0;
-
-out_unlock:
-	mutex_unlock(&log->runtime.relay_lock);
-	return ret;
-}
-
-static bool guc_log_has_relay(struct intel_guc_log *log)
-{
-	lockdep_assert_held(&log->runtime.relay_lock);
-
-	return log->runtime.relay_chan;
+	return 0;
 }
 
 static void guc_move_to_next_buf(struct intel_guc_log *log)
@@ -231,9 +216,6 @@ static void guc_move_to_next_buf(struct intel_guc_log *log)
 	 */
 	smp_wmb();
 
-	if (!guc_log_has_relay(log))
-		return;
-
 	/* All data has been written, so now move the offset of sub buffer. */
 	relay_reserve(log->runtime.relay_chan, log->vma->obj->base.size);
 
@@ -243,9 +225,6 @@ static void guc_move_to_next_buf(struct intel_guc_log *log)
 
 static void *guc_get_write_buffer(struct intel_guc_log *log)
 {
-	if (!guc_log_has_relay(log))
-		return NULL;
-
 	/*
 	 * 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
@@ -306,14 +285,14 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 	void *src_data, *dst_data;
 	bool new_overflow;
 
+	mutex_lock(&log->runtime.lock);
+
 	if (WARN_ON(!log->runtime.buf_addr))
-		return;
+		goto out_unlock;
 
 	/* Get the pointer to shared GuC log buffer */
 	log_buf_state = src_data = log->runtime.buf_addr;
 
-	mutex_lock(&log->runtime.relay_lock);
-
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(log);
 
@@ -324,9 +303,8 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 		 */
 		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
 		log->capture_miss_count++;
-		mutex_unlock(&log->runtime.relay_lock);
 
-		return;
+		goto out_unlock;
 	}
 
 	/* Actual logs are present from the 2nd page */
@@ -397,7 +375,8 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 
 	guc_move_to_next_buf(log);
 
-	mutex_unlock(&log->runtime.relay_lock);
+out_unlock:
+	mutex_unlock(&log->runtime.lock);
 }
 
 static void capture_logs_work(struct work_struct *work)
@@ -413,21 +392,21 @@ static bool guc_log_has_runtime(struct intel_guc_log *log)
 	return log->runtime.buf_addr;
 }
 
-static int guc_log_runtime_create(struct intel_guc_log *log)
+static int guc_log_map(struct intel_guc_log *log)
 {
 	struct intel_guc *guc = log_to_guc(log);
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	void *vaddr;
 	int ret;
 
-	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+	lockdep_assert_held(&log->runtime.lock);
 
 	if (!log->vma)
 		return -ENODEV;
 
-	GEM_BUG_ON(guc_log_has_runtime(log));
-
+	mutex_lock(&dev_priv->drm.struct_mutex);
 	ret = i915_gem_object_set_to_wc_domain(log->vma->obj, true);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
 	if (ret)
 		return ret;
 
@@ -447,14 +426,9 @@ static int guc_log_runtime_create(struct intel_guc_log *log)
 	return 0;
 }
 
-static void guc_log_runtime_destroy(struct intel_guc_log *log)
+static void guc_log_unmap(struct intel_guc_log *log)
 {
-	/*
-	 * It's possible that the runtime stuff was never allocated because
-	 * GuC log was disabled at the boot time.
-	 */
-	if (!guc_log_has_runtime(log))
-		return;
+	lockdep_assert_held(&log->runtime.lock);
 
 	i915_gem_object_unpin_map(log->vma->obj);
 	log->runtime.buf_addr = NULL;
@@ -462,7 +436,7 @@ static void guc_log_runtime_destroy(struct intel_guc_log *log)
 
 void intel_guc_log_init_early(struct intel_guc_log *log)
 {
-	mutex_init(&log->runtime.relay_lock);
+	mutex_init(&log->runtime.lock);
 	INIT_WORK(&log->runtime.flush_work, capture_logs_work);
 }
 
@@ -474,12 +448,7 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 	size_t n_subbufs, subbuf_size;
 	int ret;
 
-	if (!i915_modparams.guc_log_level)
-		return 0;
-
-	mutex_lock(&log->runtime.relay_lock);
-
-	GEM_BUG_ON(guc_log_has_relay(log));
+	lockdep_assert_held(&log->runtime.lock);
 
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = GUC_LOG_SIZE;
@@ -509,12 +478,9 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
 	log->runtime.relay_chan = guc_log_relay_chan;
 
-	mutex_unlock(&log->runtime.relay_lock);
-
 	return 0;
 
 err:
-	mutex_unlock(&log->runtime.relay_lock);
 	/* logging will be off */
 	i915_modparams.guc_log_level = 0;
 	return ret;
@@ -522,20 +488,10 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 
 static void guc_log_relay_destroy(struct intel_guc_log *log)
 {
-	mutex_lock(&log->runtime.relay_lock);
-
-	/*
-	 * It's possible that the relay was never allocated because
-	 * GuC log was disabled at the boot time.
-	 */
-	if (!guc_log_has_relay(log))
-		goto out_unlock;
+	lockdep_assert_held(&log->runtime.lock);
 
 	relay_close(log->runtime.relay_chan);
 	log->runtime.relay_chan = NULL;
-
-out_unlock:
-	mutex_unlock(&log->runtime.relay_lock);
 }
 
 static void guc_log_capture_logs(struct intel_guc_log *log)
@@ -621,7 +577,6 @@ int intel_guc_log_create(struct intel_guc_log *log)
 
 void intel_guc_log_destroy(struct intel_guc_log *log)
 {
-	guc_log_runtime_destroy(log);
 	i915_vma_unpin_and_release(&log->vma);
 }
 
@@ -699,52 +654,43 @@ int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
 
 int intel_guc_log_register(struct intel_guc_log *log)
 {
-	struct intel_guc *guc = log_to_guc(log);
-	struct drm_i915_private *i915 = guc_to_i915(guc);
 	int ret;
 
+	mutex_lock(&log->runtime.lock);
+
 	GEM_BUG_ON(guc_log_has_runtime(log));
 
-	/*
-	 * 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_relay_create(log);
 	if (ret)
 		goto err;
 
-	mutex_lock(&i915->drm.struct_mutex);
-	ret = guc_log_runtime_create(log);
-	mutex_unlock(&i915->drm.struct_mutex);
-
+	ret = guc_log_map(log);
 	if (ret)
 		goto err_relay;
 
 	ret = guc_log_relay_file_create(log);
 	if (ret)
-		goto err_runtime;
+		goto err_unmap;
 
-	guc_flush_log_msg_enable(guc);
+	guc_flush_log_msg_enable(log_to_guc(log));
+
+	mutex_unlock(&log->runtime.lock);
 
 	return 0;
 
-err_runtime:
-	mutex_lock(&i915->drm.struct_mutex);
-	guc_log_runtime_destroy(log);
-	mutex_unlock(&i915->drm.struct_mutex);
+err_unmap:
+	guc_log_unmap(log);
 err_relay:
 	guc_log_relay_destroy(log);
 err:
+	mutex_unlock(&log->runtime.lock);
+
 	return ret;
 }
 
 void intel_guc_log_unregister(struct intel_guc_log *log)
 {
-	struct intel_guc *guc = log_to_guc(log);
-	struct drm_i915_private *i915 = guc_to_i915(guc);
-
-	guc_flush_log_msg_disable(guc);
+	guc_flush_log_msg_disable(log_to_guc(log));
 
 	/*
 	 * Once logging is disabled, GuC won't generate logs & send an
@@ -754,9 +700,12 @@ void intel_guc_log_unregister(struct intel_guc_log *log)
 	 */
 	guc_flush_logs(log);
 
-	mutex_lock(&i915->drm.struct_mutex);
-	guc_log_runtime_destroy(log);
-	mutex_unlock(&i915->drm.struct_mutex);
+	mutex_lock(&log->runtime.lock);
+
+	GEM_BUG_ON(!guc_log_has_runtime(log));
 
+	guc_log_unmap(log);
 	guc_log_relay_destroy(log);
+
+	mutex_unlock(&log->runtime.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 6264bd5ba080..e0ea625032fb 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -49,8 +49,7 @@ 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;
+		struct mutex lock;
 	} runtime;
 	/* logging related stats */
 	u32 capture_miss_count;
-- 
2.14.3

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

[PATCH v3 03/13] drm/i915/guc: Merge log relay file and channel creation

[Michał Winiarski]

We have all the information we need at relay_open call time.
Since there's no reason to split the process into relay_open and
relay_late_setup_files, let's remove the extra code.

v2: Remove obsoleted comments (Sagar)
v3: There was one obsolete comment left (Sagar)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 65 +++---------------------------------
 1 file changed, 5 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index b82866bfbef5..767c0d00fca6 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -141,14 +141,7 @@ 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
-	 * 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
-	 * name need not be same as the filename passed as an argument.
-	 */
-	buf_file = debugfs_create_file("guc_log", mode,
+	buf_file = debugfs_create_file(filename, mode,
 				       parent, buf, &relay_file_operations);
 	return buf_file;
 }
@@ -169,45 +162,6 @@ static struct rchan_callbacks relay_callbacks = {
 	.remove_buf_file = remove_buf_file_callback,
 };
 
-static int guc_log_relay_file_create(struct intel_guc_log *log)
-{
-	struct intel_guc *guc = log_to_guc(log);
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct dentry *log_dir;
-	int ret;
-
-	lockdep_assert_held(&log->runtime.lock);
-
-	/* 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
-	 * 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
-	 * straightforward/clean manner :-
-	 * i)   Need the associated dentry pointer of the file, while opening the
-	 *      relay channel.
-	 * ii)  Should be able to use 'relay_file_operations' fops for the file.
-	 * iii) Set the 'i_private' field of file's inode to the pointer of
-	 *	relay channel buffer.
-	 */
-	if (!log_dir) {
-		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
-		return -ENODEV;
-	}
-
-	ret = relay_late_setup_files(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;
-	}
-
-	return 0;
-}
-
 static void guc_move_to_next_buf(struct intel_guc_log *log)
 {
 	/*
@@ -461,13 +415,10 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 	 */
 	n_subbufs = 8;
 
-	/*
-	 * 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.
-	 */
-	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
-					n_subbufs, &relay_callbacks, dev_priv);
+	guc_log_relay_chan = relay_open("guc_log",
+					dev_priv->drm.primary->debugfs_root,
+					subbuf_size, n_subbufs,
+					&relay_callbacks, dev_priv);
 	if (!guc_log_relay_chan) {
 		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
 
@@ -668,18 +619,12 @@ int intel_guc_log_register(struct intel_guc_log *log)
 	if (ret)
 		goto err_relay;
 
-	ret = guc_log_relay_file_create(log);
-	if (ret)
-		goto err_unmap;
-
 	guc_flush_log_msg_enable(log_to_guc(log));
 
 	mutex_unlock(&log->runtime.lock);
 
 	return 0;
 
-err_unmap:
-	guc_log_unmap(log);
 err_relay:
 	guc_log_relay_destroy(log);
 err:
-- 
2.14.3

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

[PATCH v2 04/13] drm/i915/guc: Flush directly in log unregister

[Michał Winiarski]

Having both guc_flush_logs and guc_log_flush functions is confusing.
While we could just rename things, guc_flush_logs implementation is
quite simple. Let's get rid of it and move its content to unregister.

v2: s/dev_priv/i915 (Sagar)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 38 +++++++++++++++---------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 767c0d00fca6..72a71bc94adf 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -461,26 +461,6 @@ static void guc_log_capture_logs(struct intel_guc_log *log)
 	intel_runtime_pm_put(dev_priv);
 }
 
-static void guc_flush_logs(struct intel_guc_log *log)
-{
-	struct intel_guc *guc = log_to_guc(log);
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	/*
-	 * Before initiating the forceful flush, wait for any pending/ongoing
-	 * flush to complete otherwise forceful flush may not actually happen.
-	 */
-	flush_work(&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(log);
-}
-
 int intel_guc_log_create(struct intel_guc_log *log)
 {
 	struct intel_guc *guc = log_to_guc(log);
@@ -635,7 +615,16 @@ int intel_guc_log_register(struct intel_guc_log *log)
 
 void intel_guc_log_unregister(struct intel_guc_log *log)
 {
-	guc_flush_log_msg_disable(log_to_guc(log));
+	struct intel_guc *guc = log_to_guc(log);
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+
+	guc_flush_log_msg_disable(guc);
+
+	/*
+	 * Before initiating the forceful flush, wait for any pending/ongoing
+	 * flush to complete otherwise forceful flush may not actually happen.
+	 */
+	flush_work(&log->runtime.flush_work);
 
 	/*
 	 * Once logging is disabled, GuC won't generate logs & send an
@@ -643,7 +632,12 @@ void intel_guc_log_unregister(struct intel_guc_log *log)
 	 * which is yet to be captured. So request GuC to update the log
 	 * buffer state and then collect the left over logs.
 	 */
-	guc_flush_logs(log);
+	intel_runtime_pm_get(i915);
+	guc_log_flush(guc);
+	intel_runtime_pm_put(i915);
+
+	/* GuC would have updated log buffer by now, so capture it */
+	guc_log_capture_logs(log);
 
 	mutex_lock(&log->runtime.lock);
 
-- 
2.14.3

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

[PATCH v3 05/13] drm/i915/guc: Split relay control and GuC log level

[Michał Winiarski]

Those two concepts are really separate. Since GuC is writing data into
its own buffer and we even provide a way for userspace to read directly
from it using i915_guc_log_dump debugfs, there's no real reason to tie
log level with relay creation.
Let's create a separate debugfs, giving userspace a way to create a
relay on demand, when it wants to read a continuous log rather than a
snapshot.

v2: Don't touch guc_log_level on relay creation error, adjust locking
    after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar)
    Use struct_mutex rather than runtime.lock for set_log_level
v3: Tidy ordering of definitions (Sagar)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 56 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.c      |  4 --
 drivers/gpu/drm/i915/intel_guc_log.c | 75 +++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_guc_log.h |  9 +++--
 drivers/gpu/drm/i915/intel_uc.c      | 22 -----------
 drivers/gpu/drm/i915/intel_uc.h      |  2 -
 6 files changed, 84 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5378863e3238..e857a9493b6f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2495,32 +2495,73 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
-static int i915_guc_log_control_get(void *data, u64 *val)
+static int i915_guc_log_level_get(void *data, u64 *val)
 {
 	struct drm_i915_private *dev_priv = data;
 
 	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	*val = intel_guc_log_control_get(&dev_priv->guc.log);
+	*val = intel_guc_log_level_get(&dev_priv->guc.log);
 
 	return 0;
 }
 
-static int i915_guc_log_control_set(void *data, u64 val)
+static int i915_guc_log_level_set(void *data, u64 val)
 {
 	struct drm_i915_private *dev_priv = data;
 
 	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	return intel_guc_log_control_set(&dev_priv->guc.log, val);
+	return intel_guc_log_level_set(&dev_priv->guc.log, val);
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
-			i915_guc_log_control_get, i915_guc_log_control_set,
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_level_fops,
+			i915_guc_log_level_get, i915_guc_log_level_set,
 			"%lld\n");
 
+static int i915_guc_log_relay_open(struct inode *inode, struct file *file)
+{
+	struct drm_i915_private *dev_priv = inode->i_private;
+
+	if (!USES_GUC(dev_priv))
+		return -ENODEV;
+
+	file->private_data = &dev_priv->guc.log;
+
+	return intel_guc_log_relay_open(&dev_priv->guc.log);
+}
+
+static ssize_t
+i915_guc_log_relay_write(struct file *filp,
+			 const char __user *ubuf,
+			 size_t cnt,
+			 loff_t *ppos)
+{
+	struct intel_guc_log *log = filp->private_data;
+
+	intel_guc_log_relay_flush(log);
+
+	return cnt;
+}
+
+static int i915_guc_log_relay_release(struct inode *inode, struct file *file)
+{
+	struct drm_i915_private *dev_priv = inode->i_private;
+
+	intel_guc_log_relay_close(&dev_priv->guc.log);
+
+	return 0;
+}
+
+static const struct file_operations i915_guc_log_relay_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_guc_log_relay_open,
+	.write = i915_guc_log_relay_write,
+	.release = i915_guc_log_relay_release,
+};
+
 static const char *psr2_live_status(u32 val)
 {
 	static const char * const live_status[] = {
@@ -4748,7 +4789,8 @@ static const struct i915_debugfs_files {
 	{"i915_dp_test_data", &i915_displayport_test_data_fops},
 	{"i915_dp_test_type", &i915_displayport_test_type_fops},
 	{"i915_dp_test_active", &i915_displayport_test_active_fops},
-	{"i915_guc_log_control", &i915_guc_log_control_fops},
+	{"i915_guc_log_level", &i915_guc_log_level_fops},
+	{"i915_guc_log_relay", &i915_guc_log_relay_fops},
 	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
 	{"i915_ipc_status", &i915_ipc_status_fops},
 	{"i915_drrs_ctl", &i915_drrs_ctl_fops}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3df5193487f3..1021bf40e236 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1239,9 +1239,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 		i915_debugfs_register(dev_priv);
 		i915_setup_sysfs(dev_priv);
 
-		/* Depends on debugfs having been initialized */
-		intel_uc_register(dev_priv);
-
 		/* Depends on sysfs having been initialized */
 		i915_perf_register(dev_priv);
 	} else
@@ -1299,7 +1296,6 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	i915_pmu_unregister(dev_priv);
 
 	i915_teardown_sysfs(dev_priv);
-	intel_uc_unregister(dev_priv);
 	drm_dev_unregister(&dev_priv->drm);
 
 	i915_gem_shrinker_unregister(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 72a71bc94adf..20254dde172c 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -423,18 +423,13 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
 
 		ret = -ENOMEM;
-		goto err;
+		return ret;
 	}
 
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
 	log->runtime.relay_chan = guc_log_relay_chan;
 
 	return 0;
-
-err:
-	/* logging will be off */
-	i915_modparams.guc_log_level = 0;
-	return ret;
 }
 
 static void guc_log_relay_destroy(struct intel_guc_log *log)
@@ -511,7 +506,7 @@ void intel_guc_log_destroy(struct intel_guc_log *log)
 	i915_vma_unpin_and_release(&log->vma);
 }
 
-int intel_guc_log_control_get(struct intel_guc_log *log)
+int intel_guc_log_level_get(struct intel_guc_log *log)
 {
 	GEM_BUG_ON(!log->vma);
 	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
@@ -526,11 +521,10 @@ int intel_guc_log_control_get(struct intel_guc_log *log)
 	LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0;	\
 })
 #define VERBOSITY_TO_LOG_LEVEL(x)  ((x) + 1)
-int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
+int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 {
 	struct intel_guc *guc = log_to_guc(log);
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	bool enabled = LOG_LEVEL_TO_ENABLED(val);
 	int ret;
 
 	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
@@ -553,7 +547,8 @@ int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
 	}
 
 	intel_runtime_pm_get(dev_priv);
-	ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
+	ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val),
+			      LOG_LEVEL_TO_VERBOSITY(val));
 	intel_runtime_pm_put(dev_priv);
 	if (ret) {
 		DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
@@ -562,89 +557,79 @@ int intel_guc_log_control_set(struct intel_guc_log *log, u64 val)
 
 	i915_modparams.guc_log_level = val;
 
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	if (enabled && !guc_log_has_runtime(log)) {
-		ret = intel_guc_log_register(log);
-		if (ret) {
-			/* logging will remain off */
-			i915_modparams.guc_log_level = 0;
-			goto out;
-		}
-	} else if (!enabled && guc_log_has_runtime(log)) {
-		intel_guc_log_unregister(log);
-	}
-
-	return 0;
-
 out_unlock:
 	mutex_unlock(&dev_priv->drm.struct_mutex);
-out:
+
 	return ret;
 }
 
-int intel_guc_log_register(struct intel_guc_log *log)
+int intel_guc_log_relay_open(struct intel_guc_log *log)
 {
 	int ret;
 
 	mutex_lock(&log->runtime.lock);
 
-	GEM_BUG_ON(guc_log_has_runtime(log));
+	if (guc_log_has_runtime(log)) {
+		ret = -EEXIST;
+		goto out_unlock;
+	}
 
 	ret = guc_log_relay_create(log);
 	if (ret)
-		goto err;
+		goto out_unlock;
 
 	ret = guc_log_map(log);
 	if (ret)
-		goto err_relay;
+		goto out_relay;
+
+	mutex_unlock(&log->runtime.lock);
 
 	guc_flush_log_msg_enable(log_to_guc(log));
 
-	mutex_unlock(&log->runtime.lock);
+	/*
+	 * When GuC is logging without us relaying to userspace, we're ignoring
+	 * the flush notification. This means that we need to unconditionally
+	 * flush on relay enabling, since GuC only notifies us once.
+	 */
+	queue_work(log->runtime.flush_wq, &log->runtime.flush_work);
 
 	return 0;
 
-err_relay:
+out_relay:
 	guc_log_relay_destroy(log);
-err:
+out_unlock:
 	mutex_unlock(&log->runtime.lock);
 
 	return ret;
 }
 
-void intel_guc_log_unregister(struct intel_guc_log *log)
+void intel_guc_log_relay_flush(struct intel_guc_log *log)
 {
 	struct intel_guc *guc = log_to_guc(log);
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 
-	guc_flush_log_msg_disable(guc);
-
 	/*
 	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
 	flush_work(&log->runtime.flush_work);
 
-	/*
-	 * Once logging is disabled, GuC won't generate logs & send an
-	 * interrupt. But there could be some data in the log buffer
-	 * which is yet to be captured. So request GuC to update the log
-	 * buffer state and then collect the left over logs.
-	 */
 	intel_runtime_pm_get(i915);
 	guc_log_flush(guc);
 	intel_runtime_pm_put(i915);
 
 	/* GuC would have updated log buffer by now, so capture it */
 	guc_log_capture_logs(log);
+}
 
-	mutex_lock(&log->runtime.lock);
+void intel_guc_log_relay_close(struct intel_guc_log *log)
+{
+	guc_flush_log_msg_disable(log_to_guc(log));
+	flush_work(&log->runtime.flush_work);
 
+	mutex_lock(&log->runtime.lock);
 	GEM_BUG_ON(!guc_log_has_runtime(log));
-
 	guc_log_unmap(log);
 	guc_log_relay_destroy(log);
-
 	mutex_unlock(&log->runtime.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index e0ea625032fb..3cf911eef3a8 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -61,11 +61,12 @@ struct intel_guc_log {
 
 void intel_guc_log_init_early(struct intel_guc_log *log);
 int intel_guc_log_create(struct intel_guc_log *log);
-int intel_guc_log_register(struct intel_guc_log *log);
-void intel_guc_log_unregister(struct intel_guc_log *log);
 void intel_guc_log_destroy(struct intel_guc_log *log);
 
-int intel_guc_log_control_get(struct intel_guc_log *log);
-int intel_guc_log_control_set(struct intel_guc_log *log, u64 control);
+int intel_guc_log_level_get(struct intel_guc_log *log);
+int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val);
+int intel_guc_log_relay_open(struct intel_guc_log *log);
+void intel_guc_log_relay_flush(struct intel_guc_log *log);
+void intel_guc_log_relay_close(struct intel_guc_log *log);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 765b86a53f19..9bb40cd047a0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -221,28 +221,6 @@ static void guc_free_load_err_log(struct intel_guc *guc)
 		i915_gem_object_put(guc->load_err_log);
 }
 
-int intel_uc_register(struct drm_i915_private *i915)
-{
-	int ret = 0;
-
-	if (!USES_GUC(i915))
-		return 0;
-
-	if (i915_modparams.guc_log_level)
-		ret = intel_guc_log_register(&i915->guc.log);
-
-	return ret;
-}
-
-void intel_uc_unregister(struct drm_i915_private *i915)
-{
-	if (!USES_GUC(i915))
-		return;
-
-	if (i915_modparams.guc_log_level)
-		intel_guc_log_unregister(&i915->guc.log);
-}
-
 static int guc_enable_communication(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 0a2b413e9cd0..937e61175258 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -30,8 +30,6 @@
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
-int intel_uc_register(struct drm_i915_private *dev_priv);
-void intel_uc_unregister(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_misc(struct drm_i915_private *dev_priv);
-- 
2.14.3

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

[PATCH v2 06/13] drm/i915/guc: Move check for fast memcpy_wc to relay creation

[Michał Winiarski]

We only need those fast memcpy_wc when we're using relay to read
continuous GuC log. Let's prevent the user from creating a relay if we
know we won't be able to keep up with GuC.

v2: Adjust the return value (Michał)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 20254dde172c..db89999a84e8 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -466,16 +466,6 @@ int intel_guc_log_create(struct intel_guc_log *log)
 
 	GEM_BUG_ON(log->vma);
 
-	/*
-	 * 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.
-	 */
-	if (WARN_ON(!i915_has_memcpy_from_wc())) {
-		ret = -EINVAL;
-		goto err;
-	}
-
 	vma = intel_guc_allocate_vma(guc, GUC_LOG_SIZE);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
@@ -574,6 +564,16 @@ int intel_guc_log_relay_open(struct intel_guc_log *log)
 		goto out_unlock;
 	}
 
+	/*
+	 * 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.
+	 */
+	if (!i915_has_memcpy_from_wc()) {
+		ret = -ENXIO;
+		goto out_unlock;
+	}
+
 	ret = guc_log_relay_create(log);
 	if (ret)
 		goto out_unlock;
-- 
2.14.3

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

[PATCH v2 07/13] drm/i915/guc: Get rid of GuC log runtime

[Michał Winiarski]

Runtime is not a very good name. Let's also move counting relay
overflows inside relay struct.

v2: Rename things rather than remove the struct (Chris)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  4 +--
 drivers/gpu/drm/i915/intel_guc.c     | 12 +++----
 drivers/gpu/drm/i915/intel_guc_log.c | 64 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_guc_log.h |  7 ++--
 4 files changed, 43 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e857a9493b6f..d3d4d1b29112 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2347,8 +2347,8 @@ static void i915_guc_log_info(struct seq_file *m,
 	seq_printf(m, "\tTotal flush interrupt count: %u\n",
 		   guc->log.flush_interrupt_count);
 
-	seq_printf(m, "\tCapture miss count: %u\n",
-		   guc->log.capture_miss_count);
+	seq_printf(m, "\tRelay full count: %u\n",
+		   guc->log.relay.full_count);
 }
 
 static void i915_guc_client_info(struct seq_file *m,
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 3af603536b1b..4ac5d0daa81f 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -87,9 +87,9 @@ int intel_guc_init_wq(struct intel_guc *guc)
 	 * or scheduled later on resume. This way the handling of work
 	 * item can be kept same between system suspend & rpm suspend.
 	 */
-	guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+	guc->log.relay.flush_wq = alloc_ordered_workqueue("i915-guc_log",
 						WQ_HIGHPRI | WQ_FREEZABLE);
-	if (!guc->log.runtime.flush_wq) {
+	if (!guc->log.relay.flush_wq) {
 		DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
 		return -ENOMEM;
 	}
@@ -112,7 +112,7 @@ int intel_guc_init_wq(struct intel_guc *guc)
 		guc->preempt_wq = alloc_ordered_workqueue("i915-guc_preempt",
 							  WQ_HIGHPRI);
 		if (!guc->preempt_wq) {
-			destroy_workqueue(guc->log.runtime.flush_wq);
+			destroy_workqueue(guc->log.relay.flush_wq);
 			DRM_ERROR("Couldn't allocate workqueue for GuC "
 				  "preemption\n");
 			return -ENOMEM;
@@ -130,7 +130,7 @@ void intel_guc_fini_wq(struct intel_guc *guc)
 	    USES_GUC_SUBMISSION(dev_priv))
 		destroy_workqueue(guc->preempt_wq);
 
-	destroy_workqueue(guc->log.runtime.flush_wq);
+	destroy_workqueue(guc->log.relay.flush_wq);
 }
 
 static int guc_shared_data_create(struct intel_guc *guc)
@@ -390,8 +390,8 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
 
 	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
 		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
-		queue_work(guc->log.runtime.flush_wq,
-			   &guc->log.runtime.flush_work);
+		queue_work(guc->log.relay.flush_wq,
+			   &guc->log.relay.flush_work);
 
 		guc->log.flush_interrupt_count++;
 	}
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index db89999a84e8..33e2164cf4c8 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -171,10 +171,10 @@ static void guc_move_to_next_buf(struct intel_guc_log *log)
 	smp_wmb();
 
 	/* All data has been written, so now move the offset of sub buffer. */
-	relay_reserve(log->runtime.relay_chan, log->vma->obj->base.size);
+	relay_reserve(log->relay.channel, log->vma->obj->base.size);
 
 	/* Switch to the next sub buffer */
-	relay_flush(log->runtime.relay_chan);
+	relay_flush(log->relay.channel);
 }
 
 static void *guc_get_write_buffer(struct intel_guc_log *log)
@@ -188,7 +188,7 @@ static void *guc_get_write_buffer(struct intel_guc_log *log)
 	 * done without using relay_reserve() along with relay_write(). So its
 	 * better to use relay_reserve() alone.
 	 */
-	return relay_reserve(log->runtime.relay_chan, 0);
+	return relay_reserve(log->relay.channel, 0);
 }
 
 static bool guc_check_log_buf_overflow(struct intel_guc_log *log,
@@ -239,13 +239,13 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 	void *src_data, *dst_data;
 	bool new_overflow;
 
-	mutex_lock(&log->runtime.lock);
+	mutex_lock(&log->relay.lock);
 
-	if (WARN_ON(!log->runtime.buf_addr))
+	if (WARN_ON(!log->relay.buf_addr))
 		goto out_unlock;
 
 	/* Get the pointer to shared GuC log buffer */
-	log_buf_state = src_data = log->runtime.buf_addr;
+	log_buf_state = src_data = log->relay.buf_addr;
 
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(log);
@@ -256,7 +256,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 		 * getting consumed by User at a slow rate.
 		 */
 		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
-		log->capture_miss_count++;
+		log->relay.full_count++;
 
 		goto out_unlock;
 	}
@@ -330,20 +330,20 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 	guc_move_to_next_buf(log);
 
 out_unlock:
-	mutex_unlock(&log->runtime.lock);
+	mutex_unlock(&log->relay.lock);
 }
 
 static void capture_logs_work(struct work_struct *work)
 {
 	struct intel_guc_log *log =
-		container_of(work, struct intel_guc_log, runtime.flush_work);
+		container_of(work, struct intel_guc_log, relay.flush_work);
 
 	guc_log_capture_logs(log);
 }
 
-static bool guc_log_has_runtime(struct intel_guc_log *log)
+static bool guc_log_relay_enabled(struct intel_guc_log *log)
 {
-	return log->runtime.buf_addr;
+	return log->relay.buf_addr != NULL;
 }
 
 static int guc_log_map(struct intel_guc_log *log)
@@ -353,7 +353,7 @@ static int guc_log_map(struct intel_guc_log *log)
 	void *vaddr;
 	int ret;
 
-	lockdep_assert_held(&log->runtime.lock);
+	lockdep_assert_held(&log->relay.lock);
 
 	if (!log->vma)
 		return -ENODEV;
@@ -375,23 +375,23 @@ static int guc_log_map(struct intel_guc_log *log)
 		return PTR_ERR(vaddr);
 	}
 
-	log->runtime.buf_addr = vaddr;
+	log->relay.buf_addr = vaddr;
 
 	return 0;
 }
 
 static void guc_log_unmap(struct intel_guc_log *log)
 {
-	lockdep_assert_held(&log->runtime.lock);
+	lockdep_assert_held(&log->relay.lock);
 
 	i915_gem_object_unpin_map(log->vma->obj);
-	log->runtime.buf_addr = NULL;
+	log->relay.buf_addr = NULL;
 }
 
 void intel_guc_log_init_early(struct intel_guc_log *log)
 {
-	mutex_init(&log->runtime.lock);
-	INIT_WORK(&log->runtime.flush_work, capture_logs_work);
+	mutex_init(&log->relay.lock);
+	INIT_WORK(&log->relay.flush_work, capture_logs_work);
 }
 
 static int guc_log_relay_create(struct intel_guc_log *log)
@@ -402,7 +402,7 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 	size_t n_subbufs, subbuf_size;
 	int ret;
 
-	lockdep_assert_held(&log->runtime.lock);
+	lockdep_assert_held(&log->relay.lock);
 
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = GUC_LOG_SIZE;
@@ -427,17 +427,17 @@ static int guc_log_relay_create(struct intel_guc_log *log)
 	}
 
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
-	log->runtime.relay_chan = guc_log_relay_chan;
+	log->relay.channel = guc_log_relay_chan;
 
 	return 0;
 }
 
 static void guc_log_relay_destroy(struct intel_guc_log *log)
 {
-	lockdep_assert_held(&log->runtime.lock);
+	lockdep_assert_held(&log->relay.lock);
 
-	relay_close(log->runtime.relay_chan);
-	log->runtime.relay_chan = NULL;
+	relay_close(log->relay.channel);
+	log->relay.channel = NULL;
 }
 
 static void guc_log_capture_logs(struct intel_guc_log *log)
@@ -557,9 +557,9 @@ int intel_guc_log_relay_open(struct intel_guc_log *log)
 {
 	int ret;
 
-	mutex_lock(&log->runtime.lock);
+	mutex_lock(&log->relay.lock);
 
-	if (guc_log_has_runtime(log)) {
+	if (guc_log_relay_enabled(log)) {
 		ret = -EEXIST;
 		goto out_unlock;
 	}
@@ -582,7 +582,7 @@ int intel_guc_log_relay_open(struct intel_guc_log *log)
 	if (ret)
 		goto out_relay;
 
-	mutex_unlock(&log->runtime.lock);
+	mutex_unlock(&log->relay.lock);
 
 	guc_flush_log_msg_enable(log_to_guc(log));
 
@@ -591,14 +591,14 @@ int intel_guc_log_relay_open(struct intel_guc_log *log)
 	 * the flush notification. This means that we need to unconditionally
 	 * flush on relay enabling, since GuC only notifies us once.
 	 */
-	queue_work(log->runtime.flush_wq, &log->runtime.flush_work);
+	queue_work(log->relay.flush_wq, &log->relay.flush_work);
 
 	return 0;
 
 out_relay:
 	guc_log_relay_destroy(log);
 out_unlock:
-	mutex_unlock(&log->runtime.lock);
+	mutex_unlock(&log->relay.lock);
 
 	return ret;
 }
@@ -612,7 +612,7 @@ void intel_guc_log_relay_flush(struct intel_guc_log *log)
 	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
-	flush_work(&log->runtime.flush_work);
+	flush_work(&log->relay.flush_work);
 
 	intel_runtime_pm_get(i915);
 	guc_log_flush(guc);
@@ -625,11 +625,11 @@ void intel_guc_log_relay_flush(struct intel_guc_log *log)
 void intel_guc_log_relay_close(struct intel_guc_log *log)
 {
 	guc_flush_log_msg_disable(log_to_guc(log));
-	flush_work(&log->runtime.flush_work);
+	flush_work(&log->relay.flush_work);
 
-	mutex_lock(&log->runtime.lock);
-	GEM_BUG_ON(!guc_log_has_runtime(log));
+	mutex_lock(&log->relay.lock);
+	GEM_BUG_ON(!guc_log_relay_enabled(log));
 	guc_log_unmap(log);
 	guc_log_relay_destroy(log);
-	mutex_unlock(&log->runtime.lock);
+	mutex_unlock(&log->relay.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 3cf911eef3a8..db35e548d2ed 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -43,16 +43,15 @@ struct intel_guc;
 struct intel_guc_log {
 	u32 flags;
 	struct i915_vma *vma;
-	/* The runtime stuff gets created only when GuC logging gets enabled */
 	struct {
 		void *buf_addr;
 		struct workqueue_struct *flush_wq;
 		struct work_struct flush_work;
-		struct rchan *relay_chan;
+		struct rchan *channel;
 		struct mutex lock;
-	} runtime;
+		u32 full_count;
+	} relay;
 	/* logging related stats */
-	u32 capture_miss_count;
 	u32 flush_interrupt_count;
 	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
 	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
-- 
2.14.3

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

[PATCH 08/13] drm/i915/guc: Always print log stats in i915_guc_info when using GuC

[Michał Winiarski]

While some of the content in this file is related to GuC submission
only, that's not the case with log related statistics.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d3d4d1b29112..5584736a4293 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2330,7 +2330,7 @@ static void i915_guc_log_info(struct seq_file *m,
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
-	seq_puts(m, "\nGuC logging stats:\n");
+	seq_puts(m, "GuC logging stats:\n");
 
 	seq_printf(m, "\tISR:   flush count %10u, overflow count %10u\n",
 		   guc->log.flush_count[GUC_ISR_LOG_BUFFER],
@@ -2378,14 +2378,19 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	const struct intel_guc *guc = &dev_priv->guc;
 
-	if (!USES_GUC_SUBMISSION(dev_priv))
+	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
+	i915_guc_log_info(m, dev_priv);
+
+	if (!USES_GUC_SUBMISSION(dev_priv))
+		return 0;
+
 	GEM_BUG_ON(!guc->execbuf_client);
 
-	seq_printf(m, "Doorbell map:\n");
+	seq_printf(m, "\nDoorbell map:\n");
 	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
-	seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
+	seq_printf(m, "Doorbell next cacheline: 0x%x\n", guc->db_cacheline);
 
 	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
 	i915_guc_client_info(m, dev_priv, guc->execbuf_client);
@@ -2395,8 +2400,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
 		i915_guc_client_info(m, dev_priv, guc->preempt_client);
 	}
 
-	i915_guc_log_info(m, dev_priv);
-
 	/* Add more as required ... */
 
 	return 0;
-- 
2.14.3

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

[PATCH v3 09/13] drm/i915/guc: Don't print out relay statistics when relay is disabled

[Michał Winiarski]

If nobody has enabled the relay, we're not comunicating with GuC, which
means that the stats don't have any meaning. Let's also remove interrupt
counter and tidy the debugfs formatting.

v2: Correct stats accounting (Sagar)
v3: Corrected one more error in stats accounting, move relay_enabled (Sagar)

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 49 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_guc.c     |  5 +---
 drivers/gpu/drm/i915/intel_guc_log.c | 26 +++++++++----------
 drivers/gpu/drm/i915/intel_guc_log.h | 10 +++++---
 4 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5584736a4293..964ea1a12357 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2325,30 +2325,45 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	return 0;
 }
 
-static void i915_guc_log_info(struct seq_file *m,
-			      struct drm_i915_private *dev_priv)
+static const char *
+stringify_guc_log_type(enum guc_log_buffer_type type)
 {
-	struct intel_guc *guc = &dev_priv->guc;
-
-	seq_puts(m, "GuC logging stats:\n");
+	switch (type) {
+	case GUC_ISR_LOG_BUFFER:
+		return "ISR";
+	case GUC_DPC_LOG_BUFFER:
+		return "DPC";
+	case GUC_CRASH_DUMP_LOG_BUFFER:
+		return "CRASH";
+	default:
+		MISSING_CASE(type);
+	}
 
-	seq_printf(m, "\tISR:   flush count %10u, overflow count %10u\n",
-		   guc->log.flush_count[GUC_ISR_LOG_BUFFER],
-		   guc->log.total_overflow_count[GUC_ISR_LOG_BUFFER]);
+	return "";
+}
 
-	seq_printf(m, "\tDPC:   flush count %10u, overflow count %10u\n",
-		   guc->log.flush_count[GUC_DPC_LOG_BUFFER],
-		   guc->log.total_overflow_count[GUC_DPC_LOG_BUFFER]);
+static void i915_guc_log_info(struct seq_file *m,
+			      struct drm_i915_private *dev_priv)
+{
+	struct intel_guc_log *log = &dev_priv->guc.log;
+	enum guc_log_buffer_type type;
 
-	seq_printf(m, "\tCRASH: flush count %10u, overflow count %10u\n",
-		   guc->log.flush_count[GUC_CRASH_DUMP_LOG_BUFFER],
-		   guc->log.total_overflow_count[GUC_CRASH_DUMP_LOG_BUFFER]);
+	if (!intel_guc_log_relay_enabled(log)) {
+		seq_puts(m, "GuC log relay disabled\n");
+		return;
+	}
 
-	seq_printf(m, "\tTotal flush interrupt count: %u\n",
-		   guc->log.flush_interrupt_count);
+	seq_puts(m, "GuC logging stats:\n");
 
 	seq_printf(m, "\tRelay full count: %u\n",
-		   guc->log.relay.full_count);
+		   log->relay.full_count);
+
+	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
+		seq_printf(m, "\t%s:\tflush count %10u, overflow count %10u\n",
+			   stringify_guc_log_type(type),
+			   log->stats[type].flush,
+			   log->stats[type].sampled_overflow);
+	}
 }
 
 static void i915_guc_client_info(struct seq_file *m,
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 4ac5d0daa81f..d4c2524012fa 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -389,12 +389,9 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
 	spin_unlock(&guc->irq_lock);
 
 	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
-		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
+		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
 		queue_work(guc->log.relay.flush_wq,
 			   &guc->log.relay.flush_work);
-
-		guc->log.flush_interrupt_count++;
-	}
 }
 
 int intel_guc_sample_forcewake(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 33e2164cf4c8..1e671f2b2f64 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -195,18 +195,18 @@ static bool guc_check_log_buf_overflow(struct intel_guc_log *log,
 				       enum guc_log_buffer_type type,
 				       unsigned int full_cnt)
 {
-	unsigned int prev_full_cnt = log->prev_overflow_count[type];
+	unsigned int prev_full_cnt = log->stats[type].sampled_overflow;
 	bool overflow = false;
 
 	if (full_cnt != prev_full_cnt) {
 		overflow = true;
 
-		log->prev_overflow_count[type] = full_cnt;
-		log->total_overflow_count[type] += full_cnt - prev_full_cnt;
+		log->stats[type].overflow = full_cnt;
+		log->stats[type].sampled_overflow += full_cnt - prev_full_cnt;
 
 		if (full_cnt < prev_full_cnt) {
 			/* buffer_full_cnt is a 4 bit counter */
-			log->total_overflow_count[type] += 16;
+			log->stats[type].sampled_overflow += 16;
 		}
 		DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
 	}
@@ -241,7 +241,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 
 	mutex_lock(&log->relay.lock);
 
-	if (WARN_ON(!log->relay.buf_addr))
+	if (WARN_ON(!intel_guc_log_relay_enabled(log)))
 		goto out_unlock;
 
 	/* Get the pointer to shared GuC log buffer */
@@ -279,7 +279,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log)
 		full_cnt = log_buf_state_local.buffer_full_cnt;
 
 		/* Bookkeeping stuff */
-		log->flush_count[type] += log_buf_state_local.flush_to_file;
+		log->stats[type].flush += log_buf_state_local.flush_to_file;
 		new_overflow = guc_check_log_buf_overflow(log, type, full_cnt);
 
 		/* Update the state of shared log buffer */
@@ -341,11 +341,6 @@ static void capture_logs_work(struct work_struct *work)
 	guc_log_capture_logs(log);
 }
 
-static bool guc_log_relay_enabled(struct intel_guc_log *log)
-{
-	return log->relay.buf_addr != NULL;
-}
-
 static int guc_log_map(struct intel_guc_log *log)
 {
 	struct intel_guc *guc = log_to_guc(log);
@@ -553,13 +548,18 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 	return ret;
 }
 
+bool intel_guc_log_relay_enabled(struct intel_guc_log *log)
+{
+	return log->relay.buf_addr != NULL;
+}
+
 int intel_guc_log_relay_open(struct intel_guc_log *log)
 {
 	int ret;
 
 	mutex_lock(&log->relay.lock);
 
-	if (guc_log_relay_enabled(log)) {
+	if (intel_guc_log_relay_enabled(log)) {
 		ret = -EEXIST;
 		goto out_unlock;
 	}
@@ -628,7 +628,7 @@ void intel_guc_log_relay_close(struct intel_guc_log *log)
 	flush_work(&log->relay.flush_work);
 
 	mutex_lock(&log->relay.lock);
-	GEM_BUG_ON(!guc_log_relay_enabled(log));
+	GEM_BUG_ON(!intel_guc_log_relay_enabled(log));
 	guc_log_unmap(log);
 	guc_log_relay_destroy(log);
 	mutex_unlock(&log->relay.lock);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index db35e548d2ed..cc86587a0543 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -52,10 +52,11 @@ struct intel_guc_log {
 		u32 full_count;
 	} relay;
 	/* logging related stats */
-	u32 flush_interrupt_count;
-	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
-	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
-	u32 flush_count[GUC_MAX_LOG_BUFFER];
+	struct {
+		u32 sampled_overflow;
+		u32 overflow;
+		u32 flush;
+	} stats[GUC_MAX_LOG_BUFFER];
 };
 
 void intel_guc_log_init_early(struct intel_guc_log *log);
@@ -64,6 +65,7 @@ void intel_guc_log_destroy(struct intel_guc_log *log);
 
 int intel_guc_log_level_get(struct intel_guc_log *log);
 int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val);
+bool intel_guc_log_relay_enabled(struct intel_guc_log *log);
 int intel_guc_log_relay_open(struct intel_guc_log *log);
 void intel_guc_log_relay_flush(struct intel_guc_log *log);
 void intel_guc_log_relay_close(struct intel_guc_log *log);
-- 
2.14.3

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

[PATCH v3 10/13] drm/i915/guc: Allow user to control default GuC logging

[Michał Winiarski]

While both naming and actual log enable logic in GuC interface are
confusing, we can simply expose the default log as yet another log
level.
GuC logic aside, from i915 point of view we now have the following GuC
log levels:
	0 Log disabled
	1 Non-verbose log
	2-5 Verbose log

v2: Adjust naming after rebase.
v3: Fixed the log_level logic error introduced on rebase.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v2)
---
 drivers/gpu/drm/i915/intel_guc.c      | 24 +++++++++++++++---------
 drivers/gpu/drm/i915/intel_guc_fwif.h |  5 +++--
 drivers/gpu/drm/i915/intel_guc_log.c  | 18 +++++++-----------
 drivers/gpu/drm/i915/intel_guc_log.h  | 15 +++++++++++++++
 drivers/gpu/drm/i915/intel_uc.c       | 14 +++++++++-----
 5 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index d4c2524012fa..05c3484d02a3 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -221,17 +221,23 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
 	}
 }
 
-static u32 get_log_verbosity_flags(void)
+static u32 get_log_control_flags(void)
 {
-	if (i915_modparams.guc_log_level > 0) {
-		u32 verbosity = i915_modparams.guc_log_level - 1;
+	u32 level = i915_modparams.guc_log_level;
+	u32 flags = 0;
 
-		GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX);
-		return verbosity << GUC_LOG_VERBOSITY_SHIFT;
-	}
+	GEM_BUG_ON(level < 0);
+
+	if (!GUC_LOG_LEVEL_TO_ENABLED(level))
+		flags |= GUC_LOG_DEFAULT_DISABLED;
+
+	if (!GUC_LOG_LEVEL_TO_VERBOSE(level))
+		flags |= GUC_LOG_DISABLED;
+	else
+		flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) <<
+			 GUC_LOG_VERBOSITY_SHIFT;
 
-	GEM_BUG_ON(i915_modparams.enable_guc < 0);
-	return GUC_LOG_DISABLED;
+	return flags;
 }
 
 /*
@@ -266,7 +272,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 
 	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
 
-	params[GUC_CTL_DEBUG] = get_log_verbosity_flags();
+	params[GUC_CTL_DEBUG] = get_log_control_flags();
 
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (USES_GUC_SUBMISSION(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 6a10aa6f04d3..4971685a2ea8 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -127,7 +127,7 @@
 #define   GUC_PROFILE_ENABLED		(1 << 7)
 #define   GUC_WQ_TRACK_ENABLED		(1 << 8)
 #define   GUC_ADS_ENABLED		(1 << 9)
-#define   GUC_DEBUG_RESERVED		(1 << 10)
+#define   GUC_LOG_DEFAULT_DISABLED	(1 << 10)
 #define   GUC_ADS_ADDR_SHIFT		11
 #define   GUC_ADS_ADDR_MASK		0xfffff800
 
@@ -539,7 +539,8 @@ union guc_log_control {
 		u32 logging_enabled:1;
 		u32 reserved1:3;
 		u32 verbosity:4;
-		u32 reserved2:24;
+		u32 default_logging:1;
+		u32 reserved2:23;
 	};
 	u32 value;
 } __packed;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 1e671f2b2f64..068f5e7f7594 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -57,12 +57,14 @@ static int guc_log_flush(struct intel_guc *guc)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
-static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity)
+static int guc_log_control(struct intel_guc *guc, bool enable,
+			   bool default_logging, u32 verbosity)
 {
 	union guc_log_control control_val = {
 		{
 			.logging_enabled = enable,
 			.verbosity = verbosity,
+			.default_logging = default_logging,
 		},
 	};
 	u32 action[] = {
@@ -499,13 +501,6 @@ int intel_guc_log_level_get(struct intel_guc_log *log)
 	return i915_modparams.guc_log_level;
 }
 
-#define GUC_LOG_LEVEL_DISABLED		0
-#define LOG_LEVEL_TO_ENABLED(x)		((x) > 0)
-#define LOG_LEVEL_TO_VERBOSITY(x) ({		\
-	typeof(x) _x = (x);			\
-	LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0;	\
-})
-#define VERBOSITY_TO_LOG_LEVEL(x)  ((x) + 1)
 int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 {
 	struct intel_guc *guc = log_to_guc(log);
@@ -521,7 +516,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 	 * as indication that logging should be disabled.
 	 */
 	if (val < GUC_LOG_LEVEL_DISABLED ||
-	    val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
+	    val > GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
 		return -EINVAL;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -532,8 +527,9 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 	}
 
 	intel_runtime_pm_get(dev_priv);
-	ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val),
-			      LOG_LEVEL_TO_VERBOSITY(val));
+	ret = guc_log_control(guc, GUC_LOG_LEVEL_TO_VERBOSE(val),
+			      GUC_LOG_LEVEL_TO_ENABLED(val),
+			      GUC_LOG_LEVEL_TO_VERBOSITY(val));
 	intel_runtime_pm_put(dev_priv);
 	if (ret) {
 		DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index cc86587a0543..dab27785231b 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -40,6 +40,21 @@ struct intel_guc;
 #define GUC_LOG_SIZE	((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \
 			  1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT)
 
+/*
+ * While we're using plain log level in i915, GuC controls are much more...
+ * "elaborate"? We have a couple of bits for verbosity, separate bit for actual
+ * log enabling, and separate bit for default logging - which "conveniently"
+ * ignores the enable bit.
+ */
+#define GUC_LOG_LEVEL_DISABLED			0
+#define GUC_LOG_LEVEL_TO_ENABLED(x)		((x) > 0)
+#define GUC_LOG_LEVEL_TO_VERBOSE(x)		((x) > 1)
+#define GUC_LOG_LEVEL_TO_VERBOSITY(x) ({		\
+	typeof(x) _x = (x);				\
+	GUC_LOG_LEVEL_TO_VERBOSE(_x) ? _x - 2 : 0;	\
+})
+#define GUC_VERBOSITY_TO_LOG_LEVEL(x)		((x) + 2)
+
 struct intel_guc_log {
 	u32 flags;
 	struct i915_vma *vma;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9bb40cd047a0..ad1785522497 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -75,7 +75,8 @@ static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
 	if (HAS_GUC(dev_priv) && intel_uc_is_using_guc() &&
 	    (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
 	     IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)))
-		guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
+		guc_log_level =
+			GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX);
 
 	/* Any platform specific fine-tuning can be done here */
 
@@ -142,17 +143,20 @@ static void sanitize_options_early(struct drm_i915_private *dev_priv)
 		i915_modparams.guc_log_level = 0;
 	}
 
-	if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) {
+	if (i915_modparams.guc_log_level >
+	    GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "guc_log_level", i915_modparams.guc_log_level,
 			 "verbosity too high");
-		i915_modparams.guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
+		i915_modparams.guc_log_level =
+			GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX);
 	}
 
-	DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n",
+	DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s, verbose:%s, verbosity:%d)\n",
 			 i915_modparams.guc_log_level,
 			 yesno(i915_modparams.guc_log_level),
-			 i915_modparams.guc_log_level - 1);
+			 yesno(GUC_LOG_LEVEL_TO_VERBOSE(i915_modparams.guc_log_level)),
+			 GUC_LOG_LEVEL_TO_VERBOSITY(i915_modparams.guc_log_level));
 
 	/* Make sure that sanitization was done */
 	GEM_BUG_ON(i915_modparams.enable_guc < 0);
-- 
2.14.3

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

[PATCH v2 11/13] drm/i915/guc: Default to non-verbose GuC logging

[Michał Winiarski]

Now that we've decoupled logging from relay, GuC log level is only
controlling the GuC behavior - there shouldn't be any impact on i915
behaviour. We're only going to see a single extra interrupt when log
will get half full.
That, and the fact that we're seeing igt/gem_exec_nop/basic-series
failing with non-verbose logging being disabled.

v2: Bring back the "auto" guc_log_level, now that we fixed the log

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 drivers/gpu/drm/i915/intel_uc.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 430f5f9d0ff4..c96360398072 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -48,7 +48,7 @@ struct drm_printer;
 	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_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ad1785522497..34e847d0ee4c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -69,7 +69,7 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
 
 static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
 {
-	int guc_log_level = 0; /* disabled */
+	int guc_log_level = 1; /* non-verbose */
 
 	/* Enable if we're running on platform with GuC and debug config */
 	if (HAS_GUC(dev_priv) && intel_uc_is_using_guc() &&
-- 
2.14.3

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

[PATCH 12/13] drm/i915/guc: Demote GuC error messages

[Michał Winiarski]

We're using those functions in selftests, and the callers are expected
to do the error handling anyways. Let's demote all GuC actions and
doorbell creation to DEBUG_DRIVER.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c            | 7 ++++---
 drivers/gpu/drm/i915/intel_guc_submission.c | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 05c3484d02a3..7091dfa754ee 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -361,9 +361,10 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 		if (ret != -ETIMEDOUT)
 			ret = -EIO;
 
-		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
-			 " ret=%d status=0x%08X response=0x%08X\n",
-			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
+		DRM_DEBUG_DRIVER("INTEL_GUC_SEND: Action 0x%X failed;"
+				 " ret=%d status=0x%08X response=0x%08X\n",
+				 action[0], ret, status,
+				 I915_READ(SOFT_SCRATCH(15)));
 	}
 
 	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 33af2930fc79..207cda062626 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -231,8 +231,8 @@ static int create_doorbell(struct intel_guc_client *client)
 	if (ret) {
 		__destroy_doorbell(client);
 		__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
-		DRM_ERROR("Couldn't create client %u doorbell: %d\n",
-			  client->stage_id, ret);
+		DRM_DEBUG_DRIVER("Couldn't create client %u doorbell: %d\n",
+				 client->stage_id, ret);
 		return ret;
 	}
 
-- 
2.14.3

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

[PATCH 13/13] HAX enable guc for CI

[Michał Winiarski]

---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c96360398072..53037b5eff22 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@ struct drm_printer;
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.14.3

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC
URL   : https://patchwork.freedesktop.org/series/40170/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
129696d8e7f5 drm/i915/guc: Keep GuC interrupts enabled when using GuC
b50a23b61dec drm/i915/guc: Log runtime should consist of both mapping and relay
-:337: CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
#337: FILE: drivers/gpu/drm/i915/intel_guc_log.h:52:
+		struct mutex lock;

total: 0 errors, 0 warnings, 1 checks, 287 lines checked
89ecad78c4f1 drm/i915/guc: Merge log relay file and channel creation
bee97b156e1e drm/i915/guc: Flush directly in log unregister
bad558e92b49 drm/i915/guc: Split relay control and GuC log level
6f79d3d70452 drm/i915/guc: Move check for fast memcpy_wc to relay creation
5b89313ecb62 drm/i915/guc: Get rid of GuC log runtime
-:46: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#46: FILE: drivers/gpu/drm/i915/intel_guc.c:91:
+	guc->log.relay.flush_wq = alloc_ordered_workqueue("i915-guc_log",
 						WQ_HIGHPRI | WQ_FREEZABLE);

-:120: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#120: FILE: drivers/gpu/drm/i915/intel_guc_log.c:248:
+	log_buf_state = src_data = log->relay.buf_addr;

-:154: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "log->relay.buf_addr"
#154: FILE: drivers/gpu/drm/i915/intel_guc_log.c:346:
+	return log->relay.buf_addr != NULL;

total: 0 errors, 0 warnings, 3 checks, 257 lines checked
db91e85e0a6a drm/i915/guc: Always print log stats in i915_guc_info when using GuC
-:49: WARNING:PREFER_SEQ_PUTS: Prefer seq_puts to seq_printf
#49: FILE: drivers/gpu/drm/i915/i915_debugfs.c:2391:
+	seq_printf(m, "\nDoorbell map:\n");

total: 0 errors, 1 warnings, 0 checks, 38 lines checked
522e47939af7 drm/i915/guc: Don't print out relay statistics when relay is disabled
-:15: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#15: 
v3: Corrected one more error in stats accounting, move relay_enabled (Sagar)

-:172: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "log->relay.buf_addr"
#172: FILE: drivers/gpu/drm/i915/intel_guc_log.c:553:
+	return log->relay.buf_addr != NULL;

total: 0 errors, 1 warnings, 1 checks, 173 lines checked
3f8d2707e19e drm/i915/guc: Allow user to control default GuC logging
13e0fba2179c drm/i915/guc: Default to non-verbose GuC logging
dd4f4139736e drm/i915/guc: Demote GuC error messages
6d07a4068452 HAX enable guc for CI
-:19: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 0 checks, 8 lines checked

_______________________________________________
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,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC
URL   : https://patchwork.freedesktop.org/series/40170/
State : success

== Summary ==

Series 40170v1 series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC
https://patchwork.freedesktop.org/api/1.0/series/40170/revisions/1/mbox/

---- Known issues:

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-cfl-s2) fdo#100368
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                fail       -> PASS       (fi-cfl-s2) fdo#103481

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:443s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:542s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:296s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:510s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:515s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:503s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:413s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:580s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:513s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:525s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:427s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:317s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:402s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:420s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:476s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:430s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:476s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:468s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:518s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:653s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:441s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:538s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:538s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:510s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:485s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:429s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:580s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:398s

b3a4e70c4ef29d338f6da4bea7627e9145ef3382 drm-tip: 2018y-03m-19d-08h-13m-36s UTC integration manifest
6d07a4068452 HAX enable guc for CI
dd4f4139736e drm/i915/guc: Demote GuC error messages
13e0fba2179c drm/i915/guc: Default to non-verbose GuC logging
3f8d2707e19e drm/i915/guc: Allow user to control default GuC logging
522e47939af7 drm/i915/guc: Don't print out relay statistics when relay is disabled
db91e85e0a6a drm/i915/guc: Always print log stats in i915_guc_info when using GuC
5b89313ecb62 drm/i915/guc: Get rid of GuC log runtime
6f79d3d70452 drm/i915/guc: Move check for fast memcpy_wc to relay creation
bad558e92b49 drm/i915/guc: Split relay control and GuC log level
bee97b156e1e drm/i915/guc: Flush directly in log unregister
89ecad78c4f1 drm/i915/guc: Merge log relay file and channel creation
b50a23b61dec drm/i915/guc: Log runtime should consist of both mapping and relay
129696d8e7f5 drm/i915/guc: Keep GuC interrupts enabled when using GuC

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC
URL   : https://patchwork.freedesktop.org/series/40170/
State : failure

== Summary ==

---- Possible new issues:

Test kms_cursor_legacy:
        Subgroup cursor-vs-flip-legacy:
                pass       -> FAIL       (shard-hsw)
Test perf:
        Subgroup gen8-unprivileged-single-ctx-counters:
                pass       -> FAIL       (shard-apl)
Test pm_rc6_residency:
        Subgroup rc6-accuracy:
                pass       -> SKIP       (shard-snb)

---- Known issues:

Test drv_missed_irq:
                pass       -> SKIP       (shard-apl) fdo#103199
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check:
                pass       -> FAIL       (shard-hsw) fdo#100368 +1
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                fail       -> PASS       (shard-hsw) fdo#103925
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047

fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3442 pass:1812 dwarn:1   dfail:0   fail:8   skip:1620 time:13417s
shard-hsw        total:3442 pass:1766 dwarn:1   dfail:0   fail:3   skip:1671 time:12017s
shard-snb        total:3442 pass:1358 dwarn:1   dfail:0   fail:2   skip:2081 time:7250s
Blacklisted hosts:
shard-kbl        total:3256 pass:1812 dwarn:1   dfail:0   fail:10  skip:1429 time:9030s

== Logs ==

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

Re: [PATCH v3 01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Sagar Arun Kamble]

On 3/19/2018 3:23 PM, Michał Winiarski wrote:
> The GuC log contains a separate space used for crash dump.
> We even get a separate notification for it. While we're not handling
> crash differently yet, it makes sense to decouple the two right now to
> simplify the following patches.
>
> v2: Move guc_log_flush_irq_disable up to avoid movement in following
>      patches (Sagar).
> v3: s/guc_log_flush_irq_*/guc_flush_log_msg_*, rebase after mass rename
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v2)
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.c     | 25 ++++++++++---------------
>   drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>   drivers/gpu/drm/i915/intel_guc_log.c | 31 +++++++++++++++++++------------
>   drivers/gpu/drm/i915/intel_uc.c      | 14 +++++---------
>   4 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index e70bf654d21e..3af603536b1b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -67,6 +67,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>   	intel_guc_log_init_early(&guc->log);
>   
>   	mutex_init(&guc->send_mutex);
> +	spin_lock_init(&guc->irq_lock);
>   	guc->send = intel_guc_send_nop;
>   	guc->notify = gen8_guc_raise_irq;
>   }
> @@ -368,7 +369,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>   void intel_guc_to_host_event_handler(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 msg, flush;
> +	u32 msg, val;
>   
>   	/*
>   	 * Sample the log buffer flush related bits & clear them out now
> @@ -381,24 +382,18 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
>   	 * could happen that GuC sets the bit for 2nd interrupt but Host
>   	 * clears out the bit on handling the 1st interrupt.
>   	 */
> -
> -	msg = I915_READ(SOFT_SCRATCH(15));
> -	flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
> -		       INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
> -	if (flush) {
> -		/* Clear the message bits that are handled */
> -		I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
> -
> -		/* Handle flush interrupt in bottom half */
> +	spin_lock(&guc->irq_lock);
> +	val = I915_READ(SOFT_SCRATCH(15));
> +	msg = val & guc->msg_enabled_mask;
> +	I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
> +	spin_unlock(&guc->irq_lock);
> +
> +	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> +		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
>   		queue_work(guc->log.runtime.flush_wq,
>   			   &guc->log.runtime.flush_work);
>   
>   		guc->log.flush_interrupt_count++;
> -	} else {
> -		/*
> -		 * Not clearing of unhandled event bits won't result in
> -		 * re-triggering of the interrupt.
> -		 */
>   	}
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index cdb649a9a4cf..9a95d1518aa9 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -56,7 +56,9 @@ struct intel_guc {
>   	struct drm_i915_gem_object *load_err_log;
>   
>   	/* intel_guc_recv interrupt related state */
> +	spinlock_t irq_lock;
>   	bool interrupts_enabled;
> +	unsigned int msg_enabled_mask;
>   
>   	struct i915_vma *ads_vma;
>   	struct i915_vma *stage_desc_pool;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 1c2127bc3878..1e209fcf90e1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -73,6 +73,22 @@ static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity)
>   	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>   }
>   
> +static void guc_flush_log_msg_enable(struct intel_guc *guc)
> +{
> +	spin_lock_irq(&guc->irq_lock);
> +	guc->msg_enabled_mask |= INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> +				 INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED;
> +	spin_unlock_irq(&guc->irq_lock);
> +}
> +
> +static void guc_flush_log_msg_disable(struct intel_guc *guc)
> +{
> +	spin_lock_irq(&guc->irq_lock);
> +	guc->msg_enabled_mask &= ~(INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> +				   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED);
> +	spin_unlock_irq(&guc->irq_lock);
> +}
> +
>   static inline struct intel_guc *log_to_guc(struct intel_guc_log *log)
>   {
>   	return container_of(log, struct intel_guc, log);
> @@ -709,12 +725,7 @@ int intel_guc_log_register(struct intel_guc_log *log)
>   	if (ret)
>   		goto err_runtime;
>   
> -	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	mutex_lock(&i915->drm.struct_mutex);
> -	intel_runtime_pm_get(i915);
> -	gen9_enable_guc_interrupts(i915);
> -	intel_runtime_pm_put(i915);
> -	mutex_unlock(&i915->drm.struct_mutex);
> +	guc_flush_log_msg_enable(guc);
>   
>   	return 0;
>   
> @@ -733,6 +744,8 @@ void intel_guc_log_unregister(struct intel_guc_log *log)
>   	struct intel_guc *guc = log_to_guc(log);
>   	struct drm_i915_private *i915 = guc_to_i915(guc);
>   
> +	guc_flush_log_msg_disable(guc);
> +
>   	/*
>   	 * Once logging is disabled, GuC won't generate logs & send an
>   	 * interrupt. But there could be some data in the log buffer
> @@ -742,12 +755,6 @@ void intel_guc_log_unregister(struct intel_guc_log *log)
>   	guc_flush_logs(log);
>   
>   	mutex_lock(&i915->drm.struct_mutex);
> -
> -	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	intel_runtime_pm_get(i915);
> -	gen9_disable_guc_interrupts(i915);
> -	intel_runtime_pm_put(i915);
> -
>   	guc_log_runtime_destroy(log);
>   	mutex_unlock(&i915->drm.struct_mutex);
>   
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 104c03ae2742..765b86a53f19 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -247,6 +247,8 @@ static int guc_enable_communication(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
> +	gen9_enable_guc_interrupts(dev_priv);
> +
>   	if (HAS_GUC_CT(dev_priv))
>   		return intel_guc_enable_ct(guc);
>   
> @@ -261,6 +263,8 @@ static void guc_disable_communication(struct intel_guc *guc)
>   	if (HAS_GUC_CT(dev_priv))
>   		intel_guc_disable_ct(guc);
>   
> +	gen9_disable_guc_interrupts(dev_priv);
> +
>   	guc->send = intel_guc_send_nop;
>   }
>   
> @@ -413,12 +417,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	}
>   
>   	if (USES_GUC_SUBMISSION(dev_priv)) {
> -		if (i915_modparams.guc_log_level)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
>   		ret = intel_guc_submission_enable(guc);
>   		if (ret)
> -			goto err_interrupts;
> +			goto err_communication;
>   	}
>   
>   	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
> @@ -433,8 +434,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	/*
>   	 * We've failed to load the firmware :(
>   	 */
> -err_interrupts:
> -	gen9_disable_guc_interrupts(dev_priv);
>   err_communication:
>   	guc_disable_communication(guc);
>   err_log_capture:
> @@ -464,9 +463,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   		intel_guc_submission_disable(guc);
>   
>   	guc_disable_communication(guc);
> -
> -	if (USES_GUC_SUBMISSION(dev_priv))
> -		gen9_disable_guc_interrupts(dev_priv);
>   }
>   
>   int intel_uc_suspend(struct drm_i915_private *i915)

-- 
Thanks,
Sagar

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

Re: [PATCH v3 10/13] drm/i915/guc: Allow user to control default GuC logging

[Sagar Arun Kamble]

On 3/19/2018 3:23 PM, Michał Winiarski wrote:
> While both naming and actual log enable logic in GuC interface are
> confusing, we can simply expose the default log as yet another log
> level.
> GuC logic aside, from i915 point of view we now have the following GuC
> log levels:
> 	0 Log disabled
> 	1 Non-verbose log
> 	2-5 Verbose log
>
> v2: Adjust naming after rebase.
> v3: Fixed the log_level logic error introduced on rebase.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v2)
> ---
>   drivers/gpu/drm/i915/intel_guc.c      | 24 +++++++++++++++---------
>   drivers/gpu/drm/i915/intel_guc_fwif.h |  5 +++--
>   drivers/gpu/drm/i915/intel_guc_log.c  | 18 +++++++-----------
>   drivers/gpu/drm/i915/intel_guc_log.h  | 15 +++++++++++++++
>   drivers/gpu/drm/i915/intel_uc.c       | 14 +++++++++-----
>   5 files changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index d4c2524012fa..05c3484d02a3 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -221,17 +221,23 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
>   	}
>   }
>   
> -static u32 get_log_verbosity_flags(void)
> +static u32 get_log_control_flags(void)
>   {
> -	if (i915_modparams.guc_log_level > 0) {
> -		u32 verbosity = i915_modparams.guc_log_level - 1;
> +	u32 level = i915_modparams.guc_log_level;
> +	u32 flags = 0;
>   
> -		GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX);
> -		return verbosity << GUC_LOG_VERBOSITY_SHIFT;
> -	}
> +	GEM_BUG_ON(level < 0);
> +
> +	if (!GUC_LOG_LEVEL_TO_ENABLED(level))
> +		flags |= GUC_LOG_DEFAULT_DISABLED;
> +
> +	if (!GUC_LOG_LEVEL_TO_VERBOSE(level))
> +		flags |= GUC_LOG_DISABLED;
> +	else
> +		flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) <<
> +			 GUC_LOG_VERBOSITY_SHIFT;
>   
> -	GEM_BUG_ON(i915_modparams.enable_guc < 0);
> -	return GUC_LOG_DISABLED;
> +	return flags;
>   }
>   
>   /*
> @@ -266,7 +272,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>   
>   	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
>   
> -	params[GUC_CTL_DEBUG] = get_log_verbosity_flags();
> +	params[GUC_CTL_DEBUG] = get_log_control_flags();
>   
>   	/* If GuC submission is enabled, set up additional parameters here */
>   	if (USES_GUC_SUBMISSION(dev_priv)) {
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 6a10aa6f04d3..4971685a2ea8 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -127,7 +127,7 @@
>   #define   GUC_PROFILE_ENABLED		(1 << 7)
>   #define   GUC_WQ_TRACK_ENABLED		(1 << 8)
>   #define   GUC_ADS_ENABLED		(1 << 9)
> -#define   GUC_DEBUG_RESERVED		(1 << 10)
> +#define   GUC_LOG_DEFAULT_DISABLED	(1 << 10)
>   #define   GUC_ADS_ADDR_SHIFT		11
>   #define   GUC_ADS_ADDR_MASK		0xfffff800
>   
> @@ -539,7 +539,8 @@ union guc_log_control {
>   		u32 logging_enabled:1;
>   		u32 reserved1:3;
>   		u32 verbosity:4;
> -		u32 reserved2:24;
> +		u32 default_logging:1;
> +		u32 reserved2:23;
>   	};
>   	u32 value;
>   } __packed;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 1e671f2b2f64..068f5e7f7594 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -57,12 +57,14 @@ static int guc_log_flush(struct intel_guc *guc)
>   	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>   }
>   
> -static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity)
> +static int guc_log_control(struct intel_guc *guc, bool enable,
> +			   bool default_logging, u32 verbosity)
Can we change the order of parameters as:
struct intel_guc *guc, bool default_logging, bool enable, u32 verbosity

That way I can see that param 3 and param 4 are related.
But this change can be done at your discretion.

Updated logic to handle default logging looks fine.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>   {
>   	union guc_log_control control_val = {
>   		{
>   			.logging_enabled = enable,
>   			.verbosity = verbosity,
> +			.default_logging = default_logging,
>   		},
>   	};
>   	u32 action[] = {
> @@ -499,13 +501,6 @@ int intel_guc_log_level_get(struct intel_guc_log *log)
>   	return i915_modparams.guc_log_level;
>   }
>   
> -#define GUC_LOG_LEVEL_DISABLED		0
> -#define LOG_LEVEL_TO_ENABLED(x)		((x) > 0)
> -#define LOG_LEVEL_TO_VERBOSITY(x) ({		\
> -	typeof(x) _x = (x);			\
> -	LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0;	\
> -})
> -#define VERBOSITY_TO_LOG_LEVEL(x)  ((x) + 1)
>   int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
>   {
>   	struct intel_guc *guc = log_to_guc(log);
> @@ -521,7 +516,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
>   	 * as indication that logging should be disabled.
>   	 */
>   	if (val < GUC_LOG_LEVEL_DISABLED ||
> -	    val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
> +	    val > GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
>   		return -EINVAL;
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -532,8 +527,9 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
>   	}
>   
>   	intel_runtime_pm_get(dev_priv);
> -	ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val),
> -			      LOG_LEVEL_TO_VERBOSITY(val));
> +	ret = guc_log_control(guc, GUC_LOG_LEVEL_TO_VERBOSE(val),
> +			      GUC_LOG_LEVEL_TO_ENABLED(val),
> +			      GUC_LOG_LEVEL_TO_VERBOSITY(val));
>   	intel_runtime_pm_put(dev_priv);
>   	if (ret) {
>   		DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index cc86587a0543..dab27785231b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -40,6 +40,21 @@ struct intel_guc;
>   #define GUC_LOG_SIZE	((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \
>   			  1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT)
>   
> +/*
> + * While we're using plain log level in i915, GuC controls are much more...
> + * "elaborate"? We have a couple of bits for verbosity, separate bit for actual
> + * log enabling, and separate bit for default logging - which "conveniently"
> + * ignores the enable bit.
> + */
> +#define GUC_LOG_LEVEL_DISABLED			0
> +#define GUC_LOG_LEVEL_TO_ENABLED(x)		((x) > 0)
> +#define GUC_LOG_LEVEL_TO_VERBOSE(x)		((x) > 1)
> +#define GUC_LOG_LEVEL_TO_VERBOSITY(x) ({		\
> +	typeof(x) _x = (x);				\
> +	GUC_LOG_LEVEL_TO_VERBOSE(_x) ? _x - 2 : 0;	\
> +})
> +#define GUC_VERBOSITY_TO_LOG_LEVEL(x)		((x) + 2)
> +
>   struct intel_guc_log {
>   	u32 flags;
>   	struct i915_vma *vma;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 9bb40cd047a0..ad1785522497 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -75,7 +75,8 @@ static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
>   	if (HAS_GUC(dev_priv) && intel_uc_is_using_guc() &&
>   	    (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
>   	     IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)))
> -		guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
> +		guc_log_level =
> +			GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX);
>   
>   	/* Any platform specific fine-tuning can be done here */
>   
> @@ -142,17 +143,20 @@ static void sanitize_options_early(struct drm_i915_private *dev_priv)
>   		i915_modparams.guc_log_level = 0;
>   	}
>   
> -	if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) {
> +	if (i915_modparams.guc_log_level >
> +	    GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) {
>   		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>   			 "guc_log_level", i915_modparams.guc_log_level,
>   			 "verbosity too high");
> -		i915_modparams.guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
> +		i915_modparams.guc_log_level =
> +			GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX);
>   	}
>   
> -	DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n",
> +	DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s, verbose:%s, verbosity:%d)\n",
>   			 i915_modparams.guc_log_level,
>   			 yesno(i915_modparams.guc_log_level),
> -			 i915_modparams.guc_log_level - 1);
> +			 yesno(GUC_LOG_LEVEL_TO_VERBOSE(i915_modparams.guc_log_level)),
> +			 GUC_LOG_LEVEL_TO_VERBOSITY(i915_modparams.guc_log_level));
>   
>   	/* Make sure that sanitization was done */
>   	GEM_BUG_ON(i915_modparams.enable_guc < 0);

-- 
Thanks,
Sagar

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

Re: [PATCH 12/13] drm/i915/guc: Demote GuC error messages

[Chris Wilson]

Quoting Michał Winiarski (2018-03-19 09:53:47)
> We're using those functions in selftests, and the callers are expected
> to do the error handling anyways. Let's demote all GuC actions and
> doorbell creation to DEBUG_DRIVER.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Ok,
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

Re: [PATCH v3 01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Michal Wajdeczko]

On Mon, 19 Mar 2018 10:53:36 +0100, Michał Winiarski  
<michal.winiarski@intel.com> wrote:

> The GuC log contains a separate space used for crash dump.
> We even get a separate notification for it. While we're not handling
> crash differently yet, it makes sense to decouple the two right now to
> simplify the following patches.
>
> v2: Move guc_log_flush_irq_disable up to avoid movement in following
>     patches (Sagar).
> v3: s/guc_log_flush_irq_*/guc_flush_log_msg_*, rebase after mass rename
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v2)
> ---
>  drivers/gpu/drm/i915/intel_guc.c     | 25 ++++++++++---------------
>  drivers/gpu/drm/i915/intel_guc.h     |  2 ++
>  drivers/gpu/drm/i915/intel_guc_log.c | 31  
> +++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_uc.c      | 14 +++++---------
>  4 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index e70bf654d21e..3af603536b1b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -67,6 +67,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>  	intel_guc_log_init_early(&guc->log);
> 	mutex_init(&guc->send_mutex);
> +	spin_lock_init(&guc->irq_lock);

please try to init members in definition order,
and try to avoid putting inside other init groups
(here 'send' related members)

>  	guc->send = intel_guc_send_nop;
>  	guc->notify = gen8_guc_raise_irq;
>  }
> @@ -368,7 +369,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const  
> u32 *action, u32 len)
>  void intel_guc_to_host_event_handler(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 msg, flush;
> +	u32 msg, val;
> 	/*
>  	 * Sample the log buffer flush related bits & clear them out now
> @@ -381,24 +382,18 @@ void intel_guc_to_host_event_handler(struct  
> intel_guc *guc)
>  	 * could happen that GuC sets the bit for 2nd interrupt but Host
>  	 * clears out the bit on handling the 1st interrupt.
>  	 */
> -
> -	msg = I915_READ(SOFT_SCRATCH(15));
> -	flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
> -		       INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
> -	if (flush) {
> -		/* Clear the message bits that are handled */
> -		I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
> -
> -		/* Handle flush interrupt in bottom half */
> +	spin_lock(&guc->irq_lock);
> +	val = I915_READ(SOFT_SCRATCH(15));
> +	msg = val & guc->msg_enabled_mask;
> +	I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
> +	spin_unlock(&guc->irq_lock);
> +
> +	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> +		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
>  		queue_work(guc->log.runtime.flush_wq,
>  			   &guc->log.runtime.flush_work);
> 		guc->log.flush_interrupt_count++;
> -	} else {
> -		/*
> -		 * Not clearing of unhandled event bits won't result in
> -		 * re-triggering of the interrupt.
> -		 */
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index cdb649a9a4cf..9a95d1518aa9 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -56,7 +56,9 @@ struct intel_guc {
>  	struct drm_i915_gem_object *load_err_log;
> 	/* intel_guc_recv interrupt related state */
> +	spinlock_t irq_lock;
>  	bool interrupts_enabled;
> +	unsigned int msg_enabled_mask;
> 	struct i915_vma *ads_vma;
>  	struct i915_vma *stage_desc_pool;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 1c2127bc3878..1e209fcf90e1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -73,6 +73,22 @@ static int guc_log_control(struct intel_guc *guc,  
> bool enable, u32 verbosity)
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
> +static void guc_flush_log_msg_enable(struct intel_guc *guc)
> +{
> +	spin_lock_irq(&guc->irq_lock);
> +	guc->msg_enabled_mask |= INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> +				 INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED;
> +	spin_unlock_irq(&guc->irq_lock);
> +}
> +
> +static void guc_flush_log_msg_disable(struct intel_guc *guc)
> +{
> +	spin_lock_irq(&guc->irq_lock);
> +	guc->msg_enabled_mask &= ~(INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> +				   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED);
> +	spin_unlock_irq(&guc->irq_lock);
> +}
> +

hmm, these functions take "guc" and operate on "struct guc",
so why they are located in intel_guc_log.c ?

maybe better option would be to declare them as

intel_guc.h:

static inline void intel_guc_event_enable(struct intel_guc *guc, u32 mask)
{
	spin_lock_irq(&guc->irq_lock);
	guc->msg_enabled_mask |= mask;
	spin_unlock_irq(&guc->irq_lock);
}

intel_guc_log.c:

static void guc_log_enable_flush(struct intel_guc_log *log)
{
	intel_guc_event_enable(log_to_guc(log),
			INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
			INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED);
}

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

Re: ✗ Fi.CI.IGT: failure for series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC

[Chris Wilson]

Quoting Patchwork (2018-03-19 11:08:45)
> == Series Details ==
> 
> Series: series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC
> URL   : https://patchwork.freedesktop.org/series/40170/
> State : failure
> 
> == Summary ==
> 
> ---- Possible new issues:
> 
> Test kms_cursor_legacy:
>         Subgroup cursor-vs-flip-legacy:
>                 pass       -> FAIL       (shard-hsw)
> Test perf:
>         Subgroup gen8-unprivileged-single-ctx-counters:
>                 pass       -> FAIL       (shard-apl)
> Test pm_rc6_residency:
>         Subgroup rc6-accuracy:
>                 pass       -> SKIP       (shard-snb)
> 
> ---- Known issues:
> 
> Test drv_missed_irq:
>                 pass       -> SKIP       (shard-apl) fdo#103199

Oh cibuglog, no this is not that bug.

> Test kms_flip:
>         Subgroup 2x-plain-flip-ts-check:
>                 pass       -> FAIL       (shard-hsw) fdo#100368 +1
> Test kms_rotation_crc:
>         Subgroup primary-rotation-180:
>                 fail       -> PASS       (shard-hsw) fdo#103925
> Test kms_sysfs_edid_timing:
>                 pass       -> WARN       (shard-apl) fdo#100047
> 
> fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
> fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
> fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
> fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

Pushed thanks for the patches and review,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx