[PATCH 1/3] drm/i915: Extract knowledge of register forcewake domains

[PATCH 1/3] drm/i915: Extract knowledge of register forcewake domains

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Knowledge of which register per platform belonds in which
forcewake domain was embedded in the MMIO accessors themselves.

Extract it into standalone macros so they can be used from
new code in the following patches.

This causes GCC to compile some of the MMIO accessors slightly
differently and grows the code a tiny amount. But none of the
growth is on the fast-path so it does not matter hugely.

Affected sizes before:

00000000000026f0 00000000000001a5 t gen6_read16
0000000000002390 00000000000001a5 t gen6_read32
00000000000028a0 00000000000001a5 t gen6_read64

00000000000061d0 000000000000019e t gen8_write16
0000000000006510 000000000000019d t gen8_write32
0000000000006370 000000000000019d t gen8_write64
00000000000021f0 000000000000019d t gen8_write8

Affected sizes after:

0000000000002840 00000000000001aa t gen6_read16
00000000000024e0 00000000000001a9 t gen6_read32
00000000000029f0 00000000000001a9 t gen6_read64

0000000000004f20 00000000000001b5 t gen8_write16
0000000000004ba0 00000000000001b4 t gen8_write32
00000000000050e0 00000000000001b4 t gen8_write64
0000000000004d60 00000000000001b4 t gen8_write8

Other MMIO accessors are not affected in size.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uncore.c | 255 ++++++++++++++++++++++--------------
 1 file changed, 155 insertions(+), 100 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index ac2ac07b505b..22ccaf66b476 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -551,6 +551,16 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
 
+#define __gen6_reg_read_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd; \
+	if (NEEDS_FORCE_WAKE(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else \
+		__fwd = 0; \
+	__fwd; \
+})
+
 #define REG_RANGE(reg, start, end) ((reg) >= (start) && (reg) < (end))
 
 #define FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg) \
@@ -564,6 +574,49 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
 	 REG_RANGE((reg), 0x22000, 0x24000) || \
 	 REG_RANGE((reg), 0x30000, 0x40000))
 
+#define __vlv_reg_read_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd = 0; \
+	if (!NEEDS_FORCE_WAKE(offset)) \
+		__fwd = 0; \
+	else if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_MEDIA; \
+	__fwd; \
+})
+
+static const i915_reg_t gen8_shadowed_regs[] = {
+	FORCEWAKE_MT,
+	GEN6_RPNSWREQ,
+	GEN6_RC_VIDEO_FREQ,
+	RING_TAIL(RENDER_RING_BASE),
+	RING_TAIL(GEN6_BSD_RING_BASE),
+	RING_TAIL(VEBOX_RING_BASE),
+	RING_TAIL(BLT_RING_BASE),
+	/* TODO: Other registers are not yet used */
+};
+
+static bool is_gen8_shadowed(u32 offset)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++)
+		if (offset == gen8_shadowed_regs[i].reg)
+			return true;
+
+	return false;
+}
+
+#define __gen8_reg_write_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd; \
+	if (NEEDS_FORCE_WAKE(offset) && !is_gen8_shadowed(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else \
+		__fwd = 0; \
+	__fwd; \
+})
+
 #define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \
 	(REG_RANGE((reg), 0x2000, 0x4000) || \
 	 REG_RANGE((reg), 0x5200, 0x8000) || \
@@ -586,6 +639,34 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
 	 REG_RANGE((reg), 0x9000, 0xB000) || \
 	 REG_RANGE((reg), 0xF000, 0x10000))
 
+#define __chv_reg_read_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd = 0; \
+	if (!NEEDS_FORCE_WAKE(offset)) \
+		__fwd = 0; \
+	else if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_MEDIA; \
+	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
+	__fwd; \
+})
+
+#define __chv_reg_write_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd = 0; \
+	if (!NEEDS_FORCE_WAKE(offset) || is_gen8_shadowed(offset)) \
+		__fwd = 0; \
+	else if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_MEDIA; \
+	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
+	__fwd; \
+})
+
 #define FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg) \
 	REG_RANGE((reg), 0xB00,  0x2000)
 
@@ -618,6 +699,64 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
 	 !FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(reg) && \
 	 !FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(reg))
 
+#define SKL_NEEDS_FORCE_WAKE(reg) \
+	((reg) < 0x40000 && !FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg))
+
+#define __gen9_reg_read_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd; \
+	if (!SKL_NEEDS_FORCE_WAKE(offset)) \
+		__fwd = 0; \
+	else if (FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else if (FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_MEDIA; \
+	else if (FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
+	else \
+		__fwd = FORCEWAKE_BLITTER; \
+	__fwd; \
+})
+
+static const i915_reg_t gen9_shadowed_regs[] = {
+	RING_TAIL(RENDER_RING_BASE),
+	RING_TAIL(GEN6_BSD_RING_BASE),
+	RING_TAIL(VEBOX_RING_BASE),
+	RING_TAIL(BLT_RING_BASE),
+	FORCEWAKE_BLITTER_GEN9,
+	FORCEWAKE_RENDER_GEN9,
+	FORCEWAKE_MEDIA_GEN9,
+	GEN6_RPNSWREQ,
+	GEN6_RC_VIDEO_FREQ,
+	/* TODO: Other registers are not yet used */
+};
+
+static bool is_gen9_shadowed(u32 offset)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(gen9_shadowed_regs); i++)
+		if (offset == gen9_shadowed_regs[i].reg)
+			return true;
+
+	return false;
+}
+
+#define __gen9_reg_write_fw_domains(offset) \
+({ \
+	enum forcewake_domains __fwd; \
+	if (!SKL_NEEDS_FORCE_WAKE(offset) || is_gen9_shadowed(offset)) \
+		__fwd = 0; \
+	else if (FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER; \
+	else if (FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_MEDIA; \
+	else if (FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(offset)) \
+		__fwd = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
+	else \
+		__fwd = FORCEWAKE_BLITTER; \
+	__fwd; \
+})
+
 static void
 ilk_dummy_write(struct drm_i915_private *dev_priv)
 {
@@ -743,9 +882,11 @@ static inline void __force_wake_auto(struct drm_i915_private *dev_priv,
 #define __gen6_read(x) \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
+	enum forcewake_domains fw_engine; \
 	GEN6_READ_HEADER(x); \
-	if (NEEDS_FORCE_WAKE(offset)) \
-		__force_wake_auto(dev_priv, FORCEWAKE_RENDER); \
+	fw_engine = __gen6_reg_read_fw_domains(offset); \
+	if (fw_engine) \
+		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	GEN6_READ_FOOTER; \
 }
@@ -753,14 +894,9 @@ gen6_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 #define __vlv_read(x) \
 static u##x \
 vlv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
-	enum forcewake_domains fw_engine = 0; \
+	enum forcewake_domains fw_engine; \
 	GEN6_READ_HEADER(x); \
-	if (!NEEDS_FORCE_WAKE(offset)) \
-		fw_engine = 0; \
-	else if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER; \
-	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_MEDIA; \
+	fw_engine = __vlv_reg_read_fw_domains(offset); \
 	if (fw_engine) \
 		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
@@ -770,40 +906,21 @@ vlv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 #define __chv_read(x) \
 static u##x \
 chv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
-	enum forcewake_domains fw_engine = 0; \
+	enum forcewake_domains fw_engine; \
 	GEN6_READ_HEADER(x); \
-	if (!NEEDS_FORCE_WAKE(offset)) \
-		fw_engine = 0; \
-	else if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER; \
-	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_MEDIA; \
-	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
+	fw_engine = __chv_reg_read_fw_domains(offset); \
 	if (fw_engine) \
 		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	GEN6_READ_FOOTER; \
 }
 
-#define SKL_NEEDS_FORCE_WAKE(reg) \
-	((reg) < 0x40000 && !FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg))
-
 #define __gen9_read(x) \
 static u##x \
 gen9_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 	enum forcewake_domains fw_engine; \
 	GEN6_READ_HEADER(x); \
-	if (!SKL_NEEDS_FORCE_WAKE(offset)) \
-		fw_engine = 0; \
-	else if (FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER; \
-	else if (FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_MEDIA; \
-	else if (FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
-	else \
-		fw_engine = FORCEWAKE_BLITTER; \
+	fw_engine = __gen9_reg_read_fw_domains(offset); \
 	if (fw_engine) \
 		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
@@ -942,34 +1059,14 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
 	GEN6_WRITE_FOOTER; \
 }
 
-static const i915_reg_t gen8_shadowed_regs[] = {
-	FORCEWAKE_MT,
-	GEN6_RPNSWREQ,
-	GEN6_RC_VIDEO_FREQ,
-	RING_TAIL(RENDER_RING_BASE),
-	RING_TAIL(GEN6_BSD_RING_BASE),
-	RING_TAIL(VEBOX_RING_BASE),
-	RING_TAIL(BLT_RING_BASE),
-	/* TODO: Other registers are not yet used */
-};
-
-static bool is_gen8_shadowed(struct drm_i915_private *dev_priv,
-			     i915_reg_t reg)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++)
-		if (i915_mmio_reg_equal(reg, gen8_shadowed_regs[i]))
-			return true;
-
-	return false;
-}
-
 #define __gen8_write(x) \
 static void \
 gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \
+	enum forcewake_domains fw_engine; \
 	GEN6_WRITE_HEADER; \
-	if (NEEDS_FORCE_WAKE(offset) && !is_gen8_shadowed(dev_priv, reg)) \
-		__force_wake_auto(dev_priv, FORCEWAKE_RENDER); \
+	fw_engine = __gen8_reg_write_fw_domains(offset); \
+	if (fw_engine) \
+		__force_wake_auto(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
@@ -977,64 +1074,22 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool
 #define __chv_write(x) \
 static void \
 chv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \
-	enum forcewake_domains fw_engine = 0; \
+	enum forcewake_domains fw_engine; \
 	GEN6_WRITE_HEADER; \
-	if (!NEEDS_FORCE_WAKE(offset) || \
-	    is_gen8_shadowed(dev_priv, reg)) \
-		fw_engine = 0; \
-	else if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER; \
-	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_MEDIA; \
-	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
+	fw_engine = __chv_reg_write_fw_domains(offset); \
 	if (fw_engine) \
 		__force_wake_auto(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
 
-static const i915_reg_t gen9_shadowed_regs[] = {
-	RING_TAIL(RENDER_RING_BASE),
-	RING_TAIL(GEN6_BSD_RING_BASE),
-	RING_TAIL(VEBOX_RING_BASE),
-	RING_TAIL(BLT_RING_BASE),
-	FORCEWAKE_BLITTER_GEN9,
-	FORCEWAKE_RENDER_GEN9,
-	FORCEWAKE_MEDIA_GEN9,
-	GEN6_RPNSWREQ,
-	GEN6_RC_VIDEO_FREQ,
-	/* TODO: Other registers are not yet used */
-};
-
-static bool is_gen9_shadowed(struct drm_i915_private *dev_priv,
-			     i915_reg_t reg)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(gen9_shadowed_regs); i++)
-		if (i915_mmio_reg_equal(reg, gen9_shadowed_regs[i]))
-			return true;
-
-	return false;
-}
-
 #define __gen9_write(x) \
 static void \
 gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
 		bool trace) { \
 	enum forcewake_domains fw_engine; \
 	GEN6_WRITE_HEADER; \
-	if (!SKL_NEEDS_FORCE_WAKE(offset) || \
-	    is_gen9_shadowed(dev_priv, reg)) \
-		fw_engine = 0; \
-	else if (FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER; \
-	else if (FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_MEDIA; \
-	else if (FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(offset)) \
-		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
-	else \
-		fw_engine = FORCEWAKE_BLITTER; \
+	fw_engine = __gen9_reg_write_fw_domains(offset); \
 	if (fw_engine) \
 		__force_wake_auto(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
-- 
1.9.1

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

[PATCH 2/3] drm/i915: Remove forcewake request registers from the shadowed table

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Chris Wilson points out that we can remove them from the array
since they are always written to with raw accessors.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 22ccaf66b476..b77bdf4a47f6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -587,7 +587,6 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
 })
 
 static const i915_reg_t gen8_shadowed_regs[] = {
-	FORCEWAKE_MT,
 	GEN6_RPNSWREQ,
 	GEN6_RC_VIDEO_FREQ,
 	RING_TAIL(RENDER_RING_BASE),
@@ -723,9 +722,6 @@ static const i915_reg_t gen9_shadowed_regs[] = {
 	RING_TAIL(GEN6_BSD_RING_BASE),
 	RING_TAIL(VEBOX_RING_BASE),
 	RING_TAIL(BLT_RING_BASE),
-	FORCEWAKE_BLITTER_GEN9,
-	FORCEWAKE_RENDER_GEN9,
-	FORCEWAKE_MEDIA_GEN9,
 	GEN6_RPNSWREQ,
 	GEN6_RC_VIDEO_FREQ,
 	/* TODO: Other registers are not yet used */
-- 
1.9.1

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

[PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Rather than blindly waking up all forcewake domains on command
submission, we can teach each engine what is (or are) the correct
one to take.

On platforms with multiple forcewake domains like VLV, CHV, SKL
and BXT, this has the potential of lowering the GPU and CPU
power use and submission latency.

To implement it we add two new functions named
intel_reg_read_fw_domains and intel_reg_write_fw_domains, whose
purpose is to query which forcewake domains need to be taken
to read or write a specific register with raw mmio accessors.

These enables the execlists engine setup  to query which
forcewake domains are relevant per engine on the currently
running platform.

v2:
  * Kerneldoc.
  * Split from intel_uncore.c macro extraction, WARN_ON,
    no warns on old platforms. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h         |  6 +++
 drivers/gpu/drm/i915/intel_lrc.c        | 18 +++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
 drivers/gpu/drm/i915/intel_uncore.c     | 91 +++++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ebd3ff02803..55fec504b19f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -633,6 +633,12 @@ enum forcewake_domains {
 			 FORCEWAKE_MEDIA)
 };
 
+enum forcewake_domains
+intel_reg_read_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg);
+
+enum forcewake_domains
+intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg);
+
 struct intel_uncore_funcs {
 	void (*force_wake_get)(struct drm_i915_private *dev_priv,
 							enum forcewake_domains domains);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a1db6a02cf23..cac387f38cf6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -418,6 +418,7 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
 				      struct drm_i915_gem_request *rq1)
 {
 	struct drm_i915_private *dev_priv = rq0->i915;
+	unsigned int fw_domains = rq0->engine->fw_domains_elsp;
 
 	execlists_update_context(rq0);
 
@@ -425,11 +426,11 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
 		execlists_update_context(rq1);
 
 	spin_lock_irq(&dev_priv->uncore.lock);
-	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
 
 	execlists_elsp_write(rq0, rq1);
 
-	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
 	spin_unlock_irq(&dev_priv->uncore.lock);
 }
 
@@ -552,7 +553,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 	unsigned int csb_read = 0, i;
 	unsigned int submit_contexts = 0;
 
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_get(dev_priv, engine->fw_domains_csb);
 
 	status_pointer = I915_READ_FW(RING_CONTEXT_STATUS_PTR(engine));
 
@@ -577,7 +578,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 		      _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK,
 				    engine->next_context_status_buffer << 8));
 
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_put(dev_priv, engine->fw_domains_csb);
 
 	spin_lock(&engine->execlist_lock);
 
@@ -2077,7 +2078,8 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
 static int
 logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
 {
-	struct intel_context *dctx = to_i915(dev)->kernel_context;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_context *dctx = dev_priv->kernel_context;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -2099,6 +2101,12 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
 
 	logical_ring_init_platform_invariants(engine);
 
+	engine->fw_domains_elsp =
+		intel_reg_write_fw_domains(dev_priv, RING_ELSP(engine));
+	engine->fw_domains_csb =
+		intel_reg_write_fw_domains(dev_priv,
+					   RING_CONTEXT_STATUS_PTR(engine));
+
 	ret = i915_cmd_parser_init_ring(engine);
 	if (ret)
 		goto error;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 18074ab55f61..fd248cde7164 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -270,6 +270,7 @@ struct  intel_engine_cs {
 	spinlock_t execlist_lock; /* used inside tasklet, use spin_lock_bh */
 	struct list_head execlist_queue;
 	struct list_head execlist_retired_req_list;
+	unsigned int fw_domains_elsp, fw_domains_csb;
 	unsigned int next_context_status_buffer;
 	unsigned int idle_lite_restore_wa;
 	bool disable_lite_restore_wa;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b77bdf4a47f6..550e480fedd4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1766,3 +1766,94 @@ intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
 
 	return false;
 }
+
+/**
+ * intel_reg_read_fw_domains - which forcewake domains are needed to read a register
+ * @dev_priv: pointer to struct drm_i915_private
+ * @reg: register in question
+ *
+ * Returns a set of forcewake domains required to be taken with for example
+ * intel_uncore_forcewake_get for the specified register to be readable with the
+ * raw mmio accessors.
+ */
+enum forcewake_domains
+intel_reg_read_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
+{
+	enum forcewake_domains fw_domains;
+
+	if (intel_vgpu_active(dev_priv->dev))
+		return 0;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 9:
+		fw_domains = __gen9_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 8:
+		if (IS_CHERRYVIEW(dev_priv))
+			fw_domains = __chv_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen6_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 7:
+	case 6:
+		if (IS_VALLEYVIEW(dev_priv))
+			fw_domains = __vlv_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen6_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	default:
+		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
+	case 5: /* forcewake was introduced with gen6 */
+	case 4:
+	case 3:
+	case 2:
+		return 0;
+	}
+
+	WARN_ON(fw_domains & ~dev_priv->uncore.fw_domains);
+
+	return fw_domains;
+}
+
+/**
+ * intel_reg_write_fw_domains - which forcewake domains are needed to write a register
+ * @dev_priv: pointer to struct drm_i915_private
+ * @reg: register in question
+ *
+ * Returns a set of forcewake domains required to be taken with for example
+ * intel_uncore_forcewake_get for the specified register to be writable with the
+ * raw mmio accessors.
+ */
+enum forcewake_domains
+intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
+{
+	enum forcewake_domains fw_domains;
+
+	if (intel_vgpu_active(dev_priv->dev))
+		return 0;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 9:
+		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 8:
+		if (IS_CHERRYVIEW(dev_priv))
+			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	default:
+		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
+	case 7:
+	case 6:
+	case 5:
+	case 4:
+	case 3:
+	case 2:
+		return 0;
+	}
+
+	WARN_ON(fw_domains & ~dev_priv->uncore.fw_domains);
+
+	return fw_domains;
+}
-- 
1.9.1

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

Re: [PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
> @@ -2099,6 +2101,12 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>  
>  	logical_ring_init_platform_invariants(engine);
>  
> +	engine->fw_domains_elsp =
> +		intel_reg_write_fw_domains(dev_priv, RING_ELSP(engine));
> +	engine->fw_domains_csb =
> +		intel_reg_write_fw_domains(dev_priv,
> +					   RING_CONTEXT_STATUS_PTR(engine));

So is write a superset of fw? Tends to be reads that require fw more
than writes (gen6/7 fifo, gen8 write shadowing).

I think we need a READ | WRITE direction field.

> +/**
> + * intel_reg_write_fw_domains - which forcewake domains are needed to write a register
> + * @dev_priv: pointer to struct drm_i915_private
> + * @reg: register in question
> + *
> + * Returns a set of forcewake domains required to be taken with for example
> + * intel_uncore_forcewake_get for the specified register to be writable with the
> + * raw mmio accessors.
> + */
> +enum forcewake_domains
> +intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
> +{
> +	enum forcewake_domains fw_domains;
> +
> +	if (intel_vgpu_active(dev_priv->dev))
> +		return 0;
> +
> +	switch (INTEL_INFO(dev_priv)->gen) {
> +	case 9:
> +		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
> +		break;
> +	case 8:
> +		if (IS_CHERRYVIEW(dev_priv))
> +			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
> +		else
> +			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
> +		break;
> +	default:
> +		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
> +	case 7:
> +	case 6:

This is actually a tricky one. gen6/7 maintain a FIFO to store mmio
writes whilst it is powered down. If we fill that fifo we drop writes
(and that fifo is shared with functions on the device, i.e. it is not
ours to fill exclusively). So should we be saving that if you want to
make lots of writes you should take this forcewake domain. Yes. We should
report what domains they would require, it is still up to the caller as
to whether they risk the FIFO overflowing, but they should have the right
information to hand.
-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 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index a1db6a02cf23..cac387f38cf6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -418,6 +418,7 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
>  				      struct drm_i915_gem_request *rq1)
>  {
>  	struct drm_i915_private *dev_priv = rq0->i915;
> +	unsigned int fw_domains = rq0->engine->fw_domains_elsp;

So with a slightly different layout of fw that nest the elsp fw within
the tasklet handler's fw I would have a preamble like:

fw_domains = 0;
for_each_reg({ELSP, WRITE},
	     {CONTEXT_STATUS_BUF, READ},
	     {CONTEXT_STATUS_PTR, READ | WRITE})
	fw_domains |= intel_reg_fw_domains(dev_priv, reg, direction);
engine->execlist_fw_domains = fw_domains;

Hmm, we have a name clash with i915_reg_t i915_mmio_reg and
intel_uncore_forcewake_get()

intel_uncore_forcewake_for_mmio()
i915_mmio_reg_fw_domains()
-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 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Tvrtko Ursulin]

On 07/04/16 15:24, Chris Wilson wrote:
> On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
>> @@ -2099,6 +2101,12 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>>
>>   	logical_ring_init_platform_invariants(engine);
>>
>> +	engine->fw_domains_elsp =
>> +		intel_reg_write_fw_domains(dev_priv, RING_ELSP(engine));
>> +	engine->fw_domains_csb =
>> +		intel_reg_write_fw_domains(dev_priv,
>> +					   RING_CONTEXT_STATUS_PTR(engine));
>
> So is write a superset of fw? Tends to be reads that require fw more
> than writes (gen6/7 fifo, gen8 write shadowing).
>
> I think we need a READ | WRITE direction field.

Hm, yes embedding too much knowledge in the caller, how about just:

	engine->fw_domains_csb = intel_reg_read_fw_domains(dev_priv,
				 RING_CONTEXT_STATUS_PTR(engine));

	engine->fw_domains_csb |= intel_reg_write_fw_domains(dev_priv,
				  RING_CONTEXT_STATUS_PTR(engine));

?

>> +/**
>> + * intel_reg_write_fw_domains - which forcewake domains are needed to write a register
>> + * @dev_priv: pointer to struct drm_i915_private
>> + * @reg: register in question
>> + *
>> + * Returns a set of forcewake domains required to be taken with for example
>> + * intel_uncore_forcewake_get for the specified register to be writable with the
>> + * raw mmio accessors.
>> + */
>> +enum forcewake_domains
>> +intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
>> +{
>> +	enum forcewake_domains fw_domains;
>> +
>> +	if (intel_vgpu_active(dev_priv->dev))
>> +		return 0;
>> +
>> +	switch (INTEL_INFO(dev_priv)->gen) {
>> +	case 9:
>> +		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
>> +		break;
>> +	case 8:
>> +		if (IS_CHERRYVIEW(dev_priv))
>> +			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
>> +		else
>> +			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
>> +		break;
>> +	default:
>> +		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
>> +	case 7:
>> +	case 6:
>
> This is actually a tricky one. gen6/7 maintain a FIFO to store mmio
> writes whilst it is powered down. If we fill that fifo we drop writes
> (and that fifo is shared with functions on the device, i.e. it is not
> ours to fill exclusively). So should we be saving that if you want to
> make lots of writes you should take this forcewake domain. Yes. We should
> report what domains they would require, it is still up to the caller as
> to whether they risk the FIFO overflowing, but they should have the right
> information to hand.

Missed that. But it isn't part of forcewake domains. So what would you 
return? Fake out a new domain just complicates things and adds cost for 
everyone. Maybe better just to limit the whole thing to gen8+ and leave 
olders platforms untouched?

Regards,

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

Re: [PATCH 1/3] drm/i915: Extract knowledge of register forcewake domains

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:05:38PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Knowledge of which register per platform belonds in which
> forcewake domain was embedded in the MMIO accessors themselves.
> 
> Extract it into standalone macros so they can be used from
> new code in the following patches.
> 
> This causes GCC to compile some of the MMIO accessors slightly
> differently and grows the code a tiny amount. But none of the
> growth is on the fast-path so it does not matter hugely.
> 
> Affected sizes before:
> 
> 00000000000026f0 00000000000001a5 t gen6_read16
> 0000000000002390 00000000000001a5 t gen6_read32
> 00000000000028a0 00000000000001a5 t gen6_read64
> 
> 00000000000061d0 000000000000019e t gen8_write16
> 0000000000006510 000000000000019d t gen8_write32
> 0000000000006370 000000000000019d t gen8_write64
> 00000000000021f0 000000000000019d t gen8_write8
> 
> Affected sizes after:
> 
> 0000000000002840 00000000000001aa t gen6_read16
> 00000000000024e0 00000000000001a9 t gen6_read32
> 00000000000029f0 00000000000001a9 t gen6_read64
> 
> 0000000000004f20 00000000000001b5 t gen8_write16
> 0000000000004ba0 00000000000001b4 t gen8_write32
> 00000000000050e0 00000000000001b4 t gen8_write64
> 0000000000004d60 00000000000001b4 t gen8_write8
> 
> Other MMIO accessors are not affected in size.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

Went through the macros side-by-side and confirmed it was supposed to
only be code motion. Silly compiler.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 2/3] drm/i915: Remove forcewake request registers from the shadowed table

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:05:39PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Chris Wilson points out that we can remove them from the array
> since they are always written to with raw accessors.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Tvrtko Ursulin]

On 07/04/16 15:35, Chris Wilson wrote:
> On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index a1db6a02cf23..cac387f38cf6 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -418,6 +418,7 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
>>   				      struct drm_i915_gem_request *rq1)
>>   {
>>   	struct drm_i915_private *dev_priv = rq0->i915;
>> +	unsigned int fw_domains = rq0->engine->fw_domains_elsp;
>
> So with a slightly different layout of fw that nest the elsp fw within
> the tasklet handler's fw I would have a preamble like:
>
> fw_domains = 0;
> for_each_reg({ELSP, WRITE},
> 	     {CONTEXT_STATUS_BUF, READ},
> 	     {CONTEXT_STATUS_PTR, READ | WRITE})
> 	fw_domains |= intel_reg_fw_domains(dev_priv, reg, direction);
> engine->execlist_fw_domains = fw_domains;

I actually considered this (or-ing together for all registers).. might 
as well do it now.

> Hmm, we have a name clash with i915_reg_t i915_mmio_reg and
> intel_uncore_forcewake_get()
>
> intel_uncore_forcewake_for_mmio()
> i915_mmio_reg_fw_domains()

intel_uncore_forcewake_for_reg ?

Remaining open on what to do with gen7 and below.

Regards,

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

Re: [PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:36:04PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/04/16 15:24, Chris Wilson wrote:
> >On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
> >>@@ -2099,6 +2101,12 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> >>
> >>  	logical_ring_init_platform_invariants(engine);
> >>
> >>+	engine->fw_domains_elsp =
> >>+		intel_reg_write_fw_domains(dev_priv, RING_ELSP(engine));
> >>+	engine->fw_domains_csb =
> >>+		intel_reg_write_fw_domains(dev_priv,
> >>+					   RING_CONTEXT_STATUS_PTR(engine));
> >
> >So is write a superset of fw? Tends to be reads that require fw more
> >than writes (gen6/7 fifo, gen8 write shadowing).
> >
> >I think we need a READ | WRITE direction field.
> 
> Hm, yes embedding too much knowledge in the caller, how about just:
> 
> 	engine->fw_domains_csb = intel_reg_read_fw_domains(dev_priv,
> 				 RING_CONTEXT_STATUS_PTR(engine));
> 
> 	engine->fw_domains_csb |= intel_reg_write_fw_domains(dev_priv,
> 				  RING_CONTEXT_STATUS_PTR(engine));

See my other mail for a mockup of some code. Something along these
lines.

> >>+/**
> >>+ * intel_reg_write_fw_domains - which forcewake domains are needed to write a register
> >>+ * @dev_priv: pointer to struct drm_i915_private
> >>+ * @reg: register in question
> >>+ *
> >>+ * Returns a set of forcewake domains required to be taken with for example
> >>+ * intel_uncore_forcewake_get for the specified register to be writable with the
> >>+ * raw mmio accessors.
> >>+ */
> >>+enum forcewake_domains
> >>+intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
> >>+{
> >>+	enum forcewake_domains fw_domains;
> >>+
> >>+	if (intel_vgpu_active(dev_priv->dev))
> >>+		return 0;
> >>+
> >>+	switch (INTEL_INFO(dev_priv)->gen) {
> >>+	case 9:
> >>+		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
> >>+		break;
> >>+	case 8:
> >>+		if (IS_CHERRYVIEW(dev_priv))
> >>+			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
> >>+		else
> >>+			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
> >>+		break;
> >>+	default:
> >>+		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
> >>+	case 7:
> >>+	case 6:
> >
> >This is actually a tricky one. gen6/7 maintain a FIFO to store mmio
> >writes whilst it is powered down. If we fill that fifo we drop writes
> >(and that fifo is shared with functions on the device, i.e. it is not
> >ours to fill exclusively). So should we be saving that if you want to
> >make lots of writes you should take this forcewake domain. Yes. We should
> >report what domains they would require, it is still up to the caller as
> >to whether they risk the FIFO overflowing, but they should have the right
> >information to hand.
> 
> Missed that. But it isn't part of forcewake domains. So what would
> you return? Fake out a new domain just complicates things and adds
> cost for everyone. Maybe better just to limit the whole thing to
> gen8+ and leave olders platforms untouched?

It is the FORCEWAKE_RENDER to flush the write FIFO, it is just not clear
in the implementation (i.e. we have never done that explicitly - I do
remember at odd times counting register writes though...). Only in a few
places (over ring init) have we explicitly taken the fw domain across writes.

I'm leaning towards reporting that they do require the domain, with a
note saying about the fifo. It is a specialized interface after all that
is going to be using in fairly gen-specific paths (and erring on the
side of caution when used outside of that is wise).
-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 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:52:46PM +0100, Tvrtko Ursulin wrote:
> 
> 
> On 07/04/16 15:35, Chris Wilson wrote:
> >On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
> >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>index a1db6a02cf23..cac387f38cf6 100644
> >>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>@@ -418,6 +418,7 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
> >>  				      struct drm_i915_gem_request *rq1)
> >>  {
> >>  	struct drm_i915_private *dev_priv = rq0->i915;
> >>+	unsigned int fw_domains = rq0->engine->fw_domains_elsp;
> >
> >So with a slightly different layout of fw that nest the elsp fw within
> >the tasklet handler's fw I would have a preamble like:
> >
> >fw_domains = 0;
> >for_each_reg({ELSP, WRITE},
> >	     {CONTEXT_STATUS_BUF, READ},
> >	     {CONTEXT_STATUS_PTR, READ | WRITE})
> >	fw_domains |= intel_reg_fw_domains(dev_priv, reg, direction);
> >engine->execlist_fw_domains = fw_domains;
> 
> I actually considered this (or-ing together for all registers)..
> might as well do it now.
> 
> >Hmm, we have a name clash with i915_reg_t i915_mmio_reg and
> >intel_uncore_forcewake_get()
> >
> >intel_uncore_forcewake_for_mmio()
> >i915_mmio_reg_fw_domains()
> 
> intel_uncore_forcewake_for_reg ?

for_read / for_write if you wish to stick with two functions,
for_reg if go with a combined.

We shall leave the i915_mmio_reg_blah() for another day when there is a
clear direction for i915_reg_t.
-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

[PATCH v3 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Rather than blindly waking up all forcewake domains on command
submission, we can teach each engine what is (or are) the correct
one to take.

On platforms with multiple forcewake domains like VLV, CHV, SKL
and BXT, this has the potential of lowering the GPU and CPU
power use and submission latency.

To implement it we add a function named
intel_uncore_forcewake_for_reg whose purpose is to query which
forcewake domains need to be taken to read or write a specific
register with raw mmio accessors.

These enables the execlists engine setup  to query which
forcewake domains are relevant per engine on the currently
running platform.

v2:
  * Kerneldoc.
  * Split from intel_uncore.c macro extraction, WARN_ON,
    no warns on old platforms. (Chris Wilson)

v3:
  * Single domain per engine, mention all registers,
    bi-directional function and a new name, fix handling
    of gen6 and gen7 writes. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h         |   7 +++
 drivers/gpu/drm/i915/intel_lrc.c        |  27 ++++++--
 drivers/gpu/drm/i915/intel_lrc.h        |   1 +
 drivers/gpu/drm/i915/intel_ringbuffer.h |   1 +
 drivers/gpu/drm/i915/intel_uncore.c     | 108 ++++++++++++++++++++++++++++++++
 5 files changed, 139 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ebd3ff02803..a3f2fb201758 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -633,6 +633,13 @@ enum forcewake_domains {
 			 FORCEWAKE_MEDIA)
 };
 
+#define FW_REG_READ  (1)
+#define FW_REG_WRITE (2)
+
+enum forcewake_domains
+intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
+			       i915_reg_t reg, unsigned int op);
+
 struct intel_uncore_funcs {
 	void (*force_wake_get)(struct drm_i915_private *dev_priv,
 							enum forcewake_domains domains);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a1db6a02cf23..31445aa3429b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -418,6 +418,7 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
 				      struct drm_i915_gem_request *rq1)
 {
 	struct drm_i915_private *dev_priv = rq0->i915;
+	unsigned int fw_domains = rq0->engine->fw_domains;
 
 	execlists_update_context(rq0);
 
@@ -425,11 +426,11 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
 		execlists_update_context(rq1);
 
 	spin_lock_irq(&dev_priv->uncore.lock);
-	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
 
 	execlists_elsp_write(rq0, rq1);
 
-	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
 	spin_unlock_irq(&dev_priv->uncore.lock);
 }
 
@@ -552,7 +553,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 	unsigned int csb_read = 0, i;
 	unsigned int submit_contexts = 0;
 
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_get(dev_priv, engine->fw_domains);
 
 	status_pointer = I915_READ_FW(RING_CONTEXT_STATUS_PTR(engine));
 
@@ -577,7 +578,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 		      _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK,
 				    engine->next_context_status_buffer << 8));
 
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+	intel_uncore_forcewake_put(dev_priv, engine->fw_domains);
 
 	spin_lock(&engine->execlist_lock);
 
@@ -2077,7 +2078,9 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
 static int
 logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
 {
-	struct intel_context *dctx = to_i915(dev)->kernel_context;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_context *dctx = dev_priv->kernel_context;
+	enum forcewake_domains fw_domains;
 	int ret;
 
 	/* Intentionally left blank. */
@@ -2099,6 +2102,20 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
 
 	logical_ring_init_platform_invariants(engine);
 
+	fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
+						    RING_ELSP(engine),
+						    FW_REG_WRITE);
+
+	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+						     RING_CONTEXT_STATUS_PTR(engine),
+						     FW_REG_READ | FW_REG_WRITE);
+
+	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+						     RING_CONTEXT_STATUS_BUF_BASE(engine),
+						     FW_REG_READ);
+
+	engine->fw_domains = fw_domains;
+
 	ret = i915_cmd_parser_init_ring(engine);
 	if (ret)
 		goto error;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 0b0853eee91e..8de1ea536ad4 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -34,6 +34,7 @@
 #define	  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH	(1 << 3)
 #define	  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT	(1 << 0)
 #define   CTX_CTRL_RS_CTX_ENABLE                (1 << 1)
+#define RING_CONTEXT_STATUS_BUF_BASE(ring)	_MMIO((ring)->mmio_base + 0x370)
 #define RING_CONTEXT_STATUS_BUF_LO(ring, i)	_MMIO((ring)->mmio_base + 0x370 + (i) * 8)
 #define RING_CONTEXT_STATUS_BUF_HI(ring, i)	_MMIO((ring)->mmio_base + 0x370 + (i) * 8 + 4)
 #define RING_CONTEXT_STATUS_PTR(ring)		_MMIO((ring)->mmio_base + 0x3a0)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 18074ab55f61..3a11705222fc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -270,6 +270,7 @@ struct  intel_engine_cs {
 	spinlock_t execlist_lock; /* used inside tasklet, use spin_lock_bh */
 	struct list_head execlist_queue;
 	struct list_head execlist_retired_req_list;
+	unsigned int fw_domains;
 	unsigned int next_context_status_buffer;
 	unsigned int idle_lite_restore_wa;
 	bool disable_lite_restore_wa;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b77bdf4a47f6..a465368f5edf 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1766,3 +1766,111 @@ intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
 
 	return false;
 }
+
+static enum forcewake_domains
+intel_uncore_forcewake_for_read(struct drm_i915_private *dev_priv,
+				i915_reg_t reg)
+{
+	enum forcewake_domains fw_domains;
+
+	if (intel_vgpu_active(dev_priv->dev))
+		return 0;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 9:
+		fw_domains = __gen9_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 8:
+		if (IS_CHERRYVIEW(dev_priv))
+			fw_domains = __chv_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen6_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 7:
+	case 6:
+		if (IS_VALLEYVIEW(dev_priv))
+			fw_domains = __vlv_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen6_reg_read_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	default:
+		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
+	case 5: /* forcewake was introduced with gen6 */
+	case 4:
+	case 3:
+	case 2:
+		return 0;
+	}
+
+	WARN_ON(fw_domains & ~dev_priv->uncore.fw_domains);
+
+	return fw_domains;
+}
+
+static enum forcewake_domains
+intel_uncore_forcewake_for_write(struct drm_i915_private *dev_priv,
+				 i915_reg_t reg)
+{
+	enum forcewake_domains fw_domains;
+
+	if (intel_vgpu_active(dev_priv->dev))
+		return 0;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 9:
+		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 8:
+		if (IS_CHERRYVIEW(dev_priv))
+			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 7:
+	case 6:
+		fw_domains = FORCEWAKE_RENDER;
+		break;
+	default:
+		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
+	case 5:
+	case 4:
+	case 3:
+	case 2:
+		return 0;
+	}
+
+	WARN_ON(fw_domains & ~dev_priv->uncore.fw_domains);
+
+	return fw_domains;
+}
+
+/**
+ * intel_uncore_forcewake_for_reg - which forcewake domains are needed to access
+ * 				    a register
+ * @dev_priv: pointer to struct drm_i915_private
+ * @reg: register in question
+ * @op: operation bitmask of FW_REG_READ and/or FW_REG_WRITE
+ *
+ * Returns a set of forcewake domains required to be taken with for example
+ * intel_uncore_forcewake_get for the specified register to be accessible in the
+ * specified mode (read, write or read/write) with raw mmio accessors.
+ *
+ * NOTE: On Gen6 and Gen7 write forcewake domain (FORCEWAKE_RENDER) requires the
+ * callers to do FIFO management on their own or risk losing writes.
+ */
+enum forcewake_domains
+intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
+			       i915_reg_t reg, unsigned int op)
+{
+	enum forcewake_domains fw_domains = 0;
+
+	WARN_ON(!op);
+
+	if (op & FW_REG_READ)
+		fw_domains = intel_uncore_forcewake_for_read(dev_priv, reg);
+
+	if (op & FW_REG_WRITE)
+		fw_domains |= intel_uncore_forcewake_for_write(dev_priv, reg);
+
+	return fw_domains;
+}
-- 
1.9.1

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

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Extract knowledge of register forcewake domains

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Extract knowledge of register forcewake domains
URL   : https://patchwork.freedesktop.org/series/5419/
State : failure

== Summary ==

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

Test drv_getparams_basic:
        Subgroup basic-subslice-total:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_basic:
        Subgroup bad-close:
                dmesg-warn -> INCOMPLETE (bsw-nuc-2)
        Subgroup create-close:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup create-fd-close:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_ctx_param_basic:
        Subgroup basic:
                dmesg-warn -> INCOMPLETE (bsw-nuc-2)
        Subgroup invalid-ctx-get:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup invalid-ctx-set:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup invalid-param-set:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_ctx_switch:
        Subgroup basic-default:
                skip       -> INCOMPLETE (bsw-nuc-2)
Test gem_exec_basic:
        Subgroup basic-blt:
                skip       -> INCOMPLETE (bsw-nuc-2)
        Subgroup gtt-bsd:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup readonly-bsd:
                skip       -> INCOMPLETE (bsw-nuc-2)
        Subgroup readonly-bsd2:
                skip       -> INCOMPLETE (bsw-nuc-2)
        Subgroup readonly-vebox:
                skip       -> PASS       (bsw-nuc-2)
Test gem_exec_store:
        Subgroup basic-bsd:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup basic-vebox:
                skip       -> PASS       (bsw-nuc-2)
Test gem_exec_whisper:
        Subgroup basic:
                skip       -> INCOMPLETE (bsw-nuc-2)
                pass       -> INCOMPLETE (skl-i7k-2)
Test gem_mmap:
        Subgroup basic-small-bo:
                dmesg-warn -> INCOMPLETE (bsw-nuc-2)
Test gem_mmap_gtt:
        Subgroup basic-write-gtt:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-write-read:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test gem_render_linear_blits:
        Subgroup basic:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test gem_ringfill:
        Subgroup basic-default-hang:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup basic-default-interruptible:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-all:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup basic-blt:
                skip       -> PASS       (bsw-nuc-2)
        Subgroup basic-bsd:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup basic-bsd2:
                skip       -> INCOMPLETE (bsw-nuc-2)
Test gem_tiled_pread_basic:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_addfb_basic:
        Subgroup addfb25-modifier-no-flag:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-x-tiled:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup framebuffer-vs-set-tiling:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup small-bo:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup tile-pitch-mismatch:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup too-high:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup too-wide:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-fail -> PASS       (bsw-nuc-2)
Test kms_force_connector_basic:
        Subgroup force-edid:
                skip       -> INCOMPLETE (bsw-nuc-2)
                skip       -> PASS       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> SKIP       (hsw-gt2)
Test prime_self_import:
        Subgroup basic-with_fd_dup:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-with_one_bo_two_files:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:104  dwarn:14  dfail:6   fail:0   skip:41 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:178  dwarn:0   dfail:0   fail:0   skip:18 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:16   pass:14   dwarn:0   dfail:0   fail:0   skip:1  
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 

Results at /archive/results/CI_IGT_test/Patchwork_1831/

851708c7e97537ed618fadbe5d342eaf8fa5146d drm-intel-nightly: 2016y-04m-07d-13h-56m-00s UTC integration manifest
8c10088d5b2129239dd68ae967108cb23ad9bc28 drm/i915: Only grab correct forcewake for the engine with execlists
e0089ff60bb4d7996ac6e3e44a245bd4cfd553d2 drm/i915: Remove forcewake request registers from the shadowed table
cc488b32a9f3860ba255c6fb863980eff9d38814 drm/i915: Extract knowledge of register forcewake domains

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

Re: [PATCH v3 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Chris Wilson]

On Thu, Apr 07, 2016 at 04:56:00PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Rather than blindly waking up all forcewake domains on command
> submission, we can teach each engine what is (or are) the correct
> one to take.
> 
> On platforms with multiple forcewake domains like VLV, CHV, SKL
> and BXT, this has the potential of lowering the GPU and CPU
> power use and submission latency.
> 
> To implement it we add a function named
> intel_uncore_forcewake_for_reg whose purpose is to query which
> forcewake domains need to be taken to read or write a specific
> register with raw mmio accessors.
> 
> These enables the execlists engine setup  to query which
> forcewake domains are relevant per engine on the currently
> running platform.
> 
> v2:
>   * Kerneldoc.
>   * Split from intel_uncore.c macro extraction, WARN_ON,
>     no warns on old platforms. (Chris Wilson)
> 
> v3:
>   * Single domain per engine, mention all registers,
>     bi-directional function and a new name, fix handling
>     of gen6 and gen7 writes. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

> +/**
> + * intel_uncore_forcewake_for_reg - which forcewake domains are needed to access
> + * 				    a register
> + * @dev_priv: pointer to struct drm_i915_private
> + * @reg: register in question
> + * @op: operation bitmask of FW_REG_READ and/or FW_REG_WRITE
> + *
> + * Returns a set of forcewake domains required to be taken with for example
> + * intel_uncore_forcewake_get for the specified register to be accessible in the
> + * specified mode (read, write or read/write) with raw mmio accessors.
> + *
> + * NOTE: On Gen6 and Gen7 write forcewake domain (FORCEWAKE_RENDER) requires the
> + * callers to do FIFO management on their own or risk losing writes.
> + */
> +enum forcewake_domains
> +intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
> +			       i915_reg_t reg, unsigned int op)
> +{
> +	enum forcewake_domains fw_domains = 0;
> +
> +	WARN_ON(!op);
> +
> +	if (op & FW_REG_READ)
> +		fw_domains = intel_uncore_forcewake_for_read(dev_priv, reg);
> +
> +	if (op & FW_REG_WRITE)
> +		fw_domains |= intel_uncore_forcewake_for_write(dev_priv, reg);
> +
> +	return fw_domains;
> +}

Like it, like it a lot.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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