[PATCH 0/6] Display uncore prep patches

[PATCH 0/6] Display uncore prep patches

[Daniele Ceraolo Spurio]

These are preparatory patches for the display/gt uncore split that make
sense even before the split is done. Main goal is to better isolate
forcewake-related actions and to perform some cleanup/simplification of
the code.

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

Daniele Ceraolo Spurio (6):
  drm/i915: use vfuncs for reg_read/write_fw_domains
  drm/i915: kill uncore_sanitize
  drm/i915: kill uncore_to_i915
  drm/i915: skip forcewake actions on forcewake-less uncore
  drm/i915: dynamically allocate forcewake domains
  drm/i915/gvt: decouple check_vgpu() from uncore_init()

 drivers/gpu/drm/i915/i915_drv.c              |  31 +-
 drivers/gpu/drm/i915/i915_drv.h              |   5 -
 drivers/gpu/drm/i915/i915_pvinfo.h           |   5 +-
 drivers/gpu/drm/i915/i915_vgpu.c             |  31 +-
 drivers/gpu/drm/i915/intel_uncore.c          | 483 ++++++++++---------
 drivers/gpu/drm/i915/intel_uncore.h          |  31 +-
 drivers/gpu/drm/i915/selftests/mock_uncore.c |   4 +-
 7 files changed, 321 insertions(+), 269 deletions(-)

-- 
2.20.1

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

[PATCH 1/6] drm/i915: use vfuncs for reg_read/write_fw_domains

[Daniele Ceraolo Spurio]

Instead of going through the if-else chain every time, let's save the
function in the uncore structure. Note that the new functions are
purposely not used from the reg read/write functions to keep the
inlining there.

While at it, use the new macro to call the old ones to clean the code a
bit.

v2: Rename macros for no-forcewake function assignment (Tvrtko)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c          | 172 ++++++++-----------
 drivers/gpu/drm/i915/intel_uncore.h          |   5 +
 drivers/gpu/drm/i915/selftests/mock_uncore.c |   4 +-
 3 files changed, 75 insertions(+), 106 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index da33aa672c3d..8e5716bc53e2 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -901,6 +901,12 @@ static bool is_gen##x##_shadowed(u32 offset) \
 __is_genX_shadowed(8)
 __is_genX_shadowed(11)
 
+static enum forcewake_domains
+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; \
@@ -1145,26 +1151,23 @@ func##_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { \
 	val = __raw_uncore_read##x(uncore, reg); \
 	GEN6_READ_FOOTER; \
 }
-#define __gen6_read(x) __gen_read(gen6, x)
-#define __fwtable_read(x) __gen_read(fwtable, x)
-#define __gen11_fwtable_read(x) __gen_read(gen11_fwtable, x)
-
-__gen11_fwtable_read(8)
-__gen11_fwtable_read(16)
-__gen11_fwtable_read(32)
-__gen11_fwtable_read(64)
-__fwtable_read(8)
-__fwtable_read(16)
-__fwtable_read(32)
-__fwtable_read(64)
-__gen6_read(8)
-__gen6_read(16)
-__gen6_read(32)
-__gen6_read(64)
-
-#undef __gen11_fwtable_read
-#undef __fwtable_read
-#undef __gen6_read
+
+#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)
+
+__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
 #undef GEN6_READ_HEADER
 
@@ -1225,6 +1228,9 @@ gen6_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace)
 	__raw_uncore_write##x(uncore, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
+__gen6_write(8)
+__gen6_write(16)
+__gen6_write(32)
 
 #define __gen_write(func, x) \
 static void \
@@ -1237,38 +1243,33 @@ func##_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trac
 	__raw_uncore_write##x(uncore, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
-#define __gen8_write(x) __gen_write(gen8, x)
-#define __fwtable_write(x) __gen_write(fwtable, x)
-#define __gen11_fwtable_write(x) __gen_write(gen11_fwtable, x)
-
-__gen11_fwtable_write(8)
-__gen11_fwtable_write(16)
-__gen11_fwtable_write(32)
-__fwtable_write(8)
-__fwtable_write(16)
-__fwtable_write(32)
-__gen8_write(8)
-__gen8_write(16)
-__gen8_write(32)
-__gen6_write(8)
-__gen6_write(16)
-__gen6_write(32)
 
-#undef __gen11_fwtable_write
-#undef __fwtable_write
-#undef __gen8_write
-#undef __gen6_write
+#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)
+
+__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
 #undef GEN6_WRITE_HEADER
 
-#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
+#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_READ_MMIO_VFUNCS(uncore, x) \
+#define ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x) \
 do { \
 	(uncore)->funcs.mmio_readb = x##_read8; \
 	(uncore)->funcs.mmio_readw = x##_read16; \
@@ -1276,6 +1277,17 @@ do { \
 	(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)
 
 static void fw_domain_init(struct intel_uncore *uncore,
 			   enum forcewake_domain_id domain_id,
@@ -1559,11 +1571,11 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 
 	if (!intel_uncore_has_forcewake(uncore)) {
 		if (IS_GEN(i915, 5)) {
-			ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
-			ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
+			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
+			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
 		} else {
-			ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
-			ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
+			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
+			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
 		}
 	} else if (IS_GEN_RANGE(i915, 6, 7)) {
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
@@ -1594,6 +1606,12 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	}
 
+	/* 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->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;
 
@@ -1871,62 +1889,6 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
 	return ret;
 }
 
-static enum forcewake_domains
-intel_uncore_forcewake_for_read(struct intel_uncore *uncore,
-				i915_reg_t reg)
-{
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
-	u32 offset = i915_mmio_reg_offset(reg);
-	enum forcewake_domains fw_domains;
-
-	if (INTEL_GEN(i915) >= 11) {
-		fw_domains = __gen11_fwtable_reg_read_fw_domains(uncore, offset);
-	} else if (HAS_FWTABLE(i915)) {
-		fw_domains = __fwtable_reg_read_fw_domains(uncore, offset);
-	} else if (INTEL_GEN(i915) >= 6) {
-		fw_domains = __gen6_reg_read_fw_domains(uncore, offset);
-	} else {
-		/* on devices with FW we expect to hit one of the above cases */
-		if (intel_uncore_has_forcewake(uncore))
-			MISSING_CASE(INTEL_GEN(i915));
-
-		fw_domains = 0;
-	}
-
-	WARN_ON(fw_domains & ~uncore->fw_domains);
-
-	return fw_domains;
-}
-
-static enum forcewake_domains
-intel_uncore_forcewake_for_write(struct intel_uncore *uncore,
-				 i915_reg_t reg)
-{
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
-	u32 offset = i915_mmio_reg_offset(reg);
-	enum forcewake_domains fw_domains;
-
-	if (INTEL_GEN(i915) >= 11) {
-		fw_domains = __gen11_fwtable_reg_write_fw_domains(uncore, offset);
-	} else if (HAS_FWTABLE(i915) && !IS_VALLEYVIEW(i915)) {
-		fw_domains = __fwtable_reg_write_fw_domains(uncore, offset);
-	} else if (IS_GEN(i915, 8)) {
-		fw_domains = __gen8_reg_write_fw_domains(uncore, offset);
-	} else if (IS_GEN_RANGE(i915, 6, 7)) {
-		fw_domains = FORCEWAKE_RENDER;
-	} else {
-		/* on devices with FW we expect to hit one of the above cases */
-		if (intel_uncore_has_forcewake(uncore))
-			MISSING_CASE(INTEL_GEN(i915));
-
-		fw_domains = 0;
-	}
-
-	WARN_ON(fw_domains & ~uncore->fw_domains);
-
-	return fw_domains;
-}
-
 /**
  * intel_uncore_forcewake_for_reg - which forcewake domains are needed to access
  * 				    a register
@@ -1953,10 +1915,12 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
 		return 0;
 
 	if (op & FW_REG_READ)
-		fw_domains = intel_uncore_forcewake_for_read(uncore, reg);
+		fw_domains = uncore->funcs.read_fw_domains(uncore, reg);
 
 	if (op & FW_REG_WRITE)
-		fw_domains |= intel_uncore_forcewake_for_write(uncore, reg);
+		fw_domains |= uncore->funcs.write_fw_domains(uncore, reg);
+
+	WARN_ON(fw_domains & ~uncore->fw_domains);
 
 	return fw_domains;
 }
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 804a0faacc91..4afde0c44ffe 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -70,6 +70,11 @@ struct intel_uncore_funcs {
 	void (*force_wake_put)(struct intel_uncore *uncore,
 			       enum forcewake_domains domains);
 
+	enum forcewake_domains (*read_fw_domains)(struct intel_uncore *uncore,
+						  i915_reg_t r);
+	enum forcewake_domains (*write_fw_domains)(struct intel_uncore *uncore,
+						   i915_reg_t r);
+
 	u8 (*mmio_readb)(struct intel_uncore *uncore,
 			 i915_reg_t r, bool trace);
 	u16 (*mmio_readw)(struct intel_uncore *uncore,
diff --git a/drivers/gpu/drm/i915/selftests/mock_uncore.c b/drivers/gpu/drm/i915/selftests/mock_uncore.c
index ff8999c63a12..49585f16d4a2 100644
--- a/drivers/gpu/drm/i915/selftests/mock_uncore.c
+++ b/drivers/gpu/drm/i915/selftests/mock_uncore.c
@@ -41,6 +41,6 @@ __nop_read(64)
 
 void mock_uncore_init(struct intel_uncore *uncore)
 {
-	ASSIGN_WRITE_MMIO_VFUNCS(uncore, nop);
-	ASSIGN_READ_MMIO_VFUNCS(uncore, nop);
+	ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, nop);
+	ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, nop);
 }
-- 
2.20.1

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

[PATCH 2/6] drm/i915: kill uncore_sanitize

[Daniele Ceraolo Spurio]

uncore_sanitize performs no action on the uncore structure and just
calls intel_sanitize_gt_powersave, so we can just call the latter
directly.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     | 12 ++++++++++--
 drivers/gpu/drm/i915/intel_uncore.c |  9 ---------
 drivers/gpu/drm/i915/intel_uncore.h |  1 -
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 535b9be4fc58..6cf8f1838cec 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1623,7 +1623,8 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY,
 			   PM_QOS_DEFAULT_VALUE);
 
-	intel_uncore_sanitize(dev_priv);
+	/* BIOS often leaves RC6 enabled, but disable it for hw init */
+	intel_sanitize_gt_powersave(dev_priv);
 
 	intel_gt_init_workarounds(dev_priv);
 
@@ -1915,6 +1916,9 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 out_cleanup_hw:
 	i915_driver_cleanup_hw(dev_priv);
 	i915_ggtt_cleanup_hw(dev_priv);
+
+	/* Paranoia: make sure we have disabled everything before we exit. */
+	intel_sanitize_gt_powersave(dev_priv);
 out_cleanup_mmio:
 	i915_driver_cleanup_mmio(dev_priv);
 out_runtime_pm_put:
@@ -1985,6 +1989,10 @@ static void i915_driver_release(struct drm_device *dev)
 	i915_gem_fini(dev_priv);
 
 	i915_ggtt_cleanup_hw(dev_priv);
+
+	/* Paranoia: make sure we have disabled everything before we exit. */
+	intel_sanitize_gt_powersave(dev_priv);
+
 	i915_driver_cleanup_mmio(dev_priv);
 
 	enable_rpm_wakeref_asserts(rpm);
@@ -2351,7 +2359,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 		hsw_disable_pc8(dev_priv);
 	}
 
-	intel_uncore_sanitize(dev_priv);
+	intel_sanitize_gt_powersave(dev_priv);
 
 	intel_power_domains_resume(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8e5716bc53e2..63bdadacadcc 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -537,12 +537,6 @@ void intel_uncore_runtime_resume(struct intel_uncore *uncore)
 	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
 }
 
-void intel_uncore_sanitize(struct drm_i915_private *dev_priv)
-{
-	/* BIOS often leaves RC6 enabled, but disable it for hw init */
-	intel_sanitize_gt_powersave(dev_priv);
-}
-
 static void __intel_uncore_forcewake_get(struct intel_uncore *uncore,
 					 enum forcewake_domains fw_domains)
 {
@@ -1664,9 +1658,6 @@ void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore)
 
 void intel_uncore_fini_mmio(struct intel_uncore *uncore)
 {
-	/* Paranoia: make sure we have disabled everything before we exit. */
-	intel_uncore_sanitize(uncore_to_i915(uncore));
-
 	iosf_mbi_punit_acquire();
 	iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
 		&uncore->pmic_bus_access_nb);
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 4afde0c44ffe..94c00d3778b1 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -182,7 +182,6 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
 	return uncore->flags & UNCORE_HAS_FIFO;
 }
 
-void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
 void intel_uncore_init_early(struct intel_uncore *uncore);
 int intel_uncore_init_mmio(struct intel_uncore *uncore);
 void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore);
-- 
2.20.1

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

[PATCH 3/6] drm/i915: kill uncore_to_i915

[Daniele Ceraolo Spurio]

Let's get rid of it before it proliferates, since with split GT/Display
uncores the container_of won't work anymore.

I've kept the rpm pointer as well to minimize the pointer chasing in the
MMIO accessors.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  2 +-
 drivers/gpu/drm/i915/i915_drv.h     |  5 -----
 drivers/gpu/drm/i915/intel_uncore.c | 24 ++++++++++++------------
 drivers/gpu/drm/i915/intel_uncore.h |  4 +++-
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6cf8f1838cec..d113296cbe34 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -894,7 +894,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
 
 	intel_device_info_subplatform_init(dev_priv);
 
-	intel_uncore_init_early(&dev_priv->uncore);
+	intel_uncore_init_early(dev_priv, &dev_priv->uncore);
 
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->gpu_error.lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a9c2392cc7c..3b42588b7194 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1949,11 +1949,6 @@ static inline struct drm_i915_private *huc_to_i915(struct intel_huc *huc)
 	return container_of(huc, struct drm_i915_private, huc);
 }
 
-static inline struct drm_i915_private *uncore_to_i915(struct intel_uncore *uncore)
-{
-	return container_of(uncore, struct drm_i915_private, uncore);
-}
-
 /* Simple iterator over all initialised engines */
 #define for_each_engine(engine__, dev_priv__, id__) \
 	for ((id__) = 0; \
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 63bdadacadcc..88a69bf713c9 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -322,7 +322,7 @@ static void __gen6_gt_wait_for_fifo(struct intel_uncore *uncore)
 
 	/* On VLV, FIFO will be shared by both SW and HW.
 	 * So, we need to read the FREE_ENTRIES everytime */
-	if (IS_VALLEYVIEW(uncore_to_i915(uncore)))
+	if (IS_VALLEYVIEW(uncore->i915))
 		n = fifo_free_entries(uncore);
 	else
 		n = uncore->fifo_count;
@@ -493,7 +493,7 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
 		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
 
 	/* WaDisableShadowRegForCpd:chv */
-	if (IS_CHERRYVIEW(uncore_to_i915(uncore))) {
+	if (IS_CHERRYVIEW(uncore->i915)) {
 		__raw_uncore_write32(uncore, GTFIFOCTL,
 				     __raw_uncore_read32(uncore, GTFIFOCTL) |
 				     GT_FIFO_CTL_BLOCK_ALL_POLICY_STALL |
@@ -622,7 +622,7 @@ void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
 	spin_lock_irq(&uncore->lock);
 	if (!--uncore->user_forcewake.count) {
 		if (intel_uncore_unclaimed_mmio(uncore))
-			dev_info(uncore_to_i915(uncore)->drm.dev,
+			dev_info(uncore->i915->drm.dev,
 				 "Invalid mmio detected during user access\n");
 
 		uncore->unclaimed_mmio_check =
@@ -1346,7 +1346,7 @@ static void fw_domain_fini(struct intel_uncore *uncore,
 
 static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 {
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
+	struct drm_i915_private *i915 = uncore->i915;
 
 	if (!intel_uncore_has_forcewake(uncore))
 		return;
@@ -1499,7 +1499,7 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
 
 static int uncore_mmio_setup(struct intel_uncore *uncore)
 {
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
+	struct drm_i915_private *i915 = uncore->i915;
 	struct pci_dev *pdev = i915->drm.pdev;
 	int mmio_bar;
 	int mmio_size;
@@ -1529,20 +1529,22 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)
 
 static void uncore_mmio_cleanup(struct intel_uncore *uncore)
 {
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
-	struct pci_dev *pdev = i915->drm.pdev;
+	struct pci_dev *pdev = uncore->i915->drm.pdev;
 
 	pci_iounmap(pdev, uncore->regs);
 }
 
-void intel_uncore_init_early(struct intel_uncore *uncore)
+void intel_uncore_init_early(struct drm_i915_private *i915,
+			     struct intel_uncore *uncore)
 {
 	spin_lock_init(&uncore->lock);
+	uncore->i915 = i915;
+	uncore->rpm = &i915->runtime_pm;
 }
 
 int intel_uncore_init_mmio(struct intel_uncore *uncore)
 {
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
+	struct drm_i915_private *i915 = uncore->i915;
 	int ret;
 
 	ret = uncore_mmio_setup(uncore);
@@ -1561,8 +1563,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 	uncore->pmic_bus_access_nb.notifier_call =
 		i915_pmic_bus_access_notifier;
 
-	uncore->rpm = &i915->runtime_pm;
-
 	if (!intel_uncore_has_forcewake(uncore)) {
 		if (IS_GEN(i915, 5)) {
 			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
@@ -1627,7 +1627,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
  */
 void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore)
 {
-	struct drm_i915_private *i915 = uncore_to_i915(uncore);
+	struct drm_i915_private *i915 = uncore->i915;
 
 	if (INTEL_GEN(i915) >= 11) {
 		enum forcewake_domains fw_domains = uncore->fw_domains;
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 94c00d3778b1..912616188ff5 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -102,6 +102,7 @@ struct intel_forcewake_range {
 struct intel_uncore {
 	void __iomem *regs;
 
+	struct drm_i915_private *i915;
 	struct intel_runtime_pm *rpm;
 
 	spinlock_t lock; /** lock is also taken in irq contexts. */
@@ -182,7 +183,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
 	return uncore->flags & UNCORE_HAS_FIFO;
 }
 
-void intel_uncore_init_early(struct intel_uncore *uncore);
+void intel_uncore_init_early(struct drm_i915_private *i915,
+			     struct intel_uncore *uncore);
 int intel_uncore_init_mmio(struct intel_uncore *uncore);
 void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore);
 bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore);
-- 
2.20.1

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

[PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Daniele Ceraolo Spurio]

We always call some of the setup/cleanup functions for forcewake, even
if the feature is not actually available. Skipping these operations if
forcewake is not available saves us some operations on older gens and
prepares us for having a forcewake-less display uncore.
The suspend/resume functions have also been renamed to clearly indicate
that they only operate on forcewake status.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  15 +--
 drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_uncore.h |   8 +-
 3 files changed, 101 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d113296cbe34..95b36fe17f99 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
 
 	intel_device_info_init_mmio(dev_priv);
 
-	intel_uncore_prune_mmio_domains(&dev_priv->uncore);
+	intel_uncore_prune_forcewake_domains(&dev_priv->uncore);
 
 	intel_uc_init_mmio(dev_priv);
 
@@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 
 	i915_gem_suspend_late(dev_priv);
 
-	intel_uncore_suspend(&dev_priv->uncore);
+	intel_uncore_forcewake_suspend(&dev_priv->uncore);
 
 	intel_power_domains_suspend(dev_priv,
 				    get_suspend_mode(dev_priv, hibernation));
@@ -2348,7 +2348,10 @@ static int i915_drm_resume_early(struct drm_device *dev)
 		DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
 			  ret);
 
-	intel_uncore_resume_early(&dev_priv->uncore);
+	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
+		DRM_DEBUG("unclaimed mmio detected on resume, clearing\n");
+
+	intel_uncore_forcewake_resume_early(&dev_priv->uncore);
 
 	i915_check_and_clear_faults(dev_priv);
 
@@ -2923,7 +2926,7 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
-	intel_uncore_suspend(&dev_priv->uncore);
+	intel_uncore_forcewake_suspend(&dev_priv->uncore);
 
 	ret = 0;
 	if (INTEL_GEN(dev_priv) >= 11) {
@@ -2940,7 +2943,7 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
-		intel_uncore_runtime_resume(&dev_priv->uncore);
+		intel_uncore_forcewake_runtime_resume(&dev_priv->uncore);
 
 		intel_runtime_pm_enable_interrupts(dev_priv);
 
@@ -3038,7 +3041,7 @@ static int intel_runtime_resume(struct device *kdev)
 		ret = vlv_resume_prepare(dev_priv, true);
 	}
 
-	intel_uncore_runtime_resume(&dev_priv->uncore);
+	intel_uncore_forcewake_runtime_resume(&dev_priv->uncore);
 
 	intel_runtime_pm_enable_interrupts(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 88a69bf713c9..c0f5567ee096 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -485,12 +485,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore)
 	return ret;
 }
 
-static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
+static void forcewake_early_sanitize(struct intel_uncore *uncore,
 					  unsigned int restore_forcewake)
 {
-	/* clear out unclaimed reg detection bit */
-	if (check_for_unclaimed_mmio(uncore))
-		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
+	if (!intel_uncore_has_forcewake(uncore))
+		return;
 
 	/* WaDisableShadowRegForCpd:chv */
 	if (IS_CHERRYVIEW(uncore->i915)) {
@@ -513,8 +512,11 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
 	iosf_mbi_punit_release();
 }
 
-void intel_uncore_suspend(struct intel_uncore *uncore)
+void intel_uncore_forcewake_suspend(struct intel_uncore *uncore)
 {
+	if (!intel_uncore_has_forcewake(uncore))
+		return;
+
 	iosf_mbi_punit_acquire();
 	iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
 		&uncore->pmic_bus_access_nb);
@@ -522,18 +524,24 @@ void intel_uncore_suspend(struct intel_uncore *uncore)
 	iosf_mbi_punit_release();
 }
 
-void intel_uncore_resume_early(struct intel_uncore *uncore)
+void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore)
 {
 	unsigned int restore_forcewake;
 
+	if (!intel_uncore_has_forcewake(uncore))
+		return;
+
 	restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved);
-	__intel_uncore_early_sanitize(uncore, restore_forcewake);
+	forcewake_early_sanitize(uncore, restore_forcewake);
 
 	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
 }
 
-void intel_uncore_runtime_resume(struct intel_uncore *uncore)
+void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore)
 {
+	if (!intel_uncore_has_forcewake(uncore))
+		return;
+
 	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
 }
 
@@ -1348,8 +1356,7 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
 
-	if (!intel_uncore_has_forcewake(uncore))
-		return;
+	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
 
 	if (INTEL_GEN(i915) >= 11) {
 		int i;
@@ -1542,36 +1549,29 @@ void intel_uncore_init_early(struct drm_i915_private *i915,
 	uncore->rpm = &i915->runtime_pm;
 }
 
-int intel_uncore_init_mmio(struct intel_uncore *uncore)
+static void uncore_raw_init(struct intel_uncore *uncore)
 {
-	struct drm_i915_private *i915 = uncore->i915;
-	int ret;
+	GEM_BUG_ON(intel_uncore_has_forcewake(uncore));
 
-	ret = uncore_mmio_setup(uncore);
-	if (ret)
-		return ret;
+	if (IS_GEN(uncore->i915, 5)) {
+		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
+		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
+	} else {
+		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
+		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
+	}
+}
 
-	i915_check_vgpu(i915);
+static void uncore_forcewake_init(struct intel_uncore *uncore)
+{
+	struct drm_i915_private *i915 = uncore->i915;
 
-	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
-		uncore->flags |= UNCORE_HAS_FORCEWAKE;
+	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
 
 	intel_uncore_fw_domains_init(uncore);
-	__intel_uncore_early_sanitize(uncore, 0);
+	forcewake_early_sanitize(uncore, 0);
 
-	uncore->unclaimed_mmio_check = 1;
-	uncore->pmic_bus_access_nb.notifier_call =
-		i915_pmic_bus_access_notifier;
-
-	if (!intel_uncore_has_forcewake(uncore)) {
-		if (IS_GEN(i915, 5)) {
-			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
-			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
-		} else {
-			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
-			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
-		}
-	} else if (IS_GEN_RANGE(i915, 6, 7)) {
+	if (IS_GEN_RANGE(i915, 6, 7)) {
 		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
 
 		if (IS_VALLEYVIEW(i915)) {
@@ -1585,7 +1585,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 			ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
 			ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
 			ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
-
 		} else {
 			ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
 			ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
@@ -1600,6 +1599,31 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
 	}
 
+	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
+	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
+}
+
+int intel_uncore_init_mmio(struct intel_uncore *uncore)
+{
+	struct drm_i915_private *i915 = uncore->i915;
+	int ret;
+
+	ret = uncore_mmio_setup(uncore);
+	if (ret)
+		return ret;
+
+	i915_check_vgpu(i915);
+
+	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
+		uncore->flags |= UNCORE_HAS_FORCEWAKE;
+
+	uncore->unclaimed_mmio_check = 1;
+
+	if (!intel_uncore_has_forcewake(uncore))
+		uncore_raw_init(uncore);
+	else
+		uncore_forcewake_init(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);
@@ -1615,7 +1639,9 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 	if (IS_GEN_RANGE(i915, 6, 7))
 		uncore->flags |= UNCORE_HAS_FIFO;
 
-	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
+	/* clear out unclaimed reg detection bit */
+	if (check_for_unclaimed_mmio(uncore))
+		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
 
 	return 0;
 }
@@ -1625,44 +1651,47 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
  * the forcewake domains. Prune them, to make sure they only reference existing
  * engines.
  */
-void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore)
+void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
+	enum forcewake_domains fw_domains = uncore->fw_domains;
+	enum forcewake_domain_id domain_id;
+	int i;
 
-	if (INTEL_GEN(i915) >= 11) {
-		enum forcewake_domains fw_domains = uncore->fw_domains;
-		enum forcewake_domain_id domain_id;
-		int i;
+	if (!intel_uncore_has_forcewake(uncore) || INTEL_GEN(i915) < 11)
+		return;
 
-		for (i = 0; i < I915_MAX_VCS; i++) {
-			domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
+	for (i = 0; i < I915_MAX_VCS; i++) {
+		domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
 
-			if (HAS_ENGINE(i915, _VCS(i)))
-				continue;
+		if (HAS_ENGINE(i915, _VCS(i)))
+			continue;
 
-			if (fw_domains & BIT(domain_id))
-				fw_domain_fini(uncore, domain_id);
-		}
+		if (fw_domains & BIT(domain_id))
+			fw_domain_fini(uncore, domain_id);
+	}
 
-		for (i = 0; i < I915_MAX_VECS; i++) {
-			domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
+	for (i = 0; i < I915_MAX_VECS; i++) {
+		domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
 
-			if (HAS_ENGINE(i915, _VECS(i)))
-				continue;
+		if (HAS_ENGINE(i915, _VECS(i)))
+			continue;
 
-			if (fw_domains & BIT(domain_id))
-				fw_domain_fini(uncore, domain_id);
-		}
+		if (fw_domains & BIT(domain_id))
+			fw_domain_fini(uncore, domain_id);
 	}
 }
 
 void intel_uncore_fini_mmio(struct intel_uncore *uncore)
 {
-	iosf_mbi_punit_acquire();
-	iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
-		&uncore->pmic_bus_access_nb);
-	intel_uncore_forcewake_reset(uncore);
-	iosf_mbi_punit_release();
+	if (intel_uncore_has_forcewake(uncore)) {
+		iosf_mbi_punit_acquire();
+		iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
+			&uncore->pmic_bus_access_nb);
+		intel_uncore_forcewake_reset(uncore);
+		iosf_mbi_punit_release();
+	}
+
 	uncore_mmio_cleanup(uncore);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 912616188ff5..879252735bba 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -186,13 +186,13 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
 void intel_uncore_init_early(struct drm_i915_private *i915,
 			     struct intel_uncore *uncore);
 int intel_uncore_init_mmio(struct intel_uncore *uncore);
-void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore);
+void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore);
 bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore);
 bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore);
 void intel_uncore_fini_mmio(struct intel_uncore *uncore);
-void intel_uncore_suspend(struct intel_uncore *uncore);
-void intel_uncore_resume_early(struct intel_uncore *uncore);
-void intel_uncore_runtime_resume(struct intel_uncore *uncore);
+void intel_uncore_forcewake_suspend(struct intel_uncore *uncore);
+void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore);
+void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore);
 
 void assert_forcewakes_inactive(struct intel_uncore *uncore);
 void assert_forcewakes_active(struct intel_uncore *uncore,
-- 
2.20.1

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

[PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Daniele Ceraolo Spurio]

We'd like to introduce a display uncore with no forcewake domains, so
let's avoid wasting memory and be ready to allocate only what we need.
Even without multiple uncore, we still don't need all the domains on all
gens.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 155 ++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_uncore.h |  13 +--
 2 files changed, 102 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c0f5567ee096..7f311827c3ef 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -344,7 +344,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 {
 	struct intel_uncore_forcewake_domain *domain =
 	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
-	struct intel_uncore *uncore = forcewake_domain_to_uncore(domain);
+	struct intel_uncore *uncore = domain->uncore;
 	unsigned long irqflags;
 
 	assert_rpm_device_not_suspended(uncore->rpm);
@@ -1291,23 +1291,23 @@ do { \
 	(uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
 } while (0)
 
-static void fw_domain_init(struct intel_uncore *uncore,
+static int fw_domain_init(struct intel_uncore *uncore,
 			   enum forcewake_domain_id domain_id,
 			   i915_reg_t reg_set,
 			   i915_reg_t reg_ack)
 {
 	struct intel_uncore_forcewake_domain *d;
 
-	if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
-		return;
-
-	d = &uncore->fw_domain[domain_id];
+	GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
 
-	WARN_ON(d->wake_count);
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
 
 	WARN_ON(!i915_mmio_reg_valid(reg_set));
 	WARN_ON(!i915_mmio_reg_valid(reg_ack));
 
+	d->uncore = uncore;
 	d->wake_count = 0;
 	d->reg_set = uncore->regs + i915_mmio_reg_offset(reg_set);
 	d->reg_ack = uncore->regs + i915_mmio_reg_offset(reg_ack);
@@ -1324,7 +1324,6 @@ static void fw_domain_init(struct intel_uncore *uncore,
 	BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX0 != (1 << FW_DOMAIN_ID_MEDIA_VEBOX0));
 	BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX1 != (1 << FW_DOMAIN_ID_MEDIA_VEBOX1));
 
-
 	d->mask = BIT(domain_id);
 
 	hrtimer_init(&d->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
@@ -1333,6 +1332,10 @@ static void fw_domain_init(struct intel_uncore *uncore,
 	uncore->fw_domains |= BIT(domain_id);
 
 	fw_domain_reset(d);
+
+	uncore->fw_domain[domain_id] = d;
+
+	return 0;
 }
 
 static void fw_domain_fini(struct intel_uncore *uncore,
@@ -1340,77 +1343,92 @@ static void fw_domain_fini(struct intel_uncore *uncore,
 {
 	struct intel_uncore_forcewake_domain *d;
 
-	if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
-		return;
+	GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
 
-	d = &uncore->fw_domain[domain_id];
+	d = fetch_and_zero(&uncore->fw_domain[domain_id]);
+	uncore->fw_domains &= ~BIT(domain_id);
 
-	WARN_ON(d->wake_count);
-	WARN_ON(hrtimer_cancel(&d->timer));
-	memset(d, 0, sizeof(*d));
+	if (d) {
+		WARN_ON(d->wake_count);
+		WARN_ON(hrtimer_cancel(&d->timer));
+		kfree(d);
+	}
+}
 
-	uncore->fw_domains &= ~BIT(domain_id);
+static void intel_uncore_fw_domains_fini(struct intel_uncore *uncore)
+{
+	struct intel_uncore_forcewake_domain *d;
+	int tmp;
+
+	for_each_fw_domain(d, uncore, tmp)
+		fw_domain_fini(uncore, d->id);
 }
 
-static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
+static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
+	int ret;
 
 	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
 
+#define __fw_domain_init(id__, set__, ack__) \
+	ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
+	if (ret) \
+		goto out_clean;
+
 	if (INTEL_GEN(i915) >= 11) {
 		int i;
 
-		uncore->funcs.force_wake_get =
-			fw_domains_get_with_fallback;
+		uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
 		uncore->funcs.force_wake_put = fw_domains_put;
-		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
-			       FORCEWAKE_RENDER_GEN9,
-			       FORCEWAKE_ACK_RENDER_GEN9);
-		fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
-			       FORCEWAKE_BLITTER_GEN9,
-			       FORCEWAKE_ACK_BLITTER_GEN9);
+		__fw_domain_init(FW_DOMAIN_ID_RENDER,
+				 FORCEWAKE_RENDER_GEN9,
+				 FORCEWAKE_ACK_RENDER_GEN9);
+		__fw_domain_init(FW_DOMAIN_ID_BLITTER,
+				 FORCEWAKE_BLITTER_GEN9,
+				 FORCEWAKE_ACK_BLITTER_GEN9);
+
 		for (i = 0; i < I915_MAX_VCS; i++) {
 			if (!HAS_ENGINE(i915, _VCS(i)))
 				continue;
 
-			fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
-				       FORCEWAKE_MEDIA_VDBOX_GEN11(i),
-				       FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
+			__fw_domain_init(FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
+					 FORCEWAKE_MEDIA_VDBOX_GEN11(i),
+					 FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
 		}
 		for (i = 0; i < I915_MAX_VECS; i++) {
 			if (!HAS_ENGINE(i915, _VECS(i)))
 				continue;
 
-			fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
-				       FORCEWAKE_MEDIA_VEBOX_GEN11(i),
-				       FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
+			__fw_domain_init(FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
+					 FORCEWAKE_MEDIA_VEBOX_GEN11(i),
+					 FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
 		}
 	} else if (IS_GEN_RANGE(i915, 9, 10)) {
-		uncore->funcs.force_wake_get =
-			fw_domains_get_with_fallback;
+		uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
 		uncore->funcs.force_wake_put = fw_domains_put;
-		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
-			       FORCEWAKE_RENDER_GEN9,
-			       FORCEWAKE_ACK_RENDER_GEN9);
-		fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
-			       FORCEWAKE_BLITTER_GEN9,
-			       FORCEWAKE_ACK_BLITTER_GEN9);
-		fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
-			       FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
+		__fw_domain_init(FW_DOMAIN_ID_RENDER,
+				 FORCEWAKE_RENDER_GEN9,
+				 FORCEWAKE_ACK_RENDER_GEN9);
+		__fw_domain_init(FW_DOMAIN_ID_BLITTER,
+				 FORCEWAKE_BLITTER_GEN9,
+				 FORCEWAKE_ACK_BLITTER_GEN9);
+		__fw_domain_init(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;
-		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);
+		__fw_domain_init(FW_DOMAIN_ID_RENDER,
+				 FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
+		__fw_domain_init(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;
-		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
-			       FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
+		__fw_domain_init(FW_DOMAIN_ID_RENDER,
+				 FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
 	} else if (IS_IVYBRIDGE(i915)) {
 		u32 ecobus;
 
@@ -1437,8 +1455,8 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 		__raw_uncore_write32(uncore, FORCEWAKE, 0);
 		__raw_posting_read(uncore, ECOBUS);
 
-		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
-			       FORCEWAKE_MT, FORCEWAKE_MT_ACK);
+		__fw_domain_init(FW_DOMAIN_ID_RENDER,
+				 FORCEWAKE_MT, FORCEWAKE_MT_ACK);
 
 		spin_lock_irq(&uncore->lock);
 		fw_domains_get_with_thread_status(uncore, FORCEWAKE_RENDER);
@@ -1449,19 +1467,27 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
 		if (!(ecobus & FORCEWAKE_MT_ENABLE)) {
 			DRM_INFO("No MT forcewake available on Ivybridge, this can result in issues\n");
 			DRM_INFO("when using vblank-synced partial screen updates.\n");
-			fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
-				       FORCEWAKE, FORCEWAKE_ACK);
+			__fw_domain_init(FW_DOMAIN_ID_RENDER,
+					 FORCEWAKE, FORCEWAKE_ACK);
 		}
 	} else if (IS_GEN(i915, 6)) {
 		uncore->funcs.force_wake_get =
 			fw_domains_get_with_thread_status;
 		uncore->funcs.force_wake_put = fw_domains_put;
-		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
-			       FORCEWAKE, FORCEWAKE_ACK);
+		__fw_domain_init(FW_DOMAIN_ID_RENDER,
+				 FORCEWAKE, FORCEWAKE_ACK);
 	}
 
+#undef __fw_domain_init
+
 	/* All future platforms are expected to require complex power gating */
 	WARN_ON(uncore->fw_domains == 0);
+
+	return 0;
+
+out_clean:
+	intel_uncore_fw_domains_fini(uncore);
+	return ret;
 }
 
 #define ASSIGN_FW_DOMAINS_TABLE(uncore, d) \
@@ -1562,13 +1588,17 @@ static void uncore_raw_init(struct intel_uncore *uncore)
 	}
 }
 
-static void uncore_forcewake_init(struct intel_uncore *uncore)
+static int uncore_forcewake_init(struct intel_uncore *uncore)
 {
 	struct drm_i915_private *i915 = uncore->i915;
+	int ret;
 
 	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
 
-	intel_uncore_fw_domains_init(uncore);
+	ret = intel_uncore_fw_domains_init(uncore);
+	if (ret)
+		return ret;
+
 	forcewake_early_sanitize(uncore, 0);
 
 	if (IS_GEN_RANGE(i915, 6, 7)) {
@@ -1601,6 +1631,8 @@ static void uncore_forcewake_init(struct intel_uncore *uncore)
 
 	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
 	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
+
+	return 0;
 }
 
 int intel_uncore_init_mmio(struct intel_uncore *uncore)
@@ -1619,10 +1651,13 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 
 	uncore->unclaimed_mmio_check = 1;
 
-	if (!intel_uncore_has_forcewake(uncore))
+	if (!intel_uncore_has_forcewake(uncore)) {
 		uncore_raw_init(uncore);
-	else
-		uncore_forcewake_init(uncore);
+	} else {
+		ret = uncore_forcewake_init(uncore);
+		if (ret)
+			goto out_mmio_cleanup;
+	}
 
 	/* 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);
@@ -1644,6 +1679,11 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
 
 	return 0;
+
+out_mmio_cleanup:
+	uncore_mmio_cleanup(uncore);
+
+	return ret;
 }
 
 /*
@@ -1689,6 +1729,7 @@ void intel_uncore_fini_mmio(struct intel_uncore *uncore)
 		iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
 			&uncore->pmic_bus_access_nb);
 		intel_uncore_forcewake_reset(uncore);
+		intel_uncore_fw_domains_fini(uncore);
 		iosf_mbi_punit_release();
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 879252735bba..bbff281b880d 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -126,6 +126,7 @@ struct intel_uncore {
 	enum forcewake_domains fw_domains_saved; /* user domains saved for S3 */
 
 	struct intel_uncore_forcewake_domain {
+		struct intel_uncore *uncore;
 		enum forcewake_domain_id id;
 		enum forcewake_domains mask;
 		unsigned int wake_count;
@@ -133,7 +134,7 @@ struct intel_uncore {
 		struct hrtimer timer;
 		u32 __iomem *reg_set;
 		u32 __iomem *reg_ack;
-	} fw_domain[FW_DOMAIN_ID_COUNT];
+	} *fw_domain[FW_DOMAIN_ID_COUNT];
 
 	struct {
 		unsigned int count;
@@ -147,18 +148,12 @@ struct intel_uncore {
 
 /* Iterate over initialised fw domains */
 #define for_each_fw_domain_masked(domain__, mask__, uncore__, tmp__) \
-	for (tmp__ = (mask__); \
-	     tmp__ ? (domain__ = &(uncore__)->fw_domain[__mask_next_bit(tmp__)]), 1 : 0;)
+	for (tmp__ = (mask__); tmp__ ;) \
+		for_each_if(domain__ = (uncore__)->fw_domain[__mask_next_bit(tmp__)])
 
 #define for_each_fw_domain(domain__, uncore__, tmp__) \
 	for_each_fw_domain_masked(domain__, (uncore__)->fw_domains, uncore__, tmp__)
 
-static inline struct intel_uncore *
-forcewake_domain_to_uncore(const struct intel_uncore_forcewake_domain *d)
-{
-	return container_of(d, struct intel_uncore, fw_domain[d->id]);
-}
-
 static inline bool
 intel_uncore_has_forcewake(const struct intel_uncore *uncore)
 {
-- 
2.20.1

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

[PATCH 6/6] drm/i915/gvt: decouple check_vgpu() from uncore_init()

[Daniele Ceraolo Spurio]

With multiple uncore to initialize (GT vs Display), it makes little
sense to have the vgpu_check inside uncore_init(). We also have
a catch-22 scenario where the uncore is required to read the vgpu
capabilities while the vgpu capabilities are required to decide if
we need to initialize forcewake support. To remove this circular
dependency, we can perform the required MMIO access by mmapping just
the vgtif shared page in mmio space and use raw accessors.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  2 ++
 drivers/gpu/drm/i915/i915_pvinfo.h  |  5 +++--
 drivers/gpu/drm/i915/i915_vgpu.c    | 31 +++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_uncore.c |  2 --
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 95b36fe17f99..c3d9d1a366b0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1893,6 +1893,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
+	i915_check_vgpu(dev_priv);
+
 	ret = i915_driver_init_mmio(dev_priv);
 	if (ret < 0)
 		goto out_runtime_pm_put;
diff --git a/drivers/gpu/drm/i915/i915_pvinfo.h b/drivers/gpu/drm/i915/i915_pvinfo.h
index 969e514916ab..ca4661e98f79 100644
--- a/drivers/gpu/drm/i915/i915_pvinfo.h
+++ b/drivers/gpu/drm/i915/i915_pvinfo.h
@@ -110,8 +110,9 @@ struct vgt_if {
 	u32  rsv7[0x200 - 24];    /* pad to one page */
 } __packed;
 
-#define vgtif_reg(x) \
-	_MMIO((VGT_PVINFO_PAGE + offsetof(struct vgt_if, x)))
+#define vgtif_offset(x) (offsetof(struct vgt_if, x))
+
+#define vgtif_reg(x) _MMIO(VGT_PVINFO_PAGE + vgtif_offset(x))
 
 /* vGPU display status to be used by the host side */
 #define VGT_DRV_DISPLAY_NOT_READY 0
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 94d3992b599d..5f160312fa6e 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -60,26 +60,45 @@
  */
 void i915_check_vgpu(struct drm_i915_private *dev_priv)
 {
-	struct intel_uncore *uncore = &dev_priv->uncore;
+	struct pci_dev *pdev = dev_priv->drm.pdev;
 	u64 magic;
 	u16 version_major;
+	void __iomem *shared_area;
 
 	BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
 
-	magic = __raw_uncore_read64(uncore, vgtif_reg(magic));
-	if (magic != VGT_MAGIC)
+	/*
+	 * This is called before we setup the main MMIO BAR mappings used via
+	 * the uncore structure, so we need to access the BAR directly. Since
+	 * we do not support VGT on older gens, return early so we don't have
+	 * to consider differently numbered or sized MMIO bars
+	 */
+	if (INTEL_GEN(dev_priv) < 6)
+		return;
+
+	shared_area = pci_iomap_range(pdev, 0, VGT_PVINFO_PAGE, VGT_PVINFO_SIZE);
+	if (!shared_area) {
+		DRM_ERROR("failed to map MMIO bar to check for VGT\n");
 		return;
+	}
+
+	magic = readq(shared_area + vgtif_offset(magic));
+	if (magic != VGT_MAGIC)
+		goto out;
 
-	version_major = __raw_uncore_read16(uncore, vgtif_reg(version_major));
+	version_major = readw(shared_area + vgtif_offset(version_major));
 	if (version_major < VGT_VERSION_MAJOR) {
 		DRM_INFO("VGT interface version mismatch!\n");
-		return;
+		goto out;
 	}
 
-	dev_priv->vgpu.caps = __raw_uncore_read32(uncore, vgtif_reg(vgt_caps));
+	dev_priv->vgpu.caps = readl(shared_area + vgtif_offset(vgt_caps));
 
 	dev_priv->vgpu.active = true;
 	DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
+
+out:
+	pci_iounmap(pdev, shared_area);
 }
 
 bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 7f311827c3ef..f19ee2dbb43c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1644,8 +1644,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
 	if (ret)
 		return ret;
 
-	i915_check_vgpu(i915);
-
 	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
 		uncore->flags |= UNCORE_HAS_FORCEWAKE;
 
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for Display uncore prep patches

[Patchwork]

== Series Details ==

Series: Display uncore prep patches
URL   : https://patchwork.freedesktop.org/series/62232/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2803e5056bc3 drm/i915: use vfuncs for reg_read/write_fw_domains
-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'func' - possible side-effects?
#61: FILE: drivers/gpu/drm/i915/intel_uncore.c:1155:
+#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)

-:84: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#84: FILE: drivers/gpu/drm/i915/intel_uncore.c:1231:
 }
+__gen6_write(8)

-:115: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'func' - possible side-effects?
#115: FILE: drivers/gpu/drm/i915/intel_uncore.c:1247:
+#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)

-:134: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'uncore' - possible side-effects?
#134: FILE: drivers/gpu/drm/i915/intel_uncore.c:1265:
+#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)

-:142: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'uncore' - possible side-effects?
#142: FILE: drivers/gpu/drm/i915/intel_uncore.c:1272:
+#define ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x) \
 do { \
 	(uncore)->funcs.mmio_readb = x##_read8; \
 	(uncore)->funcs.mmio_readw = x##_read16; \

-:150: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'uncore' - possible side-effects?
#150: FILE: drivers/gpu/drm/i915/intel_uncore.c:1280:
+#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)

-:156: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'uncore' - possible side-effects?
#156: FILE: drivers/gpu/drm/i915/intel_uncore.c:1286:
+#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)

total: 0 errors, 0 warnings, 7 checks, 258 lines checked
028d6e2ee6b1 drm/i915: kill uncore_sanitize
5b00f7cabec2 drm/i915: kill uncore_to_i915
e4dd10b092d7 drm/i915: skip forcewake actions on forcewake-less uncore
-:86: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#86: FILE: drivers/gpu/drm/i915/intel_uncore.c:489:
+static void forcewake_early_sanitize(struct intel_uncore *uncore,
 					  unsigned int restore_forcewake)

-:314: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#314: FILE: drivers/gpu/drm/i915/intel_uncore.c:1689:
+		iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(

total: 0 errors, 0 warnings, 2 checks, 302 lines checked
769ba7ddece2 drm/i915: dynamically allocate forcewake domains
-:32: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#32: FILE: drivers/gpu/drm/i915/intel_uncore.c:1295:
+static int fw_domain_init(struct intel_uncore *uncore,
 			   enum forcewake_domain_id domain_id,

-:115: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
#115: FILE: drivers/gpu/drm/i915/intel_uncore.c:1374:
+#define __fw_domain_init(id__, set__, ack__) \
+	ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
+	if (ret) \
+		goto out_clean;

-:115: WARNING:MACRO_WITH_FLOW_CONTROL: Macros with flow control statements should be avoided
#115: FILE: drivers/gpu/drm/i915/intel_uncore.c:1374:
+#define __fw_domain_init(id__, set__, ack__) \
+	ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
+	if (ret) \
+		goto out_clean;

-:115: WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon
#115: FILE: drivers/gpu/drm/i915/intel_uncore.c:1374:
+#define __fw_domain_init(id__, set__, ack__) \
+	ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
+	if (ret) \
+		goto out_clean;

total: 1 errors, 2 warnings, 1 checks, 321 lines checked
87b897489919 drm/i915/gvt: decouple check_vgpu() from uncore_init()

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

✓ Fi.CI.BAT: success for Display uncore prep patches

[Patchwork]

== Series Details ==

Series: Display uncore prep patches
URL   : https://patchwork.freedesktop.org/series/62232/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6287 -> Patchwork_13312
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/

Known issues
------------

  Here are the changes found in Patchwork_13312 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_reloc@basic-cpu-read-noreloc:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/fi-icl-u3/igt@gem_exec_reloc@basic-cpu-read-noreloc.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/fi-icl-u3/igt@gem_exec_reloc@basic-cpu-read-noreloc.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][3] -> [FAIL][4] ([fdo#109485])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][5] -> [DMESG-WARN][6] ([fdo#102614])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
    - fi-icl-u2:          [PASS][7] -> [FAIL][8] ([fdo#103167])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload-no-display:
    - fi-icl-u3:          [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10] +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/fi-icl-u3/igt@i915_module_load@reload-no-display.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/fi-icl-u3/igt@i915_module_load@reload-no-display.html

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (48 -> 38)
------------------------------

  Additional (1): fi-icl-guc 
  Missing    (11): fi-kbl-soraka fi-cml-u2 fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-skl-iommu fi-kbl-8809g fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6287 -> Patchwork_13312

  CI_DRM_6287: 3765c2bb2bf60f35709fba4c23070e2b74e14247 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5059: 1f67ee0d09d6513f487f2be74aae9700e755258a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13312: 87b8974899198d3c29c94dcc24dcc9e2b7d5ebbb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

87b897489919 drm/i915/gvt: decouple check_vgpu() from uncore_init()
769ba7ddece2 drm/i915: dynamically allocate forcewake domains
e4dd10b092d7 drm/i915: skip forcewake actions on forcewake-less uncore
5b00f7cabec2 drm/i915: kill uncore_to_i915
028d6e2ee6b1 drm/i915: kill uncore_sanitize
2803e5056bc3 drm/i915: use vfuncs for reg_read/write_fw_domains

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: use vfuncs for reg_read/write_fw_domains

[Tvrtko Ursulin]

On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
> Instead of going through the if-else chain every time, let's save the
> function in the uncore structure. Note that the new functions are
> purposely not used from the reg read/write functions to keep the
> inlining there.

Looks good to me.

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

On the point of inlining, alternative would be to let compiler decide. 
It doesn't matter a lot though and I don't remember/know how the end of 
the gt/display split will look to see if that influences here somehow.

Regards,

Tvrtko

> While at it, use the new macro to call the old ones to clean the code a
> bit.
> 
> v2: Rename macros for no-forcewake function assignment (Tvrtko)
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c          | 172 ++++++++-----------
>   drivers/gpu/drm/i915/intel_uncore.h          |   5 +
>   drivers/gpu/drm/i915/selftests/mock_uncore.c |   4 +-
>   3 files changed, 75 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index da33aa672c3d..8e5716bc53e2 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -901,6 +901,12 @@ static bool is_gen##x##_shadowed(u32 offset) \
>   __is_genX_shadowed(8)
>   __is_genX_shadowed(11)
>   
> +static enum forcewake_domains
> +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; \
> @@ -1145,26 +1151,23 @@ func##_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { \
>   	val = __raw_uncore_read##x(uncore, reg); \
>   	GEN6_READ_FOOTER; \
>   }
> -#define __gen6_read(x) __gen_read(gen6, x)
> -#define __fwtable_read(x) __gen_read(fwtable, x)
> -#define __gen11_fwtable_read(x) __gen_read(gen11_fwtable, x)
> -
> -__gen11_fwtable_read(8)
> -__gen11_fwtable_read(16)
> -__gen11_fwtable_read(32)
> -__gen11_fwtable_read(64)
> -__fwtable_read(8)
> -__fwtable_read(16)
> -__fwtable_read(32)
> -__fwtable_read(64)
> -__gen6_read(8)
> -__gen6_read(16)
> -__gen6_read(32)
> -__gen6_read(64)
> -
> -#undef __gen11_fwtable_read
> -#undef __fwtable_read
> -#undef __gen6_read
> +
> +#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)
> +
> +__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
>   #undef GEN6_READ_HEADER
>   
> @@ -1225,6 +1228,9 @@ gen6_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace)
>   	__raw_uncore_write##x(uncore, reg, val); \
>   	GEN6_WRITE_FOOTER; \
>   }
> +__gen6_write(8)
> +__gen6_write(16)
> +__gen6_write(32)
>   
>   #define __gen_write(func, x) \
>   static void \
> @@ -1237,38 +1243,33 @@ func##_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trac
>   	__raw_uncore_write##x(uncore, reg, val); \
>   	GEN6_WRITE_FOOTER; \
>   }
> -#define __gen8_write(x) __gen_write(gen8, x)
> -#define __fwtable_write(x) __gen_write(fwtable, x)
> -#define __gen11_fwtable_write(x) __gen_write(gen11_fwtable, x)
> -
> -__gen11_fwtable_write(8)
> -__gen11_fwtable_write(16)
> -__gen11_fwtable_write(32)
> -__fwtable_write(8)
> -__fwtable_write(16)
> -__fwtable_write(32)
> -__gen8_write(8)
> -__gen8_write(16)
> -__gen8_write(32)
> -__gen6_write(8)
> -__gen6_write(16)
> -__gen6_write(32)
>   
> -#undef __gen11_fwtable_write
> -#undef __fwtable_write
> -#undef __gen8_write
> -#undef __gen6_write
> +#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)
> +
> +__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
>   #undef GEN6_WRITE_HEADER
>   
> -#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
> +#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_READ_MMIO_VFUNCS(uncore, x) \
> +#define ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, x) \
>   do { \
>   	(uncore)->funcs.mmio_readb = x##_read8; \
>   	(uncore)->funcs.mmio_readw = x##_read16; \
> @@ -1276,6 +1277,17 @@ do { \
>   	(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)
>   
>   static void fw_domain_init(struct intel_uncore *uncore,
>   			   enum forcewake_domain_id domain_id,
> @@ -1559,11 +1571,11 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   
>   	if (!intel_uncore_has_forcewake(uncore)) {
>   		if (IS_GEN(i915, 5)) {
> -			ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
> -			ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
> +			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
> +			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
>   		} else {
> -			ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
> -			ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
> +			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
> +			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
>   		}
>   	} else if (IS_GEN_RANGE(i915, 6, 7)) {
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
> @@ -1594,6 +1606,12 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	}
>   
> +	/* 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->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;
>   
> @@ -1871,62 +1889,6 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
>   	return ret;
>   }
>   
> -static enum forcewake_domains
> -intel_uncore_forcewake_for_read(struct intel_uncore *uncore,
> -				i915_reg_t reg)
> -{
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> -	u32 offset = i915_mmio_reg_offset(reg);
> -	enum forcewake_domains fw_domains;
> -
> -	if (INTEL_GEN(i915) >= 11) {
> -		fw_domains = __gen11_fwtable_reg_read_fw_domains(uncore, offset);
> -	} else if (HAS_FWTABLE(i915)) {
> -		fw_domains = __fwtable_reg_read_fw_domains(uncore, offset);
> -	} else if (INTEL_GEN(i915) >= 6) {
> -		fw_domains = __gen6_reg_read_fw_domains(uncore, offset);
> -	} else {
> -		/* on devices with FW we expect to hit one of the above cases */
> -		if (intel_uncore_has_forcewake(uncore))
> -			MISSING_CASE(INTEL_GEN(i915));
> -
> -		fw_domains = 0;
> -	}
> -
> -	WARN_ON(fw_domains & ~uncore->fw_domains);
> -
> -	return fw_domains;
> -}
> -
> -static enum forcewake_domains
> -intel_uncore_forcewake_for_write(struct intel_uncore *uncore,
> -				 i915_reg_t reg)
> -{
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> -	u32 offset = i915_mmio_reg_offset(reg);
> -	enum forcewake_domains fw_domains;
> -
> -	if (INTEL_GEN(i915) >= 11) {
> -		fw_domains = __gen11_fwtable_reg_write_fw_domains(uncore, offset);
> -	} else if (HAS_FWTABLE(i915) && !IS_VALLEYVIEW(i915)) {
> -		fw_domains = __fwtable_reg_write_fw_domains(uncore, offset);
> -	} else if (IS_GEN(i915, 8)) {
> -		fw_domains = __gen8_reg_write_fw_domains(uncore, offset);
> -	} else if (IS_GEN_RANGE(i915, 6, 7)) {
> -		fw_domains = FORCEWAKE_RENDER;
> -	} else {
> -		/* on devices with FW we expect to hit one of the above cases */
> -		if (intel_uncore_has_forcewake(uncore))
> -			MISSING_CASE(INTEL_GEN(i915));
> -
> -		fw_domains = 0;
> -	}
> -
> -	WARN_ON(fw_domains & ~uncore->fw_domains);
> -
> -	return fw_domains;
> -}
> -
>   /**
>    * intel_uncore_forcewake_for_reg - which forcewake domains are needed to access
>    * 				    a register
> @@ -1953,10 +1915,12 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
>   		return 0;
>   
>   	if (op & FW_REG_READ)
> -		fw_domains = intel_uncore_forcewake_for_read(uncore, reg);
> +		fw_domains = uncore->funcs.read_fw_domains(uncore, reg);
>   
>   	if (op & FW_REG_WRITE)
> -		fw_domains |= intel_uncore_forcewake_for_write(uncore, reg);
> +		fw_domains |= uncore->funcs.write_fw_domains(uncore, reg);
> +
> +	WARN_ON(fw_domains & ~uncore->fw_domains);
>   
>   	return fw_domains;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 804a0faacc91..4afde0c44ffe 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -70,6 +70,11 @@ struct intel_uncore_funcs {
>   	void (*force_wake_put)(struct intel_uncore *uncore,
>   			       enum forcewake_domains domains);
>   
> +	enum forcewake_domains (*read_fw_domains)(struct intel_uncore *uncore,
> +						  i915_reg_t r);
> +	enum forcewake_domains (*write_fw_domains)(struct intel_uncore *uncore,
> +						   i915_reg_t r);
> +
>   	u8 (*mmio_readb)(struct intel_uncore *uncore,
>   			 i915_reg_t r, bool trace);
>   	u16 (*mmio_readw)(struct intel_uncore *uncore,
> diff --git a/drivers/gpu/drm/i915/selftests/mock_uncore.c b/drivers/gpu/drm/i915/selftests/mock_uncore.c
> index ff8999c63a12..49585f16d4a2 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_uncore.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_uncore.c
> @@ -41,6 +41,6 @@ __nop_read(64)
>   
>   void mock_uncore_init(struct intel_uncore *uncore)
>   {
> -	ASSIGN_WRITE_MMIO_VFUNCS(uncore, nop);
> -	ASSIGN_READ_MMIO_VFUNCS(uncore, nop);
> +	ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, nop);
> +	ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, nop);
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: kill uncore_to_i915

[Tvrtko Ursulin]

On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
> Let's get rid of it before it proliferates, since with split GT/Display
> uncores the container_of won't work anymore.
> 
> I've kept the rpm pointer as well to minimize the pointer chasing in the
> MMIO accessors.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c     |  2 +-
>   drivers/gpu/drm/i915/i915_drv.h     |  5 -----
>   drivers/gpu/drm/i915/intel_uncore.c | 24 ++++++++++++------------
>   drivers/gpu/drm/i915/intel_uncore.h |  4 +++-
>   4 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6cf8f1838cec..d113296cbe34 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -894,7 +894,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
>   
>   	intel_device_info_subplatform_init(dev_priv);
>   
> -	intel_uncore_init_early(&dev_priv->uncore);
> +	intel_uncore_init_early(dev_priv, &dev_priv->uncore);

Would swapped parameter order make more sense?

intel_uncore_init_early -> "I am operating on uncore" -> pass in uncore 
first, and dev_priv as additional information second. I think that would 
be better. With that:

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

Regards,

Tvrtko

>   
>   	spin_lock_init(&dev_priv->irq_lock);
>   	spin_lock_init(&dev_priv->gpu_error.lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a9c2392cc7c..3b42588b7194 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1949,11 +1949,6 @@ static inline struct drm_i915_private *huc_to_i915(struct intel_huc *huc)
>   	return container_of(huc, struct drm_i915_private, huc);
>   }
>   
> -static inline struct drm_i915_private *uncore_to_i915(struct intel_uncore *uncore)
> -{
> -	return container_of(uncore, struct drm_i915_private, uncore);
> -}
> -
>   /* Simple iterator over all initialised engines */
>   #define for_each_engine(engine__, dev_priv__, id__) \
>   	for ((id__) = 0; \
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 63bdadacadcc..88a69bf713c9 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -322,7 +322,7 @@ static void __gen6_gt_wait_for_fifo(struct intel_uncore *uncore)
>   
>   	/* On VLV, FIFO will be shared by both SW and HW.
>   	 * So, we need to read the FREE_ENTRIES everytime */
> -	if (IS_VALLEYVIEW(uncore_to_i915(uncore)))
> +	if (IS_VALLEYVIEW(uncore->i915))
>   		n = fifo_free_entries(uncore);
>   	else
>   		n = uncore->fifo_count;
> @@ -493,7 +493,7 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
>   		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>   
>   	/* WaDisableShadowRegForCpd:chv */
> -	if (IS_CHERRYVIEW(uncore_to_i915(uncore))) {
> +	if (IS_CHERRYVIEW(uncore->i915)) {
>   		__raw_uncore_write32(uncore, GTFIFOCTL,
>   				     __raw_uncore_read32(uncore, GTFIFOCTL) |
>   				     GT_FIFO_CTL_BLOCK_ALL_POLICY_STALL |
> @@ -622,7 +622,7 @@ void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
>   	spin_lock_irq(&uncore->lock);
>   	if (!--uncore->user_forcewake.count) {
>   		if (intel_uncore_unclaimed_mmio(uncore))
> -			dev_info(uncore_to_i915(uncore)->drm.dev,
> +			dev_info(uncore->i915->drm.dev,
>   				 "Invalid mmio detected during user access\n");
>   
>   		uncore->unclaimed_mmio_check =
> @@ -1346,7 +1346,7 @@ static void fw_domain_fini(struct intel_uncore *uncore,
>   
>   static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>   {
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> +	struct drm_i915_private *i915 = uncore->i915;
>   
>   	if (!intel_uncore_has_forcewake(uncore))
>   		return;
> @@ -1499,7 +1499,7 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
>   
>   static int uncore_mmio_setup(struct intel_uncore *uncore)
>   {
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> +	struct drm_i915_private *i915 = uncore->i915;
>   	struct pci_dev *pdev = i915->drm.pdev;
>   	int mmio_bar;
>   	int mmio_size;
> @@ -1529,20 +1529,22 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)
>   
>   static void uncore_mmio_cleanup(struct intel_uncore *uncore)
>   {
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> -	struct pci_dev *pdev = i915->drm.pdev;
> +	struct pci_dev *pdev = uncore->i915->drm.pdev;
>   
>   	pci_iounmap(pdev, uncore->regs);
>   }
>   
> -void intel_uncore_init_early(struct intel_uncore *uncore)
> +void intel_uncore_init_early(struct drm_i915_private *i915,
> +			     struct intel_uncore *uncore)
>   {
>   	spin_lock_init(&uncore->lock);
> +	uncore->i915 = i915;
> +	uncore->rpm = &i915->runtime_pm;
>   }
>   
>   int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   {
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> +	struct drm_i915_private *i915 = uncore->i915;
>   	int ret;
>   
>   	ret = uncore_mmio_setup(uncore);
> @@ -1561,8 +1563,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   	uncore->pmic_bus_access_nb.notifier_call =
>   		i915_pmic_bus_access_notifier;
>   
> -	uncore->rpm = &i915->runtime_pm;
> -
>   	if (!intel_uncore_has_forcewake(uncore)) {
>   		if (IS_GEN(i915, 5)) {
>   			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
> @@ -1627,7 +1627,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>    */
>   void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore)
>   {
> -	struct drm_i915_private *i915 = uncore_to_i915(uncore);
> +	struct drm_i915_private *i915 = uncore->i915;
>   
>   	if (INTEL_GEN(i915) >= 11) {
>   		enum forcewake_domains fw_domains = uncore->fw_domains;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 94c00d3778b1..912616188ff5 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -102,6 +102,7 @@ struct intel_forcewake_range {
>   struct intel_uncore {
>   	void __iomem *regs;
>   
> +	struct drm_i915_private *i915;
>   	struct intel_runtime_pm *rpm;
>   
>   	spinlock_t lock; /** lock is also taken in irq contexts. */
> @@ -182,7 +183,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>   	return uncore->flags & UNCORE_HAS_FIFO;
>   }
>   
> -void intel_uncore_init_early(struct intel_uncore *uncore);
> +void intel_uncore_init_early(struct drm_i915_private *i915,
> +			     struct intel_uncore *uncore);
>   int intel_uncore_init_mmio(struct intel_uncore *uncore);
>   void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore);
>   bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Tvrtko Ursulin]

On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
> We always call some of the setup/cleanup functions for forcewake, even
> if the feature is not actually available. Skipping these operations if
> forcewake is not available saves us some operations on older gens and
> prepares us for having a forcewake-less display uncore.
> The suspend/resume functions have also been renamed to clearly indicate
> that they only operate on forcewake status.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c     |  15 +--
>   drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
>   drivers/gpu/drm/i915/intel_uncore.h |   8 +-
>   3 files changed, 101 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d113296cbe34..95b36fe17f99 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
>   
>   	intel_device_info_init_mmio(dev_priv);
>   
> -	intel_uncore_prune_mmio_domains(&dev_priv->uncore);
> +	intel_uncore_prune_forcewake_domains(&dev_priv->uncore);
>   
>   	intel_uc_init_mmio(dev_priv);
>   
> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>   
>   	i915_gem_suspend_late(dev_priv);
>   
> -	intel_uncore_suspend(&dev_priv->uncore);
> +	intel_uncore_forcewake_suspend(&dev_priv->uncore);
>   
>   	intel_power_domains_suspend(dev_priv,
>   				    get_suspend_mode(dev_priv, hibernation));
> @@ -2348,7 +2348,10 @@ static int i915_drm_resume_early(struct drm_device *dev)
>   		DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
>   			  ret);
>   
> -	intel_uncore_resume_early(&dev_priv->uncore);
> +	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> +		DRM_DEBUG("unclaimed mmio detected on resume, clearing\n");
> +

Why does this bit needs to be pulled up to this level? My first line of 
thinking is that we should aim to keep the component specific steps 
down, if possible.

> +	intel_uncore_forcewake_resume_early(&dev_priv->uncore);
>   
>   	i915_check_and_clear_faults(dev_priv);
>   
> @@ -2923,7 +2926,7 @@ static int intel_runtime_suspend(struct device *kdev)
>   
>   	intel_runtime_pm_disable_interrupts(dev_priv);
>   
> -	intel_uncore_suspend(&dev_priv->uncore);
> +	intel_uncore_forcewake_suspend(&dev_priv->uncore);
>   
>   	ret = 0;
>   	if (INTEL_GEN(dev_priv) >= 11) {
> @@ -2940,7 +2943,7 @@ static int intel_runtime_suspend(struct device *kdev)
>   
>   	if (ret) {
>   		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> -		intel_uncore_runtime_resume(&dev_priv->uncore);
> +		intel_uncore_forcewake_runtime_resume(&dev_priv->uncore);
>   
>   		intel_runtime_pm_enable_interrupts(dev_priv);
>   
> @@ -3038,7 +3041,7 @@ static int intel_runtime_resume(struct device *kdev)
>   		ret = vlv_resume_prepare(dev_priv, true);
>   	}
>   
> -	intel_uncore_runtime_resume(&dev_priv->uncore);
> +	intel_uncore_forcewake_runtime_resume(&dev_priv->uncore);
>   
>   	intel_runtime_pm_enable_interrupts(dev_priv);
>   
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 88a69bf713c9..c0f5567ee096 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -485,12 +485,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore)
>   	return ret;
>   }
>   
> -static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
> +static void forcewake_early_sanitize(struct intel_uncore *uncore,
>   					  unsigned int restore_forcewake)
>   {
> -	/* clear out unclaimed reg detection bit */
> -	if (check_for_unclaimed_mmio(uncore))
> -		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
> +	if (!intel_uncore_has_forcewake(uncore))
> +		return;
>   
>   	/* WaDisableShadowRegForCpd:chv */
>   	if (IS_CHERRYVIEW(uncore->i915)) {
> @@ -513,8 +512,11 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
>   	iosf_mbi_punit_release();
>   }
>   
> -void intel_uncore_suspend(struct intel_uncore *uncore)
> +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore)
>   {
> +	if (!intel_uncore_has_forcewake(uncore))
> +		return;
> +
>   	iosf_mbi_punit_acquire();
>   	iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>   		&uncore->pmic_bus_access_nb);
> @@ -522,18 +524,24 @@ void intel_uncore_suspend(struct intel_uncore *uncore)
>   	iosf_mbi_punit_release();
>   }
>   
> -void intel_uncore_resume_early(struct intel_uncore *uncore)
> +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore)
>   {
>   	unsigned int restore_forcewake;
>   
> +	if (!intel_uncore_has_forcewake(uncore))
> +		return;
> +
>   	restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved);
> -	__intel_uncore_early_sanitize(uncore, restore_forcewake);
> +	forcewake_early_sanitize(uncore, restore_forcewake);

This call already handles !has_forcewake, so function handles it twice 
in source. Is this what you intended? Maybe just add double-underscore 
version for early sanitize without the check but GEM_BUG_ON?

>   
>   	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>   }
>   
> -void intel_uncore_runtime_resume(struct intel_uncore *uncore)
> +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore)
>   {
> +	if (!intel_uncore_has_forcewake(uncore))
> +		return;
> +
>   	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>   }
>   
> @@ -1348,8 +1356,7 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>   {
>   	struct drm_i915_private *i915 = uncore->i915;
>   
> -	if (!intel_uncore_has_forcewake(uncore))
> -		return;
> +	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>   
>   	if (INTEL_GEN(i915) >= 11) {
>   		int i;
> @@ -1542,36 +1549,29 @@ void intel_uncore_init_early(struct drm_i915_private *i915,
>   	uncore->rpm = &i915->runtime_pm;
>   }
>   
> -int intel_uncore_init_mmio(struct intel_uncore *uncore)
> +static void uncore_raw_init(struct intel_uncore *uncore)
>   {
> -	struct drm_i915_private *i915 = uncore->i915;
> -	int ret;
> +	GEM_BUG_ON(intel_uncore_has_forcewake(uncore));
>   
> -	ret = uncore_mmio_setup(uncore);
> -	if (ret)
> -		return ret;
> +	if (IS_GEN(uncore->i915, 5)) {
> +		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
> +		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
> +	} else {
> +		ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
> +		ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
> +	}
> +}
>   
> -	i915_check_vgpu(i915);
> +static void uncore_forcewake_init(struct intel_uncore *uncore)
> +{
> +	struct drm_i915_private *i915 = uncore->i915;
>   
> -	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
> -		uncore->flags |= UNCORE_HAS_FORCEWAKE;
> +	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>   
>   	intel_uncore_fw_domains_init(uncore);
> -	__intel_uncore_early_sanitize(uncore, 0);
> +	forcewake_early_sanitize(uncore, 0);
>   
> -	uncore->unclaimed_mmio_check = 1;
> -	uncore->pmic_bus_access_nb.notifier_call =
> -		i915_pmic_bus_access_notifier;
> -
> -	if (!intel_uncore_has_forcewake(uncore)) {
> -		if (IS_GEN(i915, 5)) {
> -			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
> -			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
> -		} else {
> -			ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
> -			ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
> -		}
> -	} else if (IS_GEN_RANGE(i915, 6, 7)) {
> +	if (IS_GEN_RANGE(i915, 6, 7)) {
>   		ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
>   
>   		if (IS_VALLEYVIEW(i915)) {
> @@ -1585,7 +1585,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   			ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
>   			ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>   			ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
> -
>   		} else {
>   			ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
>   			ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
> @@ -1600,6 +1599,31 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   		ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>   	}
>   
> +	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
> +	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
> +}
> +
> +int intel_uncore_init_mmio(struct intel_uncore *uncore)
> +{
> +	struct drm_i915_private *i915 = uncore->i915;
> +	int ret;
> +
> +	ret = uncore_mmio_setup(uncore);
> +	if (ret)
> +		return ret;
> +
> +	i915_check_vgpu(i915);
> +
> +	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
> +		uncore->flags |= UNCORE_HAS_FORCEWAKE;
> +
> +	uncore->unclaimed_mmio_check = 1;
> +
> +	if (!intel_uncore_has_forcewake(uncore))
> +		uncore_raw_init(uncore);

Is any of the remaining code in this function relevant after this branch 
has been taken? If not this could be changed to:

   if (!intel_uncore_has_forcewake(uncore)) {
	uncore_raw_init(uncore);
	return;
   }

   uncore_forcewake_init(uncore);
   ...

Hm, also is "unclaimed_mmio_check = 1;" above possible/relevant where no 
forcwake? Doesn't look like it. Unless vgpu?

> +	else
> +		uncore_forcewake_init(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);
> @@ -1615,7 +1639,9 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   	if (IS_GEN_RANGE(i915, 6, 7))
>   		uncore->flags |= UNCORE_HAS_FIFO;
>   
> -	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
> +	/* clear out unclaimed reg detection bit */
> +	if (check_for_unclaimed_mmio(uncore))
> +		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>   
>   	return 0;
>   }
> @@ -1625,44 +1651,47 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>    * the forcewake domains. Prune them, to make sure they only reference existing
>    * engines.
>    */
> -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore)
> +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore)
>   {
>   	struct drm_i915_private *i915 = uncore->i915;
> +	enum forcewake_domains fw_domains = uncore->fw_domains;
> +	enum forcewake_domain_id domain_id;
> +	int i;
>   
> -	if (INTEL_GEN(i915) >= 11) {
> -		enum forcewake_domains fw_domains = uncore->fw_domains;
> -		enum forcewake_domain_id domain_id;
> -		int i;
> +	if (!intel_uncore_has_forcewake(uncore) || INTEL_GEN(i915) < 11)
> +		return;
>   
> -		for (i = 0; i < I915_MAX_VCS; i++) {
> -			domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
> +	for (i = 0; i < I915_MAX_VCS; i++) {
> +		domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
>   
> -			if (HAS_ENGINE(i915, _VCS(i)))
> -				continue;
> +		if (HAS_ENGINE(i915, _VCS(i)))
> +			continue;
>   
> -			if (fw_domains & BIT(domain_id))
> -				fw_domain_fini(uncore, domain_id);
> -		}
> +		if (fw_domains & BIT(domain_id))
> +			fw_domain_fini(uncore, domain_id);
> +	}
>   
> -		for (i = 0; i < I915_MAX_VECS; i++) {
> -			domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
> +	for (i = 0; i < I915_MAX_VECS; i++) {
> +		domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
>   
> -			if (HAS_ENGINE(i915, _VECS(i)))
> -				continue;
> +		if (HAS_ENGINE(i915, _VECS(i)))
> +			continue;
>   
> -			if (fw_domains & BIT(domain_id))
> -				fw_domain_fini(uncore, domain_id);
> -		}
> +		if (fw_domains & BIT(domain_id))
> +			fw_domain_fini(uncore, domain_id);
>   	}
>   }
>   
>   void intel_uncore_fini_mmio(struct intel_uncore *uncore)
>   {
> -	iosf_mbi_punit_acquire();
> -	iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> -		&uncore->pmic_bus_access_nb);
> -	intel_uncore_forcewake_reset(uncore);
> -	iosf_mbi_punit_release();
> +	if (intel_uncore_has_forcewake(uncore)) {

To avoid hyphotetical obnoxious diffs in the future, like the one for 
intel_uncore_prune_mmio_domains above in this patch, maybe invert this 
to early return straight away.

> +		iosf_mbi_punit_acquire();
> +		iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> +			&uncore->pmic_bus_access_nb);
> +		intel_uncore_forcewake_reset(uncore);
> +		iosf_mbi_punit_release();
> +	}
> +
>   	uncore_mmio_cleanup(uncore);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 912616188ff5..879252735bba 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -186,13 +186,13 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>   void intel_uncore_init_early(struct drm_i915_private *i915,
>   			     struct intel_uncore *uncore);
>   int intel_uncore_init_mmio(struct intel_uncore *uncore);
> -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore);
> +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore);
>   bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore);
>   bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore);
>   void intel_uncore_fini_mmio(struct intel_uncore *uncore);
> -void intel_uncore_suspend(struct intel_uncore *uncore);
> -void intel_uncore_resume_early(struct intel_uncore *uncore);
> -void intel_uncore_runtime_resume(struct intel_uncore *uncore);
> +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore);
> +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore);
> +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore);
>   
>   void assert_forcewakes_inactive(struct intel_uncore *uncore);
>   void assert_forcewakes_active(struct intel_uncore *uncore,
> 

Regards,

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

✓ Fi.CI.IGT: success for Display uncore prep patches

[Patchwork]

== Series Details ==

Series: Display uncore prep patches
URL   : https://patchwork.freedesktop.org/series/62232/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6287_full -> Patchwork_13312_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_13312_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-10ms:
    - shard-kbl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#110913 ]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-kbl3/igt@gem_eio@in-flight-10ms.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-kbl3/igt@gem_eio@in-flight-10ms.html

  * igt@gem_eio@in-flight-internal-immediate:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#110913 ]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-apl1/igt@gem_eio@in-flight-internal-immediate.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-apl8/igt@gem_eio@in-flight-internal-immediate.html

  * igt@gem_eio@in-flight-suspend:
    - shard-skl:          [PASS][5] -> [FAIL][6] ([fdo#110667])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-skl2/igt@gem_eio@in-flight-suspend.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-skl5/igt@gem_eio@in-flight-suspend.html
    - shard-glk:          [PASS][7] -> [FAIL][8] ([fdo#110667])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-glk5/igt@gem_eio@in-flight-suspend.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-glk2/igt@gem_eio@in-flight-suspend.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-snb:          [PASS][9] -> [DMESG-WARN][10] ([fdo#110913 ]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-snb2/igt@gem_userptr_blits@sync-unmap-cycles.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-snb1/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@i915_suspend@debugfs-reader:
    - shard-kbl:          [PASS][11] -> [DMESG-WARN][12] ([fdo#108566])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-kbl1/igt@i915_suspend@debugfs-reader.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-kbl6/igt@i915_suspend@debugfs-reader.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-move:
    - shard-hsw:          [PASS][13] -> [SKIP][14] ([fdo#109271]) +21 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-hsw5/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-move.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-move.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite:
    - shard-iclb:         [PASS][15] -> [FAIL][16] ([fdo#103167]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-skl:          [PASS][17] -> [INCOMPLETE][18] ([fdo#104108])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-skl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-skl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-iclb2/igt@kms_psr@psr2_cursor_render.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-iclb1/igt@kms_psr@psr2_cursor_render.html

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-glk:          [PASS][21] -> [DMESG-FAIL][22] ([fdo#105763] / [fdo#106538])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-glk4/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-glk6/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html

  * igt@kms_vblank@pipe-b-ts-continuation-suspend:
    - shard-apl:          [PASS][23] -> [DMESG-WARN][24] ([fdo#108566]) +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-apl1/igt@kms_vblank@pipe-b-ts-continuation-suspend.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-apl4/igt@kms_vblank@pipe-b-ts-continuation-suspend.html

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@hang:
    - shard-kbl:          [DMESG-WARN][25] ([fdo#110913 ]) -> [PASS][26] +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-kbl7/igt@gem_mmap_gtt@hang.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-kbl1/igt@gem_mmap_gtt@hang.html
    - shard-snb:          [INCOMPLETE][27] ([fdo#105411]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-snb6/igt@gem_mmap_gtt@hang.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-snb4/igt@gem_mmap_gtt@hang.html

  * igt@gem_persistent_relocs@forked-faulting-reloc-thrashing:
    - shard-snb:          [DMESG-WARN][29] ([fdo#110789] / [fdo#110913 ]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-snb7/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-snb6/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
    - shard-snb:          [DMESG-WARN][31] ([fdo#110913 ]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-snb4/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-snb4/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup:
    - shard-apl:          [DMESG-WARN][33] ([fdo#110913 ]) -> [PASS][34] +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-apl8/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-apl2/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding:
    - shard-skl:          [FAIL][35] ([fdo#103232]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-skl3/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-kbl:          [INCOMPLETE][37] ([fdo#103665]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-kbl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-kbl1/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled:
    - shard-skl:          [FAIL][39] ([fdo#103184] / [fdo#103232]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-skl3/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-skl8/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled.html

  * igt@kms_flip@2x-plain-flip:
    - shard-hsw:          [SKIP][41] ([fdo#109271]) -> [PASS][42] +36 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-hsw1/igt@kms_flip@2x-plain-flip.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-hsw8/igt@kms_flip@2x-plain-flip.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-kbl:          [DMESG-WARN][43] ([fdo#108566]) -> [PASS][44] +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-kbl4/igt@kms_flip@flip-vs-suspend.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-kbl1/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt:
    - shard-iclb:         [FAIL][45] ([fdo#103167]) -> [PASS][46] +7 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-apl:          [DMESG-WARN][47] ([fdo#108566]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-apl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-apl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][49] ([fdo#108145]) -> [PASS][50] +1 similar issue
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-skl3/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_sprite_mmap_gtt:
    - shard-iclb:         [SKIP][51] ([fdo#109441]) -> [PASS][52] +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-iclb8/igt@kms_psr@psr2_sprite_mmap_gtt.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_gtt.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [FAIL][53] ([fdo#100047]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-iclb3/igt@kms_sysfs_edid_timing.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-iclb8/igt@kms_sysfs_edid_timing.html

  
#### Warnings ####

  * igt@i915_pm_rpm@modeset-pc8-residency-stress:
    - shard-iclb:         [INCOMPLETE][55] ([fdo#107713] / [fdo#108840]) -> [SKIP][56] ([fdo#109293])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-iclb7/igt@i915_pm_rpm@modeset-pc8-residency-stress.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-iclb4/igt@i915_pm_rpm@modeset-pc8-residency-stress.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt:
    - shard-skl:          [FAIL][57] ([fdo#108040]) -> [FAIL][58] ([fdo#103167])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6287/shard-skl4/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/shard-skl8/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt.html

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109293]: https://bugs.freedesktop.org/show_bug.cgi?id=109293
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110667]: https://bugs.freedesktop.org/show_bug.cgi?id=110667
  [fdo#110789]: https://bugs.freedesktop.org/show_bug.cgi?id=110789
  [fdo#110913 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110913 


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6287 -> Patchwork_13312

  CI_DRM_6287: 3765c2bb2bf60f35709fba4c23070e2b74e14247 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5059: 1f67ee0d09d6513f487f2be74aae9700e755258a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13312: 87b8974899198d3c29c94dcc24dcc9e2b7d5ebbb @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13312/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Tvrtko Ursulin]

On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
> We'd like to introduce a display uncore with no forcewake domains, so
> let's avoid wasting memory and be ready to allocate only what we need.
> Even without multiple uncore, we still don't need all the domains on all
> gens.

Looks good in principle, only a few conversation points below.

> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 155 ++++++++++++++++++----------
>   drivers/gpu/drm/i915/intel_uncore.h |  13 +--
>   2 files changed, 102 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c0f5567ee096..7f311827c3ef 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -344,7 +344,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>   {
>   	struct intel_uncore_forcewake_domain *domain =
>   	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
> -	struct intel_uncore *uncore = forcewake_domain_to_uncore(domain);
> +	struct intel_uncore *uncore = domain->uncore;
>   	unsigned long irqflags;
>   
>   	assert_rpm_device_not_suspended(uncore->rpm);
> @@ -1291,23 +1291,23 @@ do { \
>   	(uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
>   } while (0)
>   
> -static void fw_domain_init(struct intel_uncore *uncore,
> +static int fw_domain_init(struct intel_uncore *uncore,
>   			   enum forcewake_domain_id domain_id,
>   			   i915_reg_t reg_set,
>   			   i915_reg_t reg_ack)
>   {
>   	struct intel_uncore_forcewake_domain *d;
>   
> -	if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
> -		return;
> -
> -	d = &uncore->fw_domain[domain_id];
> +	GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
>   
> -	WARN_ON(d->wake_count);

I wonder what was this for.

Do you want to add some protection against double init of the same domain?

> +	d = kzalloc(sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
>   
>   	WARN_ON(!i915_mmio_reg_valid(reg_set));
>   	WARN_ON(!i915_mmio_reg_valid(reg_ack));
>   
> +	d->uncore = uncore;
>   	d->wake_count = 0;
>   	d->reg_set = uncore->regs + i915_mmio_reg_offset(reg_set);
>   	d->reg_ack = uncore->regs + i915_mmio_reg_offset(reg_ack);
> @@ -1324,7 +1324,6 @@ static void fw_domain_init(struct intel_uncore *uncore,
>   	BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX0 != (1 << FW_DOMAIN_ID_MEDIA_VEBOX0));
>   	BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX1 != (1 << FW_DOMAIN_ID_MEDIA_VEBOX1));
>   
> -
>   	d->mask = BIT(domain_id);
>   
>   	hrtimer_init(&d->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> @@ -1333,6 +1332,10 @@ static void fw_domain_init(struct intel_uncore *uncore,
>   	uncore->fw_domains |= BIT(domain_id);
>   
>   	fw_domain_reset(d);
> +
> +	uncore->fw_domain[domain_id] = d;
> +
> +	return 0;
>   }
>   
>   static void fw_domain_fini(struct intel_uncore *uncore,
> @@ -1340,77 +1343,92 @@ static void fw_domain_fini(struct intel_uncore *uncore,
>   {
>   	struct intel_uncore_forcewake_domain *d;
>   
> -	if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
> -		return;
> +	GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
>   
> -	d = &uncore->fw_domain[domain_id];
> +	d = fetch_and_zero(&uncore->fw_domain[domain_id]);

Maybe early if !d here and function flow just carries on?

> +	uncore->fw_domains &= ~BIT(domain_id);
>   
> -	WARN_ON(d->wake_count);
> -	WARN_ON(hrtimer_cancel(&d->timer));
> -	memset(d, 0, sizeof(*d));
> +	if (d) {
> +		WARN_ON(d->wake_count);
> +		WARN_ON(hrtimer_cancel(&d->timer));
> +		kfree(d);
> +	}
> +}
>   
> -	uncore->fw_domains &= ~BIT(domain_id);
> +static void intel_uncore_fw_domains_fini(struct intel_uncore *uncore)
> +{
> +	struct intel_uncore_forcewake_domain *d;
> +	int tmp;
> +
> +	for_each_fw_domain(d, uncore, tmp)
> +		fw_domain_fini(uncore, d->id);
>   }
>   
> -static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> +static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>   {
>   	struct drm_i915_private *i915 = uncore->i915;
> +	int ret;
>   
>   	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>   
> +#define __fw_domain_init(id__, set__, ack__) \
> +	ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
> +	if (ret) \
> +		goto out_clean;

Hidden control flow is slightly objectionable but I don't offer any nice 
alternatives so I guess will have to pass. Or maybe accumulate the error 
(err |= fw_domain_init(..)) as you go and then cleanup at the end if any 
failed?

On the other hand the idea slightly conflicts with my other suggestion 
to rename existing fw_domain_init to __fw_domain_init and call the macro 
fw_domain_init and avoid all the churn below.

> +
>   	if (INTEL_GEN(i915) >= 11) {
>   		int i;
>   
> -		uncore->funcs.force_wake_get =
> -			fw_domains_get_with_fallback;
> +		uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
>   		uncore->funcs.force_wake_put = fw_domains_put;
> -		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> -			       FORCEWAKE_RENDER_GEN9,
> -			       FORCEWAKE_ACK_RENDER_GEN9);
> -		fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
> -			       FORCEWAKE_BLITTER_GEN9,
> -			       FORCEWAKE_ACK_BLITTER_GEN9);
> +		__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +				 FORCEWAKE_RENDER_GEN9,
> +				 FORCEWAKE_ACK_RENDER_GEN9);
> +		__fw_domain_init(FW_DOMAIN_ID_BLITTER,
> +				 FORCEWAKE_BLITTER_GEN9,
> +				 FORCEWAKE_ACK_BLITTER_GEN9);
> +
>   		for (i = 0; i < I915_MAX_VCS; i++) {
>   			if (!HAS_ENGINE(i915, _VCS(i)))
>   				continue;
>   
> -			fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
> -				       FORCEWAKE_MEDIA_VDBOX_GEN11(i),
> -				       FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
> +			__fw_domain_init(FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
> +					 FORCEWAKE_MEDIA_VDBOX_GEN11(i),
> +					 FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
>   		}
>   		for (i = 0; i < I915_MAX_VECS; i++) {
>   			if (!HAS_ENGINE(i915, _VECS(i)))
>   				continue;
>   
> -			fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
> -				       FORCEWAKE_MEDIA_VEBOX_GEN11(i),
> -				       FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
> +			__fw_domain_init(FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
> +					 FORCEWAKE_MEDIA_VEBOX_GEN11(i),
> +					 FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
>   		}
>   	} else if (IS_GEN_RANGE(i915, 9, 10)) {
> -		uncore->funcs.force_wake_get =
> -			fw_domains_get_with_fallback;
> +		uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
>   		uncore->funcs.force_wake_put = fw_domains_put;
> -		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> -			       FORCEWAKE_RENDER_GEN9,
> -			       FORCEWAKE_ACK_RENDER_GEN9);
> -		fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
> -			       FORCEWAKE_BLITTER_GEN9,
> -			       FORCEWAKE_ACK_BLITTER_GEN9);
> -		fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
> -			       FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
> +		__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +				 FORCEWAKE_RENDER_GEN9,
> +				 FORCEWAKE_ACK_RENDER_GEN9);
> +		__fw_domain_init(FW_DOMAIN_ID_BLITTER,
> +				 FORCEWAKE_BLITTER_GEN9,
> +				 FORCEWAKE_ACK_BLITTER_GEN9);
> +		__fw_domain_init(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;
> -		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);
> +		__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +				 FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
> +		__fw_domain_init(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;
> -		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> -			       FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
> +		__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +				 FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
>   	} else if (IS_IVYBRIDGE(i915)) {
>   		u32 ecobus;
>   
> @@ -1437,8 +1455,8 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>   		__raw_uncore_write32(uncore, FORCEWAKE, 0);
>   		__raw_posting_read(uncore, ECOBUS);
>   
> -		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> -			       FORCEWAKE_MT, FORCEWAKE_MT_ACK);
> +		__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +				 FORCEWAKE_MT, FORCEWAKE_MT_ACK);
>   
>   		spin_lock_irq(&uncore->lock);
>   		fw_domains_get_with_thread_status(uncore, FORCEWAKE_RENDER);
> @@ -1449,19 +1467,27 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>   		if (!(ecobus & FORCEWAKE_MT_ENABLE)) {
>   			DRM_INFO("No MT forcewake available on Ivybridge, this can result in issues\n");
>   			DRM_INFO("when using vblank-synced partial screen updates.\n");
> -			fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> -				       FORCEWAKE, FORCEWAKE_ACK);
> +			__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +					 FORCEWAKE, FORCEWAKE_ACK);
>   		}
>   	} else if (IS_GEN(i915, 6)) {
>   		uncore->funcs.force_wake_get =
>   			fw_domains_get_with_thread_status;
>   		uncore->funcs.force_wake_put = fw_domains_put;
> -		fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> -			       FORCEWAKE, FORCEWAKE_ACK);
> +		__fw_domain_init(FW_DOMAIN_ID_RENDER,
> +				 FORCEWAKE, FORCEWAKE_ACK);
>   	}
>   
> +#undef __fw_domain_init
> +
>   	/* All future platforms are expected to require complex power gating */
>   	WARN_ON(uncore->fw_domains == 0);
> +
> +	return 0;
> +
> +out_clean:
> +	intel_uncore_fw_domains_fini(uncore);
> +	return ret;
>   }
>   
>   #define ASSIGN_FW_DOMAINS_TABLE(uncore, d) \
> @@ -1562,13 +1588,17 @@ static void uncore_raw_init(struct intel_uncore *uncore)
>   	}
>   }
>   
> -static void uncore_forcewake_init(struct intel_uncore *uncore)
> +static int uncore_forcewake_init(struct intel_uncore *uncore)
>   {
>   	struct drm_i915_private *i915 = uncore->i915;
> +	int ret;
>   
>   	GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>   
> -	intel_uncore_fw_domains_init(uncore);
> +	ret = intel_uncore_fw_domains_init(uncore);
> +	if (ret)
> +		return ret;
> +
>   	forcewake_early_sanitize(uncore, 0);
>   
>   	if (IS_GEN_RANGE(i915, 6, 7)) {
> @@ -1601,6 +1631,8 @@ static void uncore_forcewake_init(struct intel_uncore *uncore)
>   
>   	uncore->pmic_bus_access_nb.notifier_call = i915_pmic_bus_access_notifier;
>   	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
> +
> +	return 0;
>   }
>   
>   int intel_uncore_init_mmio(struct intel_uncore *uncore)
> @@ -1619,10 +1651,13 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   
>   	uncore->unclaimed_mmio_check = 1;
>   
> -	if (!intel_uncore_has_forcewake(uncore))
> +	if (!intel_uncore_has_forcewake(uncore)) {
>   		uncore_raw_init(uncore);
> -	else
> -		uncore_forcewake_init(uncore);
> +	} else {
> +		ret = uncore_forcewake_init(uncore);
> +		if (ret)
> +			goto out_mmio_cleanup;
> +	}
>   
>   	/* 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);
> @@ -1644,6 +1679,11 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>   		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>   
>   	return 0;
> +
> +out_mmio_cleanup:
> +	uncore_mmio_cleanup(uncore);
> +
> +	return ret;
>   }
>   
>   /*
> @@ -1689,6 +1729,7 @@ void intel_uncore_fini_mmio(struct intel_uncore *uncore)
>   		iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>   			&uncore->pmic_bus_access_nb);
>   		intel_uncore_forcewake_reset(uncore);
> +		intel_uncore_fw_domains_fini(uncore);
>   		iosf_mbi_punit_release();
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 879252735bba..bbff281b880d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -126,6 +126,7 @@ struct intel_uncore {
>   	enum forcewake_domains fw_domains_saved; /* user domains saved for S3 */
>   
>   	struct intel_uncore_forcewake_domain {
> +		struct intel_uncore *uncore;
>   		enum forcewake_domain_id id;
>   		enum forcewake_domains mask;
>   		unsigned int wake_count;
> @@ -133,7 +134,7 @@ struct intel_uncore {
>   		struct hrtimer timer;
>   		u32 __iomem *reg_set;
>   		u32 __iomem *reg_ack;
> -	} fw_domain[FW_DOMAIN_ID_COUNT];
> +	} *fw_domain[FW_DOMAIN_ID_COUNT];

The array itself could also be made dynamic, no? To much hassle for very 
little gain?

>   
>   	struct {
>   		unsigned int count;
> @@ -147,18 +148,12 @@ struct intel_uncore {
>   
>   /* Iterate over initialised fw domains */
>   #define for_each_fw_domain_masked(domain__, mask__, uncore__, tmp__) \
> -	for (tmp__ = (mask__); \
> -	     tmp__ ? (domain__ = &(uncore__)->fw_domain[__mask_next_bit(tmp__)]), 1 : 0;)
> +	for (tmp__ = (mask__); tmp__ ;) \
> +		for_each_if(domain__ = (uncore__)->fw_domain[__mask_next_bit(tmp__)])
>   
>   #define for_each_fw_domain(domain__, uncore__, tmp__) \
>   	for_each_fw_domain_masked(domain__, (uncore__)->fw_domains, uncore__, tmp__)
>   
> -static inline struct intel_uncore *
> -forcewake_domain_to_uncore(const struct intel_uncore_forcewake_domain *d)
> -{
> -	return container_of(d, struct intel_uncore, fw_domain[d->id]);
> -}
> -
>   static inline bool
>   intel_uncore_has_forcewake(const struct intel_uncore *uncore)
>   {
> 

Regards,

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

Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33)
> We always call some of the setup/cleanup functions for forcewake, even
> if the feature is not actually available. Skipping these operations if
> forcewake is not available saves us some operations on older gens and
> prepares us for having a forcewake-less display uncore.
> The suspend/resume functions have also been renamed to clearly indicate
> that they only operate on forcewake status.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c     |  15 +--
>  drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_uncore.h |   8 +-
>  3 files changed, 101 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d113296cbe34..95b36fe17f99 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
>  
>         intel_device_info_init_mmio(dev_priv);
>  
> -       intel_uncore_prune_mmio_domains(&dev_priv->uncore);
> +       intel_uncore_prune_forcewake_domains(&dev_priv->uncore);

The _prune_ was the exceptional case...

>         intel_uc_init_mmio(dev_priv);
>  
> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  
>         i915_gem_suspend_late(dev_priv);
>  
> -       intel_uncore_suspend(&dev_priv->uncore);
> +       intel_uncore_forcewake_suspend(&dev_priv->uncore);

But are you sure you want to delegate all the fw control to i915_drv.c,
and not keep this as a call into intel_uncore_suspend() ? It is meant to
be telling the uncore functionality that it is time to suspend, and it
has to work out what to save.

I am not sold on this yet. (One day this will just be a bunch of async
tasks plugged into signals with ordering determined by their
self-declared dependency tree. One day.)

So sell me on why we want an uncore_forcewake object, as currently you
are proposing a intel_uncore_suspend_forcewake().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/gvt: decouple check_vgpu() from uncore_init()

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:35)
> With multiple uncore to initialize (GT vs Display), it makes little
> sense to have the vgpu_check inside uncore_init(). We also have
> a catch-22 scenario where the uncore is required to read the vgpu
> capabilities while the vgpu capabilities are required to decide if
> we need to initialize forcewake support. To remove this circular
> dependency, we can perform the required MMIO access by mmapping just
> the vgtif shared page in mmio space and use raw accessors.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c     |  2 ++
>  drivers/gpu/drm/i915/i915_pvinfo.h  |  5 +++--
>  drivers/gpu/drm/i915/i915_vgpu.c    | 31 +++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_uncore.c |  2 --
>  4 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 95b36fe17f99..c3d9d1a366b0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1893,6 +1893,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>         disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>  
> +       i915_check_vgpu(dev_priv);

Bonus points for s/i915_check_vgpu/i915_detect_vgpu/ ?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Daniele Ceraolo Spurio]

On 6/18/19 3:22 AM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33)
>> We always call some of the setup/cleanup functions for forcewake, even
>> if the feature is not actually available. Skipping these operations if
>> forcewake is not available saves us some operations on older gens and
>> prepares us for having a forcewake-less display uncore.
>> The suspend/resume functions have also been renamed to clearly indicate
>> that they only operate on forcewake status.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c     |  15 +--
>>   drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
>>   drivers/gpu/drm/i915/intel_uncore.h |   8 +-
>>   3 files changed, 101 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index d113296cbe34..95b36fe17f99 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
>>   
>>          intel_device_info_init_mmio(dev_priv);
>>   
>> -       intel_uncore_prune_mmio_domains(&dev_priv->uncore);
>> +       intel_uncore_prune_forcewake_domains(&dev_priv->uncore);
> 
> The _prune_ was the exceptional case...
> 
>>          intel_uc_init_mmio(dev_priv);
>>   
>> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>>   
>>          i915_gem_suspend_late(dev_priv);
>>   
>> -       intel_uncore_suspend(&dev_priv->uncore);
>> +       intel_uncore_forcewake_suspend(&dev_priv->uncore);
> 
> But are you sure you want to delegate all the fw control to i915_drv.c,
> and not keep this as a call into intel_uncore_suspend() ? It is meant to
> be telling the uncore functionality that it is time to suspend, and it
> has to work out what to save.
> 
> I am not sold on this yet. (One day this will just be a bunch of async
> tasks plugged into signals with ordering determined by their
> self-declared dependency tree. One day.)
> 
> So sell me on why we want an uncore_forcewake object, as currently you
> are proposing a intel_uncore_suspend_forcewake().
> -Chris
> 

My aim was to make it clear that this call will not be required for 
display_uncore since there is nothing to do on suspend/resume if there 
is no forcewake (and you're right, intel_uncore_suspend_forcewake is a 
better naming). Ultimately I'd like to transition the GT uncore inside 
intel_gt and call this inside intel_gt_suspend(). However, I don't mind 
keeping the current naming and calling it for display uncore as well to 
be more generic, there are checks everywhere so the overhead should me 
minimal. What's your preference?

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

Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-06-18 19:40:40)
> 
> 
> On 6/18/19 3:22 AM, Chris Wilson wrote:
> > Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33)
> >> We always call some of the setup/cleanup functions for forcewake, even
> >> if the feature is not actually available. Skipping these operations if
> >> forcewake is not available saves us some operations on older gens and
> >> prepares us for having a forcewake-less display uncore.
> >> The suspend/resume functions have also been renamed to clearly indicate
> >> that they only operate on forcewake status.
> >>
> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.c     |  15 +--
> >>   drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
> >>   drivers/gpu/drm/i915/intel_uncore.h |   8 +-
> >>   3 files changed, 101 insertions(+), 69 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index d113296cbe34..95b36fe17f99 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
> >>   
> >>          intel_device_info_init_mmio(dev_priv);
> >>   
> >> -       intel_uncore_prune_mmio_domains(&dev_priv->uncore);
> >> +       intel_uncore_prune_forcewake_domains(&dev_priv->uncore);
> > 
> > The _prune_ was the exceptional case...
> > 
> >>          intel_uc_init_mmio(dev_priv);
> >>   
> >> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >>   
> >>          i915_gem_suspend_late(dev_priv);
> >>   
> >> -       intel_uncore_suspend(&dev_priv->uncore);
> >> +       intel_uncore_forcewake_suspend(&dev_priv->uncore);
> > 
> > But are you sure you want to delegate all the fw control to i915_drv.c,
> > and not keep this as a call into intel_uncore_suspend() ? It is meant to
> > be telling the uncore functionality that it is time to suspend, and it
> > has to work out what to save.
> > 
> > I am not sold on this yet. (One day this will just be a bunch of async
> > tasks plugged into signals with ordering determined by their
> > self-declared dependency tree. One day.)
> > 
> > So sell me on why we want an uncore_forcewake object, as currently you
> > are proposing a intel_uncore_suspend_forcewake().
> > -Chris
> > 
> 
> My aim was to make it clear that this call will not be required for 
> display_uncore since there is nothing to do on suspend/resume if there 
> is no forcewake (and you're right, intel_uncore_suspend_forcewake is a 
> better naming). Ultimately I'd like to transition the GT uncore inside 
> intel_gt and call this inside intel_gt_suspend(). However, I don't mind 
> keeping the current naming and calling it for display uncore as well to 
> be more generic, there are checks everywhere so the overhead should me 
> minimal. What's your preference?

I think, for the time being a sketch like
i915_drm_suspend:
	intel_uncore_suspend(i915->gt.uncore)
	intel_uncore_suspend(i915->display.uncore)

is preferable. Rather than pulling the knowlegde that we need only
suspend the gt.uncore because it has forcewake into the high level
i915_drm_suspend. A couple of ifs or a vfunc go a long way to keeping as
simple as it can be (and no simpler).

At the i915_drm_suspend level it is hard enough to manage a list of
components and the correct order in which to call them :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Daniele Ceraolo Spurio]

On 6/18/19 2:00 AM, Tvrtko Ursulin wrote:
> 
> On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
>> We always call some of the setup/cleanup functions for forcewake, even
>> if the feature is not actually available. Skipping these operations if
>> forcewake is not available saves us some operations on older gens and
>> prepares us for having a forcewake-less display uncore.
>> The suspend/resume functions have also been renamed to clearly indicate
>> that they only operate on forcewake status.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c     |  15 +--
>>   drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
>>   drivers/gpu/drm/i915/intel_uncore.h |   8 +-
>>   3 files changed, 101 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index d113296cbe34..95b36fe17f99 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct 
>> drm_i915_private *dev_priv)
>>       intel_device_info_init_mmio(dev_priv);
>> -    intel_uncore_prune_mmio_domains(&dev_priv->uncore);
>> +    intel_uncore_prune_forcewake_domains(&dev_priv->uncore);
>>       intel_uc_init_mmio(dev_priv);
>> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct 
>> drm_device *dev, bool hibernation)
>>       i915_gem_suspend_late(dev_priv);
>> -    intel_uncore_suspend(&dev_priv->uncore);
>> +    intel_uncore_forcewake_suspend(&dev_priv->uncore);
>>       intel_power_domains_suspend(dev_priv,
>>                       get_suspend_mode(dev_priv, hibernation));
>> @@ -2348,7 +2348,10 @@ static int i915_drm_resume_early(struct 
>> drm_device *dev)
>>           DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
>>                 ret);
>> -    intel_uncore_resume_early(&dev_priv->uncore);
>> +    if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
>> +        DRM_DEBUG("unclaimed mmio detected on resume, clearing\n");
>> +
> 
> Why does this bit needs to be pulled up to this level? My first line of 
> thinking is that we should aim to keep the component specific steps 
> down, if possible.

The idea was to split this out to have the function below act on 
forcewake only. Chris didn't like it either, so I'm going to roll back 
these changes.

> 
>> +    intel_uncore_forcewake_resume_early(&dev_priv->uncore);
>>       i915_check_and_clear_faults(dev_priv);
>> @@ -2923,7 +2926,7 @@ static int intel_runtime_suspend(struct device 
>> *kdev)
>>       intel_runtime_pm_disable_interrupts(dev_priv);
>> -    intel_uncore_suspend(&dev_priv->uncore);
>> +    intel_uncore_forcewake_suspend(&dev_priv->uncore);
>>       ret = 0;
>>       if (INTEL_GEN(dev_priv) >= 11) {
>> @@ -2940,7 +2943,7 @@ static int intel_runtime_suspend(struct device 
>> *kdev)
>>       if (ret) {
>>           DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>> -        intel_uncore_runtime_resume(&dev_priv->uncore);
>> +        intel_uncore_forcewake_runtime_resume(&dev_priv->uncore);
>>           intel_runtime_pm_enable_interrupts(dev_priv);
>> @@ -3038,7 +3041,7 @@ static int intel_runtime_resume(struct device 
>> *kdev)
>>           ret = vlv_resume_prepare(dev_priv, true);
>>       }
>> -    intel_uncore_runtime_resume(&dev_priv->uncore);
>> +    intel_uncore_forcewake_runtime_resume(&dev_priv->uncore);
>>       intel_runtime_pm_enable_interrupts(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index 88a69bf713c9..c0f5567ee096 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -485,12 +485,11 @@ check_for_unclaimed_mmio(struct intel_uncore 
>> *uncore)
>>       return ret;
>>   }
>> -static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
>> +static void forcewake_early_sanitize(struct intel_uncore *uncore,
>>                         unsigned int restore_forcewake)
>>   {
>> -    /* clear out unclaimed reg detection bit */
>> -    if (check_for_unclaimed_mmio(uncore))
>> -        DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>> +    if (!intel_uncore_has_forcewake(uncore))
>> +        return;
>>       /* WaDisableShadowRegForCpd:chv */
>>       if (IS_CHERRYVIEW(uncore->i915)) {
>> @@ -513,8 +512,11 @@ static void __intel_uncore_early_sanitize(struct 
>> intel_uncore *uncore,
>>       iosf_mbi_punit_release();
>>   }
>> -void intel_uncore_suspend(struct intel_uncore *uncore)
>> +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore)
>>   {
>> +    if (!intel_uncore_has_forcewake(uncore))
>> +        return;
>> +
>>       iosf_mbi_punit_acquire();
>>       iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>>           &uncore->pmic_bus_access_nb);
>> @@ -522,18 +524,24 @@ void intel_uncore_suspend(struct intel_uncore 
>> *uncore)
>>       iosf_mbi_punit_release();
>>   }
>> -void intel_uncore_resume_early(struct intel_uncore *uncore)
>> +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore)
>>   {
>>       unsigned int restore_forcewake;
>> +    if (!intel_uncore_has_forcewake(uncore))
>> +        return;
>> +
>>       restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved);
>> -    __intel_uncore_early_sanitize(uncore, restore_forcewake);
>> +    forcewake_early_sanitize(uncore, restore_forcewake);
> 
> This call already handles !has_forcewake, so function handles it twice 
> in source. Is this what you intended? Maybe just add double-underscore 
> version for early sanitize without the check but GEM_BUG_ON?

will do.

> 
>>       
>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>>   }
>> -void intel_uncore_runtime_resume(struct intel_uncore *uncore)
>> +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore)
>>   {
>> +    if (!intel_uncore_has_forcewake(uncore))
>> +        return;
>> +
>>       
>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>>   }
>> @@ -1348,8 +1356,7 @@ static void intel_uncore_fw_domains_init(struct 
>> intel_uncore *uncore)
>>   {
>>       struct drm_i915_private *i915 = uncore->i915;
>> -    if (!intel_uncore_has_forcewake(uncore))
>> -        return;
>> +    GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>>       if (INTEL_GEN(i915) >= 11) {
>>           int i;
>> @@ -1542,36 +1549,29 @@ void intel_uncore_init_early(struct 
>> drm_i915_private *i915,
>>       uncore->rpm = &i915->runtime_pm;
>>   }
>> -int intel_uncore_init_mmio(struct intel_uncore *uncore)
>> +static void uncore_raw_init(struct intel_uncore *uncore)
>>   {
>> -    struct drm_i915_private *i915 = uncore->i915;
>> -    int ret;
>> +    GEM_BUG_ON(intel_uncore_has_forcewake(uncore));
>> -    ret = uncore_mmio_setup(uncore);
>> -    if (ret)
>> -        return ret;
>> +    if (IS_GEN(uncore->i915, 5)) {
>> +        ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
>> +        ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
>> +    } else {
>> +        ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
>> +        ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
>> +    }
>> +}
>> -    i915_check_vgpu(i915);
>> +static void uncore_forcewake_init(struct intel_uncore *uncore)
>> +{
>> +    struct drm_i915_private *i915 = uncore->i915;
>> -    if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
>> -        uncore->flags |= UNCORE_HAS_FORCEWAKE;
>> +    GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>>       intel_uncore_fw_domains_init(uncore);
>> -    __intel_uncore_early_sanitize(uncore, 0);
>> +    forcewake_early_sanitize(uncore, 0);
>> -    uncore->unclaimed_mmio_check = 1;
>> -    uncore->pmic_bus_access_nb.notifier_call =
>> -        i915_pmic_bus_access_notifier;
>> -
>> -    if (!intel_uncore_has_forcewake(uncore)) {
>> -        if (IS_GEN(i915, 5)) {
>> -            ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen5);
>> -            ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen5);
>> -        } else {
>> -            ASSIGN_RAW_WRITE_MMIO_VFUNCS(uncore, gen2);
>> -            ASSIGN_RAW_READ_MMIO_VFUNCS(uncore, gen2);
>> -        }
>> -    } else if (IS_GEN_RANGE(i915, 6, 7)) {
>> +    if (IS_GEN_RANGE(i915, 6, 7)) {
>>           ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
>>           if (IS_VALLEYVIEW(i915)) {
>> @@ -1585,7 +1585,6 @@ int intel_uncore_init_mmio(struct intel_uncore 
>> *uncore)
>>               ASSIGN_FW_DOMAINS_TABLE(uncore, __chv_fw_ranges);
>>               ASSIGN_WRITE_MMIO_VFUNCS(uncore, fwtable);
>>               ASSIGN_READ_MMIO_VFUNCS(uncore, fwtable);
>> -
>>           } else {
>>               ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen8);
>>               ASSIGN_READ_MMIO_VFUNCS(uncore, gen6);
>> @@ -1600,6 +1599,31 @@ int intel_uncore_init_mmio(struct intel_uncore 
>> *uncore)
>>           ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>>       }
>> +    uncore->pmic_bus_access_nb.notifier_call = 
>> i915_pmic_bus_access_notifier;
>> +    
>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>> +}
>> +
>> +int intel_uncore_init_mmio(struct intel_uncore *uncore)
>> +{
>> +    struct drm_i915_private *i915 = uncore->i915;
>> +    int ret;
>> +
>> +    ret = uncore_mmio_setup(uncore);
>> +    if (ret)
>> +        return ret;
>> +
>> +    i915_check_vgpu(i915);
>> +
>> +    if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
>> +        uncore->flags |= UNCORE_HAS_FORCEWAKE;
>> +
>> +    uncore->unclaimed_mmio_check = 1;
>> +
>> +    if (!intel_uncore_has_forcewake(uncore))
>> +        uncore_raw_init(uncore);
> 
> Is any of the remaining code in this function relevant after this branch 
> has been taken? If not this could be changed to:
> 
>    if (!intel_uncore_has_forcewake(uncore)) {
>      uncore_raw_init(uncore);
>      return;
>    }
> 
>    uncore_forcewake_init(uncore);
>    ...
> 
> Hm, also is "unclaimed_mmio_check = 1;" above possible/relevant where no 
> forcwake? Doesn't look like it. Unless vgpu?
> 

The unclaimed mmio stuff (including the flags) below this point is not 
tied to forcewake from a functional POV, but it indeed true that all the 
platforms with the mmio debug do have forcewake. I'd still prefer to 
avoid connecting the 2 features under the same check just because they 
happen to be on the same platforms.

Also as you mentioned GVT traps the FPGA_DBG register used by the 
unclaimed mmio logic, so they do have the capability when forcewake is 
disabled.

>> +    else
>> +        uncore_forcewake_init(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);
>> @@ -1615,7 +1639,9 @@ int intel_uncore_init_mmio(struct intel_uncore 
>> *uncore)
>>       if (IS_GEN_RANGE(i915, 6, 7))
>>           uncore->flags |= UNCORE_HAS_FIFO;
>> -    
>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>> +    /* clear out unclaimed reg detection bit */
>> +    if (check_for_unclaimed_mmio(uncore))
>> +        DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>>       return 0;
>>   }
>> @@ -1625,44 +1651,47 @@ int intel_uncore_init_mmio(struct intel_uncore 
>> *uncore)
>>    * the forcewake domains. Prune them, to make sure they only 
>> reference existing
>>    * engines.
>>    */
>> -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore)
>> +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore)
>>   {
>>       struct drm_i915_private *i915 = uncore->i915;
>> +    enum forcewake_domains fw_domains = uncore->fw_domains;
>> +    enum forcewake_domain_id domain_id;
>> +    int i;
>> -    if (INTEL_GEN(i915) >= 11) {
>> -        enum forcewake_domains fw_domains = uncore->fw_domains;
>> -        enum forcewake_domain_id domain_id;
>> -        int i;
>> +    if (!intel_uncore_has_forcewake(uncore) || INTEL_GEN(i915) < 11)
>> +        return;
>> -        for (i = 0; i < I915_MAX_VCS; i++) {
>> -            domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
>> +    for (i = 0; i < I915_MAX_VCS; i++) {
>> +        domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
>> -            if (HAS_ENGINE(i915, _VCS(i)))
>> -                continue;
>> +        if (HAS_ENGINE(i915, _VCS(i)))
>> +            continue;
>> -            if (fw_domains & BIT(domain_id))
>> -                fw_domain_fini(uncore, domain_id);
>> -        }
>> +        if (fw_domains & BIT(domain_id))
>> +            fw_domain_fini(uncore, domain_id);
>> +    }
>> -        for (i = 0; i < I915_MAX_VECS; i++) {
>> -            domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
>> +    for (i = 0; i < I915_MAX_VECS; i++) {
>> +        domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
>> -            if (HAS_ENGINE(i915, _VECS(i)))
>> -                continue;
>> +        if (HAS_ENGINE(i915, _VECS(i)))
>> +            continue;
>> -            if (fw_domains & BIT(domain_id))
>> -                fw_domain_fini(uncore, domain_id);
>> -        }
>> +        if (fw_domains & BIT(domain_id))
>> +            fw_domain_fini(uncore, domain_id);
>>       }
>>   }
>>   void intel_uncore_fini_mmio(struct intel_uncore *uncore)
>>   {
>> -    iosf_mbi_punit_acquire();
>> -    iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>> -        &uncore->pmic_bus_access_nb);
>> -    intel_uncore_forcewake_reset(uncore);
>> -    iosf_mbi_punit_release();
>> +    if (intel_uncore_has_forcewake(uncore)) {
> 
> To avoid hyphotetical obnoxious diffs in the future, like the one for 
> intel_uncore_prune_mmio_domains above in this patch, maybe invert this 
> to early return straight away.

will do.

Daniele

> 
>> +        iosf_mbi_punit_acquire();
>> +        iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>> +            &uncore->pmic_bus_access_nb);
>> +        intel_uncore_forcewake_reset(uncore);
>> +        iosf_mbi_punit_release();
>> +    }
>> +
>>       uncore_mmio_cleanup(uncore);
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h 
>> b/drivers/gpu/drm/i915/intel_uncore.h
>> index 912616188ff5..879252735bba 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>> @@ -186,13 +186,13 @@ intel_uncore_has_fifo(const struct intel_uncore 
>> *uncore)
>>   void intel_uncore_init_early(struct drm_i915_private *i915,
>>                    struct intel_uncore *uncore);
>>   int intel_uncore_init_mmio(struct intel_uncore *uncore);
>> -void intel_uncore_prune_mmio_domains(struct intel_uncore *uncore);
>> +void intel_uncore_prune_forcewake_domains(struct intel_uncore *uncore);
>>   bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore);
>>   bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore 
>> *uncore);
>>   void intel_uncore_fini_mmio(struct intel_uncore *uncore);
>> -void intel_uncore_suspend(struct intel_uncore *uncore);
>> -void intel_uncore_resume_early(struct intel_uncore *uncore);
>> -void intel_uncore_runtime_resume(struct intel_uncore *uncore);
>> +void intel_uncore_forcewake_suspend(struct intel_uncore *uncore);
>> +void intel_uncore_forcewake_resume_early(struct intel_uncore *uncore);
>> +void intel_uncore_forcewake_runtime_resume(struct intel_uncore *uncore);
>>   void assert_forcewakes_inactive(struct intel_uncore *uncore);
>>   void assert_forcewakes_active(struct intel_uncore *uncore,
>>
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Daniele Ceraolo Spurio]

On 6/18/19 2:23 AM, Tvrtko Ursulin wrote:
> 
> On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
>> We'd like to introduce a display uncore with no forcewake domains, so
>> let's avoid wasting memory and be ready to allocate only what we need.
>> Even without multiple uncore, we still don't need all the domains on all
>> gens.
> 
> Looks good in principle, only a few conversation points below.
> 
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 155 ++++++++++++++++++----------
>>   drivers/gpu/drm/i915/intel_uncore.h |  13 +--
>>   2 files changed, 102 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index c0f5567ee096..7f311827c3ef 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -344,7 +344,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>>   {
>>       struct intel_uncore_forcewake_domain *domain =
>>              container_of(timer, struct intel_uncore_forcewake_domain, 
>> timer);
>> -    struct intel_uncore *uncore = forcewake_domain_to_uncore(domain);
>> +    struct intel_uncore *uncore = domain->uncore;
>>       unsigned long irqflags;
>>       assert_rpm_device_not_suspended(uncore->rpm);
>> @@ -1291,23 +1291,23 @@ do { \
>>       (uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
>>   } while (0)
>> -static void fw_domain_init(struct intel_uncore *uncore,
>> +static int fw_domain_init(struct intel_uncore *uncore,
>>                  enum forcewake_domain_id domain_id,
>>                  i915_reg_t reg_set,
>>                  i915_reg_t reg_ack)
>>   {
>>       struct intel_uncore_forcewake_domain *d;
>> -    if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
>> -        return;
>> -
>> -    d = &uncore->fw_domain[domain_id];
>> +    GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
>> -    WARN_ON(d->wake_count);
> 
> I wonder what was this for.
> 
> Do you want to add some protection against double init of the same domain?
> 

just a GEM_BUG_ON maybe? It shouldn't really ever happen unless we're 
moving something around.

>> +    d = kzalloc(sizeof(*d), GFP_KERNEL);
>> +    if (!d)
>> +        return -ENOMEM;
>>       WARN_ON(!i915_mmio_reg_valid(reg_set));
>>       WARN_ON(!i915_mmio_reg_valid(reg_ack));
>> +    d->uncore = uncore;
>>       d->wake_count = 0;
>>       d->reg_set = uncore->regs + i915_mmio_reg_offset(reg_set);
>>       d->reg_ack = uncore->regs + i915_mmio_reg_offset(reg_ack);
>> @@ -1324,7 +1324,6 @@ static void fw_domain_init(struct intel_uncore 
>> *uncore,
>>       BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX0 != (1 << 
>> FW_DOMAIN_ID_MEDIA_VEBOX0));
>>       BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX1 != (1 << 
>> FW_DOMAIN_ID_MEDIA_VEBOX1));
>> -
>>       d->mask = BIT(domain_id);
>>       hrtimer_init(&d->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> @@ -1333,6 +1332,10 @@ static void fw_domain_init(struct intel_uncore 
>> *uncore,
>>       uncore->fw_domains |= BIT(domain_id);
>>       fw_domain_reset(d);
>> +
>> +    uncore->fw_domain[domain_id] = d;
>> +
>> +    return 0;
>>   }
>>   static void fw_domain_fini(struct intel_uncore *uncore,
>> @@ -1340,77 +1343,92 @@ static void fw_domain_fini(struct intel_uncore 
>> *uncore,
>>   {
>>       struct intel_uncore_forcewake_domain *d;
>> -    if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
>> -        return;
>> +    GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
>> -    d = &uncore->fw_domain[domain_id];
>> +    d = fetch_and_zero(&uncore->fw_domain[domain_id]);
> 
> Maybe early if !d here and function flow just carries on?
> 

will do.

>> +    uncore->fw_domains &= ~BIT(domain_id);
>> -    WARN_ON(d->wake_count);
>> -    WARN_ON(hrtimer_cancel(&d->timer));
>> -    memset(d, 0, sizeof(*d));
>> +    if (d) {
>> +        WARN_ON(d->wake_count);
>> +        WARN_ON(hrtimer_cancel(&d->timer));
>> +        kfree(d);
>> +    }
>> +}
>> -    uncore->fw_domains &= ~BIT(domain_id);
>> +static void intel_uncore_fw_domains_fini(struct intel_uncore *uncore)
>> +{
>> +    struct intel_uncore_forcewake_domain *d;
>> +    int tmp;
>> +
>> +    for_each_fw_domain(d, uncore, tmp)
>> +        fw_domain_fini(uncore, d->id);
>>   }
>> -static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>> +static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>   {
>>       struct drm_i915_private *i915 = uncore->i915;
>> +    int ret;
>>       GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>> +#define __fw_domain_init(id__, set__, ack__) \
>> +    ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
>> +    if (ret) \
>> +        goto out_clean;
> 
> Hidden control flow is slightly objectionable but I don't offer any nice 
> alternatives so I guess will have to pass. Or maybe accumulate the error 
> (err |= fw_domain_init(..)) as you go and then cleanup at the end if any 
> failed?

I'd prefer to avoid accumulating the error since it'd just cause us to 
having to unroll more domains when we could've stopped early.

> 
> On the other hand the idea slightly conflicts with my other suggestion 
> to rename existing fw_domain_init to __fw_domain_init and call the macro 
> fw_domain_init and avoid all the churn below.
> 

I'll pick this suggestion among the 2, unless there is another 
suggestion on how to avoid the hidden goto.

>> +
>>       if (INTEL_GEN(i915) >= 11) {
>>           int i;
>> -        uncore->funcs.force_wake_get =
>> -            fw_domains_get_with_fallback;
>> +        uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
>>           uncore->funcs.force_wake_put = fw_domains_put;
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>> -                   FORCEWAKE_RENDER_GEN9,
>> -                   FORCEWAKE_ACK_RENDER_GEN9);
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
>> -                   FORCEWAKE_BLITTER_GEN9,
>> -                   FORCEWAKE_ACK_BLITTER_GEN9);
>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                 FORCEWAKE_RENDER_GEN9,
>> +                 FORCEWAKE_ACK_RENDER_GEN9);
>> +        __fw_domain_init(FW_DOMAIN_ID_BLITTER,
>> +                 FORCEWAKE_BLITTER_GEN9,
>> +                 FORCEWAKE_ACK_BLITTER_GEN9);
>> +
>>           for (i = 0; i < I915_MAX_VCS; i++) {
>>               if (!HAS_ENGINE(i915, _VCS(i)))
>>                   continue;
>> -            fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
>> -                       FORCEWAKE_MEDIA_VDBOX_GEN11(i),
>> -                       FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
>> +            __fw_domain_init(FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
>> +                     FORCEWAKE_MEDIA_VDBOX_GEN11(i),
>> +                     FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
>>           }
>>           for (i = 0; i < I915_MAX_VECS; i++) {
>>               if (!HAS_ENGINE(i915, _VECS(i)))
>>                   continue;
>> -            fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
>> -                       FORCEWAKE_MEDIA_VEBOX_GEN11(i),
>> -                       FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
>> +            __fw_domain_init(FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
>> +                     FORCEWAKE_MEDIA_VEBOX_GEN11(i),
>> +                     FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
>>           }
>>       } else if (IS_GEN_RANGE(i915, 9, 10)) {
>> -        uncore->funcs.force_wake_get =
>> -            fw_domains_get_with_fallback;
>> +        uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
>>           uncore->funcs.force_wake_put = fw_domains_put;
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>> -                   FORCEWAKE_RENDER_GEN9,
>> -                   FORCEWAKE_ACK_RENDER_GEN9);
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
>> -                   FORCEWAKE_BLITTER_GEN9,
>> -                   FORCEWAKE_ACK_BLITTER_GEN9);
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
>> -                   FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                 FORCEWAKE_RENDER_GEN9,
>> +                 FORCEWAKE_ACK_RENDER_GEN9);
>> +        __fw_domain_init(FW_DOMAIN_ID_BLITTER,
>> +                 FORCEWAKE_BLITTER_GEN9,
>> +                 FORCEWAKE_ACK_BLITTER_GEN9);
>> +        __fw_domain_init(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;
>> -        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);
>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                 FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
>> +        __fw_domain_init(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;
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>> -                   FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                 FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
>>       } else if (IS_IVYBRIDGE(i915)) {
>>           u32 ecobus;
>> @@ -1437,8 +1455,8 @@ static void intel_uncore_fw_domains_init(struct 
>> intel_uncore *uncore)
>>           __raw_uncore_write32(uncore, FORCEWAKE, 0);
>>           __raw_posting_read(uncore, ECOBUS);
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>> -                   FORCEWAKE_MT, FORCEWAKE_MT_ACK);
>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                 FORCEWAKE_MT, FORCEWAKE_MT_ACK);
>>           spin_lock_irq(&uncore->lock);
>>           fw_domains_get_with_thread_status(uncore, FORCEWAKE_RENDER);
>> @@ -1449,19 +1467,27 @@ static void 
>> intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>           if (!(ecobus & FORCEWAKE_MT_ENABLE)) {
>>               DRM_INFO("No MT forcewake available on Ivybridge, this 
>> can result in issues\n");
>>               DRM_INFO("when using vblank-synced partial screen 
>> updates.\n");
>> -            fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>> -                       FORCEWAKE, FORCEWAKE_ACK);
>> +            __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                     FORCEWAKE, FORCEWAKE_ACK);
>>           }
>>       } else if (IS_GEN(i915, 6)) {
>>           uncore->funcs.force_wake_get =
>>               fw_domains_get_with_thread_status;
>>           uncore->funcs.force_wake_put = fw_domains_put;
>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>> -                   FORCEWAKE, FORCEWAKE_ACK);
>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>> +                 FORCEWAKE, FORCEWAKE_ACK);
>>       }
>> +#undef __fw_domain_init
>> +
>>       /* All future platforms are expected to require complex power 
>> gating */
>>       WARN_ON(uncore->fw_domains == 0);
>> +
>> +    return 0;
>> +
>> +out_clean:
>> +    intel_uncore_fw_domains_fini(uncore);
>> +    return ret;
>>   }
>>   #define ASSIGN_FW_DOMAINS_TABLE(uncore, d) \
>> @@ -1562,13 +1588,17 @@ static void uncore_raw_init(struct 
>> intel_uncore *uncore)
>>       }
>>   }
>> -static void uncore_forcewake_init(struct intel_uncore *uncore)
>> +static int uncore_forcewake_init(struct intel_uncore *uncore)
>>   {
>>       struct drm_i915_private *i915 = uncore->i915;
>> +    int ret;
>>       GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>> -    intel_uncore_fw_domains_init(uncore);
>> +    ret = intel_uncore_fw_domains_init(uncore);
>> +    if (ret)
>> +        return ret;
>> +
>>       forcewake_early_sanitize(uncore, 0);
>>       if (IS_GEN_RANGE(i915, 6, 7)) {
>> @@ -1601,6 +1631,8 @@ static void uncore_forcewake_init(struct 
>> intel_uncore *uncore)
>>       uncore->pmic_bus_access_nb.notifier_call = 
>> i915_pmic_bus_access_notifier;
>>       
>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>> +
>> +    return 0;
>>   }
>>   int intel_uncore_init_mmio(struct intel_uncore *uncore)
>> @@ -1619,10 +1651,13 @@ int intel_uncore_init_mmio(struct intel_uncore 
>> *uncore)
>>       uncore->unclaimed_mmio_check = 1;
>> -    if (!intel_uncore_has_forcewake(uncore))
>> +    if (!intel_uncore_has_forcewake(uncore)) {
>>           uncore_raw_init(uncore);
>> -    else
>> -        uncore_forcewake_init(uncore);
>> +    } else {
>> +        ret = uncore_forcewake_init(uncore);
>> +        if (ret)
>> +            goto out_mmio_cleanup;
>> +    }
>>       /* 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);
>> @@ -1644,6 +1679,11 @@ int intel_uncore_init_mmio(struct intel_uncore 
>> *uncore)
>>           DRM_DEBUG("unclaimed mmio detected on uncore init, 
>> clearing\n");
>>       return 0;
>> +
>> +out_mmio_cleanup:
>> +    uncore_mmio_cleanup(uncore);
>> +
>> +    return ret;
>>   }
>>   /*
>> @@ -1689,6 +1729,7 @@ void intel_uncore_fini_mmio(struct intel_uncore 
>> *uncore)
>>           iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>>               &uncore->pmic_bus_access_nb);
>>           intel_uncore_forcewake_reset(uncore);
>> +        intel_uncore_fw_domains_fini(uncore);
>>           iosf_mbi_punit_release();
>>       }
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h 
>> b/drivers/gpu/drm/i915/intel_uncore.h
>> index 879252735bba..bbff281b880d 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>> @@ -126,6 +126,7 @@ struct intel_uncore {
>>       enum forcewake_domains fw_domains_saved; /* user domains saved 
>> for S3 */
>>       struct intel_uncore_forcewake_domain {
>> +        struct intel_uncore *uncore;
>>           enum forcewake_domain_id id;
>>           enum forcewake_domains mask;
>>           unsigned int wake_count;
>> @@ -133,7 +134,7 @@ struct intel_uncore {
>>           struct hrtimer timer;
>>           u32 __iomem *reg_set;
>>           u32 __iomem *reg_ack;
>> -    } fw_domain[FW_DOMAIN_ID_COUNT];
>> +    } *fw_domain[FW_DOMAIN_ID_COUNT];
> 
> The array itself could also be made dynamic, no? To much hassle for very 
> little gain?
> 

With the current approach we don't know how many domains we have in 
advance, so we'd have to either add a table for that or realloc the 
array as we increase the number of domains. Both options don't seem 
worth the hassle IMO.

Thanks,
Daniele

>>       struct {
>>           unsigned int count;
>> @@ -147,18 +148,12 @@ struct intel_uncore {
>>   /* Iterate over initialised fw domains */
>>   #define for_each_fw_domain_masked(domain__, mask__, uncore__, tmp__) \
>> -    for (tmp__ = (mask__); \
>> -         tmp__ ? (domain__ = 
>> &(uncore__)->fw_domain[__mask_next_bit(tmp__)]), 1 : 0;)
>> +    for (tmp__ = (mask__); tmp__ ;) \
>> +        for_each_if(domain__ = 
>> (uncore__)->fw_domain[__mask_next_bit(tmp__)])
>>   #define for_each_fw_domain(domain__, uncore__, tmp__) \
>>       for_each_fw_domain_masked(domain__, (uncore__)->fw_domains, 
>> uncore__, tmp__)
>> -static inline struct intel_uncore *
>> -forcewake_domain_to_uncore(const struct intel_uncore_forcewake_domain 
>> *d)
>> -{
>> -    return container_of(d, struct intel_uncore, fw_domain[d->id]);
>> -}
>> -
>>   static inline bool
>>   intel_uncore_has_forcewake(const struct intel_uncore *uncore)
>>   {
>>
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-06-19 00:06:39)
> 
> 
> On 6/18/19 2:23 AM, Tvrtko Ursulin wrote:
> > 
> > On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
> >> -static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> >> +static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> >>   {
> >>       struct drm_i915_private *i915 = uncore->i915;
> >> +    int ret;
> >>       GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
> >> +#define __fw_domain_init(id__, set__, ack__) \
> >> +    ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
> >> +    if (ret) \
> >> +        goto out_clean;
> > 
> > Hidden control flow is slightly objectionable but I don't offer any nice 
> > alternatives so I guess will have to pass. Or maybe accumulate the error 
> > (err |= fw_domain_init(..)) as you go and then cleanup at the end if any 
> > failed?
> 
> I'd prefer to avoid accumulating the error since it'd just cause us to 
> having to unroll more domains when we could've stopped early.
> 
> > 
> > On the other hand the idea slightly conflicts with my other suggestion 
> > to rename existing fw_domain_init to __fw_domain_init and call the macro 
> > fw_domain_init and avoid all the churn below.
> > 
> 
> I'll pick this suggestion among the 2, unless there is another 
> suggestion on how to avoid the hidden goto.

Be careful, or you'll give Tvrtko the chance to suggest table driven
setup. Maybe?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Daniele Ceraolo Spurio]

On 6/18/19 4:23 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2019-06-19 00:06:39)
>>
>>
>> On 6/18/19 2:23 AM, Tvrtko Ursulin wrote:
>>>
>>> On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
>>>> -static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>>> +static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>>>    {
>>>>        struct drm_i915_private *i915 = uncore->i915;
>>>> +    int ret;
>>>>        GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>>>> +#define __fw_domain_init(id__, set__, ack__) \
>>>> +    ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
>>>> +    if (ret) \
>>>> +        goto out_clean;
>>>
>>> Hidden control flow is slightly objectionable but I don't offer any nice
>>> alternatives so I guess will have to pass. Or maybe accumulate the error
>>> (err |= fw_domain_init(..)) as you go and then cleanup at the end if any
>>> failed?
>>
>> I'd prefer to avoid accumulating the error since it'd just cause us to
>> having to unroll more domains when we could've stopped early.
>>
>>>
>>> On the other hand the idea slightly conflicts with my other suggestion
>>> to rename existing fw_domain_init to __fw_domain_init and call the macro
>>> fw_domain_init and avoid all the churn below.
>>>
>>
>> I'll pick this suggestion among the 2, unless there is another
>> suggestion on how to avoid the hidden goto.
> 
> Be careful, or you'll give Tvrtko the chance to suggest table driven
> setup. Maybe?
> -Chris
> 

I did consider using a table myself, but the differences between the 
domains are not easy to put in a table since some of them are per-gen 
and other per-platform. We could combine a table with information in the 
device_info struct like we do for the engines, but that felt a bit like 
overkill to me.

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

Re: [PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Tvrtko Ursulin]

On 19/06/2019 00:06, Daniele Ceraolo Spurio wrote:
> On 6/18/19 2:23 AM, Tvrtko Ursulin wrote:
>> On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
>>> We'd like to introduce a display uncore with no forcewake domains, so
>>> let's avoid wasting memory and be ready to allocate only what we need.
>>> Even without multiple uncore, we still don't need all the domains on all
>>> gens.
>>
>> Looks good in principle, only a few conversation points below.
>>
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_uncore.c | 155 ++++++++++++++++++----------
>>>   drivers/gpu/drm/i915/intel_uncore.h |  13 +--
>>>   2 files changed, 102 insertions(+), 66 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>>> b/drivers/gpu/drm/i915/intel_uncore.c
>>> index c0f5567ee096..7f311827c3ef 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -344,7 +344,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>>>   {
>>>       struct intel_uncore_forcewake_domain *domain =
>>>              container_of(timer, struct 
>>> intel_uncore_forcewake_domain, timer);
>>> -    struct intel_uncore *uncore = forcewake_domain_to_uncore(domain);
>>> +    struct intel_uncore *uncore = domain->uncore;
>>>       unsigned long irqflags;
>>>       assert_rpm_device_not_suspended(uncore->rpm);
>>> @@ -1291,23 +1291,23 @@ do { \
>>>       (uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
>>>   } while (0)
>>> -static void fw_domain_init(struct intel_uncore *uncore,
>>> +static int fw_domain_init(struct intel_uncore *uncore,
>>>                  enum forcewake_domain_id domain_id,
>>>                  i915_reg_t reg_set,
>>>                  i915_reg_t reg_ack)
>>>   {
>>>       struct intel_uncore_forcewake_domain *d;
>>> -    if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
>>> -        return;
>>> -
>>> -    d = &uncore->fw_domain[domain_id];
>>> +    GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
>>> -    WARN_ON(d->wake_count);
>>
>> I wonder what was this for.
>>
>> Do you want to add some protection against double init of the same 
>> domain?
>>
> 
> just a GEM_BUG_ON maybe? It shouldn't really ever happen unless we're 
> moving something around.

Yes, sounds right.

> 
>>> +    d = kzalloc(sizeof(*d), GFP_KERNEL);
>>> +    if (!d)
>>> +        return -ENOMEM;
>>>       WARN_ON(!i915_mmio_reg_valid(reg_set));
>>>       WARN_ON(!i915_mmio_reg_valid(reg_ack));
>>> +    d->uncore = uncore;
>>>       d->wake_count = 0;
>>>       d->reg_set = uncore->regs + i915_mmio_reg_offset(reg_set);
>>>       d->reg_ack = uncore->regs + i915_mmio_reg_offset(reg_ack);
>>> @@ -1324,7 +1324,6 @@ static void fw_domain_init(struct intel_uncore 
>>> *uncore,
>>>       BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX0 != (1 << 
>>> FW_DOMAIN_ID_MEDIA_VEBOX0));
>>>       BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX1 != (1 << 
>>> FW_DOMAIN_ID_MEDIA_VEBOX1));
>>> -
>>>       d->mask = BIT(domain_id);
>>>       hrtimer_init(&d->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>> @@ -1333,6 +1332,10 @@ static void fw_domain_init(struct intel_uncore 
>>> *uncore,
>>>       uncore->fw_domains |= BIT(domain_id);
>>>       fw_domain_reset(d);
>>> +
>>> +    uncore->fw_domain[domain_id] = d;
>>> +
>>> +    return 0;
>>>   }
>>>   static void fw_domain_fini(struct intel_uncore *uncore,
>>> @@ -1340,77 +1343,92 @@ static void fw_domain_fini(struct 
>>> intel_uncore *uncore,
>>>   {
>>>       struct intel_uncore_forcewake_domain *d;
>>> -    if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
>>> -        return;
>>> +    GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
>>> -    d = &uncore->fw_domain[domain_id];
>>> +    d = fetch_and_zero(&uncore->fw_domain[domain_id]);
>>
>> Maybe early if !d here and function flow just carries on?
>>
> 
> will do.
> 
>>> +    uncore->fw_domains &= ~BIT(domain_id);
>>> -    WARN_ON(d->wake_count);
>>> -    WARN_ON(hrtimer_cancel(&d->timer));
>>> -    memset(d, 0, sizeof(*d));
>>> +    if (d) {
>>> +        WARN_ON(d->wake_count);
>>> +        WARN_ON(hrtimer_cancel(&d->timer));
>>> +        kfree(d);
>>> +    }
>>> +}
>>> -    uncore->fw_domains &= ~BIT(domain_id);
>>> +static void intel_uncore_fw_domains_fini(struct intel_uncore *uncore)
>>> +{
>>> +    struct intel_uncore_forcewake_domain *d;
>>> +    int tmp;
>>> +
>>> +    for_each_fw_domain(d, uncore, tmp)
>>> +        fw_domain_fini(uncore, d->id);
>>>   }
>>> -static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>> +static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>>   {
>>>       struct drm_i915_private *i915 = uncore->i915;
>>> +    int ret;
>>>       GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>>> +#define __fw_domain_init(id__, set__, ack__) \
>>> +    ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
>>> +    if (ret) \
>>> +        goto out_clean;
>>
>> Hidden control flow is slightly objectionable but I don't offer any 
>> nice alternatives so I guess will have to pass. Or maybe accumulate 
>> the error (err |= fw_domain_init(..)) as you go and then cleanup at 
>> the end if any failed?
> 
> I'd prefer to avoid accumulating the error since it'd just cause us to 
> having to unroll more domains when we could've stopped early.

Although unrolling happens close to never-ever so it isn't a practical 
concern..

> 
>>
>> On the other hand the idea slightly conflicts with my other suggestion 
>> to rename existing fw_domain_init to __fw_domain_init and call the 
>> macro fw_domain_init and avoid all the churn below.
>>
> 
> I'll pick this suggestion among the 2, unless there is another 
> suggestion on how to avoid the hidden goto.

.. but okay, hidden goto is also only a concern if someone adds new init 
steps before the fw_domain_init steps, oblivious that cleanup is also 
needed. Perhaps the out_clean label would be enough of a clue for that case.

> 
>>> +
>>>       if (INTEL_GEN(i915) >= 11) {
>>>           int i;
>>> -        uncore->funcs.force_wake_get =
>>> -            fw_domains_get_with_fallback;
>>> +        uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
>>>           uncore->funcs.force_wake_put = fw_domains_put;
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>>> -                   FORCEWAKE_RENDER_GEN9,
>>> -                   FORCEWAKE_ACK_RENDER_GEN9);
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
>>> -                   FORCEWAKE_BLITTER_GEN9,
>>> -                   FORCEWAKE_ACK_BLITTER_GEN9);
>>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                 FORCEWAKE_RENDER_GEN9,
>>> +                 FORCEWAKE_ACK_RENDER_GEN9);
>>> +        __fw_domain_init(FW_DOMAIN_ID_BLITTER,
>>> +                 FORCEWAKE_BLITTER_GEN9,
>>> +                 FORCEWAKE_ACK_BLITTER_GEN9);
>>> +
>>>           for (i = 0; i < I915_MAX_VCS; i++) {
>>>               if (!HAS_ENGINE(i915, _VCS(i)))
>>>                   continue;
>>> -            fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
>>> -                       FORCEWAKE_MEDIA_VDBOX_GEN11(i),
>>> -                       FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
>>> +            __fw_domain_init(FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
>>> +                     FORCEWAKE_MEDIA_VDBOX_GEN11(i),
>>> +                     FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
>>>           }
>>>           for (i = 0; i < I915_MAX_VECS; i++) {
>>>               if (!HAS_ENGINE(i915, _VECS(i)))
>>>                   continue;
>>> -            fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
>>> -                       FORCEWAKE_MEDIA_VEBOX_GEN11(i),
>>> -                       FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
>>> +            __fw_domain_init(FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
>>> +                     FORCEWAKE_MEDIA_VEBOX_GEN11(i),
>>> +                     FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
>>>           }
>>>       } else if (IS_GEN_RANGE(i915, 9, 10)) {
>>> -        uncore->funcs.force_wake_get =
>>> -            fw_domains_get_with_fallback;
>>> +        uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
>>>           uncore->funcs.force_wake_put = fw_domains_put;
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>>> -                   FORCEWAKE_RENDER_GEN9,
>>> -                   FORCEWAKE_ACK_RENDER_GEN9);
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_BLITTER,
>>> -                   FORCEWAKE_BLITTER_GEN9,
>>> -                   FORCEWAKE_ACK_BLITTER_GEN9);
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
>>> -                   FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
>>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                 FORCEWAKE_RENDER_GEN9,
>>> +                 FORCEWAKE_ACK_RENDER_GEN9);
>>> +        __fw_domain_init(FW_DOMAIN_ID_BLITTER,
>>> +                 FORCEWAKE_BLITTER_GEN9,
>>> +                 FORCEWAKE_ACK_BLITTER_GEN9);
>>> +        __fw_domain_init(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;
>>> -        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);
>>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                 FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
>>> +        __fw_domain_init(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;
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>>> -                   FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
>>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                 FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
>>>       } else if (IS_IVYBRIDGE(i915)) {
>>>           u32 ecobus;
>>> @@ -1437,8 +1455,8 @@ static void intel_uncore_fw_domains_init(struct 
>>> intel_uncore *uncore)
>>>           __raw_uncore_write32(uncore, FORCEWAKE, 0);
>>>           __raw_posting_read(uncore, ECOBUS);
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>>> -                   FORCEWAKE_MT, FORCEWAKE_MT_ACK);
>>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                 FORCEWAKE_MT, FORCEWAKE_MT_ACK);
>>>           spin_lock_irq(&uncore->lock);
>>>           fw_domains_get_with_thread_status(uncore, FORCEWAKE_RENDER);
>>> @@ -1449,19 +1467,27 @@ static void 
>>> intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>>           if (!(ecobus & FORCEWAKE_MT_ENABLE)) {
>>>               DRM_INFO("No MT forcewake available on Ivybridge, this 
>>> can result in issues\n");
>>>               DRM_INFO("when using vblank-synced partial screen 
>>> updates.\n");
>>> -            fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>>> -                       FORCEWAKE, FORCEWAKE_ACK);
>>> +            __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                     FORCEWAKE, FORCEWAKE_ACK);
>>>           }
>>>       } else if (IS_GEN(i915, 6)) {
>>>           uncore->funcs.force_wake_get =
>>>               fw_domains_get_with_thread_status;
>>>           uncore->funcs.force_wake_put = fw_domains_put;
>>> -        fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
>>> -                   FORCEWAKE, FORCEWAKE_ACK);
>>> +        __fw_domain_init(FW_DOMAIN_ID_RENDER,
>>> +                 FORCEWAKE, FORCEWAKE_ACK);
>>>       }
>>> +#undef __fw_domain_init
>>> +
>>>       /* All future platforms are expected to require complex power 
>>> gating */
>>>       WARN_ON(uncore->fw_domains == 0);
>>> +
>>> +    return 0;
>>> +
>>> +out_clean:
>>> +    intel_uncore_fw_domains_fini(uncore);
>>> +    return ret;
>>>   }
>>>   #define ASSIGN_FW_DOMAINS_TABLE(uncore, d) \
>>> @@ -1562,13 +1588,17 @@ static void uncore_raw_init(struct 
>>> intel_uncore *uncore)
>>>       }
>>>   }
>>> -static void uncore_forcewake_init(struct intel_uncore *uncore)
>>> +static int uncore_forcewake_init(struct intel_uncore *uncore)
>>>   {
>>>       struct drm_i915_private *i915 = uncore->i915;
>>> +    int ret;
>>>       GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>>> -    intel_uncore_fw_domains_init(uncore);
>>> +    ret = intel_uncore_fw_domains_init(uncore);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>       forcewake_early_sanitize(uncore, 0);
>>>       if (IS_GEN_RANGE(i915, 6, 7)) {
>>> @@ -1601,6 +1631,8 @@ static void uncore_forcewake_init(struct 
>>> intel_uncore *uncore)
>>>       uncore->pmic_bus_access_nb.notifier_call = 
>>> i915_pmic_bus_access_notifier;
>>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>>> +
>>> +    return 0;
>>>   }
>>>   int intel_uncore_init_mmio(struct intel_uncore *uncore)
>>> @@ -1619,10 +1651,13 @@ int intel_uncore_init_mmio(struct 
>>> intel_uncore *uncore)
>>>       uncore->unclaimed_mmio_check = 1;
>>> -    if (!intel_uncore_has_forcewake(uncore))
>>> +    if (!intel_uncore_has_forcewake(uncore)) {
>>>           uncore_raw_init(uncore);
>>> -    else
>>> -        uncore_forcewake_init(uncore);
>>> +    } else {
>>> +        ret = uncore_forcewake_init(uncore);
>>> +        if (ret)
>>> +            goto out_mmio_cleanup;
>>> +    }
>>>       /* 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);
>>> @@ -1644,6 +1679,11 @@ int intel_uncore_init_mmio(struct intel_uncore 
>>> *uncore)
>>>           DRM_DEBUG("unclaimed mmio detected on uncore init, 
>>> clearing\n");
>>>       return 0;
>>> +
>>> +out_mmio_cleanup:
>>> +    uncore_mmio_cleanup(uncore);
>>> +
>>> +    return ret;
>>>   }
>>>   /*
>>> @@ -1689,6 +1729,7 @@ void intel_uncore_fini_mmio(struct intel_uncore 
>>> *uncore)
>>>           iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>>>               &uncore->pmic_bus_access_nb);
>>>           intel_uncore_forcewake_reset(uncore);
>>> +        intel_uncore_fw_domains_fini(uncore);
>>>           iosf_mbi_punit_release();
>>>       }
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h 
>>> b/drivers/gpu/drm/i915/intel_uncore.h
>>> index 879252735bba..bbff281b880d 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>>> @@ -126,6 +126,7 @@ struct intel_uncore {
>>>       enum forcewake_domains fw_domains_saved; /* user domains saved 
>>> for S3 */
>>>       struct intel_uncore_forcewake_domain {
>>> +        struct intel_uncore *uncore;
>>>           enum forcewake_domain_id id;
>>>           enum forcewake_domains mask;
>>>           unsigned int wake_count;
>>> @@ -133,7 +134,7 @@ struct intel_uncore {
>>>           struct hrtimer timer;
>>>           u32 __iomem *reg_set;
>>>           u32 __iomem *reg_ack;
>>> -    } fw_domain[FW_DOMAIN_ID_COUNT];
>>> +    } *fw_domain[FW_DOMAIN_ID_COUNT];
>>
>> The array itself could also be made dynamic, no? To much hassle for 
>> very little gain?
>>
> 
> With the current approach we don't know how many domains we have in 
> advance, so we'd have to either add a table for that or realloc the 
> array as we increase the number of domains. Both options don't seem 
> worth the hassle IMO.

Agreed. It would also need changes in lookup so it looks even less 
appealing that it did to me yesterday. :)

Regards,

Tvrtko


> 
> Thanks,
> Daniele
> 
>>>       struct {
>>>           unsigned int count;
>>> @@ -147,18 +148,12 @@ struct intel_uncore {
>>>   /* Iterate over initialised fw domains */
>>>   #define for_each_fw_domain_masked(domain__, mask__, uncore__, tmp__) \
>>> -    for (tmp__ = (mask__); \
>>> -         tmp__ ? (domain__ = 
>>> &(uncore__)->fw_domain[__mask_next_bit(tmp__)]), 1 : 0;)
>>> +    for (tmp__ = (mask__); tmp__ ;) \
>>> +        for_each_if(domain__ = 
>>> (uncore__)->fw_domain[__mask_next_bit(tmp__)])
>>>   #define for_each_fw_domain(domain__, uncore__, tmp__) \
>>>       for_each_fw_domain_masked(domain__, (uncore__)->fw_domains, 
>>> uncore__, tmp__)
>>> -static inline struct intel_uncore *
>>> -forcewake_domain_to_uncore(const struct 
>>> intel_uncore_forcewake_domain *d)
>>> -{
>>> -    return container_of(d, struct intel_uncore, fw_domain[d->id]);
>>> -}
>>> -
>>>   static inline bool
>>>   intel_uncore_has_forcewake(const struct intel_uncore *uncore)
>>>   {
>>>
>>
>> Regards,
>>
>> Tvrtko
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Daniele Ceraolo Spurio]

<snip>

>>>   }
>>>   void intel_uncore_fini_mmio(struct intel_uncore *uncore)
>>>   {
>>> -    iosf_mbi_punit_acquire();
>>> -    iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>>> -        &uncore->pmic_bus_access_nb);
>>> -    intel_uncore_forcewake_reset(uncore);
>>> -    iosf_mbi_punit_release();
>>> +    if (intel_uncore_has_forcewake(uncore)) {
>>
>> To avoid hyphotetical obnoxious diffs in the future, like the one for 
>> intel_uncore_prune_mmio_domains above in this patch, maybe invert this 
>> to early return straight away.
> 

Just realized that I hadn't done that in the first place because there 
is a call to uncore_mmio_cleanup() below that we need to always perform 
and on platforms with forcewake it has to be done after clearing that, 
so can't return early.

Daniele

> will do.
> 
> Daniele
> 
>>
>>> +        iosf_mbi_punit_acquire();
>>> +        iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>>> +            &uncore->pmic_bus_access_nb);
>>> +        intel_uncore_forcewake_reset(uncore);
>>> +        iosf_mbi_punit_release();
>>> +    }
>>> +
>>>       uncore_mmio_cleanup(uncore);
>>>   }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx