| Message ID | 20200603211529.3005059-3-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remaining RKL patches | expand |
On Wed, Jun 03, 2020 at 02:15:16PM -0700, Matt Roper wrote: > RKL uses the same BW_BUDDY programming table as TGL, but programs the > values into a single set BUDDY0 set of registers rather than the > BUDDY1/BUDDY2 sets used by TGL. > > Bspec: 49218 > Cc: Aditya Swarup <aditya.swarup@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- > drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- > 2 files changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index 72312b67b57a..2c1ce50b572b 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > 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; > - int i; > + int config, min_buddy, max_buddy, i; > > if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) > /* Wa_1409767108: tgl */ > @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > else > table = tgl_buddy_page_masks; > > - for (i = 0; table[i].page_mask != 0; i++) > - if (table[i].num_channels == num_channels && > - table[i].type == type) > + if (IS_ROCKETLAKE(dev_priv)) { > + min_buddy = max_buddy = 0; > + } else { > + min_buddy = 1; > + max_buddy = 2; > + } > + > + for (config = 0; table[config].page_mask != 0; config++) > + if (table[config].num_channels == num_channels && > + table[config].type == type) > break; > > - if (table[i].page_mask == 0) { > + if (table[config].page_mask == 0) { > drm_dbg(&dev_priv->drm, > "Unknown memory configuration; disabling address buddy logic.\n"); > - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); > - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); > + for (i = min_buddy; i <= max_buddy; i++) > + intel_de_write(dev_priv, BW_BUDDY_CTL(i), > + BW_BUDDY_DISABLE); > } else { > - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, > - table[i].page_mask); > - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, > - table[i].page_mask); > - > - /* Wa_22010178259:tgl */ > - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, > - BW_BUDDY_TLB_REQ_TIMER_MASK, > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, > - BW_BUDDY_TLB_REQ_TIMER_MASK, > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > + for (i = min_buddy; i <= max_buddy; i++) { > + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), > + table[config].page_mask); > + > + /* Wa_22010178259:tgl,rkl */ > + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), > + BW_BUDDY_TLB_REQ_TIMER_MASK, > + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, > + 0x8)); We should be using REG_FIELD_PREP() in i915_reg.h to declare TLB_REQ_TIMER value and then use the value here. > + } > } > } > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 578cfe11cbb9..3e79cefc510a 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7837,13 +7837,19 @@ enum { > #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_BUDDY0_CTL 0x45130 > +#define _BW_BUDDY1_CTL 0x45140 > +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ > + _BW_BUDDY0_CTL, \ > + _BW_BUDDY1_CTL)) > #define BW_BUDDY_DISABLE REG_BIT(31) > #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) > > -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) > -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) > +#define _BW_BUDDY0_PAGE_MASK 0x45134 > +#define _BW_BUDDY1_PAGE_MASK 0x45144 > +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ > + _BW_BUDDY0_PAGE_MASK, \ > + _BW_BUDDY1_PAGE_MASK)) > > #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) > #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4) > -- > 2.24.1 >
On Wed, Jun 03, 2020 at 03:34:32PM -0700, Aditya Swarup wrote: > On Wed, Jun 03, 2020 at 02:15:16PM -0700, Matt Roper wrote: > > RKL uses the same BW_BUDDY programming table as TGL, but programs the > > values into a single set BUDDY0 set of registers rather than the > > BUDDY1/BUDDY2 sets used by TGL. > > > > Bspec: 49218 > > Cc: Aditya Swarup <aditya.swarup@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- > > drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- > > 2 files changed, 35 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 72312b67b57a..2c1ce50b572b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > 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; > > - int i; > > + int config, min_buddy, max_buddy, i; > > > > if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) > > /* Wa_1409767108: tgl */ > > @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > else > > table = tgl_buddy_page_masks; > > > > - for (i = 0; table[i].page_mask != 0; i++) > > - if (table[i].num_channels == num_channels && > > - table[i].type == type) > > + if (IS_ROCKETLAKE(dev_priv)) { > > + min_buddy = max_buddy = 0; > > + } else { > > + min_buddy = 1; > > + max_buddy = 2; > > + } > > + > > + for (config = 0; table[config].page_mask != 0; config++) > > + if (table[config].num_channels == num_channels && > > + table[config].type == type) > > break; > > > > - if (table[i].page_mask == 0) { > > + if (table[config].page_mask == 0) { > > drm_dbg(&dev_priv->drm, > > "Unknown memory configuration; disabling address buddy logic.\n"); > > - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); > > - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); > > + for (i = min_buddy; i <= max_buddy; i++) > > + intel_de_write(dev_priv, BW_BUDDY_CTL(i), > > + BW_BUDDY_DISABLE); > > } else { > > - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, > > - table[i].page_mask); > > - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, > > - table[i].page_mask); > > - > > - /* Wa_22010178259:tgl */ > > - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > + for (i = min_buddy; i <= max_buddy; i++) { > > + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), > > + table[config].page_mask); > > + > > + /* Wa_22010178259:tgl,rkl */ > > + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), > > + BW_BUDDY_TLB_REQ_TIMER_MASK, > > + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, > > + 0x8)); > We should be using REG_FIELD_PREP() in i915_reg.h to declare > TLB_REQ_TIMER value and then use the value here. Any specific reason why? The value "8" doesn't have any specific hardware meaning that would be meaningful to define in the general register definitions. It's just a value that this specific hardware workaround asked for in this case. I'm not sure if we want to spread the definition of the workaround into the register file if the value isn't going to be meaningful to other driver programming or workarounds. Matt > > + } > > } > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 578cfe11cbb9..3e79cefc510a 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7837,13 +7837,19 @@ enum { > > #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_BUDDY0_CTL 0x45130 > > +#define _BW_BUDDY1_CTL 0x45140 > > +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ > > + _BW_BUDDY0_CTL, \ > > + _BW_BUDDY1_CTL)) > > #define BW_BUDDY_DISABLE REG_BIT(31) > > #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) > > > > -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) > > -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) > > +#define _BW_BUDDY0_PAGE_MASK 0x45134 > > +#define _BW_BUDDY1_PAGE_MASK 0x45144 > > +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ > > + _BW_BUDDY0_PAGE_MASK, \ > > + _BW_BUDDY1_PAGE_MASK)) > > > > #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) > > #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4) > > -- > > 2.24.1 > >
On Wed, Jun 03, 2020 at 04:12:59PM -0700, Matt Roper wrote: > On Wed, Jun 03, 2020 at 03:34:32PM -0700, Aditya Swarup wrote: > > On Wed, Jun 03, 2020 at 02:15:16PM -0700, Matt Roper wrote: > > > RKL uses the same BW_BUDDY programming table as TGL, but programs the > > > values into a single set BUDDY0 set of registers rather than the > > > BUDDY1/BUDDY2 sets used by TGL. > > > > > > Bspec: 49218 > > > Cc: Aditya Swarup <aditya.swarup@intel.com> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- > > > drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- > > > 2 files changed, 35 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 72312b67b57a..2c1ce50b572b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > > 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; > > > - int i; > > > + int config, min_buddy, max_buddy, i; > > > > > > if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) > > > /* Wa_1409767108: tgl */ > > > @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > > else > > > table = tgl_buddy_page_masks; > > > > > > - for (i = 0; table[i].page_mask != 0; i++) > > > - if (table[i].num_channels == num_channels && > > > - table[i].type == type) > > > + if (IS_ROCKETLAKE(dev_priv)) { > > > + min_buddy = max_buddy = 0; > > > + } else { > > > + min_buddy = 1; > > > + max_buddy = 2; > > > + } > > > + > > > + for (config = 0; table[config].page_mask != 0; config++) > > > + if (table[config].num_channels == num_channels && > > > + table[config].type == type) > > > break; > > > > > > - if (table[i].page_mask == 0) { > > > + if (table[config].page_mask == 0) { > > > drm_dbg(&dev_priv->drm, > > > "Unknown memory configuration; disabling address buddy logic.\n"); > > > - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); > > > - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); > > > + for (i = min_buddy; i <= max_buddy; i++) > > > + intel_de_write(dev_priv, BW_BUDDY_CTL(i), > > > + BW_BUDDY_DISABLE); > > > } else { > > > - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, > > > - table[i].page_mask); > > > - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, > > > - table[i].page_mask); > > > - > > > - /* Wa_22010178259:tgl */ > > > - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, > > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > > - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, > > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > > + for (i = min_buddy; i <= max_buddy; i++) { > > > + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), > > > + table[config].page_mask); > > > + > > > + /* Wa_22010178259:tgl,rkl */ > > > + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), > > > + BW_BUDDY_TLB_REQ_TIMER_MASK, > > > + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, > > > + 0x8)); > > We should be using REG_FIELD_PREP() in i915_reg.h to declare > > TLB_REQ_TIMER value and then use the value here. > > Any specific reason why? The value "8" doesn't have any specific > hardware meaning that would be meaningful to define in the general > register definitions. It's just a value that this specific hardware > workaround asked for in this case. I'm not sure if we want to spread > the definition of the workaround into the register file if the value > isn't going to be meaningful to other driver programming or workarounds. > > > Matt The value 8 is constant and as such should be defined in a header file for that bitmask. If there was variable used to prepare the value on the fly, I would have understood this usage. If you are concerned about 8 being a random value and not spreading to i915_reg.h, I would prefer a macro in i915_reg.h like: #define TLB_REQ_TIMER(x) REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK,(x)) Then the value doesn't filter to i915_reg.h and according to me looks cleaner. Most of the usage of REG_FIELD_PREP in *.c files is based on creating bitfields using variable. Here we are using a constant which can easily be moved to i915_reg.h. It shouldn't matter if it is a WA or not. Adi > > > > + } > > > } > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 578cfe11cbb9..3e79cefc510a 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -7837,13 +7837,19 @@ enum { > > > #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_BUDDY0_CTL 0x45130 > > > +#define _BW_BUDDY1_CTL 0x45140 > > > +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ > > > + _BW_BUDDY0_CTL, \ > > > + _BW_BUDDY1_CTL)) > > > #define BW_BUDDY_DISABLE REG_BIT(31) > > > #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) > > > > > > -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) > > > -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) > > > +#define _BW_BUDDY0_PAGE_MASK 0x45134 > > > +#define _BW_BUDDY1_PAGE_MASK 0x45144 > > > +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ > > > + _BW_BUDDY0_PAGE_MASK, \ > > > + _BW_BUDDY1_PAGE_MASK)) > > > > > > #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) > > > #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4) > > > -- > > > 2.24.1 > > > > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795
On Wed, Jun 03, 2020 at 02:15:16PM -0700, Matt Roper wrote: > RKL uses the same BW_BUDDY programming table as TGL, but programs the > values into a single set BUDDY0 set of registers rather than the > BUDDY1/BUDDY2 sets used by TGL. Maybe we just want some kind of HAS_ABOX() so we could use the same thing here and in the ABOX_CTL programming? > > Bspec: 49218 > Cc: Aditya Swarup <aditya.swarup@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- > drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- > 2 files changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index 72312b67b57a..2c1ce50b572b 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > 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; > - int i; > + int config, min_buddy, max_buddy, i; > > if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) > /* Wa_1409767108: tgl */ > @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > else > table = tgl_buddy_page_masks; > > - for (i = 0; table[i].page_mask != 0; i++) > - if (table[i].num_channels == num_channels && > - table[i].type == type) > + if (IS_ROCKETLAKE(dev_priv)) { > + min_buddy = max_buddy = 0; > + } else { > + min_buddy = 1; > + max_buddy = 2; > + } > + > + for (config = 0; table[config].page_mask != 0; config++) > + if (table[config].num_channels == num_channels && > + table[config].type == type) > break; > > - if (table[i].page_mask == 0) { > + if (table[config].page_mask == 0) { > drm_dbg(&dev_priv->drm, > "Unknown memory configuration; disabling address buddy logic.\n"); > - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); > - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); > + for (i = min_buddy; i <= max_buddy; i++) > + intel_de_write(dev_priv, BW_BUDDY_CTL(i), > + BW_BUDDY_DISABLE); > } else { > - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, > - table[i].page_mask); > - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, > - table[i].page_mask); > - > - /* Wa_22010178259:tgl */ > - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, > - BW_BUDDY_TLB_REQ_TIMER_MASK, > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, > - BW_BUDDY_TLB_REQ_TIMER_MASK, > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > + for (i = min_buddy; i <= max_buddy; i++) { > + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), > + table[config].page_mask); > + > + /* Wa_22010178259:tgl,rkl */ > + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), > + BW_BUDDY_TLB_REQ_TIMER_MASK, > + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, > + 0x8)); > + } > } > } > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 578cfe11cbb9..3e79cefc510a 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7837,13 +7837,19 @@ enum { > #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_BUDDY0_CTL 0x45130 > +#define _BW_BUDDY1_CTL 0x45140 > +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ > + _BW_BUDDY0_CTL, \ > + _BW_BUDDY1_CTL)) > #define BW_BUDDY_DISABLE REG_BIT(31) > #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) > > -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) > -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) > +#define _BW_BUDDY0_PAGE_MASK 0x45134 > +#define _BW_BUDDY1_PAGE_MASK 0x45144 > +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ > + _BW_BUDDY0_PAGE_MASK, \ > + _BW_BUDDY1_PAGE_MASK)) > > #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) > #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4) > -- > 2.24.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 04, 2020 at 08:01:57PM +0300, Ville Syrjälä wrote: > On Wed, Jun 03, 2020 at 02:15:16PM -0700, Matt Roper wrote: > > RKL uses the same BW_BUDDY programming table as TGL, but programs the > > values into a single set BUDDY0 set of registers rather than the > > BUDDY1/BUDDY2 sets used by TGL. > > Maybe we just want some kind of HAS_ABOX() so we could use the same > thing here and in the ABOX_CTL programming? Although these are both related to how the display controller accesses memory, I don't think they're quite a 1:1 mapping. TGL has MBUX_ABOX_CTL{0,1,2} (and we're directed to program all three), but only has BW_BUDDY_CTL{1,2} and no 0 instance. For now I'll just add separate bw_buddy and abox masks to our platform device info structure. Matt > > > > > Bspec: 49218 > > Cc: Aditya Swarup <aditya.swarup@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- > > drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- > > 2 files changed, 35 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 72312b67b57a..2c1ce50b572b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > 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; > > - int i; > > + int config, min_buddy, max_buddy, i; > > > > if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) > > /* Wa_1409767108: tgl */ > > @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > else > > table = tgl_buddy_page_masks; > > > > - for (i = 0; table[i].page_mask != 0; i++) > > - if (table[i].num_channels == num_channels && > > - table[i].type == type) > > + if (IS_ROCKETLAKE(dev_priv)) { > > + min_buddy = max_buddy = 0; > > + } else { > > + min_buddy = 1; > > + max_buddy = 2; > > + } > > + > > + for (config = 0; table[config].page_mask != 0; config++) > > + if (table[config].num_channels == num_channels && > > + table[config].type == type) > > break; > > > > - if (table[i].page_mask == 0) { > > + if (table[config].page_mask == 0) { > > drm_dbg(&dev_priv->drm, > > "Unknown memory configuration; disabling address buddy logic.\n"); > > - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); > > - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); > > + for (i = min_buddy; i <= max_buddy; i++) > > + intel_de_write(dev_priv, BW_BUDDY_CTL(i), > > + BW_BUDDY_DISABLE); > > } else { > > - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, > > - table[i].page_mask); > > - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, > > - table[i].page_mask); > > - > > - /* Wa_22010178259:tgl */ > > - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > + for (i = min_buddy; i <= max_buddy; i++) { > > + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), > > + table[config].page_mask); > > + > > + /* Wa_22010178259:tgl,rkl */ > > + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), > > + BW_BUDDY_TLB_REQ_TIMER_MASK, > > + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, > > + 0x8)); > > + } > > } > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 578cfe11cbb9..3e79cefc510a 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7837,13 +7837,19 @@ enum { > > #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_BUDDY0_CTL 0x45130 > > +#define _BW_BUDDY1_CTL 0x45140 > > +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ > > + _BW_BUDDY0_CTL, \ > > + _BW_BUDDY1_CTL)) > > #define BW_BUDDY_DISABLE REG_BIT(31) > > #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) > > > > -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) > > -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) > > +#define _BW_BUDDY0_PAGE_MASK 0x45134 > > +#define _BW_BUDDY1_PAGE_MASK 0x45144 > > +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ > > + _BW_BUDDY0_PAGE_MASK, \ > > + _BW_BUDDY1_PAGE_MASK)) > > > > #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) > > #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4) > > -- > > 2.24.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Thu, Jun 04, 2020 at 03:12:40PM -0700, Matt Roper wrote: > On Thu, Jun 04, 2020 at 08:01:57PM +0300, Ville Syrjälä wrote: > > On Wed, Jun 03, 2020 at 02:15:16PM -0700, Matt Roper wrote: > > > RKL uses the same BW_BUDDY programming table as TGL, but programs the > > > values into a single set BUDDY0 set of registers rather than the > > > BUDDY1/BUDDY2 sets used by TGL. > > > > Maybe we just want some kind of HAS_ABOX() so we could use the same > > thing here and in the ABOX_CTL programming? > > Although these are both related to how the display controller accesses > memory, I don't think they're quite a 1:1 mapping. TGL has > MBUX_ABOX_CTL{0,1,2} (and we're directed to program all three), but only > has BW_BUDDY_CTL{1,2} and no 0 instance. I see three different notes on this: tgl style: "Abox1 and Abox2 are used for pixel data reads. Program the BW_BUDDY1 and BW_BUDDY2 registers." abox0 is the legacy path, so doesn't need this stuff apparently. abox1/2 are the dedicated path. rkl style: "Abox0 is used for pixel data reads. Program the BW_BUDDY0 registers." the dedicated path is removed so I guess they had to add the buddy registers for the legacy path abox. some other style: "Abox0 and Abox1 are used for pixel data reads. Program the BW_BUDDY0 and BW_BUDDY1 registers." I presume this means the legacy path is getting nuked, and thus the two aboxes for the dedicated path are just getting shifted down. Unfortunately I can't find a hsd which tells the story. So yeah, there is a slight difference for the tgl style in that we still want to program ABOX_CTL0 but not BW_BUDDY0. > > For now I'll just add separate bw_buddy and abox masks to our platform > device info structure. Or could maybe just add an exception for tgl. Either if (HAS_ABOX(0) || IS_TGL) write(ABOX_CTL0); ... or if (HAS_ABOX(0) && !IS_TGL) write(BUDDY0); ... would seem reasonable enough to me. > > > Matt > > > > > > > > > Bspec: 49218 > > > Cc: Aditya Swarup <aditya.swarup@intel.com> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- > > > drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- > > > 2 files changed, 35 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 72312b67b57a..2c1ce50b572b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > > 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; > > > - int i; > > > + int config, min_buddy, max_buddy, i; > > > > > > if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) > > > /* Wa_1409767108: tgl */ > > > @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) > > > else > > > table = tgl_buddy_page_masks; > > > > > > - for (i = 0; table[i].page_mask != 0; i++) > > > - if (table[i].num_channels == num_channels && > > > - table[i].type == type) > > > + if (IS_ROCKETLAKE(dev_priv)) { > > > + min_buddy = max_buddy = 0; > > > + } else { > > > + min_buddy = 1; > > > + max_buddy = 2; > > > + } > > > + > > > + for (config = 0; table[config].page_mask != 0; config++) > > > + if (table[config].num_channels == num_channels && > > > + table[config].type == type) > > > break; > > > > > > - if (table[i].page_mask == 0) { > > > + if (table[config].page_mask == 0) { > > > drm_dbg(&dev_priv->drm, > > > "Unknown memory configuration; disabling address buddy logic.\n"); > > > - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); > > > - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); > > > + for (i = min_buddy; i <= max_buddy; i++) > > > + intel_de_write(dev_priv, BW_BUDDY_CTL(i), > > > + BW_BUDDY_DISABLE); > > > } else { > > > - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, > > > - table[i].page_mask); > > > - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, > > > - table[i].page_mask); > > > - > > > - /* Wa_22010178259:tgl */ > > > - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, > > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > > - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, > > > - BW_BUDDY_TLB_REQ_TIMER_MASK, > > > - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); > > > + for (i = min_buddy; i <= max_buddy; i++) { > > > + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), > > > + table[config].page_mask); > > > + > > > + /* Wa_22010178259:tgl,rkl */ > > > + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), > > > + BW_BUDDY_TLB_REQ_TIMER_MASK, > > > + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, > > > + 0x8)); > > > + } > > > } > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 578cfe11cbb9..3e79cefc510a 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -7837,13 +7837,19 @@ enum { > > > #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_BUDDY0_CTL 0x45130 > > > +#define _BW_BUDDY1_CTL 0x45140 > > > +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ > > > + _BW_BUDDY0_CTL, \ > > > + _BW_BUDDY1_CTL)) > > > #define BW_BUDDY_DISABLE REG_BIT(31) > > > #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) > > > > > > -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) > > > -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) > > > +#define _BW_BUDDY0_PAGE_MASK 0x45134 > > > +#define _BW_BUDDY1_PAGE_MASK 0x45144 > > > +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ > > > + _BW_BUDDY0_PAGE_MASK, \ > > > + _BW_BUDDY1_PAGE_MASK)) > > > > > > #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) > > > #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4) > > > -- > > > 2.24.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 72312b67b57a..2c1ce50b572b 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -5254,7 +5254,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) 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; - int i; + int config, min_buddy, max_buddy, i; if (IS_TGL_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0)) /* Wa_1409767108: tgl */ @@ -5262,29 +5262,35 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv) else table = tgl_buddy_page_masks; - for (i = 0; table[i].page_mask != 0; i++) - if (table[i].num_channels == num_channels && - table[i].type == type) + if (IS_ROCKETLAKE(dev_priv)) { + min_buddy = max_buddy = 0; + } else { + min_buddy = 1; + max_buddy = 2; + } + + for (config = 0; table[config].page_mask != 0; config++) + if (table[config].num_channels == num_channels && + table[config].type == type) break; - if (table[i].page_mask == 0) { + if (table[config].page_mask == 0) { drm_dbg(&dev_priv->drm, "Unknown memory configuration; disabling address buddy logic.\n"); - intel_de_write(dev_priv, BW_BUDDY1_CTL, BW_BUDDY_DISABLE); - intel_de_write(dev_priv, BW_BUDDY2_CTL, BW_BUDDY_DISABLE); + for (i = min_buddy; i <= max_buddy; i++) + intel_de_write(dev_priv, BW_BUDDY_CTL(i), + BW_BUDDY_DISABLE); } else { - intel_de_write(dev_priv, BW_BUDDY1_PAGE_MASK, - table[i].page_mask); - intel_de_write(dev_priv, BW_BUDDY2_PAGE_MASK, - table[i].page_mask); - - /* Wa_22010178259:tgl */ - intel_de_rmw(dev_priv, BW_BUDDY1_CTL, - BW_BUDDY_TLB_REQ_TIMER_MASK, - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); - intel_de_rmw(dev_priv, BW_BUDDY2_CTL, - BW_BUDDY_TLB_REQ_TIMER_MASK, - REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, 0x8)); + for (i = min_buddy; i <= max_buddy; i++) { + intel_de_write(dev_priv, BW_BUDDY_PAGE_MASK(i), + table[config].page_mask); + + /* Wa_22010178259:tgl,rkl */ + intel_de_rmw(dev_priv, BW_BUDDY_CTL(i), + BW_BUDDY_TLB_REQ_TIMER_MASK, + REG_FIELD_PREP(BW_BUDDY_TLB_REQ_TIMER_MASK, + 0x8)); + } } } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 578cfe11cbb9..3e79cefc510a 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7837,13 +7837,19 @@ enum { #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_BUDDY0_CTL 0x45130 +#define _BW_BUDDY1_CTL 0x45140 +#define BW_BUDDY_CTL(x) _MMIO(_PICK_EVEN(x, \ + _BW_BUDDY0_CTL, \ + _BW_BUDDY1_CTL)) #define BW_BUDDY_DISABLE REG_BIT(31) #define BW_BUDDY_TLB_REQ_TIMER_MASK REG_GENMASK(21, 16) -#define BW_BUDDY1_PAGE_MASK _MMIO(0x45144) -#define BW_BUDDY2_PAGE_MASK _MMIO(0x45154) +#define _BW_BUDDY0_PAGE_MASK 0x45134 +#define _BW_BUDDY1_PAGE_MASK 0x45144 +#define BW_BUDDY_PAGE_MASK(x) _MMIO(_PICK_EVEN(x, \ + _BW_BUDDY0_PAGE_MASK, \ + _BW_BUDDY1_PAGE_MASK)) #define HSW_NDE_RSTWRN_OPT _MMIO(0x46408) #define RESET_PCH_HANDSHAKE_ENABLE (1 << 4)
RKL uses the same BW_BUDDY programming table as TGL, but programs the values into a single set BUDDY0 set of registers rather than the BUDDY1/BUDDY2 sets used by TGL. Bspec: 49218 Cc: Aditya Swarup <aditya.swarup@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- .../drm/i915/display/intel_display_power.c | 44 +++++++++++-------- drivers/gpu/drm/i915/i915_reg.h | 14 ++++-- 2 files changed, 35 insertions(+), 23 deletions(-)