All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam
@ 2018-05-30 13:53 Piotr Piorkowski
  2018-05-30 13:53 ` [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter Piotr Piorkowski
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

From: Piotr Piórkowski <piotr.piorkowski@intel.com>

Currently we are using modparam as placeholder for GuC log level.
Stop doing this and keep runtime GuC level in intel_guc_log struct.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c     |  8 +++-----
 drivers/gpu/drm/i915/intel_guc_log.c | 18 +++++-------------
 drivers/gpu/drm/i915/intel_guc_log.h |  9 ++++++++-
 drivers/gpu/drm/i915/intel_uc.c      |  2 +-
 4 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 116f4ccf1bbd..9025837850ad 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -203,13 +203,11 @@ void intel_guc_fini(struct intel_guc *guc)
 	guc_shared_data_destroy(guc);
 }
 
-static u32 get_log_control_flags(void)
+static u32 guc_ctl_debug_flags(struct intel_guc *guc)
 {
-	u32 level = i915_modparams.guc_log_level;
+	u32 level = intel_guc_log_level_get(&guc->log);
 	u32 flags = 0;
 
-	GEM_BUG_ON(level < 0);
-
 	if (!GUC_LOG_LEVEL_IS_ENABLED(level))
 		flags |= GUC_LOG_DEFAULT_DISABLED;
 
@@ -250,7 +248,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 
 	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
 
-	params[GUC_CTL_DEBUG] = get_log_control_flags();
+	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
 
 	/* 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_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 401e1704d61e..c036d0fac370 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -475,11 +475,12 @@ int intel_guc_log_create(struct intel_guc_log *log)
 	offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT;
 	log->flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 
+	log->level = i915_modparams.guc_log_level;
+
 	return 0;
 
 err:
-	/* logging will be off */
-	i915_modparams.guc_log_level = 0;
+	DRM_ERROR("Failed to allocate GuC log buffer. %d\n", ret);
 	return ret;
 }
 
@@ -488,14 +489,6 @@ void intel_guc_log_destroy(struct intel_guc_log *log)
 	i915_vma_unpin_and_release(&log->vma);
 }
 
-int intel_guc_log_level_get(struct intel_guc_log *log)
-{
-	GEM_BUG_ON(!log->vma);
-	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
-
-	return i915_modparams.guc_log_level;
-}
-
 int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 {
 	struct intel_guc *guc = log_to_guc(log);
@@ -504,7 +497,6 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 
 	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
 	GEM_BUG_ON(!log->vma);
-	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
 
 	/*
 	 * GuC is recognizing log levels starting from 0 to max, we're using 0
@@ -515,7 +507,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 
-	if (i915_modparams.guc_log_level == val) {
+	if (log->level == val) {
 		ret = 0;
 		goto out_unlock;
 	}
@@ -530,7 +522,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
 		goto out_unlock;
 	}
 
-	i915_modparams.guc_log_level = val;
+	log->level = val;
 
 out_unlock:
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index fa80535a6f9d..ea375c3b5d08 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -30,6 +30,7 @@
 #include <linux/workqueue.h>
 
 #include "intel_guc_fwif.h"
+#include "i915_gem.h"
 
 struct intel_guc;
 
@@ -58,6 +59,7 @@ struct intel_guc;
 #define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)
 
 struct intel_guc_log {
+	u32 level;
 	u32 flags;
 	struct i915_vma *vma;
 	struct {
@@ -80,7 +82,6 @@ void intel_guc_log_init_early(struct intel_guc_log *log);
 int intel_guc_log_create(struct intel_guc_log *log);
 void intel_guc_log_destroy(struct intel_guc_log *log);
 
-int intel_guc_log_level_get(struct intel_guc_log *log);
 int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val);
 bool intel_guc_log_relay_enabled(const struct intel_guc_log *log);
 int intel_guc_log_relay_open(struct intel_guc_log *log);
@@ -89,4 +90,10 @@ void intel_guc_log_relay_close(struct intel_guc_log *log);
 
 void intel_guc_log_handle_flush_event(struct intel_guc_log *log);
 
+static inline u32 intel_guc_log_level_get(struct intel_guc_log *log)
+{
+	GEM_BUG_ON(!log->vma);
+	return log->level;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 6a73e81f373b..5ce8d5df1b58 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -207,7 +207,7 @@ void intel_uc_init_mmio(struct drm_i915_private *i915)
 
 static void guc_capture_load_err_log(struct intel_guc *guc)
 {
-	if (!guc->log.vma || !i915_modparams.guc_log_level)
+	if (!guc->log.vma || !intel_guc_log_level_get(&guc->log))
 		return;
 
 	if (!guc->load_err_log)
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
@ 2018-05-30 13:53 ` Piotr Piorkowski
  2018-05-30 14:58   ` Michal Wajdeczko
  2018-05-30 13:53 ` [PATCH 3/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter Piotr Piorkowski
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

At the moment, the preparation of GUC_CTL_DEBUG is disordered.
Lets move all GUC_CTL_DEBUG related operations to one place.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9025837850ad..3cc99fcaaea6 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -205,6 +205,7 @@ void intel_guc_fini(struct intel_guc *guc)
 
 static u32 guc_ctl_debug_flags(struct intel_guc *guc)
 {
+	u32 ads = intel_guc_ggtt_offset(guc, guc->ads_vma) >> PAGE_SHIFT;
 	u32 level = intel_guc_log_level_get(&guc->log);
 	u32 flags = 0;
 
@@ -217,6 +218,11 @@ static u32 guc_ctl_debug_flags(struct intel_guc *guc)
 		flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) <<
 			 GUC_LOG_VERBOSITY_SHIFT;
 
+	if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
+		flags |= ads << GUC_ADS_ADDR_SHIFT;
+		flags |= GUC_ADS_ENABLED;
+	}
+
 	return flags;
 }
 
@@ -252,14 +258,9 @@ void intel_guc_init_params(struct intel_guc *guc)
 
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		u32 ads = intel_guc_ggtt_offset(guc,
-						guc->ads_vma) >> PAGE_SHIFT;
 		u32 pgs = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
 		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
 
-		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
-		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
-
 		pgs >>= PAGE_SHIFT;
 		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
 			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
  2018-05-30 13:53 ` [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter Piotr Piorkowski
@ 2018-05-30 13:53 ` Piotr Piorkowski
  2018-05-30 14:59   ` Michal Wajdeczko
  2018-05-30 13:53 ` [PATCH 4/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter Piotr Piorkowski
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

At the moment, the preparation of GUC_CTL_FEATURE is disordered.
Lets move all GUC_CTL_FEATURE related operations to one place.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 3cc99fcaaea6..9dd255a34e65 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -226,6 +226,19 @@ static u32 guc_ctl_debug_flags(struct intel_guc *guc)
 	return flags;
 }
 
+static u32 guc_ctl_feature_flags(struct intel_guc *guc)
+{
+	u32 flags = 0;
+
+	flags |=  GUC_CTL_VCS2_ENABLED;
+
+	if (USES_GUC_SUBMISSION(guc_to_i915(guc)))
+		flags |= GUC_CTL_KERNEL_SUBMISSIONS;
+	else
+		flags |= GUC_CTL_DISABLE_SCHEDULER;
+
+	return flags;
+}
 /*
  * Initialise the GuC parameter block before starting the firmware
  * transfer. These parameters are read by the firmware on startup
@@ -249,9 +262,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 
 	params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
 
-	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
-			GUC_CTL_VCS2_ENABLED;
-
+	params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
 	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
 
 	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
@@ -264,11 +275,6 @@ void intel_guc_init_params(struct intel_guc *guc)
 		pgs >>= PAGE_SHIFT;
 		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
 			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
-
-		params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
-
-		/* Unmask this bit to enable the GuC's internal scheduler */
-		params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
 	}
 
 	/*
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
  2018-05-30 13:53 ` [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter Piotr Piorkowski
  2018-05-30 13:53 ` [PATCH 3/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter Piotr Piorkowski
@ 2018-05-30 13:53 ` Piotr Piorkowski
  2018-05-30 15:01   ` Michal Wajdeczko
  2018-05-30 13:53 ` [PATCH 5/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter Piotr Piorkowski
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

At the moment, the preparation of GUC_CTL_LOG_PARAMS is disordered.
Additionally, in struct intel_guc_log we have an unnecessary field
'flags' which we use only to assign value to GuC parameter.
Lets move all GUC_CTL_LOG_PARAMS related operations to one place,
and lets remove field 'flags' from struct intel_guc_log.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c     | 18 ++++++++++++++++--
 drivers/gpu/drm/i915/intel_guc_log.c | 11 -----------
 drivers/gpu/drm/i915/intel_guc_log.h |  1 -
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9dd255a34e65..185459ee49c9 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -239,6 +239,21 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
 
 	return flags;
 }
+static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
+{
+	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
+	u32 flags;
+
+	/* each allocated unit is a page */
+	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT) |
+		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
+		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
+		(offset << GUC_LOG_BUF_ADDR_SHIFT);
+
+	return flags;
+}
+
 /*
  * Initialise the GuC parameter block before starting the firmware
  * transfer. These parameters are read by the firmware on startup
@@ -263,8 +278,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 	params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
 
 	params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
-	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
-
+	params[GUC_CTL_LOG_PARAMS]  = guc_ctl_log_params_flags(guc);
 	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
 
 	/* If GuC submission is enabled, set up additional parameters here */
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index c036d0fac370..a751025b6ad7 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -452,8 +452,6 @@ int intel_guc_log_create(struct intel_guc_log *log)
 {
 	struct intel_guc *guc = log_to_guc(log);
 	struct i915_vma *vma;
-	unsigned long offset;
-	u32 flags;
 	int ret;
 
 	GEM_BUG_ON(log->vma);
@@ -466,15 +464,6 @@ int intel_guc_log_create(struct intel_guc_log *log)
 
 	log->vma = 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) |
-		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
-		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
-
-	offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT;
-	log->flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
-
 	log->level = i915_modparams.guc_log_level;
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index ea375c3b5d08..995aeb29a3b9 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -60,7 +60,6 @@ struct intel_guc;
 
 struct intel_guc_log {
 	u32 level;
-	u32 flags;
 	struct i915_vma *vma;
 	struct {
 		void *buf_addr;
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
                   ` (2 preceding siblings ...)
  2018-05-30 13:53 ` [PATCH 4/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter Piotr Piorkowski
@ 2018-05-30 13:53 ` Piotr Piorkowski
  2018-05-30 15:06   ` Michal Wajdeczko
  2018-05-30 13:53 ` [PATCH 6/7] drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h Piotr Piorkowski
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

At the moment, the preparation of GUC_CTL_CTXINFO is disordered.
Lets move all  GUC_CTL_CTXINFO related operations to one place.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 185459ee49c9..3b45f06b1aa2 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -239,6 +239,25 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
 
 	return flags;
 }
+
+static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
+{
+	u32 flags = 0;
+	u32 ctxnum;
+	u32 base;
+
+	if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
+
+		base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
+		ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;
+
+		base >>= PAGE_SHIFT;
+		flags = (base << GUC_CTL_BASE_ADDR_SHIFT) |
+			(ctxnum << GUC_CTL_CTXNUM_IN16_SHIFT);
+	}
+	return flags;
+}
+
 static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 {
 	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
@@ -280,16 +299,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 	params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
 	params[GUC_CTL_LOG_PARAMS]  = guc_ctl_log_params_flags(guc);
 	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
-
-	/* If GuC submission is enabled, set up additional parameters here */
-	if (USES_GUC_SUBMISSION(dev_priv)) {
-		u32 pgs = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
-		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
-
-		pgs >>= PAGE_SHIFT;
-		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
-			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
-	}
+	params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
 
 	/*
 	 * All SOFT_SCRATCH registers are in FORCEWAKE_BLITTER domain and
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/7] drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
                   ` (3 preceding siblings ...)
  2018-05-30 13:53 ` [PATCH 5/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter Piotr Piorkowski
@ 2018-05-30 13:53 ` Piotr Piorkowski
  2018-05-30 16:18   ` Michal Wajdeczko
  2018-05-30 13:53 ` [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes Piotr Piorkowski
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

At this moment, we have defined GuC logs sizes in intel_guc_fwif.h, but
as these values are related directly to the GuC logs, and not to API of
GuC parameters, we should move these defines to intel_guc_log.h.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c      | 28 +++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_guc_fwif.h | 20 +++-----------------
 drivers/gpu/drm/i915/intel_guc_log.c  | 28 +++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_guc_log.h  |  9 +++------
 4 files changed, 54 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 3b45f06b1aa2..e15047fedb45 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -263,13 +263,31 @@ static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
 	u32 flags;
 
-	/* each allocated unit is a page */
-	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
-		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT) |
-		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
-		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
+	#define UNIT (4 << 10)
+
+	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
+	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
+	BUILD_BUG_ON(!DPC_BUFFER_SIZE);
+	BUILD_BUG_ON(!IS_ALIGNED(DPC_BUFFER_SIZE, UNIT));
+	BUILD_BUG_ON(!ISR_BUFFER_SIZE);
+	BUILD_BUG_ON(!IS_ALIGNED(ISR_BUFFER_SIZE, UNIT));
+
+	BUILD_BUG_ON((CRASH_BUFFER_SIZE/UNIT - 1) >
+			(GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT));
+	BUILD_BUG_ON((DPC_BUFFER_SIZE/UNIT - 1) >
+			(GUC_LOG_DPC_MASK >> GUC_LOG_DPC_SHIFT));
+	BUILD_BUG_ON((ISR_BUFFER_SIZE/UNIT - 1) >
+			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
+
+	flags = GUC_LOG_VALID |
+		GUC_LOG_NOTIFY_ON_HALF_FULL |
+		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
+		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
+		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
 		(offset << GUC_LOG_BUF_ADDR_SHIFT);
 
+	#undef UNIT
+
 	return flags;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 0867ba76d445..1a0f2a39cef9 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -84,12 +84,12 @@
 #define   GUC_LOG_VALID			(1 << 0)
 #define   GUC_LOG_NOTIFY_ON_HALF_FULL	(1 << 1)
 #define   GUC_LOG_ALLOC_IN_MEGABYTE	(1 << 3)
-#define   GUC_LOG_CRASH_PAGES		1
 #define   GUC_LOG_CRASH_SHIFT		4
-#define   GUC_LOG_DPC_PAGES		7
+#define   GUC_LOG_CRASH_MASK		(0x1 << GUC_LOG_CRASH_SHIFT)
 #define   GUC_LOG_DPC_SHIFT		6
-#define   GUC_LOG_ISR_PAGES		7
+#define   GUC_LOG_DPC_MASK	        (0x7 << GUC_LOG_DPC_SHIFT)
 #define   GUC_LOG_ISR_SHIFT		9
+#define   GUC_LOG_ISR_MASK	        (0x7 << GUC_LOG_ISR_SHIFT)
 #define   GUC_LOG_BUF_ADDR_SHIFT	12
 
 #define GUC_CTL_PAGE_FAULT_CONTROL	5
@@ -532,20 +532,6 @@ enum guc_log_buffer_type {
 };
 
 /**
- * DOC: GuC Log buffer Layout
- *
- * Page0  +-------------------------------+
- *        |   ISR state header (32 bytes) |
- *        |      DPC state header         |
- *        |   Crash dump state header     |
- * Page1  +-------------------------------+
- *        |           ISR logs            |
- * Page9  +-------------------------------+
- *        |           DPC logs            |
- * Page17 +-------------------------------+
- *        |         Crash Dump logs       |
- *        +-------------------------------+
- *
  * Below state structure is used for coordination of retrieval of GuC firmware
  * logs. Separate state is maintained for each log buffer type.
  * read_ptr points to the location where i915 read last in log buffer and
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index a751025b6ad7..1c4a8de9c305 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -27,6 +27,28 @@
 #include "intel_guc_log.h"
 #include "i915_drv.h"
 
+/*
+ *  GuC Log buffer Layout
+ *
+ *  +===============================+ 00B
+ *  |    Crash dump state header    |
+ *  +-------------------------------+ 32B
+ *  |       DPC state header        |
+ *  +-------------------------------+ 64B
+ *  |       ISR state header        |
+ *  +-------------------------------+ 96B
+ *  |                               |
+ *  +===============================+ PAGE_SIZE (4KB)
+ *  |        Crash Dump logs        |
+ *  +===============================+ + CRASH_SIZE
+ *  |           DPC logs            |
+ *  +===============================+ + DPC_SIZE
+ *  |           ISR logs            |
+ *  +===============================+ + ISR_SIZE
+ */
+#define GUC_LOG_SIZE	(PAGE_SIZE + CRASH_BUFFER_SIZE + DPC_BUFFER_SIZE + \
+			ISR_BUFFER_SIZE)
+
 static void guc_log_capture_logs(struct intel_guc_log *log);
 
 /**
@@ -215,11 +237,11 @@ static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type type)
 {
 	switch (type) {
 	case GUC_ISR_LOG_BUFFER:
-		return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
+		return ISR_BUFFER_SIZE;
 	case GUC_DPC_LOG_BUFFER:
-		return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
+		return DPC_BUFFER_SIZE;
 	case GUC_CRASH_DUMP_LOG_BUFFER:
-		return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
+		return CRASH_BUFFER_SIZE;
 	default:
 		MISSING_CASE(type);
 	}
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 995aeb29a3b9..1b3afdae6d0d 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -34,12 +34,9 @@
 
 struct intel_guc;
 
-/*
- * The first page is to save log buffer state. Allocate one
- * extra page for others in case for overlap
- */
-#define GUC_LOG_SIZE	((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \
-			  1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT)
+#define CRASH_BUFFER_SIZE	8192
+#define DPC_BUFFER_SIZE		32768
+#define ISR_BUFFER_SIZE		32768
 
 /*
  * While we're using plain log level in i915, GuC controls are much more...
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes.
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
                   ` (4 preceding siblings ...)
  2018-05-30 13:53 ` [PATCH 6/7] drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h Piotr Piorkowski
@ 2018-05-30 13:53 ` Piotr Piorkowski
  2018-05-30 16:46   ` Michal Wajdeczko
  2018-05-30 14:49 ` [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Michal Wajdeczko
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Piotr Piorkowski @ 2018-05-30 13:53 UTC (permalink / raw)
  To: intel-gfx

At this moment we can define GuC logs sizes only using pages.
But GuC also allows use for this values expressed in megabytes.
Lets add support for define guc_log_size in megabytes when we
debug of GuC.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c     | 12 ++++++++++--
 drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index e15047fedb45..5a42db47521b 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -263,7 +263,13 @@ static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
 	u32 flags;
 
+	#if (((CRASH_BUFFER_SIZE) % (1 << 20)) == 0)
+	#define UNIT (1 << 20)
+	#define FLAG GUC_LOG_ALLOC_IN_MEGABYTE
+	#else
 	#define UNIT (4 << 10)
+	#define FLAG 0
+	#endif
 
 	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
 	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
@@ -280,13 +286,15 @@ static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
 
 	flags = GUC_LOG_VALID |
-		GUC_LOG_NOTIFY_ON_HALF_FULL |
-		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
+		GUC_LOG_NOTIFY_ON_HALF_FULL;
+	flags |= FLAG;
+	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
 		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
 		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
 		(offset << GUC_LOG_BUF_ADDR_SHIFT);
 
 	#undef UNIT
+	#undef FLAG
 
 	return flags;
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 1b3afdae6d0d..de39b965ae7a 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -34,9 +34,15 @@
 
 struct intel_guc;
 
+#ifdef DRM_I915_DEBUG_GUC
+#define CRASH_BUFFER_SIZE	2097152
+#define DPC_BUFFER_SIZE		8388608
+#define ISR_BUFFER_SIZE		8388608
+#else
 #define CRASH_BUFFER_SIZE	8192
 #define DPC_BUFFER_SIZE		32768
 #define ISR_BUFFER_SIZE		32768
+#endif
 
 /*
  * While we're using plain log level in i915, GuC controls are much more...
-- 
2.14.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
                   ` (5 preceding siblings ...)
  2018-05-30 13:53 ` [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes Piotr Piorkowski
@ 2018-05-30 14:49 ` Michal Wajdeczko
  2018-05-30 15:30 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] " Patchwork
  2018-05-30 15:46 ` ✗ Fi.CI.BAT: failure " Patchwork
  8 siblings, 0 replies; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 14:49 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

Hi,

On Wed, 30 May 2018 15:53:28 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> From: Piotr Piórkowski <piotr.piorkowski@intel.com>
>
> Currently we are using modparam as placeholder for GuC log level.
> Stop doing this and keep runtime GuC level in intel_guc_log struct.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     |  8 +++-----
>  drivers/gpu/drm/i915/intel_guc_log.c | 18 +++++-------------
>  drivers/gpu/drm/i915/intel_guc_log.h |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_uc.c      |  2 +-
>  4 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 116f4ccf1bbd..9025837850ad 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -203,13 +203,11 @@ void intel_guc_fini(struct intel_guc *guc)
>  	guc_shared_data_destroy(guc);
>  }
> -static u32 get_log_control_flags(void)
> +static u32 guc_ctl_debug_flags(struct intel_guc *guc)
>  {
> -	u32 level = i915_modparams.guc_log_level;
> +	u32 level = intel_guc_log_level_get(&guc->log);
>  	u32 flags = 0;
> -	GEM_BUG_ON(level < 0);
> -
>  	if (!GUC_LOG_LEVEL_IS_ENABLED(level))
>  		flags |= GUC_LOG_DEFAULT_DISABLED;
> @@ -250,7 +248,7 @@ void intel_guc_init_params(struct intel_guc *guc)
> 	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
> -	params[GUC_CTL_DEBUG] = get_log_control_flags();
> +	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
> 	/* 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_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 401e1704d61e..c036d0fac370 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -475,11 +475,12 @@ int intel_guc_log_create(struct intel_guc_log *log)
>  	offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT;
>  	log->flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> +	log->level = i915_modparams.guc_log_level;
> +
>  	return 0;
> err:
> -	/* logging will be off */
> -	i915_modparams.guc_log_level = 0;
> +	DRM_ERROR("Failed to allocate GuC log buffer. %d\n", ret);
>  	return ret;
>  }
> @@ -488,14 +489,6 @@ void intel_guc_log_destroy(struct intel_guc_log  
> *log)
>  	i915_vma_unpin_and_release(&log->vma);
>  }
> -int intel_guc_log_level_get(struct intel_guc_log *log)
> -{
> -	GEM_BUG_ON(!log->vma);
> -	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> -
> -	return i915_modparams.guc_log_level;
> -}
> -
>  int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
>  {
>  	struct intel_guc *guc = log_to_guc(log);
> @@ -504,7 +497,6 @@ int intel_guc_log_level_set(struct intel_guc_log  
> *log, u64 val)
> 	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
>  	GEM_BUG_ON(!log->vma);
> -	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> 	/*
>  	 * GuC is recognizing log levels starting from 0 to max, we're using 0
> @@ -515,7 +507,7 @@ int intel_guc_log_level_set(struct intel_guc_log  
> *log, u64 val)
> 	mutex_lock(&dev_priv->drm.struct_mutex);
> -	if (i915_modparams.guc_log_level == val) {
> +	if (log->level == val) {
>  		ret = 0;
>  		goto out_unlock;
>  	}
> @@ -530,7 +522,7 @@ int intel_guc_log_level_set(struct intel_guc_log  
> *log, u64 val)
>  		goto out_unlock;
>  	}
> -	i915_modparams.guc_log_level = val;
> +	log->level = val;
> out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index fa80535a6f9d..ea375c3b5d08 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -30,6 +30,7 @@
>  #include <linux/workqueue.h>
> #include "intel_guc_fwif.h"
> +#include "i915_gem.h"
> struct intel_guc;
> @@ -58,6 +59,7 @@ struct intel_guc;
>  #define GUC_LOG_LEVEL_MAX  
> GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)
> struct intel_guc_log {
> +	u32 level;
>  	u32 flags;
>  	struct i915_vma *vma;
>  	struct {
> @@ -80,7 +82,6 @@ void intel_guc_log_init_early(struct intel_guc_log  
> *log);
>  int intel_guc_log_create(struct intel_guc_log *log);
>  void intel_guc_log_destroy(struct intel_guc_log *log);
> -int intel_guc_log_level_get(struct intel_guc_log *log);
>  int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val);
>  bool intel_guc_log_relay_enabled(const struct intel_guc_log *log);
>  int intel_guc_log_relay_open(struct intel_guc_log *log);
> @@ -89,4 +90,10 @@ void intel_guc_log_relay_close(struct intel_guc_log  
> *log);
> void intel_guc_log_handle_flush_event(struct intel_guc_log *log);
> +static inline u32 intel_guc_log_level_get(struct intel_guc_log *log)

hmm, it's little unfortunate that earlier this function was not named as:

	intel_guc_log_get_level(struct intel_guc_log *log)

with matching:

	intel_guc_log_set_level(struct intel_guc_log *log, u32 level)

but maybe we can fix it now or in near future?

> +{
> +	GEM_BUG_ON(!log->vma);

hmm, maybe we can drop this BUG_ON as we will have level=0(disabled)
when there is no vma - and this level value is expected/correct.

> +	return log->level;
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 6a73e81f373b..5ce8d5df1b58 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -207,7 +207,7 @@ void intel_uc_init_mmio(struct drm_i915_private  
> *i915)
> static void guc_capture_load_err_log(struct intel_guc *guc)
>  {
> -	if (!guc->log.vma || !i915_modparams.guc_log_level)
> +	if (!guc->log.vma || !intel_guc_log_level_get(&guc->log))

this could simplified if we drop BUG_ON from intel_guc_log_level_get

>  		return;
> 	if (!guc->load_err_log)

with removed GEM_BUG_ON,
with or/without renamed get/set_level functions,
this is

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter
  2018-05-30 13:53 ` [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter Piotr Piorkowski
@ 2018-05-30 14:58   ` Michal Wajdeczko
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 14:58 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

On Wed, 30 May 2018 15:53:29 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At the moment, the preparation of GUC_CTL_DEBUG is disordered.
> Lets move all GUC_CTL_DEBUG related operations to one place.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 9025837850ad..3cc99fcaaea6 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -205,6 +205,7 @@ void intel_guc_fini(struct intel_guc *guc)
> static u32 guc_ctl_debug_flags(struct intel_guc *guc)
>  {
> +	u32 ads = intel_guc_ggtt_offset(guc, guc->ads_vma) >> PAGE_SHIFT;

if 'ads' is used only in USES_GUC_SUBMISSION case, then maybe we
should define it there ?

>  	u32 level = intel_guc_log_level_get(&guc->log);
>  	u32 flags = 0;
> @@ -217,6 +218,11 @@ static u32 guc_ctl_debug_flags(struct intel_guc  
> *guc)
>  		flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) <<
>  			 GUC_LOG_VERBOSITY_SHIFT;
> +	if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
> +		flags |= ads << GUC_ADS_ADDR_SHIFT;
> +		flags |= GUC_ADS_ENABLED;

this can done in single statement

> +	}
> +
>  	return flags;
>  }
> @@ -252,14 +258,9 @@ void intel_guc_init_params(struct intel_guc *guc)
> 	/* If GuC submission is enabled, set up additional parameters here */
>  	if (USES_GUC_SUBMISSION(dev_priv)) {
> -		u32 ads = intel_guc_ggtt_offset(guc,
> -						guc->ads_vma) >> PAGE_SHIFT;
>  		u32 pgs = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
>  		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> -		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
> -		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
> -
>  		pgs >>= PAGE_SHIFT;
>  		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
>  			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);

with or/without bikesheds fixed,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter
  2018-05-30 13:53 ` [PATCH 3/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter Piotr Piorkowski
@ 2018-05-30 14:59   ` Michal Wajdeczko
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 14:59 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

On Wed, 30 May 2018 15:53:30 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At the moment, the preparation of GUC_CTL_FEATURE is disordered.
> Lets move all GUC_CTL_FEATURE related operations to one place.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter
  2018-05-30 13:53 ` [PATCH 4/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter Piotr Piorkowski
@ 2018-05-30 15:01   ` Michal Wajdeczko
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 15:01 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

On Wed, 30 May 2018 15:53:31 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At the moment, the preparation of GUC_CTL_LOG_PARAMS is disordered.
> Additionally, in struct intel_guc_log we have an unnecessary field
> 'flags' which we use only to assign value to GuC parameter.
> Lets move all GUC_CTL_LOG_PARAMS related operations to one place,
> and lets remove field 'flags' from struct intel_guc_log.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter
  2018-05-30 13:53 ` [PATCH 5/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter Piotr Piorkowski
@ 2018-05-30 15:06   ` Michal Wajdeczko
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 15:06 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

On Wed, 30 May 2018 15:53:32 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At the moment, the preparation of GUC_CTL_CTXINFO is disordered.
> Lets move all  GUC_CTL_CTXINFO related operations to one place.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 185459ee49c9..3b45f06b1aa2 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -239,6 +239,25 @@ static u32 guc_ctl_feature_flags(struct intel_guc  
> *guc)
> 	return flags;
>  }
> +
> +static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
> +{
> +	u32 flags = 0;
> +	u32 ctxnum;
> +	u32 base;

better to declare ctxnum/base inside 'if' below

> +
> +	if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
> +
> +		base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
> +		ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;
> +
> +		base >>= PAGE_SHIFT;
> +		flags = (base << GUC_CTL_BASE_ADDR_SHIFT) |

to be future friendly and not overwrite other bits use:

		flags |= ...

> +			(ctxnum << GUC_CTL_CTXNUM_IN16_SHIFT);
> +	}
> +	return flags;
> +}
> +
>  static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
>  {
>  	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
> @@ -280,16 +299,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>  	params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
>  	params[GUC_CTL_LOG_PARAMS]  = guc_ctl_log_params_flags(guc);
>  	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
> -
> -	/* If GuC submission is enabled, set up additional parameters here */
> -	if (USES_GUC_SUBMISSION(dev_priv)) {
> -		u32 pgs = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
> -		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> -
> -		pgs >>= PAGE_SHIFT;
> -		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
> -			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
> -	}
> +	params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
> 	/*
>  	 * All SOFT_SCRATCH registers are in FORCEWAKE_BLITTER domain and

with above fixed,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] drm/i915/guc: Don't store runtime GuC log level in modparam
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
                   ` (6 preceding siblings ...)
  2018-05-30 14:49 ` [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Michal Wajdeczko
@ 2018-05-30 15:30 ` Patchwork
  2018-05-30 15:46 ` ✗ Fi.CI.BAT: failure " Patchwork
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-05-30 15:30 UTC (permalink / raw)
  To: Piotr Piorkowski; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/7] drm/i915/guc: Don't store runtime GuC log level in modparam
URL   : https://patchwork.freedesktop.org/series/43952/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ef254da06d89 drm/i915/guc: Don't store runtime GuC log level in modparam
fa025916c333 drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter
492303e41449 drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter
dcdde612e96a drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter
-:30: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#30: FILE: drivers/gpu/drm/i915/intel_guc.c:242:
 }
+static u32 guc_ctl_log_params_flags(struct intel_guc *guc)

total: 0 errors, 0 warnings, 1 checks, 60 lines checked
6848b97dc926 drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter
-:35: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
#35: FILE: drivers/gpu/drm/i915/intel_guc.c:250:
+	if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
+

total: 0 errors, 0 warnings, 1 checks, 42 lines checked
f72db2efed06 drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h
-:42: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#42: FILE: drivers/gpu/drm/i915/intel_guc.c:275:
+	BUILD_BUG_ON((CRASH_BUFFER_SIZE/UNIT - 1) >
 	                               ^

-:44: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#44: FILE: drivers/gpu/drm/i915/intel_guc.c:277:
+	BUILD_BUG_ON((DPC_BUFFER_SIZE/UNIT - 1) >
 	                             ^

-:46: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#46: FILE: drivers/gpu/drm/i915/intel_guc.c:279:
+	BUILD_BUG_ON((ISR_BUFFER_SIZE/UNIT - 1) >
 	                             ^

-:51: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#51: FILE: drivers/gpu/drm/i915/intel_guc.c:284:
+		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
 		                   ^

-:52: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#52: FILE: drivers/gpu/drm/i915/intel_guc.c:285:
+		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
 		                 ^

-:53: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#53: FILE: drivers/gpu/drm/i915/intel_guc.c:286:
+		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
 		                 ^

total: 0 errors, 0 warnings, 6 checks, 128 lines checked
0ad99c4dbd3b drm/i915/guc: Add support for define guc_log_size in megabytes.
-:47: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#47: FILE: drivers/gpu/drm/i915/intel_guc.c:291:
+	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
 	                            ^

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/7] drm/i915/guc: Don't store runtime GuC log level in modparam
  2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
                   ` (7 preceding siblings ...)
  2018-05-30 15:30 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] " Patchwork
@ 2018-05-30 15:46 ` Patchwork
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-05-30 15:46 UTC (permalink / raw)
  To: Piotr Piorkowski; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/7] drm/i915/guc: Don't store runtime GuC log level in modparam
URL   : https://patchwork.freedesktop.org/series/43952/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4257 -> Patchwork_9149 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9149 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9149, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43952/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9149:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       PASS -> FAIL

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9149 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-hsw-4770:        PASS -> FAIL (fdo#100368)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-cnl-psr:         FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-bsw-n3050:       INCOMPLETE (fdo#106729) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-skl-guc:         FAIL (fdo#104724, fdo#103191) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-c:
      fi-glk-j4005:       DMESG-WARN (fdo#106097, fdo#106000) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106729 https://bugs.freedesktop.org/show_bug.cgi?id=106729


== Participating hosts (45 -> 39) ==

  Missing    (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-cfl-u2 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4257 -> Patchwork_9149

  CI_DRM_4257: 8aac35d26057479982a346c0e9cd57c2e930b7e1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4501: 6796a604bab6df9c84af149e799902360afdd157 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9149: 0ad99c4dbd3b6cbeba24c531d591d023e8b7a462 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0ad99c4dbd3b drm/i915/guc: Add support for define guc_log_size in megabytes.
f72db2efed06 drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h
6848b97dc926 drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter
dcdde612e96a drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter
492303e41449 drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter
fa025916c333 drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter
ef254da06d89 drm/i915/guc: Don't store runtime GuC log level in modparam

== Logs ==

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/7] drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h
  2018-05-30 13:53 ` [PATCH 6/7] drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h Piotr Piorkowski
@ 2018-05-30 16:18   ` Michal Wajdeczko
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 16:18 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

On Wed, 30 May 2018 15:53:33 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At this moment, we have defined GuC logs sizes in intel_guc_fwif.h, but
> as these values are related directly to the GuC logs, and not to API of
> GuC parameters, we should move these defines to intel_guc_log.h.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc.c      | 28 +++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_guc_fwif.h | 20 +++-----------------
>  drivers/gpu/drm/i915/intel_guc_log.c  | 28 +++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_guc_log.h  |  9 +++------
>  4 files changed, 54 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 3b45f06b1aa2..e15047fedb45 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -263,13 +263,31 @@ static u32 guc_ctl_log_params_flags(struct  
> intel_guc *guc)
>  	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
>  	u32 flags;
> -	/* each allocated unit is a page */
> -	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> -		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT) |
> -		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> -		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> +	#define UNIT (4 << 10)
> +
> +	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
> +	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
> +	BUILD_BUG_ON(!DPC_BUFFER_SIZE);
> +	BUILD_BUG_ON(!IS_ALIGNED(DPC_BUFFER_SIZE, UNIT));
> +	BUILD_BUG_ON(!ISR_BUFFER_SIZE);
> +	BUILD_BUG_ON(!IS_ALIGNED(ISR_BUFFER_SIZE, UNIT));
> +
> +	BUILD_BUG_ON((CRASH_BUFFER_SIZE/UNIT - 1) >
> +			(GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT));
> +	BUILD_BUG_ON((DPC_BUFFER_SIZE/UNIT - 1) >
> +			(GUC_LOG_DPC_MASK >> GUC_LOG_DPC_SHIFT));
> +	BUILD_BUG_ON((ISR_BUFFER_SIZE/UNIT - 1) >
> +			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
> +
> +	flags = GUC_LOG_VALID |
> +		GUC_LOG_NOTIFY_ON_HALF_FULL |
> +		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
> +		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
> +		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
>  		(offset << GUC_LOG_BUF_ADDR_SHIFT);
> +	#undef UNIT
> +
>  	return flags;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
> b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 0867ba76d445..1a0f2a39cef9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -84,12 +84,12 @@
>  #define   GUC_LOG_VALID			(1 << 0)
>  #define   GUC_LOG_NOTIFY_ON_HALF_FULL	(1 << 1)
>  #define   GUC_LOG_ALLOC_IN_MEGABYTE	(1 << 3)
> -#define   GUC_LOG_CRASH_PAGES		1
>  #define   GUC_LOG_CRASH_SHIFT		4
> -#define   GUC_LOG_DPC_PAGES		7
> +#define   GUC_LOG_CRASH_MASK		(0x1 << GUC_LOG_CRASH_SHIFT)
>  #define   GUC_LOG_DPC_SHIFT		6
> -#define   GUC_LOG_ISR_PAGES		7
> +#define   GUC_LOG_DPC_MASK	        (0x7 << GUC_LOG_DPC_SHIFT)
>  #define   GUC_LOG_ISR_SHIFT		9
> +#define   GUC_LOG_ISR_MASK	        (0x7 << GUC_LOG_ISR_SHIFT)
>  #define   GUC_LOG_BUF_ADDR_SHIFT	12
> #define GUC_CTL_PAGE_FAULT_CONTROL	5
> @@ -532,20 +532,6 @@ enum guc_log_buffer_type {
>  };
> /**
> - * DOC: GuC Log buffer Layout
> - *
> - * Page0  +-------------------------------+
> - *        |   ISR state header (32 bytes) |
> - *        |      DPC state header         |
> - *        |   Crash dump state header     |
> - * Page1  +-------------------------------+
> - *        |           ISR logs            |
> - * Page9  +-------------------------------+
> - *        |           DPC logs            |
> - * Page17 +-------------------------------+
> - *        |         Crash Dump logs       |
> - *        +-------------------------------+
> - *
>   * Below state structure is used for coordination of retrieval of GuC  
> firmware
>   * logs. Separate state is maintained for each log buffer type.
>   * read_ptr points to the location where i915 read last in log buffer  
> and
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index a751025b6ad7..1c4a8de9c305 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -27,6 +27,28 @@
>  #include "intel_guc_log.h"
>  #include "i915_drv.h"
> +/*
> + *  GuC Log buffer Layout
> + *
> + *  +===============================+ 00B
> + *  |    Crash dump state header    |
> + *  +-------------------------------+ 32B
> + *  |       DPC state header        |
> + *  +-------------------------------+ 64B
> + *  |       ISR state header        |
> + *  +-------------------------------+ 96B
> + *  |                               |
> + *  +===============================+ PAGE_SIZE (4KB)
> + *  |        Crash Dump logs        |
> + *  +===============================+ + CRASH_SIZE
> + *  |           DPC logs            |
> + *  +===============================+ + DPC_SIZE
> + *  |           ISR logs            |
> + *  +===============================+ + ISR_SIZE
> + */
> +#define GUC_LOG_SIZE	(PAGE_SIZE + CRASH_BUFFER_SIZE + DPC_BUFFER_SIZE +  
> \
> +			ISR_BUFFER_SIZE)

maybe we can drop this define completely and:
- sum sizes explicitly in intel_guc_log_create()
- use log->vma.size in guc_log_relay_create()

> +
>  static void guc_log_capture_logs(struct intel_guc_log *log);
> /**
> @@ -215,11 +237,11 @@ static unsigned int guc_get_log_buffer_size(enum  
> guc_log_buffer_type type)
>  {
>  	switch (type) {
>  	case GUC_ISR_LOG_BUFFER:
> -		return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
> +		return ISR_BUFFER_SIZE;
>  	case GUC_DPC_LOG_BUFFER:
> -		return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
> +		return DPC_BUFFER_SIZE;
>  	case GUC_CRASH_DUMP_LOG_BUFFER:
> -		return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
> +		return CRASH_BUFFER_SIZE;
>  	default:
>  		MISSING_CASE(type);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index 995aeb29a3b9..1b3afdae6d0d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -34,12 +34,9 @@
> struct intel_guc;
> -/*
> - * The first page is to save log buffer state. Allocate one
> - * extra page for others in case for overlap
> - */
> -#define GUC_LOG_SIZE	((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \
> -			  1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT)
> +#define CRASH_BUFFER_SIZE	8192
> +#define DPC_BUFFER_SIZE		32768
> +#define ISR_BUFFER_SIZE		32768

