[PATCH 1/3] drm/i915/guc: Fix return from guc_log_relay_file_create

[PATCH 1/3] drm/i915/guc: Fix return from guc_log_relay_file_create

[Sagar Arun Kamble]

guc_log_relay_file_create will return -EEXIST if we invoke
relay_late_setup_files multiple times as part of i915_guc_log_control.
However this is to be not cosidered as fail and need to return 0.
This was mistakenly introduced in the below commit. Fix it.

Fixes: 70deeaddc6e6 "drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex"
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 86a3321..3fbe93a 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -185,6 +185,8 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 		goto out_unlock;
 	}
 
+	ret = 0;
+
 out_unlock:
 	mutex_unlock(&guc->log.runtime.relay_lock);
 	return ret;
-- 
1.9.1

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

[PATCH 2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup

[Sagar Arun Kamble]

On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
movntdqa is needed for efficient capture of the logs from uncached GuC
log buffer. GuC init was tied with this support and other setup needed
for interrupt based GuC log capture like relay channel/file support and
uncached mapping support. With this patch, GuC init is now unblocked from
lack of this support.
SSE and relay support init/fini is now being done by new functions
intel_guc_log_init|fini_runtime() which makes relay functions static.
We have introduced two states "supported" and "enabled". Supported is set
when we have SSE4.1 support and have relay, GuC log, WC mapping available.
Enabled is set when support is present and user has requested logging
through i915_modparams.guc_log_level.
While at this change, fixed unwind order in intel_uc_fini_misc.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c     |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++-
 drivers/gpu/drm/i915/intel_uc.c      | 14 ++----
 4 files changed, 90 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 21140cc..a9e5848 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -458,7 +458,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915_modparams.guc_log_level)
+	if (guc->log.runtime.enabled)
 		gen9_enable_guc_interrupts(dev_priv);
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 3fbe93a..3454e0c 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -438,7 +438,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;
@@ -486,12 +486,10 @@ int intel_guc_log_relay_create(struct intel_guc *guc)
 
 err:
 	mutex_unlock(&guc->log.runtime.relay_lock);
-	/* logging will be off */
-	i915_modparams.guc_log_level = 0;
 	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);
 
@@ -509,18 +507,51 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc)
 	mutex_unlock(&guc->log.runtime.relay_lock);
 }
 
+void intel_guc_log_init_runtime(struct intel_guc *guc)
+{
+	/*
+	 * We require SSE 4.1 for fast reads from the GuC log buffer and
+	 * it should likely be present on the chipsets supporting GuC based
+	 * submisssions. Runtime logging is to be supported if SSE 4.1 support
+	 * is present and relay & uncached buffer can be setup.
+	 */
+	guc->log.runtime.supported = i915_has_memcpy_from_wc();
+	if (!guc->log.runtime.supported) {
+		DRM_DEBUG_DRIVER("SSE4.1 support not present. Runtime logging "
+				 "disabled\n");
+		return;
+	}
+
+	if (guc_log_relay_create(guc)) {
+		DRM_ERROR("Couldn't allocate relay for GuC log. Runtime logging"
+			  " disabled\n");
+		guc->log.runtime.supported = false;
+	}
+}
+
+void intel_guc_log_fini_runtime(struct intel_guc *guc)
+{
+	guc_log_relay_destroy(guc);
+	guc->log.runtime.supported = false;
+}
+
 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.runtime.supported) {
+		ret = -ENODEV;
+		goto err;
+	}
+
 	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);
+		ret = guc_log_relay_create(guc);
 		if (ret)
 			goto err;
 
@@ -545,10 +576,18 @@ static int guc_log_late_setup(struct intel_guc *guc)
 	guc_log_runtime_destroy(guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 err_relay:
-	intel_guc_log_relay_destroy(guc);
+	guc_log_relay_destroy(guc);
 err:
-	/* logging will remain off */
-	i915_modparams.guc_log_level = 0;
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	intel_runtime_pm_get(dev_priv);
+	gen9_disable_guc_interrupts(dev_priv);
+	guc->log.runtime.enabled = false;
+	intel_runtime_pm_put(dev_priv);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	/* runtime logging will remain off */
+	guc->log.runtime.supported = false;
+
 	return ret;
 }
 
@@ -571,7 +610,7 @@ 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)
+	if (!USES_GUC_SUBMISSION(dev_priv) || !guc->log.runtime.enabled)
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -605,16 +644,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);
@@ -623,28 +652,41 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 	guc->log.vma = vma;
 
-	if (i915_modparams.guc_log_level) {
+	/*
+	 * We are making runtime creation optional based on support as
+	 * we can continue to use GuC and base log support.
+	 */
+	if (i915_modparams.guc_log_level && guc->log.runtime.supported) {
 		ret = guc_log_runtime_create(guc);
-		if (ret < 0)
-			goto err_vma;
+		if (ret < 0) {
+			DRM_ERROR("GuC log runtime create failed. Runtime "
+				  "logging disabled\n");
+			guc->log.runtime.supported = false;
+		}
 	}
 
 	/* each allocated unit is a page */
-	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+	flags = GUC_LOG_VALID |
 		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
 		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
 		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
 
+	if (guc->log.runtime.supported) {
+		flags |= GUC_LOG_NOTIFY_ON_HALF_FULL;
+		guc->log.runtime.enabled = true;
+	}
+
 	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
 	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 
 	return 0;
 
-err_vma:
-	i915_vma_unpin_and_release(&guc->log.vma);
 err:
 	/* logging will be off */
 	i915_modparams.guc_log_level = 0;
+	/* runtime logging will remain off */
+	guc->log.runtime.supported = false;
+
 	return ret;
 }
 
@@ -720,6 +762,8 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
 		i915_modparams.guc_log_level = 0;
 	}
 
+	guc->log.runtime.enabled = enable_logging;
+
 	return ret;
 }
 
@@ -742,10 +786,11 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	intel_runtime_pm_get(dev_priv);
 	gen9_disable_guc_interrupts(dev_priv);
+	guc->log.runtime.enabled = false;
 	intel_runtime_pm_put(dev_priv);
 
 	guc_log_runtime_destroy(guc);
 	mutex_unlock(&dev_priv->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 dab0e94..c89c331 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -44,6 +44,18 @@ struct intel_guc_log {
 	struct i915_vma *vma;
 	/* The runtime stuff gets created only when GuC logging gets enabled */
 	struct {
+		/*
+		 * Support based on movntdqa availability, GuC log, relay and
+		 * runtime creation status.
+		 */
+		bool supported;
+
+		/*
+		 * Status based on "supported" and user request for logging
+		 * through i915_modparams.guc_log_level.
+		 */
+		bool enabled;
+
 		void *buf_addr;
 		struct workqueue_struct *flush_wq;
 		struct work_struct flush_work;
@@ -62,8 +74,8 @@ 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);
+void intel_guc_log_init_runtime(struct intel_guc *guc);
+void intel_guc_log_fini_runtime(struct intel_guc *guc);
 int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index e3f3509..1cc0d86 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -250,16 +250,10 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv)
 		goto err;
 	}
 
-	ret = intel_guc_log_relay_create(guc);
-	if (ret) {
-		DRM_ERROR("Couldn't allocate relay for GuC log\n");
-		goto err_relay;
-	}
+	intel_guc_log_init_runtime(guc);
 
 	return 0;
 
-err_relay:
-	intel_guc_fini_wq(guc);
 err:
 	return ret;
 }
@@ -271,9 +265,9 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
 	if (!USES_GUC(dev_priv))
 		return;
 
-	intel_guc_fini_wq(guc);
+	intel_guc_log_fini_runtime(guc);
 
-	intel_guc_log_relay_destroy(guc);
+	intel_guc_fini_wq(guc);
 }
 
 int intel_uc_init(struct drm_i915_private *dev_priv)
@@ -386,7 +380,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		if (i915_modparams.guc_log_level)
+		if (guc->log.runtime.enabled)
 			gen9_enable_guc_interrupts(dev_priv);
 
 		ret = intel_guc_submission_enable(guc);
-- 
1.9.1

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

[PATCH 3/3] [HAX] drm/i915/guc: Enable GuC and logging for CI

[Sagar Arun Kamble]

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h   | 4 ++--
 drivers/gpu/drm/i915/intel_guc_log.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 430f5f9..53037b5 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,8 +47,8 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
-	param(int, guc_log_level, 0) \
+	param(int, enable_guc, -1) \
+	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
 	param(int, mmio_debug, 0) \
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 3454e0c..72c19e2 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -251,7 +251,7 @@ static bool guc_check_log_buf_overflow(struct intel_guc *guc,
 			/* buffer_full_cnt is a 4 bit counter */
 			guc->log.total_overflow_count[type] += 16;
 		}
-		DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
+		//DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
 	}
 
 	return overflow;
@@ -298,7 +298,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		 * Used rate limited to avoid deluge of messages, logs might be
 		 * getting consumed by User at a slow rate.
 		 */
-		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
+		//DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
 		guc->log.capture_miss_count++;
 		mutex_unlock(&guc->log.runtime.relay_lock);
 
-- 
1.9.1

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Fix return from guc_log_relay_file_create

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/guc: Fix return from guc_log_relay_file_create
URL   : https://patchwork.freedesktop.org/series/37382/
State : success

== Summary ==

Series 37382v1 series starting with [1/3] drm/i915/guc: Fix return from guc_log_relay_file_create
https://patchwork.freedesktop.org/api/1.0/series/37382/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:429s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:427s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:488s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:285s
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:488s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:471s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:465s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:580s
fi-elk-e7500     total:224  pass:168  dwarn:10  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:279s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:516s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:394s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:401s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:416s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:415s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:463s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:502s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:463s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:433s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:515s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:529s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:496s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:490s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:419s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:401s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:475s

0edf4cc4e7c163fa61312f1708fc92284b855f8e drm-tip: 2018y-01m-30d-18h-33m-48s UTC integration manifest
3f6a2a11e9e8 drm/i915/guc: Enable GuC and logging for CI
586b96e8dd64 drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup
67ad37580423 drm/i915/guc: Fix return from guc_log_relay_file_create

== Logs ==

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

Re: [PATCH 1/3] drm/i915/guc: Fix return from guc_log_relay_file_create

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-31 06:14:37)
> guc_log_relay_file_create will return -EEXIST if we invoke
> relay_late_setup_files multiple times as part of i915_guc_log_control.
> However this is to be not cosidered as fail and need to return 0.
> This was mistakenly introduced in the below commit. Fix it.
> 
> Fixes: 70deeaddc6e6 "drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex"
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

Testcase?
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-31 06:14:38)
> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
> movntdqa is needed for efficient capture of the logs from uncached GuC
> log buffer. GuC init was tied with this support and other setup needed
> for interrupt based GuC log capture like relay channel/file support and
> uncached mapping support. With this patch, GuC init is now unblocked from
> lack of this support.
> SSE and relay support init/fini is now being done by new functions
> intel_guc_log_init|fini_runtime() which makes relay functions static.
> We have introduced two states "supported" and "enabled". Supported is set
> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
> Enabled is set when support is present and user has requested logging
> through i915_modparams.guc_log_level.
> While at this change, fixed unwind order in intel_uc_fini_misc.

Downside would appear to be the loss of feedback in i915.guc_log_level
when it fail? Otherwise, looks tidy enough.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/guc: Fix return from guc_log_relay_file_create

[Sagar Arun Kamble]

On 1/31/2018 3:06 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-01-31 06:14:37)
>> guc_log_relay_file_create will return -EEXIST if we invoke
>> relay_late_setup_files multiple times as part of i915_guc_log_control.
>> However this is to be not cosidered as fail and need to return 0.
>> This was mistakenly introduced in the below commit. Fix it.
>>
>> Fixes: 70deeaddc6e6 "drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex"
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Testcase?
Yes. will include.
Thanks for the review.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

-- 
Thanks,
Sagar

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

Re: [PATCH 2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup

[Sagar Arun Kamble]

On 1/31/2018 3:08 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-01-31 06:14:38)
>> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
>> movntdqa is needed for efficient capture of the logs from uncached GuC
>> log buffer. GuC init was tied with this support and other setup needed
>> for interrupt based GuC log capture like relay channel/file support and
>> uncached mapping support. With this patch, GuC init is now unblocked from
>> lack of this support.
>> SSE and relay support init/fini is now being done by new functions
>> intel_guc_log_init|fini_runtime() which makes relay functions static.
>> We have introduced two states "supported" and "enabled". Supported is set
>> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
>> Enabled is set when support is present and user has requested logging
>> through i915_modparams.guc_log_level.
>> While at this change, fixed unwind order in intel_uc_fini_misc.
> Downside would appear to be the loss of feedback in i915.guc_log_level
> when it fail?
This patch decouples the guc_log_level from only interrupt based log 
support.
We can continue to use/know guc_log_level to send the verbosity to GuC 
still.
Have to rely only on static dumps like i915_guc_log_dump or 
error_uc->guc_log wherever runtime logging is off.
>   Otherwise, looks tidy enough.
> -Chris

-- 
Thanks,
Sagar

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

Re: [PATCH 2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup

[Michał Winiarski]

On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote:
> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
> movntdqa is needed for efficient capture of the logs from uncached GuC
> log buffer. GuC init was tied with this support and other setup needed
> for interrupt based GuC log capture like relay channel/file support and
> uncached mapping support. With this patch, GuC init is now unblocked from
> lack of this support.
> SSE and relay support init/fini is now being done by new functions
> intel_guc_log_init|fini_runtime() which makes relay functions static.
> We have introduced two states "supported" and "enabled". Supported is set
> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
> Enabled is set when support is present and user has requested logging
> through i915_modparams.guc_log_level.
> While at this change, fixed unwind order in intel_uc_fini_misc.

Or, we could rework GuC log a bit.
We could change the meaning of guc_log_level - it would only affect the
"internal" GuC state (verbosity - in other words, the content of buffer used by
GuC). This allows userspace to inspect the content of GuC buffer
(i915_guc_log_dump), but i915 is not copying the data to the relay (we're
ignoring the interrupts recieved from GuC).

This means that we need to introduce alternative way of controling the relay,
let's say another file called "i915_guc_log_relay". Opening this file causes the
relay to be created, we can throw an error if we don't have SSE4.1 there.
(well - the whole runtime is created actually, things are quite badly named
today, runtime means mapping of buffer shared with GuC, relay is the buffer
"shared" with userspace, functions operating on "runtime" should probably setup
both of those rather than just the mapping).

We're also solving the problem of overflows if GuC is loaded with
"guc_log_level > 0" while nobody is consuming the data from the relay.
And we don't really need to handle the "runtime" at module load time at all,
which simplifies things.

I'm working on a series that does all of the above.

-Michał

> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     |  2 +-
>  drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++-
>  drivers/gpu/drm/i915/intel_uc.c      | 14 ++----
>  4 files changed, 90 insertions(+), 39 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup

[Chris Wilson]

Quoting Michał Winiarski (2018-01-31 10:37:20)
> On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote:
> > On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
> > movntdqa is needed for efficient capture of the logs from uncached GuC
> > log buffer. GuC init was tied with this support and other setup needed
> > for interrupt based GuC log capture like relay channel/file support and
> > uncached mapping support. With this patch, GuC init is now unblocked from
> > lack of this support.
> > SSE and relay support init/fini is now being done by new functions
> > intel_guc_log_init|fini_runtime() which makes relay functions static.
> > We have introduced two states "supported" and "enabled". Supported is set
> > when we have SSE4.1 support and have relay, GuC log, WC mapping available.
> > Enabled is set when support is present and user has requested logging
> > through i915_modparams.guc_log_level.
> > While at this change, fixed unwind order in intel_uc_fini_misc.
> 
> Or, we could rework GuC log a bit.
> We could change the meaning of guc_log_level - it would only affect the
> "internal" GuC state (verbosity - in other words, the content of buffer used by
> GuC). This allows userspace to inspect the content of GuC buffer
> (i915_guc_log_dump), but i915 is not copying the data to the relay (we're
> ignoring the interrupts recieved from GuC).
> 
> This means that we need to introduce alternative way of controling the relay,
> let's say another file called "i915_guc_log_relay". Opening this file causes the
> relay to be created, we can throw an error if we don't have SSE4.1 there.
> (well - the whole runtime is created actually, things are quite badly named
> today, runtime means mapping of buffer shared with GuC, relay is the buffer
> "shared" with userspace, functions operating on "runtime" should probably setup
> both of those rather than just the mapping).
> 
> We're also solving the problem of overflows if GuC is loaded with
> "guc_log_level > 0" while nobody is consuming the data from the relay.
> And we don't really need to handle the "runtime" at module load time at all,
> which simplifies things.
> 
> I'm working on a series that does all of the above.

I'll push the 1st patch (the -EEXISTS fix) and we can come back for the
rest.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/guc: Handle interrupt based logging with lack of SSE4.1, relay and other setup

[Sagar Arun Kamble]

On 1/31/2018 4:07 PM, Michał Winiarski wrote:
> On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote:
>> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available.
>> movntdqa is needed for efficient capture of the logs from uncached GuC
>> log buffer. GuC init was tied with this support and other setup needed
>> for interrupt based GuC log capture like relay channel/file support and
>> uncached mapping support. With this patch, GuC init is now unblocked from
>> lack of this support.
>> SSE and relay support init/fini is now being done by new functions
>> intel_guc_log_init|fini_runtime() which makes relay functions static.
>> We have introduced two states "supported" and "enabled". Supported is set
>> when we have SSE4.1 support and have relay, GuC log, WC mapping available.
>> Enabled is set when support is present and user has requested logging
>> through i915_modparams.guc_log_level.
>> While at this change, fixed unwind order in intel_uc_fini_misc.
> Or, we could rework GuC log a bit.
> We could change the meaning of guc_log_level - it would only affect the
> "internal" GuC state (verbosity - in other words, the content of buffer used by
> GuC). This allows userspace to inspect the content of GuC buffer
> (i915_guc_log_dump), but i915 is not copying the data to the relay (we're
> ignoring the interrupts recieved from GuC).
>
> This means that we need to introduce alternative way of controling the relay,
> let's say another file called "i915_guc_log_relay". Opening this file causes the
> relay to be created, we can throw an error if we don't have SSE4.1 there.
> (well - the whole runtime is created actually, things are quite badly named
> today, runtime means mapping of buffer shared with GuC, relay is the buffer
> "shared" with userspace, functions operating on "runtime" should probably setup
> both of those rather than just the mapping).
True. Setup spread over different stages of load made it little 
complicated to change and your change will help.
> We're also solving the problem of overflows if GuC is loaded with
> "guc_log_level > 0" while nobody is consuming the data from the relay.
> And we don't really need to handle the "runtime" at module load time at all,
> which simplifies things.
Yes. Agree on this approach. Thanks.
> I'm working on a series that does all of the above.
>
> -Michał
>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c     |  2 +-
>>   drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++----------
>>   drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++-
>>   drivers/gpu/drm/i915/intel_uc.c      | 14 ++----
>>   4 files changed, 90 insertions(+), 39 deletions(-)

-- 
Thanks,
Sagar

_______________________________________________
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 [1/3] drm/i915/guc: Fix return from guc_log_relay_file_create

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/guc: Fix return from guc_log_relay_file_create
URL   : https://patchwork.freedesktop.org/series/37382/
State : failure

== Summary ==

Test kms_cursor_legacy:
        Subgroup 2x-long-flip-vs-cursor-legacy:
                fail       -> PASS       (shard-hsw) fdo#104873
Test kms_flip:
        Subgroup 2x-flip-vs-modeset-interruptible:
                pass       -> INCOMPLETE (shard-hsw) fdo#102614
        Subgroup wf_vblank-ts-check-interruptible:
                fail       -> PASS       (shard-snb) fdo#100368
        Subgroup 2x-flip-vs-modeset-vs-hang:
                pass       -> DMESG-WARN (shard-hsw)
Test perf:
        Subgroup oa-exponents:
                pass       -> FAIL       (shard-apl) fdo#102254
        Subgroup gen8-unprivileged-single-ctx-counters:
                pass       -> FAIL       (shard-apl)
Test drv_selftest:
        Subgroup live_guc:
                pass       -> DMESG-WARN (shard-apl)
Test perf_pmu:
        Subgroup all-busy-check-all:
                pass       -> FAIL       (shard-apl)
Test pm_rc6_residency:
        Subgroup rc6-accuracy:
                skip       -> PASS       (shard-snb)
Test gem_exec_schedule:
        Subgroup smoketest-bsd:
                pass       -> INCOMPLETE (shard-apl) fdo#102848
        Subgroup smoketest-vebox:
                pass       -> INCOMPLETE (shard-apl)
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#102848 https://bugs.freedesktop.org/show_bug.cgi?id=102848
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-apl        total:1040 pass:648  dwarn:2   dfail:0   fail:11  skip:377 time:5125s
shard-hsw        total:2818 pass:1724 dwarn:2   dfail:0   fail:9   skip:1081 time:11559s
shard-snb        total:2838 pass:1331 dwarn:1   dfail:0   fail:10  skip:1496 time:6682s

== Logs ==

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