[Intel-gfx] [PATCH v7 7/8] drm/i915: Decode system memory bandwidth
Mahesh Kumar
mahesh1.kumar at intel.com
Wed Feb 15 15:00:13 UTC 2017
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 at 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)
More information about the Intel-gfx
mailing list