bikeshed: maybe something more friendly like (32 * 1024) ?

> /*
>   * While we're using plain log level in i915, GuC controls are much  
> more...

with GUC_LOG_SIZE removed,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes.
  2018-05-30 13:53 ` [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes Piotr Piorkowski
@ 2018-05-30 16:46   ` Michal Wajdeczko
  2018-06-04 12:53     ` Piorkowski, Piotr
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Wajdeczko @ 2018-05-30 16:46 UTC (permalink / raw)
  To: intel-gfx, Piotr Piorkowski

On Wed, 30 May 2018 15:53:34 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At this moment we can define GuC logs sizes only using pages.
> But GuC also allows use for this values expressed in megabytes.
> Lets add support for define guc_log_size in megabytes when we
> debug of GuC.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     | 12 ++++++++++--
>  drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index e15047fedb45..5a42db47521b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -263,7 +263,13 @@ static u32 guc_ctl_log_params_flags(struct  
> intel_guc *guc)
>  	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
>  	u32 flags;
> +	#if (((CRASH_BUFFER_SIZE) % (1 << 20)) == 0)
> +	#define UNIT (1 << 20)
> +	#define FLAG GUC_LOG_ALLOC_IN_MEGABYTE
> +	#else
>  	#define UNIT (4 << 10)
> +	#define FLAG 0
> +	#endif
> 	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
>  	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
> @@ -280,13 +286,15 @@ static u32 guc_ctl_log_params_flags(struct  
> intel_guc *guc)
>  			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
> 	flags = GUC_LOG_VALID |
> -		GUC_LOG_NOTIFY_ON_HALF_FULL |
> -		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
> +		GUC_LOG_NOTIFY_ON_HALF_FULL;
> +	flags |= FLAG;

I think you can inject FLAG into existing statement without
introducing two additional |=

> +	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
>  		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
>  		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
>  		(offset << GUC_LOG_BUF_ADDR_SHIFT);
> 	#undef UNIT
> +	#undef FLAG
> 	return flags;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index 1b3afdae6d0d..de39b965ae7a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -34,9 +34,15 @@
> struct intel_guc;
> +#ifdef DRM_I915_DEBUG_GUC
> +#define CRASH_BUFFER_SIZE	2097152
> +#define DPC_BUFFER_SIZE		8388608
> +#define ISR_BUFFER_SIZE		8388608

can we make it more friendly: (8 * 1024 * 1024)

> +#else
>  #define CRASH_BUFFER_SIZE	8192
>  #define DPC_BUFFER_SIZE		32768
>  #define ISR_BUFFER_SIZE		32768
> +#endif

btw, are these values just max possible or selected
as most valuable ? question for both debug/ndebug

Michal

> /*
>   * While we're using plain log level in i915, GuC controls are much  
> more...
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes.
  2018-05-30 16:46   ` Michal Wajdeczko
@ 2018-06-04 12:53     ` Piorkowski, Piotr
  0 siblings, 0 replies; 17+ messages in thread
From: Piorkowski, Piotr @ 2018-06-04 12:53 UTC (permalink / raw)
  To: intel-gfx@lists.freedesktop.org, Wajdeczko, Michal


[-- Attachment #1.1: Type: text/plain, Size: 3359 bytes --]

On Wed, 2018-05-30 at 18:46 +0200, Michal Wajdeczko wrote:
> On Wed, 30 May 2018 15:53:34 +0200, Piotr Piorkowski  
> <piotr.piorkowski@intel.com> wrote:
> 
> > At this moment we can define GuC logs sizes only using pages.
> > But GuC also allows use for this values expressed in megabytes.
> > Lets add support for define guc_log_size in megabytes when we
> > debug of GuC.
> > 
> > Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_guc.c     | 12 ++++++++++--
> >  drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> > b/drivers/gpu/drm/i915/intel_guc.c
> > index e15047fedb45..5a42db47521b 100644
> > --- a/drivers/gpu/drm/i915/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/intel_guc.c
> > @@ -263,7 +263,13 @@ static u32 guc_ctl_log_params_flags(struct  
> > intel_guc *guc)
> >  	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >>
> > PAGE_SHIFT;
> >  	u32 flags;
> > +	#if (((CRASH_BUFFER_SIZE) % (1 << 20)) == 0)
> > +	#define UNIT (1 << 20)
> > +	#define FLAG GUC_LOG_ALLOC_IN_MEGABYTE
> > +	#else
> >  	#define UNIT (4 << 10)
> > +	#define FLAG 0
> > +	#endif
> > 	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
> >  	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
> > @@ -280,13 +286,15 @@ static u32 guc_ctl_log_params_flags(struct  
> > intel_guc *guc)
> >  			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
> > 	flags = GUC_LOG_VALID |
> > -		GUC_LOG_NOTIFY_ON_HALF_FULL |
> > -		((CRASH_BUFFER_SIZE/UNIT - 1) <<
> > GUC_LOG_CRASH_SHIFT) |
> > +		GUC_LOG_NOTIFY_ON_HALF_FULL;
> > +	flags |= FLAG;
> 
> I think you can inject FLAG into existing statement without
> introducing two additional |=
> 
> > +	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) <<
> > GUC_LOG_CRASH_SHIFT) |
> >  		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT)
> > |
> >  		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT)
> > |
> >  		(offset << GUC_LOG_BUF_ADDR_SHIFT);
> > 	#undef UNIT
> > +	#undef FLAG
> > 	return flags;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> > b/drivers/gpu/drm/i915/intel_guc_log.h
> > index 1b3afdae6d0d..de39b965ae7a 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.h
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> > @@ -34,9 +34,15 @@
> > struct intel_guc;
> > +#ifdef DRM_I915_DEBUG_GUC
> > +#define CRASH_BUFFER_SIZE	2097152
> > +#define DPC_BUFFER_SIZE		8388608
> > +#define ISR_BUFFER_SIZE		8388608
> 
> can we make it more friendly: (8 * 1024 * 1024)
> 
> > +#else
> >  #define CRASH_BUFFER_SIZE	8192
> >  #define DPC_BUFFER_SIZE		32768
> >  #define ISR_BUFFER_SIZE		32768
> > +#endif
> 
> btw, are these values just max possible or selected
> as most valuable ? question for both debug/ndebug

For debug I use the possibility of defining values in MB and I set the
maximum possible values, and for ndebug I set old values of these
buffers
> 
> Michal
> 
> > /*
> >   * While we're using plain log level in i915, GuC controls are
> > much  
> > more...

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-06-04 12:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-30 13:53 [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Piotr Piorkowski
2018-05-30 13:53 ` [PATCH 2/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_DEBUG parameter Piotr Piorkowski
2018-05-30 14:58   ` Michal Wajdeczko
2018-05-30 13:53 ` [PATCH 3/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_FEATURE parameter Piotr Piorkowski
2018-05-30 14:59   ` Michal Wajdeczko
2018-05-30 13:53 ` [PATCH 4/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_LOG_PARAMS parameter Piotr Piorkowski
2018-05-30 15:01   ` Michal Wajdeczko
2018-05-30 13:53 ` [PATCH 5/7] drm/i915/guc: Refactoring preparation of the GUC_CTL_CTXINFO parameter Piotr Piorkowski
2018-05-30 15:06   ` Michal Wajdeczko
2018-05-30 13:53 ` [PATCH 6/7] drm/i915/guc: Move defines with size of GuC logs to intel_guc_log.h Piotr Piorkowski
2018-05-30 16:18   ` Michal Wajdeczko
2018-05-30 13:53 ` [PATCH 7/7] drm/i915/guc: Add support for define guc_log_size in megabytes Piotr Piorkowski
2018-05-30 16:46   ` Michal Wajdeczko
2018-06-04 12:53     ` Piorkowski, Piotr
2018-05-30 14:49 ` [PATCH 1/7] drm/i915/guc: Don't store runtime GuC log level in modparam Michal Wajdeczko
2018-05-30 15:30 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] " Patchwork
2018-05-30 15:46 ` ✗ Fi.CI.BAT: failure " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.