[PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Sujaritha Sundaresan]

We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
We also need enable_guc_loading=1 when we want to verify the HuC,
which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).

v2: Clarifying the commit message (Anusha)

v3: Unify seq_puts messages, Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating message unification into a separate patch

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 11 +++++--
 drivers/gpu/drm/i915/i915_drv.h         |  9 ++++--
 drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
 drivers/gpu/drm/i915/i915_irq.c         |  2 +-
 drivers/gpu/drm/i915/i915_params.c      |  5 ----
 drivers/gpu/drm/i915/i915_params.h      |  1 -
 drivers/gpu/drm/i915/intel_guc_loader.c |  7 +++--
 drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
 drivers/gpu/drm/i915/intel_uc.c         | 51 +++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_uc.h         |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
 12 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 53e40dd..4fde4b2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2336,8 +2336,10 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
 
-	if (!HAS_HUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)){
+		seq_puts(m, "not supported\n");
 		return 0;
+	}
 
 	seq_puts(m, "HuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n", huc_fw->path);
@@ -2369,8 +2371,11 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	u32 tmp, i;
 
-	if (!HAS_GUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)){
+		seq_puts(m, "not supported\n");
 		return 0;
+		
+	}
 
 	seq_printf(m, "GuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n",
@@ -2465,7 +2470,7 @@ static bool check_guc_submission(struct seq_file *m)
 
 	if (!guc->execbuf_client) {
 		seq_printf(m, "GuC submission %s\n",
-			   HAS_GUC_SCHED(dev_priv) ?
+			   HAS_GUC(dev_priv) ?
 			   "disabled" :
 			   "not supported");
 		return false;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 61a4be9..6479b72 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3141,9 +3141,12 @@ static inline unsigned int i915_sg_segment_size(void)
  */
 #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
 #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
-#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
+#define HAS_GUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
+#define HAS_HUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
+
+#define NEEDS_GUC_LOADING(dev_priv) \
+	(HAS_GUC(dev_priv) && \
+	(i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
 
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 921ee36..0890341 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -314,7 +314,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
 	 * present or not in use we still need a small bias as ring wraparound
 	 * at offset 0 sometimes hangs. No idea why.
 	 */
-	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
+	if (NEEDS_GUC_LOADING(dev_priv))
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 64d7852..a32935a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3292,7 +3292,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
 	 * currently don't have any bits spare to pass in this upper
 	 * restriction!
 	 */
-	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
+	if (NEEDS_GUC_LOADING(dev_priv)) {
 		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
 		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6a07ef3..60cdf0d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3956,7 +3956,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	for (i = 0; i < MAX_L3_SLICES; ++i)
 		dev_priv->l3_parity.remap_info[i] = NULL;
 
-	if (HAS_GUC_SCHED(dev_priv))
+	if (NEEDS_GUC_LOADING(dev_priv))
 		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
 
 	/* Let's track the enabled rps events */
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index ec65341..30344e1 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -63,7 +63,6 @@ struct i915_params i915_modparams __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
 	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.guc_firmware_path = NULL,
@@ -201,10 +200,6 @@ struct i915_params i915_modparams __read_mostly = {
 	"(0=use value from vbt [default], 1=low power swing(200mV),"
 	"2=default swing(400mV))");
 
-i915_param_named_unsafe(enable_guc_loading, int, 0400,
-	"Enable GuC firmware loading "
-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
-
 i915_param_named_unsafe(enable_guc_submission, int, 0400,
 	"Enable GuC submission "
 	"(-1=auto, 0=never [default], 1=if available, 2=required)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index a2cbb47..b26df06 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,7 +44,6 @@
 	func(int, disable_power_well); \
 	func(int, enable_ips); \
 	func(int, invert_brightness); \
-	func(int, enable_guc_loading); \
 	func(int, enable_guc_submission); \
 	func(int, guc_log_level); \
 	func(char *, guc_firmware_path); \
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index c9e25be..4210680 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -382,7 +382,7 @@ int intel_guc_init_hw(struct intel_guc *guc)
  *
  * Return: zero when we know firmware, non-zero in other case
  */
-int intel_guc_select_fw(struct intel_guc *guc)
+void intel_guc_select_fw(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
@@ -412,8 +412,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
 		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
 		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
 	} else {
-		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
+		if(HAS_GUC(dev_priv))
+			DRM_ERROR("NO GUC FW konown for a platform with GuC!\n");
+		return;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6e1779b..ee9e786 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
 		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
 		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
 	} else {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		/* For now, everything with a GuC also has a HuC */
+		if (HAS_GUC(dev_priv))
+			DRM_ERROR("No HuC FW known for a platform with HuC!\n");
 		return;
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9018540..4290b35 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -63,35 +63,31 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
-		if (i915_modparams.enable_guc_loading > 0 ||
-		    i915_modparams.enable_guc_submission > 0)
+		if (i915_modparams.enable_guc_submission > 0)
 			DRM_INFO("Ignoring GuC options, no hardware\n");
 
-		i915_modparams.enable_guc_loading = 0;
 		i915_modparams.enable_guc_submission = 0;
 		return;
 	}
 
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_loading < 0)
-		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
 
 	/* Verify firmware version */
-	if (i915_modparams.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
-
-		if (intel_guc_select_fw(&dev_priv->guc))
-			i915_modparams.enable_guc_loading = 0;
+	if (!HAS_GUC(dev_priv)){
+		if (i915_modparams.enable_guc_submission > 0){
+			DRM_INFO("Ignoring GuC submission enable enable, no FW\n");
+			i915_modparams.enable_guc_submission = 0;
+			return;
+		}
 	}
 
 	/* Can't enable guc submission without guc loaded */
-	if (!i915_modparams.enable_guc_loading)
+	if (!i915_modparams.enable_guc_submission < 0)
 		i915_modparams.enable_guc_submission = 0;
+	return;
 
 	/* A negative value means "use platform default" */
 	if (i915_modparams.enable_guc_submission < 0)
-		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+		i915_modparams.enable_guc_submission = 1;
 }
 
 static void gen8_guc_raise_irq(struct intel_guc *guc)
@@ -106,6 +102,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	intel_guc_ct_init_early(&guc->ct);
+	intel_guc_select_fw(&dev_priv->guc);
+	intel_huc_select_fw(&dev_priv->huc);
 
 	mutex_init(&guc->send_mutex);
 	guc->send = intel_guc_send_nop;
@@ -258,6 +256,8 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_GUC(dev_priv))
+		return;
 	__intel_uc_fw_fini(&dev_priv->guc.fw);
 	__intel_uc_fw_fini(&dev_priv->huc.fw);
 }
@@ -333,7 +333,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	int ret, attempts;
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_LOADING(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -423,19 +423,20 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	i915_ggtt_disable_guc(dev_priv);
 
 	DRM_ERROR("GuC init failed\n");
-	if (i915_modparams.enable_guc_loading > 1 ||
-	    i915_modparams.enable_guc_submission > 1)
+	if (i915_modparams.enable_guc_submission > 1){
+		DRM_NOTE("GuC is required, so marking the GPU as wedged\n");
 		ret = -EIO;
-	else
-		ret = 0;
+	
+	} 
 
-	if (i915_modparams.enable_guc_submission) {
-		i915_modparams.enable_guc_submission = 0;
-		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	else if (i915_modparams.enable_guc_submission == 1){
+			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+			i915_modparams.enable_guc_submission = 0;
+			ret = 0;
 	}
 
-	i915_modparams.enable_guc_loading = 0;
-	DRM_NOTE("GuC firmware loading disabled\n");
+	else 
+		ret = 0;	
 
 	return ret;
 }
@@ -444,7 +445,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_LOADING(dev_priv))
 		return;
 
 	if (i915_modparams.enable_guc_submission)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7703c9a..58d787e 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -223,7 +223,7 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 }
 
 /* intel_guc_loader.c */
-int intel_guc_select_fw(struct intel_guc *guc);
+void intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b3c3f94..0164f41 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1786,12 +1786,10 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (!HAS_GUC(dev_priv))
-		return -EINVAL;
+	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 
 	return ret;
 }
-- 
1.9.1

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

Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Sagar Arun Kamble]

Subject is missing "parameter" in the end. Either keep module parameter 
or i915_modparams.


On 10/4/2017 4:26 AM, Sujaritha Sundaresan wrote:
> We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
> Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
> We also need enable_guc_loading=1 when we want to verify the HuC,
> which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Need to change the order of the tag to comply with new convention. 
Applies to all patches.
should be as below as per chronology
S-o-b:
Cc:
R-b:
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     | 11 +++++--
>   drivers/gpu/drm/i915/i915_drv.h         |  9 ++++--
>   drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>   drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
>   drivers/gpu/drm/i915/i915_irq.c         |  2 +-
>   drivers/gpu/drm/i915/i915_params.c      |  5 ----
>   drivers/gpu/drm/i915/i915_params.h      |  1 -
>   drivers/gpu/drm/i915/intel_guc_loader.c |  7 +++--
>   drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
>   drivers/gpu/drm/i915/intel_uc.c         | 51 +++++++++++++++++----------------
>   drivers/gpu/drm/i915/intel_uc.h         |  2 +-
>   drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
>   12 files changed, 52 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 53e40dd..4fde4b2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2336,8 +2336,10 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
>   	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>   
> -	if (!HAS_HUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)){
> +		seq_puts(m, "not supported\n");
>   		return 0;
> +	}
>   
>   	seq_puts(m, "HuC firmware status:\n");
>   	seq_printf(m, "\tpath: %s\n", huc_fw->path);
> @@ -2369,8 +2371,11 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>   	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>   	u32 tmp, i;
>   
> -	if (!HAS_GUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)){
> +		seq_puts(m, "not supported\n");
>   		return 0;
> +		
> +	}
>   
>   	seq_printf(m, "GuC firmware status:\n");
>   	seq_printf(m, "\tpath: %s\n",
> @@ -2465,7 +2470,7 @@ static bool check_guc_submission(struct seq_file *m)
>   
>   	if (!guc->execbuf_client) {
>   		seq_printf(m, "GuC submission %s\n",
> -			   HAS_GUC_SCHED(dev_priv) ?
> +			   HAS_GUC(dev_priv) ?
>   			   "disabled" :
>   			   "not supported");
>   		return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 61a4be9..6479b72 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3141,9 +3141,12 @@ static inline unsigned int i915_sg_segment_size(void)
>    */
>   #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>   #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> +#define HAS_GUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
Is this typo? Should be (dev_priv)->huc.fw.path
> +
> +#define NEEDS_GUC_LOADING(dev_priv) \
> +	(HAS_GUC(dev_priv) && \
> +	(i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>   
>   #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 921ee36..0890341 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
>   	 * present or not in use we still need a small bias as ring wraparound
>   	 * at offset 0 sometimes hangs. No idea why.
>   	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> +	if (NEEDS_GUC_LOADING(dev_priv))
>   		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>   	else
>   		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 64d7852..a32935a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3292,7 +3292,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
>   	 * currently don't have any bits spare to pass in this upper
>   	 * restriction!
>   	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
> +	if (NEEDS_GUC_LOADING(dev_priv)) {
>   		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>   		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>   	}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6a07ef3..60cdf0d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3956,7 +3956,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>   	for (i = 0; i < MAX_L3_SLICES; ++i)
>   		dev_priv->l3_parity.remap_info[i] = NULL;
>   
> -	if (HAS_GUC_SCHED(dev_priv))
> +	if (NEEDS_GUC_LOADING(dev_priv))
>   		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>   
>   	/* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index ec65341..30344e1 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -63,7 +63,6 @@ struct i915_params i915_modparams __read_mostly = {
>   	.verbose_state_checks = 1,
>   	.nuclear_pageflip = 0,
>   	.edp_vswing = 0,
> -	.enable_guc_loading = 0,
>   	.enable_guc_submission = 0,
>   	.guc_log_level = -1,
>   	.guc_firmware_path = NULL,
> @@ -201,10 +200,6 @@ struct i915_params i915_modparams __read_mostly = {
>   	"(0=use value from vbt [default], 1=low power swing(200mV),"
>   	"2=default swing(400mV))");
>   
> -i915_param_named_unsafe(enable_guc_loading, int, 0400,
> -	"Enable GuC firmware loading "
> -	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> -
>   i915_param_named_unsafe(enable_guc_submission, int, 0400,
>   	"Enable GuC submission "
>   	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index a2cbb47..b26df06 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,6 @@
>   	func(int, disable_power_well); \
>   	func(int, enable_ips); \
>   	func(int, invert_brightness); \
> -	func(int, enable_guc_loading); \
>   	func(int, enable_guc_submission); \
>   	func(int, guc_log_level); \
>   	func(char *, guc_firmware_path); \
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index c9e25be..4210680 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -382,7 +382,7 @@ int intel_guc_init_hw(struct intel_guc *guc)
>    *
>    * Return: zero when we know firmware, non-zero in other case
Need to update the comment.
>    */
> -int intel_guc_select_fw(struct intel_guc *guc)
> +void intel_guc_select_fw(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
> @@ -412,8 +412,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
>   		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
>   		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>   	} else {
> -		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> -		return -ENOENT;
> +		if(HAS_GUC(dev_priv))
> +			DRM_ERROR("NO GUC FW konown for a platform with GuC!\n");
> +		return;
>   	}
>   
>   	return 0;
returning value is not correct
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 6e1779b..ee9e786 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>   		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>   		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>   	} else {
> -		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
> +		/* For now, everything with a GuC also has a HuC */
> +		if (HAS_GUC(dev_priv))
> +			DRM_ERROR("No HuC FW known for a platform with HuC!\n");
>   		return;
>   	}
>   }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 9018540..4290b35 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -63,35 +63,31 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
>   void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>   {
>   	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> +		if (i915_modparams.enable_guc_submission > 0)
>   			DRM_INFO("Ignoring GuC options, no hardware\n");
>   
> -		i915_modparams.enable_guc_loading = 0;
>   		i915_modparams.enable_guc_submission = 0;
>   		return;
>   	}
>   
> -	/* A negative value means "use platform default" */
> -	if (i915_modparams.enable_guc_loading < 0)
> -		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>   
>   	/* Verify firmware version */
> -	if (i915_modparams.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> -
> -		if (intel_guc_select_fw(&dev_priv->guc))
> -			i915_modparams.enable_guc_loading = 0;
> +	if (!HAS_GUC(dev_priv)){
> +		if (i915_modparams.enable_guc_submission > 0){
> +			DRM_INFO("Ignoring GuC submission enable enable, no FW\n");
> +			i915_modparams.enable_guc_submission = 0;
> +			return;
> +		}
>   	}
This is same !HAS_GUC check as the one at the beginning of the function.
>   
>   	/* Can't enable guc submission without guc loaded */
Remove this comment.
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!i915_modparams.enable_guc_submission < 0)
This is incorrect. Add ( braces to specify correct condition. Also 
missing { } till return.
>   		i915_modparams.enable_guc_submission = 0;
> +	return;
>   
>   	/* A negative value means "use platform default" */
>   	if (i915_modparams.enable_guc_submission < 0)
> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +		i915_modparams.enable_guc_submission = 1;
This is wrong. This should be HAS_GUC.
>   }
>   
>   static void gen8_guc_raise_irq(struct intel_guc *guc)
> @@ -106,6 +102,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>   	struct intel_guc *guc = &dev_priv->guc;
>   
>   	intel_guc_ct_init_early(&guc->ct);
> +	intel_guc_select_fw(&dev_priv->guc);
> +	intel_huc_select_fw(&dev_priv->huc);
Need to rebase these w.r.t Michal's latest GuC code reorg series - 
https://patchwork.freedesktop.org/series/31340/
>   
>   	mutex_init(&guc->send_mutex);
>   	guc->send = intel_guc_send_nop;
> @@ -258,6 +256,8 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>   
>   void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>   {
> +	if (!HAS_GUC(dev_priv))
> +		return;
>   	__intel_uc_fw_fini(&dev_priv->guc.fw);
>   	__intel_uc_fw_fini(&dev_priv->huc.fw);
>   }
> @@ -333,7 +333,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	struct intel_guc *guc = &dev_priv->guc;
>   	int ret, attempts;
>   
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>   		return 0;
>   
>   	guc_disable_communication(guc);
> @@ -423,19 +423,20 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	i915_ggtt_disable_guc(dev_priv);
>   
>   	DRM_ERROR("GuC init failed\n");
> -	if (i915_modparams.enable_guc_loading > 1 ||
> -	    i915_modparams.enable_guc_submission > 1)
> +	if (i915_modparams.enable_guc_submission > 1){
> +		DRM_NOTE("GuC is required, so marking the GPU as wedged\n");
>   		ret = -EIO;
> -	else
> -		ret = 0;
> +	
> +	}
>   
> -	if (i915_modparams.enable_guc_submission) {
> -		i915_modparams.enable_guc_submission = 0;
> -		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	else if (i915_modparams.enable_guc_submission == 1){
> +			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +			i915_modparams.enable_guc_submission = 0;
> +			ret = 0;
>   	}
>   
> -	i915_modparams.enable_guc_loading = 0;
> -	DRM_NOTE("GuC firmware loading disabled\n");
> +	else
> +		ret = 0;	
>   
>   	return ret;
>   }
> @@ -444,7 +445,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   {
>   	guc_free_load_err_log(&dev_priv->guc);
>   
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>   		return;
>   
>   	if (i915_modparams.enable_guc_submission)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 7703c9a..58d787e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -223,7 +223,7 @@ static inline void intel_guc_notify(struct intel_guc *guc)
>   }
>   
>   /* intel_guc_loader.c */
> -int intel_guc_select_fw(struct intel_guc *guc);
> +void intel_guc_select_fw(struct intel_guc *guc);
>   int intel_guc_init_hw(struct intel_guc *guc);
>   int intel_guc_suspend(struct drm_i915_private *dev_priv);
>   int intel_guc_resume(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b3c3f94..0164f41 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1786,12 +1786,10 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
>   {
>   	int ret;
>   
> -	if (!HAS_GUC(dev_priv))
> -		return -EINVAL;
> +	GEM_BUG_ON(!HAS_GUC(dev_priv));
>   
>   	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>   	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>   
>   	return ret;
>   }

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

Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Michal Wajdeczko]

On Wed, 04 Oct 2017 00:56:18 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> We currently have two module parameters that control GuC:  
> "enable_guc_loading" and "enable_guc_submission".
> Whenever we need i915_modparams.enable_guc_submission=1, we also need  
> enable_guc_loading=1.
> We also need enable_guc_loading=1 when we want to verify the HuC,
> which is every time we have a HuC (but all platforms with HuC have a GuC  
> and viceversa).
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 11 +++++--
>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++--
>  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c         |  2 +-
>  drivers/gpu/drm/i915/i915_params.c      |  5 ----
>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>  drivers/gpu/drm/i915/intel_guc_loader.c |  7 +++--
>  drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
>  drivers/gpu/drm/i915/intel_uc.c         | 51  
> +++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_uc.h         |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
>  12 files changed, 52 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 53e40dd..4fde4b2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2336,8 +2336,10 @@ static int i915_huc_load_status_info(struct  
> seq_file *m, void *data)
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> -	if (!HAS_HUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)){
> +		seq_puts(m, "not supported\n");

This line should be added in patch 1/5

>  		return 0;
> +	}
> 	seq_puts(m, "HuC firmware status:\n");
>  	seq_printf(m, "\tpath: %s\n", huc_fw->path);
> @@ -2369,8 +2371,11 @@ static int i915_guc_load_status_info(struct  
> seq_file *m, void *data)
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	u32 tmp, i;
> -	if (!HAS_GUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)){
> +		seq_puts(m, "not supported\n");

Same here.

>  		return 0;
> +		
> +	}
> 	seq_printf(m, "GuC firmware status:\n");
>  	seq_printf(m, "\tpath: %s\n",
> @@ -2465,7 +2470,7 @@ static bool check_guc_submission(struct seq_file  
> *m)
> 	if (!guc->execbuf_client) {
>  		seq_printf(m, "GuC submission %s\n",
> -			   HAS_GUC_SCHED(dev_priv) ?
> +			   HAS_GUC(dev_priv) ?
>  			   "disabled" :
>  			   "not supported");
>  		return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index 61a4be9..6479b72 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3141,9 +3141,12 @@ static inline unsigned int  
> i915_sg_segment_size(void)
>   */
>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>  #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> +#define HAS_GUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
                                                      ^^^
huc ?

> +
> +#define NEEDS_GUC_LOADING(dev_priv) \
> +	(HAS_GUC(dev_priv) && \
> +	(i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
> #define HAS_RESOURCE_STREAMER(dev_priv)  
> ((dev_priv)->info.has_resource_streamer)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c  
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 921ee36..0890341 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct  
> drm_i915_private *i915,
>  	 * present or not in use we still need a small bias as ring wraparound
>  	 * at offset 0 sometimes hangs. No idea why.
>  	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> +	if (NEEDS_GUC_LOADING(dev_priv))
>  		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>  	else
>  		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 64d7852..a32935a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3292,7 +3292,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private  
> *dev_priv)
>  	 * currently don't have any bits spare to pass in this upper
>  	 * restriction!
>  	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
> +	if (NEEDS_GUC_LOADING(dev_priv)) {
>  		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>  		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index 6a07ef3..60cdf0d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3956,7 +3956,7 @@ void intel_irq_init(struct drm_i915_private  
> *dev_priv)
>  	for (i = 0; i < MAX_L3_SLICES; ++i)
>  		dev_priv->l3_parity.remap_info[i] = NULL;
> -	if (HAS_GUC_SCHED(dev_priv))
> +	if (NEEDS_GUC_LOADING(dev_priv))
>  		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
> 	/* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_params.c  
> b/drivers/gpu/drm/i915/i915_params.c
> index ec65341..30344e1 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -63,7 +63,6 @@ struct i915_params i915_modparams __read_mostly = {
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
>  	.edp_vswing = 0,
> -	.enable_guc_loading = 0,

Needs rebase

>  	.enable_guc_submission = 0,
>  	.guc_log_level = -1,
>  	.guc_firmware_path = NULL,
> @@ -201,10 +200,6 @@ struct i915_params i915_modparams __read_mostly = {
>  	"(0=use value from vbt [default], 1=low power swing(200mV),"
>  	"2=default swing(400mV))");
> -i915_param_named_unsafe(enable_guc_loading, int, 0400,
> -	"Enable GuC firmware loading "
> -	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> -
>  i915_param_named_unsafe(enable_guc_submission, int, 0400,
>  	"Enable GuC submission "
>  	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h  
> b/drivers/gpu/drm/i915/i915_params.h
> index a2cbb47..b26df06 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,6 @@
>  	func(int, disable_power_well); \
>  	func(int, enable_ips); \
>  	func(int, invert_brightness); \
> -	func(int, enable_guc_loading); \

Needs rebase

>  	func(int, enable_guc_submission); \
>  	func(int, guc_log_level); \
>  	func(char *, guc_firmware_path); \
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c  
> b/drivers/gpu/drm/i915/intel_guc_loader.c
> index c9e25be..4210680 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -382,7 +382,7 @@ int intel_guc_init_hw(struct intel_guc *guc)
>   *
>   * Return: zero when we know firmware, non-zero in other case
>   */
> -int intel_guc_select_fw(struct intel_guc *guc)
> +void intel_guc_select_fw(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -412,8 +412,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
>  		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
>  		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>  	} else {
> -		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> -		return -ENOENT;
> +		if(HAS_GUC(dev_priv))
> +			DRM_ERROR("NO GUC FW konown for a platform with GuC!\n");
> +		return;
>  	}
> 	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> b/drivers/gpu/drm/i915/intel_huc.c
> index 6e1779b..ee9e786 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>  		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>  		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>  	} else {
> -		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
> +		/* For now, everything with a GuC also has a HuC */

Hmm, maybe above shall be added to
#define HAS_HUC(dev_priv) HAS_GUC(dev_priv)

> +		if (HAS_GUC(dev_priv))

And then use here HAS_HUC() ?

> +			DRM_ERROR("No HuC FW known for a platform with HuC!\n");
>  		return;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 9018540..4290b35 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -63,35 +63,31 @@ static int __intel_uc_reset_hw(struct  
> drm_i915_private *dev_priv)
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> +		if (i915_modparams.enable_guc_submission > 0)
>  			DRM_INFO("Ignoring GuC options, no hardware\n");
> -		i915_modparams.enable_guc_loading = 0;
>  		i915_modparams.enable_guc_submission = 0;
>  		return;
>  	}
> -	/* A negative value means "use platform default" */
> -	if (i915_modparams.enable_guc_loading < 0)
> -		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> 	/* Verify firmware version */
> -	if (i915_modparams.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> -
> -		if (intel_guc_select_fw(&dev_priv->guc))
> -			i915_modparams.enable_guc_loading = 0;
> +	if (!HAS_GUC(dev_priv)){

Unreachable code

> +		if (i915_modparams.enable_guc_submission > 0){
> +			DRM_INFO("Ignoring GuC submission enable enable, no FW\n");
> +			i915_modparams.enable_guc_submission = 0;
> +			return;
> +		}
>  	}
> 	/* Can't enable guc submission without guc loaded */
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!i915_modparams.enable_guc_submission < 0)
>  		i915_modparams.enable_guc_submission = 0;
> +	return;
> 	/* A negative value means "use platform default" */
>  	if (i915_modparams.enable_guc_submission < 0)
> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +		i915_modparams.enable_guc_submission = 1;
>  }
> static void gen8_guc_raise_irq(struct intel_guc *guc)
> @@ -106,6 +102,8 @@ void intel_uc_init_early(struct drm_i915_private  
> *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
> 	intel_guc_ct_init_early(&guc->ct);

Maybe following lines shall be guarded with HAS_GUC/HAS_HUC
like you did in fini_fw ?

> +	intel_guc_select_fw(&dev_priv->guc);
> +	intel_huc_select_fw(&dev_priv->huc);

Btw, note that we already has "guc" local variable ...

> 	mutex_init(&guc->send_mutex);
>  	guc->send = intel_guc_send_nop;
> @@ -258,6 +256,8 @@ void intel_uc_init_fw(struct drm_i915_private  
> *dev_priv)
> void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  {
> +	if (!HAS_GUC(dev_priv))
> +		return;
>  	__intel_uc_fw_fini(&dev_priv->guc.fw);
>  	__intel_uc_fw_fini(&dev_priv->huc.fw);
>  }
> @@ -333,7 +333,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
>  	int ret, attempts;
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>  		return 0;
> 	guc_disable_communication(guc);
> @@ -423,19 +423,20 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	i915_ggtt_disable_guc(dev_priv);
> 	DRM_ERROR("GuC init failed\n");
> -	if (i915_modparams.enable_guc_loading > 1 ||
> -	    i915_modparams.enable_guc_submission > 1)
> +	if (i915_modparams.enable_guc_submission > 1){
> +		DRM_NOTE("GuC is required, so marking the GPU as wedged\n");
>  		ret = -EIO;
> -	else
> -		ret = 0;
> +	
> +	}
> -	if (i915_modparams.enable_guc_submission) {
> -		i915_modparams.enable_guc_submission = 0;
> -		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	else if (i915_modparams.enable_guc_submission == 1){
> +			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +			i915_modparams.enable_guc_submission = 0;
> +			ret = 0;
>  	}
> -	i915_modparams.enable_guc_loading = 0;
> -	DRM_NOTE("GuC firmware loading disabled\n");
> +	else
> +		ret = 0;	
> 	return ret;
>  }
> @@ -444,7 +445,7 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  {
>  	guc_free_load_err_log(&dev_priv->guc);
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>  		return;
> 	if (i915_modparams.enable_guc_submission)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index 7703c9a..58d787e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -223,7 +223,7 @@ static inline void intel_guc_notify(struct intel_guc  
> *guc)
>  }
> /* intel_guc_loader.c */
> -int intel_guc_select_fw(struct intel_guc *guc);
> +void intel_guc_select_fw(struct intel_guc *guc);
>  int intel_guc_init_hw(struct intel_guc *guc);
>  int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c  
> b/drivers/gpu/drm/i915/intel_uncore.c
> index b3c3f94..0164f41 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1786,12 +1786,10 @@ int intel_guc_reset(struct drm_i915_private  
> *dev_priv)
>  {
>  	int ret;
> -	if (!HAS_GUC(dev_priv))
> -		return -EINVAL;
> +	GEM_BUG_ON(!HAS_GUC(dev_priv));
> 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> 	return ret;
>  }

btw, beware of https://patchwork.freedesktop.org/series/31340/

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

Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Joonas Lahtinen]

On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
> We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
> Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
> We also need enable_guc_loading=1 when we want to verify the HuC,
> which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).

Long lines in commit message, please give a look at:

https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html

Section "14) The canonical patch format".

Then, about the patch. I think the commit message should be more clear
about the fact that if we have HuC firmware to be loaded, we need to
have GuC to actually load it. So if an user wants to avoid the GuC from
getting loaded, they must not have a HuC firmware to be loaded, in
addition to not using GuC submission.

> 
> v2: Clarifying the commit message (Anusha)
> 
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> 
> v4: Rebase
> 
> v5: Separating message unification into a separate patch
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>

Try to keep the tags in chronological order, so start with Suggested-
by: (if any), Signed-off-by:, Cc: and so on.

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 v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Sujaritha]

On 10/03/2017 11:45 PM, Sagar Arun Kamble wrote:
>
> Subject is missing "parameter" in the end. Either keep module 
> parameter or i915_modparams.
Will fix the subject line.
>
>
> On 10/4/2017 4:26 AM, Sujaritha Sundaresan wrote:
>> We currently have two module parameters that control GuC: 
>> "enable_guc_loading" and "enable_guc_submission".
>> Whenever we need i915_modparams.enable_guc_submission=1, we also need 
>> enable_guc_loading=1.
>> We also need enable_guc_loading=1 when we want to verify the HuC,
>> which is every time we have a HuC (but all platforms with HuC have a 
>> GuC and viceversa).
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating message unification into a separate patch
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Need to change the order of the tag to comply with new convention. 
> Applies to all patches.
> should be as below as per chronology
> S-o-b:
> Cc:
> R-b:
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c     | 11 +++++--
>>   drivers/gpu/drm/i915/i915_drv.h         |  9 ++++--
>>   drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>>   drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
>>   drivers/gpu/drm/i915/i915_irq.c         |  2 +-
>>   drivers/gpu/drm/i915/i915_params.c      |  5 ----
>>   drivers/gpu/drm/i915/i915_params.h      |  1 -
>>   drivers/gpu/drm/i915/intel_guc_loader.c |  7 +++--
>>   drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
>>   drivers/gpu/drm/i915/intel_uc.c         | 51 
>> +++++++++++++++++----------------
>>   drivers/gpu/drm/i915/intel_uc.h         |  2 +-
>>   drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
>>   12 files changed, 52 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 53e40dd..4fde4b2 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2336,8 +2336,10 @@ static int i915_huc_load_status_info(struct 
>> seq_file *m, void *data)
>>       struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>       struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>   -    if (!HAS_HUC_UCODE(dev_priv))
>> +    if (!HAS_GUC(dev_priv)){
>> +        seq_puts(m, "not supported\n");
>>           return 0;
>> +    }
>>         seq_puts(m, "HuC firmware status:\n");
>>       seq_printf(m, "\tpath: %s\n", huc_fw->path);
>> @@ -2369,8 +2371,11 @@ static int i915_guc_load_status_info(struct 
>> seq_file *m, void *data)
>>       struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>>       u32 tmp, i;
>>   -    if (!HAS_GUC_UCODE(dev_priv))
>> +    if (!HAS_GUC(dev_priv)){
>> +        seq_puts(m, "not supported\n");
>>           return 0;
>> +
>> +    }
>>         seq_printf(m, "GuC firmware status:\n");
>>       seq_printf(m, "\tpath: %s\n",
>> @@ -2465,7 +2470,7 @@ static bool check_guc_submission(struct 
>> seq_file *m)
>>         if (!guc->execbuf_client) {
>>           seq_printf(m, "GuC submission %s\n",
>> -               HAS_GUC_SCHED(dev_priv) ?
>> +               HAS_GUC(dev_priv) ?
>>                  "disabled" :
>>                  "not supported");
>>           return false;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 61a4be9..6479b72 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3141,9 +3141,12 @@ static inline unsigned int 
>> i915_sg_segment_size(void)
>>    */
>>   #define HAS_GUC(dev_priv)    ((dev_priv)->info.has_guc)
>>   #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
>> -#define HAS_GUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
>> -#define HAS_GUC_SCHED(dev_priv)    (HAS_GUC(dev_priv))
>> -#define HAS_HUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
>> +#define HAS_GUC_UCODE(dev_priv)    ((dev_priv)->guc.fw.path != NULL)
>> +#define HAS_HUC_UCODE(dev_priv)    ((dev_priv)->guc.fw.path != NULL)
> Is this typo? Should be (dev_priv)->huc.fw.path

Yes this is a typo, I will fix this in the revision.
>> +
>> +#define NEEDS_GUC_LOADING(dev_priv) \
>> +    (HAS_GUC(dev_priv) && \
>> +    (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>>     #define HAS_RESOURCE_STREAMER(dev_priv) 
>> ((dev_priv)->info.has_resource_streamer)
>>   diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 921ee36..0890341 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct 
>> drm_i915_private *i915,
>>        * present or not in use we still need a small bias as ring 
>> wraparound
>>        * at offset 0 sometimes hangs. No idea why.
>>        */
>> -    if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
>> +    if (NEEDS_GUC_LOADING(dev_priv))
>>           ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>>       else
>>           ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 64d7852..a32935a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3292,7 +3292,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private 
>> *dev_priv)
>>        * currently don't have any bits spare to pass in this upper
>>        * restriction!
>>        */
>> -    if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
>> +    if (NEEDS_GUC_LOADING(dev_priv)) {
>>           ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>>           ggtt->mappable_end = min(ggtt->mappable_end, 
>> ggtt->base.total);
>>       }
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 6a07ef3..60cdf0d 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3956,7 +3956,7 @@ void intel_irq_init(struct drm_i915_private 
>> *dev_priv)
>>       for (i = 0; i < MAX_L3_SLICES; ++i)
>>           dev_priv->l3_parity.remap_info[i] = NULL;
>>   -    if (HAS_GUC_SCHED(dev_priv))
>> +    if (NEEDS_GUC_LOADING(dev_priv))
>>           dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>>         /* Let's track the enabled rps events */
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index ec65341..30344e1 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -63,7 +63,6 @@ struct i915_params i915_modparams __read_mostly = {
>>       .verbose_state_checks = 1,
>>       .nuclear_pageflip = 0,
>>       .edp_vswing = 0,
>> -    .enable_guc_loading = 0,
>>       .enable_guc_submission = 0,
>>       .guc_log_level = -1,
>>       .guc_firmware_path = NULL,
>> @@ -201,10 +200,6 @@ struct i915_params i915_modparams __read_mostly = {
>>       "(0=use value from vbt [default], 1=low power swing(200mV),"
>>       "2=default swing(400mV))");
>>   -i915_param_named_unsafe(enable_guc_loading, int, 0400,
>> -    "Enable GuC firmware loading "
>> -    "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> -
>>   i915_param_named_unsafe(enable_guc_submission, int, 0400,
>>       "Enable GuC submission "
>>       "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>> b/drivers/gpu/drm/i915/i915_params.h
>> index a2cbb47..b26df06 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,6 @@
>>       func(int, disable_power_well); \
>>       func(int, enable_ips); \
>>       func(int, invert_brightness); \
>> -    func(int, enable_guc_loading); \
>>       func(int, enable_guc_submission); \
>>       func(int, guc_log_level); \
>>       func(char *, guc_firmware_path); \
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index c9e25be..4210680 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -382,7 +382,7 @@ int intel_guc_init_hw(struct intel_guc *guc)
>>    *
>>    * Return: zero when we know firmware, non-zero in other case
> Need to update the comment.
Will do.
>>    */
>> -int intel_guc_select_fw(struct intel_guc *guc)
>> +void intel_guc_select_fw(struct intel_guc *guc)
>>   {
>>       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>   @@ -412,8 +412,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
>>           guc->fw.major_ver_wanted = GLK_FW_MAJOR;
>>           guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>>       } else {
>> -        DRM_ERROR("No GuC firmware known for platform with GuC!\n");
>> -        return -ENOENT;
>> +        if(HAS_GUC(dev_priv))
>> +            DRM_ERROR("NO GUC FW konown for a platform with GuC!\n");
>> +        return;
>>       }
>>         return 0;
> returning value is not correct

Will fix the return value in the revised patch.
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c 
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 6e1779b..ee9e786 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>>           huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>>           huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>>       } else {
>> -        DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>> +        /* For now, everything with a GuC also has a HuC */
>> +        if (HAS_GUC(dev_priv))
>> +            DRM_ERROR("No HuC FW known for a platform with HuC!\n");
>>           return;
>>       }
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 9018540..4290b35 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -63,35 +63,31 @@ static int __intel_uc_reset_hw(struct 
>> drm_i915_private *dev_priv)
>>   void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>>   {
>>       if (!HAS_GUC(dev_priv)) {
>> -        if (i915_modparams.enable_guc_loading > 0 ||
>> -            i915_modparams.enable_guc_submission > 0)
>> +        if (i915_modparams.enable_guc_submission > 0)
>>               DRM_INFO("Ignoring GuC options, no hardware\n");
>>   -        i915_modparams.enable_guc_loading = 0;
>>           i915_modparams.enable_guc_submission = 0;
>>           return;
>>       }
>>   -    /* A negative value means "use platform default" */
>> -    if (i915_modparams.enable_guc_loading < 0)
>> -        i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>>         /* Verify firmware version */
>> -    if (i915_modparams.enable_guc_loading) {
>> -        if (HAS_HUC_UCODE(dev_priv))
>> -            intel_huc_select_fw(&dev_priv->huc);
>> -
>> -        if (intel_guc_select_fw(&dev_priv->guc))
>> -            i915_modparams.enable_guc_loading = 0;
>> +    if (!HAS_GUC(dev_priv)){
>> +        if (i915_modparams.enable_guc_submission > 0){
>> +            DRM_INFO("Ignoring GuC submission enable enable, no FW\n");
>> +            i915_modparams.enable_guc_submission = 0;
>> +            return;
>> +        }
>>       }
> This is same !HAS_GUC check as the one at the beginning of the function.
Will fix this error.
>>         /* Can't enable guc submission without guc loaded */
> Remove this comment.

Will do
>> -    if (!i915_modparams.enable_guc_loading)
>> +    if (!i915_modparams.enable_guc_submission < 0)
> This is incorrect. Add ( braces to specify correct condition. Also 
> missing { } till return.
>> i915_modparams.enable_guc_submission = 0;
>> +    return;
>>         /* A negative value means "use platform default" */
>>       if (i915_modparams.enable_guc_submission < 0)
>> -        i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>> +        i915_modparams.enable_guc_submission = 1;
> This is wrong. This should be HAS_GUC.

Will fix this.
>>   }
>>     static void gen8_guc_raise_irq(struct intel_guc *guc)
>> @@ -106,6 +102,8 @@ void intel_uc_init_early(struct drm_i915_private 
>> *dev_priv)
>>       struct intel_guc *guc = &dev_priv->guc;
>>         intel_guc_ct_init_early(&guc->ct);
>> +    intel_guc_select_fw(&dev_priv->guc);
>> +    intel_huc_select_fw(&dev_priv->huc);
> Need to rebase these w.r.t Michal's latest GuC code reorg series - 
> https://patchwork.freedesktop.org/series/31340/

Will rebase the revised patches.
>> mutex_init(&guc->send_mutex);
>>       guc->send = intel_guc_send_nop;
>> @@ -258,6 +256,8 @@ void intel_uc_init_fw(struct drm_i915_private 
>> *dev_priv)
>>     void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>>   {
>> +    if (!HAS_GUC(dev_priv))
>> +        return;
>>       __intel_uc_fw_fini(&dev_priv->guc.fw);
>>       __intel_uc_fw_fini(&dev_priv->huc.fw);
>>   }
>> @@ -333,7 +333,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>       struct intel_guc *guc = &dev_priv->guc;
>>       int ret, attempts;
>>   -    if (!i915_modparams.enable_guc_loading)
>> +    if (!NEEDS_GUC_LOADING(dev_priv))
>>           return 0;
>>         guc_disable_communication(guc);
>> @@ -423,19 +423,20 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>       i915_ggtt_disable_guc(dev_priv);
>>         DRM_ERROR("GuC init failed\n");
>> -    if (i915_modparams.enable_guc_loading > 1 ||
>> -        i915_modparams.enable_guc_submission > 1)
>> +    if (i915_modparams.enable_guc_submission > 1){
>> +        DRM_NOTE("GuC is required, so marking the GPU as wedged\n");
>>           ret = -EIO;
>> -    else
>> -        ret = 0;
>> +
>> +    }
>>   -    if (i915_modparams.enable_guc_submission) {
>> -        i915_modparams.enable_guc_submission = 0;
>> -        DRM_NOTE("Falling back from GuC submission to execlist 
>> mode\n");
>> +    else if (i915_modparams.enable_guc_submission == 1){
>> +            DRM_NOTE("Falling back from GuC submission to execlist 
>> mode\n");
>> +            i915_modparams.enable_guc_submission = 0;
>> +            ret = 0;
>>       }
>>   -    i915_modparams.enable_guc_loading = 0;
>> -    DRM_NOTE("GuC firmware loading disabled\n");
>> +    else
>> +        ret = 0;
>>         return ret;
>>   }
>> @@ -444,7 +445,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>   {
>>       guc_free_load_err_log(&dev_priv->guc);
>>   -    if (!i915_modparams.enable_guc_loading)
>> +    if (!NEEDS_GUC_LOADING(dev_priv))
>>           return;
>>         if (i915_modparams.enable_guc_submission)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 7703c9a..58d787e 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -223,7 +223,7 @@ static inline void intel_guc_notify(struct 
>> intel_guc *guc)
>>   }
>>     /* intel_guc_loader.c */
>> -int intel_guc_select_fw(struct intel_guc *guc);
>> +void intel_guc_select_fw(struct intel_guc *guc);
>>   int intel_guc_init_hw(struct intel_guc *guc);
>>   int intel_guc_suspend(struct drm_i915_private *dev_priv);
>>   int intel_guc_resume(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index b3c3f94..0164f41 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1786,12 +1786,10 @@ int intel_guc_reset(struct drm_i915_private 
>> *dev_priv)
>>   {
>>       int ret;
>>   -    if (!HAS_GUC(dev_priv))
>> -        return -EINVAL;
>> +    GEM_BUG_ON(!HAS_GUC(dev_priv));
>>         intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>       ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
>> -    intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>         return ret;
>>   }
>
Thanks for the review.

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

Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Rodrigo Vivi]

On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
>> We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
>> Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
>> We also need enable_guc_loading=1 when we want to verify the HuC,
>> which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
>
> Long lines in commit message, please give a look at:
>
> https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
>
> Section "14) The canonical patch format".
>
> Then, about the patch. I think the commit message should be more clear
> about the fact that if we have HuC firmware to be loaded, we need to
> have GuC to actually load it. So if an user wants to avoid the GuC from
> getting loaded, they must not have a HuC firmware to be loaded, in
> addition to not using GuC submission.
>
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating message unification into a separate patch
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>
> Try to keep the tags in chronological order, so start with Suggested-
> by: (if any), Signed-off-by:, Cc: and so on.

Could we agree on have
Suggested-by:
Cc:
Signed-off-by:
as the initial chronological order and then follow the chronological
after that as well?

Few reasons for that:
1. For my brain this is the regular chronological message flow:
someone suggested, you message some one and last thing you do before
sending the message is to sign-off.
2. git commit --amend -s adds it to the end.
3. Signed-off-by: at the end of the message was always our standard
and every patch that I see around in the kernel seems to prefer this
style
4. When I look to the first email and I see cc below the first thing
that I think is: "This developer forgot to sign-off his own patch!".

Thanks,
Rodrigo.

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



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Chris Wilson]

Quoting Rodrigo Vivi (2017-11-02 23:52:45)
> On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
> > On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
> >> We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
> >> Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
> >> We also need enable_guc_loading=1 when we want to verify the HuC,
> >> which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
> >
> > Long lines in commit message, please give a look at:
> >
> > https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
> >
> > Section "14) The canonical patch format".
> >
> > Then, about the patch. I think the commit message should be more clear
> > about the fact that if we have HuC firmware to be loaded, we need to
> > have GuC to actually load it. So if an user wants to avoid the GuC from
> > getting loaded, they must not have a HuC firmware to be loaded, in
> > addition to not using GuC submission.
> >
> >>
> >> v2: Clarifying the commit message (Anusha)
> >>
> >> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> >>
> >> v4: Rebase
> >>
> >> v5: Separating message unification into a separate patch
> >>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> Cc: Oscar Mateo <oscar.mateo@intel.com>
> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> >
> > Try to keep the tags in chronological order, so start with Suggested-
> > by: (if any), Signed-off-by:, Cc: and so on.
> 
> Could we agree on have
> Suggested-by:
> Cc:
> Signed-off-by:
> as the initial chronological order and then follow the chronological

But CCs come after a s-o-b, because they are added after the commit. (I
write some code, then think who might be interested; usually by looking
at who previously worked on the same code). Then you also add new CCs
later on based on review feedback; a comment on v1 gets a CC on v2.
Bugzilla/reported-by/suggested-by are before since they presumably
prompted the commit to be written in the first place (plus also they
deserve extra credit for their effort in alerting us to the issue).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Joonas Lahtinen]

On Fri, 2017-11-03 at 00:03 +0000, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2017-11-02 23:52:45)
> > On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com> wrote:
> > > On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
> > > > We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
> > > > Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
> > > > We also need enable_guc_loading=1 when we want to verify the HuC,
> > > > which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
> > > 
> > > Long lines in commit message, please give a look at:
> > > 
> > > https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
> > > 
> > > Section "14) The canonical patch format".
> > > 
> > > Then, about the patch. I think the commit message should be more clear
> > > about the fact that if we have HuC firmware to be loaded, we need to
> > > have GuC to actually load it. So if an user wants to avoid the GuC from
> > > getting loaded, they must not have a HuC firmware to be loaded, in
> > > addition to not using GuC submission.
> > > 
> > > > 
> > > > v2: Clarifying the commit message (Anusha)
> > > > 
> > > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> > > > 
> > > > v4: Rebase
> > > > 
> > > > v5: Separating message unification into a separate patch
> > > > 
> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > 
> > > Try to keep the tags in chronological order, so start with Suggested-
> > > by: (if any), Signed-off-by:, Cc: and so on.
> > 
> > Could we agree on have
> > Suggested-by:
> > Cc:
> > Signed-off-by:
> > as the initial chronological order and then follow the chronological
> 
> But CCs come after a s-o-b, because they are added after the commit. (I
> write some code, then think who might be interested; usually by looking
> at who previously worked on the same code). Then you also add new CCs
> later on based on review feedback; a comment on v1 gets a CC on v2.
> Bugzilla/reported-by/suggested-by are before since they presumably
> prompted the commit to be written in the first place (plus also they
> deserve extra credit for their effort in alerting us to the issue).

Yeah, this is my reasoning too.

Also, when you add the machine assistance from Patchwork to
automatically spread tags from the cover letter (Acked-by, Reviewed-by
etc. and it's in the works, I understand). I don't quite see why we
would have only a portion of the tags in chronological order.

If I respin a patch, it might already have:

Bugzilla:
Suggested-by:
Signed-off-by:
Cc:
Cc:
Acked-by:
Reviewed-by:

By adding my Signed-off-by at the end and that's the only way to retain
that history information correctly.

And it's an easy convention to follow for a developer. You only need to
to write above the automatically generated S-o-b, if you reference a
bug or attribute credit (because that's literally what happened first
in chronological order, too). From then on, you just append at the end.

All the minutes spent thinking how to correctly order the tags can be
recouped as moar patches.

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 v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module

[Rodrigo Vivi]

On Fri, Nov 03, 2017 at 08:36:01AM +0000, Joonas Lahtinen wrote:
> On Fri, 2017-11-03 at 00:03 +0000, Chris Wilson wrote:
> > Quoting Rodrigo Vivi (2017-11-02 23:52:45)
> > > On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
> > > <joonas.lahtinen@linux.intel.com> wrote:
> > > > On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
> > > > > We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
> > > > > Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
> > > > > We also need enable_guc_loading=1 when we want to verify the HuC,
> > > > > which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
> > > > 
> > > > Long lines in commit message, please give a look at:
> > > > 
> > > > https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
> > > > 
> > > > Section "14) The canonical patch format".
> > > > 
> > > > Then, about the patch. I think the commit message should be more clear
> > > > about the fact that if we have HuC firmware to be loaded, we need to
> > > > have GuC to actually load it. So if an user wants to avoid the GuC from
> > > > getting loaded, they must not have a HuC firmware to be loaded, in
> > > > addition to not using GuC submission.
> > > > 
> > > > > 
> > > > > v2: Clarifying the commit message (Anusha)
> > > > > 
> > > > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> > > > > 
> > > > > v4: Rebase
> > > > > 
> > > > > v5: Separating message unification into a separate patch
> > > > > 
> > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > > 
> > > > Try to keep the tags in chronological order, so start with Suggested-
> > > > by: (if any), Signed-off-by:, Cc: and so on.
> > > 
> > > Could we agree on have
> > > Suggested-by:
> > > Cc:
> > > Signed-off-by:
> > > as the initial chronological order and then follow the chronological
> > 
> > But CCs come after a s-o-b, because they are added after the commit. (I
> > write some code, then think who might be interested; usually by looking
> > at who previously worked on the same code). Then you also add new CCs
> > later on based on review feedback; a comment on v1 gets a CC on v2.
> > Bugzilla/reported-by/suggested-by are before since they presumably
> > prompted the commit to be written in the first place (plus also they
> > deserve extra credit for their effort in alerting us to the issue).
> 
> Yeah, this is my reasoning too.

So it seems the chronological order differs from case to case
from person to person.
When I write a patch most of the times I have people in mind
that I will cc. Like when I'm writing an email.
cc: people that touch this code from last time
cc: people that can help on review
cc: people that introduced this error
cc: people that will be futurely impacted by this change

and then I sign-off on the end of the patch as I sign off in the
end of a message.

> 
> Also, when you add the machine assistance from Patchwork to
> automatically spread tags from the cover letter (Acked-by, Reviewed-by
> etc. and it's in the works, I understand). I don't quite see why we
> would have only a portion of the tags in chronological order.
> 
> If I respin a patch, it might already have:
> 
> Bugzilla:
> Suggested-by:
> Signed-off-by:
> Cc:
> Cc:
> Acked-by:
> Reviewed-by:

I really would like to have something like:

Bugzilla:
Suggested-by:
Cc:
Cc:
Signed-off-by:
Acked-by:
Reviewed-by:

This seems to be the most used in kernel.
the most intuitive and the easier to read.

The worst case this approach is creating is

Signed-off:
Cc:
Cc:
Cc:

really ugly on the first patch imho.

So, I doubt we can reach to an agreement. So let's
agree at least in not enforce this chronological thing
as a rule and let people use what ever they feel better.

Specially because I don't see any other place where
this is trying to get enforced like this.

Thanks
Signed-off: Rodrigo.

> 
> By adding my Signed-off-by at the end and that's the only way to retain
> that history information correctly.
> 
> And it's an easy convention to follow for a developer. You only need to
> to write above the automatically generated S-o-b, if you reference a
> bug or attribute credit (because that's literally what happened first
> in chronological order, too). From then on, you just append at the end.
> 
> All the minutes spent thinking how to correctly order the tags can be
> recouped as moar patches.
> 
> 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 tag ordering (Was: Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module)

[Joonas Lahtinen]

+ Jani (and Daniel as emeritus maintainer)

On Fri, 2017-11-03 at 10:08 -0700, Rodrigo Vivi wrote:
> On Fri, Nov 03, 2017 at 08:36:01AM +0000, Joonas Lahtinen wrote:
> > On Fri, 2017-11-03 at 00:03 +0000, Chris Wilson wrote:
> > > Quoting Rodrigo Vivi (2017-11-02 23:52:45)
> > > > On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
> > > > <joonas.lahtinen@linux.intel.com> wrote:
> > > > > On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
> > > > > > We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
> > > > > > Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
> > > > > > We also need enable_guc_loading=1 when we want to verify the HuC,
> > > > > > which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
> > > > > 
> > > > > Long lines in commit message, please give a look at:
> > > > > 
> > > > > https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
> > > > > 
> > > > > Section "14) The canonical patch format".
> > > > > 
> > > > > Then, about the patch. I think the commit message should be more clear
> > > > > about the fact that if we have HuC firmware to be loaded, we need to
> > > > > have GuC to actually load it. So if an user wants to avoid the GuC from
> > > > > getting loaded, they must not have a HuC firmware to be loaded, in
> > > > > addition to not using GuC submission.
> > > > > 
> > > > > > 
> > > > > > v2: Clarifying the commit message (Anusha)
> > > > > > 
> > > > > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> > > > > > 
> > > > > > v4: Rebase
> > > > > > 
> > > > > > v5: Separating message unification into a separate patch
> > > > > > 
> > > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > > > 
> > > > > Try to keep the tags in chronological order, so start with Suggested-
> > > > > by: (if any), Signed-off-by:, Cc: and so on.
> > > > 
> > > > Could we agree on have
> > > > Suggested-by:
> > > > Cc:
> > > > Signed-off-by:
> > > > as the initial chronological order and then follow the chronological
> > > 
> > > But CCs come after a s-o-b, because they are added after the commit. (I
> > > write some code, then think who might be interested; usually by looking
> > > at who previously worked on the same code). Then you also add new CCs
> > > later on based on review feedback; a comment on v1 gets a CC on v2.
> > > Bugzilla/reported-by/suggested-by are before since they presumably
> > > prompted the commit to be written in the first place (plus also they
> > > deserve extra credit for their effort in alerting us to the issue).
> > 
> > Yeah, this is my reasoning too.
> 
> So it seems the chronological order differs from case to case
> from person to person.
> When I write a patch most of the times I have people in mind
> that I will cc. Like when I'm writing an email.
> cc: people that touch this code from last time
> cc: people that can help on review
> cc: people that introduced this error
> cc: people that will be futurely impacted by this change

I don't follow this logic. Most of the Cc:s are chosen based on what
the code does, in get_maintainers.pl fashion. I think one is set to
implement a feature/bugfix, and it is not necessarily certain in the
beginning where the code will land, could be core kernel, DRM or i915
even. And Cc:s will vary accordingly depending on where the code
landed.

I can see an argument for some Cc:s before Signed-off-by:, but I never
claimed that wouldn't be the case. Just that chronological ordering
makes sense (will be easy to automate, too).

I would claim that all of the four points you listed, you'll be looking
at git blame based on what the code you wrote changed. And you don't
know the whole scope of code in advance except for really small fixes.
Where you wrote the code in your mind already. And it's really the IP
you're S-o-b:ing. But this gets pretty theoritical already.

> and then I sign-off on the end of the patch as I sign off in the
> end of a message.
> 
> > 
> > Also, when you add the machine assistance from Patchwork to
> > automatically spread tags from the cover letter (Acked-by, Reviewed-by
> > etc. and it's in the works, I understand). I don't quite see why we
> > would have only a portion of the tags in chronological order.
> > 
> > If I respin a patch, it might already have:
> > 
> > Bugzilla:
> > Suggested-by:
> > Signed-off-by:
> > Cc:
> > Cc:
> > Acked-by:
> > Reviewed-by:
> 
> I really would like to have something like:
> 
> Bugzilla:
> Suggested-by:
> Cc:
> Cc:
> Signed-off-by:
> Acked-by:
> Reviewed-by:
> 
> This seems to be the most used in kernel.
> the most intuitive and the easier to read.
> 
> The worst case this approach is creating is
> 
> Signed-off:
> Cc:
> Cc:
> Cc:
> 
> really ugly on the first patch imho.
> 
> So, I doubt we can reach to an agreement. So let's
> agree at least in not enforce this chronological thing
> as a rule and let people use what ever they feel better.
> 
> Specially because I don't see any other place where
> this is trying to get enforced like this.

Well, I don't think we should be afraid to be the first to set an
example.

I think jointly agreeing on some form would be a good idea when we
don't have a single maintainer that'd go around consolidating all the
tag orders as they see fith.

> 
> Thanks
> Signed-off: Rodrigo.

I see what you did there ;)

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 tag ordering (Was: Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module)

[Jani Nikula]

On Mon, 06 Nov 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> + Jani (and Daniel as emeritus maintainer)
>
> On Fri, 2017-11-03 at 10:08 -0700, Rodrigo Vivi wrote:
>> On Fri, Nov 03, 2017 at 08:36:01AM +0000, Joonas Lahtinen wrote:
>> > On Fri, 2017-11-03 at 00:03 +0000, Chris Wilson wrote:
>> > > Quoting Rodrigo Vivi (2017-11-02 23:52:45)
>> > > > On Wed, Oct 4, 2017 at 6:07 AM, Joonas Lahtinen
>> > > > <joonas.lahtinen@linux.intel.com> wrote:
>> > > > > On Tue, 2017-10-03 at 15:56 -0700, Sujaritha Sundaresan wrote:
>> > > > > > We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
>> > > > > > Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1.
>> > > > > > We also need enable_guc_loading=1 when we want to verify the HuC,
>> > > > > > which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
>> > > > > 
>> > > > > Long lines in commit message, please give a look at:
>> > > > > 
>> > > > > https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html
>> > > > > 
>> > > > > Section "14) The canonical patch format".
>> > > > > 
>> > > > > Then, about the patch. I think the commit message should be more clear
>> > > > > about the fact that if we have HuC firmware to be loaded, we need to
>> > > > > have GuC to actually load it. So if an user wants to avoid the GuC from
>> > > > > getting loaded, they must not have a HuC firmware to be loaded, in
>> > > > > addition to not using GuC submission.
>> > > > > 
>> > > > > > 
>> > > > > > v2: Clarifying the commit message (Anusha)
>> > > > > > 
>> > > > > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>> > > > > > 
>> > > > > > v4: Rebase
>> > > > > > 
>> > > > > > v5: Separating message unification into a separate patch
>> > > > > > 
>> > > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> > > > > > Cc: Oscar Mateo <oscar.mateo@intel.com>
>> > > > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> > > > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> > > > > 
>> > > > > Try to keep the tags in chronological order, so start with Suggested-
>> > > > > by: (if any), Signed-off-by:, Cc: and so on.
>> > > > 
>> > > > Could we agree on have
>> > > > Suggested-by:
>> > > > Cc:
>> > > > Signed-off-by:
>> > > > as the initial chronological order and then follow the chronological
>> > > 
>> > > But CCs come after a s-o-b, because they are added after the commit. (I
>> > > write some code, then think who might be interested; usually by looking
>> > > at who previously worked on the same code). Then you also add new CCs
>> > > later on based on review feedback; a comment on v1 gets a CC on v2.
>> > > Bugzilla/reported-by/suggested-by are before since they presumably
>> > > prompted the commit to be written in the first place (plus also they
>> > > deserve extra credit for their effort in alerting us to the issue).
>> > 
>> > Yeah, this is my reasoning too.
>> 
>> So it seems the chronological order differs from case to case
>> from person to person.
>> When I write a patch most of the times I have people in mind
>> that I will cc. Like when I'm writing an email.
>> cc: people that touch this code from last time
>> cc: people that can help on review
>> cc: people that introduced this error
>> cc: people that will be futurely impacted by this change
>
> I don't follow this logic. Most of the Cc:s are chosen based on what
> the code does, in get_maintainers.pl fashion. I think one is set to
> implement a feature/bugfix, and it is not necessarily certain in the
> beginning where the code will land, could be core kernel, DRM or i915
> even. And Cc:s will vary accordingly depending on where the code
> landed.
>
> I can see an argument for some Cc:s before Signed-off-by:, but I never
> claimed that wouldn't be the case. Just that chronological ordering
> makes sense (will be easy to automate, too).
>
> I would claim that all of the four points you listed, you'll be looking
> at git blame based on what the code you wrote changed. And you don't
> know the whole scope of code in advance except for really small fixes.
> Where you wrote the code in your mind already. And it's really the IP
> you're S-o-b:ing. But this gets pretty theoritical already.
>
>> and then I sign-off on the end of the patch as I sign off in the
>> end of a message.
>> 
>> > 
>> > Also, when you add the machine assistance from Patchwork to
>> > automatically spread tags from the cover letter (Acked-by, Reviewed-by
>> > etc. and it's in the works, I understand). I don't quite see why we
>> > would have only a portion of the tags in chronological order.
>> > 
>> > If I respin a patch, it might already have:
>> > 
>> > Bugzilla:
>> > Suggested-by:
>> > Signed-off-by:
>> > Cc:
>> > Cc:
>> > Acked-by:
>> > Reviewed-by:
>> 
>> I really would like to have something like:
>> 
>> Bugzilla:
>> Suggested-by:
>> Cc:
>> Cc:
>> Signed-off-by:
>> Acked-by:
>> Reviewed-by:
>> 
>> This seems to be the most used in kernel.
>> the most intuitive and the easier to read.
>> 
>> The worst case this approach is creating is
>> 
>> Signed-off:
>> Cc:
>> Cc:
>> Cc:
>> 
>> really ugly on the first patch imho.
>> 
>> So, I doubt we can reach to an agreement. So let's
>> agree at least in not enforce this chronological thing
>> as a rule and let people use what ever they feel better.
>> 
>> Specially because I don't see any other place where
>> this is trying to get enforced like this.
>
> Well, I don't think we should be afraid to be the first to set an
> example.
>
> I think jointly agreeing on some form would be a good idea when we
> don't have a single maintainer that'd go around consolidating all the
> tag orders as they see fith.

I think I've only ever cared about two things wrt tag order: Getting the
Signed-off-bys in order because that has relevance in tracking the
origin of the patch, and putting my Signed-off-by last because that was
where everyone and their dog put it when I started doing kernel work.

If you want to encourage some uniform style, go for it, but please let's
not start nitpicking and having people resend their patches just because
they didn't put the tags in the order.

BR,
Jani.

>
>> 
>> Thanks
>> Signed-off: Rodrigo.
>
> I see what you did there ;)
>
> Regards, Joonas

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx