Re: [PATCH 1/5] drm/i915/bxt: Decode memory bandwidth and parameters

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

On Tue, Aug 21, 2018 at 03:23:06PM +0530, Kumar, Mahesh wrote:
> Hi,
> 
> 
> On 8/17/2018 11:13 PM, Rodrigo Vivi wrote:
> > On Thu, Jul 26, 2018 at 07:44:06PM +0530, Mahesh Kumar wrote:
> > > This patch adds support to decode system memory bandwidth and other
> > > parameters for broxton platform, which will be used for arbitrated
> > > display memory bandwidth calculation in GEN9 based platforms and
> > > WM latency level-0 Work-around calculation on GEN9+ platforms.
> > > 
> > > Changes since V1:
> > >   - s/memdev_info/dram_info
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.c | 116 ++++++++++++++++++++++++++++++++++++++++
> > >   drivers/gpu/drm/i915/i915_drv.h |  11 ++++
> > >   drivers/gpu/drm/i915/i915_reg.h |  30 +++++++++++
> > >   3 files changed, 157 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 18a45e7a3d7c..16629601c9f4 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1070,6 +1070,116 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
> > >   	intel_gvt_sanitize_options(dev_priv);
> > >   }
> > > 
> > > +static int
> > > +bxt_get_dram_info(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > > +	u32 dram_channels;
> > > +	u32 mem_freq_khz, val;
> > > +	u8 num_active_channels;
> > > +	int i;
> > > +
> > > +	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
> > > +	mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) *
> > > +				    BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> > > +
> > > +	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
> > > +					BXT_DRAM_CHANNEL_ACTIVE_MASK;
> > > +	num_active_channels = hweight32(dram_channels);
> > > +
> > > +	/* Each active bit represents 4-byte channel */
> > > +	dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
> > > +
> > > +	if (dram_info->bandwidth_kbps == 0) {
> > > +		DRM_INFO("Couldn't get system memory bandwidth\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
> > > +	 */
> > > +	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> > > +		u8 rank, size, width;
> > > +		enum dram_rank ch_rank;
> > > +
> > > +		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
> > > +		if (val == 0xFFFFFFFF)
> > > +			continue;
> > > +
> > > +		dram_info->num_channels++;
> > > +		rank = val & BXT_DRAM_RANK_MASK;
> > > +		width = (val >> BXT_DRAM_WIDTH_SHIFT) & BXT_DRAM_WIDTH_MASK;
> > > +		size = (val >> BXT_DRAM_SIZE_SHIFT) & BXT_DRAM_SIZE_MASK;
> > > +		if (rank == BXT_DRAM_RANK_SINGLE)
> > > +			ch_rank = I915_DRAM_RANK_SINGLE;
> > > +		else if (rank == BXT_DRAM_RANK_DUAL)
> > > +			ch_rank = I915_DRAM_RANK_DUAL;
> > > +		else
> > > +			ch_rank = I915_DRAM_RANK_INVALID;
> > > +
> > > +		if (size == BXT_DRAM_SIZE_4GB)
> > > +			size = 4;
> > > +		else if (size == BXT_DRAM_SIZE_6GB)
> > > +			size = 6;
> > > +		else if (size == BXT_DRAM_SIZE_8GB)
> > > +			size = 8;
> > > +		else if (size == BXT_DRAM_SIZE_12GB)
> > > +			size = 12;
> > > +		else if (size == BXT_DRAM_SIZE_16GB)
> > > +			size = 16;
> > > +		else
> > > +			size = 0;
> > > +
> > > +		width = (1 << width) * 8;
> > > +		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
> > > +			      width, rank == BXT_DRAM_RANK_SINGLE ? "single" :
> > > +			      rank == BXT_DRAM_RANK_DUAL ? "dual" : "unknown");
> > > +
> > > +		/*
> > > +		 * 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 (dram_info->rank == I915_DRAM_RANK_INVALID)
> > > +			dram_info->rank = ch_rank;
> > > +		else if (ch_rank == I915_DRAM_RANK_SINGLE)
> > > +			dram_info->rank = I915_DRAM_RANK_SINGLE;
> > > +	}
> > > +
> > > +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> > > +		DRM_INFO("couldn't get memory rank information\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	dram_info->valid = true;
> > > +	return 0;
> > > +}
> > > +
> > > +static void
> > > +intel_get_dram_info(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > > +	int ret;
> > > +
> > > +	dram_info->valid = false;
> > > +	dram_info->rank = I915_DRAM_RANK_INVALID;
> > > +	dram_info->bandwidth_kbps = 0;
> > > +	dram_info->num_channels = 0;
> > > +
> > > +	if (!IS_BROXTON(dev_priv))
> > > +		return;
> > > +
> > > +	ret = bxt_get_dram_info(dev_priv);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
> > > +		      dram_info->bandwidth_kbps, dram_info->num_channels);
> > > +	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
> > > +		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> > > +		      "dual" : "single");
> > > +}
> > > +
> > >   /**
> > >    * i915_driver_init_hw - setup state requiring device access
> > >    * @dev_priv: device private
> > > @@ -1187,6 +1297,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
> > >   		goto err_msi;
> > > 
> > >   	intel_opregion_setup(dev_priv);
> > > +	/*
> > > +	 * Fill the dram structure to get the system raw bandwidth and
> > > +	 * dram info. This will be used for memory latency calculation.
> > > +	 */
> > > +	intel_get_dram_info(dev_priv);
> > > +
> > > 
> > >   	return 0;
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0f49f9988dfa..46f942fa7d60 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1904,6 +1904,17 @@ struct drm_i915_private {
> > >   		bool distrust_bios_wm;
> > >   	} wm;
> > > 
> > > +	struct dram_info {
> > > +		bool valid;
> > > +		u8 num_channels;
> > > +		enum dram_rank {
> > > +			I915_DRAM_RANK_INVALID = 0,
> > > +			I915_DRAM_RANK_SINGLE,
> > > +			I915_DRAM_RANK_DUAL
> > > +		} rank;
> > > +		u32 bandwidth_kbps;
> > > +	} dram_info;
> > > +
> > >   	struct i915_runtime_pm runtime_pm;
> > > 
> > >   	struct {
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 5530c470f30d..66900d027570 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9634,6 +9634,36 @@ enum skl_power_gate {
> > >   #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_CHANNEL_ACTIVE_SHIFT		12
> > > +#define  BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF
> > Thanks a lot for the spec pointers. But now that I opened it and
> > started to reading here it was hard to review and I noticed this
> > is because the way that it is defined here doesn't respect our
> > standards as documented:
> > 
> > "
> > * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Define bit field
> >   * contents so that they are already shifted in place, and can be directly
> >   * OR'd.
> > "
> So I'll modify all "_MASK" defines to make them shifted in place, Should I
> redefine all the bit definitions also to be shifted?
> BTW these bit-fields are readonly for driver,

Yes, please define everything with right shift in place so it gets easier to
read.

Thanks,
Rodrigo.

> 
> -Mahesh
> > 
> > So please provide a new version that follows the rules and
> > that it is easier to review. And sorry for not noticing this
> > on yesterday's review.
> > 
> > > +#define  BXT_MEMORY_FREQ_MULTIPLIER_HZ		133333333
> > > +
> > > +#define BXT_D_CR_DRP0_DUNIT8			0x1000
> > > +#define BXT_D_CR_DRP0_DUNIT9			0x1200
> > > +#define  BXT_D_CR_DRP0_DUNIT_START		8
> > > +#define  BXT_D_CR_DRP0_DUNIT_END		11
> > > +#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO(MCHBAR_MIRROR_BASE_SNB + \
> > > +				      _PICK_EVEN((x) - 8, BXT_D_CR_DRP0_DUNIT8,\
> > > +						 BXT_D_CR_DRP0_DUNIT9))
> > > +#define  BXT_DRAM_RANK_MASK			0x3
> > > +#define  BXT_DRAM_RANK_SINGLE			0x1
> > > +#define  BXT_DRAM_RANK_DUAL			0x3
> > > +#define  BXT_DRAM_WIDTH_MASK			0x3
> > > +#define  BXT_DRAM_WIDTH_SHIFT			4
> > > +#define  BXT_DRAM_WIDTH_X8			0x0
> > > +#define  BXT_DRAM_WIDTH_X16			0x1
> > > +#define  BXT_DRAM_WIDTH_X32			0x2
> > > +#define  BXT_DRAM_WIDTH_X64			0x3
> > > +#define  BXT_DRAM_SIZE_MASK			0x7
> > > +#define  BXT_DRAM_SIZE_SHIFT			6
> > > +#define  BXT_DRAM_SIZE_4GB			0x0
> > > +#define  BXT_DRAM_SIZE_6GB			0x1
> > > +#define  BXT_DRAM_SIZE_8GB			0x2
> > > +#define  BXT_DRAM_SIZE_12GB			0x3
> > > +#define  BXT_DRAM_SIZE_16GB			0x4
> > > +
> > >   /* 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