[Intel-gfx] [PATCH 2/5] drm/i915/skl+: Decode memory bandwidth and parameters
Kumar, Mahesh
mahesh1.kumar at intel.com
Tue Aug 21 10:21:43 UTC 2018
Hi,
On 8/17/2018 4:05 AM, Rodrigo Vivi wrote:
> On Thu, Jul 26, 2018 at 07:44:07PM +0530, Mahesh Kumar wrote:
>> This patch adds support to decode system memory bandwidth and other
>> parameters for skylake and Gen9+ platforms, which will be used for
>> arbitrated display memory bandwidth calculation in GEN9 based
>> platforms and WM latency level-0 Work-around calculation on GEN9+.
>>
>> Changes Since V1:
>> - s/memdev_info/dram_info
>> - create a struct to hold channel info
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 131 ++++++++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/i915_drv.h | 7 +++
>> drivers/gpu/drm/i915/i915_reg.h | 21 +++++++
>> 3 files changed, 155 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 16629601c9f4..ddf6bf9b500a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1070,6 +1070,118 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>> intel_gvt_sanitize_options(dev_priv);
>> }
>>
>> +static int
>> +skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>> +{
>> + u8 l_rank, s_rank;
>> + u8 l_size, s_size;
>> + u8 l_width, s_width;
>> + enum dram_rank rank;
>> +
>> + if (!val)
>> + return -1;
> -SOMEERRNO?
ok sure.
>
>> +
>> + 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_width = (val >> SKL_DRAM_WIDTH_L_SHIFT) & SKL_DRAM_WIDTH_MASK;
>> + s_width = (val >> SKL_DRAM_WIDTH_S_SHIFT) & SKL_DRAM_WIDTH_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 (l_size == 0 && s_size == 0)
>> + return -1;
> ditto
>
>> +
>> + DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
>> + l_size, (1 << l_width) * 8, l_rank ? "dual" : "single",
>> + s_size, (1 << s_width) * 8, s_rank ? "dual" : "single");
>> +
>> + if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == SKL_DRAM_RANK_DUAL)
>> + rank = I915_DRAM_RANK_DUAL;
>> + else if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) &&
>> + (s_size && s_rank == SKL_DRAM_RANK_SINGLE))
>> + rank = I915_DRAM_RANK_DUAL;
>> + else
>> + rank = I915_DRAM_RANK_SINGLE;
>> +
>> + ch->l_info.size = l_size;
>> + ch->s_info.size = s_size;
>> + ch->l_info.width = l_width;
>> + ch->s_info.width = s_width;
>> + ch->l_info.rank = l_rank;
>> + ch->s_info.rank = s_rank;
>> + ch->rank = rank;
> could we do this directly without intermediates? not clear if we change
> in the middle after printing...
This information is needed later while checking is memories are
symmetric, that's why we need to keep this as well.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>> +{
>> + struct dram_info *dram_info = &dev_priv->dram_info;
>> + struct dram_channel_info ch0, ch1;
>> + u32 val;
>> + int ret;
>> +
>> + val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>> + ret = skl_dram_get_channel_info(&ch0, val);
>> + if (ret == 0)
>> + dram_info->num_channels++;
>> +
>> + val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>> + ret = skl_dram_get_channel_info(&ch1, val);
>> + if (ret == 0)
>> + dram_info->num_channels++;
>> +
>> + if (dram_info->num_channels == 0) {
>> + DRM_INFO("Number of memory channels is zero\n");
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * If any of the channel is single rank channel, worst case output
>> + * will be same as if single rank memory, so consider single rank
>> + * memory.
>> + */
>> + if (ch0.rank == I915_DRAM_RANK_SINGLE ||
>> + ch1.rank == I915_DRAM_RANK_SINGLE)
>> + dram_info->rank = I915_DRAM_RANK_SINGLE;
>> + else
>> + dram_info->rank = max(ch0.rank, ch1.rank);
>> +
>> + if (dram_info->rank == I915_DRAM_RANK_INVALID) {
>> + DRM_INFO("couldn't get memory rank information\n");
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> +static int
>> +skl_get_dram_info(struct drm_i915_private *dev_priv)
>> +{
>> + struct dram_info *dram_info = &dev_priv->dram_info;
>> + u32 mem_freq_khz, val;
>> + int ret;
>> +
>> + ret = skl_dram_get_channels_info(dev_priv);
>> + if (ret)
>> + return ret;
>> +
>> + val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
>> + mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
>> + SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
>> +
>> + dram_info->bandwidth_kbps = dram_info->num_channels *
>> + mem_freq_khz * 8;
>> +
>> + if (dram_info->bandwidth_kbps == 0) {
>> + DRM_INFO("Couldn't get system memory bandwidth\n");
>> + return -EINVAL;
>> + }
>> +
>> + dram_info->valid = true;
>> + return 0;
>> +}
>> +
>> static int
>> bxt_get_dram_info(struct drm_i915_private *dev_priv)
>> {
>> @@ -1159,6 +1271,7 @@ static void
>> intel_get_dram_info(struct drm_i915_private *dev_priv)
>> {
>> struct dram_info *dram_info = &dev_priv->dram_info;
>> + char bandwidth_str[32];
>> int ret;
>>
>> dram_info->valid = false;
>> @@ -1166,15 +1279,25 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>> dram_info->bandwidth_kbps = 0;
>> dram_info->num_channels = 0;
>>
>> - if (!IS_BROXTON(dev_priv))
>> + if (INTEL_GEN(dev_priv) < 9)
>> return;
>>
>> - ret = bxt_get_dram_info(dev_priv);
>> + /* Need to calculate bandwidth only for Gen9 */
>> + if (IS_BROXTON(dev_priv))
>> + ret = bxt_get_dram_info(dev_priv);
>> + else if (INTEL_GEN(dev_priv) == 9)
>> + ret = skl_get_dram_info(dev_priv);
>> + else
>> + ret = skl_dram_get_channels_info(dev_priv);
> I din't get this... why newer platforms only need this partial path?
>
> also this highlights that the names of the functions are confusing...
In gen9 platform we need complete DRAM info (channel configuration +
bandwidth supported by DRAM) which will be used by bandwidth related WA,
and only applicable for GEN9 platforms. Bspec:4380 (Gen9 Watermark
Calculations: Workaround)
But Channel info is applicable for all the platforms, This will be used
to check if system has 16GB dimm or if memory channels are not symmetric.
BSpec:4381 (Retrieve Memory Latency Data)
I agree names seems little confusing I used following convention
dram_info -> channels_info -> channel_info
>
>> if (ret)
>> return;
>>
>> - DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
>> - dram_info->bandwidth_kbps, dram_info->num_channels);
>> + if (dram_info->bandwidth_kbps)
>> + sprintf(bandwidth_str, "%d KBps", dram_info->bandwidth_kbps);
>> + else
>> + sprintf(bandwidth_str, "unknown");
>> + DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
>> + bandwidth_str, dram_info->num_channels);
>> DRM_DEBUG_KMS("DRAM rank: %s rank\n",
>> (dram_info->rank == I915_DRAM_RANK_DUAL) ?
>> "dual" : "single");
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 46f942fa7d60..2d12fc152b49 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2128,6 +2128,13 @@ struct drm_i915_private {
>> */
>> };
>>
>> +struct dram_channel_info {
>> + struct info {
>> + u8 size, width, rank;
>> + } l_info, s_info;
>> + enum dram_rank rank;
> why do we need duplicated rand information?
first one represents rank of each dimm left/right dimm.
Second one stores the rank of channel.
-Mahesh
>
>> +};
>> +
>> static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
>> {
>> return container_of(dev, struct drm_i915_private, drm);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 66900d027570..e4d61167fb64 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9664,6 +9664,27 @@ enum skl_power_gate {
>> #define BXT_DRAM_SIZE_12GB 0x3
>> #define BXT_DRAM_SIZE_16GB 0x4
>>
>> +#define SKL_MEMORY_FREQ_MULTIPLIER_HZ 266666666
>> +#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 0x3F
>> +#define SKL_DRAM_SIZE_L_SHIFT 0
>> +#define SKL_DRAM_SIZE_S_SHIFT 16
>> +#define SKL_DRAM_WIDTH_MASK 0x3
>> +#define SKL_DRAM_WIDTH_L_SHIFT 8
>> +#define SKL_DRAM_WIDTH_S_SHIFT 24
>> +#define SKL_DRAM_WIDTH_X8 0x0
>> +#define SKL_DRAM_WIDTH_X16 0x1
>> +#define SKL_DRAM_WIDTH_X32 0x2
>> +#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
>
> again, spec pointers please.
>
>> +
>> /* 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)
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list