[Intel-gfx] [PATCH v7 7/8] drm/i915: Decode system memory bandwidth
Paulo Zanoni
paulo.r.zanoni at intel.com
Thu Dec 8 23:55:13 UTC 2016
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.
> +{
> + 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)
More information about the Intel-gfx
mailing list