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

Commit Message

Michal Wajdeczko May 5, 2017, 11:35 a.m. UTC

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 <[e-mail]>
Suggested-by: Daniele Ceraolo Spurio <[e-mail]>
Cc: Daniele Ceraolo Spurio <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Jani Nikula <[e-mail]>
Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>
---
 drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
 2 files changed, 41 insertions(+), 7 deletions(-)

Comments

Joonas Lahtinen May 8, 2017, 11:31 a.m. UTC | #1

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 <[e-mail]>
> Suggested-by: Daniele Ceraolo Spurio <[e-mail]>
> Cc: Daniele Ceraolo Spurio <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>

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

Jani Nikula May 8, 2017, 12:07 p.m. UTC | #2

On Mon, 08 May 2017, Joonas Lahtinen <[e-mail]> 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.

Patch

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;