drm/i915: Disable decoupled mmio for GEN9LP

From: Kai Chen <[email protected]>

The decoupled mmio feature doesn't work as intended by HW team. Enabling it with forcewake will only make debugging efforts more difficult, so let's just simply remove it.

v2:
- Remove dead code related to GEN9LP decoupled mmio.
- Change backgrounds: In theory, decoupled mmio should require less cycles for single read/write operation by avoiding frequent software forcewake. However, it turns out this design not to be true on HW practice and not to provide any decoupling benefit. It also introduces problems which cause failures in intel-gpu-tools (gem), and also cause driver code and debugging more complex.
- This change therefore reverts:

commit 85ee17ebeedd1af0dccd98f82ab4e644e29d84c0
Author: Praveen Paneri <[email protected]>
Date: Tue, 15 Nov 2016 22:49:20 +0530

drm/i915/bxt: Broxton decoupled MMIO

coomit a3f79ca63b9bcf5a527b886953092bfd65e78940
Author: Ander Conselvan de Oliveira <[email protected]>
Date: Thu, 24 Nov 2016 15:23:27 +0200

drm/i915: Don't sanitize has_decoupled_mmio if platform is not broxton

Signed-off-by Kai Chen <[email protected]>

---
 drivers/gpu/drm/i915/i915_pci.c     |   1 -
 drivers/gpu/drm/i915/i915_reg.h     |   7 --
 drivers/gpu/drm/i915/intel_uncore.c | 126 ------------------------------------
 3 files changed, 134 deletions(-)

drivers/gpu/drm/i915/i915_pci.c +0 -1

index f80db2c..cf43dc1 100644
@@ -385,7 +385,6 @@ static const struct intel_device_info intel_skylake_gt3_info = {
 	.has_gmbus_irq = 1, \
 	.has_logical_ring_contexts = 1, \
 	.has_guc = 1, \
-	.has_decoupled_mmio = 1, \
 	.has_aliasing_ppgtt = 1, \
 	.has_full_ppgtt = 1, \
 	.has_full_48bit_ppgtt = 1, \

drivers/gpu/drm/i915/i915_reg.h +0 -7

index ee144ec..78872f9 100644
@@ -7792,13 +7792,6 @@ enum {
 #define  SKL_FUSE_PG1_DIST_STATUS              (1<<26)
 #define  SKL_FUSE_PG2_DIST_STATUS              (1<<25)
 
-/* Decoupled MMIO register pair for kernel driver */
-#define GEN9_DECOUPLED_REG0_DW0			_MMIO(0xF00)
-#define GEN9_DECOUPLED_REG0_DW1			_MMIO(0xF04)
-#define GEN9_DECOUPLED_DW1_GO			(1<<31)
-#define GEN9_DECOUPLED_PD_SHIFT			28
-#define GEN9_DECOUPLED_OP_SHIFT			24
-
 /* Per-pipe DDI Function Control */
 #define _TRANS_DDI_FUNC_CTL_A		0x60400
 #define _TRANS_DDI_FUNC_CTL_B		0x61400

drivers/gpu/drm/i915/intel_uncore.c +0 -119

index a9a6933..3901800 100644
@@ -400,8 +400,6 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
 static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 					  bool restore_forcewake)
 {
-	struct intel_device_info *info = mkwrite_device_info(dev_priv);
-
 	/* clear out unclaimed reg detection bit */
 	if (check_for_unclaimed_mmio(dev_priv))
 		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");

@@ -414,9 +412,6 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 				   GT_FIFO_CTL_RC6_POLICY_STALL);
 	}
 
-	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
-		info->has_decoupled_mmio = false;
-
 	intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
 }
 

@@ -801,78 +796,6 @@ unclaimed_reg_debug(struct drm_i915_private *dev_priv,
 	__unclaimed_reg_debug(dev_priv, reg, read, before);
 }
 
-enum decoupled_power_domain {
-	GEN9_DECOUPLED_PD_BLITTER = 0,
-	GEN9_DECOUPLED_PD_RENDER,
-	GEN9_DECOUPLED_PD_MEDIA,
-	GEN9_DECOUPLED_PD_ALL
-};
-
-enum decoupled_ops {
-	GEN9_DECOUPLED_OP_WRITE = 0,
-	GEN9_DECOUPLED_OP_READ
-};
-
-static const enum decoupled_power_domain fw2dpd_domain[] = {
-	GEN9_DECOUPLED_PD_RENDER,
-	GEN9_DECOUPLED_PD_BLITTER,
-	GEN9_DECOUPLED_PD_ALL,
-	GEN9_DECOUPLED_PD_MEDIA,
-	GEN9_DECOUPLED_PD_ALL,
-	GEN9_DECOUPLED_PD_ALL,
-	GEN9_DECOUPLED_PD_ALL
-};
-
-/*
- * Decoupled MMIO access for only 1 DWORD
- */
-static void __gen9_decoupled_mmio_access(struct drm_i915_private *dev_priv,
-					 u32 reg,
-					 enum forcewake_domains fw_domain,
-					 enum decoupled_ops operation)
-{
-	enum decoupled_power_domain dp_domain;
-	u32 ctrl_reg_data = 0;
-
-	dp_domain = fw2dpd_domain[fw_domain - 1];
-
-	ctrl_reg_data |= reg;
-	ctrl_reg_data |= (operation << GEN9_DECOUPLED_OP_SHIFT);
-	ctrl_reg_data |= (dp_domain << GEN9_DECOUPLED_PD_SHIFT);
-	ctrl_reg_data |= GEN9_DECOUPLED_DW1_GO;
-	__raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW1, ctrl_reg_data);
-
-	if (wait_for_atomic((__raw_i915_read32(dev_priv,
-			    GEN9_DECOUPLED_REG0_DW1) &
-			    GEN9_DECOUPLED_DW1_GO) == 0,
-			    FORCEWAKE_ACK_TIMEOUT_MS))
-		DRM_ERROR("Decoupled MMIO wait timed out\n");
-}
-
-static inline u32
-__gen9_decoupled_mmio_read32(struct drm_i915_private *dev_priv,
-			     u32 reg,
-			     enum forcewake_domains fw_domain)
-{
-	__gen9_decoupled_mmio_access(dev_priv, reg, fw_domain,
-				     GEN9_DECOUPLED_OP_READ);
-
-	return __raw_i915_read32(dev_priv, GEN9_DECOUPLED_REG0_DW0);
-}
-
-static inline void
-__gen9_decoupled_mmio_write(struct drm_i915_private *dev_priv,
-			    u32 reg, u32 data,
-			    enum forcewake_domains fw_domain)
-{
-
-	__raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW0, data);
-
-	__gen9_decoupled_mmio_access(dev_priv, reg, fw_domain,
-				     GEN9_DECOUPLED_OP_WRITE);
-}
-
-
 #define GEN2_READ_HEADER(x) \
 	u##x val = 0; \
 	assert_rpm_wakelock_held(dev_priv);

@@ -969,28 +892,6 @@ func##_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) {
 #define __gen6_read(x) __gen_read(gen6, x)
 #define __fwtable_read(x) __gen_read(fwtable, x)
 
-#define __gen9_decoupled_read(x) \
-static u##x \
-gen9_decoupled_read##x(struct drm_i915_private *dev_priv, \
-		       i915_reg_t reg, bool trace) { \
-	enum forcewake_domains fw_engine; \
-	GEN6_READ_HEADER(x); \
-	fw_engine = __fwtable_reg_read_fw_domains(offset); \
-	if (fw_engine & ~dev_priv->uncore.fw_domains_active) { \
-		unsigned i; \
-		u32 *ptr_data = (u32 *) &val; \
-		for (i = 0; i < x/32; i++, offset += sizeof(u32), ptr_data++) \
-			*ptr_data = __gen9_decoupled_mmio_read32(dev_priv, \
-								 offset, \
-								 fw_engine); \
-	} else { \
-		val = __raw_i915_read##x(dev_priv, reg); \
-	} \
-	GEN6_READ_FOOTER; \
-}
-
-__gen9_decoupled_read(32)
-__gen9_decoupled_read(64)
 __fwtable_read(8)
 __fwtable_read(16)
 __fwtable_read(32)

@@ -1077,25 +978,6 @@ func##_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, boo
 #define __gen8_write(x) __gen_write(gen8, x)
 #define __fwtable_write(x) __gen_write(fwtable, x)
 
-#define __gen9_decoupled_write(x) \
-static void \
-gen9_decoupled_write##x(struct drm_i915_private *dev_priv, \
-			i915_reg_t reg, u##x val, \
-		bool trace) { \
-	enum forcewake_domains fw_engine; \
-	GEN6_WRITE_HEADER; \
-	fw_engine = __fwtable_reg_write_fw_domains(offset); \
-	if (fw_engine & ~dev_priv->uncore.fw_domains_active) \
-		__gen9_decoupled_mmio_write(dev_priv, \
-					    offset, \
-					    val, \
-					    fw_engine); \
-	else \
-		__raw_i915_write##x(dev_priv, reg, val); \
-	GEN6_WRITE_FOOTER; \
-}
-
-__gen9_decoupled_write(32)
 __fwtable_write(8)
 __fwtable_write(16)
 __fwtable_write(32)

@@ -1332,14 +1214,6 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 		ASSIGN_FW_DOMAINS_TABLE(__gen9_fw_ranges);
 		ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(dev_priv, fwtable);
-		if (HAS_DECOUPLED_MMIO(dev_priv)) {

Quoted 22 lines · On 17/05/2017 02:07, [email protected] wrote:

From: Kai Chen <[email protected]>

The decoupled mmio feature doesn't work as intended by HW team. Enabling it with forcewake will only make debugging efforts more difficult, so let's just simply remove it.

v2:
- Remove dead code related to GEN9LP decoupled mmio.
- Change backgrounds: In theory, decoupled mmio should require less cycles for single read/write operation by avoiding frequent software forcewake. However, it turns out this design not to be true on HW practice and not to provide any decoupling benefit. It also introduces problems which cause failures in intel-gpu-tools (gem), and also cause driver code and debugging more complex.
- This change therefore reverts:

commit 85ee17ebeedd1af0dccd98f82ab4e644e29d84c0
Author: Praveen Paneri <[email protected]>
Date: Tue, 15 Nov 2016 22:49:20 +0530

drm/i915/bxt: Broxton decoupled MMIO

Quoted 5 lines

coomit a3f79ca63b9bcf5a527b886953092bfd65e78940
Author: Ander Conselvan de Oliveira <[email protected]>
Date: Thu, 24 Nov 2016 15:23:27 +0200

drm/i915: Don't sanitize has_decoupled_mmio if platform is not broxton

Revert looks incomplete since it is leaving behind the field in the device info and HAS_DECOUPLED_MMIO macro.

But revert in principle is fine by me.

Regards,

Tvrtko

Quoted 200 lines

Signed-off-by Kai Chen <[email protected]>

---
 drivers/gpu/drm/i915/i915_pci.c     |   1 -
 drivers/gpu/drm/i915/i915_reg.h     |   7 --
 drivers/gpu/drm/i915/intel_uncore.c | 126 ------------------------------------
 3 files changed, 134 deletions(-)

drivers/gpu/drm/i915/i915_pci.c +0 -1

index f80db2c..cf43dc1 100644
@@ -385,7 +385,6 @@ static const struct intel_device_info intel_skylake_gt3_info = {
 	.has_gmbus_irq = 1, \
 	.has_logical_ring_contexts = 1, \
 	.has_guc = 1, \
-	.has_decoupled_mmio = 1, \
 	.has_aliasing_ppgtt = 1, \
 	.has_full_ppgtt = 1, \
 	.has_full_48bit_ppgtt = 1, \

drivers/gpu/drm/i915/i915_reg.h +0 -7

index ee144ec..78872f9 100644
@@ -7792,13 +7792,6 @@ enum {
 #define  SKL_FUSE_PG1_DIST_STATUS              (1<<26)
 #define  SKL_FUSE_PG2_DIST_STATUS              (1<<25)
 
-/* Decoupled MMIO register pair for kernel driver */
-#define GEN9_DECOUPLED_REG0_DW0			_MMIO(0xF00)
-#define GEN9_DECOUPLED_REG0_DW1			_MMIO(0xF04)
-#define GEN9_DECOUPLED_DW1_GO			(1<<31)
-#define GEN9_DECOUPLED_PD_SHIFT			28
-#define GEN9_DECOUPLED_OP_SHIFT			24
-
 /* Per-pipe DDI Function Control */
 #define _TRANS_DDI_FUNC_CTL_A		0x60400
 #define _TRANS_DDI_FUNC_CTL_B		0x61400

drivers/gpu/drm/i915/intel_uncore.c +0 -119

index a9a6933..3901800 100644
@@ -400,8 +400,6 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
 static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 					  bool restore_forcewake)
 {
-	struct intel_device_info *info = mkwrite_device_info(dev_priv);
-
 	/* clear out unclaimed reg detection bit */
 	if (check_for_unclaimed_mmio(dev_priv))
 		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");

@@ -414,9 +412,6 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 				   GT_FIFO_CTL_RC6_POLICY_STALL);
 	}
 
-	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
-		info->has_decoupled_mmio = false;
-
 	intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
 }
 

@@ -801,78 +796,6 @@ unclaimed_reg_debug(struct drm_i915_private *dev_priv,
 	__unclaimed_reg_debug(dev_priv, reg, read, before);
 }
 
-enum decoupled_power_domain {
-	GEN9_DECOUPLED_PD_BLITTER = 0,
-	GEN9_DECOUPLED_PD_RENDER,
-	GEN9_DECOUPLED_PD_MEDIA,
-	GEN9_DECOUPLED_PD_ALL
-};
-
-enum decoupled_ops {
-	GEN9_DECOUPLED_OP_WRITE = 0,
-	GEN9_DECOUPLED_OP_READ
-};
-
-static const enum decoupled_power_domain fw2dpd_domain[] = {
-	GEN9_DECOUPLED_PD_RENDER,
-	GEN9_DECOUPLED_PD_BLITTER,
-	GEN9_DECOUPLED_PD_ALL,
-	GEN9_DECOUPLED_PD_MEDIA,
-	GEN9_DECOUPLED_PD_ALL,
-	GEN9_DECOUPLED_PD_ALL,
-	GEN9_DECOUPLED_PD_ALL
-};
-
-/*
- * Decoupled MMIO access for only 1 DWORD
- */
-static void __gen9_decoupled_mmio_access(struct drm_i915_private *dev_priv,
-					 u32 reg,
-					 enum forcewake_domains fw_domain,
-					 enum decoupled_ops operation)
-{
-	enum decoupled_power_domain dp_domain;
-	u32 ctrl_reg_data = 0;
-
-	dp_domain = fw2dpd_domain[fw_domain - 1];
-
-	ctrl_reg_data |= reg;
-	ctrl_reg_data |= (operation << GEN9_DECOUPLED_OP_SHIFT);
-	ctrl_reg_data |= (dp_domain << GEN9_DECOUPLED_PD_SHIFT);
-	ctrl_reg_data |= GEN9_DECOUPLED_DW1_GO;
-	__raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW1, ctrl_reg_data);
-
-	if (wait_for_atomic((__raw_i915_read32(dev_priv,
-			    GEN9_DECOUPLED_REG0_DW1) &
-			    GEN9_DECOUPLED_DW1_GO) == 0,
-			    FORCEWAKE_ACK_TIMEOUT_MS))
-		DRM_ERROR("Decoupled MMIO wait timed out\n");
-}
-
-static inline u32
-__gen9_decoupled_mmio_read32(struct drm_i915_private *dev_priv,
-			     u32 reg,
-			     enum forcewake_domains fw_domain)
-{
-	__gen9_decoupled_mmio_access(dev_priv, reg, fw_domain,
-				     GEN9_DECOUPLED_OP_READ);
-
-	return __raw_i915_read32(dev_priv, GEN9_DECOUPLED_REG0_DW0);
-}
-
-static inline void
-__gen9_decoupled_mmio_write(struct drm_i915_private *dev_priv,
-			    u32 reg, u32 data,
-			    enum forcewake_domains fw_domain)
-{
-
-	__raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW0, data);
-
-	__gen9_decoupled_mmio_access(dev_priv, reg, fw_domain,
-				     GEN9_DECOUPLED_OP_WRITE);
-}
-
-
 #define GEN2_READ_HEADER(x) \
 	u##x val = 0; \
 	assert_rpm_wakelock_held(dev_priv);

@@ -969,28 +892,6 @@ func##_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) {
 #define __gen6_read(x) __gen_read(gen6, x)
 #define __fwtable_read(x) __gen_read(fwtable, x)
 
-#define __gen9_decoupled_read(x) \
-static u##x \
-gen9_decoupled_read##x(struct drm_i915_private *dev_priv, \
-		       i915_reg_t reg, bool trace) { \
-	enum forcewake_domains fw_engine; \
-	GEN6_READ_HEADER(x); \
-	fw_engine = __fwtable_reg_read_fw_domains(offset); \
-	if (fw_engine & ~dev_priv->uncore.fw_domains_active) { \
-		unsigned i; \
-		u32 *ptr_data = (u32 *) &val; \
-		for (i = 0; i < x/32; i++, offset += sizeof(u32), ptr_data++) \
-			*ptr_data = __gen9_decoupled_mmio_read32(dev_priv, \
-								 offset, \
-								 fw_engine); \
-	} else { \
-		val = __raw_i915_read##x(dev_priv, reg); \
-	} \
-	GEN6_READ_FOOTER; \
-}
-
-__gen9_decoupled_read(32)
-__gen9_decoupled_read(64)
 __fwtable_read(8)
 __fwtable_read(16)
 __fwtable_read(32)

@@ -1077,25 +978,6 @@ func##_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, boo
 #define __gen8_write(x) __gen_write(gen8, x)
 #define __fwtable_write(x) __gen_write(fwtable, x)
 
-#define __gen9_decoupled_write(x) \
-static void \
-gen9_decoupled_write##x(struct drm_i915_private *dev_priv, \
-			i915_reg_t reg, u##x val, \
-		bool trace) { \
-	enum forcewake_domains fw_engine; \
-	GEN6_WRITE_HEADER; \
-	fw_engine = __fwtable_reg_write_fw_domains(offset); \
-	if (fw_engine & ~dev_priv->uncore.fw_domains_active) \
-		__gen9_decoupled_mmio_write(dev_priv, \
-					    offset, \
-					    val, \
-					    fw_engine); \
-	else \
-		__raw_i915_write##x(dev_priv, reg, val); \
-	GEN6_WRITE_FOOTER; \
-}
-
-__gen9_decoupled_write(32)
 __fwtable_write(8)
 __fwtable_write(16)
 __fwtable_write(32)

@@ -1332,14 +1214,6 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 		ASSIGN_FW_DOMAINS_TABLE(__gen9_fw_ranges);
 		ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, fwtable);
 		ASSIGN_READ_MMIO_VFUNCS(dev_priv, fwtable);
-		if (HAS_DECOUPLED_MMIO(dev_priv)) {

drivers/gpu/drm/i915/intel_uncore.c void intel_uncore_init(struct drm_i915_private *dev_priv) · line 1219

Quoted 10 lines

drivers/gpu/drm/i915/intel_uncore.c +0 -7

-			dev_priv->uncore.funcs.mmio_readl =
-						gen9_decoupled_read32;
-			dev_priv->uncore.funcs.mmio_readq =
-						gen9_decoupled_read64;
-			dev_priv->uncore.funcs.mmio_writel =
-						gen9_decoupled_write32;
-		}
 	}
 
 	iosf_mbi_register_pmic_bus_access_notifier(

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx