[Intel-gfx] i915/uncore: constify the uncore vtables. (v2)

Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed

* [Intel-gfx] i915/uncore: constify the uncore vtables. (v2)
@ 2021-09-09  2:34 Dave Airlie
  2021-09-09  2:34 ` [Intel-gfx] [PATCH 1/2] drm/i915/uncore: split the fw get function into separate vfunc Dave Airlie
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Dave Airlie @ 2021-09-09  2:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula

static const vtables are more secure than writeable function pointers.

These two patches cleanup the uncore vtable to use static const tables.

These are based on drm-tip, and should apply to the gt tree cleanly.

Dave.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Intel-gfx] [PATCH 1/2] drm/i915/uncore: split the fw get function into separate vfunc
  2021-09-09  2:34 [Intel-gfx] i915/uncore: constify the uncore vtables. (v2) Dave Airlie
@ 2021-09-09  2:34 ` Dave Airlie
  2021-09-09  2:34 ` [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables Dave Airlie
  2021-09-09  2:44 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915/uncore: split the fw get function into separate vfunc Patchwork
  2 siblings, 0 replies; 6+ messages in thread
From: Dave Airlie @ 2021-09-09  2:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, Dave Airlie, Jani Nikula

From: Dave Airlie <airlied@redhat.com>

constify it while here. drop the put function since it was never
overloaded and always has done the same thing, no point in
indirecting it for show.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 70 ++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_uncore.h |  7 +--
 2 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f9767054dbdf..8652e4221404 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -36,6 +36,12 @@
 
 #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__))
 
+static void
+fw_domains_get(struct intel_uncore *uncore, enum forcewake_domains fw_domains)
+{
+	uncore->fw_get_funcs->force_wake_get(uncore, fw_domains);
+}
+
 void
 intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug)
 {
@@ -248,7 +254,7 @@ fw_domain_put(const struct intel_uncore_forcewake_domain *d)
 }
 
 static void
-fw_domains_get(struct intel_uncore *uncore, enum forcewake_domains fw_domains)
+fw_domains_get_normal(struct intel_uncore *uncore, enum forcewake_domains fw_domains)
 {
 	struct intel_uncore_forcewake_domain *d;
 	unsigned int tmp;
@@ -396,7 +402,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 
 	GEM_BUG_ON(!domain->wake_count);
 	if (--domain->wake_count == 0)
-		uncore->funcs.force_wake_put(uncore, domain->mask);
+		fw_domains_put(uncore, domain->mask);
 
 	spin_unlock_irqrestore(&uncore->lock, irqflags);
 
@@ -454,7 +460,7 @@ intel_uncore_forcewake_reset(struct intel_uncore *uncore)
 
 	fw = uncore->fw_domains_active;
 	if (fw)
-		uncore->funcs.force_wake_put(uncore, fw);
+		fw_domains_put(uncore, fw);
 
 	fw_domains_reset(uncore, uncore->fw_domains);
 	assert_forcewakes_inactive(uncore);
@@ -562,7 +568,7 @@ static void forcewake_early_sanitize(struct intel_uncore *uncore,
 	intel_uncore_forcewake_reset(uncore);
 	if (restore_forcewake) {
 		spin_lock_irq(&uncore->lock);
-		uncore->funcs.force_wake_get(uncore, restore_forcewake);
+		fw_domains_get(uncore, restore_forcewake);
 
 		if (intel_uncore_has_fifo(uncore))
 			uncore->fifo_count = fifo_free_entries(uncore);
@@ -623,7 +629,7 @@ static void __intel_uncore_forcewake_get(struct intel_uncore *uncore,
 	}
 
 	if (fw_domains)
-		uncore->funcs.force_wake_get(uncore, fw_domains);
+		fw_domains_get(uncore, fw_domains);
 }
 
 /**
@@ -644,7 +650,7 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
 {
 	unsigned long irqflags;
 
-	if (!uncore->funcs.force_wake_get)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	assert_rpm_wakelock_held(uncore->rpm);
@@ -711,7 +717,7 @@ void intel_uncore_forcewake_get__locked(struct intel_uncore *uncore,
 {
 	lockdep_assert_held(&uncore->lock);
 
-	if (!uncore->funcs.force_wake_get)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	__intel_uncore_forcewake_get(uncore, fw_domains);
@@ -733,7 +739,7 @@ static void __intel_uncore_forcewake_put(struct intel_uncore *uncore,
 			continue;
 		}
 
-		uncore->funcs.force_wake_put(uncore, domain->mask);
+		fw_domains_put(uncore, domain->mask);
 	}
 }
 
@@ -750,7 +756,7 @@ void intel_uncore_forcewake_put(struct intel_uncore *uncore,
 {
 	unsigned long irqflags;
 
-	if (!uncore->funcs.force_wake_put)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	spin_lock_irqsave(&uncore->lock, irqflags);
@@ -769,7 +775,7 @@ void intel_uncore_forcewake_flush(struct intel_uncore *uncore,
 	struct intel_uncore_forcewake_domain *domain;
 	unsigned int tmp;
 
-	if (!uncore->funcs.force_wake_put)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	fw_domains &= uncore->fw_domains;
@@ -793,7 +799,7 @@ void intel_uncore_forcewake_put__locked(struct intel_uncore *uncore,
 {
 	lockdep_assert_held(&uncore->lock);
 
-	if (!uncore->funcs.force_wake_put)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	__intel_uncore_forcewake_put(uncore, fw_domains);
@@ -801,7 +807,7 @@ void intel_uncore_forcewake_put__locked(struct intel_uncore *uncore,
 
 void assert_forcewakes_inactive(struct intel_uncore *uncore)
 {
-	if (!uncore->funcs.force_wake_get)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	drm_WARN(&uncore->i915->drm, uncore->fw_domains_active,
@@ -818,7 +824,7 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
 	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM))
 		return;
 
-	if (!uncore->funcs.force_wake_get)
+	if (!uncore->fw_get_funcs)
 		return;
 
 	spin_lock_irq(&uncore->lock);
@@ -1582,7 +1588,7 @@ static noinline void ___force_wake_auto(struct intel_uncore *uncore,
 	for_each_fw_domain_masked(domain, fw_domains, uncore, tmp)
 		fw_domain_arm_timer(domain);
 
-	uncore->funcs.force_wake_get(uncore, fw_domains);
+	fw_domains_get(uncore, fw_domains);
 }
 
 static inline void __force_wake_auto(struct intel_uncore *uncore,
@@ -1841,6 +1847,18 @@ static void intel_uncore_fw_domains_fini(struct intel_uncore *uncore)
 		fw_domain_fini(uncore, d->id);
 }
 
+static const struct intel_uncore_fw_get uncore_get_fallback = {
+	.force_wake_get = fw_domains_get_with_fallback
+};
+
+static const struct intel_uncore_fw_get uncore_get_normal = {
+	.force_wake_get = fw_domains_get_normal,
+};
+
+static const struct intel_uncore_fw_get uncore_get_thread_status = {
+	.force_wake_get = fw_domains_get_with_thread_status
+};
+
 static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
@@ -1856,8 +1874,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 		intel_engine_mask_t emask = INTEL_INFO(i915)->platform_engine_mask;
 		int i;
 
-		uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
-		uncore->funcs.force_wake_put = fw_domains_put;
+		uncore->fw_get_funcs = &uncore_get_fallback;
 		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE_RENDER_GEN9,
 			       FORCEWAKE_ACK_RENDER_GEN9);
@@ -1882,8 +1899,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 				       FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
 		}
 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
-		uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
-		uncore->funcs.force_wake_put = fw_domains_put;
+		uncore->fw_get_funcs = &uncore_get_fallback;
 		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE_RENDER_GEN9,
 			       FORCEWAKE_ACK_RENDER_GEN9);
@@ -1893,16 +1909,13 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 		fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
 			       FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
 	} else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
-		uncore->funcs.force_wake_get = fw_domains_get;
-		uncore->funcs.force_wake_put = fw_domains_put;
+		uncore->fw_get_funcs = &uncore_get_normal;
 		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
 		fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
 			       FORCEWAKE_MEDIA_VLV, FORCEWAKE_ACK_MEDIA_VLV);
 	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
-		uncore->funcs.force_wake_get =
-			fw_domains_get_with_thread_status;
-		uncore->funcs.force_wake_put = fw_domains_put;
+		uncore->fw_get_funcs = &uncore_get_thread_status;
 		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
 	} else if (IS_IVYBRIDGE(i915)) {
@@ -1917,9 +1930,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 		 * (correctly) interpreted by the test below as MT
 		 * forcewake being disabled.
 		 */
-		uncore->funcs.force_wake_get =
-			fw_domains_get_with_thread_status;
-		uncore->funcs.force_wake_put = fw_domains_put;
+		uncore->fw_get_funcs = &uncore_get_thread_status;
 
 		/* We need to init first for ECOBUS access and then
 		 * determine later if we want to reinit, in case of MT access is
@@ -1950,9 +1961,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 				       FORCEWAKE, FORCEWAKE_ACK);
 		}
 	} else if (GRAPHICS_VER(i915) == 6) {
-		uncore->funcs.force_wake_get =
-			fw_domains_get_with_thread_status;
-		uncore->funcs.force_wake_put = fw_domains_put;
+		uncore->fw_get_funcs = &uncore_get_thread_status;
 		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
 			       FORCEWAKE, FORCEWAKE_ACK);
 	}
@@ -2161,8 +2170,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 	}
 
 	/* make sure fw funcs are set if and only if we have fw*/
-	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_get);
-	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_put);
+	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->fw_get_funcs);
 	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.read_fw_domains);
 	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.write_fw_domains);
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 531665b08039..1950380c0d79 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -84,12 +84,12 @@ enum forcewake_domains {
 	FORCEWAKE_ALL = BIT(FW_DOMAIN_ID_COUNT) - 1,
 };
 
-struct intel_uncore_funcs {
+struct intel_uncore_fw_get {
 	void (*force_wake_get)(struct intel_uncore *uncore,
 			       enum forcewake_domains domains);
-	void (*force_wake_put)(struct intel_uncore *uncore,
-			       enum forcewake_domains domains);
+};
 
+struct intel_uncore_funcs {
 	enum forcewake_domains (*read_fw_domains)(struct intel_uncore *uncore,
 						  i915_reg_t r);
 	enum forcewake_domains (*write_fw_domains)(struct intel_uncore *uncore,
@@ -143,6 +143,7 @@ struct intel_uncore {
 	unsigned int fw_domains_table_entries;
 
 	struct notifier_block pmic_bus_access_nb;
+	const struct intel_uncore_fw_get *fw_get_funcs;
 	struct intel_uncore_funcs funcs;
 
 	unsigned int fifo_count;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables.
  2021-09-09  2:34 [Intel-gfx] i915/uncore: constify the uncore vtables. (v2) Dave Airlie
  2021-09-09  2:34 ` [Intel-gfx] [PATCH 1/2] drm/i915/uncore: split the fw get function into separate vfunc Dave Airlie
@ 2021-09-09  2:34 ` Dave Airlie
  2021-09-09  2:44 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915/uncore: split the fw get function into separate vfunc Patchwork
  2 siblings, 0 replies; 6+ messages in thread
From: Dave Airlie @ 2021-09-09  2:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, Dave Airlie, Jani Nikula

From: Dave Airlie <airlied@redhat.com>

This reworks the uncore function vtable so that it's constant.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 133 +++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_uncore.h |   8 +-
 2 files changed, 83 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8652e4221404..e0e7f133f2b9 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1737,32 +1737,24 @@ __vgpu_write(8)
 __vgpu_write(16)
 __vgpu_write(32)
 
-#define ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, x) \
-do { \
-	(uncore)->funcs.mmio_writeb = x##_write8; \
-	(uncore)->funcs.mmio_writew = x##_write16; \
-	(uncore)->funcs.mmio_writel = x##_write32; \
-} while (0)
-
-#define ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x) \
-do { \
-	(uncore)->funcs.mmio_readb = x##_read8; \
-	(uncore)->funcs.mmio_readw = x##_read16; \
-	(uncore)->funcs.mmio_readl = x##_read32; \
-	(uncore)->funcs.mmio_readq = x##_read64; \
-} while (0)
-
-#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
-do { \
-	ASSIGN_RAW_WRITE_MMIO_VFUNCS((uncore), x); \
-	(uncore)->funcs.write_fw_domains = x##_reg_write_fw_domains; \
-} while (0)
-
-#define ASSIGN_READ_MMIO_VFUNCS(uncore, x) \
-do { \
-	ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x); \
-	(uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
-} while (0)
+#define MMIO_RAW_WRITE_VFUNCS(x)     \
+	.mmio_writeb = x##_write8,   \
+	.mmio_writew = x##_write16,  \
+	.mmio_writel = x##_write32
+
+#define MMIO_RAW_READ_VFUNCS(x)	  \
+	.mmio_readb = x##_read8,  \
+	.mmio_readw = x##_read16, \
+	.mmio_readl = x##_read32, \
+	.mmio_readq = x##_read64
+
+#define MMIO_WRITE_FW_VFUNCS(x)				\
+	MMIO_RAW_WRITE_VFUNCS(x),			\
+	.write_fw_domains = x##_reg_write_fw_domains
+
+#define MMIO_READ_FW_VFUNCS(x)				\
+	MMIO_RAW_READ_VFUNCS(x),			\
+	.read_fw_domains = x##_reg_read_fw_domains
 
 static int __fw_domain_init(struct intel_uncore *uncore,
 			    enum forcewake_domain_id domain_id,
@@ -2067,22 +2059,64 @@ void intel_uncore_init_early(struct intel_uncore *uncore,
 	uncore->debug = &i915->mmio_debug;
 }
 
+static const struct intel_uncore_funcs vgpu_funcs = {
+	MMIO_RAW_WRITE_VFUNCS(vgpu),
+	MMIO_RAW_READ_VFUNCS(vgpu),
+};
+
+static const struct intel_uncore_funcs gen5_funcs = {
+	MMIO_RAW_WRITE_VFUNCS(gen5),
+	MMIO_RAW_READ_VFUNCS(gen5),
+};
+
+static const struct intel_uncore_funcs gen2_funcs = {
+	MMIO_RAW_WRITE_VFUNCS(gen2),
+	MMIO_RAW_READ_VFUNCS(gen2),
+};
+
 static void uncore_raw_init(struct intel_uncore *uncore)
 {
 	GEM_BUG_ON(intel_uncore_has_forcewake(uncore));
 
 	if (intel_vgpu_active(uncore->i915)) {
-		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, vgpu);
-		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, vgpu);
+		uncore->funcs = &vgpu_funcs;
 	} else if (GRAPHICS_VER(uncore->i915) == 5) {
-		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
-		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
+		uncore->funcs = &gen5_funcs;
 	} else {
-		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
-		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
+		uncore->funcs = &gen2_funcs;
 	}
 }
 
+static const struct intel_uncore_funcs gen12_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen12_fwtable),
+	MMIO_READ_FW_VFUNCS(gen11_fwtable)
+};
+
+static const struct intel_uncore_funcs gen11_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen11_fwtable),
+	MMIO_READ_FW_VFUNCS(gen11_fwtable)
+};
+
+static const struct intel_uncore_funcs fwtable_funcs = {
+	MMIO_WRITE_FW_VFUNCS(fwtable),
+	MMIO_READ_FW_VFUNCS(fwtable)
+};
+
+static const struct intel_uncore_funcs gen8_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen8),
+	MMIO_READ_FW_VFUNCS(gen6)
+};
+
+static const struct intel_uncore_funcs vlv_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen6),
+	MMIO_READ_FW_VFUNCS(fwtable)
+};
+
+static const struct intel_uncore_funcs gen6_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen6),
+	MMIO_READ_FW_VFUNCS(gen6)
+};
+
 static int uncore_forcewake_init(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
@@ -2097,38 +2131,29 @@ 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_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &gen12_funcs;
 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &gen12_funcs;
 	} else if (GRAPHICS_VER(i915) >= 12) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &gen12_funcs;
 	} else if (GRAPHICS_VER(i915) == 11) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &gen11_funcs;
 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
+		uncore->funcs = &fwtable_funcs;
 	} else if (IS_CHERRYVIEW(i915)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
+		uncore->funcs = &fwtable_funcs;
 	} else if (GRAPHICS_VER(i915) == 8) {
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
+		uncore->funcs = &gen8_funcs;
 	} 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);
+		uncore->funcs = &vlv_funcs;
 	} else if (IS_GRAPHICS_VER(i915, 6, 7)) {
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
+		uncore->funcs = &gen6_funcs;
 	}
 
 	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
@@ -2171,8 +2196,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 
 	/* make sure fw funcs are set if and only if we have fw*/
 	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->fw_get_funcs);
-	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.read_fw_domains);
-	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.write_fw_domains);
+	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs->read_fw_domains);
+	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs->write_fw_domains);
 
 	if (HAS_FPGA_DBG_UNCLAIMED(i915))
 		uncore->flags |= UNCORE_HAS_FPGA_DBG_UNCLAIMED;
@@ -2511,10 +2536,10 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
 		return 0;
 
 	if (op & FW_REG_READ)
-		fw_domains = uncore->funcs.read_fw_domains(uncore, reg);
+		fw_domains = uncore->funcs->read_fw_domains(uncore, reg);
 
 	if (op & FW_REG_WRITE)
-		fw_domains |= uncore->funcs.write_fw_domains(uncore, reg);
+		fw_domains |= uncore->funcs->write_fw_domains(uncore, reg);
 
 	drm_WARN_ON(&uncore->i915->drm, fw_domains & ~uncore->fw_domains);
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 1950380c0d79..92b4279119d2 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -144,7 +144,7 @@ struct intel_uncore {
 
 	struct notifier_block pmic_bus_access_nb;
 	const struct intel_uncore_fw_get *fw_get_funcs;
-	struct intel_uncore_funcs funcs;
+	const struct intel_uncore_funcs *funcs;
 
 	unsigned int fifo_count;
 
@@ -318,14 +318,14 @@ __raw_write(64, q)
 static inline u##x__ intel_uncore_##name__(struct intel_uncore *uncore, \
 					   i915_reg_t reg) \
 { \
-	return uncore->funcs.mmio_read##s__(uncore, reg, (trace__)); \
+	return uncore->funcs->mmio_read##s__(uncore, reg, (trace__)); \
 }
 
 #define __uncore_write(name__, x__, s__, trace__) \
 static inline void intel_uncore_##name__(struct intel_uncore *uncore, \
 					 i915_reg_t reg, u##x__ val) \
 { \
-	uncore->funcs.mmio_write##s__(uncore, reg, val, (trace__)); \
+	uncore->funcs->mmio_write##s__(uncore, reg, val, (trace__)); \
 }
 
 __uncore_read(read8, 8, b, true)
@@ -344,7 +344,7 @@ __uncore_write(write_notrace, 32, l, false)
  * an arbitrary delay between them. This can cause the hardware to
  * act upon the intermediate value, possibly leading to corruption and
  * machine death. For this reason we do not support intel_uncore_write64,
- * or uncore->funcs.mmio_writeq.
+ * or uncore->funcs->mmio_writeq.
  *
  * When reading a 64-bit value as two 32-bit values, the delay may cause
  * the two reads to mismatch, e.g. a timestamp overflowing. Also note that
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915/uncore: split the fw get function into separate vfunc
  2021-09-09  2:34 [Intel-gfx] i915/uncore: constify the uncore vtables. (v2) Dave Airlie
  2021-09-09  2:34 ` [Intel-gfx] [PATCH 1/2] drm/i915/uncore: split the fw get function into separate vfunc Dave Airlie
  2021-09-09  2:34 ` [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables Dave Airlie
@ 2021-09-09  2:44 ` Patchwork
  2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2021-09-09  2:44 UTC (permalink / raw)
  To: Dave Airlie; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/uncore: split the fw get function into separate vfunc
URL   : https://patchwork.freedesktop.org/series/94495/
State : failure

== Summary ==

CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND objtool
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/intel_uncore.o
In file included from drivers/gpu/drm/i915/intel_uncore.c:2605:
drivers/gpu/drm/i915/selftests/mock_uncore.c: In function ‘mock_uncore_init’:
drivers/gpu/drm/i915/selftests/mock_uncore.c:47:2: error: implicit declaration of function ‘ASSIGN_RAW_WRITE_MMIO_VFUNCS’; did you mean ‘MMIO_RAW_WRITE_VFUNCS’? [-Werror=implicit-function-declaration]
  ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, nop);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  MMIO_RAW_WRITE_VFUNCS
drivers/gpu/drm/i915/selftests/mock_uncore.c:47:39: error: ‘nop’ undeclared (first use in this function); did you mean ‘node’?
  ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, nop);
                                       ^~~
                                       node
drivers/gpu/drm/i915/selftests/mock_uncore.c:47:39: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/selftests/mock_uncore.c:48:2: error: implicit declaration of function ‘ASSIGN_RAW_READ_MMIO_VFUNCS’; did you mean ‘MMIO_RAW_READ_VFUNCS’? [-Werror=implicit-function-declaration]
  ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, nop);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  MMIO_RAW_READ_VFUNCS
At top level:
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: error: ‘nop_read64’ defined but not used [-Werror=unused-function]
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: note: in definition of macro ‘__nop_read’
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: error: ‘nop_read32’ defined but not used [-Werror=unused-function]
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: note: in definition of macro ‘__nop_read’
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: error: ‘nop_read16’ defined but not used [-Werror=unused-function]
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: note: in definition of macro ‘__nop_read’
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: error: ‘nop_read8’ defined but not used [-Werror=unused-function]
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:36:1: note: in definition of macro ‘__nop_read’
 nop_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { return 0; }
 ^~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:29:1: error: ‘nop_write32’ defined but not used [-Werror=unused-function]
 nop_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { }
 ^~~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:29:1: note: in definition of macro ‘__nop_write’
 nop_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { }
 ^~~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:29:1: error: ‘nop_write16’ defined but not used [-Werror=unused-function]
 nop_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { }
 ^~~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:29:1: note: in definition of macro ‘__nop_write’
 nop_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { }
 ^~~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:29:1: error: ‘nop_write8’ defined but not used [-Werror=unused-function]
 nop_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { }
 ^~~~~~~~~
drivers/gpu/drm/i915/selftests/mock_uncore.c:29:1: note: in definition of macro ‘__nop_write’
 nop_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace) { }
 ^~~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:271: recipe for target 'drivers/gpu/drm/i915/intel_uncore.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_uncore.o] Error 1
scripts/Makefile.build:514: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:514: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:514: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1851: recipe for target 'drivers' failed
make: *** [drivers] Error 2



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Intel-gfx] [PATCH 0/2] i915/uncore: constify the uncore vtables.
@ 2021-09-08  4:45 Dave Airlie
  2021-09-08  4:45 ` [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables Dave Airlie
  0 siblings, 1 reply; 6+ messages in thread
From: Dave Airlie @ 2021-09-08  4:45 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula

static const vtables are more secure than writeable function pointers.

These two patches cleanup the uncore vtable to use static const tables.

Dave.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables.
  2021-09-08  4:45 [Intel-gfx] [PATCH 0/2] i915/uncore: constify the uncore vtables Dave Airlie
@ 2021-09-08  4:45 ` Dave Airlie
  2021-09-08  9:13   ` Jani Nikula
  0 siblings, 1 reply; 6+ messages in thread
From: Dave Airlie @ 2021-09-08  4:45 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, Dave Airlie

From: Dave Airlie <airlied@redhat.com>

This reworks the uncore function vtable so that it's constant.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 139 +++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_uncore.h |   8 +-
 2 files changed, 89 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index d0bbfc320604..0bc6e16fc4e3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1756,32 +1756,24 @@ __vgpu_write(8)
 __vgpu_write(16)
 __vgpu_write(32)
 
-#define ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, x) \
-do { \
-	(uncore)->funcs.mmio_writeb = x##_write8; \
-	(uncore)->funcs.mmio_writew = x##_write16; \
-	(uncore)->funcs.mmio_writel = x##_write32; \
-} while (0)
-
-#define ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x) \
-do { \
-	(uncore)->funcs.mmio_readb = x##_read8; \
-	(uncore)->funcs.mmio_readw = x##_read16; \
-	(uncore)->funcs.mmio_readl = x##_read32; \
-	(uncore)->funcs.mmio_readq = x##_read64; \
-} while (0)
-
-#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
-do { \
-	ASSIGN_RAW_WRITE_MMIO_VFUNCS((uncore), x); \
-	(uncore)->funcs.write_fw_domains = x##_reg_write_fw_domains; \
-} while (0)
-
-#define ASSIGN_READ_MMIO_VFUNCS(uncore, x) \
-do { \
-	ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x); \
-	(uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
-} while (0)
+#define MMIO_RAW_WRITE_VFUNCS(x)     \
+	.mmio_writeb = x##_write8,   \
+	.mmio_writew = x##_write16,  \
+	.mmio_writel = x##_write32
+
+#define MMIO_RAW_READ_VFUNCS(x)	  \
+	.mmio_readb = x##_read8,  \
+	.mmio_readw = x##_read16, \
+	.mmio_readl = x##_read32, \
+	.mmio_readq = x##_read64
+
+#define MMIO_WRITE_FW_VFUNCS(x)				\
+	MMIO_RAW_WRITE_VFUNCS(x),			\
+	.write_fw_domains = x##_reg_write_fw_domains
+
+#define MMIO_READ_FW_VFUNCS(x)				\
+	MMIO_RAW_READ_VFUNCS(x),			\
+	.read_fw_domains = x##_reg_read_fw_domains
 
 static int __fw_domain_init(struct intel_uncore *uncore,
 			    enum forcewake_domain_id domain_id,
@@ -2086,22 +2078,70 @@ void intel_uncore_init_early(struct intel_uncore *uncore,
 	uncore->debug = &i915->mmio_debug;
 }
 
+static const struct intel_uncore_funcs vgpu_funcs = {
+	MMIO_RAW_WRITE_VFUNCS(vgpu),
+	MMIO_RAW_READ_VFUNCS(vgpu),
+};
+
+static const struct intel_uncore_funcs gen5_funcs = {
+	MMIO_RAW_WRITE_VFUNCS(gen5),
+	MMIO_RAW_READ_VFUNCS(gen5),
+};
+
+static const struct intel_uncore_funcs gen2_funcs = {
+	MMIO_RAW_WRITE_VFUNCS(gen2),
+	MMIO_RAW_READ_VFUNCS(gen2),
+};
+
+
 static void uncore_raw_init(struct intel_uncore *uncore)
 {
 	GEM_BUG_ON(intel_uncore_has_forcewake(uncore));
 
 	if (intel_vgpu_active(uncore->i915)) {
-		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, vgpu);
-		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, vgpu);
+		uncore->funcs = &vgpu_funcs;
 	} else if (GRAPHICS_VER(uncore->i915) == 5) {
-		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
-		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
+		uncore->funcs = &gen5_funcs;
 	} else {
-		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
-		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
+		uncore->funcs = &gen2_funcs;
 	}
 }
 
+static const struct intel_uncore_funcs xehp_funcs = {
+	MMIO_WRITE_FW_VFUNCS(xehp_fwtable),
+	MMIO_READ_FW_VFUNCS(gen11_fwtable)
+};
+
+static const struct intel_uncore_funcs gen12_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen12_fwtable),
+	MMIO_READ_FW_VFUNCS(gen12_fwtable)
+};
+
+static const struct intel_uncore_funcs gen11_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen11_fwtable),
+	MMIO_READ_FW_VFUNCS(gen11_fwtable)
+};
+
+static const struct intel_uncore_funcs gen9_funcs = {
+	MMIO_WRITE_FW_VFUNCS(fwtable),
+	MMIO_READ_FW_VFUNCS(fwtable)
+};
+
+static const struct intel_uncore_funcs gen8_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen8),
+	MMIO_READ_FW_VFUNCS(gen6)
+};
+
+static const struct intel_uncore_funcs vlv_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen8),
+	MMIO_READ_FW_VFUNCS(fwtable)
+};
+
+static const struct intel_uncore_funcs gen6_funcs = {
+	MMIO_WRITE_FW_VFUNCS(gen6),
+	MMIO_READ_FW_VFUNCS(gen6)
+};
+
 static int uncore_forcewake_init(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
@@ -2116,38 +2156,29 @@ 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_WRITE_MMIO_VFUNCS(uncore, xehp_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &xehp_funcs;
 	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, xehp_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &xehp_funcs;
 	} else if (GRAPHICS_VER(i915) >= 12) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen12_fwtable);
+		uncore->funcs = &gen12_funcs;
 	} else if (GRAPHICS_VER(i915) == 11) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
+		uncore->funcs = &gen11_funcs;
 	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
+		uncore->funcs = &gen9_funcs;
 	} else if (IS_CHERRYVIEW(i915)) {
 		ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
+		uncore->funcs = &gen9_funcs;
 	} else if (GRAPHICS_VER(i915) == 8) {
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
+		uncore->funcs = &gen8_funcs;
 	} 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);
+		uncore->funcs = &vlv_funcs;
 	} else if (IS_GRAPHICS_VER(i915, 6, 7)) {
-		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
-		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
+		uncore->funcs = &gen6_funcs;
 	}
 
 	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
@@ -2190,8 +2221,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 
 	/* make sure fw funcs are set if and only if we have fw*/
 	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->fw_get_funcs);
-	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.read_fw_domains);
-	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.write_fw_domains);
+	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs->read_fw_domains);
+	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs->write_fw_domains);
 
 	if (HAS_FPGA_DBG_UNCLAIMED(i915))
 		uncore->flags |= UNCORE_HAS_FPGA_DBG_UNCLAIMED;
@@ -2530,10 +2561,10 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
 		return 0;
 
 	if (op & FW_REG_READ)
-		fw_domains = uncore->funcs.read_fw_domains(uncore, reg);
+		fw_domains = uncore->funcs->read_fw_domains(uncore, reg);
 
 	if (op & FW_REG_WRITE)
-		fw_domains |= uncore->funcs.write_fw_domains(uncore, reg);
+		fw_domains |= uncore->funcs->write_fw_domains(uncore, reg);
 
 	drm_WARN_ON(&uncore->i915->drm, fw_domains & ~uncore->fw_domains);
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 1a7448f5f16f..52d4baf07656 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -138,7 +138,7 @@ struct intel_uncore {
 
 	struct notifier_block pmic_bus_access_nb;
 	const struct intel_uncore_fw_get *fw_get_funcs;
-	struct intel_uncore_funcs funcs;
+	const struct intel_uncore_funcs *funcs;
 
 	unsigned int fifo_count;
 
@@ -312,14 +312,14 @@ __raw_write(64, q)
 static inline u##x__ intel_uncore_##name__(struct intel_uncore *uncore, \
 					   i915_reg_t reg) \
 { \
-	return uncore->funcs.mmio_read##s__(uncore, reg, (trace__)); \
+	return uncore->funcs->mmio_read##s__(uncore, reg, (trace__)); \
 }
 
 #define __uncore_write(name__, x__, s__, trace__) \
 static inline void intel_uncore_##name__(struct intel_uncore *uncore, \
 					 i915_reg_t reg, u##x__ val) \
 { \
-	uncore->funcs.mmio_write##s__(uncore, reg, val, (trace__)); \
+	uncore->funcs->mmio_write##s__(uncore, reg, val, (trace__)); \
 }
 
 __uncore_read(read8, 8, b, true)
@@ -338,7 +338,7 @@ __uncore_write(write_notrace, 32, l, false)
  * an arbitrary delay between them. This can cause the hardware to
  * act upon the intermediate value, possibly leading to corruption and
  * machine death. For this reason we do not support intel_uncore_write64,
- * or uncore->funcs.mmio_writeq.
+ * or uncore->funcs->mmio_writeq.
  *
  * When reading a 64-bit value as two 32-bit values, the delay may cause
  * the two reads to mismatch, e.g. a timestamp overflowing. Also note that
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables.
  2021-09-08  4:45 ` [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables Dave Airlie
@ 2021-09-08  9:13   ` Jani Nikula
  0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2021-09-08  9:13 UTC (permalink / raw)
  To: Dave Airlie, intel-gfx; +Cc: Dave Airlie

On Wed, 08 Sep 2021, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This reworks the uncore function vtable so that it's constant.

There's a bug in there, see comment inline, with that fixed,

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


>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 139 +++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_uncore.h |   8 +-
>  2 files changed, 89 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index d0bbfc320604..0bc6e16fc4e3 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1756,32 +1756,24 @@ __vgpu_write(8)
>  __vgpu_write(16)
>  __vgpu_write(32)
>  
> -#define ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, x) \
> -do { \
> -	(uncore)->funcs.mmio_writeb = x##_write8; \
> -	(uncore)->funcs.mmio_writew = x##_write16; \
> -	(uncore)->funcs.mmio_writel = x##_write32; \
> -} while (0)
> -
> -#define ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x) \
> -do { \
> -	(uncore)->funcs.mmio_readb = x##_read8; \
> -	(uncore)->funcs.mmio_readw = x##_read16; \
> -	(uncore)->funcs.mmio_readl = x##_read32; \
> -	(uncore)->funcs.mmio_readq = x##_read64; \
> -} while (0)
> -
> -#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
> -do { \
> -	ASSIGN_RAW_WRITE_MMIO_VFUNCS((uncore), x); \
> -	(uncore)->funcs.write_fw_domains = x##_reg_write_fw_domains; \
> -} while (0)
> -
> -#define ASSIGN_READ_MMIO_VFUNCS(uncore, x) \
> -do { \
> -	ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x); \
> -	(uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
> -} while (0)
> +#define MMIO_RAW_WRITE_VFUNCS(x)     \
> +	.mmio_writeb = x##_write8,   \
> +	.mmio_writew = x##_write16,  \
> +	.mmio_writel = x##_write32
> +
> +#define MMIO_RAW_READ_VFUNCS(x)	  \
> +	.mmio_readb = x##_read8,  \
> +	.mmio_readw = x##_read16, \
> +	.mmio_readl = x##_read32, \
> +	.mmio_readq = x##_read64
> +
> +#define MMIO_WRITE_FW_VFUNCS(x)				\
> +	MMIO_RAW_WRITE_VFUNCS(x),			\
> +	.write_fw_domains = x##_reg_write_fw_domains
> +
> +#define MMIO_READ_FW_VFUNCS(x)				\
> +	MMIO_RAW_READ_VFUNCS(x),			\
> +	.read_fw_domains = x##_reg_read_fw_domains
>  
>  static int __fw_domain_init(struct intel_uncore *uncore,
>  			    enum forcewake_domain_id domain_id,
> @@ -2086,22 +2078,70 @@ void intel_uncore_init_early(struct intel_uncore *uncore,
>  	uncore->debug = &i915->mmio_debug;
>  }
>  
> +static const struct intel_uncore_funcs vgpu_funcs = {
> +	MMIO_RAW_WRITE_VFUNCS(vgpu),
> +	MMIO_RAW_READ_VFUNCS(vgpu),
> +};
> +
> +static const struct intel_uncore_funcs gen5_funcs = {
> +	MMIO_RAW_WRITE_VFUNCS(gen5),
> +	MMIO_RAW_READ_VFUNCS(gen5),
> +};
> +
> +static const struct intel_uncore_funcs gen2_funcs = {
> +	MMIO_RAW_WRITE_VFUNCS(gen2),
> +	MMIO_RAW_READ_VFUNCS(gen2),
> +};
> +
> +
>  static void uncore_raw_init(struct intel_uncore *uncore)
>  {
>  	GEM_BUG_ON(intel_uncore_has_forcewake(uncore));
>  
>  	if (intel_vgpu_active(uncore->i915)) {
> -		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, vgpu);
> -		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, vgpu);
> +		uncore->funcs = &vgpu_funcs;
>  	} else if (GRAPHICS_VER(uncore->i915) == 5) {
> -		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
> -		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
> +		uncore->funcs = &gen5_funcs;
>  	} else {
> -		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
> -		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
> +		uncore->funcs = &gen2_funcs;
>  	}
>  }
>  
> +static const struct intel_uncore_funcs xehp_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(xehp_fwtable),
> +	MMIO_READ_FW_VFUNCS(gen11_fwtable)
> +};
> +
> +static const struct intel_uncore_funcs gen12_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(gen12_fwtable),
> +	MMIO_READ_FW_VFUNCS(gen12_fwtable)
> +};
> +
> +static const struct intel_uncore_funcs gen11_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(gen11_fwtable),
> +	MMIO_READ_FW_VFUNCS(gen11_fwtable)
> +};
> +
> +static const struct intel_uncore_funcs gen9_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(fwtable),
> +	MMIO_READ_FW_VFUNCS(fwtable)
> +};
> +
> +static const struct intel_uncore_funcs gen8_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(gen8),
> +	MMIO_READ_FW_VFUNCS(gen6)
> +};
> +
> +static const struct intel_uncore_funcs vlv_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(gen8),

Should be gen6.

> +	MMIO_READ_FW_VFUNCS(fwtable)
> +};
> +
> +static const struct intel_uncore_funcs gen6_funcs = {
> +	MMIO_WRITE_FW_VFUNCS(gen6),
> +	MMIO_READ_FW_VFUNCS(gen6)
> +};
> +
>  static int uncore_forcewake_init(struct intel_uncore *uncore)
>  {
>  	struct drm_i915_private *i915 = uncore->i915;
> @@ -2116,38 +2156,29 @@ 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_WRITE_MMIO_VFUNCS(uncore, xehp_fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		uncore->funcs = &xehp_funcs;
>  	} else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>  		ASSIGN_FW_DOMAINS_TABLE(uncore, __xehp_fw_ranges);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, xehp_fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		uncore->funcs = &xehp_funcs;
>  	} else if (GRAPHICS_VER(i915) >= 12) {
>  		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen12_fw_ranges);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen12_fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen12_fwtable);
> +		uncore->funcs = &gen12_funcs;
>  	} else if (GRAPHICS_VER(i915) == 11) {
>  		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen11_fw_ranges);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen11_fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> +		uncore->funcs = &gen11_funcs;
>  	} else if (IS_GRAPHICS_VER(i915, 9, 10)) {
>  		ASSIGN_FW_DOMAINS_TABLE(uncore, __gen9_fw_ranges);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
> +		uncore->funcs = &gen9_funcs;
>  	} else if (IS_CHERRYVIEW(i915)) {
>  		ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
> +		uncore->funcs = &gen9_funcs;

Seems funny to use gen9 functions for chv.

>  	} else if (GRAPHICS_VER(i915) == 8) {
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
> +		uncore->funcs = &gen8_funcs;
>  	} 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);
> +		uncore->funcs = &vlv_funcs;
>  	} else if (IS_GRAPHICS_VER(i915, 6, 7)) {
> -		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
> -		ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
> +		uncore->funcs = &gen6_funcs;
>  	}
>  
>  	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
> @@ -2190,8 +2221,8 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>  
>  	/* make sure fw funcs are set if and only if we have fw*/
>  	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->fw_get_funcs);
> -	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.read_fw_domains);
> -	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.write_fw_domains);
> +	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs->read_fw_domains);
> +	GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs->write_fw_domains);

Gah, I hate the proliferation of GEM_BUG_ON() outside of strictly gem
code. If there's value in it beyond gem, it should not be named
GEM. Otherwise, it should stay within GEM code.

>  
>  	if (HAS_FPGA_DBG_UNCLAIMED(i915))
>  		uncore->flags |= UNCORE_HAS_FPGA_DBG_UNCLAIMED;
> @@ -2530,10 +2561,10 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
>  		return 0;
>  
>  	if (op & FW_REG_READ)
> -		fw_domains = uncore->funcs.read_fw_domains(uncore, reg);
> +		fw_domains = uncore->funcs->read_fw_domains(uncore, reg);
>  
>  	if (op & FW_REG_WRITE)
> -		fw_domains |= uncore->funcs.write_fw_domains(uncore, reg);
> +		fw_domains |= uncore->funcs->write_fw_domains(uncore, reg);
>  
>  	drm_WARN_ON(&uncore->i915->drm, fw_domains & ~uncore->fw_domains);
>  
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 1a7448f5f16f..52d4baf07656 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -138,7 +138,7 @@ struct intel_uncore {
>  
>  	struct notifier_block pmic_bus_access_nb;
>  	const struct intel_uncore_fw_get *fw_get_funcs;
> -	struct intel_uncore_funcs funcs;
> +	const struct intel_uncore_funcs *funcs;
>  
>  	unsigned int fifo_count;
>  
> @@ -312,14 +312,14 @@ __raw_write(64, q)
>  static inline u##x__ intel_uncore_##name__(struct intel_uncore *uncore, \
>  					   i915_reg_t reg) \
>  { \
> -	return uncore->funcs.mmio_read##s__(uncore, reg, (trace__)); \
> +	return uncore->funcs->mmio_read##s__(uncore, reg, (trace__)); \
>  }
>  
>  #define __uncore_write(name__, x__, s__, trace__) \
>  static inline void intel_uncore_##name__(struct intel_uncore *uncore, \
>  					 i915_reg_t reg, u##x__ val) \
>  { \
> -	uncore->funcs.mmio_write##s__(uncore, reg, val, (trace__)); \
> +	uncore->funcs->mmio_write##s__(uncore, reg, val, (trace__)); \
>  }
>  
>  __uncore_read(read8, 8, b, true)
> @@ -338,7 +338,7 @@ __uncore_write(write_notrace, 32, l, false)
>   * an arbitrary delay between them. This can cause the hardware to
>   * act upon the intermediate value, possibly leading to corruption and
>   * machine death. For this reason we do not support intel_uncore_write64,
> - * or uncore->funcs.mmio_writeq.
> + * or uncore->funcs->mmio_writeq.
>   *
>   * When reading a 64-bit value as two 32-bit values, the delay may cause
>   * the two reads to mismatch, e.g. a timestamp overflowing. Also note that

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-09  2:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-09  2:34 [Intel-gfx] i915/uncore: constify the uncore vtables. (v2) Dave Airlie
2021-09-09  2:34 ` [Intel-gfx] [PATCH 1/2] drm/i915/uncore: split the fw get function into separate vfunc Dave Airlie
2021-09-09  2:34 ` [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables Dave Airlie
2021-09-09  2:44 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915/uncore: split the fw get function into separate vfunc Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2021-09-08  4:45 [Intel-gfx] [PATCH 0/2] i915/uncore: constify the uncore vtables Dave Airlie
2021-09-08  4:45 ` [Intel-gfx] [PATCH 2/2] drm/i915/uncore: constify the register vtables Dave Airlie
2021-09-08  9:13   ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox