[PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Michal Wajdeczko]

Prepare for alternate GuC notification mechanism.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 10 +++++++++-
 drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7fd75ca..72f49e6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
 }
 
+static void guc_write_irq_trigger(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
 	mutex_init(&guc->send_mutex);
 	guc->send = intel_guc_send_nop;
+	guc->notify = guc_write_irq_trigger;
 }
 
 static void fetch_uc_fw(struct drm_i915_private *dev_priv,
@@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 
 	POSTING_READ(SOFT_SCRATCH(i - 1));
 
-	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+	intel_guc_notify(guc);
 
 	/*
 	 * No GuC command should ever take longer than 10ms.
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 1e0eecd..097289b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -210,6 +210,9 @@ struct intel_guc {
 
 	/* GuC's FW specific send function */
 	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
+
+	/* GuC's FW specific notify function */
+	void (*notify)(struct intel_guc *guc);
 };
 
 struct intel_huc {
@@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 l
 {
 	return guc->send(guc, action, len);
 }
+static inline void intel_guc_notify(struct intel_guc *guc)
+{
+	guc->notify(guc);
+}
 
 /* intel_guc_loader.c */
 int intel_guc_select_fw(struct intel_guc *guc);
-- 
2.7.4

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

[PATCH 2/3] drm/i915/guc: Make scratch register base and count flexible

[Michal Wajdeczko]

We are using some scratch registers in MMIO based send function.
Make their base and count flexible in preparation of upcoming
GuC firmware/hardware changes.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 42 +++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 72f49e6..73c3324 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -260,9 +260,35 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 	__intel_uc_fw_fini(&dev_priv->huc.fw);
 }
 
+static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
+{
+	GEM_BUG_ON(!guc->send_regs.base);
+	GEM_BUG_ON(!guc->send_regs.count);
+	GEM_BUG_ON(i >= guc->send_regs.count);
+
+	return _MMIO(guc->send_regs.base + 4 * i);
+}
+
+static void guc_init_send_regs(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	enum forcewake_domains fw_domains = 0;
+	u32 i;
+
+	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
+	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
+
+	for (i = 0; i < guc->send_regs.count; i++) {
+		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+					guc_send_reg(guc, i),
+					FW_REG_READ | FW_REG_WRITE);
+	}
+	guc->send_regs.fw_domains = fw_domains;
+}
+
 static int guc_enable_communication(struct intel_guc *guc)
 {
-	/* XXX: placeholder for alternate setup */
+	guc_init_send_regs(guc);
 	guc->send = intel_guc_send_mmio;
 	return 0;
 }
@@ -407,19 +433,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	int i;
 	int ret;
 
-	if (WARN_ON(len < 1 || len > 15))
-		return -EINVAL;
+	GEM_BUG_ON(!len);
+	GEM_BUG_ON(len > guc->send_regs.count);
 
 	mutex_lock(&guc->send_mutex);
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
 
 	dev_priv->guc.action_count += 1;
 	dev_priv->guc.action_cmd = action[0];
 
 	for (i = 0; i < len; i++)
-		I915_WRITE(SOFT_SCRATCH(i), action[i]);
+		I915_WRITE(guc_send_reg(guc, i), action[i]);
 
-	POSTING_READ(SOFT_SCRATCH(i - 1));
+	POSTING_READ(guc_send_reg(guc, i - 1));
 
 	intel_guc_notify(guc);
 
@@ -428,7 +454,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	 * Fast commands should still complete in 10us.
 	 */
 	ret = __intel_wait_for_register_fw(dev_priv,
-					   SOFT_SCRATCH(0),
+					   guc_send_reg(guc, 0),
 					   INTEL_GUC_RECV_MASK,
 					   INTEL_GUC_RECV_MASK,
 					   10, 10, &status);
@@ -450,7 +476,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	}
 	dev_priv->guc.action_status = status;
 
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
 	mutex_unlock(&guc->send_mutex);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 097289b..a37a8cc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -205,6 +205,13 @@ struct intel_guc {
 	uint64_t submissions[I915_NUM_ENGINES];
 	uint32_t last_seqno[I915_NUM_ENGINES];
 
+	/* GuC's FW specific registers used in MMIO send */
+	struct {
+		u32 base;
+		u32 count;
+		u32 fw_domains; /* enum forcewake_domains */
+	} send_regs;
+
 	/* To serialize the intel_guc_send actions */
 	struct mutex send_mutex;
 
-- 
2.7.4

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

Re: [PATCH 2/3] drm/i915/guc: Make scratch register base and count flexible

[Daniele Ceraolo Spurio]

On 02/05/17 05:39, Michal Wajdeczko wrote:
> We are using some scratch registers in MMIO based send function.
> Make their base and count flexible in preparation of upcoming
> GuC firmware/hardware changes.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 42 +++++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
>  2 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 72f49e6..73c3324 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -260,9 +260,35 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  	__intel_uc_fw_fini(&dev_priv->huc.fw);
>  }
>
> +static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> +{
> +	GEM_BUG_ON(!guc->send_regs.base);
> +	GEM_BUG_ON(!guc->send_regs.count);
> +	GEM_BUG_ON(i >= guc->send_regs.count);
> +
> +	return _MMIO(guc->send_regs.base + 4 * i);
> +}
> +
> +static void guc_init_send_regs(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	enum forcewake_domains fw_domains = 0;
> +	u32 i;
> +
> +	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> +	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> +
> +	for (i = 0; i < guc->send_regs.count; i++) {
> +		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> +					guc_send_reg(guc, i),
> +					FW_REG_READ | FW_REG_WRITE);
> +	}
> +	guc->send_regs.fw_domains = fw_domains;
> +}
> +
>  static int guc_enable_communication(struct intel_guc *guc)
>  {
> -	/* XXX: placeholder for alternate setup */

Is it worth retaining this comment? We still expect the new _send 
mechanism setup to be added here, right?

> +	guc_init_send_regs(guc);
>  	guc->send = intel_guc_send_mmio;
>  	return 0;
>  }
> @@ -407,19 +433,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	int i;
>  	int ret;
>
> -	if (WARN_ON(len < 1 || len > 15))
> -		return -EINVAL;
> +	GEM_BUG_ON(!len);
> +	GEM_BUG_ON(len > guc->send_regs.count);

Should we call out this change from WARN to GEM_BUG in the commit message?

Nitpicks aside,
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Regards,
Daniele

>
>  	mutex_lock(&guc->send_mutex);
> -	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
> +	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
>
>  	dev_priv->guc.action_count += 1;
>  	dev_priv->guc.action_cmd = action[0];
>
>  	for (i = 0; i < len; i++)
> -		I915_WRITE(SOFT_SCRATCH(i), action[i]);
> +		I915_WRITE(guc_send_reg(guc, i), action[i]);
>
> -	POSTING_READ(SOFT_SCRATCH(i - 1));
> +	POSTING_READ(guc_send_reg(guc, i - 1));
>
>  	intel_guc_notify(guc);
>
> @@ -428,7 +454,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	 * Fast commands should still complete in 10us.
>  	 */
>  	ret = __intel_wait_for_register_fw(dev_priv,
> -					   SOFT_SCRATCH(0),
> +					   guc_send_reg(guc, 0),
>  					   INTEL_GUC_RECV_MASK,
>  					   INTEL_GUC_RECV_MASK,
>  					   10, 10, &status);
> @@ -450,7 +476,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	}
>  	dev_priv->guc.action_status = status;
>
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
> +	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
>  	mutex_unlock(&guc->send_mutex);
>
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 097289b..a37a8cc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -205,6 +205,13 @@ struct intel_guc {
>  	uint64_t submissions[I915_NUM_ENGINES];
>  	uint32_t last_seqno[I915_NUM_ENGINES];
>
> +	/* GuC's FW specific registers used in MMIO send */
> +	struct {
> +		u32 base;
> +		u32 count;
> +		u32 fw_domains; /* enum forcewake_domains */
> +	} send_regs;
> +
>  	/* To serialize the intel_guc_send actions */
>  	struct mutex send_mutex;
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] HAX Enable GuC loading & submission

[Michal Wajdeczko]

This is just for CI testing, *** DO NOT MERGE ***

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e36..abd2894 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,8 +56,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
-	.enable_guc_submission = 0,
+	.enable_guc_loading = 1,
+	.enable_guc_submission = 1,
 	.guc_log_level = -1,
 	.guc_firmware_path = NULL,
 	.huc_firmware_path = NULL,
