[PATCH v7 0/2] Reorganized HuC auth and GuC v9+ Logging change.

[PATCH v7 0/2] Reorganized HuC auth and GuC v9+ Logging change.

[Sagar Arun Kamble]

These two patches are from series https://patchwork.freedesktop.org/series/30715/.
Other 5 patches from that series are related to GuC fixes w.r.t suspend/resume and
reset and are dependent on GEM functions which are to reorganized further.
Patches in this series are reviewed by Michal Wajdeczko and ready to merge.
Will float other 5 patches through different series.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Sagar Arun Kamble (2):
  drm/i915: Reorganize HuC authentication
  drm/i915/guc: Enable default/critical logging in GuC by default from
    GuC v9

 drivers/gpu/drm/i915/intel_guc_fwif.h |  4 ++--
 drivers/gpu/drm/i915/intel_guc_log.c  | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_huc.c      | 22 +++++++---------------
 drivers/gpu/drm/i915/intel_uc.c       | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h       |  3 ++-
 5 files changed, 42 insertions(+), 20 deletions(-)

-- 
1.9.1

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

[PATCH v7 1/2] drm/i915: Reorganize HuC authentication

[Sagar Arun Kamble]

Prepared intel_auth_huc to separate HuC specific functionality
from GuC send action. Created new header intel_huc.h to group
HuC specific declarations.

v2: Changed argument preparation for AUTHENTICATE_HUC.
s/intel_auth_huc/intel_huc_auth. Deferred creation of intel_huc.h
to later patch.

v3: Rebase as intel_guc.h is removed. Added param description to
intel_huc_auth. (Michal)

v4: Rebase as intel_guc.h is added again. :)

v5: Rebase w.r.t removal of GuC code restructuring.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 22 +++++++---------------
 drivers/gpu/drm/i915/intel_uc.c  | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h  |  3 ++-
 3 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6145fa0..d3da4d3 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -225,19 +225,16 @@ void intel_huc_init_hw(struct intel_huc *huc)
 }
 
 /**
- * intel_guc_auth_huc() - authenticate ucode
- * @dev_priv: the drm_i915_device
- *
- * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
- * authenticate_huc interface.
+ * intel_huc_auth() - authenticate ucode
+ * @huc: intel_huc structure
  */
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
+void intel_huc_auth(struct intel_huc *huc)
 {
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_huc *huc = &dev_priv->huc;
 	struct i915_vma *vma;
+	u32 offset;
 	int ret;
-	u32 data[2];
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return;
@@ -250,11 +247,8 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	/* Specify auth action and where public signature is. */
-	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
-	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
-
-	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
+	offset = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
+	ret = intel_guc_auth_huc(guc, offset);
 	if (ret) {
 		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
 		goto out;
@@ -266,7 +260,6 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 				HUC_FW_VERIFIED,
 				HUC_FW_VERIFIED,
 				50);
-
 	if (ret) {
 		DRM_ERROR("HuC: Authentication failed %d\n", ret);
 		goto out;
@@ -275,4 +268,3 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 out:
 	i915_vma_unpin(vma);
 }
-
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..88c9471 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -328,6 +328,24 @@ static void guc_disable_communication(struct intel_guc *guc)
 	guc->send = intel_guc_send_nop;
 }
 
+/**
+ * intel_guc_auth_huc() - authenticate ucode
+ * @guc: struct intel_guc*
+ * @offset: rsa offset w.r.t ggtt base of huc vma
+ *
+ * triggers a huc fw authentication request to the guc via intel_guc_send
+ * authenticate_huc interface.
+ */
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
+{
+	u32 action[] = {
+		INTEL_GUC_ACTION_AUTHENTICATE_HUC,
+		rsa_offset
+	};
+
+	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -390,7 +408,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
-	intel_guc_auth_huc(dev_priv);
+	intel_huc_auth(&dev_priv->huc);
 	if (i915.enable_guc_submission) {
 		if (i915.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7703c9a..6966349 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -211,6 +211,7 @@ struct intel_huc {
 int intel_guc_sample_forcewake(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);
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 
 static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 {
@@ -254,6 +255,6 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 /* intel_huc.c */
 void intel_huc_select_fw(struct intel_huc *huc);
 void intel_huc_init_hw(struct intel_huc *huc);
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
+void intel_huc_auth(struct intel_huc *huc);
 
 #endif
-- 
1.9.1

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

Re: [PATCH v7 1/2] drm/i915: Reorganize HuC authentication

[Joonas Lahtinen]

On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
> Prepared intel_auth_huc to separate HuC specific functionality
> from GuC send action. Created new header intel_huc.h to group
> HuC specific declarations.
> 
> v2: Changed argument preparation for AUTHENTICATE_HUC.
> s/intel_auth_huc/intel_huc_auth. Deferred creation of intel_huc.h
> to later patch.
> 
> v3: Rebase as intel_guc.h is removed. Added param description to
> intel_huc_auth. (Michal)
> 
> v4: Rebase as intel_guc.h is added again. :)
> 
> v5: Rebase w.r.t removal of GuC code restructuring.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

<SNIP>

> @@ -225,19 +225,16 @@ void intel_huc_init_hw(struct intel_huc *huc)
>  }
>  
>  /**
> - * intel_guc_auth_huc() - authenticate ucode
> - * @dev_priv: the drm_i915_device
> - *
> - * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
> - * authenticate_huc interface.

This is still an exported function from this file, so should have
kerneldoc comment. Especially now that we have two very similar
exported functions, it would be beneficial to crosslink them in the
resulting documentation that'll look like:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#c.intel_guc_init_hw

So proper sentences and capitalisation is much preferred.

I'm also noticing that we're not properly linking all portions of the
code to be included in docs, I'll send a fix for that.

> + * intel_huc_auth() - authenticate ucode
> + * @huc: intel_huc structure
>   */
> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
> +void intel_huc_auth(struct intel_huc *huc)
>  {
> +	struct drm_i915_private *dev_priv = huc_to_i915(huc);
>  	struct intel_guc *guc = &dev_priv->guc;

As there's only one more of instance of 'dev_priv' in this func so just
rename it to 'i915' while touching the func. GuC is really close to the
GEM so similar style is good.

> @@ -250,11 +247,8 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
>  		return;
>  	}
>  
> -	/* Specify auth action and where public signature is. */
> -	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
> -	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
> -
> -	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
> +	offset = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
> +	ret = intel_guc_auth_huc(guc, offset);

With the code cleanup, the offset variable seems unnecessary to me,
just do:

	ret = intel_guc_auth_huc(guc,
				 guc_ggtt_offset(vma) + huc->fw.rsa_offset);

Other than that, LGTM.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 1/1] drm/i915/huc: Reorganize HuC authentication

[Sagar Arun Kamble]

Prepared intel_auth_huc to separate HuC specific functionality
from GuC send action. Created new header intel_huc.h to group
HuC specific declarations.

v2: Changed argument preparation for AUTHENTICATE_HUC.
s/intel_auth_huc/intel_huc_auth. Deferred creation of intel_huc.h
to later patch.

v3: Rebase as intel_guc.h is removed. Added param description to
intel_huc_auth. (Michal)

v4: Rebase as intel_guc.h is added again. :)

v5: Rebase w.r.t removal of GuC code restructuring.

v6-v7: Rebase.

v8: Tagged subject as drm/i915/huc. (Michal Wajdeczko)
Added kernel-doc description to intel_huc_auth and intel_guc_auth_huc.
s/dev_priv/i915 and removed unnecessary variable offset. (Joonas)

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 38 ++++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_uc.c  | 23 ++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h  |  3 ++-
 3 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6145fa0..821aca4 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -225,19 +225,22 @@ void intel_huc_init_hw(struct intel_huc *huc)
 }
 
 /**
- * intel_guc_auth_huc() - authenticate ucode
- * @dev_priv: the drm_i915_device
+ * intel_huc_auth() - Authenticate HuC uCode
+ * @huc: intel_huc structure
+ *
+ * Called after HuC and GuC firmware loading during intel_uc_init_hw().
  *
- * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
- * authenticate_huc interface.
+ * This function pins HuC firmware image object into GGTT.
+ * Then it invokes GuC action to authenticate passing the offset to RSA
+ * signature through intel_guc_auth_huc(). It then waits for 50ms for
+ * firmware verification ACK and unpins the object.
  */
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
+void intel_huc_auth(struct intel_huc *huc)
 {
-	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_huc *huc = &dev_priv->huc;
+	struct drm_i915_private *i915 = huc_to_i915(huc);
+	struct intel_guc *guc = &i915->guc;
 	struct i915_vma *vma;
 	int ret;
-	u32 data[2];
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return;
@@ -250,23 +253,19 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	/* Specify auth action and where public signature is. */
-	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
-	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
-
-	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
+	ret = intel_guc_auth_huc(guc,
+				 guc_ggtt_offset(vma) + huc->fw.rsa_offset);
 	if (ret) {
 		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
 		goto out;
 	}
 
 	/* Check authentication status, it should be done by now */
-	ret = intel_wait_for_register(dev_priv,
-				HUC_STATUS2,
-				HUC_FW_VERIFIED,
-				HUC_FW_VERIFIED,
-				50);
-
+	ret = intel_wait_for_register(i915,
+				      HUC_STATUS2,
+				      HUC_FW_VERIFIED,
+				      HUC_FW_VERIFIED,
+				      50);
 	if (ret) {
 		DRM_ERROR("HuC: Authentication failed %d\n", ret);
 		goto out;
@@ -275,4 +274,3 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 out:
 	i915_vma_unpin(vma);
 }
-
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..5752c0f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -328,6 +328,27 @@ static void guc_disable_communication(struct intel_guc *guc)
 	guc->send = intel_guc_send_nop;
 }
 
+/**
+ * intel_guc_auth_huc() - Send action to GuC to authenticate HuC ucode
+ * @guc: intel_guc structure
+ * @rsa_offset: rsa offset w.r.t ggtt base of huc vma
+ *
+ * Triggers a HuC firmware authentication request to the GuC via intel_guc_send
+ * INTEL_GUC_ACTION_AUTHENTICATE_HUC interface. This function is invoked by
+ * intel_huc_auth().
+ *
+ * Return:	non-zero code on error
+ */
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
+{
+	u32 action[] = {
+		INTEL_GUC_ACTION_AUTHENTICATE_HUC,
+		rsa_offset
+	};
+
+	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -390,7 +411,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
-	intel_guc_auth_huc(dev_priv);
+	intel_huc_auth(&dev_priv->huc);
 	if (i915.enable_guc_submission) {
 		if (i915.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7703c9a..6966349 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -211,6 +211,7 @@ struct intel_huc {
 int intel_guc_sample_forcewake(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);
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 
 static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 {
@@ -254,6 +255,6 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 /* intel_huc.c */
 void intel_huc_select_fw(struct intel_huc *huc);
 void intel_huc_init_hw(struct intel_huc *huc);
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
+void intel_huc_auth(struct intel_huc *huc);
 
 #endif
-- 
1.9.1

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

[PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Sagar Arun Kamble]

With GuC v9, new type of Default/critical logging in GuC to enable
capturing minimal important logs in production systems efficiently.
This patch enables this logging in GuC by default always. It should
be noted that streaming support with half-full interrupt mechanism
that is present for normal logging is not present for this type of
logging.

v2: Emulated GuC critical logging through i915.guc_log_level.

v3: Commit message update. Enable default/critical logging in GuC always.
Fixed RPM wake during guc_log_unregister in the unload path.

v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED
and updated name of new bit to be version agnostic. Updated parameter to
struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING.
Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from
params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko)

v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to
GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko)

v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message
update. (Michal Wajdeczko)

Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
Cc: Fry Gregory P <gregory.p.fry@intel.com>
Cc: Spotswood John A <john.a.spotswood@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h |  4 ++--
 drivers/gpu/drm/i915/intel_guc_log.c  | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 7eb6b4f..fed875a 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -127,7 +127,6 @@
 #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_ADS_ADDR_SHIFT		11
 #define   GUC_ADS_ADDR_MASK		0xfffff800
 
@@ -539,7 +538,8 @@ struct guc_log_buffer_state {
 		u32 logging_enabled:1;
 		u32 reserved1:3;
 		u32 verbosity:4;
-		u32 reserved2:24;
+		u32 critical_logging_enabled: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 16d3b87..677ec3d 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
+/*
+ * Critical logging in GuC is to be enabled always from GuC v9+.
+ * (for KBL - v9.39+)
+ */
+#define GUC_NEEDS_CRITICAL_LOGGING(guc)	\
+	(HAS_GUC(guc_to_i915(guc)) && \
+	 (guc->fw.major_ver_found >= 9) && \
+	 (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0)))
+
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 {
 	struct intel_guc *guc = &dev_priv->guc;
-
 	union guc_log_control log_param;
 	int ret;
 
@@ -603,6 +611,9 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
 		return 0;
 
+	if (GUC_NEEDS_CRITICAL_LOGGING(guc))
+		log_param.critical_logging_enabled = 1;
+
 	ret = guc_log_control(guc, log_param.value);
 	if (ret < 0) {
 		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
-- 
1.9.1

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

Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Michal Wajdeczko]

On Fri, 22 Sep 2017 12:07:47 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> With GuC v9, new type of Default/critical logging in GuC to enable
> capturing minimal important logs in production systems efficiently.
> This patch enables this logging in GuC by default always. It should
> be noted that streaming support with half-full interrupt mechanism
> that is present for normal logging is not present for this type of
> logging.
>
> v2: Emulated GuC critical logging through i915.guc_log_level.
>
> v3: Commit message update. Enable default/critical logging in GuC always.
> Fixed RPM wake during guc_log_unregister in the unload path.
>
> v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED
> and updated name of new bit to be version agnostic. Updated parameter to
> struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING.
> Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from
> params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko)
>
> v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to
> GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko)
>
> v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message
> update. (Michal Wajdeczko)
>
> Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
> Cc: Fry Gregory P <gregory.p.fry@intel.com>
> Cc: Spotswood John A <john.a.spotswood@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fwif.h |  4 ++--
>  drivers/gpu/drm/i915/intel_guc_log.c  | 13 ++++++++++++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
> b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 7eb6b4f..fed875a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -127,7 +127,6 @@
>  #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_ADS_ADDR_SHIFT		11
>  #define   GUC_ADS_ADDR_MASK		0xfffff800
> @@ -539,7 +538,8 @@ struct guc_log_buffer_state {
>  		u32 logging_enabled:1;
>  		u32 reserved1:3;
>  		u32 verbosity:4;
> -		u32 reserved2:24;
> +		u32 critical_logging_enabled: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 16d3b87..677ec3d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>  	i915_vma_unpin_and_release(&guc->log.vma);
>  }
> +/*
> + * Critical logging in GuC is to be enabled always from GuC v9+.
> + * (for KBL - v9.39+)
> + */
> +#define GUC_NEEDS_CRITICAL_LOGGING(guc)	\
> +	(HAS_GUC(guc_to_i915(guc)) && \

Nitpick: As this macro is now used locally in function that is
called only for platforms with HAS_GUC() then maybe we can drop
that check completely as it is redundant and only check version?

Michal

> +	 (guc->fw.major_ver_found >= 9) && \
> +	 (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0)))
> +
>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64  
> control_val)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> -
>  	union guc_log_control log_param;
>  	int ret;
> @@ -603,6 +611,9 @@ int i915_guc_log_control(struct drm_i915_private  
> *dev_priv, u64 control_val)
>  	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>  		return 0;
> +	if (GUC_NEEDS_CRITICAL_LOGGING(guc))
> +		log_param.critical_logging_enabled = 1;
> +
>  	ret = guc_log_control(guc, log_param.value);
>  	if (ret < 0) {
>  		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Sagar Arun Kamble]

On 9/22/2017 3:54 PM, Michal Wajdeczko wrote:
> On Fri, 22 Sep 2017 12:07:47 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> With GuC v9, new type of Default/critical logging in GuC to enable
>> capturing minimal important logs in production systems efficiently.
>> This patch enables this logging in GuC by default always. It should
>> be noted that streaming support with half-full interrupt mechanism
>> that is present for normal logging is not present for this type of
>> logging.
>>
>> v2: Emulated GuC critical logging through i915.guc_log_level.
>>
>> v3: Commit message update. Enable default/critical logging in GuC 
>> always.
>> Fixed RPM wake during guc_log_unregister in the unload path.
>>
>> v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED
>> and updated name of new bit to be version agnostic. Updated parameter to
>> struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING.
>> Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from
>> params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko)
>>
>> v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to
>> GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko)
>>
>> v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message
>> update. (Michal Wajdeczko)
>>
>> Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
>> Cc: Fry Gregory P <gregory.p.fry@intel.com>
>> Cc: Spotswood John A <john.a.spotswood@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_fwif.h |  4 ++--
>>  drivers/gpu/drm/i915/intel_guc_log.c  | 13 ++++++++++++-
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h 
>> b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> index 7eb6b4f..fed875a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> @@ -127,7 +127,6 @@
>>  #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_ADS_ADDR_SHIFT        11
>>  #define   GUC_ADS_ADDR_MASK        0xfffff800
>> @@ -539,7 +538,8 @@ struct guc_log_buffer_state {
>>          u32 logging_enabled:1;
>>          u32 reserved1:3;
>>          u32 verbosity:4;
>> -        u32 reserved2:24;
>> +        u32 critical_logging_enabled: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 16d3b87..677ec3d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>>      i915_vma_unpin_and_release(&guc->log.vma);
>>  }
>> +/*
>> + * Critical logging in GuC is to be enabled always from GuC v9+.
>> + * (for KBL - v9.39+)
>> + */
>> +#define GUC_NEEDS_CRITICAL_LOGGING(guc)    \
>> +    (HAS_GUC(guc_to_i915(guc)) && \
>
> Nitpick: As this macro is now used locally in function that is
> called only for platforms with HAS_GUC() then maybe we can drop
> that check completely as it is redundant and only check version?
>
> Michal
Yes. Will update.
>
>> +     (guc->fw.major_ver_found >= 9) && \
>> +     (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 
>> : 0)))
>> +
>>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 
>> control_val)
>>  {
>>      struct intel_guc *guc = &dev_priv->guc;
>> -
>>      union guc_log_control log_param;
>>      int ret;
>> @@ -603,6 +611,9 @@ int i915_guc_log_control(struct drm_i915_private 
>> *dev_priv, u64 control_val)
>>      if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>>          return 0;
>> +    if (GUC_NEEDS_CRITICAL_LOGGING(guc))
>> +        log_param.critical_logging_enabled = 1;
>> +
>>      ret = guc_log_control(guc, log_param.value);
>>      if (ret < 0) {
>>          DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", 
>> ret);

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

Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Joonas Lahtinen]

On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
> With GuC v9, new type of Default/critical logging in GuC to enable
> capturing minimal important logs in production systems efficiently.
> This patch enables this logging in GuC by default always. It should
> be noted that streaming support with half-full interrupt mechanism
> that is present for normal logging is not present for this type of
> logging.

The commit message would be a good place to debrief the user impact. Do
we have the tools to capture the new style of log?

And question is why do we enable any logging by default, let it have
much or little impact on performance? This flag should probably be set
through debugfs or some other means when we know we're going to want
debugging output.

> v2: Emulated GuC critical logging through i915.guc_log_level.
> 
> v3: Commit message update. Enable default/critical logging in GuC always.
> Fixed RPM wake during guc_log_unregister in the unload path.
> 
> v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED
> and updated name of new bit to be version agnostic. Updated parameter to
> struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING.
> Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from
> params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko)
> 
> v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to
> GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko)
> 
> v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message
> update. (Michal Wajdeczko)
> 
> Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
> Cc: Fry Gregory P <gregory.p.fry@intel.com>
> Cc: Spotswood John A <john.a.spotswood@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

<SNIP>

> @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>  	i915_vma_unpin_and_release(&guc->log.vma);
>  }
>  
> +/*
> + * Critical logging in GuC is to be enabled always from GuC v9+.
> + * (for KBL - v9.39+)
> + */
> +#define GUC_NEEDS_CRITICAL_LOGGING(guc)	\
> +	(HAS_GUC(guc_to_i915(guc)) && \
> +	 (guc->fw.major_ver_found >= 9) && \
> +	 (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0)))

I'd like to avoid this kind of conditionals as much as possible, to the
extent that we must consider just not loading GuC at all if we're not
finding an at least the firmware version we specify our code needs.
That's what the distros are going to package, so we don't have to be
backwards compatible.

So this would be a question of IS_PLATFORM()s like elsewhere in the
code once the increased firmware version is specified in the kernel
code.

> @@ -603,6 +611,9 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>  	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>  		return 0;
>  
> +	if (GUC_NEEDS_CRITICAL_LOGGING(guc))
> +		log_param.critical_logging_enabled = 1;
> +

This would then become an "if (INTEL_GEN() >= XYZ)". And it would be
highly preferrable to get a backport to all older versions, too. They
could even ignore the value if it has no visible effect.

I have to admit I think this would be a variable that we read and maybe
print a debug if it's up, if the firmware knows it 'needs this flag up
always'.

Anyway, we just got to organize that CI has both firmware versions
(current and desired) in-place, then patches like this just have the
'increase required GuC version number' patch as as a gating in in front
of this.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Sagar Arun Kamble]

On 9/22/2017 5:34 PM, Joonas Lahtinen wrote:
> On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
>> With GuC v9, new type of Default/critical logging in GuC to enable
>> capturing minimal important logs in production systems efficiently.
>> This patch enables this logging in GuC by default always. It should
>> be noted that streaming support with half-full interrupt mechanism
>> that is present for normal logging is not present for this type of
>> logging.
> The commit message would be a good place to debrief the user impact. Do
> we have the tools to capture the new style of log?
This has been verified to have minor impact by GuC team. Greg, Harsh can 
clarify further.
Goal was to allow to get GuC logs from production systems.
These are the subset of same GuC logs available currently at all GuC log 
levels and are needed to be captured
always whenever we want to capture GuC logs at i915.guc_log_level >=0 
and <=3.
Chris had suggested on why this is not made on of the log levels and on 
discussion with Harsh it is
concluded that this will not be supported as log level as GuC is using 
fast functions to log the new
critical logs. Also there will not be half-full streaming support for 
these type of logs.
I did have a patch earlier to disable this by default or emulate it as 
log level to the end user but recommendation
from GuC team is to always turn this ON.
> And question is why do we enable any logging by default, let it have
> much or little impact on performance? This flag should probably be set
> through debugfs or some other means when we know we're going to want
> debugging output.
In my earlier patch we did have ability to turn this ON/OFF 
https://patchwork.freedesktop.org/patch/173884/

>
>> v2: Emulated GuC critical logging through i915.guc_log_level.
>>
>> v3: Commit message update. Enable default/critical logging in GuC always.
>> Fixed RPM wake during guc_log_unregister in the unload path.
>>
>> v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED
>> and updated name of new bit to be version agnostic. Updated parameter to
>> struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING.
>> Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from
>> params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko)
>>
>> v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to
>> GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko)
>>
>> v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message
>> update. (Michal Wajdeczko)
>>
>> Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
>> Cc: Fry Gregory P <gregory.p.fry@intel.com>
>> Cc: Spotswood John A <john.a.spotswood@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> <SNIP>
>
>> @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>>   	i915_vma_unpin_and_release(&guc->log.vma);
>>   }
>>   
>> +/*
>> + * Critical logging in GuC is to be enabled always from GuC v9+.
>> + * (for KBL - v9.39+)
>> + */
>> +#define GUC_NEEDS_CRITICAL_LOGGING(guc)	\
>> +	(HAS_GUC(guc_to_i915(guc)) && \
>> +	 (guc->fw.major_ver_found >= 9) && \
>> +	 (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0)))
> I'd like to avoid this kind of conditionals as much as possible, to the
> extent that we must consider just not loading GuC at all if we're not
> finding an at least the firmware version we specify our code needs.
> That's what the distros are going to package, so we don't have to be
> backwards compatible.
>
> So this would be a question of IS_PLATFORM()s like elsewhere in the
> code once the increased firmware version is specified in the kernel
> code.
Agree that if we make major-wanted = 9 we can avoid this check all together.
Currently GuC firmware versions in 01.org are pre version 9 and I have 
verified that this change is
not backward compatible w.r.t v9.  Verified that logging does not work 
for pre-v9 GuC firmwares if we set
log_param.critical_logging_enabled unconditionally.


>
>> @@ -603,6 +611,9 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>>   	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>>   		return 0;
>>   
>> +	if (GUC_NEEDS_CRITICAL_LOGGING(guc))
>> +		log_param.critical_logging_enabled = 1;
>> +
> This would then become an "if (INTEL_GEN() >= XYZ)". And it would be
> highly preferrable to get a backport to all older versions, too. They
> could even ignore the value if it has no visible effect.
>
> I have to admit I think this would be a variable that we read and maybe
> print a debug if it's up, if the firmware knows it 'needs this flag up
> always'.
>
> Anyway, we just got to organize that CI has both firmware versions
> (current and desired) in-place, then patches like this just have the
> 'increase required GuC version number' patch as as a gating in in front
> of this.
>
> Regards, Joonas

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

Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Joonas Lahtinen]

On Fri, 2017-09-22 at 20:16 +0530, Sagar Arun Kamble wrote:
> 
> On 9/22/2017 5:34 PM, Joonas Lahtinen wrote:
> > On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
> > > With GuC v9, new type of Default/critical logging in GuC to enable
> > > capturing minimal important logs in production systems efficiently.
> > > This patch enables this logging in GuC by default always. It should
> > > be noted that streaming support with half-full interrupt mechanism
> > > that is present for normal logging is not present for this type of
> > > logging.
> > 
> > The commit message would be a good place to debrief the user impact. Do
> > we have the tools to capture the new style of log?
> 
> This has been verified to have minor impact by GuC team. Greg, Harsh can 
> clarify further.
> Goal was to allow to get GuC logs from production systems.
> These are the subset of same GuC logs available currently at all GuC log 
> levels and are needed to be captured
> always whenever we want to capture GuC logs at i915.guc_log_level >=0 
> and <=3.
> Chris had suggested on why this is not made on of the log levels and on 
> discussion with Harsh it is
> concluded that this will not be supported as log level as GuC is using 
> fast functions to log the new
> critical logs. Also there will not be half-full streaming support for 
> these type of logs.
> I did have a patch earlier to disable this by default or emulate it as 
> log level to the end user but recommendation
> from GuC team is to always turn this ON.

I see little point in detecting firmware version and unconditionally
setting a flag up at boot from kernel driver, when the firmware can do
it themselves knowing their own version.

We should not be polluting driver codebase with such constructs. It's
firmware, a new version can be provided easily unlike with hardware
which might need W/As.

> > And question is why do we enable any logging by default, let it have
> > much or little impact on performance? This flag should probably be set
> > through debugfs or some other means when we know we're going to want
> > debugging output.
> 
> In my earlier patch we did have ability to turn this ON/OFF 
> https://patchwork.freedesktop.org/patch/173884/

Yeah, I think we might want to extend the existing .enable_guc_logging
parameter once we understand the relation between critical and regular
logging in a released firmware.

Could be as simple as;

(...).enable_critical_logging = (i915_modparams.guc_log_level >= 0);

For the time being, we shall wait for a fixed firmware.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Sagar Arun Kamble]

On 9/25/2017 3:31 PM, Joonas Lahtinen wrote:
> On Fri, 2017-09-22 at 20:16 +0530, Sagar Arun Kamble wrote:
>> On 9/22/2017 5:34 PM, Joonas Lahtinen wrote:
>>> On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
>>>> With GuC v9, new type of Default/critical logging in GuC to enable
>>>> capturing minimal important logs in production systems efficiently.
>>>> This patch enables this logging in GuC by default always. It should
>>>> be noted that streaming support with half-full interrupt mechanism
>>>> that is present for normal logging is not present for this type of
>>>> logging.
>>> The commit message would be a good place to debrief the user impact. Do
>>> we have the tools to capture the new style of log?
>> This has been verified to have minor impact by GuC team. Greg, Harsh can
>> clarify further.
>> Goal was to allow to get GuC logs from production systems.
>> These are the subset of same GuC logs available currently at all GuC log
>> levels and are needed to be captured
>> always whenever we want to capture GuC logs at i915.guc_log_level >=0
>> and <=3.
>> Chris had suggested on why this is not made on of the log levels and on
>> discussion with Harsh it is
>> concluded that this will not be supported as log level as GuC is using
>> fast functions to log the new
>> critical logs. Also there will not be half-full streaming support for
>> these type of logs.
>> I did have a patch earlier to disable this by default or emulate it as
>> log level to the end user but recommendation
>> from GuC team is to always turn this ON.
> I see little point in detecting firmware version and unconditionally
> setting a flag up at boot from kernel driver, when the firmware can do
> it themselves knowing their own version.
>
> We should not be polluting driver codebase with such constructs. It's
> firmware, a new version can be provided easily unlike with hardware
> which might need W/As.
Agree.
>
>>> And question is why do we enable any logging by default, let it have
>>> much or little impact on performance? This flag should probably be set
>>> through debugfs or some other means when we know we're going to want
>>> debugging output.
>> In my earlier patch we did have ability to turn this ON/OFF
>> https://patchwork.freedesktop.org/patch/173884/
> Yeah, I think we might want to extend the existing .enable_guc_logging
> parameter once we understand the relation between critical and regular
> logging in a released firmware.
>
> Could be as simple as;
>
> (...).enable_critical_logging = (i915_modparams.guc_log_level >= 0);
>
> For the time being, we shall wait for a fixed firmware.
>
> Regards, Joonas
Sure. Will request for this change to be made in the firmware.

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

✗ Fi.CI.BAT: warning for Reorganized HuC auth and GuC v9+ Logging change.

[Patchwork]

== Series Details ==

Series: Reorganized HuC auth and GuC v9+ Logging change.
URL   : https://patchwork.freedesktop.org/series/30748/
State : warning

== Summary ==

Series 30748v1 Reorganized HuC auth and GuC v9+ Logging change.
https://patchwork.freedesktop.org/api/1.0/series/30748/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup readonly-bsd:
                pass       -> DMESG-WARN (fi-kbl-7500u)
Test gem_exec_nop:
        Subgroup basic-series:
                pass       -> DMESG-WARN (fi-kbl-7500u)
Test gem_exec_reloc:
        Subgroup basic-cpu-noreloc:
                pass       -> DMESG-WARN (fi-kbl-7500u)
        Subgroup basic-cpu-gtt-noreloc:
                pass       -> DMESG-WARN (fi-kbl-7500u)
        Subgroup basic-gtt-cpu-noreloc:
                pass       -> DMESG-WARN (fi-kbl-7500u)
        Subgroup basic-gtt-active:
                pass       -> DMESG-WARN (fi-kbl-7500u)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (fi-kbl-7500u) fdo#102850
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-cfl-s) fdo#102294
Test drv_module_reload:
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-glk-1) fdo#102777

fdo#102850 https://bugs.freedesktop.org/show_bug.cgi?id=102850
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:438s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:416s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:515s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:489s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:491s
fi-cfl-s         total:289  pass:222  dwarn:35  dfail:0   fail:0   skip:32  time:538s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:420s
fi-glk-1         total:289  pass:259  dwarn:1   dfail:0   fail:0   skip:29  time:564s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:431s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:408s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:431s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:490s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:458s
fi-kbl-7500u     total:118  pass:94   dwarn:7   dfail:0   fail:0   skip:16 
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:575s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:591s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:533s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:747s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:488s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:476s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:564s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:416s

b32248fc0bd87b45bc068f234f061324a29fa809 drm-tip: 2017y-09m-21d-22h-36m-39s UTC integration manifest
56bfaaabec73 drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9
4b3276fd05fa drm/i915: Reorganize HuC authentication

== Logs ==

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