| Message ID | 20161201154940.24446-8-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qui, 2016-12-01 às 21:19 +0530, Mahesh Kumar escreveu: > This patch adds support to decode system memory bandwidth > which will be used for arbitrated display memory percentage > calculation in GEN9 based system. > > Changes from v1: > - Address comments from Paulo > - implement decode function for SKL/KBL also > Changes from v2: > - Rewrite the code as per HW team inputs > - Addresses review comments > Changes from v3: > - Fix compilation warning > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> As a general comment, indentation is weird on all multi-line statements. Also, most comments are missing periods and are not 80- column aligned. And a lot of the comments just say what the code already does instead of explaining why the code does what it does, so perhaps we could just remove them. Comments should explain why the code is written the way it is, not translate from C to English. > --- > drivers/gpu/drm/i915/i915_drv.c | 173 > ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 12 +++ > drivers/gpu/drm/i915/i915_reg.h | 37 +++++++++ > 3 files changed, 222 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index 1c689b6..0ac7122 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -979,6 +979,173 @@ static void intel_sanitize_options(struct > drm_i915_private *dev_priv) > DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", > yesno(i915.semaphores)); > } > > +static inline enum rank skl_memdev_get_channel_rank(uint32_t val) Why inline? This is already static, leave it up to the compiler to decide if it's better to inline or not. > +{ > + uint8_t l_rank, s_rank; > + uint8_t l_size, s_size; > + enum rank ch_rank = DRAM_RANK_SINGLE; Optional suggestion: get rid of this variable, just return the appropriate values once we discover them. > + > + l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & > SKL_DRAM_SIZE_MASK; > + s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & > SKL_DRAM_SIZE_MASK; > + l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & > SKL_DRAM_RANK_MASK; > + s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & > SKL_DRAM_RANK_MASK; Validate our assumptions: WARN_ON(l_size == 0 && s_size == 0); Or we could even do the appropriate check and return DRAM_RANK_INVALID, but then we'd have to restructure how our caller calls us, maybe moving some code to this function. > + > + /* > + * If any of the slot has dual rank memory consider > + * dual rank memory channel > + */ The comment says just what the code already says. What would be interesting to see in the comment is an explanation of why we do it the way we do. > + if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == > SKL_DRAM_RANK_DUAL) > + ch_rank = DRAM_RANK_DUAL; > > + > + /* > + * If both the slot has single rank memory then > configuration > + * is dual rank memory > + */ The comment says just what the code already says. What would be interesting to see in the comment is an explanation of why we do it the way we do. > + if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) && > + (s_size && s_rank == SKL_DRAM_RANK_SINGLE)) > + ch_rank = DRAM_RANK_DUAL; > + return ch_rank; > +} > + > +static int > +skl_get_memdev_info(struct drm_i915_private *dev_priv) > +{ > + struct memdev_info *memdev_info = &dev_priv->memdev_info; > + uint32_t mem_freq_khz; > + uint32_t val; > + enum rank ch0_rank = DRAM_RANK_INVALID, ch1_rank = > DRAM_RANK_INVALID; > + > + val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); > + mem_freq_khz = (val & SKL_REQ_DATA_MASK) * > + SKL_MEMORY_FREQ_MULTIPLIER_KHZ; Optional suggestion: perhaps extend the memory_freq_multiplier to HZ and then later cut the last 3 digits so the rounding errors get restricted to the units we'll cut? Also, if we do the math using the "4 * num / 3" we can try to reduce the rounding errors even more. I get annoyed that it says my system bandwidth is 25600032, while the correct number is exactly 25600000. This applies to the BXT code too. > + > + val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN); > + if (val != 0x0) { > + memdev_info->num_channels++; > + ch0_rank = skl_memdev_get_channel_rank(val); > + } Optional suggestion: Here's how I would have implemented this: ch0_rank = skl_memdev_get_channel_rank(0); if (ch0_rank != DRAM_RANK_INVALID) memdev_info->num_channels++; This way we'd move all the logic around this register to skl_memdev_get_channel_rank(), and we'd also be able to get rid of the initializations here. But this is just a suggestion in case you agree with me. Feel free to leave the code the way it is. > + > + val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN); > + if (val != 0x0) { > + memdev_info->num_channels++; > + ch1_rank = skl_memdev_get_channel_rank(val); > + } > + > + if (memdev_info->num_channels == 0) { > + DRM_ERROR("Number of mem channels are zero\n"); I'm not a native English speaker, but I would suppose that s/are/is/ would make the sentence correct (the number is zero). While at it, why not s/mem/memory/ too to make the code sound a little more formal? > + return -EINVAL; > + } > + > + memdev_info->bandwidth_kbps = (memdev_info->num_channels * > + mem_freq_khz > * 8); > + > + if (memdev_info->bandwidth_kbps == 0) { > + DRM_ERROR("Couldn't get system memory bandwidth\n"); > + return -EINVAL; > + } > + memdev_info->valid = true; My previous comment about ownership of this variable still applies. > + > + /* > + * If any of channel is single rank channel, Again, not a native English speaker, but "any of channel" sounds incorrect to me. But anyway, the comment only says what the code already says. Why do we do it like this? > + * consider single rank memory > + */ > + if (ch0_rank == DRAM_RANK_SINGLE || ch1_rank == > DRAM_RANK_SINGLE) > + memdev_info->rank = DRAM_RANK_SINGLE; > + else > + memdev_info->rank = max(ch0_rank, ch1_rank); I can't think of any situation where this wouldn't end with DRAM_RANK_DUAL, so we might just assign it to the variable. > + > + return 0; > +} > + > +static int > +bxt_get_memdev_info(struct drm_i915_private *dev_priv) > +{ > + struct memdev_info *memdev_info = &dev_priv->memdev_info; > + uint32_t dram_channels; > + uint32_t mem_freq_khz, val; > + uint8_t num_active_channels; > + int i; > + > + val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0); > + mem_freq_khz = ((val & BXT_REQ_DATA_MASK) * > + BXT_MEMORY_FREQ_MULTIPLIER_KHZ); > + > + dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) & > + BXT_DRAM_CHANNEL_ACTIVE_MASK > ; > + num_active_channels = hweight32(dram_channels); > + > + memdev_info->bandwidth_kbps = (mem_freq_khz * > num_active_channels * 4); > + > + if (memdev_info->bandwidth_kbps == 0) { > + DRM_ERROR("Couldn't get system memory bandwidth\n"); > + return -EINVAL; > + } > + memdev_info->valid = true; > + > + /* > + * Now read each DUNIT8/9/10/11 to check the rank of each > dimms. > + */ > + for (i = 0; i < BXT_D_CR_DRP0_DUNIT_MAX; i++) { > + val = I915_READ(BXT_D_CR_DRP0_DUNIT(i)); > + if (val != 0xFFFFFFFF) { > + uint8_t rank; > + enum rank ch_rank; > + > + memdev_info->num_channels++; > + rank = val & BXT_DRAM_RANK_MASK; Shouldn't the code here be: "if one of these two bits are enabled then we're RANK_SINGLE, if both bits are enabled then we're RANK_DUAL, otherwise invalid"? If not, why? In other words: is rank=0x2 really invalid? Isn't it just single rank? > + if (rank == BXT_DRAM_RANK_SINGLE) > + ch_rank = DRAM_RANK_SINGLE; > + else if (rank == BXT_DRAM_RANK_DUAL) > + ch_rank = DRAM_RANK_DUAL; > + else > + ch_rank = DRAM_RANK_INVALID; > + > + /* > + * If any of channel is having single rank > memory Again, "any of channel". > + * consider memory as single rank > + */ > + if (memdev_info->rank == DRAM_RANK_INVALID) > + memdev_info->rank = ch_rank; > + else if (ch_rank == DRAM_RANK_SINGLE) > + memdev_info->rank = > DRAM_RANK_SINGLE; > + } > + } SKL returns -EINVAL if we end up with DRAM_RANK_INVALID in memdev_info- >rank. I think we could do that in BXT too: either we have all the information to not apply the workaround, or we give up. > + return 0; > +} > + > +static void > +intel_get_memdev_info(struct drm_i915_private *dev_priv) > +{ > + struct memdev_info *memdev_info = &dev_priv->memdev_info; > + int ret; > + > + memdev_info->valid = false; > + memdev_info->rank = DRAM_RANK_INVALID; > + memdev_info->num_channels = 0; > + > + if (!IS_GEN9(dev_priv)) > + return; > + > + if (IS_BROXTON(dev_priv)) > + ret = bxt_get_memdev_info(dev_priv); > + else > + ret = skl_get_memdev_info(dev_priv); > + if (ret) > + return; > + > + DRM_DEBUG_DRIVER("DRAM bandwidth: %u KBps total-channels: > %u\n", > + memdev_info->bandwidth_kbps, > + memdev_info->num_channels); > + if (memdev_info->rank == DRAM_RANK_INVALID) > + DRM_INFO("Counld not get memory rank info\n"); Spelling mistake here. And since you use "Couldn't" instead of "Could not" in all the other messages, I'd keep the same pattern. But if you accept my suggestion above, we'll never print this message anyway. > + else { > + DRM_DEBUG_DRIVER("DRAM rank: %s rank\n", > + (memdev_info->rank == > DRAM_RANK_DUAL) ? > + "dual" : "single"); > + } > +} > + > + Two white spaces here. > /** > * i915_driver_init_hw - setup state requiring device access > * @dev_priv: device private > @@ -1081,6 +1248,12 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > DRM_DEBUG_DRIVER("can't enable MSI"); > } > > + /* > + * Fill the memdev structure to get the system raw bandwidth > + * This will be used by WM algorithm, to implement GEN9 > based WA > + */ > + intel_get_memdev_info(dev_priv); > + > return 0; > > out_ggtt: > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index b78dc9a..69213a4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2297,6 +2297,18 @@ struct drm_i915_private { > bool distrust_bios_wm; > } wm; > > + struct memdev_info { > + bool valid; > + uint32_t bandwidth_kbps; > + uint8_t num_channels; > + enum rank { > + DRAM_RANK_INVALID = 0, > + DRAM_RANK_SINGLE, > + DRAM_RANK_DUAL > + } rank; This was previously an anonymous enum (enum { ... } rank;), but now it's "enum rank", which is too generic for a .h file. Either use a more specific name (enum memdev_rank?) or try to make it anonymous again. > + } memdev_info; > + > + > struct i915_runtime_pm pm; > > struct { > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > index 649319d..e7efdd0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -8015,6 +8015,43 @@ enum { > #define DC_STATE_DEBUG_MASK_CORES (1<<0) > #define DC_STATE_DEBUG_MASK_MEMORY_UP (1<<1) > > +#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_S > NB + 0x7114) > +#define BXT_REQ_DATA_MASK 0x3F > +#define BXT_DRAM_ACTIVE_CHANNEL_SHIFT 12 > +#define BXT_DRAM_ACTIVE_CHANNEL_MASK 0xF We're not using these two definitions anywhere. > +/* > + * BIOS programs this field of REQ_DATA [5:0] in integer > + * multiple of 133333 KHz (133.33MHz) > + */ Now that we're using decimal on the definition, I don't think this comment is useful. > +#define BXT_MEMORY_FREQ_MULTIPLIER_KHZ 133333 We don't use tabs like that in our definitions. > +#define BXT_D_CR_DRP0_DUNIT8 0x1000 > +#define BXT_D_CR_DRP0_DUNIT9 0x1200 > +#define BXT_D_CR_DRP0_DUNIT_MAX 4 > +#define _MMIO_MCHBAR_DUNIT(x, a, b) _MMIO(MCHBAR_MIRROR_BASE_SNB + > (a) + (x)*((b)-(a))) > +#define BXT_D_CR_DRP0_DUNIT(x) _MMIO_MCHBAR_DUNIT(x, > BXT_D_CR_DRP0_DUNIT8, BXT_D_CR_DRP0_DUNIT9) But then BXT_D_CR_DRP0_DUNIT(1) means DUNIT9 instead of DUNIT1... That's very unintuitive. > +#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 > +#define BXT_DRAM_CHANNEL_ACTIVE_MASK 0xF These two definitions don't belong to the register immediately above. > +#define BXT_DRAM_RANK_MASK 0x3 > +#define BXT_DRAM_RANK_SINGLE 0x1 > +#define BXT_DRAM_RANK_DUAL 0x3 > + > +/* > + * SKL memory frequeny multiplier is 266667 KHz (266.67 MHz) s/frequeny/frequency/ Also, this is not how we do one-line comments. And now that the number is in decimal we probably don't even need the comment here since the definition is a little more obvious. > + */ > +#define SKL_MEMORY_FREQ_MULTIPLIER_KHZ 266667 We don't use tabs like that in our definitions. > +#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU _MMIO(MCHBAR_MIRROR > _BASE_SNB + 0x5E04) > +#define SKL_REQ_DATA_MASK (0xF << 0) Blank lines separating different registers would be good. > +#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIR > ROR_BASE_SNB + 0x500C) > +#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIR > ROR_BASE_SNB + 0x5010) > +#define SKL_DRAM_SIZE_MASK 0x1F 0x3F > +#define SKL_DRAM_SIZE_L_SHIFT 0 > +#define SKL_DRAM_SIZE_S_SHIFT 16 > +#define SKL_DRAM_RANK_MASK 0x1 > +#define SKL_DRAM_RANK_L_SHIFT 10 > +#define SKL_DRAM_RANK_S_SHIFT 26 > +#define SKL_DRAM_RANK_SINGLE 0x0 > +#define SKL_DRAM_RANK_DUAL 0x1 > + > /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using > this register, > * since on HSW we can't write to it using I915_WRITE. */ > #define D_COMP_HSW _MMIO(MCHBAR_MIRROR_BASE_S > NB + 0x5F0C)
Hi, On Friday 09 December 2016 05:25 AM, Paulo Zanoni wrote: > Em Qui, 2016-12-01 às 21:19 +0530, Mahesh Kumar escreveu: >> This patch adds support to decode system memory bandwidth >> which will be used for arbitrated display memory percentage >> calculation in GEN9 based system. >> >> Changes from v1: >> - Address comments from Paulo >> - implement decode function for SKL/KBL also >> Changes from v2: >> - Rewrite the code as per HW team inputs >> - Addresses review comments >> Changes from v3: >> - Fix compilation warning >> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > As a general comment, indentation is weird on all multi-line > statements. Also, most comments are missing periods and are not 80- > column aligned. And a lot of the comments just say what the code > already does instead of explaining why the code does what it does, so > perhaps we could just remove them. Comments should explain why the code > is written the way it is, not translate from C to English. > > >> --- >> drivers/gpu/drm/i915/i915_drv.c | 173 >> ++++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/i915_drv.h | 12 +++ >> drivers/gpu/drm/i915/i915_reg.h | 37 +++++++++ >> 3 files changed, 222 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index 1c689b6..0ac7122 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -979,6 +979,173 @@ static void intel_sanitize_options(struct >> drm_i915_private *dev_priv) >> DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", >> yesno(i915.semaphores)); >> } >> >> +static inline enum rank skl_memdev_get_channel_rank(uint32_t val) > Why inline? This is already static, leave it up to the compiler to > decide if it's better to inline or not. removed inline in new series > >> +{ >> + uint8_t l_rank, s_rank; >> + uint8_t l_size, s_size; >> + enum rank ch_rank = DRAM_RANK_SINGLE; > Optional suggestion: get rid of this variable, just return the > appropriate values once we discover them. done > >> + >> + l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & >> SKL_DRAM_SIZE_MASK; >> + s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & >> SKL_DRAM_SIZE_MASK; >> + l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & >> SKL_DRAM_RANK_MASK; >> + s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & >> SKL_DRAM_RANK_MASK; > Validate our assumptions: > > WARN_ON(l_size == 0 && s_size == 0); > > Or we could even do the appropriate check and return DRAM_RANK_INVALID, > but then we'd have to restructure how our caller calls us, maybe moving > some code to this function. made the adjustment > >> + >> + /* >> + * If any of the slot has dual rank memory consider >> + * dual rank memory channel >> + */ > The comment says just what the code already says. What would be > interesting to see in the comment is an explanation of why we do it the > way we do. updated > >> + if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == >> SKL_DRAM_RANK_DUAL) >> + ch_rank = DRAM_RANK_DUAL; >> >> + >> + /* >> + * If both the slot has single rank memory then >> configuration >> + * is dual rank memory >> + */ > The comment says just what the code already says. What would be > interesting to see in the comment is an explanation of why we do it the > way we do. > > >> + if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) && >> + (s_size && s_rank == SKL_DRAM_RANK_SINGLE)) >> + ch_rank = DRAM_RANK_DUAL; >> + return ch_rank; >> +} >> + >> +static int >> +skl_get_memdev_info(struct drm_i915_private *dev_priv) >> +{ >> + struct memdev_info *memdev_info = &dev_priv->memdev_info; >> + uint32_t mem_freq_khz; >> + uint32_t val; >> + enum rank ch0_rank = DRAM_RANK_INVALID, ch1_rank = >> DRAM_RANK_INVALID; >> + >> + val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); >> + mem_freq_khz = (val & SKL_REQ_DATA_MASK) * >> + SKL_MEMORY_FREQ_MULTIPLIER_KHZ; > Optional suggestion: perhaps extend the memory_freq_multiplier to HZ > and then later cut the last 3 digits so the rounding errors get > restricted to the units we'll cut? Also, if we do the math using the "4 > * num / 3" we can try to reduce the rounding errors even more. > > I get annoyed that it says my system bandwidth is 25600032, while the > correct number is exactly 25600000. > > This applies to the BXT code too. Modified to work on Hz & then convert to KHz > >> + >> + val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN); >> + if (val != 0x0) { >> + memdev_info->num_channels++; >> + ch0_rank = skl_memdev_get_channel_rank(val); >> + } > Optional suggestion: > > Here's how I would have implemented this: > > ch0_rank = skl_memdev_get_channel_rank(0); > if (ch0_rank != DRAM_RANK_INVALID) > memdev_info->num_channels++; > > This way we'd move all the logic around this register to > skl_memdev_get_channel_rank(), and we'd also be able to get rid of the > initializations here. > > But this is just a suggestion in case you agree with me. Feel free to > leave the code the way it is. suggestion taken :) > >> + >> + val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN); >> + if (val != 0x0) { >> + memdev_info->num_channels++; >> + ch1_rank = skl_memdev_get_channel_rank(val); >> + } >> + >> + if (memdev_info->num_channels == 0) { >> + DRM_ERROR("Number of mem channels are zero\n"); > I'm not a native English speaker, but I would suppose that s/are/is/ > would make the sentence correct (the number is zero). While at it, why > not s/mem/memory/ too to make the code sound a little more formal? > > >> + return -EINVAL; >> + } >> + >> + memdev_info->bandwidth_kbps = (memdev_info->num_channels * >> + mem_freq_khz >> * 8); >> + >> + if (memdev_info->bandwidth_kbps == 0) { >> + DRM_ERROR("Couldn't get system memory bandwidth\n"); >> + return -EINVAL; >> + } >> + memdev_info->valid = true; > My previous comment about ownership of this variable still applies. > Now assigning memdev_info-> valid to true, only if bandwidth & rank both the information are valid >> + >> + /* >> + * If any of channel is single rank channel, > Again, not a native English speaker, but "any of channel" sounds > incorrect to me. But anyway, the comment only says what the code > already says. Why do we do it like this? > > >> + * consider single rank memory >> + */ >> + if (ch0_rank == DRAM_RANK_SINGLE || ch1_rank == >> DRAM_RANK_SINGLE) >> + memdev_info->rank = DRAM_RANK_SINGLE; >> + else >> + memdev_info->rank = max(ch0_rank, ch1_rank); > I can't think of any situation where this wouldn't end with > DRAM_RANK_DUAL, so we might just assign it to the variable. If there is only one dual-rank DRAM installed out of 4-dimms, then either ch0_rank or ch1_rank will be I915_DRAM_DUAL_RANK & other will be INVALID, in that case we'll have rank as DRAM_RANK_DUAL. > >> + >> + return 0; >> +} >> + >> +static int >> +bxt_get_memdev_info(struct drm_i915_private *dev_priv) >> +{ >> + struct memdev_info *memdev_info = &dev_priv->memdev_info; >> + uint32_t dram_channels; >> + uint32_t mem_freq_khz, val; >> + uint8_t num_active_channels; >> + int i; >> + >> + val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0); >> + mem_freq_khz = ((val & BXT_REQ_DATA_MASK) * >> + BXT_MEMORY_FREQ_MULTIPLIER_KHZ); >> + >> + dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) & >> + BXT_DRAM_CHANNEL_ACTIVE_MASK >> ; >> + num_active_channels = hweight32(dram_channels); >> + >> + memdev_info->bandwidth_kbps = (mem_freq_khz * >> num_active_channels * 4); >> + >> + if (memdev_info->bandwidth_kbps == 0) { >> + DRM_ERROR("Couldn't get system memory bandwidth\n"); >> + return -EINVAL; >> + } >> + memdev_info->valid = true; >> + >> + /* >> + * Now read each DUNIT8/9/10/11 to check the rank of each >> dimms. >> + */ >> + for (i = 0; i < BXT_D_CR_DRP0_DUNIT_MAX; i++) { >> + val = I915_READ(BXT_D_CR_DRP0_DUNIT(i)); >> + if (val != 0xFFFFFFFF) { >> + uint8_t rank; >> + enum rank ch_rank; >> + >> + memdev_info->num_channels++; >> + rank = val & BXT_DRAM_RANK_MASK; > Shouldn't the code here be: "if one of these two bits are enabled then > we're RANK_SINGLE, if both bits are enabled then we're RANK_DUAL, > otherwise invalid"? If not, why? > > In other words: is rank=0x2 really invalid? Isn't it just single rank? rank=0x2 is an invalid entry. "Enable Rank 0: Must be set to 1 to enable use of this rank. Note: Setting this bit to 0 is not a functional mode" "Enable Rank 0" bit can't be a 0 anyhow. > > >> + if (rank == BXT_DRAM_RANK_SINGLE) >> + ch_rank = DRAM_RANK_SINGLE; >> + else if (rank == BXT_DRAM_RANK_DUAL) >> + ch_rank = DRAM_RANK_DUAL; >> + else >> + ch_rank = DRAM_RANK_INVALID; >> + >> + /* >> + * If any of channel is having single rank >> memory > Again, "any of channel". > >> + * consider memory as single rank >> + */ >> + if (memdev_info->rank == DRAM_RANK_INVALID) >> + memdev_info->rank = ch_rank; >> + else if (ch_rank == DRAM_RANK_SINGLE) >> + memdev_info->rank = >> DRAM_RANK_SINGLE; >> + } >> + } > SKL returns -EINVAL if we end up with DRAM_RANK_INVALID in memdev_info- >> rank. I think we could do that in BXT too: either we have all the > information to not apply the workaround, or we give up. done, returning invalid if rank information is not available. > > >> + return 0; >> +} >> + >> +static void >> +intel_get_memdev_info(struct drm_i915_private *dev_priv) >> +{ >> + struct memdev_info *memdev_info = &dev_priv->memdev_info; >> + int ret; >> + >> + memdev_info->valid = false; >> + memdev_info->rank = DRAM_RANK_INVALID; >> + memdev_info->num_channels = 0; >> + >> + if (!IS_GEN9(dev_priv)) >> + return; >> + >> + if (IS_BROXTON(dev_priv)) >> + ret = bxt_get_memdev_info(dev_priv); >> + else >> + ret = skl_get_memdev_info(dev_priv); >> + if (ret) >> + return; >> + >> + DRM_DEBUG_DRIVER("DRAM bandwidth: %u KBps total-channels: >> %u\n", >> + memdev_info->bandwidth_kbps, >> + memdev_info->num_channels); >> + if (memdev_info->rank == DRAM_RANK_INVALID) >> + DRM_INFO("Counld not get memory rank info\n"); > Spelling mistake here. And since you use "Couldn't" instead of "Could > not" in all the other messages, I'd keep the same pattern. > > But if you accept my suggestion above, we'll never print this message > anyway. > > >> + else { >> + DRM_DEBUG_DRIVER("DRAM rank: %s rank\n", >> + (memdev_info->rank == >> DRAM_RANK_DUAL) ? >> + "dual" : "single"); >> + } >> +} >> + >> + > Two white spaces here. > > >> /** >> * i915_driver_init_hw - setup state requiring device access >> * @dev_priv: device private >> @@ -1081,6 +1248,12 @@ static int i915_driver_init_hw(struct >> drm_i915_private *dev_priv) >> DRM_DEBUG_DRIVER("can't enable MSI"); >> } >> >> + /* >> + * Fill the memdev structure to get the system raw bandwidth >> + * This will be used by WM algorithm, to implement GEN9 >> based WA >> + */ >> + intel_get_memdev_info(dev_priv); >> + >> return 0; >> >> out_ggtt: >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index b78dc9a..69213a4 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2297,6 +2297,18 @@ struct drm_i915_private { >> bool distrust_bios_wm; >> } wm; >> >> + struct memdev_info { >> + bool valid; >> + uint32_t bandwidth_kbps; >> + uint8_t num_channels; >> + enum rank { >> + DRAM_RANK_INVALID = 0, >> + DRAM_RANK_SINGLE, >> + DRAM_RANK_DUAL >> + } rank; > This was previously an anonymous enum (enum { ... } rank;), but now > it's "enum rank", which is too generic for a .h file. Either use a more > specific name (enum memdev_rank?) or try to make it anonymous again. changed to memdev_rank. > >> + } memdev_info; >> + >> + >> struct i915_runtime_pm pm; >> >> struct { >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> index 649319d..e7efdd0 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -8015,6 +8015,43 @@ enum { >> #define DC_STATE_DEBUG_MASK_CORES (1<<0) >> #define DC_STATE_DEBUG_MASK_MEMORY_UP (1<<1) >> >> +#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_S >> NB + 0x7114) >> +#define BXT_REQ_DATA_MASK 0x3F >> +#define BXT_DRAM_ACTIVE_CHANNEL_SHIFT 12 >> +#define BXT_DRAM_ACTIVE_CHANNEL_MASK 0xF > We're not using these two definitions anywhere. These are duplicate for definition below, my mistake, removing them. > >> +/* >> + * BIOS programs this field of REQ_DATA [5:0] in integer >> + * multiple of 133333 KHz (133.33MHz) >> + */ > Now that we're using decimal on the definition, I don't think this > comment is useful. > > >> +#define BXT_MEMORY_FREQ_MULTIPLIER_KHZ 133333 > We don't use tabs like that in our definitions. > > >> +#define BXT_D_CR_DRP0_DUNIT8 0x1000 >> +#define BXT_D_CR_DRP0_DUNIT9 0x1200 >> +#define BXT_D_CR_DRP0_DUNIT_MAX 4 >> +#define _MMIO_MCHBAR_DUNIT(x, a, b) _MMIO(MCHBAR_MIRROR_BASE_SNB + >> (a) + (x)*((b)-(a))) >> +#define BXT_D_CR_DRP0_DUNIT(x) _MMIO_MCHBAR_DUNIT(x, >> BXT_D_CR_DRP0_DUNIT8, BXT_D_CR_DRP0_DUNIT9) > But then BXT_D_CR_DRP0_DUNIT(1) means DUNIT9 instead of DUNIT1... > That's very unintuitive. now changed it to pass 8-11 to this macro, (from start to end of dunit) > > >> +#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 >> +#define BXT_DRAM_CHANNEL_ACTIVE_MASK 0xF > These two definitions don't belong to the register immediately above. > > >> +#define BXT_DRAM_RANK_MASK 0x3 >> +#define BXT_DRAM_RANK_SINGLE 0x1 >> +#define BXT_DRAM_RANK_DUAL 0x3 >> + >> +/* >> + * SKL memory frequeny multiplier is 266667 KHz (266.67 MHz) > s/frequeny/frequency/ > > Also, this is not how we do one-line comments. > > And now that the number is in decimal we probably don't even need the > comment here since the definition is a little more obvious. > >> + */ >> +#define SKL_MEMORY_FREQ_MULTIPLIER_KHZ 266667 > We don't use tabs like that in our definitions. > > >> +#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU _MMIO(MCHBAR_MIRROR >> _BASE_SNB + 0x5E04) >> +#define SKL_REQ_DATA_MASK (0xF << 0) > Blank lines separating different registers would be good. > > >> +#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIR >> ROR_BASE_SNB + 0x500C) >> +#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIR >> ROR_BASE_SNB + 0x5010) >> +#define SKL_DRAM_SIZE_MASK 0x1F > 0x3F fixed. patch is pushed as a new series @ https://patchwork.freedesktop.org/series/18842/ Regards, -Mahesh > > >> +#define SKL_DRAM_SIZE_L_SHIFT 0 >> +#define SKL_DRAM_SIZE_S_SHIFT 16 >> +#define SKL_DRAM_RANK_MASK 0x1 >> +#define SKL_DRAM_RANK_L_SHIFT 10 >> +#define SKL_DRAM_RANK_S_SHIFT 26 >> +#define SKL_DRAM_RANK_SINGLE 0x0 >> +#define SKL_DRAM_RANK_DUAL 0x1 >> + >> /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using >> this register, >> * since on HSW we can't write to it using I915_WRITE. */ >> #define D_COMP_HSW _MMIO(MCHBAR_MIRROR_BASE_S >> NB + 0x5F0C)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 1c689b6..0ac7122 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -979,6 +979,173 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv) DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores)); } +static inline enum rank skl_memdev_get_channel_rank(uint32_t val) +{ + uint8_t l_rank, s_rank; + uint8_t l_size, s_size; + enum rank ch_rank = DRAM_RANK_SINGLE; + + l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & SKL_DRAM_SIZE_MASK; + s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & SKL_DRAM_SIZE_MASK; + l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & SKL_DRAM_RANK_MASK; + s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & SKL_DRAM_RANK_MASK; + + /* + * If any of the slot has dual rank memory consider + * dual rank memory channel + */ + if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == SKL_DRAM_RANK_DUAL) + ch_rank = DRAM_RANK_DUAL; + + /* + * If both the slot has single rank memory then configuration + * is dual rank memory + */ + if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) && + (s_size && s_rank == SKL_DRAM_RANK_SINGLE)) + ch_rank = DRAM_RANK_DUAL; + return ch_rank; +} + +static int +skl_get_memdev_info(struct drm_i915_private *dev_priv) +{ + struct memdev_info *memdev_info = &dev_priv->memdev_info; + uint32_t mem_freq_khz; + uint32_t val; + enum rank ch0_rank = DRAM_RANK_INVALID, ch1_rank = DRAM_RANK_INVALID; + + val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU); + mem_freq_khz = (val & SKL_REQ_DATA_MASK) * + SKL_MEMORY_FREQ_MULTIPLIER_KHZ; + + val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN); + if (val != 0x0) { + memdev_info->num_channels++; + ch0_rank = skl_memdev_get_channel_rank(val); + } + + val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN); + if (val != 0x0) { + memdev_info->num_channels++; + ch1_rank = skl_memdev_get_channel_rank(val); + } + + if (memdev_info->num_channels == 0) { + DRM_ERROR("Number of mem channels are zero\n"); + return -EINVAL; + } + + memdev_info->bandwidth_kbps = (memdev_info->num_channels * + mem_freq_khz * 8); + + if (memdev_info->bandwidth_kbps == 0) { + DRM_ERROR("Couldn't get system memory bandwidth\n"); + return -EINVAL; + } + memdev_info->valid = true; + + /* + * If any of channel is single rank channel, + * consider single rank memory + */ + if (ch0_rank == DRAM_RANK_SINGLE || ch1_rank == DRAM_RANK_SINGLE) + memdev_info->rank = DRAM_RANK_SINGLE; + else + memdev_info->rank = max(ch0_rank, ch1_rank); + + return 0; +} + +static int +bxt_get_memdev_info(struct drm_i915_private *dev_priv) +{ + struct memdev_info *memdev_info = &dev_priv->memdev_info; + uint32_t dram_channels; + uint32_t mem_freq_khz, val; + uint8_t num_active_channels; + int i; + + val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0); + mem_freq_khz = ((val & BXT_REQ_DATA_MASK) * + BXT_MEMORY_FREQ_MULTIPLIER_KHZ); + + dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) & + BXT_DRAM_CHANNEL_ACTIVE_MASK; + num_active_channels = hweight32(dram_channels); + + memdev_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4); + + if (memdev_info->bandwidth_kbps == 0) { + DRM_ERROR("Couldn't get system memory bandwidth\n"); + return -EINVAL; + } + memdev_info->valid = true; + + /* + * Now read each DUNIT8/9/10/11 to check the rank of each dimms. + */ + for (i = 0; i < BXT_D_CR_DRP0_DUNIT_MAX; i++) { + val = I915_READ(BXT_D_CR_DRP0_DUNIT(i)); + if (val != 0xFFFFFFFF) { + uint8_t rank; + enum rank ch_rank; + + memdev_info->num_channels++; + rank = val & BXT_DRAM_RANK_MASK; + if (rank == BXT_DRAM_RANK_SINGLE) + ch_rank = DRAM_RANK_SINGLE; + else if (rank == BXT_DRAM_RANK_DUAL) + ch_rank = DRAM_RANK_DUAL; + else + ch_rank = DRAM_RANK_INVALID; + + /* + * If any of channel is having single rank memory + * consider memory as single rank + */ + if (memdev_info->rank == DRAM_RANK_INVALID) + memdev_info->rank = ch_rank; + else if (ch_rank == DRAM_RANK_SINGLE) + memdev_info->rank = DRAM_RANK_SINGLE; + } + } + return 0; +} + +static void +intel_get_memdev_info(struct drm_i915_private *dev_priv) +{ + struct memdev_info *memdev_info = &dev_priv->memdev_info; + int ret; + + memdev_info->valid = false; + memdev_info->rank = DRAM_RANK_INVALID; + memdev_info->num_channels = 0; + + if (!IS_GEN9(dev_priv)) + return; + + if (IS_BROXTON(dev_priv)) + ret = bxt_get_memdev_info(dev_priv); + else + ret = skl_get_memdev_info(dev_priv); + if (ret) + return; + + DRM_DEBUG_DRIVER("DRAM bandwidth: %u KBps total-channels: %u\n", + memdev_info->bandwidth_kbps, + memdev_info->num_channels); + if (memdev_info->rank == DRAM_RANK_INVALID) + DRM_INFO("Counld not get memory rank info\n"); + else { + DRM_DEBUG_DRIVER("DRAM rank: %s rank\n", + (memdev_info->rank == DRAM_RANK_DUAL) ? + "dual" : "single"); + } +} + + /** * i915_driver_init_hw - setup state requiring device access * @dev_priv: device private @@ -1081,6 +1248,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) DRM_DEBUG_DRIVER("can't enable MSI"); } + /* + * Fill the memdev structure to get the system raw bandwidth + * This will be used by WM algorithm, to implement GEN9 based WA + */ + intel_get_memdev_info(dev_priv); + return 0; out_ggtt: diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b78dc9a..69213a4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2297,6 +2297,18 @@ struct drm_i915_private { bool distrust_bios_wm; } wm; + struct memdev_info { + bool valid; + uint32_t bandwidth_kbps; + uint8_t num_channels; + enum rank { + DRAM_RANK_INVALID = 0, + DRAM_RANK_SINGLE, + DRAM_RANK_DUAL + } rank; + } memdev_info; + + struct i915_runtime_pm pm; struct { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 649319d..e7efdd0 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8015,6 +8015,43 @@ enum { #define DC_STATE_DEBUG_MASK_CORES (1<<0) #define DC_STATE_DEBUG_MASK_MEMORY_UP (1<<1) +#define BXT_P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114) +#define BXT_REQ_DATA_MASK 0x3F +#define BXT_DRAM_ACTIVE_CHANNEL_SHIFT 12 +#define BXT_DRAM_ACTIVE_CHANNEL_MASK 0xF +/* + * BIOS programs this field of REQ_DATA [5:0] in integer + * multiple of 133333 KHz (133.33MHz) + */ +#define BXT_MEMORY_FREQ_MULTIPLIER_KHZ 133333 +#define BXT_D_CR_DRP0_DUNIT8 0x1000 +#define BXT_D_CR_DRP0_DUNIT9 0x1200 +#define BXT_D_CR_DRP0_DUNIT_MAX 4 +#define _MMIO_MCHBAR_DUNIT(x, a, b) _MMIO(MCHBAR_MIRROR_BASE_SNB + (a) + (x)*((b)-(a))) +#define BXT_D_CR_DRP0_DUNIT(x) _MMIO_MCHBAR_DUNIT(x, BXT_D_CR_DRP0_DUNIT8, BXT_D_CR_DRP0_DUNIT9) +#define BXT_DRAM_CHANNEL_ACTIVE_SHIFT 12 +#define BXT_DRAM_CHANNEL_ACTIVE_MASK 0xF +#define BXT_DRAM_RANK_MASK 0x3 +#define BXT_DRAM_RANK_SINGLE 0x1 +#define BXT_DRAM_RANK_DUAL 0x3 + +/* + * SKL memory frequeny multiplier is 266667 KHz (266.67 MHz) + */ +#define SKL_MEMORY_FREQ_MULTIPLIER_KHZ 266667 +#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04) +#define SKL_REQ_DATA_MASK (0xF << 0) +#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C) +#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010) +#define SKL_DRAM_SIZE_MASK 0x1F +#define SKL_DRAM_SIZE_L_SHIFT 0 +#define SKL_DRAM_SIZE_S_SHIFT 16 +#define SKL_DRAM_RANK_MASK 0x1 +#define SKL_DRAM_RANK_L_SHIFT 10 +#define SKL_DRAM_RANK_S_SHIFT 26 +#define SKL_DRAM_RANK_SINGLE 0x0 +#define SKL_DRAM_RANK_DUAL 0x1 + /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register, * since on HSW we can't write to it using I915_WRITE. */ #define D_COMP_HSW _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
This patch adds support to decode system memory bandwidth which will be used for arbitrated display memory percentage calculation in GEN9 based system. Changes from v1: - Address comments from Paulo - implement decode function for SKL/KBL also Changes from v2: - Rewrite the code as per HW team inputs - Addresses review comments Changes from v3: - Fix compilation warning Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 173 ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 12 +++ drivers/gpu/drm/i915/i915_reg.h | 37 +++++++++ 3 files changed, 222 insertions(+)