@@ -221,12 +221,12 @@ MODULE_PARM_DESC(edp_vswing,
 module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
 MODULE_PARM_DESC(enable_guc_loading,
 		"Enable GuC firmware loading "
-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
+		"(-1=auto, 0=never, 1=if available [default], 2=required)");
 
 module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
 MODULE_PARM_DESC(enable_guc_submission,
 		"Enable GuC submission "
-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
+		"(-1=auto, 0=never, 1=if available [default], 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
-- 
2.7.4

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Move notification code into virtual function

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/guc: Move notification code into virtual function
URL   : https://patchwork.freedesktop.org/series/23805/
State : success

== Summary ==

Series 23805v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/23805/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:430s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:579s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:491s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:530s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:496s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:479s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:409s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:408s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:451s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:550s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:440s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:562s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:453s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:485s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:415s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:532s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:407s

310077224306c08a82476bb616de679715e83485 drm-tip: 2017y-05m-02d-12h-04m-57s UTC integration manifest
3335977 HAX Enable GuC loading & submission
6a06ee0 drm/i915/guc: Make scratch register base and count flexible
0207b59 drm/i915/guc: Move notification code into virtual function

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4596/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Daniele Ceraolo Spurio]

On 02/05/17 05:39, Michal Wajdeczko wrote:
> Prepare for alternate GuC notification mechanism.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 7fd75ca..72f49e6 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>  }
>
> +static void guc_write_irq_trigger(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> +}
> +
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
>
>  	mutex_init(&guc->send_mutex);
>  	guc->send = intel_guc_send_nop;
> +	guc->notify = guc_write_irq_trigger;
>  }
>
>  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> @@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>
>  	POSTING_READ(SOFT_SCRATCH(i - 1));
>
> -	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> +	intel_guc_notify(guc);
>
>  	/*
>  	 * No GuC command should ever take longer than 10ms.
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 1e0eecd..097289b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -210,6 +210,9 @@ struct intel_guc {
>
>  	/* GuC's FW specific send function */
>  	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> +
> +	/* GuC's FW specific notify function */
> +	void (*notify)(struct intel_guc *guc);
>  };
>
>  struct intel_huc {
> @@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 l
>  {
>  	return guc->send(guc, action, len);
>  }
> +static inline void intel_guc_notify(struct intel_guc *guc)
> +{
> +	guc->notify(guc);
> +}
>

personal preference: I would use guc->notify directly instead of adding 
intel_guc_notify(). intel_guc_send() made more sense because a function 
with that name already existed and by keeping it we minimized the 
change, but I don't see such benefit with intel_guc_notify() and calling 
the function pointer directly feels more in sync with what we do 
elsewhere in the driver.

No strong feelings anyway, so:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Regards,
Daniele

>  /* intel_guc_loader.c */
>  int intel_guc_select_fw(struct intel_guc *guc);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Michal Wajdeczko]

On Tue, May 02, 2017 at 09:37:45AM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 02/05/17 05:39, Michal Wajdeczko wrote:
> > Prepare for alternate GuC notification mechanism.
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uc.c | 10 +++++++++-
> >  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 7fd75ca..72f49e6 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> >  		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> >  }
> > 
> > +static void guc_write_irq_trigger(struct intel_guc *guc)
> > +{
> > +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > +
> > +	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> > +}
> > +
> >  void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_guc *guc = &dev_priv->guc;
> > 
> >  	mutex_init(&guc->send_mutex);
> >  	guc->send = intel_guc_send_nop;
> > +	guc->notify = guc_write_irq_trigger;
> >  }
> > 
> >  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> > @@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> > 
> >  	POSTING_READ(SOFT_SCRATCH(i - 1));
> > 
> > -	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> > +	intel_guc_notify(guc);
> > 
> >  	/*
> >  	 * No GuC command should ever take longer than 10ms.
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 1e0eecd..097289b 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -210,6 +210,9 @@ struct intel_guc {
> > 
> >  	/* GuC's FW specific send function */
> >  	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> > +
> > +	/* GuC's FW specific notify function */
> > +	void (*notify)(struct intel_guc *guc);
> >  };
> > 
> >  struct intel_huc {
> > @@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 l
> >  {
> >  	return guc->send(guc, action, len);
> >  }
> > +static inline void intel_guc_notify(struct intel_guc *guc)
> > +{
> > +	guc->notify(guc);
> > +}
> > 
> 
> personal preference: I would use guc->notify directly instead of adding
> intel_guc_notify(). intel_guc_send() made more sense because a function with
> that name already existed and by keeping it we minimized the change, but I
> don't see such benefit with intel_guc_notify() and calling the function
> pointer directly feels more in sync with what we do elsewhere in the driver.
> 

Note that thanks to the compiler, resulting code is the same, but by keeping
intel_guc_notify() we are more flexible than when using hardcoded guc->notify.
Note that the same reason why we added intel_guc_send() "minimize the change"
can also be valid when in the future we may want to switch from vfun pointer
to other implementation that then can be hidden in that tiny inline that you
just wanted to kill.

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

[PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

[Michal Wajdeczko]

We are using some scratch registers in MMIO based send function.
Make their base and count flexible in preparation of upcoming
GuC firmware/hardware changes. While around, change cmd len
parameter verification from WARN_ON to GEM_BUG_ON as we don't
need this all the time.

v2: call out WARN/GEM_BUG change in the commit msg (Daniele)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 72f49e6..9d11c42 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 	__intel_uc_fw_fini(&dev_priv->huc.fw);
 }
 
+static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
+{
+	GEM_BUG_ON(!guc->send_regs.base);
+	GEM_BUG_ON(!guc->send_regs.count);
+	GEM_BUG_ON(i >= guc->send_regs.count);
+
+	return _MMIO(guc->send_regs.base + 4 * i);
+}
+
+static void guc_init_send_regs(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	enum forcewake_domains fw_domains = 0;
+	u32 i;
+
+	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
+	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
+
+	for (i = 0; i < guc->send_regs.count; i++) {
+		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+					guc_send_reg(guc, i),
+					FW_REG_READ | FW_REG_WRITE);
+	}
+	guc->send_regs.fw_domains = fw_domains;
+}
+
 static int guc_enable_communication(struct intel_guc *guc)
 {
 	/* XXX: placeholder for alternate setup */
+	guc_init_send_regs(guc);
 	guc->send = intel_guc_send_mmio;
 	return 0;
 }
@@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	int i;
 	int ret;
 
-	if (WARN_ON(len < 1 || len > 15))
-		return -EINVAL;
+	GEM_BUG_ON(!len);
+	GEM_BUG_ON(len > guc->send_regs.count);
 
 	mutex_lock(&guc->send_mutex);
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
 
 	dev_priv->guc.action_count += 1;
 	dev_priv->guc.action_cmd = action[0];
 
 	for (i = 0; i < len; i++)
-		I915_WRITE(SOFT_SCRATCH(i), action[i]);
+		I915_WRITE(guc_send_reg(guc, i), action[i]);
 
-	POSTING_READ(SOFT_SCRATCH(i - 1));
+	POSTING_READ(guc_send_reg(guc, i - 1));
 
 	intel_guc_notify(guc);
 
@@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	 * Fast commands should still complete in 10us.
 	 */
 	ret = __intel_wait_for_register_fw(dev_priv,
-					   SOFT_SCRATCH(0),
+					   guc_send_reg(guc, 0),
 					   INTEL_GUC_RECV_MASK,
 					   INTEL_GUC_RECV_MASK,
 					   10, 10, &status);
@@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	}
 	dev_priv->guc.action_status = status;
 
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
 	mutex_unlock(&guc->send_mutex);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 097289b..a37a8cc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -205,6 +205,13 @@ struct intel_guc {
 	uint64_t submissions[I915_NUM_ENGINES];
 	uint32_t last_seqno[I915_NUM_ENGINES];
 
+	/* GuC's FW specific registers used in MMIO send */
+	struct {
+		u32 base;
+		u32 count;
+		u32 fw_domains; /* enum forcewake_domains */
+	} send_regs;
+
 	/* To serialize the intel_guc_send actions */
 	struct mutex send_mutex;
 
-- 
2.7.4

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

Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

[Jani Nikula]

On Thu, 04 May 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> We are using some scratch registers in MMIO based send function.
> Make their base and count flexible in preparation of upcoming
> GuC firmware/hardware changes. While around, change cmd len
> parameter verification from WARN_ON to GEM_BUG_ON as we don't
> need this all the time.

I'm not generally fond of caching the registers like this or adding
_MMIO() wrapping outside of i915_reg.h. Sure, we have some of that here
and there, but here it's hard to see the rationale because you do this
in preparation for something that we you're not sharing.

BR,
Jani.

>
> v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
>  2 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 72f49e6..9d11c42 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  	__intel_uc_fw_fini(&dev_priv->huc.fw);
>  }
>  
> +static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> +{
> +	GEM_BUG_ON(!guc->send_regs.base);
> +	GEM_BUG_ON(!guc->send_regs.count);
> +	GEM_BUG_ON(i >= guc->send_regs.count);
> +
> +	return _MMIO(guc->send_regs.base + 4 * i);
> +}
> +
> +static void guc_init_send_regs(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	enum forcewake_domains fw_domains = 0;
> +	u32 i;
> +
> +	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> +	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> +
> +	for (i = 0; i < guc->send_regs.count; i++) {
> +		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> +					guc_send_reg(guc, i),
> +					FW_REG_READ | FW_REG_WRITE);
> +	}
> +	guc->send_regs.fw_domains = fw_domains;
> +}
> +
>  static int guc_enable_communication(struct intel_guc *guc)
>  {
>  	/* XXX: placeholder for alternate setup */
> +	guc_init_send_regs(guc);
>  	guc->send = intel_guc_send_mmio;
>  	return 0;
>  }
> @@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	int i;
>  	int ret;
>  
> -	if (WARN_ON(len < 1 || len > 15))
> -		return -EINVAL;
> +	GEM_BUG_ON(!len);
> +	GEM_BUG_ON(len > guc->send_regs.count);
>  
>  	mutex_lock(&guc->send_mutex);
> -	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
> +	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
>  
>  	dev_priv->guc.action_count += 1;
>  	dev_priv->guc.action_cmd = action[0];
>  
>  	for (i = 0; i < len; i++)
> -		I915_WRITE(SOFT_SCRATCH(i), action[i]);
> +		I915_WRITE(guc_send_reg(guc, i), action[i]);
>  
> -	POSTING_READ(SOFT_SCRATCH(i - 1));
> +	POSTING_READ(guc_send_reg(guc, i - 1));
>  
>  	intel_guc_notify(guc);
>  
> @@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	 * Fast commands should still complete in 10us.
>  	 */
>  	ret = __intel_wait_for_register_fw(dev_priv,
> -					   SOFT_SCRATCH(0),
> +					   guc_send_reg(guc, 0),
>  					   INTEL_GUC_RECV_MASK,
>  					   INTEL_GUC_RECV_MASK,
>  					   10, 10, &status);
> @@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	}
>  	dev_priv->guc.action_status = status;
>  
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
> +	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
>  	mutex_unlock(&guc->send_mutex);
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 097289b..a37a8cc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -205,6 +205,13 @@ struct intel_guc {
>  	uint64_t submissions[I915_NUM_ENGINES];
>  	uint32_t last_seqno[I915_NUM_ENGINES];
>  
> +	/* GuC's FW specific registers used in MMIO send */
> +	struct {
> +		u32 base;
> +		u32 count;
> +		u32 fw_domains; /* enum forcewake_domains */
> +	} send_regs;
> +
>  	/* To serialize the intel_guc_send actions */
>  	struct mutex send_mutex;

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

Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

[Michal Wajdeczko]

On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote:
> On Thu, 04 May 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> > We are using some scratch registers in MMIO based send function.
> > Make their base and count flexible in preparation of upcoming
> > GuC firmware/hardware changes. While around, change cmd len
> > parameter verification from WARN_ON to GEM_BUG_ON as we don't
> > need this all the time.
> 
> I'm not generally fond of caching the registers like this or adding
> _MMIO() wrapping outside of i915_reg.h. Sure, we have some of that here
> and there, but here it's hard to see the rationale because you do this
> in preparation for something that we you're not sharing.
> 

I can't share details atm, but as commit message says, there will be a
change in both offsets and number of scratch registers.

Imho any wrapping around these values can't go to the i915_[guc_]reg.h file
as that file shall include only raw MMIO definitions, without any extra
logic that is based on GEN or PLATFORM or FW version.

Alternate approach would be, thanks to the already defined virtual function
send(), to create new send_mmio function(s) that will be 100% the same as
the old send_mmio except offset and count of the scratch registers.

Then we can benefit from most optimal implementation per GEN|PLATFORM|FW that
can run without reading cached regs offsets/count, but at the cost of extra
code that need to be maintained to be in sync with the original function.
And then someone else can point out that we missed code sharing opportunity.

I'm afraid there is no clear winner. 

-Michal


> BR,
> Jani.
> 
> >
> > v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
> >
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
> >  2 files changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 72f49e6..9d11c42 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
> >  	__intel_uc_fw_fini(&dev_priv->huc.fw);
> >  }
> >  
> > +static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> > +{
> > +	GEM_BUG_ON(!guc->send_regs.base);
> > +	GEM_BUG_ON(!guc->send_regs.count);
> > +	GEM_BUG_ON(i >= guc->send_regs.count);
> > +
> > +	return _MMIO(guc->send_regs.base + 4 * i);
> > +}
> > +
> > +static void guc_init_send_regs(struct intel_guc *guc)
> > +{
> > +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > +	enum forcewake_domains fw_domains = 0;
> > +	u32 i;
> > +
> > +	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> > +	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> > +
> > +	for (i = 0; i < guc->send_regs.count; i++) {
> > +		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> > +					guc_send_reg(guc, i),
> > +					FW_REG_READ | FW_REG_WRITE);
> > +	}
> > +	guc->send_regs.fw_domains = fw_domains;
> > +}
> > +
> >  static int guc_enable_communication(struct intel_guc *guc)
> >  {
> >  	/* XXX: placeholder for alternate setup */
> > +	guc_init_send_regs(guc);
> >  	guc->send = intel_guc_send_mmio;
> >  	return 0;
> >  }
> > @@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> >  	int i;
> >  	int ret;
> >  
> > -	if (WARN_ON(len < 1 || len > 15))
> > -		return -EINVAL;
> > +	GEM_BUG_ON(!len);
> > +	GEM_BUG_ON(len > guc->send_regs.count);
> >  
> >  	mutex_lock(&guc->send_mutex);
> > -	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
> > +	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
> >  
> >  	dev_priv->guc.action_count += 1;
> >  	dev_priv->guc.action_cmd = action[0];
> >  
> >  	for (i = 0; i < len; i++)
> > -		I915_WRITE(SOFT_SCRATCH(i), action[i]);
> > +		I915_WRITE(guc_send_reg(guc, i), action[i]);
> >  
> > -	POSTING_READ(SOFT_SCRATCH(i - 1));
> > +	POSTING_READ(guc_send_reg(guc, i - 1));
> >  
> >  	intel_guc_notify(guc);
> >  
> > @@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> >  	 * Fast commands should still complete in 10us.
> >  	 */
> >  	ret = __intel_wait_for_register_fw(dev_priv,
> > -					   SOFT_SCRATCH(0),
> > +					   guc_send_reg(guc, 0),
> >  					   INTEL_GUC_RECV_MASK,
> >  					   INTEL_GUC_RECV_MASK,
> >  					   10, 10, &status);
> > @@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> >  	}
> >  	dev_priv->guc.action_status = status;
> >  
> > -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
> > +	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> >  	mutex_unlock(&guc->send_mutex);
> >  
> >  	return ret;
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 097289b..a37a8cc 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -205,6 +205,13 @@ struct intel_guc {
> >  	uint64_t submissions[I915_NUM_ENGINES];
> >  	uint32_t last_seqno[I915_NUM_ENGINES];
> >  
> > +	/* GuC's FW specific registers used in MMIO send */
> > +	struct {
> > +		u32 base;
> > +		u32 count;
> > +		u32 fw_domains; /* enum forcewake_domains */
> > +	} send_regs;
> > +
> >  	/* To serialize the intel_guc_send actions */
> >  	struct mutex send_mutex;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

[Chris Wilson]

On Thu, May 04, 2017 at 06:26:04PM +0200, Michal Wajdeczko wrote:
> On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote:
> > On Thu, 04 May 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> > > We are using some scratch registers in MMIO based send function.
> > > Make their base and count flexible in preparation of upcoming
> > > GuC firmware/hardware changes. While around, change cmd len
> > > parameter verification from WARN_ON to GEM_BUG_ON as we don't
> > > need this all the time.
> > 
> > I'm not generally fond of caching the registers like this or adding
> > _MMIO() wrapping outside of i915_reg.h. Sure, we have some of that here
> > and there, but here it's hard to see the rationale because you do this
> > in preparation for something that we you're not sharing.
> > 
> 
> I can't share details atm, but as commit message says, there will be a
> change in both offsets and number of scratch registers.
> 
> Imho any wrapping around these values can't go to the i915_[guc_]reg.h file
> as that file shall include only raw MMIO definitions, without any extra
> logic that is based on GEN or PLATFORM or FW version.

The guc->send.base + offset approach is reasonable; it is certainly
the tried and trusted approach. I would stick with it, but we just can't
help with any suggestions without seeing the destination. Oh well, we
can dream that instead of using mmio space for datagrams they move to
ring (even WC will be better than a bunch of UC)!

Don't overqualify the ints though, u32 base is ok, but it could be unsigned
count (though an alternative would be u32 end, and even mark it as
GEM_DEBUG_DECL!) and definitely unsigned fw_domains as that is not
defined as being u32. (Just more than 32 domains is unlikely before
tomorrow ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

[Jani Nikula]

On Thu, 04 May 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, May 04, 2017 at 06:26:04PM +0200, Michal Wajdeczko wrote:
>> On Thu, May 04, 2017 at 04:22:15PM +0300, Jani Nikula wrote:
>> > On Thu, 04 May 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
>> > > We are using some scratch registers in MMIO based send function.
>> > > Make their base and count flexible in preparation of upcoming
>> > > GuC firmware/hardware changes. While around, change cmd len
>> > > parameter verification from WARN_ON to GEM_BUG_ON as we don't
>> > > need this all the time.
>> > 
>> > I'm not generally fond of caching the registers like this or adding
>> > _MMIO() wrapping outside of i915_reg.h. Sure, we have some of that here
>> > and there, but here it's hard to see the rationale because you do this
>> > in preparation for something that we you're not sharing.
>> > 
>> 
>> I can't share details atm, but as commit message says, there will be a
>> change in both offsets and number of scratch registers.
>> 
>> Imho any wrapping around these values can't go to the i915_[guc_]reg.h file
>> as that file shall include only raw MMIO definitions, without any extra
>> logic that is based on GEN or PLATFORM or FW version.
>
> The guc->send.base + offset approach is reasonable; it is certainly
> the tried and trusted approach. I would stick with it, but we just can't
> help with any suggestions without seeing the destination.

That pretty much sums it up.

BR,
Jani.

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

[PATCH v3 2/3] drm/i915/guc: Make scratch register base and count flexible

[Michal Wajdeczko]

We are using some scratch registers in MMIO based send function.
Make their base and count flexible in preparation of upcoming
GuC firmware/hardware changes. While around, change cmd len
parameter verification from WARN_ON to GEM_BUG_ON as we don't
need this all the time.

v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
v3: don't overqualify the ints (Chris)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 72f49e6..9d11c42 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 	__intel_uc_fw_fini(&dev_priv->huc.fw);
 }
 
+static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
+{
+	GEM_BUG_ON(!guc->send_regs.base);
+	GEM_BUG_ON(!guc->send_regs.count);
+	GEM_BUG_ON(i >= guc->send_regs.count);
+
+	return _MMIO(guc->send_regs.base + 4 * i);
+}
+
+static void guc_init_send_regs(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	enum forcewake_domains fw_domains = 0;
+	u32 i;
+
+	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
+	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
+
+	for (i = 0; i < guc->send_regs.count; i++) {
+		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+					guc_send_reg(guc, i),
+					FW_REG_READ | FW_REG_WRITE);
+	}
+	guc->send_regs.fw_domains = fw_domains;
+}
+
 static int guc_enable_communication(struct intel_guc *guc)
 {
 	/* XXX: placeholder for alternate setup */
+	guc_init_send_regs(guc);
 	guc->send = intel_guc_send_mmio;
 	return 0;
 }
@@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	int i;
 	int ret;
 
-	if (WARN_ON(len < 1 || len > 15))
-		return -EINVAL;
+	GEM_BUG_ON(!len);
+	GEM_BUG_ON(len > guc->send_regs.count);
 
 	mutex_lock(&guc->send_mutex);
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
 
 	dev_priv->guc.action_count += 1;
 	dev_priv->guc.action_cmd = action[0];
 
 	for (i = 0; i < len; i++)
-		I915_WRITE(SOFT_SCRATCH(i), action[i]);
+		I915_WRITE(guc_send_reg(guc, i), action[i]);
 
-	POSTING_READ(SOFT_SCRATCH(i - 1));
+	POSTING_READ(guc_send_reg(guc, i - 1));
 
 	intel_guc_notify(guc);
 
@@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	 * Fast commands should still complete in 10us.
 	 */
 	ret = __intel_wait_for_register_fw(dev_priv,
-					   SOFT_SCRATCH(0),
+					   guc_send_reg(guc, 0),
 					   INTEL_GUC_RECV_MASK,
 					   INTEL_GUC_RECV_MASK,
 					   10, 10, &status);
@@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	}
 	dev_priv->guc.action_status = status;
 
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
 	mutex_unlock(&guc->send_mutex);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 097289b..4dd39a7 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -205,6 +205,13 @@ struct intel_guc {
 	uint64_t submissions[I915_NUM_ENGINES];
 	uint32_t last_seqno[I915_NUM_ENGINES];
 
+	/* GuC's FW specific registers used in MMIO send */
+	struct {
+		u32 base;
+		unsigned int count;
+		unsigned int fw_domains; /* enum forcewake_domains */
+	} send_regs;
+
 	/* To serialize the intel_guc_send actions */
 	struct mutex send_mutex;
 
-- 
2.7.4

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

Re: [PATCH v3 2/3] drm/i915/guc: Make scratch register base and count flexible

[Joonas Lahtinen]

On pe, 2017-05-05 at 11:35 +0000, Michal Wajdeczko wrote:
> We are using some scratch registers in MMIO based send function.
> Make their base and count flexible in preparation of upcoming
> GuC firmware/hardware changes. While around, change cmd len
> parameter verification from WARN_ON to GEM_BUG_ON as we don't
> need this all the time.
> 
> v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
> v3: don't overqualify the ints (Chris)
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -205,6 +205,13 @@ struct intel_guc {
>  	uint64_t submissions[I915_NUM_ENGINES];
>  	uint32_t last_seqno[I915_NUM_ENGINES];
>  
> +	/* GuC's FW specific registers used in MMIO send */
> +	struct {
> +		u32 base;
> +		unsigned int count;
> +		unsigned int fw_domains; /* enum forcewake_domains */

As discussed in IRC, split intel_uncore.h with what is reasonable
without refactoring, and untangle the order to make the type what it is
inside the function.

Regards, Joonas

PS. I personally don't like the enum typed bitfields, but that's an
another discussion (that's been had in the past).
-- 
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 v3 2/3] drm/i915/guc: Make scratch register base and count flexible

[Jani Nikula]

On Mon, 08 May 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> PS. I personally don't like the enum typed bitfields, but that's an
> another discussion (that's been had in the past).

I'm with you on this one. It's semi-okay to define the bits as enums,
but IMO a variable of an enum type should only ever be used to hold the
enumerated values. Too bad C doesn't give us strong type checking for
enums.

BR,
Jani.


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

[PATCH v4 2/3] drm/i915/guc: Make scratch register base and count flexible

[Michal Wajdeczko]

We are using some scratch registers in MMIO based send function.
Make their base and count flexible in preparation of upcoming
GuC firmware/hardware changes. While around, change cmd len
parameter verification from WARN_ON to GEM_BUG_ON as we don't
need this all the time.

v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
v3: don't overqualify the ints (Chris)
v4: rebase and use proper enum

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 72f49e6..07c5658 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
 	__intel_uc_fw_fini(&dev_priv->huc.fw);
 }
 
+static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
+{
+	GEM_BUG_ON(!guc->send_regs.base);
+	GEM_BUG_ON(!guc->send_regs.count);
+	GEM_BUG_ON(i >= guc->send_regs.count);
+
+	return _MMIO(guc->send_regs.base + 4 * i);
+}
+
+static void guc_init_send_regs(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	enum forcewake_domains fw_domains = 0;
+	unsigned int i;
+
+	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
+	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
+
+	for (i = 0; i < guc->send_regs.count; i++) {
+		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+					guc_send_reg(guc, i),
+					FW_REG_READ | FW_REG_WRITE);
+	}
+	guc->send_regs.fw_domains = fw_domains;
+}
+
 static int guc_enable_communication(struct intel_guc *guc)
 {
 	/* XXX: placeholder for alternate setup */
+	guc_init_send_regs(guc);
 	guc->send = intel_guc_send_mmio;
 	return 0;
 }
@@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	int i;
 	int ret;
 
-	if (WARN_ON(len < 1 || len > 15))
-		return -EINVAL;
+	GEM_BUG_ON(!len);
+	GEM_BUG_ON(len > guc->send_regs.count);
 
 	mutex_lock(&guc->send_mutex);
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
 
 	dev_priv->guc.action_count += 1;
 	dev_priv->guc.action_cmd = action[0];
 
 	for (i = 0; i < len; i++)
-		I915_WRITE(SOFT_SCRATCH(i), action[i]);
+		I915_WRITE(guc_send_reg(guc, i), action[i]);
 
-	POSTING_READ(SOFT_SCRATCH(i - 1));
+	POSTING_READ(guc_send_reg(guc, i - 1));
 
 	intel_guc_notify(guc);
 
@@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	 * Fast commands should still complete in 10us.
 	 */
 	ret = __intel_wait_for_register_fw(dev_priv,
-					   SOFT_SCRATCH(0),
+					   guc_send_reg(guc, 0),
 					   INTEL_GUC_RECV_MASK,
 					   INTEL_GUC_RECV_MASK,
 					   10, 10, &status);
@@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	}
 	dev_priv->guc.action_status = status;
 
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
+	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
 	mutex_unlock(&guc->send_mutex);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 097289b..53a3388 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -205,6 +205,13 @@ struct intel_guc {
 	uint64_t submissions[I915_NUM_ENGINES];
 	uint32_t last_seqno[I915_NUM_ENGINES];
 
+	/* GuC's FW specific registers used in MMIO send */
+	struct {
+		u32 base;
+		unsigned int count;
+		enum forcewake_domains fw_domains;
+	} send_regs;
+
 	/* To serialize the intel_guc_send actions */
 	struct mutex send_mutex;
 
-- 
2.7.4

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

Re: [PATCH v4 2/3] drm/i915/guc: Make scratch register base and count flexible

[Joonas Lahtinen]

On ti, 2017-05-09 at 14:08 +0000, Michal Wajdeczko wrote:
> We are using some scratch registers in MMIO based send function.
> Make their base and count flexible in preparation of upcoming
> GuC firmware/hardware changes. While around, change cmd len
> parameter verification from WARN_ON to GEM_BUG_ON as we don't
> need this all the time.
> 
> v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
> v3: don't overqualify the ints (Chris)
> v4: rebase and use proper enum
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

I suppose this could've had "# v2" at the end, unless Daniele acked all
the remaining changes.

Anyway, looks good to me with the changes.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Could use an A-b from Jani for disagree and commit stamp as he
initially objected the way of implementing this. 

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

Re: [PATCH v4 2/3] drm/i915/guc: Make scratch register base and count flexible

[Jani Nikula]

On Wed, 10 May 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> On ti, 2017-05-09 at 14:08 +0000, Michal Wajdeczko wrote:
>> We are using some scratch registers in MMIO based send function.
>> Make their base and count flexible in preparation of upcoming
>> GuC firmware/hardware changes. While around, change cmd len
>> parameter verification from WARN_ON to GEM_BUG_ON as we don't
>> need this all the time.
>> 
>> v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
>> v3: don't overqualify the ints (Chris)
>> v4: rebase and use proper enum
>> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> I suppose this could've had "# v2" at the end, unless Daniele acked all
> the remaining changes.
>
> Anyway, looks good to me with the changes.
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Could use an A-b from Jani for disagree and commit stamp as he
> initially objected the way of implementing this. 

Acked-by: Jani Nikula <jani.nikula@intel.com>



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

Re: [PATCH v4 2/3] drm/i915/guc: Make scratch register base and count flexible

[Joonas Lahtinen]

On ke, 2017-05-10 at 13:32 +0300, Jani Nikula wrote:
> > On Wed, 10 May 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> > 
> > On ti, 2017-05-09 at 14:08 +0000, Michal Wajdeczko wrote:
> > > 
> > > We are using some scratch registers in MMIO based send function.
> > > Make their base and count flexible in preparation of upcoming
> > > GuC firmware/hardware changes. While around, change cmd len
> > > parameter verification from WARN_ON to GEM_BUG_ON as we don't
> > > need this all the time.
> > > 
> > > v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
> > > v3: don't overqualify the ints (Chris)
> > > v4: rebase and use proper enum
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > 
> > I suppose this could've had "# v2" at the end, unless Daniele acked all
> > the remaining changes.
> > 
> > Anyway, looks good to me with the changes.
> > 
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > Could use an A-b from Jani for disagree and commit stamp as he
> > initially objected the way of implementing this. 
> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Michal, could you apply the tags and re-send the series so CI picks it
up.

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