[Intel-gfx] [PATCH] drm/i915/tgl: Program BW_BUDDY registers during display init

[Lucas De Marchi]

On Wed, Dec 04, 2019 at 12:51:59PM -0800, Matt Roper wrote:
>Gen12 can improve bandwidth efficiency by pairing up memory requests
>with similar addresses.  We need to program the BW_BUDDY1 and BW_BUDDY2
>registers according to the memory configuration during display
>initialization to take advantage of this capability.
>
>The magic numbers we program here feel like something that could
>definitely change on future platforms, so let's use a table-based
>programming scheme to make this easy to extend in the future.
>
>Bspec: 49189
>Bspec: 49218
>Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
>Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> .../drm/i915/display/intel_display_power.c    | 40 +++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h               |  8 ++++
> 2 files changed, 48 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>index ce1b64f4dd44..1d26f741b5b4 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_power.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>@@ -4781,6 +4781,42 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
> 	intel_combo_phy_uninit(dev_priv);
> }
>
>+struct buddy_page_mask {
>+	u8 num_channels;

I'd make this a word rather than introducing a hole here just to match
the type.

>+	enum intel_dram_type type;
>+	u32 page_mask;
>+};
>+
>+const struct buddy_page_mask tgl_buddy_page_masks[] = {
>+	{ 1, INTEL_DRAM_LPDDR4, 0xE },
>+	{ 1, INTEL_DRAM_DDR4,   0xF },
>+	{ 2, INTEL_DRAM_LPDDR4, 0x1C },
>+	{ 2, INTEL_DRAM_DDR4,   0x1F },
>+	{}
>+};
>+
>+static void tgl_arbiter_init(struct drm_i915_private *dev_priv)

can we call this something related to bw_buddy? Like
tgl_bw_buddy_init() or tgl_bw_buddy_arbiter_init()

>+{
>+	enum intel_dram_type type = dev_priv->dram_info.type;
>+	u8 num_channels = dev_priv->dram_info.num_channels;
>+	const struct buddy_page_mask *table = tgl_buddy_page_masks;
>+	int i;
>+
>+	for (i = 0; table[i].page_mask != 0; i++)
>+		if (table[i].num_channels == num_channels &&
>+		    table[i].type == type)
>+			break;
>+
>+	if (table[i].page_mask == 0) {
>+		DRM_DEBUG_DRIVER("Unknown memory configuration; disabling address buddy logic.\n");
>+		I915_WRITE(BW_BUDDY1_CTL, BW_BUDDY_DISABLE);
>+		I915_WRITE(BW_BUDDY2_CTL, BW_BUDDY_DISABLE);
>+	} else {
>+		I915_WRITE(BW_BUDDY1_PAGE_MASK, table[i].page_mask);
>+		I915_WRITE(BW_BUDDY2_PAGE_MASK, table[i].page_mask);
>+	}
>+}
>+
> static void icl_display_core_init(struct drm_i915_private *dev_priv,
> 				  bool resume)
> {
>@@ -4813,6 +4849,10 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
> 	/* 6. Setup MBUS. */
> 	icl_mbus_init(dev_priv);
>
>+	/* 7. Program arbiter BW_BUDDY registers */
>+	if (INTEL_GEN(dev_priv) >= 12)
>+		tgl_arbiter_init(dev_priv);

looks sane, but watch out for the WA here.

thanks
Lucas De Marchi

>+
> 	if (resume && dev_priv->csr.dmc_payload)
> 		intel_csr_load_program(dev_priv);
> }
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 1a6376a97d48..082190c2dc48 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -7765,6 +7765,14 @@ enum {
> #define GEN7_MSG_CTL	_MMIO(0x45010)
> #define  WAIT_FOR_PCH_RESET_ACK		(1 << 1)
> #define  WAIT_FOR_PCH_FLR_ACK		(1 << 0)
>+
>+#define BW_BUDDY1_CTL			_MMIO(0x45140)
>+#define BW_BUDDY2_CTL			_MMIO(0x45150)
>+#define   BW_BUDDY_DISABLE		REG_BIT(31)
>+
>+#define BW_BUDDY1_PAGE_MASK		_MMIO(0x45144)
>+#define BW_BUDDY2_PAGE_MASK		_MMIO(0x45154)
>+
> #define HSW_NDE_RSTWRN_OPT	_MMIO(0x46408)
> #define  RESET_PCH_HANDSHAKE_ENABLE	(1 << 4)
>
>-- 
>2.23.0
>