Re: [PATCH 2/5] drm/i915/skl+: Decode memory bandwidth and parameters

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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@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?

> +
> +	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...

> +
> +	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...

>  	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?

> +};
> +
>  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx