[PATCH v2 0/6] i915: Simplify mmio handling & add new DG2 shadow table

[PATCH v2 0/6] i915: Simplify mmio handling & add new DG2 shadow table

[Matt Roper]

Our uncore MMIO functions for reading/writing registers have become very
complicated over time.  There's significant macro magic used to generate
several nearly-identical functions that only really differ in terms of
which platform-specific shadow register table they should check on write
operations.  We can significantly simplify our MMIO handlers by storing
a reference to the current platform's shadow table within the 'struct
intel_uncore' the same way we already do for forcewake; this allows us
to consolidate the multiple variants of each 'write' function down to
just a single 'fwtable' version that gets the shadow table out of the
uncore struct rather than hardcoding the name of a specific platform's
table.  We can do similar consolidation on the MMIO read side by
creating a single-entry forcewake table to replace the open-coded range
check they had been using previously.

The final patch of the series adds a new shadow table for DG2; this
becomes quite clean and simple now, given the refactoring in the first
five patches.

Aside from simplifying the code signficantly, this series reduces the
size of the generated .ko in exchange for adding an extra pointer
indirection to access the tables.  The size deltas (for just the first
five patches, before we add an additional table in the final patch) are:

Old:
        $ size drivers/gpu/drm/i915/i915.ko
           text    data     bss     dec     hex filename
        2865921   88972    2912 2957805  2d21ed drivers/gpu/drm/i915/i915.ko

New:
        $ size drivers/gpu/drm/i915/i915.ko
           text    data     bss     dec     hex filename
        2854181   88236    2912 2945329  2cf131 drivers/gpu/drm/i915/i915.ko

The code size deltas will become larger as we add more platforms; we
already add one new platform table in the final patch of this series and
our next few platforms are all expected to bring new shadow tables as
well.

I don't think the impact of the indirect table reference for shadow
tables should be a concern for a few reasons:
 * The stored table + indirect lookup design is already deemed good
   enough for forcewake, which is used more frequently (both reads and
   writes, compared to shadow tables which are only used for writes) and
   operates on much larger tables.
 * Performance-critical sections of the code or those read/writing lots
   of registers in a batch usually do an explicit grab of the relevant
   forcewake domains and then perform their MMIO operations via *_fw()
   functions without considering shadowed registers and bypassing all of
   the table lookups.
 * In v2 of the series, we still apply NEEDS_FORCE_WAKE() checks that
   will bypass all of the forcewake and shadow logic for display
   register writes.

v2:
 - Drop orphaned definition of __gen6_reg_read_fw_domains. (Tvrtko)
 - Restore NEEDS_FORCE_WAKE() check to
   __fwtable_reg_{read,write}_fw_domains, but update the definition of
   NEEDS_FORCE_WAKE to also return 'true' on offsets above
   GEN11_BSD_RING_BASE for compatibility with gen11+ platforms. (Chris,
   Tvrtko).

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

Matt Roper (6):
  drm/i915/uncore: Convert gen6/gen7 read operations to fwtable
  drm/i915/uncore: Associate shadow table with uncore
  drm/i915/uncore: Replace gen8 write functions with general fwtable
  drm/i915/uncore: Drop gen11/gen12 mmio write handlers
  drm/i915/uncore: Drop gen11 mmio read handlers
  drm/i915/dg2: Add DG2-specific shadow register table

 drivers/gpu/drm/i915/intel_uncore.c           | 200 ++++++++++--------
 drivers/gpu/drm/i915/intel_uncore.h           |   7 +
 drivers/gpu/drm/i915/selftests/intel_uncore.c |   1 +
 3 files changed, 115 insertions(+), 93 deletions(-)

-- 
2.25.4

[PATCH v2 1/6] drm/i915/uncore: Convert gen6/gen7 read operations to fwtable

[Matt Roper]

On gen6-gen8 (except vlv/chv) we don't use a forcewake lookup table; we
simply check whether the register offset is < 0x40000, and return
FORCEWAKE_RENDER if it is.  To prepare for upcoming refactoring, let's
define a single-entry forcewake table from [0x0, 0x3ffff] and switch
these platforms over to use the fwtable reader functions.

v2:
 - Drop __gen6_reg_read_fw_domains which is no longer used.  (Tvrtko)

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f9767054dbdf..8c09af1e9f7a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -853,16 +853,6 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
 
-#define __gen6_reg_read_fw_domains(uncore, offset) \
-({ \
-	enum forcewake_domains __fwd; \
-	if (NEEDS_FORCE_WAKE(offset)) \
-		__fwd = FORCEWAKE_RENDER; \
-	else \
-		__fwd = 0; \
-	__fwd; \
-})
-
 static int fw_range_cmp(u32 offset, const struct intel_forcewake_range *entry)
 {
 	if (offset < entry->start)
@@ -1064,6 +1054,10 @@ gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
 	__fwd; \
 })
 
+static const struct intel_forcewake_range __gen6_fw_ranges[] = {
+	GEN_FW_RANGE(0x0, 0x3ffff, FORCEWAKE_RENDER),
+};
+
 /* *Must* be sorted by offset ranges! See intel_fw_table_check(). */
 static const struct intel_forcewake_range __chv_fw_ranges[] = {
 	GEN_FW_RANGE(0x2000, 0x3fff, FORCEWAKE_RENDER),
@@ -1623,7 +1617,6 @@ __gen_read(func, 64)
 
 __gen_reg_read_funcs(gen11_fwtable);
 __gen_reg_read_funcs(fwtable);
-__gen_reg_read_funcs(gen6);
 
 #undef __gen_reg_read_funcs
 #undef GEN6_READ_FOOTER
@@ -2111,15 +2104,17 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (GRAPHICS_VER(i915) == 8) {
+		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (IS_VALLEYVIEW(i915)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __vlv_fw_ranges);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (IS_GRAPHICS_VER(i915, 6, 7)) {
+		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	}
 
 	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
-- 
2.25.4

[PATCH v2 2/6] drm/i915/uncore: Associate shadow table with uncore

[Matt Roper]

Store a reference to a platform's shadow table inside the uncore, the
same as we do with the forcewake table.  This will allow us to use a
single set of functions that operate on the shadow table reference
rather than generating lots of nearly-identical functions via macros
that differ only in terms of the table that they reference.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 40 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_uncore.h |  7 +++++
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8c09af1e9f7a..5fa2bf26a948 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1026,17 +1026,19 @@ static int mmio_range_cmp(u32 key, const struct i915_range *range)
 		return 0;
 }
 
-#define __is_X_shadowed(x) \
-static bool is_##x##_shadowed(u32 offset) \
-{ \
-	const struct i915_range *regs = x##_shadowed_regs; \
-	return BSEARCH(offset, regs, ARRAY_SIZE(x##_shadowed_regs), \
+static bool
+is_shadowed(struct intel_uncore *uncore, u32 offset)
+{
+	if (drm_WARN_ON(&uncore->i915->drm, !uncore->shadowed_reg_table))
+		return false;
+
+	return BSEARCH(offset,
+		       uncore->shadowed_reg_table,
+		       uncore->shadowed_reg_table_entries,
 		       mmio_range_cmp); \
 }
 
-__is_X_shadowed(gen8)
-__is_X_shadowed(gen11)
-__is_X_shadowed(gen12)
+
 
 static enum forcewake_domains
 gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
@@ -1047,7 +1049,7 @@ gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
 #define __gen8_reg_write_fw_domains(uncore, offset) \
 ({ \
 	enum forcewake_domains __fwd; \
-	if (NEEDS_FORCE_WAKE(offset) && !is_gen8_shadowed(offset)) \
+	if (NEEDS_FORCE_WAKE(offset) && !is_shadowed(uncore, offset)) \
 		__fwd = FORCEWAKE_RENDER; \
 	else \
 		__fwd = 0; \
@@ -1081,7 +1083,7 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
 #define __fwtable_reg_write_fw_domains(uncore, offset) \
 ({ \
 	enum forcewake_domains __fwd = 0; \
-	if (NEEDS_FORCE_WAKE((offset)) && !is_gen8_shadowed(offset)) \
+	if (NEEDS_FORCE_WAKE((offset)) && !is_shadowed(uncore, offset)) \
 		__fwd = find_fw_domain(uncore, offset); \
 	__fwd; \
 })
@@ -1090,7 +1092,7 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
 ({ \
 	enum forcewake_domains __fwd = 0; \
 	const u32 __offset = (offset); \
-	if (!is_gen11_shadowed(__offset)) \
+	if (!is_shadowed(uncore, __offset)) \
 		__fwd = find_fw_domain(uncore, __offset); \
 	__fwd; \
 })
@@ -1099,7 +1101,7 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
 ({ \
 	enum forcewake_domains __fwd = 0; \
 	const u32 __offset = (offset); \
-	if (!is_gen12_shadowed(__offset)) \
+	if (!is_shadowed(uncore, __offset)) \
 		__fwd = find_fw_domain(uncore, __offset); \
 	__fwd; \
 })
@@ -1705,6 +1707,7 @@ __gen_write(func, 8) \
 __gen_write(func, 16) \
 __gen_write(func, 32)
 
+
 __gen_reg_write_funcs(gen12_fwtable);
 __gen_reg_write_funcs(gen11_fwtable);
 __gen_reg_write_funcs(fwtable);
@@ -1969,6 +1972,12 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 	(uncore)->fw_domains_table_entries = ARRAY_SIZE((d)); \
 }
 
+#define ASSIGN_SHADOW_TABLE(uncore, d) \
+{ \
+	(uncore)->shadowed_reg_table = d; \
+	(uncore)->shadowed_reg_table_entries = ARRAY_SIZE((d)); \
+}
+
 static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
 					 unsigned long action, void *data)
 {
@@ -2081,30 +2090,37 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
 
 	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 55)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (GRAPHICS_VER(i915) >= 12) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (GRAPHICS_VER(i915) == 11) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (IS_CHERRYVIEW(i915)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (GRAPHICS_VER(i915) == 8) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
+		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (IS_VALLEYVIEW(i915)) {
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 531665b08039..2f31c50eeae2 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -142,6 +142,13 @@ struct intel_uncore {
 	const struct intel_forcewake_range *fw_domains_table;
 	unsigned int fw_domains_table_entries;
 
+	/*
+	 * Shadowed registers are special cases where we can safely write
+	 * to the register *without* grabbing forcewake.
+	 */
+	const struct i915_range *shadowed_reg_table;
+	unsigned int shadowed_reg_table_entries;
+
 	struct notifier_block pmic_bus_access_nb;
 	struct intel_uncore_funcs funcs;
 
-- 
2.25.4

[PATCH v2 3/6] drm/i915/uncore: Replace gen8 write functions with general fwtable

[Matt Roper]

Now that we have both a standard forcewake table (albeit a single-entry
table) and the shadow table stored in the uncore, we can drop the
gen8-specific write handlers in favor of the general fwtable version.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 5fa2bf26a948..4c6898746d10 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1046,16 +1046,6 @@ gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
 	return FORCEWAKE_RENDER;
 }
 
-#define __gen8_reg_write_fw_domains(uncore, offset) \
-({ \
-	enum forcewake_domains __fwd; \
-	if (NEEDS_FORCE_WAKE(offset) && !is_shadowed(uncore, offset)) \
-		__fwd = FORCEWAKE_RENDER; \
-	else \
-		__fwd = 0; \
-	__fwd; \
-})
-
 static const struct intel_forcewake_range __gen6_fw_ranges[] = {
 	GEN_FW_RANGE(0x0, 0x3ffff, FORCEWAKE_RENDER),
 };
@@ -1711,7 +1701,6 @@ __gen_write(func, 32)
 __gen_reg_write_funcs(gen12_fwtable);
 __gen_reg_write_funcs(gen11_fwtable);
 __gen_reg_write_funcs(fwtable);
-__gen_reg_write_funcs(gen8);
 
 #undef __gen_reg_write_funcs
 #undef GEN6_WRITE_FOOTER
@@ -2121,7 +2110,7 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
 	} else if (GRAPHICS_VER(i915) == 8) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
+		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (IS_VALLEYVIEW(i915)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __vlv_fw_ranges);
-- 
2.25.4

[PATCH v2 4/6] drm/i915/uncore: Drop gen11/gen12 mmio write handlers

[Matt Roper]

Now that the reference to the shadow table is stored within the uncore,
we don't need to generate separate fwtable, gen11_fwtable, and
gen12_fwtable variants of the register write functions; a single
'fwtable' implementation will work for all of those platforms now.

While consolidating the functions, gen11/gen12 pick up a
NEEDS_FORCE_WAKE() check that they didn't have before, allowing them to
bypass a lot of forcewake/shadow checking for non-GT registers (e.g.,
display).  However since these later platforms also introduce media
engines at higher MMIO offsets, the definition of NEEDS_FORCE_WAKE() is
extended to also consider register offsets above GEN11_BSD_RING_BASE.

v2:
 - Restore NEEDS_FORCE_WAKE(), but extend it for compatibility with the
   gen11+ platforms by also passing offsets above GEN11_BSD_RING_BASE.
   (Chris, Tvrtko)

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 61 ++++++++++-------------------
 1 file changed, 21 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 4c6898746d10..bfb2a6337f9d 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -851,7 +851,10 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
 }
 
 /* We give fast paths for the really cool registers */
-#define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
+#define NEEDS_FORCE_WAKE(reg) ({ \
+	u32 __reg = (reg); \
+	__reg < 0x40000 || __reg >= GEN11_BSD_RING_BASE; \
+})
 
 static int fw_range_cmp(u32 offset, const struct intel_forcewake_range *entry)
 {
@@ -1071,27 +1074,10 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
 };
 
 #define __fwtable_reg_write_fw_domains(uncore, offset) \
-({ \
-	enum forcewake_domains __fwd = 0; \
-	if (NEEDS_FORCE_WAKE((offset)) && !is_shadowed(uncore, offset)) \
-		__fwd = find_fw_domain(uncore, offset); \
-	__fwd; \
-})
-
-#define __gen11_fwtable_reg_write_fw_domains(uncore, offset) \
 ({ \
 	enum forcewake_domains __fwd = 0; \
 	const u32 __offset = (offset); \
-	if (!is_shadowed(uncore, __offset)) \
-		__fwd = find_fw_domain(uncore, __offset); \
-	__fwd; \
-})
-
-#define __gen12_fwtable_reg_write_fw_domains(uncore, offset) \
-({ \
-	enum forcewake_domains __fwd = 0; \
-	const u32 __offset = (offset); \
-	if (!is_shadowed(uncore, __offset)) \
+	if (NEEDS_FORCE_WAKE((__offset)) && !is_shadowed(uncore, __offset)) \
 		__fwd = find_fw_domain(uncore, __offset); \
 	__fwd; \
 })
@@ -1675,34 +1661,29 @@ __gen6_write(8)
 __gen6_write(16)
 __gen6_write(32)
 
-#define __gen_write(func, x) \
+#define __gen_fwtable_write(x) \
 static void \
-func##_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { \
+fwtable_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { \
 	enum forcewake_domains fw_engine; \
 	GEN6_WRITE_HEADER; \
-	fw_engine = __##func##_reg_write_fw_domains(uncore, offset); \
+	fw_engine = __fwtable_reg_write_fw_domains(uncore, offset); \
 	if (fw_engine) \
 		__force_wake_auto(uncore, fw_engine); \
 	__raw_uncore_write##x(uncore, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
 
-#define __gen_reg_write_funcs(func) \
-static enum forcewake_domains \
-func##_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
-	return __##func##_reg_write_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
-} \
-\
-__gen_write(func, 8) \
-__gen_write(func, 16) \
-__gen_write(func, 32)
-
+static enum forcewake_domains
+fwtable_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
+{
+	return __fwtable_reg_write_fw_domains(uncore, i915_mmio_reg_offset(reg));
+}
 
-__gen_reg_write_funcs(gen12_fwtable);
-__gen_reg_write_funcs(gen11_fwtable);
-__gen_reg_write_funcs(fwtable);
+__gen_fwtable_write(8)
+__gen_fwtable_write(16)
+__gen_fwtable_write(32)
 
-#undef __gen_reg_write_funcs
+#undef __gen_fwtable_write
 #undef GEN6_WRITE_FOOTER
 #undef GEN6_WRITE_HEADER
 
@@ -2080,22 +2061,22 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
 	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 55)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
+		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
+		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (GRAPHICS_VER(i915) >= 12) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
+		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (GRAPHICS_VER(i915) == 11) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
+		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
-- 
2.25.4

[PATCH v2 5/6] drm/i915/uncore: Drop gen11 mmio read handlers

[Matt Roper]

Consolidate down to just a single 'fwtable' implementation.  For reads
we don't need to worry about shadow tables.  Also, the
NEEDS_FORCE_WAKE() check we previously had in the fwtable implementation
can be dropped --- if a register is outside that range on one of the old
platforms, then it won't belong to any forcewake range and 0 will be
returned anyway.

v2:
 - Restore NEEDS_FORCE_WAKE() check.  (Chris, Tvrtko)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 40 ++++++++++++-----------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index bfb2a6337f9d..10f124297e7c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -935,9 +935,6 @@ static const struct intel_forcewake_range __vlv_fw_ranges[] = {
 	__fwd; \
 })
 
-#define __gen11_fwtable_reg_read_fw_domains(uncore, offset) \
-	find_fw_domain(uncore, offset)
-
 /* *Must* be sorted by offset! See intel_shadow_table_check(). */
 static const struct i915_range gen8_shadowed_regs[] = {
 	{ .start =  0x2030, .end =  0x2030 },
@@ -1570,33 +1567,30 @@ static inline void __force_wake_auto(struct intel_uncore *uncore,
 		___force_wake_auto(uncore, fw_domains);
 }
 
-#define __gen_read(func, x) \
+#define __gen_fwtable_read(x) \
 static u##x \
-func##_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { \
+fwtable_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) \
+{ \
 	enum forcewake_domains fw_engine; \
 	GEN6_READ_HEADER(x); \
-	fw_engine = __##func##_reg_read_fw_domains(uncore, offset); \
+	fw_engine = __fwtable_reg_read_fw_domains(uncore, offset); \
 	if (fw_engine) \
 		__force_wake_auto(uncore, fw_engine); \
 	val = __raw_uncore_read##x(uncore, reg); \
 	GEN6_READ_FOOTER; \
 }
 
-#define __gen_reg_read_funcs(func) \
-static enum forcewake_domains \
-func##_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
-	return __##func##_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
-} \
-\
-__gen_read(func, 8) \
-__gen_read(func, 16) \
-__gen_read(func, 32) \
-__gen_read(func, 64)
+static enum forcewake_domains
+fwtable_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) {
+	return __fwtable_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg));
+}
 
-__gen_reg_read_funcs(gen11_fwtable);
-__gen_reg_read_funcs(fwtable);
+__gen_fwtable_read(8)
+__gen_fwtable_read(16)
+__gen_fwtable_read(32)
+__gen_fwtable_read(64)
 
-#undef __gen_reg_read_funcs
+#undef __gen_fwtable_read
 #undef GEN6_READ_FOOTER
 #undef GEN6_READ_HEADER
 
@@ -2062,22 +2056,22 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (GRAPHICS_VER(i915) >= 12) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (GRAPHICS_VER(i915) == 11) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
 		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
-- 
2.25.4

[PATCH v2 6/6] drm/i915/dg2: Add DG2-specific shadow register table

[Matt Roper]

We thought the DG2 table of shadowed registers would be the same as the
gen12/xehp table, but it turns out that there are a few minor
differences that require us to define a new DG2-specific table:
 * One register is removed (0xC4D4)
 * One register is added (0xC4E0)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c           | 41 ++++++++++++++++++-
 drivers/gpu/drm/i915/selftests/intel_uncore.c |  1 +
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 10f124297e7c..b3ba710d4310 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1016,6 +1016,45 @@ static const struct i915_range gen12_shadowed_regs[] = {
 	{ .start = 0x1F8510, .end = 0x1F8550 },
 };
 
+static const struct i915_range dg2_shadowed_regs[] = {
+	{ .start =   0x2030, .end =   0x2030 },
+	{ .start =   0x2510, .end =   0x2550 },
+	{ .start =   0xA008, .end =   0xA00C },
+	{ .start =   0xA188, .end =   0xA188 },
+	{ .start =   0xA278, .end =   0xA278 },
+	{ .start =   0xA540, .end =   0xA56C },
+	{ .start =   0xC4C8, .end =   0xC4C8 },
+	{ .start =   0xC4E0, .end =   0xC4E0 },
+	{ .start =   0xC600, .end =   0xC600 },
+	{ .start =   0xC658, .end =   0xC658 },
+	{ .start =  0x22030, .end =  0x22030 },
+	{ .start =  0x22510, .end =  0x22550 },
+	{ .start = 0x1C0030, .end = 0x1C0030 },
+	{ .start = 0x1C0510, .end = 0x1C0550 },
+	{ .start = 0x1C4030, .end = 0x1C4030 },
+	{ .start = 0x1C4510, .end = 0x1C4550 },
+	{ .start = 0x1C8030, .end = 0x1C8030 },
+	{ .start = 0x1C8510, .end = 0x1C8550 },
+	{ .start = 0x1D0030, .end = 0x1D0030 },
+	{ .start = 0x1D0510, .end = 0x1D0550 },
+	{ .start = 0x1D4030, .end = 0x1D4030 },
+	{ .start = 0x1D4510, .end = 0x1D4550 },
+	{ .start = 0x1D8030, .end = 0x1D8030 },
+	{ .start = 0x1D8510, .end = 0x1D8550 },
+	{ .start = 0x1E0030, .end = 0x1E0030 },
+	{ .start = 0x1E0510, .end = 0x1E0550 },
+	{ .start = 0x1E4030, .end = 0x1E4030 },
+	{ .start = 0x1E4510, .end = 0x1E4550 },
+	{ .start = 0x1E8030, .end = 0x1E8030 },
+	{ .start = 0x1E8510, .end = 0x1E8550 },
+	{ .start = 0x1F0030, .end = 0x1F0030 },
+	{ .start = 0x1F0510, .end = 0x1F0550 },
+	{ .start = 0x1F4030, .end = 0x1F4030 },
+	{ .start = 0x1F4510, .end = 0x1F4550 },
+	{ .start = 0x1F8030, .end = 0x1F8030 },
+	{ .start = 0x1F8510, .end = 0x1F8550 },
+};
+
 static int mmio_range_cmp(u32 key, const struct i915_range *range)
 {
 	if (key < range->start)
@@ -2054,7 +2093,7 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
 
 	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 55)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
-		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
+		ASSIGN_SHADOW_TABLE(uncore, dg2_shadowed_regs);
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 22ef2c87df1a..bc8128170a99 100644
--- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
+++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
@@ -68,6 +68,7 @@ static int intel_shadow_table_check(void)
 		{ gen8_shadowed_regs, ARRAY_SIZE(gen8_shadowed_regs) },
 		{ gen11_shadowed_regs, ARRAY_SIZE(gen11_shadowed_regs) },
 		{ gen12_shadowed_regs, ARRAY_SIZE(gen12_shadowed_regs) },
+		{ dg2_shadowed_regs, ARRAY_SIZE(dg2_shadowed_regs) },
 	};
 	const struct i915_range *range;
 	unsigned int i, j;
-- 
2.25.4

Re: [PATCH v2 1/6] drm/i915/uncore: Convert gen6/gen7 read operations to fwtable

[Tvrtko Ursulin]

On 10/09/2021 21:10, Matt Roper wrote:
> On gen6-gen8 (except vlv/chv) we don't use a forcewake lookup table; we
> simply check whether the register offset is < 0x40000, and return
> FORCEWAKE_RENDER if it is.  To prepare for upcoming refactoring, let's
> define a single-entry forcewake table from [0x0, 0x3ffff] and switch
> these platforms over to use the fwtable reader functions.
> 
> v2:
>   - Drop __gen6_reg_read_fw_domains which is no longer used.  (Tvrtko)
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++-------------
>   1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index f9767054dbdf..8c09af1e9f7a 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -853,16 +853,6 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
>   /* We give fast paths for the really cool registers */
>   #define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
>   
> -#define __gen6_reg_read_fw_domains(uncore, offset) \
> -({ \
> -	enum forcewake_domains __fwd; \
> -	if (NEEDS_FORCE_WAKE(offset)) \
> -		__fwd = FORCEWAKE_RENDER; \
> -	else \
> -		__fwd = 0; \
> -	__fwd; \
> -})
> -
>   static int fw_range_cmp(u32 offset, const struct intel_forcewake_range *entry)
>   {
>   	if (offset < entry->start)
> @@ -1064,6 +1054,10 @@ gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
>   	__fwd; \
>   })
>   
> +static const struct intel_forcewake_range __gen6_fw_ranges[] = {
> +	GEN_FW_RANGE(0x0, 0x3ffff, FORCEWAKE_RENDER),
> +};
> +
>   /* *Must* be sorted by offset ranges! See intel_fw_table_check(). */
>   static const struct intel_forcewake_range __chv_fw_ranges[] = {
>   	GEN_FW_RANGE(0x2000, 0x3fff, FORCEWAKE_RENDER),
> @@ -1623,7 +1617,6 @@ __gen_read(func, 64)
>   
>   __gen_reg_read_funcs(gen11_fwtable);
>   __gen_reg_read_funcs(fwtable);
> -__gen_reg_read_funcs(gen6);
>   
>   #undef __gen_reg_read_funcs
>   #undef GEN6_READ_FOOTER
> @@ -2111,15 +2104,17 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (GRAPHICS_VER(i915) == 8) {
> +		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
> +		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (IS_VALLEYVIEW(i915)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __vlv_fw_ranges);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (IS_GRAPHICS_VER(i915, 6, 7)) {
> +		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
> +		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	}
>   
>   	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/uncore: Associate shadow table with uncore

[Tvrtko Ursulin]

On 10/09/2021 21:10, Matt Roper wrote:
> Store a reference to a platform's shadow table inside the uncore, the
> same as we do with the forcewake table.  This will allow us to use a
> single set of functions that operate on the shadow table reference
> rather than generating lots of nearly-identical functions via macros
> that differ only in terms of the table that they reference.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 40 ++++++++++++++++++++---------
>   drivers/gpu/drm/i915/intel_uncore.h |  7 +++++
>   2 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8c09af1e9f7a..5fa2bf26a948 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1026,17 +1026,19 @@ static int mmio_range_cmp(u32 key, const struct i915_range *range)
>   		return 0;
>   }
>   
> -#define __is_X_shadowed(x) \
> -static bool is_##x##_shadowed(u32 offset) \
> -{ \
> -	const struct i915_range *regs = x##_shadowed_regs; \
> -	return BSEARCH(offset, regs, ARRAY_SIZE(x##_shadowed_regs), \
> +static bool
> +is_shadowed(struct intel_uncore *uncore, u32 offset)

Fits in one line if you want.

> +{
> +	if (drm_WARN_ON(&uncore->i915->drm, !uncore->shadowed_reg_table))
> +		return false;
> +
> +	return BSEARCH(offset,
> +		       uncore->shadowed_reg_table,
> +		       uncore->shadowed_reg_table_entries,
>   		       mmio_range_cmp); \
>   }
>   
> -__is_X_shadowed(gen8)
> -__is_X_shadowed(gen11)
> -__is_X_shadowed(gen12)
> +

If you want to tidy.

>   
>   static enum forcewake_domains
>   gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
> @@ -1047,7 +1049,7 @@ gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
>   #define __gen8_reg_write_fw_domains(uncore, offset) \
>   ({ \
>   	enum forcewake_domains __fwd; \
> -	if (NEEDS_FORCE_WAKE(offset) && !is_gen8_shadowed(offset)) \
> +	if (NEEDS_FORCE_WAKE(offset) && !is_shadowed(uncore, offset)) \
>   		__fwd = FORCEWAKE_RENDER; \
>   	else \
>   		__fwd = 0; \
> @@ -1081,7 +1083,7 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
>   #define __fwtable_reg_write_fw_domains(uncore, offset) \
>   ({ \
>   	enum forcewake_domains __fwd = 0; \
> -	if (NEEDS_FORCE_WAKE((offset)) && !is_gen8_shadowed(offset)) \
> +	if (NEEDS_FORCE_WAKE((offset)) && !is_shadowed(uncore, offset)) \
>   		__fwd = find_fw_domain(uncore, offset); \
>   	__fwd; \
>   })
> @@ -1090,7 +1092,7 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
>   ({ \
>   	enum forcewake_domains __fwd = 0; \
>   	const u32 __offset = (offset); \
> -	if (!is_gen11_shadowed(__offset)) \
> +	if (!is_shadowed(uncore, __offset)) \
>   		__fwd = find_fw_domain(uncore, __offset); \
>   	__fwd; \
>   })
> @@ -1099,7 +1101,7 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
>   ({ \
>   	enum forcewake_domains __fwd = 0; \
>   	const u32 __offset = (offset); \
> -	if (!is_gen12_shadowed(__offset)) \
> +	if (!is_shadowed(uncore, __offset)) \
>   		__fwd = find_fw_domain(uncore, __offset); \
>   	__fwd; \
>   })
> @@ -1705,6 +1707,7 @@ __gen_write(func, 8) \
>   __gen_write(func, 16) \
>   __gen_write(func, 32)
>   
> +

Ditto.

>   __gen_reg_write_funcs(gen12_fwtable);
>   __gen_reg_write_funcs(gen11_fwtable);
>   __gen_reg_write_funcs(fwtable);
> @@ -1969,6 +1972,12 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>   	(uncore)->fw_domains_table_entries = ARRAY_SIZE((d)); \
>   }
>   
> +#define ASSIGN_SHADOW_TABLE(uncore, d) \
> +{ \
> +	(uncore)->shadowed_reg_table = d; \
> +	(uncore)->shadowed_reg_table_entries = ARRAY_SIZE((d)); \
> +}
> +
>   static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
>   					 unsigned long action, void *data)
>   {
> @@ -2081,30 +2090,37 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>   
>   	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 55)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (GRAPHICS_VER(i915) >= 12) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (GRAPHICS_VER(i915) == 11) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (IS_CHERRYVIEW(i915)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (GRAPHICS_VER(i915) == 8) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
> +		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);

Note for later - I think by the end of the series all "assign read" in 
this block become fwtable so you could consolidate them if wanted.

>   	} else if (IS_VALLEYVIEW(i915)) {
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 531665b08039..2f31c50eeae2 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -142,6 +142,13 @@ struct intel_uncore {
>   	const struct intel_forcewake_range *fw_domains_table;
>   	unsigned int fw_domains_table_entries;
>   
> +	/*
> +	 * Shadowed registers are special cases where we can safely write
> +	 * to the register *without* grabbing forcewake.
> +	 */
> +	const struct i915_range *shadowed_reg_table;
> +	unsigned int shadowed_reg_table_entries;
> +
>   	struct notifier_block pmic_bus_access_nb;
>   	struct intel_uncore_funcs funcs;
>   
> 

Looks fine and all tidies are optional.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/uncore: Replace gen8 write functions with general fwtable

[Tvrtko Ursulin]

On 10/09/2021 21:10, Matt Roper wrote:
> Now that we have both a standard forcewake table (albeit a single-entry
> table) and the shadow table stored in the uncore, we can drop the
> gen8-specific write handlers in favor of the general fwtable version.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 13 +------------
>   1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 5fa2bf26a948..4c6898746d10 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1046,16 +1046,6 @@ gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
>   	return FORCEWAKE_RENDER;
>   }
>   
> -#define __gen8_reg_write_fw_domains(uncore, offset) \
> -({ \
> -	enum forcewake_domains __fwd; \
> -	if (NEEDS_FORCE_WAKE(offset) && !is_shadowed(uncore, offset)) \
> -		__fwd = FORCEWAKE_RENDER; \
> -	else \
> -		__fwd = 0; \
> -	__fwd; \
> -})
> -
>   static const struct intel_forcewake_range __gen6_fw_ranges[] = {
>   	GEN_FW_RANGE(0x0, 0x3ffff, FORCEWAKE_RENDER),
>   };
> @@ -1711,7 +1701,6 @@ __gen_write(func, 32)
>   __gen_reg_write_funcs(gen12_fwtable);
>   __gen_reg_write_funcs(gen11_fwtable);
>   __gen_reg_write_funcs(fwtable);
> -__gen_reg_write_funcs(gen8);
>   
>   #undef __gen_reg_write_funcs
>   #undef GEN6_WRITE_FOOTER
> @@ -2121,7 +2110,7 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>   	} else if (GRAPHICS_VER(i915) == 8) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen6_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
> +		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (IS_VALLEYVIEW(i915)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __vlv_fw_ranges);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [PATCH v2 4/6] drm/i915/uncore: Drop gen11/gen12 mmio write handlers

[Tvrtko Ursulin]

On 10/09/2021 21:10, Matt Roper wrote:
> Now that the reference to the shadow table is stored within the uncore,
> we don't need to generate separate fwtable, gen11_fwtable, and
> gen12_fwtable variants of the register write functions; a single
> 'fwtable' implementation will work for all of those platforms now.
> 
> While consolidating the functions, gen11/gen12 pick up a
> NEEDS_FORCE_WAKE() check that they didn't have before, allowing them to
> bypass a lot of forcewake/shadow checking for non-GT registers (e.g.,
> display).  However since these later platforms also introduce media
> engines at higher MMIO offsets, the definition of NEEDS_FORCE_WAKE() is
> extended to also consider register offsets above GEN11_BSD_RING_BASE.
> 
> v2:
>   - Restore NEEDS_FORCE_WAKE(), but extend it for compatibility with the
>     gen11+ platforms by also passing offsets above GEN11_BSD_RING_BASE.
>     (Chris, Tvrtko)
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 61 ++++++++++-------------------
>   1 file changed, 21 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 4c6898746d10..bfb2a6337f9d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -851,7 +851,10 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
>   }
>   
>   /* We give fast paths for the really cool registers */
> -#define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
> +#define NEEDS_FORCE_WAKE(reg) ({ \
> +	u32 __reg = (reg); \
> +	__reg < 0x40000 || __reg >= GEN11_BSD_RING_BASE; \
> +})
>   
>   static int fw_range_cmp(u32 offset, const struct intel_forcewake_range *entry)
>   {
> @@ -1071,27 +1074,10 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
>   };
>   
>   #define __fwtable_reg_write_fw_domains(uncore, offset) \
> -({ \
> -	enum forcewake_domains __fwd = 0; \
> -	if (NEEDS_FORCE_WAKE((offset)) && !is_shadowed(uncore, offset)) \
> -		__fwd = find_fw_domain(uncore, offset); \
> -	__fwd; \
> -})
> -
> -#define __gen11_fwtable_reg_write_fw_domains(uncore, offset) \
>   ({ \
>   	enum forcewake_domains __fwd = 0; \
>   	const u32 __offset = (offset); \
> -	if (!is_shadowed(uncore, __offset)) \
> -		__fwd = find_fw_domain(uncore, __offset); \
> -	__fwd; \
> -})
> -
> -#define __gen12_fwtable_reg_write_fw_domains(uncore, offset) \
> -({ \
> -	enum forcewake_domains __fwd = 0; \
> -	const u32 __offset = (offset); \
> -	if (!is_shadowed(uncore, __offset)) \
> +	if (NEEDS_FORCE_WAKE((__offset)) && !is_shadowed(uncore, __offset)) \
>   		__fwd = find_fw_domain(uncore, __offset); \
>   	__fwd; \
>   })
> @@ -1675,34 +1661,29 @@ __gen6_write(8)
>   __gen6_write(16)
>   __gen6_write(32)
>   
> -#define __gen_write(func, x) \
> +#define __gen_fwtable_write(x) \
>   static void \
> -func##_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { \
> +fwtable_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { \
>   	enum forcewake_domains fw_engine; \
>   	GEN6_WRITE_HEADER; \
> -	fw_engine = __##func##_reg_write_fw_domains(uncore, offset); \
> +	fw_engine = __fwtable_reg_write_fw_domains(uncore, offset); \
>   	if (fw_engine) \
>   		__force_wake_auto(uncore, fw_engine); \
>   	__raw_uncore_write##x(uncore, reg, val); \
>   	GEN6_WRITE_FOOTER; \
>   }
>   
> -#define __gen_reg_write_funcs(func) \
> -static enum forcewake_domains \
> -func##_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
> -	return __##func##_reg_write_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
> -} \
> -\
> -__gen_write(func, 8) \
> -__gen_write(func, 16) \
> -__gen_write(func, 32)
> -
> +static enum forcewake_domains
> +fwtable_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
> +{
> +	return __fwtable_reg_write_fw_domains(uncore, i915_mmio_reg_offset(reg));
> +}
>   
> -__gen_reg_write_funcs(gen12_fwtable);
> -__gen_reg_write_funcs(gen11_fwtable);
> -__gen_reg_write_funcs(fwtable);
> +__gen_fwtable_write(8)
> +__gen_fwtable_write(16)
> +__gen_fwtable_write(32)
>   
> -#undef __gen_reg_write_funcs
> +#undef __gen_fwtable_write
>   #undef GEN6_WRITE_FOOTER
>   #undef GEN6_WRITE_HEADER
>   
> @@ -2080,22 +2061,22 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>   	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 55)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
> +		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
> +		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (GRAPHICS_VER(i915) >= 12) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
> +		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (GRAPHICS_VER(i915) == 11) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [PATCH v2 5/6] drm/i915/uncore: Drop gen11 mmio read handlers

[Tvrtko Ursulin]

On 10/09/2021 21:10, Matt Roper wrote:
> Consolidate down to just a single 'fwtable' implementation.  For reads
> we don't need to worry about shadow tables.  Also, the
> NEEDS_FORCE_WAKE() check we previously had in the fwtable implementation
> can be dropped --- if a register is outside that range on one of the old
> platforms, then it won't belong to any forcewake range and 0 will be
> returned anyway.
> 
> v2:
>   - Restore NEEDS_FORCE_WAKE() check.  (Chris, Tvrtko)

I started liking rewording the commit message when the revision log 
starts contradicting it, but it is just a suggestion.

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 40 ++++++++++++-----------------
>   1 file changed, 17 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index bfb2a6337f9d..10f124297e7c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -935,9 +935,6 @@ static const struct intel_forcewake_range __vlv_fw_ranges[] = {
>   	__fwd; \
>   })
>   
> -#define __gen11_fwtable_reg_read_fw_domains(uncore, offset) \
> -	find_fw_domain(uncore, offset)
> -
>   /* *Must* be sorted by offset! See intel_shadow_table_check(). */
>   static const struct i915_range gen8_shadowed_regs[] = {
>   	{ .start =  0x2030, .end =  0x2030 },
> @@ -1570,33 +1567,30 @@ static inline void __force_wake_auto(struct intel_uncore *uncore,
>   		___force_wake_auto(uncore, fw_domains);
>   }
>   
> -#define __gen_read(func, x) \
> +#define __gen_fwtable_read(x) \
>   static u##x \
> -func##_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { \
> +fwtable_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) \
> +{ \
>   	enum forcewake_domains fw_engine; \
>   	GEN6_READ_HEADER(x); \
> -	fw_engine = __##func##_reg_read_fw_domains(uncore, offset); \
> +	fw_engine = __fwtable_reg_read_fw_domains(uncore, offset); \
>   	if (fw_engine) \
>   		__force_wake_auto(uncore, fw_engine); \
>   	val = __raw_uncore_read##x(uncore, reg); \
>   	GEN6_READ_FOOTER; \
>   }
>   
> -#define __gen_reg_read_funcs(func) \
> -static enum forcewake_domains \
> -func##_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
> -	return __##func##_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
> -} \
> -\
> -__gen_read(func, 8) \
> -__gen_read(func, 16) \
> -__gen_read(func, 32) \
> -__gen_read(func, 64)
> +static enum forcewake_domains
> +fwtable_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) {
> +	return __fwtable_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg));
> +}
>   
> -__gen_reg_read_funcs(gen11_fwtable);
> -__gen_reg_read_funcs(fwtable);
> +__gen_fwtable_read(8)
> +__gen_fwtable_read(16)
> +__gen_fwtable_read(32)
> +__gen_fwtable_read(64)
>   
> -#undef __gen_reg_read_funcs
> +#undef __gen_fwtable_read
>   #undef GEN6_READ_FOOTER
>   #undef GEN6_READ_HEADER
>   
> @@ -2062,22 +2056,22 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (GRAPHICS_VER(i915) >= 12) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (GRAPHICS_VER(i915) == 11) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>   	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
>   		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
>   		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [PATCH v2 5/6] drm/i915/uncore: Drop gen11 mmio read handlers

[Lucas De Marchi]

On Fri, Sep 10, 2021 at 01:10:29PM -0700, Matt Roper wrote:
>Consolidate down to just a single 'fwtable' implementation.  For reads
>we don't need to worry about shadow tables.  Also, the
>NEEDS_FORCE_WAKE() check we previously had in the fwtable implementation
>can be dropped --- if a register is outside that range on one of the old
>platforms, then it won't belong to any forcewake range and 0 will be
>returned anyway.
>
>v2:
> - Restore NEEDS_FORCE_WAKE() check.  (Chris, Tvrtko)
>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/i915/intel_uncore.c | 40 ++++++++++++-----------------
> 1 file changed, 17 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>index bfb2a6337f9d..10f124297e7c 100644
>--- a/drivers/gpu/drm/i915/intel_uncore.c
>+++ b/drivers/gpu/drm/i915/intel_uncore.c
>@@ -935,9 +935,6 @@ static const struct intel_forcewake_range __vlv_fw_ranges[] = {
> 	__fwd; \
> })
>
>-#define __gen11_fwtable_reg_read_fw_domains(uncore, offset) \
>-	find_fw_domain(uncore, offset)
>-
> /* *Must* be sorted by offset! See intel_shadow_table_check(). */
> static const struct i915_range gen8_shadowed_regs[] = {
> 	{ .start =  0x2030, .end =  0x2030 },
>@@ -1570,33 +1567,30 @@ static inline void __force_wake_auto(struct intel_uncore *uncore,
> 		___force_wake_auto(uncore, fw_domains);
> }
>
>-#define __gen_read(func, x) \
>+#define __gen_fwtable_read(x) \
> static u##x \
>-func##_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { \
>+fwtable_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) \
>+{ \
> 	enum forcewake_domains fw_engine; \
> 	GEN6_READ_HEADER(x); \
>-	fw_engine = __##func##_reg_read_fw_domains(uncore, offset); \
>+	fw_engine = __fwtable_reg_read_fw_domains(uncore, offset); \
> 	if (fw_engine) \
> 		__force_wake_auto(uncore, fw_engine); \
> 	val = __raw_uncore_read##x(uncore, reg); \
> 	GEN6_READ_FOOTER; \
> }
>
>-#define __gen_reg_read_funcs(func) \
>-static enum forcewake_domains \
>-func##_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
>-	return __##func##_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
>-} \
>-\
>-__gen_read(func, 8) \
>-__gen_read(func, 16) \
>-__gen_read(func, 32) \
>-__gen_read(func, 64)
>+static enum forcewake_domains
>+fwtable_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) {
>+	return __fwtable_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg));
>+}
>
>-__gen_reg_read_funcs(gen11_fwtable);
>-__gen_reg_read_funcs(fwtable);
>+__gen_fwtable_read(8)
>+__gen_fwtable_read(16)
>+__gen_fwtable_read(32)
>+__gen_fwtable_read(64)
>
>-#undef __gen_reg_read_funcs
>+#undef __gen_fwtable_read
> #undef GEN6_READ_FOOTER
> #undef GEN6_READ_HEADER
>
>@@ -2062,22 +2056,22 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
> 		ASSIGN_FW_DOMAINS_TABLE(uncore, __dg2_fw_ranges);
> 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
> 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
> 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> 		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
> 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
> 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
> 	} else if (GRAPHICS_VER(i915) >= 12) {
> 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
> 		ASSIGN_SHADOW_TABLE(uncore, gen12_shadowed_regs);
> 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
> 	} else if (GRAPHICS_VER(i915) == 11) {
> 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
> 		ASSIGN_SHADOW_TABLE(uncore, gen11_shadowed_regs);
> 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>+		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);

these together with patch 1 make the fwtable variant be the only one
used for reads.... so, should we pull these assignments out of if/else
chain? gen < 6 don't have forcewake so afaics we cover all the
platforms.

this can be done as a separate patch though.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

> 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
> 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
> 		ASSIGN_SHADOW_TABLE(uncore, gen8_shadowed_regs);
>-- 
>2.25.4
>

Re: [PATCH v2 6/6] drm/i915/dg2: Add DG2-specific shadow register table

[Lucas De Marchi]

On Fri, Sep 10, 2021 at 01:10:30PM -0700, Matt Roper wrote:
>We thought the DG2 table of shadowed registers would be the same as the
>gen12/xehp table, but it turns out that there are a few minor
>differences that require us to define a new DG2-specific table:
> * One register is removed (0xC4D4)
> * One register is added (0xC4E0)
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

I did the conversion from the table to this array and arrived at the
same result.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi