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

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Thu, Dec 05, 2019 at 07:31:15PM +0200, Ville Syrjälä wrote:
>On Thu, Dec 05, 2019 at 09:19:43AM -0800, Lucas De Marchi wrote:
>> 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@intel.com>
>> >Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> >Signed-off-by: Matt Roper <matthew.d.roper@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.
>
>I'd rather just shrink page_mask and reorder a bit to get a smaller
>struct. If only we could also make enums take less that 4 bytes.
>u8/s8 would be more than enough for most enums.

we can, unfortunately that would need to be definition rather than
declaration. This works for me:

enum __packed bla {
	...
};


But this doesn't if the enum wasn't defined as packed: 

enum __packed bla bla;

Lucas De Marchi

>
>>
>> >+	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
>> >
>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx