[PATCH v2 01/15] drm/i915/guc: Tidy guc_log_control

[PATCH v2 01/15] drm/i915/guc: Tidy guc_log_control

[Michał Winiarski]

We plan to decouple log runtime (mapping + relay) from verbosity control.
Let's tidy the code now to reduce the churn in the following patches.

v2: Tidy macros, keep debug messages, use helper var for enable,
    correct typo (Michał)
    Fix incorrect input validaction (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: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 11 ++---
 drivers/gpu/drm/i915/intel_guc_log.c | 80 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_guc_log.h |  3 +-
 3 files changed, 53 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 89f7ff2c652e..fa0755fe10d0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2500,13 +2500,10 @@ static int i915_guc_log_control_get(void *data, u64 *val)
 {
 	struct drm_i915_private *dev_priv = data;
 
-	if (!HAS_GUC(dev_priv))
+	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	if (!dev_priv->guc.log.vma)
-		return -EINVAL;
-
-	*val = i915_modparams.guc_log_level;
+	*val = intel_guc_log_control_get(&dev_priv->guc);
 
 	return 0;
 }
@@ -2515,10 +2512,10 @@ static int i915_guc_log_control_set(void *data, u64 val)
 {
 	struct drm_i915_private *dev_priv = data;
 
-	if (!HAS_GUC(dev_priv))
+	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	return intel_guc_log_control(&dev_priv->guc, val);
+	return intel_guc_log_control_set(&dev_priv->guc, val);
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index c0c2e7d1c7d7..7e59fb07b06b 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -659,51 +659,63 @@ void intel_guc_log_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
-int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
+int intel_guc_log_control_get(struct intel_guc *guc)
+{
+	GEM_BUG_ON(!guc->log.vma);
+	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
+
+	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_control_set(struct intel_guc *guc, u64 val)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	bool enable_logging = control_val > 0;
-	u32 verbosity;
+	bool enabled = LOG_LEVEL_TO_ENABLED(val);
 	int ret;
 
-	if (!guc->log.vma)
-		return -ENODEV;
+	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
+	GEM_BUG_ON(!guc->log.vma);
+	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
 
-	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
-	if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
+	/*
+	 * GuC is recognizing log levels starting from 0 to max, we're using 0
+	 * as indication that logging should be disabled.
+	 */
+	if (val < GUC_LOG_LEVEL_DISABLED ||
+	    val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
 		return -EINVAL;
 
-	/* This combination doesn't make sense & won't have any effect */
-	if (!enable_logging && !i915_modparams.guc_log_level)
-		return 0;
+	mutex_lock(&dev_priv->drm.struct_mutex);
 
-	verbosity = enable_logging ? control_val - 1 : 0;
+	if (i915_modparams.guc_log_level == val) {
+		ret = 0;
+		goto out_unlock;
+	}
 
-	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
-	if (ret)
-		return ret;
 	intel_runtime_pm_get(dev_priv);
-	ret = guc_log_control(guc, enable_logging, verbosity);
+	ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
 	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	if (ret < 0) {
-		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
-		return ret;
+	if (ret) {
+		DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
+		goto out_unlock;
 	}
 
-	if (enable_logging) {
-		i915_modparams.guc_log_level = 1 + verbosity;
+	i915_modparams.guc_log_level = val;
 
-		/*
-		 * If log was disabled at boot time, then the relay channel file
-		 * wouldn't have been created by now and interrupts also would
-		 * not have been enabled. Try again now, just in case.
-		 */
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	if (enabled && !guc_log_has_runtime(guc)) {
 		ret = guc_log_late_setup(guc);
-		if (ret < 0) {
+		if (ret) {
 			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
-			return ret;
+			goto out;
 		}
 
 		/* GuC logging is currently the only user of Guc2Host interrupts */
@@ -712,7 +724,7 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
 		gen9_enable_guc_interrupts(dev_priv);
 		intel_runtime_pm_put(dev_priv);
 		mutex_unlock(&dev_priv->drm.struct_mutex);
-	} else {
+	} else if (!enabled && guc_log_has_runtime(guc)) {
 		/*
 		 * Once logging is disabled, GuC won't generate logs & send an
 		 * interrupt. But there could be some data in the log buffer
@@ -720,11 +732,13 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
 		 * buffer state and then collect the left over logs.
 		 */
 		guc_flush_logs(guc);
-
-		/* As logging is disabled, update log level to reflect that */
-		i915_modparams.guc_log_level = 0;
 	}
 
+	return 0;
+
+out_unlock:
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+out:
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index dab0e949567a..141ce9ca22ce 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -64,7 +64,8 @@ void intel_guc_log_destroy(struct intel_guc *guc);
 void intel_guc_log_init_early(struct intel_guc *guc);
 int intel_guc_log_relay_create(struct intel_guc *guc);
 void intel_guc_log_relay_destroy(struct intel_guc *guc);
-int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
+int intel_guc_log_control_get(struct intel_guc *guc);
+int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
 
-- 
2.14.3

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

[PATCH v2 02/15] drm/i915/guc: Create common entry points for log register/unregister

[Michał Winiarski]

We have many functions responsible for allocating different parts of
GuC log runtime called from multiple places. Let's stick with keeping
everything in guc_log_register instead.

v2: Use more generic intel_uc_register name, keep using "misc" suffix (Michał)
    s/dev_priv/i915 (Sagar)
    Make guc_log_relay_* static (sparse)

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: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |   6 +-
 drivers/gpu/drm/i915/intel_guc_log.c | 156 ++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_guc_log.h |   6 +-
 drivers/gpu/drm/i915/intel_uc.c      |  41 +++++----
 drivers/gpu/drm/i915/intel_uc.h      |   2 +
 5 files changed, 95 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d7c4de45644d..987c6770d1a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1238,9 +1238,11 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	/* Reveal our presence to userspace */
 	if (drm_dev_register(dev, 0) == 0) {
 		i915_debugfs_register(dev_priv);
-		i915_guc_log_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
@@ -1298,7 +1300,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	i915_pmu_unregister(dev_priv);
 
 	i915_teardown_sysfs(dev_priv);
-	i915_guc_log_unregister(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 7e59fb07b06b..90b395f34808 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -443,7 +443,7 @@ void intel_guc_log_init_early(struct intel_guc *guc)
 	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
 }
 
-int intel_guc_log_relay_create(struct intel_guc *guc)
+static int guc_log_relay_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct rchan *guc_log_relay_chan;
@@ -496,7 +496,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc)
 	return ret;
 }
 
-void intel_guc_log_relay_destroy(struct intel_guc *guc)
+static void guc_log_relay_destroy(struct intel_guc *guc)
 {
 	mutex_lock(&guc->log.runtime.relay_lock);
 
@@ -514,49 +514,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc)
 	mutex_unlock(&guc->log.runtime.relay_lock);
 }
 
-static int guc_log_late_setup(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	int ret;
-
-	if (!guc_log_has_runtime(guc)) {
-		/*
-		 * If log was disabled at boot time, then setup needed to handle
-		 * log buffer flush interrupts would not have been done yet, so
-		 * do that now.
-		 */
-		ret = intel_guc_log_relay_create(guc);
-		if (ret)
-			goto err;
-
-		mutex_lock(&dev_priv->drm.struct_mutex);
-		intel_runtime_pm_get(dev_priv);
-		ret = guc_log_runtime_create(guc);
-		intel_runtime_pm_put(dev_priv);
-		mutex_unlock(&dev_priv->drm.struct_mutex);
-
-		if (ret)
-			goto err_relay;
-	}
-
-	ret = guc_log_relay_file_create(guc);
-	if (ret)
-		goto err_runtime;
-
-	return 0;
-
-err_runtime:
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_log_runtime_destroy(guc);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-err_relay:
-	intel_guc_log_relay_destroy(guc);
-err:
-	/* logging will remain off */
-	i915_modparams.guc_log_level = 0;
-	return ret;
-}
-
 static void guc_log_capture_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -576,16 +533,6 @@ static void guc_flush_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
-		return;
-
-	/* First disable the interrupts, will be renabled afterwards */
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	intel_runtime_pm_get(dev_priv);
-	gen9_disable_guc_interrupts(dev_priv);
-	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
 	/*
 	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
@@ -628,12 +575,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	guc->log.vma = vma;
 
-	if (i915_modparams.guc_log_level) {
-		ret = guc_log_runtime_create(guc);
-		if (ret < 0)
-			goto err_vma;
-	}
-
 	/* each allocated unit is a page */
 	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
 		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
@@ -645,8 +586,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	return 0;
 
-err_vma:
-	i915_vma_unpin_and_release(&guc->log.vma);
 err:
 	/* logging will be off */
 	i915_modparams.guc_log_level = 0;
@@ -712,26 +651,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
 	if (enabled && !guc_log_has_runtime(guc)) {
-		ret = guc_log_late_setup(guc);
+		ret = intel_guc_log_register(guc);
 		if (ret) {
-			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
+			/* logging will remain off */
+			i915_modparams.guc_log_level = 0;
 			goto out;
 		}
-
-		/* GuC logging is currently the only user of Guc2Host interrupts */
-		mutex_lock(&dev_priv->drm.struct_mutex);
-		intel_runtime_pm_get(dev_priv);
-		gen9_enable_guc_interrupts(dev_priv);
-		intel_runtime_pm_put(dev_priv);
-		mutex_unlock(&dev_priv->drm.struct_mutex);
 	} else if (!enabled && guc_log_has_runtime(guc)) {
-		/*
-		 * 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.
-		 */
-		guc_flush_logs(guc);
+		intel_guc_log_unregister(guc);
 	}
 
 	return 0;
@@ -742,29 +669,72 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
 	return ret;
 }
 
-void i915_guc_log_register(struct drm_i915_private *dev_priv)
+int intel_guc_log_register(struct intel_guc *guc)
 {
-	if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
-		return;
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+	int ret;
+
+	GEM_BUG_ON(guc_log_has_runtime(guc));
+
+	/*
+	 * If log was disabled at boot time, then setup needed to handle
+	 * log buffer flush interrupts would not have been done yet, so
+	 * do that now.
+	 */
+	ret = guc_log_relay_create(guc);
+	if (ret)
+		goto err;
+
+	mutex_lock(&i915->drm.struct_mutex);
+	ret = guc_log_runtime_create(guc);
+	mutex_unlock(&i915->drm.struct_mutex);
+
+	if (ret)
+		goto err_relay;
+
+	ret = guc_log_relay_file_create(guc);
+	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);
+
+	return 0;
 
-	guc_log_late_setup(&dev_priv->guc);
+err_runtime:
+	mutex_lock(&i915->drm.struct_mutex);
+	guc_log_runtime_destroy(guc);
+	mutex_unlock(&i915->drm.struct_mutex);
+err_relay:
+	guc_log_relay_destroy(guc);
+err:
+	return ret;
 }
 
-void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
+void intel_guc_log_unregister(struct intel_guc *guc)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct drm_i915_private *i915 = guc_to_i915(guc);
 
-	if (!USES_GUC_SUBMISSION(dev_priv))
-		return;
+	/*
+	 * 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.
+	 */
+	guc_flush_logs(guc);
 
-	mutex_lock(&dev_priv->drm.struct_mutex);
+	mutex_lock(&i915->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
-	intel_runtime_pm_get(dev_priv);
-	gen9_disable_guc_interrupts(dev_priv);
-	intel_runtime_pm_put(dev_priv);
+	intel_runtime_pm_get(i915);
+	gen9_disable_guc_interrupts(i915);
+	intel_runtime_pm_put(i915);
 
 	guc_log_runtime_destroy(guc);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
+	mutex_unlock(&i915->drm.struct_mutex);
 
-	intel_guc_log_relay_destroy(guc);
+	guc_log_relay_destroy(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 141ce9ca22ce..09dd2ef1933d 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -62,11 +62,9 @@ struct intel_guc_log {
 int intel_guc_log_create(struct intel_guc *guc);
 void intel_guc_log_destroy(struct intel_guc *guc);
 void intel_guc_log_init_early(struct intel_guc *guc);
-int intel_guc_log_relay_create(struct intel_guc *guc);
-void intel_guc_log_relay_destroy(struct intel_guc *guc);
 int intel_guc_log_control_get(struct intel_guc *guc);
 int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
-void i915_guc_log_register(struct drm_i915_private *dev_priv);
-void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
+int intel_guc_log_register(struct intel_guc *guc);
+void intel_guc_log_unregister(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index e5bf0d37bf43..1c1a00df010b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -219,6 +219,28 @@ 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);
+
+	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);
+}
+
 static int guc_enable_communication(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -249,23 +271,10 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv)
 		return 0;
 
 	ret = intel_guc_init_wq(guc);
-	if (ret) {
-		DRM_ERROR("Couldn't allocate workqueues for GuC\n");
-		goto err;
-	}
-
-	ret = intel_guc_log_relay_create(guc);
-	if (ret) {
-		DRM_ERROR("Couldn't allocate relay for GuC log\n");
-		goto err_relay;
-	}
+	if (ret)
+		return ret;
 
 	return 0;
-
-err_relay:
-	intel_guc_fini_wq(guc);
-err:
-	return ret;
 }
 
 void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
@@ -276,8 +285,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
 		return;
 
 	intel_guc_fini_wq(guc);
-
-	intel_guc_log_relay_destroy(guc);
 }
 
 int intel_uc_init(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index f76d51d1ce70..d6af984cd789 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -31,6 +31,8 @@
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 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 03/15] drm/i915/guc: Move GuC notification handling to separate function

[Michał Winiarski]

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

To allow future code reuse. While here, fix comment style.

v2: Notifications are a separate thing - rename the handler (Sagar)

Suggested-by: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
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: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 33 ++-------------------------------
 drivers/gpu/drm/i915/intel_guc.c | 37 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h |  1 +
 3 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 633c18785c1e..6b0cd6bc83f8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1766,37 +1766,8 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 
 static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
 {
-	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
-		/* Sample the log buffer flush related bits & clear them out now
-		 * itself from the message identity register to minimize the
-		 * probability of losing a flush interrupt, when there are back
-		 * to back flush interrupts.
-		 * There can be a new flush interrupt, for different log buffer
-		 * type (like for ISR), whilst Host is handling one (for DPC).
-		 * Since same bit is used in message register for ISR & DPC, it
-		 * could happen that GuC sets the bit for 2nd interrupt but Host
-		 * clears out the bit on handling the 1st interrupt.
-		 */
-		u32 msg, flush;
-
-		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 */
-			queue_work(dev_priv->guc.log.runtime.flush_wq,
-				   &dev_priv->guc.log.runtime.flush_work);
-
-			dev_priv->guc.log.flush_interrupt_count++;
-		} else {
-			/* Not clearing of unhandled event bits won't result in
-			 * re-triggering of the interrupt.
-			 */
-		}
-	}
+	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT)
+		intel_guc_to_host_event_handler(&dev_priv->guc);
 }
 
 static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index ff08ea0ebf49..25f92291fd40 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -364,6 +364,43 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	return ret;
 }
 
+void intel_guc_to_host_event_handler(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	u32 msg, flush;
+
+	/*
+	 * Sample the log buffer flush related bits & clear them out now
+	 * itself from the message identity register to minimize the
+	 * probability of losing a flush interrupt, when there are back
+	 * to back flush interrupts.
+	 * There can be a new flush interrupt, for different log buffer
+	 * type (like for ISR), whilst Host is handling one (for DPC).
+	 * Since same bit is used in message register for ISR & DPC, it
+	 * 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 */
+		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.
+		 */
+	}
+}
+
 int intel_guc_sample_forcewake(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b9424ac644ac..6d5aebe55039 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -125,6 +125,7 @@ int intel_guc_init(struct intel_guc *guc);
 void intel_guc_fini(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+void intel_guc_to_host_event_handler(struct intel_guc *guc);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 int intel_guc_suspend(struct intel_guc *guc);
-- 
2.14.3

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

[PATCH v2 04/15] 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).

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.c     | 25 ++++++++++---------------
 drivers/gpu/drm/i915/intel_guc.h     |  2 ++
 drivers/gpu/drm/i915/intel_guc_log.c | 30 +++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_uc.c      | 14 +++++---------
 4 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 25f92291fd40..0d92caf6a83f 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);
 
 	mutex_init(&guc->send_mutex);
+	spin_lock_init(&guc->irq_lock);
 	guc->send = intel_guc_send_nop;
 	guc->notify = gen8_guc_raise_irq;
 }
@@ -367,7 +368,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
@@ -380,24 +381,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 6d5aebe55039..78d9840cd309 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -53,7 +53,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 90b395f34808..89cb3939467f 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -592,6 +592,22 @@ int intel_guc_log_create(struct intel_guc *guc)
 	return ret;
 }
 
+static void guc_log_flush_irq_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_log_flush_irq_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);
+}
+
 void intel_guc_log_destroy(struct intel_guc *guc)
 {
 	guc_log_runtime_destroy(guc);
@@ -696,12 +712,7 @@ int intel_guc_log_register(struct intel_guc *guc)
 	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_log_flush_irq_enable(guc);
 
 	return 0;
 
@@ -719,6 +730,8 @@ void intel_guc_log_unregister(struct intel_guc *guc)
 {
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 
+	guc_log_flush_irq_disable(guc);
+
 	/*
 	 * Once logging is disabled, GuC won't generate logs & send an
 	 * interrupt. But there could be some data in the log buffer
@@ -728,11 +741,6 @@ void intel_guc_log_unregister(struct intel_guc *guc)
 	guc_flush_logs(guc);
 
 	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(guc);
 	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 1c1a00df010b..90d2f38e22c9 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -245,6 +245,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);
 
@@ -259,6 +261,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;
 }
 
@@ -397,12 +401,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",
@@ -417,8 +418,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:
@@ -448,9 +447,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 v2 05/15] 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)

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>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 113 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
 2 files changed, 36 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 89cb3939467f..4eb3ebd8d6c3 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -155,10 +155,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 	struct dentry *log_dir;
 	int ret;
 
-	if (!i915_modparams.guc_log_level)
-		return 0;
-
-	mutex_lock(&guc->log.runtime.relay_lock);
+	lockdep_assert_held(&guc->log.runtime.lock);
 
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
@@ -177,28 +174,16 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 	 */
 	if (!log_dir) {
 		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
-		ret = -ENODEV;
-		goto out_unlock;
+		return -ENODEV;
 	}
 
 	ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
 	if (ret < 0 && ret != -EEXIST) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
-		goto out_unlock;
+		return ret;
 	}
 
-	ret = 0;
-
-out_unlock:
-	mutex_unlock(&guc->log.runtime.relay_lock);
-	return ret;
-}
-
-static bool guc_log_has_relay(struct intel_guc *guc)
-{
-	lockdep_assert_held(&guc->log.runtime.relay_lock);
-
-	return guc->log.runtime.relay_chan != NULL;
+	return 0;
 }
 
 static void guc_move_to_next_buf(struct intel_guc *guc)
@@ -209,9 +194,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 	 */
 	smp_wmb();
 
-	if (!guc_log_has_relay(guc))
-		return;
-
 	/* All data has been written, so now move the offset of sub buffer. */
 	relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
 
@@ -221,9 +203,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
 {
-	if (!guc_log_has_relay(guc))
-		return NULL;
-
 	/*
 	 * Just get the base address of a new sub buffer and copy data into it
 	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
@@ -284,13 +263,14 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	void *src_data, *dst_data;
 	bool new_overflow;
 
+	mutex_lock(&guc->log.runtime.lock);
+
 	if (WARN_ON(!guc->log.runtime.buf_addr))
-		return;
+		goto out_unlock;
 
 	/* Get the pointer to shared GuC log buffer */
 	log_buf_state = src_data = guc->log.runtime.buf_addr;
 
-	mutex_lock(&guc->log.runtime.relay_lock);
 
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -302,9 +282,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		 */
 		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
 		guc->log.capture_miss_count++;
-		mutex_unlock(&guc->log.runtime.relay_lock);
 
-		return;
+		goto out_unlock;
 	}
 
 	/* Actual logs are present from the 2nd page */
@@ -375,7 +354,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 
 	guc_move_to_next_buf(guc);
 
-	mutex_unlock(&guc->log.runtime.relay_lock);
+out_unlock:
+	mutex_unlock(&guc->log.runtime.lock);
 }
 
 static void capture_logs_work(struct work_struct *work)
@@ -391,20 +371,20 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
 	return guc->log.runtime.buf_addr != NULL;
 }
 
-static int guc_log_runtime_create(struct intel_guc *guc)
+static int guc_log_map(struct intel_guc *guc)
 {
 	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(&guc->log.runtime.lock);
 
 	if (!guc->log.vma)
 		return -ENODEV;
 
-	GEM_BUG_ON(guc_log_has_runtime(guc));
-
+	mutex_lock(&dev_priv->drm.struct_mutex);
 	ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
 	if (ret)
 		return ret;
 
@@ -424,14 +404,9 @@ static int guc_log_runtime_create(struct intel_guc *guc)
 	return 0;
 }
 
-static void guc_log_runtime_destroy(struct intel_guc *guc)
+static void guc_log_unmap(struct intel_guc *guc)
 {
-	/*
-	 * It's possible that the runtime stuff was never allocated because
-	 * GuC log was disabled at the boot time.
-	 */
-	if (!guc_log_has_runtime(guc))
-		return;
+	lockdep_assert_held(&guc->log.runtime.lock);
 
 	i915_gem_object_unpin_map(guc->log.vma->obj);
 	guc->log.runtime.buf_addr = NULL;
@@ -439,7 +414,7 @@ static void guc_log_runtime_destroy(struct intel_guc *guc)
 
 void intel_guc_log_init_early(struct intel_guc *guc)
 {
-	mutex_init(&guc->log.runtime.relay_lock);
+	mutex_init(&guc->log.runtime.lock);
 	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
 }
 
@@ -450,12 +425,7 @@ static int guc_log_relay_create(struct intel_guc *guc)
 	size_t n_subbufs, subbuf_size;
 	int ret;
 
-	if (!i915_modparams.guc_log_level)
-		return 0;
-
-	mutex_lock(&guc->log.runtime.relay_lock);
-
-	GEM_BUG_ON(guc_log_has_relay(guc));
+	lockdep_assert_held(&guc->log.runtime.lock);
 
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = GUC_LOG_SIZE;
@@ -485,12 +455,9 @@ static int guc_log_relay_create(struct intel_guc *guc)
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
 	guc->log.runtime.relay_chan = guc_log_relay_chan;
 
-	mutex_unlock(&guc->log.runtime.relay_lock);
-
 	return 0;
 
 err:
-	mutex_unlock(&guc->log.runtime.relay_lock);
 	/* logging will be off */
 	i915_modparams.guc_log_level = 0;
 	return ret;
@@ -498,20 +465,10 @@ static int guc_log_relay_create(struct intel_guc *guc)
 
 static void guc_log_relay_destroy(struct intel_guc *guc)
 {
-	mutex_lock(&guc->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(guc))
-		goto out_unlock;
+	lockdep_assert_held(&guc->log.runtime.lock);
 
 	relay_close(guc->log.runtime.relay_chan);
 	guc->log.runtime.relay_chan = NULL;
-
-out_unlock:
-	mutex_unlock(&guc->log.runtime.relay_lock);
 }
 
 static void guc_log_capture_logs(struct intel_guc *guc)
@@ -610,7 +567,6 @@ static void guc_log_flush_irq_disable(struct intel_guc *guc)
 
 void intel_guc_log_destroy(struct intel_guc *guc)
 {
-	guc_log_runtime_destroy(guc);
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
@@ -687,9 +643,10 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
 
 int intel_guc_log_register(struct intel_guc *guc)
 {
-	struct drm_i915_private *i915 = guc_to_i915(guc);
 	int ret;
 
+	mutex_lock(&guc->log.runtime.lock);
+
 	GEM_BUG_ON(guc_log_has_runtime(guc));
 
 	/*
@@ -701,35 +658,32 @@ int intel_guc_log_register(struct intel_guc *guc)
 	if (ret)
 		goto err;
 
-	mutex_lock(&i915->drm.struct_mutex);
-	ret = guc_log_runtime_create(guc);
-	mutex_unlock(&i915->drm.struct_mutex);
-
+	ret = guc_log_map(guc);
 	if (ret)
 		goto err_relay;
 
 	ret = guc_log_relay_file_create(guc);
 	if (ret)
-		goto err_runtime;
+		goto err_unmap;
 
 	guc_log_flush_irq_enable(guc);
 
+	mutex_unlock(&guc->log.runtime.lock);
+
 	return 0;
 
-err_runtime:
-	mutex_lock(&i915->drm.struct_mutex);
-	guc_log_runtime_destroy(guc);
-	mutex_unlock(&i915->drm.struct_mutex);
+err_unmap:
+	guc_log_unmap(guc);
 err_relay:
 	guc_log_relay_destroy(guc);
 err:
+	mutex_unlock(&guc->log.runtime.lock);
+
 	return ret;
 }
 
 void intel_guc_log_unregister(struct intel_guc *guc)
 {
-	struct drm_i915_private *i915 = guc_to_i915(guc);
-
 	guc_log_flush_irq_disable(guc);
 
 	/*
@@ -740,9 +694,12 @@ void intel_guc_log_unregister(struct intel_guc *guc)
 	 */
 	guc_flush_logs(guc);
 
-	mutex_lock(&i915->drm.struct_mutex);
-	guc_log_runtime_destroy(guc);
-	mutex_unlock(&i915->drm.struct_mutex);
+	mutex_lock(&guc->log.runtime.lock);
 
+	GEM_BUG_ON(!guc_log_has_runtime(guc));
+
+	guc_log_unmap(guc);
 	guc_log_relay_destroy(guc);
+
+	mutex_unlock(&guc->log.runtime.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 09dd2ef1933d..8c26cce77a98 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -48,8 +48,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 v2 06/15] 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)

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 | 64 +++---------------------------------
 1 file changed, 5 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 4eb3ebd8d6c3..ee0981f5a208 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -121,14 +121,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;
 }
@@ -149,43 +142,6 @@ static struct rchan_callbacks relay_callbacks = {
 	.remove_buf_file = remove_buf_file_callback,
 };
 
-static int guc_log_relay_file_create(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct dentry *log_dir;
-	int ret;
-
-	lockdep_assert_held(&guc->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(guc->log.runtime.relay_chan, "guc_log", log_dir);
-	if (ret < 0 && ret != -EEXIST) {
-		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static void guc_move_to_next_buf(struct intel_guc *guc)
 {
 	/*
@@ -271,7 +227,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	/* Get the pointer to shared GuC log buffer */
 	log_buf_state = src_data = guc->log.runtime.buf_addr;
 
-
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
 
@@ -443,8 +398,10 @@ static int guc_log_relay_create(struct intel_guc *guc)
 	 * 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");
 
@@ -649,11 +606,6 @@ int intel_guc_log_register(struct intel_guc *guc)
 
 	GEM_BUG_ON(guc_log_has_runtime(guc));
 
-	/*
-	 * If log was disabled at boot time, then setup needed to handle
-	 * log buffer flush interrupts would not have been done yet, so
-	 * do that now.
-	 */
 	ret = guc_log_relay_create(guc);
 	if (ret)
 		goto err;
@@ -662,18 +614,12 @@ int intel_guc_log_register(struct intel_guc *guc)
 	if (ret)
 		goto err_relay;
 
-	ret = guc_log_relay_file_create(guc);
-	if (ret)
-		goto err_unmap;
-
 	guc_log_flush_irq_enable(guc);
 
 	mutex_unlock(&guc->log.runtime.lock);
 
 	return 0;
 
-err_unmap:
-	guc_log_unmap(guc);
 err_relay:
 	guc_log_relay_destroy(guc);
 err:
-- 
2.14.3

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

[PATCH v2 07/15] 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>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index ee0981f5a208..5ff4b510569a 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -443,25 +443,6 @@ static void guc_log_capture_logs(struct intel_guc *guc)
 	intel_runtime_pm_put(dev_priv);
 }
 
-static void guc_flush_logs(struct intel_guc *guc)
-{
-	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(&guc->log.runtime.flush_work);
-
-	/* Ask GuC to update the log buffer state */
-	intel_runtime_pm_get(dev_priv);
-	guc_log_flush(guc);
-	intel_runtime_pm_put(dev_priv);
-
-	/* GuC would have updated log buffer by now, so capture it */
-	guc_log_capture_logs(guc);
-}
-
 int intel_guc_log_create(struct intel_guc *guc)
 {
 	struct i915_vma *vma;
@@ -630,15 +611,28 @@ int intel_guc_log_register(struct intel_guc *guc)
 
 void intel_guc_log_unregister(struct intel_guc *guc)
 {
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+
 	guc_log_flush_irq_disable(guc);
 
+	/*
+	 * Before initiating the forceful flush, wait for any pending/ongoing
+	 * flush to complete otherwise forceful flush may not actually happen.
+	 */
+	flush_work(&guc->log.runtime.flush_work);
+
 	/*
 	 * 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.
 	 */
-	guc_flush_logs(guc);
+	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(guc);
 
 	mutex_lock(&guc->log.runtime.lock);
 
-- 
2.14.3

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

[PATCH v2 08/15] 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

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 56 ++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.c      |  4 --
 drivers/gpu/drm/i915/intel_guc_log.c | 77 +++++++++++++++---------------------
 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, 85 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fa0755fe10d0..f99fe9910634 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2496,32 +2496,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);
+	*val = intel_guc_log_level_get(&dev_priv->guc);
 
 	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, val);
+	return intel_guc_log_level_set(&dev_priv->guc, 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;
+
+	return intel_guc_log_relay_open(&dev_priv->guc);
+}
+
+static ssize_t
+i915_guc_log_relay_write(struct file *filp,
+			 const char __user *ubuf,
+			 size_t cnt,
+			 loff_t *ppos)
+{
+	struct intel_guc *guc = filp->private_data;
+
+	intel_guc_log_relay_flush(guc);
+
+	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);
+
+	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[] = {
@@ -4776,7 +4817,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 987c6770d1a6..0a238e3ed5b6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1240,9 +1240,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
@@ -1300,7 +1297,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 5ff4b510569a..62bf8da14b13 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -406,18 +406,13 @@ static int guc_log_relay_create(struct intel_guc *guc)
 		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);
 	guc->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 *guc)
@@ -508,7 +503,7 @@ void intel_guc_log_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
-int intel_guc_log_control_get(struct intel_guc *guc)
+int intel_guc_log_level_get(struct intel_guc *guc)
 {
 	GEM_BUG_ON(!guc->log.vma);
 	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
@@ -523,10 +518,9 @@ int intel_guc_log_control_get(struct intel_guc *guc)
 	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 *guc, u64 val)
+int intel_guc_log_level_set(struct intel_guc *guc, u64 val)
 {
 	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);
@@ -549,7 +543,8 @@ int intel_guc_log_control_set(struct intel_guc *guc, 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);
@@ -558,88 +553,78 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
 
 	i915_modparams.guc_log_level = val;
 
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	if (enabled && !guc_log_has_runtime(guc)) {
-		ret = intel_guc_log_register(guc);
-		if (ret) {
-			/* logging will remain off */
-			i915_modparams.guc_log_level = 0;
-			goto out;
-		}
-	} else if (!enabled && guc_log_has_runtime(guc)) {
-		intel_guc_log_unregister(guc);
-	}
-
-	return 0;
-
 out_unlock:
 	mutex_unlock(&dev_priv->drm.struct_mutex);
-out:
+
 	return ret;
 }
 
-int intel_guc_log_register(struct intel_guc *guc)
+int intel_guc_log_relay_open(struct intel_guc *guc)
 {
 	int ret;
 
 	mutex_lock(&guc->log.runtime.lock);
 
-	GEM_BUG_ON(guc_log_has_runtime(guc));
+	if (guc_log_has_runtime(guc)) {
+		ret = -EEXIST;
+		goto out_unlock;
+	}
 
 	ret = guc_log_relay_create(guc);
 	if (ret)
-		goto err;
+		goto out_unlock;
 
 	ret = guc_log_map(guc);
 	if (ret)
-		goto err_relay;
+		goto out_relay;
+
+	mutex_unlock(&guc->log.runtime.lock);
 
 	guc_log_flush_irq_enable(guc);
 
-	mutex_unlock(&guc->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(guc->log.runtime.flush_wq, &guc->log.runtime.flush_work);
 
 	return 0;
 
-err_relay:
+out_relay:
 	guc_log_relay_destroy(guc);
-err:
+out_unlock:
 	mutex_unlock(&guc->log.runtime.lock);
 
 	return ret;
 }
 
-void intel_guc_log_unregister(struct intel_guc *guc)
+void intel_guc_log_relay_flush(struct intel_guc *guc)
 {
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 
-	guc_log_flush_irq_disable(guc);
-
 	/*
 	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
 	flush_work(&guc->log.runtime.flush_work);
 
-	/*
-	 * 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(guc);
+}
 
-	mutex_lock(&guc->log.runtime.lock);
+void intel_guc_log_relay_close(struct intel_guc *guc)
+{
+	guc_log_flush_irq_disable(guc);
+	flush_work(&guc->log.runtime.flush_work);
 
+	mutex_lock(&guc->log.runtime.lock);
 	GEM_BUG_ON(!guc_log_has_runtime(guc));
-
-	guc_log_unmap(guc);
+ 	guc_log_unmap(guc);
 	guc_log_relay_destroy(guc);
-
 	mutex_unlock(&guc->log.runtime.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 8c26cce77a98..df91f12a36ed 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -61,9 +61,10 @@ struct intel_guc_log {
 int intel_guc_log_create(struct intel_guc *guc);
 void intel_guc_log_destroy(struct intel_guc *guc);
 void intel_guc_log_init_early(struct intel_guc *guc);
-int intel_guc_log_control_get(struct intel_guc *guc);
-int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
-int intel_guc_log_register(struct intel_guc *guc);
-void intel_guc_log_unregister(struct intel_guc *guc);
+int intel_guc_log_level_get(struct intel_guc *guc);
+int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
+int intel_guc_log_relay_open(struct intel_guc *guc);
+void intel_guc_log_relay_close(struct intel_guc *guc);
+void intel_guc_log_relay_flush(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 90d2f38e22c9..abce0e38528a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -219,28 +219,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);
-
-	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);
-}
-
 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 d6af984cd789..f76d51d1ce70 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -31,8 +31,6 @@
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 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 09/15] 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 62bf8da14b13..92a7bf0fd729 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -447,16 +447,6 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	GEM_BUG_ON(guc->log.vma);
 
-	/*
-	 * We require SSE 4.1 for fast reads from the GuC log buffer and
-	 * 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);
@@ -570,6 +560,16 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
 		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(guc);
 	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 10/15] 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>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  4 +--
 drivers/gpu/drm/i915/intel_guc.c     | 12 +++----
 drivers/gpu/drm/i915/intel_guc_log.c | 66 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_guc_log.h |  7 ++--
 4 files changed, 44 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f99fe9910634..d7c0bf6facf6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2348,8 +2348,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 0d92caf6a83f..cab158e42577 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)
@@ -389,8 +389,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 92a7bf0fd729..7c4339dae534 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -151,10 +151,10 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 	smp_wmb();
 
 	/* All data has been written, so now move the offset of sub buffer. */
-	relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
+	relay_reserve(guc->log.relay.channel, guc->log.vma->obj->base.size);
 
 	/* Switch to the next sub buffer */
-	relay_flush(guc->log.runtime.relay_chan);
+	relay_flush(guc->log.relay.channel);
 }
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
@@ -168,7 +168,7 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
 	 * done without using relay_reserve() along with relay_write(). So its
 	 * better to use relay_reserve() alone.
 	 */
-	return relay_reserve(guc->log.runtime.relay_chan, 0);
+	return relay_reserve(guc->log.relay.channel, 0);
 }
 
 static bool guc_check_log_buf_overflow(struct intel_guc *guc,
@@ -219,13 +219,13 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	void *src_data, *dst_data;
 	bool new_overflow;
 
-	mutex_lock(&guc->log.runtime.lock);
+	mutex_lock(&guc->log.relay.lock);
 
-	if (WARN_ON(!guc->log.runtime.buf_addr))
+	if (WARN_ON(!guc->log.relay.buf_addr))
 		goto out_unlock;
 
 	/* Get the pointer to shared GuC log buffer */
-	log_buf_state = src_data = guc->log.runtime.buf_addr;
+	log_buf_state = src_data = guc->log.relay.buf_addr;
 
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -236,7 +236,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		 * getting consumed by User at a slow rate.
 		 */
 		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
-		guc->log.capture_miss_count++;
+		guc->log.relay.full_count++;
 
 		goto out_unlock;
 	}
@@ -310,20 +310,20 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	guc_move_to_next_buf(guc);
 
 out_unlock:
-	mutex_unlock(&guc->log.runtime.lock);
+	mutex_unlock(&guc->log.relay.lock);
 }
 
 static void capture_logs_work(struct work_struct *work)
 {
 	struct intel_guc *guc =
-		container_of(work, struct intel_guc, log.runtime.flush_work);
+		container_of(work, struct intel_guc, log.relay.flush_work);
 
 	guc_log_capture_logs(guc);
 }
 
-static bool guc_log_has_runtime(struct intel_guc *guc)
+static bool guc_log_relay_enabled(struct intel_guc *guc)
 {
-	return guc->log.runtime.buf_addr != NULL;
+	return guc->log.relay.buf_addr != NULL;
 }
 
 static int guc_log_map(struct intel_guc *guc)
@@ -332,7 +332,7 @@ static int guc_log_map(struct intel_guc *guc)
 	void *vaddr;
 	int ret;
 
-	lockdep_assert_held(&guc->log.runtime.lock);
+	lockdep_assert_held(&guc->log.relay.lock);
 
 	if (!guc->log.vma)
 		return -ENODEV;
@@ -354,23 +354,23 @@ static int guc_log_map(struct intel_guc *guc)
 		return PTR_ERR(vaddr);
 	}
 
-	guc->log.runtime.buf_addr = vaddr;
+	guc->log.relay.buf_addr = vaddr;
 
 	return 0;
 }
 
 static void guc_log_unmap(struct intel_guc *guc)
 {
-	lockdep_assert_held(&guc->log.runtime.lock);
+	lockdep_assert_held(&guc->log.relay.lock);
 
 	i915_gem_object_unpin_map(guc->log.vma->obj);
-	guc->log.runtime.buf_addr = NULL;
+	guc->log.relay.buf_addr = NULL;
 }
 
 void intel_guc_log_init_early(struct intel_guc *guc)
 {
-	mutex_init(&guc->log.runtime.lock);
-	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
+	mutex_init(&guc->log.relay.lock);
+	INIT_WORK(&guc->log.relay.flush_work, capture_logs_work);
 }
 
 static int guc_log_relay_create(struct intel_guc *guc)
@@ -380,7 +380,7 @@ static int guc_log_relay_create(struct intel_guc *guc)
 	size_t n_subbufs, subbuf_size;
 	int ret;
 
-	lockdep_assert_held(&guc->log.runtime.lock);
+	lockdep_assert_held(&guc->log.relay.lock);
 
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = GUC_LOG_SIZE;
@@ -410,17 +410,17 @@ static int guc_log_relay_create(struct intel_guc *guc)
 	}
 
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
-	guc->log.runtime.relay_chan = guc_log_relay_chan;
+	guc->log.relay.channel = guc_log_relay_chan;
 
 	return 0;
 }
 
 static void guc_log_relay_destroy(struct intel_guc *guc)
 {
-	lockdep_assert_held(&guc->log.runtime.lock);
+	lockdep_assert_held(&guc->log.relay.lock);
 
-	relay_close(guc->log.runtime.relay_chan);
-	guc->log.runtime.relay_chan = NULL;
+	relay_close(guc->log.relay.channel);
+	guc->log.relay.channel = NULL;
 }
 
 static void guc_log_capture_logs(struct intel_guc *guc)
@@ -553,9 +553,9 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
 {
 	int ret;
 
-	mutex_lock(&guc->log.runtime.lock);
+	mutex_lock(&guc->log.relay.lock);
 
-	if (guc_log_has_runtime(guc)) {
+	if (guc_log_relay_enabled(guc)) {
 		ret = -EEXIST;
 		goto out_unlock;
 	}
@@ -578,7 +578,7 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
 	if (ret)
 		goto out_relay;
 
-	mutex_unlock(&guc->log.runtime.lock);
+	mutex_unlock(&guc->log.relay.lock);
 
 	guc_log_flush_irq_enable(guc);
 
@@ -587,14 +587,14 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
 	 * the flush notification. This means that we need to unconditionally
 	 * flush on relay enabling, since GuC only notifies us once.
 	 */
-	queue_work(guc->log.runtime.flush_wq, &guc->log.runtime.flush_work);
+	queue_work(guc->log.relay.flush_wq, &guc->log.relay.flush_work);
 
 	return 0;
 
 out_relay:
 	guc_log_relay_destroy(guc);
 out_unlock:
-	mutex_unlock(&guc->log.runtime.lock);
+	mutex_unlock(&guc->log.relay.lock);
 
 	return ret;
 }
@@ -607,7 +607,7 @@ void intel_guc_log_relay_flush(struct intel_guc *guc)
 	 * Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
-	flush_work(&guc->log.runtime.flush_work);
+	flush_work(&guc->log.relay.flush_work);
 
 	intel_runtime_pm_get(i915);
 	guc_log_flush(guc);
@@ -620,11 +620,11 @@ void intel_guc_log_relay_flush(struct intel_guc *guc)
 void intel_guc_log_relay_close(struct intel_guc *guc)
 {
 	guc_log_flush_irq_disable(guc);
-	flush_work(&guc->log.runtime.flush_work);
+	flush_work(&guc->log.relay.flush_work);
 
-	mutex_lock(&guc->log.runtime.lock);
-	GEM_BUG_ON(!guc_log_has_runtime(guc));
- 	guc_log_unmap(guc);
+	mutex_lock(&guc->log.relay.lock);
+	GEM_BUG_ON(!guc_log_relay_enabled(guc));
+	guc_log_unmap(guc);
 	guc_log_relay_destroy(guc);
-	mutex_unlock(&guc->log.runtime.lock);
+	mutex_unlock(&guc->log.relay.lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index df91f12a36ed..9b1257ea2673 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -42,16 +42,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 11/15] 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 d7c0bf6facf6..d29bacb1f308 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2331,7 +2331,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],
@@ -2379,14 +2379,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);
@@ -2396,8 +2401,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 v2 12/15] 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)

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 45 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_guc.c     |  5 +---
 drivers/gpu/drm/i915/intel_guc_log.c | 22 +++++++++---------
 drivers/gpu/drm/i915/intel_guc_log.h | 10 ++++----
 4 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d29bacb1f308..94516ab4eaed 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2326,30 +2326,45 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static const char *
+stringify_guc_log_type(enum guc_log_buffer_type type)
+{
+	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);
+	}
+
+	return "";
+}
+
 static void i915_guc_log_info(struct seq_file *m,
 			      struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
+	enum guc_log_buffer_type type;
 
-	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],
-		   guc->log.total_overflow_count[GUC_ISR_LOG_BUFFER]);
-
-	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]);
-
-	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(guc)) {
+		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);
+
+	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),
+			   guc->log.stats[type].flush,
+			   guc->log.stats[type].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 cab158e42577..3e2f0f8503ed 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -388,12 +388,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 7c4339dae534..a72fe4a369f4 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -171,22 +171,22 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
 	return relay_reserve(guc->log.relay.channel, 0);
 }
 
-static bool guc_check_log_buf_overflow(struct intel_guc *guc,
+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 = guc->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;
 
-		guc->log.prev_overflow_count[type] = full_cnt;
-		guc->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 */
-			guc->log.total_overflow_count[type] += 16;
+			log->stats[type].sampled_overflow += 16;
 		}
 		DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
 	}
@@ -221,7 +221,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 
 	mutex_lock(&guc->log.relay.lock);
 
-	if (WARN_ON(!guc->log.relay.buf_addr))
+	if (WARN_ON(!intel_guc_log_relay_enabled(guc)))
 		goto out_unlock;
 
 	/* Get the pointer to shared GuC log buffer */
@@ -259,8 +259,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		full_cnt = log_buf_state_local.buffer_full_cnt;
 
 		/* Bookkeeping stuff */
-		guc->log.flush_count[type] += log_buf_state_local.flush_to_file;
-		new_overflow = guc_check_log_buf_overflow(guc, type, full_cnt);
+		guc->log.stats[type].flush += log_buf_state_local.flush_to_file;
+		new_overflow = guc_check_log_buf_overflow(&guc->log, type, full_cnt);
 
 		/* Update the state of shared log buffer */
 		log_buf_state->read_ptr = write_offset;
@@ -321,7 +321,7 @@ static void capture_logs_work(struct work_struct *work)
 	guc_log_capture_logs(guc);
 }
 
-static bool guc_log_relay_enabled(struct intel_guc *guc)
+bool intel_guc_log_relay_enabled(struct intel_guc *guc)
 {
 	return guc->log.relay.buf_addr != NULL;
 }
@@ -555,7 +555,7 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
 
 	mutex_lock(&guc->log.relay.lock);
 
-	if (guc_log_relay_enabled(guc)) {
+	if (intel_guc_log_relay_enabled(guc)) {
 		ret = -EEXIST;
 		goto out_unlock;
 	}
@@ -623,7 +623,7 @@ void intel_guc_log_relay_close(struct intel_guc *guc)
 	flush_work(&guc->log.relay.flush_work);
 
 	mutex_lock(&guc->log.relay.lock);
-	GEM_BUG_ON(!guc_log_relay_enabled(guc));
+	GEM_BUG_ON(!intel_guc_log_relay_enabled(guc));
 	guc_log_unmap(guc);
 	guc_log_relay_destroy(guc);
 	mutex_unlock(&guc->log.relay.lock);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 9b1257ea2673..8804c5c27409 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -51,10 +51,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];
 };
 
 int intel_guc_log_create(struct intel_guc *guc);
@@ -65,5 +66,6 @@ int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
 int intel_guc_log_relay_open(struct intel_guc *guc);
 void intel_guc_log_relay_close(struct intel_guc *guc);
 void intel_guc_log_relay_flush(struct intel_guc *guc);
+bool intel_guc_log_relay_enabled(struct intel_guc *guc);
 
 #endif
-- 
2.14.3

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

[PATCH v2 13/15] 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.

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.c      | 21 ++++++++++++---------
 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, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 3e2f0f8503ed..fc21135e2a54 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -221,17 +221,20 @@ 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 | GUC_LOG_DISABLED;
+	else if (GUC_LOG_LEVEL_TO_VERBOSE(level))
+		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 +269,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 a72fe4a369f4..decada3cf496 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -58,12 +58,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[] = {
@@ -501,13 +503,6 @@ int intel_guc_log_level_get(struct intel_guc *guc)
 	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 *guc, u64 val)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -522,7 +517,7 @@ int intel_guc_log_level_set(struct intel_guc *guc, 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);
@@ -533,8 +528,9 @@ int intel_guc_log_level_set(struct intel_guc *guc, 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 8804c5c27409..9e7be03acd18 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -39,6 +39,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_ENABLED(_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 abce0e38528a..3fb5f75aa7c9 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 @@ void intel_uc_sanitize_options(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 14/15] 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> (v1)
---
 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 3fb5f75aa7c9..2f579fff58cd 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 15/15] 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.BAT: success for series starting with [v2,01/15] drm/i915/guc: Tidy guc_log_control

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/15] drm/i915/guc: Tidy guc_log_control
URL   : https://patchwork.freedesktop.org/series/39614/
State : success

== Summary ==

Series 39614v1 series starting with [v2,01/15] drm/i915/guc: Tidy guc_log_control
https://patchwork.freedesktop.org/api/1.0/series/39614/revisions/1/mbox/

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
Test kms_frontbuffer_tracking:
        Subgroup basic:
                fail       -> PASS       (fi-cnl-y3) fdo#103167

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:424s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:506s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:488s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:494s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:480s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:469s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:405s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:581s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:585s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:413s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:291s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:516s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:401s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:471s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:417s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:466s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:590s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:427s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:518s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:535s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:499s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:485s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:424s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:522s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:396s
fi-cfl-u failed to collect. IGT log at Patchwork_8275/fi-cfl-u/run0.log

da319f89899feb1b6206d7092b10189a926a893c drm-tip: 2018y-03m-08d-12h-49m-27s UTC integration manifest
379600aecf1b HAX enable guc for CI
561313bdbb69 drm/i915/guc: Default to non-verbose GuC logging
9919f3c4ef62 drm/i915/guc: Allow user to control default GuC logging
7688a3d6c706 drm/i915/guc: Don't print out relay statistics when relay is disabled
4df6319cba38 drm/i915/guc: Always print log stats in i915_guc_info when using GuC
d8d5b88cd0af drm/i915/guc: Get rid of GuC log runtime
c6076d262079 drm/i915/guc: Move check for fast memcpy_wc to relay creation
b39b496589fa drm/i915/guc: Split relay control and GuC log level
b75cfe289828 drm/i915/guc: Flush directly in log unregister
538047832dbe drm/i915/guc: Merge log relay file and channel creation
1756525f1bea drm/i915/guc: Log runtime should consist of both mapping and relay
db07f9ade606 drm/i915/guc: Keep GuC interrupts enabled when using GuC
b1de17c10c7f drm/i915/guc: Move GuC notification handling to separate function
aa0d71c08bab drm/i915/guc: Create common entry points for log register/unregister
8f3d72331b9e drm/i915/guc: Tidy guc_log_control

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8275/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 [v2,01/15] drm/i915/guc: Tidy guc_log_control

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/15] drm/i915/guc: Tidy guc_log_control
URL   : https://patchwork.freedesktop.org/series/39614/
State : failure

== Summary ==

---- Possible new issues:

Test drv_missed_irq:
                pass       -> SKIP       (shard-apl)
Test drv_selftest:
        Subgroup live_guc:
                pass       -> DMESG-WARN (shard-apl)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-draw-render:
                fail       -> PASS       (shard-apl)
Test perf:
        Subgroup gen8-unprivileged-single-ctx-counters:
                pass       -> FAIL       (shard-apl)

---- Known issues:

Test gem_eio:
        Subgroup in-flight:
                pass       -> INCOMPLETE (shard-apl) fdo#105341
Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-bottom-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185 +3
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check:
                fail       -> PASS       (shard-hsw) fdo#100368
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-pri-indfb-multidraw:
                pass       -> FAIL       (shard-apl) fdo#103167
Test pm_lpsp:
        Subgroup screens-disabled:
                fail       -> PASS       (shard-hsw) fdo#104941

fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#104941 https://bugs.freedesktop.org/show_bug.cgi?id=104941

shard-apl        total:3394 pass:1777 dwarn:2   dfail:0   fail:15  skip:1598 time:12071s
shard-hsw        total:3467 pass:1773 dwarn:1   dfail:0   fail:1   skip:1691 time:11960s
shard-snb        total:3467 pass:1361 dwarn:3   dfail:0   fail:2   skip:2101 time:7012s
Blacklisted hosts:
shard-kbl        total:3397 pass:1875 dwarn:16  dfail:2   fail:16  skip:1486 time:9003s

== Logs ==

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

Re: [PATCH v2 01/15] drm/i915/guc: Tidy guc_log_control

[Sagar Arun Kamble]

On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> We plan to decouple log runtime (mapping + relay) from verbosity control.
> Let's tidy the code now to reduce the churn in the following patches.
>
> v2: Tidy macros, keep debug messages, use helper var for enable,
>      correct typo (Michał)
>      Fix incorrect input validaction (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: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c  | 11 ++---
>   drivers/gpu/drm/i915/intel_guc_log.c | 80 +++++++++++++++++++++---------------
>   drivers/gpu/drm/i915/intel_guc_log.h |  3 +-
>   3 files changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 89f7ff2c652e..fa0755fe10d0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2500,13 +2500,10 @@ static int i915_guc_log_control_get(void *data, u64 *val)
>   {
>   	struct drm_i915_private *dev_priv = data;
>   
> -	if (!HAS_GUC(dev_priv))
> +	if (!USES_GUC(dev_priv))
>   		return -ENODEV;
>   
> -	if (!dev_priv->guc.log.vma)
> -		return -EINVAL;
> -
> -	*val = i915_modparams.guc_log_level;
> +	*val = intel_guc_log_control_get(&dev_priv->guc);
>   
>   	return 0;
>   }
> @@ -2515,10 +2512,10 @@ static int i915_guc_log_control_set(void *data, u64 val)
>   {
>   	struct drm_i915_private *dev_priv = data;
>   
> -	if (!HAS_GUC(dev_priv))
> +	if (!USES_GUC(dev_priv))
>   		return -ENODEV;
>   
> -	return intel_guc_log_control(&dev_priv->guc, val);
> +	return intel_guc_log_control_set(&dev_priv->guc, val);
>   }
>   
>   DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index c0c2e7d1c7d7..7e59fb07b06b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -659,51 +659,63 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>   	i915_vma_unpin_and_release(&guc->log.vma);
>   }
>   
> -int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> +int intel_guc_log_control_get(struct intel_guc *guc)
> +{
> +	GEM_BUG_ON(!guc->log.vma);
> +	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> +
> +	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_control_set(struct intel_guc *guc, u64 val)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	bool enable_logging = control_val > 0;
> -	u32 verbosity;
> +	bool enabled = LOG_LEVEL_TO_ENABLED(val);
>   	int ret;
>   
> -	if (!guc->log.vma)
> -		return -ENODEV;
> +	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
> +	GEM_BUG_ON(!guc->log.vma);
> +	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
>   
> -	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
> -	if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
> +	/*
> +	 * GuC is recognizing log levels starting from 0 to max, we're using 0
> +	 * as indication that logging should be disabled.
> +	 */
> +	if (val < GUC_LOG_LEVEL_DISABLED ||
> +	    val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
>   		return -EINVAL;
>   
> -	/* This combination doesn't make sense & won't have any effect */
> -	if (!enable_logging && !i915_modparams.guc_log_level)
> -		return 0;
> +	mutex_lock(&dev_priv->drm.struct_mutex);
>   
> -	verbosity = enable_logging ? control_val - 1 : 0;
> +	if (i915_modparams.guc_log_level == val) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
>   
> -	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> -	if (ret)
> -		return ret;
>   	intel_runtime_pm_get(dev_priv);
> -	ret = guc_log_control(guc, enable_logging, verbosity);
> +	ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
>   	intel_runtime_pm_put(dev_priv);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> -
> -	if (ret < 0) {
> -		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
> -		return ret;
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
> +		goto out_unlock;
>   	}
>   
> -	if (enable_logging) {
> -		i915_modparams.guc_log_level = 1 + verbosity;
> +	i915_modparams.guc_log_level = val;
>   
> -		/*
> -		 * If log was disabled at boot time, then the relay channel file
> -		 * wouldn't have been created by now and interrupts also would
> -		 * not have been enabled. Try again now, just in case.
> -		 */
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +	if (enabled && !guc_log_has_runtime(guc)) {
>   		ret = guc_log_late_setup(guc);
> -		if (ret < 0) {
> +		if (ret) {
>   			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> -			return ret;
> +			goto out;
>   		}
>   
>   		/* GuC logging is currently the only user of Guc2Host interrupts */
> @@ -712,7 +724,7 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
>   		gen9_enable_guc_interrupts(dev_priv);
>   		intel_runtime_pm_put(dev_priv);
>   		mutex_unlock(&dev_priv->drm.struct_mutex);
> -	} else {
> +	} else if (!enabled && guc_log_has_runtime(guc)) {
>   		/*
>   		 * Once logging is disabled, GuC won't generate logs & send an
>   		 * interrupt. But there could be some data in the log buffer
> @@ -720,11 +732,13 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
>   		 * buffer state and then collect the left over logs.
>   		 */
>   		guc_flush_logs(guc);
> -
> -		/* As logging is disabled, update log level to reflect that */
> -		i915_modparams.guc_log_level = 0;
>   	}
>   
> +	return 0;
> +
> +out_unlock:
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +out:
>   	return ret;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index dab0e949567a..141ce9ca22ce 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -64,7 +64,8 @@ void intel_guc_log_destroy(struct intel_guc *guc);
>   void intel_guc_log_init_early(struct intel_guc *guc);
>   int intel_guc_log_relay_create(struct intel_guc *guc);
>   void intel_guc_log_relay_destroy(struct intel_guc *guc);
> -int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
> +int intel_guc_log_control_get(struct intel_guc *guc);
> +int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
>   void i915_guc_log_register(struct drm_i915_private *dev_priv);
>   void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
>   

-- 
Thanks,
Sagar

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

Re: [PATCH v2 02/15] drm/i915/guc: Create common entry points for log register/unregister

[Sagar Arun Kamble]

On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> We have many functions responsible for allocating different parts of
> GuC log runtime called from multiple places. Let's stick with keeping
> everything in guc_log_register instead.
>
> v2: Use more generic intel_uc_register name, keep using "misc" suffix (Michał)
>      s/dev_priv/i915 (Sagar)
>      Make guc_log_relay_* static (sparse)
>
> 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: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c      |   6 +-
>   drivers/gpu/drm/i915/intel_guc_log.c | 156 ++++++++++++++---------------------
>   drivers/gpu/drm/i915/intel_guc_log.h |   6 +-
>   drivers/gpu/drm/i915/intel_uc.c      |  41 +++++----
>   drivers/gpu/drm/i915/intel_uc.h      |   2 +
>   5 files changed, 95 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d7c4de45644d..987c6770d1a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1238,9 +1238,11 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   	/* Reveal our presence to userspace */
>   	if (drm_dev_register(dev, 0) == 0) {
>   		i915_debugfs_register(dev_priv);
> -		i915_guc_log_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
> @@ -1298,7 +1300,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>   	i915_pmu_unregister(dev_priv);
>   
>   	i915_teardown_sysfs(dev_priv);
> -	i915_guc_log_unregister(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 7e59fb07b06b..90b395f34808 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -443,7 +443,7 @@ void intel_guc_log_init_early(struct intel_guc *guc)
>   	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
>   }
>   
> -int intel_guc_log_relay_create(struct intel_guc *guc)
> +static int guc_log_relay_create(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct rchan *guc_log_relay_chan;
> @@ -496,7 +496,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc)
>   	return ret;
>   }
>   
> -void intel_guc_log_relay_destroy(struct intel_guc *guc)
> +static void guc_log_relay_destroy(struct intel_guc *guc)
>   {
>   	mutex_lock(&guc->log.runtime.relay_lock);
>   
> @@ -514,49 +514,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc)
>   	mutex_unlock(&guc->log.runtime.relay_lock);
>   }
>   
> -static int guc_log_late_setup(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	int ret;
> -
> -	if (!guc_log_has_runtime(guc)) {
> -		/*
> -		 * If log was disabled at boot time, then setup needed to handle
> -		 * log buffer flush interrupts would not have been done yet, so
> -		 * do that now.
> -		 */
> -		ret = intel_guc_log_relay_create(guc);
> -		if (ret)
> -			goto err;
> -
> -		mutex_lock(&dev_priv->drm.struct_mutex);
> -		intel_runtime_pm_get(dev_priv);
> -		ret = guc_log_runtime_create(guc);
> -		intel_runtime_pm_put(dev_priv);
> -		mutex_unlock(&dev_priv->drm.struct_mutex);
> -
> -		if (ret)
> -			goto err_relay;
> -	}
> -
> -	ret = guc_log_relay_file_create(guc);
> -	if (ret)
> -		goto err_runtime;
> -
> -	return 0;
> -
> -err_runtime:
> -	mutex_lock(&dev_priv->drm.struct_mutex);
> -	guc_log_runtime_destroy(guc);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> -err_relay:
> -	intel_guc_log_relay_destroy(guc);
> -err:
> -	/* logging will remain off */
> -	i915_modparams.guc_log_level = 0;
> -	return ret;
> -}
> -
>   static void guc_log_capture_logs(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -576,16 +533,6 @@ static void guc_flush_logs(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
> -	if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
> -		return;
> -
> -	/* First disable the interrupts, will be renabled afterwards */
> -	mutex_lock(&dev_priv->drm.struct_mutex);
> -	intel_runtime_pm_get(dev_priv);
> -	gen9_disable_guc_interrupts(dev_priv);
> -	intel_runtime_pm_put(dev_priv);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> -
>   	/*
>   	 * Before initiating the forceful flush, wait for any pending/ongoing
>   	 * flush to complete otherwise forceful flush may not actually happen.
> @@ -628,12 +575,6 @@ int intel_guc_log_create(struct intel_guc *guc)
>   
>   	guc->log.vma = vma;
>   
> -	if (i915_modparams.guc_log_level) {
> -		ret = guc_log_runtime_create(guc);
> -		if (ret < 0)
> -			goto err_vma;
> -	}
> -
>   	/* each allocated unit is a page */
>   	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
>   		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> @@ -645,8 +586,6 @@ int intel_guc_log_create(struct intel_guc *guc)
>   
>   	return 0;
>   
> -err_vma:
> -	i915_vma_unpin_and_release(&guc->log.vma);
>   err:
>   	/* logging will be off */
>   	i915_modparams.guc_log_level = 0;
> @@ -712,26 +651,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
>   	mutex_unlock(&dev_priv->drm.struct_mutex);
>   
>   	if (enabled && !guc_log_has_runtime(guc)) {
> -		ret = guc_log_late_setup(guc);
> +		ret = intel_guc_log_register(guc);
>   		if (ret) {
> -			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> +			/* logging will remain off */
> +			i915_modparams.guc_log_level = 0;
>   			goto out;
>   		}
> -
> -		/* GuC logging is currently the only user of Guc2Host interrupts */
> -		mutex_lock(&dev_priv->drm.struct_mutex);
> -		intel_runtime_pm_get(dev_priv);
> -		gen9_enable_guc_interrupts(dev_priv);
> -		intel_runtime_pm_put(dev_priv);
> -		mutex_unlock(&dev_priv->drm.struct_mutex);
>   	} else if (!enabled && guc_log_has_runtime(guc)) {
> -		/*
> -		 * 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.
> -		 */
> -		guc_flush_logs(guc);
> +		intel_guc_log_unregister(guc);
>   	}
>   
>   	return 0;
> @@ -742,29 +669,72 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
>   	return ret;
>   }
>   
> -void i915_guc_log_register(struct drm_i915_private *dev_priv)
> +int intel_guc_log_register(struct intel_guc *guc)
>   {
> -	if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
> -		return;
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
> +	int ret;
> +
> +	GEM_BUG_ON(guc_log_has_runtime(guc));
> +
> +	/*
> +	 * If log was disabled at boot time, then setup needed to handle
> +	 * log buffer flush interrupts would not have been done yet, so
> +	 * do that now.
> +	 */
> +	ret = guc_log_relay_create(guc);
> +	if (ret)
> +		goto err;
> +
> +	mutex_lock(&i915->drm.struct_mutex);
> +	ret = guc_log_runtime_create(guc);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +
> +	if (ret)
> +		goto err_relay;
> +
> +	ret = guc_log_relay_file_create(guc);
> +	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);
> +
> +	return 0;
>   
> -	guc_log_late_setup(&dev_priv->guc);
> +err_runtime:
> +	mutex_lock(&i915->drm.struct_mutex);
> +	guc_log_runtime_destroy(guc);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +err_relay:
> +	guc_log_relay_destroy(guc);
> +err:
> +	return ret;
>   }
>   
> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> +void intel_guc_log_unregister(struct intel_guc *guc)
>   {
> -	struct intel_guc *guc = &dev_priv->guc;
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
>   
> -	if (!USES_GUC_SUBMISSION(dev_priv))
> -		return;
> +	/*
> +	 * 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.
> +	 */
> +	guc_flush_logs(guc);
>   
> -	mutex_lock(&dev_priv->drm.struct_mutex);
> +	mutex_lock(&i915->drm.struct_mutex);
>   	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	intel_runtime_pm_get(dev_priv);
> -	gen9_disable_guc_interrupts(dev_priv);
> -	intel_runtime_pm_put(dev_priv);
> +	intel_runtime_pm_get(i915);
> +	gen9_disable_guc_interrupts(i915);
> +	intel_runtime_pm_put(i915);
>   
>   	guc_log_runtime_destroy(guc);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> +	mutex_unlock(&i915->drm.struct_mutex);
>   
> -	intel_guc_log_relay_destroy(guc);
> +	guc_log_relay_destroy(guc);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index 141ce9ca22ce..09dd2ef1933d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -62,11 +62,9 @@ struct intel_guc_log {
>   int intel_guc_log_create(struct intel_guc *guc);
>   void intel_guc_log_destroy(struct intel_guc *guc);
>   void intel_guc_log_init_early(struct intel_guc *guc);
> -int intel_guc_log_relay_create(struct intel_guc *guc);
> -void intel_guc_log_relay_destroy(struct intel_guc *guc);
>   int intel_guc_log_control_get(struct intel_guc *guc);
>   int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
> -void i915_guc_log_register(struct drm_i915_private *dev_priv);
> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> +int intel_guc_log_register(struct intel_guc *guc);
> +void intel_guc_log_unregister(struct intel_guc *guc);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index e5bf0d37bf43..1c1a00df010b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -219,6 +219,28 @@ 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);
> +
> +	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);
> +}
> +
>   static int guc_enable_communication(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -249,23 +271,10 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv)
>   		return 0;
>   
>   	ret = intel_guc_init_wq(guc);
> -	if (ret) {
> -		DRM_ERROR("Couldn't allocate workqueues for GuC\n");
> -		goto err;
> -	}
> -
> -	ret = intel_guc_log_relay_create(guc);
> -	if (ret) {
> -		DRM_ERROR("Couldn't allocate relay for GuC log\n");
> -		goto err_relay;
> -	}
> +	if (ret)
> +		return ret;
>   
>   	return 0;
> -
> -err_relay:
> -	intel_guc_fini_wq(guc);
> -err:
> -	return ret;
>   }
>   
>   void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
> @@ -276,8 +285,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
>   		return;
>   
>   	intel_guc_fini_wq(guc);
> -
> -	intel_guc_log_relay_destroy(guc);
>   }
>   
>   int intel_uc_init(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index f76d51d1ce70..d6af984cd789 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -31,6 +31,8 @@
>   void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>   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);

-- 
Thanks,
Sagar

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

Re: [PATCH v2 03/15] drm/i915/guc: Move GuC notification handling to separate function

[Sagar Arun Kamble]

On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> To allow future code reuse. While here, fix comment style.
>
> v2: Notifications are a separate thing - rename the handler (Sagar)
>
> Suggested-by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 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: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
I am suggesting update to the comments below since it is not clear the 
reason for clearing. Please check and incorporate in this patch
itself if you feel it is right. Otherwise change looks good to me.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c  | 33 ++-------------------------------
>   drivers/gpu/drm/i915/intel_guc.c | 37 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_guc.h |  1 +
>   3 files changed, 40 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 633c18785c1e..6b0cd6bc83f8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1766,37 +1766,8 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   
>   static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
>   {
> -	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
> -		/* Sample the log buffer flush related bits & clear them out now
> -		 * itself from the message identity register to minimize the
> -		 * probability of losing a flush interrupt, when there are back
> -		 * to back flush interrupts.
> -		 * There can be a new flush interrupt, for different log buffer
> -		 * type (like for ISR), whilst Host is handling one (for DPC).
> -		 * Since same bit is used in message register for ISR & DPC, it
> -		 * could happen that GuC sets the bit for 2nd interrupt but Host
> -		 * clears out the bit on handling the 1st interrupt.
> -		 */
> -		u32 msg, flush;
> -
> -		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 */
> -			queue_work(dev_priv->guc.log.runtime.flush_wq,
> -				   &dev_priv->guc.log.runtime.flush_work);
> -
> -			dev_priv->guc.log.flush_interrupt_count++;
> -		} else {
> -			/* Not clearing of unhandled event bits won't result in
> -			 * re-triggering of the interrupt.
> -			 */
> -		}
> -	}
> +	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT)
> +		intel_guc_to_host_event_handler(&dev_priv->guc);
>   }
>   
>   static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index ff08ea0ebf49..25f92291fd40 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -364,6 +364,43 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>   	return ret;
>   }
>   
> +void intel_guc_to_host_event_handler(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 msg, flush;
> +
> +	/*
> +	 * Sample the log buffer flush related bits & clear them out now
> +	 * itself from the message identity register to minimize the
> +	 * probability of losing a flush interrupt, when there are back
> +	 * to back flush interrupts.
This paragraph is updated as:

                     /*
                      * Note: Logging protocol is, GuC will set the bit 
in message identity register and raise the GuC to Host
                      * interrupt and then Host will read the bit, clear 
it, copy the logs and send ack.
                      * If bits are cleared after sending ack, i915 
might lose next log flush interrupt if Host clear operation
                      * happens post GuC's update to message identity 
register corresponding to next interrupt.
                      * Hence, sample the log buffer flush related bits 
and clear them out now
                      * itself from the message identity register before 
sending ack to GuC about
                      * flush completion through guc_log_flush_complete.
                      */

and may be we can additionally include below paragraph to explain the 
scenario.
> +	 * There can be a new flush interrupt, for different log buffer
> +	 * type (like for ISR), whilst Host is handling one (for DPC).
> +	 * Since same bit is used in message register for ISR & DPC, it
> +	 * 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 */
> +		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.
> +		 */
> +	}
> +}
> +
>   int intel_guc_sample_forcewake(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index b9424ac644ac..6d5aebe55039 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -125,6 +125,7 @@ int intel_guc_init(struct intel_guc *guc);
>   void intel_guc_fini(struct intel_guc *guc);
>   int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
>   int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
> +void intel_guc_to_host_event_handler(struct intel_guc *guc);
>   int intel_guc_sample_forcewake(struct intel_guc *guc);
>   int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
>   int intel_guc_suspend(struct intel_guc *guc);

-- 
Thanks,
Sagar

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

Re: [PATCH v2 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

[Sagar Arun Kamble]

On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> 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)
>
> 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 | 113 +++++++++++------------------------
>   drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
>   2 files changed, 36 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 89cb3939467f..4eb3ebd8d6c3 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -155,10 +155,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>   	struct dentry *log_dir;
>   	int ret;
>   
> -	if (!i915_modparams.guc_log_level)
> -		return 0;
> -
> -	mutex_lock(&guc->log.runtime.relay_lock);
> +	lockdep_assert_held(&guc->log.runtime.lock);
>   
>   	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
>   	log_dir = dev_priv->drm.primary->debugfs_root;
> @@ -177,28 +174,16 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>   	 */
>   	if (!log_dir) {
>   		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
> -		ret = -ENODEV;
> -		goto out_unlock;
> +		return -ENODEV;
>   	}
>   
>   	ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
>   	if (ret < 0 && ret != -EEXIST) {
>   		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> -		goto out_unlock;
> +		return ret;
>   	}
>   
> -	ret = 0;
> -
> -out_unlock:
> -	mutex_unlock(&guc->log.runtime.relay_lock);
> -	return ret;
> -}
> -
> -static bool guc_log_has_relay(struct intel_guc *guc)
> -{
> -	lockdep_assert_held(&guc->log.runtime.relay_lock);
> -
> -	return guc->log.runtime.relay_chan != NULL;
> +	return 0;
>   }
>   
>   static void guc_move_to_next_buf(struct intel_guc *guc)
> @@ -209,9 +194,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
>   	 */
>   	smp_wmb();
>   
> -	if (!guc_log_has_relay(guc))
> -		return;
> -
>   	/* All data has been written, so now move the offset of sub buffer. */
>   	relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
>   
> @@ -221,9 +203,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
>   
>   static void *guc_get_write_buffer(struct intel_guc *guc)
>   {
> -	if (!guc_log_has_relay(guc))
> -		return NULL;
> -
>   	/*
>   	 * Just get the base address of a new sub buffer and copy data into it
>   	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
> @@ -284,13 +263,14 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	void *src_data, *dst_data;
>   	bool new_overflow;
>   
> +	mutex_lock(&guc->log.runtime.lock);
> +
>   	if (WARN_ON(!guc->log.runtime.buf_addr))
> -		return;
> +		goto out_unlock;
>   
>   	/* Get the pointer to shared GuC log buffer */
>   	log_buf_state = src_data = guc->log.runtime.buf_addr;
>   
> -	mutex_lock(&guc->log.runtime.relay_lock);
>   
>   	/* Get the pointer to local buffer to store the logs */
>   	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> @@ -302,9 +282,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   		 */
>   		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
>   		guc->log.capture_miss_count++;
> -		mutex_unlock(&guc->log.runtime.relay_lock);
>   
> -		return;
> +		goto out_unlock;
>   	}
>   
>   	/* Actual logs are present from the 2nd page */
> @@ -375,7 +354,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   
>   	guc_move_to_next_buf(guc);
>   
> -	mutex_unlock(&guc->log.runtime.relay_lock);
> +out_unlock:
> +	mutex_unlock(&guc->log.runtime.lock);
>   }
>   
>   static void capture_logs_work(struct work_struct *work)
> @@ -391,20 +371,20 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
>   	return guc->log.runtime.buf_addr != NULL;
>   }
>   
> -static int guc_log_runtime_create(struct intel_guc *guc)
> +static int guc_log_map(struct intel_guc *guc)
>   {
>   	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(&guc->log.runtime.lock);
>   
>   	if (!guc->log.vma)
>   		return -ENODEV;
>   
> -	GEM_BUG_ON(guc_log_has_runtime(guc));
> -
> +	mutex_lock(&dev_priv->drm.struct_mutex);
>   	ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
>   	if (ret)
>   		return ret;
>   
> @@ -424,14 +404,9 @@ static int guc_log_runtime_create(struct intel_guc *guc)
>   	return 0;
>   }
>   
> -static void guc_log_runtime_destroy(struct intel_guc *guc)
> +static void guc_log_unmap(struct intel_guc *guc)
>   {
> -	/*
> -	 * It's possible that the runtime stuff was never allocated because
> -	 * GuC log was disabled at the boot time.
> -	 */
> -	if (!guc_log_has_runtime(guc))
> -		return;
> +	lockdep_assert_held(&guc->log.runtime.lock);
>   
>   	i915_gem_object_unpin_map(guc->log.vma->obj);
>   	guc->log.runtime.buf_addr = NULL;
> @@ -439,7 +414,7 @@ static void guc_log_runtime_destroy(struct intel_guc *guc)
>   
>   void intel_guc_log_init_early(struct intel_guc *guc)
>   {
> -	mutex_init(&guc->log.runtime.relay_lock);
> +	mutex_init(&guc->log.runtime.lock);
>   	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
>   }
>   
> @@ -450,12 +425,7 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   	size_t n_subbufs, subbuf_size;
>   	int ret;
>   
> -	if (!i915_modparams.guc_log_level)
> -		return 0;
> -
> -	mutex_lock(&guc->log.runtime.relay_lock);
> -
> -	GEM_BUG_ON(guc_log_has_relay(guc));
> +	lockdep_assert_held(&guc->log.runtime.lock);
>   
>   	 /* Keep the size of sub buffers same as shared log buffer */
>   	subbuf_size = GUC_LOG_SIZE;
> @@ -485,12 +455,9 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
>   	guc->log.runtime.relay_chan = guc_log_relay_chan;
>   
> -	mutex_unlock(&guc->log.runtime.relay_lock);
> -
>   	return 0;
>   
>   err:
> -	mutex_unlock(&guc->log.runtime.relay_lock);
>   	/* logging will be off */
>   	i915_modparams.guc_log_level = 0;
>   	return ret;
> @@ -498,20 +465,10 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   
>   static void guc_log_relay_destroy(struct intel_guc *guc)
>   {
> -	mutex_lock(&guc->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(guc))
> -		goto out_unlock;
> +	lockdep_assert_held(&guc->log.runtime.lock);
>   
>   	relay_close(guc->log.runtime.relay_chan);
>   	guc->log.runtime.relay_chan = NULL;
> -
> -out_unlock:
> -	mutex_unlock(&guc->log.runtime.relay_lock);
>   }
>   
>   static void guc_log_capture_logs(struct intel_guc *guc)
> @@ -610,7 +567,6 @@ static void guc_log_flush_irq_disable(struct intel_guc *guc)
>   
>   void intel_guc_log_destroy(struct intel_guc *guc)
>   {
> -	guc_log_runtime_destroy(guc);
>   	i915_vma_unpin_and_release(&guc->log.vma);
>   }
>   
> @@ -687,9 +643,10 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
>   
>   int intel_guc_log_register(struct intel_guc *guc)
>   {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
>   	int ret;
>   
> +	mutex_lock(&guc->log.runtime.lock);
> +
>   	GEM_BUG_ON(guc_log_has_runtime(guc));
>   
>   	/*
> @@ -701,35 +658,32 @@ int intel_guc_log_register(struct intel_guc *guc)
>   	if (ret)
>   		goto err;
>   
> -	mutex_lock(&i915->drm.struct_mutex);
> -	ret = guc_log_runtime_create(guc);
> -	mutex_unlock(&i915->drm.struct_mutex);
> -
> +	ret = guc_log_map(guc);
>   	if (ret)
>   		goto err_relay;
>   
>   	ret = guc_log_relay_file_create(guc);
>   	if (ret)
> -		goto err_runtime;
> +		goto err_unmap;
>   
>   	guc_log_flush_irq_enable(guc);
>   
> +	mutex_unlock(&guc->log.runtime.lock);
> +
>   	return 0;
>   
> -err_runtime:
> -	mutex_lock(&i915->drm.struct_mutex);
> -	guc_log_runtime_destroy(guc);
> -	mutex_unlock(&i915->drm.struct_mutex);
> +err_unmap:
> +	guc_log_unmap(guc);
>   err_relay:
>   	guc_log_relay_destroy(guc);
>   err:
> +	mutex_unlock(&guc->log.runtime.lock);
> +
>   	return ret;
>   }
>   
>   void intel_guc_log_unregister(struct intel_guc *guc)
>   {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -
>   	guc_log_flush_irq_disable(guc);
>   
>   	/*
> @@ -740,9 +694,12 @@ void intel_guc_log_unregister(struct intel_guc *guc)
>   	 */
>   	guc_flush_logs(guc);
>   
> -	mutex_lock(&i915->drm.struct_mutex);
> -	guc_log_runtime_destroy(guc);
> -	mutex_unlock(&i915->drm.struct_mutex);
> +	mutex_lock(&guc->log.runtime.lock);
>   
> +	GEM_BUG_ON(!guc_log_has_runtime(guc));
> +
> +	guc_log_unmap(guc);
>   	guc_log_relay_destroy(guc);
> +
> +	mutex_unlock(&guc->log.runtime.lock);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index 09dd2ef1933d..8c26cce77a98 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -48,8 +48,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;

-- 
Thanks,
Sagar

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

Re: [PATCH v2 06/15] drm/i915/guc: Merge log relay file and channel creation

[Sagar Arun Kamble]

Hi Michal,

One comment was missed and another comment update suggested.

On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> 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)
>
> 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 | 64 +++---------------------------------
>   1 file changed, 5 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 4eb3ebd8d6c3..ee0981f5a208 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -121,14 +121,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;
>   }
> @@ -149,43 +142,6 @@ static struct rchan_callbacks relay_callbacks = {
>   	.remove_buf_file = remove_buf_file_callback,
>   };
>   
> -static int guc_log_relay_file_create(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct dentry *log_dir;
> -	int ret;
> -
> -	lockdep_assert_held(&guc->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(guc->log.runtime.relay_chan, "guc_log", log_dir);
> -	if (ret < 0 && ret != -EEXIST) {
> -		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>   static void guc_move_to_next_buf(struct intel_guc *guc)
>   {
>   	/*
> @@ -271,7 +227,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	/* Get the pointer to shared GuC log buffer */
>   	log_buf_state = src_data = guc->log.runtime.buf_addr;
>   
> -
>   	/* Get the pointer to local buffer to store the logs */
>   	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
>   
> @@ -443,8 +398,10 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   	 * the GuC firmware logs, the channel will be linked with a file
>   	 * later on when debugfs is registered.
>   	 */
Above comment needs to be removed/updated.
> -	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");
>   
> @@ -649,11 +606,6 @@ int intel_guc_log_register(struct intel_guc *guc)
>   
>   	GEM_BUG_ON(guc_log_has_runtime(guc));
>   
> -	/*
> -	 * If log was disabled at boot time, then setup needed to handle
> -	 * log buffer flush interrupts would not have been done yet, so
> -	 * do that now.
> -	 */
May be this comment should not have been added as part of patch 5/15
>   	ret = guc_log_relay_create(guc);
>   	if (ret)
>   		goto err;
> @@ -662,18 +614,12 @@ int intel_guc_log_register(struct intel_guc *guc)
>   	if (ret)
>   		goto err_relay;
>   
> -	ret = guc_log_relay_file_create(guc);
> -	if (ret)
> -		goto err_unmap;
> -
>   	guc_log_flush_irq_enable(guc);
>   
>   	mutex_unlock(&guc->log.runtime.lock);
>   
>   	return 0;
>   
> -err_unmap:
> -	guc_log_unmap(guc);
>   err_relay:
>   	guc_log_relay_destroy(guc);
>   err:

-- 
Thanks,
Sagar

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

Re: [PATCH v2 07/15] drm/i915/guc: Flush directly in log unregister

[Sagar Arun Kamble]

On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> 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 | 34 ++++++++++++++--------------------
>   1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index ee0981f5a208..5ff4b510569a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -443,25 +443,6 @@ static void guc_log_capture_logs(struct intel_guc *guc)
>   	intel_runtime_pm_put(dev_priv);
>   }
>   
> -static void guc_flush_logs(struct intel_guc *guc)
> -{
> -	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(&guc->log.runtime.flush_work);
> -
> -	/* Ask GuC to update the log buffer state */
> -	intel_runtime_pm_get(dev_priv);
> -	guc_log_flush(guc);
> -	intel_runtime_pm_put(dev_priv);
> -
> -	/* GuC would have updated log buffer by now, so capture it */
> -	guc_log_capture_logs(guc);
> -}
> -
>   int intel_guc_log_create(struct intel_guc *guc)
>   {
>   	struct i915_vma *vma;
> @@ -630,15 +611,28 @@ int intel_guc_log_register(struct intel_guc *guc)
>   
>   void intel_guc_log_unregister(struct intel_guc *guc)
>   {
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
> +
>   	guc_log_flush_irq_disable(guc);
>   
> +	/*
> +	 * Before initiating the forceful flush, wait for any pending/ongoing
> +	 * flush to complete otherwise forceful flush may not actually happen.
> +	 */
> +	flush_work(&guc->log.runtime.flush_work);
> +
>   	/*
>   	 * 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.
>   	 */
> -	guc_flush_logs(guc);
> +	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(guc);
>   
>   	mutex_lock(&guc->log.runtime.lock);
>   

-- 
Thanks,
Sagar

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

Re: [PATCH v2 08/15] drm/i915/guc: Split relay control and GuC log level

[Sagar Arun Kamble]

On 3/8/2018 9:17 PM, Michał Winiarski wrote:
> 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
>
> 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>
<snip>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index 8c26cce77a98..df91f12a36ed 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -61,9 +61,10 @@ struct intel_guc_log {
>   int intel_guc_log_create(struct intel_guc *guc);
>   void intel_guc_log_destroy(struct intel_guc *guc);
>   void intel_guc_log_init_early(struct intel_guc *guc);
> -int intel_guc_log_control_get(struct intel_guc *guc);
> -int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
> -int intel_guc_log_register(struct intel_guc *guc);
> -void intel_guc_log_unregister(struct intel_guc *guc);
> +int intel_guc_log_level_get(struct intel_guc *guc);
> +int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
> +int intel_guc_log_relay_open(struct intel_guc *guc);
> +void intel_guc_log_relay_close(struct intel_guc *guc);
> +void intel_guc_log_relay_flush(struct intel_guc *guc);
Need to maintain order of definition. init_early is also not in order.
With that:
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 90d2f38e22c9..abce0e38528a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -219,28 +219,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);
> -
> -	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);
> -}
> -
>   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 d6af984cd789..f76d51d1ce70 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -31,8 +31,6 @@
>   void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>   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);

-- 
Thanks,
Sagar

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

Re: [PATCH v2 10/15] drm/i915/guc: Get rid of GuC log runtime

[Sagar Arun Kamble]

On 3/8/2018 9:17 PM, Michał Winiarski wrote:
> 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>
Some might want other stats too as part of relay since we print them 
when relay is enabled.
But this is not a big issue. w/ or w/o that change:
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 | 66 ++++++++++++++++++------------------
>   drivers/gpu/drm/i915/intel_guc_log.h |  7 ++--
>   4 files changed, 44 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f99fe9910634..d7c0bf6facf6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2348,8 +2348,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 0d92caf6a83f..cab158e42577 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)
> @@ -389,8 +389,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 92a7bf0fd729..7c4339dae534 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -151,10 +151,10 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
>   	smp_wmb();
>   
>   	/* All data has been written, so now move the offset of sub buffer. */
> -	relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
> +	relay_reserve(guc->log.relay.channel, guc->log.vma->obj->base.size);
>   
>   	/* Switch to the next sub buffer */
> -	relay_flush(guc->log.runtime.relay_chan);
> +	relay_flush(guc->log.relay.channel);
>   }
>   
>   static void *guc_get_write_buffer(struct intel_guc *guc)
> @@ -168,7 +168,7 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
>   	 * done without using relay_reserve() along with relay_write(). So its
>   	 * better to use relay_reserve() alone.
>   	 */
> -	return relay_reserve(guc->log.runtime.relay_chan, 0);
> +	return relay_reserve(guc->log.relay.channel, 0);
>   }
>   
>   static bool guc_check_log_buf_overflow(struct intel_guc *guc,
> @@ -219,13 +219,13 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	void *src_data, *dst_data;
>   	bool new_overflow;
>   
> -	mutex_lock(&guc->log.runtime.lock);
> +	mutex_lock(&guc->log.relay.lock);
>   
> -	if (WARN_ON(!guc->log.runtime.buf_addr))
> +	if (WARN_ON(!guc->log.relay.buf_addr))
>   		goto out_unlock;
>   
>   	/* Get the pointer to shared GuC log buffer */
> -	log_buf_state = src_data = guc->log.runtime.buf_addr;
> +	log_buf_state = src_data = guc->log.relay.buf_addr;
>   
>   	/* Get the pointer to local buffer to store the logs */
>   	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> @@ -236,7 +236,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   		 * getting consumed by User at a slow rate.
>   		 */
>   		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
> -		guc->log.capture_miss_count++;
> +		guc->log.relay.full_count++;
>   
>   		goto out_unlock;
>   	}
> @@ -310,20 +310,20 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	guc_move_to_next_buf(guc);
>   
>   out_unlock:
> -	mutex_unlock(&guc->log.runtime.lock);
> +	mutex_unlock(&guc->log.relay.lock);
>   }
>   
>   static void capture_logs_work(struct work_struct *work)
>   {
>   	struct intel_guc *guc =
> -		container_of(work, struct intel_guc, log.runtime.flush_work);
> +		container_of(work, struct intel_guc, log.relay.flush_work);
>   
>   	guc_log_capture_logs(guc);
>   }
>   
> -static bool guc_log_has_runtime(struct intel_guc *guc)
> +static bool guc_log_relay_enabled(struct intel_guc *guc)
>   {
> -	return guc->log.runtime.buf_addr != NULL;
> +	return guc->log.relay.buf_addr != NULL;
>   }
>   
>   static int guc_log_map(struct intel_guc *guc)
> @@ -332,7 +332,7 @@ static int guc_log_map(struct intel_guc *guc)
>   	void *vaddr;
>   	int ret;
>   
> -	lockdep_assert_held(&guc->log.runtime.lock);
> +	lockdep_assert_held(&guc->log.relay.lock);
>   
>   	if (!guc->log.vma)
>   		return -ENODEV;
> @@ -354,23 +354,23 @@ static int guc_log_map(struct intel_guc *guc)
>   		return PTR_ERR(vaddr);
>   	}
>   
> -	guc->log.runtime.buf_addr = vaddr;
> +	guc->log.relay.buf_addr = vaddr;
>   
>   	return 0;
>   }
>   
>   static void guc_log_unmap(struct intel_guc *guc)
>   {
> -	lockdep_assert_held(&guc->log.runtime.lock);
> +	lockdep_assert_held(&guc->log.relay.lock);
>   
>   	i915_gem_object_unpin_map(guc->log.vma->obj);
> -	guc->log.runtime.buf_addr = NULL;
> +	guc->log.relay.buf_addr = NULL;
>   }
>   
>   void intel_guc_log_init_early(struct intel_guc *guc)
>   {
> -	mutex_init(&guc->log.runtime.lock);
> -	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
> +	mutex_init(&guc->log.relay.lock);
> +	INIT_WORK(&guc->log.relay.flush_work, capture_logs_work);
>   }
>   
>   static int guc_log_relay_create(struct intel_guc *guc)
> @@ -380,7 +380,7 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   	size_t n_subbufs, subbuf_size;
>   	int ret;
>   
> -	lockdep_assert_held(&guc->log.runtime.lock);
> +	lockdep_assert_held(&guc->log.relay.lock);
>   
>   	 /* Keep the size of sub buffers same as shared log buffer */
>   	subbuf_size = GUC_LOG_SIZE;
> @@ -410,17 +410,17 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   	}
>   
>   	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
> -	guc->log.runtime.relay_chan = guc_log_relay_chan;
> +	guc->log.relay.channel = guc_log_relay_chan;
>   
>   	return 0;
>   }
>   
>   static void guc_log_relay_destroy(struct intel_guc *guc)
>   {
> -	lockdep_assert_held(&guc->log.runtime.lock);
> +	lockdep_assert_held(&guc->log.relay.lock);
>   
> -	relay_close(guc->log.runtime.relay_chan);
> -	guc->log.runtime.relay_chan = NULL;
> +	relay_close(guc->log.relay.channel);
> +	guc->log.relay.channel = NULL;
>   }
>   
>   static void guc_log_capture_logs(struct intel_guc *guc)
> @@ -553,9 +553,9 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
>   {
>   	int ret;
>   
> -	mutex_lock(&guc->log.runtime.lock);
> +	mutex_lock(&guc->log.relay.lock);
>   
> -	if (guc_log_has_runtime(guc)) {
> +	if (guc_log_relay_enabled(guc)) {
>   		ret = -EEXIST;
>   		goto out_unlock;
>   	}
> @@ -578,7 +578,7 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
>   	if (ret)
>   		goto out_relay;
>   
> -	mutex_unlock(&guc->log.runtime.lock);
> +	mutex_unlock(&guc->log.relay.lock);
>   
>   	guc_log_flush_irq_enable(guc);
>   
> @@ -587,14 +587,14 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
>   	 * the flush notification. This means that we need to unconditionally
>   	 * flush on relay enabling, since GuC only notifies us once.
>   	 */
> -	queue_work(guc->log.runtime.flush_wq, &guc->log.runtime.flush_work);
> +	queue_work(guc->log.relay.flush_wq, &guc->log.relay.flush_work);
>   
>   	return 0;
>   
>   out_relay:
>   	guc_log_relay_destroy(guc);
>   out_unlock:
> -	mutex_unlock(&guc->log.runtime.lock);
> +	mutex_unlock(&guc->log.relay.lock);
>   
>   	return ret;
>   }
> @@ -607,7 +607,7 @@ void intel_guc_log_relay_flush(struct intel_guc *guc)
>   	 * Before initiating the forceful flush, wait for any pending/ongoing
>   	 * flush to complete otherwise forceful flush may not actually happen.
>   	 */
> -	flush_work(&guc->log.runtime.flush_work);
> +	flush_work(&guc->log.relay.flush_work);
>   
>   	intel_runtime_pm_get(i915);
>   	guc_log_flush(guc);
> @@ -620,11 +620,11 @@ void intel_guc_log_relay_flush(struct intel_guc *guc)
>   void intel_guc_log_relay_close(struct intel_guc *guc)
>   {
>   	guc_log_flush_irq_disable(guc);
> -	flush_work(&guc->log.runtime.flush_work);
> +	flush_work(&guc->log.relay.flush_work);
>   
> -	mutex_lock(&guc->log.runtime.lock);
> -	GEM_BUG_ON(!guc_log_has_runtime(guc));
> - 	guc_log_unmap(guc);
> +	mutex_lock(&guc->log.relay.lock);
> +	GEM_BUG_ON(!guc_log_relay_enabled(guc));
> +	guc_log_unmap(guc);
>   	guc_log_relay_destroy(guc);
> -	mutex_unlock(&guc->log.runtime.lock);
> +	mutex_unlock(&guc->log.relay.lock);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index df91f12a36ed..9b1257ea2673 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -42,16 +42,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];

-- 
Thanks,
Sagar

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

Re: [PATCH v2 08/15] drm/i915/guc: Split relay control and GuC log level

[Michal Wajdeczko]

On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski  
<michal.winiarski@intel.com> wrote:

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

/snip/

> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 90d2f38e22c9..abce0e38528a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -219,28 +219,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);
> -
> -	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);
> -}
> -
>  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 d6af984cd789..f76d51d1ce70 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -31,8 +31,6 @@
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>  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);

hmm, it looks that timelines of these two functions were very short
(from patch 2 till patch 8) - so maybe by doing some patch re-order
can we fully skip them ?

>  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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 12/15] drm/i915/guc: Don't print out relay statistics when relay is disabled

[Sagar Arun Kamble]

On 3/8/2018 9:17 PM, Michał Winiarski wrote:
> 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)
>
> 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>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c  | 45 ++++++++++++++++++++++++------------
>   drivers/gpu/drm/i915/intel_guc.c     |  5 +---
>   drivers/gpu/drm/i915/intel_guc_log.c | 22 +++++++++---------
>   drivers/gpu/drm/i915/intel_guc_log.h | 10 ++++----
>   4 files changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d29bacb1f308..94516ab4eaed 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2326,30 +2326,45 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>   	return 0;
>   }
>   
> +static const char *
> +stringify_guc_log_type(enum guc_log_buffer_type type)
> +{
> +	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);
> +	}
> +
> +	return "";
> +}
> +
>   static void i915_guc_log_info(struct seq_file *m,
>   			      struct drm_i915_private *dev_priv)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> +	enum guc_log_buffer_type type;
>   
> -	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],
> -		   guc->log.total_overflow_count[GUC_ISR_LOG_BUFFER]);
> -
> -	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]);
> -
> -	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(guc)) {
> +		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);
> +
> +	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),
> +			   guc->log.stats[type].flush,
> +			   guc->log.stats[type].overflow);
Didn't see this earlier. Should be sampled_overflow.
And ordering of intel_guc_log_relay_enabled() declaration w.r.t other 
declarations needs to be fixed.
With that:
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> +	}
>   }
>   
>   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 cab158e42577..3e2f0f8503ed 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -388,12 +388,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 7c4339dae534..a72fe4a369f4 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -171,22 +171,22 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
>   	return relay_reserve(guc->log.relay.channel, 0);
>   }
>   
> -static bool guc_check_log_buf_overflow(struct intel_guc *guc,
> +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 = guc->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;
>   
> -		guc->log.prev_overflow_count[type] = full_cnt;
> -		guc->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 */
> -			guc->log.total_overflow_count[type] += 16;
> +			log->stats[type].sampled_overflow += 16;
>   		}
>   		DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
>   	}
> @@ -221,7 +221,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   
>   	mutex_lock(&guc->log.relay.lock);
>   
> -	if (WARN_ON(!guc->log.relay.buf_addr))
> +	if (WARN_ON(!intel_guc_log_relay_enabled(guc)))
>   		goto out_unlock;
>   
>   	/* Get the pointer to shared GuC log buffer */
> @@ -259,8 +259,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   		full_cnt = log_buf_state_local.buffer_full_cnt;
>   
>   		/* Bookkeeping stuff */
> -		guc->log.flush_count[type] += log_buf_state_local.flush_to_file;
> -		new_overflow = guc_check_log_buf_overflow(guc, type, full_cnt);
> +		guc->log.stats[type].flush += log_buf_state_local.flush_to_file;
> +		new_overflow = guc_check_log_buf_overflow(&guc->log, type, full_cnt);
>   
>   		/* Update the state of shared log buffer */
>   		log_buf_state->read_ptr = write_offset;
> @@ -321,7 +321,7 @@ static void capture_logs_work(struct work_struct *work)
>   	guc_log_capture_logs(guc);
>   }
>   
> -static bool guc_log_relay_enabled(struct intel_guc *guc)
> +bool intel_guc_log_relay_enabled(struct intel_guc *guc)
>   {
>   	return guc->log.relay.buf_addr != NULL;
>   }
> @@ -555,7 +555,7 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
>   
>   	mutex_lock(&guc->log.relay.lock);
>   
> -	if (guc_log_relay_enabled(guc)) {
> +	if (intel_guc_log_relay_enabled(guc)) {
>   		ret = -EEXIST;
>   		goto out_unlock;
>   	}
> @@ -623,7 +623,7 @@ void intel_guc_log_relay_close(struct intel_guc *guc)
>   	flush_work(&guc->log.relay.flush_work);
>   
>   	mutex_lock(&guc->log.relay.lock);
> -	GEM_BUG_ON(!guc_log_relay_enabled(guc));
> +	GEM_BUG_ON(!intel_guc_log_relay_enabled(guc));
>   	guc_log_unmap(guc);
>   	guc_log_relay_destroy(guc);
>   	mutex_unlock(&guc->log.relay.lock);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index 9b1257ea2673..8804c5c27409 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -51,10 +51,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];
>   };
>   
>   int intel_guc_log_create(struct intel_guc *guc);
> @@ -65,5 +66,6 @@ int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
>   int intel_guc_log_relay_open(struct intel_guc *guc);
>   void intel_guc_log_relay_close(struct intel_guc *guc);
>   void intel_guc_log_relay_flush(struct intel_guc *guc);
> +bool intel_guc_log_relay_enabled(struct intel_guc *guc);
>   
>   #endif

-- 
Thanks,
Sagar

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

Re: [PATCH v2 14/15] drm/i915/guc: Default to non-verbose GuC logging

[Sagar Arun Kamble]

On 3/8/2018 9:17 PM, Michał Winiarski wrote:
> 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> (v1)
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 3fb5f75aa7c9..2f579fff58cd 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() &&

-- 
Thanks,
Sagar

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

Re: [PATCH v2 08/15] drm/i915/guc: Split relay control and GuC log level

[Michał Winiarski]

On Fri, Mar 09, 2018 at 12:00:06PM +0100, Michal Wajdeczko wrote:
> On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski
> <michal.winiarski@intel.com> wrote:
> 
> > 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
> > 
> > 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>
> > ---
> 
> /snip/
> 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > b/drivers/gpu/drm/i915/intel_uc.c
> > index 90d2f38e22c9..abce0e38528a 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -219,28 +219,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);
> > -
> > -	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);
> > -}
> > -
> >  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 d6af984cd789..f76d51d1ce70 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -31,8 +31,6 @@
> >  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
> >  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);
> 
> hmm, it looks that timelines of these two functions were very short
> (from patch 2 till patch 8) - so maybe by doing some patch re-order
> can we fully skip them ?

I could drop patch 2 entirely and go straight to the decoupling - but that means
more code movement overall and way more content in this one.

I don't really see a better split (but since I'm the author I'm biased :) ).
I'm open for suggestions.

-Michał

> 
> >  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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 08/15] drm/i915/guc: Split relay control and GuC log level

[Chris Wilson]

Quoting Michał Winiarski (2018-03-09 16:30:42)
> On Fri, Mar 09, 2018 at 12:00:06PM +0100, Michal Wajdeczko wrote:
> > On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski
> > <michal.winiarski@intel.com> wrote:
> > 
> > > 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
> > > 
> > > 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>
> > > ---
> > 
> > /snip/
> > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > > b/drivers/gpu/drm/i915/intel_uc.c
> > > index 90d2f38e22c9..abce0e38528a 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > @@ -219,28 +219,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);
> > > -
> > > -   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);
> > > -}
> > > -
> > >  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 d6af984cd789..f76d51d1ce70 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.h
> > > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > > @@ -31,8 +31,6 @@
> > >  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
> > >  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);
> > 
> > hmm, it looks that timelines of these two functions were very short
> > (from patch 2 till patch 8) - so maybe by doing some patch re-order
> > can we fully skip them ?
> 
> I could drop patch 2 entirely and go straight to the decoupling - but that means
> more code movement overall and way more content in this one.
> 
> I don't really see a better split (but since I'm the author I'm biased :) ).
> I'm open for suggestions.

I can settle the argument by grabbing the first 3 patches as code
movement, before we get into the details. Ok?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx